1
\$\begingroup\$

Last time I made a website using PHP, I didn't know of PDO, so someone dropped all my tables. I think I've made an improvement now. I'm sorry if there's a lot to go through, but I want to make sure its safe (most importantly) and also using the best practices.

Here are some snippets:

1)

$form = $_POST;     $name = $form[ 'name' ];    $desc = $form[ 'desc' ]; $link = $form[ 'link' ];   $tag = $form[ 'tag' ];  $category = $form[ 'category' ];

$sql = "INSERT INTO items ( `name`, `desc`, `link`, `tag`, `category` ) VALUES ( :name, :desc, :link, :tag, :category )";

$query = $db->prepare( $sql );  $query->execute( array( ':name'=>$name, ':desc'=>$desc, ':link'=>$link, ':tag'=>$tag, ':category'=>$category ) );

2)

$db = new PDO('mysql:host=mywebsite.com;dbname=thisdb;charset=utf8','root', '');

3)

if (isset($_GET['search'])) {
   $searchStmt = $db->prepare('SELECT * FROM items WHERE tag LIKE    :usersearch'); $searchStmt->execute(['usersearch' =>    "%".$_GET['search']."%"]);

   foreach ($searchStmt as $row) { ?> <?php    include('includes/results-layout.php'); ?> <?php }} else {    header('Location: http://localhost/photo'); }
   if ($_GET['search'] == "") {     header('Location:    http://mywebsite.com/photo');  }
   if ($searchStmt->rowCount() === 0) {     echo "Sorry, your search for    <i>\"".$_GET['search']."\"</i> was not found...";   }

4)

$page1Stmt = $db->query("SELECT * FROM items WHERE category = 1 ORDER BY id DESC");

5)

$downloadStmt = $db->prepare('SELECT * FROM items WHERE id = :downloadID'); $downloadStmt->execute(['downloadID' => $_GET['id']]); ?> $tags = explode(" ",$row['tag']);
foreach($tags as $tag) {echo "<a href='search.php?search=".$tag."'><span class='itemTag'>".$tag."</span></a>";} $relatedStmt = $db->prepare("SELECT * FROM items WHERE tag = (select tag from items where id = :downloadID)");
$relatedStmt->execute(['downloadID' => $_GET['id']]); ?>

6)

$pageStmt = $db->query("SELECT * FROM items ORDER BY id DESC");
foreach ($pageStmt as $row) { ?>
<?php include('includes/results-layout.php'); ?>
<?php } ?>
\$\endgroup\$
  • 1
    \$\begingroup\$ Please post all of your code here. "Some snippets" are not enough for review. \$\endgroup\$ – Madara's Ghost Jun 23 '14 at 15:54
  • \$\begingroup\$ @MadaraUchiha I don't wish to show all my code, I hope you can understand. These snippets are the main user inputs. \$\endgroup\$ – Rahul Khosla Jun 23 '14 at 15:55
  • 3
    \$\begingroup\$ Post all of your code, not just snippets. Unlike Stack Overflow, we don't require minimal code. \$\endgroup\$ – Jamal Jun 23 '14 at 15:56
  • 1
    \$\begingroup\$ @Madara From the help center: "you must post the relevant snippets of code in your question". There is nothing in there about requiring all of the code, I fail to understand why this question is off topic. \$\endgroup\$ – pgraham Jun 23 '14 at 17:16
  • 2
    \$\begingroup\$ Reopening this - giving it the "benefit of the doubt". I agree there is reviewable content here, and it is real code. Does not necessarily make this a good question. We will see. \$\endgroup\$ – rolfl Jun 23 '14 at 19:30
3
\$\begingroup\$

Just a couple of quick-at-first-glance issues that need addressing:

  • Horrid, and I mean truly horrid, coding style. Please subscribe to the standards. ASAP. They are unofficial, but all major players subscribe to them, as should you.
  • If a file contains nothing but PHP code, the closing tag should be omitted, as the official docs clearly state: "If a file is pure PHP code, it is preferable to omit the PHP closing tag at the end of the file"
  • Use the fourth constructor argument for PDO, which allows you to configure the actual connection (see below)
  • Think about encoding issues: SET NAMES 'UTF8'. Multi-byte chars are going to be processed as ASCII chars if you don't tell MySQL you're using the UTF-8 charset.
  • Never trust the network. Don't blindly trust prepared statements, and don't blindly trust user-supplied data, especially when it's coming from $_GET. The $_SESSION super-global is the closest you can get to the gray area of user data you can trust (because you set it). $_POST and $_GET are right out.

