From 36b975ff5b01b314e4c18b8e3c5b2c26a22eccfd Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 10:51:26 -0400 Subject: [PATCH] Feature 03 task 4: CSRF protection and GET /api/v1/me (#22) Co-authored-by: dev_agent Co-committed-by: dev_agent --- internal/auth/actor.go | 28 ++++-- internal/auth/actor_test.go | 16 +++- internal/server/csrf.go | 42 ++++++++ internal/server/csrf_test.go | 165 ++++++++++++++++++++++++++++++++ internal/server/me.go | 52 ++++++++++ internal/server/server.go | 7 +- internal/server/server_test.go | 4 +- internal/server/session.go | 6 +- internal/server/session_test.go | 9 +- 9 files changed, 306 insertions(+), 23 deletions(-) create mode 100644 internal/server/csrf.go create mode 100644 internal/server/csrf_test.go create mode 100644 internal/server/me.go diff --git a/internal/auth/actor.go b/internal/auth/actor.go index ac65b84..c781091 100644 --- a/internal/auth/actor.go +++ b/internal/auth/actor.go @@ -19,17 +19,27 @@ func (s Session) Actor() Actor { return Actor{Subject: s.Subject, Email: s.Email, Name: s.Name, Groups: slices.Clone(s.Groups)} } -type actorKey struct{} +type sessionKey struct{} -// WithActor returns a copy of ctx carrying actor. The session middleware -// calls it for every authenticated request. -func WithActor(ctx context.Context, actor Actor) context.Context { - return context.WithValue(ctx, actorKey{}, actor) +// WithSession returns a copy of ctx carrying s. The session middleware calls +// it for every authenticated request. +func WithSession(ctx context.Context, s Session) context.Context { + return context.WithValue(ctx, sessionKey{}, s) } -// ActorFrom returns the actor WithActor stored in ctx. ok is false when ctx -// has none, which behind the session middleware means a wiring bug. +// SessionFrom returns the session WithSession stored in ctx. ok is false when +// ctx has none, which behind the session middleware means a wiring bug. +func SessionFrom(ctx context.Context) (s Session, ok bool) { + s, ok = ctx.Value(sessionKey{}).(Session) + return s, ok +} + +// ActorFrom returns the actor of the session WithSession stored in ctx. ok is +// false when ctx has none. func ActorFrom(ctx context.Context) (actor Actor, ok bool) { - actor, ok = ctx.Value(actorKey{}).(Actor) - return actor, ok + s, ok := SessionFrom(ctx) + if !ok { + return Actor{}, false + } + return s.Actor(), true } diff --git a/internal/auth/actor_test.go b/internal/auth/actor_test.go index c2f239e..fff57cb 100644 --- a/internal/auth/actor_test.go +++ b/internal/auth/actor_test.go @@ -11,10 +11,18 @@ func TestActorContext(t *testing.T) { if _, ok := ActorFrom(context.Background()); ok { t.Error("ActorFrom found an actor in an empty context") } - want := Actor{Subject: "user-123", Email: "cook@example.com", Name: "Home Cook", Groups: []string{"mealquest-admin"}} - got, ok := ActorFrom(WithActor(context.Background(), want)) - if !ok || got.Subject != want.Subject || got.Email != want.Email || got.Name != want.Name || !slices.Equal(got.Groups, want.Groups) { - t.Errorf("ActorFrom = %+v (ok=%v), want %+v", got, ok, want) + if _, ok := SessionFrom(context.Background()); ok { + t.Error("SessionFrom found a session in an empty context") + } + s := Session{Subject: "user-123", Email: "cook@example.com", Name: "Home Cook", Groups: []string{"mealquest-admin"}, CSRFToken: "t"} + ctx := WithSession(context.Background(), s) + gotSession, ok := SessionFrom(ctx) + if !ok || gotSession.Subject != s.Subject || gotSession.CSRFToken != s.CSRFToken { + t.Errorf("SessionFrom = %+v (ok=%v), want %+v", gotSession, ok, s) + } + got, ok := ActorFrom(ctx) + if !ok || got.Subject != s.Subject || got.Email != s.Email || got.Name != s.Name || !slices.Equal(got.Groups, s.Groups) { + t.Errorf("ActorFrom = %+v (ok=%v), want the session's actor", got, ok) } } diff --git a/internal/server/csrf.go b/internal/server/csrf.go new file mode 100644 index 0000000..3f58ff2 --- /dev/null +++ b/internal/server/csrf.go @@ -0,0 +1,42 @@ +package server + +import ( + "crypto/subtle" + "log/slog" + "net/http" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" +) + +// CSRFHeader carries the session's CSRF token, which the client reads from +// GET /api/v1/me. +const CSRFHeader = "X-MealQuest-CSRF" + +// requireCSRF passes GET, HEAD and OPTIONS requests to next; every other +// request must carry CSRFHeader equal to its session's CSRF token or get 403 +// csrf. It runs behind requireSession, which puts the session in the context. +func requireCSRF(logger *slog.Logger) func(http.Handler) http.Handler { + forbidden := apiErrorHandler(logger, http.StatusForbidden, "csrf", "missing or invalid CSRF token") + internal := apiErrorHandler(logger, http.StatusInternalServerError, "internal", "internal error") + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions: + next.ServeHTTP(w, r) + return + } + s, ok := auth.SessionFrom(r.Context()) + if !ok || s.CSRFToken == "" { + // An empty token would match a missing header. + logger.ErrorContext(r.Context(), "csrf check without a session token", "session", ok) + internal(w, r) + return + } + if subtle.ConstantTimeCompare([]byte(r.Header.Get(CSRFHeader)), []byte(s.CSRFToken)) != 1 { + forbidden(w, r) + return + } + next.ServeHTTP(w, r) + }) + } +} diff --git a/internal/server/csrf_test.go b/internal/server/csrf_test.go new file mode 100644 index 0000000..e509c8e --- /dev/null +++ b/internal/server/csrf_test.go @@ -0,0 +1,165 @@ +package server + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" +) + +const ( + csrfJSON = `{"error":{"code":"csrf","message":"missing or invalid CSRF token","retryable":false}}` + methodNotAllowedJSON = `{"error":{"code":"method_not_allowed","message":"method not allowed","retryable":false}}` +) + +func TestCSRFThroughRouter(t *testing.T) { + t.Parallel() + + router := newTestRouter(t, "dev") + for _, tc := range []struct { + name, method, path string + header []string // nil sends no CSRF header + wantStatus int + wantBody string + }{ + {"POST without header", http.MethodPost, "/api/v1/me", nil, http.StatusForbidden, csrfJSON}, + {"POST with wrong header", http.MethodPost, "/api/v1/me", []string{"wrong-token"}, http.StatusForbidden, csrfJSON}, + {"POST with empty header", http.MethodPost, "/api/v1/me", []string{""}, http.StatusForbidden, csrfJSON}, + {"POST with token prefix", http.MethodPost, "/api/v1/me", []string{"csrf-token"}, http.StatusForbidden, csrfJSON}, + {"PATCH without header", http.MethodPatch, "/api/v1/settings/household", nil, http.StatusForbidden, csrfJSON}, + {"DELETE with wrong header", http.MethodDelete, "/api/v1/me", []string{"wrong-token"}, http.StatusForbidden, csrfJSON}, + // The right token passes the check and reaches the router, which has + // no POST /api/v1/me. + {"POST with right header", http.MethodPost, "/api/v1/me", []string{testSession.CSRFToken}, http.StatusMethodNotAllowed, methodNotAllowedJSON}, + {"PATCH with right header", http.MethodPatch, "/api/v1/settings/household", []string{testSession.CSRFToken}, http.StatusNotFound, notFoundJSON}, + {"GET without header", http.MethodGet, "/api/v1/me", nil, http.StatusOK, ""}, + // HEAD and OPTIONS pass the check and reach the router, which + // answers neither for /api/v1/me. + {"HEAD without header", http.MethodHead, "/api/v1/me", nil, http.StatusMethodNotAllowed, ""}, + {"OPTIONS without header", http.MethodOptions, "/api/v1/me", nil, http.StatusMethodNotAllowed, methodNotAllowedJSON}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + req := httptest.NewRequest(tc.method, tc.path, nil) + req.AddCookie(validSessionCookie) + if tc.header != nil { + req.Header.Set(CSRFHeader, tc.header[0]) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != tc.wantStatus { + t.Errorf("status = %d, want %d (body %s)", rec.Code, tc.wantStatus, rec.Body.String()) + } + if tc.wantBody != "" && rec.Body.String() != tc.wantBody { + t.Errorf("body = %s, want %s", rec.Body.String(), tc.wantBody) + } + }) + } +} + +func TestCSRFUnauthenticatedGets401(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodPost, "/api/v1/me", nil) + rec := httptest.NewRecorder() + newTestRouter(t, "dev").ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized || rec.Body.String() != unauthenticatedJSON { + t.Errorf("response = %d %s, want 401 %s", rec.Code, rec.Body.String(), unauthenticatedJSON) + } +} + +func TestRequireCSRF(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + for _, tc := range []struct { + name string + method string + session *auth.Session + header string + wantStatus int + }{ + {"right header reaches handler", http.MethodPost, &testSession, testSession.CSRFToken, http.StatusNoContent}, + {"wrong header", http.MethodPut, &testSession, "nope", http.StatusForbidden}, + {"GET needs no header", http.MethodGet, &testSession, "", http.StatusNoContent}, + {"no session in context", http.MethodPost, nil, "", http.StatusInternalServerError}, + {"session without token", http.MethodPost, &auth.Session{Subject: "user-123"}, "", http.StatusInternalServerError}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + called := false + h := requireCSRF(logger)(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + called = true + w.WriteHeader(http.StatusNoContent) + })) + ctx := context.Background() + if tc.session != nil { + ctx = auth.WithSession(ctx, *tc.session) + } + req := httptest.NewRequestWithContext(ctx, tc.method, "/api/v1/anything", nil) + if tc.header != "" { + req.Header.Set(CSRFHeader, tc.header) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != tc.wantStatus { + t.Errorf("status = %d, want %d", rec.Code, tc.wantStatus) + } + if called != (tc.wantStatus == http.StatusNoContent) { + t.Errorf("handler called = %v with status %d", called, rec.Code) + } + }) + } +} + +func TestMe(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodGet, "/api/v1/me", nil) + req.AddCookie(validSessionCookie) + rec := httptest.NewRecorder() + newTestRouter(t, "dev").ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", rec.Code, rec.Body.String()) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if cc := rec.Header().Get("Cache-Control"); cc != "no-store" { + t.Errorf("Cache-Control = %q, want no-store", cc) + } + var got map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode %s: %v", rec.Body.String(), err) + } + want := map[string]any{ + "name": "Home Cook", + "email": "cook@example.com", + "household": map[string]any{"label": "Our kitchen"}, + "csrfToken": "csrf-token-123", + } + gotJSON, _ := json.Marshal(got) + wantJSON, _ := json.Marshal(want) + if string(gotJSON) != string(wantJSON) { + t.Errorf("body = %s, want %s", gotJSON, wantJSON) + } +} + +func TestMeNeedsSession(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + newTestRouter(t, "dev").ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/v1/me", nil)) + if rec.Code != http.StatusUnauthorized || rec.Body.String() != unauthenticatedJSON { + t.Errorf("response = %d %s, want 401 %s", rec.Code, rec.Body.String(), unauthenticatedJSON) + } +} diff --git a/internal/server/me.go b/internal/server/me.go new file mode 100644 index 0000000..e9a8a26 --- /dev/null +++ b/internal/server/me.go @@ -0,0 +1,52 @@ +package server + +import ( + "encoding/json" + "log/slog" + "net/http" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" +) + +// householdLabel is the single shared household identity. +const householdLabel = "Our kitchen" + +type meResponse struct { + Name string `json:"name"` + Email string `json:"email"` + Household householdInfo `json:"household"` + CSRFToken string `json:"csrfToken"` +} + +type householdInfo struct { + Label string `json:"label"` +} + +// me answers GET /api/v1/me with the signed-in member, the household and the +// session's CSRF token for CSRFHeader. +func me(logger *slog.Logger) http.HandlerFunc { + internal := apiErrorHandler(logger, http.StatusInternalServerError, "internal", "internal error") + return func(w http.ResponseWriter, r *http.Request) { + s, ok := auth.SessionFrom(r.Context()) + if !ok { + logger.ErrorContext(r.Context(), "GET /api/v1/me without a session") + internal(w, r) + return + } + body, err := json.Marshal(meResponse{ + Name: s.Name, + Email: s.Email, + Household: householdInfo{Label: householdLabel}, + CSRFToken: s.CSRFToken, + }) + if err != nil { + panic(err) // string fields cannot fail to marshal + } + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Cache-Control", "no-store") + w.WriteHeader(http.StatusOK) + if _, err := w.Write(body); err != nil { + logger.WarnContext(r.Context(), "write me response", "error", err) + } + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 4ae887b..fe336a2 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -24,7 +24,8 @@ type RouteMounter interface { // NewRouter returns the HTTP handler for the service. version is reported by // GET /health; auth mounts the /auth routes; sessions guards /api/v1 and // /media; assets is the web build served for every other GET. /health, /auth -// and the web build are public. +// and the web build are public. Requests to /api/v1 other than GET, HEAD and +// OPTIONS also need the session's CSRF token in CSRFHeader. func NewRouter(logger *slog.Logger, version string, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) { notFound := apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found") methodNotAllowed := apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed") @@ -44,15 +45,17 @@ func NewRouter(logger *slog.Logger, version string, auth RouteMounter, sessions // rather than being added with Use: chi skips a subrouter's middleware // when it has no routes, which would leave it unguarded. signedIn := requireSession(logger, sessions) + csrf := requireCSRF(logger) apiV1, media := chi.NewRouter(), chi.NewRouter() for _, sub := range []*chi.Mux{apiV1, media} { sub.MethodNotAllowed(methodNotAllowed) sub.NotFound(notFound) } + apiV1.Get("/me", me(logger)) r.Route("/api", func(r chi.Router) { r.MethodNotAllowed(methodNotAllowed) r.NotFound(notFound) - r.Mount("/v1", signedIn(apiV1)) + r.Mount("/v1", signedIn(csrf(apiV1))) }) r.Mount("/media", signedIn(media)) // Catch-all for the PWA. Unknown /auth paths reach the frontend handler, diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 2b2cee1..05d877f 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -139,9 +139,11 @@ func TestReservedPathsNotFound(t *testing.T) { t.Run(tt.method+" "+tt.path, func(t *testing.T) { t.Parallel() - // Signed in, so /api/v1 and /media reach their routers. + // Signed in with the CSRF token, so /api/v1 and /media reach + // their routers. req := httptest.NewRequest(tt.method, tt.path, nil) req.AddCookie(validSessionCookie) + req.Header.Set(CSRFHeader, testSession.CSRFToken) rec := httptest.NewRecorder() router.ServeHTTP(rec, req) diff --git a/internal/server/session.go b/internal/server/session.go index ade1f51..e718389 100644 --- a/internal/server/session.go +++ b/internal/server/session.go @@ -15,8 +15,8 @@ type SessionLookup interface { } // requireSession is the RequireSession middleware. It passes requests with a -// live session to next with the session's actor in the context (read it with -// auth.ActorFrom); others get 401 unauthenticated. +// live session to next with the session in the context (read it with +// auth.SessionFrom or auth.ActorFrom); others get 401 unauthenticated. func requireSession(logger *slog.Logger, sessions SessionLookup) func(http.Handler) http.Handler { unauthenticated := apiErrorHandler(logger, http.StatusUnauthorized, "unauthenticated", "sign in required") internal := apiErrorHandler(logger, http.StatusInternalServerError, "internal", "internal error") @@ -30,7 +30,7 @@ func requireSession(logger *slog.Logger, sessions SessionLookup) func(http.Handl logger.ErrorContext(r.Context(), "look up session", "error", err) internal(w, r) default: - next.ServeHTTP(w, r.WithContext(auth.WithActor(r.Context(), s.Actor()))) + next.ServeHTTP(w, r.WithContext(auth.WithSession(r.Context(), s))) } }) } diff --git a/internal/server/session_test.go b/internal/server/session_test.go index a8b4ac0..d221b4c 100644 --- a/internal/server/session_test.go +++ b/internal/server/session_test.go @@ -21,10 +21,11 @@ var ( validSessionCookie = &http.Cookie{Name: auth.SessionCookieName, Value: "valid"} brokenSessionCookie = &http.Cookie{Name: auth.SessionCookieName, Value: "broken"} testSession = auth.Session{ - Subject: "user-123", - Email: "cook@example.com", - Name: "Home Cook", - Groups: []string{"mealquest-admin"}, + Subject: "user-123", + Email: "cook@example.com", + Name: "Home Cook", + Groups: []string{"mealquest-admin"}, + CSRFToken: "csrf-token-123", } )