Stop disabling super globals

Note: We are moving the topics of this forum and it will be deleted at some point

Publish your own request for comments/change or patches for the next version of phpBB. Discuss the contributions and proposals of others. Upcoming releases are 3.2/Rhea and 3.3.
User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Stop disabling super globals

Post by EXreaction » Sun Dec 08, 2013 1:34 am

I believe we should stop disabling super globals. This causes issues when trying to integrate with ANY other software that deals with the request (all other software).

I'm working on introducing Symfony Forms as well and it can't be done with the super globals disabled. Re-enabling and then disabling them again so errors are not thrown is not a good use of cpu time.

User avatar
MichaelC
Development Team
Development Team
Posts: 889
Joined: Thu Jan 28, 2010 6:29 pm

Re: Stop disabling super globals

Post by MichaelC » Sat Dec 21, 2013 8:29 pm

Why do forms need this specifically, forms should have full support for the symfony request class (which we use)? And if sites need this for integration, they can enable them themselves.

I fail to see reasoning why we should do this as your average novice php developer or a lot of mod authors will likely start using them if they are enabled (as it's what people do will by default if they don't know how about how use phpBB) which is a bad thing. If you don't know how to enable super globals, you shouldn't be enabling them.
Formerly known as Unknown Bliss
psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
No unsolicited PMs please except for quotes.

User avatar
John P
Posts: 156
Joined: Sun Nov 04, 2012 7:39 am
Location: Netherlands
Contact:

Re: Stop disabling super globals

Post by John P » Sat Dec 21, 2013 10:02 pm

I really like it and gives a security feeling by disabling super globals.
Where it isn't possible you can enable super globals so I don't see the problem.

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Stop disabling super globals

Post by imkingdavid » Sun Dec 22, 2013 1:52 am

There was a specific reason EXreaction pointed out to me on IRC, but I don't remember exactly how long ago it was (it's been at least a week, I think) and I don't feel like digging.

I cannot remember specifically which class had the issue, but the issue is that there is a class in Symfony Forms component that is ignorant of the Symfony request object and therefore uses $_SERVER to access a specific server variable. The method that causes the issue is at the end of a call stack with several functions in the list; to overwrite that class with one of our own would require overwriting several classes the stack. This is impractical and defeats the purpose of using Symfony in the first place.

Unless the Symfony class is somehow made aware of the Symfony request object, or we remove the whole disabling of super globals, the only way around this for us is to do:

Code: Select all

$request->enable_super_globals();
$form->some_function_that_ends_up_using_a_server_variable();
$request->disable_super_globals(); 
This, in my opinion, is very hackish and ugly, and takes up unnecessary processing time.

The only real benefit of which I'm aware that comes from the current system is to allow us to force people to not use the PHP globals and instead use our own sanitized versions. If we remove the disabled super global restriction, sure some less experienced developers may use them, but we can still require use of the $request object in Extensions (and, of course, core code), and we don't have to do hackish things like above.

Note that doing what I showed in the example above was an unacceptable solution for a similar issue I encountered when I had to add the ability to create a Symfony Request object (because it, by default, creates it directly using the globals). However, in that case, there was an alternate way of creating the Symfony Request object (using the static create() method and supplying the values myself using $request->get_super_global(); reference). In this case, there really isn't a way around it that I can see.

EDIT: And even if we find a solution that does not include removing the "disable super globals" functionality, I guarantee you this issue will continue to pop up in the future as we try to integrate more and more 3rd party stuff. I doubt it's worth the headache.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: Stop disabling super globals

Post by EXreaction » Sun Dec 22, 2013 5:10 am

There is a part in the Symfony Validator which checks $_SERVER directly (get content type or something like that) and causes this issue to come up. Many classes would need to be overwritten to be able to use the validator and it would take several hours of work. Basically the only way to get it to work now is to disable then re-enable super globals during the validation step for forms.

This isn't the first time we've run into this with libraries and certainly isn't going to be the last. This could be worked around by hours of work and a lot of ugly code, but sometime we're going to find it isn't possible at all.

User avatar
MichaelC
Development Team
Development Team
Posts: 889
Joined: Thu Jan 28, 2010 6:29 pm

Re: Stop disabling super globals

Post by MichaelC » Sat Dec 28, 2013 6:37 pm

I think is this more of a symfony bug and should be reported & fixed there (they'll likely want to fix this).
Formerly known as Unknown Bliss
psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
No unsolicited PMs please except for quotes.

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Stop disabling super globals

Post by imkingdavid » Sun Dec 29, 2013 3:42 am

For reference, I think NativeRequestHandler is the class in question. The use of $_SERVER is intended. I don't think there's a way to easily use a different Request Handler instead.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: Stop disabling super globals

Post by EXreaction » Sun Dec 29, 2013 3:10 pm

I have it setup to use the symfony request class, it's just a matter of passing it the request handler you want to use.

Frug
Registered User
Posts: 57
Joined: Thu Jul 02, 2009 3:33 am

Re: Stop disabling super globals

Post by Frug » Mon Dec 30, 2013 7:46 pm

So the only reason to cause all these issues is to force people to use best practices? Doesn't seem worth it to me. If they write insecure code and it's not caught during a review process, it's gonna happen anyway.

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: Stop disabling super globals

Post by bantu » Fri Jan 10, 2014 2:54 am

imkingdavid wrote:For reference, I think NativeRequestHandler is the class in question. The use of $_SERVER is intended. I don't think there's a way to easily use a different Request Handler instead.
The description indicates to me that you should not be using this handler at all. Use another handler or implement your own?
Frug wrote:So the only reason to cause all these issues is to force people to use best practices? Doesn't seem worth it to me. If they write insecure code and it's not caught during a review process, it's gonna happen anyway.
I strongly disagree. This is not about consciously producing insecure code. Good developers know best practices and bad developers (usually just novice people with little experience) will have to at least consciously enable super globals in order to use them. Ideally, they would get a link to best practices documentation when trying to use super globals. They can then decide whether they want to continue on the bad route by enabling super globals or do it properly. The request framework also enforces the type, so they will have to think about that too.

Post Reply