diff --git a/DECISIONS.md b/DECISIONS.md index 7c2cd06d..e0f66b8d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -662,3 +662,5 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-29] Retired the generic design skills now that emilkowalski/skills is installed globally (animation-vocabulary, apple-design, emil-design-eng, find-animation-opportunities, improve-animations, pick-ui-library, prototype, review-animations in ~/.claude/skills). Deleted .claude/skills/mobile-ux-core (52 lines of universal mobile UX whose file triggers are *.dart/*.swift/*Activity.kt, paths that do not exist in this repo; superseded by design.md's accessibility section plus apple-design) and .claude/skills/scout-design (a design scan that filed Linear tickets via mcp__claude_ai_Linear__save_issue, while this project files GitHub issues and loop-design-scan is the same scan with the right output; loop-design-scan's sibling reference updated). Kept web-design-guidelines: it is a Vercel-plugin symlink, cheap to keep, and may regenerate anyway. Also removed the global ui-ux-pro-max skill, a 67-style/96-palette catalogue that pulls against a locked editorial-monochrome system. [2026-07-29] Consent-expiry follow-up sent from invoiceservice@arcim.io, not a new sender: matching the address the original batch came from lets the two mails corroborate each other; RESEND_FROM_EMAIL alignment to accounted.se stays a separate ops task. [2026-07-29] Approval-queue MCP App widget (render_ui on list_pending_operations): high-risk confirmed=true now comes from a human click in-widget instead of agent-asserted; payload ceiling 58K->58.5K per the in-test trim-first convention. +[2026-07-29] OAuth error popup stays open instead of auto-closing: the postMessage is dropped on any popup/opener origin mismatch, and closing anyway made every such failure invisible (Fortnox silent-connect incident). +[2026-07-29] FORTNOX_REDIRECT_URI on prod deliberately left on app.gnubok.se for now: flipping it to app.accounted.se before that callback URL is registered in the Fortnox Developer Portal would break connect earlier, at the authorize step. diff --git a/extensions/general/arcim-migration/__tests__/oauth-callback-state.test.ts b/extensions/general/arcim-migration/__tests__/oauth-callback-state.test.ts index be8c8c5c..3eae9ec9 100644 --- a/extensions/general/arcim-migration/__tests__/oauth-callback-state.test.ts +++ b/extensions/general/arcim-migration/__tests__/oauth-callback-state.test.ts @@ -45,11 +45,22 @@ vi.mock('../lib/provider-client', () => ({ ConsentNotFoundError: class ConsentNotFoundError extends Error {}, })) +// The /connect handler unconditionally imports this module (for its +// pending-consent token check); the real one pulls in next/headers. +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(), + createServiceClient: vi.fn(), +})) + import { arcimMigrationExtension } from '../index' import { consumeOAuthState, exchangeAuthToken, getConsent, + createConsent, + listConsents, + generateOtc, + getAuthUrl, ConsentNotFoundError, } from '../lib/provider-client' @@ -84,6 +95,10 @@ describe('GET /callback: OAuth state binding', () => { beforeEach(() => { vi.clearAllMocks() vi.stubEnv('NEXT_PUBLIC_APP_URL', APP_URL) + // The exchange redirect_uri now consults the per-provider override; keep + // these tests on the NEXT_PUBLIC_APP_URL fallback regardless of local env. + vi.stubEnv('FORTNOX_REDIRECT_URI', '') + vi.stubEnv('VISMA_REDIRECT_URI', '') // The callback route is dispatched without an ExtensionContext (skipAuth // routes get no ctx), so console is the logger. Keep the output quiet. vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -238,6 +253,124 @@ describe('GET /callback: full-page fallback when there is no opener', () => { }) }) +/** + * The redirect_uri sent in the authorization request and the one sent in the + * token exchange must be byte-identical (RFC 6749 §4.1.3) or the provider + * rejects the code exchange. These broke apart once already: the authorize leg + * honored the FORTNOX_REDIRECT_URI override while the exchange hardcoded the + * NEXT_PUBLIC_APP_URL fallback, so when the app moved to app.accounted.se and + * the env var still pointed at app.gnubok.se, every Fortnox connect died at + * the exchange with no visible error (the error postMessage was then dropped + * by the opener's origin check). Both legs now resolve through + * resolveArcimCallbackUrl; these tests pin the symmetry. + */ +describe('OAuth redirect_uri symmetry between authorize and exchange', () => { + const OVERRIDE_URI = 'https://dev-tunnel.example.test/api/extensions/ext/arcim-migration/callback' + + const connectHandler = findRoute('POST', '/connect').handler as RouteHandler + + function connectCtx(): ExtensionContext { + const { supabase } = createMockSupabase() + ;(supabase as unknown as { auth: unknown }).auth = { + getUser: vi.fn().mockResolvedValue({ data: { user: { id: 'user-1' } } }), + } + return { supabase, companyId: 'company-1' } as unknown as ExtensionContext + } + + beforeEach(() => { + vi.clearAllMocks() + vi.stubEnv('NEXT_PUBLIC_APP_URL', APP_URL) + vi.stubEnv('FORTNOX_REDIRECT_URI', OVERRIDE_URI) + vi.spyOn(console, 'error').mockImplementation(() => {}) + ;(listConsents as Mock).mockResolvedValue([]) + ;(createConsent as Mock).mockResolvedValue({ id: 'consent-new' }) + ;(generateOtc as Mock).mockResolvedValue({ code: 'otc-code-1' }) + ;(getAuthUrl as Mock).mockResolvedValue({ url: 'https://apps.fortnox.se/oauth-v1/auth?x=1' }) + }) + + afterEach(() => { + vi.unstubAllEnvs() + vi.restoreAllMocks() + }) + + it('authorize leg passes the env-override redirect URI to getAuthUrl', async () => { + const res = await connectHandler( + createMockRequest('http://localhost/api/extensions/ext/arcim-migration/connect', { + method: 'POST', + body: { provider: 'fortnox' }, + }), + connectCtx(), + ) + + expect(res.status).toBe(200) + expect(getAuthUrl).toHaveBeenCalledWith('fortnox', 'otc-code-1', OVERRIDE_URI) + }) + + it('exchange leg passes the SAME env-override redirect URI to exchangeAuthToken', async () => { + ;(consumeOAuthState as Mock).mockResolvedValue({ + consentId: 'consent-new', + provider: 'fortnox', + }) + + await callbackHandler( + callbackRequest({ code: 'provider-auth-code', state: 'one-time-token' }), + ) + + expect(exchangeAuthToken).toHaveBeenCalledWith( + 'consent-new', + 'fortnox', + 'provider-auth-code', + OVERRIDE_URI, + ) + }) +}) + +/** + * The error page must stay open: its postMessage is dropped whenever the + * popup's origin differs from the opener's, and a window.close() right after + * turns that into "I approve in Fortnox and then nothing happens". The success + * page still closes itself. + */ +describe('GET /callback: error popup stays open, success popup closes', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.stubEnv('NEXT_PUBLIC_APP_URL', APP_URL) + vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.unstubAllEnvs() + vi.restoreAllMocks() + }) + + it('keeps the error popup open with the reason visible', async () => { + ;(consumeOAuthState as Mock).mockResolvedValue(null) + + const res = await callbackHandler( + callbackRequest({ code: 'provider-auth-code', state: 'bad-token' }), + ) + const html = await res.text() + + expect(html).toContain('Anslutningen misslyckades') + expect(html).not.toContain('window.close') + }) + + it('still closes the success popup', async () => { + ;(consumeOAuthState as Mock).mockResolvedValue({ + consentId: 'consent-1', + provider: 'fortnox', + }) + + const res = await callbackHandler( + callbackRequest({ code: 'provider-auth-code', state: 'one-time-token' }), + ) + const html = await res.text() + + expect(html).toContain('Anslutningen lyckades') + expect(html).toContain('window.close()') + }) +}) + describe('GET /preview: cross-tenant consent status oracle', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/extensions/general/arcim-migration/index.ts b/extensions/general/arcim-migration/index.ts index 2e40723c..88294e1f 100644 --- a/extensions/general/arcim-migration/index.ts +++ b/extensions/general/arcim-migration/index.ts @@ -66,6 +66,34 @@ function translateOAuthError(error: string, description: string | null): string return description ? `${error}: ${description}` : error } +/** + * Resolve the OAuth callback URL for a provider. Single source of truth for + * BOTH legs of the flow: the redirect_uri sent in the authorization request + * and the redirect_uri sent in the token exchange must be byte-identical, or + * the provider rejects the code exchange (RFC 6749 §4.1.3). The two legs used + * to resolve this independently: authorize honored the FORTNOX_REDIRECT_URI / + * VISMA_REDIRECT_URI override while the exchange hardcoded the + * NEXT_PUBLIC_APP_URL fallback, so any override differing from the fallback + * (dev ngrok URI, or an env var left on the old app domain after a domain + * cutover) silently broke every OAuth connect at the exchange step. + * + * The override exists so dev environments can route through a single + * registered URI instead of registering every ngrok URL on the OAuth client. + */ +function resolveArcimCallbackUrl(provider: ArcimProvider | ProviderName): string { + const providerRedirectEnv = + provider === 'visma' + ? process.env.VISMA_REDIRECT_URI + : provider === 'fortnox' + ? process.env.FORTNOX_REDIRECT_URI + : undefined + if (providerRedirectEnv && providerRedirectEnv.trim().length > 0) { + return providerRedirectEnv + } + const appUrl = process.env.NEXT_PUBLIC_APP_URL || '' + return `${appUrl}/api/extensions/ext/arcim-migration/callback` +} + /** * Build a provider OAuth authorization URL bound to an EXISTING consent id. * Used by both first-time connect and reconnect (token revival): the callback @@ -79,21 +107,7 @@ async function buildArcimOAuthUrl(consentId: string, provider: ArcimProvider): P // is that row's opaque random primary key, nothing more. const otc = await generateOtc(consentId) - // Prefer a provider-specific redirect override (e.g. VISMA_REDIRECT_URI) when - // set: lets dev environments route through a single registered URI rather - // than registering every ngrok URL on the OAuth client. Falls back to - // NEXT_PUBLIC_APP_URL + the canonical callback path. - const appUrl = process.env.NEXT_PUBLIC_APP_URL || '' - const providerRedirectEnv = - provider === 'visma' - ? process.env.VISMA_REDIRECT_URI - : provider === 'fortnox' - ? process.env.FORTNOX_REDIRECT_URI - : undefined - const callbackUrl = - providerRedirectEnv && providerRedirectEnv.trim().length > 0 - ? providerRedirectEnv - : `${appUrl}/api/extensions/ext/arcim-migration/callback` + const callbackUrl = resolveArcimCallbackUrl(provider) // The state is the one-time code itself: an unguessable pointer to the row // above. It deliberately encodes NOTHING. The previous base64url JSON payload @@ -459,14 +473,20 @@ export const arcimMigrationExtension: Extension = { .replace(//g, '>') + // The popup deliberately stays open on error: the postMessage is + // dropped whenever the popup's origin differs from the opener's + // (browser targetOrigin/origin checks), and closing anyway turns any + // such config drift into an invisible failure the user can only + // describe as "nothing happens". Leaving the reason on screen keeps + // every error diagnosable; the wizard also shows it when the message + // does arrive. const html = `

Anslutningen misslyckades: ${escapedReason}

` +

Anslutningen misslyckades: ${escapedReason}

Du kan stänga detta fönster.

` return new Response(html, { status: 200, @@ -518,7 +538,9 @@ export const arcimMigrationExtension: Extension = { const { consentId, provider } = resolvedState - const redirectUri = `${appUrl}/api/extensions/ext/arcim-migration/callback` + // Must match the redirect_uri the authorization request was built + // with, so both come from resolveArcimCallbackUrl. + const redirectUri = resolveArcimCallbackUrl(provider) // Exchange OAuth code directly with the provider await exchangeAuthToken(consentId, provider, code, redirectUri)