fix(arcim): resolve OAuth redirect_uri identically in authorize and exchange (#1287)

* fix(arcim): resolve OAuth redirect_uri identically in authorize and exchange The authorize leg honored the FORTNOX_REDIRECT_URI / VISMA_REDIRECT_URI override while the token-exchange leg hardcoded the NEXT_PUBLIC_APP_URL fallback. After the app-domain cutover (2026-07-21) the Fortnox env var still pointed at app.gnubok.se while NEXT_PUBLIC_APP_URL moved to app.accounted.se, so the two redirect_uri values differed and Fortnox rejected every code exchange (RFC 6749 4.1.3). The failure was invisible: the error popup posted its message from the old-domain origin, the wizard's event.origin check dropped it, and the popup closed itself. Both legs now resolve through one resolveArcimCallbackUrl() helper, and the error popup stays open with the reason on screen so a dropped postMessage can never again turn into "nothing happens". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: log OAuth popup and redirect-uri rollout decisions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-29 18:47:07 +02:00
parent ef25a87d75
commit 16f34fb214
3 changed files with 175 additions and 18 deletions
@@ -662,3 +662,5 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. 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.
@@ -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()
@@ -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, '&lt;')
            .replace(/>/g, '&gt;')

+          // 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 = `<!DOCTYPE html><html><body><script>
            if (window.opener) {
              window.opener.postMessage({ type: 'arcim-oauth-error', reason: ${jsLiteral(reason)} }, ${jsLiteral(appUrl)});
-              window.close();
            } else {
              window.location.href = ${jsLiteral(fallbackUrl.toString())};
            }
-          </script><p>Anslutningen misslyckades: ${escapedReason}</p></body></html>`
+          </script><p>Anslutningen misslyckades: ${escapedReason}</p><p>Du kan stänga detta fönster.</p></body></html>`

          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)