fix(security): complete #183 — XSS output encoding + legacy AI key purge

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');
   },

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);

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.

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"><script>alert(1)</script>',
+    '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);