osctrl-api: API extensions for a React admin frontend (round 2 of 3)

cmd/api/auth.go

@@ -2,11 +2,13 @@ package main
 
 import (
 	"context"
+	"crypto/subtle"
 	"net/http"
 	"strings"
 
 	"github.com/jmpsec/osctrl/cmd/api/handlers"
 	"github.com/jmpsec/osctrl/pkg/config"
+	"github.com/jmpsec/osctrl/pkg/types"
 	"github.com/jmpsec/osctrl/pkg/utils"
 	"github.com/rs/zerolog/log"
 )
@@ -16,14 +18,79 @@ const (
 	contextAPI string = "osctrl-api-context"
 )
 
-// Helper to extract token from header
+// Cookie + header names — kept in sync with cmd/api/handlers/login.go.
+const (
+	cookieNameToken = "osctrl_token"
+	cookieNameCSRF  = "osctrl_csrf"
+	headerNameCSRF  = "X-CSRF-Token"
+)
+
+// Helper to extract token from the Authorization header first (CLI clients),
+// falling back to the SPA's HttpOnly osctrl_token cookie.
 func extractHeaderToken(r *http.Request) string {
-	reqToken := r.Header.Get("Authorization")
-	splitToken := strings.Split(reqToken, "Bearer")
-	if len(splitToken) != 2 {
-		return ""
+	if v := r.Header.Get("Authorization"); v != "" {
+		splitToken := strings.Split(v, "Bearer")
+		if len(splitToken) == 2 {
+			if t := strings.TrimSpace(splitToken[1]); t != "" {
+				return t
+			}
+		}
+	}
+	if c, err := r.Cookie(cookieNameToken); err == nil {
+		return strings.TrimSpace(c.Value)
+	}
+	return ""
+}
+
+// mutatingMethods is the set of HTTP verbs that must carry a valid CSRF token.
+// GET/HEAD/OPTIONS are read-only and exempt.
+var mutatingMethods = map[string]bool{
+	http.MethodPost:   true,
+	http.MethodPut:    true,
+	http.MethodPatch:  true,
+	http.MethodDelete: true,
+}
+
+// checkCSRF enforces the double-submit CSRF pattern on mutating requests.
+// The SPA reads the non-HttpOnly osctrl_csrf cookie and echoes it via the
+// X-CSRF-Token header on every mutation; we constant-time-compare:
+//  1. header == cookie value (classic double-submit), AND
+//  2. cookie value == AdminUser.CSRFToken (defeats a cookie-tossing
+//     attacker who can set both header and cookie without DB write access).
+//
+// CLI clients that authenticate purely via Authorization: Bearer (no cookie)
+// are exempt — there is no browser to ride a cross-site request from.
+//
+// Note: AdminUser.CSRFToken rotates on every successful /login (see
+// LoginHandler ↦ Users.UpdateMetadata). Concurrent logins of the same user
+// race; the loser keeps a cookie that no longer matches the stored value
+// and gets 403 on the next mutation. APIToken refresh / clear also clear
+// CSRFToken (see pkg/users.UpdateToken / ClearToken) so a stale CSRF
+// cookie cannot outlive its session.
+func checkCSRF(r *http.Request, username string) bool {
+	// r.Cookie returns ErrNoCookie only when the cookie name is absent;
+	// an empty-value cookie returns (cookie, nil). Treating the empty case
+	// as "Bearer client" would bypass CSRF — instead, the call to
+	// extractHeaderToken upstream rejects empty-value cookies before we
+	// reach this function (the trimmed value falls through to "" return).
+	if _, err := r.Cookie(cookieNameToken); err != nil {
+		// No session cookie ⇒ Bearer-only client (CLI/CI). Nothing to CSRF.
+		return true
+	}
+	headerToken := strings.TrimSpace(r.Header.Get(headerNameCSRF))
+	cookie, err := r.Cookie(cookieNameCSRF)
+	if err != nil || headerToken == "" {
+		return false
+	}
+	cookieValue := strings.TrimSpace(cookie.Value)
+	if subtle.ConstantTimeCompare([]byte(headerToken), []byte(cookieValue)) != 1 {
+		return false
+	}
+	user, err := apiUsers.Get(username)
+	if err != nil || user.CSRFToken == "" {
+		return false
 	}
-	return strings.TrimSpace(splitToken[1])
+	return subtle.ConstantTimeCompare([]byte(cookieValue), []byte(user.CSRFToken)) == 1
 }
 
 // Handler to check access to a resource based on the authentication enabled
@@ -41,12 +108,51 @@ func handlerAuthCheck(h http.Handler, auth, jwtSecret string) http.Handler {
 			// Set middleware values
 			token := extractHeaderToken(r)
 			if token == "" {
-				http.Redirect(w, r, forbiddenPath, http.StatusForbidden)
+				if utils.AcceptsJSON(r) {
+					utils.HTTPResponse(w, utils.JSONApplicationUTF8, http.StatusUnauthorized,
+						types.ApiErrorResponse{Error: "unauthorized", Code: "unauthorized"})
+					return
+				}
+				// 302 is required by http.Redirect; the legacy 403 didn't actually trigger
+				// a redirect in any browser since http.Redirect demands a 3xx status.
+				http.Redirect(w, r, forbiddenPath, http.StatusFound)
 				return
 			}
 			claims, valid := apiUsers.CheckToken(jwtSecret, token)
 			if !valid {
-				http.Redirect(w, r, forbiddenPath, http.StatusForbidden)
+				if utils.AcceptsJSON(r) {
+					utils.HTTPResponse(w, utils.JSONApplicationUTF8, http.StatusUnauthorized,
+						types.ApiErrorResponse{Error: "unauthorized", Code: "unauthorized"})
+					return
+				}
+				// 302 is required by http.Redirect; the legacy 403 didn't actually trigger
+				// a redirect in any browser since http.Redirect demands a 3xx status.
+				http.Redirect(w, r, forbiddenPath, http.StatusFound)
+				return
+			}
+			// Match the presented token against the user's currently-stored APIToken
+			// so that refresh/delete on /users/{username}/token invalidates old JWTs.
+			// (CheckToken above only validates the signature.) Service users with no
+			// stored token are rejected immediately. Constant-time comparison guards
+			// against timing-side-channel leaks of the stored token.
+			user, uerr := apiUsers.Get(claims.Username)
+			tokenMatches := uerr == nil && user.APIToken != "" &&
+				subtle.ConstantTimeCompare([]byte(user.APIToken), []byte(token)) == 1
+			if !tokenMatches {
+				if utils.AcceptsJSON(r) {
+					utils.HTTPResponse(w, utils.JSONApplicationUTF8, http.StatusUnauthorized,
+						types.ApiErrorResponse{Error: "unauthorized", Code: "unauthorized"})
+					return
+				}
+				http.Redirect(w, r, forbiddenPath, http.StatusFound)
+				return
+			}
+			// CSRF guard for cookie-authenticated mutating requests. CLI Bearer
+			// clients are exempt via the cookieNameToken probe inside checkCSRF.
+			//
+			if mutatingMethods[r.Method] && !checkCSRF(r, claims.Username) {
+				utils.HTTPResponse(w, utils.JSONApplicationUTF8, http.StatusForbidden,
+					types.ApiErrorResponse{Error: "csrf token missing or invalid", Code: "csrf"})
 				return
 			}
 			// Update metadata for the user

cmd/api/auth_test.go

@@ -0,0 +1,88 @@
+package main
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/jmpsec/osctrl/pkg/config"
+)
+
+func TestHandlerAuthCheckJSONvsRedirect(t *testing.T) {
+	// A no-op inner handler — handlerAuthCheck should never call it when
+	// there's no valid token. We just need to assert the failure response.
+	inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("inner handler should not be called when auth fails")
+	})
+
+	h := handlerAuthCheck(inner, config.AuthJWT, "test-jwt-secret")
+
+	t.Run("Accept application/json returns 401 JSON", func(t *testing.T) {
+		req := httptest.NewRequest(http.MethodGet, "/api/v1/anything", nil)
+		req.Header.Set("Accept", "application/json")
+		rr := httptest.NewRecorder()
+		h.ServeHTTP(rr, req)
+		if rr.Code != http.StatusUnauthorized {
+			t.Fatalf("status: got %d, want 401", rr.Code)
+		}
+		ct := rr.Header().Get("Content-Type")
+		if ct == "" || ct[:16] != "application/json" {
+			t.Fatalf("Content-Type: got %q, want application/json...", ct)
+		}
+	})
+
+	t.Run("default client gets 302 redirect", func(t *testing.T) {
+		req := httptest.NewRequest(http.MethodGet, "/api/v1/anything", nil)
+		rr := httptest.NewRecorder()
+		h.ServeHTTP(rr, req)
+		if rr.Code != http.StatusFound {
+			t.Fatalf("status: got %d, want 302", rr.Code)
+		}
+		if rr.Header().Get("Location") == "" {
+			t.Fatal("missing Location header on redirect")
+		}
+	})
+}
+
+func TestExtractHeaderTokenPrefersBearerThenCookie(t *testing.T) {
+	cases := []struct {
+		name   string
+		header string
+		cookie string
+		want   string
+	}{
+		{"bearer header", "Bearer abc.def.ghi", "", "abc.def.ghi"},
+		{"cookie fallback", "", "xyz.uvw.123", "xyz.uvw.123"},
+		{"bearer wins over cookie", "Bearer header-token", "cookie-token", "header-token"},
+		{"no auth at all", "", "", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, "/", nil)
+			if tc.header != "" {
+				req.Header.Set("Authorization", tc.header)
+			}
+			if tc.cookie != "" {
+				req.AddCookie(&http.Cookie{Name: cookieNameToken, Value: tc.cookie})
+			}
+			got := extractHeaderToken(req)
+			if got != tc.want {
+				t.Fatalf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestMutatingMethodsTable(t *testing.T) {
+	// Lock the contract that GET/HEAD/OPTIONS bypass CSRF and PUT/PATCH/POST/DELETE require it.
+	for _, m := range []string{http.MethodGet, http.MethodHead, http.MethodOptions} {
+		if mutatingMethods[m] {
+			t.Errorf("read-only method %s should not require CSRF", m)
+		}
+	}
+	for _, m := range []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} {
+		if !mutatingMethods[m] {
+			t.Errorf("mutating method %s must require CSRF", m)
+		}
+	}
+}