From d8c2298d0738d4dc935f89f4b7a38b875d6b65bd Mon Sep 17 00:00:00 2001 From: hamsa0x7 <116961508+hamsa0x7@users.noreply.github.com> Date: Fri, 26 Jun 2026 11:01:13 +0700 Subject: [PATCH] fix(security): patch 5 vulnerabilities from security audit - mask API keys in usage stats/history responses (apiKeyMasked) - validate proxy URL scheme + reject shell metachars before env write - escape HTML in OAuth callback page to prevent XSS - atomic O_EXCL lock file to prevent TOCTOU race in MITM startServer - set mitmIsRestarting guard synchronously before any await Co-authored-by: Cursor --- src/lib/db/repos/usageRepo.js | 20 ++- src/lib/network/outboundProxy.js | 27 ++- src/lib/oauth/utils/server.js | 12 +- src/mitm/manager.js | 30 +++- tests/unit/security-audit.test.js | 289 ++++++++++++++++++++++++++++++ 5 files changed, 364 insertions(+), 14 deletions(-) create mode 100644 tests/unit/security-audit.test.js diff --git a/src/lib/db/repos/usageRepo.js b/src/lib/db/repos/usageRepo.js index c6b6cb56..b0d6bff0 100644 --- a/src/lib/db/repos/usageRepo.js +++ b/src/lib/db/repos/usageRepo.js @@ -3,6 +3,12 @@ import { getAdapter } from "../driver.js"; import { parseJson, stringifyJson } from "../helpers/jsonCol.js"; import { getMeta, setMeta } from "../helpers/metaStore.js"; +function maskApiKey(key) { + if (!key || typeof key !== "string") return null; + if (key.length <= 8) return key.charAt(0) + "***"; + return key.slice(0, 8) + "***"; +} + const PENDING_TIMEOUT_MS = 60 * 1000; const RING_CAP = 50; const CONN_CACHE_TTL_MS = 30 * 1000; @@ -342,7 +348,7 @@ export async function getUsageHistory(filter = {}) { return rows.map((r) => ({ timestamp: r.timestamp, provider: r.provider, model: r.model, - connectionId: r.connectionId, apiKey: r.apiKey, endpoint: r.endpoint, + connectionId: r.connectionId, apiKeyMasked: maskApiKey(r.apiKey), endpoint: r.endpoint, cost: r.cost, status: r.status, tokens: parseJson(r.tokens, {}), })); } @@ -516,9 +522,10 @@ export async function getUsageStats(period = "all") { const apiKeyVal = ak.apiKey; const keyInfo = apiKeyVal ? apiKeyMap[apiKeyVal] : null; const keyName = keyInfo?.name || (apiKeyVal ? apiKeyVal.slice(0, 8) + "..." : "Local (No API Key)"); - const apiKeyKey = apiKeyVal || "local-no-key"; + const apiKeyMasked = maskApiKey(apiKeyVal); + const apiKeyKey = apiKeyMasked || "local-no-key"; if (!stats.byApiKey[akKey]) { - stats.byApiKey[akKey] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel, provider: providerDisplayName, apiKey: apiKeyVal, keyName, apiKeyKey, lastUsed: dateKey }; + stats.byApiKey[akKey] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel, provider: providerDisplayName, apiKeyMasked, keyName, apiKeyKey, lastUsed: dateKey }; } stats.byApiKey[akKey].requests += ak.requests || 0; stats.byApiKey[akKey].promptTokens += ak.promptTokens || 0; @@ -627,16 +634,17 @@ export async function getUsageStats(period = "all") { if (r.apiKey && typeof r.apiKey === "string") { const keyInfo = apiKeyMap[r.apiKey]; const keyName = keyInfo?.name || r.apiKey.slice(0, 8) + "..."; - const akKey = `${r.apiKey}|${r.model}|${r.provider || "unknown"}`; + const apiKeyMasked = maskApiKey(r.apiKey); + const akKey = `${apiKeyMasked}|${r.model}|${r.provider || "unknown"}`; if (!stats.byApiKey[akKey]) { - stats.byApiKey[akKey] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel: r.model, provider: providerDisplayName, apiKey: r.apiKey, keyName, apiKeyKey: r.apiKey, lastUsed: r.timestamp }; + stats.byApiKey[akKey] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel: r.model, provider: providerDisplayName, apiKeyMasked, keyName, apiKeyKey: apiKeyMasked, lastUsed: r.timestamp }; } const ake = stats.byApiKey[akKey]; ake.requests++; ake.promptTokens += promptTokens; ake.completionTokens += completionTokens; ake.cost += entryCost; if (new Date(r.timestamp) > new Date(ake.lastUsed)) ake.lastUsed = r.timestamp; } else { if (!stats.byApiKey["local-no-key"]) { - stats.byApiKey["local-no-key"] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel: r.model, provider: providerDisplayName, apiKey: null, keyName: "Local (No API Key)", apiKeyKey: "local-no-key", lastUsed: r.timestamp }; + stats.byApiKey["local-no-key"] = { requests: 0, promptTokens: 0, completionTokens: 0, cost: 0, rawModel: r.model, provider: providerDisplayName, apiKeyMasked: null, keyName: "Local (No API Key)", apiKeyKey: "local-no-key", lastUsed: r.timestamp }; } const ake = stats.byApiKey["local-no-key"]; ake.requests++; ake.promptTokens += promptTokens; ake.completionTokens += completionTokens; ake.cost += entryCost; diff --git a/src/lib/network/outboundProxy.js b/src/lib/network/outboundProxy.js index 9c99dd3a..f85c63a2 100644 --- a/src/lib/network/outboundProxy.js +++ b/src/lib/network/outboundProxy.js @@ -3,6 +3,20 @@ function normalizeString(value) { return String(value).trim(); } +const ALLOWED_PROXY_SCHEMES = ["http:", "https:", "socks5:", "socks4:", "socks5h:", "socks4a:"]; + +function validateProxyUrl(url) { + if (!url) return null; + if (/[\n\r`$]/.test(url)) return null; + try { + const parsed = new URL(url); + if (!ALLOWED_PROXY_SCHEMES.includes(parsed.protocol)) return null; + return parsed.href; + } catch { + return null; + } +} + export function applyOutboundProxyEnv( { outboundProxyEnabled, outboundProxyUrl, outboundNoProxy } = {} ) { @@ -46,11 +60,14 @@ export function applyOutboundProxyEnv( } if (proxyUrl) { - process.env.HTTP_PROXY = proxyUrl; - process.env.HTTPS_PROXY = proxyUrl; - process.env.ALL_PROXY = proxyUrl; - process.env.NINE_ROUTER_PROXY_URL = proxyUrl; - managed = true; + const validated = validateProxyUrl(proxyUrl); + if (validated) { + process.env.HTTP_PROXY = validated; + process.env.HTTPS_PROXY = validated; + process.env.ALL_PROXY = validated; + process.env.NINE_ROUTER_PROXY_URL = validated; + managed = true; + } } if (noProxy) { diff --git a/src/lib/oauth/utils/server.js b/src/lib/oauth/utils/server.js index be54a414..b11a9a44 100644 --- a/src/lib/oauth/utils/server.js +++ b/src/lib/oauth/utils/server.js @@ -154,14 +154,24 @@ export function clearCodexSession(state) { pendingExchanges.delete(state); } +function escapeHtml(str) { + return String(str) + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +} + function renderCodexResultPage(success, message) { const color = success ? "#22c55e" : "#ef4444"; const icon = success ? "✓" : "✗"; const title = success ? "Authentication Successful" : "Authentication Failed"; + const safeMessage = escapeHtml(message); return ` ${title} -
${icon}

${title}

${message}

Closing in 3s...

+
${icon}

${title}

${safeMessage}

Closing in 3s...

`; } diff --git a/src/mitm/manager.js b/src/mitm/manager.js index bd6f99fe..b70fbdf0 100644 --- a/src/mitm/manager.js +++ b/src/mitm/manager.js @@ -41,6 +41,7 @@ async function resolveMitmRouterBaseUrl() { const MITM_PORT = 443; const MITM_WIN_NODE_PORT = 8443; const PID_FILE = path.join(MITM_DIR, ".mitm.pid"); +const LOCK_FILE = path.join(MITM_DIR, ".mitm.lock"); const MITM_MAX_RESTARTS = 5; const MITM_RESTART_DELAYS_MS = [5000, 10000, 20000, 30000, 60000]; @@ -400,19 +401,22 @@ async function getMitmStatus() { async function scheduleMitmRestart(apiKey) { if (mitmIsRestarting) return; + // Set guard synchronously before any await to prevent concurrent calls + // from passing the check above. + mitmIsRestarting = true; const aliveMs = Date.now() - mitmLastStartTime; if (aliveMs >= MITM_RESTART_RESET_MS) mitmRestartCount = 0; if (mitmRestartCount >= MITM_MAX_RESTARTS) { err("Max restart attempts reached. Giving up."); + mitmIsRestarting = false; return; } const attempt = mitmRestartCount; const delay = MITM_RESTART_DELAYS_MS[Math.min(attempt, MITM_RESTART_DELAYS_MS.length - 1)]; mitmRestartCount++; - mitmIsRestarting = true; log(`Restarting in ${delay / 1000}s... (${mitmRestartCount}/${MITM_MAX_RESTARTS})`); await new Promise((r) => setTimeout(r, delay)); @@ -486,7 +490,19 @@ async function startServer(apiKey, sudoPassword, forceKillPort443 = false) { throw new Error("MITM server is already running"); } - await killLeftoverMitm(sudoPassword); + // Atomically claim lock to prevent concurrent startServer across processes. + // O_EXCL (flag: "wx") fails with EEXIST if the file already exists. + try { + fs.writeFileSync(LOCK_FILE, String(process.pid), { flag: "wx" }); + } catch (e) { + if (e.code === "EEXIST") { + throw new Error("MITM server is already starting (lock contention)"); + } + throw e; + } + + try { + await killLeftoverMitm(sudoPassword); if (!IS_WIN) { const portStatus = await checkPort443Free(); @@ -679,6 +695,7 @@ async function startServer(apiKey, sudoPassword, forceKillPort443 = false) { serverProcess = null; serverPid = null; try { fs.unlinkSync(PID_FILE); } catch { /* ignore */ } + try { fs.unlinkSync(LOCK_FILE); } catch { /* ignore */ } // Auto-restart on unexpected exit if (code !== 0 && !mitmIsRestarting) scheduleMitmRestart(apiKey); }); @@ -706,7 +723,15 @@ async function startServer(apiKey, sudoPassword, forceKillPort443 = false) { await saveMitmSettings(true, sudoPassword); if (sudoPassword) setCachedPassword(sudoPassword); + // Server is healthy — remove lock file (PID file persists as the marker) + try { fs.unlinkSync(LOCK_FILE); } catch { /* ignore */ } + return { running: true, pid: serverPid }; + } catch (e) { + // Clean up lock on any failure + try { fs.unlinkSync(LOCK_FILE); } catch { /* ignore */ } + throw e; + } } /** @@ -779,6 +804,7 @@ async function stopServer(sudoPassword) { } try { fs.unlinkSync(PID_FILE); } catch { /* ignore */ } + try { fs.unlinkSync(LOCK_FILE); } catch { /* ignore */ } await saveMitmSettings(false, null); mitmIsRestarting = false; diff --git a/tests/unit/security-audit.test.js b/tests/unit/security-audit.test.js new file mode 100644 index 00000000..954751c1 --- /dev/null +++ b/tests/unit/security-audit.test.js @@ -0,0 +1,289 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import fs from "fs"; +import path from "path"; + +// ============================================================ +// AUDIT-002 (#1962): API key masking in usage stats +// ============================================================ +describe("AUDIT-002: API key masking", () => { + it("source should contain maskApiKey function", () => { + const source = fs.readFileSync( + path.resolve("src/lib/db/repos/usageRepo.js"), + "utf-8" + ); + expect(source).toContain("function maskApiKey"); + }); + + it("getUsageHistory should use apiKeyMasked instead of apiKey", () => { + const source = fs.readFileSync( + path.resolve("src/lib/db/repos/usageRepo.js"), + "utf-8" + ); + // The REST response should use apiKeyMasked + expect(source).toContain("apiKeyMasked: maskApiKey(r.apiKey)"); + // The return mapping in getUsageHistory should not have raw apiKey + // (The internal ring buffer still uses apiKey: r.apiKey for internal state - that's fine) + const historyReturn = source.match(/return rows\.map\(\(r\)\s*=>\s*\(\{[\s\S]*?\}\)\);/); + expect(historyReturn).not.toBeNull(); + expect(historyReturn[0]).toContain("apiKeyMasked"); + expect(historyReturn[0]).not.toContain("apiKey: r.apiKey"); + }); + + it("getUsageStats should use apiKeyMasked in byApiKey entries", () => { + const source = fs.readFileSync( + path.resolve("src/lib/db/repos/usageRepo.js"), + "utf-8" + ); + // Both code paths (daily summary + 24h live) should use apiKeyMasked + const maskedCount = (source.match(/apiKeyMasked/g) || []).length; + expect(maskedCount).toBeGreaterThanOrEqual(4); // function def + 3 usage sites + + // The byApiKey stats entries should use apiKeyMasked, not raw apiKey + // Check the daily summary path + const dailyPath = source.match(/stats\.byApiKey\[akKey\] = \{[^}]*apiKeyMasked[^}]*\}/); + expect(dailyPath).not.toBeNull(); + // Check the 24h live path + const livePath = source.match(/stats\.byApiKey\[akKey\] = \{[^}]*apiKeyMasked[^}]*\}/g); + expect(livePath).not.toBeNull(); + expect(livePath.length).toBeGreaterThanOrEqual(1); + }); + + it("byApiKey object keys should use masked key, not raw key", () => { + const source = fs.readFileSync( + path.resolve("src/lib/db/repos/usageRepo.js"), + "utf-8" + ); + // The 24h path should use apiKeyMasked in the akKey template + expect(source).toContain("${apiKeyMasked}|${r.model}|${r.provider"); + // Should NOT use raw r.apiKey in the key + expect(source).not.toContain("${r.apiKey}|${r.model}|${r.provider"); + }); +}); + +// ============================================================ +// AUDIT-003 (#1961): Proxy URL validation +// ============================================================ +describe("AUDIT-003: Proxy URL validation", () => { + beforeEach(() => { + delete process.env.HTTP_PROXY; + delete process.env.HTTPS_PROXY; + delete process.env.ALL_PROXY; + delete process.env.NINE_ROUTER_PROXY_MANAGED; + delete process.env.NINE_ROUTER_PROXY_URL; + delete process.env.NINE_ROUTER_NO_PROXY; + delete process.env.NO_PROXY; + }); + + it("source should contain validateProxyUrl function", () => { + const source = fs.readFileSync( + path.resolve("src/lib/network/outboundProxy.js"), + "utf-8" + ); + expect(source).toContain("function validateProxyUrl"); + expect(source).toContain("ALLOWED_PROXY_SCHEMES"); + }); + + it("should accept valid http proxy URLs", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "http://proxy.example.com:8080", + }); + // new URL().href normalizes (adds trailing slash) + expect(process.env.HTTP_PROXY).toContain("http://proxy.example.com:8080"); + expect(process.env.HTTPS_PROXY).toContain("http://proxy.example.com:8080"); + }); + + it("should accept valid https proxy URLs", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "https://proxy.example.com:443", + }); + // new URL().href normalizes (drops default port 443, adds trailing slash) + expect(process.env.HTTP_PROXY).toContain("https://proxy.example.com"); + }); + + it("should accept valid socks5 proxy URLs", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "socks5://proxy.example.com:1080", + }); + expect(process.env.ALL_PROXY).toBe("socks5://proxy.example.com:1080"); + }); + + it("should reject URLs with shell metacharacters (newline)", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "http://proxy.example.com:8080\nmalicious", + }); + expect(process.env.HTTP_PROXY).toBeUndefined(); + }); + + it("should reject URLs with shell metacharacters (backtick)", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "http://`whoami`.example.com:8080", + }); + expect(process.env.HTTP_PROXY).toBeUndefined(); + }); + + it("should reject URLs with shell metacharacters (dollar)", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "http://$(whoami).example.com:8080", + }); + expect(process.env.HTTP_PROXY).toBeUndefined(); + }); + + it("should reject non-allowed schemes (file://)", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "file:///etc/passwd", + }); + expect(process.env.HTTP_PROXY).toBeUndefined(); + }); + + it("should reject non-allowed schemes (javascript:)", async () => { + vi.resetModules(); + const { applyOutboundProxyEnv } = await import("../../src/lib/network/outboundProxy.js"); + applyOutboundProxyEnv({ + outboundProxyEnabled: true, + outboundProxyUrl: "javascript:alert(1)", + }); + expect(process.env.HTTP_PROXY).toBeUndefined(); + }); +}); + +// ============================================================ +// AUDIT-018 (#1972): XSS escaping in OAuth callback +// ============================================================ +describe("AUDIT-018: XSS escaping in OAuth callback", () => { + it("source should contain escapeHtml function", () => { + const source = fs.readFileSync( + path.resolve("src/lib/oauth/utils/server.js"), + "utf-8" + ); + expect(source).toContain("function escapeHtml"); + }); + + it("should escape ampersand, angle brackets, and quotes", () => { + const source = fs.readFileSync( + path.resolve("src/lib/oauth/utils/server.js"), + "utf-8" + ); + expect(source).toContain("&"); + expect(source).toContain("<"); + expect(source).toContain(">"); + expect(source).toContain("""); + expect(source).toContain("'"); + }); + + it("should use safeMessage in rendered HTML, not raw message", () => { + const source = fs.readFileSync( + path.resolve("src/lib/oauth/utils/server.js"), + "utf-8" + ); + expect(source).toContain("safeMessage"); + expect(source).toContain("${safeMessage}"); + // Should NOT use raw message in HTML body + expect(source).not.toContain("

${message}

"); + }); +}); + +// ============================================================ +// AUDIT-004 (#1963): TOCTOU race - atomic lock file +// ============================================================ +describe("AUDIT-004: Atomic lock file for MITM startup", () => { + it("manager.js should define LOCK_FILE constant", () => { + const source = fs.readFileSync( + path.resolve("src/mitm/manager.js"), + "utf-8" + ); + expect(source).toContain("LOCK_FILE"); + expect(source).toContain(".mitm.lock"); + }); + + it("should use O_EXCL flag (wx) for atomic creation", () => { + const source = fs.readFileSync( + path.resolve("src/mitm/manager.js"), + "utf-8" + ); + expect(source).toContain('"wx"'); + expect(source).toContain("EEXIST"); + }); + + it("should clean up lock file on all exit paths", () => { + const source = fs.readFileSync( + path.resolve("src/mitm/manager.js"), + "utf-8" + ); + const matches = source.match(/unlinkSync\(LOCK_FILE\)/g); + expect(matches).not.toBeNull(); + expect(matches.length).toBeGreaterThanOrEqual(4); + }); +}); + +// ============================================================ +// AUDIT-001 (#1965): Race condition in retry tracking +// ============================================================ +describe("AUDIT-001: Synchronous restart guard", () => { + it("mitmIsRestarting should be set before first await expression", () => { + const source = fs.readFileSync( + path.resolve("src/mitm/manager.js"), + "utf-8" + ); + + const funcStart = source.indexOf("async function scheduleMitmRestart"); + expect(funcStart).toBeGreaterThan(-1); + + const funcBody = source.substring(funcStart, funcStart + 2000); + + const guardCheckIdx = funcBody.indexOf("if (mitmIsRestarting) return;"); + expect(guardCheckIdx).toBeGreaterThan(-1); + + const afterGuard = funcBody.substring(guardCheckIdx); + + // Strip line comments to avoid matching "await" in comment text + const noComments = afterGuard.replace(/\/\/.*$/gm, ""); + + // Find the first actual await expression + const firstAwaitIdx = noComments.search(/\bawait\s+/); + expect(firstAwaitIdx).toBeGreaterThan(-1); + + // Find mitmIsRestarting = true + const setFlagIdx = noComments.indexOf("mitmIsRestarting = true"); + + expect(setFlagIdx).toBeGreaterThan(-1); + expect(firstAwaitIdx).toBeGreaterThan(-1); + expect(setFlagIdx).toBeLessThan(firstAwaitIdx); + }); + + it("mitmIsRestarting should be reset on max-restarts early return", () => { + const source = fs.readFileSync( + path.resolve("src/mitm/manager.js"), + "utf-8" + ); + + const funcStart = source.indexOf("async function scheduleMitmRestart"); + const funcBody = source.substring(funcStart, funcStart + 2000); + + const maxRestartsIdx = funcBody.indexOf("Max restart attempts reached"); + expect(maxRestartsIdx).toBeGreaterThan(-1); + + const afterMax = funcBody.substring(maxRestartsIdx, maxRestartsIdx + 200); + expect(afterMax).toContain("mitmIsRestarting = false"); + }); +});