Add optional inbound webhook signature verification (closes #67) #228
Reference in New Issue
Block a user
Delete Branch "issue-67-inbound-signature-verification"
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?
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_secretpair:
githubX-Hub-Signature-256sha256=prefix requiredgitlabX-Gitlab-TokenWith nothing configured an entrypoint behaves exactly as before. That
is also where every pre-existing row lands: both columns are added by
AutoMigratewith an empty default, so an upgrade never locks anoperator out of a receiver whose senders cannot be told to start
signing.
TestEntrypointSignatureColumnsMigrateToUnconfiguredprovesit 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-Signatureis not accepted, and a GitHubdigest without its
sha256=prefix is not accepted.The traps named in the issue
hmac.Equal;there is no
==on a secret anywhere.internal/signature/signature.go.readWebhookBodyreturned, beforejson.Marshalof the headers andbefore 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.
not inside the transaction. The receiver table test asserts the
stored event count for each case —
1after a 200,0after a 401or 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.
SignatureSecretisjson:"-"(covered byTestModelsDoNotMarshalTheirSecretsand a new nested-associationcase), kept out of templates by a new
handlers.EntrypointViewprojection modelled on
delivery.TargetView, absent from every logline including the rejection path (
TestReceiverLogsNoSecretasserts 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.
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
gitlabscheme the signature header is the secret, nota 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.SanitizeHeadersclones the header map — the caller'sr.Headeris never mutated — and drops the configured scheme'scredential header before
json.Marshal, above the first write. Onestrip serves both egresses, because
Task.Headersis a copy ofEvent.Headers; filtering at each egress instead would let a newconsumer of
Event.Headersreopen the leak by forgetting to filter.Stripping is driven by each scheme's own
SchemeInfoand defaultsto stripping:
HeaderIsDigestis false at the zero value, so ascheme added later is stripped unless whoever adds it positively
declares the header safe to keep.
githubdeclares it —X-Hub-Signature-256is an HMAC over the body, from which the keycannot be recovered — so it is stored and forwarded intact.
Two tests pin it, and both were confirmed to fail with the strip
removed:
TestReceiverDoesNotStoreInboundCredentialdrives the real receiverand reads
Event.Headersback out of the per-webhook database.Without the strip it reports the stored value as
{"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]}.TestApplyRequestHeadersDropsInboundCredentialbuilds the outboundrequest through
applyRequestHeadersand asserts no header carriesthe secret.
isForwardableHeaderis a blocklist of hop-by-hop namesand forwards
X-Gitlab-Tokenlike anything else, so what keeps thesecret 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
500and stores nothing. It is never treated as unverified — the whole point
is that turning verification on cannot silently turn itself back off.
500rather than401because the request may well be authentic andcalling it unauthorized would send a legitimate sender off to debug its
own signing. The form rejects an unknown scheme with a
400, so the UIcannot 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 asnot verified, and the schemeselector follows the stored scheme so such a row no longer marks two
options
selectedin 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.dbholds.Changes
internal/signature/— new package: schemes,Verify,SanitizeHeaders, UI metadata.internal/database/model_entrypoint.go—SignatureSchemeenum, thetwo columns,
SignatureConfigured(),SignatureHalfConfigured().internal/handlers/webhook.go—verifyInboundSignature, wiredbetween 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;renderSourceDetailnow 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 withper-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.mddeliberately untouched, per#112.
Gate evidence
Branch rebased onto
nextataba02bcimmediately before pushing.The branch is red on one package, and so is
nextitself.internal/gormlog'sTestGormScanIsNeverCalledOutsideTestsfails onorigin/nextclean, with nothing applied, naminginternal/delivery/queue_depth.go:109and:161. Filed as#234. Nothing here touches
internal/delivery/queue_depth.goorinternal/gormlog. Every otherpackage passes.
make check—internal/gormlogas above; all others green:Cache-defeated container build,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— zero
CACHEDlayers on either stage and zero(cached)packagelines, so every figure below was executed:
Host load average 35-69 across the gate runs. Every image built for
this was removed;
docker ps -ashows nothing of mine and no prune wasrun.
One thing worth knowing
internal/handlersis close to its 90s per-package timeout on thishost. 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 unmodifiednextunder 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
(
nextalone measured 82.8s in a container run). I consolidated toseven 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.
FAIL —
needs-rework1. BLOCKING — under the
gitlabscheme the shared secret is persisted in the clear and forwarded to every targetX-Gitlab-Tokenis 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-Tokenincluded.internal/handlers/webhook.go:370(buildEvent) then:266tx.Create(event)— the secret lands in cleartext inEvent.Headersin the per-webhookevents-*.db, once per event, durably, and in every backup of it.internal/handlers/webhook.go:423(Task.Headers = event.Headers) theninternal/delivery/target_http.go:457-483applyRequestHeaders—isForwardableHeader(target_http.go:445-455) is a blocklist of hop-by-hop names plusProxy-Authorization;X-Gitlab-Tokenis 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 namesX-Gitlab-Tokenas 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 storedEvent.Headersor 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 minimumX-Gitlab-Token— so the value reaches neitherEvent.Headersnor the target. GitHub'sX-Hub-Signature-256is an HMAC digest rather than the key, so forwarding that one is harmless and may stay. Tests should assert the persistedEvent.Headersand the requestapplyRequestHeadersbuilds 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.mdto 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:60derivesConfiguredfromSignatureConfigured(), which requires both halves. A row with a scheme and no secret (or the reverse) therefore renders "Signature: not verified" plus a Configure button, whilesignature.VerifyreturnsErrConfigand every inbound request gets a 500. The same state makestemplates/source_detail.htmlemit twoselectedoptions 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
verifyGitLab(internal/signature/signature.go:224) leaks the token's length throughhmac.Equal's length short-circuit. Documented in the code; a length oracle does not meaningfully help against an operator-chosen secret.strings.TrimSpaceon the submitted secret makes a secret whose own first or last character is a space unstorable. Documented inREADME.md.Checked and clean
Raw-body identity (same
[]bytehashed, 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-AutoMigratelegacy rows unchanged;hmac.Equalon both paths with no==/bytes.Equal/strings.Compareon a secret; GitHubsha256=prefix required with hex and length errors answered 401; GitLab missing header 401;EntrypointViewis the only entrypoint value any render path passes to a template;HandleEntrypointSecretsits inside the route group carryingCSRF,RequireAuthandMaxBodySizeand scopes byuser_idthenwebhook_id; secret isjson:"-"; no Claude/Anthropic references or attribution trailers;TODO.mduntouched; base isnext; title ends(closes #67); README accurate; tests are meaningful, not vacuous.Independently re-run gate on
c0f8427docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0, host load average 44-94 throughout:No
CACHEDon any lint or test layer;go test -v -raceacross 16 packages with zero(cached)markers. Theinternal/handlersconsolidation 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 currentnext(a13e5b7), fast-forward, no conflicts.c0f8427259to88e283f728Blocking finding: fixed
signature.SanitizeHeaders(entrypoint, header)clones the header mapand deletes the configured scheme's credential header. It is called in
processWebhookRequeston the accept path, and its result is whatjson.Marshalserializes intoEvent.Headers.r.Headeris notmutated, so verification and
Content-Typelookup still see theoriginal request.
One strip covers both egresses you named:
Task.Headersis assignedfrom
event.Headers, so removing the header above the first writekeeps 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.Headersfree to reopen it.Generic per scheme, not per header name, and fail-safe by default:
SchemeInfogainsHeaderIsDigest, whose zero value isfalse="this header is the credential, strip it". A scheme added later is
stripped unless whoever adds it positively declares its header a
digest.
githubdeclares it, soX-Hub-Signature-256is kept as youallowed;
gitlabdoes not.Required tests: both added, both proven to bite
Ran with
clone.Del(info.Header)removed and nothing else changed:TestReceiverDoesNotStoreInboundCredential(
internal/handlers/webhook_signature_test.go) drives the realreceiver with a valid GitLab token, then reads
Event.Headersbackout of the per-webhook database via
WebhookDBManager.GetDB— notfrom an in-memory struct. Asserts neither the secret nor
X-Gitlab-Tokenis present, thatContent-Typestill is (sostoring nothing would not pass), and that a GitHub digest on a
second entrypoint is kept.
TestApplyRequestHeadersDropsInboundCredential(
internal/delivery/target_http_secret_test.go) builds theoutbound request through
applyRequestHeadersfrom anEvent.Headersproduced by the receiver's own sanitizer, andasserts no header — under any name — carries the secret, while
X-Gitlab-Eventsurvives.TestApplyRequestHeadersKeepsGitHubDigestpins the other side.isForwardableHeaderis 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 rowmisconfiguredrather thannot verified, and the selector's Noneoption now tests
.Schemeinstead of.Configured, so exactly oneoption is
selectedin the<select>. Covered by two new rowsin
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
nextinternal/gormlog'sTestGormScanIsNeverCalledOutsideTestsfails onorigin/nextataba02bcclean, with nothing applied, naminginternal/delivery/queue_depth.go:109and:161. Filed as#234. Nothing on this branch
touches either file. Not the
internal/handlerstimeout 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 exceptinternal/gormlogas above.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— zero
CACHEDlayers on either stage, zero(cached)package lines:Host load average 35-69 across the runs. Rebased onto
aba02bc, onecommit, force-pushed. Images removed, no prune.
PASS —
merge-ready. The round-1 blocking finding is genuinely fixed at both egresses, the minor item is taken, and the branch is clean againstnextataba02bc(fast-forward).Anomalies and disclosures, none blocking:
X-Hub-Signature-256widens who can replay.HeaderIsDigest: trueforgithubis 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 ofevents-*.dbor 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.Noneis a303, not a400.applyEntrypointSecretclears 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.go test -rundirectly on the host rather than through amaketarget, because the reproduction needs a single-test filter andscript/testruns all 16 packages. The authoritative gate was still the container build; all linting was container-only. The clone was reverted to88e283fwithgit status --porcelainempty afterwards; nothing was committed or pushed.Probes run (all passed for the right reason):
net/httpserver handedx-GITLAB-tokenplus a secondX-Gitlab-Tokenvalue. Both canonicalise to one key,clone.Delremoves all values, andr.Headeris verifiably unmutated after the call (original still carries both, sanitized copy carries neither,X-Othersurvives).clone.Del(info.Header)removed,TestReceiverDoesNotStoreInboundCredentialfails printing{"Content-Type":["application/json"],"X-Gitlab-Token":["QQINBOUNDSECRETQQ"]}read back throughWebhookDBManager.GetDB, andTestApplyRequestHeadersDropsInboundCredentialfails on the outbound header. Both also assert other headers survive, so a store-nothing implementation cannot pass.webhook.go:104is 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:109all read the sanitized string.keptSentryHeadersis an allowlist. No newScansite.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:The 9
CACHEDlines are steps #7, #8 (base image pulls) and #14, #15, #25-#29 (the duplicate lint/builder chain fromCOPY --from=lint); steps #17-#19 and #30-#32 all executed. Zero(cached)markers ingo testoutput. Every packageokexceptinternal/gormlog, whose failure names onlyqueue_depth.go— the pre-existing red onnexttracked at issue #234, fix in review at PR #237, not attributable here.make build(Dockerfile line 64) never ran becausemake testexits first, so the build stage is unverified by this gate. Not theinternal/handlerstimeout condition of issue #225 (24.9s against a 90s budget), and no race reported. Gate image removed;docker ps -ashows 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;
SignatureHalfConfiguredcannot be reached through the form; exactly oneselectedoption in the scheme selector;make fmt-checkclean; no Claude/Anthropic references or attribution trailers;TODO.mdand.golangci.ymluntouched; one commit, basenext, title ends(closes #67); PR body's previously-false claim corrected; README accurate; inclusive terminology; naming consistent withdelivery.TargetViewand no stutter.clawbot referenced this pull request2026-08-20 08:21:53 +02:00