How should $_GET be checked

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
ngungo
Forum Commoner
Posts: 75
Joined: Thu Jun 08, 2006 10:45 pm

How should $_GET be checked

Post by ngungo »

Let's say a potential url is something like: http://mysite.com/myphp.php?param=foo
How and when (what diiference) should the $_GET['param'] be checked?

Code: Select all

if ($_GET['param']) == '') die('error');
else dosomething();
or

Code: Select all

if (isset($_GET['param'])) dosomething();
else die('error');
or

Code: Select all

if ($_GET['param'] == 'foo') dosomething();
else die('error');
or

Code: Select all

if (isset($_GET['param']) && ($_GET['param'] == 'foo')) dosomething();
else die('error');
or any other method?
Xoligy
Forum Commoner
Posts: 53
Joined: Sun Mar 04, 2007 5:35 am

Post by Xoligy »

The last one. All the others, except no2. which isn't specific enough, will create a notice and you should code with notices on, it makes life a lot easier.
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

Certainly the last one.
ngungo
Forum Commoner
Posts: 75
Joined: Thu Jun 08, 2006 10:45 pm

Post by ngungo »

What difference is between 3 & 4 (last 2)? Is that isset() of the last one redundant or overkill?
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

ngungo wrote:Is that isset() of the last one redundant or overkill?
No, it's not.
If you use #3 instead of #4 and the user doesn't send $_GET['param'] in the URL then PHP will generate a notice similar to this:
Notice: Undefined index: param in {path_to_script/script}.php on line {line_number_here}
P.S the {...}'s are just placeholders :wink:
ngungo
Forum Commoner
Posts: 75
Joined: Thu Jun 08, 2006 10:45 pm

Post by ngungo »

So would it be fair to say ... in general, always check isset first then check the value?
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

So would it be fair to say ... in general, always check isset first then check the value?
isset() checks if a variable exists. It the variable does not exist, then there not much else to do (which is why it's always the first part if the "if" condition). So yes, it would be fair to say an isset() should be done first.
User avatar
mpeacock
Forum Newbie
Posts: 10
Joined: Thu Apr 12, 2007 9:07 am
Location: Mobile AL

Post by mpeacock »

This:

Code: Select all

if (isset($_GET['param']) && ($_GET['param'] == 'foo')) dosomething(); 
else die('error');
Is similar to some pretty standard Java:

Code: Select all

if (request.getParameter("param") != null && request.getParameter("param").equalsIgnoreCase("Foo"))
{
...
}
Which is better abbreviated to:

Code: Select all

if ("Foo".equalsIgnoreCase(request.getParameter("param"))
{
...
}

So, in PHP, I bet you could dispense with the issset checks when you have a string literal you can check against, since it's doesn't really matter if the param is set. You really want to know if it's equal to the literal "foo".

This is probably what I'd do - since I rarely want my scripts to care about the request type:

Code: Select all

if ('foo' == $_REQUEST['param']) dosomething(); 
else die('error');
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

mpeacock wrote:So, in PHP, I bet you could dispense with the issset checks when you have a string literal you can check against, since it's doesn't really matter if the param is set. You really want to know if it's equal to the literal "foo".

This is probably what I'd do - since I rarely want my scripts to care about the request type:

Code: Select all

if ('foo' == $_REQUEST['param']) dosomething(); 
else die('error');
...which will promptly fire a E_NOTICE level error if $_REQUEST['param'] does not exist. Stick to isset().
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Code: Select all

if (isset($_GET['param']) && ($_GET['param'] == 'foo')) dosomething();
I'm pretty sure that's going to generate an E_NOTICE because the extra parenthesis forces the equality comparison to run before the isset.

2000th post!
dillion
Forum Commoner
Posts: 56
Joined: Thu Feb 15, 2007 8:32 am

Post by dillion »

would it not be better to do:

Code: Select all

$param = (isset($_GET['param'])) ? ($_GET['param']) : "";

if ($param == 'foo') dosomething();
else die('error');
?
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

ole wrote:

Code: Select all

if (isset($_GET['param']) && ($_GET['param'] == 'foo')) dosomething();
I'm pretty sure that's going to generate an E_NOTICE because the extra parenthesis forces the equality comparison to run before the isset.

2000th post!
No it won't, '&&' is a short-circuit AND - meaning it stops on the first FALSE.
User avatar
aaronhall
DevNet Resident
Posts: 1040
Joined: Tue Aug 13, 2002 5:10 pm
Location: Back in Phoenix, missing the microbrews
Contact:

Post by aaronhall »

No notice:

Code: Select all

if((isset($asdf)) && ($asdf == 'asdf')) echo 'not true';
Notice:

Code: Select all

if(($asdf == 'asdf') && (isset($asdf))) echo 'not true';
Post Reply