From b50a98d64ad6ef234dc9049f2b0bf351518f1796 Mon Sep 17 00:00:00 2001 From: Tanvir Farhad Date: Mon, 13 Jul 2026 12:18:27 +0100 Subject: [PATCH 1/4] fix(security): validate embed URLs by origin instead of substring match toEmbedUrl()'s fallback branch used raw.includes(...) to check for a known-safe embed URL, then returned the unvalidated input unmodified. A crafted string containing the substring anywhere could pass the check while breaking out of the iframe's src="..." attribute it gets interpolated into unescaped, injecting arbitrary markup. Validate via new URL(raw).hostname against an exact allowlist instead, and escape the result at every interpolation site as defense in depth. Also escape the two playground select-field values that were concatenated into an HTML-shaped string unescaped. Fixes #179 (CodeQL: DOM text reinterpreted as HTML x3, incomplete URL substring sanitization x1). --- .github/workflows/ci.yml | 20 ++++++- website/script.js | 22 +++++-- website/test_toEmbedUrl.mjs | 111 ++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 website/test_toEmbedUrl.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1b4a0b3..50e56eb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -551,6 +551,21 @@ jobs: - name: Build run: npm run build + website: + name: Website (script tests) + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + + - name: Set up Node.js + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: "22" + + - name: Run website script tests + run: node website/test_toEmbedUrl.mjs + # ── Enforce branch flow: main may only receive PRs from dev ─────────────── # No-op on dev PRs (step skipped -> job succeeds). To allow hotfixes straight # to main, widen the condition below to also accept a 'hotfix/*' head branch. @@ -575,7 +590,7 @@ jobs: name: CI Summary runs-on: ubuntu-latest needs: - [lint, rule-validation, secret-scan, sast-bandit, sca-pip-audit, sbom, container-scan, backend-tests, frontend, enforce-source-branch] + [lint, rule-validation, secret-scan, sast-bandit, sca-pip-audit, sbom, container-scan, backend-tests, frontend, website, enforce-source-branch] if: always() steps: - name: Build summary @@ -589,6 +604,7 @@ jobs: CONTAINER_SCAN: ${{ needs.container-scan.result }} BACKEND_TESTS: ${{ needs.backend-tests.result }} FRONTEND: ${{ needs.frontend.result }} + WEBSITE: ${{ needs.website.result }} ENFORCE_SOURCE: ${{ needs.enforce-source-branch.result }} run: | python3 - <<'PYEOF' @@ -604,6 +620,7 @@ jobs: ("Container Scan (Trivy, INFRA 1 pending)", os.environ["CONTAINER_SCAN"]), ("Backend Tests (pytest + coverage)", os.environ["BACKEND_TESTS"]), ("Frontend (lint + build)", os.environ["FRONTEND"]), + ("Website (script tests)", os.environ["WEBSITE"]), ("Enforce dev to main source", os.environ["ENFORCE_SOURCE"]), ] @@ -654,5 +671,6 @@ jobs: needs.sbom.result != 'success' || needs.backend-tests.result != 'success' || needs.frontend.result != 'success' || + needs.website.result != 'success' || needs.enforce-source-branch.result != 'success' run: exit 1 diff --git a/website/script.js b/website/script.js index 24fecfb..74ac02e 100644 --- a/website/script.js +++ b/website/script.js @@ -229,13 +229,27 @@ let selectedImageFile = null; // Base64 adds ~33% overhead, so the raw file must be under ~750 KB. const MAX_IMAGE_BYTES = 700 * 1024; +const EMBED_ALLOWED_HOSTS = new Set(['www.youtube.com', 'youtube.com', 'player.vimeo.com']); + function toEmbedUrl(raw) { if (!raw) return ''; const yt = raw.match(/(?:youtube\.com\/watch\?v=|youtu\.be\/)([a-zA-Z0-9_-]{11})/); if (yt) return `https://www.youtube.com/embed/${yt[1]}`; const vi = raw.match(/vimeo\.com\/(\d+)/); if (vi) return `https://player.vimeo.com/video/${vi[1]}`; - if (raw.includes('youtube.com/embed') || raw.includes('player.vimeo.com')) return raw; + + // Already-an-embed-URL fallback: verify the actual origin instead of a + // substring check, which a crafted string (e.g. containing + // "youtube.com/embed" anywhere but hosted elsewhere) can bypass and + // break out of the iframe's src="..." attribute when interpolated. + try { + const parsed = new URL(raw); + if (parsed.protocol === 'https:' && EMBED_ALLOWED_HOSTS.has(parsed.hostname)) { + return raw; + } + } catch { + // Not a valid absolute URL — fall through to reject below. + } return ''; } @@ -342,7 +356,7 @@ function updatePreview() { const videoRaw = document.getElementById('edit-video')?.value || ''; const embedUrl = toEmbedUrl(videoRaw); const videoHtml = embedUrl - ? `
` + ? `
` : ''; preview.textContent = ` @@ -981,7 +995,7 @@ async function runMockScan() { // Reset UI btn.disabled = true; btn.textContent = ' Running...'; - terminal.textContent = '
$ openshield scan --env ' + document.getElementById('pg-env').value + ' --pkg ' + document.getElementById('pg-framework').value + '
'; + terminal.textContent = '
$ openshield scan --env ' + escapeHTML(document.getElementById('pg-env').value) + ' --pkg ' + escapeHTML(document.getElementById('pg-framework').value) + '
'; feed.textContent = ''; scoreEl.textContent = '100'; scoreEl.className = 'text-6xl font-black text-emerald-500 transition-colors duration-500'; @@ -993,7 +1007,7 @@ async function runMockScan() { const events = [ { type: 'log', val: '[INFO] Initializing OpenShield Core v0.1.0...', delay: 400 }, - { type: 'log', val: '[INFO] Loading security modules for ' + document.getElementById('pg-framework').value.toUpperCase() + '...', delay: 600 }, + { type: 'log', val: '[INFO] Loading security modules for ' + escapeHTML(document.getElementById('pg-framework').value.toUpperCase()) + '...', delay: 600 }, { type: 'log', val: '[INFO] Authenticating with Azure Resource Manager...', delay: 800 }, { type: 'status', val: 'Status: Discovery Phase', color: 'text-blue-500' }, { type: 'log', val: '[INFO] Discovering resources in subscription \'mock-sub-123\'...', delay: 500 }, diff --git a/website/test_toEmbedUrl.mjs b/website/test_toEmbedUrl.mjs new file mode 100644 index 0000000..62274b5 --- /dev/null +++ b/website/test_toEmbedUrl.mjs @@ -0,0 +1,111 @@ +// Minimal, dependency-free test for toEmbedUrl() in script.js (issue #179). +// +// website/ is a plain static site with no build step and no existing test +// framework, so this loads the real script.js source via Node's built-in vm +// module (no duplication of the function under test) with just enough DOM +// stubbing for the file's top-level statements to execute without crashing. +// +// Run with: node website/test_toEmbedUrl.mjs + +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; +import vm from 'node:vm'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const source = readFileSync(path.join(__dirname, 'script.js'), 'utf8'); + +function stubElement() { + return { + addEventListener() {}, + classList: { contains: () => false, add() {}, remove() {}, toggle() {} }, + style: {}, + value: '', + }; +} + +const sandbox = { + window: { + addEventListener() {}, + requestAnimationFrame() {}, + location: { hash: '' }, + history: { pushState() {} }, + lucide: null, + }, + document: { + createElement: () => stubElement(), + getElementById: () => null, + querySelectorAll: () => [], + documentElement: { classList: { contains: () => false } }, + addEventListener() {}, + }, + localStorage: { + getItem: () => null, + setItem() {}, + removeItem() {}, + }, + siteContent: { blog: [], terminal: [] }, + marked: { parse: (s) => s }, + console, + URL, +}; +sandbox.window.document = sandbox.document; +vm.createContext(sandbox); +vm.runInContext(source, sandbox, { filename: 'script.js' }); + +const { toEmbedUrl } = sandbox; +assert.equal(typeof toEmbedUrl, 'function', 'toEmbedUrl must be defined at top level of script.js'); + +const cases = [ + // [input, expected output, description] + ['https://www.youtube.com/watch?v=dQw4w9WgXcQ', 'https://www.youtube.com/embed/dQw4w9WgXcQ', 'youtube watch URL'], + ['https://youtu.be/dQw4w9WgXcQ', 'https://www.youtube.com/embed/dQw4w9WgXcQ', 'youtu.be short URL'], + ['https://vimeo.com/12345678', 'https://player.vimeo.com/video/12345678', 'vimeo URL'], + ['https://www.youtube.com/embed/dQw4w9WgXcQ', 'https://www.youtube.com/embed/dQw4w9WgXcQ', 'already-embed youtube URL'], + ['https://player.vimeo.com/video/12345678', 'https://player.vimeo.com/video/12345678', 'already-embed vimeo URL'], + ['', '', 'empty input'], + [null, '', 'null input'], +]; + +const rejected = [ + // Inputs that must be rejected (return '') because they are not a genuine + // youtube.com/youtu.be/vimeo.com URL, even though some contain the + // substring "youtube.com/embed" or "player.vimeo.com" somewhere. + '">//youtube.com/embed', + 'https://evil.com/?x=youtube.com/embed', + 'https://youtube.com.evil.com/embed', + 'https://notyoutube.com/embed/xyz//player.vimeo.com', + 'javascript:alert(1)', + 'not a url at all but contains youtube.com/embed', +]; + +let failures = 0; + +for (const [input, expected, description] of cases) { + const actual = toEmbedUrl(input); + try { + assert.equal(actual, expected); + console.log(`PASS: ${description}`); + } catch { + failures++; + console.error(`FAIL: ${description} — input=${JSON.stringify(input)} got=${JSON.stringify(actual)} want=${JSON.stringify(expected)}`); + } +} + +for (const input of rejected) { + const actual = toEmbedUrl(input); + try { + assert.equal(actual, ''); + console.log(`PASS: rejects bypass attempt (${JSON.stringify(input.slice(0, 40))}...)`); + } catch { + failures++; + console.error(`FAIL: bypass NOT rejected — input=${JSON.stringify(input)} got=${JSON.stringify(actual)}`); + } +} + +if (failures > 0) { + console.error(`\n${failures} test(s) failed`); + process.exit(1); +} +console.log('\nAll toEmbedUrl tests passed'); From 38740048c98ee959e2236ddd167ba17f3575974f Mon Sep 17 00:00:00 2001 From: Tanvir Farhad Date: Mon, 13 Jul 2026 12:21:07 +0100 Subject: [PATCH 2/4] fix(security): move AI provider API key to in-memory-only storage A prior fix for this same CodeQL alert removed the localStorage.setItem call for the user's bring-your-own AI provider key but left the getter/isConfigured() reading from the now-permanently-empty key, silently breaking every AI feature (chat, summary, insights, prioritisation all gated behind isConfigured()). Store the key in a module-scoped variable instead of any browser storage. It never survives a page reload (the standard, expected trade-off for a secret that must not persist), but the feature works again and the key is never written to localStorage/sessionStorage. provider/model (not secret) still persist in localStorage as before. Fixes #180 (CodeQL: clear text storage of sensitive information). --- .github/workflows/ci.yml | 3 + frontend/src/utils/aiApi.js | 22 ++++-- frontend/src/utils/aiApi.test.mjs | 114 ++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 frontend/src/utils/aiApi.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 50e56eb..d48ce44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -548,6 +548,9 @@ jobs: - name: Lint run: npm run lint + - name: Run aiSettings tests + run: node src/utils/aiApi.test.mjs + - name: Build run: npm run build diff --git a/frontend/src/utils/aiApi.js b/frontend/src/utils/aiApi.js index d393990..e7cf108 100644 --- a/frontend/src/utils/aiApi.js +++ b/frontend/src/utils/aiApi.js @@ -15,19 +15,27 @@ const API_BASE = import.meta.env.VITE_API_URL || (import.meta.env.DEV ? 'http://localhost:5000' : 'https://openshield-api.onrender.com'); const TIMEOUT = 30000; -// ── Provider settings stored in localStorage ─────────────────────────────── +// ── Provider settings ─────────────────────────────────────────────────────── +// The API key is the user's own bring-your-own AI provider credential. It is +// kept in memory only for the life of the page — never written to +// localStorage/sessionStorage, which any script running on the page can read +// (XSS-exfiltrable) and which persists indefinitely. The trade-off is that +// the key does not survive a page reload; provider/model (not secret) still +// persist in localStorage as before. +let apiKeyInMemory = ''; + export const aiSettings = { getProvider: () => localStorage.getItem('ai_provider') || 'anthropic', - getApiKey: () => localStorage.getItem('ai_api_key') || '', + getApiKey: () => apiKeyInMemory, getModel: () => localStorage.getItem('ai_model') || '', save: ({ provider, apiKey, model }) => { - if (provider) localStorage.setItem('ai_provider', provider); - if (apiKey !== undefined) /* key storage removed */; - if (model !== undefined) localStorage.setItem('ai_model', model || ''); + if (provider) localStorage.setItem('ai_provider', provider); + if (apiKey !== undefined) apiKeyInMemory = apiKey; + if (model !== undefined) localStorage.setItem('ai_model', model || ''); }, - isConfigured: () => !!localStorage.getItem('ai_api_key'), + isConfigured: () => !!apiKeyInMemory, clear: () => { - localStorage.removeItem('ai_api_key'); + apiKeyInMemory = ''; localStorage.removeItem('ai_provider'); localStorage.removeItem('ai_model'); }, diff --git a/frontend/src/utils/aiApi.test.mjs b/frontend/src/utils/aiApi.test.mjs new file mode 100644 index 0000000..2ec9e01 --- /dev/null +++ b/frontend/src/utils/aiApi.test.mjs @@ -0,0 +1,114 @@ +// Minimal, dependency-free test for aiSettings in aiApi.js (issue #180). +// +// frontend/ has no test runner configured (only eslint + vite). aiApi.js has +// no imports and only one Vite-only construct (import.meta.env), so this +// loads the real source (no duplication of the logic under test), replaces +// that one construct with a plain string, and evaluates it with a stubbed +// localStorage — no new devDependency required. +// +// Run with: node frontend/src/utils/aiApi.test.mjs + +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +function loadAiApiModule() { + let source = readFileSync(path.join(__dirname, 'aiApi.js'), 'utf8'); + + // Neutralize the one Vite-only construct so this can run under plain Node. + source = source.replace( + /import\.meta\.env\.VITE_API_URL\s*\|\|\s*\(import\.meta\.env\.DEV \? '[^']*' : '[^']*'\)/, + "'http://localhost:5000'", + ); + assert.ok(!source.includes('import.meta'), 'failed to neutralize import.meta usage — test harness is stale'); + + // Turn `export const x = ...` into `const x = ...` and return the bindings + // via a wrapper function, so the real module body runs unmodified. + source = source.replace(/^export const /gm, 'const '); + source += '\nreturn { aiSettings, aiApi };'; + + const backingStore = new Map(); + const localStorageStub = { + getItem: (k) => (backingStore.has(k) ? backingStore.get(k) : null), + setItem: (k, v) => backingStore.set(k, String(v)), + removeItem: (k) => backingStore.delete(k), + }; + + const load = new Function('localStorage', 'fetch', 'AbortController', 'setTimeout', 'clearTimeout', source); + const noopFetch = () => Promise.reject(new Error('fetch should not be called in this test')); + const mod = load(localStorageStub, noopFetch, AbortController, setTimeout, clearTimeout); + return { ...mod, backingStore }; +} + +let failures = 0; +function check(description, fn) { + try { + fn(); + console.log(`PASS: ${description}`); + } catch (err) { + failures++; + console.error(`FAIL: ${description}\n ${err.message}`); + } +} + +// ── Fresh module per check: apiKeyInMemory is module-scoped state ────────── + +check('starts unconfigured with no key', () => { + const { aiSettings } = loadAiApiModule(); + assert.equal(aiSettings.isConfigured(), false); + assert.equal(aiSettings.getApiKey(), ''); +}); + +check('save() persists the key in memory and isConfigured() reflects it', () => { + const { aiSettings } = loadAiApiModule(); + aiSettings.save({ provider: 'anthropic', apiKey: 'sk-test-12345' }); + assert.equal(aiSettings.isConfigured(), true); + assert.equal(aiSettings.getApiKey(), 'sk-test-12345'); +}); + +check('the API key is never written to localStorage under any key', () => { + const { aiSettings, backingStore } = loadAiApiModule(); + aiSettings.save({ provider: 'groq', apiKey: 'super-secret-key-value', model: 'llama' }); + for (const storedValue of backingStore.values()) { + assert.ok( + !String(storedValue).includes('super-secret-key-value'), + `API key leaked into localStorage: ${JSON.stringify([...backingStore.entries()])}`, + ); + } +}); + +check('provider and model (non-secret) do persist in localStorage', () => { + const { aiSettings, backingStore } = loadAiApiModule(); + aiSettings.save({ provider: 'gemini', apiKey: 'irrelevant', model: 'gemini-pro' }); + assert.equal(backingStore.get('ai_provider'), 'gemini'); + assert.equal(backingStore.get('ai_model'), 'gemini-pro'); +}); + +check('clear() removes the in-memory key and localStorage entries', () => { + const { aiSettings, backingStore } = loadAiApiModule(); + aiSettings.save({ provider: 'anthropic', apiKey: 'to-be-cleared' }); + assert.equal(aiSettings.isConfigured(), true); + aiSettings.clear(); + assert.equal(aiSettings.isConfigured(), false); + assert.equal(aiSettings.getApiKey(), ''); + assert.equal(backingStore.has('ai_provider'), false); + assert.equal(backingStore.has('ai_model'), false); +}); + +check('a fresh module load never sees a key from a previous instance (no shared storage)', () => { + const first = loadAiApiModule(); + first.aiSettings.save({ provider: 'anthropic', apiKey: 'first-instance-key' }); + + const second = loadAiApiModule(); + assert.equal(second.aiSettings.isConfigured(), false); + assert.equal(second.aiSettings.getApiKey(), ''); +}); + +if (failures > 0) { + console.error(`\n${failures} test(s) failed`); + process.exit(1); +} +console.log('\nAll aiSettings tests passed'); From 15f39b199530943c48919c5d26063ec3b27fd0c9 Mon Sep 17 00:00:00 2001 From: Tanvir Farhad Date: Mon, 13 Jul 2026 12:32:32 +0100 Subject: [PATCH 3/4] fix(security): stop leaking exception details in API error responses Every affected route returned str(exc) (or an f-string embedding it) directly in the JSON response body. Some catch broad `except Exception`, which can surface raw database driver errors, connection details, or internal paths to the client rather than just intentional validation messages. Replace with a fixed, generic message per error class; the existing logger.error(...)/logger.warning(...) calls already capture full detail server-side. In api/routes/ai.py, extract a shared _ai_error_response() helper since the same 3-exception pattern (VectorStoreNotBuilt, ValueError, RuntimeError from get_completion()) repeated verbatim across 4 routes, and add the server-side logging that was missing there entirely. Also fixes an f-string leak in compliance.py's FileNotFoundError handler that the "detail" key search missed. Fixes #177 (CodeQL: information exposure through an exception, all 27 findings across api/app.py and 8 api/routes/ modules). --- api/app.py | 3 +- api/routes/ai.py | 43 ++++++++--- api/routes/compliance.py | 5 +- api/routes/drift.py | 2 +- api/routes/findings.py | 6 +- api/routes/prioritization.py | 2 +- api/routes/resources.py | 2 +- api/routes/scans.py | 10 +-- api/routes/score.py | 4 +- tests/test_ai_error_exposure.py | 79 ++++++++++++++++++++ tests/test_error_exposure.py | 127 ++++++++++++++++++++++++++++++++ 11 files changed, 255 insertions(+), 28 deletions(-) create mode 100644 tests/test_ai_error_exposure.py create mode 100644 tests/test_error_exposure.py diff --git a/api/app.py b/api/app.py index 738826a..34f5163 100644 --- a/api/app.py +++ b/api/app.py @@ -244,7 +244,8 @@ def ready(): @app.errorhandler(400) def bad_request(exc): - return jsonify({"error": "Bad request", "detail": str(exc), "request_id": get_request_id()}), 400 + logger.warning("Bad request: %s", exc) + return jsonify({"error": "Bad request", "request_id": get_request_id()}), 400 @app.errorhandler(401) def unauthorized(exc): diff --git a/api/routes/ai.py b/api/routes/ai.py index 5059ea4..776796a 100644 --- a/api/routes/ai.py +++ b/api/routes/ai.py @@ -158,6 +158,25 @@ def _read_request(): return body, None +_AI_ERROR_MESSAGES = { + 503: "AI knowledge base is not available", + 400: "Invalid AI request parameters", + 502: "AI provider request failed", +} + + +def _ai_error_response(exc: Exception, status: int, log_context: str): + """Return a safe, generic error response, logging the real exception server-side. + + Never reflect str(exc) back to the client: get_completion() wraps + third-party provider errors (e.g. "Anthropic request failed: {exc}"), + which could carry response bodies or connection details from the + underlying HTTP client that shouldn't reach the caller. + """ + logger.error("%s: %s", log_context, exc) + return jsonify({"error": _AI_ERROR_MESSAGES.get(status, "AI request failed")}), status + + @ai_bp.post("/api/ai/insights") @rate_limit(_AI_RATE_LIMIT) def insights(): @@ -223,7 +242,7 @@ def ai_summary(): try: context, sources = _context_for(findings_text) except VectorStoreNotBuilt as exc: - return jsonify({"error": str(exc)}), 503 + return _ai_error_response(exc, 503, "Vector store unavailable in ai_summary") prompt = ( "You are a cloud security advisor. Using ONLY the grounded knowledge " @@ -234,9 +253,9 @@ def ai_summary(): try: answer = get_completion(body["provider"], body["api_key"], prompt, model=body.get("model")) except ValueError as exc: - return jsonify({"error": str(exc)}), 400 + return _ai_error_response(exc, 400, "Invalid request in ai_summary") except RuntimeError as exc: - return jsonify({"error": str(exc)}), 502 + return _ai_error_response(exc, 502, "Provider failure in ai_summary") return jsonify( { @@ -262,7 +281,7 @@ def ai_prioritise(): try: context, sources = _context_for(findings_text) except VectorStoreNotBuilt as exc: - return jsonify({"error": str(exc)}), 503 + return _ai_error_response(exc, 503, "Vector store unavailable in ai_prioritise") prompt = ( "You are a cloud security advisor. Using ONLY the grounded knowledge " @@ -275,9 +294,9 @@ def ai_prioritise(): try: raw = get_completion(body["provider"], body["api_key"], prompt, model=body.get("model")) except ValueError as exc: - return jsonify({"error": str(exc)}), 400 + return _ai_error_response(exc, 400, "Invalid request in ai_prioritise") except RuntimeError as exc: - return jsonify({"error": str(exc)}), 502 + return _ai_error_response(exc, 502, "Provider failure in ai_prioritise") try: prioritised = json.loads(raw) @@ -307,7 +326,7 @@ def ai_ask(): try: context, sources = _context_for(question) except VectorStoreNotBuilt as exc: - return jsonify({"error": str(exc)}), 503 + return _ai_error_response(exc, 503, "Vector store unavailable in ai_ask") findings = body.get("findings", []) findings_text = _findings_to_text(findings) if findings else "Not provided." @@ -322,9 +341,9 @@ def ai_ask(): try: answer = get_completion(body["provider"], body["api_key"], prompt, model=body.get("model")) except ValueError as exc: - return jsonify({"error": str(exc)}), 400 + return _ai_error_response(exc, 400, "Invalid request in ai_ask") except RuntimeError as exc: - return jsonify({"error": str(exc)}), 502 + return _ai_error_response(exc, 502, "Provider failure in ai_ask") return jsonify( { @@ -352,15 +371,15 @@ def ai_threat_simulation(): try: context, sources = _context_for(findings_text) except VectorStoreNotBuilt as exc: - return jsonify({"error": str(exc)}), 503 + return _ai_error_response(exc, 503, "Vector store unavailable in ai_threat_simulation") prompt = _build_threat_simulation_prompt(findings_text, context) try: raw = get_completion(body["provider"], body["api_key"], prompt, model=body.get("model")) except ValueError as exc: - return jsonify({"error": str(exc)}), 400 + return _ai_error_response(exc, 400, "Invalid request in ai_threat_simulation") except RuntimeError as exc: - return jsonify({"error": str(exc)}), 502 + return _ai_error_response(exc, 502, "Provider failure in ai_threat_simulation") try: simulation = json.loads(raw) diff --git a/api/routes/compliance.py b/api/routes/compliance.py index 45dfef8..df65bdf 100644 --- a/api/routes/compliance.py +++ b/api/routes/compliance.py @@ -47,7 +47,8 @@ def get_compliance(framework: str): return jsonify(result) except FileNotFoundError as exc: - return jsonify({"error": f"Frameworks directory not found: {exc}"}), 500 + logger.error("Frameworks directory not found: %s", exc) + return jsonify({"error": "Compliance frameworks are not available"}), 500 except Exception as exc: logger.error("Failed to retrieve compliance score for %s: %s", framework, exc) - return jsonify({"error": "Compliance calculation failed", "detail": str(exc)}), 500 + return jsonify({"error": "Compliance calculation failed"}), 500 diff --git a/api/routes/drift.py b/api/routes/drift.py index 3ed61f4..427fa3d 100644 --- a/api/routes/drift.py +++ b/api/routes/drift.py @@ -147,4 +147,4 @@ def _rg(resource_id: str) -> str: except Exception as exc: logger.error("Failed to compute drift: %s", exc) - return jsonify({"error": "Failed to retrieve drift", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve drift"}), 500 diff --git a/api/routes/findings.py b/api/routes/findings.py index 3a4939b..91f6afb 100644 --- a/api/routes/findings.py +++ b/api/routes/findings.py @@ -43,7 +43,7 @@ def list_findings(): return jsonify({"count": len(findings), "findings": findings}) except Exception as exc: logger.error("Failed to list findings: %s", exc) - return jsonify({"error": "Failed to retrieve findings", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve findings"}), 500 @findings_bp.get("/api/findings/") @@ -57,7 +57,7 @@ def get_finding(finding_id: int): return jsonify(finding) except Exception as exc: logger.error("Failed to get finding %d: %s", finding_id, exc) - return jsonify({"error": "Database error", "detail": str(exc)}), 500 + return jsonify({"error": "Database error"}), 500 @findings_bp.get("/api/findings//playbook") @@ -126,4 +126,4 @@ def get_playbook(finding_id: int): except Exception as exc: logger.error("Failed to get playbook for finding %d: %s", finding_id, exc) - return jsonify({"error": "Failed to retrieve playbook", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve playbook"}), 500 diff --git a/api/routes/prioritization.py b/api/routes/prioritization.py index 3353252..d19fe3d 100644 --- a/api/routes/prioritization.py +++ b/api/routes/prioritization.py @@ -180,4 +180,4 @@ def get_prioritization(): except Exception as exc: logger.error("Failed to build prioritization: %s", exc) - return jsonify({"error": "Failed to retrieve prioritization", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve prioritization"}), 500 diff --git a/api/routes/resources.py b/api/routes/resources.py index ad0a4db..56a0309 100644 --- a/api/routes/resources.py +++ b/api/routes/resources.py @@ -121,4 +121,4 @@ def get_resources(): except Exception as exc: logger.error("Failed to build resources: %s", exc) - return jsonify({"error": "Failed to retrieve resources", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve resources"}), 500 diff --git a/api/routes/scans.py b/api/routes/scans.py index c87914a..1d2f275 100644 --- a/api/routes/scans.py +++ b/api/routes/scans.py @@ -32,7 +32,7 @@ def list_scans(): return jsonify(result) except Exception as exc: logger.error("Failed to list scans: %s", exc) - return jsonify({"error": "Failed to retrieve scans", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to retrieve scans"}), 500 @scans_bp.get("/api/scans/") @@ -46,7 +46,7 @@ def get_scan_status(scan_id): return jsonify(scan) except Exception as exc: logger.error("Failed to get scan status: %s", exc) - return jsonify({"error": "Database error", "detail": str(exc)}), 500 + return jsonify({"error": "Database error"}), 500 @scans_bp.post("/api/scans/trigger") @@ -73,7 +73,7 @@ def trigger_scan(): db.create_pending_scan(scan_id, subscription_id) except Exception as exc: logger.error("Failed to create pending scan: %s", exc, exc_info=True) - return jsonify({"error": "Database error", "detail": str(exc)}), 500 + return jsonify({"error": "Database error"}), 500 return jsonify( {"scan_id": scan_id, "status": "pending", "message": "Scan has been queued and will start shortly."} @@ -81,7 +81,7 @@ def trigger_scan(): except Exception as exc: logger.error("Critical error in trigger_scan route: %s", exc, exc_info=True) - return jsonify({"error": "Critical route failure", "detail": str(exc)}), 500 + return jsonify({"error": "Critical route failure"}), 500 def _run_enrichment_in_background(scan_id: str, findings: list, db_url: str) -> None: @@ -162,4 +162,4 @@ def enrich_scan(scan_id): except Exception as exc: logger.error("Failed to start enrichment for scan %s: %s", scan_id, exc) - return jsonify({"error": "Internal server error", "detail": str(exc)}), 500 + return jsonify({"error": "Internal server error"}), 500 diff --git a/api/routes/score.py b/api/routes/score.py index 9d0e125..22d157d 100644 --- a/api/routes/score.py +++ b/api/routes/score.py @@ -34,7 +34,7 @@ def get_score(): return jsonify(result) except Exception as exc: logger.error("Failed to calculate score: %s", exc) - return jsonify({"error": "Failed to calculate score", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to calculate score"}), 500 @score_bp.get("/api/score/cve-summary") @@ -46,4 +46,4 @@ def get_cve_summary(): return jsonify(result) except Exception as exc: logger.error("Failed to fetch CVE summary: %s", exc) - return jsonify({"error": "Failed to fetch CVE summary", "detail": str(exc)}), 500 + return jsonify({"error": "Failed to fetch CVE summary"}), 500 diff --git a/tests/test_ai_error_exposure.py b/tests/test_ai_error_exposure.py new file mode 100644 index 0000000..781dd76 --- /dev/null +++ b/tests/test_ai_error_exposure.py @@ -0,0 +1,79 @@ +"""Tests that /api/ai/{summary,prioritise,ask,threat-simulation} never leak +exception internals to the client (issue #177, CodeQL: information exposure +through an exception). + +get_completion() in api/services/ai_provider.py wraps raw third-party +provider errors into RuntimeError messages (e.g. "Anthropic request failed: +{exc}"), which could carry response bodies or connection details from the +underlying HTTP client. These routes must never reflect that (or any other +exception's) message back to the caller. +""" + +import secrets +from unittest.mock import patch + +from ai.retriever import VectorStoreNotBuilt + +_LEAK_MARKER = "provider-internal-secret-token-abc123" + +_VALID_FINDINGS = [ + { + "rule_id": "AZ-NET-001", + "severity": "HIGH", + "title": "Example finding", + "description": "Example description.", + "remediation": "Example remediation.", + } +] + + +def _fake_api_key() -> str: + return secrets.token_urlsafe(24) + + +def _assert_no_leak(resp, expected_status): + assert resp.status_code == expected_status + body = resp.get_json() + assert _LEAK_MARKER not in str(body) + + +ENDPOINTS = { + "/api/ai/summary": {"provider": "anthropic", "api_key": None, "findings": _VALID_FINDINGS}, + "/api/ai/prioritise": {"provider": "anthropic", "api_key": None, "findings": _VALID_FINDINGS}, + "/api/ai/ask": {"provider": "anthropic", "api_key": None, "question": "What is my risk?"}, + "/api/ai/threat-simulation": {"provider": "anthropic", "api_key": None, "findings": _VALID_FINDINGS}, +} + + +def _payload(endpoint): + body = dict(ENDPOINTS[endpoint]) + body["api_key"] = _fake_api_key() + return body + + +@patch("api.routes.ai._context_for") +def test_vector_store_not_built_does_not_leak_exception(mock_context, client, auth_headers): + mock_context.side_effect = VectorStoreNotBuilt(_LEAK_MARKER) + for endpoint in ENDPOINTS: + resp = client.post(endpoint, json=_payload(endpoint), headers=auth_headers) + _assert_no_leak(resp, 503) + + +@patch("api.routes.ai.get_completion") +@patch("api.routes.ai._context_for") +def test_provider_value_error_does_not_leak_exception(mock_context, mock_gc, client, auth_headers): + mock_context.return_value = ("grounded context", []) + mock_gc.side_effect = ValueError(_LEAK_MARKER) + for endpoint in ENDPOINTS: + resp = client.post(endpoint, json=_payload(endpoint), headers=auth_headers) + _assert_no_leak(resp, 400) + + +@patch("api.routes.ai.get_completion") +@patch("api.routes.ai._context_for") +def test_provider_runtime_error_does_not_leak_exception(mock_context, mock_gc, client, auth_headers): + mock_context.return_value = ("grounded context", []) + mock_gc.side_effect = RuntimeError(f"Anthropic request failed: {_LEAK_MARKER}") + for endpoint in ENDPOINTS: + resp = client.post(endpoint, json=_payload(endpoint), headers=auth_headers) + _assert_no_leak(resp, 502) diff --git a/tests/test_error_exposure.py b/tests/test_error_exposure.py new file mode 100644 index 0000000..2322c78 --- /dev/null +++ b/tests/test_error_exposure.py @@ -0,0 +1,127 @@ +"""Tests that API error responses never leak exception internals (issue #177, +CodeQL: information exposure through an exception). + +Each route's _get_db() (or equivalent) is patched to raise, mirroring the +established per-module DB-mocking convention (see test_findings_playbook.py). +A secret-shaped marker string is used as the exception message so a leak +would be unambiguous rather than coincidentally matching real wording. +""" + +from unittest.mock import MagicMock, patch + +import api.routes.compliance as compliance_route +import api.routes.drift as drift_route +import api.routes.findings as findings_route +import api.routes.prioritization as prioritization_route +import api.routes.resources as resources_route +import api.routes.scans as scans_route +import api.routes.score as score_route + +_LEAK_MARKER = "connection to db-internal-host-secret-1234 refused" + + +def _raising_db(method_name): + db = MagicMock() + getattr(db, method_name).side_effect = RuntimeError(_LEAK_MARKER) + return db + + +def _assert_no_leak(resp, expected_status): + assert resp.status_code == expected_status + body = resp.get_json() + assert "detail" not in body + assert _LEAK_MARKER not in str(body) + + +def test_list_scans_error_does_not_leak_exception(client, auth_headers): + with patch.object(scans_route, "_get_db", return_value=_raising_db("get_scans")): + resp = client.get("/api/scans", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_get_scan_status_error_does_not_leak_exception(client, auth_headers): + with patch.object(scans_route, "_get_db", return_value=_raising_db("get_scan")): + resp = client.get("/api/scans/some-id", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_trigger_scan_db_error_does_not_leak_exception(client, auth_headers): + with patch.object(scans_route, "_get_db", return_value=_raising_db("create_pending_scan")): + resp = client.post( + "/api/scans/trigger", + json={"subscription_id": "00000000-0000-0000-0000-000000000001"}, + headers=auth_headers, + ) + _assert_no_leak(resp, 500) + + +def test_list_findings_error_does_not_leak_exception(client, auth_headers): + with patch.object(findings_route, "_get_db", return_value=_raising_db("get_findings")): + resp = client.get("/api/findings", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_get_finding_error_does_not_leak_exception(client, auth_headers): + with patch.object(findings_route, "_get_db", return_value=_raising_db("get_finding_by_id")): + resp = client.get("/api/findings/1", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_get_playbook_error_does_not_leak_exception(client, auth_headers): + with patch.object(findings_route, "_get_db", return_value=_raising_db("get_finding_by_id")): + resp = client.get("/api/findings/1/playbook", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_score_error_does_not_leak_exception(client, auth_headers): + with patch.object(score_route, "_get_db", return_value=_raising_db("get_score")): + resp = client.get("/api/score", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_cve_summary_error_does_not_leak_exception(client, auth_headers): + with patch.object(score_route, "_get_db", return_value=_raising_db("get_cve_summary")): + resp = client.get("/api/score/cve-summary", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_resources_error_does_not_leak_exception(client, auth_headers): + with patch.object(resources_route, "_get_db", return_value=_raising_db("get_findings")): + resp = client.get("/api/resources", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_prioritization_error_does_not_leak_exception(client, auth_headers): + with patch.object(prioritization_route, "_get_db", return_value=_raising_db("get_findings")): + resp = client.get("/api/prioritization", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_drift_error_does_not_leak_exception(client, auth_headers): + with patch.object(drift_route, "_get_db", return_value=_raising_db("get_findings")): + resp = client.get("/api/drift", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_compliance_generic_error_does_not_leak_exception(client, auth_headers): + with patch.object(compliance_route, "_get_db", return_value=_raising_db("get_compliance_score")): + resp = client.get("/api/compliance/cis", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_compliance_missing_frameworks_dir_does_not_leak_exception(client, auth_headers): + db = MagicMock() + db.get_compliance_score.side_effect = FileNotFoundError(_LEAK_MARKER) + with patch.object(compliance_route, "_get_db", return_value=db): + resp = client.get("/api/compliance/cis", headers=auth_headers) + _assert_no_leak(resp, 500) + + +def test_bad_request_errorhandler_does_not_leak_exception(client, auth_headers): + resp = client.post( + "/api/scans/trigger", + data=b"{not valid json", + headers={**auth_headers, "Content-Type": "application/json"}, + ) + body = resp.get_json() + assert "detail" not in body From 054fafdc35dbb4fbe52d11bd475c06ebb4c79821 Mon Sep 17 00:00:00 2001 From: Tanvir Farhad Date: Mon, 13 Jul 2026 12:39:04 +0100 Subject: [PATCH 4/4] fix: remove escapeHTML() misapplied to .textContent sinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review caught that the defense-in-depth escapeHTML() calls added for #179 wrapped values assigned via .textContent, not .innerHTML. .textContent never interprets HTML, so escaping there adds no security value while risking visible text corruption (a literal "&" shown on screen) if the underlying value ever contains a special character. toEmbedUrl()'s URL-origin validation is the real, load-bearing fix and is unaffected by this change — it guarantees embedUrl can only ever be '' or a well-formed https://youtube.com|vimeo.com URL regardless of which DOM property ultimately consumes it. --- website/script.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/website/script.js b/website/script.js index 74ac02e..3c08ba9 100644 --- a/website/script.js +++ b/website/script.js @@ -356,7 +356,7 @@ function updatePreview() { const videoRaw = document.getElementById('edit-video')?.value || ''; const embedUrl = toEmbedUrl(videoRaw); const videoHtml = embedUrl - ? `
` + ? `
` : ''; preview.textContent = ` @@ -995,7 +995,7 @@ async function runMockScan() { // Reset UI btn.disabled = true; btn.textContent = ' Running...'; - terminal.textContent = '
$ openshield scan --env ' + escapeHTML(document.getElementById('pg-env').value) + ' --pkg ' + escapeHTML(document.getElementById('pg-framework').value) + '
'; + terminal.textContent = '
$ openshield scan --env ' + document.getElementById('pg-env').value + ' --pkg ' + document.getElementById('pg-framework').value + '
'; feed.textContent = ''; scoreEl.textContent = '100'; scoreEl.className = 'text-6xl font-black text-emerald-500 transition-colors duration-500'; @@ -1007,7 +1007,7 @@ async function runMockScan() { const events = [ { type: 'log', val: '[INFO] Initializing OpenShield Core v0.1.0...', delay: 400 }, - { type: 'log', val: '[INFO] Loading security modules for ' + escapeHTML(document.getElementById('pg-framework').value.toUpperCase()) + '...', delay: 600 }, + { type: 'log', val: '[INFO] Loading security modules for ' + document.getElementById('pg-framework').value.toUpperCase() + '...', delay: 600 }, { type: 'log', val: '[INFO] Authenticating with Azure Resource Manager...', delay: 800 }, { type: 'status', val: 'Status: Discovery Phase', color: 'text-blue-500' }, { type: 'log', val: '[INFO] Discovering resources in subscription \'mock-sub-123\'...', delay: 500 },