Skip to content

fix(jira): Add a 'key_id' block list for JIRA installed webhook endpoint #87086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 14, 2025
Merged

fix(jira): Add a 'key_id' block list for JIRA installed webhook endpoint #87086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b92ef6e
add error checking for invalid kid & invalid connect key response
Christinarlong Mar 13, 2025
a49e982
detect invalid kid instead
Christinarlong Mar 14, 2025
4578ef1
switch to absolute paths and fix test
Christinarlong Mar 14, 2025
5cb26f2
typing
Christinarlong Mar 14, 2025
8140f42
revert atlassian connect changes
Christinarlong Mar 14, 2025
395a1d7
revert atlassian connect changes
Christinarlong Mar 14, 2025
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
30 changes: 24 additions & 6 deletions src/sentry/integrations/jira/webhooks/installed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,21 @@
from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import control_silo_endpoint
from sentry.integrations.base import IntegrationDomain
from sentry.integrations.jira.integration import JiraIntegrationProvider
from sentry.integrations.jira.tasks import sync_metadata
from sentry.integrations.jira.webhooks.base import JiraWebhookBase
from sentry.integrations.pipeline import ensure_integration
from sentry.integrations.project_management.metrics import ProjectManagementFailuresReason
from sentry.integrations.utils.atlassian_connect import authenticate_asymmetric_jwt, verify_claims
from sentry.integrations.utils.metrics import (
IntegrationPipelineViewEvent,
IntegrationPipelineViewType,
)
from sentry.utils import jwt

from ...base import IntegrationDomain
from ...project_management.metrics import ProjectManagementFailuresReason
from ...utils.metrics import IntegrationPipelineViewEvent, IntegrationPipelineViewType
from ..integration import JiraIntegrationProvider
from .base import JiraWebhookBase
# Atlassian sends scanner bots to "test" Atlassian apps and they often hit this endpoint with a bad kid causing errors
INVALID_KEY_IDS = ["fake-kid"]


@control_silo_endpoint
Expand All @@ -35,14 +40,27 @@ def post(self, request: Request, *args, **kwargs) -> Response:
provider_key=self.provider,
).capture() as lifecycle:
token = self.get_token(request)

state = request.data
if not state:
lifecycle.record_failure(ProjectManagementFailuresReason.INSTALLATION_STATE_MISSING)
return self.respond(status=status.HTTP_400_BAD_REQUEST)

key_id = jwt.peek_header(token).get("kid")
lifecycle.add_extras(
{
"key_id": key_id,
"base_url": state.get("baseUrl", ""),
"description": state.get("description", ""),
"clientKey": state.get("clientKey", ""),
}
)

if key_id:
if key_id in INVALID_KEY_IDS:
lifecycle.record_halt(halt_reason="JWT contained invalid key_id (kid)")
return self.respond(
{"detail": "Invalid key id"}, status=status.HTTP_400_BAD_REQUEST
)
Comment on lines +59 to +63
Copy link
Member

@cathteng cathteng Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i wonder if instead of this, we have a try/except to verify the JWT. if we encounter a ValueError while decoding AND the key is in invalid ids, we should record halt

the reason being we should still allow the decoding to happen as that is the expected behavior, we just don't want the exception to be counted as a failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was another option I talked with @GabeVillalobos about, but we went this method as it was easiest and the installation webhook module will get rewritten soon(TM) as Jira Forge uses a different authentication method/doesn't need this JWT verification part.

decoded_claims = authenticate_asymmetric_jwt(token, key_id)
verify_claims(decoded_claims, request.path, request.GET, method="POST")

Expand Down
22 changes: 22 additions & 0 deletions tests/sentry/integrations/jira/test_installed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
AtlassianConnectValidationError,
get_query_hash,
)
from sentry.testutils.asserts import assert_count_of_metric, assert_halt_metric
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import control_silo_test
from sentry.utils.http import absolute_uri
Expand Down Expand Up @@ -135,3 +136,24 @@ def test_with_key_id(self, mock_set_tag: MagicMock):

mock_set_tag.assert_any_call("integration_id", integration.id)
assert integration.status == ObjectStatus.ACTIVE

@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_with_invalid_key_id(self, mock_record_event: MagicMock):
self.get_error_response(
**self.body(),
extra_headers=dict(
HTTP_AUTHORIZATION="JWT "
+ self._jwt_token("RS256", RS256_KEY, headers={"kid": "fake-kid"})
),
status_code=status.HTTP_400_BAD_REQUEST,
)

# SLO metric asserts
# ENSURE_CONTROL_SILO (success) -> VERIFY_INSTALLATION (halt) -> GET_CONTROL_RESPONSE (success)
assert_count_of_metric(mock_record_event, EventLifecycleOutcome.STARTED, 3)
assert_count_of_metric(mock_record_event, EventLifecycleOutcome.HALTED, 1)
assert_count_of_metric(mock_record_event, EventLifecycleOutcome.SUCCESS, 2)
assert_halt_metric(
mock_record_event,
"JWT contained invalid key_id (kid)",
)
Loading