Milestone 1.0.0: internet-facing readiness #111
Reference in New Issue
Block a user
Delete Branch "next"
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?
Taggable. Milestone 1.0.0 (https://git.eeqj.de/sneak/webhooker/milestone/9) is 0 open.
next@d61d9dc, 83 commits ahead ofmain, strict fast-forward (merge-base==mainhead).Verification at
d61d9dcCache-defeated container build per #119, from a fresh clone:
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0.Every content-bearing stage executed, confirmed per vertex rather than inferred:
[lint 7/9] make fmt-check[lint 8/9] golangci-lint config verify[lint 9/9] golangci-lint run0 issues.[builder 8/11] script/fetch-assets[builder 9/11] make test[builder 10/11] make build[builder 11/11]static relinkFive vertices were CACHED, not zero:
#8the digest-pinnedgolangci/golangci-lint:v2.12.2FROMresolve,#7the digest-pinnedgolang:1.26.1-bookwormFROMresolve, and#28/#29/#30the Alpinestage-2runtime layers (apk add ca-certificates,adduser,WORKDIR). None carries a dependency on repository content, so none can mask a stale result.One limit on that log: buildkit clipped the
make testvertex at its 2 MiB output cap, so the per-package tally is not readable from it.script/testisgo test -v -race -timeout 90s ./...underset -eu, so a failure would have failed the build — but the tally below comes from a separate run, not from that log.Separate uncapped
GOFLAGS=-count=1 make checkon the same clone at the same commit: exit 0, 23/23 packages (21ok+ 2 no-test), 0FAIL, 0(cached), 0 data races.Binary from the built image:
sha256:1f81b6749a7d5ba102d6e5ecfd2a7b9f9025ef9ad72578ea453a20f01fca7339, 32217760 bytes.A fresh clone requires
make bootstrapbeforemake check— #282.No commit ahead of
maincarries an attribution trailer.Landed since the last revision of this body
*Timestamp:* 0001-01-01T00:00:00Z, because no delivery task carriedcreated_atand nothing read it back. Verified live against a running instance at this head, not from tests: event POSTed at15:44:00.27-15:44:00.29Z, sink received*Timestamp:* `2026-08-24T15:44:00Z`.CreatedAtunset; it now renders`unknown`rather than a year-1 date, so an expected reap cannot be misread as the defect above returning.0600.SENTRY_DSNor a malformed.envaborts startup instead of silently defaulting._txlock=immediate, nocache=shared, a bounded pool, and delivery re-dispatch gated by reference-counted ownership. An operator can now runsqlite3 .dumpagainst a live database; that previously rejected 41 of 60 inbound webhooks and re-sent already-delivered events on restart. Verified across 1200 events / 7200 deliveries: 7200 POSTs, zero duplicates, zero new POSTs after restart.Your scope decisions, recorded
/metricsand Sentry are acceptable. No masking work done.For you — none of it blocking the merge
nextis not linear.9313b0fis a merge commit; the fast-forward is unaffected, but do not expect a clean rebase if you want linear history.dlopen/getaddrinfowarning — your deliberate Debian-builder/Alpine-runtime design. Delivery by DNS name worked end to end in both audits.script/dockerdiverges from the byte-identical model script to pass a version build arg, and no green CI run exercises a real stamp. Durable fix is upstream insneak/prompts.make fmtdoes not format markdown here; markdown is hand-wrapped.cp -aprocedure, but movingarchive-*.dbalone silently yields fewer rows.CSRF ran before MaxBodySize, so the CSRF middleware parsed the form body before any cap applied and an oversized request was read in full before being rejected. MaxBodySize is now the first middleware in all four route groups that parse forms, ahead of CSRF and RequireAuth. An oversize request therefore gets 413 without the handler running and without state changing, including the password-change route. Note the ordering trade: an unauthenticated client now receives 413 rather than an auth redirect on /user/{username}/password.Integration-level review of
origin/main..origin/next@543005c. FAIL —needs-rework.Gate is genuinely green:
docker build --no-cache-filter=lint,builderexit 0 with#15 RUN make fmt-checkDONE 1.1s,#16 RUN make lint->0 issues.DONE 59.3s,#23 RUN make testDONE 66.1s (5.8k lines, zero(cached)markers, 9okpackage lines); hostmake checkexit 0.mainis an ancestor ofnext— fast-forwardable, no conflict. The Gitea status on543005csays "Successful in 5s", i.e. a cache replay per #119 — the runs above are the evidence, not the badge. No Claude/Anthropic strings or attribution trailers in the tree or in any of the 15 commit messages; inclusive terminology clean.Findings, ranked.
1. The receiver rate limit does not bound abuse of the receiver —
internal/middleware/ratelimit.go:258+internal/server/routes.go:174.The limiter keys on (client IP,
httprate.KeyByEndpoint), andKeyByEndpointis literallyr.URL.Path. The chi pattern/webhook/{uuid}matches any single path segment, so a client mints a fresh 120/min bucket for every distinct string it invents —/webhook/a,/webhook/b, ... — and the aggregate rate against the route is unlimited. Every one of those requests is admitted intoHandleWebhook, which logs an INFO line carrying the attacker-supplied value (internal/handlers/webhook.go:42) and performs a DB lookup (internal/handlers/webhook.go:48) before returning 404. One IP therefore drives unbounded DB queries and unbounded log volume against the single unauthenticated, internet-exposed endpoint. That is precisely the propertyinternal/config/config.go:38-41claims ("bounding abuse of the one unauthenticated, internet-exposed endpoint") and that #64 was opened to obtain. Neither unit review could see this: #64 landed the per-endpoint key against anextwithout the trusted-proxy work, and #88 rewrote only the IP half of the same key. Acceptable: a second, coarser limiter over the whole/webhook/*group keyed on client IP alone at a higher ceiling, or resolve the entrypoint first and bucket on the resolved entrypoint ID rather than the raw path — so an unresolvable path cannot buy a bucket.2.
RETENTION_SWEEP_INTERVALis not range-validated and now panics two loops —internal/config/config.go:219,349.envDurationaccepts any parseable duration, including0sand-1h. That value reachestime.NewTickerininternal/database/retention.go:122and — new in this diff —internal/delivery/archive_sweeper.go:130.time.NewTickerpanics on a non-positive duration, in goroutines with norecover, soRETENTION_SWEEP_INTERVAL=0logs "Configuration loaded" and then aborts the process: a delayed crash, not a loud startup failure.PORTandRECEIVER_RATE_LIMITboth received lower-bound checks under #80; this variable did not, and #89 doubled its blast radius. The missing check predates this branch, but the second consumer does not. Acceptable: reject non-positiveRETENTION_SWEEP_INTERVALinloadFromEnv, asenvPositiveIntdoes.3.
RETENTION_SWEEP_INTERVALis missing from the README environment table —README.md:86-98.The table gained
MAINTENANCE_MODE,SESSION_IDLE_TIMEOUT,RECEIVER_RATE_LIMITandTRUSTED_PROXIES, but two later sections useRETENTION_SWEEP_INTERVALas if it were documented there (README.md:174cites it as a fail-loud example;README.md:642makes it the archive sweeper's interval). An operator reading the table cannot discover the variable that controls both sweepers.4.
TODO.mddoes not record five of the fifteen landed units.The landing commit is titled "Update TODO.md for the completed 1.0.0 milestone" and its Status paragraph asserts
next"holds the completed 1.0.0 milestone", but Completed Steps has no entry for #64, #79, #90, #113 or #118 — including both credential-exposure fixes.REPO_POLICIES.mdrequires the TODO be updated meticulously.5.
TODO.md:118dropped "Manual event redelivery from the web UI" from Future Steps, but it is not implemented.No redelivery handler, route or template exists anywhere in
internal/ortemplates/. The only surviving trace is the planned API endpoint (TODO.md:128,README.md:919). MeanwhileREADME.md:263still advertises it in the present tense as a core capability: "Replay — Stored events can be manually redelivered for debugging or testing, without requiring the original sender to fire the webhook again." Tagging 1.0.0 would ship a README promising a feature the binary lacks, with the tracking item deleted in the same branch. Acceptable: restore the Future Steps entry (or file an issue) and softenREADME.md:263to planned.6. Minor —
static/js/app.js:2:console.log("Webhooker loaded");ships in the production asset. Pre-existing, but it is debug scaffolding in a 1.0.0 artifact.7. Minor —
templates/sources_list.html:3: the #57 copy change to{{define "title"}}Webhooks - Webhooker{{end}}is inert, because page title blocks never render (#117). Harmless, but the commit does not achieve what it claims for that file.8. PR state. Still a draft with
WIP:in the title, and the body's "Still open in the milestone" list names #57, #88 and #118, all of which landed; the milestone is now 0 open. Body and draft state need correcting before this can merge.Probes worth recording, all of which passed for the right reason:
securecookie's max-age onnextis still gorilla's 30-day default whilestore.Options.MaxAgeis 7 days (#108), butsession.expirednow rejects anything pastcreated_at + 7dserver-side and treats a session carrying no timestamps as expired, so the skew is not reachable. #132 remains worth landing on its own merits.forwardedClientAddrhand-checked for hop splitting, empty hops, trailing commas, multi-value headers and the 64-hop cutoff falling back to the peer address: correct in each case.sweepWriterFor/releaseSweepWriter/ theevictedflag are all evaluated under the registry lock, and the sweep never creates an archive file.maxBodyShiftremains live viainternal/handlers/webhook.go:17, so removing the handler-levelMaxBytesReadercalls did not disturb the receiver's own 1 MB cap.Disclosure: 14 of 15 commits end with
(closes #N);Update TODO.md for the completed 1.0.0 milestonecloses no issue, which I judged acceptable for a housekeeping commit rather than a finding. The buffered-rendering interaction could not be assessed on this branch —renderTemplatestill writes straight to theResponseWriter(internal/handlers/handlers.go:233); #131 has not landed here.The receiver limiter keyed on the request path, and /webhook/{uuid} matches any single segment, so a client minted a fresh bucket per invented path and had unlimited aggregate rate against the only unauthenticated endpoint. An outer limiter keyed on the client address alone now bounds that, chained in front of the unchanged per-entrypoint limiter. Its rejections log at DEBUG without the path, and the README states what each limit does and does not bound.Independent integration review of
origin/main..origin/next@339548d(20 commits, 65 files). FAIL —needs-rework.Gate, run by me on
339548din a fresh clone:docker build --no-cache-filter=lint,builderexit 0 in 210s —#15 RUN make fmt-checkDONE 0.8s,#16 RUN make lint->0 issues.DONE 59.0s,#23 RUN make testDONE 67.1s (9okpackage lines, zero(cached)markers),#24 RUN make buildDONE 51.0s; hostmake checkexit 0.mainis an ancestor ofnext, fast-forwardable, no conflict. No Claude/Anthropic strings or attribution trailers in the tree or in any of the 20 commit messages; inclusive terminology clean; no non-testTODO/FIXME, commented-out blocks or debug code reachable from the production binary; external references hash-pinned. The five findings of #111 (comment) are all confirmed fixed and are not re-reported. None of the findings below duplicate the deferred set.1. In the default configuration any unauthenticated client on the internet can lock the admin out of the web UI indefinitely —
internal/middleware/ratelimit.go:151-172+:219-226, undocumented atREADME.md:1120-1122.LoginRateLimitis 5 POSTs/minute per bucket, and since #88 the bucket key isclientKey: the connection's own peer address unless that peer is inTRUSTED_PROXIES, which defaults to empty.README.md:79-80and the prod TLS story require a TLS-terminating reverse proxy, so in the default deployment every login POST from every client keys on the proxy's address and shares one 5/minute bucket. An attacker sending 5 login POSTs per minute — about 0.08 req/s, from anywhere — keeps that bucket permanently full, and the operator's own login POST gets 429 forever. There is no second administrative path.PasswordChangeRateLimitis the same 5/min shared bucket.This is not the trusted-proxy default being wrong: empty is correct. What is wrong is that the consequence is nowhere stated and one place states its opposite.
README.md:1120-1122still advertises the login limiter as "per-IP sliding-window rate limiter on the login endpoint (5 POST attempts per minute per IP)", which is false in the default deployment.README.md:891-895does disclose the shared bucket but characterises it as "the safe direction to be wrong in" — true for the receiver, exactly inverted for login, where a shared bucket converts a per-attacker throttle into a global lockout. The#### Trusted proxiessection atREADME.md:101-146, the one an operator actually reads while configuring, presents the empty default as having no downside at all.Acceptable: state in
README.md:101-146that a production deployment behind a reverse proxy MUST setTRUSTED_PROXIES, and that leaving it unset makes the login and password-change limits a single global bucket that any remote client can hold full; and correctREADME.md:1120-1122so it does not claim per-IP. A startup WARN whenTRUSTED_PROXIESis empty andWEBHOOKER_ENVIRONMENT=prodwould be better still.2. The Docker section describes a build that does not exist, and contradicts the section 15 lines below it —
README.md:1131-1135.It says the Dockerfile is a two-stage build whose "Builder stage (Debian-based
golang:1.24) — installs golangci-lint, downloads dependencies, copies source, runsmake check". The actualDockerfileis three stages:lintongolangci/golangci-lint:v2.12.2runningmake fmt-checkandmake lint(Dockerfile:5,26-27),builderongolang:1.26.1-bookwormrunningmake testandmake build(Dockerfile:32,49-50), then the runtime stage. Wrong stage count, wrong Go version, wrong image, wrong commands — andREADME.md:1147-1159, added by #119 in this branch, correctly describes "both check stages" and the four separate targets, so the two paragraphs contradict each other. The described single-builder shape is also the oneREPO_POLICIES.md:102-106forbids. Acceptable: rewriteREADME.md:1131-1135to the three stages actually in the tree.3. The API table is false about the only public endpoint —
README.md:917, andREADME.md:514.README.md:917listsANY | /webhook/{uuid} | Webhook receiver endpoint (accepts all methods).internal/handlers/webhook.go:24-33answers 405 withAllow: POSTto everything except POST, and has since #20.README.md:514compounds it, documentingEvent.methodas "HTTP method (POST, PUT, etc.)" when the column can only ever holdPOST. Pre-existing onmain, but this is the first line an integrator reads about the endpoint the release exists to serve, and the documentation-accuracy pass in this branch (0e397b3) did not catch it. Acceptable:POSTand "accepts POST only; other methods get 405", and fix themethodfield description.4. The gate status on the PR head is a cache replay and is not evidence —
339548d.check / check (push)on339548dreads success, description "Successful in 6s".339548dis docs-only so the fingerprint mechanism replays legitimately — but the two commits under it,0e397b3and95161c7(the latter is the receiver aggregate rate-limit fix this PR was failed for last round), both carry statusskipped, "Superseded by a newer commit; never tested", and Gitea's combined-status API returns"state":"success"for each of them. The newest status onnextthat describes an executed run isbe57609, "Successful in 2m52s", whose tree predates both of those code commits. So no commit status on this branch evidences an executed check of the current tree, andTODO.md:28-31's claim thatnext"is verified green both by CI and by cache-defeated container runs" is unsupported on the CI half. The tree itself is fine — my cache-defeated build above is the actual evidence — so this is an evidence defect, not a code defect, but it means the repaired gate still turns "never tested" into a green rollup whenever a run is superseded. Distinct from #147, which coversscript/cibuilddrift and the hardcoded context string. Acceptable: push a no-op commit or re-run so the head carries a real run before tagging, and soften or substantiate theTODO.mdclaim. Note for the merge: ifnextis fast-forwarded rather than squashed,maininherits commit339548dunchanged, somain's check will also be a seconds-long replay — expected, not a regression.5. The session-expiry documentation is orphaned inside the trusted-proxy section —
README.md:101vsREADME.md:148-166.#66 put the two-clock session prose directly under
### Configuration. #88 then inserted#### Trusted proxiesabove it without a closing heading, soSESSION_IDLE_TIMEOUT, the 7-day absolute cap and the 10% refresh lag now sit inside a subsection aboutX-Forwarded-Forand are unreachable from the table of contents. Visible only once both units are combined. Acceptable: a#### Sessionsheading atREADME.md:148.6. Minor —
internal/config/config.go:455-467: the startup summary omitssessionIdleTimeout. Every other value parsed inloadFromEnvis logged, including the three added in this milestone.SESSION_IDLE_TIMEOUTis the one variable where a valid setting silently turns off a security control (config.go:99, non-positive disables idle expiry), so it is the one most worth showing back to the operator.7. Minor —
TODO.md:193lists "Password change and reset flow" under Future Steps whileTODO.md:23-24records the admin password change flow (#65) as already landed onmain.Probes that passed, recorded because they were the likely failure sites: the two chained receiver limiters compose correctly (aggregate outer, per-entrypoint inner, saturating multiply at
ratelimit.go:333-339, no unbounded key growth since the outer limiter gates the inner one's key space);forwardedClientAddrhand-checked for hop splitting, empty hops, multi-value headers and the 64-hop cutoff falling back to the peer; the codec max-age / idle-timeout skew stays unreachable becausesession.expiredtreats a session with no timestamps as expired; the three background loops each root atcontext.Background()and eachOnStopcancels then waits, with no shared state between the reaper's per-webhook DBs and the sweeper's archive files;renderTemplatestill streams to theResponseWriter(internal/handlers/handlers.go:233), so the buffered-rendering interaction does not exist on this branch; the.ci-fingerprintbarrier is correctly excluded from.dockerignoreand does not disturb the policy-mandated lint stage; no raw config blob reaches any template.Disclosures: two of the 20 commit titles carry no
(closes #N)—543005cand339548d, bothTODO.mdhousekeeping — which I judged acceptable, consistent with the prior review. I could not independently determine whether run 167 or 168 completed their docker build before being cancelled, so finding 4 is stated as unsupported evidence rather than as a proven untested tree.internal/delivery/target_config_view.go:115-118renders the HTTP target URL unmasked; that is #115 and is your call, not a finding here.Milestone triage, recorded once. The 1.0.0 milestone stood at 22 closed / 1 open when I picked this up. It now has seven open, because six issues had been filed with an explicit "not milestoned 1.0.0" note that no longer held, and one is new. Every move is argued on its own issue; the summary:
Moved in, reversing the filer's original call
successfor a commit nothing ever ran is a false green that branch protection or a release script will read as fact.REPO_POLICIES.mdrules the release claims to follow. "Pre-existing" describes when it started, not whether it belongs in the tag. Since merged.docker stop./64bypasses completely, with no spoofing and nothing to detect.Opened during the work, and milestoned
Deliberately left out, though tempting: #107 (the strongest pure correctness defect in the backlog, but not unauthenticated-reachable), #117, #128, #127, #166, #168, #169, and the test-hygiene and tooling set (#93, #94, #99, #101, #103, #120, #154). The test applied throughout: can an unauthenticated attacker on the public internet exploit it, or does it make the shipped artifact wrong, untrustworthy or unverifiable?
Two decisions I took rather than parking, both reversible by closing the PR: #115 (mask unconditionally) and #125 (option 1,
/64). Only #150 is genuinely sneak's, and it is assigned to him with a corrected recommendation — the option originally recommended there does not fix the reported attack, since flooding the operator's own predictable username still locks them out.r.FormValue falls back to the query string, so POST /source/{id}/targets?url=<secret> created a working target from a value carried on the request line — where proxy logs, browser history and Referer all record it. Every form read is now r.PostFormValue, including the login password and both password-change fields, which had the same defect in a more acute form. The Sentry leg needed more than the query string. sentryhttp attaches the whole request to the scope, and ApplyToEvent copies the teed body into Request.Data with no SendDefaultPII guard — so reading every field from the body only pointed every credential this change protects at the one field the first revision did not scrub. Body and query are now redacted, Cookies and Env cleared, and Headers reduced to an allowlist, because the SDK's own filter removes four names and would otherwise ship X-Csrf-Token and the shared secrets senders put on the receiver route. Also adds json:"-" to Target.Config, APIKey.Key and Setting.Value — TargetView is the masking barrier for the HTML path only, and the first handler to marshal a model would serialise a bearer token or the session encryption key. Independently reviewed three times. The second review found the Data leak and proved it with a scratch module; the third disproved the PR's own claim that BeforeSend gets no request, so the README now records that redacting unconditionally is a deliberate choice rather than a limitation — which is what makes #179 cheap to fix.WIP: Milestone: delivery lifecycle, retention, config and session hardeningto Milestone 1.0.0: internet-facing readinessMilestone 1.0.0: internet-facing readinessto WIP: Milestone 1.0.0: internet-facing readinessHeld as
WIP:and unassigned — not merge-ready. sneak, 2026-08-20: "webhooker must be to mvp before tagging 1.0. it is prerelease now and must be usable in low volume prod by me before a 1.0".The 1.0.0 gate is no longer "milestone empty" but "deployable and usable in low-volume production". A code-level deployability audit is running now: first-run admin bootstrap, the blast radius of having no edit forms for targets and entrypoints (#127), inbound authentication on the receive endpoint (#67), retention end to end, and a live run pushing a webhook through to a real sink.
Its blockers get milestoned to 1.0.0. This PR reopens for merge when they are closed.
The receiver had no inbound authentication of any kind: /webhook/{uuid} was mounted behind a rate limiter alone, so the only thing protecting an entrypoint was the secrecy of a v4 UUID in a URL path. Inbound headers are forwarded almost verbatim to the target, so anyone who learned the URL also chose the headers the downstream service received. Adds an optional per-entrypoint secret with two schemes: github (X-Hub-Signature-256, HMAC-SHA256 hex over the raw body) and gitlab (X-Gitlab-Token, a plain shared token). Comparison is constant-time, the HMAC is computed over the raw body before any parsing, and rejection happens before persistence -- an unauthenticated request creates no event row. An entrypoint with no secret behaves exactly as before, including every row that predates this change. The scheme's credential header is stripped from the header map before it is marshalled into Event.Headers, so the GitLab token reaches neither the event store nor any delivery target. SchemeInfo.HeaderIsDigest defaults to false meaning strip, so a scheme added later is protected unless its header is positively declared a digest.There was no redelivery path anywhere: once a delivery exhausted max_retries it was failed permanently, even though the event body is durably stored. Storing an event and being unable to re-send it defeats the reason it is stored, and the ordinary case is a destination that was down longer than the backoff ladder. Adds POST /source/{sourceID}/deliveries/{deliveryID}/replay, inside the authenticated group so it inherits MaxBodySize, CSRF, NoCache and RequireAuth. Replay creates a NEW pending delivery against the target's CURRENT config and hands it to the engine through the same notifier the receiver uses, so it runs the normal path with the retry ladder, the SSRF-guarded transport and the circuit breaker. The original delivery's rows are never touched, and the stored event body is re-sent, never the recorded response. Replay is refused, with a distinct message, for a non-terminal delivery, a deleted target, a deactivated target, and when an earlier replay of the same event and target is still in flight. Bounded by a per-client rate limit and by that in-flight check. The new delivery row is written with Omit(clause.Associations) and with neither Event nor Target populated, so it cannot upsert a targets row into the per-webhook event database (#206). Counted by webhooker_delivery_replays_total on the existing target_type label. A replay also moves the ordinary attempt, outcome and duration series, because it is a real delivery.WIP: Milestone 1.0.0: internet-facing readinessto Milestone 1.0.0: internet-facing readinessclawbot referenced this pull request2026-08-24 03:34:45 +02:00