I am using this script to get the admin pages via this query string ?page= once the admin is logged in.

Just needing a review, opinions

Function:

// GET PAGE FROM QUERY STRING
public function page_select($page) {$ext = '.php';
$file = 'pages/'.$page.$ext; if(file_exists($file)) {
return $file; } else {$file = 'pages/404'.$ext; return$file;
}
}


Index.php

if(isset($_COOKIE["Cckiuas"]) && isset($_SESSION["loggedin"])) {
if(isset($_GET['page'])) {$page = trim(htmlentities($_GET['page'])); } else {$page = 'dashboard'; }
include($backend->get_base_url().INCLUDES."header.php"); include($backend->page_select($page)); include($backend->get_base_url().INCLUDES."footer.php");
}


• You are vulnerable to directory attacks, I can get you to include any file by using the ../ to traverse up the directory structure, revealing details on your system:

example.com/index.php?page=../../../../../../../etc/passwd%00


The %00 represents the null byte character (See Null Byte Injection), which will terminate the string (thus ignoring your .php suffix). If I input that, I got all the users and passwords on the server (assuming the PHP user has read access, which it shouldn't, but most likely has).

• The $ext variable is redundant, are you by any chance going to stop using .php extension even though you won't stop using PHP? (Because, you know, the code is PHP) • htmlentities is pointless if you aren't placing the target string in HTML. (And even if you were, htmlspecialchars is probably better) • INCLUDES is magic. Immutable global variables (like constants) are still global! • Consistent spacing and line-breaks - If you started with multiline in functions and control structures, stick with it. • Ok, so it would be better to call / link the page as normal for example /page.php and then pass a query to it, ie. ?action=manage ? The idea behind this btw was to only call header and footer once with a side menu and just include the page content needed, for example data tables – CodeX Jul 2 '14 at 9:24 • The best thing is a very specific whitelist. (If the route matches this, it goes there). Don't try to do this part dynamically, at least until you know what you are doing. – Madara's Ghost Jul 2 '14 at 10:55 • Great advice, I'm changing it to a straight link to the page and then I'll use query strings to call specific functions within the page dependant on the action – CodeX Jul 2 '14 at 10:58 • The above would be similar to how wordpress does it btw – CodeX Jul 2 '14 at 10:59 I can't say much on the functionality, but I got a small comment on your if-statements: Be consistent: In your "Function" you have the blocks clearly cut out (linebreak & indention), In your "index.php" they are inlined. I suggest you always do it like in your "Function": if(isset($_COOKIE["Cckiuas"]) && isset($_SESSION["loggedin")) { if(isset($_GET['page'])) {
$page = trim(htmlentities($_GET['page']));
}
else {
$page = 'dashboard'; } //includes here }  That btw. also applies for your usage of single and double quotes. Compare: isset($_COOKIE["Ccuias"])
isset(\$_GET['page'])


Decide on one

• Are there any security concerns doing this? – CodeX Jul 2 '14 at 6:37
• @CodeX "I can't say much on the functionality." My PHP is quite bad. in fact I don't actually use PHP anywhere. it's just a few bits I caught here and there. For security you'll have to wait on another review. – Vogel612 Jul 2 '14 at 6:39