Skip to content

fix(security): resolve 82 CodeQL code-scanning alerts#37

Open
erseco wants to merge 6 commits into
mainfrom
hotfix/fix-codeql-warnings
Open

fix(security): resolve 82 CodeQL code-scanning alerts#37
erseco wants to merge 6 commits into
mainfrom
hotfix/fix-codeql-warnings

Conversation

@erseco

@erseco erseco commented Jun 5, 2026

Copy link
Copy Markdown
Member

Summary

Resolves 82 CodeQL "code scanning" alerts (all high/medium severity) across the backend, the frontend, the iDevice runtimes, the exporters and the legacy importers. Every fix is a minimal, behaviour-preserving change to a real source file, paired with a colocated test asserting the security property. No iDevice behaviour, export output or accepted input is altered for legitimate content.

Alerts come from the upstream code-scanning dashboard (exelearning/exelearning → Security → Code scanning). This PR targets the safe, mechanical, low-risk classes; the larger js/xss-through-dom campaign in shipped iDevice rendering is intentionally deferred (see below).

What's fixed (by rule)

Rule Fixed Fix applied
js/incomplete-multi-character-sanitization 27 Loop each tag/handler strip to a fixed point so removing one match can't splice two halves into a new <script>/on*=/scheme match
js/incomplete-sanitization 20 Add the global flag, or escape backslashes before the delimiter (e.g. escapeCatalogueKey)
js/double-escaping 11 Decode &amp; last in entity-decode chains so &amp;lt; no longer collapses to <
js/incomplete-url-substring-sanitization 7 Replace url.includes('host')/startsWith('https://host') with new URL() + exact hostname/pathname checks (rejects host.evil.com)
js/incomplete-url-scheme-check 5 Also reject vbscript:/data: (case-insensitive, whitespace/control/entity tolerant); allowlist where possible
js/bad-tag-filter 5 End-tag regex now tolerates </script > (whitespace/newline/case)
js/prototype-pollution-utility 1 Skip __proto__/prototype/constructor keys in the deep-merge
js/cross-window-information-leak 1 Drop the postMessage(msg, '*') fallback in EmbeddingBridge; log instead
js/insecure-randomness 1 Web Crypto (crypto.getRandomValues/randomUUID) instead of the non-crypto PRNG
actions/missing-workflow-permissions 1 Least-privilege permissions: block on e2e-db-matrix.yml
js/insecure-download 1 Validate/enforce a safe scheme before fetching
js/functionality-from-untrusted-source 1 https + Subresource Integrity (verified sha512 against cdnjs) for the FileSaver.js CDN tag
js/overly-large-range 1 Narrow [a-zA-z][a-zA-Z] in the mathproblems placeholder regex (both export and edition twins)

