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

BaseURL on the source detail page uses the raw X-Forwarded-Proto value as the URL scheme #272

New Issue
Closed
opened 2026-08-24 02:15:44 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-24 02:15:44 +02:00
Collaborator
Copy Link

Found by the TLS-detection audit in #269, which asked for a third site that decides "is this request TLS?". This is it, and it is the worst-behaved of the three.

internal/handlers/source_management.go:432-440:

host := r.Host
scheme := "https"

if r.TLS == nil {
    scheme = "http"
}

if fwdProto := r.Header.Get("X-Forwarded-Proto"); fwdProto != "" {
    scheme = fwdProto
}

The header value is not parsed, not normalized and not validated — it is assigned straight into the scheme, and the result is concatenated into BaseURL (scheme + "://" + host) and rendered on the source detail page as the entrypoint URL an operator copies into the sending system.

Consequences, using the same spellings probed in #269:

  • X-Forwarded-Proto: HTTPS produces HTTPS://host/...
  • X-Forwarded-Proto: https, http produces https, http://host/...
  • X-Forwarded-Proto: https,https produces https,https://host/...
  • Any other token — the header is not constrained to http/https — lands in the scheme verbatim.

Note also the first branch is dead in the common case: when the header is present at all it overwrites whatever r.TLS decided, so a direct-TLS request behind a proxy that sets http is displayed as http.

#269 deliberately did NOT fix this — internal/handlers was held by a parallel unit at the time and the scope was internal/session plus internal/middleware. It landed the shared predicate this should adopt.

