0

Hey been fiddling with this all night just wondering if anyone can notice any flaws with my code? I was told before I was using out of date sql connection strings so I've updated. feedback would be great! 

 <?php
    if ( empty( $_POST ) ){
    ?>

    <form method="post" action="">
        <div class="form-group">
        <label>Full Name:</label>
        <input class="form-control" type="text" name="sub_name" />
        <br>
        <label>Email:</label>
        <input class="form-control" type="text" name="sub_email" />

            <div class="pull-right">
                <input class="btn btn-success" type="submit" value="Subscribe" />
            </div>

        </div>
    </form>

    <?php
    } else {

    try {


    $db = new PDO( 'mysql:host=localhost;dbname=test', $subsc_username, $subsc_password );

    $form = $_POST;
    $subsc_name = $form[ 'sub_name' ];
    $subsc_email = $form[ 'sub_email' ];

    $sql = "INSERT INTO subscribers ( 
        subsc_name, subsc_email ) 
        VALUES ( 
        $subsc_name, $subsc_email )";

    $query = $db->prepare( $sql );
    $query->execute( array( 'subsc_name'=>$subsc_name, 'subsc_email'=>$subsc_email, ) );

    }
    catch(PDOException $e)
    {
    echo $e->getMessage();
    }
    }

    ?>
BenMorel
  • 34,448
  • 50
  • 182
  • 322
user3242538
  • 3
  • 2
  • 5
    This question appears to be off-topic because it belongs on http://codereview.stackexchange.com – deceze Jan 28 '14 at 10:01
  • 1
    read ircmaxell's answer here http://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection/12202218#12202218 – dachi Jan 28 '14 at 10:02
  • It does not look good... this is not how you prepare/bind variables. – Daniel W. Jan 28 '14 at 10:03

4 Answers4

1

You try to bind values in the execute(), but insert the variables directly.

And you got a useless , at the end of your array.

Also you should be sure that you're not using emulated statements.

You should it change to this:

$sql = "INSERT INTO subscribers ( 
        subsc_name, subsc_email ) 
        VALUES ( 
        :subsc_name, :subsc_email )";
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$query = $db->prepare( $sql );
$query->execute( array( ':subsc_name'=>$subsc_name, ':subsc_email'=>$subsc_email ) );
Maarkoize
  • 2,601
  • 2
  • 16
  • 34
1

You're not using placeholders in your prepared statement. The correct usage is:

<?php
// $_POST might be empty even if the request is a POST
if ('POST' === $_SERVER['REQUEST_METHOD']) {
    try {
        $db = new PDO('mysql:host=localhost;dbname=test', $subsc_username, $subsc_password);
        // default error mode is to return FALSE
        $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    } catch (PDOException $e) {
        die($e->getMessage());
    }

    $subsc_name = $_POST['sub_name'];
    $subsc_email = $_POST['sub_email'];

    // never put variables directly in your SQL queries
    $sql = 'INSERT INTO subscribers (subsc_name, subsc_email) VALUES (:subsc_name, :subsc_email)';

    try {
        $query = $db->prepare($sql);
        $query->execute(array(
            ':subsc_name'  => $subsc_name,
            ':subsc_email' => $subsc_email,
        ));
    } catch (PDOException $e) {
        die($e->getMessage());
    }
} else {
    echo <<<EOF
    <form method="post" action="">
        <div class="form-group">
            <label>Full Name:</label>
            <input class="form-control" type="text" name="sub_name" />

            <br>

            <label>Email:</label>
            <input class="form-control" type="text" name="sub_email" />

            <div class="pull-right">
                <input class="btn btn-success" type="submit" value="Subscribe" />
            </div>
        </div>
    </form>
EOF;
}
?>
Alessandro Desantis
  • 14,098
  • 1
  • 26
  • 32
  • really apreciate this working example alessandro! – user3242538 Jan 28 '14 at 10:38
  • Alessandro what would be the best way to add some basic validation/success messaging to this? would i need to use jquery? – user3242538 Jan 28 '14 at 12:47
  • If you want to perform live validation you can use some jQuery plugin, but it can easily be disabled so you *have* to validate on the server as well. – Alessandro Desantis Jan 28 '14 at 13:54
  • sure, can these messages which are happening at the moment be made a bit more user friendly? so instead of saying: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'user@email.com' for key 'subsc_email' it could say "sorry you are already subscribed to this mailing list" ? thannks – user3242538 Jan 28 '14 at 13:59
  • Unless you want to parse the exception's message, I suggest you perform a SELECT query on the ```subscribers``` table. If the query returns a row, then the user is already subscribed. – Alessandro Desantis Jan 28 '14 at 14:04
  • could i not catch exceptions like duplicates? for example: catch(PDOException $e) { # 23000 error code means the key already exists, so UPDATE! if($e->getCode() == 23000) { echo 'User is already subscribed'; } else { echo << – user3242538 Jan 28 '14 at 14:59
  • I guess you could! :) – Alessandro Desantis Jan 28 '14 at 16:07
0

Yes, it is. You have no prepare template, you just have ready-to-go query in $sql with non-escaped values.

mr. Pavlikov
  • 982
  • 5
  • 7
0

If you check this line:

$sql = "INSERT INTO subscribers ( 
    subsc_name, subsc_email ) 
    VALUES ( 
    $subsc_name, $subsc_email )";

Try to understand what you are doing. You are using double qoutes, which means variables are actually parsed. So this line already evaluates to 

$sql = "INSERT INTO subscribers ( 
    subsc_name, subsc_email ) 
    VALUES ( 
    VALUE_OF_$subsc_name, VALUE_OF_subsc_email )";

So yes, it is vulnerable to SQL injection. If you would use single quotes then the line would stay as the first version, but then its still not how PDO works. Check this site for examples on how to use it.

Hugo Delsing
  • 13,803
  • 5
  • 45
  • 72