22

I've seen people (who generally write good code) directly alter the $_POST array with code like this:

// Add some value that wasn't actually posted
$_POST['last_activity'] = time();

// Alter an existing post value
$_POST['name'] = trim($_POST['name']);

// Our pretend function
// Pass the entire $_POST array as data to work with in the function
// The function update_record() will read only the values we actually need
update_record($_POST);

// ...That sure was easier than creating a new array 
//  with only the $_POST values we actually need.

It makes sense that update_record() should not access $_POST directly, so we can pass other arrays of data to it for instance, but surely this is lazy, bad design, or possibly just wrong? However, we are still passing a valid array to update_record(), so why create a new one?

This is not the point of the question, just an example of usage. However, I have heard plenty of people say that this should not be done with $_REQUEST data, and it's bad practice. But why? Looks harmless enough.

Examples:

  • Setting a default $_GET (or post) value that doesn't really exist

  • Adding $_POST values that weren't actually posted after a form submission

  • Directly sanitizing or filtering the $_GET array values or keys very early in the script (fallback sanitation... why not?)

  • Setting a $_POST value manually before form submission to populate an input with a default value (when the input reads $_POST for it's default value; I have done this)

  • Making up your own $_SERVER values? Sure, hey why not?

  • How about the others, like $_COOKIE and $_SESSION? Of course we have to modify those directly right? Then why not the others?

Should direct modification of superglobals never be done, or is it OK to do in some instances?

Improve this question
4
  • I'd agree with #1, #2, and #3 because it is unexpected usage (especially #1 and #2). Commented May 12, 2011 at 23:13
  • Good question. Modifying global arrays is wrong in the same way using global values is wrong. Also these arrays have their purpose (passing parameters from outside) that makes altering them a straight way to mess within the code. But, I believe some of these arrays may be sanitized at the begginning of the script, just not to cause problems within the code. Commented May 12, 2011 at 23:17
  • 1
    I'm using OO input array wrappers (implicit filtering), which print an additional notice when $_GET or $_POST variables get tampered with. It's still possible, but should be constrained to narrow situations. (Cross-module signaling, albeit only the dispatcher / front controller should need it.) Commented May 12, 2011 at 23:50
  • @mario: I'd love to hear more about how you accomplished that if you can take a look at this question: stackoverflow.com/questions/12954656 Commented Oct 18, 2012 at 12:34

6 Answers 6

Reset to default
15

Given that PHP is already setting those superglobals, I don't think it's evil to modify them. In some cases, it may be the best way to solve problems... particularly when dealing with third party code that you cannot easily modify. (They might use $_GET directly or assume some key exists in $_SERVER, etc.)

However, generally speaking, I think it's a bad practice when you are writing your own code. Modifying the $_REQUEST data with some behind the scenes filter that runs on every page automatically is likely to introduce side effects. (See all the problems that "magic quotes" caused for proof.)

So if you aren't going to do that (automatically filter the superglobals), then the following doesn't give you any benefits: 

$_POST['foo'] = filter($_POST['foo']);

when you can easily just do:

$foo = filter($_POST['foo']);

I think it's much more clear to make the site-wide distinction that $_POST and $_GET are always unfiltered, untrusted data, and they should never be used as-is.

By copying the filtered value to another variable, you are making the claim that, "I understand what I'm doing... I've filtered this input, and it's safe to use."

Improve this answer
3
  • Thanks for the input, my internet's been out for nearly 2 days so I haven't had a chance to reply to anyone. In the example, I modified $_POST and used it as an array of data to pass to an update function, presuming there are several other $_POST keys we'll be reading in that function. I would prefer to create a new array but I have seen people do this instead, so I'm still a bit unsure whether or not to call it "bad code", but I think it is at least leaning that way. I think any time you would feel the need to do this, there's always a better way. Commented May 14, 2011 at 11:41
  • 1
    @Wesley, the primary reason it is "bad" is that it makes it much more likely that you'll forget to sanitize some user data. In your example, you modify one key then pass the entire array. What if some of that data contains malicious input that is not processed? It's much better to build that new array by hand, copying only the things you need from $_POST into it, sanitizing as you go along. And regarding other people doing this... well, a lot of people write very bad PHP code, but that's no excuse for you to as well. :) Commented May 14, 2011 at 15:12
  • I think I should have stressed the other applications of superglobals abuse besides just sanitizing data. I probably should have left it out altogether and written a clearer question, people particularly like to pick up on the security aspects and often ignore the rest. Not to sound ungrateful, I really do appreciate the feedback. I'm going to wait one day and give this one the check mark, as you've addressed some good points but missed the voting party during the 3 minutes when the question was new :) Thanks again! Commented May 14, 2011 at 22:58
8

I would generally suggest that you shouldn't modify the pre-defined super-globals so that it's clear what is sanitised data and what's raw/untrusted data.

Others might suggest that if you clean up the superglobals at the start of the request cycle then you don't need to worry about them elsewhere.

