Derive the source detail BaseURL scheme from reqtls (closes #272) #286

clawbot · 2026-08-24T03:55:55+02:00

clawbot commented

2026-08-24 03:55:55 +02:00

Closes #272.

renderSourceDetail assigned the raw X-Forwarded-Proto value into the URL scheme. The scheme now comes from reqtls.IsTLS(r) — the shared predicate, consumed, not reimplemented — so it is only ever http or https, and all three TLS-decision sites agree. This also fixes the inverted precedence: a present header no longer overwrites what a direct TLS connection proved.

The host: left as the request sent it, deliberately

r.Host stays unconstrained.

Nothing in the configuration names a canonical hostname, so a check could only reject a value with nothing correct to fall back to. A placeholder authority breaks the copyable URL rather than repairing it.
Any constraint risks exactly the deployments that matter: a proxy on a non-default port (the Host header carries host:port there) and an IPv6 literal. Both are covered by a test, and the live run below is on a non-default port.
The exposure that would make host-header poisoning matter — a poisoned link persisted, emailed, or served from a cache to someone else — does not exist here. The value is rendered into the same response, to the same authenticated requester, on a page the NoCache middleware marks no-store; it is never stored.
The render context makes it inert: templates/source_detail.html:73 puts BaseURL in HTML text inside <code>, so html/template's urlFilter never runs, and static/js/app.js copies it with textContent.

That last point is contextual, not intrinsic, so a comment at the assignment site marks the trap for anyone later moving BaseURL into an href.

A garbage token renders http://host: anything that is not https is not TLS, per the shared predicate. The operator sees a URL that is visibly the wrong scheme rather than one that is malformed.

Verification

make check green with GOFLAGS=-count=1 after the rebase onto current next — tests ran uncached (ok sneak.berlin/go/webhooker/internal/handlers 20.774s, no (cached) lines), lint ran in Docker, 0 issues.

The new test drives the real handler and extracts the rendered entrypoint URL from the page, so it asserts what the operator actually copies. It was written first and failed on unmodified next for every spelling below.

Live instance on a non-default port, Host: hooks.example.com:19520, before and after:

`X-Forwarded-Proto`	before	after
(none, plain http)	`http://hooks.example.com:19520/webhook/...`	unchanged
`https`	`https://...`	unchanged
`HTTPS`	`HTTPS://hooks.example.com:19520/webhook/...`	`https://...`
`https, http`	`https, http://hooks.example.com:19520/webhook/...`	`https://...`
`http`	`http://...`	unchanged
`javascript:alert(1)//`	`javascript:alert(1)//://hooks.example.com:19520/webhook/...`	`http://...`

Unit coverage adds https,https, https (trailing space), direct TLS outranking a header claiming http, and the host authority surviving verbatim for hooks.example.com:8443, [2001:db8::1]:8443 and a bare hostname.

Scope: internal/handlers only. internal/reqtls is consumed unchanged.

Closes https://git.eeqj.de/sneak/webhooker/issues/272. `renderSourceDetail` assigned the raw `X-Forwarded-Proto` value into the URL scheme. The scheme now comes from `reqtls.IsTLS(r)` — the shared predicate, consumed, not reimplemented — so it is only ever `http` or `https`, and all three TLS-decision sites agree. This also fixes the inverted precedence: a present header no longer overwrites what a direct TLS connection proved. ## The host: left as the request sent it, deliberately `r.Host` stays unconstrained. - Nothing in the configuration names a canonical hostname, so a check could only reject a value with nothing correct to fall back to. A placeholder authority breaks the copyable URL rather than repairing it. - Any constraint risks exactly the deployments that matter: a proxy on a non-default port (the `Host` header carries `host:port` there) and an IPv6 literal. Both are covered by a test, and the live run below is on a non-default port. - The exposure that would make host-header poisoning matter — a poisoned link persisted, emailed, or served from a cache to someone else — does not exist here. The value is rendered into the same response, to the same authenticated requester, on a page the `NoCache` middleware marks `no-store`; it is never stored. - The render context makes it inert: `templates/source_detail.html:73` puts `BaseURL` in HTML text inside `<code>`, so `html/template`'s `urlFilter` never runs, and `static/js/app.js` copies it with `textContent`. That last point is contextual, not intrinsic, so a comment at the assignment site marks the trap for anyone later moving `BaseURL` into an `href`. A garbage token renders `http://host`: anything that is not `https` is not TLS, per the shared predicate. The operator sees a URL that is visibly the wrong scheme rather than one that is malformed. ## Verification `make check` green with `GOFLAGS=-count=1` after the rebase onto current `next` — tests ran uncached (`ok sneak.berlin/go/webhooker/internal/handlers 20.774s`, no `(cached)` lines), lint ran in Docker, `0 issues`. The new test drives the real handler and extracts the rendered entrypoint URL from the page, so it asserts what the operator actually copies. It was written first and failed on unmodified `next` for every spelling below. Live instance on a non-default port, `Host: hooks.example.com:19520`, before and after: | `X-Forwarded-Proto` | before | after | | --- | --- | --- | | (none, plain http) | `http://hooks.example.com:19520/webhook/...` | unchanged | | `https` | `https://...` | unchanged | | `HTTPS` | `HTTPS://hooks.example.com:19520/webhook/...` | `https://...` | | `https, http` | `https, http://hooks.example.com:19520/webhook/...` | `https://...` | | `http` | `http://...` | unchanged | | `javascript:alert(1)//` | `javascript:alert(1)//://hooks.example.com:19520/webhook/...` | `http://...` | Unit coverage adds `https,https`, `https ` (trailing space), direct TLS outranking a header claiming `http`, and the host authority surviving verbatim for `hooks.example.com:8443`, `[2001:db8::1]:8443` and a bare hostname. Scope: `internal/handlers` only. `internal/reqtls` is consumed unchanged.

clawbot added 1 commit 2026-08-24 03:55:55 +02:00

Derive the source detail BaseURL scheme from reqtls (closes #272 )

check / check (push) Successful in 3m10s

Details

3939cd87d4

The source detail page assigned the raw X-Forwarded-Proto value
straight into the URL scheme, so `HTTPS` rendered `HTTPS://host`,
a chained proxy's `https, http` rendered `https, http://host`, and
any token at all landed there verbatim. That URL is what the
operator pastes into GitHub or Stripe, so a malformed scheme means
the webhook never arrives. A present header also overwrote what a
direct TLS connection had already proved.

The scheme now comes from reqtls.IsTLS, the shared predicate, so it
is only ever http or https and all three TLS-decision sites agree.

The host stays as the request sent it: nothing in the configuration
names a canonical hostname to validate against, and constraining it
would break the deployments behind a proxy on a non-default port or
an IPv6 literal. A comment marks why the unvalidated value is inert
where it is rendered.

clawbot added the needs-review label 2026-08-24 03:55:58 +02:00

clawbot self-assigned this 2026-08-24 03:55:59 +02:00

clawbot referenced this pull request

2026-08-24 03:56:07 +02:00

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

clawbot commented

2026-08-24 04:03:20 +02:00

PASS. Independently reproduced on unmodified next and confirmed fixed: my own 23-case probe on the rendered page (all issue spellings plus HtTpS, leading space, HTTP, ftp, data:text/html,x, https://evil, a 64-char token, TLS+garbage, IPv6 without a port) yields 17 out-of-range schemes on next — including javascript:alert(1)//://host — and 0 on 3939cd8, with the authority preserved verbatim for host:port and [2001:db8::1]. reqtls consumed unchanged, no second parse anywhere, direct TLS now outranks a header claiming http. Trap comment accurate: BaseURL has exactly one render site, source_detail.html:73, still HTML text inside <code>. The author's tests fail on next and pass on head. make check green from a clean clone with GOFLAGS=-count=1 in 73s, 0 (cached) markers, lint in Docker at 0 issues; CI green on the head commit; fast-forwardable onto next.

Host decision: the argument holds. Traced every escape path — no redirect builds a Location from r.Host, no Header.Get("Host") anywhere, no mail path, the delivery engine rejects a Host override, and net/http promotes Host out of r.Header (verified) so the receiver's json.Marshal(r.Header) cannot persist it. The value round-trips only to the authenticated requester who supplied it, on a NoCache route. The one pre-existing outbound use is internal/server/sentry.go:175, which is deliberate, documented, and unrelated to this change.

Non-blocking nit: source_detail_baseurl_test.go:117 asserts require.Len(t, match, 2, "the page must render exactly one entrypoint URL"), but FindStringSubmatch returns only the first match, so this proves at-least-one, not exactly-one. Harmless — the fixture seeds a single entrypoint — but the message overclaims.

Disclosure: the probe and the next-side runs were done in a throwaway copy of the tree, where go test was invoked directly; the reviewed tree itself saw only make targets.

**PASS.** Independently reproduced on unmodified `next` and confirmed fixed: my own 23-case probe on the rendered page (all issue spellings plus `HtTpS`, leading space, `HTTP`, `ftp`, `data:text/html,x`, `https://evil`, a 64-char token, TLS+garbage, IPv6 without a port) yields 17 out-of-range schemes on `next` — including `javascript:alert(1)//://host` — and 0 on `3939cd8`, with the authority preserved verbatim for `host:port` and `[2001:db8::1]`. `reqtls` consumed unchanged, no second parse anywhere, direct TLS now outranks a header claiming `http`. Trap comment accurate: `BaseURL` has exactly one render site, `source_detail.html:73`, still HTML text inside `<code>`. The author's tests fail on `next` and pass on head. `make check` green from a clean clone with `GOFLAGS=-count=1` in 73s, 0 `(cached)` markers, lint in Docker at `0 issues`; CI green on the head commit; fast-forwardable onto `next`. **Host decision: the argument holds.** Traced every escape path — no redirect builds a `Location` from `r.Host`, no `Header.Get("Host")` anywhere, no mail path, the delivery engine rejects a `Host` override, and `net/http` promotes `Host` out of `r.Header` (verified) so the receiver's `json.Marshal(r.Header)` cannot persist it. The value round-trips only to the authenticated requester who supplied it, on a `NoCache` route. The one pre-existing outbound use is `internal/server/sentry.go:175`, which is deliberate, documented, and unrelated to this change. Non-blocking nit: `source_detail_baseurl_test.go:117` asserts `require.Len(t, match, 2, "the page must render exactly one entrypoint URL")`, but `FindStringSubmatch` returns only the first match, so this proves at-least-one, not exactly-one. Harmless — the fixture seeds a single entrypoint — but the message overclaims. Disclosure: the probe and the `next`-side runs were done in a throwaway copy of the tree, where `go test` was invoked directly; the reviewed tree itself saw only `make` targets.

clawbot merged commit 48cf93ec7e into next

2026-08-24 04:04:04 +02:00

clawbot deleted branch issue-272-baseurl-scheme

2026-08-24 04:04:05 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#286