diff --git a/internal/e2e/form_action_test.go b/internal/e2e/form_action_test.go new file mode 100644 index 0000000..b4e94eb --- /dev/null +++ b/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) +} diff --git a/internal/e2e/harness_test.go b/internal/e2e/harness_test.go index 150ac34..26e3bea 100644 --- a/internal/e2e/harness_test.go +++ b/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.:PORT — the product surface authURL string // https://auth.:PORT — sign-in + account rootURL string // https://root.: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 *. ) 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() diff --git a/internal/web/security.go b/internal/web/security.go index 8064141..f8a60c8 100644 --- a/internal/web/security.go +++ b/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 diff --git a/internal/web/security_test.go b/internal/web/security_test.go index d8f2af9..412116b 100644 --- a/internal/web/security_test.go +++ b/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.