Fix CSP

internal/e2e/form_action_test.go

@@ -0,0 +1,125 @@
+//go:build browser
+
+package e2e
+
+import (
+	"strings"
+	"testing"
+
+	cdplog "github.com/chromedp/cdproto/log"
+	"github.com/chromedp/cdproto/network"
+	"github.com/chromedp/chromedp"
+)
+
+// Browser coverage for the CSP form-action directive.
+//
+// Every form in this app POSTs to one surface and may redirect to another (sign-in and
+// sign-up hand off to the app host; sign-out lands on the root host). Chrome enforces
+// form-action across that redirect, so a policy of `form-action 'self'` makes the browser
+// drop the redirect and leave the page sitting where it was. The POST is delivered and
+// the handler succeeds, so nothing server-side looks wrong — only the navigation is
+// lost. These tests are the only thing that can see that: handler tests enforce no CSP,
+// and a fetch() is governed by connect-src instead.
+
+// TestE2EPasswordFormsSurviveCSP guards the CSP regression directly, because the
+// round-trip above would only fail at its last step and the cause would read as a
+// recovery bug rather than what it is.
+//
+// Both credential forms POST to the auth host and redirect to the app host, and Chrome
+// checks form-action against that redirect. Under `form-action 'self'` the browser
+// dropped the redirect from both, leaving the visitor on the form with no indication
+// anything had happened — while the account was in fact created and the server logged a
+// 303. That gap is why every server-side test passed for a month while the default
+// sign-up and sign-in paths were broken in the majority browser.
+//
+// Deliberately driven through the rendered forms rather than posting directly: the
+// block happens in the browser, so only a browser can catch it.
+func TestE2EPasswordFormsSurviveCSP(t *testing.T) {
+	srv := newE2EServer(t)
+
+	bctx, cancel, watch := startBrowser(t)
+	defer cancel()
+
+	const (
+		username = "csp-form-user"
+		password = "a-perfectly-fine-password"
+	)
+
+	// Sign up with a password: auth host → handoff → app host.
+	var afterSignup string
+	if err := chromedp.Run(bctx,
+		network.Enable(),
+		cdplog.Enable(),
+		chromedp.Navigate(srv.authURL+"/signup"),
+		chromedp.WaitVisible(`#password`, chromedp.ByQuery),
+		chromedp.SendKeys(`#username`, username, chromedp.ByQuery),
+		chromedp.SendKeys(`#password`, password, chromedp.ByQuery),
+		chromedp.Submit(`#password`, chromedp.ByQuery),
+		waitForLocation(srv.appURL),
+		chromedp.Location(&afterSignup),
+	); err != nil {
+		t.Fatalf("password sign-up in the browser: %v", err)
+	}
+	if !strings.HasPrefix(afterSignup, srv.appURL) {
+		t.Errorf("after sign-up the browser is at %q, want the app host", afterSignup)
+	}
+
+	// And sign in again from a clean session.
+	var afterLogin string
+	if err := chromedp.Run(bctx,
+		network.ClearBrowserCookies(),
+		chromedp.Navigate(srv.authURL+"/login"),
+		chromedp.WaitVisible(`#password`, chromedp.ByQuery),
+		chromedp.SendKeys(`#username`, username, chromedp.ByQuery),
+		chromedp.SendKeys(`#password`, password, chromedp.ByQuery),
+		chromedp.Submit(`#password`, chromedp.ByQuery),
+		waitForLocation(srv.appURL),
+		chromedp.Location(&afterLogin),
+	); err != nil {
+		t.Fatalf("password sign-in in the browser: %v", err)
+	}
+	if !strings.HasPrefix(afterLogin, srv.appURL) {
+		t.Errorf("after sign-in the browser is at %q, want the app host", afterLogin)
+	}
+
+	// assertClean only looks for CSP violations, which is exactly the failure mode:
+	// a form-action block reports here and nowhere else.
+	watch.assertClean(t)
+}
+
+// TestE2ELogoutFormNavigatesCrossHost completes the form-action coverage. Sign-out is
+// the third flow the old policy broke: POST /logout answers 303 to the root host, so
+// Chrome blocked it exactly like sign-in.
+//
+// The existing logout test drives the same endpoint with fetch() and reads the form's
+// action attribute — deliberately, to stay independent of the cross-host redirect. But
+// fetch() is governed by connect-src, not form-action, so it cannot see this class of
+// bug. This test submits the real form and follows the navigation.
+func TestE2ELogoutFormNavigatesCrossHost(t *testing.T) {
+	srv := newE2EServer(t)
+	_, cookie := srv.login(t, "logout-form-user")
+
+	bctx, cancel, watch := startBrowser(t)
+	defer cancel()
+
+	var landed string
+	if err := chromedp.Run(bctx,
+		network.Enable(),
+		cdplog.Enable(),
+		setSessionCookie(cookie),
+		chromedp.Navigate(srv.appURL+"/repeaters"),
+		// The form lives in the user-menu dropdown: in the DOM but not laid out until
+		// the menu opens, so wait for readiness rather than visibility.
+		chromedp.WaitReady(`[data-testid="logout-form"]`, chromedp.ByQuery),
+		chromedp.Evaluate(`document.querySelector('[data-testid="logout-form"]').submit()`, nil),
+		// Sign-out lands on the public root host.
+		waitForLocation(srv.rootURL),
+		chromedp.Location(&landed),
+	); err != nil {
+		t.Fatalf("submit the sign-out form: %v", err)
+	}
+	if !strings.HasPrefix(landed, srv.rootURL) {
+		t.Errorf("after sign-out the browser is at %q, want the root host %q", landed, srv.rootURL)
+	}
+	watch.assertClean(t)
+}

