Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/sentry/auth/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -1079,12 +1079,12 @@ def _from_sentry_app(user: User, organization: Organization | None = None) -> Ac


def _from_rpc_sentry_app(context: RpcUserOrganizationContext | None = None) -> Access:
from sentry.sentry_apps.services.app import app_service
from sentry.sentry_apps.services.app.service import get_installation_by_proxy_user

if not context or context.user_id is None:
return NoAccess()

installation = app_service.find_installation_by_proxy_user(
installation = get_installation_by_proxy_user(
proxy_user_id=context.user_id, organization_id=context.organization.id
)
if installation is None:
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/ratelimits/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ def get_rate_limit_key(


def get_organization_id_from_token(token_id: int) -> int | None:
from sentry.sentry_apps.services.app import app_service
from sentry.sentry_apps.services.app.service import get_installation_org_id_by_token_id

organization_id = app_service.get_installation_org_id_by_token_id(token_id=token_id)
organization_id = get_installation_org_id_by_token_id(token_id)
# Return None to avoid collisions caused by tokens not being associated with
# a SentryAppInstallation. We fallback to IP address rate limiting in this case.
if not organization_id:
Expand Down
28 changes: 27 additions & 1 deletion src/sentry/sentry_apps/models/sentry_app_installation.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@
payload={
"sentry_app_id": self.sentry_app_id,
"organization_id": self.organization_id,
"proxy_user_id": self.sentry_app.proxy_user_id,

Check warning on line 170 in src/sentry/sentry_apps/models/sentry_app_installation.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

SentryApp.DoesNotExist not handled in outboxes_for_delete when accessing proxy_user_id

The new code at line 170 accesses `self.sentry_app.proxy_user_id` without handling `SentryApp.DoesNotExist`. Since SentryApp uses ParanoidModel (soft-delete), the parent SentryApp may be soft-deleted before the installation's outboxes_for_delete is called. The same file already demonstrates this pattern at lines 146-149 where `api_application_id` catches `SentryApp.DoesNotExist`. This will cause deletion to fail with an unhandled exception when the SentryApp has been deleted first.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SentryApp.DoesNotExist not handled in outboxes_for_delete when accessing proxy_user_id

The new code at line 170 accesses self.sentry_app.proxy_user_id without handling SentryApp.DoesNotExist. Since SentryApp uses ParanoidModel (soft-delete), the parent SentryApp may be soft-deleted before the installation's outboxes_for_delete is called. The same file already demonstrates this pattern at lines 146-149 where api_application_id catches SentryApp.DoesNotExist. This will cause deletion to fail with an unhandled exception when the SentryApp has been deleted first.

Verification

Read sentry_app_installation.py to verify: 1) SentryApp FK relationship at line 72, 2) Existing DoesNotExist handling pattern at lines 146-149 for similar access to self.sentry_app.application_id, 3) SentryApp is a ParanoidModel (soft-delete) confirmed in sentry_app.py line 91. The same exception risk applies to proxy_user_id access.

Identified by Warden sentry-backend-bugs · JDV-DGX

},
)
for cell_name in find_all_cell_names()
Expand All @@ -184,7 +185,10 @@

def handle_async_replication(self, cell_name: str, shard_identifier: int) -> None:
from sentry.hybridcloud.rpc.caching import cell_caching_service
from sentry.sentry_apps.services.app.service import get_installation
from sentry.sentry_apps.services.app.service import (
clear_installation_by_proxy_user_cache,
get_installation,
)

if self.api_token is not None:
# ApiTokens replicate the organization_id they are associated with.
Expand All @@ -193,6 +197,18 @@
ob.save()
cell_caching_service.clear_key(key=get_installation.key_from(self.id), cell_name=cell_name)

