Yeah, never do that. It's why register globals are deprecated and soon to be removed from the language. I repeat: DON'T DO THAT
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
The correct method, using $_GET, $_POST, $_SESSION, $_COOKIE , etc. You may think it's a clever fix but it is a huge security hole. Clever may be fun but it's usually not the correct solution.
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
Saying, "Clever may be fun but it's usually not the correct solution." is just plain wrong.
Clever is usually wrong, elegant is usually right. It's when you dont know the difference when things go bad. And if you think clever is the correct answer more often than elegant then I don't want to be anywhere near your code :)
You mentioned using a whitelist, he doesn't. Like I said, there's a reason register_globals is going away.
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
Well if we want to play word games you could dodge like that.
I would rather have a clever script than an elegant script, mainly because clever shows inventiveness and skill while elegant is just saying the script is pretty, graceful, and smooth. I would prefer a script that shows skill than a pretty script; tho these kinda go hand in hand. After all skill usually is accompanied by good security measures.
...
I just disagree with your statement about clever. Not trying to argue with you but this is just a play on words. You seem to be distraught about my statement.
It's not a play on words. There is a VERY large difference. Elegant can be clever but clever isn't always elegant.
And I don't mean elegant as in style like your braces are in alignment and you have consistant tabs/spaces, etc. That's not what I mean by elegant.
What I mean by elegant is the simplest solution possible to solve the problem that is A) maintainable and B) understandable. As I said, clever can be elegant but it rarely is. Cleverness lends itself to code show-boating. When programming you shouldn't be trying to prove you're the best programmer, you should be solving the problem. If you prefer coding in such a way that shows off your skills then I wouldn't want to be on your team.
Maybe I'm jaded from professional development but there is one quote that has always stuck with me:"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." --Brian Kernighan
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
OK, I'm not sure where I tried to offend you, if I did I apologize since that was never my goal and I'm not sure why you were reiterating the point about a word game when I already cleared that up. And you can't say that something is a matter of opinion then say the other person is wrong.
I've also said nothing of your skill, I've only made mention of your choice of development principle.
One could say it is a testament to your pride as a developer that you've been a PHP programmer for 9 years and still believe in your development principles which could be considered a good thing.
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
That's debatable.
I use foreach on $_POST/$_GET but i use a white list to prevent injection.
clever works as long as you cover all angles.
Saying, "Clever may be fun but it's usually not the correct solution." is just plain wrong. Usually clever works, sometimes it can backfire.
The foreach method saves time and looks cleaner. If you use this method I suggest using a white list, then comparing in the loop by using in_array. I have used this method for years.
If you're still around, it would be great if you could post the code for the white list method. I've been doing this for a long time and didn't realize is was a problem.
scaiferw
Junior Poster in Training
85 posts since May 2010
Reputation Points: 25
Solved Threads: 6
I'm a bit confused about the $_REQUEST variable now after reading this.
Any user input should always be treated as tainted, but as far as I can see, $_REQUEST is just a catch-all "automatic global". As long as the data is cleaned, where is the harm?
I went here:
http://php.net/manual/en/reserved.variables.request.php
It says nothing about about being depreciated. In fact, they have added a new method to define the array order as of 5.3.
Can someone clear this up for me?
Thanks.
JRM
Practically a Master Poster
621 posts since Nov 2006
Reputation Points: 130
Solved Threads: 75
I said nothing about _REQUEST being deprecated, I said register_globals is being deprecated. I said it was the reason WHY register_globals was deprecated because
foreach($_REQUEST as $key => $value)
$$key = $value;
is exactly what register_globals did which is a massive security risk. If you clean your variables that's different but in my opinion variable variables ( $$var ) and extract() are bad practices that make code hard to follow anyway.
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
Can anybody post a safe alternative, or the foreach($_REQUEST as $key => $value) {$$key = $value;} code modified to render it safe?
I've been using this for a long time, not realizing it wasn't safe. It would be great to have a safe version or equivalent.
scaiferw
Junior Poster in Training
85 posts since May 2010
Reputation Points: 25
Solved Threads: 6
Here's the safe alternative
$myvar = filter_var($_GET['myvar'], FILTER_VALIDATE_INT, array('options' => array('default' => 15)));
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
Here's the safe alternative
$myvar = filter_var($_GET['myvar'], FILTER_VALIDATE_INT, array('options' => array('default' => 15)));
Thanks for providing that.
Is that something we'd put inside the foreach noted previously, or would be do that separately for each variable we know we want to use?
scaiferw
Junior Poster in Training
85 posts since May 2010
Reputation Points: 25
Solved Threads: 6
no, foreach would go away. The way I usually do it is use a "clean" variable and filter_input_array_array to save time like.,
$clean = filter_input_array(INPUT_GET, array(
'page' => FILTER_VALIDATE_INT,
'email' => FILTER_VALIDATE_EMAIL
));
echo 'My page is ' . $clean['page'];
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268
It sounds then like I'd still have to be using concatenation to move in and out of quoted strings in my echo statements, which is what I was trying to avoid when I started using the foreach - I found it made my statements hard to read and debug.
If I understand your code, I'd still have to manually enter each attribute in the array?
What about using your code to sanitize the data, then use a foreach loop to create the variables as before? It there is a concern about malicious attributes being inserted, perhaps the list of attribute names could be used as a whitelist.
scaiferw
Junior Poster in Training
85 posts since May 2010
Reputation Points: 25
Solved Threads: 6
My opinion is that the foreach method is insecure, difficult to follow and overall bad practice. I don't believe it should ever be used A) because a method exists to do that already ie., extract($_REQUEST); is the same as foreach($_REQUEST as $key => $val) $$key = $val; B) it makes it hard to search for the initialization of a variable.
With the filter_input method it is easy to see where the variable comes from, what type it is (email, string, int, etc.), it has built in validation and sanitization based on what filter you used.
As for "I'd still have to manually enter each attribute in the array?" yes, yes you will. That's the point is that you should KNOW exactly which variables are coming in from GET. If you're trying to do it automatically because there are a lot of them or you don't know what it will be then there is something wrong with your application design.
As far as concatenation goes, I prefer that style, I find it cleaner and if you use an editor/IDE with any form of decent syntax highlighting it is much easier to read/debug (though more verbose than keeping it in the string). Some people will say something about performance but it's negligible either way so don't believe that one. It all boils down to preference, if you really don't want to concatenate you can still do echo "I have a string with $somearray[key]";
ShawnCplus
Code Monkey
1,583 posts since Apr 2005
Reputation Points: 526
Solved Threads: 268