Fix MaskErrors for synchronous pre-execution errors

Author: patrick91

RELEASE.md

@@ -0,0 +1,18 @@
+---
+release type: patch
+social_messages:
+  x: >-
+    {project_name} {version} is out! This release ensures `MaskErrors` also
+    masks parsing and validation errors during synchronous execution. 🍓
+    https://strawberry.rocks/release/{version}
+  linkedin: >-
+    {project_name} {version} is out. This release fixes `MaskErrors` so
+    synchronous parsing and validation failures no longer expose their original
+    error details.
+---
+
+This release fixes `MaskErrors` leaking parsing and validation error details
+during synchronous execution.
+
+Synchronous execution now masks pre-execution errors consistently with
+asynchronous execution, including when `ValidationCache` is enabled.

strawberry/extensions/mask_errors.py

@@ -39,12 +39,7 @@ def anonymise_error(self, error: GraphQLError) -> GraphQLError:
             original_error=None,
         )
 
-    # TODO: proper typing
-    def _process_result(self, result: Any) -> None:
-        errors = getattr(result, "errors", None)
-        if not errors:
-            return
-
+    def _process_errors(self, errors: list[GraphQLError]) -> list[GraphQLError]:
         processed_errors: list[GraphQLError] = []
 
         for error in errors:
@@ -53,7 +48,13 @@ def _process_result(self, result: Any) -> None:
             else:
                 processed_errors.append(error)
 
-        result.errors = processed_errors
+        return processed_errors
+
+    # TODO: proper typing
+    def _process_result(self, result: Any) -> None:
+        errors = getattr(result, "errors", None)
+        if errors:
+            result.errors = self._process_errors(errors)
 
     def _process_stream_result(self, result: StreamExecutionResult) -> None:
         self._process_result(result)
@@ -79,6 +80,11 @@ def on_operation(self) -> Iterator[None]:
             self._process_result(result)
         elif initial_result := getattr(result, "initial_result", None):
             self._process_result(initial_result)
+        # Synchronous parsing and validation failures don't populate `result`.
+        elif pre_execution_errors := self.execution_context.pre_execution_errors:
+            self.execution_context.pre_execution_errors = self._process_errors(
+                pre_execution_errors
+            )
 
     def on_stream_result(self, result: StreamExecutionResult) -> Iterator[None]:
         """Mask errors before a streamed execution result reaches the client."""

strawberry/schema/schema.py

@@ -831,54 +831,63 @@ def execute_sync(
             operation_extensions, sync=True
         )
 
+        pre_execution_result: ExecutionResult | None = None
+
         try:
             with extensions_runner.operation():
                 # Note: In graphql-core the schema would be validated here but in
                 # Strawberry we are validating it at initialisation time instead
-                if (
-                    pre_execution_result := self._prepare_operation_sync(
-                        execution_context, extensions_runner
-                    )
-                ) is not None:
-                    return pre_execution_result
-
-                assert execution_context.graphql_document is not None
-                with extensions_runner.executing():
-                    if not execution_context.result:
-                        result = execute_function(
-                            self._schema,
-                            execution_context.graphql_document,
-                            root_value=execution_context.root_value,
-                            middleware=middleware_manager,
-                            variable_values=execution_context.variables,
-                            operation_name=execution_context.operation_name,
-                            context_value=execution_context.context,
-                            is_awaitable=optimized_is_awaitable,
-                            **execution_context_class_kwargs(
-                                self.execution_context_class
-                            ),
-                            **custom_context_kwargs,
-                        )
+                pre_execution_result = self._prepare_operation_sync(
+                    execution_context, extensions_runner
+                )
 
-                        if isawaitable(result):
-                            result = cast("Awaitable[GraphQLExecutionResult]", result)
-                            ensure_future(result).cancel()
-                            raise RuntimeError(  # noqa: TRY301
-                                "GraphQL execution failed to complete synchronously."
+                if pre_execution_result is None:
+                    assert execution_context.graphql_document is not None
+                    with extensions_runner.executing():
+                        if not execution_context.result:
+                            result = execute_function(
+                                self._schema,
+                                execution_context.graphql_document,
+                                root_value=execution_context.root_value,
+                                middleware=middleware_manager,
+                                variable_values=execution_context.variables,
+                                operation_name=execution_context.operation_name,
+                                context_value=execution_context.context,
+                                is_awaitable=optimized_is_awaitable,
+                                **execution_context_class_kwargs(
+                                    self.execution_context_class
+                                ),
+                                **custom_context_kwargs,
                             )
 
-                        result = cast("GraphQLExecutionResult", result)
-                        execution_context.result = result
-                        # Also set errors on the context so that it's easier
-                        # to access in extensions
-                        if result.errors:
-                            execution_context.pre_execution_errors = result.errors
-
-                            # Run the `Schema.process_errors` function here before
-                            # extensions have a chance to modify them (see the MaskErrors
-                            # extension). That way we can log the original errors but
-                            # only return a sanitised version to the client.
-                            self._process_errors(result.errors, execution_context)
+                            if isawaitable(result):
+                                result = cast(
+                                    "Awaitable[GraphQLExecutionResult]", result
+                                )
+                                ensure_future(result).cancel()
+                                raise RuntimeError(  # noqa: TRY301
+                                    "GraphQL execution failed to complete synchronously."
+                                )
+
+                            result = cast("GraphQLExecutionResult", result)
+                            execution_context.result = result
+                            # Also set errors on the context so that it's easier
+                            # to access in extensions
+                            if result.errors:
+                                execution_context.pre_execution_errors = result.errors
+
+                                # Run the `Schema.process_errors` function here before
+                                # extensions have a chance to modify them (see the
+                                # MaskErrors extension). That way we can log the original
+                                # errors but only return a sanitised version to the client.
+                                self._process_errors(result.errors, execution_context)
+
+            if pre_execution_result is not None:
+                # Operation hooks may replace pre-execution errors (for example,
+                # MaskErrors anonymises them), so finalise the returned result only
+                # after those hooks have completed.
+                pre_execution_result.errors = execution_context.pre_execution_errors
+                return pre_execution_result
         except (
             MissingQueryError,
             CannotGetOperationTypeError,
@@ -894,6 +903,8 @@ def execute_sync(
                 errors=errors,
                 extensions=extensions_runner.get_extensions_results_sync(),
             )
+
+        assert execution_context.result is not None
         return ExecutionResult(
             data=execution_context.result.data,
             errors=execution_context.result.errors,

tests/schema/extensions/test_mask_errors.py

@@ -4,7 +4,32 @@
 from graphql.error import GraphQLError
 
 import strawberry
-from strawberry.extensions import MaskErrors
+from strawberry.extensions import MaskErrors, ValidationCache
+
+
+@pytest.mark.parametrize(
+    "query",
+    [
+        "query { testField( }",
+        "query { missingField }",
+    ],
+)
+def test_mask_pre_execution_errors_sync(query: str):
+    @strawberry.type
+    class Query:
+        @strawberry.field
+        def test_field(self) -> str:
+            return "TestField"
+
+    schema = strawberry.Schema(
+        query=Query,
+        extensions=[ValidationCache, MaskErrors],
+    )
+
+    result = schema.execute_sync(query)
+
+    assert result.errors is not None
+    assert [error.message for error in result.errors] == ["Unexpected error."]
 
 
 def test_mask_all_errors():