I've been quite vocal on the fourth argument of the PDO constructor, and on the UTF-8 business, just recently. Instead of copy-pasting my previous answer, here's a link.
Basically, what I'd recommend you do is this:

$db = new PDO(
    'mysql:host=127.0.0.1;port=3306;dbname=myDb;charset=UTF8',
    $userName,
    $pass,
    array(
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_ORACLE_NULLS       => PDO::NULL_NATURAL,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ,
        //production only - read warning below, though
        PDO::ATTR_EMULATE_PREPARES   => false,
        PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES 'UTF8'"
    )
);

Why only disable emulate prepares in production code: because debugging is probably something you do on your local machine, or using a VBox + vagrant. Emulating prepared statements is a good way to spot malformed queries without having to burden your DB server. Still, once you go live: use real prepared statements, though.
Warning: don't just go ahead and switch to native prepared statements and push to production, of course. Always make sure your queries still work, and produce the same results as before! There are differences and gotcha's to keep in mind

What do I mean by "never trust the network"? I mean you really ought to sanitize, validate and check the user input you are using in your code. Take snippet 3 for example - I'll apply some critiques I gave both here, and on the linked review

//proper indentation, whitespace is your friend
$searchStmt = $db->prepare(
    'SELECT * FROM items WHERE tag LIKE :usersearch'
);
$searchStmt->execute(
    [
        'usersearch' => "%".$_GET['search']."%"
    ]
);

Now suppose $_GET['search']'s value was either one of the MySQL wildcards (_ , or % for example). They are perfectly valid strings, so they won't be escaped, resulting in a query not unlike

SELECT * FROM items WHERE tag LIKE '%%%';
//or
SELECT * FROM items WHERE tag LIKE '%_%';

Both of which equate to a slower version of this query:

SELECT * FROM items;

ie: you might end up selecting everything, and are most likely performing a full table scan whenever this query is executed. That's bad.
But a few lines later, you seem to have realized that the value of $_GET['search'] might just be an empty string:

if ($_GET['search'] == "") {
    header('Location:    http://mywebsite.com/photo');
}

But why perform the query first? Why not check the value, and prevent performing a full table select?

There are a couple of other issues, like foreach + include? Why not pour that code you need a couple of times into a function, include or require that file once, and call that function? I mean: it's less disk access, and less IO means better performance.
You also have the bad habit of re-assigning things without good reason:

$form = $_POST;
$name = $form[ 'name' ];
$desc = $form[ 'desc' ];
$link = $form[ 'link' ];
$tag = $form[ 'tag' ];
$category = $form[ 'category' ];

$sql = "INSERT INTO items ( `name`, `desc`, `link`, `tag`, `category` ) VALUES ( :name, :desc, :link, :tag, :category )";

$query = $db->prepare( $sql );
$query->execute( array( ':name'=>$name, ':desc'=>$desc, ':link'=>$link, ':tag'=>$tag, ':category'=>$category ) );

So you assign $_POST to $form, for every parameter, you create a new variable. Then you create a string and assign that to a variable called $sql, then, from that string, you create a PDOStatement instance, and assign that to yet another variable called $query, on which you call the execute method, constructing a new array using the variables you just extracting from an array?? Doesn't that seem a bit silly? Why not write the code like this:

$stmt = $db->prepare(
    'INSERT INTO items ( `name`, `desc`, `link`, `tag`, `category` )
     VALUES ( :name, :desc, :link, :tag, :category )'
);
$stmt->execute(
    array(
        ':name'     => $_POST['name'],
        ':desc'     => $_POST['desc'],
        ':link'     => $_POST['link'],
        ':tag'      => $_POST['tag'],
        ':category' => $_POST['category'],
    )
);

Not only does that look a hell of a lot tidier, it's actually more efficient, too. But there still is a problem with this code: I'm missing the isset checks. Now I could use a bunch of if's or ternaries here. Depending on what you want. If and which values are missing, you could then redirect, or use null as a default value. But a basic loop could work here, too:

/**
 * Wrap the code in a function, so you can re-use it easily
 * @param array $data the data to bind
 * @param array $keys the keys which contain the values we need
 * @return array
 */
