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
vaind
added a commit
that referenced
this pull request
Oct 26, 2026
## Problem The release workflow computes the next version with `uv version --bump` from a bump type picked by hand. A breaking PR (a `type!:` title, e.g. #162) only becomes a major release if the person running the release remembers to pick `major`. ## Approach - Pass the workflow input straight to `action-prepare-release`, which hands it to `craft prepare`. The new default is `auto`: craft finds the highest bump among the PR titles since the last tag, using its built-in conventional-commit categories (`type!:` → major, `feat:` → minor, everything else → patch). - Keep `minor`/`patch`/`major` as manual overrides. Craft resolves those against the last tag too, so the uv calculation step is removed. uv is still installed because `scripts/bump-version.sh` writes the chosen version with it. `auto` and bump types are supported from craft 2.14, below our `minVersion` of 2.21.1. `auto` fails the release with a clear message if there are no new commits, or if none of them has a conventional-commit title. Pick an explicit bump in that case. This works on its own, but pairs with #163, which turns on the changelog these categories feed into. Checked with actionlint. The real test is the next release run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
3 tasks done
vaind
added a commit
to getsentry/sentry
that referenced
this pull request
Oct 26, 2026
## Problem [sentry-scm 2.0.0](https://github.com/getsentry/scm-platform/releases/tag/2.0.0) ([getsentry/scm-platform#162](getsentry/scm-platform#162)) takes RPC signing secrets as `scm.Secret`, which shows as `[Filtered]` in error reports. The SDK scrubs a local named `secret`, but not `secrets` and not values inside a list. So a failure while verifying a request could send every `SCM_RPC_SHARED_SECRET` to Sentry. `RpcServer` now refuses plain strings, so this bump is required before Sentry can pick up 2.0. ## Approach - `make_server` wraps each configured secret in `Secret` when building the `RpcServer`. The `SCM_RPC_SHARED_SECRET` setting stays a `list[str]`, so deploy config doesn't change. - `test_scm_rpc.py` passes `Secret` to `make_proxy_client`, `sign_get` and `sign_post`. - The test `DjangoTestClientSessionAdapter.post` accepts `stream`, which the RPC client has passed to `Session.post` since 1.10.0. Sentry was still on 1.9.0, so this hadn't come up before. The signature on the wire is the same, so this can deploy independently of the matching Seer change ([getsentry/seer#9056](getsentry/seer#9056)). ## Testing - `pytest tests/sentry/scm` plus every other test module that imports `scm` passes (767 tests). mypy and prek are clean on the changed files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 27, 2026
vaind
added a commit
that referenced
this pull request
Oct 28, 2026
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`, and `GitServer.handle` and `GitRelay.forward` both hold the token as a plain string. #162 fixed this for grant secrets and the grant, but left the token a `str`. ## Approach - `GetToken` returns `scm.Secret | None`, and `GitServer.handle` passes the `Secret` on to `GitRelay.forward`, so both frames show `[Filtered]`. The relay calls `reveal()` only inline, for the ASCII check and `HTTPBasicAuth`. - `GitServer.handle` refuses a token that isn't a `Secret` with a `TypeError`, 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 an `AttributeError` whose frames hold it. - `bin/git-server` wraps its tokens. `get_token` now has to wrap the token in `Secret`. 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 `BlacklistAdapter` refusing an internal address, passes through urllib3's frames, which hold the `Authorization` header. The relay can't mask those, so `GitRelay`'s docstring now says to report such an error without its frames' local variables; Sentry's endpoint handles that separately.
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.
Contributor
Author
|
Follow-up: #170 (CW-2334) switches grants to Ed25519. The |
doc-sheet
pushed a commit
to doc-sheet/scm-platform
that referenced
this pull request
Nov 2, 2026
## Problem The release workflow computes the next version with `uv version --bump` from a bump type picked by hand. A breaking PR (a `type!:` title, e.g. getsentry#162) only becomes a major release if the person running the release remembers to pick `major`. ## Approach - Pass the workflow input straight to `action-prepare-release`, which hands it to `craft prepare`. The new default is `auto`: craft finds the highest bump among the PR titles since the last tag, using its built-in conventional-commit categories (`type!:` → major, `feat:` → minor, everything else → patch). - Keep `minor`/`patch`/`major` as manual overrides. Craft resolves those against the last tag too, so the uv calculation step is removed. uv is still installed because `scripts/bump-version.sh` writes the chosen version with it. `auto` and bump types are supported from craft 2.14, below our `minVersion` of 2.21.1. `auto` fails the release with a clear message if there are no new commits, or if none of them has a conventional-commit title. Pick an explicit bump in that case. This works on its own, but pairs with getsentry#163, which turns on the changelog these categories feed into. Checked with actionlint. The real test is the next release run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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