From f7f40c04e4b15b1b35937f466117b139d8165440 Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:35:11 +0200 Subject: [PATCH] fix: return provider OAuth callbacks to the initiating brand domain (#2305) * fix: hand provider OAuth callbacks back to initiating brand * fix: encrypt provider OAuth handoff payloads * fix: purge expired provider OAuth handoffs --- DECISIONS.md | 3 + .../cleanup/cron/__tests__/route.test.ts | 72 +++++++ .../arcim-migration/cleanup/cron/route.ts | 18 ++ docker/crontab.hosted | 1 + docker/crontab.self-hosted | 1 + .../__tests__/handoff-crypto.test.ts | 43 ++++ .../oauth-callback-initiator.test.ts | 4 +- .../__tests__/oauth-callback-state.test.ts | 199 ++++++++++++++++++ .../provider-client-oauth-state.test.ts | 100 ++++++++- extensions/general/arcim-migration/index.ts | 99 ++++++--- .../arcim-migration/lib/handoff-crypto.ts | 27 +++ .../arcim-migration/lib/provider-client.ts | 88 +++++++- ...905094806_provider_oauth_brand_handoff.sql | 15 ++ vercel.json | 4 + 14 files changed, 642 insertions(+), 32 deletions(-) create mode 100644 app/api/extensions/arcim-migration/cleanup/cron/__tests__/route.test.ts create mode 100644 app/api/extensions/arcim-migration/cleanup/cron/route.ts create mode 100644 extensions/general/arcim-migration/__tests__/handoff-crypto.test.ts create mode 100644 extensions/general/arcim-migration/lib/handoff-crypto.ts create mode 100644 supabase/migrations/20260905094806_provider_oauth_brand_handoff.sql diff --git a/DECISIONS.md b/DECISIONS.md index 024d2fb6..ce6c6f75 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1592,3 +1592,6 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-09-04] Connector-hop failures (timeout, error envelope, wire-contract mismatch) are transient in every sync path: the row keeps its status and the user message says no renewal is needed, same as AspspUnavailableError (#2202), and the cron now treats AspspUnavailableError the same way instead of parking it in 'error'. Why: on 2026-09-04 the Connect service answered a shape the client rejects and the cron flipped four canary companies to 'error' with SYNC_FAILED_MESSAGE, so users re-authorized consents that were fine. The Zod issues are logged (field paths) because a bare 'unexpected shape' left the failure undiagnosable. Rejected: a new 'degraded' connection status (one more state every filter and the probe would have to learn; the health probe already catches a dead session on the same run) and removing the canary companies from the env (hides the contract bug instead of exposing its field paths). [2026-09-05] Fortnox VAT-inclusive invoices (VATIncluded: true) now map their rows net of VAT (lib/providers/fortnox/mapper.ts netOfVat, preferring TotalExcludingVAT / PriceExcludingVAT when the payload carries them), and the migrated-row completion pass refuses a row set whose net or VAT disagrees with the header the same payload established by more than 1 kr (rowsMismatch, reported, never stored). Why: the first production run of the completion pass (#2291) wrote 345 Profilio invoices whose rows summed to the gross with 25 % on top, beside a correct header; the mapper had always read row Total as net, and the pass's only cross-check was the invoice total, which the header satisfied. Rows that contradict their own header are worse than no rows: the invoice page shows both, and for an open invoice the booking engine sums the rows. Rejected: comparing against the stored header (it may itself be the pre-#1745 default) and a wider tolerance (öresavrundning is at most 0.50 kr; the real disagreements are kronor). [2026-09-05] Fortnox header-level Freight and AdministrationFee become synthetic rows in the sales mapper (lib/providers/fortnox/mapper.ts headerChargeLines), free-text rows (no quantity, no amount) land as line_type 'text' and no longer count as a stated 0 % rate in the migration's VAT resolver. Why: Fortnox keeps both charges outside InvoiceRows while Total and TotalVAT include them, so the rows summed to less than the header by exactly the charge, and after #2302 the rows-versus-header check refused those invoices (Profilio 14 of 384); the *VAT fields are amounts, not rates, and the charge is gross when VATIncluded, all verified on live payloads (invoices 295 and 242) rather than the spec, whose endpoint answered 429 all day. Text rows with VAT 0 beside 25 % rows had made roughly half of the Loftux and Clearstoq registers "mixed" with a null header rate. Rejected: dropping the charge into the first priced row (it is its own line on the customer's invoice, often on 3520) and trusting Net for the header (Net excludes the charges; gross minus TotalVAT is the net the rows must reach). +[2026-09-05] Provider OAuth returns to the initiating validated app/brand origin through a two-minute provider_otc handoff. State rows and handoffs have disjoint consume predicates, handoff consumption also binds the destination origin, and both success and provider denial require the original user on hop 2. The token exchange keeps the original configured provider redirect URI. Staging prerequisites 20260831111519, 20260902090000 and 20260902100000 were replayed from the existing SQL files before 20260905094806; MCP-assigned history timestamps were reconciled to those repository versions. Production and the live amnas Fortnox connect remain pending deployment and specific production-write approval. +[2026-09-05] PR #2305 review: encrypt both OAuth handoff payload columns with AES-256-GCM using a purpose-scoped derivation of the existing server-only service-role secret, matching other extension credential storage. Authenticate the handoff token, consent, initiating user, destination origin and column as additional data; reject plaintext or unreadable payloads after atomic consume. This resolves the at-rest encryption finding without new configuration, dependencies, or edits to the already-applied migration. +[2026-09-05] PR #2305 cleanup review: expire provider_otc rows through a service-role cron every five minutes, including abandoned states and encrypted handoffs whose consents remain. Use the existing cron-auth wrapper and generated hosted/self-hosted schedules; the migration is already applied on staging and remains unchanged. diff --git a/app/api/extensions/arcim-migration/cleanup/cron/__tests__/route.test.ts b/app/api/extensions/arcim-migration/cleanup/cron/__tests__/route.test.ts new file mode 100644 index 00000000..3dd83dc8 --- /dev/null +++ b/app/api/extensions/arcim-migration/cleanup/cron/__tests__/route.test.ts @@ -0,0 +1,72 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + from: vi.fn(), + delete: vi.fn(), + lte: vi.fn(), + createServiceClient: vi.fn(), +})) + +vi.mock('@/lib/supabase/server', () => ({ createServiceClient: mocks.createServiceClient })) +vi.mock('@/lib/observability', () => ({ captureException: vi.fn(), captureMessage: vi.fn() })) + +import { GET } from '../route' + +describe('provider OAuth expiry cleanup', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.stubEnv('CRON_SECRET', 'test-cron-secret') + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-09-05T12:00:00Z')) + mocks.createServiceClient.mockReturnValue({ from: mocks.from }) + mocks.from.mockReturnValue({ delete: mocks.delete }) + mocks.delete.mockReturnValue({ lte: mocks.lte }) + mocks.lte.mockResolvedValue({ count: 3, error: null }) + }) + + afterEach(() => { + vi.unstubAllEnvs() + vi.useRealTimers() + }) + + function request(authorization = 'Bearer test-cron-secret') { + return new Request('https://app.accounted.se/api/extensions/arcim-migration/cleanup/cron', { + headers: authorization ? { authorization } : {}, + }) + } + + it.each(['', 'Bearer wrong-secret'])('rejects unauthorized requests before database access (%s)', async (authorization) => { + const response = await GET(request(authorization)) + expect(response.status).toBe(401) + expect(mocks.createServiceClient).not.toHaveBeenCalled() + }) + + it('fails closed when the cron secret is not configured', async () => { + vi.stubEnv('CRON_SECRET', '') + expect((await GET(request())).status).toBe(401) + expect(mocks.createServiceClient).not.toHaveBeenCalled() + }) + + it('deletes only rows that have reached expiry and reports the count', async () => { + const response = await GET(request()) + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ data: { deleted: 3 } }) + expect(mocks.from).toHaveBeenCalledWith('provider_otc') + expect(mocks.delete).toHaveBeenCalledWith({ count: 'exact' }) + expect(mocks.lte).toHaveBeenCalledWith('expires_at', '2026-09-05T12:00:00.000Z') + }) + + it('succeeds when no rows have expired', async () => { + mocks.lte.mockResolvedValue({ count: 0, error: null }) + const response = await GET(request()) + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ data: { deleted: 0 } }) + }) + + it('reports database failures without exposing their details', async () => { + mocks.lte.mockResolvedValue({ count: null, error: new Error('private database detail') }) + const response = await GET(request()) + expect(response.status).toBe(500) + expect(await response.text()).not.toContain('private database detail') + }) +}) diff --git a/app/api/extensions/arcim-migration/cleanup/cron/route.ts b/app/api/extensions/arcim-migration/cleanup/cron/route.ts new file mode 100644 index 00000000..1853df85 --- /dev/null +++ b/app/api/extensions/arcim-migration/cleanup/cron/route.ts @@ -0,0 +1,18 @@ +import { NextResponse } from 'next/server' +import { withCronContext } from '@/lib/api/with-cron-context' +import { createServiceClient } from '@/lib/supabase/server' + +// Purge abandoned OAuth states and handoffs even when their consent is retained. +export const GET = withCronContext('cron.provider_oauth_cleanup', async (_request, ctx) => { + const supabase = createServiceClient() + const { count, error } = await supabase + .from('provider_otc') + .delete({ count: 'exact' }) + .lte('expires_at', new Date().toISOString()) + + if (error) throw error + + const deleted = count ?? 0 + ctx.log.info('provider OAuth cleanup summary', { deleted }) + return NextResponse.json({ data: { deleted } }) +}) diff --git a/docker/crontab.hosted b/docker/crontab.hosted index c80eeb7c..869cc897 100644 --- a/docker/crontab.hosted +++ b/docker/crontab.hosted @@ -48,6 +48,7 @@ */2 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/invoice-inbox/sweep/cron 45 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/invoice-inbox/underlag-reconcile/cron 20 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/arcim-migration/complete-invoice-lines/cron +*/5 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/arcim-migration/cleanup/cron 15 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/bookkeeping/accruals/post-due/cron 30 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/receipt-hunt/cron 45 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/notifications/bookkeeping-digest/cron diff --git a/docker/crontab.self-hosted b/docker/crontab.self-hosted index 3947813f..cf1156ad 100644 --- a/docker/crontab.self-hosted +++ b/docker/crontab.self-hosted @@ -48,6 +48,7 @@ */2 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/invoice-inbox/sweep/cron 45 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/invoice-inbox/underlag-reconcile/cron 20 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/arcim-migration/complete-invoice-lines/cron +*/5 * * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/extensions/arcim-migration/cleanup/cron 15 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/bookkeeping/accruals/post-due/cron 30 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/receipt-hunt/cron 45 5 * * * curl -sf -H "Authorization: Bearer ${CRON_SECRET}" ${APP_URL}/api/notifications/bookkeeping-digest/cron diff --git a/extensions/general/arcim-migration/__tests__/handoff-crypto.test.ts b/extensions/general/arcim-migration/__tests__/handoff-crypto.test.ts new file mode 100644 index 00000000..c69256aa --- /dev/null +++ b/extensions/general/arcim-migration/__tests__/handoff-crypto.test.ts @@ -0,0 +1,43 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { decryptHandoffValue, encryptHandoffValue } from '../lib/handoff-crypto' + +describe('handoff encryption', () => { + const context = JSON.stringify(['token', 'consent', 'user', 'https://brand.example', 'provider_code']) + + beforeEach(() => vi.stubEnv('SUPABASE_SERVICE_ROLE_KEY', 'test-only-server-secret')) + afterEach(() => vi.unstubAllEnvs()) + + it('encrypts identical values differently and preserves Unicode error text', () => { + const plaintext = 'Återanrop: känsligt fel ', + }) + const html = await (await callbackHandler(request(BRAND_ORIGIN, { handoff: 'fresh-handoff' }))).text() + expect(html).not.toContain('') + expect(html).toContain('</script>') + }) + + it('rejects a replayed first hop before minting a second handoff', async () => { + vi.mocked(consumeOAuthState).mockResolvedValueOnce(state).mockResolvedValueOnce(null) + expect((await callbackHandler(request(APP_URL, { state: 'state', code: 'code' }))).status).toBe(302) + const replay = await callbackHandler(request(APP_URL, { state: 'state', code: 'code' })) + expect(await replay.text()).toContain(GENERIC_REJECTION) + expect(mintHandoff).toHaveBeenCalledOnce() + }) + + it('rejects replayed handoffs after exactly one exchange', async () => { + vi.mocked(consumeHandoff).mockResolvedValueOnce(storedHandoff).mockResolvedValueOnce(null) + await callbackHandler(request(BRAND_ORIGIN, { handoff: 'fresh-handoff' })) + const replay = await callbackHandler(request(BRAND_ORIGIN, { handoff: 'fresh-handoff' })) + expect(await replay.text()).toContain(GENERIC_REJECTION) + expect(exchangeAuthToken).toHaveBeenCalledOnce() + }) + + it.each(['expired', 'unknown', 'wrong-origin'])('rejects %s handoffs without exchanging', async (token) => { + vi.mocked(consumeHandoff).mockResolvedValue(null) + const response = await callbackHandler(request(BRAND_ORIGIN, { handoff: token })) + expect(await response.text()).toContain(GENERIC_REJECTION) + expect(exchangeAuthToken).not.toHaveBeenCalled() + expect(requireFlowInitiator).not.toHaveBeenCalled() + }) + + it.each(['no_session', 'mismatch'] as const)('refuses hop 2 with %s, including provider-error handoffs', async (reason) => { + vi.mocked(requireFlowInitiator).mockResolvedValue({ ok: false, reason, response: new Response(null, { status: 403 }), sessionUserId: 'other-user' }) + for (const providerError of [null, 'provider rejected']) { + vi.mocked(consumeHandoff).mockResolvedValue({ ...storedHandoff, providerError }) + const html = await (await callbackHandler(request(BRAND_ORIGIN, { handoff: 'token' }))).text() + expect(html).toContain('arcim-oauth-error') + expect(html).not.toContain('consent-1') + expect(html).not.toContain('provider rejected') + } + expect(exchangeAuthToken).not.toHaveBeenCalled() + }) + + it('refuses a legacy row with no initiator before minting a handoff', async () => { + vi.mocked(consumeOAuthState).mockResolvedValue({ ...state, userId: null }) + const response = await callbackHandler(request(APP_URL, { state: 'state', code: 'code' })) + expect(await response.text()).toContain(GENERIC_REJECTION) + expect(mintHandoff).not.toHaveBeenCalled() + }) + + it.each([ + [APP_URL, false, APP_URL], + [BRAND_ORIGIN, true, BRAND_ORIGIN], + ['https://unknown.accounted.se', false, APP_URL], + ['http://solbo.accounted.se', true, APP_URL], + ['https://solbo.accounted.se:444', true, APP_URL], + ])('connect from %s stores only an allowed origin', async (origin, knownBrand, expected) => { + vi.mocked(resolveBrandByHost).mockResolvedValue(knownBrand ? { domain: 'solbo.accounted.se' } as Awaited> : null) + vi.mocked(listConsents).mockResolvedValue([]) + vi.mocked(createConsent).mockResolvedValue({ id: 'consent-new' } as Awaited>) + vi.mocked(generateOtc).mockResolvedValue({ code: 'state', consentId: 'consent-new', expiresAt: '' }) + vi.mocked(getAuthUrl).mockResolvedValue({ url: 'https://provider.test/login' }) + const { supabase } = createMockSupabase() + ;(supabase as unknown as { auth: unknown }).auth = { + getUser: vi.fn().mockResolvedValue({ data: { user: { id: 'user-1' } } }), + } + const connect = findRoute('POST', '/connect').handler as RouteHandler + const response = await connect(createMockRequest(`${origin}/api/extensions/ext/arcim-migration/connect`, { + method: 'POST', body: { provider: 'fortnox', origin: 'https://attacker.test' }, + }), { supabase, companyId: 'company-1' } as unknown as ExtensionContext) + expect(response.status).toBe(200) + expect(generateOtc).toHaveBeenCalledWith('consent-new', 'user-1', expected) + }) + + it.each([ + ['solbo.accounted.se', BRAND_ORIGIN], + ['unknown.accounted.se', APP_URL], + ['invalid host', APP_URL], + ])('reconnect records the validated request Host %s', async (host, expected) => { + vi.mocked(resolveBrandByHost).mockImplementation(async (value) => + value === 'solbo.accounted.se' ? { domain: value } as Awaited> : null) + vi.mocked(listConsents).mockResolvedValue([{ id: 'consent-1', provider: 'fortnox', status: 1 }] as Awaited>) + vi.mocked(generateOtc).mockResolvedValue({ code: 'state', consentId: 'consent-1', expiresAt: '' }) + vi.mocked(getAuthUrl).mockResolvedValue({ url: 'https://provider.test/login' }) + const ctx = { + companyId: 'company-1', + supabase: { auth: { getUser: vi.fn().mockResolvedValue({ data: { user: { id: 'user-1' } } }) } }, + } as unknown as ExtensionContext + const connect = findRoute('POST', '/connect').handler as RouteHandler + const response = await connect(createMockRequest(`${APP_URL}/api/extensions/ext/arcim-migration/connect`, { + method: 'POST', headers: { host }, body: { provider: 'fortnox', reconnect: true }, + }), ctx) + expect(response.status).toBe(200) + expect(generateOtc).toHaveBeenCalledWith('consent-1', 'user-1', expected) + }) +}) + describe('GET /callback: OAuth state binding', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/extensions/general/arcim-migration/__tests__/provider-client-oauth-state.test.ts b/extensions/general/arcim-migration/__tests__/provider-client-oauth-state.test.ts index cfcb89bb..c2b1c2ea 100644 --- a/extensions/general/arcim-migration/__tests__/provider-client-oauth-state.test.ts +++ b/extensions/general/arcim-migration/__tests__/provider-client-oauth-state.test.ts @@ -1,4 +1,5 @@ -import { describe, it, expect, beforeEach, vi } from 'vitest' +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { encryptHandoffValue } from '../lib/handoff-crypto' /** * Unit-level guard on the two tenant boundaries in provider-client: @@ -63,6 +64,8 @@ vi.mock('@/lib/supabase/server', () => ({ import { consumeOAuthState, + mintHandoff, + consumeHandoff, generateOtc, getConsent, ConsentNotFoundError, @@ -97,7 +100,9 @@ describe('consumeOAuthState', () => { expect(findOp(otcCall, 'is', 'used_at')?.[1]).toEqual(['used_at', null]) // Expiry is enforced in the same statement, not in JavaScript afterwards. expect(findOp(otcCall, 'gt', 'expires_at')).toBeDefined() - expect(findOp(otcCall, 'select', 'consent_id, user_id')).toBeDefined() + expect(findOp(otcCall, 'select', 'consent_id, user_id, origin')).toBeDefined() + expect(findOp(otcCall, 'is', 'provider_code')?.[1]).toEqual(['provider_code', null]) + expect(findOp(otcCall, 'is', 'provider_error')?.[1]).toEqual(['provider_error', null]) }) it('returns the consent and the provider read from the server-side rows', async () => { @@ -107,6 +112,7 @@ describe('consumeOAuthState', () => { consentId: 'consent-1', provider: 'visma', userId: 'user-1', + origin: null, }) }) @@ -142,6 +148,7 @@ describe('consumeOAuthState', () => { consentId: 'consent-1', provider: 'fortnox', userId: 'user-1', + origin: null, }) // Replay: used_at is now set, so `is('used_at', null)` matches nothing. @@ -158,6 +165,7 @@ describe('consumeOAuthState', () => { consentId: 'consent-1', provider: 'fortnox', userId: null, + origin: null, }) }) @@ -186,7 +194,7 @@ describe('generateOtc', () => { const { calls } = useResults([{ data: null }]) const before = Date.now() - const { expiresAt } = await generateOtc('consent-1', 'user-1') + const { expiresAt } = await generateOtc('consent-1', 'user-1', 'https://solbo.accounted.se') const after = Date.now() expect(calls[0].table).toBe('provider_otc') @@ -195,6 +203,7 @@ describe('generateOtc', () => { // The initiator travels with the row: the callback binds the completing // session to it, so it must be written here and nowhere else. expect(inserted.user_id).toBe('user-1') + expect(inserted).toHaveProperty('origin', 'https://solbo.accounted.se') const tenMinutes = 10 * 60 * 1000 const expiry = new Date(expiresAt).getTime() @@ -204,6 +213,91 @@ describe('generateOtc', () => { }) }) +describe('OAuth handoff storage', () => { + const origin = 'https://solbo.accounted.se' + + beforeEach(() => { + vi.stubEnv('SUPABASE_SERVICE_ROLE_KEY', 'test-only-handoff-encryption-key') + }) + + afterEach(() => vi.unstubAllEnvs()) + + it.each([{ providerCode: 'auth-code' }, { providerError: 'access denied' }])( + 'mints a fresh two-minute handoff holding %j', async (result) => { + const { calls } = useResults([{}, {}]) + const before = Date.now() + const first = await mintHandoff('consent-1', 'user-1', origin, result) + const second = await mintHandoff('consent-1', 'user-1', origin, result) + expect(first.code).toMatch(/^[A-Za-z0-9_-]{43}$/) + expect(first.code).not.toBe(second.code) + expect(Date.parse(first.expiresAt)).toBeGreaterThanOrEqual(before + 120_000) + expect(Date.parse(first.expiresAt)).toBeLessThanOrEqual(Date.now() + 120_000) + const inserted = findOp(calls[0], 'insert')?.[1][0] as Record + expect(inserted).toEqual({ + code: first.code, consent_id: 'consent-1', user_id: 'user-1', origin, + expires_at: first.expiresAt, + provider_code: result.providerCode ? expect.stringMatching(/^v1:/) : null, + provider_error: result.providerError ? expect.stringMatching(/^v1:/) : null, + }) + expect(JSON.stringify(inserted)).not.toContain(result.providerCode ?? result.providerError) + useResults([{ data: inserted }, { data: { provider: 'fortnox' } }]) + await expect(consumeHandoff(first.code, origin)).resolves.toMatchObject({ + providerCode: result.providerCode ?? null, providerError: result.providerError ?? null, + }) + }, + ) + + it('deletes atomically, binding origin, expiry, unused status and handoff purpose', async () => { + const { calls } = useResults([ + { data: { + consent_id: 'consent-1', user_id: 'user-1', origin, provider_error: null, + provider_code: encryptHandoffValue('stored-code', JSON.stringify(['handoff-token', 'consent-1', 'user-1', origin, 'provider_code'])), + } }, + { data: { provider: 'visma' } }, + ]) + await expect(consumeHandoff('handoff-token', origin)).resolves.toEqual({ + consentId: 'consent-1', userId: 'user-1', origin, provider: 'visma', + providerCode: 'stored-code', providerError: null, + }) + expect(calls[0].ops[0][0]).toBe('delete') + expect(findOp(calls[0], 'eq', 'code')?.[1]).toEqual(['code', 'handoff-token']) + expect(findOp(calls[0], 'eq', 'origin')?.[1]).toEqual(['origin', origin]) + expect(findOp(calls[0], 'is', 'used_at')?.[1]).toEqual(['used_at', null]) + expect(findOp(calls[0], 'gt', 'expires_at')).toBeDefined() + expect(findOp(calls[0], 'or')?.[1]).toEqual(['provider_code.not.is.null,provider_error.not.is.null']) + expect(calls[1].table).toBe('provider_consents') + expect(findOp(calls[1], 'eq', 'id')?.[1]).toEqual(['id', 'consent-1']) + }) + + it('rejects unmatched or already consumed handoffs without reading a consent', async () => { + const { calls } = useResults([{ data: null }]) + await expect(consumeHandoff('spent-token', origin)).resolves.toBeNull() + expect(calls).toHaveLength(1) + }) + + it('fails closed when the delete errors or the consent no longer exists', async () => { + useResults([{ error: { message: 'unavailable' } }]) + await expect(consumeHandoff('token', origin)).resolves.toBeNull() + useResults([{ data: { consent_id: 'deleted-consent' } }, { data: null }]) + await expect(consumeHandoff('token', origin)).resolves.toBeNull() + }) + + it('fails the flow if the handoff cannot be stored', async () => { + useResults([{ error: { message: 'unavailable' } }]) + await expect(mintHandoff('consent-1', 'user-1', origin, { providerCode: 'code' })) + .rejects.toThrow('Failed to mint OAuth handoff') + }) + + it('rejects unencrypted or corrupted handoff credentials after deleting the row', async () => { + const { calls } = useResults([ + { data: { consent_id: 'consent-1', user_id: 'user-1', origin, provider_code: 'plaintext-code', provider_error: null } }, + { data: { provider: 'fortnox' } }, + ]) + await expect(consumeHandoff('token', origin)).resolves.toBeNull() + expect(calls[0].ops[0][0]).toBe('delete') + }) +}) + describe('getConsent', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/extensions/general/arcim-migration/index.ts b/extensions/general/arcim-migration/index.ts index 1eab0b1d..87747937 100644 --- a/extensions/general/arcim-migration/index.ts +++ b/extensions/general/arcim-migration/index.ts @@ -6,6 +6,8 @@ import { listConsents, generateOtc, consumeOAuthState, + mintHandoff, + consumeHandoff, getAuthUrl, exchangeAuthToken, submitProviderToken, @@ -54,6 +56,7 @@ import { classifyProviderError } from '@/lib/providers/with-provider-call' import { getProviderResourceForbiddenMessage } from '@/lib/errors/get-error-message' import { FortnoxApiError, fortnoxErrorMessage } from '@/lib/providers/fortnox/client' import { createLogger } from '@/lib/logger' +import { resolveBrandByHost } from '@/lib/branding/resolve' const moduleLog = createLogger('extensions/arcim-migration') @@ -137,6 +140,32 @@ function resolveArcimCallbackUrl(provider: ArcimProvider | ProviderName): string return `${appUrl}/api/extensions/ext/arcim-migration/callback` } +function requestOrigin(request: Request): string { + const url = new URL(request.url) + const host = request.headers.get('host') ?? url.host + try { + const candidate = new URL(`${url.protocol}//${host}`) + if (candidate.host !== host.toLowerCase() || candidate.pathname !== '/' || candidate.search || candidate.hash) { + return url.origin + } + return candidate.origin + } catch { + return url.origin + } +} + +async function resolveOAuthOrigin(request: Request): Promise { + const origin = requestOrigin(request) + const appOrigin = new URL(process.env.NEXT_PUBLIC_APP_URL || request.url).origin + if (origin === appOrigin) return appOrigin + const url = new URL(origin) + // Brand domains are HTTPS origins. A hostname lookup must not authorize an + // arbitrary port or a downgrade to HTTP on the same host. + if (url.protocol !== 'https:' || url.port) return appOrigin + const brand = await resolveBrandByHost(url.host) + return brand && new URL(`https://${brand.domain}`).origin === origin ? origin : appOrigin +} + /** * Build a provider OAuth authorization URL bound to an EXISTING consent id. * Used by both first-time connect and reconnect (token revival): the callback @@ -148,13 +177,14 @@ async function buildArcimOAuthUrl( consentId: string, provider: ArcimProvider, initiatedByUserId: string, + origin: string, options?: { documentScopes?: boolean }, ): Promise { // Server-side state row: consent id, provider (via the consent), the user who // started the flow, expiry and a consumed marker all live in provider_otc. // The `state` handed to the provider is that row's opaque random primary // key, nothing more. - const otc = await generateOtc(consentId, initiatedByUserId) + const otc = await generateOtc(consentId, initiatedByUserId, origin) const callbackUrl = resolveArcimCallbackUrl(provider) @@ -369,7 +399,7 @@ export const arcimMigrationExtension: Extension = { await ctx.settings.set('provider', provider) } if (providerInfo.authType === 'oauth') { - const authUrl = await buildArcimOAuthUrl(stale.id, provider, user.id, { + const authUrl = await buildArcimOAuthUrl(stale.id, provider, user.id, await resolveOAuthOrigin(request), { documentScopes: documentScopes === true, }) return NextResponse.json({ @@ -454,7 +484,7 @@ export const arcimMigrationExtension: Extension = { } if (providerInfo.authType === 'oauth') { - const authUrl = await buildArcimOAuthUrl(consent.id, provider, user.id) + const authUrl = await buildArcimOAuthUrl(consent.id, provider, user.id, await resolveOAuthOrigin(request)) return NextResponse.json({ consentId: consent.id, @@ -581,11 +611,13 @@ export const arcimMigrationExtension: Extension = { handler: async (request: Request, ctx?: ExtensionContext) => { const log = ctx?.log ?? console const url = new URL(request.url) - const code = url.searchParams.get('code') + let code = url.searchParams.get('code') + const handoff = url.searchParams.get('handoff') const stateRaw = url.searchParams.get('state') const oauthError = url.searchParams.get('error') const oauthErrorDescription = url.searchParams.get('error_description') - const appUrl = process.env.NEXT_PUBLIC_APP_URL || '' + const currentOrigin = requestOrigin(request) + let responseOrigin = new URL(process.env.NEXT_PUBLIC_APP_URL || request.url).origin // JSON-encode for safe embedding inside