fix(auth): resolve BankID confirmation and email-hook link hosts through the trusted-origin registry (#2380)
* fix(auth): resolve BankID confirmation and email-hook link hosts through the trusted-origin registry The BankID confirmation mail built its /auth/callback link from the raw forwarded host and protocol; it is the one auth link GoTrue's redirect allowlist never sees, since the link is minted here and sent through Resend. The Send Email hook followed GoTrue's redirect_to verbatim: the webhook signature proves who sent the payload, not that every destination in it should be followed, and the GoTrue allowlist is a hand-configured glob. Both now resolve the destination through lib/domains/trusted-app-origin like every other auth link (canonical, this deployment's own Vercel hosts, or a registered brands.domain). Unknown, lookalike, credential-bearing, non-default-port and malformed destinations collapse to the canonical /auth/callback with no next path; a registered brand host over http is upgraded to https. Brand sender identity is taken from the RESOLVED host, so mail branding and link destination always agree. A brands-table read failure refuses instead of mailing a wrong-host link: the BankID helper returns step resolve_origin (signup rolls back, login re-send logs), the hook answers 500 so Supabase retries. Drops the proto parameter from the BankID helper; the resolver owns the scheme. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGB2YxptqVxBLJ2RkB5T * fix(auth): read the sender brand once, failure-aware, before minting or sending auth mail CodeRabbit: resolveTrustedAppOrigin could classify a brand host, then the separate resolveBrandByHost read for the sender could fail and return null, so a brand link went out with the platform sender; the BankID helper had already minted the magic link by then. Both sites now read the brand with resolveBrandResultByHost on the resolved host and refuse on a failed read for any non-canonical origin (BankID: step resolve_origin before generateLink; hook: 500 so Supabase retries). On the canonical origin a failed read is the platform sender either way, so mail still goes out. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGB2YxptqVxBLJ2RkB5T * fix(auth): treat a credential-bearing redirect_to as untrusted in the email hook Superagent P2: URL.origin drops userinfo, so a redirect_to with credentials on a served host passed the origin comparison and was cloned into the auth link with the credentials still in it. No flow of ours sends one; the hook now rejects any redirect_to carrying username or password outright and links to the canonical /auth/callback with no next path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGB2YxptqVxBLJ2RkB5T --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5.1
Claude Fable 5.1
parent
e0244b95a7
commit
cb962fae88
@@ -295,7 +295,6 @@ describe('POST /bankid/complete', () => {
|
||||
supabase: client,
|
||||
email: 'fresh@example.com',
|
||||
host: 'app.gnubok.se',
|
||||
proto: 'https',
|
||||
})
|
||||
expect(admin.deleteUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest'
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
|
||||
import type { SupabaseClient } from '@supabase/supabase-js'
|
||||
|
||||
const sendEmailMock = vi.hoisted(() => vi.fn())
|
||||
@@ -6,9 +6,12 @@ vi.mock('@/lib/email/service', () => ({
|
||||
getEmailService: () => ({ sendEmail: sendEmailMock, isConfigured: () => true }),
|
||||
}))
|
||||
|
||||
const resolveBrandByHostMock = vi.hoisted(() => vi.fn())
|
||||
const resolveBrandResultByHostMock = vi.hoisted(() => vi.fn())
|
||||
vi.mock('@/lib/branding/resolve', () => ({
|
||||
resolveBrandByHost: resolveBrandByHostMock,
|
||||
resolveBrandByHost: vi.fn(),
|
||||
// The one registry read: the trusted-origin resolver classifies the
|
||||
// request host through it, and the helper reads the sender brand from it.
|
||||
resolveBrandResultByHost: (...args: unknown[]) => resolveBrandResultByHostMock(...args),
|
||||
// Imported by lib/email/brand-sender (not called on this path).
|
||||
resolveBrandForCompany: vi.fn(),
|
||||
}))
|
||||
@@ -22,6 +25,18 @@ import {
|
||||
sendBankIdSignupConfirmation,
|
||||
} from '../lib/bankid-confirmation-mail'
|
||||
|
||||
const CANONICAL = 'https://app.gnubok.se'
|
||||
const BRAND_HOST = 'app.testbrand.example'
|
||||
const TESTBRAND = {
|
||||
appName: 'Testbrand',
|
||||
domain: BRAND_HOST,
|
||||
supportEmail: 'support@testbrand.example',
|
||||
authEmailFrom: 'noreply@post.testbrand.example',
|
||||
senderDomainStatus: 'verified',
|
||||
}
|
||||
|
||||
const ORIGINAL_APP_URL = process.env.NEXT_PUBLIC_APP_URL
|
||||
|
||||
function serviceClient(generateLinkResult: unknown) {
|
||||
const generateLink = vi.fn().mockResolvedValue(generateLinkResult)
|
||||
return {
|
||||
@@ -34,24 +49,25 @@ const LINK_OK = { data: { properties: { hashed_token: 'hashed-123' } }, error: n
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
resolveBrandByHostMock.mockResolvedValue(null)
|
||||
process.env.NEXT_PUBLIC_APP_URL = CANONICAL
|
||||
// Registry: only the test brand host is registered; everything else is
|
||||
// unknown. Both resolvers read the same table.
|
||||
resolveBrandResultByHostMock.mockImplementation(async (host: string) => ({
|
||||
brand: host === BRAND_HOST ? TESTBRAND : null,
|
||||
lookupFailed: false,
|
||||
}))
|
||||
sendEmailMock.mockResolvedValue({ success: true, messageId: 'm-1' })
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
if (ORIGINAL_APP_URL === undefined) delete process.env.NEXT_PUBLIC_APP_URL
|
||||
else process.env.NEXT_PUBLIC_APP_URL = ORIGINAL_APP_URL
|
||||
})
|
||||
|
||||
describe('buildConfirmationUrl', () => {
|
||||
it('lands on the originating host with the token_hash + magiclink verify pattern', () => {
|
||||
expect(buildConfirmationUrl('app.siffra.se', 'https', 'tok')).toBe(
|
||||
'https://app.siffra.se/auth/callback?token_hash=tok&type=magiclink',
|
||||
)
|
||||
})
|
||||
|
||||
it('defaults to https when the proxy did not forward a protocol', () => {
|
||||
expect(buildConfirmationUrl('app.siffra.se', null, 'tok')).toMatch(/^https:\/\/app\.siffra\.se\//)
|
||||
})
|
||||
|
||||
it('falls back to the canonical app URL without a host', () => {
|
||||
expect(buildConfirmationUrl('', undefined, 'tok')).toBe(
|
||||
'https://app.gnubok.se/auth/callback?token_hash=tok&type=magiclink',
|
||||
it('appends the token_hash + magiclink verify pattern to the resolved origin', () => {
|
||||
expect(buildConfirmationUrl('https://app.testbrand.example', 'tok')).toBe(
|
||||
'https://app.testbrand.example/auth/callback?token_hash=tok&type=magiclink',
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -64,7 +80,6 @@ describe('sendBankIdSignupConfirmation', () => {
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host: 'app.gnubok.se',
|
||||
proto: 'https',
|
||||
})
|
||||
|
||||
expect(result).toEqual({ ok: true })
|
||||
@@ -82,32 +97,103 @@ describe('sendBankIdSignupConfirmation', () => {
|
||||
expect(mail.fromAddress).toBeUndefined()
|
||||
})
|
||||
|
||||
it('sends in the brand of the requesting host', async () => {
|
||||
resolveBrandByHostMock.mockResolvedValue({
|
||||
appName: 'Siffra',
|
||||
domain: 'app.siffra.se',
|
||||
supportEmail: 'support@siffra.se',
|
||||
authEmailFrom: 'noreply@post.siffra.se',
|
||||
senderDomainStatus: 'verified',
|
||||
})
|
||||
it('links to and sends in the brand of a registered requesting host', async () => {
|
||||
const { supabase } = serviceClient(LINK_OK)
|
||||
|
||||
await sendBankIdSignupConfirmation({
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host: 'app.siffra.se',
|
||||
proto: 'https',
|
||||
host: BRAND_HOST,
|
||||
})
|
||||
|
||||
expect(resolveBrandByHostMock).toHaveBeenCalledWith('app.siffra.se')
|
||||
expect(resolveBrandResultByHostMock).toHaveBeenCalledWith(BRAND_HOST)
|
||||
const mail = sendEmailMock.mock.calls[0][0]
|
||||
expect(mail.fromName).toBe('Siffra')
|
||||
expect(mail.fromAddress).toBe('noreply@post.siffra.se')
|
||||
expect(mail.replyTo).toBe('support@siffra.se')
|
||||
expect(mail.text).toContain('https://app.siffra.se/auth/callback?token_hash=hashed-123')
|
||||
expect(mail.fromName).toBe('Testbrand')
|
||||
expect(mail.fromAddress).toBe('noreply@post.testbrand.example')
|
||||
expect(mail.replyTo).toBe('support@testbrand.example')
|
||||
expect(mail.text).toContain(
|
||||
'https://app.testbrand.example/auth/callback?token_hash=hashed-123',
|
||||
)
|
||||
expect(mail.html).not.toMatch(/accounted/i)
|
||||
})
|
||||
|
||||
it.each([
|
||||
['a spoofed unknown host', 'evil.example'],
|
||||
['a lookalike of a registered host', 'app.testbrand.example.evil.example'],
|
||||
['a registered host on a non-default port', 'app.testbrand.example:8443'],
|
||||
['a credential-bearing host', 'app.testbrand.example@evil.example'],
|
||||
['a missing host', ''],
|
||||
])('sends a canonical link for %s instead of following the header', async (_label, host) => {
|
||||
const { supabase } = serviceClient(LINK_OK)
|
||||
|
||||
const result = await sendBankIdSignupConfirmation({
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host,
|
||||
})
|
||||
|
||||
expect(result).toEqual({ ok: true })
|
||||
const mail = sendEmailMock.mock.calls[0][0]
|
||||
expect(mail.text).toContain(
|
||||
'https://app.gnubok.se/auth/callback?token_hash=hashed-123&type=magiclink',
|
||||
)
|
||||
expect(mail.text).not.toContain('evil.example')
|
||||
expect(mail.text).not.toContain(':8443')
|
||||
// Canonical link means canonical sender: brand and destination agree.
|
||||
expect(mail.fromName).toBeUndefined()
|
||||
expect(mail.fromAddress).toBeUndefined()
|
||||
})
|
||||
|
||||
it('refuses before minting a link when the brand registry cannot be read', async () => {
|
||||
resolveBrandResultByHostMock.mockResolvedValue({ brand: null, lookupFailed: true })
|
||||
const { supabase, generateLink } = serviceClient(LINK_OK)
|
||||
|
||||
const result = await sendBankIdSignupConfirmation({
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host: BRAND_HOST,
|
||||
})
|
||||
|
||||
expect(result).toMatchObject({ ok: false, step: 'resolve_origin' })
|
||||
expect(generateLink).not.toHaveBeenCalled()
|
||||
expect(sendEmailMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('refuses before minting a link when the brand read fails after the origin resolved', async () => {
|
||||
// First read (origin classification) succeeds, second (sender) fails:
|
||||
// never platform-branded mail carrying a brand link, and no token minted.
|
||||
resolveBrandResultByHostMock
|
||||
.mockResolvedValueOnce({ brand: TESTBRAND, lookupFailed: false })
|
||||
.mockResolvedValueOnce({ brand: null, lookupFailed: true })
|
||||
const { supabase, generateLink } = serviceClient(LINK_OK)
|
||||
|
||||
const result = await sendBankIdSignupConfirmation({
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host: BRAND_HOST,
|
||||
})
|
||||
|
||||
expect(result).toMatchObject({ ok: false, step: 'resolve_origin' })
|
||||
expect(generateLink).not.toHaveBeenCalled()
|
||||
expect(sendEmailMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still sends canonical mail when the brand read fails on the canonical origin', async () => {
|
||||
resolveBrandResultByHostMock.mockResolvedValue({ brand: null, lookupFailed: true })
|
||||
const { supabase } = serviceClient(LINK_OK)
|
||||
|
||||
const result = await sendBankIdSignupConfirmation({
|
||||
supabase,
|
||||
email: 'fresh@example.com',
|
||||
host: 'app.gnubok.se',
|
||||
})
|
||||
|
||||
expect(result).toEqual({ ok: true })
|
||||
const mail = sendEmailMock.mock.calls[0][0]
|
||||
expect(mail.text).toContain('https://app.gnubok.se/auth/callback?token_hash=hashed-123')
|
||||
expect(mail.fromName).toBeUndefined()
|
||||
})
|
||||
|
||||
it('reports a generateLink failure without sending anything', async () => {
|
||||
const { supabase } = serviceClient({ data: null, error: { message: 'link boom', code: 'x' } })
|
||||
|
||||
|
||||
Reference in New Issue
Block a user