PHP add query string if it does not exist

Question

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:

Each 'Post' will have it's own folder for it's resources. (?p=) Then they can put anything within their folder and link to it with the SUBPAGE (&sp=).
This keeps everything organised and stops any internal linking to anywhere but the /page/ area.

What's wrong:

If there is no Page (p) set I need to add ?p=home as the default and if their is no SubPage (sp) set I need to add &sp=index.php by default.
If there is no Page then the SubPage will be reset to default.
If you are on a page www.example.com/?p=blog then it will add the subpage without removing the page (?p=blog to ?p=blog&sp=index.php)
However if there is no page then the subpage will be reset to default.

The question:

How can I come about this?

Thank you, in advance.

DannyTheDev · Accepted Answer · 2015-03-02 17:20:46Z

1

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.

edited Mar 2, 2015 at 17:20

answered Mar 2, 2015 at 17:10

DannyTheDev

4,1732 gold badges24 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Austin Collins Over a year ago

Thank You, this works great. However I had to change one thing. Could you edit to add brackets around the include function and you need to put a / before the 'Page/'. This pay be my software but it will not work for without. Final: include ($_SERVER['DOCUMENT_ROOT'] . '/Page/' . $page . '/' . $inner_page);

DannyTheDev Over a year ago

@AustinCollins I'm glad it helped, brackets are optional for the include and require but you're right you may need the / for the path

Austin Collins Over a year ago

When it comes to the linking, they can only link within the /Page/ area. Is their a way to stop check if the $Page containes a slash before linking them?

DannyTheDev Over a year ago

@AustinCollins sure you could do something like if( strpos( $page, '/' ) === false ) { die('No Slashes allowed'); }

alfallouji · Accepted Answer · 2015-03-02 17:43:09Z

0

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;

edited Mar 2, 2015 at 17:43

answered Mar 2, 2015 at 17:32

alfallouji

1,1708 silver badges12 bronze badges

6 Comments

Austin Collins Over a year ago

But with the current code they can only link within the /Page folder?

alfallouji Over a year ago

That's what you had that on your original code : include('/Page/' . htmlspecialchars($_GET["p"]) . '/' . htmlspecialchars($_GET["sp"])); If you want other base path, you can just define that in your route config file or modify that line.

Austin Collins Over a year ago

I still have it on the current code. 'Page/' should be '/Page/'. For a quick fix could I remove all slashes (/). This would mean they would only go within one file?

alfallouji Over a year ago

It is up to you. You don't have to remove all slashes. Whatever decision you take, just make sure the values received as parameter don't contain ..

DannyTheDev Over a year ago

@AustinCollins ../ moves you up (back) a directory level.. i.e /var/www/Page/../ would take you to /var/www/

|

Collectives™ on Stack Overflow

PHP add query string if it does not exist

2 Answers 2

4 Comments

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related