1 Commits

Author SHA1 Message Date
clawbot
24af4b4200 Harden operator-set target headers (closes #233)
All checks were successful
check / check (push) Successful in 2m56s
Three findings from the review of the per-target request headers
feature, plus the follow-up they raised about the inbound headers
the same delivery path forwards.

One rule now governs every header a delivery carries on someone
else's behalf: a redirect hop that leaves the origin the target
names carries none of them. That covers the operator's configured
headers and the inbound event headers forwarded from the sender
alike. net/http withholds only Authorization and Cookie across a
host change, so an operator's X-Api-Key or a sender's
X-Hub-Signature would otherwise follow a 302 to a host nobody
configured. Redirects are still followed — refusing them would
break every destination that legitimately redirects and would
record the 3xx as the delivery's result — but a hop to another
host, another port, or down from https to http drops the lot. The
shared SSRF-safe transport is kept on that client, so each hop is
still dialled through the private-IP guard.

The set to strip is not a name list. applyRequestHeaders now
returns the canonical names of everything it applied on the
sender's or operator's behalf, and the redirect policy strips
exactly that, so a header added to the forward set is covered
without a second edit. Content-Type and User-Agent are the
delivery path's own rather than anyone else's, and both are
excluded from that set so they always travel: Content-Type is set
from the event and a 307 preserves the body across hosts, so it
has to stay typed, and User-Agent is overwritten with this
delivery path's own after the forwarded headers are applied, so
the sender's never reaches the wire and stripping it off-origin
would only substitute net/http's default.

The origin comparison no longer collapses two IPv6 origins into
one. Hostname() unwraps a literal's brackets, so re-appending the
port with a bare colon rendered https://[2001:db8::1]:8080 and
https://[2001:db8::1:8080] identically — a different address on a
different port passing as the same origin. The port is joined with
net.JoinHostPort, and both spellings are in TestSameDeliveryOrigin.

The ten-hop cap gains a regression test. Installing a CheckRedirect
is precisely what discards net/http's own limit, so a
self-redirecting destination is driven through the policy and
asserted to stop after exactly ten requests with the sentinel
surfacing to the caller.

Trailer joins the reserved names. net/http strips it from the
request it writes, so a configured one was accepted, stored, and
provably never sent.

The invalid-header-name error no longer quotes the text before the
first colon. That text is only a name if it parses as one; when it
does not, a pasted value whose own colon split the line put half a
token into a 400 body. TestParseTargetHeaders_ErrorsNeverQuoteAValue
asserted this invariant while only exercising the after-the-colon
case, and now covers the before-the-colon one.

README documents the http target's config keys, the 300-second
timeout ceiling, the reserved-header list and the redirect
behaviour as one rule over both header classes, including that the
drop is per hop rather than permanent: net/http re-copies the
initial request's headers each hop, so a chain returning to the
configured origin carries them again, exactly as it treats
Authorization. It also records what following a 301, 302 or 303
costs, since that is net/http's own behaviour and the decision to
follow redirects is what buys it: the POST becomes a GET and the
event body and its Content-Type are dropped, so the destination
the chain ends at receives no event while the delivery is still
recorded Delivered. The edit form's hint gains Trailer and the
redirect note.

Closes #243
2026-08-20 08:39:21 +00:00

113
TODO.md
View File

@@ -18,52 +18,68 @@ Issue branches do NOT touch this file — the manager maintains it on
# Status # Status
1.0.0 is complete: 55 closed, 0 open. `next` (6874059) is 62 commits pre-1.0. No git tags exist. `main` (4f5ecb1) is a working webhook proxy
ahead of `main` and a strict fast-forward. No git tags exist yet. with auth, CSRF/SSRF protections, login rate limiting, Slack target,
event retention (#63), the database archiving target (#43), the admin
password change flow (#65), policy compliance (#6), pinned lint tooling
(#55), and fail-loud configuration parsing (#80).
The bar was not "the milestone is empty" but "sneak can deploy this and `next` is green — verified both by CI and by cache-defeated container
use it in low-volume production". Every gap the deployability audit runs (`docker build --no-cache-filter=lint --no-cache-filter=builder`) —
named against that bar is now closed: but the **1.0.0 milestone is no longer complete**. It was reopened on
2026-08-20 by a code-level deployability audit that ran the service end
to end (verdict:
https://git.eeqj.de/sneak/webhooker/issues/33#issuecomment-66686).
- `DATA_DIR` locking, so two instances cannot both deliver The bar for 1.0 is not "the milestone is empty" but "sneak can deploy
(https://git.eeqj.de/sneak/webhooker/issues/201) this and use it in low-volume production". The audit found the gap
- shutdown on listener failure, rather than a live non-serving process between those two: two instances on one `DATA_DIR` both deliver
(https://git.eeqj.de/sneak/webhooker/issues/200) (reproduced), a failed listen leaves a live non-serving process that
- inbound signature verification restart policies never fire on, there is no inbound authentication of
(https://git.eeqj.de/sneak/webhooker/issues/67) any kind, delivery failures render as a bare word with no status code or
- per-attempt delivery detail in the event log error, a terminally failed delivery can never be replayed, the SSRF
(https://git.eeqj.de/sneak/webhooker/issues/202) blocklist has no escape hatch so the proxy cannot forward to your own
- replay of a terminally failed delivery network at all, and target credentials leak into the per-webhook event
(https://git.eeqj.de/sneak/webhooker/issues/203) databases.
- `ALLOWED_EGRESS_CIDRS`, an allowlist escape hatch for the SSRF guard
(https://git.eeqj.de/sneak/webhooker/issues/204)
- the three credential exposures
(https://git.eeqj.de/sneak/webhooker/issues/205,
https://git.eeqj.de/sneak/webhooker/issues/206,
https://git.eeqj.de/sneak/webhooker/issues/207)
One caveat on reading a green check: a docs-only commit deliberately One caveat on reading a green check, narrower than it used to be. A
replays from the layer cache docs-only commit deliberately replays from the layer cache (#119), so a
(https://git.eeqj.de/sneak/webhooker/issues/119), so a green status on green status on such a commit evidences a replay rather than an executed
such a commit evidences a replay rather than an executed run. A code run; a code commit invalidates the `COPY` layer and genuinely executes.
commit invalidates the `COPY` layer and genuinely executes. Superseded runs are no longer the hazard they were: before #152 they
were recorded as `skipped` and rolled up green, and before #119 a warm
layer cache let the gate report success without executing anything,
replaying the previous build's console log so the lie looked like a real
run. Both are fixed. Note: `TODO.md` was deliberately
deleted from this repo in f9a9569 (2026-03-01, #6); its content was
folded into the README TODO section, which this draft reconstructs as
of 2026-07-06.
# Next Step # Next Step
Merge the milestone PR (https://git.eeqj.de/sneak/webhooker/pulls/111) Clear the reopened 1.0.0 milestone. The milestone PR
and tag `v1.0.0`. It is `merge-ready` and assigned to sneak; nothing (https://git.eeqj.de/sneak/webhooker/pulls/111) is held: it carries a
else gates it. `WIP: ` prefix, no labels and is assigned to `clawbot`, and it stays
that way until the milestone is empty. Correctness first — the
duplicate-delivery lock and the listen-failure shutdown — then the
operability gaps that make the service usable in production, then the
three credential exposures.
Post-1.0 follow-ups are open, none blocking the tag: Three items belong to the owner, none of them blocking. #150 was decided
https://git.eeqj.de/sneak/webhooker/issues/245, by the manager rather than left to stall the queue and is flagged on the
https://git.eeqj.de/sneak/webhooker/issues/246, issue for reversal if that call was wrong. #112 (whether `Completed
https://git.eeqj.de/sneak/webhooker/issues/247 and Steps` should exist at all, given it once conflicted on every unit) is
https://git.eeqj.de/sneak/webhooker/issues/248. Also still open and unanswered; the provisional ruling in force is that issue branches do
unmilestoned: https://git.eeqj.de/sneak/webhooker/issues/193 (a design not touch this file. #198 records that `make test` is past the org 20s
question, not a defect), https://git.eeqj.de/sneak/webhooker/issues/198 target — 46s of test execution inside a 62.8s CI layer — and turns on
(`make test` is past the org 20s target) and which quantity the 60s hard cap governs; it is scoped as the improvement
https://git.eeqj.de/sneak/webhooker/issues/212 (encrypting target config bug the 20-60s band requires, and should be milestoned instead if the
at rest). cap is read as covering the whole invocation.
After the tag, the largest open cluster is the unmilestoned follow-up
backlog these units generated: #183, #184, #185, #190, #191, #193, #198,
#211 and #212 (encrypting target config at rest, split out of the
credential-leak fix because it needs a key-rotation and re-wrap story).
# Completed Steps # Completed Steps
@@ -292,16 +308,14 @@ at rest).
# Future Steps # Future Steps
- Delivery status and retry management UI. Replay of a terminally - Manual event redelivery from the web UI — the "Replay" capability the
failed delivery and per-attempt detail already landed README describes as planned. No redelivery code exists anywhere in the
(https://git.eeqj.de/sneak/webhooker/issues/203, tree; events are stored in full, which is all it would be built on
https://git.eeqj.de/sneak/webhooker/issues/202) - Delivery status and retry management UI
- Per-webhook rate limiting in the receiver handler (per-webhook config - Per-webhook rate limiting in the receiver handler (per-webhook config
plus handler enforcement; global limits must not apply to receiver plus handler enforcement; global limits must not apply to receiver
endpoints) endpoints)
- Stripe HMAC signature verification. The GitHub and GitLab schemes - Webhook signature verification for GitHub and Stripe HMAC formats
landed with inbound verification
(https://git.eeqj.de/sneak/webhooker/issues/67)
- API key authentication for programmatic access (APIKey model exists; - API key authentication for programmatic access (APIKey model exists;
Bearer token middleware does not) Bearer token middleware does not)
- REST API v1 - REST API v1
@@ -311,10 +325,9 @@ at rest).
- OpenAPI specification - OpenAPI specification
- Analytics dashboard: success rates, response times, volume - Analytics dashboard: success rates, response times, volume
- A remember-me option at login - A remember-me option at login
- Password reset flow for a forgotten password over the web. The - Password reset flow for a forgotten password. The authenticated
authenticated password *change* flow already landed, and a lost password *change* flow already landed on `main` (#65); reset does not
password is recoverable from the console with `webhooker resetpw` exist
(https://git.eeqj.de/sneak/webhooker/issues/208)
- Later, nice to have - Later, nice to have
- email delivery target type - email delivery target type
- SNS and S3 delivery targets - SNS and S3 delivery targets