fix: address review feedback for PE baseline security

CSRF only skipped after successful Bearer auth; fsd-jwt verifies password; reject suspended form login; document XFP/Secure; regression tests.
2026-08-13 04:55:42 +08:00 · 2026-07-12 21:09:24 -04:00
parent ef1a883b48
commit 1970257d27
6 changed files with 288 additions and 25 deletions
--- a/internal/web/README.md
+++ b/internal/web/README.md
@@ -1,23 +1,50 @@
 # openfsd REST & frontend interface

 ## Overview
-This API provides programmatic access to manage users, configurations, authentication, and FSD connections. All API endpoints are versioned under `/api/v1` and use JSON for request and response bodies unless otherwise specified. Authentication is primarily handled via JWT bearer tokens.
+This API provides programmatic access to manage users, configurations, authentication, and FSD connections. All API endpoints are versioned under `/api/v1` and use JSON for request and response bodies unless otherwise specified.
+
+First-party browser UI is a progressive-enhancement MPA: form login sets a signed **HttpOnly session cookie**; `/api/v1` dual-accepts that cookie **or** a Bearer access token. External tools should use Bearer API tokens.

 ---

 ## Authentication
-Most endpoints require a valid JWT access token included in the `Authorization` header as a Bearer token:
+
+### Browser (first-party UI)
+- `POST /login` (form) → signed session cookie `openfsd_session` (HttpOnly, SameSite=Lax)
+- Session claims: CID, network rating, display name, expiry (stateless JWT, `token_type=session`)
+- TTL: **24h** default; **30 days** with “Remember me”
+- `POST /logout` clears the session cookie
+- Cookie-authenticated API mutations require a CSRF synchronizer token (`csrf_token` form field or `X-CSRF-Token` header matching the `openfsd_csrf` cookie)
+- Suspended/inactive ratings cannot open a web session (same as FSD policy)
+- **Rating in the cookie is fixed until expiry.** Demotion/suspension does not revoke existing sessions until `exp` unless the JWT secret is rotated (Configure Server → Reset All). Prefer shorter TTL if faster revoke is required.
+
+### Cookie `Secure` flag (`COOKIE_SECURE`)
+| Condition | Secure |
+|-----------|--------|
+| `COOKIE_SECURE=true` (or `1`/`yes`) | forced **true** |
+| `COOKIE_SECURE=false` (or `0`/`no`) | forced **false** |
+| unset + TLS listener | **true** |
+| unset + `X-Forwarded-Proto: https` | **true** |
+| unset + plain HTTP (local compose default) | **false** |
+
+**Production / reverse proxy:** terminate TLS at the proxy, strip or overwrite client `X-Forwarded-Proto`, and set **`COOKIE_SECURE=true`**. Do not rely on client-supplied XFP alone — any direct client can send that header when the app is reachable without a trusted proxy hop.
+
+### External API (Bearer)
+Most endpoints accept a valid JWT access token:
 ```
 Authorization: Bearer <access_token>
 ```
 - **API tokens** can be created via `/api/v1/config/createtoken` with a custom expiry date. See the **Server Configuration** menu in the frontend UI to generate one.
+- Bearer-authenticated clients do **not** need CSRF (CSRF applies only when the request is authenticated via the session cookie).
+- Dual-accept: a **valid** Bearer token wins over a session cookie; a garbage Bearer header does **not** disable CSRF if the session cookie is what authenticates the request.

 ---

 ## Network Ratings
-The API enforces role-based access control using `NetworkRating` values defined in the `fsd` package. Key thresholds:
+The API enforces role-based access control using `NetworkRating` values defined in `pkg/protocol`. Key thresholds:
 - **Supervisor (11)**: Can manage users (create, update, retrieve) and kick active connections.
 - **Administrator (12)**: Can manage server configuration, reset JWT secret keys, and create API tokens.
+- **Suspended (0) / Inactive (-1)**: Cannot log in to the web UI or obtain FSD JWTs.

 ---

--- a/internal/web/auth.go
+++ b/internal/web/auth.go
@@ -40,6 +40,12 @@ func (s *Server) getAccessRefreshTokens(c *gin.Context) {
 		return
 	}

+	// Align with FSD policy: suspended/inactive cannot mint web tokens.
+	if user.NetworkRating <= int(protocol.NetworkRatingSuspended) {
+		writeAPIV1Response(c, http.StatusUnauthorized, &unauthRes)
+		return
+	}
+
 	access, refresh, err := s.makeAccessRefreshTokens(user, reqBody.RememberMe)
 	if err != nil {
 		writeAPIV1Response(c, http.StatusInternalServerError, &genericAPIV1InternalServerError)
@@ -156,6 +162,14 @@ func (s *Server) getFsdJwt(c *gin.Context) {
 		return
 	}

+	if !s.dbRepo.UserRepo.VerifyPasswordHash(reqBody.Password, user.Password) {
+		resBody := ResponseBody{
+			ErrorMsg: "Invalid CID and/or password",
+		}
+		c.JSON(http.StatusUnauthorized, &resBody)
+		return
+	}
+
 	if user.NetworkRating <= int(protocol.NetworkRatingSuspended) {
 		c.JSON(http.StatusForbidden, &ResponseBody{ErrorMsg: "Certificate suspended or inactive"})
 		return
@@ -191,15 +205,25 @@ func (s *Server) getFsdJwt(c *gin.Context) {
 	})
 }

