views:

149

answers:

6

I'm making a little site for a friend, noobfriendly, so she can add pages easy. I think I'm just gonna include everything in index.php. So she can just drop a page in a folder named /pages/ and it's done.

index.php

if (preg_match('/[a-zA-Z]/', $_GET['page'])){
$page = 'pages/'.$_GET['page'].'.php';

if ($page) {
include $page;
} else {
exit;
}

} else {
include('pages/home.php');
}

This is pretty safe right? I'm only allowing letters a-z. I mean it's not gonna be a big site like google. Well I'm just a hobbycoder so I'm asking guys to make sure :)

What do you think?

+1  A: 

No. a/../../../ will also match the regexp

Use '/^[a-zA-Z]+$/'

This way you make sure that there's nothing before and after the sequence of the allowed letters.

Ivan Krechetov
+3  A: 

You could use this.

ctype_alnum($_GET["page"]);
Ólafur Waage
+2  A: 

To secure this, make sure your regex is /^[a-z]+$/i. This checks the whole string (^ is the start, and $ is the end) to be sure that it is alphabetical only. Then your work will be secure (at least in this part).

The Wicked Flea
+1  A: 

It would be a great idea to run some checks on the input before you decide to include it. For one thing, you could do an is_file($filename) before including.

One other thing, your regex is set to only allow one character atm - probably not what you want. Another thing: as the previous poster noted, the regex only checks if the input looks sort of ok. After that, you use the _GET variable. You SHOULD use the result from preg_match (you can have it assign results to an array) to avoid the above problem.

Regards Fake

Fake51
A: 
preg_match('/[a-zA-Z]/', $_GET['page'], $match)

if ($match) {
    $page = "pages/$match.php";
    if(file_exists($page){
        include $page;
    } else {
        include "pages/404.php";
    }
} else {
    include "pages/home.php";
}

Maybe? If 'page' is set to 'blabla89349' this will include page 'blabla.php'. I'm not sure if that is what you intended? Otherwise you could be strict about it and

if ($match == $_GET['page']) {
...
0scar
+1  A: 

I really don't like reg editing out strings in paths, you never know what quirks of browsers and filesystems can be exploited. What you really should do is not actually use their input but use it to check against valid filenames.

Use glob or scandir to get a list of the files in the pages directory and then use in_array to see if the requested string is a file. If you try to strip out path elements, you leave yourself open to making a mistake.

wizard