Skip to content
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.

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

feat(bitbucket): check bitbucket webhook signature if webhook_secret is defined #82541

Closed
wants to merge 3 commits into from
Closed
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
45 changes: 44 additions & 1 deletion src/sentry/integrations/bitbucket/webhook.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import hashlib
import hmac
import ipaddress
import logging
from abc import ABC
Expand Down Expand Up @@ -31,6 +33,29 @@
PROVIDER_NAME = "integrations:bitbucket"


def is_valid_signature(body: bytes, secret: str, signature: str) -> bool:
hash_object = hmac.new(
secret.encode("utf-8"),
msg=body,
digestmod=hashlib.sha256,
)
expected_signature = hash_object.hexdigest()

if not hmac.compare_digest(expected_signature, signature):
logger.info(
"%s.webhook.invalid-signature",
PROVIDER_NAME,
extra={"expected": expected_signature, "given": signature},
)
return False
return True


class WebhookSignatureException(Exception):
def __init__(self, message: str = ""):
super().__init__(message)


class BitbucketWebhook(SCMWebhook, ABC):
@property
def provider(self) -> str:
Expand Down Expand Up @@ -73,6 +98,8 @@

def __call__(self, event: Mapping[str, Any], **kwargs) -> None:
authors = {}
if not (request := kwargs.get("request")):
raise ValueError("Missing request")

if not (organization := kwargs.get("organization")):
raise ValueError("Missing organization")
Expand All @@ -86,6 +113,19 @@
except Repository.DoesNotExist:
raise Http404()

if "webhook_secret" in repo.config:
Copy link
Member

Choose a reason for hiding this comment

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

why are we storing the webhook secret in each repository? should the webhook secret be for each integration instance, like we have for gitlab?

