6
\$\begingroup\$

Mostly asking for critiques of vulnerability. Am I using any functions or methods that are unsafe?

<?php
$menu = array( "page1","page2","page3" );
$defpage = "page1";

$section = $defpage;
if ( isset( $_GET['section'] ) ) $section = $_GET['section'];
if ( !in_array( $section, $menu ) ) $section = $defpage;
?>

This is code that checks if the section is in the array and then sets it as such, but hardwires it back to default if it's not valid.

\$\endgroup\$

4 Answers 4

6
\$\begingroup\$

Is it safe? Yes, it will currently do the right thing.

One of the features that plays in to best practice though, is how future proof it is. Over time, code gets edited, changed, etc. What you want is to make the code 'fail safe' in the future too. What if someone comments out the second line, you end up with a problem.

A better way to write your code would be to set the default, and only change it if the input is valid:

<?php
$menu = array( "page1","page2","page3" );

$section = "page1";
$input = $_GET['section'];
if ( isset( $input ) && in_array( $input, $menu ) ) {
    $section = $input;
}

?>
\$\endgroup\$
3
  • \$\begingroup\$ What I mean by "safe" is will it be safe from malicious code, injection, exploitation, etc, somehow people taking advantage of $_GET, etc. \$\endgroup\$ Jun 21, 2014 at 2:47
  • \$\begingroup\$ @user3761993 - 'safe' is a relative term. It is as safe as you can get (pun intended), within reason. Short of a PHP bug, you're good. \$\endgroup\$
    – rolfl
    Jun 21, 2014 at 3:19
  • 2
    \$\begingroup\$ For what it matters: Will raise an Notice error if $_GET['section'] isn't defined and all errors are displayed (which I advise). Overall, I think it's a bad habit to avoid. +1 for the fail proof behavior. \$\endgroup\$
    – jeromej
    Jun 21, 2014 at 11:07
3
\$\begingroup\$

I think it is.

In general, you should be careful when using any type of data that the user can set when that data is passed to a SQL query, to a fopen call, to the HTML document or to the system shell (or similar things), because you would be giving that user some control over those resources.

In your case, you are allowing just the values in $menu for the $section variable. Anything else would set that to $defpage, so it looks fine to me.

\$\endgroup\$
3
  • \$\begingroup\$ But is the $_GET safe? \$\endgroup\$ Jun 21, 2014 at 3:08
  • 2
    \$\begingroup\$ "But is the $_GET safe" - what is the problem with $_GET? If you handle insecurely, everything is unsafe. There is no "safe by default" thing. \$\endgroup\$
    – AKS
    Jun 21, 2014 at 6:32
  • \$\begingroup\$ "NEVER trust user input". \$\endgroup\$
    – jeromej
    Jun 21, 2014 at 11:08
1
\$\begingroup\$

It's safe, but bad practice.

A 404 page would be a better default and practise. You should show a relevant page, in this case 404. Just set the headers correctly, and make one of them pretty/funny 404 pages.

\$\endgroup\$
1
  • \$\begingroup\$ i agree with @Darius. It will help, in the long run, if you send failed attempts to a 404 page - and log these attempts. You will be surprised on what you will find in these logs. \$\endgroup\$
    – andrew
    Jun 21, 2014 at 14:34
1
\$\begingroup\$

Small note:

you keep on asking for "safe" - get is never 100% safe not even post. Just like Jerome suggested to never trust a user input the best safe way is to sanitize always your GET just in case.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.