Notable details

  • Shared SVG scrubber (slide)src/sanitizer.ts and its hand-maintained export/slide.js twin: fixed-point scrubbing loop, tolerant </script\s*>, vbscript:/data:text/html removal (legit data:image/... preserved), and the <foreignObject> strip is now present in both twins.
  • LinkValidationAdapter._hasDangerousScheme — numeric-entity decoding is guarded so out-of-range code points (e.g. &#x110000;) can't throw a RangeError (regression test added).
  • common.js media URLs — Google Drive / mediateca extraction now parses the URL and matches the exact host + path; relative URLs and look-alike hosts are rejected, all legitimate links still resolve.

Deferred / not changed (deliberate)

  • 2 × js/stored-xss in LatexPreRenderer.jsskipped, documented as safe. The flow parses author HTML into an inert DOMParser document and re-serialises it; the only newly-injected markup is the library-rendered MathJax <svg>/<math> wrapper whose data-latex is attribute-escaped. Scrubbing here would delete legitimate author markup (TikZ/code iDevices, embedded widgets). A security-note comment plus round-trip tests were added.
  • 4 × js/incomplete-url-substring-sanitization located inside test bodies — skipped; these are intentional assertions/matchers over hardcoded, controlled strings (not URL-trust decisions). Rewriting them would weaken the tests for zero security benefit.
  • 146 × js/xss-through-dom in shipped iDevice rendering — out of scope for this PR. They are heterogeneous and behaviour-sensitive (jQuery $(attr) selectors, .html('<p>'+el.text()+'</p>') round-trips, trusted rendered SVG/mermaid) and warrant their own focused, E2E-backed effort.

CodeQL fixture/vendor alerts (context, no action here)

The dashboard also lists ~304 alerts inside generated export fixtures (test/fixtures/export/**) and a couple in vendored libs. These are copies of source that regenerate; they can't be fixed by editing the generated files. The repo uses CodeQL default setup, which ignores .github/codeql/codeql-config.yml, so a paths-ignore entry would not clear them. They are left untouched here (they age out as fixtures are regenerated from the fixed source, or can be dismissed in the UI).

Testing

  • make fix — clean (the single remaining Biome warning, noExplicitAny in src/routes/export.ts, pre-exists on main and is unrelated).
  • make test-unit7160 pass / 0 fail, and all files meet the 90% coverage threshold.
  • make test-frontend — green (Vitest + happy-dom).
  • make test-integration — green.
  • New colocated tests ship with every changed source file; behaviour-sensitive fixes (scheme checks, decode order, scrubbing loops, URL parsing, prototype-pollution, postMessage origin) assert both "payload blocked" and "legitimate input preserved".

Each fix was additionally checked by an independent adversarial review pass (correctness, behaviour preservation, regex backtracking, twin-sync). Two blockers it surfaced — a RangeError in the entity decoder and an out-of-sync mathproblems edition twin — are fixed in this PR.

Review follow-ups (post-review hardening)

A multi-agent review of this branch surfaced three items, now fixed in commit b53a50f:

  • js/cross-window-information-leak — residual wildcard postMessage closed. EmbeddingBridge._announceDocumentLoaded still posted DOCUMENT_LOADED (projectId, isDirty, pageCount) to this.parentOrigin || '*', so project metadata could broadcast to any embedding origin if the document loaded before the parent handshake set a trusted origin. It now routes through postToParent, inheriting the known-origin guard (dropped instead of broadcast when the origin is unknown). The pre-existing test that asserted the old '*' behaviour was updated to assert the non-leak.
  • Server/client scheme-filter parity. The server normalizeUrlForSchemeCheck (link-validator.ts) only decoded whitespace/control entities, so entity-encoded scheme characters (&#106;avascript:, javascript&#58;, data&colon;…) slipped past the broken-link checker while the client LinkValidationAdapter._hasDangerousScheme caught them. The server now decodes the full named/numeric entity set with the same out-of-range-safe code-point helper, keeping both ends in sync. New tests cover the entity-encoded scheme chars and out-of-range numeric entities.
  • Dead code removed. isUnsafeLinkScheme() in project.ts had zero call sites (superseded by the shared link-validator service's removeInvalidLinks); it and its spec block were removed so coverage isn't inflated by an unused export. The dangerous-scheme property remains covered server-side by link-validator.spec.ts.
  • mindmaps CDN tags pinned (js/functionality-from-untrusted-source). The remaining protocol-relative CDN scripts in mindmaps/src/index.html were upgraded to https; jQuery 1.6.1 now carries a verified sha384 Subresource Integrity hash + crossorigin, matching the FileSaver.js tag. Filestack's legacy loader is an unversioned/mutable endpoint, so an SRI hash cannot be pinned without breaking on its next update — it is forced to https (documented inline).

Verified after the fixes: link-validator.spec.ts + project.spec.ts (260 pass), EmbeddingBridge.test.js + LinkValidationAdapter.test.js (109 pass), Biome clean on the changed src/ files. jQuery's SRI hash was computed from the bytes served by the CDN.

One item is deferred to its own PR: consolidating the duplicated fixed-point tag-strip into a shared sanitizer. The strip is a per-iDevice method duplicated across 30+ devices spanning two runtimes (the authoring iframe and the exported student package) that share no module system, so a clean dedup needs a new cross-runtime delivery mechanism — an architectural change better made on its own, with full test runs across all gamification iDevices, rather than expanding this 90-file security PR. The current strip code is correct and well-tested as-is.

erseco added 2 commits June 5, 2026 17:40
Fix the safe, mechanical security-warning classes flagged by CodeQL across
the backend, frontend, iDevice runtimes, exporters and legacy importers.
Every change is minimal and behaviour-preserving, with a colocated test.

- double-escaping: decode &amp; last so &amp;lt; no longer collapses to <
- incomplete-(multi-character-)sanitization: loop tag/handler strips to a
  fixed point; add global flags; escape backslash before the delimiter
- bad-tag-filter: tolerant </script > end-tag regex (whitespace/newline/case)
- incomplete-url-scheme-check: also reject vbscript:/data: (entity/ws tolerant)
- incomplete-url-substring-sanitization: new URL() exact host/path checks
- prototype-pollution: guard __proto__/prototype/constructor in deep-merge
- cross-window-information-leak: drop postMessage('*') fallback
- insecure-randomness: Web Crypto instead of the non-crypto PRNG
- insecure-download / functionality-from-untrusted-source: scheme validation, SRI
- overly-large-range: narrow [a-zA-z] to [a-zA-Z] (export + edition twins)
- actions/missing-workflow-permissions: least-privilege permissions block

Guards a RangeError in the numeric-entity decoder and keeps the slide SVG
scrubber's src/export twins in sync. 2 stored-xss (inert DOMParser round-trip)
and 4 url-substring alerts inside test bodies are skipped with documented
rationale. The 146 js/xss-through-dom alerts in iDevice rendering are deferred.
Resolve conflicts in two files:
- src/routes/project.ts: drop the inline broken-link validator in favor of
  the shared SSRF-hardened link-validator service (single source of truth);
  the surrounding code already used the shared validateLink/toBrokenLinkInfo.
- electrical-circuits.test.js: keep both test suites (importGlosary HTML
  sanitization + TikZ/SVG rendering), deduplicating the loadIdevice helper
  onto the shared global.loadIdevice.
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 503 bytes (0.03%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-bundle 856.95kB 387 bytes (0.05%) ⬆️
exporters-bundle 713.77kB 116 bytes (0.02%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: app-bundle

Assets Changed:

Asset Name Size Change Total Size Change (%)
app.bundle.js 387 bytes 856.95kB 0.05%
view changes for bundle: exporters-bundle

Assets Changed:

Asset Name Size Change Total Size Change (%)
exporters.bundle.js 116 bytes 713.77kB 0.02%

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

⚠️ JUnit XML file not found

The CLI was unable to find any JUnit XML files to upload.
For more help, visit our troubleshooting guide.

erseco added 3 commits June 18, 2026 16:41
- EmbeddingBridge: route _announceDocumentLoaded through postToParent so the
  DOCUMENT_LOADED metadata (projectId, isDirty, pageCount) is no longer
  broadcast to '*' when the parent origin is unknown. Closes the residual
  js/cross-window-information-leak the postToParent fix had missed. Updated the
  pre-existing test that asserted the old wildcard behavior.
- link-validator: make the server-side normalizeUrlForSchemeCheck decode the
  full named/numeric HTML-entity set (e.g. &exelearning#106;avascript:, javascript&exelearning#58;,
  data&colon;) with an out-of-range-safe code-point helper, matching the
  client LinkValidationAdapter._hasDangerousScheme so both ends reject the same
  links. Added tests for entity-encoded scheme chars and out-of-range entities.
- project.ts: remove the now-dead isUnsafeLinkScheme() (superseded by the shared
  link-validator service; zero call sites) and its spec block.
…ted-source)

- jQuery 1.6.1 (ajax.googleapis.com): upgrade protocol-relative // to https and
  add Subresource Integrity (sha384, verified against the served bytes) plus
  crossorigin, matching the FileSaver.js tag fixed earlier in this PR.
- Filestack legacy loader: upgrade to https. Its URL is unversioned/mutable, so
  an SRI hash cannot be pinned without breaking on the next update; documented
  inline. This at least removes the protocol-downgrade vector.
@erseco erseco closed this Jul 15, 2026
@erseco erseco reopened this Jul 15, 2026
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-15 19:41 UTC

@erseco
erseco force-pushed the hotfix/fix-codeql-warnings branch 5 times, most recently from 1ed29af to 345f79c Compare July 15, 2026 20:09
Resolve legacy importer conflicts while preserving the security hardening and the latest main fixes.
@github-actions
github-actions Bot force-pushed the hotfix/fix-codeql-warnings branch from 50d5bb7 to 9f55fd0 Compare July 15, 2026 20:11
@erseco
erseco force-pushed the hotfix/fix-codeql-warnings branch 2 times, most recently from 345f79c to 9f55fd0 Compare July 15, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant