1

Possible Duplicate:
PHP: “Notice: Undefined variable” and “Notice: Undefined index”

I'm doing youtube mvc tutorial http://www.youtube.com/watch?v=Aw28-krO7ZM and have stopped on the best first step:

my index.php file:

<?php
$url = $_GET['url'];
require 'controllers/' . $url . '.php';

My .htaccess file:

RewriteEngine On
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-l
RewriteRule ^(.+)$ index.php?url=$1 [QSA,L]

So, the question is next: when I go on any other URL except 'index' it works good. So what's wrong with 'index' url?

Notice: Undefined index: url in /var/www/sadman/index.php on line 2
Warning: require(controllers/.php) [<a href='function.require'>function.require</a>]:
 failed to open stream: No such file or directory in /var/www/sadman/index.php on line 4

Btw, i have LAMP, and I think that my settings are right.

Community
  • 1
  • 1
user1906992
  • 17
  • 1
  • 5
  • 2
    nothing in that "tutorial" is even remotely related to proper MVC pattern – tereško Dec 15 '12 at 22:52
  • 2
    This is highly vulnerable to a directory traversal attack via a NULL byte injection. You _MUST_ validate the contents of `$_GET['url']` against things like `../` and `%00`, or it is possible for a user to read any file on your filesystem that the web server has read access to (like /etc/passwd) – Michael Berkowski Dec 15 '12 at 23:00

1 Answers1

1

Notice tells you that key url does not exists in $_GET - probably index.php was called directly (like http://yourdomain.com/index.php - in this case rewrite does nothing as file exists).

Warning is because file does not exists.

To fix both do something like that:

$url = 'default'; // set default value (you need to have also existing file controllers/default.php)
// check if `url` exists in $_GET
// check if it is string
// check if it match proper pattern (here it has to be built from letters a-z, A-Z and/or _ characters - you can change it to match your requirements)
// check if file exists 
if (isset($_GET['url']) && is_string($_GET['url']) && preg_match('/^[a-z_]+$/i',$_GET['url']) && file_exists('controllers/'.$_GET['url'].'.php')) {
   $url = $_GET['url']; // name in $_GET['url'] is ok, so you can set it
}
require('controllers/'.$url.'.php');
lupatus
  • 4,208
  • 17
  • 19
  • +1 for file_exists() and better regex check, however not sure why you have is_string() as the regex check will only allow a string to pass. Replace isset() and is_string() with !empty() – kittycat Dec 15 '12 at 23:23
  • is_string is because if there will be an array (ex. direct call to index.php?url[]=something) you'll get something like that: `PHP Warning: preg_match() expects parameter 2 to be string...` – lupatus Dec 15 '12 at 23:25
  • There won't be an array as OP is not passing an array to require() – kittycat Dec 15 '12 at 23:26
  • there can be, see edit to my comment, rewrite is not passing array, but some bad dirty hacker can do that – lupatus Dec 15 '12 at 23:27
  • But they won't be able to do an attack though, it will just error. If we use this same mentality about an attacker converting a query to arrays then we would need to do is_string() for basically every call to $_POST or $_GET. – kittycat Dec 15 '12 at 23:33
  • yeah, it won't be attack, but anyway it will rise warning - I don't like warnings in my code ;). At least one can do something like that to avoid it: `preg_match($pattern, ((string) $_GET['url']))` – lupatus Dec 15 '12 at 23:39
  • passing any GET query as an array when it is not suppose to will likely cause an error somewhere. Reason you don't display errors on production sites so when attackers do crazy things it won't show them errors. – kittycat Dec 15 '12 at 23:43
  • I have turned off errors displaying in productional environment, but only 'just in case' - I prefer to have really full control on my code. second thing is that is_string is really cheap operation, it does not hurt to check it. in case when you'll compare execution time of `is_string($value) && preg_match($pattern, $value)` to just `preg_match($pattern, $value)` with `$value` not being a string - first version will be much faster, when it is a string there is almost no difference. – lupatus Dec 16 '12 at 00:23
  • I doubt a hacker would be worried about the speed difference when passing a non-string value, but ok whatever you're reasoning. – kittycat Dec 16 '12 at 00:30
  • Its not even about hackers (anyway they sometimes care about script execution time - preparing DOS attack its nice to find slowest script), I'm talking more about general behavior of testing input data. Mostly I'm logging all warnings and errors in productional environment, cause message like this means that something is wrong. I don't care about some guy that is passing strange data into my app unless it is handled properly by code, also during development I would rather like to see 'Invalid value in _GET[url]' message instead of 'PHP Warning: blah blah'. – lupatus Dec 16 '12 at 00:45