function extractBind(array $data, array $keys)
{
    $bind = array();//the return array
    foreach ($keys as $key)
        $bind[':'.$key] = isset($data[$key]) ? $data[$key] : null;
    return $bind;
}
//in your case, this would be how you use this function
$stmt = $db->prepare(
    'INSERT INTO items ( `name`, `desc`, `link`, `tag`, `category` )
     VALUES ( :name, :desc, :link, :tag, :category )'
);
$stmt->execute(
    extractBind(
        $_POST,
        array(
            'name',
            'desc',
            'tag',
            'category'
        )
    )
);

Now I'm not saying you should use this code, but I'm just pointing out ways to make your life easier. You can use the function I wrote here, but you shouldn't pass the array it returns to the $stmt->execute call right away. Remember: never trust the network:
The values in the bind array should still be sanitized and validated. Check my profile on this site, and read through a couple of my "sanitize" and "validate" answers, I've also posted a couple of answers on XSS vulnerabilities, which might interest you, too...

\$\endgroup\$
  • \$\begingroup\$ Why would you only disable emulated prepares in production? This should be done always. And personally I'd disagree with always calling set names, I would say you should inspect the current PHP version and use that to either specify the charset in the DSN or issue a SET NAMES. \$\endgroup\$ – DaveRandom Jun 24 '14 at 14:18
  • \$\begingroup\$ @DaveRandom: not emulating prepares doesn't always mean better performance. In some cases, quite the opposite. I only disable emulation on environments where I know that it'll boost performance. the DSN + SET NAMES is simply because I don't know what the target environment will be: is charset=UTF8 going to be ignored, then if so: SET NAMES will fix that. If not, ah well: that's hardly going to be a noticeable bottleneck, is it? \$\endgroup\$ – Elias Van Ootegem Jun 24 '14 at 14:28
  • \$\begingroup\$ Disabling emulated prepares is not primarily a performance issue, it is a security issue. Moreover, disabling emulated prepares fundamentally alters the behaviour of PDO, to the point where the exact same code may work in dev but not in production... SET NAMES is less of an issue, I admit, it just looks dirty to me (actually, expecting to run code on <5.3.7 is just generally dirty). \$\endgroup\$ – DaveRandom Jun 24 '14 at 14:33
  • \$\begingroup\$ @DaveRandom: I agree on the last bit (supporting older PHP versions is, sadly, a necessary evil for some). Security-wise: differences between native prepares and emulated ones should be the same (last I checked). Yes, native prepares are more sensitive to malformed queries, I'll grant you that. But it's down to us to write good queries. If you do that, it shouldn't really matter. The main issue I have with MySQL prepared statements is: No Cache! No Query Cache, and on some versions no prepared statements cache! \$\endgroup\$ – Elias Van Ootegem Jun 24 '14 at 14:43
  • \$\begingroup\$ PDO does address the query result cache issue somewhat. I'm looking through the source to see how the emulated prepares are allocated, but I suspect they are allocated using pemalloc (PHP's internal persistent allocator), effectively caching the prepares, too \$\endgroup\$ – Elias Van Ootegem Jun 24 '14 at 14:45
4
\$\begingroup\$

Okay

All of them

  • Don't have multiple statements on the same line like that.
  • Use either spaces or tabs, don't use both.
  • <?php something; ?> <?php somethingElse; ?> is pointless. Don't do that.
  • Be consistent. Sometimes you're doing funcName( $param ) and sometimes funcName($param). Pick one and stick with it.

1)

  • Looks good security wise. See above All of them for improving.

2)

  • Looks good, but too little information to know. If you haven't already, set PDO::ATTR_ERRMODE to PDO::ERRMODE_EXCEPTION and PDO::ATTR_EMULATE_PREPARES to false. See `PDO::setAttribute().

3)

  • Sometimes you are redirecting to localhost, sometimes to mywebsite. Use a variable. See $_SERVER.
  • }} is horrible.

4)

  • Very good. Queries that don't use parameters don't need to be prepared. Note however that there's a very fine line! Every time you need a variable inside of a query, you want a prepared statement.

5)

6)

  • Look at the all of them list for details on how to improve.

All in all, you must improve your code-style. Your code is extremely unreadable and will be hard to maintain in the future.

