Skip to content

Conversation

oioki
Copy link
Member

@oioki oioki commented Jan 30, 2025

Preparing Bitbucket webhook secret validation. This is actual signature header validation, but no integrations/repos have the associated secret yet.

Follow-up PRs:

Previous attempt (#82541) had repository-level secrets but we decided to go with integration-level secret to align with other integrations (GitLab, GitHub).

@oioki oioki requested review from a team as code owners January 30, 2025 14:38
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 30, 2025
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 96.00000% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/integrations/bitbucket/webhook.py 92.68% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #84309       +/-   ##
===========================================
+ Coverage   46.61%   87.71%   +41.10%     
===========================================
  Files        9563     9599       +36     
  Lines      541874   544269     +2395     
  Branches    21264    21252       -12     
===========================================
+ Hits       252589   477423   +224834     
+ Misses     288933    66494   -222439     
  Partials      352      352               

Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

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

looks good, not sure what the request is being used for in the handler though

@@ -206,6 +262,6 @@ def post(self, request: HttpRequest, organization_id: int) -> HttpResponse:
domain=IntegrationDomain.SOURCE_CODE_MANAGEMENT,
provider_key=event_handler.provider,
).capture():
event_handler(event, organization=organization)
event_handler(event, request=request, organization=organization)
Copy link
Member

Choose a reason for hiding this comment

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

what do we need the request for here? it's not being used in BitbucketWebhook except to check that it's passed

Copy link
Member Author

Choose a reason for hiding this comment

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

request contains X-Hub-Signature header and request body which are used here:

method, signature = request.META["HTTP_X_HUB_SIGNATURE"].split("=", 1)
except (IndexError, KeyError, ValueError):
raise WebhookMissingSignatureException()
if method != "sha256":
raise WebhookUnsupportedSignatureMethodException()
if not is_valid_signature(request.body, secret, signature):

Copy link
Member

Choose a reason for hiding this comment

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

oh wait, can we do this in the webhook itself before calling the handler?

external_id=str(event["repository"]["uuid"]),
)
except Repository.DoesNotExist:
raise Http404()
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are now doing signature check on the upper level (in post() method), and repository is always in the payload (https://support.atlassian.com/bitbucket-cloud/docs/event-payloads/#Repository), we will always lookup the repo on the upper level as well. Hence passing the repo as kwarg and removing this code.

@oioki oioki merged commit ca43d5f into master Feb 13, 2025
49 checks passed
@oioki oioki deleted the feat/bitbucket-signed-webhooks-1 branch February 13, 2025 12:32
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants