feat(security): Add locking to prevent race conditions in API grant exchanges

- Add InvalidGrantError and ExpiredGrantError
- Add get_lock_key method to ApiGrant
- Add locking around grant exchanges in ApiToken.from_grant
- Add tests for race conditions

Author: mdtro

src/sentry/models/apigrant.py

@@ -15,6 +15,14 @@
 DEFAULT_EXPIRATION = timedelta(minutes=10)
 
 
+class InvalidGrantError(Exception):
+    pass
+
+
+class ExpiredGrantError(Exception):
+    pass
+
+
 def default_expiration():
     return timezone.now() + DEFAULT_EXPIRATION
 
@@ -97,6 +105,10 @@ def is_expired(self):
     def redirect_uri_allowed(self, uri):
         return uri == self.redirect_uri
 
+    @classmethod
+    def get_lock_key(cls, grant_id):
+        return f"api_grant:{grant_id}"
+
     @classmethod
     def sanitize_relocation_json(
         cls, json: Any, sanitizer: Sanitizer, model_name: NormalizedModelName | None = None

src/sentry/models/apitoken.py

@@ -19,7 +19,9 @@
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
 from sentry.hybridcloud.outbox.base import ControlOutboxProducingManager, ReplicatedControlModel
 from sentry.hybridcloud.outbox.category import OutboxCategory
-from sentry.models.apigrant import ApiGrant
+from sentry.locks import locks
+from sentry.models.apiapplication import ApiApplicationStatus
+from sentry.models.apigrant import ApiGrant, ExpiredGrantError, InvalidGrantError
 from sentry.models.apiscopes import HasApiScopes
 from sentry.types.region import find_all_region_names
 from sentry.types.token import AuthTokenType
@@ -261,17 +263,33 @@ def handle_async_replication(self, region_name: str, shard_identifier: int) -> N
 
     @classmethod
     def from_grant(cls, grant: ApiGrant):
-        with transaction.atomic(router.db_for_write(cls)):
-            api_token = cls.objects.create(
-                application=grant.application,
-                user=grant.user,
-                scope_list=grant.get_scopes(),
-                scoping_organization_id=grant.organization_id,
-            )
-
-            # remove the ApiGrant from the database to prevent reuse of the same
-            # authorization code
-            grant.delete()
+        if grant.application.status != ApiApplicationStatus.active:
+            raise InvalidGrantError()
+
+        if grant.is_expired():
+            raise ExpiredGrantError()
+
+        lock = locks.get(
+            ApiGrant.get_lock_key(grant.id),
+            duration=10,
+            name="api_grant",
+        )
+
+        # we use a lock to prevent race conditions when creating the ApiToken
+        # an attacker could send two requests to create an access/refresh token pair
+        # at the same time, using the same grant, and get two different tokens
+        with lock.acquire():
+            with transaction.atomic(router.db_for_write(cls)):
+                api_token = cls.objects.create(
+                    application=grant.application,
+                    user=grant.user,
+                    scope_list=grant.get_scopes(),
+                    scoping_organization_id=grant.organization_id,
+                )
+
+                # remove the ApiGrant from the database to prevent reuse of the same
+                # authorization code
+                grant.delete()
 
             return api_token
 

tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py

@@ -104,6 +104,22 @@ def test_deletes_grant_on_successful_exchange(self):
         self.grant_exchanger.run()
         assert not ApiGrant.objects.filter(id=grant_id)
 
+    def test_race_condition_on_grant_exchange(self):
+        from sentry.locks import locks
+        from sentry.utils.locking import UnableToAcquireLock
+
+        # simulate a race condition on the grant exchange
+        grant_id = self.orm_install.api_grant_id
+        lock = locks.get(
+            ApiGrant.get_lock_key(grant_id),
+            duration=10,
+            name="api_grant",
+        )
+        lock.acquire()
+
+        with pytest.raises(UnableToAcquireLock):
+            self.grant_exchanger.run()
+
     @patch("sentry.analytics.record")
     def test_records_analytics(self, record):
         GrantExchanger(