CODEWITHSUNDEEP
phphtmlformsincludedirectory
bear
bear

Reputation: 11625

Anything wrong with this PHP

Can you see anything wrong with this code, or can it be optimised?

Code from index.php, to include the file

if(empty($_GET['t'])) { 
    $folder = "apps/"; 
}else {
    $folder = str_replace("/", "", $_GET['t']) . "/"; 
}    

if(empty($_GET['app'])) { 
    include('apps/home.php'); 
} else { 
    if(file_exists($folder.$app.".php")) { 
        include($folder.$app.".php"); 
    } else  { 
        include("/home/radonsys/public_html/global/error/404.php");
    }
}

My problem? one page, which posts to itself doesnt find it's page and returns to that 404 page.

If you want, I can include the form code for that page?

Code from bugs.php

<form method="post" action="">
    <div>Title</div>
    <div><input name="title" type="text" class="bginput" value="" size="59" tabindex="1" /></div>
    <br />
    <div>
        <label class="smallfont">
            Application
            <select name="app" style="display:block; width:200px" tabindex="2">
                <option value="Admin CP">AdminCP</option>
                <option value="Add User">Add User</option>
                <option value="Bugzilla">Bugzilla</option>
                <option value="Portal">Portal</option>
                <option value="To Do">To Do</option>
                <option value="Internal Messages">Internal Messages</option>
                <option value="User CP">UserCP</option>
                <option value="Change Password">Change Password</option>
                <option value="Change Email">Change Email</option>
                <option value="General">General</option>
            </select>
        </label>
    </div>
    <br />
    <div>Bug Description</div>
    <textarea name="content" style="width:7%"></textarea>
    <br />
    <div>
        <label class="smallfont">
            Priority
            <select name="priority" style="display:block; width:200px" tabindex="2">
                <option value="0" selected="selected">Unknown</option>
                <option value="1">1 - Highest</option>
                <option value="2">2</option>
                <option value="3">3</option>
                <option value="4">4</option>
                <option value="5">5 - Medium</option>
                <option value="6">6</option>
                <option value="7">7</option>
                <option value="8">8</option>
                <option value="9">9</option>
                <option value="10">10 - Lowest</option>
            </select>
        </label>
    </div>
    <br />
    <input type="submit" value="Save" />
</form>

Clarification

The above script is in index.php, which calls on a page, e.g, ?app=bugs includes bugs.php in the apps folder.

Stuff on the bugs.php script uses POST to itself to send the data, however, post data never reaches the page itself since we're stuck with the error page, 404.php

Upvotes: 0

Views: 233

Answers (4)

dbr
dbr

Reputation: 169633

The problem you're having is because you are using method="post" in the form tag, and trying to get the data from $_GET.

When method = "post" the form values are accessible via $_POST['fieldname'] or $_REQUEST['fieldname'] (which contains both POST and GET values). You could also change the form's method to GET

However, the biggest problem I can see is..

include($folder.$app.".php");

That is scary, especially if you're using register_globals (which is the only place $app could come from, in the code you posted)

Say $_GET['app'] is set to..

../../something/else.php

..you would be including..

$_GET['t'] . "../../something/else.php"

If you have to dynamically include files based on user-input, strip out all non-alpha-numeric characters, and have a whitelist of valid files - something like the following:

$valid_files = array("General", "Todo");
$safe_filename = preg_replace("/[^a-zA-Z0-9]/", "", $_REQEST["app"]);
if(in_array($safe_filename, $valid_files)){
    include("apps/" . $safe_filename . ".php");
}

There are other ways of doing routing, for example using header("location: ...") :

header ('HTTP/1.1 301 Moved Permanently');
header ('Location: ' . $new_location);

Of course you'd have to safely sanitise $new_location but it has less issues than using include() (since it's not dynamically executing arbitrary scripts on your server)

Basically the script would do something like:

$safe_filename = preg_replace("/[^a-zA-Z0-9]/", "", $_REQUEST["app"]);
$new_location = "/apps/" . $safe_filename . ".php"; // construct new URL

// If it's valid, redirect, if not, return error 404
if(in_array($safe_filename, $valid_destinations)){
    header ('HTTP/1.1 301 Moved Permanently');
    header ('Location: ' . $new_location);
} else {
    header("HTTP/1.0 404 Not Found");
}

Upvotes: 0

bear
bear

Reputation: 11625

Well, the answer to why that certain application/page was having the problem was becuase the select id was app, which incidentally, was the app variable for the page, so it wasn't fetching the correct pages.

Tip Remember to name your properties carefully!

Upvotes: 0

Tom Haigh
Tom Haigh

Reputation: 57825

Some comments:

  • You can use $_REQUEST if you want to get the variable from POST or GET.
  • You don't seem to be setting $app anywhere.
  • You might want to consider being even stricter on what files you include (e.g. a whitelist or pattern that a filename must match).

Upvotes: 1

Robert Greiner
Robert Greiner

Reputation: 29722

you are saying the form posts to itself, does that mean you are using POST?
if so, you need to change $_GET[] to $_POST[]

The more code you post, the better.

Upvotes: 2

Related Questions