From 389ac6b44e38ae6fa4a88c21b1e02c9eaf67cee8 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 27 Oct 2026 23:39:33 +0100 Subject: [PATCH] fix(git)!: Keep the provider token out of error reports GitServer.handle and GitRelay.forward held the provider token as a plain string. Sentry's SDK sends every frame's local variables with an error report, and Sentry's SDK configuration turns off the scrubber that would replace a local named `token`. So in Sentry's git endpoint (https://github.com/getsentry/sentry/pull/128444), every refusal after the token is fetched, such as an unreachable provider or a rejected token, would report it. get_token now returns an scm.Secret, which shows as [Filtered]. The relay reveals it only inline, for the ASCII check and HTTPBasicAuth. GitServer refuses a plain string with a TypeError, after dropping it from its frame: one that got past the type checker would otherwise fail every request in the relay and report the token. Breaking: get_token must wrap the token it returns, e.g. Secret(access_token.token). --- bin/git-server | 6 ++--- src/scm/git/relay.py | 11 +++++--- src/scm/git/server.py | 9 +++++++ src/scm/git/types.py | 8 ++++-- tests/unit/error_reports.py | 17 +++++++++++-- tests/unit/test_git_relay.py | 18 ++++++++------ tests/unit/test_git_server.py | 47 +++++++++++++++++++++++++++++------ 7 files changed, 91 insertions(+), 25 deletions(-) diff --git a/bin/git-server b/bin/git-server index 22962e1..831d567 100755 --- a/bin/git-server +++ b/bin/git-server @@ -174,7 +174,7 @@ def github_backend(path: str, creds: dict[str, str], ca_bundle: str | None) -> B repository_at(path), repository_at, fetch_provider, - lambda repository, operation: token_manager.installation_token, + lambda repository, operation: Secret(token_manager.installation_token), ) @@ -194,7 +194,7 @@ def gitlab_backend(path: str, creds: dict[str, str]) -> Backend: return GitLabProvider(client=client, organization_id=organization_id, repository=repository) return Backend( - "gitlab", repository_at(path), repository_at, fetch_provider, lambda repository, operation: access_token + "gitlab", repository_at(path), repository_at, fetch_provider, lambda repository, operation: Secret(access_token) ) @@ -477,7 +477,7 @@ class Checks: ) server.shutdown() - rejecting = self.start(get_token=lambda repository, operation: "not-a-valid-token") + rejecting = self.start(get_token=lambda repository, operation: Secret("not-a-valid-token")) result = self.git(fetch_grant, "ls-remote", f"{rejecting.url}/{self.backend.repository['name']}.git") self.check( "a rejected token fails without asking for credentials", diff --git a/src/scm/git/relay.py b/src/scm/git/relay.py index 1d60ed8..eab0d79 100644 --- a/src/scm/git/relay.py +++ b/src/scm/git/relay.py @@ -21,6 +21,7 @@ ) from scm.git.types import GitOperation, Grant, Readable from scm.rpc.types import StreamResponse +from scm.secret import Secret from scm.types import GitRemote, RepositoryId # The most bytes the relay reads at once, from the client's body or from the provider's response, so also the @@ -107,7 +108,9 @@ class GitRelay: GitHub Enterprise or self-managed GitLab host is whatever the customer configured, and the relay connects wherever it resolves. Sentry's ``BlacklistAdapter`` does this for its API clients, and the class itself can be passed. An error the adapter raises for a refused host reaches the caller unchanged, unless it is a requests or - urllib3 exception, which the relay replaces like any other so that it cannot carry the token. The adapter must + urllib3 exception, which the relay replaces like any other so that it cannot carry the token. One that reaches + the caller does carry it: an adapter refusing the host as it connects raises through urllib3's frames, which hold + the Authorization header, so report such an error without its frames' local variables. The adapter must not retry, because a retry cannot resend a body that has already been streamed. Without one, the relay uses a plain ``HTTPAdapter``, which checks nothing. @@ -132,7 +135,7 @@ def __init__( def forward( self, remote: GitRemote, - token: str, + token: Secret, *, method: str, route: str, @@ -154,7 +157,7 @@ def forward( # The token never goes to anything but a git route, whatever the caller checked. if (method, route, query) not in _ROUTES: raise GitRouteNotSupported(detail=f"{method} {route} is not a git fetch or push route.") - if not f"{remote['basic_auth_username']}:{token}".isascii(): + if not f"{remote['basic_auth_username']}:{token.reveal()}".isascii(): # requests would fail to encode the credentials with a UnicodeEncodeError that holds them. No provider # issues such a token, so the caller has a bug; the message leaves the token out. raise ValueError(f"The credentials for {remote['url']} contain characters that are not ASCII.") @@ -196,7 +199,7 @@ def forward( url, headers=upstream_headers, data=data, - auth=HTTPBasicAuth(remote["basic_auth_username"], token), + auth=HTTPBasicAuth(remote["basic_auth_username"], token.reveal()), stream=True, # Following a redirect would send the request somewhere other than the repository's remote. allow_redirects=False, diff --git a/src/scm/git/server.py b/src/scm/git/server.py index c8bdd0c..6f054cc 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -141,6 +141,15 @@ def handle( raise GitTokenUnavailable( detail=f"Sentry could not get a {repository['provider_name']} token for this repository." ) + if not isinstance(token, Secret): + # A plain string, from code the type checker does not see. Every frame from here to the provider + # would show it in an error report; dropped before refusing it, so this error's report does not. + token_type = type(token).__name__ + del token + raise TypeError( + f"get_token must return an scm.Secret, not {token_type}. Wrap the token in scm.Secret where you " + "read it." + ) response = self.relay.forward( remote, token, method=method, route=route, query=query, headers=headers, body=counted_body ) diff --git a/src/scm/git/types.py b/src/scm/git/types.py index fd503ac..8d734ee 100644 --- a/src/scm/git/types.py +++ b/src/scm/git/types.py @@ -3,6 +3,7 @@ import msgspec +from scm.secret import Secret from scm.types import Referrer, Repository, RepositoryId type GitOperation = Literal["fetch", "push"] @@ -48,8 +49,11 @@ class Grant(msgspec.Struct, frozen=True, kw_only=True, forbid_unknown_fields=Tru """The format version. A verifier refuses a version it does not know.""" -type GetToken = Callable[[Repository, GitOperation], str | None] -"""Return a provider token that can run ``operation`` on ``repository``, or None if there is none.""" +type GetToken = Callable[[Repository, GitOperation], Secret | None] +"""Return a provider token that can run ``operation`` on ``repository``, or None if there is none. + +Wrap the token in a ``Secret`` where you read it. GitServer and the relay hold it in their frames' local variables, +which an error report sends.""" class Readable(Protocol): diff --git a/tests/unit/error_reports.py b/tests/unit/error_reports.py index 1bb905e..af4a846 100644 --- a/tests/unit/error_reports.py +++ b/tests/unit/error_reports.py @@ -7,6 +7,19 @@ def assert_not_in_frames(error: BaseException, value: str) -> None: tb = error.__traceback__ while tb is not None: for name, local in tb.tb_frame.f_locals.items(): - shown = local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local) - assert value not in shown, f"{tb.tb_frame.f_code.co_name} holds it in {name}" + assert value not in _shown(local), f"{tb.tb_frame.f_code.co_name} holds it in {name}" tb = tb.tb_next + + +def reported_local(error: BaseException, function: str, name: str) -> str: + """How an error report shows the local variable ``name`` of ``function``'s frame in the error's traceback.""" + tb = error.__traceback__ + while tb is not None: + if tb.tb_frame.f_code.co_name == function: + return _shown(tb.tb_frame.f_locals[name]) + tb = tb.tb_next + raise AssertionError(f"The traceback has no frame of {function}.") + + +def _shown(local: object) -> str: + return local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local) diff --git a/tests/unit/test_git_relay.py b/tests/unit/test_git_relay.py index 6f54b98..8a853a2 100644 --- a/tests/unit/test_git_relay.py +++ b/tests/unit/test_git_relay.py @@ -23,11 +23,13 @@ SCMCodedError, ) from scm.git.relay import GitRelay, _UpstreamStream, authorize_request +from scm.secret import Secret from scm.types import GitRemote +from tests.unit.error_reports import assert_not_in_frames, reported_local from tests.unit.git_builders import make_grant from tests.unit.git_upstream import ClosingAdapter, UpstreamHandler, basic_auth, ca_bundle, running_upstream -TOKEN = "ghs_secrettoken" +TOKEN = Secret("ghs_secrettoken") # Long enough that a relay that buffers fails the test instead of hanging it. WAIT = 5 @@ -78,7 +80,7 @@ def test_url_and_auth(self, relay, upstream): [request] = upstream.requests assert request.method == "GET" assert request.path == "/getsentry/sentry.git/info/refs?service=git-upload-pack" - assert request.headers["Authorization"] == basic_auth("x-access-token", TOKEN) + assert request.headers["Authorization"] == basic_auth("x-access-token", TOKEN.reveal()) def test_git_headers_pass_through(self, relay, upstream): body = gzip.compress(b"0000") @@ -121,7 +123,7 @@ def test_other_headers_stay_behind(self, relay, upstream): ).content ) [request] = upstream.requests - assert request.headers["Authorization"] == basic_auth("x-access-token", TOKEN) + assert request.headers["Authorization"] == basic_auth("x-access-token", TOKEN.reveal()) assert request.headers["Host"] == upstream.url.removeprefix("https://") for name in ("X-Scm-Git-Grant", "Cookie", "X-Forwarded-For"): assert name not in request.headers @@ -495,7 +497,7 @@ def send(self, request, *args, **kwargs): @pytest.mark.parametrize( ("basic_auth_username", "token"), - [("x-access-token", "ghs_tok\u20acn"), ("x-acc\u00e9ss-token", TOKEN)], + [("x-access-token", "ghs_tok\u20acn"), ("x-acc\u00e9ss-token", TOKEN.reveal())], ids=["token", "basic_auth_username"], ) def test_refuses_credentials_that_are_not_ascii(upstream, basic_auth_username, token): @@ -503,7 +505,7 @@ def test_refuses_credentials_that_are_not_ascii(upstream, basic_auth_username, t with pytest.raises(ValueError, match="not ASCII") as exc_info: GitRelay(ca_bundle=ca_bundle()).forward( remote, - token, + Secret(token), method="GET", route="info/refs", query="service=git-upload-pack", @@ -519,8 +521,10 @@ def assert_token_not_in(error: Exception) -> None: # The requests exception holds the prepared request and its Authorization header, so it must not be chained. assert error.__cause__ is None assert error.__suppress_context__ - assert TOKEN not in str(error) - assert basic_auth("x-access-token", TOKEN) not in repr(error) + assert TOKEN.reveal() not in str(error) + assert_not_in_frames(error, TOKEN.reveal()) + assert reported_local(error, "forward", "token") == "[Filtered]" + assert basic_auth("x-access-token", TOKEN.reveal()) not in repr(error) def upstream_stream(*chunks: bytes | Exception) -> tuple[_UpstreamStream, MagicMock, MagicMock]: diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index bdf6747..84d6652 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -15,12 +15,12 @@ from scm.rpc.types import StreamResponse from scm.secret import Secret from scm.types import GitRemote, Provider, Repository, RepositoryId -from tests.unit.error_reports import assert_not_in_frames +from tests.unit.error_reports import assert_not_in_frames, reported_local from tests.unit.git_builders import make_grant, make_provider, make_repository from tests.unit.git_upstream import ClosingAdapter, basic_auth, running_upstream SECRET = Secret("test-secret-0000000000000000000000") -TOKEN = "ghs_secrettoken" +TOKEN = Secret("ghs_secrettoken") REFERRER = make_grant().referrer @@ -29,7 +29,7 @@ def signed_grant(**overrides) -> str: class Recorder: - def __init__(self, repository: Repository | None = None, token: str | None = TOKEN) -> None: + def __init__(self, repository: Repository | None = None, token: Secret | None = TOKEN) -> None: self.repository = make_repository() if repository is None else repository self.token = token self.repository_lookups: list[tuple[int, RepositoryId]] = [] @@ -47,7 +47,7 @@ def fetch_provider(self, organization_id: int, repository: Repository) -> Provid self.provider_lookups.append((organization_id, repository)) return make_provider(organization_id, repository) - def get_token(self, repository: Repository, operation: GitOperation) -> str | None: + def get_token(self, repository: Repository, operation: GitOperation) -> Secret | None: self.token_lookups.append((repository, operation)) return self.token @@ -379,7 +379,7 @@ def test_upstream_unavailable(self): response = request(make_server(recorder, GitRelay(timeout=(1, 1))), signed_grant()) assert_refused(response, 503, "temporarily unavailable") assert "127.0.0.1" not in b"".join(response.content).decode() - assert TOKEN not in b"".join(response.content).decode() + assert TOKEN.reveal() not in b"".join(response.content).decode() def test_reported_remote_host_leaves_out_credentials_in_the_url(self): recorder = Recorder( @@ -521,12 +521,12 @@ def test_relays_end_to_end(self): b"".join(response.content) [received] = upstream.requests assert received.path == "/getsentry/sentry.git/info/refs?service=git-upload-pack" - assert received.headers["Authorization"] == basic_auth("x-access-token", TOKEN) + assert received.headers["Authorization"] == basic_auth("x-access-token", TOKEN.reveal()) assert GRANT_HEADER not in received.headers class TestKeptOutOfErrorReports: - """What GitServer passes to emit_error is reported to Sentry, with every frame's local variables.""" + """What GitServer passes to emit_error or raises is reported to Sentry, with every frame's local variables.""" def test_refused_grant_leaves_out_the_signing_secret(self): recorder = Recorder() @@ -547,3 +547,36 @@ def test_relay_failure_leaves_out_the_grant_and_the_signing_secret(self, header) assert error.code != "git_invalid_grant" # type: ignore[attr-defined] assert_not_in_frames(error, grant) assert_not_in_frames(error, SECRET.reveal()) + + def test_relay_failure_leaves_out_the_provider_token(self): + recorder = Recorder( + repository=make_repository(provider_name="github_enterprise", web_base_url="https://127.0.0.1:1") + ) + request(make_server(recorder, GitRelay(timeout=(1, 1))), signed_grant()) + [(error, _)] = recorder.errors + assert error.code == "resource_service_unavailable" # type: ignore[attr-defined] + assert_not_in_frames(error, TOKEN.reveal()) + # Not because the frames dropped it: both hold it, masked. + assert reported_local(error, "handle", "token") == "[Filtered]" + assert reported_local(error, "forward", "token") == "[Filtered]" + + def test_rejected_token_is_left_out(self): + recorder = Recorder() + request(make_server(recorder, relay_returning(chunks(b"Bad credentials"), 401)), signed_grant()) + [(error, _)] = recorder.errors + assert error.code == "git_token_rejected" # type: ignore[attr-defined] + assert_not_in_frames(error, TOKEN.reveal()) + assert reported_local(error, "handle", "token") == "[Filtered]" + + def test_token_that_is_not_a_secret_is_refused_and_left_out(self): + # A plain string from code the type checker does not see, which the caller would report with its frames. + recorder = Recorder(token=TOKEN.reveal()) # type: ignore[arg-type] + server = make_server(recorder) + with pytest.raises(TypeError, match=r"^get_token must return an scm\.Secret, not str\.") as exc_info: + request(server, signed_grant()) + assert TOKEN.reveal() not in str(exc_info.value) + assert_not_in_frames(exc_info.value, TOKEN.reveal()) + server.relay.forward.assert_not_called() # type: ignore[attr-defined] + assert recorder.counts == [ + ("sentry.scm.git.request.failed", {"referrer": REFERRER, "code": "unhandled_exception"}) + ]