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

Slack webhook credential still leaks via delivery errors, SSRF logs, and the source_logs template data #118

New Issue
Closed
opened 2026-08-11 14:37:28 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-11 14:37:28 +02:00
Collaborator
Copy Link

Found during the review of #114, which masked the credential in the source detail page. These three sibling paths leak the same secret and were outside that PR's scope. Milestoned 1.0.0 for the same reason #113 was: the milestone is internet-facing readiness.

The first one is the most serious, because it writes the credential to disk.

  1. Delivery errors persist the full URL. internal/delivery/target_slack.go:128,142 and internal/delivery/target_http.go:337 wrap the *url.Error from the HTTP client into DeliveryResult.Error. Go embeds the request URL in that error, so any transport failure — DNS, TLS, timeout, SSRF dial block — stores Post "https://hooks.slack.com/services/T../B../XXXX": dial tcp ... into the per-webhook SQLite database. It is not rendered today, but the field is tagged json:"error,omitempty", so the REST API on the roadmap serializes it the moment it exists.
  2. SSRF rejection logs the submitted URL. internal/handlers/source_management.go:1019 — h.log.Warn("target URL blocked by SSRF protection", "url", targetURL, ...) writes the full URL to the application log.
  3. source_logs.html still receives raw config. internal/handlers/source_management.go:757 attaches raw database.Target values, config included, to that page's template data. Nothing renders it today, so it is one {{.Target.Config}} away from reintroducing the exact bug #113 just fixed — the view projection does not cover this page.

Definition of done

  • Delivery targets convert *url.Error into a message carrying only maskURL(...) before it reaches DeliveryResult.Error.
  • source_management.go:1019 logs the masked URL.
  • loadTargetMap hands source_logs.html a TargetView rather than a raw database.Target, so the raw config is unreachable from that template too.
  • A test asserting a failed Slack delivery's stored DeliveryResult.Error contains no path segment of the configured webhook URL. This is the load-bearing one.

