simple ignore arguments in exceptions implementation

[krakjoe]

As alternative to and for the author of #4281

[eriklundin]

Short and elegant. Should be enabled by default to protect blissfully unaware sysadmins though. It would probably require fixing a lot of tests which seems to be dependant on the data in stack traces?

[nikic]

Would be good to test that this doesn't affect debug_backtrace/debug_print_backtrace (which already support this as an explicit call option).

[krakjoe]

Would be good to test that this doesn't affect debug_backtrace/debug_print_backtrace (which already support this as an explicit call option).

Fixed that, we don't want to modify the behaviour of the API function, as I did at first , we just want new exceptions to take notice of the ini setting, and retain all other (internal/user) APIs as they are ...

[krakjoe]

Note, no need to test debug_backtrace and debug_print_backtrace as modification is contained in new exception routine ...

[nikic]

This needs entries in php.ini-development and php.ini-production for the new ini option.

[krakjoe]

CI failures are unrelated ...

[nikic]

LGTM, but let's wait a couple of days in case anyone has more feedback here.

[krakjoe]

ACK

[nikic]

Short and elegant. Should be enabled by default to protect blissfully unaware sysadmins though. It would probably require fixing a lot of tests which seems to be dependant on the data in stack traces?

This PR enables zend.exception_ignore_args in php.ini-production. I think that's the right way to do this, as the general PHP defaults (without explicit ini file) are mostly geared towards development (we should probably fix the remaining discrepancies).

[dstogov]

Personally, I think it's not a good idea to use INI directive to control argument collection.
declare() might be more natural.

[eriklundin]

I have tested this now on a few development servers and Joe's fix seems to work great. I definitely think this should be declared on a server level (INI-file) and not disabled on a per script basis with declare().

[staabm]

Personally, I think it's not a good idea to use INI directive to control argument collection.
declare() might be more natural.

maybe it makes sense to forbidd setting it via ini_set() at runtime but leave it in php.ini ?

[krakjoe]

declare() might be more natural.

The decision to change this behaviour should be at application level, not really the author of some piece of code within an application. I can't think of any code that both throws exceptions and should need to be certain that stacktraces do not contain arguments. No such code can exist today.

maybe it makes sense to forbidd setting it via ini_set() at runtime but leave it in php.ini ?

The best justification for limiting a configuration setting to PHP_INI_SYSTEM is a technical justification/limitation, and since that doesn't exist here, it would seem like an arbitrary restriction to impose.

If we were to impose a system limit, that would remove the ability of an application to make the decision to hide arguments from traces, making shared hosting of applications that want to use this feature impossible.

[eriklundin]

Opening up security holes from php scripts on various levels isn't especially hard to do for a developer. It’s the default behaviour and the ability to control the default behavior that's important.

If someone writes scripts that logs all postdata on all requests. I would blame the developer. If an unexpected fatal error automatically logs all parameters (including passwords etc). I would blame the sysadmin. And if the sysadmin cant control this i would blame the engine for not allowing control of this.

These errors now occurs on fatal errors which could occur because an extension was enabled or disabled, php upgrades etc. Outside circumstances would then introduce this logging. If a developer would enable argument logging with ini_set() or such i would argue that this should fall under the first category (developers fault).

[bwoebi]

I have mixed feelings about the production default: In particular we already have display_errors=Off - stacktraces can and should probably land with full detail in the normal error logs (hidden from the consumer, visible for the developer/sysadmin)?

[nikic]

Let's land this.

[krakjoe]

Merged as 0819e6d

Thanks.