Is my php codes safe?

Question

Is my php codes safe ?

<?php

$item = (int)$_GET['item'];

if (!isset($_GET['item'])) {
    header('Location: index.php');
    exit;
}

$fileName = "items/" . $item . ".php";

if (file_exists($fileName)) {
    require_once ("items/" . $item . ".php");
} else {
    header('Location: index.php');
}

?>

@rcapote What do you mean? `(int)` is safe if you want to cast it to an integer. — alex, Nov 12 '10 at 08:06

score 2 · Answer 1 · answered Nov 12 '10 at 08:04

2

For better security, I think it should be better if you add validation on item:

$valid_items = array('item1', 'item2', 'item3');

if(in_array($item, $valid_items)) {
  // something if item is valid item
}

answered Nov 12 '10 at 08:04

subosito

3,420
2
24
12

1

based on his code, I don't think that would be viable, because he may not know what were the valid items at runtime – Gabi Purcaru Nov 12 '10 at 08:08
That's what I was about to write, until I noticed that `$item` is an integer. So in this case, there is no need for validation (since a cast to an integer makes it impossible to pass values like `..\..\..\protected-directory\index`). – Arseni Mourzenko Nov 12 '10 at 08:12
@MainMa give me some example of _working_ string literal that would result in something like that. All that I tried results in 0 when casted as int. – Gabi Purcaru Nov 12 '10 at 08:16
Yeh, I know the item is integer. I keep post the answer just in case as suggestion. But on my mind was, I thought weird if we require files but don't know what the content on those files, since it also parsed by PHP. We can skip item validation if we only stream it to the user without any parsing on server side. – subosito Nov 12 '10 at 08:21
agree. But we don't know the context and why there is a need to do such a thing. And personally, I can't even imagine why somebody would do it, instead of having a single PHP file for every item and calling `item.php?i=123`. – Arseni Mourzenko Nov 12 '10 at 10:10

score 1 · Accepted Answer · answered Nov 12 '10 at 08:07

1

I may use is_int() instead of casting. But your code seems fine to me.

You should handle exception messages with a ExceptionHandler.

Check if $_GET is defined before trying to access $_GET['item'].

answered Nov 12 '10 at 08:07

Sébastien VINCENT

1,096
6
11

"if $_GET is defined" ? what do you mean ? how can i do that ? – faressoft Nov 12 '10 at 08:19

score 0 · Answer 3 · answered Nov 12 '10 at 09:23

You can check first for the type of request method, something like

if($_SERVER['REQUEST_METHOD'] != 'GET') { 
       header('Location: index.php'); exit; 
}


if (!isset($_GET['item'])) {
    header('Location: index.php');
    exit;
 }

 $item = (int)$_GET['item']; 
/*
 * just make sure that all you pass is numeric before typecasting it. If you're not                                           
 * sure...you can do this 
 * $item = is_numeric($_GET['item']) ? (int)$_GET['item'] : null; //or 0
 *
 */

 //your code here

Is my php codes safe?

3 Answers3