diff --git a/internal/models/activity.go b/internal/models/activity.go index 220fc2153..42755cfa0 100644 --- a/internal/models/activity.go +++ b/internal/models/activity.go @@ -44,6 +44,10 @@ const ( // ActionEmailVerified is logged when a user confirms their email address // via a verification link (POST /auth/verify-email). PLAN-1933 / TASK-1936. ActionEmailVerified = "email_verified" + // ActionAccountClaimed is logged when the owner of an address claims a + // never-verified account registered with it, through the verification + // link (BUG-3382): every credential is reset. + ActionAccountClaimed = "account_claimed" // ActionEmailVerifiedByAdmin is logged when an admin force-verifies a // user's email from the admin console (DR-7). PLAN-1933 / TASK-1936. ActionEmailVerifiedByAdmin = "email_verified_by_admin" diff --git a/internal/models/user.go b/internal/models/user.go index b54abaa96..f1a168fea 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -7,29 +7,33 @@ import ( // User represents a registered user in the system. type User struct { - ID string `json:"id"` - Email string `json:"email"` - Username string `json:"username"` // Unique handle; empty until set - Name string `json:"name"` - PasswordHash string `json:"-"` // Never serialized - Role string `json:"role"` // "admin" or "member" - AvatarURL string `json:"avatar_url,omitempty"` - TOTPSecret string `json:"-"` // Never serialized - TOTPEnabled bool `json:"totp_enabled"` - RecoveryCodes string `json:"-"` // Never serialized - Plan string `json:"plan"` // "free", "pro", or "self-hosted" - PlanExpiresAt string `json:"plan_expires_at,omitempty"` - PlanSource string `json:"-"` // Who set Plan: "manual" or "stripe" (TASK-3295); surfaced only on admin user responses - StripeCustomerID string `json:"-"` // Never serialized - PlanOverrides string `json:"plan_overrides,omitempty"` // JSON overrides for per-user limits - OAuthProviders string `json:"-"` // JSON array of linked providers, e.g. ["github","google"] - PasswordSet bool `json:"password_set"` // True if the user explicitly set a password (vs. OAuth placeholder hash) - DisabledAt string `json:"disabled_at,omitempty"` // Non-empty = account disabled - EmailVerifiedAt string `json:"email_verified_at,omitempty"` // Non-empty = email verified (mirror DisabledAt). Empty/NULL = unverified. PLAN-1933 / TASK-1935. - LastActiveAt string `json:"last_active_at,omitempty"` // Last authenticated API request (any read or write) - LastWriteAt string `json:"last_write_at,omitempty"` // Last mutating action (item/comment/attachment); see Store.TouchUserWrite. PLAN-1542 / TASK-1543. - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + ID string `json:"id"` + Email string `json:"email"` + Username string `json:"username"` // Unique handle; empty until set + Name string `json:"name"` + PasswordHash string `json:"-"` // Never serialized + Role string `json:"role"` // "admin" or "member" + AvatarURL string `json:"avatar_url,omitempty"` + TOTPSecret string `json:"-"` // Never serialized + TOTPEnabled bool `json:"totp_enabled"` + RecoveryCodes string `json:"-"` // Never serialized + Plan string `json:"plan"` // "free", "pro", or "self-hosted" + PlanExpiresAt string `json:"plan_expires_at,omitempty"` + PlanSource string `json:"-"` // Who set Plan: "manual" or "stripe" (TASK-3295); surfaced only on admin user responses + StripeCustomerID string `json:"-"` // Never serialized + PlanOverrides string `json:"plan_overrides,omitempty"` // JSON overrides for per-user limits + OAuthProviders string `json:"-"` // JSON array of linked providers, e.g. ["github","google"] + PasswordSet bool `json:"password_set"` // True if the user explicitly set a password (vs. OAuth placeholder hash) + // CredentialEpoch is bumped by every credential change (password reset or + // change, disable, account claim). A sign-in mints a session only if the + // epoch it read when checking the credential is still current (BUG-3382). + CredentialEpoch int64 `json:"-"` + DisabledAt string `json:"disabled_at,omitempty"` // Non-empty = account disabled + EmailVerifiedAt string `json:"email_verified_at,omitempty"` // Non-empty = email verified (mirror DisabledAt). Empty/NULL = unverified. PLAN-1933 / TASK-1935. + LastActiveAt string `json:"last_active_at,omitempty"` // Last authenticated API request (any read or write) + LastWriteAt string `json:"last_write_at,omitempty"` // Last mutating action (item/comment/attachment); see Store.TouchUserWrite. PLAN-1542 / TASK-1543. + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } // IsDisabled returns true if the user account has been disabled. @@ -123,6 +127,12 @@ type UserUpdate struct { Username *string `json:"username,omitempty"` Password *string `json:"password,omitempty"` // Plaintext, will be hashed AvatarURL *string `json:"avatar_url,omitempty"` + // ExpectedEpoch, when set with Password, makes the password write + // conditional on the account's credential_epoch still being this value: + // the epoch the caller's authority (the current password, a reset token) + // was checked under (BUG-3382). A mismatch writes nothing and returns + // store.ErrCredentialsChanged. + ExpectedEpoch *int64 `json:"-"` } // Session represents a database-backed authentication session. diff --git a/internal/server/bug3382_verify_claim_test.go b/internal/server/bug3382_verify_claim_test.go new file mode 100644 index 000000000..f683b20a5 --- /dev/null +++ b/internal/server/bug3382_verify_claim_test.go @@ -0,0 +1,305 @@ +package server + +import ( + "net/http" + "strings" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-3382: a squatter registers the victim's address; the verification +// link goes to the victim. Clicking it used to verify the SQUATTER's account +// (keeping the squatter's password and session). Now a link verifies only for +// a request signed in as that account, and the mailbox owner can instead +// CLAIM the address: every credential is reset and they set their own. + +type squatFixture struct { + srv *Server + mails chan capturedMail + squatter *models.User + sessTok string // the squatter's session + token string // the verification token, as mailed to the victim + adminTok string + adminWS *models.Workspace + adminItem *models.Item +} + +func newSquatFixture(t *testing.T) squatFixture { + t.Helper() + srv := testServer(t) + adminTok := bootstrapFirstUser(t, srv, "admin@pad.test", "Admin") + sender, mails := newMailSink(t) + srv.baseURL = "https://app.getpad.dev" + srv.SetEmailSender(sender) + srv.cloudMode = true + + rr := doRequest(srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "victim@example.com", "name": "Squatter", "password": "squatter-password-123", + }) + if rr.Code != http.StatusCreated { + t.Fatalf("squatter register: %d %s", rr.Code, rr.Body.String()) + } + var reg struct { + Token string `json:"token"` + } + parseJSON(t, rr, ®) + token := extractVerifyToken(t, waitMail(t, mails)) + squatter, err := srv.store.GetUserByEmail("victim@example.com") + if err != nil || squatter == nil { + t.Fatalf("lookup: %v", err) + } + admin, _ := srv.store.GetUserByEmail("admin@pad.test") + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Admin WS", OwnerID: admin.ID}) + if err != nil { + t.Fatal(err) + } + if err := srv.store.AddWorkspaceMember(ws.ID, admin.ID, "owner"); err != nil { + t.Fatal(err) + } + item := mustItem(t, srv, ws.ID, mustCollection(t, srv, ws.ID, "Tasks").ID, "shared") + return squatFixture{srv, mails, squatter, reg.Token, token, adminTok, ws, item} +} + +// What a squatter can plant before the claim: membership elsewhere with a +// grant, a watch and a tab there, and credentials of every kind. +func (f squatFixture) plant(t *testing.T) { + t.Helper() + s := f.srv.store + admin, _ := s.GetUserByEmail("admin@pad.test") + if err := s.AddWorkspaceMember(f.adminWS.ID, f.squatter.ID, "viewer"); err != nil { + t.Fatal(err) + } + if _, err := s.CreateItemGrant(f.adminWS.ID, f.adminItem.ID, f.squatter.ID, "view", admin.ID); err != nil { + t.Fatal(err) + } + if _, err := s.CreateWatch(f.adminWS.ID, f.squatter.ID, f.adminItem.ID, ""); err != nil { + t.Fatal(err) + } + if _, err := s.OpenWorkspaceTab(f.squatter.ID, f.adminWS.ID, false); err != nil { + t.Fatal(err) + } + if _, err := s.CreateAPIToken(f.squatter.ID, models.APITokenCreate{Name: "planted"}, 30, 0); err != nil { + t.Fatal(err) + } + cli, err := s.CreateCLIAuthSession() + if err != nil { + t.Fatal(err) + } + cliTok, err := s.CreateSession(f.squatter.ID, "cli-browser-auth", "192.0.2.1", "", time.Hour) + if err != nil { + t.Fatal(err) + } + if err := s.ApproveCLIAuthSession(cli.Code, cliTok, f.squatter.ID); err != nil { + t.Fatal(err) + } +} + +func count(t *testing.T, srv *Server, query string, args ...any) int { + t.Helper() + var n int + if err := srv.store.DB().QueryRow(query, args...).Scan(&n); err != nil { + t.Fatalf("%s: %v", query, err) + } + return n +} + +func TestBUG3382_LinkWithoutTheAccountsSessionDoesNotVerify(t *testing.T) { + f := newSquatFixture(t) + + // The victim clicks with no session: nothing is verified, the token is + // not spent, and the squatter still holds what they held. + rr := doRequest(f.srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": f.token}) + if rr.Code != http.StatusConflict || !strings.Contains(rr.Body.String(), "verify_needs_session") { + t.Fatalf("verify without a session: %d %s", rr.Code, rr.Body.String()) + } + if u, _ := f.srv.store.GetUser(f.squatter.ID); u.IsEmailVerified() { + t.Fatal("a link clicked without the account's session verified it") + } + + // Signed in as someone else: the same answer. + rr = doRequestWithCookie(f.srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": f.token}, f.adminTok) + if rr.Code != http.StatusConflict { + t.Fatalf("verify as another account: %d %s", rr.Code, rr.Body.String()) + } + + // Control: the account's own session verifies, so the token survived. + rr = doRequestWithCookie(f.srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": f.token}, f.sessTok) + if rr.Code != http.StatusOK { + t.Fatalf("verify with the account's session: %d %s", rr.Code, rr.Body.String()) + } +} + +func TestBUG3382_MailboxOwnerClaimsTheAddress(t *testing.T) { + f := newSquatFixture(t) + f.plant(t) + s := f.srv.store + owned := count(t, f.srv, `SELECT COUNT(*) FROM workspaces WHERE owner_id = ? AND deleted_at IS NULL`, f.squatter.ID) + if owned == 0 { + t.Fatal("precondition: signup should have created the squatter's workspace") + } + + rr := doRequest(f.srv, "POST", "/api/v1/auth/verify-email/claim", map[string]string{"token": f.token}) + if rr.Code != http.StatusOK { + t.Fatalf("claim: %d %s", rr.Code, rr.Body.String()) + } + var claim struct { + ResetPath string `json:"reset_path"` + StrippedWorkspaces []string `json:"stripped_workspaces"` + DeletedWorkspaces []string `json:"deleted_workspaces"` + } + parseJSON(t, rr, &claim) + if !strings.HasPrefix(claim.ResetPath, "/reset-password/") { + t.Fatalf("no set-password path: %+v", claim) + } + if len(claim.StrippedWorkspaces) != 1 || claim.StrippedWorkspaces[0] != "Admin WS" || len(claim.DeletedWorkspaces) != owned { + t.Errorf("claim reported %+v", claim) + } + + u, _ := s.GetUser(f.squatter.ID) + if !u.IsEmailVerified() || u.PasswordSet || u.TOTPEnabled { + t.Errorf("after the claim: verified=%v password_set=%v totp=%v", u.IsEmailVerified(), u.PasswordSet, u.TOTPEnabled) + } + // The squatter's session, password, PAT and CLI handoff are dead. + if rr := doRequestWithCookie(f.srv, "GET", "/api/v1/auth/me", nil, f.sessTok); rr.Code == http.StatusOK { + t.Error("the squatter's session survived the claim") + } + if ok, _ := s.ValidatePassword("victim@example.com", "squatter-password-123"); ok != nil { + t.Error("the squatter's password survived the claim") + } + for what, q := range map[string]string{ + "sessions": `SELECT COUNT(*) FROM sessions WHERE user_id = ?`, + "api tokens": `SELECT COUNT(*) FROM api_tokens WHERE user_id = ?`, + "cli handoffs": `SELECT COUNT(*) FROM cli_auth_sessions WHERE user_id = ?`, + "memberships": `SELECT COUNT(*) FROM workspace_members WHERE user_id = ? AND workspace_id = '` + f.adminWS.ID + `'`, + "item grants": `SELECT COUNT(*) FROM item_grants WHERE user_id = ?`, + "watches": `SELECT COUNT(*) FROM watches WHERE user_id = ?`, + "tabs": `SELECT COUNT(*) FROM user_workspace_tabs WHERE user_id = ?`, + "owned live ws": `SELECT COUNT(*) FROM workspaces WHERE owner_id = ? AND deleted_at IS NULL`, + } { + if n := count(t, f.srv, q, f.squatter.ID); n != 0 { + t.Errorf("%s survived the claim: %d", what, n) + } + } + + // The claimant sets a password through the link and is signed in. + resetToken := strings.TrimPrefix(claim.ResetPath, "/reset-password/") + rr = doRequest(f.srv, "POST", "/api/v1/auth/reset-password", map[string]string{"token": resetToken, "password": "the-real-owners-password-9"}) + if rr.Code != http.StatusOK { + t.Fatalf("set password: %d %s", rr.Code, rr.Body.String()) + } + var cookie string + for _, c := range rr.Result().Cookies() { + if c.Name == sessionCookieName(f.srv.secureCookies) { + cookie = c.Value + } + } + if rr := doRequestWithCookie(f.srv, "GET", "/api/v1/auth/me", nil, cookie); rr.Code != http.StatusOK { + t.Errorf("the claimant's new session does not work: %d", rr.Code) + } + + // The token is spent: a second claim is refused. + if rr := doRequest(f.srv, "POST", "/api/v1/auth/verify-email/claim", map[string]string{"token": f.token}); rr.Code != http.StatusBadRequest { + t.Errorf("a spent token claimed again: %d", rr.Code) + } +} + +func TestBUG3382_ClaimRefusals(t *testing.T) { + t.Run("billing attached", func(t *testing.T) { + f := newSquatFixture(t) + if _, err := f.srv.store.DB().Exec(`UPDATE users SET stripe_customer_id = 'cus_x' WHERE id = ?`, f.squatter.ID); err != nil { + t.Fatal(err) + } + rr := doRequest(f.srv, "POST", "/api/v1/auth/verify-email/claim", map[string]string{"token": f.token}) + if rr.Code != http.StatusConflict || !strings.Contains(rr.Body.String(), "account_claim_needs_support") { + t.Fatalf("claim with billing: %d %s", rr.Code, rr.Body.String()) + } + if rr := doRequestWithCookie(f.srv, "GET", "/api/v1/auth/me", nil, f.sessTok); rr.Code != http.StatusOK { + t.Error("a refused claim still reset the account") + } + }) + t.Run("already verified", func(t *testing.T) { + f := newSquatFixture(t) + if err := f.srv.store.SetUserEmailVerified(f.squatter.ID); err != nil { + t.Fatal(err) + } + if rr := doRequest(f.srv, "POST", "/api/v1/auth/verify-email/claim", map[string]string{"token": f.token}); rr.Code != http.StatusBadRequest { + t.Fatalf("claim of a verified account: %d %s", rr.Code, rr.Body.String()) + } + }) +} + +// The credential epoch: a sign-in that checked its credential before a +// credential change mints nothing after it. +func TestBUG3382_StaleCredentialCheckMintsNoSession(t *testing.T) { + f := newSquatFixture(t) + s := f.srv.store + checked, _ := s.GetUser(f.squatter.ID) // a sign-in read the account here + + if _, err := s.UpdateUser(f.squatter.ID, models.UserUpdate{Password: ptr("a-new-password-123")}); err != nil { + t.Fatal(err) + } + if _, err := s.CreateSessionFenced(checked.ID, checked.CredentialEpoch, "web", "192.0.2.1", "", time.Hour, time.Time{}); err != store.ErrCredentialsChanged { + t.Errorf("a session minted on a stale credential check: err=%v", err) + } + fresh, _ := s.GetUser(f.squatter.ID) + if _, err := s.CreateSessionFenced(fresh.ID, fresh.CredentialEpoch, "web", "192.0.2.1", "", time.Hour, time.Time{}); err != nil { + t.Errorf("control: a current check could not mint: %v", err) + } + + // The 2FA challenge carries the first factor's epoch. + ch := generateTwoFAChallengeAt(checked.ID, "192.0.2.1", checked.CredentialEpoch, []byte("k")) + if id, epoch, err := validateTwoFAChallengeEpoch(ch, "192.0.2.1", []byte("k")); err != nil || id != checked.ID || epoch != checked.CredentialEpoch { + t.Errorf("challenge round trip: %q %d %v", id, epoch, err) + } +} + +// The cookie fallbacks in PATCH /auth/me and CLI approval do not let an +// unverified account write (codex: SessionAuth resolves the cookie first). +func TestBUG3382_UnverifiedCookieCannotUseFallbackRoutes(t *testing.T) { + f := newSquatFixture(t) + rr := doRequestWithCookie(f.srv, "PATCH", "/api/v1/auth/me", map[string]string{"name": "Renamed"}, f.sessTok) + if rr.Code != http.StatusForbidden || veErrorCode(rr) != "email_not_verified" { + t.Errorf("PATCH /auth/me as an unverified account: %d %s", rr.Code, rr.Body.String()) + } + cli, err := f.srv.store.CreateCLIAuthSession() + if err != nil { + t.Fatal(err) + } + rr = doRequestWithCookie(f.srv, "POST", "/api/v1/auth/cli/sessions/"+cli.Code+"/approve", nil, f.sessTok) + if rr.Code != http.StatusForbidden || veErrorCode(rr) != "email_not_verified" { + t.Errorf("CLI approval as an unverified account: %d %s", rr.Code, rr.Body.String()) + } +} + +func ptr[T any](v T) *T { return &v } + +// Opening the link a few times, claiming, then setting the password must fit +// one address's budget: the claim has already reset the old credentials, so +// a 429 on the set-password step strands the claimant. +func TestBUG3382_ClaimThenSetPasswordFitsTheBudget(t *testing.T) { + f := newSquatFixture(t) + const addr = "198.51.100.77:1234" + for i := 0; i < 3; i++ { + rr := doRequestFromRemoteAddr(f.srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": f.token}, addr) + if rr.Code != http.StatusConflict { + t.Fatalf("open %d: %d %s", i, rr.Code, rr.Body.String()) + } + } + rr := doRequestFromRemoteAddr(f.srv, "POST", "/api/v1/auth/verify-email/claim", map[string]string{"token": f.token}, addr) + if rr.Code != http.StatusOK { + t.Fatalf("claim: %d %s", rr.Code, rr.Body.String()) + } + var claim struct { + ResetPath string `json:"reset_path"` + } + parseJSON(t, rr, &claim) + rr = doRequestFromRemoteAddr(f.srv, "POST", "/api/v1/auth/reset-password", map[string]string{ + "token": strings.TrimPrefix(claim.ResetPath, "/reset-password/"), "password": "the-real-owners-password-9", + }, addr) + if rr.Code != http.StatusOK { + t.Fatalf("set password: %d %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handlers_2fa.go b/internal/server/handlers_2fa.go index ed64ccc91..a7a947f9a 100644 --- a/internal/server/handlers_2fa.go +++ b/internal/server/handlers_2fa.go @@ -265,7 +265,7 @@ func (s *Server) handleTOTPLoginVerify(w http.ResponseWriter, r *http.Request) { } // Validate the challenge token (proves password was verified, checks IP + expiry) - userID, err := validateTwoFAChallenge(input.ChallengeToken, clientIP(r), s.twoFAChallengeSecret) + userID, challengeEpoch, err := validateTwoFAChallengeEpoch(input.ChallengeToken, clientIP(r), s.twoFAChallengeSecret) if err != nil { time.Sleep(500 * time.Millisecond) writeError(w, http.StatusUnauthorized, "unauthorized", "Invalid or expired 2FA challenge") @@ -393,7 +393,12 @@ func (s *Server) handleTOTPLoginVerify(w http.ResponseWriter, r *http.Request) { return } - // Create full session + // Create full session, fenced on the credential epoch the FIRST factor + // was checked under (BUG-3382): a password reset or account claim since + // then makes this challenge mint nothing. + if challengeEpoch >= 0 { + user.CredentialEpoch = challengeEpoch + } token, err := s.createAuthSession(w, r, user, webSessionTTL) if err != nil { return diff --git a/internal/server/handlers_auth.go b/internal/server/handlers_auth.go index ca8ef699d..3e93bf382 100644 --- a/internal/server/handlers_auth.go +++ b/internal/server/handlers_auth.go @@ -348,7 +348,9 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h if info.Kind == store.SessionKindCLI { device = "cli" } - token, err := s.store.CreateSessionIssuedAt(user.ID, device, clientIP(r), r.UserAgent(), webSessionTTL, info.CreatedAt) + // Fenced on the epoch of the user the caller passes, which is the + // credential state this rotation follows (BUG-3382). + token, err := s.store.CreateSessionFenced(user.ID, user.CredentialEpoch, device, clientIP(r), r.UserAgent(), webSessionTTL, info.CreatedAt) if errors.Is(err, store.ErrUserDisabled) { writeError(w, http.StatusForbidden, "account_disabled", "Your account has been disabled. Contact an administrator.") return "", false @@ -382,7 +384,14 @@ func (s *Server) createAuthSession(w http.ResponseWriter, r *http.Request, user if isCLIClient(r) { device = "cli" } - token, err := s.store.CreateSession(user.ID, device, clientIP(r), r.UserAgent(), ttl) + // Fenced on the credential epoch read when the sign-in checked its + // credential (BUG-3382): user is the account as that check loaded it. + token, err := s.store.CreateSessionFenced(user.ID, user.CredentialEpoch, device, clientIP(r), r.UserAgent(), ttl, time.Time{}) + if errors.Is(err, store.ErrCredentialsChanged) { + writeError(w, http.StatusUnauthorized, "credentials_changed", + "Your sign-in details changed while you were signing in. Sign in again.") + return "", err + } if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to create session") return "", err @@ -1158,7 +1167,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { // If 2FA is enabled, return a challenge token instead of a full session. // The challenge token is HMAC-signed, IP-bound, and expires in 5 minutes. if user.TOTPEnabled { - challenge := generateTwoFAChallenge(user.ID, clientIP(r), s.twoFAChallengeSecret) + challenge := generateTwoFAChallengeAt(user.ID, clientIP(r), user.CredentialEpoch, s.twoFAChallengeSecret) writeJSON(w, http.StatusOK, map[string]interface{}{ "requires_2fa": true, "challenge_token": challenge, @@ -1403,6 +1412,7 @@ func (s *Server) handleUpdateCurrentUser(w http.ResponseWriter, r *http.Request) } // Validate password change + var passwordEpoch int64 if input.NewPassword != "" { if input.CurrentPassword == "" { writeError(w, http.StatusBadRequest, "validation_error", "Current password is required to set a new password") @@ -1438,6 +1448,7 @@ func (s *Server) handleUpdateCurrentUser(w http.ResponseWriter, r *http.Request) writeError(w, http.StatusForbidden, "invalid_password", "Current password is incorrect") return } + passwordEpoch = valid.CredentialEpoch } // Build update @@ -1447,9 +1458,17 @@ func (s *Server) handleUpdateCurrentUser(w http.ResponseWriter, r *http.Request) } if input.NewPassword != "" { update.Password = &input.NewPassword + // Fenced on the epoch the current password was checked under, so a + // claim or reset that lands in between is not overwritten (BUG-3382). + update.ExpectedEpoch = &passwordEpoch } updated, err := s.store.UpdateUser(user.ID, update) + if errors.Is(err, store.ErrCredentialsChanged) { + writeError(w, http.StatusUnauthorized, "credentials_changed", + "Your sign-in details changed while this request was in progress. Sign in again.") + return + } if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to update profile") return @@ -1713,9 +1732,18 @@ func (s *Server) handleResetPassword(w http.ResponseWriter, r *http.Request) { } // Update password + // Fenced on the epoch read as the token was spent (BUG-3382): a claim or + // another password change that landed since wins, and this reset writes + // nothing. password := input.Password - update := models.UserUpdate{Password: &password} - if _, err := s.store.UpdateUser(user.ID, update); err != nil { + update := models.UserUpdate{Password: &password, ExpectedEpoch: &user.CredentialEpoch} + updated, err := s.store.UpdateUser(user.ID, update) + if errors.Is(err, store.ErrCredentialsChanged) { + writeError(w, http.StatusConflict, "credentials_changed", + "This account's sign-in details changed after this link was opened. Request a new reset link.") + return + } + if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to update password") return } @@ -1741,8 +1769,9 @@ func (s *Server) handleResetPassword(w http.ResponseWriter, r *http.Request) { return } - // Create a fresh session so the user is logged in - sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL) + // Create a fresh session so the user is logged in, fenced on the epoch + // this reset produced (BUG-3382). + sessionToken, err := s.store.CreateSessionFenced(updated.ID, updated.CredentialEpoch, "web", clientIP(r), r.UserAgent(), webSessionTTL, time.Time{}) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Password updated but failed to create session") return @@ -1806,12 +1835,32 @@ func (s *Server) handleVerifyEmail(w http.ResponseWriter, r *http.Request) { return } + pending, err := s.store.LookupEmailVerification(input.Token) + if err != nil { + writeError(w, http.StatusInternalServerError, "internal_error", "Failed to verify email") + return + } // A disabled account does not act, here either (BUG-3349): refuse before // the token is consumed, so it still works if the account is re-enabled. - if pending, err := s.store.LookupEmailVerification(input.Token); err == nil && pending != nil && pending.IsDisabled() { + if pending != nil && pending.IsDisabled() { writeError(w, http.StatusForbidden, "account_disabled", "Your account has been disabled. Contact an administrator.") return } + // BUG-3382: the link verifies only for a request signed in as the + // account it names. Registration mails this link to the ADDRESS, and the + // address may belong to someone other than whoever registered it: a + // squatter's account, verified by the real owner's click, would keep the + // squatter's password and sessions. A clicker who is not signed in as + // that account is told to sign in, or, if it was not them, to claim the + // address (handleClaimByVerification). The token is not spent. + if pending != nil { + if cu := currentUser(r); cu == nil || cu.ID != pending.ID { + writeError2(w, http.StatusConflict, "verify_needs_session", + "Sign in to the account registered with this address to verify it. If you didn't register it, you can claim the address.", + map[string]interface{}{"email": pending.Email, "can_claim": !pending.IsEmailVerified()}) + return + } + } user, err := s.store.ConsumeEmailVerification(input.Token) if errors.Is(err, store.ErrUserDisabled) { writeError(w, http.StatusForbidden, "account_disabled", "Your account has been disabled. Contact an administrator.") @@ -1846,6 +1895,70 @@ func (s *Server) handleVerifyEmail(w http.ResponseWriter, r *http.Request) { }) } +// handleClaimByVerification: POST /api/v1/auth/verify-email/claim {token}. +// +// The verification link was mailed to the address, so whoever holds it owns +// the mailbox. When the account registered with that address was never +// verified, its credentials may be a squatter's (BUG-3382): the claim resets +// every one of them (store.ClaimAccountByVerification) and answers with a +// password-reset path, where the claimant sets a password and is signed in. +// The previous holder's live connections are kicked now. +func (s *Server) handleClaimByVerification(w http.ResponseWriter, r *http.Request) { + var input struct { + Token string `json:"token"` + } + if err := decodeJSON(r, &input); err != nil { + writeError(w, http.StatusBadRequest, "bad_request", "Invalid request body") + return + } + input.Token = strings.TrimSpace(input.Token) + if input.Token == "" { + writeError(w, http.StatusBadRequest, "validation_error", "Verification token is required") + return + } + claim, err := s.store.ClaimAccountByVerification(input.Token) + if errors.Is(err, store.ErrClaimNotEligible) { + writeError(w, http.StatusBadRequest, "invalid_token", + "This link is invalid or has expired, or the address is already verified. Request a new link, or sign in.") + return + } + if errors.Is(err, store.ErrClaimNeedsSupport) { + writeError(w, http.StatusConflict, "account_claim_needs_support", + "This account has billing attached, so it can't be claimed automatically. Contact support@getpad.dev and we'll sort it out.") + return + } + if err != nil { + writeInternalError(w, err) + return + } + // Every session of the account is gone; its live connections end now. + s.invalidateUserAccess(claim.UserID) + s.logAuditEventForUser(models.ActionAccountClaimed, r, claim.UserID, auditMeta(map[string]string{ + "stripped_workspaces": strconv.Itoa(len(claim.StrippedWorkspaces)), + "deleted_workspaces": strconv.Itoa(len(claim.DeletedWorkspaces)), + })) + + resetToken, err := s.store.CreatePasswordReset(claim.UserID) + if err != nil { + // The account is claimed and safe; the claimant can still use + // forgot-password to set a password. + slog.Error("account claim: could not mint the set-password link", "error", err, "user_id", claim.UserID) + writeJSON(w, http.StatusOK, map[string]interface{}{ + "claimed": true, + "reset_path": "", + "stripped_workspaces": nonNilStrings(claim.StrippedWorkspaces), + "deleted_workspaces": nonNilStrings(claim.DeletedWorkspaces), + }) + return + } + writeJSON(w, http.StatusOK, map[string]interface{}{ + "claimed": true, + "reset_path": "/reset-password/" + resetToken, + "stripped_workspaces": nonNilStrings(claim.StrippedWorkspaces), + "deleted_workspaces": nonNilStrings(claim.DeletedWorkspaces), + }) +} + // handleResendVerification re-sends an email-verification link for an // UNVERIFIED account (PLAN-1933 DR-5). // diff --git a/internal/server/handlers_cli_auth.go b/internal/server/handlers_cli_auth.go index 41eabec66..99ee47281 100644 --- a/internal/server/handlers_cli_auth.go +++ b/internal/server/handlers_cli_auth.go @@ -154,7 +154,9 @@ func (s *Server) handleApproveCLIAuthSession(w http.ResponseWriter, r *http.Requ writeError(w, http.StatusUnauthorized, "unauthorized", "You must be logged in to approve a CLI session") return } - token, err := s.store.CreateSessionIssuedAt(user.ID, "cli-browser-auth", clientIP(r), "", 30*24*time.Hour, approver.CreatedAt) + // 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) 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/handlers_cloud.go b/internal/server/handlers_cloud.go index a27307a8e..9d5780f49 100644 --- a/internal/server/handlers_cloud.go +++ b/internal/server/handlers_cloud.go @@ -291,7 +291,7 @@ func (s *Server) handleOAuthLogin(w http.ResponseWriter, r *http.Request) { "email": input.Email, "reason": "two_factor_required", })) - challenge := generateTwoFAChallenge(user.ID, clientIP(r), s.twoFAChallengeSecret) + challenge := generateTwoFAChallengeAt(user.ID, clientIP(r), user.CredentialEpoch, s.twoFAChallengeSecret) writeError2(w, http.StatusForbidden, "two_factor_required", "This account has two-factor authentication turned on. Sign in on the web to finish with your 2FA code.", map[string]interface{}{"challenge_token": challenge}) diff --git a/internal/server/handlers_verify_email_test.go b/internal/server/handlers_verify_email_test.go index e74229eed..23548549e 100644 --- a/internal/server/handlers_verify_email_test.go +++ b/internal/server/handlers_verify_email_test.go @@ -181,8 +181,9 @@ func TestCloudSignup_UnverifiedThenVerifyUnblocksSameSession(t *testing.T) { t.Fatalf("pre-verify item-create: expected 403 email_not_verified, got %d: %s", rr.Code, rr.Body.String()) } - // Verify the email. - rr = doRequest(srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": token}) + // Verify the email, from the session that registered it (BUG-3382: a + // link verifies only for a request signed in as its account). + rr = doRequestWithCookie(srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": token}, reg.Token) if rr.Code != http.StatusOK { t.Fatalf("verify-email: expected 200, got %d: %s", rr.Code, rr.Body.String()) } @@ -251,6 +252,10 @@ func TestResendVerification_InvalidatesPriorTokenAndIsEnumerationSafe(t *testing if rr.Code != http.StatusCreated { t.Fatalf("register: expected 201, got %d: %s", rr.Code, rr.Body.String()) } + var reg struct { + Token string `json:"token"` + } + parseJSON(t, rr, ®) firstToken := extractVerifyToken(t, waitMail(t, mails)) // Resend → always 200, and a NEW token lands in the inbox. @@ -269,8 +274,9 @@ func TestResendVerification_InvalidatesPriorTokenAndIsEnumerationSafe(t *testing t.Fatalf("stale first token: expected 400 (invalidated by resend), got %d: %s", rr.Code, rr.Body.String()) } - // The new token still verifies. - rr = post("/api/v1/auth/verify-email", map[string]string{"token": secondToken}) + // The new token still verifies, for the account's own session (BUG-3382). + ipN++ + rr = doRequestWithCookieFrom(srv, "POST", "/api/v1/auth/verify-email", map[string]string{"token": secondToken}, reg.Token, "203.0.113."+strconv.Itoa(ipN)+":1234") if rr.Code != http.StatusOK { t.Fatalf("new token verify: expected 200, got %d: %s", rr.Code, rr.Body.String()) } diff --git a/internal/server/middleware_csrf.go b/internal/server/middleware_csrf.go index 2a5ecc44c..b28dfc908 100644 --- a/internal/server/middleware_csrf.go +++ b/internal/server/middleware_csrf.go @@ -75,6 +75,7 @@ var authCSRFUnconditionalExemptPaths = map[string]bool{ "/api/v1/auth/reset-password": true, "/api/v1/auth/local-reset": true, "/api/v1/auth/verify-email": true, + "/api/v1/auth/verify-email/claim": true, // BUG-3382: pre-auth, proven by the mailed token "/api/v1/auth/resend-verification": true, "/api/v1/auth/2fa/login-verify": true, "/api/v1/auth/oauth-login": true, diff --git a/internal/server/middleware_ratelimit.go b/internal/server/middleware_ratelimit.go index 87e984c97..4bdb567ac 100644 --- a/internal/server/middleware_ratelimit.go +++ b/internal/server/middleware_ratelimit.go @@ -259,6 +259,8 @@ type RateLimiters struct { AuthEmail *ipRateLimiter // Password reset: per-IP PasswordReset *ipRateLimiter + // Email-verification link consume and claim: per-IP (BUG-3382) + EmailVerify *ipRateLimiter // Registration: per-IP Register *ipRateLimiter // OAuth login: per-IP (higher limit since pad-cloud sidecar calls this) @@ -420,6 +422,15 @@ func NewRateLimiters() *RateLimiters { Rate: rate.Limit(3.0 / 3600.0), Burst: 3, }), + // Verification link consume + claim: 10 per hour per IP, burst 10 + // (BUG-3382). Separate from PasswordReset so one verification page + // load, a refresh and a claim do not spend the budget the claimant + // then needs to set a password. The token is a 256-bit secret, so + // this bounds load, not guessing. + EmailVerify: newIPRateLimiter(rateLimitConfig{ + Rate: rate.Limit(10.0 / 3600.0), + Burst: 10, + }), // Registration: 5 per hour per IP (= 5/3600 per second, burst 5) Register: newIPRateLimiter(rateLimitConfig{ Rate: rate.Limit(5.0 / 3600.0), @@ -589,6 +600,7 @@ func (rls *RateLimiters) Stop() { rls.Auth, rls.AuthEmail, rls.PasswordReset, + rls.EmailVerify, rls.Register, rls.OAuthLogin, rls.CloudAdmin, @@ -665,12 +677,14 @@ func (s *Server) RateLimit(next http.Handler) http.Handler { case path == "/api/v1/auth/login" || path == "/api/v1/auth/bootstrap" || path == "/api/v1/auth/2fa/login-verify": limiter = s.rateLimiters.Auth case path == "/api/v1/auth/forgot-password" || path == "/api/v1/auth/reset-password" || path == "/api/v1/auth/local-reset" || - path == "/api/v1/auth/verify-email" || path == "/api/v1/auth/resend-verification": - // Email-verification endpoints (PLAN-1933 DR-5) reuse the - // PasswordReset bucket — same low-frequency, enumeration-safe - // shape as forgot/reset-password. Without an entry here they'd - // fall through to the looser default API limiter. + path == "/api/v1/auth/resend-verification": + // resend-verification (PLAN-1933 DR-5) reuses the + // PasswordReset bucket — same low-frequency, enumeration-safe, + // mail-sending shape as forgot/reset-password. Without an entry + // here it would fall through to the looser default API limiter. limiter = s.rateLimiters.PasswordReset + case path == "/api/v1/auth/verify-email" || path == "/api/v1/auth/verify-email/claim": + limiter = s.rateLimiters.EmailVerify case path == "/api/v1/auth/register": limiter = s.rateLimiters.Register case path == "/api/v1/auth/oauth-login" || path == "/api/v1/auth/oauth-link": diff --git a/internal/server/middleware_verified_email.go b/internal/server/middleware_verified_email.go index da777f4bb..fecd26a6b 100644 --- a/internal/server/middleware_verified_email.go +++ b/internal/server/middleware_verified_email.go @@ -89,6 +89,7 @@ func verifiedEmailExemptPath(path string) bool { switch path { case "/api/v1/auth/logout", "/api/v1/auth/verify-email", + "/api/v1/auth/verify-email/claim", "/api/v1/auth/resend-verification", "/api/v1/auth/delete-account": return true diff --git a/internal/server/server.go b/internal/server/server.go index 449469820..f42c7e90d 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1686,6 +1686,9 @@ func (s *Server) setupRouter() { // RequireVerifiedEmail's exempt list so an unverified user // can reach them to clear their own unverified state. r.Post("/verify-email", s.handleVerifyEmail) + // BUG-3382: the mailbox owner claims a never-verified + // account someone else registered with their address. + r.Post("/verify-email/claim", s.handleClaimByVerification) r.Post("/resend-verification", s.handleResendVerification) // Two-factor authentication diff --git a/internal/server/twofactor_challenge.go b/internal/server/twofactor_challenge.go index 4f75938ae..c837facbb 100644 --- a/internal/server/twofactor_challenge.go +++ b/internal/server/twofactor_challenge.go @@ -22,12 +22,19 @@ func generateTwoFASecret() ([]byte, error) { return b, nil } -// generateTwoFAChallenge creates a short-lived, HMAC-signed challenge token +// generateTwoFAChallengeAt creates a short-lived, HMAC-signed challenge token // that proves the user already passed password verification. The token is -// bound to the user ID and client IP, and expires after 5 minutes. -func generateTwoFAChallenge(userID, clientIP string, secret []byte) string { +// bound to the user ID and client IP, and expires after 5 minutes. It also +// carries the account's credential_epoch as of the FIRST factor's check +// (BUG-3382), so the session minted when the second +// factor completes is fenced on the epoch the password (or provider) was +// checked under, not the one read minutes later. A negative epoch omits it. +func generateTwoFAChallengeAt(userID, clientIP string, epoch int64, secret []byte) string { expires := time.Now().UTC().Add(twoFAChallengeExpiry).Unix() payload := fmt.Sprintf("%s|%s|%d", userID, clientIP, expires) + if epoch >= 0 { + payload += fmt.Sprintf("|%d", epoch) + } encoded := base64.RawURLEncoding.EncodeToString([]byte(payload)) mac := hmac.New(sha256.New, secret) @@ -37,12 +44,14 @@ func generateTwoFAChallenge(userID, clientIP string, secret []byte) string { return encoded + "." + sig } -// validateTwoFAChallenge verifies a challenge token's signature, expiry, and -// IP binding. Returns the user ID if valid, or an error describing the failure. -func validateTwoFAChallenge(token, clientIP string, secret []byte) (string, error) { +// validateTwoFAChallengeEpoch verifies a challenge token's signature, expiry, +// and IP binding. Returns the user ID and the credential epoch the challenge +// was issued under, or -1 for a token that carries none (issued before +// BUG-3382; they expire within minutes), or an error describing the failure. +func validateTwoFAChallengeEpoch(token, clientIP string, secret []byte) (string, int64, error) { parts := strings.SplitN(token, ".", 2) if len(parts) != 2 { - return "", fmt.Errorf("malformed challenge token") + return "", -1, fmt.Errorf("malformed challenge token") } encoded, sig := parts[0], parts[1] @@ -52,19 +61,19 @@ func validateTwoFAChallenge(token, clientIP string, secret []byte) (string, erro mac.Write([]byte(encoded)) expectedSig := base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) if !hmac.Equal([]byte(sig), []byte(expectedSig)) { - return "", fmt.Errorf("invalid challenge signature") + return "", -1, fmt.Errorf("invalid challenge signature") } // Decode payload payloadBytes, err := base64.RawURLEncoding.DecodeString(encoded) if err != nil { - return "", fmt.Errorf("invalid challenge encoding") + return "", -1, fmt.Errorf("invalid challenge encoding") } payload := string(payloadBytes) - fields := strings.SplitN(payload, "|", 3) - if len(fields) != 3 { - return "", fmt.Errorf("invalid challenge payload") + fields := strings.SplitN(payload, "|", 4) + if len(fields) != 3 && len(fields) != 4 { + return "", -1, fmt.Errorf("invalid challenge payload") } userID := fields[0] @@ -74,16 +83,24 @@ func validateTwoFAChallenge(token, clientIP string, secret []byte) (string, erro // Check expiry expiryUnix, err := strconv.ParseInt(expiryStr, 10, 64) if err != nil { - return "", fmt.Errorf("invalid challenge expiry") + return "", -1, fmt.Errorf("invalid challenge expiry") } if time.Now().UTC().Unix() > expiryUnix { - return "", fmt.Errorf("challenge token expired") + return "", -1, fmt.Errorf("challenge token expired") } // Check IP binding if tokenIP != clientIP { - return "", fmt.Errorf("challenge IP mismatch") + return "", -1, fmt.Errorf("challenge IP mismatch") } - return userID, nil + epoch := int64(-1) + if len(fields) == 4 { + e, err := strconv.ParseInt(fields[3], 10, 64) + if err != nil || e < 0 { + return "", -1, fmt.Errorf("invalid challenge epoch") + } + epoch = e + } + return userID, epoch, nil } diff --git a/internal/store/account_claim.go b/internal/store/account_claim.go new file mode 100644 index 000000000..fee75d58f --- /dev/null +++ b/internal/store/account_claim.go @@ -0,0 +1,179 @@ +package store + +import ( + "crypto/rand" + "crypto/sha256" + "database/sql" + "encoding/hex" + "errors" + "fmt" + + "golang.org/x/crypto/bcrypt" +) + +// ErrClaimNotEligible: the token is invalid, spent or expired, or its account +// is already verified, disabled or gone. Nothing was changed. +var ErrClaimNotEligible = errors.New("account claim: not eligible") + +// ErrClaimNeedsSupport: the account has a billing customer, which a claim +// must not transfer or cancel silently (TASK-3351 ruling (4)). Nothing was +// changed; the token is still unspent. +var ErrClaimNeedsSupport = errors.New("account claim: needs support") + +// AccountClaim reports what a claim changed. +type AccountClaim struct { + UserID string + // Epoch is the account's credential_epoch after the claim, for minting + // the claimant's first session. + Epoch int64 + // StrippedWorkspaces names the workspaces the account was a member of but + // did not own, from which its membership, grants, watches and tabs were + // removed. + StrippedWorkspaces []string + // DeletedWorkspaces names the workspaces the account owned, now + // soft-deleted: they were made by whoever held the account before. + DeletedWorkspaces []string +} + +// ClaimAccountByVerification hands a never-verified account to whoever holds +// its mailbox, proven by the email-verification token (BUG-3382). The token +// was mailed to the address, so the clicker owns the mailbox; the account's +// current credentials may belong to someone who registered the address +// first (a squatter). One transaction: +// +// 1. spend the token (same first step, and lock order, as +// ConsumeEmailVerification); +// 2. lock the account and require it unverified, live and with no billing +// customer; +// 3. reset every credential: an unusable password, no 2FA, every session, +// PAT, OAuth grant, CLI handoff, reset and verification token gone, and +// credential_epoch bumped so no sign-in checked before this can mint; +// 4. mark the address verified; +// 5. remove the account from workspaces it does not own (membership, +// grants, watches, tabs), and soft-delete the ones it owns (TASK-3351 +// ruling (3)): what the previous holder joined or built is not the +// mailbox owner's. +// +// The caller then gives the claimant a way to set a password and kicks the +// account's live connections. +func (s *Store) ClaimAccountByVerification(token string) (*AccountClaim, error) { + sum := sha256.Sum256([]byte(token)) + tokenHash := hex.EncodeToString(sum[:]) + + // An unusable password: a random secret nobody keeps. + raw := make([]byte, 32) + if _, err := rand.Read(raw); err != nil { + return nil, fmt.Errorf("account claim: random password: %w", err) + } + unusable, err := bcrypt.GenerateFromPassword([]byte(hex.EncodeToString(raw)), bcryptCost) + if err != nil { + return nil, fmt.Errorf("account claim: hash password: %w", err) + } + + tx, err := s.db.Begin() + if err != nil { + return nil, fmt.Errorf("account claim: begin: %w", err) + } + defer func() { _ = tx.Rollback() }() + ts := now() + + var userID string + err = tx.QueryRow(s.q(` + UPDATE email_verification_tokens SET used_at = ? + WHERE token_hash = ? AND used_at IS NULL AND expires_at > ? + RETURNING user_id`), ts, tokenHash, ts).Scan(&userID) + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrClaimNotEligible + } + if err != nil { + return nil, fmt.Errorf("account claim: spend token: %w", err) + } + + lockQ := `SELECT COALESCE(email_verified_at, ''), CASE WHEN disabled_at IS NULL THEN 0 ELSE 1 END, COALESCE(stripe_customer_id, '') FROM users WHERE id = ?` + if s.dialect.Driver() == DriverPostgres { + lockQ += ` FOR NO KEY UPDATE` + } + var verifiedAt, customer string + var disabled int + err = tx.QueryRow(s.q(lockQ), userID).Scan(&verifiedAt, &disabled, &customer) + if errors.Is(err, sql.ErrNoRows) { + return nil, ErrClaimNotEligible + } + if err != nil { + return nil, fmt.Errorf("account claim: lock user: %w", err) + } + if verifiedAt != "" || disabled == 1 { + return nil, ErrClaimNotEligible + } + if customer != "" { + return nil, ErrClaimNeedsSupport + } + + claim := &AccountClaim{UserID: userID} + names := func(query string) ([]string, error) { + rows, err := tx.Query(s.q(query), userID) + if err != nil { + return nil, err + } + defer rows.Close() + var out []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + return nil, err + } + out = append(out, n) + } + return out, rows.Err() + } + if claim.StrippedWorkspaces, err = names(` + SELECT w.name FROM workspace_members m JOIN workspaces w ON w.id = m.workspace_id + WHERE m.user_id = ? AND w.owner_id <> m.user_id AND w.deleted_at IS NULL ORDER BY w.name`); err != nil { + return nil, fmt.Errorf("account claim: list joined workspaces: %w", err) + } + if claim.DeletedWorkspaces, err = names(` + SELECT name FROM workspaces WHERE owner_id = ? AND deleted_at IS NULL ORDER BY name`); err != nil { + return nil, fmt.Errorf("account claim: list owned workspaces: %w", err) + } + + notOwned := `(SELECT id FROM workspaces WHERE owner_id <> ?)` + stmts := []struct { + what, query string + args []any + }{ + {"reset credentials", `UPDATE users SET password_hash = ?, password_set = ?, totp_secret = '', totp_enabled = ?, recovery_codes = '', + email_verified_at = ?, updated_at = ?, credential_epoch = credential_epoch + 1 WHERE id = ?`, + []any{string(unusable), s.dialect.BoolToInt(false), s.dialect.BoolToInt(false), ts, ts, userID}}, + {"delete sessions", `DELETE FROM sessions WHERE user_id = ?`, []any{userID}}, + {"delete api tokens", `DELETE FROM api_tokens WHERE user_id = ?`, []any{userID}}, + {"revoke oauth access tokens", `UPDATE oauth_access_tokens SET active = ? WHERE subject = ?`, []any{s.dialect.BoolToInt(false), userID}}, + {"revoke oauth refresh tokens", `UPDATE oauth_refresh_tokens SET active = ? WHERE subject = ?`, []any{s.dialect.BoolToInt(false), userID}}, + {"revoke oauth authorization codes", `UPDATE oauth_authorization_codes SET active = ? WHERE request_id IN (SELECT request_id FROM oauth_connections WHERE user_id = ?)`, []any{s.dialect.BoolToInt(false), userID}}, + {"delete oauth pkce requests", `DELETE FROM oauth_pkce_requests WHERE request_id IN (SELECT request_id FROM oauth_connections WHERE user_id = ?)`, []any{userID}}, + {"delete oauth connections", `DELETE FROM oauth_connections WHERE user_id = ?`, []any{userID}}, + {"delete cli handoffs", `DELETE FROM cli_auth_sessions WHERE user_id = ?`, []any{userID}}, + {"delete reset tokens", `DELETE FROM password_reset_tokens WHERE user_id = ?`, []any{userID}}, + {"delete verification tokens", `DELETE FROM email_verification_tokens WHERE user_id = ?`, []any{userID}}, + {"delete grants elsewhere (collections)", `DELETE FROM collection_grants WHERE user_id = ? AND workspace_id IN ` + notOwned, []any{userID, userID}}, + {"delete grants elsewhere (items)", `DELETE FROM item_grants WHERE user_id = ? AND workspace_id IN ` + notOwned, []any{userID, userID}}, + {"delete watches elsewhere", `DELETE FROM watches WHERE user_id = ? AND workspace_id IN ` + notOwned, []any{userID, userID}}, + {"delete tabs elsewhere", `DELETE FROM user_workspace_tabs WHERE user_id = ? AND workspace_id IN ` + notOwned, []any{userID, userID}}, + {"delete memberships elsewhere", `DELETE FROM workspace_members WHERE user_id = ? AND workspace_id IN ` + notOwned, []any{userID, userID}}, + // Owned workspaces go as account deletion takes them: soft-deleted, + // with every holder's tab on them (users.go deleteAccountAtomic). + {"delete tabs of owned workspaces", `DELETE FROM user_workspace_tabs WHERE workspace_id IN (SELECT id FROM workspaces WHERE owner_id = ? AND deleted_at IS NULL)`, []any{userID}}, + {"soft-delete owned workspaces", `UPDATE workspaces SET deleted_at = ?, updated_at = ? WHERE owner_id = ? AND deleted_at IS NULL`, []any{ts, ts, userID}}, + } + for _, st := range stmts { + if _, err := tx.Exec(s.q(st.query), st.args...); err != nil { + return nil, fmt.Errorf("account claim: %s: %w", st.what, err) + } + } + if err := tx.QueryRow(s.q(`SELECT credential_epoch FROM users WHERE id = ?`), userID).Scan(&claim.Epoch); err != nil { + return nil, fmt.Errorf("account claim: read epoch: %w", err) + } + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("account claim: commit: %w", err) + } + return claim, nil +} diff --git a/internal/store/bug3382_password_fence_test.go b/internal/store/bug3382_password_fence_test.go new file mode 100644 index 000000000..8394169ae --- /dev/null +++ b/internal/store/bug3382_password_fence_test.go @@ -0,0 +1,107 @@ +package store + +import ( + "errors" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3382: a password write is fenced on the credential_epoch its authority +// was checked under, so one that started before a claim cannot overwrite the +// claim's reset afterwards. + +func TestBUG3382_ResetSpentBeforeAClaimWritesNothingAfterIt(t *testing.T) { + s := testStore(t) + u := createUnverifiedUser(t, s, "squat@example.com") + + resetTok, err := s.CreatePasswordReset(u.ID) + if err != nil { + t.Fatal(err) + } + spent, err := s.ConsumePasswordReset(resetTok) + if err != nil || spent == nil { + t.Fatalf("consume reset: %v %v", spent, err) + } + + // The mailbox owner claims between the reset's spend and its write. + vTok, err := s.CreateEmailVerification(u.ID) + if err != nil { + t.Fatal(err) + } + if _, err := s.ClaimAccountByVerification(vTok); err != nil { + t.Fatalf("claim: %v", err) + } + + pwd := "squatters-new-password-1" + _, err = s.UpdateUser(u.ID, models.UserUpdate{Password: &pwd, ExpectedEpoch: &spent.CredentialEpoch}) + if !errors.Is(err, ErrCredentialsChanged) { + t.Fatalf("stale reset write: got %v, want ErrCredentialsChanged", err) + } + if got, _ := s.ValidatePassword(u.Email, pwd); got != nil { + t.Fatal("the stale reset's password was stored over the claim") + } +} + +func TestBUG3382_FencedPasswordWriteReturnsItsOwnEpoch(t *testing.T) { + s := testStore(t) + u := createUnverifiedUser(t, s, "own@example.com") + + resetTok, err := s.CreatePasswordReset(u.ID) + if err != nil { + t.Fatal(err) + } + spent, err := s.ConsumePasswordReset(resetTok) + if err != nil || spent == nil { + t.Fatalf("consume reset: %v %v", spent, err) + } + pwd := "a-fresh-password-2" + updated, err := s.UpdateUser(u.ID, models.UserUpdate{Password: &pwd, ExpectedEpoch: &spent.CredentialEpoch}) + if err != nil { + t.Fatalf("fenced write: %v", err) + } + if updated.CredentialEpoch != spent.CredentialEpoch+1 { + t.Fatalf("epoch = %d, want %d", updated.CredentialEpoch, spent.CredentialEpoch+1) + } + if got, _ := s.ValidatePassword(u.Email, pwd); got == nil { + t.Fatal("the fenced write did not store the password") + } +} + +// The claim lands INSIDE the spend, after the token is marked used and before +// the user is read back: the epoch the reset is fenced on must be the one from +// the spend, not the claim's. +func TestBUG3382_ClaimInsideTheResetSpendStillFencesIt(t *testing.T) { + s := testStore(t) + u := createUnverifiedUser(t, s, "inside@example.com") + resetTok, err := s.CreatePasswordReset(u.ID) + if err != nil { + t.Fatal(err) + } + vTok, err := s.CreateEmailVerification(u.ID) + if err != nil { + t.Fatal(err) + } + claimed := false + passwordResetSpentHook = func(string) { + if _, err := s.ClaimAccountByVerification(vTok); err != nil { + t.Errorf("claim: %v", err) + return + } + claimed = true + } + t.Cleanup(func() { passwordResetSpentHook = nil }) + + spent, err := s.ConsumePasswordReset(resetTok) + if err != nil || spent == nil { + t.Fatalf("consume reset: %v %v", spent, err) + } + if !claimed { + t.Fatal("control: the claim did not run inside the spend") + } + pwd := "squatters-new-password-3" + _, err = s.UpdateUser(u.ID, models.UserUpdate{Password: &pwd, ExpectedEpoch: &spent.CredentialEpoch}) + if !errors.Is(err, ErrCredentialsChanged) { + t.Fatalf("reset spent before the claim: got %v, want ErrCredentialsChanged", err) + } +} diff --git a/internal/store/migrations/110_credential_epoch.sql b/internal/store/migrations/110_credential_epoch.sql new file mode 100644 index 000000000..47d27154f --- /dev/null +++ b/internal/store/migrations/110_credential_epoch.sql @@ -0,0 +1,11 @@ +-- Migration 110: a per-account credential epoch (BUG-3382, TASK-3351). +-- +-- Every sign-in checks a credential (a password, a provider assertion, a 2FA +-- code, an approving session) and THEN mints a session, in a separate step. A +-- credential change landing between the two (a reset, a change, a disable, an +-- account claim) used to be invisible to the mint: CreateSession checked only +-- disabled_at, so a login that validated the OLD password could still mint a +-- session just after every session was deleted. The epoch is bumped by every +-- credential change; a mint carries the epoch it read when it checked the +-- credential, and inserts only if it is unchanged. +ALTER TABLE users ADD COLUMN credential_epoch INTEGER NOT NULL DEFAULT 0; diff --git a/internal/store/password_resets.go b/internal/store/password_resets.go index ee88cb22e..a170893aa 100644 --- a/internal/store/password_resets.go +++ b/internal/store/password_resets.go @@ -82,13 +82,22 @@ func (s *Store) ConsumePasswordReset(token string) (*models.User, error) { // Atomically mark the token as used and return it, only if it's // currently unused and not expired. The WHERE clause ensures only // one concurrent caller can succeed. + // + // The account's credential_epoch is read in the SAME statement + // (BUG-3382): the password write that follows is fenced on it, and a + // claim that committed after this token was spent must refuse that + // write. A later read could return the claim's epoch instead. The token + // row this statement locks is one the claim deletes, so a claim in + // flight either commits before this statement (the token is gone) or + // after it (its epoch bump is not seen). var userID string + var epoch int64 err := s.db.QueryRow(s.q(` UPDATE password_reset_tokens SET used_at = ? WHERE token_hash = ? AND used_at IS NULL AND expires_at > ? - RETURNING user_id - `), now(), tokenHash, now()).Scan(&userID) + RETURNING user_id, (SELECT credential_epoch FROM users WHERE users.id = password_reset_tokens.user_id) + `), now(), tokenHash, now()).Scan(&userID, &epoch) if err == sql.ErrNoRows { return nil, nil // Invalid, expired, or already used @@ -97,15 +106,27 @@ func (s *Store) ConsumePasswordReset(token string) (*models.User, error) { return nil, fmt.Errorf("consume reset token: %w", err) } + if passwordResetSpentHook != nil { + passwordResetSpentHook(userID) + } + // Fetch the user user, err := s.GetUser(userID) if err != nil { return nil, fmt.Errorf("get user: %w", err) } + if user != nil { + user.CredentialEpoch = epoch + } return user, nil } +// passwordResetSpentHook, when set by a test, runs after the reset token is +// spent and before the user is read back, which is where a claim can land +// (BUG-3382). +var passwordResetSpentHook func(userID string) + // CleanExpiredPasswordResets removes old reset tokens. func (s *Store) CleanExpiredPasswordResets() error { _, err := s.db.Exec(s.q(` diff --git a/internal/store/pgmigrations/084_credential_epoch.sql b/internal/store/pgmigrations/084_credential_epoch.sql new file mode 100644 index 000000000..ecf858c92 --- /dev/null +++ b/internal/store/pgmigrations/084_credential_epoch.sql @@ -0,0 +1,3 @@ +-- Migration 084: a per-account credential epoch (BUG-3382, TASK-3351). +-- Mirrors SQLite migration 110; see it for the rationale. +ALTER TABLE users ADD COLUMN IF NOT EXISTS credential_epoch BIGINT NOT NULL DEFAULT 0; diff --git a/internal/store/sessions.go b/internal/store/sessions.go index efbea790e..ec241b7b2 100644 --- a/internal/store/sessions.go +++ b/internal/store/sessions.go @@ -64,6 +64,29 @@ func (s *Store) CreateSession(userID, deviceInfo, ipAddress, userAgent string, t // measured from created_at. The expiry is capped at that lifetime too. A // zero issuedAt means a new sign-in, now. func (s *Store) CreateSessionIssuedAt(userID, deviceInfo, ipAddress, userAgent string, ttl time.Duration, issuedAt time.Time) (string, error) { + return s.createSession(userID, -1, deviceInfo, ipAddress, userAgent, ttl, issuedAt) +} + +// ErrCredentialsChanged refuses a session mint whose credential check is +// stale: the account's credential_epoch moved between the check and the mint +// (a password reset or change, a disable, an account claim). The caller +// answers as for a failed sign-in (BUG-3382). +var ErrCredentialsChanged = errors.New("credentials changed since the sign-in was checked") + +// CreateSessionFenced is CreateSessionIssuedAt for a sign-in: it mints only if +// the account's credential_epoch still equals epoch, the value read when the +// credential was checked (models.User.CredentialEpoch on the user the check +// loaded), else ErrCredentialsChanged. Every production sign-in uses it +// (BUG-3382); CreateSession / CreateSessionIssuedAt carry no fence and are +// for tests and tooling. +func (s *Store) CreateSessionFenced(userID string, epoch int64, deviceInfo, ipAddress, userAgent string, ttl time.Duration, issuedAt time.Time) (string, error) { + if epoch < 0 { + return "", fmt.Errorf("insert session: negative credential epoch %d", epoch) + } + return s.createSession(userID, epoch, deviceInfo, ipAddress, userAgent, ttl, issuedAt) +} + +func (s *Store) createSession(userID string, epoch int64, deviceInfo, ipAddress, userAgent string, ttl time.Duration, issuedAt time.Time) (string, error) { // Generate 32 random bytes → hex → prefix raw := make([]byte, 32) if _, err := rand.Read(raw); err != nil { @@ -106,6 +129,18 @@ func (s *Store) CreateSessionIssuedAt(userID, deviceInfo, ipAddress, userAgent s } return "", fmt.Errorf("insert session: %w", err) } + // The epoch is read under requireActiveUserTx's lock (FOR SHARE on + // Postgres; SQLite serializes writers), which a credential change's + // UPDATE of the row conflicts with, so the two serialize. + if epoch >= 0 { + var current int64 + if err := tx.QueryRow(s.q(`SELECT credential_epoch FROM users WHERE id = ?`), userID).Scan(¤t); err != nil { + return "", fmt.Errorf("insert session: read credential epoch: %w", err) + } + if current != epoch { + return "", ErrCredentialsChanged + } + } if _, err := tx.Exec(s.q(` INSERT INTO sessions (id, user_id, token_hash, device_info, ip_address, ua_hash, expires_at, created_at, renew_ttl_seconds, kind) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) diff --git a/internal/store/users.go b/internal/store/users.go index 1d1046cf3..4739551d5 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -30,7 +30,7 @@ var usernameCleanRe = regexp.MustCompile(`[^a-z0-9-]+`) var bcryptCost = 12 // user SELECT columns — used by all user queries. -const userColumns = `id, email, username, name, password_hash, role, avatar_url, totp_secret, totp_enabled, recovery_codes, plan, plan_expires_at, plan_source, stripe_customer_id, plan_overrides, oauth_providers, password_set, disabled_at, email_verified_at, last_active_at, last_write_at, created_at, updated_at` +const userColumns = `id, email, username, name, password_hash, role, avatar_url, totp_secret, totp_enabled, recovery_codes, plan, plan_expires_at, plan_source, stripe_customer_id, plan_overrides, oauth_providers, password_set, disabled_at, email_verified_at, last_active_at, last_write_at, created_at, updated_at, credential_epoch` // scanUser scans a user row into a User struct. // Note: does NOT decrypt the TOTP secret — call store.decryptUserTOTP() after @@ -46,6 +46,7 @@ func scanUser(row interface{ Scan(...interface{}) error }) (*models.User, error) &u.Plan, &u.PlanExpiresAt, &u.PlanSource, &u.StripeCustomerID, &u.PlanOverrides, &u.OAuthProviders, &u.PasswordSet, &disabledAt, &emailVerifiedAt, &lastActiveAt, &lastWriteAt, &createdAt, &updatedAt, + &u.CredentialEpoch, ) if disabledAt.Valid { u.DisabledAt = disabledAt.String @@ -190,6 +191,9 @@ func (s *Store) UpdateUser(id string, input models.UserUpdate) (*models.User, er // (clears the OAuth placeholder-hash state set by CreateOAuthUser). sets = append(sets, "password_set = ?") args = append(args, true) + // A new password is a credential change: a sign-in that checked the + // old one must not mint a session after it (BUG-3382). + sets = append(sets, "credential_epoch = credential_epoch + 1") } if input.AvatarURL != nil { sets = append(sets, "avatar_url = ?") @@ -205,16 +209,32 @@ func (s *Store) UpdateUser(id string, input models.UserUpdate) (*models.User, er args = append(args, id) query := fmt.Sprintf("UPDATE users SET %s WHERE id = ?", strings.Join(sets, ", ")) + fenced := input.Password != nil && input.ExpectedEpoch != nil + if fenced { + query += " AND credential_epoch = ?" + args = append(args, *input.ExpectedEpoch) + } result, err := s.db.Exec(s.q(query), args...) if err != nil { return nil, fmt.Errorf("update user: %w", err) } n, _ := result.RowsAffected() if n == 0 { + if fenced { + if u, err := s.GetUser(id); err == nil && u != nil { + return nil, ErrCredentialsChanged + } + } return nil, sql.ErrNoRows } - return s.GetUser(id) + u, err := s.GetUser(id) + if err == nil && u != nil && fenced { + // The epoch THIS write produced, not whatever a later change left: + // a session minted on it must not survive a change after this one. + u.CredentialEpoch = *input.ExpectedEpoch + 1 + } + return u, err } // ValidatePassword checks an email/password combination. Returns the user @@ -495,6 +515,7 @@ func (s *Store) SearchUsers(params AdminUserSearchParams) (*AdminUserSearchResul &entry.Plan, &entry.PlanExpiresAt, &entry.PlanSource, &entry.StripeCustomerID, &entry.PlanOverrides, &entry.OAuthProviders, &entry.PasswordSet, &disabledAt, &emailVerifiedAt, &lastActiveAt, &lastWriteAt, &createdAt, &updatedAt, + &entry.CredentialEpoch, &workspaceCount, &storageBytes, ); err != nil { return nil, fmt.Errorf("search users scan: %w", err) @@ -915,7 +936,7 @@ func throttleTime(ts string) string { // DisableUser soft-disables a user account by setting disabled_at. func (s *Store) DisableUser(userID string) error { - _, err := s.db.Exec(s.q(`UPDATE users SET disabled_at = ?, updated_at = ? WHERE id = ?`), + _, err := s.db.Exec(s.q(`UPDATE users SET disabled_at = ?, updated_at = ?, credential_epoch = credential_epoch + 1 WHERE id = ?`), now(), now(), userID) if err != nil { return fmt.Errorf("disable user: %w", err) @@ -979,7 +1000,7 @@ func (s *Store) DisableUserAndRevokeAccess(userID string) error { what, query string args []any }{ - {"disable", `UPDATE users SET disabled_at = ?, updated_at = ? WHERE id = ?`, []any{ts, ts, userID}}, + {"disable", `UPDATE users SET disabled_at = ?, updated_at = ?, credential_epoch = credential_epoch + 1 WHERE id = ?`, []any{ts, ts, userID}}, {"delete sessions", `DELETE FROM sessions WHERE user_id = ?`, []any{userID}}, {"delete api tokens", `DELETE FROM api_tokens WHERE user_id = ?`, []any{userID}}, {"revoke oauth access tokens", `UPDATE oauth_access_tokens SET active = ? WHERE subject = ?`, []any{s.dialect.BoolToInt(false), userID}}, diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index d96b07e30..fe34aa8be 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -2747,12 +2747,24 @@ export const api = { // The token in the link is the credential, so this is a bare POST with // no form. Returns the freshly-verified user so the SPA can refresh its // auth store without a second round-trip; 400 invalid_token when the - // link is invalid, expired, or already used. + // link is invalid, expired, or already used; 409 verify_needs_session + // (details {email, can_claim}) when the request has no session for the + // account the link names (BUG-3382). verifyEmailToken: (token: string) => request<{ ok: boolean; user: { id: string; email: string; username: string; name: string; role: string } }>('/auth/verify-email', { method: 'POST', body: JSON.stringify({ token }) }), + // Claim the address the verification link was mailed to (BUG-3382). + // Offered when the link is opened without a session for its account + // (409 verify_needs_session with details.can_claim). Resets every + // credential on the account and answers with a set-password path; + // 409 account_claim_needs_support when billing is attached. + claimByVerification: (token: string) => + request<{ claimed: boolean; reset_path: string; stripped_workspaces: string[]; deleted_workspaces: string[] }>('/auth/verify-email/claim', { + method: 'POST', + body: JSON.stringify({ token }) + }), me: () => request('/auth/me'), updateProfile: (data: UserProfileUpdate) => request('/auth/me', { diff --git a/web/src/routes/verify-email/[token]/+page.svelte b/web/src/routes/verify-email/[token]/+page.svelte index 4e27b9d5e..3347f7b35 100644 --- a/web/src/routes/verify-email/[token]/+page.svelte +++ b/web/src/routes/verify-email/[token]/+page.svelte @@ -2,17 +2,28 @@ import { onMount } from 'svelte'; import { page } from '$app/state'; import { goto } from '$app/navigation'; - import { api } from '$lib/api/client'; + import { api, PadApiError } from '$lib/api/client'; import { authStore } from '$lib/stores/auth.svelte'; import AuthHeader from '$lib/components/auth/AuthHeader.svelte'; import AuthFooter from '$lib/components/auth/AuthFooter.svelte'; let token = $derived(page.params.token ?? ''); - // verifying → success (terminal, redirects) | error (terminal, offers resend). - let status = $state<'verifying' | 'success' | 'error'>('verifying'); + // verifying → success (terminal, redirects) | error (terminal, offers resend) + // | needs-session (BUG-3382: the link was opened without a session for the + // account it names; offers sign-in, or a claim when it wasn't them). + let status = $state<'verifying' | 'success' | 'error' | 'needs-session'>('verifying'); let error = $state(''); + // needs-session details, from the 409's details. + let pendingEmail = $state(''); + let canClaim = $state(false); + // idle → confirming → claiming → claimed (terminal) | failed (retryable). + let claimState = $state<'idle' | 'confirming' | 'claiming' | 'claimed' | 'failed'>('idle'); + let claimError = $state(''); + let claimed = $state<{ reset_path: string; stripped_workspaces: string[]; deleted_workspaces: string[] } | null>(null); + let signInHref = $derived(`/login?redirect=${encodeURIComponent(`/verify-email/${token}`)}`); + // Mirrors VerifyEmailBanner's resend flow: idle → sending → sent (terminal) ; // error is retryable. let resendState = $state<'idle' | 'sending' | 'sent' | 'error'>('idle'); @@ -44,6 +55,12 @@ status = 'success'; await goto('/console', { replaceState: true }); } catch (err: unknown) { + if (err instanceof PadApiError && err.code === 'verify_needs_session') { + pendingEmail = typeof err.details?.email === 'string' ? err.details.email : ''; + canClaim = err.details?.can_claim === true; + status = 'needs-session'; + return; + } if (err instanceof Error) { error = err.message || 'This verification link is invalid or has expired.'; } else { @@ -53,6 +70,42 @@ } } + // Signed in as a DIFFERENT account: /login would bounce straight back + // here still signed in as it, so sign out first (the CLI-approval page's + // switch-account pattern). + let switching = $state(false); + let switchError = $state(''); + async function switchAccount() { + if (switching) return; + switching = true; + switchError = ''; + try { + await api.auth.logout(); + } catch (err: unknown) { + switching = false; + switchError = + err instanceof Error && err.message + ? `Couldn't sign you out: ${err.message}` + : "Couldn't sign you out. Please try again."; + return; + } + await goto(signInHref, { replaceState: true }); + } + + async function claim() { + if (claimState === 'claiming') return; + claimState = 'claiming'; + claimError = ''; + try { + claimed = await api.auth.claimByVerification(token); + claimState = 'claimed'; + } catch (err: unknown) { + claimError = + err instanceof Error && err.message ? err.message : 'Could not claim this address. Try again.'; + claimState = 'failed'; + } + } + async function resend() { const email = authStore.user?.email; if (!email || resendState === 'sending') return; @@ -87,6 +140,71 @@
Email verified — redirecting…
+ {:else if status === 'needs-session'} + {#if claimState === 'claimed' && claimed} +

Address claimed

+
+

+ Every password, session and token on this account has been reset. Set a password to + sign in. +

+ {#if claimed.deleted_workspaces.length > 0} +

Workspaces created by whoever registered this address were deleted:

+
    + {#each claimed.deleted_workspaces as name, i (i)}
  • {name}
  • {/each} +
+ {/if} + {#if claimed.stripped_workspaces.length > 0} +

The account was removed from workspaces it had joined:

+
    + {#each claimed.stripped_workspaces as name, i (i)}
  • {name}
  • {/each} +
+ {/if} + {#if claimed.reset_path} + Set a password + {:else} + Set a password + {/if} +
+ {:else} +

Sign in to verify

+
+

+ This link verifies {pendingEmail || 'this address'}. Sign in to that account to finish + verifying. +

+ {#if authStore.user} +

You're signed in as {authStore.user.email}.

+ {#if switchError} + + {/if} + + {:else} + Sign in to verify + {/if} + {#if canClaim} + {#if claimState === 'idle'} + + {:else} +

+ If someone else registered your address, you can claim it. This resets every + password, session and token on the account, deletes workspaces it created, and + removes it from workspaces it joined. You then set your own password. +

+ {#if claimState === 'failed'} + + {/if} + + {/if} + {/if} +
+ {/if} {:else}

This verification link is invalid or expired.

@@ -220,6 +338,35 @@ button:hover:not(:disabled) { opacity: 0.9; } button:disabled { opacity: 0.6; cursor: not-allowed; } + .claim { + text-align: left; + color: var(--text-secondary); + font-size: 0.85rem; + } + + .claim ul { + margin: 0; + padding-left: var(--space-5); + } + + .button-link { + display: block; + text-align: center; + padding: var(--space-3) var(--space-4); + background: var(--accent-blue); + color: #fff; + border-radius: var(--radius); + font-size: 0.95rem; + font-weight: 500; + text-decoration: none; + } + + button.secondary { + background: transparent; + color: var(--accent-blue); + border: 1px solid var(--border); + } + .signin-link { display: inline-block; color: var(--accent-blue); diff --git a/web/src/routes/verify-email/[token]/verifyNeedsSession.svelte.test.ts b/web/src/routes/verify-email/[token]/verifyNeedsSession.svelte.test.ts new file mode 100644 index 000000000..5b0979831 --- /dev/null +++ b/web/src/routes/verify-email/[token]/verifyNeedsSession.svelte.test.ts @@ -0,0 +1,152 @@ +// BUG-3382: a verification link opened without a session for the account it +// names does not verify. The page offers sign-in (back to this link), and, +// when the address is unverified, a claim that resets the account and lists +// what the claim removed. +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { cleanup, fireEvent, render, screen } from '@testing-library/svelte'; + +const calls = vi.hoisted(() => ({ verify: [] as string[], claim: [] as string[], logout: 0, goto: [] as string[] })); +const signedIn = vi.hoisted(() => ({ user: null as null | { email: string } })); +const next = vi.hoisted(() => ({ verify: null as unknown, claim: null as unknown })); + +vi.mock('$lib/api/client', () => { + class PadApiError extends Error { + code: string; + details?: Record; + constructor(o: { code: string; message: string; details?: Record }) { + super(o.message); + this.code = o.code; + this.details = o.details; + } + } + const settle = (v: unknown) => (v instanceof Error ? Promise.reject(v) : Promise.resolve(v)); + return { + PadApiError, + api: { + auth: { + verifyEmailToken: vi.fn(async (t: string) => { + calls.verify.push(t); + return settle(next.verify); + }), + claimByVerification: vi.fn(async (t: string) => { + calls.claim.push(t); + return settle(next.claim); + }), + resendVerification: vi.fn(async () => ({})), + logout: vi.fn(async () => { + calls.logout++; + }), + }, + }, + }; +}); +vi.mock('$lib/stores/auth.svelte', () => ({ + authStore: { + cloudMode: true, + get user() { + return signedIn.user; + }, + ensureLoaded: async () => {}, + load: async () => {}, + }, +})); + +vi.mock('$app/navigation', () => ({ + goto: vi.fn(async (url: string) => { + calls.goto.push(url); + }), +})); + +import { page } from '$app/state'; +import { PadApiError } from '$lib/api/client'; +import VerifyPage from './+page.svelte'; + +async function settle() { + for (let i = 0; i < 20; i++) await Promise.resolve(); +} + +const needsSession = (canClaim: boolean) => + new (PadApiError as unknown as new (o: object) => Error)({ + code: 'verify_needs_session', + message: 'Sign in', + details: { email: 'victim@example.com', can_claim: canClaim }, + }); + +beforeEach(() => { + calls.verify.length = 0; + calls.claim.length = 0; + calls.logout = 0; + calls.goto.length = 0; + signedIn.user = null; + (page as { params: Record }).params = { token: 'tok123' }; +}); +afterEach(() => cleanup()); + +describe('verify link without the account’s session (BUG-3382)', () => { + it('offers sign-in back to this link, and a confirmed claim', async () => { + next.verify = needsSession(true); + next.claim = { + claimed: true, + reset_path: '/reset-password/rst', + stripped_workspaces: ['Joined WS'], + deleted_workspaces: ['Squat WS'], + }; + render(VerifyPage); + await settle(); + + expect(screen.getByText(/victim@example\.com/)).toBeTruthy(); + const signIn = screen.getByRole('link', { name: 'Sign in to verify' }); + expect(signIn.getAttribute('href')).toBe('/login?redirect=%2Fverify-email%2Ftok123'); + + // The first click only explains; nothing is claimed without the second. + await fireEvent.click(screen.getByRole('button', { name: "I didn't register this account" })); + await settle(); + expect(calls.claim).toEqual([]); + + await fireEvent.click(screen.getByRole('button', { name: 'Claim this address' })); + await settle(); + expect(calls.claim).toEqual(['tok123']); + expect(screen.getByText('Squat WS')).toBeTruthy(); + expect(screen.getByText('Joined WS')).toBeTruthy(); + expect(screen.getByRole('link', { name: 'Set a password' }).getAttribute('href')).toBe( + '/reset-password/rst', + ); + }); + + it('offers no claim for an already-verified address', async () => { + next.verify = needsSession(false); + render(VerifyPage); + await settle(); + expect(screen.getByRole('link', { name: 'Sign in to verify' })).toBeTruthy(); + expect(screen.queryByRole('button', { name: "I didn't register this account" })).toBeNull(); + }); + + it('shows a refused claim and lets it be retried', async () => { + next.verify = needsSession(true); + next.claim = new (PadApiError as unknown as new (o: object) => Error)({ + code: 'account_claim_needs_support', + message: 'Contact support@getpad.dev', + }); + render(VerifyPage); + await settle(); + await fireEvent.click(screen.getByRole('button', { name: "I didn't register this account" })); + await fireEvent.click(screen.getByRole('button', { name: 'Claim this address' })); + await settle(); + expect(screen.getByRole('alert').textContent).toContain('support@getpad.dev'); + expect(screen.getByRole('button', { name: 'Claim this address' })).toBeTruthy(); + }); + + it('signed in as another account: signs out before sending to sign-in', async () => { + signedIn.user = { email: 'other@example.com' }; + next.verify = needsSession(true); + render(VerifyPage); + await settle(); + expect(screen.getByText(/signed in as other@example\.com/)).toBeTruthy(); + // A plain link to /login would bounce back here still signed in. + expect(screen.queryByRole('link', { name: 'Sign in to verify' })).toBeNull(); + await fireEvent.click(screen.getByRole('button', { name: 'Switch account to verify' })); + await settle(); + expect(calls.logout).toBe(1); + expect(calls.goto).toEqual(['/login?redirect=%2Fverify-email%2Ftok123']); + }); +});