Fetching a post from the database based on a query parameter

Question

I get an id via Get request to fetch an object from the database.

I have used mysqli prepared statements to avoid any security problems. As I am new to these stuff I would like a confirmation that implemented the logic correctly and safe.

User goes to

website.com/post.php?post=130

In post.php i do the following:

Check if user sent a value in "post" parameter, otherwise redirect to the start page:

if(strlen($_GET["post"]) == 0){
    header("Location: index.php");
    exit(); 
}

then i use the paramter to fetch the object from the database:

$requestedID = $_GET["post"];
if(is_numeric($requestedID)){
  if($stmt = $mysqli->prepare("SELECT * FROM posts WHERE id =?")) {
     $stmt->bind_param('s', $requestedID);
     $stmt->execute();
     $result = $stmt->get_result();

     if($result->num_rows > 0) {
        $row = $result->fetch_assoc();
        // logic for the content 
     }else{
        header("Location: index.php");
        exit();   
     }

   } else {
     header("Location: index.php");
     exit();        
   }
 }else {
    header("Location: index.php");
    exit(); 
 }

Is my mysqli code secure? Is my handling of GET parameter secure enough? Do you see anything that concerns you?

cHao · Accepted Answer · 2017-01-04 07:03:32Z

There are two things that jump out at me:

You assume $_GET['post'] exists.

If someone just goes to post.php without specifying a parameter, $_GET['post'] is not set -- and any attempt to read it would trigger a notice like "Undefined index: post". Checking the length won't make that notice go away; even the act of getting the length starts with the assumption that your parameter exists.

You really ought to put that variable into a known state before you make any assumptions about it. It might be as simple as, say:
```
$requestedID = isset($_GET['post']) ? $_GET['post'] : null;
```
You don't even need the strlen check; null and '' aren't numeric, so they'd both fail the is_numeric check.

(Course, the bigger issue is that you have error reporting set to ignore notices. You should generally be developing with error reporting turned all the way up, even if you turn it back down for production.)

You repeat the same error-case code three times. If you ever want to change how you handle nonexistent or unspecified post IDs, you have to change the code in three places.

That could be fixed by changing the flow a bit.

$requestedID = isset($_GET["post"]) ? $_GET['post'] : null;
$row = null;

if (is_numeric($requestedID)) {
  if ($stmt = $mysqli->prepare("SELECT * FROM posts WHERE id =?")) {
    $stmt->bind_param('s', $requestedID);
    $stmt->execute();
    $result = $stmt->get_result();

    # oh...this too.  $result can be false if something went wrong.
    if ($result && $result->num_rows > 0) {
      $row = $result->fetch_assoc();
    }
  }
}

// $row is only set (and truthy!) if you succeeded
if ($row) {
  // logic for the content
}
else {
  header("Location: index.php");
  // exit(); not necessary if this is the last line of the script 
}

Stack Exchange Network

Fetching a post from the database based on a query parameter

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Fetching a post from the database based on a query parameter

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions