Repository navigation
fix!: Keep signing secrets and git grants out of error reports - #162
Merged
Merged
Conversation
Sentry's SDK sends the local variables of every frame in a reported traceback, and its default scrubbing only replaces a few exact variable names, never looking inside a list or a dict. "secret" is one of them; "secrets" and "signing_secret" are not. So when make_proxy_client fails to fetch the repository, for example because Sentry is unreachable, the event carries the client's signing secret three times: in make_proxy_client, in its fetch_repository lambda and in fetch_repository. If anything fails while RpcServer verifies a signature, the verify frames carry the server's list of secrets. A small Secret wrapper shows as [Filtered] to the SDK, through __sentry_repr__, and to repr() and str(). The RPC client and server wrap the secrets they are given on entry, and sign() unwraps one inline where it signs. Callers can keep passing plain strings. The plain string is still in the caller's frame and in any frame after reveal(), so this adds to the SDK's scrubbing rather than replacing it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Wrapping the secret on entry keeps it out of this library's frames, but a caller that passes a plain str still holds it in its own frame, where an error report shows it. Only the caller can wrap it where it reads it. Requiring Secret outright would fail type checks in Sentry and Seer, and dropping the coercion would break their RPC calls at runtime, so plain strings keep working for now with a DeprecationWarning that points at the calling line. Secret is exported from the scm package so callers have a stable import, and the bin/ clients and servers wrap theirs already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The helpers in scm.rpc.helpers look internal, but Sentry's tests import sign_get and sign_post to sign requests with plain strings, so moving them would break those tests on upgrade. They warn the same way as the other entry points, so the next major release can require Secret everywhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vaind
marked this pull request as ready for review
October 22, 2026 16:12
GitServer reports every refused grant through emit_error, and the SDK sends each frame's locals without looking inside lists or dicts. So a bad or expired grant put every signing secret in the event, through verify_grant's `secrets`, and any failure after the grant check put the grant itself in the event, through `headers` in GitServer.handle and in GitRelay.forward. The git API has not been released, so it takes Secret only rather than going through the plain-string deprecation the RPC API needs. A plain string gets a TypeError that says what to do, for untyped callers. Secret is falsy when empty, so the existing empty-secret checks still refuse one. handle takes the grant out of the headers on entry. The relay never sent it upstream, so it only loses a copy it had no use for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vaind
changed the title
cmanallen
requested changes
Oct 22, 2026
A plain string stays in the caller's own frame, where an error report shows it, so wrapping it on entry only ever protected this library's frames. Requiring Secret moves the wrapping to where the caller reads the secret, as the git API already does, and one rule for both is simpler than a deprecation period. This breaks Sentry and Seer, which pass strings today, until they wrap theirs; their changes and the major release are coordinated separately. Secret.of goes too, since nothing coerces strings any more. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mypy already refuses a plain string wherever a caller is type-checked, Sentry's settings included. The runtime check only matters where a string that slipped past it does harm: a server would hold it for every request, fail each one inside verification, and report the strings in its frames. Elsewhere a string just fails with an AttributeError on first use, so the checks there were noise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vaind
changed the title
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.
Signing secrets and git grants reach Sentry's error reports. The SDK sends every frame's local variables and scrubs only a few exact names (
secret, notsecretsorsigning_secret), never looking inside a list or dict. Confirmed with sentry-sdk 2.68.0 under Sentry's and Seer's defaults:make_proxy_clientcan't fetch the repository, the event carries the signing secret.headersinGitServer.handleandGitRelay.forward.Approach
scm.Secretwraps a string and shows as[Filtered]through__sentry_repr__,repr()andstr().reveal()unwraps it, inline where it's used. It is falsy when empty, so the empty-secret checks still work.Secretonly: the RPC client and server, thescm.rpc.helperssigning functions,sign_grant,verify_grantandGitServer. A plain string would stay in the caller's own frame, so callers wrap it where they read it, e.g.Secret(os.environ["SCM_RPC_SHARED_SECRET"]), as thebin/scripts now do.RpcServerandGitServeralso refuse astrwith aTypeErrorwhen built: one that got past the type checker would fail every request and report the strings in its frames.GitServer.handletakes the grant out of the headers on entry. The relay never sent it upstream.Breaking: Sentry and Seer pass plain strings to the RPC API today, and Sentry's tests call
sign_getandsign_postwith them. Both need to wrap their secrets before upgrading, so this ships in a major release.Tests
For each leak above, no frame of the reported error's traceback shows the value, checked the way the SDK serializes locals (
tests/unit/error_reports.py); each test fails without the change. Also: the wrapper's representations, and both servers refusing a plain string without putting it in the message.🤖 Generated with Claude Code