From ede9467aeff49bfa6205b12e16914dac560eb790 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sat, 8 Aug 2026 22:14:13 +0300 Subject: [PATCH] csrf: the shared same-origin guard Five services carried their own copy of the Origin/Referer check and the copies had drifted, not only in wording: tokens, cover and bench apply it as router-wide middleware, while dolt and specs call a predicate from individual handlers, so a form added to either of those goes out unguarded. Require is therefore the API this package leads with, and SameOrigin is the escape hatch for a service migrating one route at a time. --- csrf/csrf.go | 230 +++++++++++++++++++++++++ csrf/csrf_test.go | 424 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 654 insertions(+) create mode 100644 csrf/csrf.go create mode 100644 csrf/csrf_test.go diff --git a/csrf/csrf.go b/csrf/csrf.go new file mode 100644 index 0000000000000000000000000000000000000000..172961eeb0e4e63ec4460af0df2581c6c2324b2f --- /dev/null +++ b/csrf/csrf.go @@ -0,0 +1,230 @@ +// Package csrf is the same-origin guard that stands in front of the forms of +// every custom service on a self-hosted SourceHut instance (compare, spec, +// dolt, cover, bench, ...), and is the one copy of that rule. +// +// Before this package each service carried its own: tokens.sr.ht and bench and +// cover as a router-wide middleware, dolt as a predicate three handlers +// remember to call, spec as a predicate one handler pair calls. The copies had +// already drifted — byte-equal versus case-insensitive host comparison, four +// different refusal sentences, and, in the two services that check per handler, +// a form added later that nobody guards at all. This is a security rule, so the +// interesting drift is not the wording: it is that in two of five services the +// default for a new POST route is *unprotected*. Hence Require, the middleware, +// is the API this package leads with, and SameOrigin exists mainly so a service +// mid-migration is not forced to move its whole router in one commit. +// +// # Why a header check and not a token +// +// There is no CSRF token on any of these services and nowhere to keep one. +// Identity is meta.sr.ht's unified-login cookie, set on the parent domain: no +// individual service issues it, and none of them can set its SameSite +// attribute. A synchronizer-token scheme would mean every daemon inventing a +// session store of its own for the sake of two forms. +// +// What is left is what the browser itself says about where the request came +// from. Origin — and Referer, when Origin is absent — are set by the user agent +// and cannot be forged from script across origins, which is exactly the +// attacker this defends against: a page on another site submitting a form at us +// with the viewer's cookie attached. +// +// # The rule +// +// Read Origin; failing that, read Referer; compare scheme and host against the +// service's own origin; and refuse a request that carries neither. +// +// That last clause is the one a reader is tempted to relax, and it is the one +// holding the guard up. A request that will not say where it came from cannot be +// shown to have come from us. Our own forms are same-origin, every engine has +// sent Origin on a form POST for years, and a request arriving with neither +// header is a script, a stripped proxy or a hand-rolled client — none of which +// is the browser this check exists to protect. Waving it through would reduce +// the whole guard to a header an attacker's page simply omits. +// +// Only scheme and host are compared, and the host comparison includes the port, +// because that is what an origin is (RFC 6454 §4): a page served from :8443 is +// not this origin whatever its hostname says, and http:// is not https:// +// whatever the host says. The path a Referer carries is ignored — a Referer is +// a whole URL and any page of ours is an acceptable referrer for our own form. +// The comparison is case-insensitive in both components, since RFC 3986 §3.1 +// and §3.2.2 say they are: browsers send them lower-cased, but the service's own +// origin comes from a config.ini line a human typed, and an operator who wrote +// https://Bench.Example.org would otherwise get a service whose every form +// answers 403 with nothing in the logs to explain it. +// +// Safe methods are exempt for RFC 9110 §9.2.1's reason — they change nothing — +// and because every read on these surfaces has to keep working from a bookmark, +// a README's , a probe on /healthz and a curl with no headers at all. +// +// # Usage +// +// The middleware is the one to reach for, on the whole router: +// +// r.Use(csrf.Require(selfOrigin, func(w http.ResponseWriter, r *http.Request) { +// s.renderError(w, r, http.StatusForbidden, csrf.Message) +// })) +// +// Installed there rather than per handler, the guard covers the routes that are +// not written yet as well as the ones that are: a POST added to the table below +// is protected by having been registered, which is the only form of "do not +// forget" that survives a year. Install it after whatever sets the cache +// headers, so the refusal page carries the same private, no-store every other +// answer of the surface does. It also runs before routing, so a POST to an +// address the surface does not serve is refused rather than 404'd — the right +// way round, since an unrouted POST answering differently from a routed one +// would be a way to enumerate which of them exist without ever passing the +// check. +// +// A service whose bearer-token API lives in its own mux keeps that exemption by +// not mounting this middleware there, which is a fact about where Mount is +// called and not a path test this package could get wrong. There is deliberately +// no "is this /api?" option here and there must never be one: every escape, +// every case fold and every dot segment a client can spell would then be a way +// to ask for the exemption. +// +// selfOrigin is the service's own configured origin, e.g. +// config.GetOrigin(conf, "bench.sr.ht", true). It is parsed once, when the +// middleware is built. If it is not a URL with a scheme and a host, every +// mutating request is refused — the failure is closed, because the alternative +// is a guard that compares against nothing and admits everyone. Services +// validate their origin at startup, so this is unreachable in practice; it is +// written down because it is the arm nobody would notice in production except +// as "all forms 403". +package csrf + +import ( + "net/http" + "net/url" + "strings" +) + +// Message is what a refused mutation tells the viewer, shared so the five +// services answer the same sentence. +// +// It is deliberately not the services' ownership or visibility refusal: this +// says nothing about whether the viewer may do the thing — they may very well +// own the object — only that there is no evidence they asked for it. A human +// who meets it has usually reached the form through something that stripped the +// header, which is worth saying out loud. +const Message = "That request did not come from this site, so it was not carried out." + +// Require builds the middleware that refuses a mutating request which cannot +// show it came from selfOrigin. +// +// deny renders the refusal; it is the service's own error page, so that the +// refusal looks like the rest of the surface. It should answer 403 and not 400 +// — the request is well-formed and the viewer may well be logged in, what is +// missing is evidence that they asked for this — and it must not redirect: a +// redirect after a POST drops the body and would silently turn a refused +// mutation into a page that looks like it worked. A nil deny answers a plain +// text 403 carrying Message, which is a usable default rather than an invitation +// to skip the middleware. +// +// The returned value has the shape every net/http middleware chain expects, +// including chi's Use. +func Require(selfOrigin string, deny http.HandlerFunc) func(http.Handler) http.Handler { + // Parsed once here rather than per request: the origin is process-wide + // configuration, and a middleware built against a broken one should not pay + // to rediscover that on every POST. ok == false means every mutating request + // is refused; see the package comment. + self, ok := parseSelf(selfOrigin) + if deny == nil { + deny = denyPlain + } + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if SafeMethod(r.Method) || (ok && claimMatches(r, self)) { + next.ServeHTTP(w, r) + return + } + deny(w, r) + }) + } +} + +// SameOrigin reports whether r names selfOrigin in its Origin header — or, +// failing that, in its Referer. +// +// This is the predicate behind Require, exported for the handler that cannot +// yet sit behind the middleware: a route mounted outside the guarded router, or +// a service migrating one handler at a time. Prefer Require. A predicate is +// protection somebody has to remember, and the services this package was +// extracted from are the proof: the two that check per handler are the two +// where a form added later would go out unguarded. +// +// It does not consult the method — a caller reaching for it has already decided +// the request mutates. Pair it with SafeMethod if that is not true. +func SameOrigin(r *http.Request, selfOrigin string) bool { + self, ok := parseSelf(selfOrigin) + if !ok { + return false + } + return claimMatches(r, self) +} + +// SafeMethod reports whether a method may not change state and is therefore +// exempt (RFC 9110 §9.2.1). Everything else — POST today, a PUT, PATCH or +// DELETE tomorrow — is checked, which is the direction this list has to fail +// in: an unknown method is guarded, not waved through. +// +// TRACE is on the list because RFC 9110 defines it as safe and because net/http +// neither routes nor echoes it by default; leaving it off would have made this +// list disagree with the four donors for no gain. +func SafeMethod(method string) bool { + switch method { + case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace: + return true + default: + return false + } +} + +// claimMatches applies the rule to a request whose own origin has already +// parsed. +// +// Origin is consulted first and, when present, alone: a browser that sends both +// means them to agree, and treating Referer as a second chance after Origin has +// already said "somewhere else" would turn the stronger statement into the +// weaker one. +func claimMatches(r *http.Request, self *url.URL) bool { + if origin := r.Header.Get("Origin"); origin != "" { + return originMatches(origin, self) + } + if referer := r.Header.Get("Referer"); referer != "" { + return originMatches(referer, self) + } + // Neither header: refuse rather than assume same-origin. + return false +} + +// originMatches reports whether raw — a whole URL, which is what both headers +// carry — has self's scheme and host. +// +// A value that does not parse matches nothing, and so does one that parses to +// no scheme or no host: the "null" that a sandboxed iframe or a form redirected +// across origins posts is the everyday example, and it must not be mistaken for +// "no header", which is refused anyway. +func originMatches(raw string, self *url.URL) bool { + u, err := url.Parse(raw) + if err != nil { + return false + } + return strings.EqualFold(u.Scheme, self.Scheme) && strings.EqualFold(u.Host, self.Host) +} + +// parseSelf parses the service's own origin, reporting false for anything that +// could not be an origin. Both components are required: a host with no scheme +// would compare equal to a protocol-relative "//host/..." claim, and a value +// with no host would compare equal to every claim that also has none. +func parseSelf(selfOrigin string) (*url.URL, bool) { + u, err := url.Parse(selfOrigin) + if err != nil || u.Scheme == "" || u.Host == "" { + return nil, false + } + return u, true +} + +// denyPlain is the refusal Require uses when the caller supplies none: the +// shared sentence, as text, with the status the services' own error pages use. +func denyPlain(w http.ResponseWriter, _ *http.Request) { + http.Error(w, Message, http.StatusForbidden) +} diff --git a/csrf/csrf_test.go b/csrf/csrf_test.go new file mode 100644 index 0000000000000000000000000000000000000000..2a2b46c6ebd42e868e8f8f072107c5495eaccc00 --- /dev/null +++ b/csrf/csrf_test.go @@ -0,0 +1,424 @@ +package csrf + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// selfOrigin is the origin under test throughout: an https service on the +// default port, spelled the way a config.ini line would be. +const selfOrigin = "https://bench.example.org" + +// request builds a POST carrying the given headers; a header with an empty +// value is not set at all, which is how a request that omits it looks to +// Header.Get. +func request(method string, headers map[string]string) *http.Request { + r := httptest.NewRequest(method, "/settings", nil) + for k, v := range headers { + if v != "" { + r.Header.Set(k, v) + } + } + return r +} + +func TestSameOrigin(t *testing.T) { + tests := []struct { + name string + self string + origin string + referer string + want bool + }{ + { + name: "matching Origin", + self: selfOrigin, + origin: "https://bench.example.org", + want: true, + }, + { + name: "matching Origin differing only in case", + self: "https://Bench.Example.org", + origin: "https://bench.example.org", + want: true, + // The operator types the config line; the browser sends the + // lower-cased form. Byte equality would 403 every form on this + // instance with nothing in the logs to explain it. + }, + { + name: "mismatching Origin", + self: selfOrigin, + origin: "https://evil.example.com", + want: false, + }, + { + name: "Origin whose host is a suffix of ours", + self: selfOrigin, + origin: "https://notbench.example.org", + want: false, + }, + { + name: "Origin that only prefixes ours", + self: selfOrigin, + origin: "https://bench.example.org.evil.com", + want: false, + }, + { + name: "scheme mismatch", + self: selfOrigin, + origin: "http://bench.example.org", + want: false, + }, + { + name: "port mismatch", + self: selfOrigin, + origin: "https://bench.example.org:8443", + want: false, + // The port is part of the origin (RFC 6454 §4): a page served from + // :8443 is not this origin whatever its hostname says. + }, + { + name: "our port spelled out where ours has none", + self: selfOrigin, + origin: "https://bench.example.org:443", + want: false, + // Refused deliberately: this compares hosts, not effective ports. + // Browsers elide the default port, so a real request never looks + // like this, and normalising it would mean teaching this package + // every scheme's default. + }, + { + name: "matching Referer with no Origin", + self: selfOrigin, + referer: "https://bench.example.org/~bigbes/foo", + want: true, + // The path is ignored: any page of ours may refer to our own form. + }, + { + name: "matching Referer, bare origin with no path", + self: selfOrigin, + referer: "https://bench.example.org", + want: true, + }, + { + name: "mismatching Referer", + self: selfOrigin, + referer: "https://evil.example.com/attack.html", + want: false, + }, + { + name: "neither header", + self: selfOrigin, + want: false, + // The clause the whole guard rests on. A request that will not say + // where it came from cannot be shown to have come from us, and + // admitting it would reduce the check to a header an attacker's + // page simply omits. + }, + { + name: "mismatching Origin beats a matching Referer", + self: selfOrigin, + origin: "https://evil.example.com", + referer: "https://bench.example.org/~bigbes/foo", + want: false, + // Referer is the fallback for an absent Origin, not a second chance + // after Origin has already said "somewhere else". + }, + { + name: "matching Origin outvotes a foreign Referer", + self: selfOrigin, + origin: "https://bench.example.org", + referer: "https://evil.example.com/attack.html", + want: true, + }, + { + name: "the null origin of a sandboxed iframe", + self: selfOrigin, + origin: "null", + want: false, + }, + { + name: "malformed Origin", + self: selfOrigin, + origin: "http://[::1", + want: false, + }, + { + name: "Origin that is not a URL at all", + self: selfOrigin, + origin: "not a url", + want: false, + }, + { + name: "protocol-relative Origin naming our host", + self: selfOrigin, + origin: "//bench.example.org", + want: false, + // Parses to our host with no scheme. Refused, because the guard + // compares schemes too and the empty one is not https. + }, + { + name: "malformed own origin", + self: "http://[::1", + origin: "http://[::1", + want: false, + // Fails closed: a broken config line refuses every mutation rather + // than making every claim match a nil comparison. + }, + { + name: "own origin with no scheme", + self: "bench.example.org", + origin: "https://bench.example.org", + want: false, + }, + { + name: "empty own origin", + self: "", + origin: "https://bench.example.org", + want: false, + }, + { + name: "empty own origin and no headers", + self: "", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := request(http.MethodPost, map[string]string{ + "Origin": tt.origin, + "Referer": tt.referer, + }) + assert.Equal(t, tt.want, SameOrigin(r, tt.self)) + }) + } +} + +// SameOrigin says nothing about the method — a caller reaching for the +// predicate has already decided the request mutates. +func TestSameOriginIgnoresTheMethod(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodDelete} { + r := request(method, nil) + assert.False(t, SameOrigin(r, selfOrigin), "%s with no headers", method) + } +} + +func TestSafeMethod(t *testing.T) { + tests := []struct { + method string + want bool + }{ + {http.MethodGet, true}, + {http.MethodHead, true}, + {http.MethodOptions, true}, + {http.MethodTrace, true}, + {http.MethodPost, false}, + {http.MethodPut, false}, + {http.MethodPatch, false}, + {http.MethodDelete, false}, + {http.MethodConnect, false}, + {"PROPFIND", false}, + {"", false}, + {"get", false}, // methods are case-sensitive; an unknown one is guarded + } + + for _, tt := range tests { + t.Run(tt.method, func(t *testing.T) { + assert.Equal(t, tt.want, SafeMethod(tt.method)) + }) + } +} + +func TestRequire(t *testing.T) { + tests := []struct { + name string + self string + method string + origin string + referer string + wantServed bool + }{ + { + name: "matching Origin on a POST", + self: selfOrigin, + method: http.MethodPost, + origin: "https://bench.example.org", + wantServed: true, + }, + { + name: "mismatching Origin on a POST", + self: selfOrigin, + method: http.MethodPost, + origin: "https://evil.example.com", + wantServed: false, + }, + { + name: "matching Referer with no Origin", + self: selfOrigin, + method: http.MethodPost, + referer: "https://bench.example.org/settings", + wantServed: true, + }, + { + name: "mismatching Referer", + self: selfOrigin, + method: http.MethodPost, + referer: "https://evil.example.com/attack.html", + wantServed: false, + }, + { + name: "POST with neither header", + self: selfOrigin, + method: http.MethodPost, + wantServed: false, + }, + { + name: "DELETE with neither header", + self: selfOrigin, + method: http.MethodDelete, + wantServed: false, + // The methods nobody has written a route for yet are guarded by + // default; that is the whole reason this is a middleware. + }, + { + name: "PUT with a matching Origin", + self: selfOrigin, + method: http.MethodPut, + origin: "https://bench.example.org", + wantServed: true, + }, + { + name: "GET with no headers at all", + self: selfOrigin, + method: http.MethodGet, + wantServed: true, + // A bookmark, a README's , a probe on /healthz. + }, + { + name: "HEAD from another site", + self: selfOrigin, + method: http.MethodHead, + origin: "https://evil.example.com", + wantServed: true, + }, + { + name: "OPTIONS preflight from another site", + self: selfOrigin, + method: http.MethodOptions, + origin: "https://evil.example.com", + wantServed: true, + }, + { + name: "malformed Origin on a POST", + self: selfOrigin, + method: http.MethodPost, + origin: "http://[::1", + wantServed: false, + }, + { + name: "broken own origin refuses a matching POST", + self: "not-an-origin", + method: http.MethodPost, + origin: "https://bench.example.org", + wantServed: false, + }, + { + name: "broken own origin still serves a GET", + self: "not-an-origin", + method: http.MethodGet, + wantServed: true, + // Fail-closed applies to mutations. A misconfigured origin must not + // take the whole read surface down with it. + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var served, denied bool + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + served = true + w.WriteHeader(http.StatusNoContent) + }) + deny := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + denied = true + http.Error(w, Message, http.StatusForbidden) + }) + + w := httptest.NewRecorder() + Require(tt.self, deny)(next).ServeHTTP(w, request(tt.method, map[string]string{ + "Origin": tt.origin, + "Referer": tt.referer, + })) + + assert.Equal(t, tt.wantServed, served, "handler reached") + assert.Equal(t, !tt.wantServed, denied, "refusal rendered") + if tt.wantServed { + assert.Equal(t, http.StatusNoContent, w.Code) + } else { + assert.Equal(t, http.StatusForbidden, w.Code) + } + }) + } +} + +// A nil deny is a usable default rather than a reason to skip the middleware. +func TestRequireDefaultRefusal(t *testing.T) { + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("handler must not be reached") + }) + + w := httptest.NewRecorder() + Require(selfOrigin, nil)(next).ServeHTTP(w, request(http.MethodPost, nil)) + + require.Equal(t, http.StatusForbidden, w.Code) + assert.Equal(t, Message, strings.TrimSpace(w.Body.String())) +} + +// The refused request's body must not reach the handler, and the refusal must +// not be a redirect: a redirect after a POST drops the body and turns a refused +// mutation into a page that looks like it worked. +func TestRequireRefusalIsNotARedirect(t *testing.T) { + w := httptest.NewRecorder() + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("handler must not be reached") + }) + r := httptest.NewRequest(http.MethodPost, "/settings", strings.NewReader("name=x")) + r.Header.Set("Origin", "https://evil.example.com") + + Require(selfOrigin, nil)(next).ServeHTTP(w, r) + + require.Equal(t, http.StatusForbidden, w.Code) + assert.Empty(t, w.Header().Get("Location")) +} + +// The guard runs before routing, so a mutating request to an address the +// service does not serve is refused rather than answered 404 — an unrouted POST +// that answered differently would enumerate which routes exist without ever +// passing the check. +func TestRequireRunsBeforeRouting(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/settings", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + guarded := Require(selfOrigin, nil)(mux) + + for _, target := range []string{"/settings", "/no/such/route"} { + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, target, nil) + r.Header.Set("Origin", "https://evil.example.com") + guarded.ServeHTTP(w, r) + assert.Equal(t, http.StatusForbidden, w.Code, "POST %s", target) + } +} + +// Message is part of the package's contract: the five services answer the same +// sentence, and a service's own error page renders it verbatim. +func TestMessageIsShared(t *testing.T) { + assert.Equal(t, "That request did not come from this site, so it was not carried out.", Message) +}