Skip to content

feat: add account deletion and data export (PR 1.7) - #16

Merged
Jeston10 merged 6 commits into
milestone/p1-securityfrom
feat/account-deletion-export
Oct 25, 2026
Merged

Jeston10 merged 6 commits into
milestone/p1-securityfrom
feat/account-deletion-export

Conversation

@Jeston10

@Jeston10 Jeston10 commented Oct 25, 2026 •

Copy link
Copy Markdown
Owner

PR 1.7 — feat/account-deletion-export (MILESTONES §P1, last PR of Phase 1)

Right of access and right of erasure, plus a defect this PR's testing exposed.

Risk register (docs/RISKS.md)

  • R22 (sessions live up to 7 days after a revoke, because checkRevoked is only used on sensitive ops): deletion and export are exactly those sensitive ops — both call requireUserChecked, and deletion additionally requires a password re-authentication from the last 5 minutes. Row unchanged (Accepted), honoured here.
  • R28 — new. While testing, the configured Upstash host stopped resolving (getaddrinfo ENOTFOUND). Every rate-limited path — sign-in, sign-up, feedback, profile — failed closed, so a free-tier vendor blip with no SLA took the whole app down. enforceLimit now catches limiter transport errors, reports them (ratelimit.unavailable → Sentry), and falls back to the process-local limiter, so a cap stays in force per instance instead of the request dying. Accepted degradation (documented in the row): with N instances the effective limit is N× while Upstash is unreachable.

What

  • server/services/account.service.ts — one list (USER_COLLECTIONS) drives both export and deletion, so a collection added later is wired in one place. Deletion: owned documents in 400-doc batches → user document → Auth user (which revokes every session) → the uid's rate-limit windows → the PostHog person. Side-state failures are logged with the uid, never swallowed, and never leave data behind.
  • features/account/actions.ts — requireUserChecked plus verifyIdToken(token, true) with an auth_time recency check. A stolen session cookie alone cannot delete an account.
  • DangerZone replaces the "Delete account feature coming soon" stub: download your data as JSON, type DELETE, re-enter your password. Edged surfaces, no rounded cards.
  • PostHog erasure via the Persons API when POSTHOG_PERSONAL_API_KEY + POSTHOG_PROJECT_ID are set; otherwise the uid is logged for manual follow-up. Sentry holds no PII to erase — events carry the uid only and expire with the tier's 30-day retention. SECURITY §2.11 now says this instead of implying a Sentry deletion API call.
  • Profile N+1 removed — server/services/stats.service.ts does one aggregate query where the page previously did one feedback read per interview (GUARDRAILS B8). Denormalised onto users.stats in P2.5.
  • New limits: account.export 3/day, account.delete 3/hour, both by uid.

Verified

  • Rebased onto security: patch critical advisories in the dependency tree #17 (dependency advisories), so the gates below are measured on the patched tree: pnpm typecheck · pnpm lint · pnpm test → 94/94 (20 files) · pnpm audit --audit-level=high clean · build clean · size 660.27 kB / 675 kB.
  • Full deletion flow against the real Firebase project, with a throwaway account driven through the UI in a browser: signed up → seeded an interview + feedback for the uid → signed in → Settings → Delete Account → typed DELETE + password → redirected to /sign-in. Server log: {"op":"account.delete","deleted":{"interviews":1,"feedback":1,"users":1}}. Admin-SDK check afterwards: {"authUser":null,"userDoc":false,"interviewDoc":false,"feedbackDoc":false}. The throwaway accounts were cleaned up.
  • The export button returned success and triggered the JSON download (content shape covered by unit tests).
  • R28 fix verified live: with Upstash unreachable, sign-up and sign-in now succeed and each logs ratelimit.unavailable at error level; before the fix the same conditions produced sign-up failed.
  • Unit tests cover: export returns only the caller's documents (asserts no other uid appears anywhere in the payload), deletion removes exactly the right documents and survives a limiter failure, stale/foreign/absent re-auth is refused, rate limits are enforced, the degraded limiter still caps.

Three defects the E2E spec exposed (all pre-existing, all fixed here)

  1. No form field had a label. FormField rendered a bare <label> beside an <Input> with no id, so every field on sign-in and sign-up was an unnamed textbox to a screen reader and clicking a label did nothing (WCAG 1.3.1/4.1.2). Labels are now associated, and validation messages are wired with aria-describedby/aria-invalid.
  2. The signed-in header overflowed at 375 px. The clock, logo, streak star and the w-full max-w-2xl career quote shared one non-wrapping flex row, pushing the page to 558 px wide on a 375 px screen. A page wider than the screen widens the layout viewport, and every position: fixed overlay is then laid out against that wider box — which is why the delete dialog rendered half off-screen and the click aimed at its button landed on the panel instead. The row now wraps; desktop is unchanged. This affected every phone user, not just the test.
  3. The delete dialog was not a dialog. It is now role="dialog" aria-modal aria-labelledby, portals to document.body, scrolls within max-h-[calc(100dvh-2rem)] instead of squeezing its children, and keeps its actions pinned to the bottom of the panel.

Not verified locally

tests/e2e/account-deletion.spec.ts (sign up → delete → documents gone → credentials rejected, against the emulators) needs JDK 21; this machine has 19. It now passes in CI at all three widths — that is the P1.7 exit gate. The same flow was also driven by hand against the real project at 375 px: dialog fully on screen, account deleted {users: 1}, redirect to /sign-in, throwaway account cleaned up.

New warning recorded

R29 — next dev prints server-action arguments, so a live Firebase ID token appears in the dev terminal (ƒ signUp({"idToken":"eyJ…"})). It never reaches our pino redaction because the framework prints it, not us. Dev-only (next start does not), so it is recorded as Open with a handling note rather than silently accepted.

