From f97c4fedcb136bbe68d4b3399636fa19b95cbae3 Mon Sep 17 00:00:00 2001 From: San Kilkis Date: Tue, 8 Mar 2022 05:23:17 +0100 Subject: [PATCH] Make `strawberry.Private` compatible with PEP-563 (#1684) * Make `strawberry.Private` compatible with PEP-563 This commit Closes #1586 by checking for private fields at schema conversion time. As per [PEP-563], in the future Python annotations will no longer be evaluated at definition time. As reported by Issue #1586, this means that `strawberry.Private` is incompatible with postponed evaluation as the check for a private field requires an evaluated type-hint annotation to work. By checking for private fields at schema conversion time, it is guaranteed that all fields that should be included in the schema are resolvable using an eval. This ensures that the current approach for defining private fields can be left intact. The current filtering for fields annotated with `strawberry.Private` in `types.type_resolver.py` are left intact to not needlessly instantiate `StrawberryField` objects when `strawberry.Private` is resolvable. Summary of Changes: - Added check for private fields at schema evaluation time - Added test to check that postponed evaluation with `strawberry.Private` functions correctly - Reduced code duplication in `schema_converter.py` [PEP-563]: https://www.python.org/dev/peps/pep-0563/ * Add RELEASE.md * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add docstring to `_get_thunk_mapping` * Improve testing of private fields This commit mainly addresses a limitation of PEP-563 wherby annotations tagetting classes defined outside of module-level scope (i.e. a function) cannot be resolved. As a result, even at schema-conversion time, checking `is_private` is not guaranteed to evaluate to a valid type. As a result, a test is added to ensure that a `strawberry.Private` annotated field that forward-references a locally-defined class does not appear in the generated Schema. Although this is a failing test for now it demonstrates the issue to prevent a potential security vulnerability. Summary of Changes: - Check that the `notSeen` private field does not exist in the schema and that querying it returns no data. - Add failing test to show issue of classes defined outside module-scope - Move import of dataclass to the top of the file - Correct typo in the word `accessible` * Prevent unresolved types from entering the schema This commit changes `StrawberryField.type` to return a new sentinel `UNRESOLVED` instead of `None` to make sure that unresolvable types such as those defined within a local scope of a function do not appear in the schema as a `Void` field. With the new behavior a `TypeError` will be raised during schema conversion if any types are still unresolved. Most importantly, this new behavior prevents a security vulnerability where `Strawberry.Private` annotated fields would previously appear in the schema as a `Void` field; thereby, becomming executible and potentially leaking sensitive data in the resultant error message. Summary of Changes: - Return a new sentinel `UNRESOLVED` from `StrawberryField.type` instead of `None` - Update `GraphQLCoreConverter._get_thunk_mapping` to raise a `TypeError` if any unresolved types exist during schema conversion - Update failing `test_private_field_defined_outside_module_scope` to check if the correct `TypeError` is raised and to further ensure that if no error is raised, that the `Strawberry.Private` annotated field does not enter the schema. * Make `strawberry.Private` compatible with PEP-563 This commit Closes #1586 by checking for private fields at schema conversion time. As per [PEP-563], in the future Python annotations will no longer be evaluated at definition time. As reported by Issue #1586, this means that `strawberry.Private` is incompatible with postponed evaluation as the check for a private field requires an evaluated type-hint annotation to work. By checking for private fields at schema conversion time, it is guaranteed that all fields that should be included in the schema are resolvable using an eval. This ensures that the current approach for defining private fields can be left intact. The current filtering for fields annotated with `strawberry.Private` in `types.type_resolver.py` are left intact to not needlessly instantiate `StrawberryField` objects when `strawberry.Private` is resolvable. Summary of Changes: - Added check for private fields at schema evaluation time - Added test to check that postponed evaluation with `strawberry.Private` functions correctly - Reduced code duplication in `schema_converter.py` [PEP-563]: https://www.python.org/dev/peps/pep-0563/ * Create custom exception for unresolved field types * Remove dependency on sentinel package * Fix sentinel Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Patrick Arminio --- strawberry/exceptions.py | 9 +++ strawberry/field.py | 7 +- strawberry/schema/schema_converter.py | 98 ++++++++++++++++++--------- tests/schema/test_private_field.py | 59 ++++++++++++++++ 4 files changed, 138 insertions(+), 35 deletions(-) diff --git a/strawberry/exceptions.py b/strawberry/exceptions.py index 6c121477a8..a0a61b7919 100644 --- a/strawberry/exceptions.py +++ b/strawberry/exceptions.py @@ -131,6 +131,15 @@ def __init__(self, annotation): super().__init__(message) +class UnresolvedFieldTypeError(Exception): + def __init__(self, field_name: str): + message = ( + f"Could not resolve the type of '{field_name}'. Check that the class is " + "accessible from the global module scope." + ) + super().__init__(message) + + class MissingFieldAnnotationError(Exception): def __init__(self, field_name: str): message = ( diff --git a/strawberry/field.py b/strawberry/field.py index 9b81f38f08..ffac40bd4d 100644 --- a/strawberry/field.py +++ b/strawberry/field.py @@ -40,6 +40,9 @@ _RESOLVER_TYPE = Union[StrawberryResolver, Callable, staticmethod, classmethod] +UNRESOLVED = object() + + class StrawberryField(dataclasses.Field): python_name: str @@ -192,7 +195,7 @@ def base_resolver(self, resolver: StrawberryResolver) -> None: _ = resolver.arguments @property # type: ignore - def type(self) -> Union[StrawberryType, type]: # type: ignore + def type(self) -> Union[StrawberryType, type, Literal[UNRESOLVED]]: # type: ignore # We are catching NameError because dataclasses tries to fetch the type # of the field from the class before the class is fully defined. # This triggers a NameError error when using forward references because @@ -212,7 +215,7 @@ def type(self) -> Union[StrawberryType, type]: # type: ignore return self.type_annotation.resolve() except NameError: - return None # type: ignore + return UNRESOLVED @type.setter def type(self, type_: Any) -> None: diff --git a/strawberry/schema/schema_converter.py b/strawberry/schema/schema_converter.py index 0a056c3bec..9e014532bf 100644 --- a/strawberry/schema/schema_converter.py +++ b/strawberry/schema/schema_converter.py @@ -1,7 +1,18 @@ from __future__ import annotations from enum import Enum -from typing import Any, Callable, Dict, List, Optional, Tuple, Type, Union, cast +from typing import ( + Any, + Callable, + Dict, + List, + Optional, + Tuple, + Type, + TypeVar, + Union, + cast, +) from graphql import ( GraphQLArgument, @@ -31,9 +42,11 @@ from strawberry.exceptions import ( MissingTypesForGenericError, ScalarAlreadyRegisteredError, + UnresolvedFieldTypeError, ) -from strawberry.field import StrawberryField +from strawberry.field import UNRESOLVED, StrawberryField from strawberry.lazy_type import LazyType +from strawberry.private import is_private from strawberry.schema.config import StrawberryConfig from strawberry.schema.types.scalar import _make_scalar_type from strawberry.type import StrawberryList, StrawberryOptional, StrawberryType @@ -166,6 +179,53 @@ def from_input_field(self, field: StrawberryField) -> GraphQLInputField: deprecation_reason=field.deprecation_reason, ) + FieldType = TypeVar("FieldType", GraphQLField, GraphQLInputField) + + @staticmethod + def _get_thunk_mapping( + fields: List[StrawberryField], + name_converter: Callable[[StrawberryField], str], + field_converter: Callable[[StrawberryField], FieldType], + ) -> Dict[str, FieldType]: + """Create a GraphQL core `ThunkMapping` mapping of field names to field types. + + This method filters out remaining `strawberry.Private` annotated fields that + could not be filtered during the initialization of a `TypeDefinition` due to + postponed type-hint evaluation (PEP-563). Performing this filtering now (at + schema conversion time) ensures that all types to be included in the schema + should have already been resolved. + + Raises: + TypeError: If the type of a field in ``fields`` is `UNRESOLVED` + """ + thunk_mapping = {} + + for f in fields: + if f.type is UNRESOLVED: + raise UnresolvedFieldTypeError(f.name) + + if not is_private(f.type): + thunk_mapping[name_converter(f)] = field_converter(f) + return thunk_mapping + + def get_graphql_fields( + self, type_definition: TypeDefinition + ) -> Dict[str, GraphQLField]: + return self._get_thunk_mapping( + fields=type_definition.fields, + name_converter=self.config.name_converter.from_field, + field_converter=self.from_field, + ) + + def get_graphql_input_fields( + self, type_definition: TypeDefinition + ) -> Dict[str, GraphQLInputField]: + return self._get_thunk_mapping( + fields=type_definition.fields, + name_converter=self.config.name_converter.from_field, + field_converter=self.from_input_field, + ) + def from_input_object(self, object_type: type) -> GraphQLInputObjectType: type_definition = object_type._type_definition # type: ignore @@ -177,18 +237,9 @@ def from_input_object(self, object_type: type) -> GraphQLInputObjectType: assert isinstance(graphql_object_type, GraphQLInputObjectType) # For mypy return graphql_object_type - def get_graphql_fields() -> Dict[str, GraphQLInputField]: - graphql_fields = {} - for field in type_definition.fields: - field_name = self.config.name_converter.from_field(field) - - graphql_fields[field_name] = self.from_input_field(field) - - return graphql_fields - graphql_object_type = GraphQLInputObjectType( name=type_name, - fields=get_graphql_fields, + fields=lambda: self.get_graphql_input_fields(type_definition), description=type_definition.description, ) @@ -209,18 +260,9 @@ def from_interface(self, interface: TypeDefinition) -> GraphQLInterfaceType: assert isinstance(graphql_interface, GraphQLInterfaceType) # For mypy return graphql_interface - def get_graphql_fields() -> Dict[str, GraphQLField]: - graphql_fields = {} - - for field in interface.fields: - field_name = self.config.name_converter.from_field(field) - graphql_fields[field_name] = self.from_field(field) - - return graphql_fields - graphql_interface = GraphQLInterfaceType( name=interface_name, - fields=get_graphql_fields, + fields=lambda: self.get_graphql_fields(interface), interfaces=list(map(self.from_interface, interface.interfaces)), description=interface.description, ) @@ -246,16 +288,6 @@ def from_object(self, object_type: TypeDefinition) -> GraphQLObjectType: assert isinstance(graphql_object_type, GraphQLObjectType) # For mypy return graphql_object_type - def get_graphql_fields() -> Dict[str, GraphQLField]: - graphql_fields = {} - - for field in object_type.fields: - field_name = self.config.name_converter.from_field(field) - - graphql_fields[field_name] = self.from_field(field) - - return graphql_fields - is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]] if object_type.is_type_of: is_type_of = object_type.is_type_of @@ -269,7 +301,7 @@ def is_type_of(obj: Any, _info: GraphQLResolveInfo) -> bool: graphql_object_type = GraphQLObjectType( name=object_type_name, - fields=get_graphql_fields, + fields=lambda: self.get_graphql_fields(object_type), interfaces=list(map(self.from_interface, object_type.interfaces)), description=object_type.description, is_type_of=is_type_of, diff --git a/tests/schema/test_private_field.py b/tests/schema/test_private_field.py index 76a4396f3c..821a7a3b6f 100644 --- a/tests/schema/test_private_field.py +++ b/tests/schema/test_private_field.py @@ -1,3 +1,5 @@ +from dataclasses import dataclass + import pytest import strawberry @@ -55,3 +57,60 @@ def age_in_months(self) -> int: assert result.data == { "ageInMonths": 84, } + + +@strawberry.type +class Query: + not_seen: "strawberry.Private[SensitiveData]" + + @strawberry.field + def accessible_info(self) -> str: + return self.not_seen.info + + +@dataclass +class SensitiveData: + value: int + info: str + + +def test_private_field_with_str_annotations(): + """Check compatibility of strawberry.Private with annotations as string.""" + + schema = strawberry.Schema(query=Query) + + result = schema.execute_sync( + "query { accessibleInfo }", + root_value=Query(not_seen=SensitiveData(1, "foo")), + ) + assert result.data == {"accessibleInfo": "foo"} + + # Check if querying `notSeen` raises error and no data is returned + assert "notSeen" not in str(schema) + failed_result = schema.execute_sync( + "query { notSeen }", root_value=Query(not_seen=SensitiveData(1, "foo")) + ) + assert failed_result.data is None + + +def test_private_field_defined_outside_module_scope(): + """Check compatibility of strawberry.Private when defined outside module scope.""" + + @strawberry.type + class LocallyScopedQuery: + not_seen: "strawberry.Private[LocallyScopedSensitiveData]" + + @strawberry.field + def accessible_info(self) -> str: + return self.not_seen.info + + @dataclass + class LocallyScopedSensitiveData: + value: int + info: str + + with pytest.raises(TypeError, match=r"Could not resolve the type of 'not_seen'."): + schema = strawberry.Schema(query=LocallyScopedQuery) + + # If schema-conversion does not raise, check if private field is visible + assert "notSeen" not in str(schema)