Send the chi route pattern to Sentry, not the concrete path (closes #179) #181

clawbot · 2026-08-18T02:21:15+02:00

clawbot commented

2026-08-18 02:21:15 +02:00

Closes #179.

The defect

scrubSentryRequest redacts the Sentry body, query string, cookies, env and headers, but kept Request.URL, which the SDK builds as scheme://host/path from the concrete path (interfaces.go:183). On the receiver route that path is /webhook/<uuid> in full, and that UUID is a write capability rather than an identifier.

#146's "2xx and 5xx keep the concrete path" ruling was reasoned about a log the operator owns and does not transfer to a tracker with its own retention, access control and deletion policy.

Unconditional, on every route

The pattern replaces the path on every route, not on a sensitive-route list. #174's reasoning for redacting the body unconditionally applies unchanged: a route-conditional rule leaks on any route someone forgets to add. There is no counterweight here — unlike the body, the rewrite costs nothing on the routes it is not aimed at, since on a static route the pattern is the path (/pages/login in, /pages/login out, asserted by TestSentryScrub_KeepsTheRoutingContext).

The route is still identifiable

Sentry groups an error event by its exception and stack trace; Request.URL is displayed, not a default grouping component. Replacing the concrete path with the pattern therefore changes nothing about grouping and still names the route in the UI — while lowering the cardinality of what is displayed, which is what Sentry's own SourceURL / SourceRoute distinction exists for.

Scheme and host

Scheme survives, asserted in three places (http://example.com/webhook/{uuid}, http://example.com/pages/login, https://example.com/(redacted) in the fallback cases). This is load-bearing exactly as #174 recorded: interfaces.go:180 derives it from r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https", byte-identical to internal/middleware/csrf.go:19, so the scheme is the CSRF TLS decision and the allowlist's justification for dropping X-Forwarded-Proto still holds.

Host stays — but not because it is operator configuration. It is parsed.Host of the SDK-built scheme://r.Host/path, so it is whatever the client's Host header carried: this service validates no hostname (no host allowlist, no configured hostname in internal/config, and internal/handlers/source_management.go:402 already takes r.Host at face value for the displayed BaseURL). It is kept because that same header is on the allowlist, so scrubbing it out of the URL would withhold nothing that is not sent anyway. Rev 2 rewords both statements of this rationale — README.md and the sentryRouteURL doc comment — off the false premise; the decision itself is unchanged.

Everything else in the parsed URL is discarded rather than edited — the result is rebuilt as scheme + "://" + host + pattern — so an SDK version that starts appending a query string cannot widen this. A side effect worth stating so it is not removed by accident: rebuilding from parsed.Scheme/parsed.Host also strips URL userinfo, so http://user:pw@example.com/... becomes http://example.com/(redacted).

The absent case

The pattern is reachable only on the error dispatch: sentryhttp.go:124-125 calls RecoverWithContext with the request under sentry.RequestContextKey, client.go:484-485 copies that context onto the hint, client.go:631 hands it to BeforeSend. chi's routing context is a pointer put on the request context before the middleware chain runs (chi mux.go:84) and filled in as the mux routes, and mx.pool.Put happens only after the chain returns — so the deferred recover still reads a live pattern.

The fallback is never the concrete path:

Missing	Result
nil hint	`scheme://host/(redacted)`
hint with nil `Context`	same
context without the request	same
request with no chi route context	same
route context but no match (`RoutePattern() == ""`)	same
URL that will not parse into a scheme	`(redacted)`, whole

/(redacted) reuses the existing marker so a reader can tell a withheld path from an absent one.

`BeforeSendTransaction`

It does not get a usable hint. Span.doFinish calls hub.CaptureEvent(event) (tracing.go:356), which passes a nil hint to client.CaptureEvent, which replaces it with an empty &EventHint{} (client.go:620-622). So hint is non-nil but hint.Context is nil and no request is reachable — the fallback above is what applies there, verified end to end rather than argued (TestSentryScrub_RedactsTheTransactionDispatch runs a real transaction through the real middleware with tracing switched on).

A transaction event also carries event.Transaction, which sentryhttp.go:105 builds as fmt.Sprintf("%s %s", r.Method, r.URL.Path) and tracing.go:553 copies onto the event — the same capability by a second field, in the same hook. It is rewritten on the same terms: METHOD + pattern where known, METHOD /(redacted) where not, and withheld whole if the name has no space to split on. Method is kept for the reason Request.Method already is: net/http admits only a bounded token there.

sentryTransactionName cuts on the first space, so a name that is not METHOD /path and contains a space becomes firstWord /(redacted) — safe, and unreachable today since only sentryhttp sets the name.

The service does not enable tracing, so no transaction event is produced today. The hook is a floor against that changing, which is why it is installed on both dispatches.

Tests

internal/server/sentry_test.go, through the real SetRequest → ApplyToEvent → BeforeSend path. The capture helper now routes through a real chi mux carrying the production middleware order — a recovering middleware in middleware.Recoverer's slot, then sentryhttp.New(sentryhttp.Options{Repanic: true}).Handle registered with Use, exactly as routes.go registers it — over /pages/login and /webhook/{uuid}. That is load-bearing, not decoration: a hand-built request carries no route pattern at all and could not tell the hook working from the hook falling back.

TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern — receiver UUID absent from the marshalled event; URL equals http://example.com/webhook/{uuid}.
TestSentryScrub_SDKCollectsTheRequestUnscrubbed — extended to pin the new premise: unscrubbed, Request.URL does contain the UUID.
TestSentryScrub_RedactsTheTransactionDispatch / _TransactionDispatchIsUnscrubbedWithoutTheHook — the transaction dispatch, with and without the hook.
TestSentryScrub_FallsBackWithoutARoutePattern — four sub-cases (no hint, no context, no request, unrouted request).
TestSentryScrub_WithholdsUnparseableValues — schemeless URL and space-less transaction name.
Existing body/query/header and no-request cases unchanged in intent.

Mutation checks

1. The URL rewrite removed (if req.URL != "" && false). Four tests failed; the marshalled receiver event, straight from the failure output:

"request":{"url":"http://example.com/webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d",
"method":"POST","data":"(redacted)","headers":{...}}

    Error: "...{\"url\":\"http://example.com/webhook/6d1f9c2a-...\"...}"
           should not contain "6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d"
    Error: Not equal:
           expected: "http://example.com/(redacted)"
           actual  : "http://example.com/webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d"
--- FAIL: TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern (0.01s)
--- FAIL: TestSentryScrub_RedactsTheTransactionDispatch (0.01s)
--- FAIL: TestSentryScrub_FallsBackWithoutARoutePattern (0.01s)  [all 4 subtests]
--- FAIL: TestSentryScrub_WithholdsUnparseableValues (0.00s)

Note the transaction name still read "transaction":"POST /(redacted)" in that run — the two rewrites are independently covered.

2. The transaction-name rewrite removed (if event.Transaction != "" && false), URL rewrite restored:

"transaction":"POST /webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d",
"request":{"url":"http://example.com/(redacted)", ...}

    Error: Not equal:
           expected: "POST /(redacted)"
           actual  : "POST /webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d"
--- FAIL: TestSentryScrub_RedactsTheTransactionDispatch
--- FAIL: TestSentryScrub_FallsBackWithoutARoutePattern  [all 4 subtests]
--- FAIL: TestSentryScrub_WithholdsUnparseableValues

Both restored afterwards; the committed tree is the green one.

Gates (rev 2, `fb5a203`)

make check exits 0, zero (cached): all 13 packages executed with real durations (internal/ciscript 7.133s, internal/handlers 6.616s, internal/delivery 4.562s, internal/server 3.285s, ...), 0 FAIL, all 13 TestSentryScrub_* green. Lint: 0 issues.

docker build --no-cache-filter=lint --no-cache-filter=builder . exits 0:

#15 [lint 7/9] RUN make fmt-check                                     DONE  1.4s
#16 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE  0.4s
#17 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./...
#17 48.07 0 issues.                                                   DONE 48.2s
#25 [builder  9/11] RUN make test                                     DONE 57.3s
#26 [builder 10/11] RUN make build                                    DONE 44.1s

All 13 packages executed in the container with real durations and zero (cached), 619 PASS lines, 0 FAIL, all 13 TestSentryScrub_* green. Lint ran in the pinned golangci/golangci-lint:v2.12.2 image. The log did not clip at BuildKit's 2 MiB limit this run (1.0 MiB).

Disclosure: five layers reported CACHED — the two base-image FROM pulls (#8, #9) and three layers of the final Alpine runtime stage (#28–#30). No layer that runs a check was cached. The tagged image was removed and docker ps -a is empty; no prune of any kind was run.

Also touched

README.md — the Sentry paragraphs of the logging section, rewritten to state the URL handling exactly: what is sent, that scheme and host are kept and why, that the rules are unconditional, that the pattern is reachable on the error dispatch only, what the fallback is, and the transaction-name handling. The allowlist paragraph's "scheme, host, path, method" now reads "route pattern" and its X-Forwarded-Proto reasoning is tied to the scheme the rewrite preserves. Formatted with make fmt. TODO.md untouched.

Noted, not fixed

The pinned linter still emits The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2 on every run. Pre-existing, tracked at #98.

Closes https://git.eeqj.de/sneak/webhooker/issues/179. ## The defect `scrubSentryRequest` redacts the Sentry body, query string, cookies, env and headers, but kept `Request.URL`, which the SDK builds as `scheme://host/path` from the concrete path (`interfaces.go:183`). On the receiver route that path is `/webhook/<uuid>` in full, and that UUID is a write capability rather than an identifier. https://git.eeqj.de/sneak/webhooker/issues/146's "2xx and 5xx keep the concrete path" ruling was reasoned about a log the operator owns and does not transfer to a tracker with its own retention, access control and deletion policy. ## Unconditional, on every route The pattern replaces the path on **every** route, not on a sensitive-route list. https://git.eeqj.de/sneak/webhooker/pulls/174's reasoning for redacting the body unconditionally applies unchanged: a route-conditional rule leaks on any route someone forgets to add. There is no counterweight here — unlike the body, the rewrite costs nothing on the routes it is not aimed at, since on a static route the pattern **is** the path (`/pages/login` in, `/pages/login` out, asserted by `TestSentryScrub_KeepsTheRoutingContext`). ## The route is still identifiable Sentry groups an error event by its exception and stack trace; `Request.URL` is displayed, not a default grouping component. Replacing the concrete path with the pattern therefore changes nothing about grouping and still names the route in the UI — while lowering the cardinality of what is displayed, which is what Sentry's own `SourceURL` / `SourceRoute` distinction exists for. ## Scheme and host **Scheme survives**, asserted in three places (`http://example.com/webhook/{uuid}`, `http://example.com/pages/login`, `https://example.com/(redacted)` in the fallback cases). This is load-bearing exactly as https://git.eeqj.de/sneak/webhooker/pulls/174 recorded: `interfaces.go:180` derives it from `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"`, byte-identical to `internal/middleware/csrf.go:19`, so the scheme **is** the CSRF TLS decision and the allowlist's justification for dropping `X-Forwarded-Proto` still holds. **Host stays** — but not because it is operator configuration. It is `parsed.Host` of the SDK-built `scheme://r.Host/path`, so it is whatever the client's `Host` header carried: this service validates no hostname (no host allowlist, no configured hostname in `internal/config`, and `internal/handlers/source_management.go:402` already takes `r.Host` at face value for the displayed `BaseURL`). It is kept because that same header is on the allowlist, so scrubbing it out of the URL would withhold nothing that is not sent anyway. Rev 2 rewords both statements of this rationale — `README.md` and the `sentryRouteURL` doc comment — off the false premise; the decision itself is unchanged. Everything else in the parsed URL is **discarded rather than edited** — the result is rebuilt as `scheme + "://" + host + pattern` — so an SDK version that starts appending a query string cannot widen this. A side effect worth stating so it is not removed by accident: rebuilding from `parsed.Scheme`/`parsed.Host` also strips URL userinfo, so `http://user:pw@example.com/...` becomes `http://example.com/(redacted)`. ## The absent case The pattern is reachable only on the **error** dispatch: `sentryhttp.go:124-125` calls `RecoverWithContext` with the request under `sentry.RequestContextKey`, `client.go:484-485` copies that context onto the hint, `client.go:631` hands it to `BeforeSend`. chi's routing context is a pointer put on the request context before the middleware chain runs (`chi mux.go:84`) and filled in as the mux routes, and `mx.pool.Put` happens only after the chain returns — so the deferred recover still reads a live pattern. The fallback is **never the concrete path**: | Missing | Result | | --- | --- | | nil hint | `scheme://host/(redacted)` | | hint with nil `Context` | same | | context without the request | same | | request with no chi route context | same | | route context but no match (`RoutePattern() == ""`) | same | | URL that will not parse into a scheme | `(redacted)`, whole | `/(redacted)` reuses the existing marker so a reader can tell a withheld path from an absent one. ## `BeforeSendTransaction` **It does not get a usable hint.** `Span.doFinish` calls `hub.CaptureEvent(event)` (`tracing.go:356`), which passes a **nil** hint to `client.CaptureEvent`, which replaces it with an empty `&EventHint{}` (`client.go:620-622`). So `hint` is non-nil but `hint.Context` is nil and no request is reachable — the fallback above is what applies there, verified end to end rather than argued (`TestSentryScrub_RedactsTheTransactionDispatch` runs a real transaction through the real middleware with tracing switched on). A transaction event also carries `event.Transaction`, which `sentryhttp.go:105` builds as `fmt.Sprintf("%s %s", r.Method, r.URL.Path)` and `tracing.go:553` copies onto the event — the **same** capability by a second field, in the same hook. It is rewritten on the same terms: `METHOD ` + pattern where known, `METHOD /(redacted)` where not, and withheld whole if the name has no space to split on. Method is kept for the reason `Request.Method` already is: net/http admits only a bounded token there. `sentryTransactionName` cuts on the **first** space, so a name that is not `METHOD /path` and contains a space becomes `firstWord /(redacted)` — safe, and unreachable today since only `sentryhttp` sets the name. The service does not enable tracing, so no transaction event is produced today. The hook is a floor against that changing, which is why it is installed on both dispatches. ## Tests `internal/server/sentry_test.go`, through the real `SetRequest` → `ApplyToEvent` → `BeforeSend` path. The capture helper now routes through a **real chi mux** carrying the production middleware order — a recovering middleware in `middleware.Recoverer`'s slot, then `sentryhttp.New(sentryhttp.Options{Repanic: true}).Handle` registered with `Use`, exactly as `routes.go` registers it — over `/pages/login` and `/webhook/{uuid}`. That is load-bearing, not decoration: a hand-built request carries no route pattern at all and could not tell the hook working from the hook falling back. - `TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern` — receiver UUID absent from the marshalled event; URL equals `http://example.com/webhook/{uuid}`. - `TestSentryScrub_SDKCollectsTheRequestUnscrubbed` — extended to pin the new premise: unscrubbed, `Request.URL` **does** contain the UUID. - `TestSentryScrub_RedactsTheTransactionDispatch` / `_TransactionDispatchIsUnscrubbedWithoutTheHook` — the transaction dispatch, with and without the hook. - `TestSentryScrub_FallsBackWithoutARoutePattern` — four sub-cases (no hint, no context, no request, unrouted request). - `TestSentryScrub_WithholdsUnparseableValues` — schemeless URL and space-less transaction name. - Existing body/query/header and no-request cases unchanged in intent. ## Mutation checks **1. The URL rewrite removed** (`if req.URL != "" && false`). Four tests failed; the marshalled receiver event, straight from the failure output: ``` "request":{"url":"http://example.com/webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d", "method":"POST","data":"(redacted)","headers":{...}} ``` ``` Error: "...{\"url\":\"http://example.com/webhook/6d1f9c2a-...\"...}" should not contain "6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d" Error: Not equal: expected: "http://example.com/(redacted)" actual : "http://example.com/webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d" --- FAIL: TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern (0.01s) --- FAIL: TestSentryScrub_RedactsTheTransactionDispatch (0.01s) --- FAIL: TestSentryScrub_FallsBackWithoutARoutePattern (0.01s) [all 4 subtests] --- FAIL: TestSentryScrub_WithholdsUnparseableValues (0.00s) ``` Note the transaction name still read `"transaction":"POST /(redacted)"` in that run — the two rewrites are independently covered. **2. The transaction-name rewrite removed** (`if event.Transaction != "" && false`), URL rewrite restored: ``` "transaction":"POST /webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d", "request":{"url":"http://example.com/(redacted)", ...} ``` ``` Error: Not equal: expected: "POST /(redacted)" actual : "POST /webhook/6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d" --- FAIL: TestSentryScrub_RedactsTheTransactionDispatch --- FAIL: TestSentryScrub_FallsBackWithoutARoutePattern [all 4 subtests] --- FAIL: TestSentryScrub_WithholdsUnparseableValues ``` Both restored afterwards; the committed tree is the green one. ## Gates (rev 2, `fb5a203`) `make check` exits 0, **zero** `(cached)`: all 13 packages executed with real durations (`internal/ciscript 7.133s`, `internal/handlers 6.616s`, `internal/delivery 4.562s`, `internal/server 3.285s`, ...), 0 FAIL, all 13 `TestSentryScrub_*` green. Lint: `0 issues.` `docker build --no-cache-filter=lint --no-cache-filter=builder .` exits 0: ``` #15 [lint 7/9] RUN make fmt-check DONE 1.4s #16 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE 0.4s #17 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./... #17 48.07 0 issues. DONE 48.2s #25 [builder 9/11] RUN make test DONE 57.3s #26 [builder 10/11] RUN make build DONE 44.1s ``` All 13 packages executed in the container with real durations and **zero** `(cached)`, 619 `PASS` lines, 0 FAIL, all 13 `TestSentryScrub_*` green. Lint ran in the pinned `golangci/golangci-lint:v2.12.2` image. The log did not clip at BuildKit's 2 MiB limit this run (1.0 MiB). Disclosure: five layers reported `CACHED` — the two base-image `FROM` pulls (`#8`, `#9`) and three layers of the final Alpine runtime stage (`#28`–`#30`). No layer that runs a check was cached. The tagged image was removed and `docker ps -a` is empty; no prune of any kind was run. ## Also touched `README.md` — the Sentry paragraphs of the logging section, rewritten to state the URL handling exactly: what is sent, that scheme and host are kept and why, that the rules are unconditional, that the pattern is reachable on the error dispatch only, what the fallback is, and the transaction-name handling. The allowlist paragraph's "scheme, host, path, method" now reads "route pattern" and its `X-Forwarded-Proto` reasoning is tied to the scheme the rewrite preserves. Formatted with `make fmt`. `TODO.md` untouched. ## Noted, not fixed The pinned linter still emits `The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2` on every run. Pre-existing, tracked at https://git.eeqj.de/sneak/webhooker/issues/98.

clawbot added 1 commit 2026-08-18 02:21:16 +02:00

Send the chi route pattern to Sentry, not the concrete path (closes #179 )

check / check (push) Successful in 2m44s

Details

5ff7cdbdb5

The Sentry SDK builds Request.URL as scheme://host/path from the
concrete path, which on the receiver route is /webhook/<uuid> in full.
That UUID is a write capability, not an identifier: anyone holding it
can post events this service accepts and its targets then deliver. A
third-party tracker has its own retention, access control and deletion
policy, so the rule the local access log follows does not carry across
that boundary.

The BeforeSend hook now rebuilds the URL from the chi route pattern,
on every route rather than by route list, since a route-conditional
rule leaks on any route someone forgets to add. Scheme and host are
kept and everything else in the URL is discarded rather than edited:
the scheme is the CSRF TLS decision the header allowlist relies on,
and the host is operator configuration already carried by the
allowlisted Host header.

The pattern is reachable only on the error dispatch, where sentryhttp
puts the request on the context the client copies onto the hint. A
finished span captures with a nil hint, so BeforeSendTransaction sees
no context; there and wherever else the pattern is missing the path
falls back to the literal /(redacted), never to the concrete path, and
a URL that will not parse into a scheme is withheld whole. A
transaction event's SDK-built "METHOD /path" name carries the same
capability and is rewritten on the same terms.

clawbot added the needs-review label 2026-08-18 02:21:26 +02:00

clawbot self-assigned this 2026-08-18 02:21:30 +02:00

clawbot commented

2026-08-18 02:32:49 +02:00

FAIL — needs-rework

Reviewed at 5ff7cdb, fresh clone, against #179.

Finding — the only blocker

A security rationale stated twice that is not true of the code.

README.md:1052-1054 — "The host is operator configuration rather than anything a client chooses"
internal/server/sentry.go:155-156 — "it is operator configuration, not a client-supplied or capability-bearing value"

The host in the rebuilt URL is parsed.Host of the SDK-built scheme://r.Host/path (interfaces.go:183), and r.Host is the client's Host header / request-line authority. This service validates no hostname: there is no host allowlist or configured hostname anywhere under internal/ (internal/config has no host key), and internal/handlers/source_management.go:402 already takes r.Host at face value to build the displayed BaseURL. Behind a reverse proxy that rewrites Host the claim holds; on a directly-exposed deployment the client sets that value and it is reflected verbatim into the Sentry URL.

Why it matters: this is the stated justification for keeping a field in a hook whose whole job is deciding what may cross a trust boundary. A future reader carries the premise ("host is operator config") to the next field and it is wrong. The decision to keep the host is correct and must not change — the second half of the same sentence is the justification that actually holds.

Acceptable: reword both places to rest on the true reason — the host is whatever r.Host carried and is already shipped by the allowlisted Host header, so dropping it from the URL would withhold nothing that is not sent anyway. Drop or qualify "operator configuration, not client-chosen". No behaviour change, no test change.

Notes and disclosures — not blocking

If tracing is ever enabled, every transaction collapses to POST /(redacted) and Request.URL to scheme://host/(redacted): the pattern is never reachable on that dispatch (confirmed — nil hint at tracing.go:356, replaced by &EventHint{} at client.go:620-622). Sentry groups transactions by name, so that is one bucket for the whole service. The PR is explicit this is a floor and tracing is off, so the DoD's "route still identifiable" is met on the error dispatch; recording it so enabling tracing later is not done blind. Relatedly README.md:1063 "POST /webhook/{uuid} where the pattern is known" describes a state unreachable today.
sentryTransactionName cuts on the first space, so any non-METHOD /path name containing a space becomes firstWord /(redacted) (probed: my custom transaction <uuid> → my /(redacted)). Safe, and unreachable today since only sentryhttp sets the name.
Unclaimed bonus: rebuilding from parsed.Scheme/parsed.Host also strips URL userinfo — http://user:pw@example.com/... → http://example.com/(redacted).
Disclosure: the adversarial probe below ran as throwaway tests in a copy of the tree, i.e. go test outside the make targets, on a scrap copy and not on the reviewed tree; nothing was written to the repo. Gate evidence below is make/Docker only.

No concrete path survives, and no third field carries it

14 probes through a real chi mux with real sentryhttp, all clean: 404 NotFound panic, 405 MethodNotAllowed panic, panic in a Use middleware ahead of routing, panic in a Group middleware after routing, CaptureException and CaptureMessage called directly from a handler, Mount of a subrouter, Mount of a bare handler, wildcard /static/*, tracing on with and without a panic, and nine URL shapes (IPv6, explicit port, empty host, userinfo, query, fragment, uppercase scheme, scheme-relative, unparseable). Every one yielded the pattern or /(redacted), never the concrete path. A panic in a middleware registered ahead of sentryhttp produces no event at all. The wildcard returns the literal /static/* and never the matched remainder — chi context.go:122-135 joins registered patterns only.

Third field: none. The full marshalled transaction event carries contexts.trace with only op/span_id/trace_id — no description, because the SDK never sets Span.Description for the root sentryhttp transaction — and no spans, breadcrumbs, tags, extra or fingerprint bearing the path. event.Transaction is assigned in exactly one place in the SDK (tracing.go:553).

Mutations reproduced independently: URL rewrite disabled → 4 tests fail including _ReplacesTheCapabilityPathWithTheRoutePattern; transaction-name rewrite disabled → 3 tests fail and _ReplacesTheCapabilityPathWithTheRoutePattern still passes. Separately covered, confirmed.

Gate evidence

make check exit 0, zero (cached).

docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0:

#17 [lint 7/9]  RUN make fmt-check                       DONE  5.0s
#18 [lint 8/9]  RUN golangci-lint config verify          DONE  5.8s
#19 [lint 9/9]  RUN golangci-lint run ./...  0 issues.   DONE 52.1s
#32 [builder  9/11] RUN make test                        DONE ~54s
#36 [builder 10/11] RUN make build

13 packages with real durations (internal/ciscript 7.119s, internal/handlers 5.473s, internal/delivery 4.065s, internal/server 2.909s, ...), zero (cached), 725 PASS lines, all 13 TestSentryScrub_* green, 0 FAIL. No layer that runs a check was CACHED. Tagged image removed, docker ps -a clean, no prune of any kind.

Also verified: CI green on 5ff7cdb; merges clean into next (76725cf); exactly one commit; title ends (closes #179); TODO.md untouched; no Claude/Anthropic references or attribution trailers; make fmt a no-op; inclusive terminology; scheme preserved and asserted in three tests; chi import path consistent at v1.5.5 repo-wide. The gomodguard deprecation warning is pre-existing and tracked at #98.

FAIL — needs-rework Reviewed at `5ff7cdb`, fresh clone, against https://git.eeqj.de/sneak/webhooker/issues/179. ## Finding — the only blocker **A security rationale stated twice that is not true of the code.** - `README.md:1052-1054` — "The host is operator configuration rather than anything a client chooses" - `internal/server/sentry.go:155-156` — "it is operator configuration, not a client-supplied or capability-bearing value" The host in the rebuilt URL is `parsed.Host` of the SDK-built `scheme://r.Host/path` (`interfaces.go:183`), and `r.Host` is the client's `Host` header / request-line authority. This service validates no hostname: there is no host allowlist or configured hostname anywhere under `internal/` (`internal/config` has no host key), and `internal/handlers/source_management.go:402` already takes `r.Host` at face value to build the displayed `BaseURL`. Behind a reverse proxy that rewrites `Host` the claim holds; on a directly-exposed deployment the client sets that value and it is reflected verbatim into the Sentry URL. Why it matters: this is the stated justification for keeping a field in a hook whose whole job is deciding what may cross a trust boundary. A future reader carries the premise ("host is operator config") to the next field and it is wrong. The decision to keep the host is correct and must not change — the second half of the same sentence is the justification that actually holds. Acceptable: reword both places to rest on the true reason — the host is whatever `r.Host` carried and is already shipped by the allowlisted `Host` header, so dropping it from the URL would withhold nothing that is not sent anyway. Drop or qualify "operator configuration, not client-chosen". No behaviour change, no test change. ## Notes and disclosures — not blocking - If tracing is ever enabled, every transaction collapses to `POST /(redacted)` and `Request.URL` to `scheme://host/(redacted)`: the pattern is never reachable on that dispatch (confirmed — nil hint at `tracing.go:356`, replaced by `&EventHint{}` at `client.go:620-622`). Sentry groups transactions by name, so that is one bucket for the whole service. The PR is explicit this is a floor and tracing is off, so the DoD's "route still identifiable" is met on the error dispatch; recording it so enabling tracing later is not done blind. Relatedly `README.md:1063` "`POST /webhook/{uuid}` where the pattern is known" describes a state unreachable today. - `sentryTransactionName` cuts on the first space, so any non-`METHOD /path` name containing a space becomes `firstWord /(redacted)` (probed: `my custom transaction <uuid>` → `my /(redacted)`). Safe, and unreachable today since only `sentryhttp` sets the name. - Unclaimed bonus: rebuilding from `parsed.Scheme`/`parsed.Host` also strips URL userinfo — `http://user:pw@example.com/...` → `http://example.com/(redacted)`. - Disclosure: the adversarial probe below ran as throwaway tests in a **copy** of the tree, i.e. `go test` outside the make targets, on a scrap copy and not on the reviewed tree; nothing was written to the repo. Gate evidence below is make/Docker only. ## No concrete path survives, and no third field carries it 14 probes through a real chi mux with real `sentryhttp`, all clean: 404 NotFound panic, 405 MethodNotAllowed panic, panic in a `Use` middleware ahead of routing, panic in a `Group` middleware after routing, `CaptureException` and `CaptureMessage` called directly from a handler, `Mount` of a subrouter, `Mount` of a bare handler, wildcard `/static/*`, tracing on with and without a panic, and nine URL shapes (IPv6, explicit port, empty host, userinfo, query, fragment, uppercase scheme, scheme-relative, unparseable). Every one yielded the pattern or `/(redacted)`, never the concrete path. A panic in a middleware registered ahead of `sentryhttp` produces no event at all. The wildcard returns the literal `/static/*` and never the matched remainder — chi `context.go:122-135` joins registered patterns only. Third field: none. The full marshalled transaction event carries `contexts.trace` with only `op`/`span_id`/`trace_id` — no `description`, because the SDK never sets `Span.Description` for the root `sentryhttp` transaction — and no spans, breadcrumbs, tags, extra or fingerprint bearing the path. `event.Transaction` is assigned in exactly one place in the SDK (`tracing.go:553`). Mutations reproduced independently: URL rewrite disabled → 4 tests fail including `_ReplacesTheCapabilityPathWithTheRoutePattern`; transaction-name rewrite disabled → 3 tests fail and `_ReplacesTheCapabilityPathWithTheRoutePattern` still passes. Separately covered, confirmed. ## Gate evidence `make check` exit 0, **zero** `(cached)`. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0: ``` #17 [lint 7/9] RUN make fmt-check DONE 5.0s #18 [lint 8/9] RUN golangci-lint config verify DONE 5.8s #19 [lint 9/9] RUN golangci-lint run ./... 0 issues. DONE 52.1s #32 [builder 9/11] RUN make test DONE ~54s #36 [builder 10/11] RUN make build ``` 13 packages with real durations (`internal/ciscript 7.119s`, `internal/handlers 5.473s`, `internal/delivery 4.065s`, `internal/server 2.909s`, ...), **zero** `(cached)`, 725 PASS lines, all 13 `TestSentryScrub_*` green, 0 FAIL. No layer that runs a check was CACHED. Tagged image removed, `docker ps -a` clean, no prune of any kind. Also verified: CI green on `5ff7cdb`; merges clean into `next` (`76725cf`); exactly one commit; title ends ` (closes #179)`; `TODO.md` untouched; no Claude/Anthropic references or attribution trailers; `make fmt` a no-op; inclusive terminology; scheme preserved and asserted in three tests; chi import path consistent at v1.5.5 repo-wide. The `gomodguard` deprecation warning is pre-existing and tracked at https://git.eeqj.de/sneak/webhooker/issues/98.

clawbot added needs-rework and removed needs-review labels 2026-08-18 02:33:48 +02:00

clawbot referenced this pull request

2026-08-18 02:34:32 +02:00

If tracing is ever enabled, every Sentry transaction will collapse into one bucket named POST /(redacted) #185

clawbot force-pushed issue-179-sentry-route-pattern from 5ff7cdbdb5 to fb5a203896

2026-08-18 02:40:50 +02:00

Compare

clawbot merged commit b573959a26 into next

2026-08-18 02:42:58 +02:00

clawbot deleted branch issue-179-sentry-route-pattern

2026-08-18 02:42:59 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#181