0

I have the same question but...I'm redirecting the user depending on an if statement using headers to a dynamic page that is constructed through a function. For that function to work properly, it needs the parameters passed in the GET portion of the headers.

According to what to the answers provided, this is a bad practice. What way should I be doing it?

function page($title,$msg){
    $title = $_GET['title'];
    $msg = $_GET['msg'];
    echo '<h1>'.$title.'</h1>';

    echo '<p>';
    switch($msg){
        case 1:
            echo 'dwasdwadawdwadwa';
        break;
        case 2:
            echo 'wasdadwadwdad';
        break;
        default:
            echo 'wadasdasd';
        break;
    }
    echo '</p>';
}

ps: feel free to point out anything else you see wrong.

I found this but it doesn't really help me.

Community
  • 1
  • 1
alexdmejias
  • 1,399
  • 4
  • 19
  • 34
  • Which answers? Which question do you have the same question as? – ceejayoz Oct 10 '11 at 02:16
  • Well, first, not `echo` from a function and instead store the content into a variable that the function returns, ie, `return $func_html`. – Jared Farrish Oct 10 '11 at 02:18
  • 6
    Why does your function take arguments which are immediately overwritten? – Kerrek SB Oct 10 '11 at 02:22
  • @ceejayoz See the "Linked" section in the right-hand sidebar – Phil Oct 10 '11 at 02:30
  • It's not best practice, but it's also not bad practice. Since this seems to be a templating function (too abstract, please post real code next time), it's probably even appropriate for the `$title` to be taken directly from the GET parameters. It's imperative to use proper escaping here however (`htmlspecialchars`). – mario Oct 10 '11 at 03:19

4 Answers4

1

Although you aren't necessarily using the $_GET input for something that requires security considerations (in this case), it's a bad practice not to be sanitizing values from the URL.

Not only should you be checking for malicious input (especially if you are using the input to query a database), but you should be validating that expected integer values are indeed integers, and required strings are not empty.

Also, your page($title, $msg) function accepts $title and $msg and sets them, even though they are not passed by reference.

  1. If you expect to modify the input parameters, pass them by reference.

  2. If you need to use the input parameters, don't overwrite them immediately.

  3. If you don't need input parameters and only use values from $_GET locally to your function, declare page() without any arguments.

Rob
  • 5,223
  • 5
  • 41
  • 62
0

Why do you need to use GET? you can access all the same properties if you use POST which is also more safe

Bankzilla
  • 2,086
  • 3
  • 25
  • 52
  • how is POST more safe than GET? The only advantage of POST is that it allows you to send more data (fileuploads, very long textareas); but you dont need much more than Firebug, TamperData or any other webdev tool to manipulate POST data. – iHaveacomputer Oct 10 '11 at 03:07
  • It doesn't clutter url's/ show user data. example login.php?username=iHaveacomputer&password=generic. To get the information they entered all you need to do is, type the url of the site and hit the down arrow key. Instead of encouraging using GET for messages encourage POST as that's what it's there for. Nobody wants a 400 length url. – Bankzilla Oct 10 '11 at 03:17
  • URL clutter is a reasonable argument. The security scaremongering is not. Also please wait [a few more](http://stackoverflow.com/privileges/comment) points until you can post short questions as comments. – mario Oct 10 '11 at 03:21
0

The answer to the question you linked suggests that functions should not rely on any external (e.g. global) variables. $_GET and $_POST (amongst others) are 'super globals', a language feature of PHP that makes them available in any scope. This means they may be unexpectedly modified from anywhere in your scripts.

One way to help avoid this is to avoid using super globals in methods and instead - as the answer to the other question suggests - is to instead require parameters for the variables you would otherwise get from the super globals.

E.g., instead of:

function add_user() {
  $user = $_GET['user'];
  // ...
}
add_user();

You would use:

function add_user($user) {
  // ...
}
add_user($_GET['user']);

In your situation, what you would want is:

function page($title, $msg){
  echo '<h1>'.$title.'</h1>';
  echo '<p>';
  switch($msg){
    case 1:
      echo 'dwasdwadawdwadwa';
    break;
    case 2:
      echo 'wasdadwadwdad';
    break;
    default:
      echo 'wadasdasd';
    break;
  }
  echo '</p>';
}

Then, when you call page you would call it as:

page($_GET['title'], $_GET['msg']);
connec
  • 7,231
  • 3
  • 23
  • 26
  • `"...unexpectedly modified from anywhere in your scripts"` - Is that a realistic problem? – mario Oct 10 '11 at 03:16
  • @mario: Yes. If you are calling functions that modify the `$_GET` and `$_POST` variables, and others that read from them, changing the order of function calls can produce logical errors. – Rob Oct 10 '11 at 03:48
  • @mario: Fictional in this case, but completely realistic and common a cause of bugs. Minimizing the side effects of a function is always a good idea. This mindset is the reason for practices like immutability and stateless programming: http://stackoverflow.com/questions/844536/advantages-of-stateless-programming – Rob Oct 10 '11 at 04:00
  • @robjb: Well, possible and probable are two different things. I for one have never witnessed such code with the outcome being unexpected. – mario Oct 10 '11 at 04:03
  • @mario: I would say it's highly probable if you're modifying a commonly used global from various functions. But of course doing this isn't such a good idea in the first place, so it would be unlikely in well-developed code. – Rob Oct 10 '11 at 04:20
-1

Not sure if i understand your question, but here is some code i use handle my ajax calls with:

$mc = new MyClass();
echo $mc->{$_GET["operation"]}($_GET);

This means "operation" refers to your method name inside MyClass and i dont have to add a new switch statement for each method. Now i can just add a function "addRecord($args)" to MyClass, and my ajax call would look like this:

ajax_users.php?operation=addRecord&name=testuser&dob=1980-01-01

your php function receives the arguments in an array, so inside function addRecord() you have to access the variables like $args['name'] and $args['dob'], and it dosnt matter how many parameters you have to pass on to your method.

Make sure you use prepared statements here or proper escaping to prevent sql injections.

iHaveacomputer
  • 1,427
  • 4
  • 14
  • 30