sneak/webhooker
1
0
Fork 0
Code Issues 67 Pull Requests Actions Packages Projects Releases Wiki Activity

Mask the http target's destination URL in the UI (closes #115) #126

Merged
clawbot merged 1 commits from issue-115-mask-http-destination-url into next 2026-08-17 22:50:27 +02:00
Conversation 1 Commits 1 Files Changed 3 +80 -4
clawbot commented 2026-08-12 11:41:40 +02:00
Collaborator
Copy Link

Speculative, ahead of your decision on
#115. Merging it decides
option 1; closing it decides against. Title keeps the WIP: prefix so
it cannot land by accident.

Rebased onto current next (2ee720a) and re-verified against that
tree; head is 855439c, still a single commit.

Option chosen

Option 1 — mask unconditionally, reusing the existing MaskURL from
internal/delivery/url_mask.go, which renders scheme://host/.... It is
the rule already applied to the slack target, so both target types now
behave identically, and it needs no new masker and no heuristic.

Rejected:

  • Option 2 — mask only credential-shaped URLs (known webhook hosts,
    or high-entropy path segments). A heuristic that can be wrong in both
    directions: it leaks a self-hosted Mattermost or a bespoke endpoint it
    does not recognise, and it hides a plain /hook path an operator
    wanted to read.
  • Option 3 — render in full, on the grounds that the page is
    authenticated. It keeps a written-down credential on screen, so it
    survives into screenshots, screen shares and browser-rendered PDFs
    regardless of who is logged in.

Still non-redundant against current next

