From 393d986ea9ece0dccf69a4a4a0ce9eee1b4f4609 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 20 Oct 2026 13:30:31 -0400 Subject: [PATCH] fix(auth): a browser-approved CLI token gets its own lifetime (TASK-3975) (#2252) * fix(auth): a browser-approved CLI token gets its own lifetime (TASK-3975) The CLI token minted by /auth/cli/sessions/{code}/approve was dated by the approving web session's created_at, so approving from an 85-day-old session left the token a 5-day window under the 90-day cap. It is now minted with no issuedAt: created_at is the approval, 30-day sliding window, 90-day cap from approval. No TTL lengths change. The widening is accepted: a stolen near-expiry web session can now mint a CLI token good for up to 90 days from approval. Step-up-on-approve is filed as its own idea. created_at doubled as the sign-in recency anchor for confirm-only account delete (BUG-3336), so dating the CLI token from approval would have made it read as a fresh sign-in. CLI-kind sessions can therefore no longer satisfy confirm-only delete recency (403 reauth_required): approach (b), which keeps the BUG-3336 property with no migration. Passwordless accounts confirm from the web. The credential-change rotation keeps the replaced session's created_at (pinned by a new test); its 7d-vs-30d TTL is BUG-3978, untouched. Claude-Session: https://claude.ai/code/session_01JRXHP9ZVfEN3YEuzmucxP3 * fix(auth): only a browser session approves a CLI sign-in (TASK-3975 r2) With the approval now minting the CLI token with its own lifetime, a CLI-kind session that could approve could mint its own successor: a CLI bearer creates a pending code with POST /auth/cli/sessions, approves it with itself, and receives a new token with created_at = now and a fresh 90-day cap. Repeated before each cap, that chain keeps a CLI credential (or a stolen one) alive forever. Before this PR the approval inherited the approver's created_at, so the chain stayed bounded by the original sign-in. Reproduced on the previous head: an 85-day-old CLI bearer got 200 and a session aged 0s. The accepted widening is ONE hop from a web session; the chain never was. /approve now refuses any approver that is not a web-kind session with 403 session_required, before anything is minted. The web client approves on its cookie and is the only caller. docs/api.md says so. Claude-Session: https://claude.ai/code/session_01JRXHP9ZVfEN3YEuzmucxP3 * test(auth): the rotation cap pin starts from a 7-day web session (TASK-3975) #2254 (BUG-3978) makes a credential-change rotation keep the replaced session's own sliding window. TestTASK3975_RotationKeepsParentCap rotated the 24-hour session oauthOnlyCloud mints, so after that change the 24-hour window bound before the 90-day cap it exists to pin, and it failed on the rebased tree (expiry now+24h, want now+5d). The parent is now a 7-day web session, so the cap five days away is what binds. Test-only. Claude-Session: https://claude.ai/code/session_01JRXHP9ZVfEN3YEuzmucxP3 * docs(api): who may approve a CLI sign-in, by credential (TASK-3975 r4) A CLI-kind bearer gets 403 session_required, a CLI-kind session sent as a cookie is no credential (BUG-3350) and gets 401, and an API token gets 403. The sentence said any CLI-kind session got 403, and called the web-kind check "browser" though a web session sent as a bearer passes. Claude-Session: https://claude.ai/code/session_01JRXHP9ZVfEN3YEuzmucxP3 --- docs/api.md | 4 +- internal/server/handlers_account.go | 6 +- internal/server/handlers_cli_auth.go | 21 ++- .../task3975_cli_token_lifetime_test.go | 150 ++++++++++++++++++ 4 files changed, 172 insertions(+), 9 deletions(-) create mode 100644 internal/server/task3975_cli_token_lifetime_test.go diff --git a/docs/api.md b/docs/api.md index 17f6b1eef..1daa94bcd 100644 --- a/docs/api.md +++ b/docs/api.md @@ -121,10 +121,10 @@ One browser tab's content writes to an item are ordered by the server, also **pe - `POST /api/v1/auth/logout` — destroy session - `GET/PATCH /api/v1/auth/me` — current user profile (GET) and update name/password (PATCH) - `GET /api/v1/auth/export` — the caller's account export (GDPR portability), streamed as `pad-account-export.json` (not `pad-export.json`, which is the workspace bundle's file): `{format: "pad-account-export", version: 1, user, workspaces: [...], complete: true}`, where owned workspaces carry their collections and items. `complete: true` is written last and only after every workspace (BUG-3626). The status line goes out before the body, so a timeout (60s) or a store error mid-export cannot become an error status. The server cuts the connection instead, and the body never reaches the trailer. A reader must refuse a file without `complete: true`; the web client does. 403 `forbidden` when the caller is collection-restricted in any owned workspace (BUG-1945) -- `POST /api/v1/auth/delete-account` — delete the caller's account: `{password}`, or `{confirm: true}` for a cloud account with no password, plus `totp_code` when 2FA is on. The instance's last ACTIVE human admin is refused `409 last_active_admin` (BUG-3811), by a check that runs before any Stripe cancel; the deletion re-checks under its lock, and an admin set that changed in between refuses it there: `409 last_active_admin` again, or `500 partial_delete` when a Stripe cancel already ran. A disabled admin is not the admin who remains +- `POST /api/v1/auth/delete-account` — delete the caller's account: `{password}`, or `{confirm: true}` for a cloud account with no password (it needs a web session signed in within the last 10 minutes, else `403 reauth_required`; a CLI-kind session, however it was minted, never satisfies it, TASK-3975, so a passwordless account confirms from the web, not the CLI), plus `totp_code` when 2FA is on. The instance's last ACTIVE human admin is refused `409 last_active_admin` (BUG-3811), by a check that runs before any Stripe cancel; the deletion re-checks under its lock, and an admin set that changed in between refuses it there: `409 last_active_admin` again, or `500 partial_delete` when a Stripe cancel already ran. A disabled admin is not the admin who remains - `POST /api/v1/auth/forgot-password` — request password reset email - `POST /api/v1/auth/reset-password` — reset password with token -- `POST /api/v1/auth/cli/sessions`, `GET /api/v1/auth/cli/sessions/{code}`, `POST /api/v1/auth/cli/sessions/{code}/approve`, `POST /api/v1/auth/cli/sessions/{code}/deny` — browser sign-in for `pad auth login`: the CLI creates a session and polls it, and a signed-in user approves it at `/auth/cli/{code}`, which mints a 30-day session the poll hands to the CLI. Since TASK-2253 the session records `requester_ip` (the server's view of the creating client) and `requester_user_agent` (what the creator sent, capped at 200 characters, so requester-controlled text), and a PENDING poll returns them with `created_at` (additive, omitempty) for the approval page, which renders the agent as text labelled "reported by the requester". `deny` checks no account, because the code is the authority (it is already a bearer secret: whoever holds it can poll and collect an approval's token); it moves pending → denied, answers 200 again on a denied session, and 409 `already_approved` / 410 `expired` otherwise. A denied session POLLS as **410 `cli_auth_denied`**, not as a 200 status: a CLI from before Deny has no case for an unknown status and would poll it for 20 minutes, whereas a non-2xx counts as a poll error there and it stops after five in a row, printing the message; a current CLI stops on the first. Approving a denied session answers 409 `cli_auth_denied`, and an approval that loses to a concurrent deny ends the session it minted +- `POST /api/v1/auth/cli/sessions`, `GET /api/v1/auth/cli/sessions/{code}`, `POST /api/v1/auth/cli/sessions/{code}/approve`, `POST /api/v1/auth/cli/sessions/{code}/deny` — browser sign-in for `pad auth login`: the CLI creates a session and polls it, and a signed-in user approves it at `/auth/cli/{code}`, which mints a 30-day sliding session the poll hands to the CLI, with its own lifetime: it is dated from the approval (hard cap 90 days from then), not from the approving web session's sign-in, so approving from an old web session does not shorten it (TASK-3975). Only a web-kind session approves (the browser's cookie session, or a web session sent as a bearer). A CLI-kind bearer gets `403 session_required`, because a CLI token that could approve could mint its own successor and chain past the cap (TASK-3975 r2); a CLI-kind session sent as a cookie is not a credential at all (BUG-3350) and gets `401 unauthorized`; an API token gets `403 session_required`. Since TASK-2253 the session records `requester_ip` (the server's view of the creating client) and `requester_user_agent` (what the creator sent, capped at 200 characters, so requester-controlled text), and a PENDING poll returns them with `created_at` (additive, omitempty) for the approval page, which renders the agent as text labelled "reported by the requester". `deny` checks no account, because the code is the authority (it is already a bearer secret: whoever holds it can poll and collect an approval's token); it moves pending → denied, answers 200 again on a denied session, and 409 `already_approved` / 410 `expired` otherwise. A denied session POLLS as **410 `cli_auth_denied`**, not as a 200 status: a CLI from before Deny has no case for an unknown status and would poll it for 20 minutes, whereas a non-2xx counts as a poll error there and it stops after five in a row, printing the message; a current CLI stops on the first. Approving a denied session answers 409 `cli_auth_denied`, and an approval that loses to a concurrent deny ends the session it minted - `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. **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 - **`scopes` on create** (TASK-3866), on both `POST /auth/tokens` and `POST /workspaces/{ws}/tokens`: a STRING holding a JSON array of `*`, `read`, `write`, `pad:read`, `pad:write`, `pad:admin` (exactly what the request-time check `tokenScopeAllows` recognizes). It is stored compact (`[ "read" ]` → `["read"]`). Only an omitted field keeps the default `["*"]`. Refused `400 validation_error`, with the message naming the bad value: a `scopes` field that is JSON `null` or not a string (an explicit null used to read as omitted and mint a full-access key), the empty string and the empty array (`""` and `[]` are the legacy unrestricted forms, and no new key is minted in them; omit `scopes` instead), a string that is not a JSON array of strings (including `"null"`), and any value outside the vocabulary. These used to be stored verbatim, minting a key that every request then refused. There are two levels: `read`/`pad:read` allow GET, HEAD and OPTIONS (plus the playbook `run` POST), and every other recognized value allows everything the user's workspace role allows. `pad:admin` grants nothing beyond `write` for an API key. The web settings form offers "Read and write" (`["*"]`) and "Read only" (`["read"]`), and the token list labels each key from `web/src/lib/settings/tokenScopes.ts`, which is held to the server by `internal/server/testdata/token_scope_levels.json`. diff --git a/internal/server/handlers_account.go b/internal/server/handlers_account.go index bdbefa58a..a2a40494a 100644 --- a/internal/server/handlers_account.go +++ b/internal/server/handlers_account.go @@ -36,7 +36,9 @@ func writeAccountDisabledIf(w http.ResponseWriter, err error) bool { // Without a password to check, a recent sign-in is the proof of identity: a // stolen or long-lived session is not enough, and the real user signs in // again. A credential-change rotation does not restart the clock -// (CreateSessionIssuedAt). +// (CreateSessionIssuedAt). A CLI-kind session never satisfies it (TASK-3975): +// a CLI token approved from a web session has its own created_at, so its age +// says nothing about when anyone signed in. const accountDeleteReauthWindow = 10 * time.Minute // cookieKindAccepted reports whether a session may be used as a browser @@ -141,7 +143,7 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { // linked OAuth, reads false too. So confirm-only also needs a recent // sign-in (codex review). info := s.requestSessionInfo(r) - if info == nil || info.User.ID != fullUser.ID || time.Since(info.CreatedAt) > accountDeleteReauthWindow { + if info == nil || info.User.ID != fullUser.ID || info.Kind == store.SessionKindCLI || time.Since(info.CreatedAt) > accountDeleteReauthWindow { writeError(w, http.StatusForbidden, "reauth_required", "For your security, sign in again to delete your account") return diff --git a/internal/server/handlers_cli_auth.go b/internal/server/handlers_cli_auth.go index 4ba7543b6..a3bf19f12 100644 --- a/internal/server/handlers_cli_auth.go +++ b/internal/server/handlers_cli_auth.go @@ -184,18 +184,29 @@ func (s *Server) handleApproveCLIAuthSession(w http.ResponseWriter, r *http.Requ return } - // Create a new session token for the CLI (long-lived, 30 days). It is - // dated by the approving session's sign-in (BUG-3336): approving from an - // existing session is not a fresh sign-in, so it must not hand out a - // session that reads as one. + // Create a new session token for the CLI (long-lived, 30 days sliding, + // capped at 90 days from this approval). It is a new credential on another + // device, so it gets its own lifetime (TASK-3975): inheriting the approving + // web session's age would hand a CLI approved from an 85-day-old session + // only 5 days. It still must not read as a fresh sign-in (BUG-3336); the + // confirm-only account delete refuses CLI-kind sessions outright. approver := s.requestSessionInfo(r) if approver == nil || approver.User.ID != user.ID { writeError(w, http.StatusUnauthorized, "unauthorized", "You must be logged in to approve a CLI session") return } + // Only a browser session approves (TASK-3975 r2). The token minted here + // starts its own 90-day cap, so a CLI token that could approve could mint + // its own successor, and a chain of them would never expire. The web + // client approves on its cookie; nothing else calls this. + if approver.Kind != store.SessionKindWeb { + writeError(w, http.StatusForbidden, "session_required", + "Approving a CLI sign-in requires a browser session") + return + } // Fenced on the approver's credential epoch as this request was admitted // (BUG-3382): a claim or reset since then makes the approval mint nothing. - token, err := s.store.CreateSessionFenced(user.ID, user.CredentialEpoch, "cli-browser-auth", clientIP(r), "", 30*24*time.Hour, approver.CreatedAt) + token, err := s.store.CreateSessionFenced(user.ID, user.CredentialEpoch, "cli-browser-auth", clientIP(r), "", 30*24*time.Hour, time.Time{}) if errors.Is(err, store.ErrUserDisabled) { // Disabled between this request's admission and the mint (BUG-3349). writeError(w, http.StatusForbidden, "account_disabled", "Your account has been disabled. Contact an administrator.") diff --git a/internal/server/task3975_cli_token_lifetime_test.go b/internal/server/task3975_cli_token_lifetime_test.go new file mode 100644 index 000000000..93ef1950b --- /dev/null +++ b/internal/server/task3975_cli_token_lifetime_test.go @@ -0,0 +1,150 @@ +package server + +import ( + "crypto/sha256" + "encoding/hex" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/store" +) + +// TASK-3975: a CLI token minted by approving from a web session gets its own +// lifetime (30-day slide, 90-day cap from the approval), not the approving +// session's remaining age. + +func within(t *testing.T, what string, got, want time.Time, tol time.Duration) { + t.Helper() + if d := got.Sub(want); d > tol || d < -tol { + t.Errorf("%s = %v, want %v +/- %v", what, got, want, tol) + } +} + +func sessionExpiry(t *testing.T, srv *Server, token string) time.Time { + t.Helper() + var s string + sum := sha256.Sum256([]byte(token)) + if err := srv.store.DB().QueryRow(`SELECT expires_at FROM sessions WHERE token_hash = ?`, hex.EncodeToString(sum[:])).Scan(&s); err != nil { + t.Fatal(err) + } + exp, err := time.Parse(time.RFC3339, s) + if err != nil { + t.Fatal(err) + } + return exp +} + +func TestTASK3975_CLIApprovalFromAnEightyFiveDayWebSession_GetsFullWindow(t *testing.T) { + srv, u, token := oauthOnlyCloud(t) + ageAllSessions(t, srv, u.ID, 85*24*time.Hour) + pending, err := srv.store.CreateCLIAuthSession() + if err != nil { + t.Fatal(err) + } + if rr := doRequestWithCookieFrom(srv, "POST", "/api/v1/auth/cli/sessions/"+pending.Code+"/approve", nil, token, "198.51.100.7:5555"); rr.Code != http.StatusOK { + t.Fatalf("approve: %d %s", rr.Code, rr.Body.String()) + } + approved, err := srv.store.GetCLIAuthSession(pending.Code) + if err != nil || approved == nil || approved.Token == "" { + t.Fatalf("approved CLI session: %+v %v", approved, err) + } + info, err := srv.store.ValidateSession(approved.Token) + if err != nil || info == nil { + t.Fatalf("validate: %v", err) + } + now := time.Now() + within(t, "created_at", info.CreatedAt, now, time.Minute) + within(t, "expires_at", sessionExpiry(t, srv, approved.Token), now.Add(30*24*time.Hour), time.Minute) + + // Sliding renewal reaches the full 30 days, not the approver's remaining 5. + if _, err := srv.store.DB().Exec(`UPDATE sessions SET expires_at = ? WHERE user_id = ? AND kind = ?`, + now.Add(10*24*time.Hour).UTC().Format(time.RFC3339), u.ID, store.SessionKindCLI); err != nil { + t.Fatal(err) + } + newExp, renewed, err := srv.store.RenewSessionIfStale(approved.Token) + if err != nil || !renewed { + t.Fatalf("renew: renewed=%v err=%v", renewed, err) + } + within(t, "renewed expiry", newExp, now.Add(30*24*time.Hour), time.Minute) +} + +// Dating the CLI token from the approval must not make it a fresh sign-in +// for the confirm-only delete recency check (BUG-3336). +func TestTASK3975_CLITokenFromOldSessionStillCannotConfirmDelete(t *testing.T) { + srv, u, token := oauthOnlyCloud(t) + ageAllSessions(t, srv, u.ID, 85*24*time.Hour) + pending, _ := srv.store.CreateCLIAuthSession() + if rr := doRequestWithCookieFrom(srv, "POST", "/api/v1/auth/cli/sessions/"+pending.Code+"/approve", nil, token, "198.51.100.7:5555"); rr.Code != http.StatusOK { + t.Fatalf("approve: %d %s", rr.Code, rr.Body.String()) + } + approved, _ := srv.store.GetCLIAuthSession(pending.Code) + rr := doRequestWithBearer(srv, "POST", "/api/v1/auth/delete-account", approved.Token, map[string]any{"confirm": true}) + if rr.Code != http.StatusForbidden || errorCode(t, rr) != "reauth_required" { + t.Fatalf("confirm-only with a fresh CLI token: %d %s, want 403 reauth_required", rr.Code, rr.Body.String()) + } + stillThere(t, srv, u.ID) +} + +// A credential-change rotation REPLACES the session, so it keeps the +// replaced session's cap (it must not extend a stolen session's life). +func TestTASK3975_RotationKeepsParentCap(t *testing.T) { + srv, u, _ := oauthOnlyCloud(t) + // A 7-day web session, so the 90-day cap (5 days away) binds rather than + // the session's own window, which a rotation keeps (BUG-3978). + token, err := srv.store.CreateSession(u.ID, "web", "198.51.100.7", "", webSessionTTL) + if err != nil { + t.Fatal(err) + } + ageAllSessions(t, srv, u.ID, 85*24*time.Hour) + req := httptest.NewRequest("POST", "/api/v1/auth/totp/enable", nil) + req.AddCookie(&http.Cookie{Name: sessionCookieName(srv.secureCookies), Value: token}) + rotated, ok := srv.rotateSessionsAfterCredentialChange(httptest.NewRecorder(), req, u) + if !ok { + t.Fatal("rotation failed") + } + info, err := srv.store.ValidateSession(rotated) + if err != nil || info == nil { + t.Fatalf("rotated session: %v", err) + } + within(t, "rotated created_at", info.CreatedAt, time.Now().Add(-85*24*time.Hour), time.Minute) + within(t, "rotated expiry", sessionExpiry(t, srv, rotated), time.Now().Add(5*24*time.Hour), time.Minute) +} + +// TASK-3975 r2: only a browser session approves. A CLI token that could +// approve would mint its own successor with a fresh 90-day cap, and a chain +// of them would never expire. An 85-day-old CLI bearer is refused and mints +// nothing. +func TestTASK3975_CLIBearerCannotApprove(t *testing.T) { + t.Parallel() + srv, u, _ := oauthOnlyCloud(t) + cli, err := srv.store.CreateSession(u.ID, "cli-browser-auth", "198.51.100.7", "", 30*24*time.Hour) + if err != nil { + t.Fatal(err) + } + ageAllSessions(t, srv, u.ID, 85*24*time.Hour) + countSessions := func() int { + var n int + if err := srv.store.DB().QueryRow(`SELECT COUNT(*) FROM sessions WHERE user_id = ?`, u.ID).Scan(&n); err != nil { + t.Fatal(err) + } + return n + } + before := countSessions() + pending, err := srv.store.CreateCLIAuthSession() + if err != nil { + t.Fatal(err) + } + rr := doRequestWithBearer(srv, "POST", "/api/v1/auth/cli/sessions/"+pending.Code+"/approve", cli, nil) + if rr.Code != http.StatusForbidden || errorCode(t, rr) != "session_required" { + t.Fatalf("approve with a CLI bearer: %d %s, want 403 session_required", rr.Code, rr.Body.String()) + } + if after := countSessions(); after != before { + t.Fatalf("a refused approval left %d sessions, want %d", after, before) + } + got, err := srv.store.GetCLIAuthSession(pending.Code) + if err != nil || got == nil || got.Status != "pending" || got.Token != "" { + t.Fatalf("the CLI auth session after a refused approval: %+v %v, want pending with no token", got, err) + } +}