Repository navigation
fix(git): Keep the provider token out of error reports - #167
Merged
Merged
Conversation
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 (getsentry/sentry#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).
This was referenced Oct 27, 2026
giovanni-guidini
approved these changes
Oct 28, 2026
vaind
added a commit
that referenced
this pull request
Oct 29, 2026
When the RPC server reports an error, the report can carry the request's signature (`Authorization: rpcsignature rpc0:...`). Sentry's SDK sends the local variables of every frame in the traceback, Sentry's SDK configuration turns off the scrubber, and `RpcServer` holds the signature as a plain string, in `headers` and in `authorization`. `RpcServer.post` reports every coded error, including a provider failure such as GitHub being down, so each of those reports has the signature right next to `data`, the body it signs. #162 and #167 did the same masking for the signing secrets, the git grant and the provider token. ## Why it matters A signature has no timestamp or nonce. The POST signature covers only the body, not `X-Organization-Id`, `X-Repository-Id` or `X-Credentials-Set`. So anyone who reads a body and its signature in an error report can replay that request whenever they like, with other values in those headers. Changing the protocol would touch Sentry and Seer, so it's a separate decision and not part of this PR. ## Approach - **Server:** `RpcServer.get` and `RpcServer.post` take `Authorization` out of `headers` as soon as they're called, the way `GitServer.handle` does with the git grant. They keep it as an `scm.Secret`, which shows as `[Filtered]`. `verify_get` and `verify_post` accept a `Secret` and reveal it only inline, inside the comparison. They still accept a plain string, so this doesn't break callers. - **Client:** `RpcApiClient.request` keeps its signature as a `Secret` for the whole retry loop and reveals it only inside the call to the session. When retries on connection errors run out, the `ResourceServiceUnavailable` it raises no longer chains the connection error (`from None`, as `GitRelay` does). The detail message still names that error. A chained exception is reported with its own frames, and the session's and requests' frames hold the header. Two behavior changes: - The server now accepts an `authorization` header in any case. Before, a lowercase one got a 400. Django's `request.headers` already gives `Authorization`, so Sentry's endpoint sees no change. - `ResourceServiceUnavailable.__cause__` is now `None`. ## What this doesn't cover - A raw requests error, which reaches the caller when connection retries are off or for a write, still has the header in the session's and requests' frames. `RpcApiClient`'s docstring says so. The same goes for `fetch_repository` and `RequestsSession.get`. Masking `RequestsSession`'s own frames would mean changing the `Session` protocol, which is a breaking change, and it wouldn't help: the frame right below, in `requests.api.request`, holds the same headers. - Frames inside requests and urllib3 can't be masked. ## Tests - `TestKeptOutOfErrorReports` in `test_rpc_server.py`: a provider failure that `post` reports, with `Authorization` and with `authorization`, and a raw failure from `get`. - `TestKeptOutOfErrorReports` in `test_rpc_client.py`: retries exhausted on a connection error. Each test failed before this change. `reported_local` in `tests/unit/error_reports.py` is the same as in #167, so whichever PR lands second merges without a conflict.
vaind
added a commit
that referenced
this pull request
Oct 30, 2026
… failures (#171) Porting Sentry and Seer to the Ed25519 grants from #170 found gaps that should be fixed before 3.0.0: - A bad key raised plain `ValueError`/`TypeError` from the same calls that parse untrusted grants, so callers couldn't tell their own misconfiguration apart from a bad or oversized grant. - There was no helper to make a key, so `bin/git-server` and every test suite copied the same `cryptography` calls. - Three permanent failures answered 502, which Seer's checkout (getsentry/seer#9229) retries for about a minute before failing. Follow-up to #170 (CW-2334). ## Approach - New `scm.errors.GitGrantKeyError(SCMError, ValueError)` for every bad key, on both sides. It isn't an `SCMCodedError`, because a bad key is the server's fault: `GitServer` raises it from its constructor rather than answering requests with a 403. An oversized grant still raises a plain `ValueError`. - New `require_private_key(Secret) -> Secret`, the signer's counterpart to `require_public_keys`, to check a key at startup. - New `generate_private_key() -> Secret`, which replaces the copied `cryptography` calls in `bin/git-server` and `make_key_pair`. - `GitServer` answers `git_remote_not_https`, `git_remote_refused` and `resource_certificate_invalid` with a 500 instead of a 502. The integration or host is misconfigured, so no retry helps. 502 still means the provider or the token failed; 503 and 504 still mean a retry can succeed. - `GetToken` and `GitServer` now document that `get_token` may raise a coded error, which `GitServer` answers by its code. Sentry relies on this to answer an unreachable control silo with a 503. Not included, as decided: a public `key_id()`, or reporting which key matched a grant. ## Release notes Craft uses this section for the changelog in place of the title. The last item covers earlier PRs whose titles don't show a breaking change. ### Changelog Entry - Raise `GitGrantKeyError` for a missing or unusable git grant key, so callers can tell their own misconfiguration apart from a bad grant - It's a `ValueError`, not an `SCMCodedError`. A public key that isn't a string used to raise `TypeError`. - `verify_grant([], token)` raises it. 2.x refused the grant with `GitInvalidGrant` (a 403 from `GitServer`). - Add `require_private_key` to check a signing key at startup, and `generate_private_key` to make one for tests - Answer a remote that isn't https, a refused remote host and an untrusted TLS certificate with a 500 instead of a 502, since no retry can fix them - Document that `get_token` may raise a coded error, which `GitServer` answers by its code, e.g. `ResourceServiceUnavailable` as a 503 - Upgrade notes for earlier 3.0.0 changes - `GitServer`'s `get_token` must return an `scm.Secret`, or every relayed request fails with `TypeError` ([#167](#167)). - Once Sentry maps `RestrictedIPAddress` to `GitRemoteRefused` with `GitRelay(map_error=...)`, its refusal of internal addresses changes from a 503 to a 500 ([#168](#168)). ## Tests Each bad-key case raises `GitGrantKeyError` on both sides, and no message quotes the key. That check also now works for multi-line keys: it used to join the lines, so it never matched RSA, EC or Ed448 keys. A generated key round-trips through `sign_grant`, `public_key_of` and `verify_grant`. New `GitServer` tests cover the three 500s and a coded and an uncoded error from `get_token`. The suite, ruff, mypy and prek pass, and I smoke-tested `bin/git-server`'s key loading. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When Sentry's git endpoint (getsentry/sentry#128444) refuses a request after it has fetched a provider token, for example because the provider is unreachable or rejects the token, the error report carries the token. Sentry's SDK sends the local variables of every frame in the traceback, Sentry's SDK configuration turns off the scrubber that would otherwise replace a variable named
token, andGitServer.handleandGitRelay.forwardboth hold the token as a plain string. #162 fixed this for grant secrets and the grant, but left the token astr.Approach
GetTokenreturnsscm.Secret | None, andGitServer.handlepasses theSecreton toGitRelay.forward, so both frames show[Filtered]. The relay callsreveal()only inline, for the ASCII check andHTTPBasicAuth.GitServer.handlerefuses a token that isn't aSecretwith aTypeError, deleting the local first so that error's report doesn't carry it either. A plain string from untyped code would otherwise fail every request in the relay with anAttributeErrorwhose frames hold it.bin/git-serverwraps its tokens.get_tokennow has to wrap the token inSecret. Nothing calls it yet: Sentry's endpoint is still unmerged and will wrap it when it bumps to the release with this change.Not covered: an error the relay's adapter raises itself, such as Sentry's
BlacklistAdapterrefusing an internal address, passes through urllib3's frames, which hold theAuthorizationheader. The relay can't mask those, soGitRelay's docstring now says to report such an error without its frames' local variables; Sentry's endpoint handles that separately.