internal/e2e/harness_test.go

@@ -35,6 +35,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"os"
+	"regexp"
 	"strings"
 	"sync"
 	"testing"
@@ -46,11 +47,13 @@ import (
 	"github.com/chromedp/cdproto/security"
 	"github.com/chromedp/chromedp"
 	meshcore "github.com/meshcore-go/meshcore-go"
+	"golang.org/x/crypto/bcrypt"
 
 	"github.com/jleight/meshtender/internal/auth"
 	"github.com/jleight/meshtender/internal/config"
 	"github.com/jleight/meshtender/internal/core"
 	"github.com/jleight/meshtender/internal/identity"
+	mailer "github.com/jleight/meshtender/internal/mail"
 	"github.com/jleight/meshtender/internal/store"
 	"github.com/jleight/meshtender/internal/testdb"
 )
@@ -102,8 +105,46 @@ type e2eServer struct {
 	appURL  string // https://app.<browserHost>:PORT  — the product surface
 	authURL string // https://auth.<browserHost>:PORT — sign-in + account
 	rootURL string // https://root.<browserHost>:PORT — public discovery
+	// mail captures what the app would have sent. Account-recovery tests read the
+	// links out of it, which is the only way to drive those flows the way a real
+	// recipient does — the token exists nowhere else in plaintext.
+	mail *captureSender
 }
 
+// captureSender records messages instead of delivering them.
+type captureSender struct {
+	mu   sync.Mutex
+	sent []mailer.Message
+}
+
+func (c *captureSender) Send(_ context.Context, m mailer.Message) error {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.sent = append(c.sent, m)
+	return nil
+}
+
+// lastLink returns the path of the first recovery link in the most recent message.
+// Tests navigate to that path, so the browser follows exactly what a recipient
+// would click.
+func (c *captureSender) lastLink(t *testing.T) string {
+	t.Helper()
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	if len(c.sent) == 0 {
+		t.Fatal("no mail was sent")
+	}
+	body := c.sent[len(c.sent)-1].Text
+	match := recoveryLinkRe.FindStringSubmatch(body)
+	if match == nil {
+		t.Fatalf("no recovery link in message body:\n%s", body)
+	}
+	return match[1]
+}
+
+// recoveryLinkRe captures the path of a verification or reset link.
+var recoveryLinkRe = regexp.MustCompile(`https?://[^/\s]+(/(?:verify-email|reset)/[A-Za-z0-9_-]+)`)
+
 // Surface hostnames. All three are subdomains of browserHost() so a single
 // host-resolver rule (MAP *.<browserHost> <browserHost>) makes every surface
 // reachable from the browser at once — the Dispatcher then routes by Host header.
@@ -140,6 +181,7 @@ func newE2EServer(t *testing.T) *e2eServer {
 		0x2d, 0x69, 0x6e, 0x2d, 0x70, 0x72, 0x6f, 0x64,
 	}
 	idSvc, _ := identity.LoadOrCreate(ctx, st, masterKey)
+	sender := &captureSender{}
 
 	// Listen up front so the RP origins can include the concrete (dynamic) port.
 	ln, err := net.Listen("tcp", "0.0.0.0:0")