Implementation requirements

  • Reuse the existing maskURL in internal/delivery/target_config_view.go; do not write a second masker.
  • Branch from next, PR based on next, single commit, title ending (closes #N).
  • Gate on make check plus the Docker lint path with the cache defeated — a fully cached script/cibuild is not evidence.
Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/114, which masked the credential in the source detail page. These three sibling paths leak the same secret and were outside that PR's scope. Milestoned 1.0.0 for the same reason https://git.eeqj.de/sneak/webhooker/issues/113 was: the milestone is internet-facing readiness. The first one is the most serious, because it writes the credential to disk. 1. **Delivery errors persist the full URL.** `internal/delivery/target_slack.go:128,142` and `internal/delivery/target_http.go:337` wrap the `*url.Error` from the HTTP client into `DeliveryResult.Error`. Go embeds the request URL in that error, so any transport failure — DNS, TLS, timeout, SSRF dial block — stores `Post "https://hooks.slack.com/services/T../B../XXXX": dial tcp ...` into the per-webhook SQLite database. It is not rendered today, but the field is tagged `json:"error,omitempty"`, so the REST API on the roadmap serializes it the moment it exists. 2. **SSRF rejection logs the submitted URL.** `internal/handlers/source_management.go:1019` — `h.log.Warn("target URL blocked by SSRF protection", "url", targetURL, ...)` writes the full URL to the application log. 3. **`source_logs.html` still receives raw config.** `internal/handlers/source_management.go:757` attaches raw `database.Target` values, config included, to that page's template data. Nothing renders it today, so it is one `{{.Target.Config}}` away from reintroducing the exact bug https://git.eeqj.de/sneak/webhooker/issues/113 just fixed — the view projection does not cover this page. ## Definition of done - Delivery targets convert `*url.Error` into a message carrying only `maskURL(...)` before it reaches `DeliveryResult.Error`. - `source_management.go:1019` logs the masked URL. - `loadTargetMap` hands `source_logs.html` a `TargetView` rather than a raw `database.Target`, so the raw config is unreachable from that template too. - A test asserting a failed Slack delivery's stored `DeliveryResult.Error` contains no path segment of the configured webhook URL. This is the load-bearing one. ## Implementation requirements - Reuse the existing `maskURL` in `internal/delivery/target_config_view.go`; do not write a second masker. - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Gate on `make check` plus the Docker lint path with the cache defeated — a fully cached `script/cibuild` is not evidence.
clawbot added this to the 1.0.0 milestone 2026-08-11 14:37:28 +02:00
clawbot commented 2026-08-11 14:41:24 +02:00
Author
Collaborator
Copy Link

Plan:

  1. Move maskURL into internal/delivery/url_mask.go, export it as MaskURL (handlers need it), and add maskURLError beside it: it rebuilds a *url.Error with URL replaced by MaskURL(...) and Op/Err preserved, so the operation and the underlying transport error (DNS, TLS, refused, timeout) survive and only path/query/userinfo are dropped. One masker, one set of semantics.
  2. Apply maskURLError at the two points where a transport or request-construction error is born: executeHTTPRequest (shared by the Slack and HTTP targets) and the http.NewRequestWithContext error paths. Masking at birth means every wrapping site downstream is safe by construction. Also applied to ValidateTargetURL's url.Parse branch, which embeds the raw URL in the error the SSRF log line prints.
  3. loadTargetMap returns map[string]delivery.TargetView, and EventWithDeliveries.Deliveries becomes a []DeliveryView (status plus TargetView), so source_logs.html has no path to a stored config blob at all.
  4. Tests: load-bearing assertion that a failed Slack delivery's stored DeliveryResult.Error contains no path segment of the webhook URL while still naming the failure class, plus a rendered-page test for the logs template.
Plan: 1. Move `maskURL` into `internal/delivery/url_mask.go`, export it as `MaskURL` (handlers need it), and add `maskURLError` beside it: it rebuilds a `*url.Error` with `URL` replaced by `MaskURL(...)` and `Op`/`Err` preserved, so the operation and the underlying transport error (DNS, TLS, refused, timeout) survive and only path/query/userinfo are dropped. One masker, one set of semantics. 2. Apply `maskURLError` at the two points where a transport or request-construction error is born: `executeHTTPRequest` (shared by the Slack and HTTP targets) and the `http.NewRequestWithContext` error paths. Masking at birth means every wrapping site downstream is safe by construction. Also applied to `ValidateTargetURL`'s `url.Parse` branch, which embeds the raw URL in the error the SSRF log line prints. 3. `loadTargetMap` returns `map[string]delivery.TargetView`, and `EventWithDeliveries.Deliveries` becomes a `[]DeliveryView` (status plus `TargetView`), so `source_logs.html` has no path to a stored config blob at all. 4. Tests: load-bearing assertion that a failed Slack delivery's stored `DeliveryResult.Error` contains no path segment of the webhook URL while still naming the failure class, plus a rendered-page test for the logs template.
clawbot commented 2026-08-11 14:58:12 +02:00
Author
Collaborator
Copy Link

Implemented in #121 (issue-118-mask-credential-leaks, commit b6529f4).

All three leaks are closed with the one masker: maskURL moved to internal/delivery/url_mask.go, exported as MaskURL, joined by maskURLError, which rebuilds a *url.Error with the URL masked and Op plus the wrapped cause preserved. Masking happens where the errors are raised — executeHTTPRequest (shared by both targets), the request-construction paths, and ValidateTargetURL's url.Parse branch, whose error is logged and shown — so the stored error keeps its diagnostic value:

before: sending request: Post "http://host:45897/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX": dial tcp 127.0.0.1:45897: connect: connection refused
after:  sending request: Post "http://host:45897/...": dial tcp 127.0.0.1:45897: connect: connection refused

loadTargetMap returns map[string]delivery.TargetView and EventWithDeliveries.Deliveries is now a []DeliveryView, so source_logs.html has no reachable path to a stored config blob.

Verified: the load-bearing test asserts the stored DeliveryResult.Error holds no path segment of the webhook URL while still naming the operation, the masked host and the transport failure. Removing the mask from executeHTTPRequest makes it fail on all five segments and on the missing masked host; restoring it makes it pass. make check green, and the Docker path with the cache defeated ran make lint (0 issues) and make test (all packages with real durations, none cached) after rebasing onto current next.

Implemented in https://git.eeqj.de/sneak/webhooker/pulls/121 (`issue-118-mask-credential-leaks`, commit `b6529f4`). All three leaks are closed with the one masker: `maskURL` moved to `internal/delivery/url_mask.go`, exported as `MaskURL`, joined by `maskURLError`, which rebuilds a `*url.Error` with the URL masked and `Op` plus the wrapped cause preserved. Masking happens where the errors are raised — `executeHTTPRequest` (shared by both targets), the request-construction paths, and `ValidateTargetURL`'s `url.Parse` branch, whose error is logged and shown — so the stored error keeps its diagnostic value: before: sending request: Post "http://host:45897/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX": dial tcp 127.0.0.1:45897: connect: connection refused after: sending request: Post "http://host:45897/...": dial tcp 127.0.0.1:45897: connect: connection refused `loadTargetMap` returns `map[string]delivery.TargetView` and `EventWithDeliveries.Deliveries` is now a `[]DeliveryView`, so `source_logs.html` has no reachable path to a stored config blob. Verified: the load-bearing test asserts the stored `DeliveryResult.Error` holds no path segment of the webhook URL while still naming the operation, the masked host and the transport failure. Removing the mask from `executeHTTPRequest` makes it fail on all five segments and on the missing masked host; restoring it makes it pass. `make check` green, and the Docker path with the cache defeated ran `make lint` (0 issues) and `make test` (all packages with real durations, none cached) after rebasing onto current `next`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#118
Delete Branch "%!s()"

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?