Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide boot error #2633

Merged
merged 6 commits into from
Mar 2, 2021
Merged

Hide boot error #2633

merged 6 commits into from
Mar 2, 2021

Conversation

clarkwinkelmann
Copy link
Member

@clarkwinkelmann clarkwinkelmann commented Feb 23, 2021

Fixes #2290

Changes proposed in this pull request:

Completely redact boot error unless debug mode or display_errors is enabled. Attempt to use Flarum log file when possible.

Use HTTP error code 500 whenever a boot error occurs, whatever the handling method is.

Reviewers should focus on:

Screenshot

If debug mode is enabled, most errors will look exactly as before:
image

If debug mode is disabled, most errors including database and extension related will end up showing this:
image

If an error happened even earlier, which is unlikely, the error will appear in one of two ways (example: the file vendor/flarum/core/src/Foundation/Application.php is missing).

If display_errors is Off:
image

If display_errors is On:
image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@clarkwinkelmann
Copy link
Member Author

There's one thing you could give me input on: do you think the two special cases (debug mode and logger available) should be wrapped in a try/catch themselves? If the container is broken, app() could fail. There's also the possibility of an extension registering a binding to either of the abstracts resolved in this code, in which case the code could be faulty. This is very unlikely, but if this happens, there will be no way of accessing the original error.

flarum.config is probably safe, and I don't see why someone would bind an alias to LogReporter. I was considering manually instantiating LogReporter by passing the LoggerInterface so we know for sure our own code always runs here.

The remaining extensible aspect is if an extension binds to the LoggerInterface. But even if that happened, it would be very unlikely to have a boot error after that binding and also having the logger fail. If a custom logger fails, then the extension likely is very broken and wouldn't be fit for production.

@askvortsov1
Copy link
Member

There's one thing you could give me input on: do you think the two special cases (debug mode and logger available) should be wrapped in a try/catch themselves? If the container is broken, app() could fail. There's also the possibility of an extension registering a binding to either of the abstracts resolved in this code, in which case the code could be faulty. This is very unlikely, but if this happens, there will be no way of accessing the original error.

As incredibly niche as this edge case is, I'm leaning towards handling it too, just so we don't need to worry about it ever again.

@clarkwinkelmann
Copy link
Member Author

It does seem a bit overkill now, but at least everything should be covered.

I also moved the code into methods that more accurately describe what is ideal and what is fallback.

src/Http/Server.php Show resolved Hide resolved

// Throwing the exception ensures it will be visible with PHP display_errors=On
// but invisible if that feature is turned off
// PHP will also automatically choose a valid place to log it based on the system settings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last line of the comment feels redundant given the contents of the echo above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I could remove it

@askvortsov1 askvortsov1 merged commit e37fdef into master Mar 2, 2021
@askvortsov1 askvortsov1 deleted the cw/hide-boot-error branch March 2, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Boot exception exposes database username and file system path
3 participants