From fd376eff94c0e40ff215b433b3967ccaaa4e0706 Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:08:37 +0200 Subject: [PATCH] fix: audit Cloud Backup OAuth redirects (#1324) * fix(cloud-backup): pin OAuth callback origin * fix: reject non-web cloud backup origins --- DECISIONS.md | 2 + .../__tests__/provider-routes.test.ts | 63 +++++++++++++++++++ extensions/general/cloud-backup/index.ts | 18 +++--- .../lib/__tests__/callback-origin.test.ts | 63 +++++++++++++++++++ .../cloud-backup/lib/callback-origin.ts | 29 +++++++++ 5 files changed, 168 insertions(+), 7 deletions(-) create mode 100644 extensions/general/cloud-backup/lib/__tests__/callback-origin.test.ts create mode 100644 extensions/general/cloud-backup/lib/callback-origin.ts diff --git a/DECISIONS.md b/DECISIONS.md index 970ec6b5..a248b289 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -725,3 +725,5 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-31] Seeded chart names corrected to BAS 2026 where the label contradicted what the engine books on the account (7210 'Semesterlöner' -> 'Löner till tjänstemän', 7010, 3001/3002), with an exact-literal + is_system_account backfill so user renames survive. The mislabel class fixed here is "name says X, bookings are Y": payroll books salaries to 7210, 12% revenue books to 3002. The 3100 'Momsfri försäljning' deviation from BAS ('Försäljning av varor utanför Sverige') stays: there the label and the ruta 42 mapping agree by design, so renaming to BAS would create the mismatch, not fix one. [2026-07-31] Assistant product knowledge REVERTED before merge (founder call): the assistant gets no internal knowledge of Accounted's own features yet. The working product-tier implementation (agent_atom_registry CHECK widening, product/bokforingsmallar atom, discovery, MCP + panel wiring, all CI green at bb65224c) lives in fix/mall-line-type-clarity branch history and a follow-up issue for when it is wanted. Only the UI clarification ships for the radtyp confusion: unified "Kostnad/Intäkt" label and an InfoTooltip explaining the three radtyper at the point of choice. + +[2026-08-01] Cloud Backup OAuth redirect URIs resolve from NEXT_PUBLIC_APP_URL, with request origin only as a self-hosted fallback: Google and Dropbox require pre-registered callbacks, so deriving them from an old alias or preview host can reject the flow before consent; one resolver keeps authorization, exchange, revoke, and sync origins consistent. diff --git a/extensions/general/cloud-backup/__tests__/provider-routes.test.ts b/extensions/general/cloud-backup/__tests__/provider-routes.test.ts index fc9b3ec6..36687887 100644 --- a/extensions/general/cloud-backup/__tests__/provider-routes.test.ts +++ b/extensions/general/cloud-backup/__tests__/provider-routes.test.ts @@ -1,7 +1,14 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +vi.mock('next/server', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, after: vi.fn() } +}) + import { cloudBackupExtension } from '../index' import type { ExtensionContext } from '@/lib/extensions/types' import type { CloudBackupStatus } from '../types' +import { createOAuthState } from '../lib/crypto' +import { googleDriveProvider } from '../lib/google-provider' const BASE = 'https://test.local/api/extensions/ext/cloud-backup' @@ -66,6 +73,9 @@ const ENV_KEYS = [ 'DROPBOX_APP_SECRET', // The OAuth state parameter is encrypted with a key derived from this. 'SUPABASE_SERVICE_ROLE_KEY', + // Canonical app origin. Cleared so request-origin fallback tests remain + // deterministic regardless of the runner's environment. + 'NEXT_PUBLIC_APP_URL', ] as const const originalEnv: Record = {} @@ -77,9 +87,11 @@ beforeEach(() => { process.env.DROPBOX_APP_KEY = 'dropbox-key' process.env.DROPBOX_APP_SECRET = 'dropbox-secret' process.env.SUPABASE_SERVICE_ROLE_KEY = 'service-role-key' + delete process.env.NEXT_PUBLIC_APP_URL }) afterEach(() => { + vi.restoreAllMocks() for (const key of ENV_KEYS) { if (originalEnv[key] === undefined) delete process.env[key] else process.env[key] = originalEnv[key] @@ -148,6 +160,57 @@ describe('provider routing', () => { }) }) +describe('canonical redirect URI', () => { + it('builds the Google callback from NEXT_PUBLIC_APP_URL, not the request host', async () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se' + const route = findRoute('POST', '/connect') + const { ctx } = makeContext() + + const res = await route.handler(makeRequest('/connect?provider=google_drive'), ctx) + + expect(res.status).toBe(200) + const { url } = (await res.json()) as { url: string } + expect(new URL(url).searchParams.get('redirect_uri')).toBe( + 'https://app.accounted.se/api/extensions/ext/cloud-backup/oauth/callback' + ) + }) + + it('builds the Dropbox callback from NEXT_PUBLIC_APP_URL, not the request host', async () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se' + const route = findRoute('POST', '/connect') + const { ctx } = makeContext() + + const res = await route.handler(makeRequest('/connect?provider=dropbox'), ctx) + + expect(res.status).toBe(200) + const { url } = (await res.json()) as { url: string } + expect(new URL(url).searchParams.get('redirect_uri')).toBe( + 'https://app.accounted.se/api/extensions/ext/cloud-backup/oauth/dropbox/callback' + ) + }) + + it('uses the same canonical origin for the callback token exchange', async () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se' + const route = findRoute('GET', '/oauth/callback') + const { ctx } = makeContext() + const exchange = vi.spyOn(googleDriveProvider, 'exchangeCode').mockResolvedValue({ + refreshToken: 'refresh-token', + accountLabel: 'backup@example.com', + }) + const callbackUrl = new URL(`${BASE}/oauth/callback`) + callbackUrl.searchParams.set('code', 'provider-code') + callbackUrl.searchParams.set('state', createOAuthState(ctx.userId, ctx.companyId)) + + const res = await route.handler(new Request(callbackUrl), ctx) + + expect(exchange).toHaveBeenCalledWith('https://app.accounted.se', 'provider-code') + const location = new URL(res.headers.get('location')!) + expect(location.origin).toBe('https://app.accounted.se') + expect(location.pathname).toBe('/settings/backup') + expect(location.searchParams.get('cloud_backup')).toBe('connected_first') + }) +}) + describe('GET /status', () => { it('reports every provider independently', async () => { const route = findRoute('GET', '/status') diff --git a/extensions/general/cloud-backup/index.ts b/extensions/general/cloud-backup/index.ts index 2a0dc1e2..d704ea6b 100644 --- a/extensions/general/cloud-backup/index.ts +++ b/extensions/general/cloud-backup/index.ts @@ -7,6 +7,7 @@ import { verifyOAuthState, } from './lib/crypto' import { performSync } from './lib/sync' +import { resolveCallbackOrigin } from './lib/callback-origin' import { stockholmHourToUtcHour } from './lib/schedule' import { CLOUD_PROVIDERS, providerFromRequest } from './lib/provider-registry' import { googleDriveProvider } from './lib/google-provider' @@ -91,7 +92,9 @@ async function handleOAuthCallback( const code = url.searchParams.get('code') const state = url.searchParams.get('state') const errorParam = url.searchParams.get('error') - const origin = url.origin + // Resolve exactly as /connect did: the token exchange repeats the + // redirect_uri from the authorization request. + const origin = resolveCallbackOrigin(url.origin) const redirect = (status: string, reason?: string) => { const target = new URL('/settings/backup', origin) target.searchParams.set('cloud_backup', status) @@ -146,14 +149,13 @@ async function handleOAuthCallback( // Kick off the first backup after the redirect response is sent, so // the user lands back on the card immediately while the archive // builds in the background. - const syncOrigin = process.env.NEXT_PUBLIC_APP_URL || origin after(async () => { try { await performSync({ supabase: ctx.supabase, companyId: ctx.companyId, userId: ctx.userId, - origin: syncOrigin, + origin, includeDocuments: true, allowDocumentFallback: true, provider, @@ -198,7 +200,7 @@ export const cloudBackupExtension: Extension = { const resolved = resolveProvider(request, { requireConfigured: true }) if ('error' in resolved) return resolved.error try { - const origin = new URL(request.url).origin + const origin = resolveCallbackOrigin(new URL(request.url).origin) const state = createOAuthState(ctx.userId, ctx.companyId) const url = resolved.provider.buildAuthorizationUrl(origin, state) return NextResponse.json({ url }) @@ -244,7 +246,10 @@ export const cloudBackupExtension: Extension = { if (connection) { try { const refreshToken = decryptToken(connection.refresh_token_encrypted) - await provider.revoke(refreshToken, new URL(request.url).origin) + await provider.revoke( + refreshToken, + resolveCallbackOrigin(new URL(request.url).origin) + ) } catch (err) { ctx.log.warn('token revoke failed (continuing)', err) } @@ -382,8 +387,7 @@ export const cloudBackupExtension: Extension = { include_documents?: boolean allow_document_fallback?: boolean } - const origin = - process.env.NEXT_PUBLIC_APP_URL || new URL(request.url).origin + const origin = resolveCallbackOrigin(new URL(request.url).origin) const result = await performSync({ supabase: ctx.supabase, companyId: ctx.companyId, diff --git a/extensions/general/cloud-backup/lib/__tests__/callback-origin.test.ts b/extensions/general/cloud-backup/lib/__tests__/callback-origin.test.ts new file mode 100644 index 00000000..b1cc8fd1 --- /dev/null +++ b/extensions/general/cloud-backup/lib/__tests__/callback-origin.test.ts @@ -0,0 +1,63 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { resolveCallbackOrigin } from '../callback-origin' + +const originalAppUrl = process.env.NEXT_PUBLIC_APP_URL + +beforeEach(() => { + delete process.env.NEXT_PUBLIC_APP_URL +}) + +afterEach(() => { + if (originalAppUrl === undefined) delete process.env.NEXT_PUBLIC_APP_URL + else process.env.NEXT_PUBLIC_APP_URL = originalAppUrl +}) + +describe('resolveCallbackOrigin', () => { + it('falls back to the request origin when no canonical URL is configured', () => { + expect(resolveCallbackOrigin('https://self-hosted.example')).toBe( + 'https://self-hosted.example' + ) + }) + + it('prefers the canonical app origin over the request origin', () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se' + expect(resolveCallbackOrigin('https://app.gnubok.se')).toBe( + 'https://app.accounted.se' + ) + }) + + it('normalizes a trailing slash so the redirect URI stays byte-identical', () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se/' + expect(resolveCallbackOrigin('https://other.example')).toBe( + 'https://app.accounted.se' + ) + }) + + it('strips any path from the configured URL', () => { + process.env.NEXT_PUBLIC_APP_URL = 'https://app.accounted.se/dashboard' + expect(resolveCallbackOrigin('https://other.example')).toBe( + 'https://app.accounted.se' + ) + }) + + it('ignores a blank value', () => { + process.env.NEXT_PUBLIC_APP_URL = ' ' + expect(resolveCallbackOrigin('https://other.example')).toBe( + 'https://other.example' + ) + }) + + it('falls back to the request origin when the configured URL is malformed', () => { + process.env.NEXT_PUBLIC_APP_URL = 'not a url' + expect(resolveCallbackOrigin('https://other.example')).toBe( + 'https://other.example' + ) + }) + + it('falls back to the request origin for a non-web URL scheme', () => { + process.env.NEXT_PUBLIC_APP_URL = 'mailto:backup@example.com' + expect(resolveCallbackOrigin('https://other.example')).toBe( + 'https://other.example' + ) + }) +}) diff --git a/extensions/general/cloud-backup/lib/callback-origin.ts b/extensions/general/cloud-backup/lib/callback-origin.ts new file mode 100644 index 00000000..00defad6 --- /dev/null +++ b/extensions/general/cloud-backup/lib/callback-origin.ts @@ -0,0 +1,29 @@ +/** + * Resolve the origin used to build OAuth redirect URIs. + * + * Both OAuth legs must send the same redirect_uri. Pinning it to the + * deployment's canonical app URL also prevents an old domain alias or preview + * host from generating a callback that is not registered with the provider. + * Self-hosted deployments without NEXT_PUBLIC_APP_URL fall back to the + * request origin. + */ +export function resolveCallbackOrigin(requestOrigin: string): string { + const appUrl = process.env.NEXT_PUBLIC_APP_URL + if (appUrl && appUrl.trim().length > 0) { + try { + // Normalizes trailing slashes and strips paths so the provider receives + // the same bare origin on the authorization and token-exchange legs. + const configuredUrl = new URL(appUrl) + if ( + configuredUrl.protocol !== 'http:' && + configuredUrl.protocol !== 'https:' + ) { + return requestOrigin + } + return configuredUrl.origin + } catch { + return requestOrigin + } + } + return requestOrigin +}