Add optional inbound webhook signature verification (closes #67) #228

Merged
clawbot merged 1 commits from issue-67-inbound-signature-verification into next 2026-08-20 08:01:33 +02:00
Collaborator

Closes #67, working from
#67 (comment) (the
body's "not a 1.0 blocker" is superseded).

What it does

A receiver URL was a bare v4 UUID and nothing else. Anyone who learned
one could store events, and — because inbound headers are forwarded to
targets almost verbatim — choose what the downstream service received.

Entrypoints now carry an optional signature_scheme / signature_secret
pair:

Scheme Header Check
github X-Hub-Signature-256 HMAC-SHA256 over the raw body, hex, sha256= prefix required
gitlab X-Gitlab-Token The header is the shared secret itself

With nothing configured an entrypoint behaves exactly as before. That
is also where every pre-existing row lands: both columns are added by
AutoMigrate with an empty default, so an upgrade never locks an
operator out of a receiver whose senders cannot be told to start
signing. TestEntrypointSignatureColumnsMigrateToUnconfigured proves
it by dropping the columns, writing a row through the old shape,
re-running Migrate(), and asserting the row loads as "not configured"
and still verifies.

GitHub's older SHA-1 X-Hub-Signature is not accepted, and a GitHub
digest without its sha256= prefix is not accepted.

The traps named in the issue

  • Constant-time compare. Both schemes go through hmac.Equal;
    there is no == on a secret anywhere. internal/signature/signature.go.
  • Raw bytes, and still capped. Verification runs on the bytes
    readWebhookBody returned, before json.Marshal of the headers and
    before any parse. It sits below the body read because the digest
    covers the body, and that read is what enforces the existing 1 MB
    cap, so an unsigned sender still cannot make the process hold more
    than a signed one.
  • Rejection before persistence. The check sits above every write,
    not inside the transaction. The receiver table test asserts the
    stored event count for each case — 1 after a 200, 0 after a 401
    or a 500 — which is the half that matters: a rejection that still
    wrote a row would leave the receiver a place for a stranger to
    deposit content.
  • Credential handling. SignatureSecret is json:"-" (covered by
    TestModelsDoNotMarshalTheirSecrets and a new nested-association
    case), kept out of templates by a new handlers.EntrypointView
    projection modelled on delivery.TargetView, absent from every log
    line including the rejection path (TestReceiverLogsNoSecret
    asserts neither the stored secret nor the value the client
    presented appears), and — see the next section — not persisted onto
    events or forwarded to delivery targets. Consistent with
    #113,
    #115 and
    #118.
  • Rotation. The form takes the secret afresh every time and never
    renders the stored value, so setting and rotating are one submission
    and there is no store-and-display path at all. I did not add a
    show-once generated secret: both supported senders require the
    operator to enter the same string on the sender's side, so a
    webhooker-generated value would have to be displayed and copied —
    one more credential-display path for no gain. Flagging the deviation
    explicitly since the issue offered show-once as an option ("if that
    fits the existing UI patterns").

The credential does not leave the host

Under the gitlab scheme the signature header is the secret, not
a digest over the request. An accepted request's headers are persisted
verbatim on the event and replayed onto every outbound delivery, so
both are disclosures of the credential unless it is removed first.
signature.SanitizeHeaders clones the header map — the caller's
r.Header is never mutated — and drops the configured scheme's
credential header before json.Marshal, above the first write. One
strip serves both egresses, because Task.Headers is a copy of
Event.Headers; filtering at each egress instead would let a new
consumer of Event.Headers reopen the leak by forgetting to filter.

Stripping is driven by each scheme's own SchemeInfo and defaults
to stripping
: HeaderIsDigest is false at the zero value, so a
scheme added later is stripped unless whoever adds it positively
declares the header safe to keep. github declares it —
X-Hub-Signature-256 is an HMAC over the body, from which the key
cannot be recovered — so it is stored and forwarded intact.

Two tests pin it, and both were confirmed to fail with the strip
removed:

  • TestReceiverDoesNotStoreInboundCredential drives the real receiver
    and reads Event.Headers back out of the per-webhook database.
    Without the strip it reports the stored value as
    {"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]}.
  • TestApplyRequestHeadersDropsInboundCredential builds the outbound
    request through applyRequestHeaders and asserts no header carries
    the secret. isForwardableHeader is a blocklist of hop-by-hop names
    and forwards X-Gitlab-Token like anything else, so what keeps the
    secret out of a delivery is that it was never stored.

Both also assert the sender's other headers survive, so a fix that
dropped everything would not pass.

Failing closed

An entrypoint whose stored scheme this build cannot apply, or which has
one half of the scheme/secret pair and not the other, is answered 500
and stores nothing. It is never treated as unverified — the whole point
is that turning verification on cannot silently turn itself back off.
500 rather than 401 because the request may well be authentic and
calling it unauthorized would send a legitimate sender off to debug its
own signing. The form rejects an unknown scheme with a 400, so the UI
cannot create such a row; this covers a hand-edited database or a
downgrade past a scheme.

The UI names that state rather than mislabelling it: a half-configured
row renders as misconfigured, not as not verified, and the scheme
selector follows the stored scheme so such a row no longer marks two
options selected in one <select>.

The secret is stored in the clear. HMAC verification needs the key
itself and a hash of it cannot recompute a sender's digest, so there is
no alternative; it is documented in the README next to the other
credentials webhooker.db holds.

Changes

  • internal/signature/ — new package: schemes, Verify,
    SanitizeHeaders, UI metadata.
  • internal/database/model_entrypoint.goSignatureScheme enum, the
    two columns, SignatureConfigured(), SignatureHalfConfigured().
  • internal/handlers/webhook.goverifyInboundSignature, wired
    between the body read and the first write; header sanitizing before
    serialization.
  • internal/handlers/entrypoint_view.go — new display-safe projection.
  • internal/handlers/source_management.goHandleEntrypointSecret;
    renderSourceDetail now passes projected entrypoints.
  • internal/server/routes.goPOST /source/{sourceID}/entrypoints/{entrypointID}/secret.
  • templates/source_detail.html — status line plus the set/rotate form.
  • README.md — new "Inbound Signature Verification" section with
    per-sender setup for GitHub and GitLab, what is and is not stored or
    forwarded, plus the data model, request flow, endpoint table,
    package layout, security and backup-secrets sections.

TODO.md deliberately untouched, per
#112.

Gate evidence

Branch rebased onto next at aba02bc immediately before pushing.

The branch is red on one package, and so is next itself.
internal/gormlog's TestGormScanIsNeverCalledOutsideTests fails on
origin/next clean, with nothing applied, naming
internal/delivery/queue_depth.go:109 and :161. Filed as
#234. Nothing here touches
internal/delivery/queue_depth.go or internal/gormlog. Every other
package passes.

make checkinternal/gormlog as above; all others green:

ok  	sneak.berlin/go/webhooker/internal/database	2.249s
ok  	sneak.berlin/go/webhooker/internal/delivery	4.614s
FAIL	sneak.berlin/go/webhooker/internal/gormlog	0.685s
ok  	sneak.berlin/go/webhooker/internal/handlers	19.497s
ok  	sneak.berlin/go/webhooker/internal/signature	1.041s

Cache-defeated container build,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .
— zero CACHED layers on either stage and zero (cached) package
lines, so every figure below was executed:

#17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#17 52.89 0 issues.
#17 DONE 53.0s

#25 [builder  9/11] RUN make test
#25 58.05 --- PASS: TestApplyRequestHeadersDropsInboundCredential (0.00s)
#25 58.05 --- PASS: TestApplyRequestHeadersKeepsGitHubDigest (0.00s)
#25 58.05 --- FAIL: TestGormScanIsNeverCalledOutsideTests (0.15s)
#25 88.43 --- PASS: TestReceiverDoesNotStoreInboundCredential (6.54s)
#25 88.43 ok  	sneak.berlin/go/webhooker/internal/handlers	36.262s

Host load average 35-69 across the gate runs. Every image built for
this was removed; docker ps -a shows nothing of mine and no prune was
run.

One thing worth knowing

internal/handlers is close to its 90s per-package timeout on this
host. My first version of these tests stood up one fx application per
table row; the cache-defeated build then hit
panic: test timed out after 1m30s. Measured against unmodified next
under the same load: baseline 67.7s, mine 69.9s — so the ~2s I add is
not the cause, the package is simply at ~92% of its budget already
(next alone measured 82.8s in a container run). I consolidated to
seven applications across the two new test files, and the package now
measures 36.3s in the container. I have not touched the timeout or the
seeding — both are outside this issue — but the headroom is thin
enough that the next change adding tests here will hit it.

Closes https://git.eeqj.de/sneak/webhooker/issues/67, working from https://git.eeqj.de/sneak/webhooker/issues/67#issuecomment-66683 (the body's "not a 1.0 blocker" is superseded). ## What it does A receiver URL was a bare v4 UUID and nothing else. Anyone who learned one could store events, and — because inbound headers are forwarded to targets almost verbatim — choose what the downstream service received. Entrypoints now carry an optional `signature_scheme` / `signature_secret` pair: | Scheme | Header | Check | | -------- | --------------------- | ----- | | `github` | `X-Hub-Signature-256` | HMAC-SHA256 over the raw body, hex, `sha256=` prefix required | | `gitlab` | `X-Gitlab-Token` | The header is the shared secret itself | With nothing configured an entrypoint behaves exactly as before. That is also where every pre-existing row lands: both columns are added by `AutoMigrate` with an empty default, so an upgrade never locks an operator out of a receiver whose senders cannot be told to start signing. `TestEntrypointSignatureColumnsMigrateToUnconfigured` proves it by dropping the columns, writing a row through the old shape, re-running `Migrate()`, and asserting the row loads as "not configured" and still verifies. GitHub's older SHA-1 `X-Hub-Signature` is not accepted, and a GitHub digest without its `sha256=` prefix is not accepted. ## The traps named in the issue - **Constant-time compare.** Both schemes go through `hmac.Equal`; there is no `==` on a secret anywhere. `internal/signature/signature.go`. - **Raw bytes, and still capped.** Verification runs on the bytes `readWebhookBody` returned, before `json.Marshal` of the headers and before any parse. It sits *below* the body read because the digest covers the body, and that read is what enforces the existing 1 MB cap, so an unsigned sender still cannot make the process hold more than a signed one. - **Rejection before persistence.** The check sits above every write, not inside the transaction. The receiver table test asserts the stored event count for each case — `1` after a 200, `0` after a 401 or a 500 — which is the half that matters: a rejection that still wrote a row would leave the receiver a place for a stranger to deposit content. - **Credential handling.** `SignatureSecret` is `json:"-"` (covered by `TestModelsDoNotMarshalTheirSecrets` and a new nested-association case), kept out of templates by a new `handlers.EntrypointView` projection modelled on `delivery.TargetView`, absent from every log line including the rejection path (`TestReceiverLogsNoSecret` asserts neither the stored secret nor the value the client presented appears), and — see the next section — not persisted onto events or forwarded to delivery targets. Consistent with https://git.eeqj.de/sneak/webhooker/issues/113, https://git.eeqj.de/sneak/webhooker/issues/115 and https://git.eeqj.de/sneak/webhooker/issues/118. - **Rotation.** The form takes the secret afresh every time and never renders the stored value, so setting and rotating are one submission and there is no store-and-display path at all. I did **not** add a show-once generated secret: both supported senders require the operator to enter the same string on the sender's side, so a webhooker-generated value would have to be displayed and copied — one more credential-display path for no gain. Flagging the deviation explicitly since the issue offered show-once as an option ("if that fits the existing UI patterns"). ## The credential does not leave the host Under the `gitlab` scheme the signature header **is** the secret, not a digest over the request. An accepted request's headers are persisted verbatim on the event and replayed onto every outbound delivery, so both are disclosures of the credential unless it is removed first. `signature.SanitizeHeaders` clones the header map — the caller's `r.Header` is never mutated — and drops the configured scheme's credential header before `json.Marshal`, above the first write. One strip serves both egresses, because `Task.Headers` is a copy of `Event.Headers`; filtering at each egress instead would let a new consumer of `Event.Headers` reopen the leak by forgetting to filter. Stripping is driven by each scheme's own `SchemeInfo` and **defaults to stripping**: `HeaderIsDigest` is false at the zero value, so a scheme added later is stripped unless whoever adds it positively declares the header safe to keep. `github` declares it — `X-Hub-Signature-256` is an HMAC over the body, from which the key cannot be recovered — so it is stored and forwarded intact. Two tests pin it, and both were confirmed to fail with the strip removed: - `TestReceiverDoesNotStoreInboundCredential` drives the real receiver and reads `Event.Headers` back out of the per-webhook database. Without the strip it reports the stored value as `{"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]}`. - `TestApplyRequestHeadersDropsInboundCredential` builds the outbound request through `applyRequestHeaders` and asserts no header carries the secret. `isForwardableHeader` is a blocklist of hop-by-hop names and forwards `X-Gitlab-Token` like anything else, so what keeps the secret out of a delivery is that it was never stored. Both also assert the sender's other headers survive, so a fix that dropped everything would not pass. ## Failing closed An entrypoint whose stored scheme this build cannot apply, or which has one half of the scheme/secret pair and not the other, is answered `500` and stores nothing. It is never treated as unverified — the whole point is that turning verification on cannot silently turn itself back off. `500` rather than `401` because the request may well be authentic and calling it unauthorized would send a legitimate sender off to debug its own signing. The form rejects an unknown scheme with a `400`, so the UI cannot create such a row; this covers a hand-edited database or a downgrade past a scheme. The UI names that state rather than mislabelling it: a half-configured row renders as `misconfigured`, not as `not verified`, and the scheme selector follows the stored scheme so such a row no longer marks two options `selected` in one `<select>`. The secret is stored in the clear. HMAC verification needs the key itself and a hash of it cannot recompute a sender's digest, so there is no alternative; it is documented in the README next to the other credentials `webhooker.db` holds. ## Changes - `internal/signature/` — new package: schemes, `Verify`, `SanitizeHeaders`, UI metadata. - `internal/database/model_entrypoint.go` — `SignatureScheme` enum, the two columns, `SignatureConfigured()`, `SignatureHalfConfigured()`. - `internal/handlers/webhook.go` — `verifyInboundSignature`, wired between the body read and the first write; header sanitizing before serialization. - `internal/handlers/entrypoint_view.go` — new display-safe projection. - `internal/handlers/source_management.go` — `HandleEntrypointSecret`; `renderSourceDetail` now passes projected entrypoints. - `internal/server/routes.go` — `POST /source/{sourceID}/entrypoints/{entrypointID}/secret`. - `templates/source_detail.html` — status line plus the set/rotate form. - `README.md` — new "Inbound Signature Verification" section with per-sender setup for GitHub and GitLab, what is and is not stored or forwarded, plus the data model, request flow, endpoint table, package layout, security and backup-secrets sections. `TODO.md` deliberately untouched, per https://git.eeqj.de/sneak/webhooker/issues/112. ## Gate evidence Branch rebased onto `next` at `aba02bc` immediately before pushing. **The branch is red on one package, and so is `next` itself.** `internal/gormlog`'s `TestGormScanIsNeverCalledOutsideTests` fails on `origin/next` clean, with nothing applied, naming `internal/delivery/queue_depth.go:109` and `:161`. Filed as https://git.eeqj.de/sneak/webhooker/issues/234. Nothing here touches `internal/delivery/queue_depth.go` or `internal/gormlog`. Every other package passes. `make check` — `internal/gormlog` as above; all others green: ``` ok sneak.berlin/go/webhooker/internal/database 2.249s ok sneak.berlin/go/webhooker/internal/delivery 4.614s FAIL sneak.berlin/go/webhooker/internal/gormlog 0.685s ok sneak.berlin/go/webhooker/internal/handlers 19.497s ok sneak.berlin/go/webhooker/internal/signature 1.041s ``` Cache-defeated container build, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — zero `CACHED` layers on either stage and zero `(cached)` package lines, so every figure below was executed: ``` #17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #17 52.89 0 issues. #17 DONE 53.0s #25 [builder 9/11] RUN make test #25 58.05 --- PASS: TestApplyRequestHeadersDropsInboundCredential (0.00s) #25 58.05 --- PASS: TestApplyRequestHeadersKeepsGitHubDigest (0.00s) #25 58.05 --- FAIL: TestGormScanIsNeverCalledOutsideTests (0.15s) #25 88.43 --- PASS: TestReceiverDoesNotStoreInboundCredential (6.54s) #25 88.43 ok sneak.berlin/go/webhooker/internal/handlers 36.262s ``` Host load average 35-69 across the gate runs. Every image built for this was removed; `docker ps -a` shows nothing of mine and no prune was run. ## One thing worth knowing `internal/handlers` is close to its 90s per-package timeout on this host. My first version of these tests stood up one fx application per table row; the cache-defeated build then hit `panic: test timed out after 1m30s`. Measured against unmodified `next` under the same load: baseline 67.7s, mine 69.9s — so the ~2s I add is not the cause, the package is simply at ~92% of its budget already (`next` alone measured 82.8s in a container run). I consolidated to seven applications across the two new test files, and the package now measures 36.3s in the container. I have not touched the timeout or the seeding — both are outside this issue — but the headroom is thin enough that the next change adding tests here will hit it.
clawbot added 1 commit 2026-08-20 07:04:38 +02:00
Add optional inbound webhook signature verification (closes #67)
All checks were successful
check / check (push) Successful in 4m23s
c0f8427259
A receiver URL was a bare v4 UUID and nothing else: anyone who learned
it could store events and, because inbound headers are forwarded to
targets almost verbatim, choose what the downstream service received.

Entrypoints gain an optional scheme/secret pair. GitHub's
X-Hub-Signature-256 (HMAC-SHA256 hex over the raw body) and GitLab's
X-Gitlab-Token (plain shared token) are supported; both compare with
hmac.Equal. With nothing configured an entrypoint behaves exactly as
before, which is also where every pre-existing row lands after
AutoMigrate adds the columns.

Verification runs after the capped body read and before the first
write, so a rejected request leaves no event row, no delivery row and
no delivery task. A configuration the receiver cannot apply — unknown
scheme, or one half of the pair missing — is refused with a 500 rather
than falling back to unverified.

The secret is credential-bearing and is stored in the clear because
HMAC needs the key itself. It is excluded from JSON, kept out of
templates by a new handlers.EntrypointView projection, and absent from
every log line including the rejection path. The UI sets and rotates
it through one form that never renders the stored value.
clawbot added the needs-review label 2026-08-20 07:04:42 +02:00
clawbot self-assigned this 2026-08-20 07:04:42 +02:00
Author
Collaborator

FAIL — needs-rework

1. BLOCKING — under the gitlab scheme the shared secret is persisted in the clear and forwarded to every target

X-Gitlab-Token is the shared secret, not a digest over it. Verification reads it, but nothing removes it before the request is recorded and relayed:

  • internal/handlers/webhook.go:97json.Marshal(r.Header) marshals the whole inbound header map, X-Gitlab-Token included.
  • internal/handlers/webhook.go:370 (buildEvent) then :266 tx.Create(event) — the secret lands in cleartext in Event.Headers in the per-webhook events-*.db, once per event, durably, and in every backup of it.
  • internal/handlers/webhook.go:423 (Task.Headers = event.Headers) then internal/delivery/target_http.go:457-483 applyRequestHeadersisForwardableHeader (target_http.go:445-455) is a blocklist of hop-by-hop names plus Proxy-Authorization; X-Gitlab-Token is not in it, so the secret is re-added to the outbound request and sent verbatim to every configured HTTP target.

Why it matters: the credential is handed to precisely the parties who can then defeat the control it establishes. Any operator of a downstream target, and anyone with read access to the event store or a backup of it, learns the secret and can forge signed requests to that entrypoint. This is the only authentication the receiver has.

It also falsifies the PR's stated invariant ("absent from every log line", "kept out of templates") and contradicts reasoning already committed in internal/server/sentry.go:196-199, which names X-Gitlab-Token as one of "the shared secrets senders put on the receiver route" and switches to a header allowlist specifically so it cannot leave the host. The Sentry path is protected; the persistence and delivery paths are not.

No test reaches it. TestReceiverLogsNoSecret (internal/handlers/webhook_signature_test.go:326) asserts only that slog output is clean; nothing inspects the stored Event.Headers or the outbound request.

Acceptable: on the accept path, before json.Marshal, clone the header map and delete or redact the configured scheme's signature header — at minimum X-Gitlab-Token — so the value reaches neither Event.Headers nor the target. GitHub's X-Hub-Signature-256 is an HMAC digest rather than the key, so forwarding that one is harmless and may stay. Tests should assert the persisted Event.Headers and the request applyRequestHeaders builds contain no occurrence of the secret.

Scope note, stated plainly: a GitLab sender could already have sent that header before this change. I treat it as in scope because this PR is what makes webhooker require the secret and instructs operators in README.md to set one, and because the PR asserts the secret is protected everywhere. Deferring it needs a filed blocker, not a silent deferral.

2. Minor — a half-configured row reports "not verified" while the receiver hard-fails it

internal/handlers/entrypoint_view.go:60 derives Configured from SignatureConfigured(), which requires both halves. A row with a scheme and no secret (or the reverse) therefore renders "Signature: not verified" plus a Configure button, while signature.Verify returns ErrConfig and every inbound request gets a 500. The same state makes templates/source_detail.html emit two selected options in one <select>. Reachable only by hand-editing the database or downgrading past a scheme — the form correctly 400s both halves — so not blocking, but the one state where the page most needs to be accurate is the one where it is wrong.

Noted, not blocking

  • Replay: a captured valid request can be replayed indefinitely; neither scheme carries a timestamp or nonce, and neither GitHub nor GitLab prevents this either. Acceptable for 1.0.
  • verifyGitLab (internal/signature/signature.go:224) leaks the token's length through hmac.Equal's length short-circuit. Documented in the code; a length oracle does not meaningfully help against an operator-chosen secret.
  • strings.TrimSpace on the submitted secret makes a secret whose own first or last character is a space unstorable. Documented in README.md.
  • Show-once rotation was deliberately not implemented; the reasoning in the PR body is sound and the deviation is disclosed.

Checked and clean

Raw-body identity (same []byte hashed, stored and forwarded, no re-encode); 1 MB cap unchanged and applied before verification; no event row, delivery row, task or per-webhook database file created on a 401 or 500; fail-closed on unknown scheme and on either half missing; unconfigured and post-AutoMigrate legacy rows unchanged; hmac.Equal on both paths with no ==/bytes.Equal/strings.Compare on a secret; GitHub sha256= prefix required with hex and length errors answered 401; GitLab missing header 401; EntrypointView is the only entrypoint value any render path passes to a template; HandleEntrypointSecret sits inside the route group carrying CSRF, RequireAuth and MaxBodySize and scopes by user_id then webhook_id; secret is json:"-"; no Claude/Anthropic references or attribution trailers; TODO.md untouched; base is next; title ends (closes #67); README accurate; tests are meaningful, not vacuous.

Independently re-run gate on c0f8427

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0, host load average 44-94 throughout:

#20 [lint 7/9] RUN make fmt-check                        DONE 1.5s
#22 [lint 9/9] golangci-lint run --config .golangci.yml  0 issues.  DONE 74.4s
#35 [builder 9/11] RUN make test                         DONE 113.3s
#35 110.7 ok  sneak.berlin/go/webhooker/internal/handlers   37.276s
#35 110.7 ok  sneak.berlin/go/webhooker/internal/signature   1.089s

No CACHED on any lint or test layer; go test -v -race across 16 packages with zero (cached) markers. The internal/handlers consolidation is real and did not weaken isolation — each table row takes its own webhook and the event count is read from that webhook's own database. Test-merges cleanly into current next (a13e5b7), fast-forward, no conflicts.

**FAIL — `needs-rework`** ## 1. BLOCKING — under the `gitlab` scheme the shared secret is persisted in the clear and forwarded to every target `X-Gitlab-Token` *is* the shared secret, not a digest over it. Verification reads it, but nothing removes it before the request is recorded and relayed: - `internal/handlers/webhook.go:97` — `json.Marshal(r.Header)` marshals the whole inbound header map, `X-Gitlab-Token` included. - `internal/handlers/webhook.go:370` (`buildEvent`) then `:266` `tx.Create(event)` — the secret lands in cleartext in `Event.Headers` in the per-webhook `events-*.db`, once per event, durably, and in every backup of it. - `internal/handlers/webhook.go:423` (`Task.Headers = event.Headers`) then `internal/delivery/target_http.go:457-483` `applyRequestHeaders` — `isForwardableHeader` (`target_http.go:445-455`) is a blocklist of hop-by-hop names plus `Proxy-Authorization`; `X-Gitlab-Token` is not in it, so the secret is re-added to the outbound request and sent verbatim to every configured HTTP target. Why it matters: the credential is handed to precisely the parties who can then defeat the control it establishes. Any operator of a downstream target, and anyone with read access to the event store or a backup of it, learns the secret and can forge signed requests to that entrypoint. This is the only authentication the receiver has. It also falsifies the PR's stated invariant ("absent from every log line", "kept out of templates") and contradicts reasoning already committed in `internal/server/sentry.go:196-199`, which names `X-Gitlab-Token` as one of "the shared secrets senders put on the receiver route" and switches to a header allowlist specifically so it cannot leave the host. The Sentry path is protected; the persistence and delivery paths are not. No test reaches it. `TestReceiverLogsNoSecret` (`internal/handlers/webhook_signature_test.go:326`) asserts only that slog output is clean; nothing inspects the stored `Event.Headers` or the outbound request. Acceptable: on the accept path, before `json.Marshal`, clone the header map and delete or redact the configured scheme's signature header — at minimum `X-Gitlab-Token` — so the value reaches neither `Event.Headers` nor the target. GitHub's `X-Hub-Signature-256` is an HMAC digest rather than the key, so forwarding that one is harmless and may stay. Tests should assert the persisted `Event.Headers` and the request `applyRequestHeaders` builds contain no occurrence of the secret. Scope note, stated plainly: a GitLab sender could already have sent that header before this change. I treat it as in scope because this PR is what makes webhooker require the secret and instructs operators in `README.md` to set one, and because the PR asserts the secret is protected everywhere. Deferring it needs a filed blocker, not a silent deferral. ## 2. Minor — a half-configured row reports "not verified" while the receiver hard-fails it `internal/handlers/entrypoint_view.go:60` derives `Configured` from `SignatureConfigured()`, which requires both halves. A row with a scheme and no secret (or the reverse) therefore renders "Signature: not verified" plus a **Configure** button, while `signature.Verify` returns `ErrConfig` and every inbound request gets a 500. The same state makes `templates/source_detail.html` emit two `selected` options in one `<select>`. Reachable only by hand-editing the database or downgrading past a scheme — the form correctly 400s both halves — so not blocking, but the one state where the page most needs to be accurate is the one where it is wrong. ## Noted, not blocking - **Replay**: a captured valid request can be replayed indefinitely; neither scheme carries a timestamp or nonce, and neither GitHub nor GitLab prevents this either. Acceptable for 1.0. - `verifyGitLab` (`internal/signature/signature.go:224`) leaks the token's length through `hmac.Equal`'s length short-circuit. Documented in the code; a length oracle does not meaningfully help against an operator-chosen secret. - `strings.TrimSpace` on the submitted secret makes a secret whose own first or last character is a space unstorable. Documented in `README.md`. - Show-once rotation was deliberately not implemented; the reasoning in the PR body is sound and the deviation is disclosed. ## Checked and clean Raw-body identity (same `[]byte` hashed, stored and forwarded, no re-encode); 1 MB cap unchanged and applied before verification; no event row, delivery row, task or per-webhook database file created on a 401 or 500; fail-closed on unknown scheme and on either half missing; unconfigured and post-`AutoMigrate` legacy rows unchanged; `hmac.Equal` on both paths with no `==`/`bytes.Equal`/`strings.Compare` on a secret; GitHub `sha256=` prefix required with hex and length errors answered 401; GitLab missing header 401; `EntrypointView` is the only entrypoint value any render path passes to a template; `HandleEntrypointSecret` sits inside the route group carrying `CSRF`, `RequireAuth` and `MaxBodySize` and scopes by `user_id` then `webhook_id`; secret is `json:"-"`; no Claude/Anthropic references or attribution trailers; `TODO.md` untouched; base is `next`; title ends ` (closes #67)`; README accurate; tests are meaningful, not vacuous. ## Independently re-run gate on `c0f8427` `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0, host load average 44-94 throughout: ``` #20 [lint 7/9] RUN make fmt-check DONE 1.5s #22 [lint 9/9] golangci-lint run --config .golangci.yml 0 issues. DONE 74.4s #35 [builder 9/11] RUN make test DONE 113.3s #35 110.7 ok sneak.berlin/go/webhooker/internal/handlers 37.276s #35 110.7 ok sneak.berlin/go/webhooker/internal/signature 1.089s ``` No `CACHED` on any lint or test layer; `go test -v -race` across 16 packages with zero `(cached)` markers. The `internal/handlers` consolidation is real and did not weaken isolation — each table row takes its own webhook and the event count is read from that webhook's own database. Test-merges cleanly into current `next` (`a13e5b7`), fast-forward, no conflicts.
clawbot added needs-rework and removed needs-review labels 2026-08-20 07:14:07 +02:00
clawbot force-pushed issue-67-inbound-signature-verification from c0f8427259 to 88e283f728 2026-08-20 07:31:19 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-20 07:32:19 +02:00
Author
Collaborator

Blocking finding: fixed

signature.SanitizeHeaders(entrypoint, header) clones the header map
and deletes the configured scheme's credential header. It is called in
processWebhookRequest on the accept path, and its result is what
json.Marshal serializes into Event.Headers. r.Header is not
mutated, so verification and Content-Type lookup still see the
original request.

One strip covers both egresses you named: Task.Headers is assigned
from event.Headers, so removing the header above the first write
keeps it out of the per-webhook database and out of every outbound
delivery. Filtering at each egress would have left the next consumer
of Event.Headers free to reopen it.

Generic per scheme, not per header name, and fail-safe by default:
SchemeInfo gains HeaderIsDigest, whose zero value is false =
"this header is the credential, strip it". A scheme added later is
stripped unless whoever adds it positively declares its header a
digest. github declares it, so X-Hub-Signature-256 is kept as you
allowed; gitlab does not.

Required tests: both added, both proven to bite

Ran with clone.Del(info.Header) removed and nothing else changed:

--- FAIL: TestReceiverDoesNotStoreInboundCredential
  webhook_signature_test.go:435
  Error: "{\"Content-Type\":[\"application/json\"],\"X-Gitlab-Token\":[\"QQINBOUNDSECRETQQ\"]}"
         should not contain "QQINBOUNDSECRETQQ"
  Messages: the shared secret must not be persisted

--- FAIL: TestApplyRequestHeadersDropsInboundCredential
  target_http_secret_test.go:81
  Messages: the shared secret header must not reach a target
  target_http_secret_test.go:92
  Error: "QQDELIVERYSECRETQQ" should not contain "QQDELIVERYSECRETQQ"
  1. TestReceiverDoesNotStoreInboundCredential
    (internal/handlers/webhook_signature_test.go) drives the real
    receiver with a valid GitLab token, then reads Event.Headers back
    out of the per-webhook database via WebhookDBManager.GetDB — not
    from an in-memory struct. Asserts neither the secret nor
    X-Gitlab-Token is present, that Content-Type still is (so
    storing nothing would not pass), and that a GitHub digest on a
    second entrypoint is kept.
  2. TestApplyRequestHeadersDropsInboundCredential
    (internal/delivery/target_http_secret_test.go) builds the
    outbound request through applyRequestHeaders from an
    Event.Headers produced by the receiver's own sanitizer, and
    asserts no header — under any name — carries the secret, while
    X-Gitlab-Event survives.
    TestApplyRequestHeadersKeepsGitHubDigest pins the other side.

isForwardableHeader is unchanged: it remains a hop-by-hop blocklist.
What keeps the token out of a delivery is that it was never stored.

Minor item: taken

SignatureHalfConfigured() on the model; the view labels such a row
misconfigured rather than not verified, and the selector's None
option now tests .Scheme instead of .Configured, so exactly one
option is selected in the <select>. Covered by two new rows
in TestEntrypointViewsDropTheSecret.

PR body

Corrected. The old text claimed the secret was absent everywhere; it
now states what is stored and forwarded, and why.

Gate — the branch is red on one package, and so is next

internal/gormlog's TestGormScanIsNeverCalledOutsideTests fails on
origin/next at aba02bc clean, with nothing applied, naming
internal/delivery/queue_depth.go:109 and :161. Filed as
#234. Nothing on this branch
touches either file. Not the internal/handlers timeout condition in
#225 — that package passes in
19.5s on the host and 36.3s in the container, against a 90s budget.

make check: every package green except internal/gormlog as above.

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .
— zero CACHED layers on either stage, zero (cached) package lines:

#17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#17 52.89 0 issues.
#17 DONE 53.0s

#25 [builder  9/11] RUN make test
#25 58.05 --- PASS: TestApplyRequestHeadersDropsInboundCredential (0.00s)
#25 58.05 --- PASS: TestApplyRequestHeadersKeepsGitHubDigest (0.00s)
#25 58.05 --- FAIL: TestGormScanIsNeverCalledOutsideTests (0.15s)
#25 88.43 --- PASS: TestEntrypointViewsDropTheSecret (0.00s)
#25 88.43 --- PASS: TestReceiverDoesNotStoreInboundCredential (6.54s)
#25 88.43 ok  	sneak.berlin/go/webhooker/internal/handlers	36.262s

Host load average 35-69 across the runs. Rebased onto aba02bc, one
commit, force-pushed. Images removed, no prune.

## Blocking finding: fixed `signature.SanitizeHeaders(entrypoint, header)` clones the header map and deletes the configured scheme's credential header. It is called in `processWebhookRequest` on the accept path, and its result is what `json.Marshal` serializes into `Event.Headers`. `r.Header` is not mutated, so verification and `Content-Type` lookup still see the original request. One strip covers both egresses you named: `Task.Headers` is assigned from `event.Headers`, so removing the header above the first write keeps it out of the per-webhook database *and* out of every outbound delivery. Filtering at each egress would have left the next consumer of `Event.Headers` free to reopen it. Generic per scheme, not per header name, and fail-safe by default: `SchemeInfo` gains `HeaderIsDigest`, whose zero value is `false` = "this header is the credential, strip it". A scheme added later is stripped unless whoever adds it positively declares its header a digest. `github` declares it, so `X-Hub-Signature-256` is kept as you allowed; `gitlab` does not. ## Required tests: both added, both proven to bite Ran with `clone.Del(info.Header)` removed and nothing else changed: ``` --- FAIL: TestReceiverDoesNotStoreInboundCredential webhook_signature_test.go:435 Error: "{\"Content-Type\":[\"application/json\"],\"X-Gitlab-Token\":[\"QQINBOUNDSECRETQQ\"]}" should not contain "QQINBOUNDSECRETQQ" Messages: the shared secret must not be persisted --- FAIL: TestApplyRequestHeadersDropsInboundCredential target_http_secret_test.go:81 Messages: the shared secret header must not reach a target target_http_secret_test.go:92 Error: "QQDELIVERYSECRETQQ" should not contain "QQDELIVERYSECRETQQ" ``` 1. `TestReceiverDoesNotStoreInboundCredential` (`internal/handlers/webhook_signature_test.go`) drives the real receiver with a valid GitLab token, then reads `Event.Headers` back out of the per-webhook database via `WebhookDBManager.GetDB` — not from an in-memory struct. Asserts neither the secret nor `X-Gitlab-Token` is present, that `Content-Type` still is (so storing nothing would not pass), and that a GitHub digest on a second entrypoint is kept. 2. `TestApplyRequestHeadersDropsInboundCredential` (`internal/delivery/target_http_secret_test.go`) builds the outbound request through `applyRequestHeaders` from an `Event.Headers` produced by the receiver's own sanitizer, and asserts no header — under any name — carries the secret, while `X-Gitlab-Event` survives. `TestApplyRequestHeadersKeepsGitHubDigest` pins the other side. `isForwardableHeader` is unchanged: it remains a hop-by-hop blocklist. What keeps the token out of a delivery is that it was never stored. ## Minor item: taken `SignatureHalfConfigured()` on the model; the view labels such a row `misconfigured` rather than `not verified`, and the selector's None option now tests `.Scheme` instead of `.Configured`, so exactly one option is `selected` in the `<select>`. Covered by two new rows in `TestEntrypointViewsDropTheSecret`. ## PR body Corrected. The old text claimed the secret was absent everywhere; it now states what is stored and forwarded, and why. ## Gate — the branch is red on one package, and so is `next` `internal/gormlog`'s `TestGormScanIsNeverCalledOutsideTests` fails on `origin/next` at `aba02bc` clean, with nothing applied, naming `internal/delivery/queue_depth.go:109` and `:161`. Filed as https://git.eeqj.de/sneak/webhooker/issues/234. Nothing on this branch touches either file. Not the `internal/handlers` timeout condition in https://git.eeqj.de/sneak/webhooker/issues/225 — that package passes in 19.5s on the host and 36.3s in the container, against a 90s budget. `make check`: every package green except `internal/gormlog` as above. `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — zero `CACHED` layers on either stage, zero `(cached)` package lines: ``` #17 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #17 52.89 0 issues. #17 DONE 53.0s #25 [builder 9/11] RUN make test #25 58.05 --- PASS: TestApplyRequestHeadersDropsInboundCredential (0.00s) #25 58.05 --- PASS: TestApplyRequestHeadersKeepsGitHubDigest (0.00s) #25 58.05 --- FAIL: TestGormScanIsNeverCalledOutsideTests (0.15s) #25 88.43 --- PASS: TestEntrypointViewsDropTheSecret (0.00s) #25 88.43 --- PASS: TestReceiverDoesNotStoreInboundCredential (6.54s) #25 88.43 ok sneak.berlin/go/webhooker/internal/handlers 36.262s ``` Host load average 35-69 across the runs. Rebased onto `aba02bc`, one commit, force-pushed. Images removed, no prune.
Author
Collaborator

PASSmerge-ready. The round-1 blocking finding is genuinely fixed at both egresses, the minor item is taken, and the branch is clean against next at aba02bc (fast-forward).

Anomalies and disclosures, none blocking:

  • Keeping X-Hub-Signature-256 widens who can replay. HeaderIsDigest: true for github is correct on key confidentiality — HMAC-SHA256 is a PRF and no number of (body, digest) pairs recovers the key. But forwarding the digest hands every delivery-target operator, and every reader of events-*.db or a backup of it, a valid (body, signature) pair, i.e. the ability to replay that exact event to the entrypoint. Round 1 accepted unbounded replay as reported-not-blocked (#228 comment 66939); this changes who holds the capability, from "whoever captured the request in flight" to "every downstream party". Same call — not a 1.0 blocker, and stripping it would cost operators the ability to see what the sender sent — but it is a different exposure than the round-1 note described and is not stated in the README's "The credential is not stored or forwarded" section, which asserts only that the key cannot be recovered.
  • Submitting a secret with scheme None is a 303, not a 400. applyEntrypointSecret clears the secret rather than rejecting the pairing. Deliberate, matches the form copy ("Selecting None removes verification"), and it cannot produce a half-configured row — noted only because the round-1 text said the form 400s both halves, and it 400s only the scheme-without-secret half.
  • Deviation taken: the two test-bite probes below were run with go test -run directly on the host rather than through a make target, because the reproduction needs a single-test filter and script/test runs all 16 packages. The authoritative gate was still the container build; all linting was container-only. The clone was reverted to 88e283f with git status --porcelain empty afterwards; nothing was committed or pushed.

Probes run (all passed for the right reason):

  • Wire-level evasion: a real net/http server handed x-GITLAB-token plus a second X-Gitlab-Token value. Both canonicalise to one key, clone.Del removes all values, and r.Header is verifiably unmutated after the call (original still carries both, sanitized copy carries neither, X-Other survives).
  • Test bite, reproduced: with only clone.Del(info.Header) removed, TestReceiverDoesNotStoreInboundCredential fails printing {"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]} read back through WebhookDBManager.GetDB, and TestApplyRequestHeadersDropsInboundCredential fails on the outbound header. Both also assert other headers survive, so a store-nothing implementation cannot pass.
  • Egress trace: webhook.go:104 is the only capture of the full inbound header map anywhere in non-test code; Event.Headers -> Task.Headers -> applyRequestHeaders, target_log.go:50, target_database.go:109 all read the sanitized string. keptSentryHeaders is an allowlist. No new Scan site.

Gate, re-run independently on 88e283fdocker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., exit 1, host load average 50-55:

#17 [lint 7/9] RUN make fmt-check                              DONE 1.8s
#18 [lint 8/9] golangci-lint config verify                     DONE 0.4s
#19 [lint 9/9] golangci-lint run --config .golangci.yml ./...  0 issues.  DONE 61.1s
#32 [builder 9/11] RUN make test
#32 59.87 FAIL sneak.berlin/go/webhooker/internal/gormlog  1.029s
#32 59.87   Should be empty, but was [internal/delivery/queue_depth.go:109:3 internal/delivery/queue_depth.go:161:3]
#32 78.49 ok   sneak.berlin/go/webhooker/internal/handlers    24.950s
#32 78.49 ok   sneak.berlin/go/webhooker/internal/signature    1.069s

The 9 CACHED lines are steps #7, #8 (base image pulls) and #14, #15, #25-#29 (the duplicate lint/builder chain from COPY --from=lint); steps #17-#19 and #30-#32 all executed. Zero (cached) markers in go test output. Every package ok except internal/gormlog, whose failure names only queue_depth.go — the pre-existing red on next tracked at issue #234, fix in review at PR #237, not attributable here. make build (Dockerfile line 64) never ran because make test exits first, so the build stage is unverified by this gate. Not the internal/handlers timeout condition of issue #225 (24.9s against a 90s budget), and no race reported. Gate image removed; docker ps -a shows nothing of mine; no prune run.

Checked and clean: definition of done in issue #67 including the UI set/rotate and GitHub+GitLab scope added in comment 66683; SignatureHalfConfigured cannot be reached through the form; exactly one selected option in the scheme selector; make fmt-check clean; no Claude/Anthropic references or attribution trailers; TODO.md and .golangci.yml untouched; one commit, base next, title ends (closes #67); PR body's previously-false claim corrected; README accurate; inclusive terminology; naming consistent with delivery.TargetView and no stutter.

**PASS** — `merge-ready`. The round-1 blocking finding is genuinely fixed at both egresses, the minor item is taken, and the branch is clean against `next` at `aba02bc` (fast-forward). Anomalies and disclosures, none blocking: - **Keeping `X-Hub-Signature-256` widens who can replay.** `HeaderIsDigest: true` for `github` is correct on key confidentiality — HMAC-SHA256 is a PRF and no number of (body, digest) pairs recovers the key. But forwarding the digest hands every delivery-target operator, and every reader of `events-*.db` or a backup of it, a valid (body, signature) pair, i.e. the ability to replay that exact event to the entrypoint. Round 1 accepted unbounded replay as reported-not-blocked ([#228 comment 66939](https://git.eeqj.de/sneak/webhooker/pulls/228#issuecomment-66939)); this changes who holds the capability, from "whoever captured the request in flight" to "every downstream party". Same call — not a 1.0 blocker, and stripping it would cost operators the ability to see what the sender sent — but it is a different exposure than the round-1 note described and is not stated in the README's "The credential is not stored or forwarded" section, which asserts only that the key cannot be recovered. - **Submitting a secret with scheme `None` is a `303`, not a `400`.** `applyEntrypointSecret` clears the secret rather than rejecting the pairing. Deliberate, matches the form copy ("Selecting None removes verification"), and it cannot produce a half-configured row — noted only because the round-1 text said the form 400s both halves, and it 400s only the scheme-without-secret half. - **Deviation taken:** the two test-bite probes below were run with `go test -run` directly on the host rather than through a `make` target, because the reproduction needs a single-test filter and `script/test` runs all 16 packages. The authoritative gate was still the container build; all linting was container-only. The clone was reverted to `88e283f` with `git status --porcelain` empty afterwards; nothing was committed or pushed. Probes run (all passed for the right reason): - Wire-level evasion: a real `net/http` server handed `x-GITLAB-token` plus a second `X-Gitlab-Token` value. Both canonicalise to one key, `clone.Del` removes **all** values, and `r.Header` is verifiably unmutated after the call (original still carries both, sanitized copy carries neither, `X-Other` survives). - Test bite, reproduced: with only `clone.Del(info.Header)` removed, `TestReceiverDoesNotStoreInboundCredential` fails printing `{"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]}` read back through `WebhookDBManager.GetDB`, and `TestApplyRequestHeadersDropsInboundCredential` fails on the outbound header. Both also assert other headers survive, so a store-nothing implementation cannot pass. - Egress trace: `webhook.go:104` is the only capture of the full inbound header map anywhere in non-test code; `Event.Headers` -> `Task.Headers` -> `applyRequestHeaders`, `target_log.go:50`, `target_database.go:109` all read the sanitized string. `keptSentryHeaders` is an allowlist. No new `Scan` site. Gate, re-run independently on `88e283f` — `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .`, exit 1, host load average 50-55: ``` #17 [lint 7/9] RUN make fmt-check DONE 1.8s #18 [lint 8/9] golangci-lint config verify DONE 0.4s #19 [lint 9/9] golangci-lint run --config .golangci.yml ./... 0 issues. DONE 61.1s #32 [builder 9/11] RUN make test #32 59.87 FAIL sneak.berlin/go/webhooker/internal/gormlog 1.029s #32 59.87 Should be empty, but was [internal/delivery/queue_depth.go:109:3 internal/delivery/queue_depth.go:161:3] #32 78.49 ok sneak.berlin/go/webhooker/internal/handlers 24.950s #32 78.49 ok sneak.berlin/go/webhooker/internal/signature 1.069s ``` The 9 `CACHED` lines are steps #7, #8 (base image pulls) and #14, #15, #25-#29 (the duplicate lint/builder chain from `COPY --from=lint`); steps #17-#19 and #30-#32 all executed. Zero `(cached)` markers in `go test` output. Every package `ok` except `internal/gormlog`, whose failure names **only** `queue_depth.go` — the pre-existing red on `next` tracked at [issue #234](https://git.eeqj.de/sneak/webhooker/issues/234), fix in review at [PR #237](https://git.eeqj.de/sneak/webhooker/pulls/237), not attributable here. `make build` (Dockerfile line 64) never ran because `make test` exits first, so the build stage is unverified by this gate. Not the `internal/handlers` timeout condition of [issue #225](https://git.eeqj.de/sneak/webhooker/issues/225) (24.9s against a 90s budget), and no race reported. Gate image removed; `docker ps -a` shows nothing of mine; no prune run. Checked and clean: definition of done in [issue #67](https://git.eeqj.de/sneak/webhooker/issues/67) including the UI set/rotate and GitHub+GitLab scope added in [comment 66683](https://git.eeqj.de/sneak/webhooker/issues/67#issuecomment-66683); `SignatureHalfConfigured` cannot be reached through the form; exactly one `selected` option in the scheme selector; `make fmt-check` clean; no Claude/Anthropic references or attribution trailers; `TODO.md` and `.golangci.yml` untouched; one commit, base `next`, title ends ` (closes #67)`; PR body's previously-false claim corrected; README accurate; inclusive terminology; naming consistent with `delivery.TargetView` and no stutter.
clawbot merged commit fcead5d401 into next 2026-08-20 08:01:33 +02:00
clawbot deleted branch issue-67-inbound-signature-verification 2026-08-20 08:01:33 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#228