StrawberryGraphQLError in Custom Extension causes 500 error response instead of formatted error #3204
Comments
|
Hey @staubina, with our current implementation this seems to be expected.
MAX_QUERY_LENGTH = 8192 # 8KB
class MaxQueryLengthExtension(SchemaExtension):
async def on_operation(self):
execution_context = self.execution_context
if len(self.execution_context.query) > MAX_QUERY_LENGTH:
error = StrawberryGraphQLError(message="Query too large")
execution_context.errors = [error]
yieldEDIT: as pointed out by Patrick, this won't work either since further execution is not stopped by simply adding an error to the execution context. |
|
@jkimbo @DoctorJohn do you remember why we didn't send these errors to the response object? /cc @erikwrede do you have any opinion on this? Context: #1324 |
|
@patrick91 for continuity this should be handled the same way we handle field extension errors or normal resolver errors, like in the permission extension. The Schema Extension error should in any case lead to stopping the request. There should be no way to throw an error during |
I missed your reply before answering! I think this won't work well because we'd have parsed the query by then, and we want to prevent to parse huge strings 😊
So you'd suggest to only handle GraphQL errors in our execute function?
Yup, that makes sense! |
Well spotted! Just noticed too that only adding the error does not stop further execution.
As far as I can tell, rrrors raised during I don't see a reason not catch those errors (at least GraphQL errors) and to add them to the result and end execution. Pretty sure this was just an oversight or that we thought there was no need for raising GraphQL errors |
Wondering about that too :) At least from the perspective of integrations, all "normal resolvers errors" are eventually converted into GraphQL(core)Errors and included in the results |
After looking at it again, I believe we should handle all errors the same, no matter if they originate in the extension or in the resolver. The detailed decision of how non-GraphQL Errors are handled could be moved to an ExceptionFilter in the web views. |
I think so too, we already treat any error inside resolvers as GraphQL errors, it is weird to not do that for parsing and operation errors 😊 @DoctorJohn what do you think? |
I agree. Already started working on it. Turns out 83 testc cases depend on the current behaviour. Looks like mainly for assertions within tests. I'll make sure to check whether any public facing and tested behaviour changes. |
|
After reading the docs here: https://strawberry.rocks/docs/guides/custom-extensions a few more times, I was able to get the following to work for now. Any potential pitfalls I am not seeing yet? Response |
Good job! You're correct, supplying a result from within the This is because of this check: strawberry/strawberry/schema/execute.py Lines 226 to 227 in 4130a75
Your query will be fully parsed and validated even when it's too long. If your goal is to mitigate DDoS attacks, performing the parsing and validation steps might be unwanted. |
|
That was my fear, I would like to stop it at an earlier stage for DDoS protection, but as you are all discussing any level above execute throws a 500. I was not able to determine an approach to catching the exception and handling it in my FastAPI app code. This is a start at least. |
Description
I am attempting to add a Custom Extension to catch queries that are too large. I then want the extension to raise StrawberryGraphQLError and return a properly formatted error message.
The issue is that when I raise StrawberryGraphQLError an internal error response is sent to the requester.
Stack Trace
Describe the Bug
System Information
strawberry-graphql[fastapi] == 0.209.2Upvote & Fund
The text was updated successfully, but these errors were encountered: