Derive the entrypoint URL scheme from the shared TLS predicate (closes #272)

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(
+	`<code id="entrypoint-url-[^"]*"[^>]*>([^<]*)</code>`,
+)
+
+// 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",
+			)
+		})
+	}
+}

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 := "http"
-	scheme := "https"
+	if reqtls.IsTLS(r) {
-
+		scheme = "https"
-	if r.TLS == nil {
-		scheme = "http"
 	}
 
-	if fwdProto := r.Header.Get("X-Forwarded-Proto"); fwdProto != "" {
+	// The host is the client's Host header, unvalidated. It is
-		scheme = fwdProto
+	// inert only because source_detail.html renders BaseURL as
-	}
+	// text inside a <code> 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)