# Invalidate the proxy-user installation cache so that permission checks and rate-limit
# key lookups for Sentry Apps don't serve stale data after an install is updated.
try:
proxy_user_id = self.sentry_app.proxy_user_id
if proxy_user_id is not None:
clear_installation_by_proxy_user_cache(
proxy_user_id=proxy_user_id,
organization_id=self.organization_id,
)
except Exception:
pass

@classmethod
def handle_async_deletion(
cls,
Expand All @@ -202,6 +218,7 @@
payload: Mapping[str, Any] | None,
) -> None:
from sentry.models.apitoken import ApiToken
from sentry.sentry_apps.services.app.service import clear_installation_by_proxy_user_cache

if payload:
api_token_id = payload.get("api_token_id", None)
Expand All @@ -211,6 +228,15 @@
for ob in ApiToken(id=api_token_id, user_id=user_id).outboxes_for_update():
ob.save()

# Invalidate the proxy-user installation cache on deletion.
proxy_user_id = payload.get("proxy_user_id")
organization_id = payload.get("organization_id")
if isinstance(proxy_user_id, int) and isinstance(organization_id, int):
clear_installation_by_proxy_user_cache(
proxy_user_id=proxy_user_id,
organization_id=organization_id,
)

def payload_for_update(self) -> dict[str, Any] | None:
from sentry.models.apitoken import ApiToken

Expand Down
67 changes: 67 additions & 0 deletions src/sentry/sentry_apps/services/app/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,4 +266,71 @@ def get_by_application_id(application_id: int) -> RpcSentryApp | None:
return app_service.find_service_hook_sentry_app(api_application_id=application_id)


def get_installation_org_id_by_token_id(token_id: int) -> int | None:
"""
Cached wrapper around app_service.get_installation_org_id_by_token_id.

Avoids a synchronous cross-silo RPC call on every Sentry App request
hitting the rate-limiter by caching the result in the cell silo's Django cache.
"""
from django.core.cache import cache

cache_key = f"sentry-app-install-token:{token_id}"
result = cache.get(cache_key)
if result is not None:
if result == _INSTALLATION_NOT_FOUND_SENTINEL:
return None
return result

organization_id = app_service.get_installation_org_id_by_token_id(token_id=token_id)
# Cache both found and not-found results for 5 minutes.
cache.set(
cache_key,
organization_id if organization_id is not None else _INSTALLATION_NOT_FOUND_SENTINEL,
300,
)
return organization_id


def get_installation_by_proxy_user(
proxy_user_id: int, organization_id: int
) -> "RpcSentryAppInstallation | None":
"""
Cached wrapper around app_service.find_installation_by_proxy_user.

Uses a short-lived Django cache entry to avoid a synchronous cross-silo RPC
call on every Sentry App request during the permission check. The cache is
invalidated whenever the installation changes via handle_async_replication /
handle_async_deletion on SentryAppInstallation.
"""
from django.core.cache import cache

cache_key = f"sentry-app-install-proxy:{proxy_user_id}:{organization_id}"
result = cache.get(cache_key)
if result is not None:
# Sentinel: we cached a "not found" result
if result == _INSTALLATION_NOT_FOUND_SENTINEL:
return None
return result

installation = app_service.find_installation_by_proxy_user(
proxy_user_id=proxy_user_id, organization_id=organization_id
)
# Cache both found and not-found results for 5 minutes to avoid repeated RPCs.
cache.set(cache_key, installation if installation is not None else _INSTALLATION_NOT_FOUND_SENTINEL, 300)
return installation


# Sentinel value used to distinguish a cached "not found" result from a cache miss.
_INSTALLATION_NOT_FOUND_SENTINEL = "NOT_FOUND"


def clear_installation_by_proxy_user_cache(proxy_user_id: int, organization_id: int) -> None:
"""Invalidate the find_installation_by_proxy_user cache entry for the given pair."""
from django.core.cache import cache

cache_key = f"sentry-app-install-proxy:{proxy_user_id}:{organization_id}"
cache.delete(cache_key)


app_service = AppService.create_delegation()
Loading