Skip to content

fix(auth): enforce OAuth policy after 2FA #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BunsDev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(auth): enforce OAuth policy after 2FA #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions apps/web/src/lib/server/auth/__tests__/hooks-callback-cleanup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ const mockPrincipalDeleteWhere = vi.fn(async () => undefined)
const mockDeleteSessionCookie = vi.fn()
const mockGetPublicPortalConfig = vi.fn()
const mockHasPlatformCredentials = vi.fn()
const mockGetSignedCookie = vi.fn()
const mockSetSignedCookie = vi.fn()
const mockSetCookie = vi.fn()

vi.mock('@/lib/server/db', () => ({
db: {
Expand Down Expand Up @@ -103,6 +106,7 @@ function ctxFor(opts: {
userId?: string
email?: string
token?: string
secret?: string
}) {
return {
path: opts.path,
Expand All @@ -113,7 +117,11 @@ function ctxFor(opts: {
user: opts.userId ? { id: opts.userId, email: opts.email } : undefined,
session: opts.token ? { token: opts.token } : undefined,
},
secret: opts.secret,
},
getSignedCookie: mockGetSignedCookie,
setSignedCookie: mockSetSignedCookie,
setCookie: mockSetCookie,
redirect: vi.fn((url: string) => new Error(`REDIRECT:${url}`)),
}
}
Expand All @@ -127,6 +135,8 @@ beforeEach(() => {
})
mockHasPlatformCredentials.mockResolvedValue(true)
mockIsSsoActuallyRegistered.mockImplementation(async (sso) => sso?.enabled === true)
mockGetSignedCookie.mockResolvedValue(undefined)
mockSetSignedCookie.mockResolvedValue(undefined)
})

// ============================================================
Expand Down Expand Up @@ -189,6 +199,67 @@ describe('handleCallbackPolicyCleanup — guards', () => {
})
})

describe('handleCallbackPolicyCleanup — two-factor deferred OAuth policy', () => {
it('remembers the first-factor provider when an OAuth callback defers final session issuance to 2FA', async () => {
mockPrincipalFindFirst.mockResolvedValue({ role: 'admin' })
const ctx = ctxFor({
path: '/oauth2/callback/:providerId',
providerParam: 'google',
userId: 'user_1',
email: 'a@external.com',
secret: 'test-secret',
})

await handleCallbackPolicyCleanup(ctx, tenantSettings({ googleEnabled: true }))

expect(mockSetSignedCookie).toHaveBeenCalledWith(
'quackback.2fa_policy_provider',
'user_1:google',
'test-secret',
expect.objectContaining({ httpOnly: true, maxAge: 600 })
)
})

it('applies the remembered OAuth provider policy on /two-factor/verify-totp session creation', async () => {
mockPrincipalFindFirst.mockResolvedValue({ role: 'admin' })
mockUserFindFirst.mockResolvedValue({ createdAt: new Date(Date.now() - 60 * 60_000) })
mockGetSignedCookie.mockResolvedValue('user_1:google')
const ctx = ctxFor({
path: '/two-factor/verify-totp',
userId: 'user_1',
email: 'a@external.com',
token: 'tok',
secret: 'test-secret',
})

await expect(
handleCallbackPolicyCleanup(ctx, tenantSettings({ googleEnabled: false }))
).rejects.toThrow(/\/admin\/login\?error=oauth_method_not_allowed/)

expect(mockSetCookie).toHaveBeenCalledWith('quackback.2fa_policy_provider', '', {
path: '/',
maxAge: 0,
})
expect(mockSessionDeleteWhere).toHaveBeenCalled()
expect(mockDeleteSessionCookie).toHaveBeenCalled()
})

it('skips two-factor verification without a remembered first-factor provider', async () => {
const ctx = ctxFor({
path: '/two-factor/verify-totp',
userId: 'user_1',
email: 'a@external.com',
token: 'tok',
secret: 'test-secret',
})

await handleCallbackPolicyCleanup(ctx, tenantSettings({ googleEnabled: false }))

expect(mockSessionDeleteWhere).not.toHaveBeenCalled()
expect(ctx.redirect).not.toHaveBeenCalled()
})
})

// ============================================================
// SSO callback — always allowed for team
// ============================================================
Expand Down
101 changes: 94 additions & 7 deletions apps/web/src/lib/server/auth/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ export type AuthProviderId =
| 'sso' // genericOAuth provider id 'sso'
| string // social ('google'|'github'|...) or other generic OAuth

const AUTH_PATH_PREFIX = '@opencoven-feedback'

function normalizeAuthPath(path: string | undefined): string | undefined {
if (!path) return undefined
return path.startsWith(`${AUTH_PATH_PREFIX}/`) ? path.slice(AUTH_PATH_PREFIX.length) : path
}

