0
\$\begingroup\$

I have a MySQL database, with a table called articles. There is a column in articles for its id, name, date_created, etc.

I want to create a page for each article, for example:

example.com/article.php?id=1

example.com/article.php?id=2

...

My code works right now, but is there anything I should do differently? I'm worried about the safety of the code. Is there a better way to validate/sanitize the id?

<?php
        require_once('../mysqli_connect.php');




        if(isset($_GET['id'])){



            // Replace 
            $idv = preg_replace("#[^0-9]#","", $_GET['id']);


            $query = "SELECT name, description, date, id, date_created 
            FROM articles WHERE id = $idv LIMIT 1";

            $result = mysqli_query($dbc, $query);




            if ($result){

                $row = mysqli_fetch_assoc($result);


                if(!$row)
                {
                    //Row doesnt exist
                    //Redirect user somewhere else
                }
                else
                {
                    // echo blog post, with html
                }
            }



            else
            {
                echo "Couldn't issue database query <br>";  
                echo '<p>' . mysqli_error($dbc) . '</p>';
                // Redirect user
            }



        }

        else
        {
            //Redirect user
        }




        mysqli_close($dbc);

    ?>
\$\endgroup\$
3
  • \$\begingroup\$ Welcome to Code Review! I see a lot of //...s in your code that are in places where it seems like code would be. Have you included all the code that you would live reviewed? \$\endgroup\$ – SirPython Oct 25 '15 at 1:20
  • \$\begingroup\$ Hey Thanks! Those are placeholders. I have the code, but I took it out because I thought it would be simpler to understand. (As well, I mainly would like the overall logic of my code to be reviewed.) \$\endgroup\$ – deelie Oct 25 '15 at 1:23
  • 1
    \$\begingroup\$ I think that your post would be a lot more well-received if you included all the code in your post. \$\endgroup\$ – SirPython Oct 25 '15 at 1:25
1
\$\begingroup\$

In this case you just want your input to be an integer (or a long). So using the regex is one idea.

But you should also consider malicious users. What if you try

example.com/article.php?id=1000000000000000000000000000

Which is too big for int/bigint. When the exception is not caught well and your error pages expose information. Worst case this may expose information about the underlying directory structure. Which can be used by hackers while doing other attacks. And even when this is not the case, passing through exceptions are not a best practice.

I would rather use a built-in php method for that like intval.

As an addition to this, you can also use prepared statements which is another very good way to protect against sql injections.

$idv = intval($_GET['id']);      
if($idv > 0)
{
    $stmt = $mysqli->prepare("SELECT name, description, date, id, date_created FROM articles WHERE id = ? LIMIT 1");
    $stmt->bind_param("i", $idv);
    $stmt->execute();
    $stmt->bind_result($result);
    $stmt->fetch();
    $stmt->close();
}

ref:

\$\endgroup\$
0
\$\begingroup\$

There is a PHP function specifically for filtering input. Funny enough, it's called filter_input()

You would use it like so:

$id = filter_input(
        INPUT_GET,
        'id',
        FILTER_SANITIZE_NUMBER_INT
      );

if ( false === $id )
{
    // sanitization failed.
} 
else if ( is_null ( $id ) )
{
    // the 'id' key doesn't exist
}

This gives you a reliably clean way of doing what you want; no regex to get wrong, no ambiguous code (since filter_input() does what it says on the tin), and an easy way to tell whether the key didn't exist and if the sanitization failed. It leaves nothing to chance. With the above conditionals, you can be sure that you get an int when you think you'll get an int, and that you'll know when you didn't get an int.

When you want to sanitise for things other than an int, or when you want to perform validation on email addresses and so on, this function takes all the hard work and error-prone code out of it. (No more regex! Woohoo!)

It's a recent addition to PHP5 (PHP 5 >= 5.2.0, PHP 7), so it's not yet well known, but it's been added because there are all too many ways of trying to solve this problem, and even for those programmers that write comprehensive unit tests, there are always gotcha's which can leave security holes. PHP now has our back with this function; experts have written that function and tested it to death to fill the need.

You should check out that page's "See Also" section for related functions that can be used to sanitise arrays of input etc.

\$\endgroup\$

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.