Make strawberry.Private compatible with PEP-563
#1684
Conversation
|
Thanks for adding the Here's a preview of the changelog: Add support for postponed evaluation of annotations ExampleThis release fixes Issue #1586 using schema-conversion time filtering of @strawberry.type
class Query:
foo: "strawberry.Private[int]"Forward references are supported as well: from __future__ import annotations
from dataclasses import dataclass
@strawberry.type
class Query:
private_foo: strawberry.Private[SensitiveData]
@strawberry.field
def foo(self) -> int:
return self.private_foo.visible
@dataclass
class SensitiveData:
visible: int
not_visible intHere's the preview release card for twitter: Here's the tweet text: |
Codecov Report
@@ Coverage Diff @@
## main #1684 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 130 130
Lines 4592 4594 +2
Branches 790 794 +4
=======================================
+ Hits 4510 4512 +2
Misses 43 43
Partials 39 39 |
| name_converter: Callable[[StrawberryField], str], | ||
| field_converter: Callable[[StrawberryField], FieldType], |
There was a problem hiding this comment.
Is there a reason these pull from args instead of using self. variables directly (which is where the values come from anywhere). Is this abstraction beneficial?
There was a problem hiding this comment.
name_converter can be pulled from self directly since all type definitions seem to use the same converter, but from what I've seen field_converter changes based on if it is called from self.from_input_object or self.from_object. The reason I made an abstraction here is to separate the logic of the filtering and the creation of a GraphQL Core ThunkMapping from that of selecting which converter(s) to use for the field. This is to prevent having to change _get_thunk_mapping if new types are added. With this in mind I didn't pull the converter(s) from self directly so that other functions (or in the future a mapping of Strawberry types to GraphQL types ) can decide which one to use.
The amount of code here could be reduced further if there is a way to have a single get_graphql_fields method that is responsible for selecting the right converters. But right now that's difficult to do since the static type of type_definition in from_input_object evaluates to Any. I couldn't find a way to address that quickly so I left it as is for now. There is still a lot of room for improvement but I need your views/opinions on that!
tests/schema/test_private_field.py
Outdated
| def test_private_field_with_str_annotations(): | ||
| """Check compatibility of strawberry.Private with annotations as string.""" | ||
|
|
||
| from dataclasses import dataclass | ||
|
|
||
| @strawberry.type | ||
| class Query: | ||
| not_seen: "strawberry.Private[SensitiveData]" | ||
|
|
||
| @strawberry.field | ||
| def accesible_info(self) -> str: | ||
| return self.not_seen.info | ||
|
|
||
| @dataclass | ||
| class SensitiveData: | ||
| value: int | ||
| info: str | ||
|
|
||
| schema = strawberry.Schema(query=Query) | ||
|
|
||
| result = schema.execute_sync( | ||
| "query { accesibleInfo }", root_value=Query(not_seen=SensitiveData(1, "foo")) | ||
| ) | ||
| assert result.data == {"accesibleInfo": "foo"} |
There was a problem hiding this comment.
Should we be instead (or in addition) having this test ensure that not_seen does not exist on the schema? This test passes exactly the same if the strawberry.Private wrapping is simply removed.
Also, a couple nitpicky requests:
- Unless it serves a purpose for this test (which I don't think it does 🤔), can you move the
dataclassimport to the top of the file? accessibleis spelled wrong
There was a problem hiding this comment.
Hey @BryceBeagle thanks a lot for the review! You made a good point about the test still passing if strawberry.Private is removed. I'll fix that right away
|
@BryceBeagle I've implemented your suggestions, thanks again for the great feedback. Adding a negative-engineering test actually revealed an important limitation: this PR works for classes defined on module-scope but not for within a function for example. This happens since From here I see three options:
def is_private(field: dataclasses.Field, raise_if_unresolved: bool = False) -> bool:
_type = field.type
if get_origin(_type) is Annotated:
return any(
isinstance(argument, StrawberryPrivate) for argument in get_args(_type)
)
elif _type is UNRESOLVED and raise_if_unresolved:
raise TypeError(
f"Could not determine if type of '{field.name}' is private as it cannot be"
"resolved. Ensure that the parent class of this field is defined in the "
"module namespace."
)
else:
return False
@staticmethod
def _get_thunk_mapping(
fields: List[StrawberryField],
name_converter: Callable[[StrawberryField], str],
field_converter: Callable[[StrawberryField], FieldType],
) -> Dict[str, FieldType]:
thunk_mapping = {}
for f in fields:
if f.type is UNRESOLVED:
raise TypeError(
f"Could not resolve the type of '{f.name}'. Check that the class "
"is accessible from the global module scope."
)
else:
if not is_private(f.type):
thunk_mapping[name_converter(f)] = field_converter(f)
return thunk_mapping
Let me know how you would like to proceed! Personally, I feel that Option 2 makes the most sense, since this scope problem is not unique to |
There was a problem hiding this comment.
Looks good to me! I'll wait for @BryceBeagle's final review before merging this :)
|
@skilkis I'd go with option 2, but I'd do it in another PR :) we don't support types defined in functions already and I think it does make sense to add a user friendly error :) |
There was a problem hiding this comment.
Hey @skilkis, thanks for the very thorough responses! I'm going to approve it now so it can get merged when needed, but I'll also leave my comments here:
this PR works for classes defined on module-scope but not for within a function for example
Yeah... this is an unsolved problem we've had for a while with a lot of our typing things. Main use-case has been in tests, so it's not been the worst thing at least.
From here I see three options:
I like option #2 the best. All of our types are supposed to be considered "unresolved" until schema generation to give room for everything to get defined in one step, then resolved in another. Feel free to add it in this PR, or open a ticket tracking it so it can be performed in a future one.
I don't understand #3, though. What "potential security vulnerability" are you referring to? Is there a potential attack vector here?
|
@BryceBeagle @patrick91 thanks for approving this PR! I'm excited to be contributing to Strawberry and happy to see that we all agreed on going for Option 2 😄 I would prefer to implement it here quickly before the PR gets merged. It's not a lot of code but can tie up this loose-end with the failing test and more importantly with private fields making their way into the schema. @BryceBeagle the security vulnerability I mentioned is about leaking sensitive data. I assume it occurs since the type gets resolved as 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
@strawberry.type
class Query:
@strawberry.field
def local_query(self) -> LocallyScopedQuery:
return LocallyScopedQuery(not_seen=LocallyScopedSensitiveData(1, "foo"))
schema = strawberry.Schema(query=Query)
import uvicorn
graphql_app = GraphQLRouter(schema)
app = FastAPI()
app.include_router(graphql_app, prefix="/graphql")
uvicorn.run(app, host="0.0.0.0", port=8000)Query: query LeakingData {
localQuery {
notSeen
}
}Output: {
"data": {
"localQuery": {
"notSeen": null
}
},
"errors": [
{
"message": "Expected 'None', got 'test_private_field_defined_outside_module_scope.<locals>.LocallyScopedSensitiveData(value=1, info='foo')'",
"locations": [
{
"line": 3,
"column": 5
}
],
"path": [
"localQuery",
"notSeen"
]
}
]
}The error message is long, but if you scroll you can see that the sensitive data gets leaked from the |
this is probably not the best behaviour, do you know in what other cases it happens? |
|
@patrick91 this is the only one I'm aware of right now. It can be solved by Option 2 since right now the type gets resolved to 'None' and then gets converted to a 'Void' field. I'll push a commit to showcase Option 2, and then you can decide if it belongs in this PR. Otherwise we drop it with a rebase and I make a new PR |
| raise TypeError( | ||
| f"Could not resolve the type of '{f.name}'. Check that the class " | ||
| "is accessible from the global module scope." | ||
| ) |
There was a problem hiding this comment.
can we make a custom exception for this? 😊
strawberry/field.py
Outdated
| @@ -39,6 +40,8 @@ | |||
|
|
|||
| _RESOLVER_TYPE = Union[StrawberryResolver, Callable, staticmethod, classmethod] | |||
|
|
|||
| UNRESOLVED = sentinel.create("UNRESOLVED") | |||
There was a problem hiding this comment.
I think we removed the sentinel package recently, can you rebase?
maybe we create our own sentinel without add the package back again :)
This commit Closes strawberry-graphql#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 strawberry-graphql#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/
for more information, see https://pre-commit.ci
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`
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.
|
@patrick91 I rebased to the latest release and implemented your suggestions! Should be ready to merge 😄 |
This commit Closes strawberry-graphql#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 strawberry-graphql#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/
patrick91
force-pushed
the
feature/make_strawberry_private_compatible_pep563
branch
from
cd1f40c to
a2905e2
Compare
|
I've rebased this again and fixed the sentinel 😊 |
|
Thanks for contributing to Strawberry! 🎉 You've been invited to join You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67 And don't forget to join our discord server: https://strawberry.rocks/discord 🔥 |
…#1684) * Make `strawberry.Private` compatible with PEP-563 This commit Closes strawberry-graphql#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 strawberry-graphql#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 strawberry-graphql#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 strawberry-graphql#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 <[email protected]>
Description
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.Privateis incompatible with postponedevaluation 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.Privateintypes.type_resolver.pyare left intact to not needlessly instantiateStrawberryFieldobjects whenstrawberry.Privateis resolvable.Summary of Changes:
strawberry.Privatefunctions correctlyTypes of Changes
Issues Fixed or Closed by This PR
Issue #1586
Checklist