From cdc5b301e2bbf95c597527ecf2453ec416e7cbc6 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 6 Sep 2026 22:02:05 -0400 Subject: [PATCH] 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 --- CLAUDE.md | 2 +- internal/server/handlers_tokens.go | 50 +++++ .../handlers_tokens_session_auth_test.go | 188 ++++++++++++++++++ 3 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 internal/server/handlers_tokens_session_auth_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 85b76f560..592057df6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 diff --git a/internal/server/handlers_tokens.go b/internal/server/handlers_tokens.go index f7c6efcde..a7d34cd71 100644 --- a/internal/server/handlers_tokens.go +++ b/internal/server/handlers_tokens.go @@ -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") diff --git a/internal/server/handlers_tokens_session_auth_test.go b/internal/server/handlers_tokens_session_auth_test.go new file mode 100644 index 000000000..a4e2df008 --- /dev/null +++ b/internal/server/handlers_tokens_session_auth_test.go @@ -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()) + } +}