[minor-5] Webhook transport errors persist raw userinfo URLs into the bucket #90

New issue

Closed

opened 2026-09-05 02:40:26 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 02:40:26 +00:00

Owner

[minor-5] Webhook transport errors persist raw userinfo URLs into the bucket

internal/notify/webhooks.go:467-469: failed deliveries store raw derr.Error() into webhooks/<id>/deliveries/recent.json (bucket + admin API). Go transport errors embed the URL — Post "https://user:pass@host/hook": dial tcp ... — and hook URLs commonly carry userinfo for basic-auth sinks. Credentials at rest in the bucket + readable via ReadDeliveries.

Fix

Scrub credentials (userinfo, and any query/token material) from the error string before storing — scrubURL-equivalent over derr.Error(). Regression test (failing delivery against a userinfo URL → stored entry contains no password). Coverage gate holds; doc note if the deliveries shape changes (law 12).

Acceptance criteria

No credential material persisted for failed deliveries; test green -race.

# [minor-5] Webhook transport errors persist raw userinfo URLs into the bucket `internal/notify/webhooks.go:467-469`: failed deliveries store raw `derr.Error()` into `webhooks/<id>/deliveries/recent.json` (bucket + admin API). Go transport errors embed the URL — `Post "https://user:pass@host/hook": dial tcp ...` — and hook URLs commonly carry userinfo for basic-auth sinks. Credentials at rest in the bucket + readable via `ReadDeliveries`. ## Fix Scrub credentials (userinfo, and any query/token material) from the error string before storing — `scrubURL`-equivalent over `derr.Error()`. Regression test (failing delivery against a userinfo URL → stored entry contains no password). Coverage gate holds; doc note if the deliveries shape changes (law 12). ## Acceptance criteria - [ ] No credential material persisted for failed deliveries; test green `-race`.

crueber referenced this issue from a pull request that will close it,

2026-09-05 02:43:19 +00:00

Fix #90: scrub credentials from delivery errors #99

crueber referenced this issue from a commit

2026-09-05 02:43:19 +00:00

Fix #90: scrub credentials from webhook delivery errors

crueber commented

2026-09-05 02:43:22 +00:00

Author

Owner

Fixed by PR #99 (branch fix/issue-90): delivery errors are scrubbed of userinfo/query-token/KV/bearer material before hitting the deliveries ring. TestScrubDeliveryError + TestWebhookDeliveryErrorScrubbed green (-race), package coverage 97.2%. Note: rings written before the fix still hold old raw errors until they trim; no migration.

crueber commented

2026-09-05 02:45:36 +00:00

Author

Owner

Review PR #99 (fix/issue-90, 0fdfe8a) — verified in scratch worktree (removed afterward), main worktree untouched.

Scrub completeness (internal/notify/webhooks.go): userinfo stripped via u.User=nil (:528-531); query keys redacted case-insensitively through sensitiveDeliveryParams (:489-494, :536); fragment cleared (:547-550); Bearer material redacted (:485, :512); bare KV secrets redacted (:559-575). Authorization: Basic echoes checked — not a vector: postEvent (:425-447) only sets Content-Type/X-Walgit-* headers and Go transport errors never echo headers; hook secret itself travels only as HMAC, never in URL/error text. No credential shape missed for the realistic (URL-echo) vector. Two theoretical-only notes, deliberately left unfixed: redactDeliveryKV is case-sensitive (Token=/SECRET= bypass) and has no bare key=/auth_token= entries — but Go errors never carry bare KV secrets (that layer is already defense-in-depth beyond the issue), and adding bare key= would false-positive on substrings like monkey=. Not blockers.

No over-redaction: host/path/status/non-secret keys preserved; scrubDeliveryURL returns match untouched when nothing credential-shaped (:551-553). Clean errors byte-identical — pinned by exact-match cases hostless-match-untouched, clean-error-untouched, clean-url-untouched.

Single persistence point: confirmed by grep — sole bucket write is recordDelivery :584 (sole caller :399); other .Error() uses in internal/notify are transient HTTP responses (http.go, collab.go:78) or test assertions, never persisted. Negative control genuine (three exact byte-for-byte cases). Concurrency: pure function, no shared state, pre-compiled regexes safe under DeliverRepo fan-out. Law 12: deliveries shape unchanged (Error still string), no doc update needed. Stdlib only (added regexp). gofmt clean, go vet clean.

Tests (scratch worktree @0fdfe8a): go test -race ./internal/notify/... ok (1.7s); targeted TestScrubDeliveryError (9 subcases) + TestWebhookDeliveryErrorScrubbed PASS; package coverage 97.2% (gate 95% holds). No code changes made — nothing to fix.

MERGE RECOMMENDATION: ready to merge.

Review PR #99 (fix/issue-90, 0fdfe8a) — verified in scratch worktree (removed afterward), main worktree untouched. Scrub completeness (internal/notify/webhooks.go): userinfo stripped via u.User=nil (:528-531); query keys redacted case-insensitively through sensitiveDeliveryParams (:489-494, :536); fragment cleared (:547-550); Bearer material redacted (:485, :512); bare KV secrets redacted (:559-575). Authorization: Basic echoes checked — not a vector: postEvent (:425-447) only sets Content-Type/X-Walgit-* headers and Go transport errors never echo headers; hook secret itself travels only as HMAC, never in URL/error text. No credential shape missed for the realistic (URL-echo) vector. Two theoretical-only notes, deliberately left unfixed: redactDeliveryKV is case-sensitive (Token=/SECRET= bypass) and has no bare key=/auth_token= entries — but Go errors never carry bare KV secrets (that layer is already defense-in-depth beyond the issue), and adding bare key= would false-positive on substrings like monkey=. Not blockers. No over-redaction: host/path/status/non-secret keys preserved; scrubDeliveryURL returns match untouched when nothing credential-shaped (:551-553). Clean errors byte-identical — pinned by exact-match cases hostless-match-untouched, clean-error-untouched, clean-url-untouched. Single persistence point: confirmed by grep — sole bucket write is recordDelivery :584 (sole caller :399); other .Error() uses in internal/notify are transient HTTP responses (http.go, collab.go:78) or test assertions, never persisted. Negative control genuine (three exact byte-for-byte cases). Concurrency: pure function, no shared state, pre-compiled regexes safe under DeliverRepo fan-out. Law 12: deliveries shape unchanged (Error still string), no doc update needed. Stdlib only (added regexp). gofmt clean, go vet clean. Tests (scratch worktree @0fdfe8a): go test -race ./internal/notify/... ok (1.7s); targeted TestScrubDeliveryError (9 subcases) + TestWebhookDeliveryErrorScrubbed PASS; package coverage 97.2% (gate 95% holds). No code changes made — nothing to fix. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 02:45:48 +00:00

crueber commented

2026-09-05 02:45:49 +00:00

Author

Owner

Fixed by PR #99 (review clean; 9-case scrub table + e2e; 97.2% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 02:45:50 +00:00

Merge pull request 'Fix #90: scrub credentials from delivery errors' (#99) from fix/issue-90 into main

crueber added the

webhooks