Skip to content
Merged
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
11 changes: 3 additions & 8 deletions src/sentry/sentry_apps/api/serializers/app_platform_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
from typing import Any, TypedDict
from uuid import uuid4

from sentry.sentry_apps.models.sentry_app import MASKED_VALUE
from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation
from sentry.sentry_apps.services.app.model import RpcSentryAppInstallation
from sentry.sentry_apps.utils.headers import mask_header_values, parse_custom_headers
from sentry.sentry_apps.utils.webhooks import SentryAppActionType, SentryAppResourceType
from sentry.users.models.user import User
from sentry.users.services.user import RpcUser
Expand Down Expand Up @@ -111,12 +111,7 @@ def sentry_headers(self) -> dict[str, str]:
@property
def custom_headers(self) -> dict[str, str]:
"""User-configured headers parsed from the SentryApp's webhook_headers."""
headers: dict[str, str] = {}
for header in self.install.sentry_app.webhook_headers or []:
name, separator, value = header.partition(":")
if separator:
headers[name.strip()] = value.strip()
return headers
return parse_custom_headers(self.install.sentry_app.webhook_headers)

@property
def headers(self) -> dict[str, str]:
Expand All @@ -132,7 +127,7 @@ def masked_custom_headers(self) -> dict[str, str]:
never persisted to the request buffer. The names are kept so the debug UI
can show which custom headers were sent without leaking the values.
"""
return {name: MASKED_VALUE for name in self.custom_headers}
return mask_header_values(self.custom_headers)

@property
def loggable_headers(self) -> dict[str, str]:
Expand Down
16 changes: 16 additions & 0 deletions src/sentry/sentry_apps/utils/headers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from collections.abc import Mapping, Sequence

from sentry.sentry_apps.models.sentry_app import MASKED_VALUE


def parse_custom_headers(webhook_headers: Sequence[str]) -> dict[str, str]:
headers: dict[str, str] = {}
for header in webhook_headers:
name, separator, value = header.partition(":")
Comment on lines +8 to +9

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.

Bug: The parse_custom_headers function will crash with a TypeError if webhook_headers is None, which can happen for older SentryApp database records.
Severity: HIGH

Suggested Fix

Add a guard to the parse_custom_headers function to handle cases where webhook_headers is None. A simple way to do this is to default to an empty list, for example: for header in webhook_headers or []:. This restores the safe behavior that existed before the refactor.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/sentry/sentry_apps/utils/headers.py#L8-L9

Potential issue: The function `parse_custom_headers` does not handle `None` values for
its `webhook_headers` argument. This can lead to a `TypeError` when the function
attempts to iterate over `None`. The `webhook_headers` field can be `None` for
`SentryApp` records that were created before the database migration that added this
field, as the migration did not backfill existing rows, leaving them as `NULL` in the
database. The original code handled this possibility with `or []`, but this safeguard
was removed during refactoring, introducing a regression.

Also affects:

  • src/sentry/sentry_apps/api/serializers/app_platform_event.py:114~114

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pretty sure the column defaults to [] rather than null

if separator:
headers[name.strip()] = value.strip()
return headers


def mask_header_values(headers: Mapping[str, str]) -> dict[str, str]:
return {name: MASKED_VALUE for name in headers}
Loading