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.
Stop disabling super globals
- EXreaction
- Registered User
- Posts: 1555
- Joined: Sat Sep 10, 2005 2:15 am
Re: Stop disabling super globals
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.
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
No unsolicited PMs please except for quotes.psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
Re: Stop disabling super globals
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.
Where it isn't possible you can enable super globals so I don't see the problem.
- imkingdavid
- Registered User
- Posts: 1050
- Joined: Thu Jul 30, 2009 12:06 pm
Re: Stop disabling super globals
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:
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
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 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();
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.
- EXreaction
- Registered User
- Posts: 1555
- Joined: Sat Sep 10, 2005 2:15 am
Re: Stop disabling super globals
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.
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.
Re: Stop disabling super globals
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
No unsolicited PMs please except for quotes.psoTFX wrote: I went with Olympus because as I said to the teams ... "It's been one hell of a hill to climb"
- imkingdavid
- Registered User
- Posts: 1050
- Joined: Thu Jul 30, 2009 12:06 pm
Re: Stop disabling super globals
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.
- EXreaction
- Registered User
- Posts: 1555
- Joined: Sat Sep 10, 2005 2:15 am
Re: Stop disabling super globals
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.
Re: Stop disabling super globals
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.
- bantu
- 3.0 Release Manager
- Posts: 557
- Joined: Thu Sep 07, 2006 11:22 am
- Location: Karlsruhe, Germany
- Contact:
Re: Stop disabling super globals
The description indicates to me that you should not be using this handler at all. Use another handler or implement your own?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.
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.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.