set_error_handler should accept "only globally callable callback"

[mvorisek]

Description

see sebastianbergmann/phpunit#5844

set_error_handler (and set_exception_handler, maybe even more handlers) should accept only callables callable from any context. Accepting any callable like php does not is dangerous and unwanted, the error/exception can from any context.

I would expect php to evaluate "if the callable is callable" in global context on set phase.

repro: https://3v4l.org/mZ9E3

Resulted in this output:

...
Fatal error: Uncaught Error: Invalid callback Foo\SomeExternalDependency::logError, cannot access private
method Foo\SomeExternalDependency::logError()
...

But I expected this output instead:

Fatal error: Uncaught TypeError: set_error_handler(): Argument #1 ($callback) must be a valid globally
callable callback or null, Foo\SomeExternalDependency::logError() method is private"
...

[Girgias]

I don't think that's the solution, we could just convert the callable to a proper closure.

[mvorisek]

Do you mean accepting only \Closure?

The linked issue is about restoring the handler, but this issue is about set_error_handler in general - as long as callable is accepted, this feature request is about requiring the parameter to be "only globally callable callable".

[kkmuffme]

Against it.

Here's an example of why this matters and is more important than ever to allowe non-public methods:
we use a private method error handler to ensure all user privacy related things are removed from a stack trace.
If we now had a composer dependency that would do what phpunit does, they could just remove our error handler, send a stack trace to their remote logging with user data included and then just restore our error handler.
There would be no way for us to know (unless we manually inspect 1000s of composer dependencies recursively used in various projects we use) that we might accidentally have a data privacy leak somewhere.

[mvorisek]

private method/callable is not more secure than public \Closure as the private callable can be invoken by Closure::bind or reflection from any context.

[kkmuffme]

But it cannot be added back to the error handler callstack once it has been removed (from outside context)

[mvorisek]

Good clear point, but if set_error_handler is called in rebound context, it still can :)

[kkmuffme]

Could you provide an example of what you mean, bc it's not fully clear to me

[mvorisek]

https://3v4l.org/0Zf3P

[kkmuffme]

That's true but the instance of Foo is not the same as when we registered the exception handler, so we would know that someone tampered with it when logError is run

[sebastianbergmann]

I would consider a pull requests that implements this, of course.