if not constant_time_compare(secret, integration.metadata["webhook_secret"]):
# Summary and potential workaround mentioned here:
# https://github.com/getsentry/sentry/issues/34903#issuecomment-1262754478
extra["reason"] = GITHUB_WEBHOOK_SECRET_INVALID_ERROR
logger.info("gitlab.webhook.invalid-token-secret", extra=extra)
return HttpResponse(status=409, reason=GITHUB_WEBHOOK_SECRET_INVALID_ERROR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, we create a webhook via corresponding API once the integration is complete, I will look in that direction for Bitbucket.

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can see, all of GitLab, GitHub, and Bitbucket support setting separate webhook secrets on repository level.

It seems like for GitLab, we're creating webhooks on project (=repository) level as well:

def create_project_webhook(self, project_id):
"""Create a webhook on a project
See https://docs.gitlab.com/ee/api/projects.html#add-project-hook
"""
path = GitLabApiClientPath.project_hooks.format(project=project_id)
hook_uri = reverse("sentry-extensions-gitlab-webhook")
model = self.installation.model
data = {
"url": absolute_uri(hook_uri),
"token": "{}:{}".format(model.external_id, model.metadata["webhook_secret"]),

While this change seems misaligned with existing GitLab/GitHub integrations, but it is more secure to have separate secrets for each repo.

Copy link
Member

@cathteng cathteng Jan 30, 2025

Choose a reason for hiding this comment

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

we had a DM conversation but the summary is that other SCM integrations currently have integration-level webhook validation so i think it would be a good idea to get bitbucket up to parity with that first. if we want to do repo-level webhooks we should consider adding it across all SCM integrations at once

secret = repo.config["webhook_secret"]
try:
method, signature = request.META["HTTP_X_HUB_SIGNATURE"].split("=", 1)
except (IndexError, KeyError, ValueError):
raise WebhookSignatureException("Missing webhook signature")

if method != "sha256":
raise WebhookSignatureException("Signature method is not supported")

if not is_valid_signature(request.body, secret, signature):
raise WebhookSignatureException("Webhook signature is invalid")

# while we're here, make sure repo data is up to date
self.update_repo_data(repo, event)

Expand Down Expand Up @@ -206,6 +246,9 @@
domain=IntegrationDomain.SOURCE_CODE_MANAGEMENT,
provider_key=event_handler.provider,
).capture():
event_handler(event, organization=organization)
try:
event_handler(event, request=request, organization=organization)
except WebhookSignatureException as e:
return HttpResponse(str(e), status=400)

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.

Copilot Autofix

AI 3 months ago

To fix the problem, we should avoid returning the exception message directly to the user. Instead, we should log the exception message on the server and return a generic error message to the user. This way, developers can still access the detailed error information in the logs, but external users will not see any sensitive information.

  • Modify the code to log the exception message using the logger and return a generic error message in the HTTP response.
  • Ensure that the logging captures the necessary details for debugging without exposing them to the user.
Suggested changeset 1
src/sentry/integrations/bitbucket/webhook.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/sentry/integrations/bitbucket/webhook.py b/src/sentry/integrations/bitbucket/webhook.py
--- a/src/sentry/integrations/bitbucket/webhook.py
+++ b/src/sentry/integrations/bitbucket/webhook.py
@@ -251,3 +251,8 @@
             except WebhookSignatureException as e:
-                return HttpResponse(str(e), status=400)
+                logger.exception(
+                    "%s.webhook.signature-exception",
+                    PROVIDER_NAME,
+                    extra={"organization_id": organization.id, "error": str(e)},
+                )
+                return HttpResponse("An error occurred while processing the webhook.", status=400)
 
EOF
@@ -251,3 +251,8 @@
except WebhookSignatureException as e:
return HttpResponse(str(e), status=400)
logger.exception(
"%s.webhook.signature-exception",
PROVIDER_NAME,
extra={"organization_id": organization.id, "error": str(e)},
)
return HttpResponse("An error occurred while processing the webhook.", status=400)

Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated

return HttpResponse(status=204)
55 changes: 54 additions & 1 deletion tests/sentry/integrations/bitbucket/test_webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from unittest.mock import patch

from fixtures.bitbucket import PUSH_EVENT_EXAMPLE
from sentry.integrations.bitbucket.webhook import PROVIDER_NAME
from sentry.integrations.bitbucket.webhook import PROVIDER_NAME, is_valid_signature
from sentry.models.commit import Commit
from sentry.models.commitauthor import CommitAuthor
from sentry.models.repository import Repository
Expand Down Expand Up @@ -192,3 +192,56 @@ def test_update_repo_url(self):
# url has been updated
repo_out_of_date_url.refresh_from_db()
assert repo_out_of_date_url.url == "https://bitbucket.org/maxbittker/newsdiffs"


class WebhookSignatureTest(WebhookBaseTest):
method = "post"

def setUp(self):
super().setUp()

repo = self.create_repository()
repo.config["webhook_secret"] = "test_secret"
repo.save()

def send_signed_webhook(self):
return self.get_response(
self.organization_id,
raw_data=PUSH_EVENT_EXAMPLE,
extra_headers=dict(
HTTP_X_EVENT_KEY="repo:push",
HTTP_X_HUB_SIGNATURE=self.signature,
REMOTE_ADDR=BITBUCKET_IP,
),
)

def test_is_valid_signature(self):
# https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/#Examples
assert is_valid_signature(
b"Hello World!",
"It's a Secret to Everybody",
"a4771c39fbe90f317c7824e83ddef3caae9cb3d976c214ace1f2937e133263c9",
)

def test_success(self):
self.signature = "sha256=ee07bac3b2fa849cf4346113dc5f6b9738660673aca6fa8f07ce459e7543f980"
response = self.send_signed_webhook()
assert response.status_code == 204

def test_missing_signature(self):
self.signature = ""
response = self.send_signed_webhook()
assert response.status_code == 400
assert response.content == b"Missing webhook signature"

def test_invalid_signature(self):
self.signature = "sha256=definitely-invalid"
response = self.send_signed_webhook()
assert response.status_code == 400
assert response.content == b"Webhook signature is invalid"

def test_invalid_method(self):
self.signature = "sha1=b842d7b7d535c446133bcf18cf085fb9472175c7"
response = self.send_signed_webhook()
assert response.status_code == 400
assert response.content == b"Signature method is not supported"
Loading