+// Auth method keys for dual-accept (Bearer vs session cookie).
+// csrfIfCookieSession only skips CSRF when auth actually succeeded via Bearer.
+const (
+	authMethodContextKey = "auth_method"
+	authMethodBearer     = "bearer"
+	authMethodSession    = "session"
+)
+
 // jwtBearerMiddleware verifies a Bearer access token OR a signed session cookie
 // (KD-18 dual-accept). Cookie-authenticated mutations are CSRF-checked by
 // csrfIfCookieSession on the API group.
 func (s *Server) jwtBearerMiddleware(c *gin.Context) {
 	if s.tryBearerAuth(c) {
+		c.Set(authMethodContextKey, authMethodBearer)
 		c.Next()
 		return
 	}
 	if s.trySessionAuth(c) {
+		c.Set(authMethodContextKey, authMethodSession)
 		c.Next()
 		return
 	}
@@ -211,10 +235,10 @@ func (s *Server) jwtBearerMiddleware(c *gin.Context) {

 // tryBearerAuth parses Authorization: Bearer access tokens into the gin context.
 // Returns true when a valid access token was accepted.
+// The scheme match is case-insensitive ("Bearer " / "bearer ").
 func (s *Server) tryBearerAuth(c *gin.Context) bool {
-	authHeader := c.GetHeader("Authorization")
-	raw, found := strings.CutPrefix(authHeader, "Bearer ")
-	if !found || raw == "" {
+	raw, ok := cutBearerToken(c.GetHeader("Authorization"))
+	if !ok {
 		return false
 	}

@@ -237,6 +261,22 @@ func (s *Server) tryBearerAuth(c *gin.Context) bool {
 	return true
 }

+// cutBearerToken extracts the token from an Authorization header with a
+// case-insensitive "Bearer " scheme. Empty tokens are rejected.
+func cutBearerToken(header string) (token string, ok bool) {
+	if len(header) < 7 {
+		return "", false
+	}
+	if !equalFoldASCII(header[:7], "Bearer ") {
+		return "", false
+	}
+	token = strings.TrimSpace(header[7:])
+	if token == "" {
+		return "", false
+	}
+	return token, true
+}
+
 // trySessionAuth parses the signed session cookie into the gin context.
 func (s *Server) trySessionAuth(c *gin.Context) bool {
 	claims, err := s.parseSessionCookie(c)
--- a/internal/web/csrf.go
+++ b/internal/web/csrf.go
@@ -92,8 +92,12 @@ func (s *Server) requireCSRF(c *gin.Context) {
 	c.AbortWithStatus(http.StatusForbidden)
 }

-// csrfIfCookieSession enforces CSRF on state-changing requests that use the
-// session cookie (not Bearer). Bearer API clients skip CSRF.
+// csrfIfCookieSession enforces CSRF on state-changing requests authenticated
+// via the session cookie. CSRF is skipped only when dual-accept auth
+// actually succeeded with a valid Bearer access token (auth_method=bearer).
+//
+// A junk Authorization: Bearer header must NOT disable CSRF while a session
+// cookie still authenticates the request (Issue 1 dual-accept bypass).
 func (s *Server) csrfIfCookieSession(c *gin.Context) {
 	switch c.Request.Method {
 	case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace:
@@ -101,21 +105,13 @@ func (s *Server) csrfIfCookieSession(c *gin.Context) {
 		return
 	}

-	// Bearer auth is not browser-cookie CSRF-vulnerable in the same way.
-	if authHeader := c.GetHeader("Authorization"); len(authHeader) >= 7 {
-		prefix := authHeader[:7]
-		if equalFoldASCII(prefix, "Bearer ") {
-			c.Next()
-			return
-		}
-	}
-
-	// No session cookie → nothing to CSRF-protect; auth middleware will 401.
-	if raw, err := c.Cookie(sessionCookieName); err != nil || raw == "" {
+	// Only skip CSRF when jwtBearerMiddleware recorded a successful Bearer auth.
+	if method, ok := c.Get(authMethodContextKey); ok && method == authMethodBearer {
 		c.Next()
 		return
 	}

+	// Session-authenticated (or any non-bearer) mutation requires CSRF.
 	if s.validateCSRF(c) {
 		c.Next()
 		return
--- a/internal/web/frontend.go
+++ b/internal/web/frontend.go
@@ -6,6 +6,7 @@ import (
 	"strings"

 	"github.com/gin-gonic/gin"
+	"github.com/renorris/openfsd/pkg/protocol"
 )

 func (s *Server) handleFrontendLanding(c *gin.Context) {
@@ -69,6 +70,14 @@ func (s *Server) handleFrontendLoginPost(c *gin.Context) {
 		return
 	}

+	// Align with FSD policy: suspended/inactive cannot open a web session.
+	// Generic error avoids an account-status oracle.
+	if user.NetworkRating <= int(protocol.NetworkRatingSuspended) {
+		page.Error = "Bad CID and/or password"
+		s.writeTemplate(c, "login", page)
+		return
+	}
+
 	if err := s.setSessionCookie(c, user, rememberMe); err != nil {
 		page.Error = "Unable to create session"
 		s.writeTemplate(c, "login", page)
--- a/internal/web/pe_test.go
+++ b/internal/web/pe_test.go
@@ -414,6 +414,84 @@ func TestAPIBearerAuthStillWorksWithoutCSRF(t *testing.T) {
 func TestObserverCannotAccessUserEditor(t *testing.T) {
 	ts := newTestServer(t)
 	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingObserver))
+	cookies := formLogin(t, ts, user.CID, "pw")
+
+	req := httptest.NewRequest(http.MethodGet, "/usereditor", nil)
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusSeeOther {
+		t.Fatalf("status %d", w.Code)
+	}
+	if loc := w.Header().Get("Location"); loc != "/dashboard" {
+		t.Fatalf("Location = %q want /dashboard", loc)
+	}
+}
+
+// TestAPICookieAuthCSRFNotBypassedByGarbageBearer is the Issue 1 regression:
+// valid session + Authorization: Bearer garbage + no CSRF must 403 (not skip CSRF).
+func TestAPICookieAuthCSRFNotBypassedByGarbageBearer(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingSupervisor))
+	cookies := formLogin(t, ts, user.CID, "pw")
+
+	body := `{"cid":` + itoa(user.CID) + `}`
+	req := httptest.NewRequest(http.MethodPost, "/api/v1/user/load", strings.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	req.Header.Set("Authorization", "Bearer not-a-jwt")
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusForbidden {
+		t.Fatalf("status %d want 403 (CSRF required when Bearer fails), body %s", w.Code, w.Body.String())
+	}
+
+	// lowercase scheme also must not bypass CSRF when token is garbage
+	req = httptest.NewRequest(http.MethodPost, "/api/v1/user/load", strings.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	req.Header.Set("Authorization", "bearer x")
+	w = httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusForbidden {
+		t.Fatalf("lowercase bearer garbage status %d want 403, body %s", w.Code, w.Body.String())
+	}
+}
+
+func TestSupervisorCannotAccessConfigEditor(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingSupervisor))
+	cookies := formLogin(t, ts, user.CID, "pw")
+
+	req := httptest.NewRequest(http.MethodGet, "/configeditor", nil)
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusSeeOther {
+		t.Fatalf("status %d", w.Code)
+	}
+	if loc := w.Header().Get("Location"); loc != "/dashboard" {
+		t.Fatalf("Location = %q want /dashboard", loc)
+	}
+}
+
+func TestAdminCanAccessConfigEditor(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingAdministator))
+	cookies := formLogin(t, ts, user.CID, "pw")
+
+	req := httptest.NewRequest(http.MethodGet, "/configeditor", nil)
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusOK {
+		t.Fatalf("status %d want 200", w.Code)
+	}
+}
+
+func TestSuspendedUserCannotFormLogin(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingSuspended))

 	csrf, cookies := getLoginCSRF(t, ts)
 	form := url.Values{}
@@ -425,18 +503,120 @@ func TestObserverCannotAccessUserEditor(t *testing.T) {
 	req.Header.Set("Cookie", cookieHeader(cookies))
 	w := httptest.NewRecorder()
 	ts.engine.ServeHTTP(w, req)
-	cookies = mergeCookies(cookies, w.Result())
+	if w.Code != http.StatusOK {
+		t.Fatalf("status %d want 200 re-render", w.Code)
+	}
+	if extractCookie(w.Result(), sessionCookieName) != "" {
+		t.Fatal("suspended user must not receive session cookie")
+	}
+	if !strings.Contains(w.Body.String(), "Bad CID and/or password") {
+		t.Fatalf("expected generic error, body=%s", clip(w.Body.String(), 300))
+	}
+}

-	req = httptest.NewRequest(http.MethodGet, "/usereditor", nil)
+func TestInactiveUserCannotFormLogin(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingInactive))
+
+	csrf, cookies := getLoginCSRF(t, ts)
+	form := url.Values{}
+	form.Set("cid", itoa(user.CID))
+	form.Set("password", "pw")
+	form.Set("csrf_token", csrf)
+	req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if extractCookie(w.Result(), sessionCookieName) != "" {
+		t.Fatal("inactive user must not receive session cookie")
+	}
+}
+
+func TestFsdJwtRequiresPassword(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "correct-password", int(protocol.NetworkRatingObserver))
+
+	// Wrong password → 401
+	form := url.Values{}
+	form.Set("cid", itoa(user.CID))
+	form.Set("password", "wrong")
+	req := httptest.NewRequest(http.MethodPost, "/api/v1/fsd-jwt", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusUnauthorized {
+		t.Fatalf("wrong password status %d want 401, body %s", w.Code, w.Body.String())
+	}
+
+	// Correct password → 200 + token
+	form.Set("password", "correct-password")
+	req = httptest.NewRequest(http.MethodPost, "/api/v1/fsd-jwt", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	w = httptest.NewRecorder()
 	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusOK {
+		t.Fatalf("good password status %d want 200, body %s", w.Code, w.Body.String())
+	}
+	if !strings.Contains(w.Body.String(), `"success":true`) && !strings.Contains(w.Body.String(), `"success": true`) {
+		// gin may encode without spaces
+		if !strings.Contains(w.Body.String(), "token") {
+			t.Fatalf("expected token in response: %s", w.Body.String())
+		}
+	}
+}
+
+func TestRememberMeSetsLongerSessionMaxAge(t *testing.T) {
+	ts := newTestServer(t)
+	user := createTestUser(t, ts, "pw", int(protocol.NetworkRatingObserver))
+
+	csrf, cookies := getLoginCSRF(t, ts)
+	form := url.Values{}
+	form.Set("cid", itoa(user.CID))
+	form.Set("password", "pw")
+	form.Set("csrf_token", csrf)
+	form.Set("remember_me", "on")
+	req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
 	if w.Code != http.StatusSeeOther {
-		t.Fatalf("status %d", w.Code)
+		t.Fatalf("login status %d", w.Code)
 	}
-	if loc := w.Header().Get("Location"); loc != "/dashboard" {
-		t.Fatalf("Location = %q want /dashboard", loc)
+	found := false
+	for _, sc := range w.Result().Header.Values("Set-Cookie") {
+		if !strings.HasPrefix(sc, sessionCookieName+"=") {
+			continue
+		}
+		found = true
+		// Max-Age for 30 days = 2592000
+		if !strings.Contains(sc, "Max-Age=2592000") && !strings.Contains(sc, "max-age=2592000") {
+			t.Fatalf("remember-me Max-Age want 2592000, Set-Cookie=%s", sc)
+		}
 	}
+	if !found {
+		t.Fatal("missing session Set-Cookie")
+	}
+}
+
+// formLogin performs a successful no-JS form login and returns merged cookies.
+func formLogin(t *testing.T, ts *testServer, cid int, password string) []*http.Cookie {
+	t.Helper()
+	csrf, cookies := getLoginCSRF(t, ts)
+	form := url.Values{}
+	form.Set("cid", itoa(cid))
+	form.Set("password", password)
+	form.Set("csrf_token", csrf)
+	req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.Header.Set("Cookie", cookieHeader(cookies))
+	w := httptest.NewRecorder()
+	ts.engine.ServeHTTP(w, req)
+	if w.Code != http.StatusSeeOther {
+		t.Fatalf("form login status %d body %s", w.Code, w.Body.String())
+	}
+	return mergeCookies(cookies, w.Result())
 }

 func itoa(n int) string {
--- a/internal/web/session.go
+++ b/internal/web/session.go
@@ -26,7 +26,18 @@ var (
 )

 // cookieSecureFlag decides whether Set-Cookie should include the Secure attribute.
-// cookieSecureEnv is the COOKIE_SECURE value: "true"/"false" force; empty = auto.
+//
+// Policy:
+//   - COOKIE_SECURE=true|false|1|0|yes|no → force
+//   - unset + TLS listener → Secure
+//   - unset + X-Forwarded-Proto: https → Secure
+//   - otherwise (local docker-compose HTTP) → not Secure
+//
+// Operator note: X-Forwarded-Proto is only trustworthy when a reverse proxy
+// terminates TLS and overwrites/strips client-supplied XFP. For production
+// behind TLS termination, prefer COOKIE_SECURE=true so Secure does not depend
+// on client-controlled headers. Spoofing XFP=https on plain HTTP only makes
+// browsers drop the cookie (fail-closed for session use on that hop).
 func cookieSecureFlag(cookieSecureEnv string, tls bool, xForwardedProto string) bool {
 	switch strings.ToLower(strings.TrimSpace(cookieSecureEnv)) {
 	case "true", "1", "yes":