From 908ec23eda38526170322c3edf71ba45ecb1ca95 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Wed, 12 Aug 2026 12:08:54 -0700 Subject: [PATCH] fix: sanitize user data (#3552) --- extensions/whoami.test.ts | 78 +++++++++++++++++++++++++++++++++++++++ extensions/whoami.ts | 77 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 152 insertions(+), 3 deletions(-) diff --git a/extensions/whoami.test.ts b/extensions/whoami.test.ts index 0e34b1b14..9efa08380 100644 --- a/extensions/whoami.test.ts +++ b/extensions/whoami.test.ts @@ -206,6 +206,84 @@ describe('whoami extension — handleWhoami', () => { expect(JSON.stringify(body)).not.toContain('bootstrap-secret'); }); + it('never exposes phone, card fingerprint or signup identity', async () => { + const user = await seedUser(); + await server.stores.user.update(user.id as number, { + phone: '+15551234567', + card_fingerprint: 'fp_ABC123', + requires_phone_verification: false, + requires_card_verification: false, + }); + // Guard against a vacuous assertion below: the columns really do hold + // the values we then expect never to see on the wire. + const stored = await server.stores.user.getById(user.id as number, { + cached: false, + force: true, + }); + expect(stored?.phone).toBe('+15551234567'); + expect(stored?.card_fingerprint).toBe('fp_ABC123'); + + for (const actor of [ + { user: { uuid: user.uuid, id: user.id as number } }, + { + user: { uuid: user.uuid, id: user.id as number }, + app: { uid: 'app-test-actor' }, + }, + ]) { + const { res, captured } = makeRes(); + await runWithContext({ actor }, () => + handleWhoami(makeReq(), res), + ); + + const body = captured.body as Record; + expect(body.phone).toBeUndefined(); + expect(body.card_fingerprint).toBeUndefined(); + expect(body.password).toBeUndefined(); + expect(body.otp_secret).toBeUndefined(); + expect(body.signup_ip).toBeUndefined(); + // Not just absent as a key — the values must not appear anywhere + // in the payload (nested under metadata, taskbar items, …). + const serialized = JSON.stringify(body); + expect(serialized).not.toContain('5551234567'); + expect(serialized).not.toContain('fp_ABC123'); + // The verification flags, which the GUI acts on, still ship. + expect(body).toHaveProperty('requires_phone_verification'); + expect(body).toHaveProperty('requires_card_verification'); + } + }); + + it('scrubs sensitive keys added to metadata, and leaves the cached row intact', async () => { + const user = await seedUser(); + await server.stores.user.updateMetadata(user.id as number, { + tmp_password: 'bootstrap-secret', + billing: { card_fingerprint: 'fp_NESTED', tier: 'pro' }, + }); + + const { res, captured } = makeRes(); + await runWithContext( + { actor: { user: { uuid: user.uuid, id: user.id as number } } }, + () => handleWhoami(makeReq(), res), + ); + + const body = captured.body as Record; + const metadata = body.metadata as Record; + expect(metadata.tmp_password).toBeUndefined(); + // Nested sensitive keys are removed too; siblings survive. + expect(metadata.billing).toEqual({ tier: 'pro' }); + expect(JSON.stringify(body)).not.toContain('fp_NESTED'); + + // The scrub works on a copy — server-side state is untouched. + const fresh = await server.stores.user.getById(user.id as number, { + cached: false, + force: true, + }); + expect(fresh?.metadata?.tmp_password).toBe('bootstrap-secret'); + expect( + (fresh?.metadata?.billing as Record) + ?.card_fingerprint, + ).toBe('fp_NESTED'); + }); + it('marks the user as oidc_only when password is null', async () => { const slug = Math.random().toString(36).slice(2, 8); const oidcUser = await server.stores.user.create({ diff --git a/extensions/whoami.ts b/extensions/whoami.ts index 2d44f8042..fc245b943 100644 --- a/extensions/whoami.ts +++ b/extensions/whoami.ts @@ -33,6 +33,68 @@ const CLIENT_VISIBLE_FEATURE_FLAGS: ReadonlySet = new Set([ 'prompt_user_when_navigation_away_from_puter', ]); +// Keys that must never leave the server, whoever put them on the response. +// `details` is an explicit pick, but the `whoami.details` event hands +// listeners the full UserRow next to the object they may write to, and +// `metadata` is a free-form blob — so any of these can arrive on the +// response without an edit to the pick above. The scrub runs last, over the +// whole payload, and is the one place that decides what "sensitive" means. +// +// Credentials and single-use tokens, the payment/phone identifiers used for +// verification (`card_fingerprint` is the Stripe fingerprint, stable per +// card number), the network identity recorded at signup, and internal +// anti-abuse bookkeeping. `requires_phone_verification` / +// `requires_card_verification` stay: they are the flags the GUI acts on, and +// they carry no identifier. +const SENSITIVE_KEYS: ReadonlySet = new Set([ + 'password', + 'tmp_password', + 'pass_recovery_token', + 'email_confirm_code', + 'email_confirm_token', + 'change_email_confirm_token', + 'otp_secret', + 'otp_recovery_codes', + 'card_fingerprint', + 'phone', + 'clean_email', + 'signup_ip', + 'signup_ip_forwarded', + 'signup_user_agent', + 'signup_origin', + 'signup_server', + 'audit_metadata', +]); + +// Depth-limited, cycle-safe walk deleting every SENSITIVE_KEYS entry it finds +// at any level (`metadata` and `taskbar_items` are both nested structures). +const scrubSensitive = ( + value: unknown, + seen: Set = new Set(), + depth = 0, +): void => { + if (depth > 8 || value === null || typeof value !== 'object') return; + if (seen.has(value as object)) return; + seen.add(value as object); + + if (Array.isArray(value)) { + for (const entry of value) scrubSensitive(entry, seen, depth + 1); + return; + } + + for (const key of Object.keys(value as Record)) { + if (SENSITIVE_KEYS.has(key)) { + delete (value as Record)[key]; + continue; + } + scrubSensitive( + (value as Record)[key], + seen, + depth + 1, + ); + } +}; + export const handleWhoami = async ( req: Request, res: Response, @@ -74,8 +136,12 @@ export const handleWhoami = async ( } } - const metadata = user.metadata ? { ...user.metadata } : user.metadata; - if (metadata) delete metadata.tmp_password; + // Deep-copied (it is decoded JSON) so the scrub below edits the response + // and not the cached UserRow. Sensitive keys inside it — tmp_password and + // anything else on the denylist — are removed by scrubSensitive. + const metadata = user.metadata + ? structuredClone(user.metadata) + : user.metadata; const details: Record = { username: user.username, @@ -84,7 +150,9 @@ export const handleWhoami = async ( unconfirmed_email: user.email, email_confirmed: user.email_confirmed || user.username === 'admin', requires_email_confirmation: user.requires_email_confirmation, - phone: user.phone, + // The phone number itself is deliberately absent: nothing on the + // client reads it, and it is PII that would otherwise be handed to + // every app actor. Only the verification flag ships. requires_phone_verification: user.requires_phone_verification, requires_card_verification: user.requires_card_verification, desktop_bg_url: user.desktop_bg_url, @@ -196,6 +264,9 @@ export const handleWhoami = async ( delete subscription.offering.price_id; } + // Last word on what ships, after every listener has had its say. + scrubSensitive(details); + res.json(details); };