fix(server): minting or rotating an API token requires an interactive session (BUG-2890) (#1267)

A PAT that could reach POST /auth/tokens minted further tokens with independent names and expiries. Those survive the revocation of the token that created them, and nothing in the token list records which token minted which -- so revoking a leaked credential did not end the access it had been used to establish. Ruled on the item's trail day 57: create and rotate require session auth and refuse a PAT-authenticated call with 403 session_required; list and revoke stay PAT-reachable. Population is THREE doors, not the two the filing named. Enumerating every route that reaches store.CreateAPIToken / RotateAPIToken turns up the workspace-scoped mint at POST /workspaces/{ws}/tokens, which had only requireMinRole("owner") in front of it -- and a user-owned PAT held by an owner satisfies that. Measured, not read: with the fixture's owner also written into workspace_members, that door returned 201 with a live token on the unfixed tree. All three doors are gated; list and revoke on both the user and workspace routes are deliberately untouched, because neither extends access and revocation is the compromised-credential response. The gate is on the CREDENTIAL, not on the door: isAPITokenAuth is false for a session cookie AND for a padsess_ CLI bearer, a distinction that predates this fix (see ctxValidatedSessionBearer's note in middleware_auth.go) because a CLI session IS an interactive session. At the workspace door the check runs before requireMinRole, so a PAT-borne caller cannot learn its own membership status from the difference between the two 403s. Five mutants run, each detected by its own leg: dropping the gate at each of the three doors fails that door's test; gating on Authorization != "" instead of the credential kind fails the CLI-session leg AND NOTHING ELSE; over-applying the gate to list and revoke fails both PAT-still-works controls. The fourth is why the CLI leg exists -- a header-shaped gate passes every PAT assertion and breaks every logged-in CLI. Also documents the refusal on the CLAUDE.md route line: the routes were listed without saying anything about credential kind, so an agent holding a PAT would have met an unexplained 403. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR
2026-10-26 06:19:49 +00:00 · 2026-09-06 22:02:05 -04:00
parent e32d4f468e
commit cdc5b301e2
3 changed files with 239 additions and 1 deletions
@@ -107,7 +107,7 @@ Both SSE endpoints share one admission budget, enforced **per instance**: `PAD_S
 - `POST /api/v1/auth/forgot-password` — request password reset email
 - `POST /api/v1/auth/reset-password` — reset password with token
 - `POST /api/v1/auth/local-reset` — localhost-only account recovery (self-host, non-cloud). Loopback-gated, no auth — the bootstrap trust model. Returns a single-use reset link, or a temp password with `{"temp_password": true}`. Backs `pad auth reset-password`.
- `GET/POST/DELETE /api/v1/auth/tokens` — user-scoped API tokens
+- `GET/POST/DELETE /api/v1/auth/tokens` — user-scoped API tokens. **Minting and rotating require an INTERACTIVE SESSION** (BUG-2890): a call authenticated by a PAT is refused `403 session_required` on `POST /auth/tokens`, `POST /auth/tokens/{id}/rotate` and `POST /workspaces/{ws}/tokens`, because a token that can mint tokens outlives its own revocation. A session cookie and a `padsess_` CLI bearer both count as interactive; LIST and REVOKE stay PAT-reachable, deliberately — revocation is the compromised-credential response
 - `GET/PATCH /api/v1/admin/settings` — platform settings (admin-only)
 - `POST /api/v1/admin/test-email` — send test email (admin-only)
 - `POST /api/v1/invitations/{code}/accept` — accept workspace invitation
@@ -32,9 +32,51 @@ func (s *Server) getTokenExpirySettings() (defaultDays, maxDays int) {
 	return
 }

+// requireInteractiveSession refuses a request authenticated by a long-lived
+// API token, and is the gate on every door that MINTS or ROTATES one
+// (BUG-2890, ruled on that item's trail day 57).
+//
+// The escalation it closes: a PAT that can reach a mint door issues further
+// tokens with independent names and expiries, which outlive the revocation
+// of the token that created them. Revoking a leaked credential then does
+// not end the access it was used to establish, and nothing in the token
+// list says which token minted which.
+//
+// It gates the CREDENTIAL, not the door. `isAPITokenAuth` is false for a
+// session cookie AND for a `padsess_` CLI bearer — that distinction is
+// deliberate and predates this fix (see ctxValidatedSessionBearer's note in
+// middleware_auth.go): a CLI session IS an interactive session. A gate
+// written against "carries an Authorization: Bearer header" would look
+// identical on every PAT test and break every logged-in CLI.
+//
+// LIST and REVOKE deliberately stay reachable by a PAT. Neither extends
+// access, and revocation is the compromised-credential response — gating it
+// behind a browser would put a session in the way of the incident path.
+//
+// Same shape as the 2FA handlers' gate; the code differs because this one
+// is `session_required` rather than the generic `forbidden`, so a client
+// can tell "wrong credential kind" from "insufficient permissions".
+func requireInteractiveSession(w http.ResponseWriter, r *http.Request) bool {
+	if isAPITokenAuth(r) {
+		writeError(w, http.StatusForbidden, "session_required",
+			"Creating or rotating API tokens requires an interactive session, not an API token")
+		return false
+	}
+	return true
+}
+
 // handleCreateToken creates a new API token scoped to a workspace.
 // The token is owned by the authenticated user (if any).
 func (s *Server) handleCreateToken(w http.ResponseWriter, r *http.Request) {
+	// Before requireMinRole, not after: this door mints through the same
+	// store call as /auth/tokens and is reachable by an OWNER's PAT
+	// (measured on BUG-2890's trail — the filing named only the two
+	// /auth/tokens doors). Checking the credential kind first also stops
+	// a PAT-borne caller learning its own membership status from the
+	// difference between the two 403s.
+	if !requireInteractiveSession(w, r) {
+		return
+	}
 	if !requireMinRole(w, r, "owner") {
 		return
 	}
@@ -144,6 +186,10 @@ func (s *Server) handleListUserTokens(w http.ResponseWriter, r *http.Request) {

 // handleCreateUserToken creates a new API token owned by the authenticated user.
 func (s *Server) handleCreateUserToken(w http.ResponseWriter, r *http.Request) {
+	if !requireInteractiveSession(w, r) {
+		return
+	}
+
 	userID := currentUserID(r)
 	if userID == "" {
 		writeError(w, http.StatusUnauthorized, "unauthorized", "Not logged in")
@@ -205,6 +251,10 @@ func (s *Server) handleDeleteUserToken(w http.ResponseWriter, r *http.Request) {
 // handleRotateUserToken generates a new secret for an existing token,
 // invalidating the old one. The token metadata is preserved.
 func (s *Server) handleRotateUserToken(w http.ResponseWriter, r *http.Request) {
+	if !requireInteractiveSession(w, r) {
+		return
+	}
+
 	userID := currentUserID(r)
 	if userID == "" {
 		writeError(w, http.StatusUnauthorized, "unauthorized", "Not logged in")
@@ -0,0 +1,188 @@
+package server
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-2890: minting and rotating API tokens requires an INTERACTIVE
+// SESSION. A PAT that can reach the create door mints further tokens with
+// independent names and expiries, which survive the revocation of the
+// token that made them — so revoking a leaked credential does not end the
+// access it was used to establish.
+//
+// Dave ruled the shape on day 57: create and rotate require session auth
+// and refuse a PAT-authenticated call with 403 `session_required`; list
+// and revoke stay PAT-reachable, because neither extends access.
+//
+// The controls matter more than usual here. A PAT-refused leg alone is
+// satisfied by a handler that refuses EVERYONE, and a gate written against
+// "carries an Authorization header" rather than against "is a PAT" would
+// pass every leg below except the CLI one — `padsess_` Bearer credentials
+// are interactive sessions and must keep working.
+
+type tokenAuthEnv struct {
+	srv     *Server
+	user    *models.User
+	ws      *models.Workspace
+	sessTok string // padsess_ — cookie AND CLI bearer
+	patTok  string // pad_ — the credential under test
+	patID   string
+	spareID string // a second PAT, so revoke/rotate legs need not eat the one they authenticate with
+}
+
+func setupTokenAuthEnv(t *testing.T) *tokenAuthEnv {
+	t.Helper()
+	srv := testServer(t)
+
+	user, err := srv.store.CreateUser(models.UserCreate{
+		Email: "tokens@example.com", Name: "Token Owner",
+		Password: "correct-horse-battery-staple", Role: "member",
+	})
+	if err != nil {
+		t.Fatalf("create user: %v", err)
+	}
+	ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Tokens WS", OwnerID: user.ID})
+	if err != nil {
+		t.Fatalf("create workspace: %v", err)
+	}
+	// MEMBERSHIP IS LOAD-BEARING FOR THE WORKSPACE-DOOR LEG, not setup
+	// boilerplate. CreateWorkspace records an owner_id; it does not add a
+	// workspace_members row, and RequireWorkspaceAccess gates on
+	// membership. Without this the workspace-door test gets its 403 from
+	// "not a member" and discriminates nothing about the session gate —
+	// measured, not assumed: the first run of this file failed on exactly
+	// that, with `You are not a member of this workspace`.
+	if err := srv.store.AddWorkspaceMember(ws.ID, user.ID, "owner"); err != nil {
+		t.Fatalf("add owner as member: %v", err)
+	}
+
+	pat, err := srv.store.CreateAPIToken(user.ID, models.APITokenCreate{
+		Name: "the-pat", WorkspaceID: ws.ID,
+	}, 0, 0)
+	if err != nil {
+		t.Fatalf("CreateAPIToken: %v", err)
+	}
+	spare, err := srv.store.CreateAPIToken(user.ID, models.APITokenCreate{
+		Name: "spare", WorkspaceID: ws.ID,
+	}, 0, 0)
+	if err != nil {
+		t.Fatalf("CreateAPIToken (spare): %v", err)
+	}
+
+	return &tokenAuthEnv{
+		srv:     srv,
+		user:    user,
+		ws:      ws,
+		sessTok: loginUser(t, srv, "tokens@example.com", "correct-horse-battery-staple"),
+		patTok:  pat.Token,
+		patID:   pat.ID,
+		spareID: spare.ID,
+	}
+}
+
+// THE RULE, create: a PAT cannot mint a token.
+func TestCreateUserToken_PATIsRefused(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "POST", "/api/v1/auth/tokens", env.patTok,
+		map[string]any{"name": "minted-by-a-pat"})
+	if rr.Code != http.StatusForbidden {
+		t.Fatalf("PAT minted a token: got %d, want 403; body=%s", rr.Code, rr.Body.String())
+	}
+	if got := errorCode(t, rr); got != "session_required" {
+		t.Errorf("error code = %v, want session_required; body=%s", got, rr.Body.String())
+	}
+}
+
+// THE RULE, rotate: a PAT cannot rotate a token. Rotation re-issues a
+// secret and can extend an expiry, so it establishes access exactly the
+// way create does.
+func TestRotateUserToken_PATIsRefused(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "POST", "/api/v1/auth/tokens/"+env.spareID+"/rotate", env.patTok, nil)
+	if rr.Code != http.StatusForbidden {
+		t.Fatalf("PAT rotated a token: got %d, want 403; body=%s", rr.Code, rr.Body.String())
+	}
+	if got := errorCode(t, rr); got != "session_required" {
+		t.Errorf("error code = %v, want session_required; body=%s", got, rr.Body.String())
+	}
+}
+
+// THE THIRD DOOR (population, not the two the filing named): the
+// workspace-scoped mint at POST /workspaces/{ws}/tokens reaches the same
+// store call with only requireMinRole("owner") in front of it, and a
+// user-owned PAT held by an owner satisfies that.
+func TestCreateWorkspaceToken_PATIsRefused(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "POST", "/api/v1/workspaces/"+env.ws.Slug+"/tokens", env.patTok,
+		map[string]any{"name": "minted-by-a-pat-workspace-door"})
+	if rr.Code != http.StatusForbidden {
+		t.Fatalf("PAT minted a workspace-scoped token: got %d, want 403; body=%s", rr.Code, rr.Body.String())
+	}
+	if got := errorCode(t, rr); got != "session_required" {
+		t.Errorf("error code = %v, want session_required; body=%s", got, rr.Body.String())
+	}
+}
+
+// CONTROL — the gate is on the CREDENTIAL, not on the door. A browser
+// session still mints. Without this leg, a handler that refuses everyone
+// passes every assertion above.
+func TestCreateUserToken_SessionCookieStillWorks(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithCookie(env.srv, "POST", "/api/v1/auth/tokens",
+		map[string]any{"name": "minted-by-a-session"}, env.sessTok)
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("session could not mint a token: got %d, want 201; body=%s", rr.Code, rr.Body.String())
+	}
+}
+
+// CONTROL — and the sharpest one. A `padsess_` CLI bearer IS an
+// interactive session: ctxValidatedSessionBearer, not ctxIsAPIToken. A
+// gate written against "has an Authorization: Bearer header" would pass
+// every other leg in this file and break every logged-in CLI.
+func TestCreateUserToken_CLISessionBearerStillWorks(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "POST", "/api/v1/auth/tokens", env.sessTok,
+		map[string]any{"name": "minted-by-a-cli-session"})
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("a CLI session bearer was refused: got %d, want 201; body=%s", rr.Code, rr.Body.String())
+	}
+}
+
+// CONTROL — list stays PAT-reachable. Reading which tokens exist extends
+// no access, and gating it would break every agent that inventories its
+// own credentials.
+func TestListUserTokens_PATStillWorks(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "GET", "/api/v1/auth/tokens", env.patTok, nil)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("PAT could not list tokens: got %d, want 200; body=%s", rr.Code, rr.Body.String())
+	}
+}
+
+// CONTROL — revoke stays PAT-reachable, and this is the load-bearing one:
+// a compromised-credential response is REVOCATION, so gating it behind a
+// session would make the incident path need a browser.
+func TestDeleteUserToken_PATStillWorks(t *testing.T) {
+	t.Parallel()
+	env := setupTokenAuthEnv(t)
+
+	rr := doRequestWithBearer(env.srv, "DELETE", "/api/v1/auth/tokens/"+env.spareID, env.patTok, nil)
+	if rr.Code != http.StatusNoContent {
+		t.Fatalf("PAT could not revoke a token: got %d, want 204; body=%s", rr.Code, rr.Body.String())
+	}
+}