diff --git a/frontend/src/utils/aiApi.js b/frontend/src/utils/aiApi.js index e7cf108..6c8d62a 100644 --- a/frontend/src/utils/aiApi.js +++ b/frontend/src/utils/aiApi.js @@ -24,6 +24,13 @@ const TIMEOUT = 30000; // persist in localStorage as before. let apiKeyInMemory = ''; +// One-time migration: builds before the in-memory switch (issue #180) stored +// the key at localStorage['ai_api_key']. Purge it on load so existing users +// don't keep a plaintext key sitting in storage indefinitely. Deliberately not +// read into apiKeyInMemory — the whole point is that the secret no longer lives +// in a persistent, XSS-readable store. +localStorage.removeItem('ai_api_key'); + export const aiSettings = { getProvider: () => localStorage.getItem('ai_provider') || 'anthropic', getApiKey: () => apiKeyInMemory, @@ -36,6 +43,7 @@ export const aiSettings = { isConfigured: () => !!apiKeyInMemory, clear: () => { apiKeyInMemory = ''; + localStorage.removeItem('ai_api_key'); // defence in depth: also drop any legacy stored key localStorage.removeItem('ai_provider'); localStorage.removeItem('ai_model'); }, diff --git a/frontend/src/utils/aiApi.test.mjs b/frontend/src/utils/aiApi.test.mjs index 2ec9e01..5cd7294 100644 --- a/frontend/src/utils/aiApi.test.mjs +++ b/frontend/src/utils/aiApi.test.mjs @@ -15,7 +15,7 @@ import path from 'node:path'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); -function loadAiApiModule() { +function loadAiApiModule(seed = {}) { let source = readFileSync(path.join(__dirname, 'aiApi.js'), 'utf8'); // Neutralize the one Vite-only construct so this can run under plain Node. @@ -30,7 +30,10 @@ function loadAiApiModule() { source = source.replace(/^export const /gm, 'const '); source += '\nreturn { aiSettings, aiApi };'; - const backingStore = new Map(); + // `seed` pre-populates localStorage before the module body runs, so tests can + // simulate a browser that already has values from a previous app version + // (e.g. a legacy plaintext ai_api_key that predates the in-memory switch). + const backingStore = new Map(Object.entries(seed)); const localStorageStub = { getItem: (k) => (backingStore.has(k) ? backingStore.get(k) : null), setItem: (k, v) => backingStore.set(k, String(v)), @@ -107,6 +110,32 @@ check('a fresh module load never sees a key from a previous instance (no shared assert.equal(second.aiSettings.getApiKey(), ''); }); +check('loading the module purges a legacy plaintext ai_api_key left by an older version', () => { + // Users who ran a build before the in-memory switch (issue #180) still have + // their key sitting in localStorage. The module must delete it on load, or + // the very leak the fix was meant to close persists indefinitely for them. + const { backingStore } = loadAiApiModule({ ai_api_key: 'legacy-plaintext-key' }); + assert.equal(backingStore.has('ai_api_key'), false); + assert.ok( + ![...backingStore.values()].some((v) => String(v).includes('legacy-plaintext-key')), + 'legacy key still present in localStorage after module load', + ); +}); + +check('the migration does not resurrect the legacy key into the in-memory field', () => { + // Purging the stored key must not silently load it into memory either — a + // fresh load with a legacy key present should still start unconfigured. + const { aiSettings } = loadAiApiModule({ ai_api_key: 'legacy-plaintext-key' }); + assert.equal(aiSettings.isConfigured(), false); + assert.equal(aiSettings.getApiKey(), ''); +}); + +check('clear() also removes any legacy ai_api_key still in storage', () => { + const { aiSettings, backingStore } = loadAiApiModule({ ai_api_key: 'legacy-plaintext-key' }); + aiSettings.clear(); + assert.equal(backingStore.has('ai_api_key'), false); +}); + if (failures > 0) { console.error(`\n${failures} test(s) failed`); process.exit(1); diff --git a/website/script.js b/website/script.js index 3c08ba9..a54b0c0 100644 --- a/website/script.js +++ b/website/script.js @@ -245,7 +245,12 @@ function toEmbedUrl(raw) { try { const parsed = new URL(raw); if (parsed.protocol === 'https:' && EMBED_ALLOWED_HOSTS.has(parsed.hostname)) { - return raw; + // Return the canonicalised href, not the raw input: a valid host + // still lets an attribute-injection payload (e.g. a literal + // double-quote) through on an allowed origin. parsed.href + // percent-encodes quotes/spaces/brackets so the value is safe to + // interpolate into the iframe src="..." attribute. + return parsed.href; } } catch { // Not a valid absolute URL — fall through to reject below. diff --git a/website/test_toEmbedUrl.mjs b/website/test_toEmbedUrl.mjs index 62274b5..39332a1 100644 --- a/website/test_toEmbedUrl.mjs +++ b/website/test_toEmbedUrl.mjs @@ -80,8 +80,34 @@ const rejected = [ 'not a url at all but contains youtube.com/embed', ]; +// Inputs on an ALLOWED host that still carry an attribute-injection payload. +// The host passes the allowlist, so the earlier "rejected" cases don't cover +// this — the returned value is interpolated into an iframe src="..." attribute, +// so it must never contain a raw double-quote that could break out of it. +// The fix returns the canonicalised URL.href (which percent-encodes quotes and +// spaces) instead of the raw input. +const sanitizedPassthrough = [ + [ + 'https://www.youtube.com/embed/abc" onload="alert(1)', + 'https://www.youtube.com/embed/abc%22%20onload=%22alert(1)', + 'attribute-injection payload on allowed host is percent-encoded', + ], + [ + 'https://player.vimeo.com/video/1">', + 'https://player.vimeo.com/video/1%22%3E%3Cscript%3Ealert(1)%3C/script%3E', + 'script-injection payload on allowed vimeo host is percent-encoded', + ], +]; + let failures = 0; +function assertNoDoubleQuote(value, description) { + assert.ok( + !String(value).includes('"'), + `${description}: return value must not contain a raw double-quote (iframe src breakout): ${JSON.stringify(value)}`, + ); +} + for (const [input, expected, description] of cases) { const actual = toEmbedUrl(input); try { @@ -104,6 +130,18 @@ for (const input of rejected) { } } +for (const [input, expected, description] of sanitizedPassthrough) { + const actual = toEmbedUrl(input); + try { + assert.equal(actual, expected); + assertNoDoubleQuote(actual, description); + console.log(`PASS: ${description}`); + } catch (err) { + failures++; + console.error(`FAIL: ${description} — input=${JSON.stringify(input)} got=${JSON.stringify(actual)}\n ${err.message}`); + } +} + if (failures > 0) { console.error(`\n${failures} test(s) failed`); process.exit(1);