/**
* Map a Better-Auth `ctx.path` template to the conceptual provider id
* the policy table operates on. Returns `null` for paths that aren't
Expand All @@ -68,7 +75,7 @@ export function inferProvider(ctx: {
params?: Record<string, unknown>
body?: Record<string, unknown>
}): AuthProviderId | null {
const p = ctx.path
const p = normalizeAuthPath(ctx.path)
if (!p) return null
switch (p) {
case '/sign-in/email':
Expand Down Expand Up @@ -115,12 +122,79 @@ export function inferProvider(ctx: {
* session synchronously without going through `/callback/:id` — Layer
* B can't see the email pre-session, so Layer C is the only gate.
*/
export const SESSION_CREATING_CALLBACK_PATHS = new Set<string>([
const OAUTH_CALLBACK_POLICY_PATHS = new Set<string>([
'/callback/:id',
'/oauth2/callback/:providerId',
'/sign-in/social',
])

const TWO_FACTOR_SESSION_COMPLETION_PATHS = new Set<string>([
'/two-factor/verify',
'/two-factor/verify-totp',
'/two-factor/verify-otp',
'/two-factor/verify-backup-code',
])

export const SESSION_CREATING_CALLBACK_PATHS = new Set<string>([
...OAUTH_CALLBACK_POLICY_PATHS,
...TWO_FACTOR_SESSION_COMPLETION_PATHS,
])

const TWO_FACTOR_POLICY_PROVIDER_COOKIE = 'quackback.2fa_policy_provider'
const TWO_FACTOR_POLICY_COOKIE_MAX_AGE = 10 * 60

type TwoFactorPolicyCookieCtx = {
context?: { secret?: string }
getSignedCookie?: (
name: string,
secret: string
) => string | undefined | Promise<string | undefined>
setSignedCookie?: (
name: string,
value: string,
secret: string,
attrs?: Record<string, unknown>
) => unknown | Promise<unknown>
setCookie?: (name: string, value: string, opts?: Record<string, unknown>) => unknown
}

async function rememberTwoFactorPolicyProvider(
ctx: TwoFactorPolicyCookieCtx,
userId: string,
provider: AuthProviderId
): Promise<void> {
if (!ctx.setSignedCookie || typeof ctx.context?.secret !== 'string') return
await ctx.setSignedCookie(
TWO_FACTOR_POLICY_PROVIDER_COOKIE,
`${userId}:${provider}`,
ctx.context.secret,
{
httpOnly: true,
sameSite: 'lax',
secure: process.env.NODE_ENV === 'production',
path: '/',
maxAge: TWO_FACTOR_POLICY_COOKIE_MAX_AGE,
}
)
}

async function consumeTwoFactorPolicyProvider(
ctx: TwoFactorPolicyCookieCtx,
userId: string
): Promise<AuthProviderId | null> {
if (!ctx.getSignedCookie || typeof ctx.context?.secret !== 'string') return null
const value = await ctx.getSignedCookie(TWO_FACTOR_POLICY_PROVIDER_COOKIE, ctx.context.secret)
if (!value) return null
ctx.setCookie?.(TWO_FACTOR_POLICY_PROVIDER_COOKIE, '', { path: '/', maxAge: 0 })

const separator = value.indexOf(':')
if (separator <= 0) return null
const cookieUserId = value.slice(0, separator)
const provider = value.slice(separator + 1)
if (cookieUserId !== userId || !provider) return null
return provider
}

/**
* Paths where the email isn't in `ctx.body` — Layer B can't gate them
* because there's no caller identity yet. Layer A (registration filter)
Expand Down Expand Up @@ -522,13 +596,26 @@ export async function handleCallbackPolicyCleanup(
ReturnType<typeof import('@/lib/server/domains/settings/settings.service').getTenantSettings>
>
): Promise<void> {
if (!SESSION_CREATING_CALLBACK_PATHS.has(ctx.path ?? '')) return
const path = normalizeAuthPath(ctx.path)
if (!SESSION_CREATING_CALLBACK_PATHS.has(path ?? '')) return
const userId = ctx.context?.newSession?.user?.id
const userEmail = ctx.context?.newSession?.user?.email
const token = ctx.context?.newSession?.session?.token
if (typeof userId !== 'string' || typeof token !== 'string') return
if (typeof userId !== 'string') return

const provider = inferProvider(ctx as Parameters<typeof inferProvider>[0])
let provider = inferProvider(ctx as Parameters<typeof inferProvider>[0])
if (provider && path && OAUTH_CALLBACK_POLICY_PATHS.has(path) && typeof token !== 'string') {
// Better-Auth's 2FA plugin can turn a successful first-factor
// callback into a pending challenge by deleting the provisional
// session and issuing the final session later from `/two-factor/*`.
// Keep the first-factor provider in a signed, short-lived cookie
// so the completion endpoint is evaluated against the same policy.
await rememberTwoFactorPolicyProvider(ctx as TwoFactorPolicyCookieCtx, userId, provider)
}

if (!provider && path && TWO_FACTOR_SESSION_COMPLETION_PATHS.has(path)) {
provider = await consumeTwoFactorPolicyProvider(ctx as TwoFactorPolicyCookieCtx, userId)
}
if (!provider) return

const {
Expand Down Expand Up @@ -592,7 +679,7 @@ export async function handleCallbackPolicyCleanup(
typeof userEmail === 'string' &&
isHardBound(provider, userEmail, role, tenant?.authConfig, verifiedDomains, ssoRegistered)
) {
await revokeSession(ctx as SessionCtx, token)
if (typeof token === 'string') await revokeSession(ctx as SessionCtx, token)
await wipeBrandNewShellsIfFresh()
throw blockedRedirect('verified_domain_requires_sso')
}
Expand All @@ -602,7 +689,7 @@ export async function handleCallbackPolicyCleanup(
const result = await isAuthMethodAllowed(provider, role, tenant)
if (result.allowed) return

await revokeSession(ctx as SessionCtx, token)
if (typeof token === 'string') await revokeSession(ctx as SessionCtx, token)
await wipeBrandNewShellsIfFresh()
throw blockedRedirect(result.error ?? 'auth_method_blocked')
}
Expand Down