Aside from that one XSS vulnerability, it looks good to me, but again, it's hard to tell just with the code you've given.

\$\endgroup\$
1
\$\begingroup\$

First off a general comment about your formatting, I would suggest one statement per line. Program your fingers to type ;<cr>. This will not only make it easier for you to find the statement you want to change later, but it will also make it easier for you and others (like say, users of codereview.stackexchange.com) to read. Also, if you use VC (you are aren't you?), one statement per line makes it easier to determine what has changed when reviewing diffs.

That said, this code does appear to be safe from SQL injection through your use of PDO and parameterized statements.

Onto the nitpicking :).

  1. Other than the one statement per line thing I mentioned above nothing really wrong here. One thing that took me a long time to learn is that PDO doesn't actually require the : prefix on the array keys, so your array could be written as: [ 'name' => ..., ... ]

  2. I'm not sure if you're using the root user here as an example or not, but if this is your actual setup I would recommend creating a database user with only the permissions needed by your app and disable remote access for the root user.

  3. There are a couple of issues with this block of code. First off, the indentation is inconsistent. It's difficult to understand what this code is doing despite being a relatively short piece. Also, there is no need to drop out of PHP to include another PHP file in the foreach. Aside from the this, the overall flow is not optimal. If the search parameter is the empty string, this code will perform a DB query and process the results before discarding everything and redirecting to another page. The last if statement belongs inside the initial if s$tatement. Also, using rowCount() on the result of a SELECT statement will not return consistent results for different database drivers.This entire block should be restructured, e.g.:

    if (isset($_GET['search']) && !empty($_GET['search'])) {
        $searchStmt = $db->prepare('SELECT * FROM items WHERE tag LIKE    :usersearch');
        $searchStmt->execute([ 'usersearch' => "%".$_GET['search']."%" ]);
    
        $count = 0;
        foreach ($searchStmt as $row) {
            include '...';
            $count++;
        }
        if ($count === 0) {
            echo "Sorry, your search for <i>\"".$_GET['search']."\"</i> was not found...";
        }
    } else {
        header('Location: http://mywebsite.com/photo');
    }
    
  4. Nothing to say here, looks fine to me.

  5. Other than the issues I've already covered, there appears to be phantom closing PHP tag. I'm guessing this is a copy page error. Also, nothing is being done with the results of the related tags query. If this just because it has not been included in the posted segment? Also, if tags can be created by users, they should be URL/HTML encoded before output to avoid subjecting your users to XSS attacks.

  6. Same comment about dropping out of PHP to include a file.

Finally, a general comment about code styles, try and be consistent in your use of syntax. Some snippets use array(...) syntax while others use the array shorthand [ ... ]. I would suggest following a published PHP style guide. PHP-FIG offers a couple of levels of guide. PSR-1 defines how to organize code into different files and how to name some elements. PSR-2 is much more comprehensive and covers things like how much and where to use whitespace.

\$\endgroup\$
  • \$\begingroup\$ "a published PHP style guide" I agree, 100%. That's why it's the first thing I mentioned in my answer. Just wanted to add that the PHP-FIG is probably the best style to adopt and therefore advocate on sites like these \$\endgroup\$ – Elias Van Ootegem Jun 24 '14 at 14:31
  • \$\begingroup\$ Thanks, I've edited my answer to point to the PHP-FIG style guides. Although it should be noted that PSR-2 is a little bit contentious in it's declaration of spaces over tabs. Because of this not even all of the PHP-FIG member projects completely follow PSR-2. \$\endgroup\$ – pgraham Jun 24 '14 at 15:02
  • \$\begingroup\$ If you check my reviews here, you'll note that I often point towards PHP-FIG, urging people to "follow the coding standards as much as possible". In respect to PSR-2: Personally, I prefer the allman-style indentation, so I am one of those people who doesn't fully follow PSR-2. That doesn't prevent me from promoting it: any standard, no matter how contentious, is better than no standard at all (with respect to coding style, of course...) \$\endgroup\$ – Elias Van Ootegem Jun 24 '14 at 15:40
  • \$\begingroup\$ @EliasVanOotegem I agree with you about PSR-2. My comment was directed towards others who may read my answer. I personally follow PHP-FIG closely and support what they are trying to do. Glad to hear others are spreading the word :) \$\endgroup\$ – pgraham Jun 25 '14 at 0:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.