diff --git a/internal/core/crosssite_test.go b/internal/core/crosssite_test.go new file mode 100644 index 0000000..0a9c34b --- /dev/null +++ b/internal/core/crosssite_test.go @@ -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) + } + } + }) +} diff --git a/internal/web/env.go b/internal/web/env.go index 54609fb..b19bce4 100644 --- a/internal/web/env.go +++ b/internal/web/env.go @@ -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) } diff --git a/internal/web/security.go b/internal/web/security.go index 3d45b50..a23a440 100644 --- a/internal/web/security.go +++ b/internal/web/security.go @@ -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