From 3939cd87d45daffb84d5247214ae1b9751fe67c2 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 24 Aug 2026 01:54:04 +0000 Subject: [PATCH] Derive the source detail BaseURL scheme from reqtls (closes #272) 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. --- .../handlers/source_detail_baseurl_test.go | 283 ++++++++++++++++++ internal/handlers/source_management.go | 19 +- 2 files changed, 293 insertions(+), 9 deletions(-) create mode 100644 internal/handlers/source_detail_baseurl_test.go diff --git a/internal/handlers/source_detail_baseurl_test.go b/internal/handlers/source_detail_baseurl_test.go new file mode 100644 index 0000000..c3e0417 --- /dev/null +++ b/internal/handlers/source_detail_baseurl_test.go @@ -0,0 +1,283 @@ +package handlers_test + +import ( + "context" + "crypto/tls" + "net/http" + "net/http/httptest" + "regexp" + "testing" + + "github.com/go-chi/chi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +// The only two schemes a rendered entrypoint URL may carry, +// whatever the request claimed. +const ( + schemeHTTPS = "https" + schemeHTTP = "http" +) + +// entrypointURLPattern captures the entrypoint URL the source +// detail page renders, which is the operator-visible product of +// BaseURL. Asserting on the extracted string rather than on a +// substring of the page proves the raw header value cannot reach +// the scheme by any route. +var entrypointURLPattern = regexp.MustCompile( + `]*>([^<]*)`, +) + +// baseURLFixture is one started app plus the webhook whose +// entrypoint URL the BaseURL cases read. +type baseURLFixture struct { + handlers *handlers.Handlers + session *session.Session + webhook string + path string +} + +// newBaseURLFixture starts the app and seeds a webhook with one +// entrypoint. +func newBaseURLFixture(t *testing.T) *baseURLFixture { + t.Helper() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + ) + + app := newTestApp(t, &h, &sess, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + seedEntrypoint(t, db, wh.ID) + + return &baseURLFixture{ + handlers: h, + session: sess, + webhook: wh.ID, + path: "ep-" + wh.ID, + } +} + +// entrypointURL renders the source detail page for the fixture's +// webhook over a request the caller shapes, and returns the +// entrypoint URL as an operator would copy it. +func (f *baseURLFixture) entrypointURL( + t *testing.T, + host string, + shape func(*http.Request), +) string { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodGet, + "/source/"+f.webhook, + nil, + ) + req.Host = host + + shape(req) + + for _, c := range authenticatedCookies( + t, f.session, deleteTestUserID, deleteTestUsername, + ) { + req.AddCookie(c) + } + + rctx := chi.NewRouteContext() + rctx.URLParams.Add(paramSourceID, f.webhook) + req = req.WithContext( + context.WithValue( + req.Context(), chi.RouteCtxKey, rctx, + ), + ) + + w := httptest.NewRecorder() + f.handlers.HandleSourceDetail().ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + + match := entrypointURLPattern.FindStringSubmatch(w.Body.String()) + require.Len( + t, match, 2, + "the page must render exactly one entrypoint URL", + ) + + return match[1] +} + +// forwardedProto returns a request shaper setting +// X-Forwarded-Proto, or leaving the request alone for "". +func forwardedProto(value string) func(*http.Request) { + return func(r *http.Request) { + if value == "" { + return + } + + r.Header.Set("X-Forwarded-Proto", value) + } +} + +// baseURLCase is one X-Forwarded-Proto spelling and the scheme +// the rendered entrypoint URL owes it. +type baseURLCase struct { + name string + header string + scheme string + why string +} + +// baseURLCases enumerate the spellings a proxy really emits. The +// scheme is only ever http or https: the header value itself is +// never a scheme, however it is spelled. +func baseURLCases() []baseURLCase { + return []baseURLCase{ + { + name: "lowercase", + header: schemeHTTPS, + scheme: schemeHTTPS, + why: "the ordinary spelling", + }, + { + name: "uppercase", + header: "HTTPS", + scheme: schemeHTTPS, + why: "the token is case-insensitive; the scheme " + + "in a copyable URL is not", + }, + { + name: "chain with plaintext inner hop", + header: "https, http", + scheme: schemeHTTPS, + why: "a chained proxy appends its hop; the " + + "leftmost element faces the client", + }, + { + name: "chain of two TLS hops", + header: "https,https", + scheme: schemeHTTPS, + why: "appended chain with no space after the comma", + }, + { + name: "trailing space", + header: "https ", + scheme: schemeHTTPS, + why: "whitespace is not part of the token", + }, + { + name: "plaintext", + header: schemeHTTP, + scheme: schemeHTTP, + why: "the negative control: the proxy reports plaintext", + }, + { + name: "no header", + header: "", + scheme: schemeHTTP, + why: "a plaintext request asserting nothing is http", + }, + { + name: "garbage token", + header: "javascript:alert(1)//", + scheme: schemeHTTP, + why: "anything that is not https is not TLS, and " + + "the token never becomes the scheme", + }, + } +} + +// TestSourceDetailBaseURL_ForwardedProtoSpellings is the +// regression test for the entrypoint URL an operator pastes into +// the sending system: a header spelling that used to land in the +// scheme verbatim produced a URL no sender could deliver to. +func TestSourceDetailBaseURL_ForwardedProtoSpellings(t *testing.T) { + t.Parallel() + + const host = "hooks.example.com" + + fixture := newBaseURLFixture(t) + + for _, tc := range baseURLCases() { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal( + t, + tc.scheme+"://"+host+"/webhook/"+fixture.path, + fixture.entrypointURL( + t, host, forwardedProto(tc.header), + ), + "X-Forwarded-Proto %q: %s", tc.header, tc.why, + ) + }) + } +} + +// TestSourceDetailBaseURL_DirectTLSBeatsPlaintextHeader pins the +// precedence the old code had backwards: it let any present +// header overwrite what the connection itself proved, so a +// direct-TLS request behind a proxy reporting http rendered an +// http URL. +func TestSourceDetailBaseURL_DirectTLSBeatsPlaintextHeader( + t *testing.T, +) { + t.Parallel() + + const host = "hooks.example.com" + + fixture := newBaseURLFixture(t) + + got := fixture.entrypointURL(t, host, func(r *http.Request) { + r.TLS = &tls.ConnectionState{} + r.Header.Set("X-Forwarded-Proto", "http") + }) + + assert.Equal( + t, + "https://"+host+"/webhook/"+fixture.path, + got, + "a connection this process terminated with TLS "+ + "outranks a header claiming plaintext", + ) +} + +// TestSourceDetailBaseURL_KeepsHostAuthority pins the host half +// of the URL: it is taken from the request unchanged, so the +// deployments that do not sit on port 443 still get a URL that +// works. Constraining the host would break exactly these. +func TestSourceDetailBaseURL_KeepsHostAuthority(t *testing.T) { + t.Parallel() + + fixture := newBaseURLFixture(t) + + hosts := []string{ + "hooks.example.com:8443", + "[2001:db8::1]:8443", + "internal-host", + } + + for _, host := range hosts { + t.Run(host, func(t *testing.T) { + t.Parallel() + + assert.Equal( + t, + "https://"+host+"/webhook/"+fixture.path, + fixture.entrypointURL( + t, host, forwardedProto("HTTPS"), + ), + "the authority must survive verbatim, port and all", + ) + }) + } +} diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 5ddf7e4..4abb7fc 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -13,6 +13,7 @@ import ( "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" + "sneak.berlin/go/webhooker/internal/reqtls" ) // WebhookListItem holds data for the webhook list view. @@ -427,16 +428,16 @@ func (h *Handlers) renderSourceDetail( } } - host := r.Host - scheme := "https" - - if r.TLS == nil { - scheme = "http" + scheme := "http" + if reqtls.IsTLS(r) { + scheme = "https" } - if fwdProto := r.Header.Get("X-Forwarded-Proto"); fwdProto != "" { - scheme = fwdProto - } + // The host is the client's Host header, unvalidated. It is + // inert only because source_detail.html renders BaseURL as + // text inside a element; putting it in an href or any + // other URL context needs it constrained first. + baseURL := scheme + "://" + r.Host // The template calls Webhook methods, which take pointer // receivers; html/template cannot address a value stored in a map. @@ -448,7 +449,7 @@ func (h *Handlers) renderSourceDetail( "Entrypoints": NewEntrypointViews(entrypoints), "Targets": delivery.NewTargetViews(targets), "Events": events, - "BaseURL": scheme + "://" + host, + "BaseURL": baseURL, } h.renderTemplate(w, r, "source_detail.html", data) -- 2.49.1