next has since gained target config masking
(#113), credential masking in
delivery errors and logs
(#118) and buffered template
rendering (#123). None of them
covers this field: on 2ee720a, httpConfigFields still renders
Value: cfg.URL raw. MaskURL exists unchanged in
internal/delivery/url_mask.go and still elides path, query and
userinfo, returning the neutral placeholder for a URL without scheme and
host.

Correction to the issue's usability mitigation

The issue argues the cost is acceptable because "the full value stays
available in the edit form". That is not true in the code today.
internal/server/routes.go registers POST /source/{sourceID}/targets,
.../targets/{targetID}/toggle and .../targets/{targetID}/delete —
there is no target edit route, and templates/source_edit.html edits
only the webhook's name, description and retention. A target's
destination URL is write-once: after this change it is not recoverable
anywhere in the UI, only from the database. Still true on 2ee720a —
routes.go and templates/ are byte-identical to the tree this was
first written against.

So the real cost of option 1 is higher than the issue states. It still
looks like the right call — an unrecoverable credential is the normal
outcome for a secret, and the operator holds the value at the source
(Slack, Discord, Teams) — but the decision should be made knowing that.
#127 is the target edit form
that repays the usability debt; it is a gap in your existing design
rather than a defect this PR introduces, so it is filed as the work that
makes option 1 cheap again, not as a bug.

What changed

  • internal/delivery/target_config_view.go — httpConfigFields renders
    MaskURL(cfg.URL) for the Destination URL field. No new masker; no
    other field changed.
  • Tests updated for the new expected value, plus two new ones.

Sibling-path audit, re-walked against 2ee720a

Every path where target data reaches an operator, re-checked in the
current tree. No verdict changed.

Path Renders Verdict
GET /source/{id} (templates/source_detail.html) per target .Name, .Type, .Active, then each .Config label/value pair leaked; fixed here
GET /source/{id}/logs (templates/source_logs.html) .Target.Name and .Status only — no delivery error, no config clean
GET /sources (templates/sources_list.html) .TargetCount only clean
GET /source/{id}/edit (templates/source_edit.html) webhook fields only; the template contains no target, config or url reference at all clean
delivery errors persisted to DeliveryResult.Error and logged masked at the raise site by maskURLError (target_http.go:367, target_http.go:507, target_slack.go:128) already fixed by #118
POST /source/{id}/targets SSRF rejection (response body + log line) body is ValidateTargetURL's error, which reports scheme, hostname or IP only; its url.Parse branch goes through maskURLError (ssrf.go:99), and the log line through delivery.MaskURL (source_management.go:1142) clean
/metrics go-http-metrics labels only: route pattern, method, code, service. No URL-valued label. clean

No new path to add. internal/server/routes.go and every file under
templates/ are unchanged between the tree this was first audited
against and 2ee720a. The twelve commits that landed on next in
between touch config parsing, session codec, receiver rate limiting,
lifecycle shutdown, buffered rendering and the sweepers; the only new
log call sites they add are in the archive sweeper and the webhook
receiver, neither of which carries target configuration. TargetView
still has no raw config field, so no template can reach the stored blob.

Mutation evidence, re-run on the rebased head

With MaskURL(cfg.URL) reverted to cfg.URL and nothing else changed,
make test exits 2:

--- FAIL: TestHandleSourceDetail_MasksHTTPDestinationURL (1.21s)
--- FAIL: TestHandleSourceDetail_RendersNamedTargetFields (1.51s)
--- FAIL: TestNewTargetViews_HTTP (0.01s)
--- FAIL: TestNewTargetViews_HTTPFireAndForget (0.01s)
--- FAIL: TestNewTargetViews_HTTPMasksDestinationURL (0.01s)
FAIL	sneak.berlin/go/webhooker/internal/delivery	2.740s
FAIL	sneak.berlin/go/webhooker/internal/handlers	2.038s

The done-criterion test fails on the secret path segments themselves —
source_detail_test.go:164 through :167 report the rendered page
containing /services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX,
T00000000, B00000000 and XXXXXXXXXXXXXXXXXXXXXXXX, and :172
reports it no longer containing https://hooks.slack.com/.... The view
test fails the same way at target_config_view_test.go:213 and
:220-:223. TestHandleSourceDetail_MasksSlackWebhookURL keeps
passing throughout, so the mutation is scoped to the http path. Reverted
after; the tree is identical to the pushed commit.

New tests:

  • TestHandleSourceDetail_MasksHTTPDestinationURL — the done-criterion.
    Seeds an http target whose destination is a secret-bearing incoming
    webhook URL, runs the real handler, asserts the rendered page contains
    none of the path segments and does show https://hooks.slack.com/....
  • TestNewTargetViews_HTTPMasksDestinationURL — same rule at the view
    layer.

Gates, on 855439c

make check exits 0 (test, lint, fmt-check).

Containerized, cache defeated on both check-running stages:
docker build --no-cache-filter=lint --no-cache-filter=builder . exits
0 in 2m48s. Not a cached no-op:

  • lint stage: make fmt-check in 0.9s, then make lint for 47.6s
    reporting 0 issues.
  • builder stage: make test for 51.5s, every package with a real
    duration and zero (cached) markers in the whole log — config 1.124s, database 2.053s, delivery 3.739s, globals 1.016s,
    handlers 2.819s, lifecycle 1.123s, logger 1.023s, middleware 1.102s, server 1.611s, session 1.055s. Both new tests log
    --- PASS in that run.
  • Only 5 layers reported CACHED, all in the final runtime stage.
  • A scoped --no-cache-filter rather than a prune, per the shared-host
    rule. No containers were started; the tagged image was removed after.

Disclosure on host lint: the first host make check failed with 8 lint
issues reported against files under /tmp/rev-130-clawbot-merge/ —
another session's checkout, not present in this one. Re-running with an
isolated GOLANGCI_LINT_CACHE gives 0 issues. and make check exit 0.
That is first-hand reproduction of the shared-cache corruption behind
#106 and
#109, and it is why the
containerized run above is the evidence that counts.

Speculative, ahead of your decision on https://git.eeqj.de/sneak/webhooker/issues/115. Merging it decides option 1; closing it decides against. Title keeps the `WIP: ` prefix so it cannot land by accident. Rebased onto current `next` (`2ee720a`) and re-verified against that tree; head is `855439c`, still a single commit. ## Option chosen **Option 1 — mask unconditionally**, reusing the existing `MaskURL` from `internal/delivery/url_mask.go`, which renders `scheme://host/...`. It is the rule already applied to the slack target, so both target types now behave identically, and it needs no new masker and no heuristic. Rejected: - **Option 2 — mask only credential-shaped URLs** (known webhook hosts, or high-entropy path segments). A heuristic that can be wrong in both directions: it leaks a self-hosted Mattermost or a bespoke endpoint it does not recognise, and it hides a plain `/hook` path an operator wanted to read. - **Option 3 — render in full**, on the grounds that the page is authenticated. It keeps a written-down credential on screen, so it survives into screenshots, screen shares and browser-rendered PDFs regardless of who is logged in. ## Still non-redundant against current `next` `next` has since gained target config masking (https://git.eeqj.de/sneak/webhooker/issues/113), credential masking in delivery errors and logs (https://git.eeqj.de/sneak/webhooker/issues/118) and buffered template rendering (https://git.eeqj.de/sneak/webhooker/issues/123). None of them covers this field: on `2ee720a`, `httpConfigFields` still renders `Value: cfg.URL` raw. `MaskURL` exists unchanged in `internal/delivery/url_mask.go` and still elides path, query and userinfo, returning the neutral placeholder for a URL without scheme and host. ## Correction to the issue's usability mitigation The issue argues the cost is acceptable because "the full value stays available in the edit form". **That is not true in the code today.** `internal/server/routes.go` registers `POST /source/{sourceID}/targets`, `.../targets/{targetID}/toggle` and `.../targets/{targetID}/delete` — there is no target edit route, and `templates/source_edit.html` edits only the webhook's name, description and retention. A target's destination URL is write-once: after this change it is not recoverable anywhere in the UI, only from the database. Still true on `2ee720a` — `routes.go` and `templates/` are byte-identical to the tree this was first written against. So the real cost of option 1 is higher than the issue states. It still looks like the right call — an unrecoverable credential is the normal outcome for a secret, and the operator holds the value at the source (Slack, Discord, Teams) — but the decision should be made knowing that. https://git.eeqj.de/sneak/webhooker/issues/127 is the target edit form that repays the usability debt; it is a gap in your existing design rather than a defect this PR introduces, so it is filed as the work that makes option 1 cheap again, not as a bug. ## What changed - `internal/delivery/target_config_view.go` — `httpConfigFields` renders `MaskURL(cfg.URL)` for the `Destination URL` field. No new masker; no other field changed. - Tests updated for the new expected value, plus two new ones. ## Sibling-path audit, re-walked against `2ee720a` Every path where target data reaches an operator, re-checked in the current tree. No verdict changed. | Path | Renders | Verdict | | --- | --- | --- | | `GET /source/{id}` (`templates/source_detail.html`) | per target `.Name`, `.Type`, `.Active`, then each `.Config` label/value pair | leaked; fixed here | | `GET /source/{id}/logs` (`templates/source_logs.html`) | `.Target.Name` and `.Status` only — no delivery error, no config | clean | | `GET /sources` (`templates/sources_list.html`) | `.TargetCount` only | clean | | `GET /source/{id}/edit` (`templates/source_edit.html`) | webhook fields only; the template contains no `target`, `config` or `url` reference at all | clean | | delivery errors persisted to `DeliveryResult.Error` and logged | masked at the raise site by `maskURLError` (`target_http.go:367`, `target_http.go:507`, `target_slack.go:128`) | already fixed by https://git.eeqj.de/sneak/webhooker/issues/118 | | `POST /source/{id}/targets` SSRF rejection (response body + log line) | body is `ValidateTargetURL`'s error, which reports scheme, hostname or IP only; its `url.Parse` branch goes through `maskURLError` (`ssrf.go:99`), and the log line through `delivery.MaskURL` (`source_management.go:1142`) | clean | | `/metrics` | `go-http-metrics` labels only: route pattern, method, code, service. No URL-valued label. | clean | No new path to add. `internal/server/routes.go` and every file under `templates/` are unchanged between the tree this was first audited against and `2ee720a`. The twelve commits that landed on `next` in between touch config parsing, session codec, receiver rate limiting, lifecycle shutdown, buffered rendering and the sweepers; the only new log call sites they add are in the archive sweeper and the webhook receiver, neither of which carries target configuration. `TargetView` still has no raw config field, so no template can reach the stored blob. ## Mutation evidence, re-run on the rebased head With `MaskURL(cfg.URL)` reverted to `cfg.URL` and nothing else changed, `make test` exits 2: ``` --- FAIL: TestHandleSourceDetail_MasksHTTPDestinationURL (1.21s) --- FAIL: TestHandleSourceDetail_RendersNamedTargetFields (1.51s) --- FAIL: TestNewTargetViews_HTTP (0.01s) --- FAIL: TestNewTargetViews_HTTPFireAndForget (0.01s) --- FAIL: TestNewTargetViews_HTTPMasksDestinationURL (0.01s) FAIL sneak.berlin/go/webhooker/internal/delivery 2.740s FAIL sneak.berlin/go/webhooker/internal/handlers 2.038s ``` The done-criterion test fails on the secret path segments themselves — `source_detail_test.go:164` through `:167` report the rendered page containing `/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX`, `T00000000`, `B00000000` and `XXXXXXXXXXXXXXXXXXXXXXXX`, and `:172` reports it no longer containing `https://hooks.slack.com/...`. The view test fails the same way at `target_config_view_test.go:213` and `:220`-`:223`. `TestHandleSourceDetail_MasksSlackWebhookURL` keeps passing throughout, so the mutation is scoped to the http path. Reverted after; the tree is identical to the pushed commit. New tests: - `TestHandleSourceDetail_MasksHTTPDestinationURL` — the done-criterion. Seeds an `http` target whose destination is a secret-bearing incoming webhook URL, runs the real handler, asserts the rendered page contains none of the path segments and does show `https://hooks.slack.com/...`. - `TestNewTargetViews_HTTPMasksDestinationURL` — same rule at the view layer. ## Gates, on `855439c` `make check` exits 0 (test, lint, fmt-check). Containerized, cache defeated on both check-running stages: `docker build --no-cache-filter=lint --no-cache-filter=builder .` exits 0 in 2m48s. Not a cached no-op: - `lint` stage: `make fmt-check` in 0.9s, then `make lint` for 47.6s reporting `0 issues.` - `builder` stage: `make test` for 51.5s, every package with a real duration and zero `(cached)` markers in the whole log — `config 1.124s`, `database 2.053s`, `delivery 3.739s`, `globals 1.016s`, `handlers 2.819s`, `lifecycle 1.123s`, `logger 1.023s`, `middleware 1.102s`, `server 1.611s`, `session 1.055s`. Both new tests log `--- PASS` in that run. - Only 5 layers reported `CACHED`, all in the final runtime stage. - A scoped `--no-cache-filter` rather than a prune, per the shared-host rule. No containers were started; the tagged image was removed after. Disclosure on host lint: the first host `make check` failed with 8 lint issues reported against files under `/tmp/rev-130-clawbot-merge/` — another session's checkout, not present in this one. Re-running with an isolated `GOLANGCI_LINT_CACHE` gives `0 issues.` and `make check` exit 0. That is first-hand reproduction of the shared-cache corruption behind https://git.eeqj.de/sneak/webhooker/issues/106 and https://git.eeqj.de/sneak/webhooker/issues/109, and it is why the containerized run above is the evidence that counts.
clawbot added the needs-review label 2026-08-12 11:41:40 +02:00
clawbot added 1 commit 2026-08-12 11:41:40 +02:00
Mask the http target's destination URL in the UI (closes #115)
All checks were successful
check / check (push) Successful in 4m17s
Details
5c0ea2b44f 
An http target's destination is frequently a Slack, Discord or
Teams incoming-webhook endpoint whose path segments are the
credential — the same property that made the Slack target's
webhook URL a bearer token. The source detail page rendered it
in full, so the leak closed for slack targets stayed reachable
through a different target type.

Render it through the existing MaskURL, which reduces a URL to
scheme and host. The field accepts an arbitrary URL, so no path
segment can be assumed non-secret and none is shown.
clawbot self-assigned this 2026-08-12 11:41:44 +02:00
clawbot changed title from WIP: Mask the http target's destination URL in the UI (closes #115) to Mask the http target's destination URL in the UI (closes #115) 2026-08-17 22:31:40 +02:00
clawbot added needs-rebase and removed needs-review labels 2026-08-17 22:31:40 +02:00
clawbot force-pushed issue-115-mask-http-destination-url from 5c0ea2b44f to 855439cc56 2026-08-17 22:37:01 +02:00 Compare
clawbot changed title from Mask the http target's destination URL in the UI (closes #115) to WIP: Mask the http target's destination URL in the UI (closes #115) 2026-08-17 22:38:22 +02:00
clawbot changed title from WIP: Mask the http target's destination URL in the UI (closes #115) to Mask the http target's destination URL in the UI (closes #115) 2026-08-17 22:39:16 +02:00
clawbot added needs-review and removed needs-rebase labels 2026-08-17 22:39:16 +02:00
clawbot commented 2026-08-17 22:49:30 +02:00
Author
Collaborator
Copy Link

PASS — satisfies the done-criterion of #115; single commit 855439c on next, clean fast-forward, no attribution trailers, no scope creep.

Gate, in an independent clone at the PR head:

  • docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0. Executed, not replayed: #17 [lint 7/8] RUN make fmt-check 0.3s; #18 make lint 63.1s emitting 0 issues.; #30 make test 62.2s with real per-package durations (delivery 4.460s, handlers 3.901s, …) and zero (cached) markers. The 15 CACHED layers are base images, go mod download and the runtime stage only — none on a check-running layer. Both new tests logged --- PASS in that run. Image removed; no containers started.
  • Gitea check / check (push) green on 855439c.

Probes that were the likely failure sites:

  • Mutation. Reverting MaskURL(cfg.URL) to cfg.URL and changing nothing else makes make test exit 2 with 5 failures. The done-criterion test fails on the absence assertions, not merely the masked-form one: source_detail_test.go:164–:167 (NotContains of the secret path and each of its segments) and target_config_view_test.go:220–:223. Tree restored; git status clean at 855439c.
  • MaskURL coverage. Exercised in a throwaway copy against userinfo, query-only, fragment, path+query+fragment, explicit port, IPv6 literal, no-path, trailing-slash-only, ftp/mailto/javascript/data/unknown scheme, relative URL, embedded space/tab/NUL, and %zz. Every credential-bearing component is dropped; anything without both scheme and host yields (unavailable), never the raw string. No leak found.
  • Non-redundancy. Confirmed on 2ee720a: httpConfigFields rendered Value: cfg.URL raw before this commit.
  • Sibling paths, re-walked independently (not read off the PR table): source_detail.html, source_logs.html, sources_list.html, source_edit.html, the add-target form (no value= prefill), the target create/toggle/delete redirects, every slog site in internal/delivery, internal/handlers, internal/server, DeliveryResult.Error (masked at all raise sites), ValidateTargetURL's rejection body, and the go-http-metrics label set. Clean, with one exception below.

Out of scope, pre-existing, non-blocking — but it narrows the PR body's "no second leak" claim:

  • internal/handlers/source_management.go:1002 reads the destination with r.FormValue("url"), which falls back to the query string when the body has no such field, while internal/middleware/middleware.go:123 logs "url", r.URL.String() unmasked. POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/SECRET therefore creates the target and writes the full credential to the access log (and into Sentry request context when SENTRY_DSN is set). Neither line is touched by this PR and the browser form posts in the body, so this is not a defect introduced here — but it is a live path where the destination URL still escapes unmasked, and it deserves its own issue rather than being covered by the audit table's "clean" verdicts. r.PostFormValue at :1001–:1005 closes it.

Anomalies that pass anyway:

  • MaskURL emits no /... marker for a query-only URL (https://h?token=x renders as https://h), because the elision is keyed on a non-empty path. The query is still dropped, so no leak — cosmetic only.
  • TODO.md:41 will be stale on merge (it lists #115 as an open owner decision). TODO.md is updated in batches on this branch, not per-PR, so this is convention rather than an omission.

Disclosure: per the reviewer's instruction the PR body's opening paragraph (still describing the change as speculative and WIP: -prefixed) was treated as known-stale and excluded from judgement.

PASS — satisfies the done-criterion of https://git.eeqj.de/sneak/webhooker/issues/115; single commit `855439c` on `next`, clean fast-forward, no attribution trailers, no scope creep. Gate, in an independent clone at the PR head: - `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0. Executed, not replayed: `#17 [lint 7/8] RUN make fmt-check` 0.3s; `#18 make lint` 63.1s emitting `0 issues.`; `#30 make test` 62.2s with real per-package durations (`delivery 4.460s`, `handlers 3.901s`, …) and **zero** `(cached)` markers. The 15 `CACHED` layers are base images, `go mod download` and the runtime stage only — none on a check-running layer. Both new tests logged `--- PASS` in that run. Image removed; no containers started. - Gitea `check / check (push)` green on `855439c`. Probes that were the likely failure sites: - **Mutation.** Reverting `MaskURL(cfg.URL)` to `cfg.URL` and changing nothing else makes `make test` exit 2 with 5 failures. The done-criterion test fails on the **absence** assertions, not merely the masked-form one: `source_detail_test.go:164`–`:167` (`NotContains` of the secret path and each of its segments) and `target_config_view_test.go:220`–`:223`. Tree restored; `git status` clean at `855439c`. - **`MaskURL` coverage.** Exercised in a throwaway copy against userinfo, query-only, fragment, path+query+fragment, explicit port, IPv6 literal, no-path, trailing-slash-only, `ftp`/`mailto`/`javascript`/`data`/unknown scheme, relative URL, embedded space/tab/NUL, and `%zz`. Every credential-bearing component is dropped; anything without both scheme and host yields `(unavailable)`, never the raw string. No leak found. - **Non-redundancy.** Confirmed on `2ee720a`: `httpConfigFields` rendered `Value: cfg.URL` raw before this commit. - **Sibling paths, re-walked independently** (not read off the PR table): `source_detail.html`, `source_logs.html`, `sources_list.html`, `source_edit.html`, the add-target form (no `value=` prefill), the target create/toggle/delete redirects, every `slog` site in `internal/delivery`, `internal/handlers`, `internal/server`, `DeliveryResult.Error` (masked at all raise sites), `ValidateTargetURL`'s rejection body, and the `go-http-metrics` label set. Clean, with one exception below. Out of scope, pre-existing, non-blocking — but it narrows the PR body's "no second leak" claim: - `internal/handlers/source_management.go:1002` reads the destination with `r.FormValue("url")`, which falls back to the **query string** when the body has no such field, while `internal/middleware/middleware.go:123` logs `"url", r.URL.String()` unmasked. `POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/SECRET` therefore creates the target and writes the full credential to the access log (and into Sentry request context when `SENTRY_DSN` is set). Neither line is touched by this PR and the browser form posts in the body, so this is not a defect introduced here — but it is a live path where the destination URL still escapes unmasked, and it deserves its own issue rather than being covered by the audit table's "clean" verdicts. `r.PostFormValue` at `:1001`–`:1005` closes it. Anomalies that pass anyway: - `MaskURL` emits no `/...` marker for a query-only URL (`https://h?token=x` renders as `https://h`), because the elision is keyed on a non-empty path. The query is still dropped, so no leak — cosmetic only. - `TODO.md:41` will be stale on merge (it lists https://git.eeqj.de/sneak/webhooker/issues/115 as an open owner decision). `TODO.md` is updated in batches on this branch, not per-PR, so this is convention rather than an omission. Disclosure: per the reviewer's instruction the PR body's opening paragraph (still describing the change as speculative and `WIP: `-prefixed) was treated as known-stale and excluded from judgement.
clawbot merged commit 9ae19159a3 into next 2026-08-17 22:50:27 +02:00
clawbot deleted branch issue-115-mask-http-destination-url 2026-08-17 22:50:27 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#126
Delete Branch "issue-115-mask-http-destination-url"

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?