@@ -159,6 +201,9 @@ func newE2EServer(t *testing.T) *e2eServer {
 		RPOrigins: []string{origin(appHost()), origin(authHost()), origin(rootHost())},
 		AppHost:   appHost(), AuthHost: authHost(), RootHost: rootHost(),
 		Secure: true,
+		// Mail is reported as configured so the recovery UI is live, while nothing
+		// leaves the process. sender captures the links the tests follow.
+		Mail: sender, MailEnabled: true,
 	})
 	if err != nil {
 		t.Fatalf("auth: %v", err)
@@ -187,6 +232,7 @@ func newE2EServer(t *testing.T) *e2eServer {
 		appURL:  origin(appHost()),
 		authURL: origin(authHost()),
 		rootURL: origin(rootHost()),
+		mail:    sender,
 	}
 }
 
@@ -241,6 +287,41 @@ const (
 	stateCookieName   = "__Host-mt_state"
 )
 
+// setPassword gives an existing account a password, so tests can drive the
+// password-dependent flows (sign-in, recovery) without going through the sign-up
+// form. MinCost keeps it cheap — these tests aren't measuring bcrypt.
+func (e *e2eServer) setPassword(t *testing.T, userID int64, plaintext string) {
+	t.Helper()
+	hash, err := bcrypt.GenerateFromPassword([]byte(plaintext), bcrypt.MinCost)
+	if err != nil {
+		t.Fatalf("hash password: %v", err)
+	}
+	if err := e.store.SetPassword(e.ctx, userID, string(hash)); err != nil {
+		t.Fatalf("set password: %v", err)
+	}
+}
+
+// waitForLocation polls the browser's URL until it carries prefix.
+//
+// chromedp has no "wait for this navigation to settle" primitive, and a form submit
+// here can redirect more than once (auth host → handoff → app host). Waiting on an
+// element instead is a trap: any selector that already matches the current page
+// returns immediately, and a Navigate issued while a redirect is still in flight
+// fails with ERR_ABORTED.
+func waitForLocation(prefix string) chromedp.ActionFunc {
+	return func(ctx context.Context) error {
+		deadline := time.Now().Add(15 * time.Second)
+		var loc string
+		for time.Now().Before(deadline) {
+			if err := chromedp.Location(&loc).Do(ctx); err == nil && strings.HasPrefix(loc, prefix) {
+				return nil
+			}
+			time.Sleep(100 * time.Millisecond)
+		}
+		return fmt.Errorf("browser never reached %s (last location %q)", prefix, loc)
+	}
+}
+
 // newRepeater creates a repeater owned by ownerID with a valid MeshCore key.
 func (e *e2eServer) newRepeater(t *testing.T, ownerID int64, name string) *store.Repeater {
 	t.Helper()

internal/web/security.go

@@ -6,6 +6,7 @@ import (
 	"encoding/base64"
 	"log/slog"
 	"net/http"
+	"slices"
 	"strings"
 
 	"github.com/go-chi/chi/v5/middleware"
@@ -28,10 +29,46 @@ var cspDirectives = strings.Join([]string{
 	"font-src 'self' data:",
 	"frame-ancestors 'none'",
 	"base-uri 'self'",
-	"form-action 'self'",
 	"object-src 'none'",
+	// form-action is NOT here: it has to name the sibling surfaces, and their origins
+	// include the request's port, so it's built per request. See formAction.
 }, "; ")
 
+// formAction builds the form-action directive: 'self' plus the other two surface
+// origins.
+//
+// 'self' alone is wrong here, and silently so. A credential POST lands on the auth
+// host and answers 303 to the app host's handoff callback — and **Chrome enforces
+// form-action across the redirect chain**, not just on the initial request (the CSP
+// spec says it shouldn't, and Firefox doesn't, which is exactly why this survived).
+//
+// The symptom is why it went unnoticed for a month in production. The POST itself
+// arrives normally — the handler runs, the account is created, the session is set, and
+// the log shows a clean 303 — and then the browser refuses to follow that redirect,
+// reporting it only to the console. To the user the button simply does nothing, so it
+// reads as flakiness; to the server everything looks successful. Analytics for the
+// affected window shows the tell: five sign-up POSTs in six minutes, all 303, from
+// someone pressing a dead button.
+//
+// So this must list every surface a form on one host may redirect to. Origins are
+// computed from the request, since dev serves all three on a non-default port and a
+// source expression without a port only matches 443.
+func (e *Env) formAction(r *http.Request) string {
+	sources := []string{"'self'"}
+	if e.Cfg != nil {
+		for _, host := range []string{e.Cfg.PrimaryHost, e.Cfg.AuthHost, e.Cfg.RootHost} {
+			if host == "" {
+				continue
+			}
+			origin := originFor(e.Cfg, r, host)
+			if !slices.Contains(sources, origin) {
+				sources = append(sources, origin)
+			}
+		}
+	}
+	return "form-action " + strings.Join(sources, " ")
+}
+
 // permissionsPolicy denies powerful browser features we don't use and explicitly
 // allows the two we do: WebSerial (the KISS modem on the confirm/console pages)
 // and WebAuthn (passkeys). Unknown tokens are ignored by browsers that don't
@@ -47,7 +84,8 @@ func (e *Env) securityHeaders(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		nonce := newNonce()
 		h := w.Header()
-		policy := cspDirectives + "; script-src 'self' 'nonce-" + nonce + "'"
+		policy := cspDirectives + "; " + e.formAction(r) +
+			"; script-src 'self' 'nonce-" + nonce + "'"
 		if e.csp != nil {
 			// report-uri ONLY, deliberately — not the modern report-to /
 			// Reporting-Endpoints pair, despite report-uri being deprecated.
@@ -116,8 +154,9 @@ func unsafeMethod(method string) bool {
 //   - "cross-site" — rejected. This is the CSRF case: an attacker's page driving a
 //     write against us.
 //   - "same-origin" / "same-site" — allowed. Every form action and fetch() in the
-//     app is a relative path, and the CSP pins form-action to 'self', so real
-//     writes are same-origin. "same-site" is also allowed because it is exactly
+//     app is a relative path, and the CSP's form-action allows only this app's own
+//     surfaces (see formAction), so real writes are same-origin or between our own
+//     hosts. "same-site" is also allowed because it is exactly
 //     what Lax cookies already permit (sibling subdomains), so rejecting it would
 //     buy nothing this doesn't already concede.
 //   - "none" — allowed. It means the user initiated the request directly (address

internal/web/security_test.go

@@ -52,6 +52,50 @@ func TestSecurityHeadersCSPNonce(t *testing.T) {
 	}
 }
 
+// TestCSPFormActionAllowsSiblingSurfaces is the regression test for a bug that broke
+// password sign-in and sign-up in Chrome while every server-side test passed.
+//
+// Credential POSTs land on the auth host and answer 303 to the app host's handoff.
+// Chrome enforces form-action across the redirect chain (the spec says it shouldn't,
+// and Firefox doesn't), so `form-action 'self'` made the browser refuse to follow that
+// redirect. The POST still arrived and the handler still succeeded — the server logged a
+// clean 303 — so the only visible symptom was a button that did nothing, and nothing
+// server-side looked wrong at all.
+//
+// The port matters as much as the host: a source expression without one only matches
+// 443, so a dev deployment on :8080 needs the port present or it's blocked all over
+// again.
+func TestCSPFormActionAllowsSiblingSurfaces(t *testing.T) {
+	t.Parallel()
+	cfg := &config.Config{
+		PrimaryHost: "app.example.test",
+		AuthHost:    "auth.example.test",
+		RootHost:    "example.test",
+		Secure:      true,
+	}
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/login", nil)
+	req.Host = "auth.example.test:8443"
+	(&Env{Cfg: cfg}).securityHeaders(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})).
+		ServeHTTP(rec, req)
+
+	csp := rec.Header().Get("Content-Security-Policy")
+	for _, want := range []string{
+		"'self'",
+		"https://app.example.test:8443",  // the handoff target — the one that was blocked
+		"https://auth.example.test:8443", // where credential forms live
+		"https://example.test:8443",      // the root beacon
+	} {
+		if !strings.Contains(csp, want) {
+			t.Errorf("form-action missing %q: %q", want, csp)
+		}
+	}
+	// Still a closed list — a foreign origin must not be able to receive our forms.
+	if strings.Contains(csp, "form-action *") || strings.Contains(csp, "form-action 'unsafe") {
+		t.Errorf("form-action was widened to a wildcard: %q", csp)
+	}
+}
+
 func TestSecurityHeadersHSTSGatedOnTLS(t *testing.T) {
 	t.Parallel()
 	// No TLS (nil/insecure config): no HSTS.