Search code examples
phpmysqlsql-injection

SQL, Magic Quotes and 301 redirects - SQL injection risk?

We're upgrading php to 5.5 and magic quotes are now deprecated, so we're removing them and converting to PDO where we haven't already.

However, I came across a situation where there is no query to convert and I'm trying to determine if my code is okay.

This page is purely a 301 redirect from an old legacy code. This is basically the entire page code BEFORE:

<?php
$id = "0";
if (isset($HTTP_GET_VARS["id"])) {
  $id = (get_magic_quotes_gpc()) ? $HTTP_GET_VARS["id"] : addslashes($HTTP_GET_VARS["id"]);
}

header('HTTP/1.1 301 Moved Permanently');
header('Location: http://example.com/newpage.php?id='.$id);
exit();

?>
<head>
</head>

Here's the new code that I converted it to. I got rid of the magic stuff, also put in ctype_digit () to validate the input as a positive integer.

Again, this is the entire code for the page:

<?php

// grab the variable
$rid = 1;
if (isset($_GET["id"]) && ctype_digit($_GET["id"])) // if the variable exists and is a positive integer
    {$rid = ($_GET["id"]);}
else
{
//die("This page doesn't exist");
}

header('HTTP/1.1 301 Moved Permanently');
header('Location: http://example.com/newpage.php?id='.$rid);
exit();

?>
<head>
</head>

So, my question(s):

  1. Is this change reasonable and appropriate?

  2. I don't think anything bad could be passed through since I'm using ctype_digit(), plus you can assume that newpage.php is using PDO queries. Am I thinking about this correctly?

Thanks as always.

Solution

  • Your new code looks good.

    More security is possible before redirection:

    1) Parse the id to an int:

    $rid = intval($_GET['id']);

    See the documentation: http://php.net/manual/en/function.intval.php

    2) Check if the 'parsed' id exists in the database (or what you want)