Owner action

Check your Upstash database — close-marlin-287011.upstash.io no longer resolves from here. If it was deleted or recreated, update UPSTASH_REDIS_REST_URL/_TOKEN in .env.local and Vercel. The app now survives it, but limits are per-instance until it is reachable.

🤖 Generated with Claude Code

Copilot AI balanced review requested due to automatic review settings October 25, 2026 11:56
@vercel

vercel Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
just-prep-ai-interview-z3cv Error Error Oct 25, 2026 4:07pm UTC

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Right of access and right of erasure, both gated on a live session.

- server/services/account.service.ts walks one list (USER_COLLECTIONS)
  for both export and deletion, so a future collection is added in one
  place. Deletion removes owned documents in batches, the user
  document, the Auth user (revoking every session), the uid's
  rate-limit windows, and the PostHog person; side-state failures are
  logged, never swallowed, and never leave data behind.
- features/account/actions.ts requires requireUserChecked plus a
  password re-authentication from the last 5 minutes (verifyIdToken
  with checkRevoked and an auth_time check), so a stolen session
  cookie cannot delete an account (RISKS R22).
- DangerZone replaces the "coming soon" stub: download your JSON, type
  DELETE, re-enter your password. Edged surfaces, no rounded cards.
- PostHog erasure uses the Persons API when POSTHOG_PERSONAL_API_KEY
  and POSTHOG_PROJECT_ID are set; otherwise the uid is logged for
  manual follow-up. Sentry holds no PII to erase.
- Profile page: one aggregate query instead of one read per interview
  (server/services/stats.service.ts), removing the N+1.

Also fixes a defect this PR's testing exposed (RISKS R28): when the
Upstash host stopped resolving, every rate-limited action failed
closed, so sign-in, sign-up, feedback, and profile were all down.
enforceLimit now reports the outage to Sentry and falls back to the
process-local limiter, keeping a cap in force per instance.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The E2E deletion spec could not find the sign-up fields, and the reason
was not the test: FormField rendered a bare <label> next to an <Input>
with no id, so every field on sign-in and sign-up was an unnamed
textbox to assistive technology (WCAG 1.3.1/4.1.2), and clicking a
label did nothing. The label now points at the input, and a validation
message is wired to it with aria-describedby and aria-invalid.

DangerZone is now a labelled modal (role=dialog, aria-modal,
aria-labelledby). The settings menu that opens it stays mounted behind
it, so "Delete Account" existed twice in the accessibility tree; the
spec scopes to the dialog.

Verified in a browser at 375 px and 1440 px: each input resolves by its
label, clicking "Email" focuses the email field, no horizontal overflow
at either width.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The delete panel is taller than a 375x667 screen. It sat inside a
centred `overflow-y-auto` container, which clips an overflowing child
at the top and leaves the submit button under the description text —
Playwright caught it as a pointer interception at all three widths, and
a real phone would hit the same wall. The panel now scrolls itself
within `max-h-[calc(100dvh-2rem)]`, so every control stays reachable.

Also records R29: `next dev` prints server-action arguments, so a live
ID token appears in the dev terminal. It is dev-only (`next start` does
not), but it bypasses our pino redaction entirely, so it is a tracked
warning rather than a silent one.

Verified against the real project at 375 px and on desktop: signed up,
signed in, opened Settings -> Delete Account, confirmed with DELETE and
the password, landed on /sign-in, and the server logged
`account deleted {users: 1}`. The throwaway account removed itself.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Capping the panel height was not enough: a flex column that cannot grow
shrinks its children instead of scrolling, so the description and the
controls ended up sharing space and a click on "Delete account" landed
on the paragraph above it. Children no longer shrink, and the
Cancel/Delete row is sticky at the bottom of the panel, so the action
is reachable at any viewport height without scrolling the dialog.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CI screenshot showed the cause the logs only hinted at: at 375 px
the dialog hung off both edges of the screen, clipped on the left, so
the click Playwright aimed at the button landed on the panel instead.
The dialog is rendered from the settings menu, inside the animated
header, and an animated or transformed ancestor makes `position: fixed`
resolve against that ancestor rather than the viewport. On a phone the
delete dialog was genuinely half off-screen.

It now portals to document.body, so the overlay is the viewport.
Measured with Playwright against the dev server: the overlay rect is
exactly the viewport box and the button is the hit-test target at
every point across its width.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This is what the E2E failure was actually reporting. The header packs
the clock, logo, streak star and the career quote into one
non-wrapping flex row; the quote is `w-full max-w-2xl`, so at 375 px it
pushed the page to 558 px wide. A page wider than the screen widens the
layout viewport, and every `position: fixed` overlay is then laid out
against that wider box — which is why the delete dialog rendered
half off-screen and a click aimed at its button landed on the panel.
The dialog was the symptom; the header was the bug, and it affected
every phone user, not just the test.

The row now wraps. Desktop alignment is unchanged.

Measured at 375x812 in the browser: documentElement.scrollWidth is 375
(was 558), the dialog overlay is exactly the viewport box, the panel
sits at x=16 with width 343, and the delete button is the hit-test
target across its full width. Deleted a throwaway account through the
dialog at that width: `account deleted {users: 1}`, redirected to
/sign-in. At 1440 px nothing shifts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Jeston10
Jeston10 merged commit 9a9a01d into milestone/p1-security Oct 25, 2026
6 of 7 checks passed
@Jeston10
Jeston10 deleted the feat/account-deletion-export branch October 25, 2026 16:09

This branch had an error being deployed

1 failed deployment
Preview — 0495a903 Deployed Oct 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants