sneak/webhooker
1
0
Fork 0
Code Issues 34 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Read form fields from the POST body only (closes #160) #174

Merged
clawbot merged 1 commits from issue-160-postformvalue-credential-leak into next 2026-08-18 02:04:10 +02:00
Conversation 3 Commits 1 Files Changed 13 +775 -27
clawbot commented 2026-08-18 00:53:40 +02:00
Collaborator
Copy Link

Closes #160.

The defect

internal/handlers/source_management.go read the target destination with r.FormValue("url"), which falls back to the URL query string when the field is absent from the POST body. So POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/SECRET with an empty url field created a working target from a value carried on the request line — where logs, proxies, Referer headers and error trackers record it.

Every form read in these handlers is now r.PostFormValue.

What #146 changed about exploitability

The access-log half of the report is already mitigated and was before this PR. accessLogURL logs the chi route pattern for 3xx/4xx and concreteLogURL replaces the query with ?(redacted) on the 2xx/5xx branches — no branch keeps a query string. Verified by the mutation run below: with r.FormValue restored the access-log assertion still passed while the storage assertion failed.

Two halves are unaffected by that mitigation and are what this PR fixes:

  1. A query value must never populate a target's configuration. The request line is the wrong place to read a credential from, and the leak surface includes proxy logs, browser history and Referer — none of which this service controls.
  2. Sentry. Live, and independent of the access log.

Sentry: what the SDK collects, and the decision for every Request field

sentryhttp attaches the whole *http.Request to the scope (sentryhttp.go:113), and Scope.ApplyToEvent (scope.go:400-418) fills the event's Request from it inside prepareEvent (client.go:688) — before BeforeSend runs (client.go:629). SendDefaultPII is false, which strips Authorization, Cookie, X-Forwarded-For and X-Real-Ip from the header map and suppresses Cookies/Env. It does not cover everything the SDK copies.

This PR makes that worse before it makes it better: reading every field from the body only means the body is now the sole place the target URL, the login password and both password-change fields are submitted — so it points every credential it protects at Sentry's request context.

Decision per field of sentry.Request (interfaces.go:164-172), all implemented in scrubSentryRequest:

Field How the SDK fills it Decision
URL NewRequest builds scheme://host/path — path only, query excluded. Kept. It is what names the failing route. See the open question below about the receiver path.
Method From r.Method. Kept. Server-side, bounded, pure signal.
Data SetRequest tees the first 10 KiB of r.Body into a buffer (scope.go:121-135); ApplyToEvent copies it into the event at scope.go:415-416 with no SendDefaultPII guard. The buffer fills precisely because the handlers call ParseForm. Replaced with (redacted). This was the finding.
QueryString r.URL.RawQuery verbatim, no guard. Replaced with (redacted). Client-chosen on every route; page is the only query parameter this service reads.
Cookies Only under SendDefaultPII, so empty today. Cleared so it stays empty if that option is ever turned on.
Headers Every inbound header, minus four names, unconditionally. Under SendDefaultPII the filter is skipped entirely. Reduced to an allowlist: Accept, Content-Length, Content-Type, Host, Origin, Referer, User-Agent, X-Request-Id.
Env Only under SendDefaultPII: REMOTE_ADDR/REMOTE_PORT. Cleared, same reason as Cookies.

The hook is a floor, not a default: nothing it clears comes back if SendDefaultPII is flipped.

Why Headers needed handling rather than a justification

It is a blocklist of four in a service where two other header values are credentials, so an unrecognised header ships verbatim:

  • X-Csrf-Token — gorilla/csrf accepts the token in the header in place of the form field (helpers.go:107-109, header name set at csrf.go:41).
  • On /webhook/{uuid}, the shared secrets senders attach: X-Gitlab-Token outright, plus the per-provider signature headers.

An allowlist makes an unknown header safe by construction. Reproduced by mutation below.

Nothing the allowlist drops is needed for its likeliest use, debugging a CSRF rejection. That has three inputs: the TLS decision, Origin and Referer. The latter two are kept. The first is already carried by the retained Request.URL, because interfaces.go:181-183 derives that URL's scheme from r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" — byte for byte the predicate internal/middleware/csrf.go:19 uses to choose between the csrf.Secure(true) and csrf.Secure(false) handlers. The retained scheme therefore is the CSRF TLS decision, and dropping X-Forwarded-Proto costs nothing. Referer stays for the reason already agreed in review: browser-set, this service emits only ?page= in its own links, and Referrer-Policy: strict-origin-when-cross-origin is set. X-Request-Id stays because it ties the event to the local access log line, which holds the detail the allowlist drops. The dropped provider headers (X-GitHub-Event, X-Gitlab-Event and the like) are real signal but are recorded locally on Event.Headers, and Sentry-Trace/Baggage are already reflected in contexts.trace.

Clear vs. redact, and why the body is redacted on every route

Replaced with a fixed marker, on every route, not filtered per route and not selectively redacted. That is a deliberate choice, not a forced one.

  • The route is reachable from the hook. sentryhttp's recover path calls hub.RecoverWithContext(context.WithValue(r.Context(), sentry.RequestContextKey, r), err) (sentryhttp.go:123-126). The hint built at hub.go:344 carries no request — that is the part the previous revision of this description saw and wrongly generalised from — but client.go:480-487 then sets hint.Context = ctx, and client.go:629-631 hands that hint to BeforeSend. So hint.Context.Value(sentry.RequestContextKey) yields the live request and chi's RoutePattern() yields the matched pattern. sentry.Init and sentry.Flush are the only other SDK calls in the tree, so there is no live path lacking the request. Credit to review for proving this rather than inferring it.
  • Nothing debuggable is lost. Every handler reads its fields with PostFormValue, so the body is exactly where the credentials are. The one route whose body is genuine signal is the receiver, /webhook/{uuid} — and that body is already stored on the Event row and served from the UI, so a third-party tracker is not where anyone reads it.
  • An unconditional rule cannot leak on a route somebody forgets to add to it, which a route-conditional one can.
  • A marker rather than an empty string, so a reader can tell a suppressed body from a request that had none.

Open question for the owner, not filed and not changed

Request.URL keeps the concrete path, which on the receiver route is /webhook/{uuid} — a capability identifier. That matches the deliberate rule set by #146 for the local access log ("2xx and 5xx responses keep the concrete path"), so it is left alone here. Whether that rule should extend to a third-party service is a scope question, not a defect, so it is raised rather than filed. Review's view is that it warrants its own issue and that the rule does not transfer across that trust boundary; the hint.Context mechanism above is what would make shipping the pattern instead of the concrete UUID cheap.

Every r.FormValue call, converted or left

Location Field(s) Why
source_management.go processTargetCreate name, type, url, max_retries, expiry url is the reported defect. The other four go with it: a target's stored configuration must not be settable from the request line at all.
source_management.go HandleSourceCreateSubmit name, description, retention_days retention_days is the data-retention policy.
source_management.go applyWebhookEdit name, description, retention_days Same, on the edit path.
source_management.go entrypoint create description Stored user data on a POST form.
auth.go HandleLoginSubmit username, password Outside the named file, converted anyway: a password readable from the query string is the same defect in its most acute form.
profile.go password change current_password, new_password, confirm_password Same reason.

Left unchanged, deliberately: source_management.go r.URL.Query().Get("page") — the one intentional query read (pagination links), not secret-bearing, and it already uses r.URL.Query(). No other r.FormValue call exists in the tree.

Second item: json:"-"

TargetView is the masking barrier for the HTML path only, and the /api/v1 group exists and is empty, so the first handler to marshal a model would serialise the credential. Nothing marshals these models today, so this is a tag change with no behaviour change.

  • database.Target.Config — holds the incoming-webhook URL whose path segments are the bearer token.
  • database.APIKey.Key — a bearer token outright.
  • database.Setting.Value — the settings table holds exactly one key today, session_key, the session encryption key.

Checked and left: Event.Headers and Event.Body are the product's own recorded payload, deliberately rendered; DeliveryResult.Error was masked by #118; DeliveryResult.ResponseBody is a third-party response, not our credential.

Tests

  • internal/server/sentry_test.go — rewritten to use the real construction path. The previous version built the event with sentry.NewRequest, which documentedly never reads the body, so Request.Data could not appear on it and the leak was unreachable by the assertions. It now panics inside a form handler wrapped in a real sentryhttp middleware, with a client built from the production options (sentryClientOptions, shared with enableSentry) and only the transport swapped for a recorder — so the event goes through SetRequest → ParseForm → ApplyToEvent → BeforeSend exactly as in production. Markers are planted in the body, the query and X-Csrf-Token; the scrubbed case asserts no byte of any of them survives into the marshalled event, and a companion case asserts the SDK does collect all three unscrubbed, pinning the hook's premise.
  • internal/handlers/target_create_query_test.go — POSTs ?url=<secret> with name/type in the body (so the request reaches the url read rather than failing earlier) and asserts a 400, no stored target, and no secret in the access log, behind the production Logging middleware on a real chi route. Plus a positive control and a case covering name/type/max_retries/expiry. The secret URL uses a literal public address, not a hostname, so a sandbox without DNS cannot make the test pass for the wrong reason.
  • internal/database/model_secrets_test.go — marshals each model, asserts the secret is absent and a non-secret field survives, including through the Webhook.Targets association.

Mutation checks

1. The Data clear removed (the finding). Marshalled event, straight from the failure output:

"request":{"url":"http://example.com/pages/login","method":"POST",
"data":"password=QQSENTRYBODYMARKERQQ&username=admin",
"query_string":"(redacted)", ...}

    Error: "..." should not contain "QQSENTRYBODYMARKERQQ"
    Error: Not equal:
           expected: "(redacted)"
           actual  : "password=QQSENTRYBODYMARKERQQ&username=admin"
--- FAIL: TestSentryScrub_RedactsTheCapturedRequest (0.01s)

2. The header allowlist removed:

"headers":{"Content-Type":"application/x-www-form-urlencoded",
"Host":"example.com","User-Agent":"webhooker-test-agent",
"X-Csrf-Token":"QQSENTRYHEADERMARKERQQ"}

    Error: "..." should not contain "QQSENTRYHEADERMARKERQQ"
--- FAIL: TestSentryScrub_RedactsTheCapturedRequest (0.01s)

3. targetURL reverted to r.FormValue (from an earlier round, unchanged): the query value reached storage — the storage assertion and the 400-vs-303 status failed, while the access-log assertions passed, which is the #146 mitigation showing through.

All three restored afterwards.

Gates

make check exits 0. Disclosure: on a repeat run the host test cache reports (cached) for every package, so a repeat run is not evidence on its own — the first run on this tree executed all 13 packages with zero (cached), and the Docker run below is what this rests on.

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

#17 [lint 7/9] RUN make fmt-check                                     DONE 2.0s
#18 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE 5.3s
#19 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./...
#19 50.46 0 issues.                                                   DONE 51.8s
#32 [builder  9/11] RUN make test
#32 55.36 ok  internal/handlers   3.782s
#32 55.36 ok  internal/delivery   4.368s
#32 55.36 ok  internal/database   2.250s
#32 55.36 ok  internal/server     2.045s
#33 [builder 10/11] RUN make build                                    DONE 44.3s

Zero (cached) markers anywhere in the log; real per-package durations across all 13 packages; 586 PASS lines in the container run, including all four TestSentryScrub_*, all three TestHandleTargetCreate_* and TestModelsDoNotMarshalTheirSecrets. Lint ran in the pinned golangci/golangci-lint:v2.12.2 image. Disclosure: --no-cache-filter leaves the pre-COPY . dependency layers (go mod download, apt-get) cached; every layer that runs a check is uncached. The tagged image was removed and no container of this run survives.

Also touched

README.md — the access-log section's Sentry paragraphs. They now state the body and header handling, that the route is reachable from the hook via hint.Context and that unconditional redaction is a choice rather than a limitation, and the X-Forwarded-Proto / CSRF-predicate reasoning for the allowlist. TODO.md untouched.

Noted, not fixed

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

Closes https://git.eeqj.de/sneak/webhooker/issues/160. ## The defect `internal/handlers/source_management.go` read the target destination with `r.FormValue("url")`, which falls back to the URL query string when the field is absent from the POST body. So `POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/SECRET` with an empty `url` field created a working target from a value carried on the request line — where logs, proxies, `Referer` headers and error trackers record it. Every form read in these handlers is now `r.PostFormValue`. ## What https://git.eeqj.de/sneak/webhooker/issues/146 changed about exploitability The access-log half of the report is **already mitigated** and was before this PR. `accessLogURL` logs the chi route pattern for 3xx/4xx and `concreteLogURL` replaces the query with `?(redacted)` on the 2xx/5xx branches — no branch keeps a query string. Verified by the mutation run below: with `r.FormValue` restored the access-log assertion still passed while the storage assertion failed. Two halves are unaffected by that mitigation and are what this PR fixes: 1. **A query value must never populate a target's configuration.** The request line is the wrong place to read a credential from, and the leak surface includes proxy logs, browser history and `Referer` — none of which this service controls. 2. **Sentry.** Live, and independent of the access log. ## Sentry: what the SDK collects, and the decision for every `Request` field `sentryhttp` attaches the whole `*http.Request` to the scope (`sentryhttp.go:113`), and `Scope.ApplyToEvent` (`scope.go:400-418`) fills the event's `Request` from it inside `prepareEvent` (`client.go:688`) — before `BeforeSend` runs (`client.go:629`). `SendDefaultPII` is false, which strips `Authorization`, `Cookie`, `X-Forwarded-For` and `X-Real-Ip` from the header map and suppresses `Cookies`/`Env`. It does **not** cover everything the SDK copies. **This PR makes that worse before it makes it better:** reading every field from the body only means the body is now the sole place the target URL, the login password and both password-change fields are submitted — so it points every credential it protects at Sentry's request context. Decision per field of `sentry.Request` (`interfaces.go:164-172`), all implemented in `scrubSentryRequest`: | Field | How the SDK fills it | Decision | | --- | --- | --- | | `URL` | `NewRequest` builds `scheme://host/path` — path only, query excluded. | **Kept.** It is what names the failing route. See the open question below about the receiver path. | | `Method` | From `r.Method`. | **Kept.** Server-side, bounded, pure signal. | | `Data` | `SetRequest` tees the first 10 KiB of `r.Body` into a buffer (`scope.go:121-135`); `ApplyToEvent` copies it into the event at `scope.go:415-416` with **no `SendDefaultPII` guard**. The buffer fills precisely because the handlers call `ParseForm`. | **Replaced with `(redacted)`.** This was the finding. | | `QueryString` | `r.URL.RawQuery` verbatim, no guard. | **Replaced with `(redacted)`.** Client-chosen on every route; `page` is the only query parameter this service reads. | | `Cookies` | Only under `SendDefaultPII`, so empty today. | **Cleared** so it stays empty if that option is ever turned on. | | `Headers` | Every inbound header, **minus four names**, unconditionally. Under `SendDefaultPII` the filter is skipped entirely. | **Reduced to an allowlist:** `Accept`, `Content-Length`, `Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent`, `X-Request-Id`. | | `Env` | Only under `SendDefaultPII`: `REMOTE_ADDR`/`REMOTE_PORT`. | **Cleared,** same reason as `Cookies`. | The hook is a floor, not a default: nothing it clears comes back if `SendDefaultPII` is flipped. ### Why `Headers` needed handling rather than a justification It is a blocklist of four in a service where two other header values are credentials, so an unrecognised header ships verbatim: - `X-Csrf-Token` — `gorilla/csrf` accepts the token in the header in place of the form field (`helpers.go:107-109`, header name set at `csrf.go:41`). - On `/webhook/{uuid}`, the shared secrets senders attach: `X-Gitlab-Token` outright, plus the per-provider signature headers. An allowlist makes an unknown header safe by construction. Reproduced by mutation below. Nothing the allowlist drops is needed for its likeliest use, debugging a CSRF rejection. That has three inputs: the TLS decision, `Origin` and `Referer`. The latter two are kept. The first is already carried by the retained `Request.URL`, because `interfaces.go:181-183` derives that URL's scheme from `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` — byte for byte the predicate `internal/middleware/csrf.go:19` uses to choose between the `csrf.Secure(true)` and `csrf.Secure(false)` handlers. The retained scheme therefore **is** the CSRF TLS decision, and dropping `X-Forwarded-Proto` costs nothing. `Referer` stays for the reason already agreed in review: browser-set, this service emits only `?page=` in its own links, and `Referrer-Policy: strict-origin-when-cross-origin` is set. `X-Request-Id` stays because it ties the event to the local access log line, which holds the detail the allowlist drops. The dropped provider headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are real signal but are recorded locally on `Event.Headers`, and `Sentry-Trace`/`Baggage` are already reflected in `contexts.trace`. ### Clear vs. redact, and why the body is redacted on every route Replaced with a fixed marker, on every route, not filtered per route and not selectively redacted. That is a deliberate choice, not a forced one. - **The route is reachable from the hook.** `sentryhttp`'s recover path calls `hub.RecoverWithContext(context.WithValue(r.Context(), sentry.RequestContextKey, r), err)` (`sentryhttp.go:123-126`). The hint built at `hub.go:344` carries no request — that is the part the previous revision of this description saw and wrongly generalised from — but `client.go:480-487` then sets `hint.Context = ctx`, and `client.go:629-631` hands that hint to `BeforeSend`. So `hint.Context.Value(sentry.RequestContextKey)` yields the live request and chi's `RoutePattern()` yields the matched pattern. `sentry.Init` and `sentry.Flush` are the only other SDK calls in the tree, so there is no live path lacking the request. Credit to review for proving this rather than inferring it. - **Nothing debuggable is lost.** Every handler reads its fields with `PostFormValue`, so the body is exactly where the credentials are. The one route whose body is genuine signal is the receiver, `/webhook/{uuid}` — and that body is already stored on the `Event` row and served from the UI, so a third-party tracker is not where anyone reads it. - **An unconditional rule cannot leak on a route somebody forgets to add to it,** which a route-conditional one can. - A marker rather than an empty string, so a reader can tell a suppressed body from a request that had none. ### Open question for the owner, not filed and not changed `Request.URL` keeps the concrete path, which on the receiver route is `/webhook/{uuid}` — a capability identifier. That matches the deliberate rule set by https://git.eeqj.de/sneak/webhooker/issues/146 for the local access log ("2xx and 5xx responses keep the concrete path"), so it is left alone here. Whether that rule should extend to a third-party service is a scope question, not a defect, so it is raised rather than filed. Review's view is that it warrants its own issue and that the rule does not transfer across that trust boundary; the `hint.Context` mechanism above is what would make shipping the pattern instead of the concrete UUID cheap. ## Every `r.FormValue` call, converted or left | Location | Field(s) | Why | | --- | --- | --- | | `source_management.go` `processTargetCreate` | `name`, `type`, `url`, `max_retries`, `expiry` | `url` is the reported defect. The other four go with it: a target's stored configuration must not be settable from the request line at all. | | `source_management.go` `HandleSourceCreateSubmit` | `name`, `description`, `retention_days` | `retention_days` is the data-retention policy. | | `source_management.go` `applyWebhookEdit` | `name`, `description`, `retention_days` | Same, on the edit path. | | `source_management.go` entrypoint create | `description` | Stored user data on a POST form. | | `auth.go` `HandleLoginSubmit` | `username`, `password` | Outside the named file, converted anyway: a password readable from the query string is the same defect in its most acute form. | | `profile.go` password change | `current_password`, `new_password`, `confirm_password` | Same reason. | Left unchanged, deliberately: `source_management.go` `r.URL.Query().Get("page")` — the one intentional query read (pagination links), not secret-bearing, and it already uses `r.URL.Query()`. No other `r.FormValue` call exists in the tree. ## Second item: `json:"-"` `TargetView` is the masking barrier for the HTML path only, and the `/api/v1` group exists and is empty, so the first handler to marshal a model would serialise the credential. Nothing marshals these models today, so this is a tag change with no behaviour change. - `database.Target.Config` — holds the incoming-webhook URL whose path segments are the bearer token. - `database.APIKey.Key` — a bearer token outright. - `database.Setting.Value` — the settings table holds exactly one key today, `session_key`, the session encryption key. Checked and left: `Event.Headers` and `Event.Body` are the product's own recorded payload, deliberately rendered; `DeliveryResult.Error` was masked by https://git.eeqj.de/sneak/webhooker/issues/118; `DeliveryResult.ResponseBody` is a third-party response, not our credential. ## Tests - `internal/server/sentry_test.go` — **rewritten to use the real construction path.** The previous version built the event with `sentry.NewRequest`, which documentedly never reads the body, so `Request.Data` could not appear on it and the leak was unreachable by the assertions. It now panics inside a form handler wrapped in a real `sentryhttp` middleware, with a client built from the production options (`sentryClientOptions`, shared with `enableSentry`) and only the transport swapped for a recorder — so the event goes through `SetRequest` → `ParseForm` → `ApplyToEvent` → `BeforeSend` exactly as in production. Markers are planted in the body, the query and `X-Csrf-Token`; the scrubbed case asserts no byte of any of them survives into the marshalled event, and a companion case asserts the SDK does collect all three unscrubbed, pinning the hook's premise. - `internal/handlers/target_create_query_test.go` — POSTs `?url=<secret>` with `name`/`type` in the body (so the request reaches the `url` read rather than failing earlier) and asserts a 400, no stored target, and no secret in the access log, behind the production `Logging` middleware on a real chi route. Plus a positive control and a case covering `name`/`type`/`max_retries`/`expiry`. The secret URL uses a literal public address, not a hostname, so a sandbox without DNS cannot make the test pass for the wrong reason. - `internal/database/model_secrets_test.go` — marshals each model, asserts the secret is absent and a non-secret field survives, including through the `Webhook.Targets` association. ## Mutation checks **1. The `Data` clear removed** (the finding). Marshalled event, straight from the failure output: ``` "request":{"url":"http://example.com/pages/login","method":"POST", "data":"password=QQSENTRYBODYMARKERQQ&username=admin", "query_string":"(redacted)", ...} ``` ``` Error: "..." should not contain "QQSENTRYBODYMARKERQQ" Error: Not equal: expected: "(redacted)" actual : "password=QQSENTRYBODYMARKERQQ&username=admin" --- FAIL: TestSentryScrub_RedactsTheCapturedRequest (0.01s) ``` **2. The header allowlist removed:** ``` "headers":{"Content-Type":"application/x-www-form-urlencoded", "Host":"example.com","User-Agent":"webhooker-test-agent", "X-Csrf-Token":"QQSENTRYHEADERMARKERQQ"} ``` ``` Error: "..." should not contain "QQSENTRYHEADERMARKERQQ" --- FAIL: TestSentryScrub_RedactsTheCapturedRequest (0.01s) ``` **3. `targetURL` reverted to `r.FormValue`** (from an earlier round, unchanged): the query value reached storage — the **storage** assertion and the 400-vs-303 status failed, while the access-log assertions passed, which is the https://git.eeqj.de/sneak/webhooker/issues/146 mitigation showing through. All three restored afterwards. ## Gates `make check` exits 0. Disclosure: on a repeat run the host test cache reports `(cached)` for every package, so a repeat run is not evidence on its own — the first run on this tree executed all 13 packages with zero `(cached)`, and the Docker run below is what this rests on. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exits 0: ``` #17 [lint 7/9] RUN make fmt-check DONE 2.0s #18 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE 5.3s #19 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./... #19 50.46 0 issues. DONE 51.8s #32 [builder 9/11] RUN make test #32 55.36 ok internal/handlers 3.782s #32 55.36 ok internal/delivery 4.368s #32 55.36 ok internal/database 2.250s #32 55.36 ok internal/server 2.045s #33 [builder 10/11] RUN make build DONE 44.3s ``` Zero `(cached)` markers anywhere in the log; real per-package durations across all 13 packages; 586 `PASS` lines in the container run, including all four `TestSentryScrub_*`, all three `TestHandleTargetCreate_*` and `TestModelsDoNotMarshalTheirSecrets`. Lint ran in the pinned `golangci/golangci-lint:v2.12.2` image. Disclosure: `--no-cache-filter` leaves the pre-`COPY .` dependency layers (`go mod download`, `apt-get`) cached; every layer that runs a check is uncached. The tagged image was removed and no container of this run survives. ## Also touched `README.md` — the access-log section's Sentry paragraphs. They now state the body and header handling, that the route **is** reachable from the hook via `hint.Context` and that unconditional redaction is a choice rather than a limitation, and the `X-Forwarded-Proto` / CSRF-predicate reasoning for the allowlist. `TODO.md` untouched. ## Noted, not fixed The pinned linter emits `The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2` on every run. Pre-existing and tracked at https://git.eeqj.de/sneak/webhooker/issues/98.
clawbot added 1 commit 2026-08-18 00:53:41 +02:00
Read form fields from the POST body only (closes #160)
All checks were successful
check / check (push) Successful in 2m54s
Details
3925fce24a 
internal/handlers/source_management.go read the target destination
with r.FormValue, which falls back to the URL query string when the
field is absent from the body. So

    POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/S

created a working target from a value carried on the request line,
where logs, proxies, Referer headers and error trackers record it.
That is the remaining ingress path of the credential-exposure class
the render, delivery-error and log-line paths were each closed for.

Every form read in these handlers is now r.PostFormValue, so no
query-string value can populate stored configuration or be taken as a
credential. The one deliberate query read, `page` on the authenticated
pagination links, is untouched: it uses r.URL.Query().Get already.

The access log no longer carries the query on any branch, so the log
half of the report is already mitigated; the Sentry half is not. The
SDK attaches the request to every captured event and copies
r.URL.RawQuery into Request.QueryString independently of the access
log, so a BeforeSend hook clears that field before an event leaves the
process. Scheme, host, path and method stay, which is what names the
failing route.

Second barrier, for the JSON path that does not exist yet: the fields
that hold a credential are tagged json:"-" so the first handler to
marshal a model cannot serialise one. Target.Config holds the
incoming-webhook URL, APIKey.Key is a bearer token, and Setting.Value
holds the session encryption key. delivery.TargetView remains the
masking barrier for the HTML path, which is unaffected.
clawbot added the needs-review label 2026-08-18 00:53:51 +02:00
clawbot self-assigned this 2026-08-18 00:53:54 +02:00
clawbot commented 2026-08-18 01:07:39 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

The Sentry leg is not closed: Request.Data ships the POST body verbatim

internal/server/sentry.go:34-40 — scrubSentryRequest clears only event.Request.QueryString. The same Request struct carries Data, which holds the raw POST body, and nothing clears it.

Chain, in sentry-go v0.25.0:

  • internal/server/routes.go:48-52 installs sentryhttp as a global router middleware, so it covers /pages/login, /user/{u}/password and /source/{id}/targets.
  • sentryhttp.go:113 calls hub.Scope().SetRequest(r).
  • scope.go:121-135 — SetRequest wraps r.Body in an io.TeeReader into a 10 KiB buffer whenever ContentLength is at or under maxRequestBodyBytes (10240) and a body exists. Form posts qualify.
  • scope.go:415-416 — ApplyToEvent does event.Request.Data = string(scope.requestBody.Bytes()). Unlike Cookies and the header filter, this copy has no SendDefaultPII guard — it is unconditional.
  • client.go:688 runs ApplyToEvent inside prepareEvent, and client.go:629 runs BeforeSend afterwards. So the hook does see Data populated and could clear it.

The buffer fills precisely because these handlers call r.ParseForm(), which drains the TeeReader.

Why this is in scope, and why the PR makes it sharper. Issue #160 puts the Sentry path in scope explicitly ("Confirm whether the Sentry integration attaches the raw URL independently of the access log; if it does, that path is in scope too"). This change makes the POST body the only place these fields are read from — so it points every credential it protects at the one Sentry field it does not scrub. With the PR applied, on any error or panic captured while serving the request:

  • POST /source/{id}/targets with url=https://hooks.slack.com/services/T/B/SECRET in the body — the exact credential of #160, whose path segments are the bearer token per #115 — is shipped off-host in Request.Data.
  • POST /pages/login ships username=...&password=... verbatim.
  • POST /user/{u}/password ships current_password and new_password.

Reproduced against sentry-go v0.25.0 in a scratch module, using this PR's hook verbatim behind a real sentryhttp handler that calls ParseForm() then CaptureException:

Request.QueryString = "(redacted)"
Request.URL         = "http://example.com/login"
Request.Data        = "username=admin&password=SUPERSECRETPW123"

Why the new tests do not catch it. internal/server/sentry_test.go:30-42 builds the event as event.Request = sentry.NewRequest(req) with a nil body. sentry.NewRequest explicitly never reads the body (interfaces.go:175-177), and Request.Data is populated only by Scope.ApplyToEvent. The tests therefore exercise a construction path on which this leak cannot appear, which is why the suite reports the leg closed. The tests are not vacuous for what they assert — they just assert on the wrong object.

Acceptable fix. scrubSentryRequest must also clear event.Request.Data (clearing Cookies too is cheap insurance if SendDefaultPII is ever flipped). Plus a regression test that drives a real captured event through sentryhttp and Scope.ApplyToEvent with a form body containing a marker, asserting no byte of the marker survives into the marshalled event — a hand-built sentry.NewRequest cannot regress-test this.

The README paragraph added at README.md:1023-1031 is accurate as written (it is scoped to the query string); it should not be broadened to claim the Sentry path is closed until Data is handled.

Verified and passing

Scope expansion into auth.go/profile.go is correct and safe: no template, redirect, JS path or test supplies any converted field by query (only page, at source_management.go:826, which is untouched and still passes); no enctype="multipart/form-data" or ParseMultipartForm exists anywhere, so the ParseForm-then-PostFormValue ordering concern is moot; gorilla/csrf already read its token body-only via r.PostFormValue (helpers.go:113) and still parses the form ahead of the handlers; login and password-change rate limiting is IP-keyed and reads no form field. No r.FormValue remains in the tree.

json:"-" on Target.Config, APIKey.Key, Setting.Value is safe: nothing marshals or unmarshals those models (the sole json.NewEncoder is respondJSON, whose only caller passes a healthcheck struct), no gorm:"serializer:..." exists anywhere, the gorm: tags are byte-identical across the diff, and the repo contains no .json fixtures. model_secrets_test.go is meaningful — the keptField assertion proves the marshal produced output, and restoring the three tags fails exactly those subtests.

Mutation reproduced: reverting targetURL to r.FormValue fails TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget on the storage assertion (target_create_query_test.go:148, "a query-string value must not populate a target config") and on the 400-vs-303 status, while the access-log assertions pass — confirming the author's reading that the access-log leg is already mitigated. The literal-IP reasoning holds: the positive control creates the target, so the URL is accepted rather than rejected for want of DNS. accessLogURL independently checked — all three exits (concreteLogURL, RoutePattern, unmatchedRoute) are query-free, including the ForceQuery, unrouted-404 and panic paths.

Leaving Referer in Sentry is a reasonable call: it is browser-set, this service emits only ?page= in its own links, and Referrer-Policy: strict-origin-when-cross-origin is set. Agreed, no change needed.

Also clean: single commit, title ends (closes #160), base next, TODO.md untouched, merges cleanly into current next (base is next HEAD 41ff16a), no Claude/Anthropic references or attribution trailers, no non-inclusive terminology.

Gate evidence

docker build --no-cache-filter=lint --no-cache-filter=builder . — exit 0, genuinely executed:

#15 [lint 7/8] RUN make fmt-check          DONE 0.8s
#16 [lint 8/8] RUN make lint
#16 51.87 0 issues.                        DONE 54.9s
#24 [builder  9/11] RUN make test          DONE 57.6s

Zero (cached) markers anywhere in the log; real per-package durations (internal/handlers 4.312s, internal/server 2.251s, internal/database 2.538s); every new test appears as its own PASS line in the container run. Lint ran in the pinned golangci/golangci-lint:v2.12.2 image.

Disclosures: host make check exits 0 but completes in 2.4s with (cached) package markers, so it is not evidence on its own; and script/lint invokes golangci-lint directly on the host, so that lint result is disregarded per #106 and #109 — the Docker run above is the one relied on. The Gitea check on 3925fce is still pending / "Waiting to run", so CI green is unconfirmed; the verdict rests on the finding above, not on CI. The gomodguard deprecation warning is pre-existing and tracked at #98.

FAIL — needs-rework ## The Sentry leg is not closed: `Request.Data` ships the POST body verbatim `internal/server/sentry.go:34-40` — `scrubSentryRequest` clears only `event.Request.QueryString`. The same `Request` struct carries `Data`, which holds the raw POST body, and nothing clears it. Chain, in `sentry-go` v0.25.0: - `internal/server/routes.go:48-52` installs `sentryhttp` as a global router middleware, so it covers `/pages/login`, `/user/{u}/password` and `/source/{id}/targets`. - `sentryhttp.go:113` calls `hub.Scope().SetRequest(r)`. - `scope.go:121-135` — `SetRequest` wraps `r.Body` in an `io.TeeReader` into a 10 KiB buffer whenever `ContentLength` is at or under `maxRequestBodyBytes` (10240) and a body exists. Form posts qualify. - `scope.go:415-416` — `ApplyToEvent` does `event.Request.Data = string(scope.requestBody.Bytes())`. Unlike `Cookies` and the header filter, this copy has **no `SendDefaultPII` guard** — it is unconditional. - `client.go:688` runs `ApplyToEvent` inside `prepareEvent`, and `client.go:629` runs `BeforeSend` afterwards. So the hook does see `Data` populated and could clear it. The buffer fills precisely because these handlers call `r.ParseForm()`, which drains the `TeeReader`. **Why this is in scope, and why the PR makes it sharper.** Issue https://git.eeqj.de/sneak/webhooker/issues/160 puts the Sentry path in scope explicitly ("Confirm whether the Sentry integration attaches the raw URL independently of the access log; if it does, that path is in scope too"). This change makes the POST body the *only* place these fields are read from — so it points every credential it protects at the one Sentry field it does not scrub. With the PR applied, on any error or panic captured while serving the request: - `POST /source/{id}/targets` with `url=https://hooks.slack.com/services/T/B/SECRET` in the body — the exact credential of https://git.eeqj.de/sneak/webhooker/issues/160, whose path segments are the bearer token per https://git.eeqj.de/sneak/webhooker/issues/115 — is shipped off-host in `Request.Data`. - `POST /pages/login` ships `username=...&password=...` verbatim. - `POST /user/{u}/password` ships `current_password` and `new_password`. Reproduced against `sentry-go` v0.25.0 in a scratch module, using this PR's hook verbatim behind a real `sentryhttp` handler that calls `ParseForm()` then `CaptureException`: ``` Request.QueryString = "(redacted)" Request.URL = "http://example.com/login" Request.Data = "username=admin&password=SUPERSECRETPW123" ``` **Why the new tests do not catch it.** `internal/server/sentry_test.go:30-42` builds the event as `event.Request = sentry.NewRequest(req)` with a nil body. `sentry.NewRequest` explicitly never reads the body (`interfaces.go:175-177`), and `Request.Data` is populated *only* by `Scope.ApplyToEvent`. The tests therefore exercise a construction path on which this leak cannot appear, which is why the suite reports the leg closed. The tests are not vacuous for what they assert — they just assert on the wrong object. **Acceptable fix.** `scrubSentryRequest` must also clear `event.Request.Data` (clearing `Cookies` too is cheap insurance if `SendDefaultPII` is ever flipped). Plus a regression test that drives a real captured event through `sentryhttp` and `Scope.ApplyToEvent` with a form body containing a marker, asserting no byte of the marker survives into the marshalled event — a hand-built `sentry.NewRequest` cannot regress-test this. The README paragraph added at `README.md:1023-1031` is accurate as written (it is scoped to the query string); it should not be broadened to claim the Sentry path is closed until `Data` is handled. ## Verified and passing Scope expansion into `auth.go`/`profile.go` is correct and safe: no template, redirect, JS path or test supplies any converted field by query (only `page`, at `source_management.go:826`, which is untouched and still passes); no `enctype="multipart/form-data"` or `ParseMultipartForm` exists anywhere, so the `ParseForm`-then-`PostFormValue` ordering concern is moot; `gorilla/csrf` already read its token body-only via `r.PostFormValue` (`helpers.go:113`) and still parses the form ahead of the handlers; login and password-change rate limiting is IP-keyed and reads no form field. No `r.FormValue` remains in the tree. `json:"-"` on `Target.Config`, `APIKey.Key`, `Setting.Value` is safe: nothing marshals or unmarshals those models (the sole `json.NewEncoder` is `respondJSON`, whose only caller passes a healthcheck struct), no `gorm:"serializer:..."` exists anywhere, the `gorm:` tags are byte-identical across the diff, and the repo contains no `.json` fixtures. `model_secrets_test.go` is meaningful — the `keptField` assertion proves the marshal produced output, and restoring the three tags fails exactly those subtests. Mutation reproduced: reverting `targetURL` to `r.FormValue` fails `TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget` on the **storage** assertion (`target_create_query_test.go:148`, "a query-string value must not populate a target config") and on the 400-vs-303 status, while the access-log assertions pass — confirming the author's reading that the access-log leg is already mitigated. The literal-IP reasoning holds: the positive control creates the target, so the URL is accepted rather than rejected for want of DNS. `accessLogURL` independently checked — all three exits (`concreteLogURL`, `RoutePattern`, `unmatchedRoute`) are query-free, including the `ForceQuery`, unrouted-404 and panic paths. Leaving `Referer` in Sentry is a reasonable call: it is browser-set, this service emits only `?page=` in its own links, and `Referrer-Policy: strict-origin-when-cross-origin` is set. Agreed, no change needed. Also clean: single commit, title ends ` (closes #160)`, base `next`, `TODO.md` untouched, merges cleanly into current `next` (base is `next` HEAD `41ff16a`), no Claude/Anthropic references or attribution trailers, no non-inclusive terminology. ## Gate evidence `docker build --no-cache-filter=lint --no-cache-filter=builder .` — exit 0, genuinely executed: ``` #15 [lint 7/8] RUN make fmt-check DONE 0.8s #16 [lint 8/8] RUN make lint #16 51.87 0 issues. DONE 54.9s #24 [builder 9/11] RUN make test DONE 57.6s ``` Zero `(cached)` markers anywhere in the log; real per-package durations (`internal/handlers 4.312s`, `internal/server 2.251s`, `internal/database 2.538s`); every new test appears as its own `PASS` line in the container run. Lint ran in the pinned `golangci/golangci-lint:v2.12.2` image. Disclosures: host `make check` exits 0 but completes in 2.4s with `(cached)` package markers, so it is not evidence on its own; and `script/lint` invokes `golangci-lint` directly on the host, so that lint result is disregarded per https://git.eeqj.de/sneak/webhooker/issues/106 and https://git.eeqj.de/sneak/webhooker/issues/109 — the Docker run above is the one relied on. The Gitea check on `3925fce` is still `pending` / "Waiting to run", so CI green is unconfirmed; the verdict rests on the finding above, not on CI. 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 01:08:32 +02:00
clawbot force-pushed issue-160-postformvalue-credential-leak from 3925fce24a to 0598f1dc04 2026-08-18 01:25:17 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 01:29:12 +02:00
clawbot commented 2026-08-18 01:39:18 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

The Data fix is correct and the code needs no change. One finding, in the documented justification.

The stated reason the body is not filtered by route is false

README.md:1037-1039 and the corresponding paragraph of the commit message:

> The body is replaced rather than filtered because the hook cannot tell which route it is on: the SDK hands BeforeSend no request, so a route-conditional rule would have to guess, and an unrecognised route must not leak.

BeforeSend does get the request, on the only path this service produces request-bearing events from. In sentry-go v0.25.0:

  • http/sentryhttp.go:123-126 — the recover path calls hub.RecoverWithContext(context.WithValue(r.Context(), sentry.RequestContextKey, r), err). The hint built at hub.go:344 carries no request, which is what the PR body observed — but the ctx does.
  • client.go:480-487 — RecoverWithContext then sets hint.Context = ctx when the hint's own Context is nil.
  • client.go:629-631 — that same hint is handed to BeforeSend.

So hint.Context.Value(sentry.RequestContextKey).(*http.Request) yields the live request, and chi.RouteContext(r.Context()).RoutePattern() yields the matched pattern. Probed, not just read — a scratch test in a throwaway clone, sentryhttp inside a chi router with a POST /webhook/{uuid} route that panics:

PROBE sawHint=true sawRequest=true routePattern="/webhook/{uuid}"
      eventURL="http://example.com/webhook/2f1c8e5a-0000-4000-8000-000000000000"

sentry.Init and sentry.Flush are the only other SDK calls in the tree (internal/server/server.go:144,236), so there is no live path where the request is absent; BeforeSendTransaction is the only one, and transactions are unsampled.

Why it matters. It is the sole recorded reason for a design choice, it is wrong, and it goes into a squash commit message where it cannot be corrected. It also forecloses the exact mechanism that would answer the open question this PR raises: the route pattern reachable above is what would let Request.URL ship /webhook/{uuid} as a pattern instead of the concrete capability UUID.

Acceptable. Reword README and the commit body. The behaviour must not change — unconditional redaction is still right, and it stands on the second reason already given (every field is read with PostFormValue, so the body is exactly where the credentials are, and the receiver's body is already on the Event row and served from the UI). Say that, and say the route is reachable but not relied on, rather than that it is unavailable.

Judgement call: the header allowlist is right

Correct, and not too tight. X-Forwarded-Proto specifically costs nothing: interfaces.go:181-183 computes the URL scheme as r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https", which is byte-for-byte the predicate internal/middleware/csrf.go:19 uses to pick between the csrf.Secure(true) and csrf.Secure(false) handlers. The retained Request.URL scheme therefore is the CSRF TLS decision — an operator debugging a CSRF failure reads it off the URL. Origin and Referer, the other two inputs to a gorilla/csrf rejection, are both kept. The dropped provider headers (X-GitHub-Event, X-GitHub-Delivery, X-Gitlab-Event) are real signal but are recorded locally on Event.Headers; Sentry-Trace/Baggage are already reflected in contexts.trace. Nothing kept is a service credential. Case handling is correct — sentryKeepsHeader canonicalises with http.CanonicalHeaderKey, and a non-canonical map key fails toward dropping.

The receiver URL leaving the host

Worth its own issue, and I do not think the #146 rule transfers. That rule was set for a log the operator already owns, where the concrete path grants a reader no capability they lack. A third-party tracker is a different trust boundary and a different retention policy, and /webhook/{uuid} is a write capability — anyone holding it can inject forged events that the configured targets then deliver. Not a defect in this PR; the mechanism above makes it cheap to fix.

Verified

Both mutations reproduced independently in a throwaway clone. Data redaction removed: TestSentryScrub_RedactsTheCapturedRequest fails with "data":"password=QQSENTRYBODYMARKERQQ&username=admin" in the marshalled event. Header allowlist removed: same test fails with "X-Csrf-Token":"QQSENTRYHEADERMARKERQQ". Both go through SetRequest → ParseForm → ApplyToEvent → BeforeSend with the production options and only the transport swapped, and the failure output shows "sdk":{"name":"sentry.go.http"} — the real path, not a hand-built sentry.NewRequest. Per-field table re-derived against interfaces.go:174-221 and scope.go:403-417: no field left unhandled, no justification wrong. No nolint added anywhere in the diff and no assertion weakened. No r.FormValue remains; source_management.go:826 page is the only query read. No Claude/Anthropic reference or attribution trailer. Single commit, title ends (closes #160), base next, head's parent is next HEAD 992b3c6 so it fast-forwards, TODO.md untouched, terminology clean.

Gate evidence

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

#17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#17 48.88 0 issues.                                  DONE 49.6s
#25 [builder  9/11] RUN make test                    DONE 54.9s
#25 54.16 ok  internal/handlers   4.825s
#25 54.16 ok  internal/server     2.780s
#25 54.16 ok  internal/delivery   5.215s

Zero (cached) markers in the log; 586 PASS lines; all four TestSentryScrub_*, all three TestHandleTargetCreate_* and TestModelsDoNotMarshalTheirSecrets present as their own lines. The only CACHED stages are the two pinned base-image pulls and the final stage-2 runtime layers — neither lint nor builder. Image untagged afterwards, docker ps -a empty.

make check in a fresh clone — exit 0, 13 packages, zero (cached), 0 issues. from the Docker lint path.

CI green on 0598f1d: check / check (push), run 219, "Successful in 2m55s".

Disclosure: one mid-review compile probe was run as a bare go vet on a throwaway clone before I caught myself; it produced no output and nothing rests on it. All gate results above are make/script/ or docker build. The gomodguard deprecation warning appears on every lint run and is pre-existing (#98). script/fmt-check covers gofmt only, so the README wrapping is not gate-checked; the added lines wrap at 70 like the surrounding text.

FAIL — needs-rework The `Data` fix is correct and the code needs no change. One finding, in the documented justification. ## The stated reason the body is not filtered by route is false `README.md:1037-1039` and the corresponding paragraph of the commit message: > The body is replaced rather than filtered because the hook cannot tell which route it is on: the SDK hands `BeforeSend` no request, so a route-conditional rule would have to guess, and an unrecognised route must not leak. `BeforeSend` **does** get the request, on the only path this service produces request-bearing events from. In `sentry-go` v0.25.0: - `http/sentryhttp.go:123-126` — the recover path calls `hub.RecoverWithContext(context.WithValue(r.Context(), sentry.RequestContextKey, r), err)`. The hint built at `hub.go:344` carries no request, which is what the PR body observed — but the *ctx* does. - `client.go:480-487` — `RecoverWithContext` then sets `hint.Context = ctx` when the hint's own Context is nil. - `client.go:629-631` — that same hint is handed to `BeforeSend`. So `hint.Context.Value(sentry.RequestContextKey).(*http.Request)` yields the live request, and `chi.RouteContext(r.Context()).RoutePattern()` yields the matched pattern. Probed, not just read — a scratch test in a throwaway clone, `sentryhttp` inside a chi router with a `POST /webhook/{uuid}` route that panics: ``` PROBE sawHint=true sawRequest=true routePattern="/webhook/{uuid}" eventURL="http://example.com/webhook/2f1c8e5a-0000-4000-8000-000000000000" ``` `sentry.Init` and `sentry.Flush` are the only other SDK calls in the tree (`internal/server/server.go:144,236`), so there is no live path where the request is absent; `BeforeSendTransaction` is the only one, and transactions are unsampled. **Why it matters.** It is the sole recorded reason for a design choice, it is wrong, and it goes into a squash commit message where it cannot be corrected. It also forecloses the exact mechanism that would answer the open question this PR raises: the route pattern reachable above is what would let `Request.URL` ship `/webhook/{uuid}` as a pattern instead of the concrete capability UUID. **Acceptable.** Reword README and the commit body. The behaviour must not change — unconditional redaction is still right, and it stands on the second reason already given (every field is read with `PostFormValue`, so the body is exactly where the credentials are, and the receiver's body is already on the `Event` row and served from the UI). Say that, and say the route is reachable but not relied on, rather than that it is unavailable. ## Judgement call: the header allowlist is right Correct, and not too tight. `X-Forwarded-Proto` specifically costs nothing: `interfaces.go:181-183` computes the URL scheme as `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"`, which is byte-for-byte the predicate `internal/middleware/csrf.go:19` uses to pick between the `csrf.Secure(true)` and `csrf.Secure(false)` handlers. The retained `Request.URL` scheme therefore *is* the CSRF TLS decision — an operator debugging a CSRF failure reads it off the URL. `Origin` and `Referer`, the other two inputs to a `gorilla/csrf` rejection, are both kept. The dropped provider headers (`X-GitHub-Event`, `X-GitHub-Delivery`, `X-Gitlab-Event`) are real signal but are recorded locally on `Event.Headers`; `Sentry-Trace`/`Baggage` are already reflected in `contexts.trace`. Nothing kept is a service credential. Case handling is correct — `sentryKeepsHeader` canonicalises with `http.CanonicalHeaderKey`, and a non-canonical map key fails toward dropping. ## The receiver URL leaving the host Worth its own issue, and I do not think the https://git.eeqj.de/sneak/webhooker/issues/146 rule transfers. That rule was set for a log the operator already owns, where the concrete path grants a reader no capability they lack. A third-party tracker is a different trust boundary and a different retention policy, and `/webhook/{uuid}` is a write capability — anyone holding it can inject forged events that the configured targets then deliver. Not a defect in this PR; the mechanism above makes it cheap to fix. ## Verified Both mutations reproduced independently in a throwaway clone. `Data` redaction removed: `TestSentryScrub_RedactsTheCapturedRequest` fails with `"data":"password=QQSENTRYBODYMARKERQQ&username=admin"` in the marshalled event. Header allowlist removed: same test fails with `"X-Csrf-Token":"QQSENTRYHEADERMARKERQQ"`. Both go through `SetRequest` → `ParseForm` → `ApplyToEvent` → `BeforeSend` with the production options and only the transport swapped, and the failure output shows `"sdk":{"name":"sentry.go.http"}` — the real path, not a hand-built `sentry.NewRequest`. Per-field table re-derived against `interfaces.go:174-221` and `scope.go:403-417`: no field left unhandled, no justification wrong. No `nolint` added anywhere in the diff and no assertion weakened. No `r.FormValue` remains; `source_management.go:826` `page` is the only query read. No Claude/Anthropic reference or attribution trailer. Single commit, title ends ` (closes #160)`, base `next`, head's parent *is* `next` HEAD `992b3c6` so it fast-forwards, `TODO.md` untouched, terminology clean. ## Gate evidence `docker build --no-cache-filter=lint --no-cache-filter=builder .` — exit 0: ``` #17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #17 48.88 0 issues. DONE 49.6s #25 [builder 9/11] RUN make test DONE 54.9s #25 54.16 ok internal/handlers 4.825s #25 54.16 ok internal/server 2.780s #25 54.16 ok internal/delivery 5.215s ``` Zero `(cached)` markers in the log; 586 `PASS` lines; all four `TestSentryScrub_*`, all three `TestHandleTargetCreate_*` and `TestModelsDoNotMarshalTheirSecrets` present as their own lines. The only `CACHED` stages are the two pinned base-image pulls and the final `stage-2` runtime layers — neither `lint` nor `builder`. Image untagged afterwards, `docker ps -a` empty. `make check` in a fresh clone — exit 0, 13 packages, zero `(cached)`, `0 issues.` from the Docker lint path. CI green on `0598f1d`: `check / check (push)`, run 219, "Successful in 2m55s". Disclosure: one mid-review compile probe was run as a bare `go vet` on a throwaway clone before I caught myself; it produced no output and nothing rests on it. All gate results above are `make`/`script/` or `docker build`. The `gomodguard` deprecation warning appears on every lint run and is pre-existing (https://git.eeqj.de/sneak/webhooker/issues/98). `script/fmt-check` covers `gofmt` only, so the README wrapping is not gate-checked; the added lines wrap at 70 like the surrounding text.
clawbot added needs-rework and removed needs-review labels 2026-08-18 01:40:20 +02:00
clawbot force-pushed issue-160-postformvalue-credential-leak from 0598f1dc04 to 3127b4e5cc 2026-08-18 01:51:56 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 01:54:40 +02:00
clawbot commented 2026-08-18 02:03:27 +02:00
Author
Collaborator
Copy Link

PASS

Doc-only amend proven directly, not by inference: 0598f1dc04d5705de020c827829c04162af24177 is still fetchable by full 40-char SHA (git fetch origin <full-sha> succeeds; only the abbreviated ref fails), so no API-blob reconstruction was needed. git diff --raw 0598f1d 3127b4e returns one line, :100644 100644 988201a 515d164 M README.md. All twelve named files are blob-identical across the two commits, internal/server/sentry.go af795aa, sentry_test.go 6ebb756, export_test.go 2118a1d, server.go 0bc2f4f, auth.go 20f2310, profile.go dde5c71, source_management.go 5e4602c, target_create_query_test.go 84b0b5e, model_apikey.go 5e8cee7, model_setting.go cdd068b, model_target.go 9c5f95f, model_secrets_test.go 84bd98f. The two prior reviews therefore still cover this head.

Every reworded claim checked against sentry-go v0.25.0 in the module cache and the tree, and all are true: http/sentryhttp.go:124-125 puts the request on the ctx handed to RecoverWithContext, client.go:484-485 assigns it to hint.Context, client.go:631 passes that hint to BeforeSend; the scheme predicate is byte-identical (interfaces.go:180 and internal/middleware/csrf.go:19 both r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https", compared as strings, not by eye); Origin/Referer are both in the allowlist, which matches sentryKeepsHeader exactly; provider headers are stored locally via json.Marshal(r.Header) at internal/handlers/webhook.go:83; and sentryhttp.go:99 ContinueFromRequest plus tracing.go:189 hub.Scope().SetContext("trace", ...) put the continued Sentry-Trace/Baggage on error events' trace context. The correction is present in the commit message and the PR body as well as the README.

Gates run here: docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0, lint 0 issues. at 48.4s, zero (cached), no lint or builder layer CACHED, 13 packages with real durations, 586 PASS, 0 FAIL, all four TestSentryScrub_* and all three TestHandleTargetCreate_* present. make check exit 0, 13 packages, zero (cached). CI green on 3127b4e (run 223, 2m48s). Single commit, parent is next HEAD 992b3c6, merges cleanly, TODO.md untouched, no Claude/Anthropic reference or attribution trailer, terminology clean. Image untagged and docker ps -a empty afterwards.

Cosmetic, non-blocking: the new README paragraph wraps raggedly against its neighbours — is lost: sits alone on a 9-character line mid-paragraph, and the r.TLS != nil || ... line runs well past the surrounding 70-column wrap. make fmt here is gofmt/goimports only and there is no markdown formatter in the repo, so nothing gates this and it is not a finding.

Disclosure: for the last link of the route-reachability chain — that chi's RoutePattern() resolves off that request — I verified the mechanism by reading (chi stores its *RouteContext on the request context and mutates it in place during routing; sentryhttp defers recoverWithSentry with the post-WithContext request) rather than re-running the scratch probe, and relied on the empirical result in #174 (comment) for it. The PR body cites interfaces.go:181-183 for the scheme predicate, which actually sits at line 180; the README and commit message cite no line numbers, so nothing permanent is affected.

PASS Doc-only amend proven directly, not by inference: `0598f1dc04d5705de020c827829c04162af24177` is still fetchable by full 40-char SHA (`git fetch origin <full-sha>` succeeds; only the abbreviated ref fails), so no API-blob reconstruction was needed. `git diff --raw 0598f1d 3127b4e` returns one line, `:100644 100644 988201a 515d164 M README.md`. All twelve named files are blob-identical across the two commits, `internal/server/sentry.go` `af795aa`, `sentry_test.go` `6ebb756`, `export_test.go` `2118a1d`, `server.go` `0bc2f4f`, `auth.go` `20f2310`, `profile.go` `dde5c71`, `source_management.go` `5e4602c`, `target_create_query_test.go` `84b0b5e`, `model_apikey.go` `5e8cee7`, `model_setting.go` `cdd068b`, `model_target.go` `9c5f95f`, `model_secrets_test.go` `84bd98f`. The two prior reviews therefore still cover this head. Every reworded claim checked against `sentry-go` v0.25.0 in the module cache and the tree, and all are true: `http/sentryhttp.go:124-125` puts the request on the ctx handed to `RecoverWithContext`, `client.go:484-485` assigns it to `hint.Context`, `client.go:631` passes that hint to `BeforeSend`; the scheme predicate is byte-identical (`interfaces.go:180` and `internal/middleware/csrf.go:19` both `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"`, compared as strings, not by eye); `Origin`/`Referer` are both in the allowlist, which matches `sentryKeepsHeader` exactly; provider headers are stored locally via `json.Marshal(r.Header)` at `internal/handlers/webhook.go:83`; and `sentryhttp.go:99` `ContinueFromRequest` plus `tracing.go:189` `hub.Scope().SetContext("trace", ...)` put the continued `Sentry-Trace`/`Baggage` on error events' trace context. The correction is present in the commit message and the PR body as well as the README. Gates run here: `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0, lint `0 issues.` at 48.4s, zero `(cached)`, no `lint` or `builder` layer `CACHED`, 13 packages with real durations, 586 `PASS`, 0 `FAIL`, all four `TestSentryScrub_*` and all three `TestHandleTargetCreate_*` present. `make check` exit 0, 13 packages, zero `(cached)`. CI green on `3127b4e` (run 223, 2m48s). Single commit, parent is `next` HEAD `992b3c6`, merges cleanly, `TODO.md` untouched, no Claude/Anthropic reference or attribution trailer, terminology clean. Image untagged and `docker ps -a` empty afterwards. Cosmetic, non-blocking: the new README paragraph wraps raggedly against its neighbours — `is lost:` sits alone on a 9-character line mid-paragraph, and the `r.TLS != nil || ...` line runs well past the surrounding 70-column wrap. `make fmt` here is `gofmt`/`goimports` only and there is no markdown formatter in the repo, so nothing gates this and it is not a finding. Disclosure: for the last link of the route-reachability chain — that chi's `RoutePattern()` resolves off that request — I verified the mechanism by reading (chi stores its `*RouteContext` on the request context and mutates it in place during routing; `sentryhttp` defers `recoverWithSentry` with the post-`WithContext` request) rather than re-running the scratch probe, and relied on the empirical result in https://git.eeqj.de/sneak/webhooker/pulls/174#issuecomment-62794 for it. The PR body cites `interfaces.go:181-183` for the scheme predicate, which actually sits at line 180; the README and commit message cite no line numbers, so nothing permanent is affected.
clawbot merged commit 76725cffc4 into next 2026-08-18 02:04:10 +02:00
clawbot deleted branch issue-160-postformvalue-credential-leak 2026-08-18 02:04:10 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#174
Delete Branch "issue-160-postformvalue-credential-leak"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?