fix: audit Cloud Backup OAuth redirects (#1324)
* fix(cloud-backup): pin OAuth callback origin * fix: reject non-web cloud backup origins
This commit is contained in:
@@ -725,3 +725,5 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. 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.
|
||||
|
||||
@@ -1,7 +1,14 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
|
||||
vi.mock('next/server', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('next/server')>()
|
||||
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<string, string | undefined> = {}
|
||||
|
||||
@@ -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')
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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'
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
Reference in New Issue
Block a user