From 92d646076532507d59e16a51a34f82e830757ff4 Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Sun, 13 Sep 2026 17:35:57 +0100 Subject: [PATCH] fix(export): restrict provider endpoint overrides to loopback test servers The endpoint overrides are read through `$env/dynamic/private`, so they are evaluated per request in production, not baked in at build time. That made a single injected environment variable enough to redirect the authorization-code and refresh-token POSTs - which carry the OAuth client secret and the user's refresh token - to an arbitrary host, and to redirect the user's authorize hop to an arbitrary URL. Overrides are now ignored unless ALLOW_PROVIDER_ENDPOINT_OVERRIDES is exactly "true" and the value is a loopback URL. `npm run test:bdd` sets the flag; nothing else should. resolveProviderEndpoints is pure so the refusals are unit tested, including near-miss hosts such as http://127.0.0.1.example. Also drops the unused `pokedex` parameter from buildCsv rather than silencing it with `void`, and the dead hasGigantamaxed field from its fallback record. --- .env.example | 6 +- package.json | 2 +- src/lib/services/PokedexExportFormatting.ts | 5 +- src/lib/services/PokedexExportService.ts | 2 +- src/lib/services/providerEndpoints.ts | 70 ++++++++++++++++--- tests/support/envStub.ts | 5 ++ tests/unit/pokedexExportService.test.ts | 53 +++++++------- tests/unit/providerEndpoints.test.ts | 77 +++++++++++++++++++++ vitest.config.mts | 3 +- 9 files changed, 178 insertions(+), 45 deletions(-) create mode 100644 tests/support/envStub.ts create mode 100644 tests/unit/providerEndpoints.test.ts diff --git a/.env.example b/.env.example index 55337fa..12452af 100644 --- a/.env.example +++ b/.env.example @@ -8,7 +8,11 @@ GOOGLE_OAUTH_CLIENT_SECRET="your-google-client-secret" DROPBOX_OAUTH_CLIENT_ID="your-dropbox-client-id" DROPBOX_OAUTH_CLIENT_SECRET="your-dropbox-client-secret" -# Optional endpoint overrides for deterministic local provider tests. +# Endpoint overrides for deterministic local provider tests. They are ignored unless +# ALLOW_PROVIDER_ENDPOINT_OVERRIDES is exactly "true", and only loopback URLs are accepted - +# these endpoints receive the OAuth client secret and refresh token, so never set this in a +# deployed environment. `npm run test:bdd` sets it for you. +ALLOW_PROVIDER_ENDPOINT_OVERRIDES="" GOOGLE_OAUTH_AUTHORIZE_URL="" GOOGLE_OAUTH_TOKEN_URL="" GOOGLE_DRIVE_API_URL="" diff --git a/package.json b/package.json index a5e81b7..4121062 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "test:integration": "node scripts/run-with-local-supabase.mjs vitest run --config vitest.integration.config.mts", "test:bdd:generate": "bddgen", "test:bdd:inner": "npm run build-inject-manifest && npm run test:bdd:generate && playwright test --project=chromium", - "test:bdd": "GOOGLE_OAUTH_CLIENT_ID=mock-google GOOGLE_OAUTH_CLIENT_SECRET=mock-google-secret DROPBOX_OAUTH_CLIENT_ID=mock-dropbox DROPBOX_OAUTH_CLIENT_SECRET=mock-dropbox-secret GOOGLE_OAUTH_AUTHORIZE_URL=http://127.0.0.1:4199/google/authorize GOOGLE_OAUTH_TOKEN_URL=http://127.0.0.1:4199/google/token GOOGLE_DRIVE_API_URL=http://127.0.0.1:4199/google/drive GOOGLE_DRIVE_UPLOAD_URL=http://127.0.0.1:4199/google/upload DROPBOX_OAUTH_AUTHORIZE_URL=http://127.0.0.1:4199/dropbox/authorize DROPBOX_OAUTH_TOKEN_URL=http://127.0.0.1:4199/dropbox/token DROPBOX_UPLOAD_URL=http://127.0.0.1:4199/dropbox/files/upload node scripts/run-with-local-supabase.mjs npm run test:bdd:inner", + "test:bdd": "ALLOW_PROVIDER_ENDPOINT_OVERRIDES=true GOOGLE_OAUTH_CLIENT_ID=mock-google GOOGLE_OAUTH_CLIENT_SECRET=mock-google-secret DROPBOX_OAUTH_CLIENT_ID=mock-dropbox DROPBOX_OAUTH_CLIENT_SECRET=mock-dropbox-secret GOOGLE_OAUTH_AUTHORIZE_URL=http://127.0.0.1:4199/google/authorize GOOGLE_OAUTH_TOKEN_URL=http://127.0.0.1:4199/google/token GOOGLE_DRIVE_API_URL=http://127.0.0.1:4199/google/drive GOOGLE_DRIVE_UPLOAD_URL=http://127.0.0.1:4199/google/upload DROPBOX_OAUTH_AUTHORIZE_URL=http://127.0.0.1:4199/dropbox/authorize DROPBOX_OAUTH_TOKEN_URL=http://127.0.0.1:4199/dropbox/token DROPBOX_UPLOAD_URL=http://127.0.0.1:4199/dropbox/files/upload node scripts/run-with-local-supabase.mjs npm run test:bdd:inner", "test:build:generate-static": "npm run build-generate-sw && GENERATE_SW=true vitest run --config vitest.build.config.mts", "test:build:generate-node": "npm run build-generate-sw-node && NODE_ADAPTER=true GENERATE_SW=true vitest run --config vitest.build.config.mts", "test:build:inject-static": "npm run build-inject-manifest && vitest run --config vitest.build.config.mts", diff --git a/src/lib/services/PokedexExportFormatting.ts b/src/lib/services/PokedexExportFormatting.ts index 3933c4f..ec02015 100644 --- a/src/lib/services/PokedexExportFormatting.ts +++ b/src/lib/services/PokedexExportFormatting.ts @@ -1,5 +1,4 @@ import type { CombinedData } from '$lib/models/CombinedData'; -import type { Pokedex } from '$lib/models/Pokedex'; export function csvEscape(value: unknown): string { if (value === null || value === undefined) return ''; @@ -15,8 +14,7 @@ export function sanitizeFileName(name: string, fallback: string): string { return safe.endsWith('.csv') ? safe : `${safe}.csv`; } -export function buildCsv(pokedex: Pokedex, combinedData: CombinedData[]): string { - void pokedex; +export function buildCsv(combinedData: CombinedData[]): string { const headers = [ 'pokemonId', 'pokedexNumber', @@ -35,7 +33,6 @@ export function buildCsv(pokedex: Pokedex, combinedData: CombinedData[]): string caught: false, haveToEvolve: false, inHome: false, - hasGigantamaxed: false, personalNotes: '' }; lines.push( diff --git a/src/lib/services/PokedexExportService.ts b/src/lib/services/PokedexExportService.ts index 6d9dcd2..064f19a 100644 --- a/src/lib/services/PokedexExportService.ts +++ b/src/lib/services/PokedexExportService.ts @@ -398,7 +398,7 @@ export async function exportPokedexIfConfigured( pokedex.gameScope || '', dexScopes ); - const csv = buildCsv(pokedex, combinedData); + const csv = buildCsv(combinedData); const failures: ExportFailure[] = []; let successes = 0; diff --git a/src/lib/services/providerEndpoints.ts b/src/lib/services/providerEndpoints.ts index 28211d5..df7deb3 100644 --- a/src/lib/services/providerEndpoints.ts +++ b/src/lib/services/providerEndpoints.ts @@ -1,18 +1,70 @@ import { getEnv } from '$lib/utils/env'; -export function getProviderEndpoints() { - const env = getEnv(); +export type ProviderEndpoints = { + google: { authorize: string; token: string; driveApi: string; driveUpload: string }; + dropbox: { authorize: string; token: string; upload: string }; +}; + +const DEFAULTS: ProviderEndpoints = { + google: { + authorize: 'https://accounts.google.com/o/oauth2/v2/auth', + token: 'https://oauth2.googleapis.com/token', + driveApi: 'https://www.googleapis.com/drive/v3', + driveUpload: 'https://www.googleapis.com/upload/drive/v3' + }, + dropbox: { + authorize: 'https://www.dropbox.com/oauth2/authorize', + token: 'https://api.dropbox.com/oauth2/token', + upload: 'https://content.dropboxapi.com/2/files/upload' + } +}; + +const LOOPBACK_HOSTS = new Set(['127.0.0.1', 'localhost', '[::1]']); + +/** + * These endpoints receive the OAuth client secret and the user's refresh token, so an override + * is only ever a local test seam - never a deployment knob. Two guards, because the env is + * read at runtime (`$env/dynamic/private`) and a single injected variable would otherwise be + * enough to redirect those credentials to an arbitrary host: + * + * 1. overrides are ignored unless ALLOW_PROVIDER_ENDPOINT_OVERRIDES is exactly "true", and + * 2. even then, only loopback URLs are accepted. + */ +function isLocalOverride(value: string): boolean { + try { + const url = new URL(value); + return ( + (url.protocol === 'http:' || url.protocol === 'https:') && LOOPBACK_HOSTS.has(url.hostname) + ); + } catch { + return false; + } +} + +export function resolveProviderEndpoints( + env: Record +): ProviderEndpoints { + const overridesAllowed = env.ALLOW_PROVIDER_ENDPOINT_OVERRIDES === 'true'; + const pick = (override: string | undefined, fallback: string) => + overridesAllowed && override && isLocalOverride(override) ? override : fallback; + return { google: { - authorize: env.GOOGLE_OAUTH_AUTHORIZE_URL || 'https://accounts.google.com/o/oauth2/v2/auth', - token: env.GOOGLE_OAUTH_TOKEN_URL || 'https://oauth2.googleapis.com/token', - driveApi: env.GOOGLE_DRIVE_API_URL || 'https://www.googleapis.com/drive/v3', - driveUpload: env.GOOGLE_DRIVE_UPLOAD_URL || 'https://www.googleapis.com/upload/drive/v3' + authorize: pick(env.GOOGLE_OAUTH_AUTHORIZE_URL, DEFAULTS.google.authorize), + token: pick(env.GOOGLE_OAUTH_TOKEN_URL, DEFAULTS.google.token), + driveApi: pick(env.GOOGLE_DRIVE_API_URL, DEFAULTS.google.driveApi), + driveUpload: pick(env.GOOGLE_DRIVE_UPLOAD_URL, DEFAULTS.google.driveUpload) }, dropbox: { - authorize: env.DROPBOX_OAUTH_AUTHORIZE_URL || 'https://www.dropbox.com/oauth2/authorize', - token: env.DROPBOX_OAUTH_TOKEN_URL || 'https://api.dropbox.com/oauth2/token', - upload: env.DROPBOX_UPLOAD_URL || 'https://content.dropboxapi.com/2/files/upload' + authorize: pick(env.DROPBOX_OAUTH_AUTHORIZE_URL, DEFAULTS.dropbox.authorize), + token: pick(env.DROPBOX_OAUTH_TOKEN_URL, DEFAULTS.dropbox.token), + upload: pick(env.DROPBOX_UPLOAD_URL, DEFAULTS.dropbox.upload) } }; } + +export function getProviderEndpoints(): ProviderEndpoints { + return resolveProviderEndpoints(getEnv()); +} + +export const PROVIDER_ENDPOINT_DEFAULTS = DEFAULTS; diff --git a/tests/support/envStub.ts b/tests/support/envStub.ts new file mode 100644 index 0000000..9586c11 --- /dev/null +++ b/tests/support/envStub.ts @@ -0,0 +1,5 @@ +/** + * Stands in for `$env/dynamic/private` so modules that reach for runtime env can be unit + * tested. Vitest maps the virtual module here; see vitest.config.mts. + */ +export const env: Record = process.env; diff --git a/tests/unit/pokedexExportService.test.ts b/tests/unit/pokedexExportService.test.ts index 27c54c5..ef24d67 100644 --- a/tests/unit/pokedexExportService.test.ts +++ b/tests/unit/pokedexExportService.test.ts @@ -4,7 +4,7 @@ import { csvEscape, sanitizeFileName, shouldRefreshToken -} from '../../src/lib/services/PokedexExportFormatting'; +} from '$lib/services/PokedexExportFormatting'; describe('Pokédex export formatting', () => { it.each([ @@ -26,35 +26,32 @@ describe('Pokédex export formatting', () => { }); it('builds a stable, escaped CSV with defaults for missing catch records', () => { - const csv = buildCsv( - { _id: 'dex-1', name: 'Test' } as never, - [ - { - pokedexEntry: { - _id: '25', - pokedexNumber: 25, - pokemon: 'Pikachu', - form: null - }, - catchRecord: { - caught: true, - haveToEvolve: false, - inHome: true, - hasGigantamaxed: false, - personalNotes: 'Comma, and "quote"' - } + const csv = buildCsv([ + { + pokedexEntry: { + _id: '25', + pokedexNumber: 25, + pokemon: 'Pikachu', + form: null }, - { - pokedexEntry: { - _id: '26', - pokedexNumber: 26, - pokemon: 'Raichu', - form: 'Alolan' - }, - catchRecord: null + catchRecord: { + caught: true, + haveToEvolve: false, + inHome: true, + hasGigantamaxed: false, + personalNotes: 'Comma, and "quote"' } - ] as never - ); + }, + { + pokedexEntry: { + _id: '26', + pokedexNumber: 26, + pokemon: 'Raichu', + form: 'Alolan' + }, + catchRecord: null + } + ] as never); expect(csv.split('\r\n')).toEqual([ 'pokemonId,pokedexNumber,pokemon,form,caught,haveToEvolve,inHome,personalNotes', '25,25,Pikachu,,true,false,true,"Comma, and ""quote"""', diff --git a/tests/unit/providerEndpoints.test.ts b/tests/unit/providerEndpoints.test.ts new file mode 100644 index 0000000..33be91a --- /dev/null +++ b/tests/unit/providerEndpoints.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from 'vitest'; +import { + PROVIDER_ENDPOINT_DEFAULTS, + resolveProviderEndpoints +} from '$lib/services/providerEndpoints'; + +const localOverrides = { + GOOGLE_OAUTH_AUTHORIZE_URL: 'http://127.0.0.1:4199/google/authorize', + GOOGLE_OAUTH_TOKEN_URL: 'http://127.0.0.1:4199/google/token', + GOOGLE_DRIVE_API_URL: 'http://127.0.0.1:4199/google/drive', + GOOGLE_DRIVE_UPLOAD_URL: 'http://127.0.0.1:4199/google/upload', + DROPBOX_OAUTH_AUTHORIZE_URL: 'http://127.0.0.1:4199/dropbox/authorize', + DROPBOX_OAUTH_TOKEN_URL: 'http://127.0.0.1:4199/dropbox/token', + DROPBOX_UPLOAD_URL: 'http://127.0.0.1:4199/dropbox/files/upload' +}; + +describe('provider endpoints', () => { + it('uses the real provider endpoints when nothing is configured', () => { + expect(resolveProviderEndpoints({})).toEqual(PROVIDER_ENDPOINT_DEFAULTS); + }); + + it('ignores overrides unless they are explicitly allowed', () => { + // The exfiltration case: these variables carry the client secret and refresh token, and + // the env is read per request in production. + expect(resolveProviderEndpoints(localOverrides)).toEqual(PROVIDER_ENDPOINT_DEFAULTS); + expect( + resolveProviderEndpoints({ ...localOverrides, ALLOW_PROVIDER_ENDPOINT_OVERRIDES: 'false' }) + ).toEqual(PROVIDER_ENDPOINT_DEFAULTS); + expect( + resolveProviderEndpoints({ ...localOverrides, ALLOW_PROVIDER_ENDPOINT_OVERRIDES: '1' }) + ).toEqual(PROVIDER_ENDPOINT_DEFAULTS); + }); + + it('applies allowed loopback overrides', () => { + expect( + resolveProviderEndpoints({ ...localOverrides, ALLOW_PROVIDER_ENDPOINT_OVERRIDES: 'true' }) + ).toEqual({ + google: { + authorize: localOverrides.GOOGLE_OAUTH_AUTHORIZE_URL, + token: localOverrides.GOOGLE_OAUTH_TOKEN_URL, + driveApi: localOverrides.GOOGLE_DRIVE_API_URL, + driveUpload: localOverrides.GOOGLE_DRIVE_UPLOAD_URL + }, + dropbox: { + authorize: localOverrides.DROPBOX_OAUTH_AUTHORIZE_URL, + token: localOverrides.DROPBOX_OAUTH_TOKEN_URL, + upload: localOverrides.DROPBOX_UPLOAD_URL + } + }); + }); + + it.each([ + 'https://attacker.example/token', + 'http://127.0.0.1.attacker.example/token', + 'http://[::2]/token', + 'file:///etc/passwd', + 'not-a-url', + '' + ])('refuses the non-loopback override %j even when overrides are allowed', (value) => { + const endpoints = resolveProviderEndpoints({ + ALLOW_PROVIDER_ENDPOINT_OVERRIDES: 'true', + GOOGLE_OAUTH_TOKEN_URL: value, + DROPBOX_OAUTH_TOKEN_URL: value + }); + expect(endpoints.google.token).toBe(PROVIDER_ENDPOINT_DEFAULTS.google.token); + expect(endpoints.dropbox.token).toBe(PROVIDER_ENDPOINT_DEFAULTS.dropbox.token); + }); + + it('accepts localhost as well as 127.0.0.1', () => { + expect( + resolveProviderEndpoints({ + ALLOW_PROVIDER_ENDPOINT_OVERRIDES: 'true', + GOOGLE_OAUTH_TOKEN_URL: 'http://localhost:4199/google/token' + }).google.token + ).toBe('http://localhost:4199/google/token'); + }); +}); diff --git a/vitest.config.mts b/vitest.config.mts index 9ab640e..9171383 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -4,7 +4,8 @@ import { fileURLToPath } from 'node:url'; export default defineConfig({ resolve: { alias: { - $lib: fileURLToPath(new URL('./src/lib', import.meta.url)) + $lib: fileURLToPath(new URL('./src/lib', import.meta.url)), + '$env/dynamic/private': fileURLToPath(new URL('./tests/support/envStub.ts', import.meta.url)) } }, test: {