graphql-core-legacy icon indicating copy to clipboard operation
graphql-core-legacy copied to clipboard

Error masking and internals leaking in error handling

Open ods opened this issue 7 years ago • 3 comments

There are several places (execute_graphql(), complete_value(), complete_value_catching_error(), resolve_or_error(), execute() ExecutionContext.report_error(), may be there is more) where the library indiscriminately catches all exceptions and reports them to client. It's correct behaviour for parsing/usage errors. But for programming and runtime errors there are problems:

  • original error is lost and traceback is not reported, so it becomes hard to debug;
  • potentially sensitive information is leaked to client via error message.

I believe the the right behaviour would be to catch and report to client specific exceptions only (GraphQLError and subclasses?) while propagating the rest.

Related issues:

ods avatar Sep 28 '18 14:09 ods

This has been bugging me a lot, since I want to raise and catch my own Exception subclasses for some occasions in my app. This library currently makes this impossible, without changing the code in the library itself too.

Using except Exception as e has pretty much never been a good idea :/ But looking through the code, I see it pop up in a lot of places, so I wasn't sure where to start. @ods I saw that you changed a lot (maybe all) of it in your linked commit f81ca08. Did this fix it completely for you?

cobalamin avatar Oct 07 '18 23:10 cobalamin

Did this fix it completely for you?

Yes, it did so far. There are actually two commits in my fork: second is to silence annoying noise in logs. I'm going to issue a pull request after some time of using it in our project.

ods avatar Oct 08 '18 05:10 ods

Pull request: #211

ods avatar Oct 17 '18 18:10 ods