From f2df287b355375e80a5550c2fd616980302e4bfd Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Mon, 18 May 2026 17:01:25 -0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20Phase=203=20post-deploy=20audi?= =?UTF-8?q?t=20=E2=80=94=202=20P0=20+=20P1=20+=20P2=20(R5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent adversarial audit verdict was NOT SAFE; fixed: - P0-1 /auth/confirm no longer trusts the query `type` (pinned to 'email_change'); a public endpoint feeding an attacker-chosen OTP type into verifyOtp was an unintended auth entrypoint. - P0-2 email sync uses the verifyOtp response user, not a follow-up getUser() that may not reflect the just-verified identity; /me now reconciles space_members.email against the authoritative auth email on load, so a missed sync self-heals (no permanent stale email). - P1-1 requestEmailChange builds the confirm origin from NEXT_PUBLIC_APP_URL only, never request headers (forged Host could redirect the confirmation link / capture token_hash). - P2-1 isSecretConfigField also catches password/token/private_key/ *_token/*_password so non-conventionally-named credentials can't be stored in plaintext integrations.config. 521 tests; build clean. --- __tests__/secrets-vault.test.ts | 8 ++++++-- app/(app)/me/page.tsx | 14 ++++++++++++++ app/auth/confirm/route.ts | 30 ++++++++++++++++++------------ lib/actions/members.ts | 11 ++++------- lib/secrets/vault.ts | 16 ++++++++++++---- 5 files changed, 54 insertions(+), 25 deletions(-) diff --git a/__tests__/secrets-vault.test.ts b/__tests__/secrets-vault.test.ts index 3bf416e..f7032dc 100644 --- a/__tests__/secrets-vault.test.ts +++ b/__tests__/secrets-vault.test.ts @@ -3,13 +3,17 @@ import { isSecretConfigField } from '@/lib/secrets/vault' describe('isSecretConfigField', () => { it('treats credential-named fields as secrets to vault', () => { - for (const k of ['client_secret', 'api_key', 'secret_key', 'webhook_secret', 'signing_secret']) { + for (const k of [ + 'client_secret', 'api_key', 'secret_key', 'webhook_secret', 'signing_secret', + 'password', 'token', 'private_key', 'access_token', 'refresh_token', + 'admin_password', 'bot_token', 'CLIENT_SECRET', + ]) { expect(isSecretConfigField(k)).toBe(true) } }) it('leaves non-secret config fields in plaintext config', () => { - for (const k of ['client_id', 'sandbox', 'mode', 'publishable_key', 'username']) { + for (const k of ['client_id', 'sandbox', 'mode', 'publishable_key', 'username', 'tokenize_mode']) { expect(isSecretConfigField(k)).toBe(false) } }) diff --git a/app/(app)/me/page.tsx b/app/(app)/me/page.tsx index a56d8a4..1e72bcf 100644 --- a/app/(app)/me/page.tsx +++ b/app/(app)/me/page.tsx @@ -1,5 +1,6 @@ import { redirect } from 'next/navigation' import { createClient } from '@/lib/supabase/server' +import { createAdminClient } from '@/lib/supabase/admin' import { PageHeader, PageTitle } from '@/components/ui/page-title' import { PERMISSIONS, PERMISSION_CODES } from '@/lib/permissions-catalog' import { getMyClassSignups, getMyReservations, getMyCards, getMyVisits, getMyBilling, getMyNotifications, getMyPayments } from '@/lib/actions' @@ -33,6 +34,19 @@ export default async function MePage() { .maybeSingle() if (!member) redirect('/signup') + // Self-heal: auth.users.email is authoritative (only changes after Supabase + // confirmation). If the denormalized space_members.email drifted (e.g. the + // /auth/confirm sync was missed because the second secure-change link was + // opened in a different browser), reconcile it here on the owner's own row. + if (user.email && member.email !== user.email) { + await createAdminClient() + .from('space_members') + .update({ email: user.email }) + .eq('id', member.id) + .eq('space_id', member.space_id) + member.email = user.email + } + const profile = { email: (member.email as string | null) ?? '', display_name: (member.display_name as string | null) ?? '', diff --git a/app/auth/confirm/route.ts b/app/auth/confirm/route.ts index eeb3cbd..653030f 100644 --- a/app/auth/confirm/route.ts +++ b/app/auth/confirm/route.ts @@ -5,35 +5,41 @@ // the change applies to auth.users only after both links are verified; this // route is hit once per link and is idempotent. // -// The denormalized space_members.email is synced to the authoritative -// auth.users email ONLY here, after a successful verifyOtp — never at request -// time (the user has not proven ownership yet). +// This route does EXACTLY ONE thing: confirm an email change. The OTP type is +// pinned to 'email_change' and the query `type` is intentionally ignored — a +// public endpoint that fed an attacker-chosen type into verifyOtp would be an +// unintended auth entrypoint (recovery/magiclink/signup tokens minting a +// session here, bypassing their real flows). +// +// The denormalized space_members.email is synced from the verifyOtp response +// user (NOT a follow-up getUser(), which is not guaranteed to reflect the +// just-verified identity in this SSR request). /me additionally reconciles on +// load, so a missed sync here self-heals. import { createClient } from '@/lib/supabase/server' import { createAdminClient } from '@/lib/supabase/admin' import { NextResponse } from 'next/server' -import type { EmailOtpType } from '@supabase/supabase-js' export async function GET(request: Request) { const { searchParams, origin } = new URL(request.url) const tokenHash = searchParams.get('token_hash') - const type = (searchParams.get('type') ?? 'email_change') as EmailOtpType if (!tokenHash) { return NextResponse.redirect(`${origin}/login?error=email_change`) } const supabase = await createClient() - const { error } = await supabase.auth.verifyOtp({ type, token_hash: tokenHash }) + const { data, error } = await supabase.auth.verifyOtp({ + type: 'email_change', + token_hash: tokenHash, + }) if (error) { return NextResponse.redirect(`${origin}/login?error=email_change`) } - // Sync the denormalized copy to whatever auth.users now holds. On the first - // of two secure-change links the email may still be the old one (no-op); - // the second link's hit syncs it. Idempotent either way. - const { - data: { user }, - } = await supabase.auth.getUser() + // Use the verifyOtp response user. On the first of two secure-change links + // auth.users.email is still the old value (no-op sync); the second link's + // hit carries the new email. Idempotent; /me reconciles any miss. + const user = data.user if (user?.email) { await createAdminClient() .from('space_members') diff --git a/lib/actions/members.ts b/lib/actions/members.ts index a6f3a06..d706abd 100644 --- a/lib/actions/members.ts +++ b/lib/actions/members.ts @@ -1,7 +1,6 @@ 'use server' import { revalidatePath } from 'next/cache' -import { headers } from 'next/headers' import { createClient } from '@/lib/supabase/server' import { createAdminClient } from '@/lib/supabase/admin' import { linkSubmissionsByEmail } from './forms' @@ -240,12 +239,10 @@ export async function requestEmailChange(input: { email: string }) { const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } - const h = await headers() - const host = h.get('x-forwarded-host') ?? h.get('host') - const proto = h.get('x-forwarded-proto') ?? 'https' - const origin = host - ? `${proto}://${host}` - : process.env.NEXT_PUBLIC_APP_URL || 'https://hackerspace.sh' + // Origin from trusted server config ONLY — never request headers. A forged + // Host/X-Forwarded-Host could otherwise point the confirmation link at an + // attacker origin and capture the victim's token_hash. + const origin = process.env.NEXT_PUBLIC_APP_URL || 'https://hackerspace.sh' const { error } = await supabase.auth.updateUser( { email: v.data.email }, diff --git a/lib/secrets/vault.ts b/lib/secrets/vault.ts index db394ca..3152a31 100644 --- a/lib/secrets/vault.ts +++ b/lib/secrets/vault.ts @@ -72,10 +72,18 @@ export async function readSecret( // are never themselves secrets. export function isSecretConfigField(key: string): boolean { if (key.endsWith('_set') || key.endsWith('_ref')) return false + const k = key.toLowerCase() return ( - key === 'client_secret' || - key === 'api_key' || - key === 'secret_key' || - key.endsWith('_secret') + k === 'client_secret' || + k === 'api_key' || + k === 'secret_key' || + k === 'password' || + k === 'token' || + k === 'private_key' || + k === 'access_token' || + k === 'refresh_token' || + k.endsWith('_secret') || + k.endsWith('_token') || + k.endsWith('_password') ) }