fix(security): Phase 3 post-deploy audit — 2 P0 + P1 + P2 (R5)
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.
This commit is contained in:
@@ -3,13 +3,17 @@ import { isSecretConfigField } from '@/lib/secrets/vault'
|
|||||||
|
|
||||||
describe('isSecretConfigField', () => {
|
describe('isSecretConfigField', () => {
|
||||||
it('treats credential-named fields as secrets to vault', () => {
|
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)
|
expect(isSecretConfigField(k)).toBe(true)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
it('leaves non-secret config fields in plaintext config', () => {
|
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)
|
expect(isSecretConfigField(k)).toBe(false)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { redirect } from 'next/navigation'
|
import { redirect } from 'next/navigation'
|
||||||
import { createClient } from '@/lib/supabase/server'
|
import { createClient } from '@/lib/supabase/server'
|
||||||
|
import { createAdminClient } from '@/lib/supabase/admin'
|
||||||
import { PageHeader, PageTitle } from '@/components/ui/page-title'
|
import { PageHeader, PageTitle } from '@/components/ui/page-title'
|
||||||
import { PERMISSIONS, PERMISSION_CODES } from '@/lib/permissions-catalog'
|
import { PERMISSIONS, PERMISSION_CODES } from '@/lib/permissions-catalog'
|
||||||
import { getMyClassSignups, getMyReservations, getMyCards, getMyVisits, getMyBilling, getMyNotifications, getMyPayments } from '@/lib/actions'
|
import { getMyClassSignups, getMyReservations, getMyCards, getMyVisits, getMyBilling, getMyNotifications, getMyPayments } from '@/lib/actions'
|
||||||
@@ -33,6 +34,19 @@ export default async function MePage() {
|
|||||||
.maybeSingle()
|
.maybeSingle()
|
||||||
if (!member) redirect('/signup')
|
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 = {
|
const profile = {
|
||||||
email: (member.email as string | null) ?? '',
|
email: (member.email as string | null) ?? '',
|
||||||
display_name: (member.display_name as string | null) ?? '',
|
display_name: (member.display_name as string | null) ?? '',
|
||||||
|
|||||||
+18
-12
@@ -5,35 +5,41 @@
|
|||||||
// the change applies to auth.users only after both links are verified; this
|
// the change applies to auth.users only after both links are verified; this
|
||||||
// route is hit once per link and is idempotent.
|
// route is hit once per link and is idempotent.
|
||||||
//
|
//
|
||||||
// The denormalized space_members.email is synced to the authoritative
|
// This route does EXACTLY ONE thing: confirm an email change. The OTP type is
|
||||||
// auth.users email ONLY here, after a successful verifyOtp — never at request
|
// pinned to 'email_change' and the query `type` is intentionally ignored — a
|
||||||
// time (the user has not proven ownership yet).
|
// 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 { createClient } from '@/lib/supabase/server'
|
||||||
import { createAdminClient } from '@/lib/supabase/admin'
|
import { createAdminClient } from '@/lib/supabase/admin'
|
||||||
import { NextResponse } from 'next/server'
|
import { NextResponse } from 'next/server'
|
||||||
import type { EmailOtpType } from '@supabase/supabase-js'
|
|
||||||
|
|
||||||
export async function GET(request: Request) {
|
export async function GET(request: Request) {
|
||||||
const { searchParams, origin } = new URL(request.url)
|
const { searchParams, origin } = new URL(request.url)
|
||||||
const tokenHash = searchParams.get('token_hash')
|
const tokenHash = searchParams.get('token_hash')
|
||||||
const type = (searchParams.get('type') ?? 'email_change') as EmailOtpType
|
|
||||||
|
|
||||||
if (!tokenHash) {
|
if (!tokenHash) {
|
||||||
return NextResponse.redirect(`${origin}/login?error=email_change`)
|
return NextResponse.redirect(`${origin}/login?error=email_change`)
|
||||||
}
|
}
|
||||||
|
|
||||||
const supabase = await createClient()
|
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) {
|
if (error) {
|
||||||
return NextResponse.redirect(`${origin}/login?error=email_change`)
|
return NextResponse.redirect(`${origin}/login?error=email_change`)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Sync the denormalized copy to whatever auth.users now holds. On the first
|
// Use the verifyOtp response user. On the first of two secure-change links
|
||||||
// of two secure-change links the email may still be the old one (no-op);
|
// auth.users.email is still the old value (no-op sync); the second link's
|
||||||
// the second link's hit syncs it. Idempotent either way.
|
// hit carries the new email. Idempotent; /me reconciles any miss.
|
||||||
const {
|
const user = data.user
|
||||||
data: { user },
|
|
||||||
} = await supabase.auth.getUser()
|
|
||||||
if (user?.email) {
|
if (user?.email) {
|
||||||
await createAdminClient()
|
await createAdminClient()
|
||||||
.from('space_members')
|
.from('space_members')
|
||||||
|
|||||||
@@ -1,7 +1,6 @@
|
|||||||
'use server'
|
'use server'
|
||||||
|
|
||||||
import { revalidatePath } from 'next/cache'
|
import { revalidatePath } from 'next/cache'
|
||||||
import { headers } from 'next/headers'
|
|
||||||
import { createClient } from '@/lib/supabase/server'
|
import { createClient } from '@/lib/supabase/server'
|
||||||
import { createAdminClient } from '@/lib/supabase/admin'
|
import { createAdminClient } from '@/lib/supabase/admin'
|
||||||
import { linkSubmissionsByEmail } from './forms'
|
import { linkSubmissionsByEmail } from './forms'
|
||||||
@@ -240,12 +239,10 @@ export async function requestEmailChange(input: { email: string }) {
|
|||||||
const auth = await requireMember(supabase)
|
const auth = await requireMember(supabase)
|
||||||
if (!auth.ok) return { error: auth.error }
|
if (!auth.ok) return { error: auth.error }
|
||||||
|
|
||||||
const h = await headers()
|
// Origin from trusted server config ONLY — never request headers. A forged
|
||||||
const host = h.get('x-forwarded-host') ?? h.get('host')
|
// Host/X-Forwarded-Host could otherwise point the confirmation link at an
|
||||||
const proto = h.get('x-forwarded-proto') ?? 'https'
|
// attacker origin and capture the victim's token_hash.
|
||||||
const origin = host
|
const origin = process.env.NEXT_PUBLIC_APP_URL || 'https://hackerspace.sh'
|
||||||
? `${proto}://${host}`
|
|
||||||
: process.env.NEXT_PUBLIC_APP_URL || 'https://hackerspace.sh'
|
|
||||||
|
|
||||||
const { error } = await supabase.auth.updateUser(
|
const { error } = await supabase.auth.updateUser(
|
||||||
{ email: v.data.email },
|
{ email: v.data.email },
|
||||||
|
|||||||
+12
-4
@@ -72,10 +72,18 @@ export async function readSecret(
|
|||||||
// are never themselves secrets.
|
// are never themselves secrets.
|
||||||
export function isSecretConfigField(key: string): boolean {
|
export function isSecretConfigField(key: string): boolean {
|
||||||
if (key.endsWith('_set') || key.endsWith('_ref')) return false
|
if (key.endsWith('_set') || key.endsWith('_ref')) return false
|
||||||
|
const k = key.toLowerCase()
|
||||||
return (
|
return (
|
||||||
key === 'client_secret' ||
|
k === 'client_secret' ||
|
||||||
key === 'api_key' ||
|
k === 'api_key' ||
|
||||||
key === 'secret_key' ||
|
k === 'secret_key' ||
|
||||||
key.endsWith('_secret')
|
k === 'password' ||
|
||||||
|
k === 'token' ||
|
||||||
|
k === 'private_key' ||
|
||||||
|
k === 'access_token' ||
|
||||||
|
k === 'refresh_token' ||
|
||||||
|
k.endsWith('_secret') ||
|
||||||
|
k.endsWith('_token') ||
|
||||||
|
k.endsWith('_password')
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user