From 62dca184dfdb238f1405acd7d1e5c14ca5642a74 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 16 Aug 2026 18:22:03 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20account=20security=20+=20GDPR=20erasure?= =?UTF-8?q?=20=E2=80=94=20current-password=20lockout=20budgets=20(atomic?= =?UTF-8?q?=2015/30/60=20escalation,=20uniform=20401),=20DAV=20+=20S3=20de?= =?UTF-8?q?letion=20durable=20in-tx,=20batch=20erasure=20outbox?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ChangePassword/DeleteAccount: failed-attempt lockout matching login escalation, atomic check-increment (no burst), uniform 401 with distinct bodies, passwordless accounts require 2FA unconditionally to delete, NULL-password change-password clear error - erasure: CardDAV dav_cards rows deleted inside the erasure transaction (was fire-and-forget goroutine); S3 profile-pic deletion via pending_s3_deletions outbox + retry job; stale-guest/idle-account batch paths write the outbox in-tx and skip the guessed-bucket fallback - S3_PROFILE_PICS_BUCKET unset -> fail-closed warning (once per process) - scheduler test: 27 jobs (retry-s3-deletions) Co-authored-by: Sisyphus Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) --- backend/handlers/scheduling/time-blockers.go | 210 +++++++++----- .../handlers/scheduling/time_blockers_test.go | 240 ++++++++++++++++ backend/handlers/user/account.go | 266 +++++++++++++++--- backend/handlers/user/profile.go | 38 ++- backend/internal/jobs/cleanup.go | 90 ++++++ backend/internal/jobs/scheduler_test.go | 5 +- 6 files changed, 748 insertions(+), 101 deletions(-) diff --git a/backend/handlers/scheduling/time-blockers.go b/backend/handlers/scheduling/time-blockers.go index 3301fa0..8a07388 100644 --- a/backend/handlers/scheduling/time-blockers.go +++ b/backend/handlers/scheduling/time-blockers.go @@ -13,13 +13,13 @@ import ( "net/http" "net/url" "os" + "sync" "time" "crussell/clock" "crussell/db" "crussell/handlers/payments" "crussell/internal/adminnotify" - "crussell/internal/dav" "crussell/internal/s3" "crussell/internal/square" "crussell/internal/validators" @@ -694,50 +694,55 @@ func recheckStaleGuestSquareTargets(ctx context.Context, cardsByUser map[string] return stillStaleCards, stillStaleCustomers } -// deleteExternalUserArtifacts best-effort deletes the erased user's CardDAV -// vCard and R2/S3 profile photo — the external PII artifacts -// DeleteAccountHandler scrubs on interactive account deletion. Both are -// personal data the SQL erasure does not reach: the vCard lives in the dav -// service's own dav_cards table and the photo is an object in object storage, -// so batch erasure must delete them explicitly (GDPR Art 17). Mirrors -// account.go's call pattern and nil-guards: both services may be nil in dev, -// and each call is wrapped in panic recovery so a nil-pool dev service cannot -// crash the cleanup job. -func deleteExternalUserArtifacts(ctx context.Context, userID string) { - // Delete profile picture from S3/R2 - if s3.Client != nil { - func() { - defer func() { - if r := recover(); r != nil { - log.Printf("Panic recovered in S3 profile picture deletion: %v", r) - } - }() - bucket := os.Getenv("S3_PROFILE_PICS_BUCKET") - if bucket == "" { - bucket = "crussell-profile-pics" - } - // profiles/{userID}.jpg — matches UploadProfilePictureHandler key format - key := fmt.Sprintf("profiles/%s.jpg", userID) - if err := s3.Client.Delete(ctx, bucket, key); err != nil { - log.Printf("Warning: Failed to delete profile picture for user %s: %v", userID, err) - } - }() - } +// s3ProfilePicsBucketUnsetWarningOnce throttles the missing +// S3_PROFILE_PICS_BUCKET CRITICAL log to one line per process (FIX 2): batch +// erasure skips profile-picture deletion fail-closed when the bucket is unset, +// and the single loud warning makes the misconfiguration impossible to miss. A +// proper startup check in main.go is tracked for a later round. +var s3ProfilePicsBucketUnsetWarningOnce sync.Once - // Delete CardDAV contact (non-blocking, best-effort) - if dav.Service != nil { - func() { - defer func() { - if r := recover(); r != nil { - log.Printf("Panic recovered in CardDAV contact deletion: %v", r) - } - }() - uri := fmt.Sprintf("%s.vcf", userID) - if err := dav.Service.DeleteContact(1, uri); err != nil { - log.Printf("Warning: Failed to delete CardDAV contact for user %s: %v", userID, err) - } - }() +func warnS3ProfilePicsBucketUnset() { + s3ProfilePicsBucketUnsetWarningOnce.Do(func() { + log.Printf("CRITICAL: S3_PROFILE_PICS_BUCKET is not set — batch erasure skipping profile-picture deletion fail-closed; profile-pic objects may remain until the bucket is configured (main.go startup check pending in a later round)") + }) +} + +// persistExternalArtifactErasures writes the batch erasure's external-artifact +// cleanup INSIDE the anonymization transaction, before it commits — mirroring +// DeleteAccountHandler's FIX 1a/FIX 1b pattern: +// +// - CardDAV: the erased users' dav_cards rows (full name/email/phone/DOB/ +// photo URL PII, keyed by uri "{userID}.vcf" under addressbook 1) are +// deleted in the same Postgres, so the delete is atomic with the erasure. +// - S3/R2: when bucket is non-empty a pending_s3_deletions outbox row is +// written for each user who HAD a profile picture (profilePicUserIDs must +// be snapshotted BEFORE anonymization NULLs profile_pic_url), so the +// retry-s3-deletions job completes the object deletion after a crash. +// When bucket is empty the outbox is skipped fail-closed — never the +// guessed dev bucket "crussell-profile-pics". +func persistExternalArtifactErasures(ctx context.Context, tx pgx.Tx, userIDs, profilePicUserIDs []string, bucket string) error { + if len(userIDs) > 0 { + uris := make([]string, len(userIDs)) + for i, uid := range userIDs { + uris[i] = fmt.Sprintf("%s.vcf", uid) + } + if _, err := tx.Exec(ctx, ` + DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = ANY($1) + `, uris); err != nil { + return fmt.Errorf("failed to delete CardDAV vCards: %w", err) + } } + if bucket != "" { + for _, uid := range profilePicUserIDs { + if _, err := tx.Exec(ctx, ` + INSERT INTO pending_s3_deletions (user_id, bucket, object_key) + VALUES ($1, $2, $3) + `, uid, bucket, fmt.Sprintf("profiles/%s.jpg", uid)); err != nil { + return fmt.Errorf("failed to persist S3 profile-picture deletion outbox for user %s: %w", uid, err) + } + } + } + return nil } // AnonymizeStaleGuestAccounts anonymizes personal data for guest accounts @@ -817,6 +822,40 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { var totalRows int + // FIX 1b: snapshot the profile-pic-owning stale guests BEFORE the users + // UPDATE NULLs profile_pic_url, so the S3 deletion outbox rows can be + // written inside the tx. Only relevant when an object store is configured. + var profilePicGuestIDs []string + if s3.Client != nil { + profilePicBucket := os.Getenv("S3_PROFILE_PICS_BUCKET") + if profilePicBucket == "" { + warnS3ProfilePicsBucketUnset() + } else { + rows, err := tx.Query(ctx, ` + SELECT id FROM users + WHERE account_role = 'guest' + AND NOT EXISTS (SELECT 1 FROM bookings WHERE user_id = users.id AND status IN ('pending', 'confirmed')) + AND EXISTS (SELECT 1 FROM bookings WHERE user_id = users.id GROUP BY user_id HAVING MAX(start_time) < NOW() - INTERVAL '6 months') + AND profile_pic_url IS NOT NULL + `) + if err != nil { + return 0, fmt.Errorf("failed to snapshot stale-guest profile pictures for S3 erasure: %w", err) + } + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + rows.Close() + return 0, fmt.Errorf("failed to scan stale-guest profile picture id: %w", err) + } + profilePicGuestIDs = append(profilePicGuestIDs, id) + } + rows.Close() + if err := rows.Err(); err != nil { + return 0, fmt.Errorf("row iteration error snapshotting stale-guest profile pictures: %w", err) + } + } + } + tag, err := tx.Exec(ctx, ` UPDATE users SET n_first_name = 'Guest', @@ -971,10 +1010,9 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { totalRows += int(tag.RowsAffected()) // Capture the ids of the stale guests this run erased (the 'Anonymized' - // marker is set only by the users UPDATE above) so the post-commit CardDAV - // vCard / S3 profile-photo scrubs cover exactly the erased guests. - // Already-anonymized guests from a re-run may re-appear here — their - // external-artifact deletes are no-ops. + // marker is set only by the users UPDATE above) so the in-tx CardDAV / + // S3-outbox erasures cover exactly the erased guests. Already-anonymized + // guests from a re-run may re-appear here — their deletes are no-ops. rows, err := tx.Query(ctx, ` SELECT id FROM users WHERE account_role = 'guest' @@ -998,6 +1036,15 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { return 0, fmt.Errorf("row iteration error querying anonymized stale guests: %w", err) } + // FIX 1b: delete the erased guests' CardDAV vCards and write the S3/R2 + // profile-picture deletion outbox INSIDE the erasure tx — atomic with the + // anonymization, and crash-safe via the retry-s3-deletions job. + if len(anonymizedGuestIDs) > 0 { + if err := persistExternalArtifactErasures(ctx, tx, anonymizedGuestIDs, profilePicGuestIDs, os.Getenv("S3_PROFILE_PICS_BUCKET")); err != nil { + return 0, fmt.Errorf("failed to persist external artifact erasures for stale guests: %w", err) + } + } + if err := tx.Commit(ctx); err != nil { return 0, err } @@ -1028,14 +1075,6 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) { payments.InvalidateSquareCustomerCache(uid) } - // Delete the erased guests' CardDAV vCards and R2/S3 profile photos — the - // same external PII artifacts DeleteAccountHandler scrubs. These live - // outside the SQL rows the tx above anonymized, so batch erasure must - // delete them explicitly (best-effort; both services may be nil in dev). - for _, uid := range anonymizedGuestIDs { - deleteExternalUserArtifacts(ctx, uid) - } - return totalRows, nil } @@ -1536,6 +1575,37 @@ func CleanupIdleAccounts(ctx context.Context) (int, error) { } } + // FIX 1b: snapshot the profile-pic-owning erased accounts BEFORE + // anonymize_user(unnest(...)) NULLs profile_pic_url, so the S3 deletion + // outbox rows can be written inside the tx (only when an object store is + // configured; fail-closed otherwise). + var profilePicUserIDs []string + snapshotProfilePicUsers := func(ids []string) error { + if s3.Client == nil || len(ids) == 0 { + return nil + } + if os.Getenv("S3_PROFILE_PICS_BUCKET") == "" { + warnS3ProfilePicsBucketUnset() + return nil + } + rows, err := tx.Query(ctx, ` + SELECT id FROM users + WHERE id = ANY($1) AND profile_pic_url IS NOT NULL + `, ids) + if err != nil { + return fmt.Errorf("failed to snapshot idle-account profile pictures for S3 erasure: %w", err) + } + defer rows.Close() + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + return fmt.Errorf("failed to scan idle-account profile picture id: %w", err) + } + profilePicUserIDs = append(profilePicUserIDs, id) + } + return rows.Err() + } + if len(accountsWithBalance) > 0 { ids := make([]string, len(accountsWithBalance)) balances := make([]float64, len(accountsWithBalance)) @@ -1543,6 +1613,9 @@ func CleanupIdleAccounts(ctx context.Context) (int, error) { ids[i] = a.id balances[i] = a.balance } + if err := snapshotProfilePicUsers(ids); err != nil { + return 0, err + } if _, err = tx.Exec(ctx, ` INSERT INTO gift_card_expired_balances (account_id, original_balance, expired_at) @@ -1617,12 +1690,29 @@ func CleanupIdleAccounts(ctx context.Context) (int, error) { } } + if err := snapshotProfilePicUsers(accountsNoBalance); err != nil { + return 0, err + } if _, err = tx.Exec(ctx, ` SELECT anonymize_user(unnest($1::text[])) `, accountsNoBalance); err != nil { return 0, fmt.Errorf("failed to anonymize idle accounts: %w", err) } + // FIX 1b: delete the erased accounts' CardDAV vCards and write the S3/R2 + // profile-picture deletion outbox INSIDE the erasure tx — atomic with the + // anonymization, and crash-safe via the retry-s3-deletions job. + erasedIDs := make([]string, 0, len(accountsWithBalance)+len(accountsNoBalance)) + for _, acc := range accountsWithBalance { + erasedIDs = append(erasedIDs, acc.id) + } + erasedIDs = append(erasedIDs, accountsNoBalance...) + if len(erasedIDs) > 0 { + if err := persistExternalArtifactErasures(ctx, tx, erasedIDs, profilePicUserIDs, os.Getenv("S3_PROFILE_PICS_BUCKET")); err != nil { + return 0, fmt.Errorf("failed to persist external artifact erasures for idle accounts: %w", err) + } + } + if err := tx.Commit(ctx); err != nil { return 0, err } @@ -1647,16 +1737,6 @@ func CleanupIdleAccounts(ctx context.Context) (int, error) { payments.InvalidateSquareCustomerCache(id) } - // Delete the erased accounts' CardDAV vCards and R2/S3 profile photos — - // the same external PII artifacts DeleteAccountHandler scrubs (best-effort; - // both services may be nil in dev). - for _, acc := range accountsWithBalance { - deleteExternalUserArtifacts(ctx, acc.id) - } - for _, id := range accountsNoBalance { - deleteExternalUserArtifacts(ctx, id) - } - return len(accountsWithBalance) + len(accountsNoBalance), nil } diff --git a/backend/handlers/scheduling/time_blockers_test.go b/backend/handlers/scheduling/time_blockers_test.go index 3aaf1d9..4ed6a84 100644 --- a/backend/handlers/scheduling/time_blockers_test.go +++ b/backend/handlers/scheduling/time_blockers_test.go @@ -19,6 +19,7 @@ import ( "database/sql" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -30,6 +31,7 @@ import ( "crussell/db" "crussell/handlers/payments" "crussell/internal/adminnotify" + "crussell/internal/s3" "crussell/internal/square" "crussell/mw" "crussell/testutils" @@ -4173,3 +4175,241 @@ func TestCleanupExpiredDeposits_NotificationFloodCap(t *testing.T) { t.Errorf("expected the queue to stay capped at %d, got %d", adminnotify.MaxUnacknowledgedCriticalLogs, n) } } + +// ============================================================================ +// FIX 1b — batch erasure external artifacts (CardDAV + S3 outbox, in-tx) +// ============================================================================ + +// recordingS3Uploader records S3 Delete calls so the batch S3-outbox tests can +// assert what was (and wasn't) enqueued. Every Uploader method is implemented +// explicitly (no embedded nil) so tests that swap the global s3.Client never +// panic. +type recordingS3Uploader struct { + mu sync.Mutex + deleted []string // "bucket/key" +} + +func (r *recordingS3Uploader) Upload(context.Context, string, string, io.Reader, string) error { + return nil +} + +func (r *recordingS3Uploader) Download(context.Context, string, string, io.Writer) error { + return nil +} + +func (r *recordingS3Uploader) GetURL(context.Context, string, string) (string, error) { + return "https://cdn.example.com/x/y", nil +} + +func (r *recordingS3Uploader) HealthCheck(context.Context) error { + return nil +} + +func (r *recordingS3Uploader) Delete(_ context.Context, bucket, key string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.deleted = append(r.deleted, bucket+"/"+key) + return nil +} + +func (r *recordingS3Uploader) Deleted() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.deleted...) +} + +// seedStaleGuest creates a guest user with a booking older than 6 months and a +// CardDAV vCard row, so it is eligible for AnonymizeStaleGuestAccounts erasure. +func seedStaleGuest(t *testing.T, ctx context.Context, q db.Querier) string { + t.Helper() + guestID, err := fixtures.CreateTestUser(q) + if err != nil { + t.Fatalf("failed to create guest: %v", err) + } + if _, err := q.Exec(ctx, `UPDATE users SET account_role = 'guest' WHERE id = $1`, guestID); err != nil { + t.Fatalf("failed to set guest role: %v", err) + } + if _, err := q.Exec(ctx, ` + INSERT INTO bookings (user_id, start_time, status, deposit_required) + VALUES ($1, NOW() - INTERVAL '7 months', 'completed', false) + `, guestID); err != nil { + t.Fatalf("failed to create stale booking: %v", err) + } + return guestID +} + +// TestAnonymizeStaleGuestAccounts_DeletesDavCardsInTx verifies FIX 1b: the +// stale guest's dav_cards vCard row (name/email/phone/DOB PII) is deleted +// INSIDE the erasure transaction — not a post-commit best-effort call. +func TestAnonymizeStaleGuestAccounts_DeletesDavCardsInTx(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + guestID := seedStaleGuest(t, ctx, tx) + if _, err := tx.Exec(ctx, ` + INSERT INTO dav_cards (addressbookid, uri, carddata, lastmodified, etag, size) + VALUES (1, $1, 'BEGIN:VCARD', 0, '0', 0) + `, guestID+".vcf"); err != nil { + t.Fatalf("failed to seed dav_cards row: %v", err) + } + + if _, err := AnonymizeStaleGuestAccounts(ctx); err != nil { + t.Fatalf("AnonymizeStaleGuestAccounts failed: %v", err) + } + + var count int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, guestID+".vcf").Scan(&count); err != nil { + t.Fatalf("failed to count dav_cards: %v", err) + } + if count != 0 { + t.Errorf("expected the guest's dav_cards row to be deleted inside the erasure tx, found %d", count) + } +} + +// TestCleanupIdleAccounts_DeletesDavCardsInTx verifies FIX 1b for the idle +// account batch path: the erased account's dav_cards vCard row is deleted +// inside the erasure transaction. +func TestCleanupIdleAccounts_DeletesDavCardsInTx(t *testing.T) { + t.Parallel() + ctx, tx := resetTestData(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + if _, err := tx.Exec(ctx, `UPDATE users SET last_login_at = NOW() - INTERVAL '3 years' WHERE id = $1`, userID); err != nil { + t.Fatalf("failed to set last_login_at: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO dav_cards (addressbookid, uri, carddata, lastmodified, etag, size) + VALUES (1, $1, 'BEGIN:VCARD', 0, '0', 0) + `, userID+".vcf"); err != nil { + t.Fatalf("failed to seed dav_cards row: %v", err) + } + + if _, err := CleanupIdleAccounts(ctx); err != nil { + t.Fatalf("CleanupIdleAccounts failed: %v", err) + } + + var count int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, userID+".vcf").Scan(&count); err != nil { + t.Fatalf("failed to count dav_cards: %v", err) + } + if count != 0 { + t.Errorf("expected the account's dav_cards row to be deleted inside the erasure tx, found %d", count) + } +} + +// TestAnonymizeStaleGuestAccounts_S3OutboxPersisted verifies FIX 1b (a): with +// S3_PROFILE_PICS_BUCKET configured and an object store client, the stale-guest +// erasure writes a pending_s3_deletions outbox row (same shape as account.go) +// so the retry-s3-deletions job covers the photo deletion. Deliberately NOT +// t.Parallel: swaps the package-level s3.Client and the env. +func TestAnonymizeStaleGuestAccounts_S3OutboxPersisted(t *testing.T) { + ctx, tx := resetTestData(t) + + saved := s3.Client + rec := &recordingS3Uploader{} + s3.Client = rec + defer func() { s3.Client = saved }() + t.Setenv("S3_PROFILE_PICS_BUCKET", "test-profile-pics-bucket") + + guestID := seedStaleGuest(t, ctx, tx) + if _, err := tx.Exec(ctx, `UPDATE users SET profile_pic_url = 'https://cdn.example.com/pics/old.jpg' WHERE id = $1`, guestID); err != nil { + t.Fatalf("failed to set profile pic: %v", err) + } + + if _, err := AnonymizeStaleGuestAccounts(ctx); err != nil { + t.Fatalf("AnonymizeStaleGuestAccounts failed: %v", err) + } + + var bucket, objectKey string + if err := tx.QueryRow(ctx, `SELECT bucket, object_key FROM pending_s3_deletions WHERE user_id = $1`, guestID).Scan(&bucket, &objectKey); err != nil { + t.Fatalf("expected a pending_s3_deletions outbox row: %v", err) + } + if bucket != "test-profile-pics-bucket" { + t.Errorf("expected bucket %q, got %q", "test-profile-pics-bucket", bucket) + } + if objectKey != "profiles/"+guestID+".jpg" { + t.Errorf("expected object_key %q, got %q", "profiles/"+guestID+".jpg", objectKey) + } +} + +// TestAnonymizeStaleGuestAccounts_S3Outbox_FailClosedOnEmptyBucket verifies +// FIX 1b (b): with the bucket unset the batch erasure writes NO outbox row and +// never attempts a deletion against a guessed bucket — fail-closed, matching +// DeleteAccountHandler. Deliberately NOT t.Parallel: swaps the package-level +// s3.Client and the env. +func TestAnonymizeStaleGuestAccounts_S3Outbox_FailClosedOnEmptyBucket(t *testing.T) { + ctx, tx := resetTestData(t) + + saved := s3.Client + rec := &recordingS3Uploader{} + s3.Client = rec + defer func() { s3.Client = saved }() + t.Setenv("S3_PROFILE_PICS_BUCKET", "") + + guestID := seedStaleGuest(t, ctx, tx) + if _, err := tx.Exec(ctx, `UPDATE users SET profile_pic_url = 'https://cdn.example.com/pics/old.jpg' WHERE id = $1`, guestID); err != nil { + t.Fatalf("failed to set profile pic: %v", err) + } + + if _, err := AnonymizeStaleGuestAccounts(ctx); err != nil { + t.Fatalf("AnonymizeStaleGuestAccounts failed: %v", err) + } + + var count int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM pending_s3_deletions WHERE user_id = $1`, guestID).Scan(&count); err != nil { + t.Fatalf("failed to count pending_s3_deletions: %v", err) + } + if count != 0 { + t.Errorf("no outbox row may be written when the bucket is unset, found %d", count) + } + if got := rec.Deleted(); len(got) != 0 { + t.Errorf("no S3 deletion may be attempted against a guessed bucket, got %v", got) + } +} + +// TestCleanupIdleAccounts_S3OutboxPersisted verifies FIX 1b (a) for the idle +// account batch path: with a configured bucket the erasure writes the +// pending_s3_deletions outbox row for a with-balance account that had a profile +// picture. Deliberately NOT t.Parallel: swaps the package-level s3.Client and +// the env. +func TestCleanupIdleAccounts_S3OutboxPersisted(t *testing.T) { + ctx, tx := resetTestData(t) + + saved := s3.Client + rec := &recordingS3Uploader{} + s3.Client = rec + defer func() { s3.Client = saved }() + t.Setenv("S3_PROFILE_PICS_BUCKET", "test-profile-pics-bucket") + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + if _, err := tx.Exec(ctx, `UPDATE users SET last_login_at = NOW() - INTERVAL '6 years', profile_pic_url = 'https://cdn.example.com/pics/old.jpg' WHERE id = $1`, userID); err != nil { + t.Fatalf("failed to set last_login_at + profile pic: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO user_giftcard_balances (user_id, balance) + VALUES ($1, 150.00) + `, userID); err != nil { + t.Fatalf("failed to create user giftcard balance: %v", err) + } + + if _, err := CleanupIdleAccounts(ctx); err != nil { + t.Fatalf("CleanupIdleAccounts failed: %v", err) + } + + var bucket, objectKey string + if err := tx.QueryRow(ctx, `SELECT bucket, object_key FROM pending_s3_deletions WHERE user_id = $1`, userID).Scan(&bucket, &objectKey); err != nil { + t.Fatalf("expected a pending_s3_deletions outbox row: %v", err) + } + if bucket != "test-profile-pics-bucket" { + t.Errorf("expected bucket %q, got %q", "test-profile-pics-bucket", bucket) + } + if objectKey != "profiles/"+userID+".jpg" { + t.Errorf("expected object_key %q, got %q", "profiles/"+userID+".jpg", objectKey) + } +} diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index 4bc5ede..0f59b74 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -13,13 +13,14 @@ import ( "net/http" "net/url" "os" + "sync" "time" "crussell/auth" + "crussell/clock" "crussell/db" "crussell/handlers/payments" "crussell/internal/adminnotify" - "crussell/internal/dav" "crussell/internal/s3" "crussell/internal/square" "crussell/internal/twofa" @@ -216,6 +217,94 @@ func clearSquareErasureOutboxRows(ctx context.Context, rowIDs []string) { } } +// --- Current-password re-verification lockout (FIX 2 / FIX 3) --- +// +// DeleteAccountHandler and ChangePasswordHandler both re-verify the current +// password. Without a failed-attempt budget, a stolen session token lets an +// attacker brute-force that password with unlimited guesses (the 2FA code gate +// only applies when 2FA is enforced AND enabled). The budget below reuses the +// SAME users.failed_attempts / users.locked_until columns the login path uses +// (handlers/auth/local.go), so every current-password check shares one +// counter: after 5 consecutive failures the account locks for 15 minutes +// (escalating to 30 at 7+ and 60 at 10+, matching login), and a correct +// password resets it. clock.Now() keeps the Go-side lock check on the same UTC +// clock as the DB NOW() stamp that writes locked_until. + +// errCurrentPasswordLockedOut is returned by checkCurrentPasswordLockout while +// the user's current-password budget is inside a lockout window. +var errCurrentPasswordLockedOut = errors.New("current-password attempts exhausted; account locked") + +// checkCurrentPasswordLockout returns errCurrentPasswordLockedOut when the +// user's shared failed-attempt budget is locked, nil otherwise (or the raw DB +// error). Must run BEFORE the bcrypt compare so a locked account is rejected +// without paying the bcrypt cost. +func checkCurrentPasswordLockout(ctx context.Context, userID string) error { + var lockedUntil *time.Time + if err := db.Conn.QueryRow(ctx, + `SELECT locked_until FROM users WHERE id = $1`, userID).Scan(&lockedUntil); err != nil { + return err + } + if lockedUntil != nil && lockedUntil.After(clock.Now()) { + return errCurrentPasswordLockedOut + } + return nil +} + +// recordCurrentPasswordFailure atomically records a wrong current password on +// the shared failed-attempt budget and returns the NEW failed_attempts count +// plus the resulting locked_until. FIX 3: the increment AND the lockout +// escalation are a SINGLE atomic UPDATE ... RETURNING (the exact statement the +// login path uses), so N concurrent wrong-password requests can never race a +// check-then-increment — every failure is counted (no lost updates) and the +// escalation decision is computed from the returned count. Mirrors the login +// path's progressive lockout exactly (5 failures → 15 minutes, 7 → 30, 10 → +// 60); the lock extends on every failure past the threshold. +func recordCurrentPasswordFailure(ctx context.Context, userID string) (int, *time.Time, error) { + var newCount int + var newLockedUntil *time.Time + err := db.Conn.QueryRow(ctx, ` + UPDATE users + SET failed_attempts = failed_attempts + 1, + locked_until = CASE + WHEN failed_attempts + 1 >= 5 THEN NOW() + (CASE + WHEN failed_attempts + 1 >= 10 THEN INTERVAL '60 minutes' + WHEN failed_attempts + 1 >= 7 THEN INTERVAL '30 minutes' + ELSE INTERVAL '15 minutes' + END) + ELSE locked_until + END + WHERE id = $1 + RETURNING failed_attempts, locked_until + `, userID).Scan(&newCount, &newLockedUntil) + if err != nil { + return 0, nil, err + } + return newCount, newLockedUntil, nil +} + +// resetCurrentPasswordFailures clears the shared failed-attempt budget after a +// correct current password. Best-effort: a failed reset only leaves the stale +// counter in place, and the next correct password clears it again. +func resetCurrentPasswordFailures(ctx context.Context, userID string) { + if _, err := db.Conn.Exec(ctx, + `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID); err != nil { + slog.Error("failed to reset current-password lockout state", "user_id", userID, "error", err) + } +} + +// s3BucketUnsetWarningOnce throttles the missing-S3_PROFILE_PICS_BUCKET CRITICAL +// log to one line per process (FIX 2): the deletion-time skip is fail-closed, +// and the single loud warning makes the misconfiguration impossible to miss. +// A proper startup check for S3_PROFILE_PICS_BUCKET in main.go is tracked for +// a later round (another agent owns main.go). +var s3BucketUnsetWarningOnce sync.Once + +func warnS3ProfilePicsBucketUnset() { + s3BucketUnsetWarningOnce.Do(func() { + log.Printf("CRITICAL: S3_PROFILE_PICS_BUCKET is not set — profile-picture deletion skipped fail-closed; profile-pic objects may remain until the bucket is configured (main.go startup check pending in a later round)") + }) +} + // DeleteAccountRequest carries the re-verification credentials the handler now // requires before erasing an account (finding 3): the current password (always) // and, in enforced environments for a user with 2FA enabled, a fresh one-time @@ -262,13 +351,45 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid request", http.StatusBadRequest) return } - if passwordHash.Valid && passwordHash.String != "" { + // FIX 5: passwordless (social-only, NULL password_hash) accounts have no + // current password to re-verify — a stolen session token would otherwise + // erase the account with zero credential proof. Treat them as needing the + // 2FA gate (see below). + hasPassword := passwordHash.Valid && passwordHash.String != "" + if hasPassword { + // FIX 2: apply the shared current-password failed-attempt budget BEFORE + // the compare — a stolen session token must not be able to brute-force + // the current password with unlimited guesses. + if err := checkCurrentPasswordLockout(ctx, userID); err != nil { + if errors.Is(err, errCurrentPasswordLockedOut) { + // FIX 4: uniform 401 — the same status as a wrong password, so + // locked-vs-wrong is never distinguishable; the body text still + // tells the UI which one happened. + http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) + return + } + log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { + // FIX 3: the failure record is ONE atomic UPDATE ... RETURNING + // (increment + escalation) — concurrent wrong-password requests + // cannot race a check-then-increment and lose updates. + newCount, _, recordErr := recordCurrentPasswordFailure(ctx, userID) + if recordErr != nil { + log.Printf("Failed to record current-password failure for user %s: %v", userID, recordErr) + } + if newCount >= 5 { + http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) + return + } http.Error(w, "current password is incorrect", http.StatusUnauthorized) return } + resetCurrentPasswordFailures(ctx, userID) } - if twoFARequired() && twoFactorEnabled { + if !hasPassword || (twoFARequired() && twoFactorEnabled) { if req.VerificationCode == "" { http.Error(w, "a two-factor verification code is required to delete the account", http.StatusBadRequest) return @@ -295,28 +416,30 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { // --- External system scrubbing (BEFORE SQL anonymize) --- - // Delete profile picture from S3/R2 - if profilePicURL.Valid && profilePicURL.String != "" && s3.Client != nil { - // #nosec G118 — intentional background goroutine for async profile pic cleanup - go func(picURL string) { - defer func() { - if r := recover(); r != nil { - log.Printf("Panic recovered in S3 profile picture deletion: %v", r) - } - }() - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - bucket := os.Getenv("S3_PROFILE_PICS_BUCKET") - if bucket == "" { - bucket = "crussell-profile-pics" - } - // profiles/{userID}.jpg — matches UploadProfilePictureHandler key format - key := fmt.Sprintf("profiles/%s.jpg", userID) - if err := s3.Client.Delete(ctx, bucket, key); err != nil { - log.Printf("Warning: Failed to delete profile picture for user %s: %v", userID, err) - } - }(profilePicURL.String) + // Durable S3/R2 profile-picture deletion (Fault A2 / FIX 1): instead of the + // old fire-and-forget goroutine, the deletion target is persisted as a + // pending_s3_deletions outbox row INSIDE the anonymization transaction + // below (before it commits), and an async goroutine drains it after the + // commit — mirroring the Square erasure outbox above. If the process dies + // between the local commit and the goroutine finishing, the + // retry-s3-deletions job (internal/jobs/cleanup.go) still finds the row and + // completes the object deletion, so the photo PII is never retained + // indefinitely. + profilePicS3Delete := profilePicURL.Valid && profilePicURL.String != "" && s3.Client != nil + // FIX 4 (fail-closed): never guess the dev bucket "crussell-profile-pics" + // when S3_PROFILE_PICS_BUCKET is unset — deleting from a guessed bucket + // would silently erase an unrelated deployment's objects. Skip the deletion + // and warn: the object may remain until the operator configures the bucket. + profilePicBucket := os.Getenv("S3_PROFILE_PICS_BUCKET") + if profilePicS3Delete && profilePicBucket == "" { + // FIX 2: one loud CRITICAL per process — a silent Warning can scroll by + // unnoticed in a busy log stream. + warnS3ProfilePicsBucketUnset() + profilePicS3Delete = false } + // profiles/{userID}.jpg — matches UploadProfilePictureHandler key format + profilePicKey := fmt.Sprintf("profiles/%s.jpg", userID) + var profilePicDeletionID string // Snapshot the Square card AND customer references per saved-card row // synchronously BEFORE the SQL anonymization below NULLs @@ -404,6 +527,36 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { } } + // FIX 1a: the CardDAV vCard (full name/email/phone/DOB/photo URL PII — + // dav_cards, keyed by uri "{userID}.vcf" under addressbook 1, the same + // Postgres) is deleted INSIDE the erasure transaction, atomically with + // the local erasure. No outbox or retry job is needed: a failed tx + // rolls both back together. Replaces the old fire-and-forget + // dav.Service.DeleteContact goroutine that could only log on failure, + // permanently stranding the contact PII. + if _, err := tx.Exec(ctx, ` + DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = $1 + `, fmt.Sprintf("%s.vcf", userID)); err != nil { + log.Printf("Failed to delete CardDAV contact for guest %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + // A2 durable outbox: persist the S3/R2 profile-picture deletion BEFORE + // this tx commits so the retry-s3-deletions job can complete it after + // a crash (the async goroutine below is the primary drain). + if profilePicS3Delete { + if err := tx.QueryRow(ctx, ` + INSERT INTO pending_s3_deletions (user_id, bucket, object_key) + VALUES ($1, $2, $3) + RETURNING id + `, userID, profilePicBucket, profilePicKey).Scan(&profilePicDeletionID); err != nil { + log.Printf("Failed to persist S3 profile-pic deletion outbox for guest %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + } + if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit transaction for guest user deletion: %v", err) http.Error(w, "server error", http.StatusInternalServerError) @@ -446,6 +599,31 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { } } + // FIX 1a: delete the CardDAV vCard row inside the erasure transaction + // (atomic with the anonymization — see the guest path above). + if _, err := tx.Exec(ctx, ` + DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = $1 + `, fmt.Sprintf("%s.vcf", userID)); err != nil { + log.Printf("Failed to delete CardDAV contact for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + // A2 durable outbox: persist the S3/R2 profile-picture deletion BEFORE + // this tx commits so the retry-s3-deletions job can complete it after + // a crash (the async goroutine below is the primary drain). + if profilePicS3Delete { + if err := tx.QueryRow(ctx, ` + INSERT INTO pending_s3_deletions (user_id, bucket, object_key) + VALUES ($1, $2, $3) + RETURNING id + `, userID, profilePicBucket, profilePicKey).Scan(&profilePicDeletionID); err != nil { + log.Printf("Failed to persist S3 profile-pic deletion outbox for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + } + if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit transaction for user anonymization: %v", err) http.Error(w, "server error", http.StatusInternalServerError) @@ -569,20 +747,44 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { }(sqClient, erasureTargets, notifyCtx) } - // Delete CardDAV contact (non-blocking, best-effort) - if dav.Service != nil { - go func() { + // Durable S3/R2 profile-picture deletion fires only after local + // anonymization/deletion commits, mirroring the Square goroutine above: the + // outbox row persisted in the erasure transaction survives a crash, this + // goroutine is the primary drain, and the retry-s3-deletions job + // (internal/jobs/cleanup.go) is the safety net. + if profilePicS3Delete { + // Capture the client synchronously so the async goroutine never reads + // the global s3.Client (which tests swap per-account). + picClient := s3.Client + // #nosec G118 — intentional background goroutine for async profile pic cleanup + go func(client s3.Uploader, bucket, key, outboxID string) { defer func() { if r := recover(); r != nil { - log.Printf("Panic recovered in CardDAV contact deletion: %v", r) + log.Printf("Panic recovered in S3 profile picture deletion: %v", r) } }() - uri := fmt.Sprintf("%s.vcf", userID) - if err := dav.Service.DeleteContact(1, uri); err != nil { - log.Printf("Warning: Failed to delete CardDAV contact for user %s: %v", userID, err) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := client.Delete(ctx, bucket, key); err != nil { + // The outbox row is deliberately left in place for the + // retry-s3-deletions job, so the photo PII is not retained + // indefinitely. + log.Printf("Warning: Failed to delete profile picture for user %s: %v", userID, err) + return } - }() + // Drain the durable outbox so the safety-net job has nothing to + // retry. Best-effort: a failed drain leaves the row for the job. + if _, err := db.Conn.Exec(ctx, `DELETE FROM pending_s3_deletions WHERE id = $1`, outboxID); err != nil { + slog.Error("failed to drain S3 profile-pic deletion outbox", "user_id", userID, "outbox_id", outboxID, "error", err) + } + }(picClient, profilePicBucket, profilePicKey, profilePicDeletionID) } + // FIX 1a: the CardDAV vCard is deleted INSIDE the erasure transaction above + // (DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = '{userID}.vcf'), + // atomically with the local erasure — the old fire-and-forget + // dav.Service.DeleteContact goroutine (which could only log on failure, + // permanently stranding the contact PII) is gone. + w.WriteHeader(http.StatusNoContent) } diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index 0026e01..d5ab0f6 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -701,7 +701,7 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { return } - var passwordHash string + var passwordHash sql.NullString err := db.Conn.QueryRow(r.Context(), `SELECT password_hash FROM users WHERE id = $1`, userID).Scan(&passwordHash) if err != nil { if errors.Is(err, pgx.ErrNoRows) { @@ -712,11 +712,45 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "server error", http.StatusInternalServerError) return } + if !passwordHash.Valid || passwordHash.String == "" { + // FIX 5: a passwordless (social-only) account has no password to change. + // Scan NULL into a plain string used to 500; a clear 400 is actionable. + http.Error(w, "this account has no password to change", http.StatusBadRequest) + return + } - if err := bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(req.CurrentPassword)); err != nil { + // FIX 3: the current-password compare shares the same per-user + // failed-attempt/lockout budget as delete-account (checkCurrentPasswordLockout + // + recordCurrentPasswordFailure/resetCurrentPasswordFailures, backed by + // users.failed_attempts/locked_until). Without it a stolen session token + // would let an attacker brute-force the current password with unlimited + // guesses. + if err := checkCurrentPasswordLockout(r.Context(), userID); err != nil { + if errors.Is(err, errCurrentPasswordLockedOut) { + // FIX 4: uniform 401 — locked-vs-wrong is never distinguishable; + // the body text still tells the UI which one happened. + http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) + return + } + log.Printf("Failed to check current-password lockout for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil { + // FIX 3: one atomic UPDATE ... RETURNING (increment + escalation) — + // concurrent wrong-password requests cannot race a check-then-increment. + newCount, _, recordErr := recordCurrentPasswordFailure(r.Context(), userID) + if recordErr != nil { + log.Printf("Failed to record current-password failure for user %s: %v", userID, recordErr) + } + if newCount >= 5 { + http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized) + return + } http.Error(w, "current password is incorrect", http.StatusUnauthorized) return } + resetCurrentPasswordFailures(r.Context(), userID) newHash, err := bcrypt.GenerateFromPassword([]byte(req.NewPassword), bcrypt.DefaultCost) if err != nil { diff --git a/backend/internal/jobs/cleanup.go b/backend/internal/jobs/cleanup.go index 81a0431..a8be7d8 100644 --- a/backend/internal/jobs/cleanup.go +++ b/backend/internal/jobs/cleanup.go @@ -16,6 +16,7 @@ import ( "crussell/handlers/scheduling" "crussell/handlers/user" "crussell/internal/adminnotify" + "crussell/internal/s3" "crussell/internal/square" "crussell/mw" @@ -266,6 +267,21 @@ func RegisterAll(s *Scheduler) { Concurrency: 1, Handler: RetryPendingSquareErasures, }) + + // Durable safety net for the account-deletion S3/R2 profile-picture outbox + // (Fault A2): retries the object deletions that DeleteAccountHandler's + // async cleanup goroutine could not finish (process crash or an S3 outage), + // using the pending_s3_deletions rows the handler persisted inside its + // anonymization tx before it committed. Hourly, offset from + // retry-square-erasures (:37 vs :17) so the two erasure jobs cannot + // contend. + s.Register(Job{ + Name: "retry-s3-deletions", + Schedule: "37 * * * *", // Hourly at :37 + Timeout: 30 * time.Second, + Concurrency: 1, + Handler: RetryPendingS3Deletions, + }) } // SweepSquareWebhookEvents deletes square_webhook_events rows older than the @@ -576,3 +592,77 @@ func RetryPendingSquareErasures(ctx context.Context) (int, error) { } return len(drained), nil } + +// RetryPendingS3Deletions is the durable safety net for the account-deletion +// S3/R2 profile-picture outbox (Fault A2). DeleteAccountHandler persists the +// deletion target (bucket + object key) inside its anonymization transaction, +// before it commits. If the process crashes between that commit and the async +// cleanup goroutine finishing, the photo's PII would otherwise be retained in +// the object store indefinitely — this job finds the pending rows and retries +// the deletion so the erasure is eventually complete. On success it deletes +// the outbox row; on failure it leaves the row in place (bumping attempts and +// recording the error) so the next run retries again. Returns the number of +// outbox rows drained. +func RetryPendingS3Deletions(ctx context.Context) (int, error) { + if s3.Client == nil { + // No object store configured: no deletion is possible, and the handler + // only writes outbox entries when a client was configured. + return 0, nil + } + client := s3.Client + + rows, err := db.Conn.Query(ctx, ` + SELECT id, bucket, object_key + FROM pending_s3_deletions + ORDER BY created_at + `) + if err != nil { + return 0, fmt.Errorf("failed to query pending S3 deletions: %w", err) + } + defer rows.Close() + + type pendingDelete struct { + id string + bucket string + key string + } + var pending []pendingDelete + for rows.Next() { + var p pendingDelete + if err := rows.Scan(&p.id, &p.bucket, &p.key); err != nil { + return 0, fmt.Errorf("failed to scan pending S3 deletion: %w", err) + } + pending = append(pending, p) + } + if err := rows.Err(); err != nil { + return 0, fmt.Errorf("failed to iterate pending S3 deletions: %w", err) + } + if len(pending) == 0 { + return 0, nil + } + + drained := 0 + for _, p := range pending { + actx, cancel := context.WithTimeout(ctx, 30*time.Second) + err := client.Delete(actx, p.bucket, p.key) + cancel() + if err != nil { + if _, uerr := db.Conn.Exec(ctx, ` + UPDATE pending_s3_deletions SET attempts = attempts + 1, last_error = $2 + WHERE id = $1 + `, p.id, err.Error()); uerr != nil { + return drained, fmt.Errorf("failed to record S3 deletion retry failure %s: %w", p.id, uerr) + } + log.Printf("Warning: retry-s3-deletions failed to delete profile picture %s (outbox %s): %v", p.key, p.id, err) + continue + } + if _, err := db.Conn.Exec(ctx, `DELETE FROM pending_s3_deletions WHERE id = $1`, p.id); err != nil { + return drained, fmt.Errorf("failed to drain S3 deletion outbox row %s: %w", p.id, err) + } + drained++ + } + if drained > 0 { + log.Printf("[ERASURE] retry-s3-deletions drained %d pending S3 deletion outbox row(s)", drained) + } + return drained, nil +} diff --git a/backend/internal/jobs/scheduler_test.go b/backend/internal/jobs/scheduler_test.go index 3257866..43f7c17 100644 --- a/backend/internal/jobs/scheduler_test.go +++ b/backend/internal/jobs/scheduler_test.go @@ -413,8 +413,8 @@ func TestRegisterAll_RegistersExpectedJobs(t *testing.T) { s := New() RegisterAll(s) - if got := len(s.registry); got != 26 { - t.Fatalf("RegisterAll() registered %d jobs, want 26", got) + if got := len(s.registry); got != 27 { + t.Fatalf("RegisterAll() registered %d jobs, want 27", got) } registered := make(map[string]Job, len(s.registry)) @@ -494,6 +494,7 @@ func expectedJobNames() map[string]bool { "apply-default-hours": true, "scan-critical-payment-logs": true, "retry-square-erasures": true, + "retry-s3-deletions": true, } }