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:
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.
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.
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 next2026-08-24 04:04:04 +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.
Closes #272.
renderSourceDetailassigned the rawX-Forwarded-Protovalue into the URL scheme. The scheme now comes fromreqtls.IsTLS(r)— the shared predicate, consumed, not reimplemented — so it is only everhttporhttps, 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.Hoststays unconstrained.Hostheader carrieshost:portthere) and an IPv6 literal. Both are covered by a test, and the live run below is on a non-default port.NoCachemiddleware marksno-store; it is never stored.templates/source_detail.html:73putsBaseURLin HTML text inside<code>, sohtml/template'surlFilternever runs, andstatic/js/app.jscopies it withtextContent.That last point is contextual, not intrinsic, so a comment at the assignment site marks the trap for anyone later moving
BaseURLinto anhref.A garbage token renders
http://host: anything that is nothttpsis 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 checkgreen withGOFLAGS=-count=1after the rebase onto currentnext— 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
nextfor every spelling below.Live instance on a non-default port,
Host: hooks.example.com:19520, before and after:X-Forwarded-Protohttp://hooks.example.com:19520/webhook/...httpshttps://...HTTPSHTTPS://hooks.example.com:19520/webhook/...https://...https, httphttps, http://hooks.example.com:19520/webhook/...https://...httphttp://...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 claiminghttp, and the host authority surviving verbatim forhooks.example.com:8443,[2001:db8::1]:8443and a bare hostname.Scope:
internal/handlersonly.internal/reqtlsis consumed unchanged.PASS. Independently reproduced on unmodified
nextand confirmed fixed: my own 23-case probe on the rendered page (all issue spellings plusHtTpS, 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 onnext— includingjavascript:alert(1)//://host— and 0 on3939cd8, with the authority preserved verbatim forhost:portand[2001:db8::1].reqtlsconsumed unchanged, no second parse anywhere, direct TLS now outranks a header claiminghttp. Trap comment accurate:BaseURLhas exactly one render site,source_detail.html:73, still HTML text inside<code>. The author's tests fail onnextand pass on head.make checkgreen from a clean clone withGOFLAGS=-count=1in 73s, 0(cached)markers, lint in Docker at0 issues; CI green on the head commit; fast-forwardable ontonext.Host decision: the argument holds. Traced every escape path — no redirect builds a
Locationfromr.Host, noHeader.Get("Host")anywhere, no mail path, the delivery engine rejects aHostoverride, andnet/httppromotesHostout ofr.Header(verified) so the receiver'sjson.Marshal(r.Header)cannot persist it. The value round-trips only to the authenticated requester who supplied it, on aNoCacheroute. The one pre-existing outbound use isinternal/server/sentry.go:175, which is deliberate, documented, and unrelated to this change.Non-blocking nit:
source_detail_baseurl_test.go:117assertsrequire.Len(t, match, 2, "the page must render exactly one entrypoint URL"), butFindStringSubmatchreturns 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, wherego testwas invoked directly; the reviewed tree itself saw onlymaketargets.