CODEWITHSUNDEEP
php
Austin Collins
Austin Collins

Reputation: 451

PHP add query string if it does not exist

Before I start, I am completely new to PHP. This is probably a stupid error and I am probably doing this in the worst way possible.

I have this code:

<?php
    if (!isset($_GET['p'])) {
    $url .= '?p=home';
    header('Location:' . $url);
    exit;
}
elseif (!isset($_GET['sp'])) {
    header("Location: ".$_SERVER['REQUEST_URI'].'&sp=index.php';);
die();
}
include('/Page/' . htmlspecialchars($_GET["p"]) . '/' . htmlspecialchars($_GET["sp"]));
?>

Basically the url format is www.example.com/?p=PAGE&sp=SUBPAGE.php

The internal format from this will be /Page/PAGE/SUBPAGE.php

Reasons:

What's wrong:

The question:

How can I come about this?

Thank you, in advance.

Upvotes: 0

Views: 194

Answers (2)

alfallouji
alfallouji

Reputation: 1170

Your current code has a big security flaw.

One of the most important rule when you write an app, never trust data coming from client. Always assume, they will send you wrong or bogus data.

I would recommend you to create a configuration file defining what are the allowed routes.

By executing the following line, you just allowed anyone to access any file that would be readable by your webserver.

include('/Page/' . htmlspecialchars($_GET["p"]) . '/' . htmlspecialchars($_GET["sp"]));

What happens If you sent a valid value for $_GET['p'] and this for $_GET['sp'] : 

"/../../../../../../../../../../../etc/apache2/httpd.conf"

In this example, it will output your apache configuration file (assuming that this is the right path, if not an hacker can just keep trying path until he finds it).

To avoid this, you have two solutions.

1# Quick fix - You can sanitize & filter $_GET['p'] and $_GET['sp'] to ensure that you are not getting something '/../'. Make sure to add at least a file_exists before to avoid getting unwanted warning messages.

2# More elegant fix - You can create a configuration file that would contain the routes that you are accepting. If you are using a framework, there is a very high chance that you have a routing class or a routing module that you can just use. Otherwise, if you feel like implementing your own basic routing mechanism, you could do something as simple as in your case : 

<?php
/**
 * Defines the different routes for the web app
 * @file : config.routes.php
 */
return array(
    'page1' => array(
         'index.php',
         'subpage2.php',
         'subpage3.php',
    ),
);

Then, your code you would check if the $_GET['p'] and $_GET['sp'] are allowed values or not.

<?php
// Load config file containing allowed routes
$allowedRoutes = require(__DIR__ . '/config.routes.php');

$defaultPage = 'home';
$defaultSubPage = 'index.php';

$page = isset($_GET['p']) ? $_GET['p'] : defaultPage;
$subPage = isset($_GET['sp']) ? $_GET['sp'] : $defaultSubPage;

// If it is an invalid route, then reset values to default
if (!isset($allowedRoutes[$page][$subPage])) 
{
    $page = $defaultPage;
    $subPage = $defaultSubPage;
}

include $_SERVER['DOCUMENT_ROOT'] . '/Page/' . $page . '/' . $subPage;

Upvotes: 0

DannyTheDev
DannyTheDev

Reputation: 4173

I would approach the situation differently.

Set up some variables, for page and inner page with the default values and only re-assign them if the $_GET params are set

$page = 'home';
$inner_page = 'index.php';

if( isset( $_GET['p'] ) )
{
    $page = htmlspecialchars( $_GET['p'] );
}

if( isset( $_GET['sp'] ) )
{
    $inner_page = htmlspecialchars( $_GET['sp'] );
}

include $_SERVER['DOCUMENT_ROOT'] . '/Page/' . $page . '/' . $inner_page;

I've added $_SERVER['DOCUMENT_ROOT'] in too because if you use /Page then it is an absolute path, probably not what you were expecting, so prepending $_SERVER['DOCUMENT_ROOT'] ensures you're looking at the path inside your web directory.

Something else worth noting; Don't assume the data is good, people could try manipulate the files that are included.

http://your.domain/index.php?p=../../../../../&sp=passwd

So it would be worth trying to sanitize the data first.

Upvotes: 1

Related Questions