Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question

migrated from stackoverflow.com May 15 '11 at 7:27

This question came from our site for professional and enthusiast programmers.

    
I'd agree with #1, #2, and #3 because it is unexpected usage (especially #1 and #2). –  Kevin Peno May 12 '11 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. –  Tadeck May 12 '11 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.) –  mario May 12 '11 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 –  Wesley Murch Oct 18 '12 at 12:34

5 Answers 5

up vote 8 down vote accepted

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."

share|improve this answer
    
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. –  Wesley Murch May 14 '11 at 11:41
    
@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. :) –  konforce May 14 '11 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! –  Wesley Murch May 14 '11 at 22:58

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.

share|improve this answer
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. –  Kevin Peno May 13 '11 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? –  Wesley Murch May 14 '11 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. –  Michel Feldheim Feb 6 at 19:20

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.

share|improve this answer

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.

share|improve this answer
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 –  KingCrunch May 12 '11 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. –  Thomas Hupkens May 12 '11 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. –  Kevin Peno May 13 '11 at 5:00

When you modify superglobals such as $_POST, you are tampering with the original data the user submitted.

This data is important. For instance, you can keep a log of everything your users have ever submitted and see where they sometimes are going wrong.

Ultimately, there is no reason to modify superglobals when you see how easy it is to simply create an array based on what the user has submitted. For instance:

$post = $_POST;

This creates a new array that you can tamper with to your heart's content. It looks nicer too:

echo $_POST["surname"];
// is the same as:
echo $post["surname"];

Also, I'm not a fan of creating a new variable for every bit of information the user has sent. Arrays are much easier to work with - even simply trimming the values:

// Why have this:
$username = trim($username);
$firstname = trim($firstname);
$surname = trim($surname);

// When you can have this instead:
array_walk_recursive($post, function (&$val) { 
  $val = trim($val); 
});
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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