fix(git): Keep the provider token out of error reports

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",

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,

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
             )

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):

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)

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,15 +497,15 @@ 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):
     remote = GitRemote(url=f"{upstream.url}/getsentry/sentry.git", basic_auth_username=basic_auth_username)
     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]:

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"})
+        ]