Block cross-site writes via Sec-Fetch-Site

2026-09-02 01:38:17 +00:00 · 2026-07-29 20:02:13 -04:00
parent 2ad7a59af6
commit 080132ea36
3 changed files with 226 additions and 5 deletions
@@ -0,0 +1,153 @@
+package core
+
+import (
+	"net/http"
+	"net/http/cookiejar"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/jleight/meshtender/internal/store"
+)
+
+// postFetchSite issues a form POST carrying an explicit Sec-Fetch-Site header
+// (omitted entirely when fetchSite is ""), so a test can imitate what a browser
+// would report about the request's initiator.
+func postFetchSite(t *testing.T, ts *httptest.Server, host, path, fetchSite string, form url.Values, cookies ...*http.Cookie) *http.Response {
+	t.Helper()
+	req, err := http.NewRequest(http.MethodPost, ts.URL+path, strings.NewReader(form.Encode()))
+	if err != nil {
+		t.Fatalf("request: %v", err)
+	}
+	req.Host = host
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	if fetchSite != "" {
+		req.Header.Set("Sec-Fetch-Site", fetchSite)
+	}
+	for _, c := range cookies {
+		req.AddCookie(c)
+	}
+	resp, err := noRedirect().Do(req)
+	if err != nil {
+		t.Fatalf("post %s%s: %v", host, path, err)
+	}
+	return resp
+}
+
+// TestCrossSiteWritesBlocked covers the second layer of CSRF defense (the first
+// being the session cookie's SameSite=Lax):
+//   - a state-changing POST reporting Sec-Fetch-Site: cross-site is refused 403,
+//     and — the part that matters — its side effect does NOT happen
+//   - same-origin, same-site, none, and a missing header are all allowed through,
+//     so current browsers, sibling hosts, and pre-2020 / non-browser clients keep
+//     working
+//   - safe methods are never blocked, since cross-site GET navigation is normal
+//
+// Regression for audit finding S1.
+func TestCrossSiteWritesBlocked(t *testing.T) {
+	t.Parallel()
+
+	// signedIn spins up a server with an authenticated app-host session and returns
+	// what's needed to drive POSTs as that user. Each subtest gets its own so a
+	// successful logout in one can't affect another.
+	signedIn := func(t *testing.T) (*httptest.Server, hostEnv, *store.Store, []*http.Cookie) {
+		t.Helper()
+		st, ctx, ts, h := splitServer(t)
+		jar, err := cookiejar.New(nil)
+		if err != nil {
+			t.Fatalf("cookiejar: %v", err)
+		}
+		seedSession(t, ts, st, ctx, jar, "csrfuser-"+strings.ToLower(t.Name()[strings.LastIndex(t.Name(), "/")+1:]))
+		return ts, h, st, jar.Cookies(mustURL(t, ts.URL))
+	}
+
+	// Logout is the cleanest probe: it's a real state change (revokes the login row)
+	// whose effect is observable from a later request.
+	t.Run("cross-site POST is blocked and has no effect", func(t *testing.T) {
+		ts, h, _, cookies := signedIn(t)
+
+		// Confirm the session works before the attempt.
+		before := do(t, ts, h.app, "/repeaters", cookies...)
+		before.Body.Close()
+		if before.StatusCode != http.StatusOK {
+			t.Fatalf("precondition: /repeaters = %d, want 200", before.StatusCode)
+		}
+
+		resp := postFetchSite(t, ts, h.app, "/logout", "cross-site", url.Values{}, cookies...)
+		resp.Body.Close()
+		if resp.StatusCode != http.StatusForbidden {
+			t.Fatalf("cross-site POST /logout = %d, want 403", resp.StatusCode)
+		}
+
+		// The whole point: the logout must not have happened.
+		after := do(t, ts, h.app, "/repeaters", cookies...)
+		after.Body.Close()
+		if after.StatusCode != http.StatusOK {
+			t.Fatalf("session was destroyed by a blocked cross-site logout: /repeaters = %d, want 200",
+				after.StatusCode)
+		}
+	})
+
+	// Everything a legitimate client can report must still get through. A successful
+	// logout answers 303 to the root host.
+	for _, tc := range []struct{ name, fetchSite string }{
+		{"same-origin", "same-origin"},
+		{"same-site", "same-site"},
+		{"none", "none"},
+		{"missing header (old or non-browser client)", ""},
+		{"unrecognized value", "future-value"},
+	} {
+		t.Run("allowed: "+tc.name, func(t *testing.T) {
+			ts, h, _, cookies := signedIn(t)
+			resp := postFetchSite(t, ts, h.app, "/logout", tc.fetchSite, url.Values{}, cookies...)
+			resp.Body.Close()
+			if resp.StatusCode != http.StatusSeeOther {
+				t.Fatalf("POST /logout with Sec-Fetch-Site=%q = %d, want 303", tc.fetchSite, resp.StatusCode)
+			}
+		})
+	}
+
+	// Case shouldn't matter; browsers send lowercase but don't rely on it.
+	t.Run("blocking is case-insensitive", func(t *testing.T) {
+		ts, h, _, cookies := signedIn(t)
+		resp := postFetchSite(t, ts, h.app, "/logout", "Cross-Site", url.Values{}, cookies...)
+		resp.Body.Close()
+		if resp.StatusCode != http.StatusForbidden {
+			t.Fatalf("POST /logout with Sec-Fetch-Site=Cross-Site = %d, want 403", resp.StatusCode)
+		}
+	})
+
+	// The check must apply on every surface, not just the app host.
+	t.Run("applies to the auth host", func(t *testing.T) {
+		_, _, ts, h := splitServer(t)
+		resp := postFetchSite(t, ts, h.auth, "/login/password", "cross-site",
+			url.Values{"username": {"someone"}, "password": {"whatever"}})
+		resp.Body.Close()
+		if resp.StatusCode != http.StatusForbidden {
+			t.Fatalf("cross-site POST /login/password = %d, want 403", resp.StatusCode)
+		}
+	})
+
+	// Cross-site GET navigation is how people arrive from a link — never blocked.
+	t.Run("safe methods are not blocked cross-site", func(t *testing.T) {
+		_, _, ts, h := splitServer(t)
+		for _, page := range []struct{ host, path string }{
+			{h.root, "/"},
+			{h.root, "/orgs"},
+			{h.auth, "/login"},
+		} {
+			req, _ := http.NewRequest(http.MethodGet, ts.URL+page.path, nil)
+			req.Host = page.host
+			req.Header.Set("Sec-Fetch-Site", "cross-site")
+			resp, err := noRedirect().Do(req)
+			if err != nil {
+				t.Fatalf("get %s: %v", page.path, err)
+			}
+			resp.Body.Close()
+			if resp.StatusCode != http.StatusOK {
+				t.Errorf("cross-site GET %s%s = %d, want 200", page.host, page.path, resp.StatusCode)
+			}
+		}
+	})
+}
@@ -352,11 +352,12 @@ func (rn *Renderer) render(w http.ResponseWriter, r *http.Request, status int, p
 // auth-free. chi requires all Use() calls before any route is registered.
 func (e *Env) CommonMiddleware(r chi.Router) {
 	r.Use(middleware.RequestID)
-	r.Use(CaptureRemoteAddr) // preserve the true TCP peer before we resolve
-	r.Use(e.resolveClientIP) // trusted-proxy-aware X-Forwarded-For resolution
-	r.Use(e.securityHeaders) // CSP (+ per-request script nonce) and hardening headers
-	r.Use(limitBody)         // cap request bodies before any handler reads them
-	r.Use(compressHTML)      // gzip the server-rendered pages
+	r.Use(CaptureRemoteAddr)    // preserve the true TCP peer before we resolve
+	r.Use(e.resolveClientIP)    // trusted-proxy-aware X-Forwarded-For resolution
+	r.Use(e.securityHeaders)    // CSP (+ per-request script nonce) and hardening headers
+	r.Use(blockCrossSiteWrites) // CSRF second layer, before any handler reads the body
+	r.Use(limitBody)            // cap request bodies before any handler reads them
+	r.Use(compressHTML)         // gzip the server-rendered pages
 	r.Use(middleware.Recoverer)
 }

@@ -4,8 +4,11 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/base64"
+	"log/slog"
 	"net/http"
 	"strings"
+
+	"github.com/go-chi/chi/v5/middleware"
 )

 // nonceCtxKey keys the per-request CSP nonce in the request context.
