From 4d5d1f674ecdeb8ce8acdfc7a8c91dc7b509a313 Mon Sep 17 00:00:00 2001 From: Claus Lohmar Date: Wed, 14 Oct 2026 11:30:13 +0000 Subject: [PATCH] fix(security): gate /setup per request and pin the setup module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW-001 (M0) HIGH H2: loopback was enforced only at BIND, so a reverse proxy on 127.0.0.1 relaying remote clients reached /setup and could create the top admin — exactly the first-visitor-wins D-025 claims to close "in code". In setup mode, reject any request whose RESOLVED client IP is not loopback (404 before dispatch); an unparseable/empty client IP fails closed. The resolved IP is the one XFF produces, so a trusted proxy forwarding a remote client does not launder it (see the new test). Also: - M1c/L3s: pin the setup module at the door. serveSetupAction rejects Module != "setup", and any other resolution naming it (a GUID path, a reserved workflow) 404s instead of falling through to serveGUID. - L1s: read/clear the session cookie under its transport-correct name. Tests: the harness speaks from a loopback peer by default (the operator reaches /setup over a tunnel) and configures a public base_url where the nudge must fire (L7c). New tests cover the per-request setup gate (including the trusted-proxy relay and the loopback tunnel) and the module pin, both of which fail without the fix. --- internal/core/server/server.go | 34 ++++- internal/core/server/server_test.go | 118 +++++++++++++++++- internal/core/server/setup_wizard_test.go | 5 +- .../core/server/spec006_passkey_nudge_test.go | 19 ++- internal/core/server/spec007_recovery_test.go | 12 +- 5 files changed, 180 insertions(+), 8 deletions(-) diff --git a/internal/core/server/server.go b/internal/core/server/server.go index d343b49..ac707b7 100644 --- a/internal/core/server/server.go +++ b/internal/core/server/server.go @@ -253,6 +253,18 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { // dispatch is the Kind switch (SPEC-003 §4 step 5). func (s *Server) dispatch(w http.ResponseWriter, r *http.Request, res routing.Resolution, cid, ip string) { + // SPEC-003 §7 / D-025 / A13 (H2): setup mode is loopback-only PER REQUEST. + // The bind-time check (`Serve`/`CheckSetupBind`) is necessary but not + // sufficient: a reverse proxy on 127.0.0.1 relaying remote clients makes + // /setup reachable past it, which is exactly the first-visitor-wins the + // design claims to close. An unparseable/empty client IP fails closed. + if s.setup() && !httpx.IsLoopbackIP(ip) { + s.log.Warn("setup request from a non-loopback client rejected", + "correlation_id", cid, "client_ip", ip) + s.writeError(w, http.StatusNotFound, "not found") + return + } + // SPEC-005 §3: in setup mode the app serves ONLY /setup (+ static assets); // every other route is 404 (delete, don't redirect). Reserved names like // /home and /access are 404'd too — the bootstrap has no app yet. @@ -300,6 +312,15 @@ func (s *Server) dispatch(w http.ResponseWriter, r *http.Request, res routing.Re s.serveSetupAction(w, r, res, cid, ip) return } + // M1c/L3s — pin the setup MODULE at the door: the wizard's functions are + // reachable ONLY through the reserved `setup` workflow. Any other + // resolution naming `do=setup` (a GUID path, a reserved workflow) is + // invalid in every mode and 404s rather than falling through to the + // module registry or the GUID door. + if res.Module == setupModule { + s.writeError(w, http.StatusNotFound, "not found") + return + } // SPEC-006: the `auth` module's actions are the enroll/login flows. // They run BEFORE the session gate (the whole point is to obtain a // session); logout is the exception and stays session-gated. @@ -395,6 +416,13 @@ func (s *Server) serveSetupAction(w http.ResponseWriter, r *http.Request, res ro s.writeError(w, http.StatusNotFound, "not found") return } + // M1c: the door pins the action door to the `setup` module. Without this a + // `do=core&action=modules.toggle` under /setup would reach a core action + // that the session-gated role gate (admin-only) would otherwise protect. + if res.Module != setupModule { + s.writeError(w, http.StatusNotFound, "not found") + return + } if s.precsrf == nil { s.writeError(w, http.StatusNotFound, "not found") return @@ -438,7 +466,9 @@ func (s *Server) serveGUID(w http.ResponseWriter, r *http.Request, res routing.R ctx := r.Context() // (a) load the session cookie; an absent or empty id is rejected (A2). - cookie, err := r.Cookie(httpx.SessionCookieName) + // Over https the cookie name carries the `__Host-` prefix (L1s). + cookieName := httpx.SessionCookieNameFor(s.secure) + cookie, err := r.Cookie(cookieName) if err != nil || cookie.Value == "" { s.loginRequired(w, r, res, cid, ip) return @@ -447,7 +477,7 @@ func (s *Server) serveGUID(w http.ResponseWriter, r *http.Request, res routing.R // Validate: exists · not expired · not revoked · non-empty (A2/AC-5). sess, err := s.sessions.Validate(ctx, cookie.Value) if err != nil { - httpx.ClearSessionCookie(w, httpx.SessionCookieName, s.secure) + httpx.ClearSessionCookie(w, cookieName, s.secure) s.loginRequired(w, r, res, cid, ip) return } diff --git a/internal/core/server/server_test.go b/internal/core/server/server_test.go index 8b602ec..0324469 100644 --- a/internal/core/server/server_test.go +++ b/internal/core/server/server_test.go @@ -186,8 +186,11 @@ func newHarness(t *testing.T, opts ...func(*server.Deps)) *harness { } cfg := &config.Config{ - Setup: false, - Server: config.Server{Listen: "127.0.0.1:8080", BaseURL: "http://127.0.0.1:8080"}, + Setup: false, + Server: config.Server{Listen: "127.0.0.1:8080", BaseURL: "http://example.com"}, + // BaseURL is a PUBLIC origin on purpose: the passkey nudge only fires + // off-loopback (SPEC-006 §8), and L7c makes the CONFIGURED origin the + // signal — so a test that wants loopback must configure it. Database: config.Database{Path: db.Path()}, Secrets: config.Secrets{CSRFKey: csrfKey, CookieSecret: cookieSecret}, } @@ -221,6 +224,9 @@ func newHarness(t *testing.T, opts ...func(*server.Deps)) *harness { CSRF: deps.PreSession, Audit: aud, Logger: deps.Logger, + // L7c: the nudge's origin signal is the CONFIGURED base_url, not the + // (proxy-rewritable) request Host. + BaseURL: deps.Config.Server.BaseURL, // The passkey nudge (SPEC-006 §8) is suppressed in setup mode; the // door's SetupMode is the same predicate (nil = nudge enabled). SuppressNudge: deps.SetupMode, @@ -261,9 +267,15 @@ func (h *harness) cookie() *http.Cookie { return &http.Cookie{Name: httpx.SessionCookieName, Value: h.session.ID} } +// loopbackPeer is the RemoteAddr the test door speaks from. Setup mode is +// loopback-only PER REQUEST (SPEC-003 §7, H2), so tests must explicitly opt in +// to a non-loopback peer to exercise the rejection. +const loopbackPeer = "127.0.0.1:52345" + // request issues a request directly against the door (no network). func request(h http.Handler, method, target string, cookies ...*http.Cookie) *httptest.ResponseRecorder { req := httptest.NewRequest(method, target, nil) + req.RemoteAddr = loopbackPeer // the operator reaches /setup over a tunnel for _, c := range cookies { req.AddCookie(c) } @@ -588,6 +600,108 @@ func TestSetupModeRouting(t *testing.T) { } } +// TestSpec003SetupIsLoopbackOnlyPerRequest is H2: the bind-time check is not +// enough — a reverse proxy on 127.0.0.1 relaying a remote client must not make +// /setup reachable (first-visitor-wins). +func TestSpec003SetupIsLoopbackOnlyPerRequest(t *testing.T) { + h := newHarness(t, func(d *server.Deps) { d.SetupMode = func() bool { return true } }) + + // A remote client → 404 before dispatch, on every setup route. + for _, tc := range []struct{ method, target string }{ + {"GET", "/setup"}, + {"POST", "/setup?do=setup&action=domain.save"}, + {"GET", "/static/app.css"}, + } { + req := httptest.NewRequest(tc.method, tc.target, nil) + req.RemoteAddr = "203.0.113.9:4444" // a remote peer through the proxy + rec := httptest.NewRecorder() + h.srv.ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("%s %s from a non-loopback client = %d, want 404 (H2)", tc.method, tc.target, rec.Code) + } + } + + // The tunnel (a loopback peer) still works. + if rec := request(h.srv, "GET", "/setup"); rec.Code != http.StatusOK { + t.Errorf("GET /setup from loopback = %d, want 200 (H2 must not break the tunnel)", rec.Code) + } + + // A trusted proxy forwarding X-Forwarded-For does NOT launder a remote + // client into loopback: the resolved client IP is what is checked. + h2 := newHarness(t, func(d *server.Deps) { + d.SetupMode = func() bool { return true } + d.Config.Server.TrustedProxies = []string{"127.0.0.1"} + }) + req := httptest.NewRequest("GET", "/setup", nil) + req.RemoteAddr = "127.0.0.1:9999" // the proxy itself is local... + req.Header.Set("X-Forwarded-For", "203.0.113.9") + rec := httptest.NewRecorder() + h2.srv.ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("GET /setup relayed by a trusted proxy = %d, want 404 (H2: the RESOLVED client is remote)", rec.Code) + } +} + +// TestSpec003SetupModulePinnedAtTheDoor covers M1c/L3s: the wizard's module is +// reachable ONLY through the reserved `setup` workflow. The registry mounts a +// `setup` module and a `core` action module, so a missing pin would actually +// dispatch them (that is what makes the assertion meaningful). +func TestSpec003SetupModulePinnedAtTheDoor(t *testing.T) { + ok := func(msg string) server.Handler { + return func(w http.ResponseWriter, _ *http.Request, _ *server.RequestContext) error { + w.WriteHeader(http.StatusOK) + _, err := w.Write([]byte(msg)) + return err + } + } + registry := func() *server.MapRegistry { + reg := server.NewMapRegistry() + reg.RegisterCore("home", server.Home) + reg.RegisterModule(server.NewModule("home"). + SetDefault("dashboard", server.Home). + AddRoute("home", "home", server.Home)) + reg.RegisterModule(server.NewModule("setup"). + AddAction("admin.create", "admin.create", ok("created"))) + reg.RegisterModule(server.NewModule("core"). + AddAction("modules.toggle", "modules.toggle", ok("toggled"))) + return reg + } + + // In setup mode: `do=core` under /setup must not reach the core action, + // even with a valid pre-session CSRF token (M1c). + inSetup := newHarness(t, func(d *server.Deps) { + d.Registry = registry() + d.SetupMode = func() bool { return true } + }) + cookie, token := inSetup.preCSRF() + form := url.Values{"csrf": {token}}.Encode() + req := httptest.NewRequest("POST", "/setup?do=core&action=modules.toggle", strings.NewReader(form)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.RemoteAddr = loopbackPeer + req.AddCookie(cookie) + rec := httptest.NewRecorder() + inSetup.srv.ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("do=core under /setup in setup mode = %d, want 404 (M1c)", rec.Code) + } + + // In productive mode: a GUID action naming the setup module must 404 rather + // than fall through to serveGUID and dispatch it (L3s). + productive := newHarness(t, func(d *server.Deps) { d.Registry = registry() }) + target := "/" + productive.guid + "?do=setup&action=admin.create" + token2 := url.QueryEscape(productive.csrf.Token(productive.session.ID)) + rec2 := request(productive.srv, "POST", target+"&csrf="+token2, productive.cookie()) + if rec2.Code != http.StatusNotFound { + t.Errorf("POST %s = %d, want 404 (L3s: the setup module is pinned to the setup workflow)", target, rec2.Code) + } + + // And under a reserved level-1 workflow (not just a GUID). + rec3 := request(productive.srv, "POST", "/home?do=setup&action=admin.create&csrf="+token2) + if rec3.Code != http.StatusNotFound { + t.Errorf("do=setup under /home = %d, want 404 (L3s)", rec3.Code) + } +} + // TestEnrollRouting: a pending identity routes to the enrollment screen. func TestEnrollRouting(t *testing.T) { h := newHarness(t) diff --git a/internal/core/server/setup_wizard_test.go b/internal/core/server/setup_wizard_test.go index ab14504..a62ec60 100644 --- a/internal/core/server/setup_wizard_test.go +++ b/internal/core/server/setup_wizard_test.go @@ -198,6 +198,9 @@ func (h *setupHarness) post(action string, form url.Values, cookies ...*http.Coo req := httptest.NewRequest("POST", "/setup?do=setup&action="+url.QueryEscape(action), strings.NewReader(form.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // The wizard is loopback-only per request (SPEC-003 §7, H2): the operator + // reaches it over an SSH tunnel, so the tests speak from loopback. + req.RemoteAddr = loopbackPeer req.AddCookie(cookie) for _, c := range cookies { req.AddCookie(c) @@ -283,7 +286,6 @@ func TestSpec005AC1_SetupModeServesOnlySetup(t *testing.T) { func TestR6_WizardGeneratesStrongSecrets(t *testing.T) { h := newSetupHarness(t, func(h *setupHarness) { h.cfg.Secrets = config.Secrets{ - SessionKey: "CHANGE_ME_32_BYTES_RANDOM", CookieSecret: "CHANGE_ME_32_BYTES_RANDOM", CSRFKey: "CHANGE_ME_32_BYTES_RANDOM", TOTPEncryptionKey: "CHANGE_ME_32_BYTES_RANDOM", @@ -308,7 +310,6 @@ func TestR6_WizardGeneratesStrongSecrets(t *testing.T) { t.Fatalf("the generated config does not load (R-6): %v", err) } for name, v := range map[string]string{ - "session_key": cfg.Secrets.SessionKey, "cookie_secret": cfg.Secrets.CookieSecret, "csrf_key": cfg.Secrets.CSRFKey, "totp_encryption_key": cfg.Secrets.TOTPEncryptionKey, diff --git a/internal/core/server/spec006_passkey_nudge_test.go b/internal/core/server/spec006_passkey_nudge_test.go index 86fdf21..273f30f 100644 --- a/internal/core/server/spec006_passkey_nudge_test.go +++ b/internal/core/server/spec006_passkey_nudge_test.go @@ -146,7 +146,11 @@ func TestSpec006AC9_DontAskAgainPersistsAndSilences(t *testing.T) { // ── AC-10 ──────────────────────────────────────────────────────────────────── func TestSpec006AC10_NoNudgeOnLoopback(t *testing.T) { - h := newHarness(t) + // L7c: the origin signal is the CONFIGURED base_url — the harness defaults + // to a public origin, so the loopback case configures it explicitly. + h := newHarness(t, func(d *server.Deps) { + d.Config.Server.BaseURL = "http://127.0.0.1:8080" + }) code := h.issueTOTP(t, h.session.IdentityID) rec := h.totpLogin(t, h.guid, code, "127.0.0.1") // the loopback origin @@ -158,6 +162,19 @@ func TestSpec006AC10_NoNudgeOnLoopback(t *testing.T) { } } +// TestSpec006_NudgeIgnoresARe writtenHost is L7c: a reverse proxy that rewrites +// Host to a loopback value must not wrongly SUPPRESS the nudge, because the +// configured base_url is the authority on the origin. +func TestSpec006_NudgeIgnoresAReWrittenHost(t *testing.T) { + h := newHarness(t) // base_url = http://example.com (public) + code := h.issueTOTP(t, h.session.IdentityID) + + rec := h.totpLogin(t, h.guid, code, "127.0.0.1") // the proxy rewrote Host + if loc := rec.Header().Get("Location"); loc != "/"+h.guid+"/auth/passkey" { + t.Errorf("TOTP login with a proxy-rewritten loopback Host landed on %q, want the nudge (L7c)", loc) + } +} + func TestSpec006AC10_NoNudgeInSetupMode(t *testing.T) { h := newHarness(t, func(d *server.Deps) { d.SetupMode = func() bool { return true } }) diff --git a/internal/core/server/spec007_recovery_test.go b/internal/core/server/spec007_recovery_test.go index b2440f6..3fc53fd 100644 --- a/internal/core/server/spec007_recovery_test.go +++ b/internal/core/server/spec007_recovery_test.go @@ -112,6 +112,9 @@ func newRecoveryHarness(t *testing.T) *recoveryHarness { Audit: aud, BaseURL: "https://myday.example.com", Logger: nil, + // M2s makes the notification asynchronous by default; these assertions + // are about no-spam semantics, so run it synchronously for determinism. + Dispatch: func(fn func()) { fn() }, }) if err != nil { t.Fatalf("recovery.New: %v", err) @@ -184,6 +187,13 @@ func (h *recoveryHarness) access(email string) *httptest.ResponseRecorder { return rec } +// sessionCookie is the session cookie the door expects for this harness's +// transport. base_url is https here, so the cookie carries the `__Host-` +// prefix (L1s). +func (h *recoveryHarness) sessionCookie(id string) *http.Cookie { + return &http.Cookie{Name: httpx.SessionCookieNameFor(true), Value: id} +} + // decide issues a session-gated approve/deny action as actor. func (h *recoveryHarness) decide(action string, actor member, requestID int64) *httptest.ResponseRecorder { h.t.Helper() @@ -193,7 +203,7 @@ func (h *recoveryHarness) decide(action string, actor member, requestID int64) * } req := httptest.NewRequest("POST", "/"+actor.guid+"?do=recovery&action="+action, strings.NewReader(form.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.AddCookie(&http.Cookie{Name: httpx.SessionCookieName, Value: actor.sessionID}) + req.AddCookie(h.sessionCookie(actor.sessionID)) rec := httptest.NewRecorder() h.srv.ServeHTTP(rec, req) return rec