From e04928c9954cfe97811816cdbe87ab8a36db0a06 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 9 Aug 2026 00:22:19 +0300 Subject: [PATCH] login: take the unified-login cookie decode from ecore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit authn's CookieName, the fernet decrypt, the auth.AuthCookie unmarshal and the empty-name check were one of six copies of the same decode on this instance. They are now sr-ht-ecore/login.UsernameFromRequest; what stays here is the half that is ours, turning that name into a row in our user table. Two things the local copy did not do. It passed the cookie's name through with a leading '~' still on it, which meta answers for nobody, and it validated nothing at all — a name went from an attacker-supplied cookie straight into a GraphQL query and a log line. Both are now login's, and the middleware's own rule is unchanged: every failure is anonymity, so public browsing and public clones keep working. --- authn/authn_test.go | 7 +++ authn/cookie.go | 64 +++++++++---------- authn/cookie_test.go | 143 +++++++++++++++++++++++++------------------ 3 files changed, 122 insertions(+), 92 deletions(-) diff --git a/authn/authn_test.go b/authn/authn_test.go index 698112318197335e9dada0593a7ec61983ac1d40..5d972ab96390be517cc34f52552a2b712d9320ef 100644 --- a/authn/authn_test.go +++ b/authn/authn_test.go @@ -48,9 +48,16 @@ type stubBackend struct { revoked map[[64]byte]bool lookupErr error revokeErr error + + // lookedUp records every name LookupUser was asked for, in order. It is how + // a test asserts that a name was refused *before* it reached the backend — + // which is what a validator buys, and which an unresolved caller alone + // cannot distinguish from a name the backend merely did not know. + lookedUp []string } func (s *stubBackend) LookupUser(ctx context.Context, username string, out *auth.AuthContext) error { + s.lookedUp = append(s.lookedUp, username) if s.lookupErr != nil { return s.lookupErr } diff --git a/authn/cookie.go b/authn/cookie.go index a4edfaebd1bdd155bc2ccf73cc717d04161ff8d1..0ee82e38f5b3920b6e9f7eab3377f02fa79eb918 100644 --- a/authn/cookie.go +++ b/authn/cookie.go @@ -1,33 +1,32 @@ package authn import ( - "encoding/json" "log/slog" "net/http" "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-core/auth" - "sourcecraft.dev/bigbes/sr-ht-core/crypto" -) -// CookieName is the unified-login cookie shared across every SourceHut service. -const CookieName = "sr.ht.unified-login.v1" + "sourcecraft.dev/bigbes/sr-ht-ecore/login" +) -// OptionalCookieMiddleware reads the sr.ht.unified-login.v1 cookie and, when it -// is present and valid, attaches the resolved caller to the request context -// (retrievable with CallerFromContext). It NEVER rejects a request: a missing, -// malformed, undecryptable, or unresolvable cookie leaves the request +// OptionalCookieMiddleware reads the unified-login cookie and, when it names a +// user this service can resolve, attaches the resolved caller to the request +// context (retrievable with CallerFromContext). It NEVER rejects a request: a +// missing, malformed, undecryptable, or unresolvable cookie leaves the request // anonymous. This is what allows public browsing and public clones to work // without credentials — unlike core-go's auth.Middleware, which 401s any // request lacking a cookie or Authorization header. // -// It replicates the logic of core-go's unexported cookieAuth: decrypt the -// fernet-sealed cookie with crypto.DecryptWithoutExpiration, decode the -// {"name": ...} payload, and resolve the user via the meta backend (core-go's -// auth.LookupUser path). Suspended users are resolved normally; the suspension -// flag is carried on the caller (via AsCoreCaller) and gates writes at the -// access-control layer rather than being rejected here. +// The two halves of that sentence are two packages, and the split is the point. +// Decoding the cookie is instance-wide — one session, one seal, one grammar for +// the name inside it — and lives in sr-ht-ecore's login. Turning the name into a +// row is dolt.sr.ht's alone: our user table, our mirror-on-first-sight, our +// answer for a user meta has but we have never seen. Only the second half is +// here, which is also why this is not simply login.Optional: what the rest of +// the service reads out of the context is an *auth.AuthContext with a UserID, +// not a username. // // Requires crypto.InitCrypto to have run (server.New does this at startup) and, // for the user lookup, config.Middleware + database.Middleware installed @@ -46,31 +45,28 @@ func OptionalCookieMiddleware() func(http.Handler) http.Handler { // resolveCookie returns the caller authenticated by the request's unified-login // cookie, or nil if there is no cookie or it cannot be resolved for any reason. // Every failure path returns nil (anonymous) — none is fatal. +// +// Suspended users are resolved normally; the suspension flag is carried on the +// caller (via AsCoreCaller) and gates writes at the access-control layer rather +// than being rejected here. func resolveCookie(r *http.Request) *auth.AuthContext { - cookie, err := r.Cookie(CookieName) - if err != nil { - return nil // no cookie: anonymous - } - - payload := crypto.DecryptWithoutExpiration([]byte(cookie.Value)) - if payload == nil { - return nil // bad/forged/undecryptable cookie: anonymous - } - - var authCookie auth.AuthCookie - if err := json.Unmarshal(payload, &authCookie); err != nil { - return nil // malformed payload: anonymous - } - if authCookie.Name == "" { - return nil // no username in payload: anonymous + // No cookie, a forged one, a payload that is not core-go's JSON, or a name + // that could not be an account name: all "" and all anonymous. Nothing is + // logged, because the cookie value is attacker-supplied and arrives on every + // request — a warning per bad decode is a log flood anyone can turn on. + username := login.UsernameFromRequest(r) + if username == "" { + return nil } var ac auth.AuthContext - if err := meta.LookupUser(r.Context(), authCookie.Name, &ac); err != nil { + if err := meta.LookupUser(r.Context(), username, &ac); err != nil { // meta/database unreachable or unknown user: degrade to anonymous - // rather than failing the request (browsing must keep working). + // rather than failing the request (browsing must keep working). This + // one *is* logged: the name has already passed login's grammar, so it + // is bounded text, and an unreachable meta is an operator's problem. slog.WarnContext(r.Context(), "resolving the login cookie's user failed", - "component", "authn", "username", authCookie.Name, scribe.Err(err)) + "component", "authn", "username", username, scribe.Err(err)) return nil } ac.AuthMethod = auth.AUTH_COOKIE diff --git a/authn/cookie_test.go b/authn/cookie_test.go index 04a322be3b02d571cf07e6dadea13b23339478a8..776b79abd24473d4495debac1115b2f8571f5d18 100644 --- a/authn/cookie_test.go +++ b/authn/cookie_test.go @@ -5,7 +5,12 @@ import ( "net/http/httptest" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sourcecraft.dev/bigbes/sr-ht-core/auth" + + "sourcecraft.dev/bigbes/sr-ht-ecore/login" ) // captureHandler records the caller present on the request context when reached. @@ -30,44 +35,48 @@ func runCookieMiddleware(t *testing.T, cookie *http.Cookie) (*auth.AuthContext, rec := httptest.NewRecorder() h.ServeHTTP(rec, req) - if !reached { - t.Fatal("middleware must always call next (never rejects)") - } + require.True(t, reached, "middleware must always call next (never rejects)") return got, rec.Code } -func TestOptionalCookieMiddleware_NoCookie(t *testing.T) { - withStubBackend(t, &stubBackend{users: map[string]auth.AuthContext{}}) - got, code := runCookieMiddleware(t, nil) - if got != nil { - t.Fatalf("expected anonymous, got %+v", got) - } - if code != http.StatusOK { - t.Fatalf("expected 200, got %d", code) - } -} - func TestOptionalCookieMiddleware_ValidCookie(t *testing.T) { withStubBackend(t, &stubBackend{users: map[string]auth.AuthContext{ "bigbes": sampleUser(1, "bigbes", auth.USER_TYPE_USER), }}) got, code := runCookieMiddleware(t, &http.Cookie{ - Name: CookieName, + Name: login.CookieName, Value: forgeCookie(t, "bigbes"), }) - if code != http.StatusOK { - t.Fatalf("expected 200, got %d", code) - } - if got == nil { - t.Fatal("expected an authenticated caller") - } - if got.Username != "bigbes" || got.UserID != 1 { - t.Fatalf("wrong caller resolved: %+v", got) - } - if got.AuthMethod != auth.AUTH_COOKIE { - t.Fatalf("AuthMethod = %q, want %q", got.AuthMethod, auth.AUTH_COOKIE) - } + require.Equal(t, http.StatusOK, code) + require.NotNil(t, got, "expected an authenticated caller") + assert.Equal(t, "bigbes", got.Username) + assert.Equal(t, 1, got.UserID) + assert.Equal(t, auth.AUTH_COOKIE, got.AuthMethod) +} + +// TestOptionalCookieMiddleware_StripsCanonicalSigil pins a difference from the +// decode this package used to carry: a payload holding the canonical "~name" +// form resolves to the bare name rather than being looked up with the sigil +// still on it — which meta would answer for nobody. +func TestOptionalCookieMiddleware_StripsCanonicalSigil(t *testing.T) { + stub := &stubBackend{users: map[string]auth.AuthContext{ + "bigbes": sampleUser(1, "bigbes", auth.USER_TYPE_USER), + }} + withStubBackend(t, stub) + + got, code := runCookieMiddleware(t, &http.Cookie{ + Name: login.CookieName, + Value: forgeCookie(t, "~bigbes"), + }) + require.Equal(t, http.StatusOK, code) + require.NotNil(t, got) + assert.Equal(t, "bigbes", got.Username) + // The backend is asked for the bare name, not "~bigbes". Asserting on the + // resolved caller alone would not show this: the stub normalises the sigil + // away itself, as meta.LookupUser does, so both spellings resolve either + // way and only the recorded argument says which one was sent. + assert.Equal(t, []string{"bigbes"}, stub.lookedUp) } func TestOptionalCookieMiddleware_SuspendedStillResolves(t *testing.T) { @@ -75,55 +84,73 @@ func TestOptionalCookieMiddleware_SuspendedStillResolves(t *testing.T) { "susp": sampleUser(2, "susp", auth.USER_TYPE_SUSPENDED), }}) got, _ := runCookieMiddleware(t, &http.Cookie{ - Name: CookieName, + Name: login.CookieName, Value: forgeCookie(t, "susp"), }) - if got == nil { - t.Fatal("suspended users must still resolve (reads are allowed)") - } - if !AsCoreCaller(got).Suspended { - t.Fatal("resolved caller must be flagged suspended") - } + require.NotNil(t, got, "suspended users must still resolve (reads are allowed)") + assert.True(t, AsCoreCaller(got).Suspended, "resolved caller must be flagged suspended") } -func TestOptionalCookieMiddleware_GarbageCookie(t *testing.T) { - withStubBackend(t, &stubBackend{users: map[string]auth.AuthContext{}}) - got, code := runCookieMiddleware(t, &http.Cookie{ - Name: CookieName, - Value: "not-a-valid-fernet-token", - }) - if got != nil { - t.Fatalf("garbage cookie must degrade to anonymous, got %+v", got) +// TestOptionalCookieMiddleware_UnusableCookieIsAnonymous is this package's own +// contract rather than login's: whatever the decode answers, the middleware +// resolves nobody and still calls next. It is what keeps public browsing and +// public clones working when anything about the session goes wrong. +// +// The name cases are the second thing this package used not to do. A cookie +// naming "../../etc/passwd" or an empty string decoded to that string and went +// straight to the user lookup; now it never reaches the backend at all, and the +// assertion below is that it did not. +func TestOptionalCookieMiddleware_UnusableCookieIsAnonymous(t *testing.T) { + cases := []struct { + name string + cookie func(t *testing.T) *http.Cookie + }{ + {"no cookie at all", func(*testing.T) *http.Cookie { return nil }}, + {"undecryptable value", func(*testing.T) *http.Cookie { + return &http.Cookie{Name: login.CookieName, Value: "not-a-valid-fernet-token"} + }}, + {"empty name in payload", func(t *testing.T) *http.Cookie { + return &http.Cookie{Name: login.CookieName, Value: forgeCookie(t, "")} + }}, + {"path separator in name", func(t *testing.T) *http.Cookie { + return &http.Cookie{Name: login.CookieName, Value: forgeCookie(t, "../../etc/passwd")} + }}, + {"newline in name", func(t *testing.T) *http.Cookie { + return &http.Cookie{Name: login.CookieName, Value: forgeCookie(t, "alice\nWARN forged")} + }}, } - if code != http.StatusOK { - t.Fatalf("must not reject: expected 200, got %d", code) + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + stub := &stubBackend{users: map[string]auth.AuthContext{ + "bigbes": sampleUser(1, "bigbes", auth.USER_TYPE_USER), + }} + withStubBackend(t, stub) + + got, code := runCookieMiddleware(t, c.cookie(t)) + assert.Nil(t, got, "must degrade to anonymous") + assert.Equal(t, http.StatusOK, code, "must not reject") + assert.Empty(t, stub.lookedUp, "an unusable cookie must not reach the user lookup") + }) } } func TestOptionalCookieMiddleware_UnknownUser(t *testing.T) { withStubBackend(t, &stubBackend{users: map[string]auth.AuthContext{}}) got, code := runCookieMiddleware(t, &http.Cookie{ - Name: CookieName, + Name: login.CookieName, Value: forgeCookie(t, "ghost"), }) - if got != nil { - t.Fatalf("unknown user must degrade to anonymous, got %+v", got) - } - if code != http.StatusOK { - t.Fatalf("must not reject: expected 200, got %d", code) - } + assert.Nil(t, got, "unknown user must degrade to anonymous") + assert.Equal(t, http.StatusOK, code, "must not reject") } func TestOptionalCookieMiddleware_BackendDownIsAnonymous(t *testing.T) { withStubBackend(t, &stubBackend{lookupErr: errBackendDown}) got, code := runCookieMiddleware(t, &http.Cookie{ - Name: CookieName, + Name: login.CookieName, Value: forgeCookie(t, "bigbes"), }) - if got != nil { - t.Fatalf("backend failure must degrade to anonymous, got %+v", got) - } - if code != http.StatusOK { - t.Fatalf("must not 500: expected 200, got %d", code) - } + assert.Nil(t, got, "backend failure must degrade to anonymous") + assert.Equal(t, http.StatusOK, code, "must not 500") }