From c2603dc37c5fcc3334d9bec0559a7e9f6330cec2 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 21 Oct 2026 13:06:43 +0200 Subject: [PATCH 01/30] feat(scm): Add a git endpoint view that relays through scm-platform Coding jobs need to clone, and Seer needs to push, without holding a provider token. The view verifies the grant Seer signs, picks the grant's repository that the request path names, and hands the request to scm-platform's GitServer. GitServer relays it to the provider with the integration's token and streams both bodies. The view is behind organizations:scm-git-endpoint and has no route yet, because the URL layout is still being settled; its tests mount it on a stand-in route. It imports scm.git, which no released sentry-scm has yet. GitLab and Cursor Origin are refused until a cell can get their tokens from control. Refs CW-2222 --- src/sentry/conf/server.py | 6 + src/sentry/features/temporary.py | 2 + src/sentry/scm/endpoints/scm_git.py | 185 +++++++++++ tests/sentry/scm/endpoints/test_scm_git.py | 365 +++++++++++++++++++++ 4 files changed, 558 insertions(+) create mode 100644 src/sentry/scm/endpoints/scm_git.py create mode 100644 tests/sentry/scm/endpoints/test_scm_git.py diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 6dbd6c1663f6..70771dbbab30 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -801,6 +801,12 @@ def SOCIAL_AUTH_DEFAULT_USERNAME() -> str: # First element is used to sign requests; request is accepted if signed with any element in the list. SCM_RPC_SHARED_SECRET: list[str] | None = None +# Verifies the grants Seer signs for the SCM git endpoint. Seer signs with the first element; a grant +# is accepted if signed with any element in the list. Only Sentry and Seer may hold these, since a +# holder can mint a git grant for any organization's repositories. Never reuse a secret that signs +# anything else, such as SEER_API_SHARED_SECRET: whatever holds it could then mint grants too. +SCM_GIT_GRANT_SECRETS: list[str] | None = None + # Shared secret used to sign cross-region RPC requests from the launchpad microservice. LAUNCHPAD_RPC_SHARED_SECRET: list[str] | None = None if (val := os.environ.get("LAUNCHPAD_RPC_SHARED_SECRET")) is not None: diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 5a3f05388466..1007d826cf40 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -270,6 +270,8 @@ def register_temporary_features(manager: FeatureManager) -> None: manager.add("organizations:events-endpoint-transactions-discover-blocked", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Enable detecting SDK crashes during event processing manager.add("organizations:sdk-crash-detection", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) + # Relay git fetches and pushes for coding jobs through Sentry's git endpoint, with the org's integration credentials + manager.add("organizations:scm-git-endpoint", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Enable Seer Agent panel for AI-powered data exploration manager.add("organizations:seer-explorer", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable the Seer Automations UI and APIs diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py new file mode 100644 index 000000000000..953891af1313 --- /dev/null +++ b/src/sentry/scm/endpoints/scm_git.py @@ -0,0 +1,185 @@ +import io +from collections.abc import Generator + +from django.conf import settings +from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse +from rest_framework.request import Request +from scm.errors import GitProviderNotSupported, GitRepositoryNotGranted, SCMCodedError +from scm.git.grant import verify_grant +from scm.git.relay import GitRelay +from scm.git.server import GitServer +from scm.git.types import GRANT_HEADER, GitOperation, Grant, Readable +from scm.types import Repository, RepositoryId +from sentry_sdk import traces + +from sentry import features +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.base import Endpoint, internal_cell_silo_endpoint +from sentry.api.negotiation import PassthroughContentNegotiation +from sentry.exceptions import RestrictedIPAddress +from sentry.integrations.services.integration.service import integration_service +from sentry.models.organization import Organization +from sentry.net.http import BlacklistAdapter +from sentry.scm.private.helpers import ( + fetch_repository, + fetch_service_provider, + record_count_metric, +) +from sentry.shared_integrations.exceptions import ApiHostError +from sentry.utils.http import request_body_stream + +FAILED_METRIC = "sentry.scm.git.request.failed" + + +def get_git_token(repository: Repository, operation: GitOperation) -> str | None: + """Return a token that can run git against the repository, minted in control. + + A GitHub installation token carries all of the installation's permissions, so a fetch gets the + same token as a push. + """ + provider_name = repository["provider_name"] + if provider_name in ("github", "github_enterprise"): + integration = integration_service.refresh_github_access_token( + integration_id=repository["integration_id"], + organization_id=repository["organization_id"], + ) + if integration is None: + return None + return integration.metadata.get("access_token") + raise GitProviderNotSupported( + detail=f"Sentry can't relay git for {provider_name} repositories yet." + ) + + +def make_git_server() -> GitServer: + return GitServer( + secrets=settings.SCM_GIT_GRANT_SECRETS or [], + fetch_repository=fetch_repository, + fetch_provider=fetch_service_provider, + get_token=get_git_token, + record_count=record_count_metric, + # A GitHub Enterprise or self-managed GitLab host is whatever the customer configured, so + # it may resolve to one of our own addresses. + relay=GitRelay(adapter=BlacklistAdapter), + ) + + +def find_granted_repository(grant: Grant, repository_path: str) -> RepositoryId: + """Return the grant's entry for the repository the request's path names. + + The path only picks among the grant's repositories, and the entry passed on is the grant's own, + so no path reaches a repository the grant doesn't list. Each entry is looked up among the + organization's connected repositories, so one the organization hasn't connected matches nothing. + """ + for repository_id in grant.repositories: + repository = fetch_repository(grant.organization_id, repository_id) + if repository is None: + continue + host_path = _host_path(repository) + # GitHub and GitLab both treat a repository's path case-insensitively. + if host_path is not None and host_path.casefold() == repository_path.casefold(): + return repository_id + raise GitRepositoryNotGranted(detail="The git grant does not cover this repository.") + + +def _host_path(repository: Repository) -> str | None: + # A GitLab repository's name is its display name ("Group / Project"); its path on the host is + # kept apart. + if repository["provider_name"] == "gitlab": + return repository.get("path") + return repository["name"] + + +@internal_cell_silo_endpoint +class ScmGitEndpoint(Endpoint): + """ + Serves git's smart-HTTP protocol to coding jobs, relaying each request to the repository's + provider with the organization's integration credentials. A grant Seer signs authorizes each + request, so the job never holds a provider credential. + """ + + publish_status = {"GET": ApiPublishStatus.PRIVATE, "POST": ApiPublishStatus.PRIVATE} + owner = ApiOwner.CODING_WORKFLOWS + authentication_classes = () + permission_classes = () + # git's bodies are packs and git asks for its own media types, so nothing is parsed or + # negotiated. + parser_classes = () + content_negotiation_class = PassthroughContentNegotiation + enforce_rate_limit = False + + @traces.trace + def get(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: + return self._relay(request, repository_path, route) + + @traces.trace + def post(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: + return self._relay(request, repository_path, route) + + def _relay(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: + # The grant is verified before anything is looked up, so a request without a valid grant + # never reaches the database. GitServer verifies it again, which costs one HMAC. + try: + grant = verify_grant( + settings.SCM_GIT_GRANT_SECRETS or [], request.headers.get(GRANT_HEADER, "") + ) + except SCMCodedError as e: + return _refuse(e.code, 403, e.detail) + + try: + organization = Organization.objects.get_from_cache(id=grant.organization_id) + except Organization.DoesNotExist: + return _refuse( + "organization_not_found", 404, "The git grant's organization was not found." + ) + if not features.has("organizations:scm-git-endpoint", organization): + return _refuse( + "git_endpoint_disabled", + 404, + "The git endpoint is not enabled for this organization.", + ) + + try: + repository_id = find_granted_repository(grant, repository_path) + except GitRepositoryNotGranted as e: + return _refuse(e.code, 403, e.detail) + + try: + response = make_git_server().handle( + method=request.method or "", + repository_id=repository_id, + route=route, + query=request.META.get("QUERY_STRING", ""), + headers=dict(request.headers), + body=_request_body(request._request), + ) + except RestrictedIPAddress as e: + # A 503, like the ApiHostError Sentry's API clients raise for it, and without the + # address. GitServer has already counted it. + return _plain_text(ApiHostError.from_exception(e).text, 503) + return StreamingHttpResponse( + response.content, status=response.status_code, headers=response.headers + ) + + +def _request_body(request: HttpRequest) -> Readable | None: + body = request_body_stream(request) + if isinstance(body, bytes): + return io.BytesIO(body) + if isinstance(body, Generator): + # Only uwsgi hands over a chunked body as a generator, and Sentry serves with granian. + raise NotImplementedError("The git endpoint can't stream a chunked body under uwsgi.") + return body + + +def _refuse(code: str, status: int, detail: str | None) -> HttpResponse: + # Counted like the refusals GitServer answers itself. + record_count_metric(FAILED_METRIC, 1, {"code": code}) + # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. + return _plain_text(detail or code, status) + + +def _plain_text(message: str, status: int) -> HttpResponse: + # git shows a text/plain body to the user as "remote: ". + return HttpResponse(f"{message}\n", status=status, content_type="text/plain; charset=utf-8") diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py new file mode 100644 index 000000000000..68cab7e344df --- /dev/null +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -0,0 +1,365 @@ +from __future__ import annotations + +import base64 +import time +from collections.abc import Sequence +from unittest import mock + +import pytest +import responses +from django.http import HttpResponseBase +from django.test import override_settings +from django.urls import include, path, re_path +from responses import matchers +from scm.errors import GitProviderNotSupported +from scm.git.grant import sign_grant +from scm.git.types import GRANT_HEADER, GitOperation, Grant +from scm.types import Repository as ScmRepository +from scm.types import RepositoryId + +from sentry.constants import ObjectStatus +from sentry.scm.endpoints.scm_git import ScmGitEndpoint, get_git_token +from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.features import with_feature +from sentry.testutils.silo import cell_silo_test + +SECRET = "a-long-value-that-is-hard-to-guess" +TOKEN = "ghs_installation_token" +FEATURE = "organizations:scm-git-endpoint" +# Far enough ahead that the GitHub client never refreshes the token over the network. +TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" +REFS_URL = "https://github.com/getsentry/sentry.git/info/refs" + + +def sign( + organization_id: int, + operation: GitOperation, + repositories: Sequence[RepositoryId], + *, + expires_in: int = 300, + secret: str = SECRET, +) -> str: + grant = Grant( + organization_id=organization_id, + operation=operation, + repositories=tuple(repositories), + expires_at=int(time.time()) + expires_in, + ) + return sign_grant(secret, grant) + + +# Stands in for the endpoint's route in sentry.api.urls until the URL layout is settled. Sentry's +# own routes stay, since the test fixtures call cross-silo RPCs through them. +urlpatterns = [ + re_path( + r"^git/(?P.+?)\.git/(?P.+)$", + ScmGitEndpoint.as_view(), + name="sentry-api-0-scm-git", + ), + path("", include("sentry.conf.urls")), +] + + +@cell_silo_test +@override_settings(ROOT_URLCONF=__name__, SCM_GIT_GRANT_SECRETS=[SECRET]) +@with_feature(FEATURE) +class ScmGitEndpointTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="github", + external_id="1", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) + self.repo = self.create_repo( + self.project, + name="getsentry/sentry", + provider="integrations:github", + integration_id=self.integration.id, + external_id="12345", + ) + + def fetch_grant(self) -> str: + return sign(self.organization.id, "fetch", [self.repo.id]) + + def git( + self, + method: str, + route: str, + query: str, + grant: str, + repository_path: str = "getsentry/sentry", + body: bytes = b"", + ) -> HttpResponseBase: + return self.client.generic( + method, + f"/git/{repository_path}.git/{route}?{query}", + data=body, + content_type="application/x-git-request", + headers={GRANT_HEADER: grant}, + ) + + def assert_refused(self, response: HttpResponseBase, status: int, message: bytes) -> None: + assert response.status_code == status + assert response["Content-Type"] == "text/plain; charset=utf-8" + assert message in b"".join(response) + assert len(responses.calls) == 0 + + @responses.activate + def test_relays_a_fetch_with_the_installation_token(self) -> None: + responses.add( + responses.GET, + REFS_URL, + match=[matchers.query_string_matcher("service=git-upload-pack")], + body=b"001e# service=git-upload-pack\n0000", + content_type="application/x-git-upload-pack-advertisement", + ) + + response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + + assert response.status_code == 200 + assert response["Content-Type"] == "application/x-git-upload-pack-advertisement" + assert b"".join(response) == b"001e# service=git-upload-pack\n0000" + upstream = responses.calls[0].request + credentials = base64.b64encode(f"x-access-token:{TOKEN}".encode()).decode() + assert upstream.headers["Authorization"] == f"Basic {credentials}" + assert GRANT_HEADER not in upstream.headers + + @responses.activate + def test_relays_a_push_body(self) -> None: + responses.add( + responses.POST, + "https://github.com/getsentry/sentry.git/git-receive-pack", + body=b"000eunpack ok\n0000", + content_type="application/x-git-receive-pack-result", + ) + grant = sign(self.organization.id, "push", [self.repo.id]) + + response = self.git("POST", "git-receive-pack", "", grant, body=b"0000PACK-bytes") + + assert response.status_code == 200 + assert b"".join(response) == b"000eunpack ok\n0000" + assert b"".join(responses.calls[0].request.body) == b"0000PACK-bytes" + + @responses.activate + def test_matches_the_path_case_insensitively(self) -> None: + responses.add(responses.GET, REFS_URL, body=b"0000") + + response = self.git( + "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "GetSentry/Sentry" + ) + + assert response.status_code == 200 + + @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") + def test_refuses_a_request_without_a_grant(self, mock_record_count: mock.MagicMock) -> None: + response = self.git("GET", "info/refs", "service=git-upload-pack", "") + + self.assert_refused(response, 403, b"The git grant is missing.") + mock_record_count.assert_called_once_with( + "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} + ) + + @responses.activate + def test_refuses_a_grant_signed_with_an_unknown_secret(self) -> None: + grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") + + response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + + self.assert_refused(response, 403, b"not signed with a known secret") + + @responses.activate + @override_settings(SCM_GIT_GRANT_SECRETS=None) + def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: + response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + + self.assert_refused(response, 403, b"not signed with a known secret") + + @responses.activate + def test_refuses_an_expired_grant(self) -> None: + grant = sign(self.organization.id, "fetch", [self.repo.id], expires_in=-1) + + response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + + self.assert_refused(response, 403, b"The git grant has expired.") + + @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") + def test_refuses_an_organization_without_the_feature( + self, mock_record_count: mock.MagicMock + ) -> None: + with self.feature({FEATURE: False}): + response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + + self.assert_refused(response, 404, b"not enabled for this organization") + mock_record_count.assert_called_once_with( + "sentry.scm.git.request.failed", 1, {"code": "git_endpoint_disabled"} + ) + + @responses.activate + def test_refuses_a_grant_for_an_organization_that_does_not_exist(self) -> None: + grant = sign(self.organization.id + 1000, "fetch", [self.repo.id]) + + response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + + self.assert_refused(response, 404, b"organization was not found") + + @responses.activate + def test_refuses_a_push_with_a_fetch_grant(self) -> None: + response = self.git("GET", "info/refs", "service=git-receive-pack", self.fetch_grant()) + + self.assert_refused(response, 403, b"The git grant allows fetch, not push.") + + @responses.activate + def test_refuses_a_fetch_with_a_push_grant(self) -> None: + grant = sign(self.organization.id, "push", [self.repo.id]) + + response = self.git("POST", "git-upload-pack", "", grant, body=b"0000") + + self.assert_refused(response, 403, b"The git grant allows push, not fetch.") + + @responses.activate + def test_refuses_a_route_git_does_not_use(self) -> None: + response = self.git("GET", "HEAD", "", self.fetch_grant()) + + self.assert_refused(response, 404, b"GET HEAD is not a git fetch or push route.") + + @responses.activate + def test_refuses_a_connected_repository_outside_the_grant(self) -> None: + self.create_repo( + self.project, + name="getsentry/other", + provider="integrations:github", + integration_id=self.integration.id, + external_id="67890", + ) + + response = self.git( + "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "getsentry/other" + ) + + self.assert_refused(response, 403, b"The git grant does not cover this repository.") + + @responses.activate + def test_refuses_a_path_that_does_not_match_the_granted_repository(self) -> None: + response = self.git( + "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "getsentry/sentry2" + ) + + self.assert_refused(response, 403, b"The git grant does not cover this repository.") + + @responses.activate + def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: + grant = sign(self.organization.id, "fetch", [("github", "99999")]) + + response = self.git( + "GET", "info/refs", "service=git-upload-pack", grant, "getsentry/unconnected" + ) + + self.assert_refused(response, 403, b"The git grant does not cover this repository.") + + @responses.activate + def test_refuses_another_organizations_repository(self) -> None: + other_org = self.create_organization() + other_repo = self.create_repo( + self.create_project(organization=other_org), + name="other-org/private", + provider="integrations:github", + external_id="24680", + ) + grant = sign(self.organization.id, "fetch", [other_repo.id]) + + response = self.git( + "GET", "info/refs", "service=git-upload-pack", grant, "other-org/private" + ) + + self.assert_refused(response, 403, b"The git grant does not cover this repository.") + + @responses.activate + def test_refuses_an_inactive_repository(self) -> None: + self.repo.update(status=ObjectStatus.DISABLED) + + response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + + assert response.status_code == 404 + assert len(responses.calls) == 0 + + def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) -> None: + integration = self.create_integration( + organization=self.organization, + provider="github_enterprise", + external_id="github_enterprise_external_id", + metadata={ + "domain_name": "127.0.0.1/getsentry", + "installation": {"private_key": "private_key", "id": 1, "verify_ssl": True}, + "installation_id": 1234, + "access_token": TOKEN, + "expires_at": TOKEN_EXPIRES_AT, + }, + ) + repo = self.create_repo( + self.project, + name="getsentry/internal", + provider="integrations:github_enterprise", + integration_id=integration.id, + external_id="13579", + ) + grant = sign(self.organization.id, "fetch", [repo.id]) + + response = self.git( + "GET", "info/refs", "service=git-upload-pack", grant, "getsentry/internal" + ) + + assert response.status_code == 503 + assert response["Content-Type"] == "text/plain; charset=utf-8" + assert b"".join(response) == b"Unable to reach host\n" + + +@cell_silo_test +class GetGitTokenTest(TestCase): + def scm_repository(self, provider_name: str, integration_id: int) -> ScmRepository: + return ScmRepository( + external_id="1", + id=1, + integration_id=integration_id, + is_active=True, + name="getsentry/sentry", + organization_id=self.organization.id, + provider_name=provider_name, # type: ignore[typeddict-item] + web_base_url=None, + installation_id=None, + ) + + def test_returns_the_github_installation_token(self) -> None: + integration = self.create_integration( + organization=self.organization, + provider="github", + external_id="1", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) + + token = get_git_token(self.scm_repository("github", integration.id), "fetch") + + assert token == TOKEN + + def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> None: + integration = self.create_integration( + organization=self.create_organization(), + provider="github", + external_id="2", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) + + token = get_git_token(self.scm_repository("github", integration.id), "fetch") + + assert token is None + + def test_refuses_gitlab(self) -> None: + with pytest.raises(GitProviderNotSupported): + get_git_token(self.scm_repository("gitlab", 1), "fetch") + + def test_refuses_cursor_origin(self) -> None: + with pytest.raises(GitProviderNotSupported): + get_git_token(self.scm_repository("cursor_origin", 1), "fetch") From 387e31dfdd343e5984aaa07eee6fddcce867e99e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 21 Oct 2026 22:14:10 +0200 Subject: [PATCH 02/30] feat(scm): Route the git endpoint by repository id The settled layout is /api/0/internal/scm-git/repositories/{repository_id}.git/{route}. The path carries the Sentry Repository.id that the grant must list, so GitServer's own grant check refuses any other repository before the database, the token or the provider. The view no longer matches the path against repository names, which collide between GitHub, GitHub Enterprise and Cursor Origin. scm-git/hosts/ stays free for a later route that names repositories by host. Refs CW-2222 --- src/sentry/api/urls.py | 8 ++ src/sentry/scm/endpoints/scm_git.py | 52 +++-------- tests/sentry/scm/endpoints/test_scm_git.py | 100 +++++++++++---------- 3 files changed, 74 insertions(+), 86 deletions(-) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index fd86c3a2c19a..08ee6dc5b6ea 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -572,6 +572,7 @@ from sentry.replays.endpoints.project_replay_summary import ProjectReplaySummaryEndpoint from sentry.replays.endpoints.project_replay_video_details import ProjectReplayVideoDetailsEndpoint from sentry.replays.endpoints.project_replay_viewed_by import ProjectReplayViewedByEndpoint +from sentry.scm.endpoints.scm_git import ScmGitEndpoint from sentry.scm.endpoints.scm_rpc import ScmRpcServiceEndpoint from sentry.seer.endpoints.admin_agentic_triage_trigger import SeerAdminAgenticTriageTriggerEndpoint from sentry.seer.endpoints.admin_autofix_retry import SeerAdminAutofixRetryEndpoint @@ -3704,6 +3705,13 @@ def create_group_urls( ScmRpcServiceEndpoint.as_view(), name="sentry-api-0-scm-rpc-service", ), + # No trailing slash, because git never sends one. scm-git/hosts/ is kept free for a later route + # that names repositories by their provider host. + re_path( + r"^scm-git/repositories/(?P\d+)\.git/(?P.+)$", + ScmGitEndpoint.as_view(), + name="sentry-api-0-scm-git", + ), re_path( r"^seer-rpc/(?P\w+)/$", SeerRpcServiceEndpoint.as_view(), diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 953891af1313..808d9cae96ee 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -4,12 +4,12 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request -from scm.errors import GitProviderNotSupported, GitRepositoryNotGranted, SCMCodedError +from scm.errors import GitProviderNotSupported, SCMCodedError from scm.git.grant import verify_grant from scm.git.relay import GitRelay from scm.git.server import GitServer -from scm.git.types import GRANT_HEADER, GitOperation, Grant, Readable -from scm.types import Repository, RepositoryId +from scm.git.types import GRANT_HEADER, GitOperation, Readable +from scm.types import Repository from sentry_sdk import traces from sentry import features @@ -65,32 +65,6 @@ def make_git_server() -> GitServer: ) -def find_granted_repository(grant: Grant, repository_path: str) -> RepositoryId: - """Return the grant's entry for the repository the request's path names. - - The path only picks among the grant's repositories, and the entry passed on is the grant's own, - so no path reaches a repository the grant doesn't list. Each entry is looked up among the - organization's connected repositories, so one the organization hasn't connected matches nothing. - """ - for repository_id in grant.repositories: - repository = fetch_repository(grant.organization_id, repository_id) - if repository is None: - continue - host_path = _host_path(repository) - # GitHub and GitLab both treat a repository's path case-insensitively. - if host_path is not None and host_path.casefold() == repository_path.casefold(): - return repository_id - raise GitRepositoryNotGranted(detail="The git grant does not cover this repository.") - - -def _host_path(repository: Repository) -> str | None: - # A GitLab repository's name is its display name ("Group / Project"); its path on the host is - # kept apart. - if repository["provider_name"] == "gitlab": - return repository.get("path") - return repository["name"] - - @internal_cell_silo_endpoint class ScmGitEndpoint(Endpoint): """ @@ -110,16 +84,17 @@ class ScmGitEndpoint(Endpoint): enforce_rate_limit = False @traces.trace - def get(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: - return self._relay(request, repository_path, route) + def get(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: + return self._relay(request, int(repository_id), route) @traces.trace - def post(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: - return self._relay(request, repository_path, route) + def post(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: + return self._relay(request, int(repository_id), route) - def _relay(self, request: Request, repository_path: str, route: str) -> HttpResponseBase: - # The grant is verified before anything is looked up, so a request without a valid grant - # never reaches the database. GitServer verifies it again, which costs one HMAC. + def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: + # The grant names the organization whose flag gates the endpoint, and it's verified before + # the organization is looked up, so a request without a valid grant never reaches the + # database. GitServer verifies it again, which costs one HMAC. try: grant = verify_grant( settings.SCM_GIT_GRANT_SECRETS or [], request.headers.get(GRANT_HEADER, "") @@ -140,11 +115,6 @@ def _relay(self, request: Request, repository_path: str, route: str) -> HttpResp "The git endpoint is not enabled for this organization.", ) - try: - repository_id = find_granted_repository(grant, repository_path) - except GitRepositoryNotGranted as e: - return _refuse(e.code, 403, e.detail) - try: response = make_git_server().handle( method=request.method or "", diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 68cab7e344df..11d6b98863c1 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -9,7 +9,7 @@ import responses from django.http import HttpResponseBase from django.test import override_settings -from django.urls import include, path, re_path +from django.urls import reverse from responses import matchers from scm.errors import GitProviderNotSupported from scm.git.grant import sign_grant @@ -18,7 +18,7 @@ from scm.types import RepositoryId from sentry.constants import ObjectStatus -from sentry.scm.endpoints.scm_git import ScmGitEndpoint, get_git_token +from sentry.scm.endpoints.scm_git import get_git_token from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -48,20 +48,8 @@ def sign( return sign_grant(secret, grant) -# Stands in for the endpoint's route in sentry.api.urls until the URL layout is settled. Sentry's -# own routes stay, since the test fixtures call cross-silo RPCs through them. -urlpatterns = [ - re_path( - r"^git/(?P.+?)\.git/(?P.+)$", - ScmGitEndpoint.as_view(), - name="sentry-api-0-scm-git", - ), - path("", include("sentry.conf.urls")), -] - - @cell_silo_test -@override_settings(ROOT_URLCONF=__name__, SCM_GIT_GRANT_SECRETS=[SECRET]) +@override_settings(SCM_GIT_GRANT_SECRETS=[SECRET]) @with_feature(FEATURE) class ScmGitEndpointTest(TestCase): def setUp(self) -> None: @@ -89,12 +77,19 @@ def git( route: str, query: str, grant: str, - repository_path: str = "getsentry/sentry", + repository_id: int | None = None, body: bytes = b"", ) -> HttpResponseBase: + url = reverse( + "sentry-api-0-scm-git", + kwargs={"repository_id": repository_id or self.repo.id, "route": route}, + ) + return self.send(method, f"{url}?{query}", grant, body) + + def send(self, method: str, url: str, grant: str, body: bytes = b"") -> HttpResponseBase: return self.client.generic( method, - f"/git/{repository_path}.git/{route}?{query}", + url, data=body, content_type="application/x-git-request", headers={GRANT_HEADER: grant}, @@ -142,16 +137,6 @@ def test_relays_a_push_body(self) -> None: assert b"".join(response) == b"000eunpack ok\n0000" assert b"".join(responses.calls[0].request.body) == b"0000PACK-bytes" - @responses.activate - def test_matches_the_path_case_insensitively(self) -> None: - responses.add(responses.GET, REFS_URL, body=b"0000") - - response = self.git( - "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "GetSentry/Sentry" - ) - - assert response.status_code == 200 - @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") def test_refuses_a_request_without_a_grant(self, mock_record_count: mock.MagicMock) -> None: @@ -227,8 +212,8 @@ def test_refuses_a_route_git_does_not_use(self) -> None: self.assert_refused(response, 404, b"GET HEAD is not a git fetch or push route.") @responses.activate - def test_refuses_a_connected_repository_outside_the_grant(self) -> None: - self.create_repo( + def test_refuses_a_repository_outside_the_grant(self) -> None: + other_repo = self.create_repo( self.project, name="getsentry/other", provider="integrations:github", @@ -237,28 +222,27 @@ def test_refuses_a_connected_repository_outside_the_grant(self) -> None: ) response = self.git( - "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "getsentry/other" + "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), other_repo.id ) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @responses.activate - def test_refuses_a_path_that_does_not_match_the_granted_repository(self) -> None: - response = self.git( - "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), "getsentry/sentry2" - ) + def test_refuses_a_grant_that_names_the_repository_by_external_id(self) -> None: + grant = sign(self.organization.id, "fetch", [("github", "12345")]) + + response = self.git("GET", "info/refs", "service=git-upload-pack", grant) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @responses.activate def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: - grant = sign(self.organization.id, "fetch", [("github", "99999")]) + unconnected_id = self.repo.id + 1000 + grant = sign(self.organization.id, "fetch", [unconnected_id]) - response = self.git( - "GET", "info/refs", "service=git-upload-pack", grant, "getsentry/unconnected" - ) + response = self.git("GET", "info/refs", "service=git-upload-pack", grant, unconnected_id) - self.assert_refused(response, 403, b"The git grant does not cover this repository.") + self.assert_refused(response, 404, b"A repository could not be found.") @responses.activate def test_refuses_another_organizations_repository(self) -> None: @@ -271,11 +255,39 @@ def test_refuses_another_organizations_repository(self) -> None: ) grant = sign(self.organization.id, "fetch", [other_repo.id]) - response = self.git( - "GET", "info/refs", "service=git-upload-pack", grant, "other-org/private" + response = self.git("GET", "info/refs", "service=git-upload-pack", grant, other_repo.id) + + self.assert_refused(response, 404, b"A repository could not be found.") + + @responses.activate + def test_does_not_route_a_repository_id_that_is_not_a_number(self) -> None: + response = self.send( + "GET", + "/api/0/internal/scm-git/repositories/getsentry/sentry.git/info/refs" + "?service=git-upload-pack", + self.fetch_grant(), ) - self.assert_refused(response, 403, b"The git grant does not cover this repository.") + assert response.status_code == 404 + assert len(responses.calls) == 0 + + @responses.activate + def test_does_not_route_a_path_without_the_git_suffix(self) -> None: + response = self.send( + "GET", + f"/api/0/internal/scm-git/repositories/{self.repo.id}/info/refs" + "?service=git-upload-pack", + self.fetch_grant(), + ) + + assert response.status_code == 404 + assert len(responses.calls) == 0 + + @responses.activate + def test_refuses_a_route_with_a_trailing_slash(self) -> None: + response = self.git("GET", "info/refs/", "service=git-upload-pack", self.fetch_grant()) + + self.assert_refused(response, 404, b"is not a git fetch or push route.") @responses.activate def test_refuses_an_inactive_repository(self) -> None: @@ -308,9 +320,7 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) ) grant = sign(self.organization.id, "fetch", [repo.id]) - response = self.git( - "GET", "info/refs", "service=git-upload-pack", grant, "getsentry/internal" - ) + response = self.git("GET", "info/refs", "service=git-upload-pack", grant, repo.id) assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" From a5b8cae85a7503d001f2d4ecb94bb1bfcf8ae4dd Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 21 Oct 2026 22:15:19 +0200 Subject: [PATCH 03/30] test(scm): Cover a git endpoint URL that ends at the repository Refs CW-2222 --- tests/sentry/scm/endpoints/test_scm_git.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 11d6b98863c1..526aabb8e1f6 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -283,6 +283,15 @@ def test_does_not_route_a_path_without_the_git_suffix(self) -> None: assert response.status_code == 404 assert len(responses.calls) == 0 + @responses.activate + def test_does_not_route_a_path_that_ends_at_the_repository(self) -> None: + response = self.send( + "GET", f"/api/0/internal/scm-git/repositories/{self.repo.id}.git/", self.fetch_grant() + ) + + assert response.status_code == 404 + assert len(responses.calls) == 0 + @responses.activate def test_refuses_a_route_with_a_trailing_slash(self) -> None: response = self.git("GET", "info/refs/", "service=git-upload-pack", self.fetch_grant()) From 42ea6082448f0c24deb20013e953ec86c1aad7be Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 21 Oct 2026 22:40:05 +0200 Subject: [PATCH 04/30] fix(scm): Keep git grants out of telemetry and answer token failures Review fixes for the git endpoint: - Add "grant" to the SDK's request header denylist. None of its terms matched X-Scm-Git-Grant, so the grant reached spans and error events. - Answer a GitHub token that can't be minted (e.g. a suspended app) with GitServer's text/plain 502 rather than a JSON 500, which git shows only as "HTTP 500". - Limit the route's repository id to 19 ASCII digits. \d also matches other scripts' digits, and int() refuses an id of 4,301 digits with a 500. - Test org scoping with a repository that has a working integration, and test unmatched URLs at the resolver, where they reach the API catch-all. Refs CW-2222 --- src/sentry/api/urls.py | 4 +- src/sentry/conf/server.py | 4 +- src/sentry/scm/endpoints/scm_git.py | 21 ++++-- tests/sentry/scm/endpoints/test_scm_git.py | 86 ++++++++++++++-------- 4 files changed, 78 insertions(+), 37 deletions(-) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 08ee6dc5b6ea..77b3674c97f0 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -3708,7 +3708,9 @@ def create_group_urls( # No trailing slash, because git never sends one. scm-git/hosts/ is kept free for a later route # that names repositories by their provider host. re_path( - r"^scm-git/repositories/(?P\d+)\.git/(?P.+)$", + # [0-9] rather than \d, which also matches other scripts' digits. 19 digits hold any + # Repository.id, and keep int() from refusing a longer id with a 500. + r"^scm-git/repositories/(?P[0-9]{1,19})\.git/(?P.+)$", ScmGitEndpoint.as_view(), name="sentry-api-0-scm-git", ), diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 70771dbbab30..5eda986c0582 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -2578,7 +2578,9 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: "http_headers": { "request": { "mode": "denylist", - "terms": ["forwarded", "-ip", "remote-", "via", "-user"], + # "grant" keeps the SCM git endpoint's X-Scm-Git-Grant out of spans and events. + # It's a credential, but none of the SDK's own terms match it. + "terms": ["forwarded", "-ip", "remote-", "via", "-user", "grant"], } }, "cookies": {"mode": "off"}, diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 808d9cae96ee..69298fcee026 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -1,6 +1,7 @@ import io from collections.abc import Generator +import sentry_sdk from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request @@ -18,6 +19,7 @@ from sentry.api.base import Endpoint, internal_cell_silo_endpoint from sentry.api.negotiation import PassthroughContentNegotiation from sentry.exceptions import RestrictedIPAddress +from sentry.hybridcloud.rpc.service import RpcRemoteException from sentry.integrations.services.integration.service import integration_service from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter @@ -26,7 +28,7 @@ fetch_service_provider, record_count_metric, ) -from sentry.shared_integrations.exceptions import ApiHostError +from sentry.shared_integrations.exceptions import ApiError, ApiHostError from sentry.utils.http import request_body_stream FAILED_METRIC = "sentry.scm.git.request.failed" @@ -40,10 +42,16 @@ def get_git_token(repository: Repository, operation: GitOperation) -> str | None """ provider_name = repository["provider_name"] if provider_name in ("github", "github_enterprise"): - integration = integration_service.refresh_github_access_token( - integration_id=repository["integration_id"], - organization_id=repository["organization_id"], - ) + try: + integration = integration_service.refresh_github_access_token( + integration_id=repository["integration_id"], + organization_id=repository["organization_id"], + ) + except (ApiError, RpcRemoteException): + # GitHub refused to mint a token, e.g. for a suspended app. Answered as a 500, git + # would show only "HTTP 500"; GitServer turns None into a 502 that says why. + sentry_sdk.capture_exception() + return None if integration is None: return None return integration.metadata.get("access_token") @@ -134,6 +142,9 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon def _request_body(request: HttpRequest) -> Readable | None: + # Endpoint.dispatch has already read a body sent with a Content-Length into memory, and nginx + # sends every body that way in production, so a push is held whole up to nginx's size cap. + # Only a chunked body, as on the devserver, is still read as it streams. body = request_body_stream(request) if isinstance(body, bytes): return io.BytesIO(body) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 526aabb8e1f6..6e5940456e83 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -7,18 +7,21 @@ import pytest import responses +from django.conf import settings from django.http import HttpResponseBase from django.test import override_settings -from django.urls import reverse +from django.urls import resolve, reverse from responses import matchers from scm.errors import GitProviderNotSupported from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import Repository as ScmRepository from scm.types import RepositoryId +from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus from sentry.scm.endpoints.scm_git import get_git_token +from sentry.shared_integrations.exceptions import ApiError from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -247,10 +250,17 @@ def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: @responses.activate def test_refuses_another_organizations_repository(self) -> None: other_org = self.create_organization() + other_integration = self.create_integration( + organization=other_org, + provider="github", + external_id="2", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) other_repo = self.create_repo( self.create_project(organization=other_org), name="other-org/private", provider="integrations:github", + integration_id=other_integration.id, external_id="24680", ) grant = sign(self.organization.id, "fetch", [other_repo.id]) @@ -260,37 +270,20 @@ def test_refuses_another_organizations_repository(self) -> None: self.assert_refused(response, 404, b"A repository could not be found.") @responses.activate - def test_does_not_route_a_repository_id_that_is_not_a_number(self) -> None: - response = self.send( - "GET", - "/api/0/internal/scm-git/repositories/getsentry/sentry.git/info/refs" - "?service=git-upload-pack", - self.fetch_grant(), - ) - - assert response.status_code == 404 - assert len(responses.calls) == 0 - - @responses.activate - def test_does_not_route_a_path_without_the_git_suffix(self) -> None: - response = self.send( - "GET", - f"/api/0/internal/scm-git/repositories/{self.repo.id}/info/refs" - "?service=git-upload-pack", - self.fetch_grant(), - ) - - assert response.status_code == 404 - assert len(responses.calls) == 0 + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk.capture_exception") + @mock.patch( + "sentry.scm.endpoints.scm_git.integration_service.refresh_github_access_token", + side_effect=ApiError("Forbidden", code=403), + ) + def test_answers_a_token_github_refuses_to_mint_with_a_bad_gateway( + self, mock_refresh: mock.MagicMock, mock_capture_exception: mock.MagicMock + ) -> None: + response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) - @responses.activate - def test_does_not_route_a_path_that_ends_at_the_repository(self) -> None: - response = self.send( - "GET", f"/api/0/internal/scm-git/repositories/{self.repo.id}.git/", self.fetch_grant() + self.assert_refused( + response, 502, b"Sentry could not get a github token for this repository." ) - - assert response.status_code == 404 - assert len(responses.calls) == 0 + mock_capture_exception.assert_called_once_with() @responses.activate def test_refuses_a_route_with_a_trailing_slash(self) -> None: @@ -336,6 +329,39 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) assert b"".join(response) == b"Unable to reach host\n" +@pytest.mark.parametrize( + "path", + [ + "/api/0/internal/scm-git/repositories/getsentry/sentry.git/info/refs", + "/api/0/internal/scm-git/repositories/42/info/refs", + "/api/0/internal/scm-git/repositories/42.git/", + "/api/0/internal/scm-git/repositories/42.git", + # More digits than any Repository.id, which int() would refuse with a 500 at 4,301. + f"/api/0/internal/scm-git/repositories/{'4' * 20}.git/info/refs", + # Arabic-Indic digits, which int() would read as 42. + "/api/0/internal/scm-git/repositories/\u0664\u0662.git/info/refs", + ], + ids=[ + "name", + "no-git-suffix", + "nothing-after-git", + "no-slash-after-git", + "20-digits", + "arabic-indic", + ], +) +def test_does_not_route(path: str) -> None: + assert resolve(path).url_name == "sentry-api-catchall" + + +def test_the_sdk_filters_the_grant_header_out_of_telemetry() -> None: + behaviour = settings.SENTRY_SDK_CONFIG["data_collection"]["http_headers"]["request"] + + filtered = _apply_key_value_collection_filtering({GRANT_HEADER: "a.signed.grant"}, behaviour) + + assert filtered == {GRANT_HEADER: "[Filtered]"} + + @cell_silo_test class GetGitTokenTest(TestCase): def scm_repository(self, provider_name: str, integration_id: int) -> ScmRepository: From 50f7a899355a705f16405de7c83365f115dc47ad Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 21 Oct 2026 22:46:56 +0200 Subject: [PATCH 05/30] ref(scm): Simplify the git endpoint's refusals and tests SCM errors always carry a detail, so the refusal helper no longer falls back to the error code. The tests share a helper for the info/refs request that opens every fetch, and the inactive repository test now checks the refusal like its siblings. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 4 +- tests/sentry/scm/endpoints/test_scm_git.py | 44 ++++++++++------------ 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 69298fcee026..51db9cf8c86c 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -154,11 +154,11 @@ def _request_body(request: HttpRequest) -> Readable | None: return body -def _refuse(code: str, status: int, detail: str | None) -> HttpResponse: +def _refuse(code: str, status: int, detail: str) -> HttpResponse: # Counted like the refusals GitServer answers itself. record_count_metric(FAILED_METRIC, 1, {"code": code}) # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. - return _plain_text(detail or code, status) + return _plain_text(detail, status) def _plain_text(message: str, status: int) -> HttpResponse: diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 6e5940456e83..06862f08db62 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -31,7 +31,6 @@ FEATURE = "organizations:scm-git-endpoint" # Far enough ahead that the GitHub client never refreshes the token over the network. TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" -REFS_URL = "https://github.com/getsentry/sentry.git/info/refs" def sign( @@ -87,17 +86,17 @@ def git( "sentry-api-0-scm-git", kwargs={"repository_id": repository_id or self.repo.id, "route": route}, ) - return self.send(method, f"{url}?{query}", grant, body) - - def send(self, method: str, url: str, grant: str, body: bytes = b"") -> HttpResponseBase: return self.client.generic( method, - url, + f"{url}?{query}", data=body, content_type="application/x-git-request", headers={GRANT_HEADER: grant}, ) + def fetch_refs(self, grant: str, repository_id: int | None = None) -> HttpResponseBase: + return self.git("GET", "info/refs", "service=git-upload-pack", grant, repository_id) + def assert_refused(self, response: HttpResponseBase, status: int, message: bytes) -> None: assert response.status_code == status assert response["Content-Type"] == "text/plain; charset=utf-8" @@ -108,13 +107,13 @@ def assert_refused(self, response: HttpResponseBase, status: int, message: bytes def test_relays_a_fetch_with_the_installation_token(self) -> None: responses.add( responses.GET, - REFS_URL, + "https://github.com/getsentry/sentry.git/info/refs", match=[matchers.query_string_matcher("service=git-upload-pack")], body=b"001e# service=git-upload-pack\n0000", content_type="application/x-git-upload-pack-advertisement", ) - response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + response = self.fetch_refs(self.fetch_grant()) assert response.status_code == 200 assert response["Content-Type"] == "application/x-git-upload-pack-advertisement" @@ -143,7 +142,7 @@ def test_relays_a_push_body(self) -> None: @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") def test_refuses_a_request_without_a_grant(self, mock_record_count: mock.MagicMock) -> None: - response = self.git("GET", "info/refs", "service=git-upload-pack", "") + response = self.fetch_refs("") self.assert_refused(response, 403, b"The git grant is missing.") mock_record_count.assert_called_once_with( @@ -154,14 +153,14 @@ def test_refuses_a_request_without_a_grant(self, mock_record_count: mock.MagicMo def test_refuses_a_grant_signed_with_an_unknown_secret(self) -> None: grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") - response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + response = self.fetch_refs(grant) self.assert_refused(response, 403, b"not signed with a known secret") @responses.activate @override_settings(SCM_GIT_GRANT_SECRETS=None) def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: - response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + response = self.fetch_refs(self.fetch_grant()) self.assert_refused(response, 403, b"not signed with a known secret") @@ -169,7 +168,7 @@ def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: def test_refuses_an_expired_grant(self) -> None: grant = sign(self.organization.id, "fetch", [self.repo.id], expires_in=-1) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + response = self.fetch_refs(grant) self.assert_refused(response, 403, b"The git grant has expired.") @@ -179,7 +178,7 @@ def test_refuses_an_organization_without_the_feature( self, mock_record_count: mock.MagicMock ) -> None: with self.feature({FEATURE: False}): - response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + response = self.fetch_refs(self.fetch_grant()) self.assert_refused(response, 404, b"not enabled for this organization") mock_record_count.assert_called_once_with( @@ -190,7 +189,7 @@ def test_refuses_an_organization_without_the_feature( def test_refuses_a_grant_for_an_organization_that_does_not_exist(self) -> None: grant = sign(self.organization.id + 1000, "fetch", [self.repo.id]) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + response = self.fetch_refs(grant) self.assert_refused(response, 404, b"organization was not found") @@ -224,9 +223,7 @@ def test_refuses_a_repository_outside_the_grant(self) -> None: external_id="67890", ) - response = self.git( - "GET", "info/refs", "service=git-upload-pack", self.fetch_grant(), other_repo.id - ) + response = self.fetch_refs(self.fetch_grant(), other_repo.id) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @@ -234,7 +231,7 @@ def test_refuses_a_repository_outside_the_grant(self) -> None: def test_refuses_a_grant_that_names_the_repository_by_external_id(self) -> None: grant = sign(self.organization.id, "fetch", [("github", "12345")]) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant) + response = self.fetch_refs(grant) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @@ -243,7 +240,7 @@ def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: unconnected_id = self.repo.id + 1000 grant = sign(self.organization.id, "fetch", [unconnected_id]) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant, unconnected_id) + response = self.fetch_refs(grant, unconnected_id) self.assert_refused(response, 404, b"A repository could not be found.") @@ -265,7 +262,7 @@ def test_refuses_another_organizations_repository(self) -> None: ) grant = sign(self.organization.id, "fetch", [other_repo.id]) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant, other_repo.id) + response = self.fetch_refs(grant, other_repo.id) self.assert_refused(response, 404, b"A repository could not be found.") @@ -278,7 +275,7 @@ def test_refuses_another_organizations_repository(self) -> None: def test_answers_a_token_github_refuses_to_mint_with_a_bad_gateway( self, mock_refresh: mock.MagicMock, mock_capture_exception: mock.MagicMock ) -> None: - response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + response = self.fetch_refs(self.fetch_grant()) self.assert_refused( response, 502, b"Sentry could not get a github token for this repository." @@ -295,10 +292,9 @@ def test_refuses_a_route_with_a_trailing_slash(self) -> None: def test_refuses_an_inactive_repository(self) -> None: self.repo.update(status=ObjectStatus.DISABLED) - response = self.git("GET", "info/refs", "service=git-upload-pack", self.fetch_grant()) + response = self.fetch_refs(self.fetch_grant()) - assert response.status_code == 404 - assert len(responses.calls) == 0 + self.assert_refused(response, 404, b"A repository was found but it is inactive.") def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) -> None: integration = self.create_integration( @@ -322,7 +318,7 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) ) grant = sign(self.organization.id, "fetch", [repo.id]) - response = self.git("GET", "info/refs", "service=git-upload-pack", grant, repo.id) + response = self.fetch_refs(grant, repo.id) assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" From 2104f8b9e8c760425bde7360ef6bb12b8dca185d Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 08:24:22 +0200 Subject: [PATCH 06/30] ref(scm): Look up git tokens beside the provider factory Which token a repository's provider takes is per-provider knowledge, like which adapter to build, so the lookup moves from the git endpoint into sentry.scm.private.helpers next to fetch_service_provider. The GitLab and Cursor Origin branches then land in the same place as their adapters. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 39 ++---------- src/sentry/scm/private/helpers.py | 32 +++++++++- tests/sentry/scm/endpoints/test_scm_git.py | 55 +--------------- .../integration/test_helpers_integration.py | 62 ++++++++++++++++++- 4 files changed, 98 insertions(+), 90 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 51db9cf8c86c..bd2175021f72 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -1,16 +1,14 @@ import io from collections.abc import Generator -import sentry_sdk from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request -from scm.errors import GitProviderNotSupported, SCMCodedError +from scm.errors import SCMCodedError from scm.git.grant import verify_grant from scm.git.relay import GitRelay from scm.git.server import GitServer -from scm.git.types import GRANT_HEADER, GitOperation, Readable -from scm.types import Repository +from scm.git.types import GRANT_HEADER, Readable from sentry_sdk import traces from sentry import features @@ -19,53 +17,26 @@ from sentry.api.base import Endpoint, internal_cell_silo_endpoint from sentry.api.negotiation import PassthroughContentNegotiation from sentry.exceptions import RestrictedIPAddress -from sentry.hybridcloud.rpc.service import RpcRemoteException -from sentry.integrations.services.integration.service import integration_service from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter from sentry.scm.private.helpers import ( + fetch_access_token, fetch_repository, fetch_service_provider, record_count_metric, ) -from sentry.shared_integrations.exceptions import ApiError, ApiHostError +from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream FAILED_METRIC = "sentry.scm.git.request.failed" -def get_git_token(repository: Repository, operation: GitOperation) -> str | None: - """Return a token that can run git against the repository, minted in control. - - A GitHub installation token carries all of the installation's permissions, so a fetch gets the - same token as a push. - """ - provider_name = repository["provider_name"] - if provider_name in ("github", "github_enterprise"): - try: - integration = integration_service.refresh_github_access_token( - integration_id=repository["integration_id"], - organization_id=repository["organization_id"], - ) - except (ApiError, RpcRemoteException): - # GitHub refused to mint a token, e.g. for a suspended app. Answered as a 500, git - # would show only "HTTP 500"; GitServer turns None into a 502 that says why. - sentry_sdk.capture_exception() - return None - if integration is None: - return None - return integration.metadata.get("access_token") - raise GitProviderNotSupported( - detail=f"Sentry can't relay git for {provider_name} repositories yet." - ) - - def make_git_server() -> GitServer: return GitServer( secrets=settings.SCM_GIT_GRANT_SECRETS or [], fetch_repository=fetch_repository, fetch_provider=fetch_service_provider, - get_token=get_git_token, + get_token=fetch_access_token, record_count=record_count_metric, # A GitHub Enterprise or self-managed GitLab host is whatever the customer configured, so # it may resolve to one of our own addresses. diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index fbf0883ae04b..d166fa29978e 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -4,20 +4,22 @@ import sentry_sdk from django.db.models import Q -from scm.errors import SCMCodedError, UnhandledException +from scm.errors import GitProviderNotSupported, SCMCodedError, UnhandledException +from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider from scm.providers.gitlab.provider import GitLabProvider from scm.types import Provider, Repository, RepositoryId from sentry.constants import ObjectStatus +from sentry.hybridcloud.rpc.service import RpcRemoteException from sentry.integrations.errors import OrganizationIntegrationNotFound from sentry.integrations.services.integration.model import RpcIntegration from sentry.integrations.services.integration.service import integration_service from sentry.models.organization import Organization from sentry.models.repository import Repository as RepositoryModel from sentry.shared_integrations.client.proxy import IntegrationProxyClient -from sentry.shared_integrations.exceptions import IntegrationError +from sentry.shared_integrations.exceptions import ApiError, IntegrationError from sentry.utils import metrics type IntegrationGetter = Callable[[int, int], RpcIntegration | None] @@ -84,6 +86,32 @@ def fetch_service_provider( return None +def fetch_access_token(repository: Repository, operation: GitOperation) -> str | None: + """Return a token that can run git against the repository, minted in control. + + A GitHub installation token carries all of the installation's permissions, so a fetch gets the + same token as a push. + """ + provider_name = repository["provider_name"] + if provider_name in ("github", "github_enterprise"): + try: + integration = integration_service.refresh_github_access_token( + integration_id=repository["integration_id"], + organization_id=repository["organization_id"], + ) + except (ApiError, RpcRemoteException): + # GitHub refused to mint a token, e.g. for a suspended app. Answered as a 500, git + # would show only "HTTP 500"; GitServer turns None into a 502 that says why. + sentry_sdk.capture_exception() + return None + if integration is None: + return None + return integration.metadata.get("access_token") + raise GitProviderNotSupported( + detail=f"Sentry can't relay git for {provider_name} repositories yet." + ) + + def fetch_repository( organization_id: int, repository_id: RepositoryId, diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 06862f08db62..84169aba5a4f 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -12,15 +12,12 @@ from django.test import override_settings from django.urls import resolve, reverse from responses import matchers -from scm.errors import GitProviderNotSupported from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant -from scm.types import Repository as ScmRepository from scm.types import RepositoryId from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus -from sentry.scm.endpoints.scm_git import get_git_token from sentry.shared_integrations.exceptions import ApiError from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature @@ -267,9 +264,9 @@ def test_refuses_another_organizations_repository(self) -> None: self.assert_refused(response, 404, b"A repository could not be found.") @responses.activate - @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk.capture_exception") + @mock.patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") @mock.patch( - "sentry.scm.endpoints.scm_git.integration_service.refresh_github_access_token", + "sentry.scm.private.helpers.integration_service.refresh_github_access_token", side_effect=ApiError("Forbidden", code=403), ) def test_answers_a_token_github_refuses_to_mint_with_a_bad_gateway( @@ -356,51 +353,3 @@ def test_the_sdk_filters_the_grant_header_out_of_telemetry() -> None: filtered = _apply_key_value_collection_filtering({GRANT_HEADER: "a.signed.grant"}, behaviour) assert filtered == {GRANT_HEADER: "[Filtered]"} - - -@cell_silo_test -class GetGitTokenTest(TestCase): - def scm_repository(self, provider_name: str, integration_id: int) -> ScmRepository: - return ScmRepository( - external_id="1", - id=1, - integration_id=integration_id, - is_active=True, - name="getsentry/sentry", - organization_id=self.organization.id, - provider_name=provider_name, # type: ignore[typeddict-item] - web_base_url=None, - installation_id=None, - ) - - def test_returns_the_github_installation_token(self) -> None: - integration = self.create_integration( - organization=self.organization, - provider="github", - external_id="1", - metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, - ) - - token = get_git_token(self.scm_repository("github", integration.id), "fetch") - - assert token == TOKEN - - def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> None: - integration = self.create_integration( - organization=self.create_organization(), - provider="github", - external_id="2", - metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, - ) - - token = get_git_token(self.scm_repository("github", integration.id), "fetch") - - assert token is None - - def test_refuses_gitlab(self) -> None: - with pytest.raises(GitProviderNotSupported): - get_git_token(self.scm_repository("gitlab", 1), "fetch") - - def test_refuses_cursor_origin(self) -> None: - with pytest.raises(GitProviderNotSupported): - get_git_token(self.scm_repository("cursor_origin", 1), "fetch") diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 7e18e6c98c64..41a84a24c967 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -1,5 +1,7 @@ from unittest.mock import patch +import pytest +from scm.errors import GitProviderNotSupported from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider from scm.types import Repository @@ -7,8 +9,18 @@ from sentry.constants import ObjectStatus from sentry.integrations.services.integration import integration_service from sentry.models.repository import Repository as RepositoryModel -from sentry.scm.private.helpers import fetch_repository, fetch_service_provider, make_fetchers +from sentry.scm.private.helpers import ( + fetch_access_token, + fetch_repository, + fetch_service_provider, + make_fetchers, +) from sentry.testutils.cases import TestCase +from sentry.testutils.silo import cell_silo_test + +TOKEN = "ghs_installation_token" +# Far enough ahead that the GitHub client never refreshes the token over the network. +TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" class TestFetchRepository(TestCase): @@ -455,3 +467,51 @@ def test_fetchers_fetch_the_integration_once(self) -> None: assert isinstance(provider, GitHubProvider) assert service.get_integration.call_count == 1 + + +@cell_silo_test +class TestFetchAccessToken(TestCase): + def scm_repository(self, provider_name: str, integration_id: int) -> Repository: + return Repository( + external_id="1", + id=1, + integration_id=integration_id, + is_active=True, + name="getsentry/sentry", + organization_id=self.organization.id, + provider_name=provider_name, # type: ignore[typeddict-item] + web_base_url=None, + installation_id=None, + ) + + def test_returns_the_github_installation_token(self) -> None: + integration = self.create_integration( + organization=self.organization, + provider="github", + external_id="1", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) + + token = fetch_access_token(self.scm_repository("github", integration.id), "fetch") + + assert token == TOKEN + + def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> None: + integration = self.create_integration( + organization=self.create_organization(), + provider="github", + external_id="2", + metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, + ) + + token = fetch_access_token(self.scm_repository("github", integration.id), "fetch") + + assert token is None + + def test_refuses_gitlab(self) -> None: + with pytest.raises(GitProviderNotSupported): + fetch_access_token(self.scm_repository("gitlab", 1), "fetch") + + def test_refuses_cursor_origin(self) -> None: + with pytest.raises(GitProviderNotSupported): + fetch_access_token(self.scm_repository("cursor_origin", 1), "fetch") From 34855c575f53143c4ebf645a0a56e62438ecad5e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 08:37:56 +0200 Subject: [PATCH 07/30] feat(scm): Tag git endpoint failures with the grant's referrer scm-platform's GitServer now takes an emit_error callback and tags its counts with the referrer Seer puts in each grant. The view logs each failure with what the request had resolved, because git shows the client only the message. Its own refusals (no organization, flag off) carry the same referrer tag and log line, so they read like the ones GitServer answers. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 38 ++++++++++++++-- tests/sentry/scm/endpoints/test_scm_git.py | 53 ++++++++++++++++++++-- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index bd2175021f72..9117d9466251 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -1,4 +1,5 @@ import io +import logging from collections.abc import Generator from django.conf import settings @@ -28,9 +29,17 @@ from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream +logger = logging.getLogger(__name__) + FAILED_METRIC = "sentry.scm.git.request.failed" +def report_git_error(error: Exception, context: dict[str, str]) -> None: + code = error.code if isinstance(error, SCMCodedError) else type(error).__name__ + detail = error.detail if isinstance(error, SCMCodedError) else str(error) + _log_failure(code, detail, context) + + def make_git_server() -> GitServer: return GitServer( secrets=settings.SCM_GIT_GRANT_SECRETS or [], @@ -38,6 +47,7 @@ def make_git_server() -> GitServer: fetch_provider=fetch_service_provider, get_token=fetch_access_token, record_count=record_count_metric, + emit_error=report_git_error, # A GitHub Enterprise or self-managed GitLab host is whatever the customer configured, so # it may resolve to one of our own addresses. relay=GitRelay(adapter=BlacklistAdapter), @@ -74,24 +84,33 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the # database. GitServer verifies it again, which costs one HMAC. + context = {"repository_id": str(repository_id)} try: grant = verify_grant( settings.SCM_GIT_GRANT_SECRETS or [], request.headers.get(GRANT_HEADER, "") ) except SCMCodedError as e: - return _refuse(e.code, 403, e.detail) + return _refuse(e.code, 403, e.detail, context, tags={}) + context |= {"organization_id": str(grant.organization_id), "referrer": grant.referrer} + tags = {"referrer": grant.referrer} try: organization = Organization.objects.get_from_cache(id=grant.organization_id) except Organization.DoesNotExist: return _refuse( - "organization_not_found", 404, "The git grant's organization was not found." + "organization_not_found", + 404, + "The git grant's organization was not found.", + context, + tags, ) if not features.has("organizations:scm-git-endpoint", organization): return _refuse( "git_endpoint_disabled", 404, "The git endpoint is not enabled for this organization.", + context, + tags, ) try: @@ -125,13 +144,22 @@ def _request_body(request: HttpRequest) -> Readable | None: return body -def _refuse(code: str, status: int, detail: str) -> HttpResponse: - # Counted like the refusals GitServer answers itself. - record_count_metric(FAILED_METRIC, 1, {"code": code}) +def _refuse( + code: str, status: int, detail: str, context: dict[str, str], tags: dict[str, str] +) -> HttpResponse: + # Counted and logged like the refusals GitServer answers itself. + record_count_metric(FAILED_METRIC, 1, {"code": code} | tags) + _log_failure(code, detail, context) # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. return _plain_text(detail, status) +def _log_failure(code: str, detail: str, context: dict[str, str]) -> None: + # git shows the client only the message, which leaves out the repository, organization and + # provider, so the log carries them. + logger.warning("scm.git.request_failed", extra={"code": code, "detail": detail, **context}) + + def _plain_text(message: str, status: int) -> HttpResponse: # git shows a text/plain body to the user as "remote: ". return HttpResponse(f"{message}\n", status=status, content_type="text/plain; charset=utf-8") diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 84169aba5a4f..27035ab4ceea 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -26,6 +26,7 @@ SECRET = "a-long-value-that-is-hard-to-guess" TOKEN = "ghs_installation_token" FEATURE = "organizations:scm-git-endpoint" +REFERRER = "seer.coding_job" # Far enough ahead that the GitHub client never refreshes the token over the network. TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" @@ -41,8 +42,9 @@ def sign( grant = Grant( organization_id=organization_id, operation=operation, - repositories=tuple(repositories), + repositories=list(repositories), expires_at=int(time.time()) + expires_in, + referrer=REFERRER, ) return sign_grant(secret, grant) @@ -137,14 +139,25 @@ def test_relays_a_push_body(self) -> None: assert b"".join(responses.calls[0].request.body) == b"0000PACK-bytes" @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.logger") @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") - def test_refuses_a_request_without_a_grant(self, mock_record_count: mock.MagicMock) -> None: + def test_refuses_a_request_without_a_grant( + self, mock_record_count: mock.MagicMock, mock_logger: mock.MagicMock + ) -> None: response = self.fetch_refs("") self.assert_refused(response, 403, b"The git grant is missing.") mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) + mock_logger.warning.assert_called_once_with( + "scm.git.request_failed", + extra={ + "code": "git_invalid_grant", + "detail": "The git grant is missing.", + "repository_id": str(self.repo.id), + }, + ) @responses.activate def test_refuses_a_grant_signed_with_an_unknown_secret(self) -> None: @@ -170,16 +183,29 @@ def test_refuses_an_expired_grant(self) -> None: self.assert_refused(response, 403, b"The git grant has expired.") @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.logger") @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") def test_refuses_an_organization_without_the_feature( - self, mock_record_count: mock.MagicMock + self, mock_record_count: mock.MagicMock, mock_logger: mock.MagicMock ) -> None: with self.feature({FEATURE: False}): response = self.fetch_refs(self.fetch_grant()) self.assert_refused(response, 404, b"not enabled for this organization") mock_record_count.assert_called_once_with( - "sentry.scm.git.request.failed", 1, {"code": "git_endpoint_disabled"} + "sentry.scm.git.request.failed", + 1, + {"code": "git_endpoint_disabled", "referrer": REFERRER}, + ) + mock_logger.warning.assert_called_once_with( + "scm.git.request_failed", + extra={ + "code": "git_endpoint_disabled", + "detail": "The git endpoint is not enabled for this organization.", + "repository_id": str(self.repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, ) @responses.activate @@ -196,6 +222,25 @@ def test_refuses_a_push_with_a_fetch_grant(self) -> None: self.assert_refused(response, 403, b"The git grant allows fetch, not push.") + @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.logger") + def test_logs_a_refusal_with_what_the_request_resolved( + self, mock_logger: mock.MagicMock + ) -> None: + response = self.git("GET", "info/refs", "service=git-receive-pack", self.fetch_grant()) + + self.assert_refused(response, 403, b"The git grant allows fetch, not push.") + mock_logger.warning.assert_called_once_with( + "scm.git.request_failed", + extra={ + "code": "git_operation_not_granted", + "detail": "The git grant allows fetch, not push.", + "repository_id": str(self.repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, + ) + @responses.activate def test_refuses_a_fetch_with_a_push_grant(self) -> None: grant = sign(self.organization.id, "push", [self.repo.id]) From e4014f8962c93abcc06eecc7725c04c683b12e09 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 09:17:23 +0200 Subject: [PATCH 08/30] feat(scm): Get git tokens from the provider-neutral access token RPC integration_service.get_access_token mints or refreshes the token in control and sends only the token to the cell, for GitHub, GitHub Enterprise, GitLab and Cursor Origin. The git endpoint drops its own switch on provider, GitLab and Cursor Origin repositories now relay instead of being refused, and the cell no longer receives the full integration metadata just to read a token. The git server also builds its repository and provider lookups with make_fetchers, so the two share one integration fetch per request. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 12 +++---- src/sentry/scm/private/helpers.py | 36 ++++++++----------- tests/sentry/scm/endpoints/test_scm_git.py | 10 ++---- .../integration/test_helpers_integration.py | 32 +++++++++-------- 4 files changed, 39 insertions(+), 51 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 9117d9466251..1725bf6b3090 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -20,12 +20,7 @@ from sentry.exceptions import RestrictedIPAddress from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter -from sentry.scm.private.helpers import ( - fetch_access_token, - fetch_repository, - fetch_service_provider, - record_count_metric, -) +from sentry.scm.private.helpers import fetch_access_token, make_fetchers, record_count_metric from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream @@ -41,10 +36,11 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: + fetchers = make_fetchers() return GitServer( secrets=settings.SCM_GIT_GRANT_SECRETS or [], - fetch_repository=fetch_repository, - fetch_provider=fetch_service_provider, + fetch_repository=fetchers.fetch_repository, + fetch_provider=fetchers.fetch_provider, get_token=fetch_access_token, record_count=record_count_metric, emit_error=report_git_error, diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index d166fa29978e..2d3f900540c5 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -4,7 +4,7 @@ import sentry_sdk from django.db.models import Q -from scm.errors import GitProviderNotSupported, SCMCodedError, UnhandledException +from scm.errors import SCMCodedError, UnhandledException from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider @@ -19,7 +19,7 @@ from sentry.models.organization import Organization from sentry.models.repository import Repository as RepositoryModel from sentry.shared_integrations.client.proxy import IntegrationProxyClient -from sentry.shared_integrations.exceptions import ApiError, IntegrationError +from sentry.shared_integrations.exceptions import IntegrationError from sentry.utils import metrics type IntegrationGetter = Callable[[int, int], RpcIntegration | None] @@ -89,27 +89,19 @@ def fetch_service_provider( def fetch_access_token(repository: Repository, operation: GitOperation) -> str | None: """Return a token that can run git against the repository, minted in control. - A GitHub installation token carries all of the installation's permissions, so a fetch gets the - same token as a push. + The token reaches everything the installation can, so a fetch gets the same token as a push. """ - provider_name = repository["provider_name"] - if provider_name in ("github", "github_enterprise"): - try: - integration = integration_service.refresh_github_access_token( - integration_id=repository["integration_id"], - organization_id=repository["organization_id"], - ) - except (ApiError, RpcRemoteException): - # GitHub refused to mint a token, e.g. for a suspended app. Answered as a 500, git - # would show only "HTTP 500"; GitServer turns None into a 502 that says why. - sentry_sdk.capture_exception() - return None - if integration is None: - return None - return integration.metadata.get("access_token") - raise GitProviderNotSupported( - detail=f"Sentry can't relay git for {provider_name} repositories yet." - ) + try: + access_token = integration_service.get_access_token( + integration_id=repository["integration_id"], + organization_id=repository["organization_id"], + ) + except RpcRemoteException: + # Control couldn't be reached. Answered as a 500, git would show only "HTTP 500"; + # GitServer turns None into a 502 that says why. + sentry_sdk.capture_exception() + return None + return access_token.token if access_token is not None else None def fetch_repository( diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 27035ab4ceea..be5683724f13 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -18,7 +18,6 @@ from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus -from sentry.shared_integrations.exceptions import ApiError from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -309,20 +308,17 @@ def test_refuses_another_organizations_repository(self) -> None: self.assert_refused(response, 404, b"A repository could not be found.") @responses.activate - @mock.patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") @mock.patch( - "sentry.scm.private.helpers.integration_service.refresh_github_access_token", - side_effect=ApiError("Forbidden", code=403), + "sentry.scm.private.helpers.integration_service.get_access_token", return_value=None ) - def test_answers_a_token_github_refuses_to_mint_with_a_bad_gateway( - self, mock_refresh: mock.MagicMock, mock_capture_exception: mock.MagicMock + def test_answers_a_token_control_cannot_get_with_a_bad_gateway( + self, mock_get_access_token: mock.MagicMock ) -> None: response = self.fetch_refs(self.fetch_grant()) self.assert_refused( response, 502, b"Sentry could not get a github token for this repository." ) - mock_capture_exception.assert_called_once_with() @responses.activate def test_refuses_a_route_with_a_trailing_slash(self) -> None: diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 41a84a24c967..90eb839f4b9f 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -1,12 +1,11 @@ -from unittest.mock import patch +from unittest.mock import MagicMock, patch -import pytest -from scm.errors import GitProviderNotSupported from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider from scm.types import Repository from sentry.constants import ObjectStatus +from sentry.hybridcloud.rpc.service import RpcRemoteException from sentry.integrations.services.integration import integration_service from sentry.models.repository import Repository as RepositoryModel from sentry.scm.private.helpers import ( @@ -471,7 +470,7 @@ def test_fetchers_fetch_the_integration_once(self) -> None: @cell_silo_test class TestFetchAccessToken(TestCase): - def scm_repository(self, provider_name: str, integration_id: int) -> Repository: + def scm_repository(self, integration_id: int) -> Repository: return Repository( external_id="1", id=1, @@ -479,12 +478,12 @@ def scm_repository(self, provider_name: str, integration_id: int) -> Repository: is_active=True, name="getsentry/sentry", organization_id=self.organization.id, - provider_name=provider_name, # type: ignore[typeddict-item] + provider_name="github", web_base_url=None, installation_id=None, ) - def test_returns_the_github_installation_token(self) -> None: + def test_returns_the_installation_token(self) -> None: integration = self.create_integration( organization=self.organization, provider="github", @@ -492,7 +491,7 @@ def test_returns_the_github_installation_token(self) -> None: metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, ) - token = fetch_access_token(self.scm_repository("github", integration.id), "fetch") + token = fetch_access_token(self.scm_repository(integration.id), "fetch") assert token == TOKEN @@ -504,14 +503,19 @@ def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> metadata={"access_token": TOKEN, "expires_at": TOKEN_EXPIRES_AT}, ) - token = fetch_access_token(self.scm_repository("github", integration.id), "fetch") + token = fetch_access_token(self.scm_repository(integration.id), "fetch") assert token is None - def test_refuses_gitlab(self) -> None: - with pytest.raises(GitProviderNotSupported): - fetch_access_token(self.scm_repository("gitlab", 1), "fetch") + @patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") + @patch( + "sentry.scm.private.helpers.integration_service.get_access_token", + side_effect=RpcRemoteException("integration", "get_access_token", "timed out"), + ) + def test_returns_none_when_control_cannot_be_reached( + self, mock_get_access_token: MagicMock, mock_capture_exception: MagicMock + ) -> None: + token = fetch_access_token(self.scm_repository(1), "fetch") - def test_refuses_cursor_origin(self) -> None: - with pytest.raises(GitProviderNotSupported): - fetch_access_token(self.scm_repository("cursor_origin", 1), "fetch") + assert token is None + mock_capture_exception.assert_called_once_with() From 4b87d215a0d7092e54b4915470cee0602432ba7e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 09:43:23 +0200 Subject: [PATCH 09/30] fix(scm): Keep refusing GitLab repositories on the git endpoint The token RPC now leaves GitLab out until GitLab's client refreshes tokens before they expire, and answers None for it. The git server turns None into a 502, which tells git there was an outage when Sentry simply can't relay GitLab yet, so the endpoint refuses GitLab repositories as not supported before asking for a token, as it did before the switch to the RPC. Refs CW-2222 --- src/sentry/scm/private/helpers.py | 6 +++- tests/sentry/scm/endpoints/test_scm_git.py | 31 +++++++++++++++++++ .../integration/test_helpers_integration.py | 17 ++++++++-- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 2d3f900540c5..19d4d29ee777 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -4,7 +4,7 @@ import sentry_sdk from django.db.models import Q -from scm.errors import SCMCodedError, UnhandledException +from scm.errors import GitProviderNotSupported, SCMCodedError, UnhandledException from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider @@ -91,6 +91,10 @@ def fetch_access_token(repository: Repository, operation: GitOperation) -> str | The token reaches everything the installation can, so a fetch gets the same token as a push. """ + if repository["provider_name"] == "gitlab": + # The token RPC leaves GitLab out until GitLab's client refreshes tokens before they expire. + # Its None would reach git as a 502, which reads like an outage rather than a refusal. + raise GitProviderNotSupported(detail="Sentry can't relay git for gitlab repositories yet.") try: access_token = integration_service.get_access_token( integration_id=repository["integration_id"], diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index be5683724f13..0f38e4791e1d 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -320,6 +320,37 @@ def test_answers_a_token_control_cannot_get_with_a_bad_gateway( response, 502, b"Sentry could not get a github token for this repository." ) + @responses.activate + def test_refuses_a_gitlab_repository(self) -> None: + identity_provider = self.create_identity_provider( + type="gitlab", external_id="gitlab.com:acme" + ) + identity = self.create_identity(self.user, identity_provider, "gitlab-user") + integration = self.create_integration( + organization=self.organization, + provider="gitlab", + external_id="gitlab.com:acme", + metadata={ + "instance": "gitlab.com", + "base_url": "https://gitlab.com", + "verify_ssl": True, + }, + oi_params={"default_auth_id": identity.id}, + ) + repo = self.create_repo( + self.project, + name="acme / widget", + provider="integrations:gitlab", + integration_id=integration.id, + external_id="gitlab.com:42", + ) + repo.update(config={"path": "acme/widget"}) + grant = sign(self.organization.id, "fetch", [repo.id]) + + response = self.fetch_refs(grant, repo.id) + + self.assert_refused(response, 404, b"Sentry can't relay git for gitlab repositories yet.") + @responses.activate def test_refuses_a_route_with_a_trailing_slash(self) -> None: response = self.git("GET", "info/refs/", "service=git-upload-pack", self.fetch_grant()) diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 90eb839f4b9f..e410bdab136f 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -1,8 +1,10 @@ from unittest.mock import MagicMock, patch +import pytest +from scm.errors import GitProviderNotSupported from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider -from scm.types import Repository +from scm.types import ProviderName, Repository from sentry.constants import ObjectStatus from sentry.hybridcloud.rpc.service import RpcRemoteException @@ -470,7 +472,9 @@ def test_fetchers_fetch_the_integration_once(self) -> None: @cell_silo_test class TestFetchAccessToken(TestCase): - def scm_repository(self, integration_id: int) -> Repository: + def scm_repository( + self, integration_id: int, provider_name: ProviderName = "github" + ) -> Repository: return Repository( external_id="1", id=1, @@ -478,7 +482,7 @@ def scm_repository(self, integration_id: int) -> Repository: is_active=True, name="getsentry/sentry", organization_id=self.organization.id, - provider_name="github", + provider_name=provider_name, web_base_url=None, installation_id=None, ) @@ -519,3 +523,10 @@ def test_returns_none_when_control_cannot_be_reached( assert token is None mock_capture_exception.assert_called_once_with() + + @patch("sentry.scm.private.helpers.integration_service.get_access_token") + def test_refuses_gitlab(self, mock_get_access_token: MagicMock) -> None: + with pytest.raises(GitProviderNotSupported): + fetch_access_token(self.scm_repository(1, "gitlab"), "fetch") + + mock_get_access_token.assert_not_called() From f5b72ea6cab3aa950718758b84284aed38aa3a6d Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 26 Oct 2026 11:18:59 +0100 Subject: [PATCH 10/30] fix(scm): Hold git grant secrets as scm.Secret sentry-scm 2.0.0 takes signing secrets as scm.Secret, which shows as [Filtered] in error reports. Held as plain strings in a list, a failure while verifying a grant would have sent every SCM_GIT_GRANT_SECRETS entry to Sentry in the frame's locals, and GitServer now refuses to be built with them. The setting stays a list of strings and each entry is wrapped where it's read. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 15 ++++++++++----- tests/sentry/scm/endpoints/test_scm_git.py | 3 ++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 1725bf6b3090..73dff7970ff8 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -5,6 +5,7 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request +from scm import Secret from scm.errors import SCMCodedError from scm.git.grant import verify_grant from scm.git.relay import GitRelay @@ -38,7 +39,7 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: fetchers = make_fetchers() return GitServer( - secrets=settings.SCM_GIT_GRANT_SECRETS or [], + secrets=_grant_secrets(), fetch_repository=fetchers.fetch_repository, fetch_provider=fetchers.fetch_provider, get_token=fetch_access_token, @@ -79,12 +80,12 @@ def post(self, request: Request, repository_id: str, route: str) -> HttpResponse def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the - # database. GitServer verifies it again, which costs one HMAC. + # database. GitServer verifies it again, which costs one HMAC. With no secret configured, + # every grant is refused here as unknown, before GitServer, which can't be built without + # one. context = {"repository_id": str(repository_id)} try: - grant = verify_grant( - settings.SCM_GIT_GRANT_SECRETS or [], request.headers.get(GRANT_HEADER, "") - ) + grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) except SCMCodedError as e: return _refuse(e.code, 403, e.detail, context, tags={}) @@ -127,6 +128,10 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon ) +def _grant_secrets() -> list[Secret]: + return [Secret(secret) for secret in settings.SCM_GIT_GRANT_SECRETS or []] + + def _request_body(request: HttpRequest) -> Readable | None: # Endpoint.dispatch has already read a body sent with a Content-Length into memory, and nginx # sends every body that way in production, so a push is held whole up to nginx's size cap. diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 0f38e4791e1d..a39a73b0b600 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -12,6 +12,7 @@ from django.test import override_settings from django.urls import resolve, reverse from responses import matchers +from scm import Secret from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import RepositoryId @@ -45,7 +46,7 @@ def sign( expires_at=int(time.time()) + expires_in, referrer=REFERRER, ) - return sign_grant(secret, grant) + return sign_grant(Secret(secret), grant) @cell_silo_test From b3d2ace41964e29e357ad45aab2056603174281c Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 26 Oct 2026 13:11:05 +0100 Subject: [PATCH 11/30] ref(scm): Say the git endpoint re-verifies a JWT, not a bare HMAC The grant is an HS256 JWT. The comment named only the HMAC it costs, which read as if grants were bare HMAC signatures. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 73dff7970ff8..2c659d28d3e7 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -80,9 +80,9 @@ def post(self, request: Request, repository_id: str, route: str) -> HttpResponse def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the - # database. GitServer verifies it again, which costs one HMAC. With no secret configured, - # every grant is refused here as unknown, before GitServer, which can't be built without - # one. + # database. GitServer verifies the JWT again, which costs one HMAC-SHA256. With no secret + # configured, every grant is refused here as unknown, before GitServer, which can't be built + # without one. context = {"repository_id": str(repository_id)} try: grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) From 5c2d2b18ad6c9709452d9df9f227b8bfcd8cde93 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 26 Oct 2026 15:50:33 +0100 Subject: [PATCH 12/30] fix(scm): Log git requests refused for an internal provider address A GitHub Enterprise or self-managed GitLab host that resolves to an internal address was answered with a 503 but never logged, and GitServer counts it as an unhandled_exception. Nothing named the organization or repository, so a customer's misconfigured host looked like a bug in the endpoint. The view now logs it as restricted_ip_address with the blocked address and the request's context. Refs CW-2222 --- src/sentry/scm/endpoints/scm_git.py | 6 ++++-- tests/sentry/scm/endpoints/test_scm_git.py | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 2c659d28d3e7..e7d4bf1b53fb 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -120,8 +120,10 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon body=_request_body(request._request), ) except RestrictedIPAddress as e: - # A 503, like the ApiHostError Sentry's API clients raise for it, and without the - # address. GitServer has already counted it. + # The provider host resolves to an internal address. GitServer counts that as an + # unhandled_exception and doesn't log it, so the log is what names the repository. The + # client gets a 503 without the address, like the ApiHostError Sentry's API clients raise. + _log_failure("restricted_ip_address", str(e), context) return _plain_text(ApiHostError.from_exception(e).text, 503) return StreamingHttpResponse( response.content, status=response.status_code, headers=response.headers diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index a39a73b0b600..3c406884c38b 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -366,7 +366,10 @@ def test_refuses_an_inactive_repository(self) -> None: self.assert_refused(response, 404, b"A repository was found but it is inactive.") - def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) -> None: + @mock.patch("sentry.scm.endpoints.scm_git.logger") + def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( + self, mock_logger: mock.MagicMock + ) -> None: integration = self.create_integration( organization=self.organization, provider="github_enterprise", @@ -393,6 +396,16 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error(self) assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" assert b"".join(response) == b"Unable to reach host\n" + mock_logger.warning.assert_called_once_with( + "scm.git.request_failed", + extra={ + "code": "restricted_ip_address", + "detail": "(127.0.0.1) matches the URL blocklist", + "repository_id": str(repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, + ) @pytest.mark.parametrize( From 38215c449b1acc6ef88fcfde791be91d7fbb0a15 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 26 Oct 2026 15:50:38 +0100 Subject: [PATCH 13/30] fix(scm): Answer an unreadable token RPC response with a 502 When control answered the token RPC with something other than an RPC response, such as a load balancer's error page, RpcResponseException escaped fetch_access_token and git showed a bare "HTTP 500". It's now handled like an unreachable control silo: reported to Sentry and answered as the 502 that says Sentry couldn't get a token. Refs CW-2222 --- src/sentry/scm/private/helpers.py | 9 +++++---- .../scm/integration/test_helpers_integration.py | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 19d4d29ee777..b1cb3042b8e3 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -12,7 +12,7 @@ from scm.types import Provider, Repository, RepositoryId from sentry.constants import ObjectStatus -from sentry.hybridcloud.rpc.service import RpcRemoteException +from sentry.hybridcloud.rpc.service import RpcRemoteException, RpcResponseException from sentry.integrations.errors import OrganizationIntegrationNotFound from sentry.integrations.services.integration.model import RpcIntegration from sentry.integrations.services.integration.service import integration_service @@ -100,9 +100,10 @@ def fetch_access_token(repository: Repository, operation: GitOperation) -> str | integration_id=repository["integration_id"], organization_id=repository["organization_id"], ) - except RpcRemoteException: - # Control couldn't be reached. Answered as a 500, git would show only "HTTP 500"; - # GitServer turns None into a 502 that says why. + except (RpcRemoteException, RpcResponseException): + # Control couldn't be reached, or answered with something other than an RPC response, + # such as a load balancer's error page. Answered as a 500, git would show only + # "HTTP 500"; GitServer turns None into a 502 that says why. sentry_sdk.capture_exception() return None return access_token.token if access_token is not None else None diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index e410bdab136f..5c5b08f6d217 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -7,7 +7,7 @@ from scm.types import ProviderName, Repository from sentry.constants import ObjectStatus -from sentry.hybridcloud.rpc.service import RpcRemoteException +from sentry.hybridcloud.rpc.service import RpcRemoteException, RpcResponseException from sentry.integrations.services.integration import integration_service from sentry.models.repository import Repository as RepositoryModel from sentry.scm.private.helpers import ( @@ -524,6 +524,19 @@ def test_returns_none_when_control_cannot_be_reached( assert token is None mock_capture_exception.assert_called_once_with() + @patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") + @patch( + "sentry.scm.private.helpers.integration_service.get_access_token", + side_effect=RpcResponseException("integration", "get_access_token", "not JSON"), + ) + def test_returns_none_when_control_answers_with_something_else( + self, mock_get_access_token: MagicMock, mock_capture_exception: MagicMock + ) -> None: + token = fetch_access_token(self.scm_repository(1), "fetch") + + assert token is None + mock_capture_exception.assert_called_once_with() + @patch("sentry.scm.private.helpers.integration_service.get_access_token") def test_refuses_gitlab(self, mock_get_access_token: MagicMock) -> None: with pytest.raises(GitProviderNotSupported): From 3888e77d7d48749a8be96fa546e081e93899fd1a Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 18:49:05 +0100 Subject: [PATCH 14/30] feat(scm): Report every git endpoint refusal as a Sentry issue Only the credential proxy Seer's coding jobs go through can reach the git endpoint, so a refused request is never a stranger knocking: a missing or unsigned grant, a disabled flag or a grant that doesn't match the request means Sentry or Seer is misconfigured. A warning log doesn't surface that, and Sentry's logging integration never turns a log into an issue, so the endpoint now captures each refusal explicitly and logs it at error level. The view's own refusals, the ones GitServer hands back through emit_error, and a provider host on an internal address share one helper, so all three report the same way. Issues group by refusal code, since the detail varies within a code, and carry the repository, organization, referrer and whatever else the request had resolved as tags, which the message leaves out. --- src/sentry/scm/endpoints/scm_git.py | 43 ++++++++--- tests/sentry/scm/endpoints/test_scm_git.py | 84 +++++++++++++++++++--- 2 files changed, 106 insertions(+), 21 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index e7d4bf1b53fb..5c73ebf1a844 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -2,6 +2,7 @@ import logging from collections.abc import Generator +import sentry_sdk from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request @@ -33,7 +34,7 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: code = error.code if isinstance(error, SCMCodedError) else type(error).__name__ detail = error.detail if isinstance(error, SCMCodedError) else str(error) - _log_failure(code, detail, context) + _report_failure(code, detail, context, error) def make_git_server() -> GitServer: @@ -57,6 +58,10 @@ class ScmGitEndpoint(Endpoint): Serves git's smart-HTTP protocol to coding jobs, relaying each request to the repository's provider with the organization's integration credentials. A grant Seer signs authorizes each request, so the job never holds a provider credential. + + Only the credential proxy Seer's jobs go through can reach this route, so every refusal is + reported as a Sentry issue: a refusal means Sentry or Seer is misconfigured, not that a + client misbehaved. """ publish_status = {"GET": ApiPublishStatus.PRIVATE, "POST": ApiPublishStatus.PRIVATE} @@ -87,7 +92,7 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon try: grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) except SCMCodedError as e: - return _refuse(e.code, 403, e.detail, context, tags={}) + return _refuse(e.code, 403, e.detail, context, tags={}, error=e) context |= {"organization_id": str(grant.organization_id), "referrer": grant.referrer} tags = {"referrer": grant.referrer} @@ -121,9 +126,10 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon ) except RestrictedIPAddress as e: # The provider host resolves to an internal address. GitServer counts that as an - # unhandled_exception and doesn't log it, so the log is what names the repository. The - # client gets a 503 without the address, like the ApiHostError Sentry's API clients raise. - _log_failure("restricted_ip_address", str(e), context) + # unhandled_exception and doesn't report it, so the report is what names the repository. + # The client gets a 503 without the address, like the ApiHostError Sentry's API clients + # raise. + _report_failure("restricted_ip_address", str(e), context, e) return _plain_text(ApiHostError.from_exception(e).text, 503) return StreamingHttpResponse( response.content, status=response.status_code, headers=response.headers @@ -148,19 +154,34 @@ def _request_body(request: HttpRequest) -> Readable | None: def _refuse( - code: str, status: int, detail: str, context: dict[str, str], tags: dict[str, str] + code: str, + status: int, + detail: str, + context: dict[str, str], + tags: dict[str, str], + error: SCMCodedError | None = None, ) -> HttpResponse: - # Counted and logged like the refusals GitServer answers itself. + # Counted and reported like the refusals GitServer answers itself. record_count_metric(FAILED_METRIC, 1, {"code": code} | tags) - _log_failure(code, detail, context) + _report_failure(code, detail, context, error) # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. return _plain_text(detail, status) -def _log_failure(code: str, detail: str, context: dict[str, str]) -> None: +def _report_failure( + code: str, detail: str, context: dict[str, str], error: Exception | None +) -> None: # git shows the client only the message, which leaves out the repository, organization and - # provider, so the log carries them. - logger.warning("scm.git.request_failed", extra={"code": code, "detail": detail, **context}) + # provider, so the log and the issue's tags carry them. The detail can vary within a code, so + # the fingerprint groups by code alone. + logger.error("scm.git.request_failed", extra={"code": code, "detail": detail, **context}) + with sentry_sdk.new_scope() as scope: + scope.fingerprint = ["scm-git", code] + scope.set_tags(context) + if error is None: + sentry_sdk.capture_message(detail, level="error") + else: + sentry_sdk.capture_exception(error) def _plain_text(message: str, status: int) -> HttpResponse: diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 3c406884c38b..0c5725dfb614 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -13,12 +13,14 @@ from django.urls import resolve, reverse from responses import matchers from scm import Secret +from scm.errors import SCMCodedError from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import RepositoryId from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus +from sentry.exceptions import RestrictedIPAddress from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -102,8 +104,19 @@ def assert_refused(self, response: HttpResponseBase, status: int, message: bytes assert message in b"".join(response) assert len(responses.calls) == 0 + def assert_reported(self, mock_sdk: mock.MagicMock, code: str, tags: dict[str, str]) -> None: + scope = mock_sdk.new_scope.return_value.__enter__.return_value + assert scope.fingerprint == ["scm-git", code] + scope.set_tags.assert_called_once_with(tags) + + def reported_exception(self, mock_sdk: mock.MagicMock) -> BaseException: + mock_sdk.capture_exception.assert_called_once() + mock_sdk.capture_message.assert_not_called() + return mock_sdk.capture_exception.call_args.args[0] + @responses.activate - def test_relays_a_fetch_with_the_installation_token(self) -> None: + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") + def test_relays_a_fetch_with_the_installation_token(self, mock_sdk: mock.MagicMock) -> None: responses.add( responses.GET, "https://github.com/getsentry/sentry.git/info/refs", @@ -121,6 +134,8 @@ def test_relays_a_fetch_with_the_installation_token(self) -> None: credentials = base64.b64encode(f"x-access-token:{TOKEN}".encode()).decode() assert upstream.headers["Authorization"] == f"Basic {credentials}" assert GRANT_HEADER not in upstream.headers + mock_sdk.capture_exception.assert_not_called() + mock_sdk.capture_message.assert_not_called() @responses.activate def test_relays_a_push_body(self) -> None: @@ -139,10 +154,14 @@ def test_relays_a_push_body(self) -> None: assert b"".join(responses.calls[0].request.body) == b"0000PACK-bytes" @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") def test_refuses_a_request_without_a_grant( - self, mock_record_count: mock.MagicMock, mock_logger: mock.MagicMock + self, + mock_record_count: mock.MagicMock, + mock_logger: mock.MagicMock, + mock_sdk: mock.MagicMock, ) -> None: response = self.fetch_refs("") @@ -150,7 +169,7 @@ def test_refuses_a_request_without_a_grant( mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) - mock_logger.warning.assert_called_once_with( + mock_logger.error.assert_called_once_with( "scm.git.request_failed", extra={ "code": "git_invalid_grant", @@ -158,6 +177,10 @@ def test_refuses_a_request_without_a_grant( "repository_id": str(self.repo.id), }, ) + error = self.reported_exception(mock_sdk) + assert isinstance(error, SCMCodedError) + assert error.code == "git_invalid_grant" + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate def test_refuses_a_grant_signed_with_an_unknown_secret(self) -> None: @@ -183,10 +206,14 @@ def test_refuses_an_expired_grant(self) -> None: self.assert_refused(response, 403, b"The git grant has expired.") @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") def test_refuses_an_organization_without_the_feature( - self, mock_record_count: mock.MagicMock, mock_logger: mock.MagicMock + self, + mock_record_count: mock.MagicMock, + mock_logger: mock.MagicMock, + mock_sdk: mock.MagicMock, ) -> None: with self.feature({FEATURE: False}): response = self.fetch_refs(self.fetch_grant()) @@ -197,7 +224,7 @@ def test_refuses_an_organization_without_the_feature( 1, {"code": "git_endpoint_disabled", "referrer": REFERRER}, ) - mock_logger.warning.assert_called_once_with( + mock_logger.error.assert_called_once_with( "scm.git.request_failed", extra={ "code": "git_endpoint_disabled", @@ -207,6 +234,19 @@ def test_refuses_an_organization_without_the_feature( "referrer": REFERRER, }, ) + mock_sdk.capture_message.assert_called_once_with( + "The git endpoint is not enabled for this organization.", level="error" + ) + mock_sdk.capture_exception.assert_not_called() + self.assert_reported( + mock_sdk, + "git_endpoint_disabled", + { + "repository_id": str(self.repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, + ) @responses.activate def test_refuses_a_grant_for_an_organization_that_does_not_exist(self) -> None: @@ -223,14 +263,15 @@ def test_refuses_a_push_with_a_fetch_grant(self) -> None: self.assert_refused(response, 403, b"The git grant allows fetch, not push.") @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") - def test_logs_a_refusal_with_what_the_request_resolved( - self, mock_logger: mock.MagicMock + def test_reports_a_refusal_with_what_the_request_resolved( + self, mock_logger: mock.MagicMock, mock_sdk: mock.MagicMock ) -> None: response = self.git("GET", "info/refs", "service=git-receive-pack", self.fetch_grant()) self.assert_refused(response, 403, b"The git grant allows fetch, not push.") - mock_logger.warning.assert_called_once_with( + mock_logger.error.assert_called_once_with( "scm.git.request_failed", extra={ "code": "git_operation_not_granted", @@ -240,6 +281,18 @@ def test_logs_a_refusal_with_what_the_request_resolved( "referrer": REFERRER, }, ) + error = self.reported_exception(mock_sdk) + assert isinstance(error, SCMCodedError) + assert error.code == "git_operation_not_granted" + self.assert_reported( + mock_sdk, + "git_operation_not_granted", + { + "repository_id": str(self.repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, + ) @responses.activate def test_refuses_a_fetch_with_a_push_grant(self) -> None: @@ -366,9 +419,10 @@ def test_refuses_an_inactive_repository(self) -> None: self.assert_refused(response, 404, b"A repository was found but it is inactive.") + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( - self, mock_logger: mock.MagicMock + self, mock_logger: mock.MagicMock, mock_sdk: mock.MagicMock ) -> None: integration = self.create_integration( organization=self.organization, @@ -396,7 +450,7 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" assert b"".join(response) == b"Unable to reach host\n" - mock_logger.warning.assert_called_once_with( + mock_logger.error.assert_called_once_with( "scm.git.request_failed", extra={ "code": "restricted_ip_address", @@ -406,6 +460,16 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( "referrer": REFERRER, }, ) + assert isinstance(self.reported_exception(mock_sdk), RestrictedIPAddress) + self.assert_reported( + mock_sdk, + "restricted_ip_address", + { + "repository_id": str(repo.id), + "organization_id": str(self.organization.id), + "referrer": REFERRER, + }, + ) @pytest.mark.parametrize( From 64f6ce529da7237eb66904600b87ad5831cd5b6a Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 23:03:26 +0100 Subject: [PATCH 15/30] :hammer_and_wrench: Sync API Urls to TypeScript --- static/app/utils/api/knownSentryApiUrls.generated.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/static/app/utils/api/knownSentryApiUrls.generated.ts b/static/app/utils/api/knownSentryApiUrls.generated.ts index 4a90a709d2f8..424310612cfc 100644 --- a/static/app/utils/api/knownSentryApiUrls.generated.ts +++ b/static/app/utils/api/knownSentryApiUrls.generated.ts @@ -69,6 +69,7 @@ export type KnownSentryApiUrls = | '/internal/project-config/' | '/internal/projectkey-cell-mappings/' | '/internal/rpc/$serviceName/$methodName/' + | '/internal/scm-git/repositories/$repositoryId.git/$route' | '/internal/scm-rpc/' | '/internal/seer-rpc/$methodName/' | '/internal/seer/autofix/retry/' From f214b2b3bf919ad77907eb719d024d231af23b74 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 23:06:33 +0100 Subject: [PATCH 16/30] feat(scm): Relay git for GitLab repositories The endpoint refused GitLab only because the token RPC left it out. getsentry/sentry#128762 added GitLab to the RPC, and sentry-scm already relays to a GitLab remote with the oauth2 user, so the refusal now only blocks repositories that work. --- src/sentry/scm/private/helpers.py | 12 +++++------- tests/sentry/scm/endpoints/test_scm_git.py | 19 ++++++++++++++++--- .../integration/test_helpers_integration.py | 17 +++-------------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index b1cb3042b8e3..983c52c3f48b 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -4,7 +4,7 @@ import sentry_sdk from django.db.models import Q -from scm.errors import GitProviderNotSupported, SCMCodedError, UnhandledException +from scm.errors import SCMCodedError, UnhandledException from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider @@ -87,14 +87,12 @@ def fetch_service_provider( def fetch_access_token(repository: Repository, operation: GitOperation) -> str | None: - """Return a token that can run git against the repository, minted in control. + """Return a token that can run git against the repository, minted or refreshed in control. - The token reaches everything the installation can, so a fetch gets the same token as a push. + The token is the integration's: an installation token for GitHub and Cursor Origin, or the OAuth + token of the user who installed GitLab. It isn't scoped to the repository or the operation, so a + fetch gets the same token as a push. """ - if repository["provider_name"] == "gitlab": - # The token RPC leaves GitLab out until GitLab's client refreshes tokens before they expire. - # Its None would reach git as a 502, which reads like an outage rather than a refusal. - raise GitProviderNotSupported(detail="Sentry can't relay git for gitlab repositories yet.") try: access_token = integration_service.get_access_token( integration_id=repository["integration_id"], diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 0c5725dfb614..d6e8812965af 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -27,6 +27,7 @@ SECRET = "a-long-value-that-is-hard-to-guess" TOKEN = "ghs_installation_token" +GITLAB_TOKEN = "gitlab_oauth_token" FEATURE = "organizations:scm-git-endpoint" REFERRER = "seer.coding_job" # Far enough ahead that the GitHub client never refreshes the token over the network. @@ -375,11 +376,13 @@ def test_answers_a_token_control_cannot_get_with_a_bad_gateway( ) @responses.activate - def test_refuses_a_gitlab_repository(self) -> None: + def test_relays_a_gitlab_fetch_with_the_identity_token(self) -> None: identity_provider = self.create_identity_provider( type="gitlab", external_id="gitlab.com:acme" ) - identity = self.create_identity(self.user, identity_provider, "gitlab-user") + identity = self.create_identity( + self.user, identity_provider, "gitlab-user", data={"access_token": GITLAB_TOKEN} + ) integration = self.create_integration( organization=self.organization, provider="gitlab", @@ -399,11 +402,21 @@ def test_refuses_a_gitlab_repository(self) -> None: external_id="gitlab.com:42", ) repo.update(config={"path": "acme/widget"}) + responses.add( + responses.GET, + "https://gitlab.com/acme/widget.git/info/refs", + match=[matchers.query_string_matcher("service=git-upload-pack")], + body=b"001e# service=git-upload-pack\n0000", + content_type="application/x-git-upload-pack-advertisement", + ) grant = sign(self.organization.id, "fetch", [repo.id]) response = self.fetch_refs(grant, repo.id) - self.assert_refused(response, 404, b"Sentry can't relay git for gitlab repositories yet.") + assert response.status_code == 200 + assert b"".join(response) == b"001e# service=git-upload-pack\n0000" + credentials = base64.b64encode(f"oauth2:{GITLAB_TOKEN}".encode()).decode() + assert responses.calls[0].request.headers["Authorization"] == f"Basic {credentials}" @responses.activate def test_refuses_a_route_with_a_trailing_slash(self) -> None: diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 5c5b08f6d217..0fd4305c192c 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -1,10 +1,8 @@ from unittest.mock import MagicMock, patch -import pytest -from scm.errors import GitProviderNotSupported from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider -from scm.types import ProviderName, Repository +from scm.types import Repository from sentry.constants import ObjectStatus from sentry.hybridcloud.rpc.service import RpcRemoteException, RpcResponseException @@ -472,9 +470,7 @@ def test_fetchers_fetch_the_integration_once(self) -> None: @cell_silo_test class TestFetchAccessToken(TestCase): - def scm_repository( - self, integration_id: int, provider_name: ProviderName = "github" - ) -> Repository: + def scm_repository(self, integration_id: int) -> Repository: return Repository( external_id="1", id=1, @@ -482,7 +478,7 @@ def scm_repository( is_active=True, name="getsentry/sentry", organization_id=self.organization.id, - provider_name=provider_name, + provider_name="github", web_base_url=None, installation_id=None, ) @@ -536,10 +532,3 @@ def test_returns_none_when_control_answers_with_something_else( assert token is None mock_capture_exception.assert_called_once_with() - - @patch("sentry.scm.private.helpers.integration_service.get_access_token") - def test_refuses_gitlab(self, mock_get_access_token: MagicMock) -> None: - with pytest.raises(GitProviderNotSupported): - fetch_access_token(self.scm_repository(1, "gitlab"), "fetch") - - mock_get_access_token.assert_not_called() From 3a75c94d7bb3efaaa19a14e4e0ec5b231dab9fd5 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 23:12:33 +0100 Subject: [PATCH 17/30] test(scm): Keep a forged git grant's referrer out of metrics and issues Anyone can write a referrer into a grant they sign themselves, and the referrer tags Datadog metrics and Sentry issues. Only a missing grant was checked for leaving it out; a forged one that carries a referrer was not. --- tests/sentry/scm/endpoints/test_scm_git.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index d6e8812965af..a61dd7d8cdef 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -184,12 +184,22 @@ def test_refuses_a_request_without_a_grant( self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate - def test_refuses_a_grant_signed_with_an_unknown_secret(self) -> None: + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") + @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") + def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( + self, mock_record_count: mock.MagicMock, mock_sdk: mock.MagicMock + ) -> None: + # Anyone can write a referrer into a grant they sign themselves, so only a verified one + # may tag metrics and issues. grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") response = self.fetch_refs(grant) self.assert_refused(response, 403, b"not signed with a known secret") + mock_record_count.assert_called_once_with( + "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} + ) + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate @override_settings(SCM_GIT_GRANT_SECRETS=None) From 90377d80660ccc713647ac9c92603dd6ec38ca95 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 23:34:40 +0100 Subject: [PATCH 18/30] fix(scm): Report git refusals without their exceptions Sentry's SDK config sets data_collection, which turns off the SDK's event scrubber while every frame's local variables are still sent. A refusal raised after the token lookup has GitServer.handle and GitRelay.forward on its stack, both holding the provider token, and a host on an internal address raises from inside requests, whose frames hold the Authorization header. capture_exception sent all of that. Each refusal is now a message grouped and tagged as before, which carries no stack. The internal-address report also leaves out the address, and an exception that isn't an SCMCodedError is reported by its name, since its message could hold a URL or a credential. --- src/sentry/scm/endpoints/scm_git.py | 42 +++++++++---------- tests/sentry/scm/endpoints/test_scm_git.py | 48 ++++++++++++++-------- 2 files changed, 52 insertions(+), 38 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 5c73ebf1a844..6b360a4e34fc 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -32,9 +32,12 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: - code = error.code if isinstance(error, SCMCodedError) else type(error).__name__ - detail = error.detail if isinstance(error, SCMCodedError) else str(error) - _report_failure(code, detail, context, error) + # An SCMCodedError's detail never holds a URL or a credential. Any other exception is named + # rather than quoted, since its message could hold either. + if isinstance(error, SCMCodedError): + _report_failure(error.code, error.detail, context) + else: + _report_failure(type(error).__name__, type(error).__name__, context) def make_git_server() -> GitServer: @@ -92,7 +95,7 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon try: grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) except SCMCodedError as e: - return _refuse(e.code, 403, e.detail, context, tags={}, error=e) + return _refuse(e.code, 403, e.detail, context, tags={}) context |= {"organization_id": str(grant.organization_id), "referrer": grant.referrer} tags = {"referrer": grant.referrer} @@ -127,9 +130,13 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon except RestrictedIPAddress as e: # The provider host resolves to an internal address. GitServer counts that as an # unhandled_exception and doesn't report it, so the report is what names the repository. - # The client gets a 503 without the address, like the ApiHostError Sentry's API clients - # raise. - _report_failure("restricted_ip_address", str(e), context, e) + # Neither the report nor the client's 503 carries the address, like the ApiHostError + # Sentry's API clients raise. + _report_failure( + "restricted_ip_address", + "The provider host resolves to an address Sentry doesn't connect to.", + context, + ) return _plain_text(ApiHostError.from_exception(e).text, 503) return StreamingHttpResponse( response.content, status=response.status_code, headers=response.headers @@ -154,23 +161,16 @@ def _request_body(request: HttpRequest) -> Readable | None: def _refuse( - code: str, - status: int, - detail: str, - context: dict[str, str], - tags: dict[str, str], - error: SCMCodedError | None = None, + code: str, status: int, detail: str, context: dict[str, str], tags: dict[str, str] ) -> HttpResponse: # Counted and reported like the refusals GitServer answers itself. record_count_metric(FAILED_METRIC, 1, {"code": code} | tags) - _report_failure(code, detail, context, error) + _report_failure(code, detail, context) # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. return _plain_text(detail, status) -def _report_failure( - code: str, detail: str, context: dict[str, str], error: Exception | None -) -> None: +def _report_failure(code: str, detail: str, context: dict[str, str]) -> None: # git shows the client only the message, which leaves out the repository, organization and # provider, so the log and the issue's tags carry them. The detail can vary within a code, so # the fingerprint groups by code alone. @@ -178,10 +178,10 @@ def _report_failure( with sentry_sdk.new_scope() as scope: scope.fingerprint = ["scm-git", code] scope.set_tags(context) - if error is None: - sentry_sdk.capture_message(detail, level="error") - else: - sentry_sdk.capture_exception(error) + # A message, never the exception: Sentry's SDK sends every frame's local variables + # unscrubbed, and the frames a refusal is raised from hold the provider token, and + # requests' hold its Authorization header. + sentry_sdk.capture_message(detail, level="error") def _plain_text(message: str, status: int) -> HttpResponse: diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index a61dd7d8cdef..e651f5274790 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -13,14 +13,13 @@ from django.urls import resolve, reverse from responses import matchers from scm import Secret -from scm.errors import SCMCodedError from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import RepositoryId from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus -from sentry.exceptions import RestrictedIPAddress +from sentry.scm.endpoints.scm_git import report_git_error from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test @@ -110,10 +109,10 @@ def assert_reported(self, mock_sdk: mock.MagicMock, code: str, tags: dict[str, s assert scope.fingerprint == ["scm-git", code] scope.set_tags.assert_called_once_with(tags) - def reported_exception(self, mock_sdk: mock.MagicMock) -> BaseException: - mock_sdk.capture_exception.assert_called_once() - mock_sdk.capture_message.assert_not_called() - return mock_sdk.capture_exception.call_args.args[0] + def assert_reported_message(self, mock_sdk: mock.MagicMock, detail: str) -> None: + # Never the exception, whose frames hold the provider token. + mock_sdk.capture_message.assert_called_once_with(detail, level="error") + mock_sdk.capture_exception.assert_not_called() @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @@ -178,9 +177,7 @@ def test_refuses_a_request_without_a_grant( "repository_id": str(self.repo.id), }, ) - error = self.reported_exception(mock_sdk) - assert isinstance(error, SCMCodedError) - assert error.code == "git_invalid_grant" + self.assert_reported_message(mock_sdk, "The git grant is missing.") self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate @@ -199,6 +196,7 @@ def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) + self.assert_reported_message(mock_sdk, "The git grant was not signed with a known secret.") self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate @@ -245,10 +243,9 @@ def test_refuses_an_organization_without_the_feature( "referrer": REFERRER, }, ) - mock_sdk.capture_message.assert_called_once_with( - "The git endpoint is not enabled for this organization.", level="error" + self.assert_reported_message( + mock_sdk, "The git endpoint is not enabled for this organization." ) - mock_sdk.capture_exception.assert_not_called() self.assert_reported( mock_sdk, "git_endpoint_disabled", @@ -292,9 +289,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( "referrer": REFERRER, }, ) - error = self.reported_exception(mock_sdk) - assert isinstance(error, SCMCodedError) - assert error.code == "git_operation_not_granted" + self.assert_reported_message(mock_sdk, "The git grant allows fetch, not push.") self.assert_reported( mock_sdk, "git_operation_not_granted", @@ -477,13 +472,15 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( "scm.git.request_failed", extra={ "code": "restricted_ip_address", - "detail": "(127.0.0.1) matches the URL blocklist", + "detail": "The provider host resolves to an address Sentry doesn't connect to.", "repository_id": str(repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, ) - assert isinstance(self.reported_exception(mock_sdk), RestrictedIPAddress) + self.assert_reported_message( + mock_sdk, "The provider host resolves to an address Sentry doesn't connect to." + ) self.assert_reported( mock_sdk, "restricted_ip_address", @@ -520,6 +517,23 @@ def test_does_not_route(path: str) -> None: assert resolve(path).url_name == "sentry-api-catchall" +@mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") +@mock.patch("sentry.scm.endpoints.scm_git.logger") +def test_reports_an_uncoded_error_by_its_name_alone( + mock_logger: mock.MagicMock, mock_sdk: mock.MagicMock +) -> None: + error = ValueError("https://x-access-token:a-token@github.com/getsentry/sentry.git") + + report_git_error(error, {"repository_id": "1"}) + + mock_logger.error.assert_called_once_with( + "scm.git.request_failed", + extra={"code": "ValueError", "detail": "ValueError", "repository_id": "1"}, + ) + mock_sdk.capture_message.assert_called_once_with("ValueError", level="error") + mock_sdk.capture_exception.assert_not_called() + + def test_the_sdk_filters_the_grant_header_out_of_telemetry() -> None: behaviour = settings.SENTRY_SDK_CONFIG["data_collection"]["http_headers"]["request"] From 56fb36e97436e309b2c5b636a7e8d7f4b6d07005 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 28 Oct 2026 18:58:56 +0100 Subject: [PATCH 19/30] feat(scm): Name git repositories by provider and external id Seer signs git grants from its repository definitions, which hold only the provider and external id, and scm-platform covers a request only when the grant lists its repository in the same form. With Sentry's integer id in the URL, no grant Seer can sign would ever match. The repository is looked up by external id alone. The RPC lookup also matches a repository's name, which is chosen on the provider, so a repository named after a granted external id could stand in for the granted one. The RPC endpoint keeps matching names. --- src/sentry/api/urls.py | 9 +- src/sentry/scm/endpoints/scm_git.py | 31 +++- src/sentry/scm/private/helpers.py | 24 ++- .../utils/api/knownSentryApiUrls.generated.ts | 2 +- tests/sentry/scm/endpoints/test_scm_git.py | 145 ++++++++++++------ .../integration/test_helpers_integration.py | 29 ++++ 6 files changed, 180 insertions(+), 60 deletions(-) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 77b3674c97f0..17089d65a977 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -3708,9 +3708,12 @@ def create_group_urls( # No trailing slash, because git never sends one. scm-git/hosts/ is kept free for a later route # that names repositories by their provider host. re_path( - # [0-9] rather than \d, which also matches other scripts' digits. 19 digits hold any - # Repository.id, and keep int() from refusing a longer id with a 500. - r"^scm-git/repositories/(?P[0-9]{1,19})\.git/(?P.+)$", + # A repository is named the way Seer's grants name it, by provider and external id, since a + # grant only covers a request whose repository is in the same form. The providers are + # scm's ProviderName. The external id is one percent-encoded segment, which Django decodes + # before matching, e.g. GitLab's "gitlab.example.com:123". + r"^scm-git/repositories/(?Pbitbucket|cursor_origin|github|github_enterprise|gitlab)" + r"/(?P[^/]+)\.git/(?P.+)$", ScmGitEndpoint.as_view(), name="sentry-api-0-scm-git", ), diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 6b360a4e34fc..6eb7dced5dae 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -1,6 +1,7 @@ import io import logging from collections.abc import Generator +from typing import cast import sentry_sdk from django.conf import settings @@ -12,6 +13,7 @@ from scm.git.relay import GitRelay from scm.git.server import GitServer from scm.git.types import GRANT_HEADER, Readable +from scm.types import ProviderName, RepositoryId from sentry_sdk import traces from sentry import features @@ -41,7 +43,9 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: - fetchers = make_fetchers() + # By external id alone: a repository's name is chosen on the provider, so matching names too + # could resolve a granted external id to another repository whose name is that id. + fetchers = make_fetchers(match_names=False) return GitServer( secrets=_grant_secrets(), fetch_repository=fetchers.fetch_repository, @@ -60,7 +64,8 @@ class ScmGitEndpoint(Endpoint): """ Serves git's smart-HTTP protocol to coding jobs, relaying each request to the repository's provider with the organization's integration credentials. A grant Seer signs authorizes each - request, so the job never holds a provider credential. + request, so the job never holds a provider credential. Requests name the repository by its + provider and external id, as the grants do, never by Sentry's Repository.id. Only the credential proxy Seer's jobs go through can reach this route, so every refusal is reported as a Sentry issue: a refusal means Sentry or Seer is misconfigured, not that a @@ -78,19 +83,24 @@ class ScmGitEndpoint(Endpoint): enforce_rate_limit = False @traces.trace - def get(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: - return self._relay(request, int(repository_id), route) + def get( + self, request: Request, provider: str, external_id: str, route: str + ) -> HttpResponseBase: + return self._relay(request, _repository_id(provider, external_id), route) @traces.trace - def post(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: - return self._relay(request, int(repository_id), route) + def post( + self, request: Request, provider: str, external_id: str, route: str + ) -> HttpResponseBase: + return self._relay(request, _repository_id(provider, external_id), route) - def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: + def _relay(self, request: Request, repository_id: RepositoryId, route: str) -> HttpResponseBase: # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the # database. GitServer verifies the JWT again, which costs one HMAC-SHA256. With no secret # configured, every grant is refused here as unknown, before GitServer, which can't be built - # without one. + # without one. The repository is tagged the way GitServer tags its own refusals, e.g. + # "('github', '12345')", so both kinds of refusal group under one value. context = {"repository_id": str(repository_id)} try: grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) @@ -143,6 +153,11 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon ) +def _repository_id(provider: str, external_id: str) -> RepositoryId: + # The URL only routes scm's provider names. + return (cast(ProviderName, provider), external_id) + + def _grant_secrets() -> list[Secret]: return [Secret(secret) for secret in settings.SCM_GIT_GRANT_SECRETS or []] diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 983c52c3f48b..89da0d9199ec 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -30,16 +30,19 @@ class Fetchers(NamedTuple): fetch_provider: Callable[[int, Repository], Provider | None] -def make_fetchers(*, disable_proxy_retries: bool = False) -> Fetchers: +def make_fetchers(*, disable_proxy_retries: bool = False, match_names: bool = True) -> Fetchers: """Return the repository and provider lookups to build one SourceCodeManager with. Both lookups can need the repository's integration, and fetching it is an RPC to the control silo, so they share a single fetch. The integration stays cached for as long as the lookups live: make new - ones for every SourceCodeManager, never share them across requests. + ones for every SourceCodeManager, never share them across requests. ``match_names`` is passed on to + ``fetch_repository``. """ get_integration = functools.cache(_get_integration) return Fetchers( - fetch_repository=functools.partial(fetch_repository, get_integration=get_integration), + fetch_repository=functools.partial( + fetch_repository, get_integration=get_integration, match_names=match_names + ), fetch_provider=functools.partial( fetch_service_provider, get_integration=get_integration, @@ -112,15 +115,26 @@ def fetch_repository( repository_id: RepositoryId, *, get_integration: IntegrationGetter = _get_integration, + match_names: bool = True, ) -> Repository | None: + """Look up the organization's repository by Sentry's id or by a ``(provider, external id)`` pair. + + A pair also matches a repository whose name is the pair's second value, unless ``match_names`` is + off. Only the external id is unique within a provider, and a name is chosen on the provider, so a + caller that must never resolve to another repository turns it off. + """ try: if isinstance(repository_id, int): repo = RepositoryModel.objects.get(organization_id=organization_id, id=repository_id) else: + provider, external_id = repository_id + identity = Q(external_id=external_id) + if match_names: + identity |= Q(name=external_id) repo = RepositoryModel.objects.get( - Q(external_id=repository_id[1]) | Q(name=repository_id[1]), + identity, organization_id=organization_id, - provider=f"integrations:{repository_id[0]}", + provider=f"integrations:{provider}", ) except RepositoryModel.DoesNotExist: return None diff --git a/static/app/utils/api/knownSentryApiUrls.generated.ts b/static/app/utils/api/knownSentryApiUrls.generated.ts index 424310612cfc..e9ad0af32c2e 100644 --- a/static/app/utils/api/knownSentryApiUrls.generated.ts +++ b/static/app/utils/api/knownSentryApiUrls.generated.ts @@ -69,7 +69,7 @@ export type KnownSentryApiUrls = | '/internal/project-config/' | '/internal/projectkey-cell-mappings/' | '/internal/rpc/$serviceName/$methodName/' - | '/internal/scm-git/repositories/$repositoryId.git/$route' + | '/internal/scm-git/repositories/$provider/$externalId.git/$route' | '/internal/scm-rpc/' | '/internal/seer-rpc/$methodName/' | '/internal/seer/autofix/retry/' diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index e651f5274790..a5786860578c 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -3,6 +3,7 @@ import base64 import time from collections.abc import Sequence +from typing import cast from unittest import mock import pytest @@ -15,10 +16,11 @@ from scm import Secret from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant -from scm.types import RepositoryId +from scm.types import PROVIDER_SET, ProviderName, RepositoryId from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus +from sentry.models.repository import Repository from sentry.scm.endpoints.scm_git import report_git_error from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature @@ -31,6 +33,8 @@ REFERRER = "seer.coding_job" # Far enough ahead that the GitHub client never refreshes the token over the network. TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" +# How refusals tag the test's GitHub repository, the same way GitServer tags its own. +REPOSITORY_TAG = "('github', '12345')" def sign( @@ -51,6 +55,11 @@ def sign( return sign_grant(Secret(secret), grant) +def repository_id(repo: Repository) -> tuple[ProviderName, str]: + assert repo.provider is not None and repo.external_id is not None + return (cast(ProviderName, repo.provider.removeprefix("integrations:")), repo.external_id) + + @cell_silo_test @override_settings(SCM_GIT_GRANT_SECRETS=[SECRET]) @with_feature(FEATURE) @@ -72,7 +81,7 @@ def setUp(self) -> None: ) def fetch_grant(self) -> str: - return sign(self.organization.id, "fetch", [self.repo.id]) + return sign(self.organization.id, "fetch", [repository_id(self.repo)]) def git( self, @@ -80,12 +89,13 @@ def git( route: str, query: str, grant: str, - repository_id: int | None = None, + repo: tuple[ProviderName, str] | None = None, body: bytes = b"", ) -> HttpResponseBase: + provider, external_id = repo or repository_id(self.repo) url = reverse( "sentry-api-0-scm-git", - kwargs={"repository_id": repository_id or self.repo.id, "route": route}, + kwargs={"provider": provider, "external_id": external_id, "route": route}, ) return self.client.generic( method, @@ -95,8 +105,10 @@ def git( headers={GRANT_HEADER: grant}, ) - def fetch_refs(self, grant: str, repository_id: int | None = None) -> HttpResponseBase: - return self.git("GET", "info/refs", "service=git-upload-pack", grant, repository_id) + def fetch_refs( + self, grant: str, repo: tuple[ProviderName, str] | None = None + ) -> HttpResponseBase: + return self.git("GET", "info/refs", "service=git-upload-pack", grant, repo) def assert_refused(self, response: HttpResponseBase, status: int, message: bytes) -> None: assert response.status_code == status @@ -145,7 +157,7 @@ def test_relays_a_push_body(self) -> None: body=b"000eunpack ok\n0000", content_type="application/x-git-receive-pack-result", ) - grant = sign(self.organization.id, "push", [self.repo.id]) + grant = sign(self.organization.id, "push", [repository_id(self.repo)]) response = self.git("POST", "git-receive-pack", "", grant, body=b"0000PACK-bytes") @@ -174,11 +186,11 @@ def test_refuses_a_request_without_a_grant( extra={ "code": "git_invalid_grant", "detail": "The git grant is missing.", - "repository_id": str(self.repo.id), + "repository_id": REPOSITORY_TAG, }, ) self.assert_reported_message(mock_sdk, "The git grant is missing.") - self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": REPOSITORY_TAG}) @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @@ -188,7 +200,9 @@ def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( ) -> None: # Anyone can write a referrer into a grant they sign themselves, so only a verified one # may tag metrics and issues. - grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") + grant = sign( + self.organization.id, "fetch", [repository_id(self.repo)], secret="some-other-secret" + ) response = self.fetch_refs(grant) @@ -197,7 +211,7 @@ def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) self.assert_reported_message(mock_sdk, "The git grant was not signed with a known secret.") - self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": REPOSITORY_TAG}) @responses.activate @override_settings(SCM_GIT_GRANT_SECRETS=None) @@ -208,7 +222,7 @@ def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: @responses.activate def test_refuses_an_expired_grant(self) -> None: - grant = sign(self.organization.id, "fetch", [self.repo.id], expires_in=-1) + grant = sign(self.organization.id, "fetch", [repository_id(self.repo)], expires_in=-1) response = self.fetch_refs(grant) @@ -238,7 +252,7 @@ def test_refuses_an_organization_without_the_feature( extra={ "code": "git_endpoint_disabled", "detail": "The git endpoint is not enabled for this organization.", - "repository_id": str(self.repo.id), + "repository_id": REPOSITORY_TAG, "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -250,7 +264,7 @@ def test_refuses_an_organization_without_the_feature( mock_sdk, "git_endpoint_disabled", { - "repository_id": str(self.repo.id), + "repository_id": REPOSITORY_TAG, "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -258,7 +272,7 @@ def test_refuses_an_organization_without_the_feature( @responses.activate def test_refuses_a_grant_for_an_organization_that_does_not_exist(self) -> None: - grant = sign(self.organization.id + 1000, "fetch", [self.repo.id]) + grant = sign(self.organization.id + 1000, "fetch", [repository_id(self.repo)]) response = self.fetch_refs(grant) @@ -284,7 +298,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( extra={ "code": "git_operation_not_granted", "detail": "The git grant allows fetch, not push.", - "repository_id": str(self.repo.id), + "repository_id": REPOSITORY_TAG, "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -294,7 +308,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( mock_sdk, "git_operation_not_granted", { - "repository_id": str(self.repo.id), + "repository_id": REPOSITORY_TAG, "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -302,7 +316,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( @responses.activate def test_refuses_a_fetch_with_a_push_grant(self) -> None: - grant = sign(self.organization.id, "push", [self.repo.id]) + grant = sign(self.organization.id, "push", [repository_id(self.repo)]) response = self.git("POST", "git-upload-pack", "", grant, body=b"0000") @@ -324,13 +338,15 @@ def test_refuses_a_repository_outside_the_grant(self) -> None: external_id="67890", ) - response = self.fetch_refs(self.fetch_grant(), other_repo.id) + response = self.fetch_refs(self.fetch_grant(), repository_id(other_repo)) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @responses.activate - def test_refuses_a_grant_that_names_the_repository_by_external_id(self) -> None: - grant = sign(self.organization.id, "fetch", [("github", "12345")]) + def test_refuses_a_grant_that_names_the_repository_by_its_sentry_id(self) -> None: + # authorize_request matches the request's form exactly, so the same repository's database + # id doesn't cover it. + grant = sign(self.organization.id, "fetch", [self.repo.id]) response = self.fetch_refs(grant) @@ -338,13 +354,43 @@ def test_refuses_a_grant_that_names_the_repository_by_external_id(self) -> None: @responses.activate def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: - unconnected_id = self.repo.id + 1000 - grant = sign(self.organization.id, "fetch", [unconnected_id]) + unconnected: tuple[ProviderName, str] = ("github", "99999") + grant = sign(self.organization.id, "fetch", [unconnected]) + + response = self.fetch_refs(grant, unconnected) + + self.assert_refused(response, 404, b"A repository could not be found.") + + @responses.activate + def test_refuses_a_repository_named_like_the_granted_external_id(self) -> None: + # A name is chosen on the provider, so a repository named after the granted external id + # mustn't stand in for the repository that has it. + self.create_repo( + self.project, + name="99999", + provider="integrations:github", + integration_id=self.integration.id, + external_id="67890", + ) + granted: tuple[ProviderName, str] = ("github", "99999") + grant = sign(self.organization.id, "fetch", [granted]) - response = self.fetch_refs(grant, unconnected_id) + response = self.fetch_refs(grant, granted) self.assert_refused(response, 404, b"A repository could not be found.") + @responses.activate + def test_answers_an_unknown_provider_with_a_not_found(self) -> None: + grant = sign(self.organization.id, "fetch", [repository_id(self.repo)]) + + response = self.client.get( + "/api/0/internal/scm-git/repositories/gitea/12345.git/info/refs?service=git-upload-pack", + headers={GRANT_HEADER: grant}, + ) + + assert response.status_code == 404 + assert len(responses.calls) == 0 + @responses.activate def test_refuses_another_organizations_repository(self) -> None: other_org = self.create_organization() @@ -361,9 +407,9 @@ def test_refuses_another_organizations_repository(self) -> None: integration_id=other_integration.id, external_id="24680", ) - grant = sign(self.organization.id, "fetch", [other_repo.id]) + grant = sign(self.organization.id, "fetch", [repository_id(other_repo)]) - response = self.fetch_refs(grant, other_repo.id) + response = self.fetch_refs(grant, repository_id(other_repo)) self.assert_refused(response, 404, b"A repository could not be found.") @@ -414,9 +460,14 @@ def test_relays_a_gitlab_fetch_with_the_identity_token(self) -> None: body=b"001e# service=git-upload-pack\n0000", content_type="application/x-git-upload-pack-advertisement", ) - grant = sign(self.organization.id, "fetch", [repo.id]) + grant = sign(self.organization.id, "fetch", [("gitlab", "gitlab.com:42")]) - response = self.fetch_refs(grant, repo.id) + # Percent-encoded, as Seer sends it. + response = self.client.get( + "/api/0/internal/scm-git/repositories/gitlab/gitlab.com%3A42.git/info/refs" + "?service=git-upload-pack", + headers={GRANT_HEADER: grant}, + ) assert response.status_code == 200 assert b"".join(response) == b"001e# service=git-upload-pack\n0000" @@ -461,9 +512,9 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( integration_id=integration.id, external_id="13579", ) - grant = sign(self.organization.id, "fetch", [repo.id]) + grant = sign(self.organization.id, "fetch", [repository_id(repo)]) - response = self.fetch_refs(grant, repo.id) + response = self.fetch_refs(grant, repository_id(repo)) assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" @@ -473,7 +524,7 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( extra={ "code": "restricted_ip_address", "detail": "The provider host resolves to an address Sentry doesn't connect to.", - "repository_id": str(repo.id), + "repository_id": str(("github_enterprise", "13579")), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -485,32 +536,40 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( mock_sdk, "restricted_ip_address", { - "repository_id": str(repo.id), + "repository_id": str(("github_enterprise", "13579")), "organization_id": str(self.organization.id), "referrer": REFERRER, }, ) +@pytest.mark.parametrize("provider", sorted(PROVIDER_SET)) +def test_routes_every_provider(provider: str) -> None: + match = resolve(f"/api/0/internal/scm-git/repositories/{provider}/42.git/info/refs") + + assert match.url_name == "sentry-api-0-scm-git" + assert match.kwargs == {"provider": provider, "external_id": "42", "route": "info/refs"} + + @pytest.mark.parametrize( "path", [ - "/api/0/internal/scm-git/repositories/getsentry/sentry.git/info/refs", - "/api/0/internal/scm-git/repositories/42/info/refs", - "/api/0/internal/scm-git/repositories/42.git/", - "/api/0/internal/scm-git/repositories/42.git", - # More digits than any Repository.id, which int() would refuse with a 500 at 4,301. - f"/api/0/internal/scm-git/repositories/{'4' * 20}.git/info/refs", - # Arabic-Indic digits, which int() would read as 42. - "/api/0/internal/scm-git/repositories/\u0664\u0662.git/info/refs", + "/api/0/internal/scm-git/repositories/gitea/42.git/info/refs", + "/api/0/internal/scm-git/repositories/42.git/info/refs", + "/api/0/internal/scm-git/repositories/github/getsentry/sentry.git/info/refs", + "/api/0/internal/scm-git/repositories/github/.git/info/refs", + "/api/0/internal/scm-git/repositories/github/42/info/refs", + "/api/0/internal/scm-git/repositories/github/42.git/", + "/api/0/internal/scm-git/repositories/github/42.git", ], ids=[ - "name", + "unknown-provider", + "no-provider", + "slash-in-external-id", + "empty-external-id", "no-git-suffix", "nothing-after-git", "no-slash-after-git", - "20-digits", - "arabic-indic", ], ) def test_does_not_route(path: str) -> None: diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 0fd4305c192c..d1c97f1467ae 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -85,6 +85,35 @@ def test_fetch_by_provider_and_name_returns_repository(self) -> None: assert result is not None assert result["name"] == "test-org/test-repo" + def test_fetch_by_provider_and_name_returns_none_without_name_matching(self) -> None: + self.create_repo( + self.project, + name="test-org/test-repo", + provider="integrations:github", + external_id="99999", + integration_id=1, + ) + + result = fetch_repository( + self.organization.id, ("github", "test-org/test-repo"), match_names=False + ) + + assert result is None + + def test_fetch_by_provider_and_external_id_without_name_matching(self) -> None: + self.create_repo( + self.project, + name="test-org/test-repo", + provider="integrations:github", + external_id="99999", + integration_id=1, + ) + + result = fetch_repository(self.organization.id, ("github", "99999"), match_names=False) + + assert result is not None + assert result["name"] == "test-org/test-repo" + def test_fetch_by_provider_and_external_id_returns_none_for_nonexistent(self) -> None: assert fetch_repository(self.organization.id, ("github", "nonexistent")) is None From b9787e5435648b9e3b3c91ef73b003e3d97caeb4 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 28 Oct 2026 20:55:15 +0100 Subject: [PATCH 20/30] ref(scm): Give the git endpoint its own repository lookup A git grant only ever names a repository by provider and external id, so the git endpoint has no use for fetch_repository's lookups by Sentry's id or by name. A flag on the shared lookup left those paths a default away. The git endpoint now looks repositories up with a function that has neither, and fetch_repository is back to what the RPC endpoint had. Both still build the repository the same way. --- src/sentry/scm/endpoints/scm_git.py | 6 +- src/sentry/scm/private/helpers.py | 69 ++++++++---- .../integration/test_helpers_integration.py | 103 +++++++++++++----- 3 files changed, 126 insertions(+), 52 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 6eb7dced5dae..d4621c3c3ad5 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -24,7 +24,7 @@ from sentry.exceptions import RestrictedIPAddress from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter -from sentry.scm.private.helpers import fetch_access_token, make_fetchers, record_count_metric +from sentry.scm.private.helpers import fetch_access_token, make_git_fetchers, record_count_metric from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream @@ -43,9 +43,7 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: - # By external id alone: a repository's name is chosen on the provider, so matching names too - # could resolve a granted external id to another repository whose name is that id. - fetchers = make_fetchers(match_names=False) + fetchers = make_git_fetchers() return GitServer( secrets=_grant_secrets(), fetch_repository=fetchers.fetch_repository, diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 89da0d9199ec..481377c2b5bb 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -30,19 +30,16 @@ class Fetchers(NamedTuple): fetch_provider: Callable[[int, Repository], Provider | None] -def make_fetchers(*, disable_proxy_retries: bool = False, match_names: bool = True) -> Fetchers: +def make_fetchers(*, disable_proxy_retries: bool = False) -> Fetchers: """Return the repository and provider lookups to build one SourceCodeManager with. Both lookups can need the repository's integration, and fetching it is an RPC to the control silo, so they share a single fetch. The integration stays cached for as long as the lookups live: make new - ones for every SourceCodeManager, never share them across requests. ``match_names`` is passed on to - ``fetch_repository``. + ones for every SourceCodeManager, never share them across requests. """ get_integration = functools.cache(_get_integration) return Fetchers( - fetch_repository=functools.partial( - fetch_repository, get_integration=get_integration, match_names=match_names - ), + fetch_repository=functools.partial(fetch_repository, get_integration=get_integration), fetch_provider=functools.partial( fetch_service_provider, get_integration=get_integration, @@ -51,6 +48,20 @@ def make_fetchers(*, disable_proxy_retries: bool = False, match_names: bool = Tr ) +def make_git_fetchers() -> Fetchers: + """Return the lookups to build one GitServer with, which share an integration like make_fetchers'. + + The repository is found with fetch_repository_by_external_id, the only form a git grant names. + """ + get_integration = functools.cache(_get_integration) + return Fetchers( + fetch_repository=functools.partial( + fetch_repository_by_external_id, get_integration=get_integration + ), + fetch_provider=functools.partial(fetch_service_provider, get_integration=get_integration), + ) + + def _get_integration(organization_id: int, integration_id: int) -> RpcIntegration | None: return integration_service.get_integration( integration_id=integration_id, @@ -115,30 +126,50 @@ def fetch_repository( repository_id: RepositoryId, *, get_integration: IntegrationGetter = _get_integration, - match_names: bool = True, ) -> Repository | None: - """Look up the organization's repository by Sentry's id or by a ``(provider, external id)`` pair. - - A pair also matches a repository whose name is the pair's second value, unless ``match_names`` is - off. Only the external id is unique within a provider, and a name is chosen on the provider, so a - caller that must never resolve to another repository turns it off. - """ try: if isinstance(repository_id, int): repo = RepositoryModel.objects.get(organization_id=organization_id, id=repository_id) else: - provider, external_id = repository_id - identity = Q(external_id=external_id) - if match_names: - identity |= Q(name=external_id) repo = RepositoryModel.objects.get( - identity, + Q(external_id=repository_id[1]) | Q(name=repository_id[1]), organization_id=organization_id, - provider=f"integrations:{provider}", + provider=f"integrations:{repository_id[0]}", ) except RepositoryModel.DoesNotExist: return None + return _to_scm_repository(repo, organization_id, get_integration) + + +def fetch_repository_by_external_id( + organization_id: int, + repository_id: RepositoryId, + *, + get_integration: IntegrationGetter = _get_integration, +) -> Repository | None: + """Look up the organization's repository by ``(provider, external id)`` alone. + + Unlike fetch_repository, never by Sentry's id or by name. ``(organization, provider, external + id)`` is unique, but a name is chosen on the provider, so matching names could resolve an + external id to another repository that is named after it. + """ + if isinstance(repository_id, int): + return None + provider, external_id = repository_id + try: + repo = RepositoryModel.objects.get( + organization_id=organization_id, + provider=f"integrations:{provider}", + external_id=external_id, + ) + except RepositoryModel.DoesNotExist: + return None + return _to_scm_repository(repo, organization_id, get_integration) + +def _to_scm_repository( + repo: RepositoryModel, organization_id: int, get_integration: IntegrationGetter +) -> Repository | None: # Tag the current Sentry scope with the organization so that any exceptions # captured later in this request (e.g. ResourceForbidden from the SCM # provider) include the affected customer for distribution analysis. diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index d1c97f1467ae..db5725ad278a 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -11,8 +11,10 @@ from sentry.scm.private.helpers import ( fetch_access_token, fetch_repository, + fetch_repository_by_external_id, fetch_service_provider, make_fetchers, + make_git_fetchers, ) from sentry.testutils.cases import TestCase from sentry.testutils.silo import cell_silo_test @@ -85,35 +87,6 @@ def test_fetch_by_provider_and_name_returns_repository(self) -> None: assert result is not None assert result["name"] == "test-org/test-repo" - def test_fetch_by_provider_and_name_returns_none_without_name_matching(self) -> None: - self.create_repo( - self.project, - name="test-org/test-repo", - provider="integrations:github", - external_id="99999", - integration_id=1, - ) - - result = fetch_repository( - self.organization.id, ("github", "test-org/test-repo"), match_names=False - ) - - assert result is None - - def test_fetch_by_provider_and_external_id_without_name_matching(self) -> None: - self.create_repo( - self.project, - name="test-org/test-repo", - provider="integrations:github", - external_id="99999", - integration_id=1, - ) - - result = fetch_repository(self.organization.id, ("github", "99999"), match_names=False) - - assert result is not None - assert result["name"] == "test-org/test-repo" - def test_fetch_by_provider_and_external_id_returns_none_for_nonexistent(self) -> None: assert fetch_repository(self.organization.id, ("github", "nonexistent")) is None @@ -464,6 +437,45 @@ def test_github_enterprise_client_error_returns_none(self) -> None: assert fetch_service_provider(self.organization.id, repository) is None +class TestFetchRepositoryByExternalId(TestCase): + def setUp(self) -> None: + super().setUp() + self.repo = self.create_repo( + self.project, + name="test-org/test-repo", + provider="integrations:github", + external_id="99999", + integration_id=1, + ) + + def test_returns_the_repository(self) -> None: + result = fetch_repository_by_external_id(self.organization.id, ("github", "99999")) + + assert result is not None + assert result["id"] == self.repo.id + assert result["name"] == "test-org/test-repo" + + def test_does_not_match_a_name(self) -> None: + result = fetch_repository_by_external_id( + self.organization.id, ("github", "test-org/test-repo") + ) + + assert result is None + + def test_does_not_match_sentrys_id(self) -> None: + assert fetch_repository_by_external_id(self.organization.id, self.repo.id) is None + + def test_does_not_match_another_provider(self) -> None: + result = fetch_repository_by_external_id(self.organization.id, ("gitlab", "99999")) + + assert result is None + + def test_does_not_match_another_organizations_repository(self) -> None: + result = fetch_repository_by_external_id(self.create_organization().id, ("github", "99999")) + + assert result is None + + class TestMakeFetchers(TestCase): def test_fetchers_fetch_the_integration_once(self) -> None: integration = self.create_integration( @@ -496,6 +508,39 @@ def test_fetchers_fetch_the_integration_once(self) -> None: assert isinstance(provider, GitHubProvider) assert service.get_integration.call_count == 1 + def test_git_fetchers_fetch_the_integration_once(self) -> None: + integration = self.create_integration( + organization=self.organization, + provider="github_enterprise", + name="GHE Acme", + external_id="ghe-fetchers-1", + metadata={ + "domain_name": "github.acme.com", + "installation_id": "1", + "installation": {"id": "1", "private_key": "x", "verify_ssl": True}, + }, + ) + self.create_repo( + project=self.project, + name="acme/widget", + provider="integrations:github_enterprise", + external_id="9001", + integration_id=integration.id, + ) + fetchers = make_git_fetchers() + + with patch( + "sentry.scm.private.helpers.integration_service", wraps=integration_service + ) as service: + repository = fetchers.fetch_repository( + self.organization.id, ("github_enterprise", "9001") + ) + assert repository is not None + provider = fetchers.fetch_provider(self.organization.id, repository) + + assert isinstance(provider, GitHubProvider) + assert service.get_integration.call_count == 1 + @cell_silo_test class TestFetchAccessToken(TestCase): From 893d00e5a03b4fb8b2fffbe4f33721c01397ea98 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 28 Oct 2026 20:56:29 +0100 Subject: [PATCH 21/30] fix(scm): Fetch a GitLab repository's integration once per request Looking up a GitLab repository fetched its integration from control directly, bypassing the cache it shares with the provider lookup, so every GitLab request made the same RPC twice. --- src/sentry/scm/private/helpers.py | 5 +-- .../integration/test_helpers_integration.py | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 481377c2b5bb..921f401e8bac 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -204,10 +204,7 @@ def _to_scm_repository( base_host = domain_name.split("/", 1)[0] web_base_url = f"https://{base_host}" elif provider_name == "gitlab": - integration = integration_service.get_integration( - integration_id=repo.integration_id, - organization_id=organization_id, - ) + integration = get_integration(organization_id, repo.integration_id) if integration: # The same instance URL the GitLab API client and source links are built from, so it # keeps the scheme and any path prefix of a self-hosted instance. Installs from older diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index db5725ad278a..8d49e8f14efa 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -2,6 +2,7 @@ from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider +from scm.providers.gitlab.provider import GitLabProvider from scm.types import Repository from sentry.constants import ObjectStatus @@ -508,6 +509,43 @@ def test_fetchers_fetch_the_integration_once(self) -> None: assert isinstance(provider, GitHubProvider) assert service.get_integration.call_count == 1 + def test_fetchers_fetch_a_gitlab_integration_once(self) -> None: + identity = self.create_identity( + self.user, + self.create_identity_provider(type="gitlab", external_id="gitlab.com:acme"), + "gitlab-user", + data={"access_token": "gitlab_oauth_token"}, + ) + integration = self.create_integration( + organization=self.organization, + provider="gitlab", + external_id="gitlab.com:acme", + metadata={ + "instance": "gitlab.com", + "base_url": "https://gitlab.com", + "verify_ssl": True, + }, + oi_params={"default_auth_id": identity.id}, + ) + repo = self.create_repo( + project=self.project, + name="acme / widget", + provider="integrations:gitlab", + external_id="gitlab.com:42", + integration_id=integration.id, + ) + fetchers = make_fetchers() + + with patch( + "sentry.scm.private.helpers.integration_service", wraps=integration_service + ) as service: + repository = fetchers.fetch_repository(self.organization.id, repo.id) + assert repository is not None + provider = fetchers.fetch_provider(self.organization.id, repository) + + assert isinstance(provider, GitLabProvider) + assert service.get_integration.call_count == 1 + def test_git_fetchers_fetch_the_integration_once(self) -> None: integration = self.create_integration( organization=self.organization, From 1f4633f3e5c7912712884859293edf33f3b4e3f6 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 28 Oct 2026 21:21:27 +0100 Subject: [PATCH 22/30] feat(scm): Name git repositories by Sentry's repository id again A provider and external id don't identify a repository across hosts. GitHub Enterprise external ids are each server's own repository ids, so two servers connected to one organization number their repositories from the same range. Repository.id stays unique however Sentry comes to tell such repositories apart, so the URL and grants name it again, and Seer signs grants with the id Sentry already sends it for each repository. This takes back the provider and external id route and the lookup only it needed, and keeps fetching a GitLab repository's integration once per request. --- src/sentry/api/urls.py | 9 +- src/sentry/scm/endpoints/scm_git.py | 31 ++-- src/sentry/scm/private/helpers.py | 45 ------ .../utils/api/knownSentryApiUrls.generated.ts | 2 +- tests/sentry/scm/endpoints/test_scm_git.py | 145 ++++++------------ .../integration/test_helpers_integration.py | 74 --------- 6 files changed, 56 insertions(+), 250 deletions(-) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 17089d65a977..77b3674c97f0 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -3708,12 +3708,9 @@ def create_group_urls( # No trailing slash, because git never sends one. scm-git/hosts/ is kept free for a later route # that names repositories by their provider host. re_path( - # A repository is named the way Seer's grants name it, by provider and external id, since a - # grant only covers a request whose repository is in the same form. The providers are - # scm's ProviderName. The external id is one percent-encoded segment, which Django decodes - # before matching, e.g. GitLab's "gitlab.example.com:123". - r"^scm-git/repositories/(?Pbitbucket|cursor_origin|github|github_enterprise|gitlab)" - r"/(?P[^/]+)\.git/(?P.+)$", + # [0-9] rather than \d, which also matches other scripts' digits. 19 digits hold any + # Repository.id, and keep int() from refusing a longer id with a 500. + r"^scm-git/repositories/(?P[0-9]{1,19})\.git/(?P.+)$", ScmGitEndpoint.as_view(), name="sentry-api-0-scm-git", ), diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index d4621c3c3ad5..6b360a4e34fc 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -1,7 +1,6 @@ import io import logging from collections.abc import Generator -from typing import cast import sentry_sdk from django.conf import settings @@ -13,7 +12,6 @@ from scm.git.relay import GitRelay from scm.git.server import GitServer from scm.git.types import GRANT_HEADER, Readable -from scm.types import ProviderName, RepositoryId from sentry_sdk import traces from sentry import features @@ -24,7 +22,7 @@ from sentry.exceptions import RestrictedIPAddress from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter -from sentry.scm.private.helpers import fetch_access_token, make_git_fetchers, record_count_metric +from sentry.scm.private.helpers import fetch_access_token, make_fetchers, record_count_metric from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream @@ -43,7 +41,7 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: - fetchers = make_git_fetchers() + fetchers = make_fetchers() return GitServer( secrets=_grant_secrets(), fetch_repository=fetchers.fetch_repository, @@ -62,8 +60,7 @@ class ScmGitEndpoint(Endpoint): """ Serves git's smart-HTTP protocol to coding jobs, relaying each request to the repository's provider with the organization's integration credentials. A grant Seer signs authorizes each - request, so the job never holds a provider credential. Requests name the repository by its - provider and external id, as the grants do, never by Sentry's Repository.id. + request, so the job never holds a provider credential. Only the credential proxy Seer's jobs go through can reach this route, so every refusal is reported as a Sentry issue: a refusal means Sentry or Seer is misconfigured, not that a @@ -81,24 +78,19 @@ class ScmGitEndpoint(Endpoint): enforce_rate_limit = False @traces.trace - def get( - self, request: Request, provider: str, external_id: str, route: str - ) -> HttpResponseBase: - return self._relay(request, _repository_id(provider, external_id), route) + def get(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: + return self._relay(request, int(repository_id), route) @traces.trace - def post( - self, request: Request, provider: str, external_id: str, route: str - ) -> HttpResponseBase: - return self._relay(request, _repository_id(provider, external_id), route) + def post(self, request: Request, repository_id: str, route: str) -> HttpResponseBase: + return self._relay(request, int(repository_id), route) - def _relay(self, request: Request, repository_id: RepositoryId, route: str) -> HttpResponseBase: + def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the # database. GitServer verifies the JWT again, which costs one HMAC-SHA256. With no secret # configured, every grant is refused here as unknown, before GitServer, which can't be built - # without one. The repository is tagged the way GitServer tags its own refusals, e.g. - # "('github', '12345')", so both kinds of refusal group under one value. + # without one. context = {"repository_id": str(repository_id)} try: grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) @@ -151,11 +143,6 @@ def _relay(self, request: Request, repository_id: RepositoryId, route: str) -> H ) -def _repository_id(provider: str, external_id: str) -> RepositoryId: - # The URL only routes scm's provider names. - return (cast(ProviderName, provider), external_id) - - def _grant_secrets() -> list[Secret]: return [Secret(secret) for secret in settings.SCM_GIT_GRANT_SECRETS or []] diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 921f401e8bac..e8c7994d3adb 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -48,20 +48,6 @@ def make_fetchers(*, disable_proxy_retries: bool = False) -> Fetchers: ) -def make_git_fetchers() -> Fetchers: - """Return the lookups to build one GitServer with, which share an integration like make_fetchers'. - - The repository is found with fetch_repository_by_external_id, the only form a git grant names. - """ - get_integration = functools.cache(_get_integration) - return Fetchers( - fetch_repository=functools.partial( - fetch_repository_by_external_id, get_integration=get_integration - ), - fetch_provider=functools.partial(fetch_service_provider, get_integration=get_integration), - ) - - def _get_integration(organization_id: int, integration_id: int) -> RpcIntegration | None: return integration_service.get_integration( integration_id=integration_id, @@ -138,38 +124,7 @@ def fetch_repository( ) except RepositoryModel.DoesNotExist: return None - return _to_scm_repository(repo, organization_id, get_integration) - - -def fetch_repository_by_external_id( - organization_id: int, - repository_id: RepositoryId, - *, - get_integration: IntegrationGetter = _get_integration, -) -> Repository | None: - """Look up the organization's repository by ``(provider, external id)`` alone. - - Unlike fetch_repository, never by Sentry's id or by name. ``(organization, provider, external - id)`` is unique, but a name is chosen on the provider, so matching names could resolve an - external id to another repository that is named after it. - """ - if isinstance(repository_id, int): - return None - provider, external_id = repository_id - try: - repo = RepositoryModel.objects.get( - organization_id=organization_id, - provider=f"integrations:{provider}", - external_id=external_id, - ) - except RepositoryModel.DoesNotExist: - return None - return _to_scm_repository(repo, organization_id, get_integration) - -def _to_scm_repository( - repo: RepositoryModel, organization_id: int, get_integration: IntegrationGetter -) -> Repository | None: # Tag the current Sentry scope with the organization so that any exceptions # captured later in this request (e.g. ResourceForbidden from the SCM # provider) include the affected customer for distribution analysis. diff --git a/static/app/utils/api/knownSentryApiUrls.generated.ts b/static/app/utils/api/knownSentryApiUrls.generated.ts index e9ad0af32c2e..424310612cfc 100644 --- a/static/app/utils/api/knownSentryApiUrls.generated.ts +++ b/static/app/utils/api/knownSentryApiUrls.generated.ts @@ -69,7 +69,7 @@ export type KnownSentryApiUrls = | '/internal/project-config/' | '/internal/projectkey-cell-mappings/' | '/internal/rpc/$serviceName/$methodName/' - | '/internal/scm-git/repositories/$provider/$externalId.git/$route' + | '/internal/scm-git/repositories/$repositoryId.git/$route' | '/internal/scm-rpc/' | '/internal/seer-rpc/$methodName/' | '/internal/seer/autofix/retry/' diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index a5786860578c..e651f5274790 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -3,7 +3,6 @@ import base64 import time from collections.abc import Sequence -from typing import cast from unittest import mock import pytest @@ -16,11 +15,10 @@ from scm import Secret from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant -from scm.types import PROVIDER_SET, ProviderName, RepositoryId +from scm.types import RepositoryId from sentry_sdk.data_collection import _apply_key_value_collection_filtering from sentry.constants import ObjectStatus -from sentry.models.repository import Repository from sentry.scm.endpoints.scm_git import report_git_error from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature @@ -33,8 +31,6 @@ REFERRER = "seer.coding_job" # Far enough ahead that the GitHub client never refreshes the token over the network. TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" -# How refusals tag the test's GitHub repository, the same way GitServer tags its own. -REPOSITORY_TAG = "('github', '12345')" def sign( @@ -55,11 +51,6 @@ def sign( return sign_grant(Secret(secret), grant) -def repository_id(repo: Repository) -> tuple[ProviderName, str]: - assert repo.provider is not None and repo.external_id is not None - return (cast(ProviderName, repo.provider.removeprefix("integrations:")), repo.external_id) - - @cell_silo_test @override_settings(SCM_GIT_GRANT_SECRETS=[SECRET]) @with_feature(FEATURE) @@ -81,7 +72,7 @@ def setUp(self) -> None: ) def fetch_grant(self) -> str: - return sign(self.organization.id, "fetch", [repository_id(self.repo)]) + return sign(self.organization.id, "fetch", [self.repo.id]) def git( self, @@ -89,13 +80,12 @@ def git( route: str, query: str, grant: str, - repo: tuple[ProviderName, str] | None = None, + repository_id: int | None = None, body: bytes = b"", ) -> HttpResponseBase: - provider, external_id = repo or repository_id(self.repo) url = reverse( "sentry-api-0-scm-git", - kwargs={"provider": provider, "external_id": external_id, "route": route}, + kwargs={"repository_id": repository_id or self.repo.id, "route": route}, ) return self.client.generic( method, @@ -105,10 +95,8 @@ def git( headers={GRANT_HEADER: grant}, ) - def fetch_refs( - self, grant: str, repo: tuple[ProviderName, str] | None = None - ) -> HttpResponseBase: - return self.git("GET", "info/refs", "service=git-upload-pack", grant, repo) + def fetch_refs(self, grant: str, repository_id: int | None = None) -> HttpResponseBase: + return self.git("GET", "info/refs", "service=git-upload-pack", grant, repository_id) def assert_refused(self, response: HttpResponseBase, status: int, message: bytes) -> None: assert response.status_code == status @@ -157,7 +145,7 @@ def test_relays_a_push_body(self) -> None: body=b"000eunpack ok\n0000", content_type="application/x-git-receive-pack-result", ) - grant = sign(self.organization.id, "push", [repository_id(self.repo)]) + grant = sign(self.organization.id, "push", [self.repo.id]) response = self.git("POST", "git-receive-pack", "", grant, body=b"0000PACK-bytes") @@ -186,11 +174,11 @@ def test_refuses_a_request_without_a_grant( extra={ "code": "git_invalid_grant", "detail": "The git grant is missing.", - "repository_id": REPOSITORY_TAG, + "repository_id": str(self.repo.id), }, ) self.assert_reported_message(mock_sdk, "The git grant is missing.") - self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": REPOSITORY_TAG}) + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @@ -200,9 +188,7 @@ def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( ) -> None: # Anyone can write a referrer into a grant they sign themselves, so only a verified one # may tag metrics and issues. - grant = sign( - self.organization.id, "fetch", [repository_id(self.repo)], secret="some-other-secret" - ) + grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") response = self.fetch_refs(grant) @@ -211,7 +197,7 @@ def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) self.assert_reported_message(mock_sdk, "The git grant was not signed with a known secret.") - self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": REPOSITORY_TAG}) + self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate @override_settings(SCM_GIT_GRANT_SECRETS=None) @@ -222,7 +208,7 @@ def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: @responses.activate def test_refuses_an_expired_grant(self) -> None: - grant = sign(self.organization.id, "fetch", [repository_id(self.repo)], expires_in=-1) + grant = sign(self.organization.id, "fetch", [self.repo.id], expires_in=-1) response = self.fetch_refs(grant) @@ -252,7 +238,7 @@ def test_refuses_an_organization_without_the_feature( extra={ "code": "git_endpoint_disabled", "detail": "The git endpoint is not enabled for this organization.", - "repository_id": REPOSITORY_TAG, + "repository_id": str(self.repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -264,7 +250,7 @@ def test_refuses_an_organization_without_the_feature( mock_sdk, "git_endpoint_disabled", { - "repository_id": REPOSITORY_TAG, + "repository_id": str(self.repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -272,7 +258,7 @@ def test_refuses_an_organization_without_the_feature( @responses.activate def test_refuses_a_grant_for_an_organization_that_does_not_exist(self) -> None: - grant = sign(self.organization.id + 1000, "fetch", [repository_id(self.repo)]) + grant = sign(self.organization.id + 1000, "fetch", [self.repo.id]) response = self.fetch_refs(grant) @@ -298,7 +284,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( extra={ "code": "git_operation_not_granted", "detail": "The git grant allows fetch, not push.", - "repository_id": REPOSITORY_TAG, + "repository_id": str(self.repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -308,7 +294,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( mock_sdk, "git_operation_not_granted", { - "repository_id": REPOSITORY_TAG, + "repository_id": str(self.repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -316,7 +302,7 @@ def test_reports_a_refusal_with_what_the_request_resolved( @responses.activate def test_refuses_a_fetch_with_a_push_grant(self) -> None: - grant = sign(self.organization.id, "push", [repository_id(self.repo)]) + grant = sign(self.organization.id, "push", [self.repo.id]) response = self.git("POST", "git-upload-pack", "", grant, body=b"0000") @@ -338,15 +324,13 @@ def test_refuses_a_repository_outside_the_grant(self) -> None: external_id="67890", ) - response = self.fetch_refs(self.fetch_grant(), repository_id(other_repo)) + response = self.fetch_refs(self.fetch_grant(), other_repo.id) self.assert_refused(response, 403, b"The git grant does not cover this repository.") @responses.activate - def test_refuses_a_grant_that_names_the_repository_by_its_sentry_id(self) -> None: - # authorize_request matches the request's form exactly, so the same repository's database - # id doesn't cover it. - grant = sign(self.organization.id, "fetch", [self.repo.id]) + def test_refuses_a_grant_that_names_the_repository_by_external_id(self) -> None: + grant = sign(self.organization.id, "fetch", [("github", "12345")]) response = self.fetch_refs(grant) @@ -354,43 +338,13 @@ def test_refuses_a_grant_that_names_the_repository_by_its_sentry_id(self) -> Non @responses.activate def test_refuses_a_repository_the_organization_has_not_connected(self) -> None: - unconnected: tuple[ProviderName, str] = ("github", "99999") - grant = sign(self.organization.id, "fetch", [unconnected]) - - response = self.fetch_refs(grant, unconnected) - - self.assert_refused(response, 404, b"A repository could not be found.") - - @responses.activate - def test_refuses_a_repository_named_like_the_granted_external_id(self) -> None: - # A name is chosen on the provider, so a repository named after the granted external id - # mustn't stand in for the repository that has it. - self.create_repo( - self.project, - name="99999", - provider="integrations:github", - integration_id=self.integration.id, - external_id="67890", - ) - granted: tuple[ProviderName, str] = ("github", "99999") - grant = sign(self.organization.id, "fetch", [granted]) + unconnected_id = self.repo.id + 1000 + grant = sign(self.organization.id, "fetch", [unconnected_id]) - response = self.fetch_refs(grant, granted) + response = self.fetch_refs(grant, unconnected_id) self.assert_refused(response, 404, b"A repository could not be found.") - @responses.activate - def test_answers_an_unknown_provider_with_a_not_found(self) -> None: - grant = sign(self.organization.id, "fetch", [repository_id(self.repo)]) - - response = self.client.get( - "/api/0/internal/scm-git/repositories/gitea/12345.git/info/refs?service=git-upload-pack", - headers={GRANT_HEADER: grant}, - ) - - assert response.status_code == 404 - assert len(responses.calls) == 0 - @responses.activate def test_refuses_another_organizations_repository(self) -> None: other_org = self.create_organization() @@ -407,9 +361,9 @@ def test_refuses_another_organizations_repository(self) -> None: integration_id=other_integration.id, external_id="24680", ) - grant = sign(self.organization.id, "fetch", [repository_id(other_repo)]) + grant = sign(self.organization.id, "fetch", [other_repo.id]) - response = self.fetch_refs(grant, repository_id(other_repo)) + response = self.fetch_refs(grant, other_repo.id) self.assert_refused(response, 404, b"A repository could not be found.") @@ -460,14 +414,9 @@ def test_relays_a_gitlab_fetch_with_the_identity_token(self) -> None: body=b"001e# service=git-upload-pack\n0000", content_type="application/x-git-upload-pack-advertisement", ) - grant = sign(self.organization.id, "fetch", [("gitlab", "gitlab.com:42")]) + grant = sign(self.organization.id, "fetch", [repo.id]) - # Percent-encoded, as Seer sends it. - response = self.client.get( - "/api/0/internal/scm-git/repositories/gitlab/gitlab.com%3A42.git/info/refs" - "?service=git-upload-pack", - headers={GRANT_HEADER: grant}, - ) + response = self.fetch_refs(grant, repo.id) assert response.status_code == 200 assert b"".join(response) == b"001e# service=git-upload-pack\n0000" @@ -512,9 +461,9 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( integration_id=integration.id, external_id="13579", ) - grant = sign(self.organization.id, "fetch", [repository_id(repo)]) + grant = sign(self.organization.id, "fetch", [repo.id]) - response = self.fetch_refs(grant, repository_id(repo)) + response = self.fetch_refs(grant, repo.id) assert response.status_code == 503 assert response["Content-Type"] == "text/plain; charset=utf-8" @@ -524,7 +473,7 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( extra={ "code": "restricted_ip_address", "detail": "The provider host resolves to an address Sentry doesn't connect to.", - "repository_id": str(("github_enterprise", "13579")), + "repository_id": str(repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, @@ -536,40 +485,32 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( mock_sdk, "restricted_ip_address", { - "repository_id": str(("github_enterprise", "13579")), + "repository_id": str(repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, }, ) -@pytest.mark.parametrize("provider", sorted(PROVIDER_SET)) -def test_routes_every_provider(provider: str) -> None: - match = resolve(f"/api/0/internal/scm-git/repositories/{provider}/42.git/info/refs") - - assert match.url_name == "sentry-api-0-scm-git" - assert match.kwargs == {"provider": provider, "external_id": "42", "route": "info/refs"} - - @pytest.mark.parametrize( "path", [ - "/api/0/internal/scm-git/repositories/gitea/42.git/info/refs", - "/api/0/internal/scm-git/repositories/42.git/info/refs", - "/api/0/internal/scm-git/repositories/github/getsentry/sentry.git/info/refs", - "/api/0/internal/scm-git/repositories/github/.git/info/refs", - "/api/0/internal/scm-git/repositories/github/42/info/refs", - "/api/0/internal/scm-git/repositories/github/42.git/", - "/api/0/internal/scm-git/repositories/github/42.git", + "/api/0/internal/scm-git/repositories/getsentry/sentry.git/info/refs", + "/api/0/internal/scm-git/repositories/42/info/refs", + "/api/0/internal/scm-git/repositories/42.git/", + "/api/0/internal/scm-git/repositories/42.git", + # More digits than any Repository.id, which int() would refuse with a 500 at 4,301. + f"/api/0/internal/scm-git/repositories/{'4' * 20}.git/info/refs", + # Arabic-Indic digits, which int() would read as 42. + "/api/0/internal/scm-git/repositories/\u0664\u0662.git/info/refs", ], ids=[ - "unknown-provider", - "no-provider", - "slash-in-external-id", - "empty-external-id", + "name", "no-git-suffix", "nothing-after-git", "no-slash-after-git", + "20-digits", + "arabic-indic", ], ) def test_does_not_route(path: str) -> None: diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 8d49e8f14efa..11e8b47be82c 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -12,10 +12,8 @@ from sentry.scm.private.helpers import ( fetch_access_token, fetch_repository, - fetch_repository_by_external_id, fetch_service_provider, make_fetchers, - make_git_fetchers, ) from sentry.testutils.cases import TestCase from sentry.testutils.silo import cell_silo_test @@ -438,45 +436,6 @@ def test_github_enterprise_client_error_returns_none(self) -> None: assert fetch_service_provider(self.organization.id, repository) is None -class TestFetchRepositoryByExternalId(TestCase): - def setUp(self) -> None: - super().setUp() - self.repo = self.create_repo( - self.project, - name="test-org/test-repo", - provider="integrations:github", - external_id="99999", - integration_id=1, - ) - - def test_returns_the_repository(self) -> None: - result = fetch_repository_by_external_id(self.organization.id, ("github", "99999")) - - assert result is not None - assert result["id"] == self.repo.id - assert result["name"] == "test-org/test-repo" - - def test_does_not_match_a_name(self) -> None: - result = fetch_repository_by_external_id( - self.organization.id, ("github", "test-org/test-repo") - ) - - assert result is None - - def test_does_not_match_sentrys_id(self) -> None: - assert fetch_repository_by_external_id(self.organization.id, self.repo.id) is None - - def test_does_not_match_another_provider(self) -> None: - result = fetch_repository_by_external_id(self.organization.id, ("gitlab", "99999")) - - assert result is None - - def test_does_not_match_another_organizations_repository(self) -> None: - result = fetch_repository_by_external_id(self.create_organization().id, ("github", "99999")) - - assert result is None - - class TestMakeFetchers(TestCase): def test_fetchers_fetch_the_integration_once(self) -> None: integration = self.create_integration( @@ -546,39 +505,6 @@ def test_fetchers_fetch_a_gitlab_integration_once(self) -> None: assert isinstance(provider, GitLabProvider) assert service.get_integration.call_count == 1 - def test_git_fetchers_fetch_the_integration_once(self) -> None: - integration = self.create_integration( - organization=self.organization, - provider="github_enterprise", - name="GHE Acme", - external_id="ghe-fetchers-1", - metadata={ - "domain_name": "github.acme.com", - "installation_id": "1", - "installation": {"id": "1", "private_key": "x", "verify_ssl": True}, - }, - ) - self.create_repo( - project=self.project, - name="acme/widget", - provider="integrations:github_enterprise", - external_id="9001", - integration_id=integration.id, - ) - fetchers = make_git_fetchers() - - with patch( - "sentry.scm.private.helpers.integration_service", wraps=integration_service - ) as service: - repository = fetchers.fetch_repository( - self.organization.id, ("github_enterprise", "9001") - ) - assert repository is not None - provider = fetchers.fetch_provider(self.organization.id, repository) - - assert isinstance(provider, GitHubProvider) - assert service.get_integration.call_count == 1 - @cell_silo_test class TestFetchAccessToken(TestCase): From 013c3f23937c6386e0df41007289b0f61dfd9be1 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 29 Oct 2026 10:02:59 +0100 Subject: [PATCH 23/30] test(scm): Check that the git endpoint continues the caller's trace Seer and the credential proxy send sentry-trace with every git request, without baggage, so a fetch or push should join the trace of the Seer step that ran it. The SDK's WSGI middleware does this already, and nothing on the endpoint's path starts a trace of its own. Sentry and getsentry don't set strict_trace_continuation, which would refuse a trace that comes without baggage. Tests run without an SDK client, so nothing showed it before. The test serves a fetch inside the middleware with a client configured like Sentry's. The provider request's span is in the caller's trace, under the caller's span, and is sampled as the caller's flag says or by Sentry's sampler without one. sentry-trace isn't passed on to the provider. --- tests/sentry/scm/endpoints/test_scm_git.py | 96 +++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index e651f5274790..7dd4c706bf5b 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -2,27 +2,36 @@ import base64 import time -from collections.abc import Sequence +from collections.abc import Callable, Sequence +from typing import Any from unittest import mock import pytest import responses +import sentry_sdk from django.conf import settings +from django.core.signals import request_finished +from django.db import close_old_connections from django.http import HttpResponseBase -from django.test import override_settings +from django.test import RequestFactory, override_settings +from django.test.client import ClientHandler from django.urls import resolve, reverse +from requests import PreparedRequest from responses import matchers from scm import Secret from scm.git.grant import sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import RepositoryId +from sentry_sdk import traces from sentry_sdk.data_collection import _apply_key_value_collection_filtering +from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware from sentry.constants import ObjectStatus from sentry.scm.endpoints.scm_git import report_git_error from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import cell_silo_test +from sentry.utils.sdk import traces_sampler SECRET = "a-long-value-that-is-hard-to-guess" TOKEN = "ghs_installation_token" @@ -153,6 +162,89 @@ def test_relays_a_push_body(self) -> None: assert b"".join(response) == b"000eunpack ok\n0000" assert b"".join(responses.calls[0].request.body) == b"0000PACK-bytes" + @responses.activate + @override_settings(SENTRY_BACKEND_APM_SAMPLING=0) + def test_continues_the_callers_trace(self) -> None: + # Seer and the credential proxy send sentry-trace with every git request, without baggage, + # so a fetch or push joins the trace of the step that ran it. The flag is the caller's + # sampling decision, and the endpoint's own sample rate decides without one. + incoming_trace, incoming_span = "771a43a4192642f0b136d5159a501700", "b0e6f15b45c36b12" + for flag, sampled in (("-1", True), ("-0", False), ("", False)): + seen: list[traces.StreamedSpan] = [] + + def advertise(request: PreparedRequest) -> tuple[int, dict[str, str], bytes]: + current = traces.get_current_span() + assert isinstance(current, traces.StreamedSpan) + seen.append(current) + return 200, {}, b"001e# service=git-upload-pack\n0000" + + responses.reset() + responses.add_callback( + responses.GET, + "https://github.com/getsentry/sentry.git/info/refs", + callback=advertise, + content_type="application/x-git-upload-pack-advertisement", + ) + + status, body = self.serve_through_sdk( + "info/refs", + "service=git-upload-pack", + { + GRANT_HEADER: self.fetch_grant(), + "sentry-trace": f"{incoming_trace}-{incoming_span}{flag}", + }, + ) + + assert (status, body) == ("200 OK", b"001e# service=git-upload-pack\n0000") + [span] = seen + assert span.trace_id == incoming_trace + assert span._segment._parent_span_id == incoming_span + assert span._segment.sampled is sampled + # Sentry's trace stays in Sentry. + assert "sentry-trace" not in responses.calls[0].request.headers + + def serve_through_sdk( + self, route: str, query: str, headers: dict[str, str] + ) -> tuple[str, bytes]: + """Serve a request the way a deployed Sentry does, inside the SDK's WSGI middleware. + + Tests run without an SDK client, so this one starts a streaming client like + sentry.utils.sdk's and takes it back afterwards. + """ + url = reverse( + "sentry-api-0-scm-git", kwargs={"repository_id": self.repo.id, "route": route} + ) + environ = RequestFactory().get(f"{url}?{query}", headers=headers).environ + handler = ClientHandler() + + def app(environ: dict[str, Any], start_response: Callable[..., Any]) -> HttpResponseBase: + response = handler(environ) + start_response(f"{response.status_code} {response.reason_phrase}", response.items()) + return response + + statuses: list[str] = [] + previous_client = sentry_sdk.get_client() + sentry_sdk.init( + default_integrations=False, + auto_enabling_integrations=False, + trace_lifecycle="stream", + traces_sampler=traces_sampler, + ) + try: + body = SentryWsgiMiddleware(app)(environ, lambda status, _: statuses.append(status)) + try: + return statuses[0], b"".join(body) + finally: + # A WSGI server closes the body, which sends request_finished. As in Django's test + # client, that mustn't close the test's database connection. + request_finished.disconnect(close_old_connections) + try: + body.close() + finally: + request_finished.connect(close_old_connections) + finally: + sentry_sdk.get_global_scope().set_client(previous_client) + @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") From 60c8bb1e7bc40738e260c5fc15880c9d36e895df Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 29 Oct 2026 22:23:05 +0100 Subject: [PATCH 24/30] fix(scm): Hand the git endpoint's provider token over as a Secret scm-platform now requires get_token to return an scm.Secret, so the token shows as [Filtered] in the frames of an error report instead of in plain text. GitServer refuses a plain string with a TypeError, which failed every relayed request against the unreleased 3.0.0 code. --- src/sentry/scm/private/helpers.py | 8 +++++--- tests/sentry/scm/integration/test_helpers_integration.py | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index e8c7994d3adb..597dd07f5f7b 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -4,6 +4,7 @@ import sentry_sdk from django.db.models import Q +from scm import Secret from scm.errors import SCMCodedError, UnhandledException from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider @@ -86,12 +87,13 @@ def fetch_service_provider( return None -def fetch_access_token(repository: Repository, operation: GitOperation) -> str | None: +def fetch_access_token(repository: Repository, operation: GitOperation) -> Secret | None: """Return a token that can run git against the repository, minted or refreshed in control. The token is the integration's: an installation token for GitHub and Cursor Origin, or the OAuth token of the user who installed GitLab. It isn't scoped to the repository or the operation, so a - fetch gets the same token as a push. + fetch gets the same token as a push. It's wrapped in a Secret right away, so the frames an + error report carries show it as [Filtered]. """ try: access_token = integration_service.get_access_token( @@ -104,7 +106,7 @@ def fetch_access_token(repository: Repository, operation: GitOperation) -> str | # "HTTP 500"; GitServer turns None into a 502 that says why. sentry_sdk.capture_exception() return None - return access_token.token if access_token is not None else None + return Secret(access_token.token) if access_token is not None else None def fetch_repository( diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 11e8b47be82c..3788ad7c29d2 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -531,7 +531,8 @@ def test_returns_the_installation_token(self) -> None: token = fetch_access_token(self.scm_repository(integration.id), "fetch") - assert token == TOKEN + assert token is not None + assert token.reveal() == TOKEN def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> None: integration = self.create_integration( From 502855969207015b2e8e0acf8cafbb4a336460f8 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 29 Oct 2026 22:23:16 +0100 Subject: [PATCH 25/30] feat(scm): Verify git grants with Ed25519 public keys Seer signs git grants and Sentry only verifies them. With a shared HS256 secret, anything that can read Sentry's settings could mint a grant for any connected repository. scm-platform 3.0.0 signs grants with Ed25519 instead: Seer holds the only private key, and Sentry lists the public keys, which aren't secret, as it already does for objectstore tokens and Relay. SCM_GIT_GRANT_SECRETS becomes SCM_GIT_GRANT_PUBLIC_KEYS. An empty or unusable key list is now answered with a 503 that names the setting, since scm-platform raises on it rather than refusing the grant, and it is Sentry's misconfiguration rather than a bad grant. --- src/sentry/conf/server.py | 10 +-- src/sentry/scm/endpoints/scm_git.py | 29 ++++--- tests/sentry/scm/endpoints/test_scm_git.py | 94 +++++++++++++++++++--- 3 files changed, 107 insertions(+), 26 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 5eda986c0582..badf7b2d4872 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -801,11 +801,11 @@ def SOCIAL_AUTH_DEFAULT_USERNAME() -> str: # First element is used to sign requests; request is accepted if signed with any element in the list. SCM_RPC_SHARED_SECRET: list[str] | None = None -# Verifies the grants Seer signs for the SCM git endpoint. Seer signs with the first element; a grant -# is accepted if signed with any element in the list. Only Sentry and Seer may hold these, since a -# holder can mint a git grant for any organization's repositories. Never reuse a secret that signs -# anything else, such as SEER_API_SHARED_SECRET: whatever holds it could then mint grants too. -SCM_GIT_GRANT_SECRETS: list[str] | None = None +# Ed25519 public keys, as SubjectPublicKeyInfo PEM strings, that verify the grants Seer signs for +# the SCM git endpoint. Only Seer holds a private key, so these aren't secret. A grant is accepted +# if any listed key verifies it. To rotate, add the new public key here, switch Seer to the new +# private key, wait 15 minutes for its old grants to expire, then remove the old public key. +SCM_GIT_GRANT_PUBLIC_KEYS: list[str] = [] # Shared secret used to sign cross-region RPC requests from the launchpad microservice. LAUNCHPAD_RPC_SHARED_SECRET: list[str] | None = None diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 6b360a4e34fc..e2f0d9fab8f8 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -6,9 +6,8 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request -from scm import Secret from scm.errors import SCMCodedError -from scm.git.grant import verify_grant +from scm.git.grant import require_public_keys, verify_grant from scm.git.relay import GitRelay from scm.git.server import GitServer from scm.git.types import GRANT_HEADER, Readable @@ -43,7 +42,7 @@ def report_git_error(error: Exception, context: dict[str, str]) -> None: def make_git_server() -> GitServer: fetchers = make_fetchers() return GitServer( - secrets=_grant_secrets(), + public_keys=_grant_public_keys(), fetch_repository=fetchers.fetch_repository, fetch_provider=fetchers.fetch_provider, get_token=fetch_access_token, @@ -88,12 +87,24 @@ def post(self, request: Request, repository_id: str, route: str) -> HttpResponse def _relay(self, request: Request, repository_id: int, route: str) -> HttpResponseBase: # The grant names the organization whose flag gates the endpoint, and it's verified before # the organization is looked up, so a request without a valid grant never reaches the - # database. GitServer verifies the JWT again, which costs one HMAC-SHA256. With no secret - # configured, every grant is refused here as unknown, before GitServer, which can't be built - # without one. + # database. GitServer verifies the JWT again, which costs one Ed25519 signature check. context = {"repository_id": str(repository_id)} + public_keys = _grant_public_keys() try: - grant = verify_grant(_grant_secrets(), request.headers.get(GRANT_HEADER, "")) + # verify_grant and GitServer raise on a key list no grant could verify against, which + # is Sentry's misconfiguration rather than the grant's fault, so it isn't a 403. + require_public_keys(public_keys) + except (ValueError, TypeError) as e: + # scm-platform's messages name a key by its place in the list, never its contents. + return _refuse( + "grant_public_keys_invalid", + 503, + f"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured: {e}", + context, + tags={}, + ) + try: + grant = verify_grant(public_keys, request.headers.get(GRANT_HEADER, "")) except SCMCodedError as e: return _refuse(e.code, 403, e.detail, context, tags={}) @@ -143,8 +154,8 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon ) -def _grant_secrets() -> list[Secret]: - return [Secret(secret) for secret in settings.SCM_GIT_GRANT_SECRETS or []] +def _grant_public_keys() -> list[str]: + return settings.SCM_GIT_GRANT_PUBLIC_KEYS def _request_body(request: HttpRequest) -> Readable | None: diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 7dd4c706bf5b..cabe36245133 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -3,12 +3,19 @@ import base64 import time from collections.abc import Callable, Sequence -from typing import Any +from typing import Any, NamedTuple from unittest import mock import pytest import responses import sentry_sdk +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from cryptography.hazmat.primitives.serialization import ( + Encoding, + NoEncryption, + PrivateFormat, + PublicFormat, +) from django.conf import settings from django.core.signals import request_finished from django.db import close_old_connections @@ -33,7 +40,6 @@ from sentry.testutils.silo import cell_silo_test from sentry.utils.sdk import traces_sampler -SECRET = "a-long-value-that-is-hard-to-guess" TOKEN = "ghs_installation_token" GITLAB_TOKEN = "gitlab_oauth_token" FEATURE = "organizations:scm-git-endpoint" @@ -42,13 +48,31 @@ TOKEN_EXPIRES_AT = "3000-01-01T00:00:00" +class KeyPair(NamedTuple): + private: Secret + """The private key in PEM, as Seer signs grants with it.""" + public: str + """The public key in PEM, as SCM_GIT_GRANT_PUBLIC_KEYS lists it.""" + + +def make_key_pair() -> KeyPair: + # Generated, so no private key is committed. + key = Ed25519PrivateKey.generate() + private_pem = key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption()) + public_pem = key.public_key().public_bytes(Encoding.PEM, PublicFormat.SubjectPublicKeyInfo) + return KeyPair(private=Secret(private_pem.decode()), public=public_pem.decode()) + + +KEY = make_key_pair() + + def sign( organization_id: int, operation: GitOperation, repositories: Sequence[RepositoryId], *, expires_in: int = 300, - secret: str = SECRET, + key: KeyPair = KEY, ) -> str: grant = Grant( organization_id=organization_id, @@ -57,11 +81,11 @@ def sign( expires_at=int(time.time()) + expires_in, referrer=REFERRER, ) - return sign_grant(Secret(secret), grant) + return sign_grant(key.private, grant) @cell_silo_test -@override_settings(SCM_GIT_GRANT_SECRETS=[SECRET]) +@override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[KEY.public]) @with_feature(FEATURE) class ScmGitEndpointTest(TestCase): def setUp(self) -> None: @@ -275,28 +299,74 @@ def test_refuses_a_request_without_a_grant( @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") - def test_refuses_a_grant_signed_with_an_unknown_secret_without_its_referrer( + def test_refuses_a_grant_signed_with_an_unknown_key_without_its_referrer( self, mock_record_count: mock.MagicMock, mock_sdk: mock.MagicMock ) -> None: # Anyone can write a referrer into a grant they sign themselves, so only a verified one # may tag metrics and issues. - grant = sign(self.organization.id, "fetch", [self.repo.id], secret="some-other-secret") + grant = sign(self.organization.id, "fetch", [self.repo.id], key=make_key_pair()) response = self.fetch_refs(grant) - self.assert_refused(response, 403, b"not signed with a known secret") + self.assert_refused(response, 403, b"not signed with a known key") mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "git_invalid_grant"} ) - self.assert_reported_message(mock_sdk, "The git grant was not signed with a known secret.") + self.assert_reported_message(mock_sdk, "The git grant was not signed with a known key.") self.assert_reported(mock_sdk, "git_invalid_grant", {"repository_id": str(self.repo.id)}) @responses.activate - @override_settings(SCM_GIT_GRANT_SECRETS=None) - def test_refuses_every_grant_when_no_secret_is_configured(self) -> None: + def test_accepts_a_grant_signed_with_any_listed_key(self) -> None: + # During a rotation Sentry lists the old and the new public key, and Seer may sign with + # either. + new_key = make_key_pair() + responses.add( + responses.GET, + "https://github.com/getsentry/sentry.git/info/refs", + body=b"0000", + content_type="application/x-git-upload-pack-advertisement", + ) + + with override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[KEY.public, new_key.public]): + response = self.fetch_refs( + sign(self.organization.id, "fetch", [self.repo.id], key=new_key) + ) + + assert response.status_code == 200 + + @responses.activate + @override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[]) + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") + @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") + def test_refuses_every_grant_when_no_public_key_is_configured( + self, mock_record_count: mock.MagicMock, mock_sdk: mock.MagicMock + ) -> None: response = self.fetch_refs(self.fetch_grant()) - self.assert_refused(response, 403, b"not signed with a known secret") + self.assert_refused(response, 503, b"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured") + mock_record_count.assert_called_once_with( + "sentry.scm.git.request.failed", 1, {"code": "grant_public_keys_invalid"} + ) + self.assert_reported( + mock_sdk, "grant_public_keys_invalid", {"repository_id": str(self.repo.id)} + ) + + @responses.activate + @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") + def test_refuses_every_grant_without_quoting_a_misplaced_private_key( + self, mock_sdk: mock.MagicMock + ) -> None: + private_pem = KEY.private.reveal() + + with override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[private_pem]): + response = self.fetch_refs(self.fetch_grant()) + + self.assert_refused(response, 503, b"public_keys[0] is a private key") + body = b"".join(response).decode() + (detail,), _ = mock_sdk.capture_message.call_args + key_body = private_pem.splitlines()[1] + assert key_body not in body + assert key_body not in detail @responses.activate def test_refuses_an_expired_grant(self) -> None: From e12c67214341cf7261a66aac3f70aad409c13779 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 29 Oct 2026 22:25:24 +0100 Subject: [PATCH 26/30] ref(scm): Let GitServer refuse a provider host on an internal address scm-platform's GitRelay now takes map_error, so BlacklistAdapter's RestrictedIPAddress becomes its git_remote_refused error. GitServer then counts, reports and answers it like any other refusal, with the operation, provider and host in the report, and the endpoint no longer needs its own handler. The answer moves from a 503 to a 502: retrying a refused host never helps. --- src/sentry/scm/endpoints/scm_git.py | 42 ++++++++++------------ tests/sentry/scm/endpoints/test_scm_git.py | 34 ++++++++++++------ 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index e2f0d9fab8f8..986adc3dacbd 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -6,7 +6,7 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request -from scm.errors import SCMCodedError +from scm.errors import GitRemoteRefused, SCMCodedError from scm.git.grant import require_public_keys, verify_grant from scm.git.relay import GitRelay from scm.git.server import GitServer @@ -22,7 +22,6 @@ from sentry.models.organization import Organization from sentry.net.http import BlacklistAdapter from sentry.scm.private.helpers import fetch_access_token, make_fetchers, record_count_metric -from sentry.shared_integrations.exceptions import ApiHostError from sentry.utils.http import request_body_stream logger = logging.getLogger(__name__) @@ -50,10 +49,19 @@ def make_git_server() -> GitServer: emit_error=report_git_error, # A GitHub Enterprise or self-managed GitLab host is whatever the customer configured, so # it may resolve to one of our own addresses. - relay=GitRelay(adapter=BlacklistAdapter), + relay=GitRelay(adapter=BlacklistAdapter, map_error=_map_relay_error), ) +def _map_relay_error(error: Exception) -> SCMCodedError | None: + # BlacklistAdapter refuses an internal address with Sentry's own exception, which scm-platform + # doesn't know. Mapped, GitServer counts, reports and answers it like its other refusals, and + # the report leaves out the address and the urllib3 frames that hold the token. + if isinstance(error, RestrictedIPAddress): + return GitRemoteRefused() + return None + + @internal_cell_silo_endpoint class ScmGitEndpoint(Endpoint): """ @@ -129,26 +137,14 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon tags, ) - try: - response = make_git_server().handle( - method=request.method or "", - repository_id=repository_id, - route=route, - query=request.META.get("QUERY_STRING", ""), - headers=dict(request.headers), - body=_request_body(request._request), - ) - except RestrictedIPAddress as e: - # The provider host resolves to an internal address. GitServer counts that as an - # unhandled_exception and doesn't report it, so the report is what names the repository. - # Neither the report nor the client's 503 carries the address, like the ApiHostError - # Sentry's API clients raise. - _report_failure( - "restricted_ip_address", - "The provider host resolves to an address Sentry doesn't connect to.", - context, - ) - return _plain_text(ApiHostError.from_exception(e).text, 503) + response = make_git_server().handle( + method=request.method or "", + repository_id=repository_id, + route=route, + query=request.META.get("QUERY_STRING", ""), + headers=dict(request.headers), + body=_request_body(request._request), + ) return StreamingHttpResponse( response.content, status=response.status_code, headers=response.headers ) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index cabe36245133..4ced363dd02b 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -601,8 +601,12 @@ def test_refuses_an_inactive_repository(self) -> None: @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") @mock.patch("sentry.scm.endpoints.scm_git.logger") - def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( - self, mock_logger: mock.MagicMock, mock_sdk: mock.MagicMock + @mock.patch("sentry.scm.endpoints.scm_git.record_count_metric") + def test_refuses_a_provider_host_on_an_internal_address( + self, + mock_record_count: mock.MagicMock, + mock_logger: mock.MagicMock, + mock_sdk: mock.MagicMock, ) -> None: integration = self.create_integration( organization=self.organization, @@ -627,29 +631,39 @@ def test_answers_a_provider_host_on_an_internal_address_with_a_host_error( response = self.fetch_refs(grant, repo.id) - assert response.status_code == 503 + detail = "The repository's git remote is at an address Sentry does not connect to." + assert response.status_code == 502 assert response["Content-Type"] == "text/plain; charset=utf-8" - assert b"".join(response) == b"Unable to reach host\n" + assert b"".join(response) == f"{detail}\n".encode() + mock_record_count.assert_called_once_with( + "sentry.scm.git.request.failed", + 1, + {"code": "git_remote_refused", "referrer": REFERRER}, + ) mock_logger.error.assert_called_once_with( "scm.git.request_failed", extra={ - "code": "restricted_ip_address", - "detail": "The provider host resolves to an address Sentry doesn't connect to.", + "code": "git_remote_refused", + "detail": detail, "repository_id": str(repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, + "operation": "fetch", + "provider": "github_enterprise", + "remote_host": "127.0.0.1", }, ) - self.assert_reported_message( - mock_sdk, "The provider host resolves to an address Sentry doesn't connect to." - ) + self.assert_reported_message(mock_sdk, detail) self.assert_reported( mock_sdk, - "restricted_ip_address", + "git_remote_refused", { "repository_id": str(repo.id), "organization_id": str(self.organization.id), "referrer": REFERRER, + "operation": "fetch", + "provider": "github_enterprise", + "remote_host": "127.0.0.1", }, ) From d3e77aac8cde238d8bad8b5f8070649991b1c9fe Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 08:25:34 +0100 Subject: [PATCH 27/30] fix(scm): Keep the git endpoint's 503s for failures Seer can retry Seer retries a 503 during checkout, so a 503 has to mean a retry can work. A misconfigured SCM_GIT_GRANT_PUBLIC_KEYS never fixes itself, so it's now a 500, like any other Sentry bug or misconfiguration. Control being unreachable while Sentry fetches a provider token usually passes, so it's now a 503 rather than the 502 GitServer gives a missing token. --- src/sentry/scm/endpoints/scm_git.py | 7 ++++--- src/sentry/scm/private/helpers.py | 15 +++++++++++---- tests/sentry/scm/endpoints/test_scm_git.py | 18 ++++++++++++++++-- .../integration/test_helpers_integration.py | 18 ++++++++++++------ 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 986adc3dacbd..db3135d9366c 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -99,14 +99,15 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon context = {"repository_id": str(repository_id)} public_keys = _grant_public_keys() try: - # verify_grant and GitServer raise on a key list no grant could verify against, which - # is Sentry's misconfiguration rather than the grant's fault, so it isn't a 403. + # verify_grant and GitServer raise on a key list no grant could verify against. That's + # Sentry's misconfiguration, not the grant's fault, so it's a 500 rather than a 403, + # and not a 503, which Seer retries. require_public_keys(public_keys) except (ValueError, TypeError) as e: # scm-platform's messages name a key by its place in the list, never its contents. return _refuse( "grant_public_keys_invalid", - 503, + 500, f"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured: {e}", context, tags={}, diff --git a/src/sentry/scm/private/helpers.py b/src/sentry/scm/private/helpers.py index 597dd07f5f7b..d923ed38fbb9 100644 --- a/src/sentry/scm/private/helpers.py +++ b/src/sentry/scm/private/helpers.py @@ -5,7 +5,7 @@ import sentry_sdk from django.db.models import Q from scm import Secret -from scm.errors import SCMCodedError, UnhandledException +from scm.errors import ResourceServiceUnavailable, SCMCodedError, UnhandledException from scm.git.types import GitOperation from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider @@ -94,6 +94,10 @@ def fetch_access_token(repository: Repository, operation: GitOperation) -> Secre token of the user who installed GitLab. It isn't scoped to the repository or the operation, so a fetch gets the same token as a push. It's wrapped in a Secret right away, so the frames an error report carries show it as [Filtered]. + + Raises ResourceServiceUnavailable if control can't be reached, which GitServer answers with a + 503 that Seer retries. None, which GitServer answers with a 502, means control answered but + has no token for the repository. """ try: access_token = integration_service.get_access_token( @@ -102,10 +106,13 @@ def fetch_access_token(repository: Repository, operation: GitOperation) -> Secre ) except (RpcRemoteException, RpcResponseException): # Control couldn't be reached, or answered with something other than an RPC response, - # such as a load balancer's error page. Answered as a 500, git would show only - # "HTTP 500"; GitServer turns None into a 502 that says why. + # such as a load balancer's error page. Both usually pass, so the request is worth + # retrying. Answered as an uncaught exception, it would be a 500 that git shows only as + # "HTTP 500". sentry_sdk.capture_exception() - return None + raise ResourceServiceUnavailable( + detail="Sentry could not reach its control silo for a provider token. The request can be retried." + ) from None return Secret(access_token.token) if access_token is not None else None diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 4ced363dd02b..ed1c7544d1d3 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -34,6 +34,7 @@ from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware from sentry.constants import ObjectStatus +from sentry.hybridcloud.rpc.service import RpcRemoteException from sentry.scm.endpoints.scm_git import report_git_error from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature @@ -343,7 +344,7 @@ def test_refuses_every_grant_when_no_public_key_is_configured( ) -> None: response = self.fetch_refs(self.fetch_grant()) - self.assert_refused(response, 503, b"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured") + self.assert_refused(response, 500, b"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured") mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "grant_public_keys_invalid"} ) @@ -361,7 +362,7 @@ def test_refuses_every_grant_without_quoting_a_misplaced_private_key( with override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[private_pem]): response = self.fetch_refs(self.fetch_grant()) - self.assert_refused(response, 503, b"public_keys[0] is a private key") + self.assert_refused(response, 500, b"public_keys[0] is a private key") body = b"".join(response).decode() (detail,), _ = mock_sdk.capture_message.call_args key_body = private_pem.splitlines()[1] @@ -591,6 +592,19 @@ def test_refuses_a_route_with_a_trailing_slash(self) -> None: self.assert_refused(response, 404, b"is not a git fetch or push route.") + @responses.activate + @mock.patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") + @mock.patch( + "sentry.scm.private.helpers.integration_service.get_access_token", + side_effect=RpcRemoteException("integration", "get_access_token", "timed out"), + ) + def test_answers_with_a_retryable_503_when_control_cannot_be_reached( + self, mock_get_access_token: mock.MagicMock, mock_capture_exception: mock.MagicMock + ) -> None: + response = self.fetch_refs(self.fetch_grant()) + + self.assert_refused(response, 503, b"The request can be retried.") + @responses.activate def test_refuses_an_inactive_repository(self) -> None: self.repo.update(status=ObjectStatus.DISABLED) diff --git a/tests/sentry/scm/integration/test_helpers_integration.py b/tests/sentry/scm/integration/test_helpers_integration.py index 3788ad7c29d2..9cbdf9f1dccd 100644 --- a/tests/sentry/scm/integration/test_helpers_integration.py +++ b/tests/sentry/scm/integration/test_helpers_integration.py @@ -1,5 +1,7 @@ from unittest.mock import MagicMock, patch +import pytest +from scm.errors import ResourceServiceUnavailable from scm.providers.cursor_origin.provider import CursorOriginProvider from scm.providers.github.provider import GitHubProvider from scm.providers.gitlab.provider import GitLabProvider @@ -551,12 +553,14 @@ def test_returns_none_for_an_integration_the_organization_does_not_have(self) -> "sentry.scm.private.helpers.integration_service.get_access_token", side_effect=RpcRemoteException("integration", "get_access_token", "timed out"), ) - def test_returns_none_when_control_cannot_be_reached( + def test_is_retryable_when_control_cannot_be_reached( self, mock_get_access_token: MagicMock, mock_capture_exception: MagicMock ) -> None: - token = fetch_access_token(self.scm_repository(1), "fetch") + with pytest.raises(ResourceServiceUnavailable) as e: + fetch_access_token(self.scm_repository(1), "fetch") - assert token is None + # Not chained: the RPC exception's frames are reported once, by capture_exception. + assert e.value.__suppress_context__ mock_capture_exception.assert_called_once_with() @patch("sentry.scm.private.helpers.sentry_sdk.capture_exception") @@ -564,10 +568,12 @@ def test_returns_none_when_control_cannot_be_reached( "sentry.scm.private.helpers.integration_service.get_access_token", side_effect=RpcResponseException("integration", "get_access_token", "not JSON"), ) - def test_returns_none_when_control_answers_with_something_else( + def test_is_retryable_when_control_answers_with_something_else( self, mock_get_access_token: MagicMock, mock_capture_exception: MagicMock ) -> None: - token = fetch_access_token(self.scm_repository(1), "fetch") + with pytest.raises(ResourceServiceUnavailable) as e: + fetch_access_token(self.scm_repository(1), "fetch") - assert token is None + # Not chained: the RPC exception's frames are reported once, by capture_exception. + assert e.value.__suppress_context__ mock_capture_exception.assert_called_once_with() From 49d0b94e33f4a8033b4062abfc131eceba75a85e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 08:25:37 +0100 Subject: [PATCH 28/30] feat(scm): Trust the dev Seer's git grant key in local development A local Seer signs git grants with a dev-only private key committed to its .envrc. Listing the matching public key in Sentry's dev settings lets the two talk without each developer generating a key pair. Since that private key is public, only the dev guard sets it, and a test checks that the production default stays empty. --- src/sentry/conf/server.py | 7 +++++ tests/sentry/conf/test_server.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 tests/sentry/conf/test_server.py diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index badf7b2d4872..92b76e3f3a1f 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -3623,6 +3623,13 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: if SILO_DEVSERVER or IS_DEV: LAUNCHPAD_RPC_SHARED_SECRET = ["launchpad-also-very-long-value-haha"] + # Dev only: verifies the grants a local Seer signs. Its private half is committed to Seer's + # .envrc, in a public repository, so anyone can sign grants this key accepts. + SCM_GIT_GRANT_PUBLIC_KEYS = [ + "-----BEGIN PUBLIC KEY-----\n" + "MCowBQYDK2VwAyEAq8SFj6uFiEB74zliRdruAG69shgf1FNJbj/UCnK0jFM=\n" + "-----END PUBLIC KEY-----\n" + ] if SILO_DEVSERVER: # Add connections for the region & control silo databases. diff --git a/tests/sentry/conf/test_server.py b/tests/sentry/conf/test_server.py new file mode 100644 index 000000000000..a64bb2610c10 --- /dev/null +++ b/tests/sentry/conf/test_server.py @@ -0,0 +1,48 @@ +import os +import subprocess +import sys + +import pytest + +from sentry.utils import json + +DEV_GRANT_PUBLIC_KEY = ( + "-----BEGIN PUBLIC KEY-----\n" + "MCowBQYDK2VwAyEAq8SFj6uFiEB74zliRdruAG69shgf1FNJbj/UCnK0jFM=\n" + "-----END PUBLIC KEY-----\n" +) + + +def scm_git_grant_public_keys(**environ: str) -> list[str]: + # The settings module reads the environment once, when it's imported, so each environment + # gets its own interpreter. + env = { + name: value + for name, value in os.environ.items() + if name not in ("SENTRY_ENVIRONMENT", "SENTRY_SILO_DEVSERVER") + } + result = subprocess.run( + [ + sys.executable, + "-c", + "import json, sentry.conf.server as s; print(json.dumps(s.SCM_GIT_GRANT_PUBLIC_KEYS))", + ], + env=env | environ, + capture_output=True, + text=True, + check=True, + ) + return json.loads(result.stdout) + + +def test_production_trusts_no_git_grant_key() -> None: + # Self-hosted inherits this default, and the dev key's private half is public. + assert scm_git_grant_public_keys() == [] + + +@pytest.mark.parametrize( + "environ", + [{"SENTRY_ENVIRONMENT": "development"}, {"SENTRY_SILO_DEVSERVER": "1"}], +) +def test_dev_trusts_the_key_local_seer_signs_with(environ: dict[str, str]) -> None: + assert scm_git_grant_public_keys(**environ) == [DEV_GRANT_PUBLIC_KEY] From 2681e6eb43c0dd92d40e5fed7e351c287ceded90 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 09:17:36 +0100 Subject: [PATCH 29/30] ref(scm): Adopt scm-platform's grant key error and key helpers scm-platform now raises GitGrantKeyError for every unusable grant key, so the endpoint catches that from verify_grant instead of checking the key list separately for ValueError and TypeError. Tests make keys with generate_private_key and public_key_of rather than copying the cryptography calls. A provider host on an internal address now answers 500, as scm-platform maps git_remote_refused: Seer retries a 502, and no retry reaches a host Sentry refuses to connect to. --- src/sentry/scm/endpoints/scm_git.py | 19 ++++++++----------- tests/sentry/scm/endpoints/test_scm_git.py | 17 ++++------------- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index db3135d9366c..61bb6305d651 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -6,8 +6,8 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse, HttpResponseBase, StreamingHttpResponse from rest_framework.request import Request -from scm.errors import GitRemoteRefused, SCMCodedError -from scm.git.grant import require_public_keys, verify_grant +from scm.errors import GitGrantKeyError, GitRemoteRefused, SCMCodedError +from scm.git.grant import verify_grant from scm.git.relay import GitRelay from scm.git.server import GitServer from scm.git.types import GRANT_HEADER, Readable @@ -97,14 +97,13 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon # the organization is looked up, so a request without a valid grant never reaches the # database. GitServer verifies the JWT again, which costs one Ed25519 signature check. context = {"repository_id": str(repository_id)} - public_keys = _grant_public_keys() try: - # verify_grant and GitServer raise on a key list no grant could verify against. That's - # Sentry's misconfiguration, not the grant's fault, so it's a 500 rather than a 403, - # and not a 503, which Seer retries. - require_public_keys(public_keys) - except (ValueError, TypeError) as e: - # scm-platform's messages name a key by its place in the list, never its contents. + grant = verify_grant(_grant_public_keys(), request.headers.get(GRANT_HEADER, "")) + except GitGrantKeyError as e: + # No grant could verify against the key list. That's Sentry's misconfiguration, not the + # grant's fault, so it's a 500 rather than a 403. Seer fails a checkout on a 500 at + # once, where it retries a 502 or 503, which no retry would fix. The message names a + # key by its place in the list, never its contents. return _refuse( "grant_public_keys_invalid", 500, @@ -112,8 +111,6 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon context, tags={}, ) - try: - grant = verify_grant(public_keys, request.headers.get(GRANT_HEADER, "")) except SCMCodedError as e: return _refuse(e.code, 403, e.detail, context, tags={}) diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index ed1c7544d1d3..30a5cddc8f3e 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -9,13 +9,6 @@ import pytest import responses import sentry_sdk -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey -from cryptography.hazmat.primitives.serialization import ( - Encoding, - NoEncryption, - PrivateFormat, - PublicFormat, -) from django.conf import settings from django.core.signals import request_finished from django.db import close_old_connections @@ -26,7 +19,7 @@ from requests import PreparedRequest from responses import matchers from scm import Secret -from scm.git.grant import sign_grant +from scm.git.grant import generate_private_key, public_key_of, sign_grant from scm.git.types import GRANT_HEADER, GitOperation, Grant from scm.types import RepositoryId from sentry_sdk import traces @@ -58,10 +51,8 @@ class KeyPair(NamedTuple): def make_key_pair() -> KeyPair: # Generated, so no private key is committed. - key = Ed25519PrivateKey.generate() - private_pem = key.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption()) - public_pem = key.public_key().public_bytes(Encoding.PEM, PublicFormat.SubjectPublicKeyInfo) - return KeyPair(private=Secret(private_pem.decode()), public=public_pem.decode()) + private_key = generate_private_key() + return KeyPair(private=private_key, public=public_key_of(private_key)) KEY = make_key_pair() @@ -646,7 +637,7 @@ def test_refuses_a_provider_host_on_an_internal_address( response = self.fetch_refs(grant, repo.id) detail = "The repository's git remote is at an address Sentry does not connect to." - assert response.status_code == 502 + assert response.status_code == 500 assert response["Content-Type"] == "text/plain; charset=utf-8" assert b"".join(response) == f"{detail}\n".encode() mock_record_count.assert_called_once_with( From 88b708673bbc3f242738e521707cb65f21309c7c Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 30 Oct 2026 11:09:52 +0100 Subject: [PATCH 30/30] fix(scm): Keep the grant key error's detail out of the git response The git endpoint answered a misconfigured key list with scm-platform's exception message. That message names a bad key by its place in the list without quoting it, but the client can't fix Sentry's settings, and CodeQL flags exception text reaching a response. The client now gets a fixed message, and the log and the Sentry issue keep the detail. --- src/sentry/scm/endpoints/scm_git.py | 18 +++++++++++++----- tests/sentry/scm/endpoints/test_scm_git.py | 18 +++++++++++------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/sentry/scm/endpoints/scm_git.py b/src/sentry/scm/endpoints/scm_git.py index 61bb6305d651..fd10bdb8750e 100644 --- a/src/sentry/scm/endpoints/scm_git.py +++ b/src/sentry/scm/endpoints/scm_git.py @@ -102,14 +102,15 @@ def _relay(self, request: Request, repository_id: int, route: str) -> HttpRespon except GitGrantKeyError as e: # No grant could verify against the key list. That's Sentry's misconfiguration, not the # grant's fault, so it's a 500 rather than a 403. Seer fails a checkout on a 500 at - # once, where it retries a 502 or 503, which no retry would fix. The message names a - # key by its place in the list, never its contents. + # once, where it retries a 502 or 503, which no retry would fix. Only the report says + # which key is wrong: the client can't fix Sentry's settings. return _refuse( "grant_public_keys_invalid", 500, f"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured: {e}", context, tags={}, + client_detail="Sentry's git grant keys are misconfigured.", ) except SCMCodedError as e: return _refuse(e.code, 403, e.detail, context, tags={}) @@ -166,13 +167,20 @@ def _request_body(request: HttpRequest) -> Readable | None: def _refuse( - code: str, status: int, detail: str, context: dict[str, str], tags: dict[str, str] + code: str, + status: int, + detail: str, + context: dict[str, str], + tags: dict[str, str], + *, + client_detail: str | None = None, ) -> HttpResponse: - # Counted and reported like the refusals GitServer answers itself. + # Counted and reported like the refusals GitServer answers itself. The client gets the detail + # too, unless client_detail replaces it. record_count_metric(FAILED_METRIC, 1, {"code": code} | tags) _report_failure(code, detail, context) # Never a 401: on a 401 git asks for credentials, which a coding job doesn't have. - return _plain_text(detail, status) + return _plain_text(detail if client_detail is None else client_detail, status) def _report_failure(code: str, detail: str, context: dict[str, str]) -> None: diff --git a/tests/sentry/scm/endpoints/test_scm_git.py b/tests/sentry/scm/endpoints/test_scm_git.py index 30a5cddc8f3e..17f35c2be3c4 100644 --- a/tests/sentry/scm/endpoints/test_scm_git.py +++ b/tests/sentry/scm/endpoints/test_scm_git.py @@ -335,17 +335,22 @@ def test_refuses_every_grant_when_no_public_key_is_configured( ) -> None: response = self.fetch_refs(self.fetch_grant()) - self.assert_refused(response, 500, b"SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured") + self.assert_refused(response, 500, b"Sentry's git grant keys are misconfigured.") mock_record_count.assert_called_once_with( "sentry.scm.git.request.failed", 1, {"code": "grant_public_keys_invalid"} ) + self.assert_reported_message( + mock_sdk, + "SCM_GIT_GRANT_PUBLIC_KEYS is misconfigured: " + "There are no git grant public keys, so no grant could verify.", + ) self.assert_reported( mock_sdk, "grant_public_keys_invalid", {"repository_id": str(self.repo.id)} ) @responses.activate @mock.patch("sentry.scm.endpoints.scm_git.sentry_sdk") - def test_refuses_every_grant_without_quoting_a_misplaced_private_key( + def test_reports_a_misplaced_private_key_without_quoting_it( self, mock_sdk: mock.MagicMock ) -> None: private_pem = KEY.private.reveal() @@ -353,12 +358,11 @@ def test_refuses_every_grant_without_quoting_a_misplaced_private_key( with override_settings(SCM_GIT_GRANT_PUBLIC_KEYS=[private_pem]): response = self.fetch_refs(self.fetch_grant()) - self.assert_refused(response, 500, b"public_keys[0] is a private key") - body = b"".join(response).decode() + # Only the report names the bad entry, and neither quotes it. + assert b"".join(response) == b"Sentry's git grant keys are misconfigured.\n" (detail,), _ = mock_sdk.capture_message.call_args - key_body = private_pem.splitlines()[1] - assert key_body not in body - assert key_body not in detail + assert "public_keys[0] is a private key" in detail + assert private_pem.splitlines()[1] not in detail @responses.activate def test_refuses_an_expired_grant(self) -> None: