Prevent permission bypass when sync has_permission returns an awaitable (#4605)

Author: patrick91

RELEASE.md

@@ -0,0 +1,31 @@
+---
+release type: patch
+social_messages:
+  x: >-
+    Strawberry {version} is out! This release fixes a permission bypass where a
+    sync `has_permission` returning an awaitable could grant access to protected
+    fields. 🍓 https://strawberry.rocks/release/{version}
+  linkedin: >-
+    Strawberry {version} is out. This release fixes a permission bypass
+    (GHSA-pfvf-fwfp-25mp) where a synchronous `has_permission` that returned an
+    awaitable could unintentionally grant access to protected fields. The
+    synchronous permission path now fails closed with a clear error.
+---
+
+This release fixes a permission bypass (GHSA-pfvf-fwfp-25mp) where a custom
+permission could unintentionally authorize access to a protected field.
+
+When a permission's `has_permission` was a normal `def` that returned an
+awaitable (for example a wrapper returning a coroutine), Strawberry classified
+the permission as synchronous because only `async def` methods are detected as
+async. On the synchronous resolve path the returned awaitable was evaluated for
+truthiness directly, and an awaitable is always truthy — so the check passed and
+the protected resolver ran even when the awaitable resolved to `False`. This
+affected any field with a synchronous resolver, under both `execute_sync` and
+`execute`.
+
+Strawberry now detects this case and fails closed: the synchronous permission
+path raises a clear error instead of trusting the awaitable, so access is never
+granted by accident. Permissions written as `async def has_permission` continue
+to work as before. If you intend a permission to be asynchronous, declare it
+with `async def` (or return a plain boolean from a synchronous one).

strawberry/exceptions/__init__.py

@@ -169,6 +169,29 @@ def __init__(self, payload: dict[str, object] | None = None) -> None:
         self.payload = payload
 
 
+class PermissionReturnedAwaitableInSyncContextError(Exception):
+    """A permission returned an awaitable while being resolved synchronously.
+
+    Raised when a permission's ``has_permission`` returns an awaitable (for
+    example a plain ``def`` that returns a coroutine) but the field is being
+    resolved on the synchronous path, where the awaitable cannot be awaited.
+    Returning it as-is would be truthy and silently grant access, so we fail
+    closed instead.
+    """
+
+    def __init__(self, permission: object) -> None:
+        self.permission = permission
+        permission_name = type(permission).__name__
+        message = (
+            f"Permission {permission_name!r} returned an awaitable from "
+            "`has_permission` but is being resolved synchronously, so the "
+            "result cannot be awaited. Declare `has_permission` as "
+            "`async def` so Strawberry runs it on the asynchronous path, or "
+            "return a plain boolean."
+        )
+        super().__init__(message)
+
+
 __all__ = [
     "ConflictingArgumentsError",
     "DuplicatedTypeName",
@@ -191,6 +214,7 @@ def __init__(self, payload: dict[str, object] | None = None) -> None:
     "MultipleStrawberryFieldsError",
     "ObjectIsNotAnEnumError",
     "ObjectIsNotClassError",
+    "PermissionReturnedAwaitableInSyncContextError",
     "PrivateStrawberryFieldError",
     "ScalarAlreadyRegisteredError",
     "StrawberryException",

strawberry/permission.py

@@ -10,7 +10,10 @@
     ClassVar,
 )
 
-from strawberry.exceptions import StrawberryGraphQLError
+from strawberry.exceptions import (
+    PermissionReturnedAwaitableInSyncContextError,
+    StrawberryGraphQLError,
+)
 from strawberry.exceptions.permission_fail_silently_requires_optional import (
     PermissionFailSilentlyRequiresOptionalError,
 )
@@ -205,8 +208,25 @@ def resolve(
     ) -> Any:
         """Checks if the permission should be accepted and raises an exception if not."""
         for permission in self.permissions:
-            if not permission.has_permission(source, info, **kwargs):
+            has_permission = permission.has_permission(source, info, **kwargs)
+
+            # A permission whose `has_permission` returns an awaitable (e.g. a
+            # plain `def` returning a coroutine) cannot be resolved here: the
+            # awaitable is truthy regardless of what it resolves to, so trusting
+            # it would silently grant access. `supports_sync` only detects
+            # `async def` via `iscoroutinefunction`, so this case reaches the
+            # sync path. Fail closed instead of leaking the field.
+            if inspect.isawaitable(has_permission):
+                # Close the coroutine so we don't leak a "coroutine was never
+                # awaited" warning on top of the error we are about to raise.
+                if inspect.iscoroutine(has_permission):
+                    has_permission.close()
+
+                raise PermissionReturnedAwaitableInSyncContextError(permission)
+
+            if not has_permission:
                 return self._on_unauthorized(permission)
+
         return next_(source, info, **kwargs)
 
     async def resolve_async(

tests/schema/test_permission.py

@@ -705,3 +705,68 @@ def name(self, a_key: str) -> str:  # pragma: no cover
     result = schema.execute_sync(query)
 
     assert result.data["name"] == "Erik"
+
+
+def test_sync_permission_returning_awaitable_denies_access():
+    calls = 0
+
+    class DenyViaAwaitable(BasePermission):
+        message = "denied"
+
+        def has_permission(
+            self, source: typing.Any, info: strawberry.Info, **kwargs: typing.Any
+        ) -> typing.Any:
+            async def result() -> bool:
+                return False
+
+            return result()
+
+    @strawberry.type
+    class Query:
+        @strawberry.field(permission_classes=[DenyViaAwaitable])
+        def secret(self) -> str:
+            nonlocal calls
+            calls += 1
+            return "secret"
+
+    schema = strawberry.Schema(query=Query)
+
+    result = schema.execute_sync("{ secret }")
+
+    assert result.data is None
+    assert result.errors is not None
+    assert "returned an awaitable" in result.errors[0].message
+    assert calls == 0
+
+
+@pytest.mark.asyncio
+async def test_sync_permission_returning_awaitable_denies_access_on_async_execution():
+    calls = 0
+
+    class DenyViaAwaitable(BasePermission):
+        message = "denied"
+
+        def has_permission(
+            self, source: typing.Any, info: strawberry.Info, **kwargs: typing.Any
+        ) -> typing.Any:
+            async def result() -> bool:
+                return False
+
+            return result()
+
+    @strawberry.type
+    class Query:
+        @strawberry.field(permission_classes=[DenyViaAwaitable])
+        def secret(self) -> str:
+            nonlocal calls
+            calls += 1
+            return "secret"
+
+    schema = strawberry.Schema(query=Query)
+
+    result = await schema.execute("{ secret }")
+
+    assert result.data is None
+    assert result.errors is not None
+    assert "returned an awaitable" in result.errors[0].message
+    assert calls == 0