@@ -59,6 +62,70 @@ func (e *Env) securityHeaders(next http.Handler) http.Handler {
 	})
 }

+// unsafeMethod reports whether a method can change server state. The safe set is
+// closed (per RFC 9110), so anything unrecognized — including a method added by a
+// future router — counts as unsafe and gets checked.
+func unsafeMethod(method string) bool {
+	switch method {
+	case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace:
+		return false
+	}
+	return true
+}
+
+// blockCrossSiteWrites rejects state-changing requests that the browser tells us
+// came from another site. It is the second layer of CSRF defense: the first is the
+// session cookie's SameSite=Lax, which browsers use to withhold the cookie from a
+// cross-site POST. Lax is sound but it is a *single* control, and it is the wrong
+// shape for two failure modes — a browser or embedded webview that mishandles
+// SameSite re-opens every mutation, and a state-changing GET (easy to add by
+// accident) is forgeable outright, because Lax deliberately permits top-level GET
+// navigation.
+//
+// Sec-Fetch-Site is set by the browser and cannot be forged by page JavaScript (it
+// is a forbidden header name), so it is trustworthy when present. Values are
+// handled as follows:
+//
+//   - "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
+//     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
+//     bar, bookmark), which an attacker cannot arrange; rejecting it would add no
+//     security and risk odd client behavior.
+//   - missing / unrecognized — allowed. Pre-2020 browsers and non-browser clients
+//     send nothing, and this is defense in depth layered on SameSite, not a
+//     replacement for it. Failing closed here would break those clients for no
+//     gain against an attacker, who cannot suppress the header in a real browser.
+//
+// Rejections are logged at Warn: this is a new control, and if it ever fires on
+// legitimate traffic we want to find out from the logs rather than a bug report.
+//
+// Note this is a header check by design, which is what makes it cheap. The
+// alternative — a synchronizer token in every form — would put a secret in an HTML
+// body, and HTML is compressed (see compressHTML), which is the BREACH
+// precondition. Keeping the check in headers sidesteps that entirely.
+func blockCrossSiteWrites(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if unsafeMethod(r.Method) && strings.EqualFold(r.Header.Get("Sec-Fetch-Site"), "cross-site") {
+			slog.Warn("blocked cross-site write",
+				"method", r.Method,
+				"path", r.URL.Path,
+				"host", r.Host,
+				"request_id", middleware.GetReqID(r.Context()),
+				"client_ip", clientIP(r),
+			)
+			http.Error(w, "This request appears to have come from another site and was blocked.",
+				http.StatusForbidden)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}
+
 // NoStore marks a response as never-cacheable. Without it a response carrying no
 // Cache-Control and no Expires is *heuristically* cacheable, so the browser's
 // back button re-renders a signed-in page after sign-out — on a shared machine