`MaskErrors` does not mask validation errors

[alimony]

The MaskErrors extension does not seem to mask validation errors. This is because the .on_operation() handler only looks at self.execution_context.result.errors while validation errors (such as misspelled field names, which is how I stumbled upon this) are in self.execution_context.errors.

I have subclassed MaskErrors in my project and added a .on_validate() handler that does the following:

def on_validate(self) -> Iterator[None]:
    if self.execution_context.errors:
        processed_errors: List[GraphQLError] = []
        for error in self.execution_context.errors:
            if self.should_mask_error(error):
                processed_errors.append(self.anonymise_error(error))
            else:
                processed_errors.append(error)

        self.execution_context.errors = processed_errors
    yield

This will correctly mask such validation errors as well.

Since I'm not very familiar with the internals of this project it was not immediately obvious how to write a test for this, since the existing tests in test_mask_errors.py all look at execution results. With some pointers on that, I could turn this ticket into a PR.

[erikwrede]

I believe this is by design and that there are two distinct cases of masking errors:

• don't leak unintended error information on accident (current implementation)
• don't leak private schema information (e.g. available fields) through validation

Both cases are valid and may be required on their own without implying the other case. Please keep in mind that both cases exclude parsing, but I don't see any immediate security risks from leaving these open.

An ideal PR would add your suggestion as a configurable option. Since this is a new feature, you could write an integration-style test with a basic schema and an invalid query that fails validation against the given schema.

Feel free to tag me on the PR regarding any additional questions, or ask on our Discord in the #development channel 😊

[alimony]

@erikwrede A configurable option on the MaskErrors class, or e.g. a MaskValidationErrors subclass of MaskErrors?

[erikwrede]

@alimony I'd prefer an option on the MaskErrors constructor. CCing @patrick91 for a second opinion.

[alimony]

@patrick91 What do you think?