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

Make bad request exception generic unless in debug mode (0.15) #1131

Open
wants to merge 3 commits into
base: 0.15
Choose a base branch
from

Conversation

glye
Copy link

@glye glye commented Aug 17, 2023

Q A
Bug fix? debatable
New feature? no
BC breaks? tbd
Deprecations? no
Tests pass? tbd
Documented? not yet
Fixed tickets #1121
License MIT

(This is a 0.15 port of #1128 The README is outdated and it's not entirely clear which branches are stable now, but it seems to be 0.15 and 1.0. I'm unsure if this will be accepted, I will add doc and possibly tests if there is some approval.)

Exceptions and specifically stack traces should not be shown in production. This bundle has a customisable error handler, but as the documentation states: "Only query parsed error won't be replaced.". I could make my own Parser override class which catches the exceptions in prod mode, but then how can I produce a usable JSON response? I needed to modify the GraphController to do that.

The fix is to check kernel.debug and catch bad request exceptions. If debug mode is enabled, it rethrows the exception (i.e. works as before). If disabled (prod mode), it instead returns a blank JsonResponse with HTTP 400 Bad Request status code and the exception message, but not the stack trace. The simplest manual test is to access example.com/graphql in a browser, in debug and prod modes.

If rejected, please let me know if you can see ways of doing this in my project without modifying your bundle. Thanks!

Related: It would also be nice to disable introspection in prod mode by default, like shown in the doc. But since it hasn't been done yet, I assume the maintainers aren't keen on it.

@glye
Copy link
Author

glye commented Aug 18, 2023

Update in e1c522f: Use $e->getMessage() so that the HTTP 400 response includes the exception message, but not the stack trace. Works fine in my test.

Copy link
Member

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

Thank you @glye! Can you please check and make this change if good for you ?

src/Controller/GraphController.php Outdated Show resolved Hide resolved
Co-authored-by: Jeremiah VALERIE <[email protected]>
@glye glye requested a review from mcg-web February 23, 2024 14:54
@glye
Copy link
Author

glye commented Mar 20, 2024

@mcg-web Hi, I have made the change as requested. Could you please review it?

@glye
Copy link
Author

glye commented Jul 31, 2024

@mcg-web Hi, I have made the change as requested. Could you please review it?

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.

3 participants