I'd always match them out when you need them with:

$id = (int)$_POST['id'];

or similar.

In terms of the other variables it's good practice to not write to any of $_GET, $_POST, $_REQUEST, $_SERVER or $_COOKIE. $_SESSION however is different because you often want to write data into the session that's then persisted across different requests in the session.

Improve this answer
3
  • 2
    More reason why these kinds of globals should go away replaced with objects/methods that can be called on to get them when necessary. Why does setcookie exist, but we get cookies via $_COOKIE? Also, since $_COOKIE is only set when the current session starts, and is never updated, it requires that you alter/set cookies in both areas so that later areas of the code have up to date information. Commented May 13, 2011 at 5:03
  • Thanks James, I've been offline for a while so I couldn't respond. To make a long story short - I agree with you. There's always a better solution than writing to post/get/etc, but I'm still not sure if it's considered a strictly bad idea, as in "never do this ever". So, if I come across this type of code again, do you think I have a right to "call them out" on sloppy code, or can this be used in a clever, safe way sometimes? Commented May 14, 2011 at 11:45
  • @Wesley If it would be "never do this ever" the superglobals would be probably strictly read-only - they are not. I'd just call it bad practise to set or overwrite them in your application code - for said reasons. Commented Feb 6, 2014 at 19:20
2

You should avoid it. Maybe some time you forgot to sanitize something, then you can retrieve dangerous data. If you copy the data into a new structure while sanitizing

  • You only get, what you want/need and not what is in $_POST too
  • You will probably get an error, if the newly created array is missing some keys or is missing at all

Additional other scripts may assume, that the array is untouched and may react curious.

Improve this answer
2

I've never liked the idea of modifying superglobal because it is misleading. It is a quick hacky way to do something that there will almost likely be a better way to do.

If you change the value of $_POST, for example, then you're saying that software received data that it did not.

THE REAL PROBLEM

There is a real life situation where this becomes a big problem:

Imagine you're working in a team. In an ideal world, everyone uses the same syntax, but we don't live in an ideal world. One developer, John, likes to access posted data using $_POST. He changes something in the post vars:

$_POST['ranking'] = 2; // John has changed ranking from 1 to 2 for whatever reason

Then you have another developer, Chris, who prefers to use filter_input to access inputted (i.e. GET, POST, SERVER, COOKIE) data as it goes to protect the software when processing data that the user can tamper with. In his part of the software, he needs to get the post value of ranking. His part of the code is AFTER John's.

$ranking = filter_input(INPUT_POST, 'ranking', FILTER_SANITIZE_NUMBER_INT);
// $ranking = 1

From the example above, by changing a superglobal, you have broken PHP. John has set the value of $_POST['ranking'] to 2 for whatever reason, but now Chris has received a value of 1

When I've seen no other way to do it:

I worked on a project that used wordpress as its blog behind an AWS load-balancer. This changes the value of $_SERVER['remote_address']. In this instance, the other developer had no choice but to do the following:

if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
    $parts = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
    $_SERVER['REMOTE_ADDR'] = $parts[0];
}

Conclusion

There's almost certainly a better way than to change superglobals

Improve this answer
1

I think the real question here is “why should you modify theme?”. I don't see any valid reason to do so. If you need to sanitize an imput, you might want to use a local variable…

Unless you code is short enough (say, less than 50 lines long), modifying those super-global would only make your code harder to maintain and to undersand.

By the way you don't need to pass $_POST to the function, since it's a superglobal array that can ben accessed even within the local scope of a function.

Improve this answer
3
  • 3
    But he should pass it. Else its very hard to test and its not possible to call the function/method with other values without any (even uglier) hacks Commented May 12, 2011 at 23:29
  • Well, it depends on what his method does. If it's designed only to parse whatever is on the $_POST array, he doesn't need to pass it. Of course, if it serve a more general/abstract purpose, than you are right. Commented May 12, 2011 at 23:39
  • 2
    @Thomas, I agree with King here. Even if they are global, you shouldn't use global anything within other scopes because it causes tight coupling (which is why the function cannot be reused). Given your example, if the function is to sanitize data, why does it only sanitize $_POST data? Passing $_POST in makes the function sanitize any data. Commented May 13, 2011 at 5:00
0

After initially answering this question by saying there should be no reason to modify superglobals, I'm editing this answer with an example of a time when I've decided to.

I'm currently working on a URL rewrite database table whereby the request column directs the user to its corresponding target column.

For instance, a request might be blog/title-here and its target might be blog.php?id=1.

Since blog.php is expecting $_GET variables, and I don't want to change the header("Location:"), I'm left doing something like this:

$uri    = explode('?', $uri_request)[0];
$params = explode('?', $uri_request)[1];
parse_str($params, $_GET);

This creates a $_GET array containing the intended parameters passed by the target column.

Ultimately, I would strongly advise against modifying superglobals unless you absolutely have to.

Improve this answer