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)
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)
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:
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.
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
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 next2026-08-17 22:50:27 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Speculative, ahead of your decision on
#115. Merging it decides
option 1; closing it decides against. Title keeps the
WIP:prefix soit cannot land by accident.
Rebased onto current
next(2ee720a) and re-verified against thattree; head is
855439c, still a single commit.Option chosen
Option 1 — mask unconditionally, reusing the existing
MaskURLfrominternal/delivery/url_mask.go, which rendersscheme://host/.... It isthe rule already applied to the slack target, so both target types now
behave identically, and it needs no new masker and no heuristic.
Rejected:
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
/hookpath an operatorwanted to read.
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
nextnexthas 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,httpConfigFieldsstill rendersValue: cfg.URLraw.MaskURLexists unchanged ininternal/delivery/url_mask.goand still elides path, query anduserinfo, 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.goregistersPOST /source/{sourceID}/targets,.../targets/{targetID}/toggleand.../targets/{targetID}/delete—there is no target edit route, and
templates/source_edit.htmleditsonly 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.goandtemplates/are byte-identical to the tree this wasfirst 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—httpConfigFieldsrendersMaskURL(cfg.URL)for theDestination URLfield. No new masker; noother field changed.
Sibling-path audit, re-walked against
2ee720aEvery path where target data reaches an operator, re-checked in the
current tree. No verdict changed.
GET /source/{id}(templates/source_detail.html).Name,.Type,.Active, then each.Configlabel/value pairGET /source/{id}/logs(templates/source_logs.html).Target.Nameand.Statusonly — no delivery error, no configGET /sources(templates/sources_list.html).TargetCountonlyGET /source/{id}/edit(templates/source_edit.html)target,configorurlreference at allDeliveryResult.Errorand loggedmaskURLError(target_http.go:367,target_http.go:507,target_slack.go:128)POST /source/{id}/targetsSSRF rejection (response body + log line)ValidateTargetURL's error, which reports scheme, hostname or IP only; itsurl.Parsebranch goes throughmaskURLError(ssrf.go:99), and the log line throughdelivery.MaskURL(source_management.go:1142)/metricsgo-http-metricslabels only: route pattern, method, code, service. No URL-valued label.No new path to add.
internal/server/routes.goand every file undertemplates/are unchanged between the tree this was first auditedagainst and
2ee720a. The twelve commits that landed onnextinbetween 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.
TargetViewstill 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 tocfg.URLand nothing else changed,make testexits 2:The done-criterion test fails on the secret path segments themselves —
source_detail_test.go:164through:167report the rendered pagecontaining
/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX,T00000000,B00000000andXXXXXXXXXXXXXXXXXXXXXXXX, and:172reports it no longer containing
https://hooks.slack.com/.... The viewtest fails the same way at
target_config_view_test.go:213and:220-:223.TestHandleSourceDetail_MasksSlackWebhookURLkeepspassing 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
httptarget whose destination is a secret-bearing incomingwebhook 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 viewlayer.
Gates, on
855439cmake checkexits 0 (test, lint, fmt-check).Containerized, cache defeated on both check-running stages:
docker build --no-cache-filter=lint --no-cache-filter=builder .exits0 in 2m48s. Not a cached no-op:
lintstage:make fmt-checkin 0.9s, thenmake lintfor 47.6sreporting
0 issues.builderstage:make testfor 51.5s, every package with a realduration 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--- PASSin that run.CACHED, all in the final runtime stage.--no-cache-filterrather than a prune, per the shared-hostrule. No containers were started; the tagged image was removed after.
Disclosure on host lint: the first host
make checkfailed with 8 lintissues 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_CACHEgives0 issues.andmake checkexit 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.
5c0ea2b44fto855439cc56PASS — satisfies the done-criterion of #115; single commit
855439connext, 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-check0.3s;#18 make lint63.1s emitting0 issues.;#30 make test62.2s with real per-package durations (delivery 4.460s,handlers 3.901s, …) and zero(cached)markers. The 15CACHEDlayers are base images,go mod downloadand the runtime stage only — none on a check-running layer. Both new tests logged--- PASSin that run. Image removed; no containers started.check / check (push)green on855439c.Probes that were the likely failure sites:
MaskURL(cfg.URL)tocfg.URLand changing nothing else makesmake testexit 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(NotContainsof the secret path and each of its segments) andtarget_config_view_test.go:220–:223. Tree restored;git statusclean at855439c.MaskURLcoverage. 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.2ee720a:httpConfigFieldsrenderedValue: cfg.URLraw before this commit.source_detail.html,source_logs.html,sources_list.html,source_edit.html, the add-target form (novalue=prefill), the target create/toggle/delete redirects, everyslogsite ininternal/delivery,internal/handlers,internal/server,DeliveryResult.Error(masked at all raise sites),ValidateTargetURL's rejection body, and thego-http-metricslabel 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:1002reads the destination withr.FormValue("url"), which falls back to the query string when the body has no such field, whileinternal/middleware/middleware.go:123logs"url", r.URL.String()unmasked.POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/SECRETtherefore creates the target and writes the full credential to the access log (and into Sentry request context whenSENTRY_DSNis 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.PostFormValueat:1001–:1005closes it.Anomalies that pass anyway:
MaskURLemits no/...marker for a query-only URL (https://h?token=xrenders ashttps://h), because the elision is keyed on a non-empty path. The query is still dropped, so no leak — cosmetic only.TODO.md:41will be stale on merge (it lists #115 as an open owner decision).TODO.mdis 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.