Definition of done

  • internal/handlers/source_management.go derives the scheme from reqtls.IsTLS(r) (added by #269, internal/reqtls) rather than from the raw header, so the scheme can only ever be http or https and all three call sites agree.
  • A test covering the spellings above, asserting BaseURL is https://host for each of https, HTTPS, https, http, https,https, and http://host for http.
  • make check green.
Found by the TLS-detection audit in https://git.eeqj.de/sneak/webhooker/issues/269, which asked for a third site that decides "is this request TLS?". This is it, and it is the worst-behaved of the three. `internal/handlers/source_management.go:432-440`: ```go host := r.Host scheme := "https" if r.TLS == nil { scheme = "http" } if fwdProto := r.Header.Get("X-Forwarded-Proto"); fwdProto != "" { scheme = fwdProto } ``` The header value is not parsed, not normalized and not validated — it is assigned straight into the scheme, and the result is concatenated into `BaseURL` (`scheme + "://" + host`) and rendered on the source detail page as the entrypoint URL an operator copies into the sending system. Consequences, using the same spellings probed in https://git.eeqj.de/sneak/webhooker/issues/269: - `X-Forwarded-Proto: HTTPS` produces `HTTPS://host/...` - `X-Forwarded-Proto: https, http` produces `https, http://host/...` - `X-Forwarded-Proto: https,https` produces `https,https://host/...` - Any other token — the header is not constrained to `http`/`https` — lands in the scheme verbatim. Note also the first branch is dead in the common case: when the header is present at all it overwrites whatever `r.TLS` decided, so a direct-TLS request behind a proxy that sets `http` is displayed as `http`. https://git.eeqj.de/sneak/webhooker/issues/269 deliberately did NOT fix this — `internal/handlers` was held by a parallel unit at the time and the scope was `internal/session` plus `internal/middleware`. It landed the shared predicate this should adopt. ## Definition of done - `internal/handlers/source_management.go` derives the scheme from `reqtls.IsTLS(r)` (added by https://git.eeqj.de/sneak/webhooker/issues/269, `internal/reqtls`) rather than from the raw header, so the scheme can only ever be `http` or `https` and all three call sites agree. - A test covering the spellings above, asserting `BaseURL` is `https://host` for each of `https`, `HTTPS`, `https, http`, `https,https`, and `http://host` for `http`. - `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-24 02:33:57 +02:00
clawbot commented 2026-08-24 02:48:21 +02:00
Author
Collaborator
Copy Link

Safety question settled during the review of #276, since the raw header lands in a rendered URL.

No XSS, and no dangerous href. But the protection is not what you would guess: html/template's urlFilter never runs. templates/source_detail.html:73 renders {{$.BaseURL}}/webhook/{{.Path}} inside a <code> element — HTML text context, not an href or any URL attribute — so a hostile scheme token gets ordinary entity escaping and appears as inert text. static/js/app.js:33 copies it with textContent, which navigates nowhere.

Two things that follow, both worth handling in this issue:

  • The safety is CONTEXTUAL, not intrinsic. Anyone later moving BaseURL into an href changes the analysis. It would still be safe, via #ZgotmplZ, but for an entirely different reason. Put a short comment at the assignment site saying the value is unvalidated and safe only because of where it is rendered — that is exactly the kind of trap a comment should mark.
  • host := r.Host on the adjacent line is equally unvalidated and fully client-controlled — a larger lever than the scheme, with the same benign outcome in this context. Fixing the scheme while leaving the host unvalidated addresses the smaller half. Decide deliberately whether to constrain the host too, and say which you chose.

Confirms the severity already recorded here: a correctness defect, because the operator copies a wrong URL into the sending system and the webhook never arrives.

Safety question settled during the review of https://git.eeqj.de/sneak/webhooker/pulls/276, since the raw header lands in a rendered URL. **No XSS, and no dangerous `href`.** But the protection is not what you would guess: `html/template`'s `urlFilter` never runs. `templates/source_detail.html:73` renders `{{$.BaseURL}}/webhook/{{.Path}}` inside a `<code>` element — HTML text context, not an `href` or any URL attribute — so a hostile scheme token gets ordinary entity escaping and appears as inert text. `static/js/app.js:33` copies it with `textContent`, which navigates nowhere. Two things that follow, both worth handling in this issue: - The safety is CONTEXTUAL, not intrinsic. Anyone later moving `BaseURL` into an `href` changes the analysis. It would still be safe, via `#ZgotmplZ`, but for an entirely different reason. Put a short comment at the assignment site saying the value is unvalidated and safe only because of where it is rendered — that is exactly the kind of trap a comment should mark. - `host := r.Host` on the adjacent line is equally unvalidated and fully client-controlled — a larger lever than the scheme, with the same benign outcome in this context. Fixing the scheme while leaving the host unvalidated addresses the smaller half. Decide deliberately whether to constrain the host too, and say which you chose. Confirms the severity already recorded here: a correctness defect, because the operator copies a wrong URL into the sending system and the webhook never arrives.
clawbot commented 2026-08-24 03:45:26 +02:00
Author
Collaborator
Copy Link

Plan, on branch issue-272-baseurl-scheme:

  • Replace the r.TLS/raw-header scheme derivation in renderSourceDetail with reqtls.IsTLS(r) (http/https only). No parallel parse.
  • Host: keep r.Host unconstrained, deliberately. There is no canonical hostname anywhere in config to validate against, so a shape check could only reject with nothing correct to fall back to, and any constraint risks breaking exactly the deployments that matter (proxy on a non-default port, IPv6 literal). The reasoning goes in the PR body.
  • Short comment at the assignment site marking the trap: the value is unvalidated and inert only because templates/source_detail.html:73 renders it in HTML text context inside <code>; moving it into an href changes that.
  • Test in internal/handlers driving the real handler and asserting the rendered entrypoint URL for https, HTTPS, https, http, https,https, https (trailing space), http, a garbage token, and no header at all.
Plan, on branch `issue-272-baseurl-scheme`: - Replace the `r.TLS`/raw-header scheme derivation in `renderSourceDetail` with `reqtls.IsTLS(r)` (`http`/`https` only). No parallel parse. - Host: keep `r.Host` unconstrained, deliberately. There is no canonical hostname anywhere in config to validate against, so a shape check could only reject with nothing correct to fall back to, and any constraint risks breaking exactly the deployments that matter (proxy on a non-default port, IPv6 literal). The reasoning goes in the PR body. - Short comment at the assignment site marking the trap: the value is unvalidated and inert only because `templates/source_detail.html:73` renders it in HTML text context inside `<code>`; moving it into an `href` changes that. - Test in `internal/handlers` driving the real handler and asserting the rendered entrypoint URL for `https`, `HTTPS`, `https, http`, `https,https`, `https ` (trailing space), `http`, a garbage token, and no header at all.
clawbot commented 2026-08-24 03:56:07 +02:00
Author
Collaborator
Copy Link

PR: #286

Scheme now derives from reqtls.IsTLS(r), so it is only ever http or https, and the dead branch is gone: direct TLS outranks a header claiming plaintext. A garbage token renders http://host — not TLS by the shared predicate, and never the token.

Host: left unconstrained, deliberately. No canonical hostname exists in config to check against, so a check could only reject with nothing correct to fall back to; the page is authenticated, no-store and rendered back to the same requester, so nothing persists a poisoned value; and constraining it would break proxy-on-a-non-default-port and IPv6-literal deployments. Comment at the assignment site marks that the value is inert only because of the <code> render context.

Verified: probe written first, failing on unmodified next for HTTPS, https, http, https,https, https , the garbage token and the TLS-precedence case; green after. make check green with GOFLAGS=-count=1, tests uncached, lint in Docker at 0 issues. Also a live instance on hooks.example.com:19520 — X-Forwarded-Proto: HTTPS rendered HTTPS://hooks.example.com:19520/webhook/... before and https://hooks.example.com:19520/webhook/... after, with the port preserved; plain HTTP with no header renders http://....

PR: https://git.eeqj.de/sneak/webhooker/pulls/286 Scheme now derives from `reqtls.IsTLS(r)`, so it is only ever `http` or `https`, and the dead branch is gone: direct TLS outranks a header claiming plaintext. A garbage token renders `http://host` — not TLS by the shared predicate, and never the token. Host: left unconstrained, deliberately. No canonical hostname exists in config to check against, so a check could only reject with nothing correct to fall back to; the page is authenticated, `no-store` and rendered back to the same requester, so nothing persists a poisoned value; and constraining it would break proxy-on-a-non-default-port and IPv6-literal deployments. Comment at the assignment site marks that the value is inert only because of the `<code>` render context. Verified: probe written first, failing on unmodified `next` for `HTTPS`, `https, http`, `https,https`, `https `, the garbage token and the TLS-precedence case; green after. `make check` green with `GOFLAGS=-count=1`, tests uncached, lint in Docker at 0 issues. Also a live instance on `hooks.example.com:19520` — `X-Forwarded-Proto: HTTPS` rendered `HTTPS://hooks.example.com:19520/webhook/...` before and `https://hooks.example.com:19520/webhook/...` after, with the port preserved; plain HTTP with no header renders `http://...`.
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#272
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?