From 888eaf526bbd8a6e150bdcc48e3d31cb58502e0f Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 30 Aug 2026 04:05:38 +0200 Subject: [PATCH 1/3] State the UUID-is-the-credential rule as a rule (closes #301) (#302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://git.eeqj.de/sneak/webhooker/issues/301. Docs-only apart from one test comment; no behaviour change. The receiver has authenticated on the entrypoint UUID alone since inbound signature verification was removed in https://git.eeqj.de/sneak/webhooker/pulls/279. The README described that as the current state. It did not say it is the decision, which leaves a future contributor free to propose HMAC as an improvement rather than as a reversal. What changed: - `## The entrypoint URL is the authentication secret` now states the rule: the v4 UUID at `/webhook/{uuid}` is the credential and the only one; no shared secret, HMAC signature, bearer token or second factor will be added, including as defence in depth. It names the removal that settled it, and it says explicitly that signature headers a sender sends anyway are stored and forwarded but never checked — the previous text left that ambiguous. - The same section carries the two consequences an operator has to act on: the URL is a capability, so keep it out of logs, tickets and screenshots; and rotation means minting a new entrypoint, not changing a key. - It also handles the case the rule will next be argued from: a sender that only supports signed payloads to a well-known URL is a constraint on that integration, to be raised on its own terms, not grounds to reintroduce shared secrets. - The rule is reachable without scrolling 1,100 lines: a pointer in the intro, a new first bullet under Authentication (which previously listed the web UI, the API and `/metrics` and said nothing about the receiver at all), and a sharpened bullet under Security. Stale language found and corrected: one, in `internal/delivery/redirect_test.go`. Its comment justified same-origin header retention partly by "the inbound signature the receiver verifies" — in this repo's vocabulary "the receiver" is `/webhook/{uuid}`, which verifies nothing. The endpoint that verifies it is the delivery target's, and the comment now says so. Two places that read like stale signing language were checked and left alone as accurate: `internal/delivery/redirect.go` and `internal/server/sentry.go` describe signature headers senders put on the receiver route, which do arrive and are forwarded — neither claims webhooker checks them. `REPO_POLICIES.md` was deliberately not touched. It is the cross-project policy document synced from `sneak/prompts` and carries `last_modified` front matter for that purpose, so a webhooker-specific carve-out does not belong in it. Worth knowing: its hardening section ends "if a standard security hardening measure exists for HTTP services and is not listed here, it is still expected. When in doubt, harden" — that is the sentence a future HMAC proposal will cite, and only the README now answers it. `TODO.md` is untouched per its own Workflow section (issue branches do not touch it). Co-authored-by: sneak Reviewed-on: https://git.eeqj.de/sneak/webhooker/pulls/302 Co-authored-by: clawbot Co-committed-by: clawbot --- README.md | 52 +++++++++++++++++++++++++----- internal/delivery/redirect_test.go | 3 +- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index c66e5ca..319989c 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,13 @@ services, durably stores them, and delivers them to configured targets with retry support, logging, and observability. Category: infrastructure / web service. License: MIT. +Each entrypoint is a version 4 UUID served at `/webhook/{uuid}`, and +that UUID is the entrypoint's only credential. webhooker does not use +shared secrets, HMAC signatures or token headers on the receiver, and +will not add them — read +[The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret) +before deploying one. + ## Getting Started ### Prerequisites @@ -1149,14 +1156,38 @@ backups at rest and restrict who can read them. ## The entrypoint URL is the authentication secret -The receiver verifies nothing about an inbound request. The UUID in an -entrypoint's URL is its credential: anyone who holds that URL can -submit events to it, and the receiver checks nothing else about the -sender. Treat an entrypoint URL the way you would treat an API token. +**The entrypoint UUID is the credential, and it is the only one.** +webhooker mints a version 4 UUID per entrypoint and serves it at +`/webhook/{uuid}`. Possession of that URL is the authentication: +anyone who holds it can submit events to the entrypoint, and the +receiver verifies nothing else about the sender. -There is no way to rotate the UUID in place. To retire one, delete the -entrypoint (or deactivate it, which answers `410`) and create a new -one, then point the sender at the new URL. +There is no shared secret, no HMAC signature, no bearer token and no +second factor on the receiver, and none will be added. This was +considered and rejected; the implementation that existed was removed +in [PR #279](https://git.eeqj.de/sneak/webhooker/pulls/279), closing +[issue #67](https://git.eeqj.de/sneak/webhooker/issues/67) and +[issue #241](https://git.eeqj.de/sneak/webhooker/issues/241). A +proposal to reintroduce any of them — including as "defence in depth" +alongside the UUID — is answered by this section. Inbound signature +headers a sender sends anyway (`X-Hub-Signature` and its +per-provider equivalents) are stored and forwarded as ordinary +headers; nothing checks them. + +What that means for an operator: + +- **The URL is a capability, so treat it as a secret.** Keep it out of + logs, ticket bodies, chat messages and screenshots. Anyone who reads + it anywhere can post events as that sender. +- **Rotating means minting a new entrypoint, not changing a key.** + There is no way to rotate the UUID in place. To retire one, delete + the entrypoint (or deactivate it, which answers `410`) and create a + new one, then point the sender at the new URL. +- **A sender that cannot be given a secret URL is a constraint on that + integration, not a reason to change this.** If a service only + supports signed payloads to a well-known URL, raise it as its own + problem — pick a different integration path, or accept that it + cannot be used. It is not grounds to reintroduce shared secrets. ## Entrypoints @@ -2867,6 +2898,10 @@ check, see [The login endpoint](#the-login-endpoint). ### Authentication +- **Webhook receiver:** the entrypoint UUID in the URL, and nothing + else. No shared secret, no HMAC signature, no token header, and none + will be added — see + [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret). - **Web UI:** Cookie-based sessions using gorilla/sessions with encrypted cookies. Sessions are configured with HttpOnly, SameSite Lax, and Secure whenever the request is on TLS — the flag follows the @@ -2906,7 +2941,8 @@ check, see [The login endpoint](#the-login-endpoint). mode - **The entrypoint URL is the receiver's only credential.** Nothing about an inbound request is verified; possession of the UUID - authorises submission (see + authorises submission, and no shared secret or signature check will + be added alongside it (see [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret)) - **SSRF prevention** for HTTP delivery targets: private/reserved IP ranges (RFC 1918, loopback, link-local, cloud metadata) are blocked diff --git a/internal/delivery/redirect_test.go b/internal/delivery/redirect_test.go index 31e3d66..1534697 100644 --- a/internal/delivery/redirect_test.go +++ b/internal/delivery/redirect_test.go @@ -170,7 +170,8 @@ func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders( // Stripping must not fire within the configured origin, or every // destination that redirects its own path would lose its // credential and start answering 401 — and would lose the inbound -// signature the receiver verifies. +// signature header the target endpoint verifies. webhooker's own +// receiver verifies no signature; it only forwards the header. func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders( t *testing.T, ) { -- 2.54.0 From 39afa69bfc3439b0a1d911eccfc83131f7ecadec Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 10:01:51 +0200 Subject: [PATCH 2/3] Consolidate the data directory mode into one owner (closes #288) Two packages each declared the 0o750 mode for DATA_DIR and both created the directory. internal/datadir now exports DirPerm as the single definition, and internal/database uses it in both places it creates the directory. The value is unchanged, so existing deployments see no permission change. datadir owns it because guarding and creating DATA_DIR is that package's whole purpose and it imports nothing that would form a cycle. Model: opus-4-8 (implementation and review); fable-5-1 (merge) --- internal/database/database.go | 6 ++++-- internal/database/webhook_db_manager.go | 6 ++++-- internal/datadir/lock.go | 10 ++++++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/database/database.go b/internal/database/database.go index f2880e8..ba28bae 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -17,12 +17,12 @@ import ( "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/banner" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" "sneak.berlin/go/webhooker/internal/logger" ) const ( - dataDirPerm = 0750 randomPasswordLen = 16 sessionKeyLen = 32 ) @@ -185,7 +185,9 @@ func (d *Database) connect() error { // caller's decision. func (d *Database) connectTo(dataDir string) error { // Ensure the data directory exists before opening the database. - err := os.MkdirAll(dataDir, dataDirPerm) + // datadir.DirPerm is the single source of the directory mode; this + // package creates the directory too, since either may run first. + err := os.MkdirAll(dataDir, datadir.DirPerm) if err != nil { return fmt.Errorf( "creating data directory %s: %w", diff --git a/internal/database/webhook_db_manager.go b/internal/database/webhook_db_manager.go index 81ca427..7da8169 100644 --- a/internal/database/webhook_db_manager.go +++ b/internal/database/webhook_db_manager.go @@ -13,6 +13,7 @@ import ( "gorm.io/driver/sqlite" "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" "sneak.berlin/go/webhooker/internal/logger" ) @@ -53,8 +54,9 @@ func NewWebhookDBManager( log: params.Logger.Get(), } - // Create data directory if it doesn't exist - err := os.MkdirAll(m.dataDir, dataDirPerm) + // Create data directory if it doesn't exist. datadir.DirPerm is the + // single source of the directory mode; either package may run first. + err := os.MkdirAll(m.dataDir, datadir.DirPerm) if err != nil { return nil, fmt.Errorf( "creating data directory %s: %w", diff --git a/internal/datadir/lock.go b/internal/datadir/lock.go index a50929c..bb605cd 100644 --- a/internal/datadir/lock.go +++ b/internal/datadir/lock.go @@ -29,9 +29,11 @@ import ( // process that was killed with SIGKILL blocks nothing. const LockFileName = "webhooker.lock" -// dirPerm is the mode Acquire creates DATA_DIR with. It matches what -// internal/database uses, since whichever runs first creates it. -const dirPerm = 0o750 +// DirPerm is the mode DATA_DIR is created with. It is the single +// source of that mode: internal/database consumes it rather than +// keeping its own copy, so the two packages that both create the +// directory cannot drift into disagreeing about its permissions. +const DirPerm = 0o750 // ErrLocked reports that another live process holds the data // directory. Callers that need to know whether a deployment is running @@ -64,7 +66,7 @@ func Acquire(dir string) (*Lock, error) { return nil, ErrNoDir } - err := os.MkdirAll(dir, dirPerm) + err := os.MkdirAll(dir, DirPerm) if err != nil { return nil, fmt.Errorf( "creating data directory %s: %w", dir, err, -- 2.54.0 From b05182137050351e5359746ebcedca9e5b19f467 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 18:33:09 +0200 Subject: [PATCH 3/3] Say max_retries is the total attempt count, not a retry count (closes #316) The help text under the field on both target forms and the max_retries rows in the README now say the number is the total number of delivery attempts: 0 is a single attempt with no retries and no circuit breaker, and N is N attempts in total. The delivery code already worked this way; only the wording was wrong, so an operator wanting one try plus two retries would have entered 2 instead of 3. A UI copy test renders both forms and pins the wording. Delivery behaviour is unchanged. Model: opus-4-8 (implementation); fable-5-1 (merge) --- README.md | 14 +++--- internal/handlers/ui_copy_test.go | 77 +++++++++++++++++++++++++++++++ templates/source_detail.html | 9 ++-- templates/target_edit.html | 2 +- 4 files changed, 91 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 319989c..e1289af 100644 --- a/README.md +++ b/README.md @@ -1507,7 +1507,7 @@ events should be forwarded. | `type` | TargetType | One of: `http`, `slack`, `database`, `log` | | `active` | boolean | Whether deliveries are enabled (default: true) | | `config` | JSON text | Type-specific configuration | -| `max_retries` | integer | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets | +| `max_retries` | integer | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets | | `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels | **Relations:** Belongs to Webhook. Has many Deliveries. @@ -1515,12 +1515,12 @@ events should be forwarded. **Target types:** - **`http`** — Forward the event as an HTTP POST to a configured URL. - Behavior depends on `max_retries`: when `max_retries` is 0 (the - default), the target operates in fire-and-forget mode — a single - attempt with no retries and no circuit breaker. When `max_retries` is - greater than 0, failed deliveries are retried with exponential backoff - up to `max_retries` attempts, protected by a per-target circuit - breaker. + `max_retries` is the total number of delivery attempts, not retries on + top of the first: when `max_retries` is 0 (the default), the target + operates in fire-and-forget mode, a single attempt with no retries and + no circuit breaker; a value of N makes up to N attempts in all, + retrying failed deliveries with exponential backoff and protecting them + with a per-target circuit breaker. - **`slack`** — Post the event as a formatted message to a Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It is built on the same HTTP core as `http` and honours `max_retries` diff --git a/internal/handlers/ui_copy_test.go b/internal/handlers/ui_copy_test.go index da9b6d6..8598de9 100644 --- a/internal/handlers/ui_copy_test.go +++ b/internal/handlers/ui_copy_test.go @@ -300,3 +300,80 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) { "the page must render to completion, not abort partway", ) } + +// maxRetriesHelp is the wording both target forms must carry. The +// delivery core makes max_retries attempts in total, not that many +// retries on top of a first try (a fresh delivery starts at attempt 1 +// and target_http gives up once the attempt number reaches +// max_retries), and 0 is special-cased to a single fire-and-forget +// attempt with no circuit breaker. +const maxRetriesHelp = "This is the total number of delivery attempts, " + + "not retries on top of the first: a value of 3 makes three attempts " + + "in all. 0 means a single attempt with no retries and no circuit " + + "breaker." + +// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries +// help text on both the create form (the add-target form on the webhook +// detail page) and the edit form, so the copy cannot drift back to +// calling the number a retry count. +func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + + entrypoint := database.Entrypoint{Path: "abc123"} + entrypoint.ID = "ep-1" + + createBody := renderPage( + t, h, sess, "source_detail.html", map[string]any{ + dataKeyWebhook: webhook, + "Entrypoints": handlers.NewEntrypointViews( + []database.Entrypoint{entrypoint}, + ), + "Targets": delivery.NewTargetViews(nil), + "Events": []database.Event{}, + "BaseURL": "https://hooks.example.com", + }, + ) + + assert.Contains( + t, createBody, maxRetriesHelp, + "the add-target form must explain max_retries as total attempts", + ) + + // A slack target exercises the same max_retries field while needing + // only Config.URL from the edit template, so the test data stays + // minimal. The Target key mirrors the field names the template reads + // off the handler's view value. + editBody := renderPage( + t, h, sess, "target_edit.html", map[string]any{ + dataKeyWebhook: webhook, + "Target": map[string]any{ + "ID": "tg-1", + "Name": "t", + "Type": "slack", + "Active": true, + "MaxRetries": 3, + "Config": map[string]any{ + "URL": "https://hooks.slack.com/services/x", + }, + }, + dataKeyError: "", + }, + ) + + assert.Contains( + t, editBody, maxRetriesHelp, + "the target edit form must explain max_retries as total attempts", + ) +} diff --git a/templates/source_detail.html b/templates/source_detail.html index bde8cae..3b26967 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -120,9 +120,12 @@ -
- - +
+
+ + +
+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

diff --git a/templates/target_edit.html b/templates/target_edit.html index 9194721..a2ff56c 100644 --- a/templates/target_edit.html +++ b/templates/target_edit.html @@ -69,7 +69,7 @@
-

0 is fire-and-forget: one attempt, no circuit breaker.

+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

{{end}} -- 2.54.0