Skip to content

security: resolve 17 CodeQL alerts (XSS / attribute-sanitization / insecure-randomness / shell-quote)#49

Merged
tbitcs merged 1 commit intodevelopfrom
fix/codeql-alerts
May 4, 2026
Merged

security: resolve 17 CodeQL alerts (XSS / attribute-sanitization / insecure-randomness / shell-quote)#49
tbitcs merged 1 commit intodevelopfrom
fix/codeql-alerts

Conversation

@tbitcs
Copy link
Copy Markdown
Contributor

@tbitcs tbitcs commented May 4, 2026

What

Closes the 17 open CodeQL code-scanning alerts on
BitConcepts/specsmith-vscode develop so we hit zero open alerts
across the org (CLI is already at zero). No user-visible behaviour
changes — this is a defence-in-depth pass.

Rule Severity Where Fix
js/insecure-randomness ×2 warning src/ChatStreamConsumer.ts:155, 232 generateSessionId() uses crypto.randomUUID() instead of Math.random().
js/xss ×5 (error) error media/session.js:20, 25, 78, 298, 510 Wrapped LLM-controlled values (customTs, lbl, args.path hint, addImg src, vcs additions/deletions) through esc() / numeric coercion.
js/incomplete-html-attribute-sanitization ×5 warning media/session.js:104, 166, 172, 218, 299 esc() now escapes " and ' too. The Report Bug / View Full / Crash buttons no longer use inline onclick="rptTool(this,'X','Y')"; they switched to data-action/data-* attributes plus a single delegated click listener, removing the brittle JS-string smuggling.
js/identity-replacement ×1 warning media/session.js:172 Same data-attribute refactor as above eliminates the cleanR.replace(/'/g,"\\'") pair the linter flagged.
js/incomplete-sanitization ×2 warning src/extension.ts:584 (preflight), src/GovernancePanel.ts:528 (agent task) Shell-quote helpers now escape \ before " so a trailing backslash can no longer escape the closing quote.
js/bad-tag-filter ×2 warning src/test/session-logic.test.ts:238, 249 Build-output integrity tests use /<script\b[^>]*>...<\/script\s*>/gi instead of the lower-case-only literal regex.

Why

The 17 alerts came from CodeQL's default JS query suite. The XSS class
is the highest-priority subset — when the chat panel rendered an
agent-controlled customTs, args.path, image data URL, or vcs diff
counts, a hostile LLM response could in theory smuggle markup into the
webview. The shell-quote alerts in extension.ts and
GovernancePanel.ts could likewise be exploited by a hostile utterance
that ends in \. The insecure-randomness alerts on session ids are
mostly cosmetic (they're not used for auth) but crypto.randomUUID()
is a one-line drop-in.

How

  • media/session.js

    • esc() now also escapes " and ' so it is safe in attribute
      context, not just element-text context.
    • addU / addA escape customTs (history-replay timestamps).
    • addTStart escapes lbl and the args.path hint branch.
    • addImg escapes the u (data:/file path) before <img src=…>.
    • vcs_state additions/deletions go through Number(x)|0 before
      HTML concatenation.
    • Report Bug / View Full / Crash buttons rebuilt with
      data-action="rptTool" / data-action="rptCrash" /
      data-action="viewFull" etc., wired through one
      document.addEventListener('click', …) handler at the bottom of
      the file. The brittle cleanR.replace(/'/g,"\\'") is gone.
  • src/ChatStreamConsumer.tsgenerateSessionId() now does
    'sess_' + crypto.randomUUID().replace(/-/g, '').slice(0, 12).

  • src/extension.ts (specsmith.runPreflight) and
    src/GovernancePanel.ts (case 'agentTask') — escape
    backslashes before double quotes when building the shell argument.

  • src/test/session-logic.test.ts — case-insensitive <script>
    regex with explicit \b and \s*> boundaries.

  • CHANGELOG.md — added a ### Security bullet under
    [Unreleased] describing the fix.

Verification

  • npm run lint (tsc --noEmit): clean.
  • npm run build (esbuild): 290.9 KB bundle, no errors.
  • npm test: 144 passing (no test changes, no regressions).
  • After this lands, gh api /repos/BitConcepts/specsmith-vscode/code-scanning/alerts?state=open should re-scan and return length=0 once the CodeQL workflow finishes.

Risk

Low. The biggest behavioural delta is the strengthened esc() — it now
double-encodes " and ' in attribute strings, which the browser
decodes back transparently. The
data-attribute click handlers are wired via the same vscode.postMessage
calls as before.


Co-Authored-By: Oz oz-agent@warp.dev

…secure-randomness / shell-quote)

This is a defence-in-depth pass that closes every open CodeQL finding
on the develop branch. No user-visible behaviour changes.

media/session.js
- esc() now also escapes " and ' so it is safe to use in HTML attribute
  context, not just element text. This fixes the underlying cause of
  the five js/incomplete-html-attribute-sanitization alerts at
  lines 104, 166, 172, 218, 299.
- addU / addA: pass customTs (history-replay timestamps) through esc()
  before interpolating into innerHTML (was bare).
- addTStart: escape lbl and the args.path branch of hint (was bare).
  These were the user-controlled values flagged by js/xss at line 78.
- addImg: escape u (the data: URL / file path) before interpolating
  into <img src=...>. Closes the js/xss alert at line 298.
- vcs_state additions/deletions span: coerce to integers via
  Number(x)|0 before HTML concat; closes the js/xss alert at line 510.
- The Report Bug + View Full + Crash buttons used inline
  onclick="rptTool(this,'X','Y')" with brittle JS-string escaping
  via cleanR.replace(/'/g,"\\'") -> CodeQL flagged the regex pair as
  js/identity-replacement and the surrounding template as
  js/incomplete-html-attribute-sanitization. Replaced with
  data-action / data-tool / data-output / data-text / data-title /
  data-detail / data-repo attributes (HTML-escaped via esc()) plus
  one delegated document.addEventListener('click',...) handler that
  routes by btn.dataset.action. The brittle regex is gone; values
  cross the JS boundary as plain DOM attributes.

src/ChatStreamConsumer.ts
- generateSessionId() now uses crypto.randomUUID() instead of
  Math.random(); the 12-char hex slice is preserved so log lines
  remain readable. Closes both js/insecure-randomness alerts.

src/extension.ts (specsmith.runPreflight) and src/GovernancePanel.ts
(case 'agentTask')
- The double-quoted shell argument was previously built with
  utterance.replace(/"/g,'\\"') which leaves a trailing backslash able
  to escape the closing quote. Now backslashes are escaped first, then
  double quotes. Closes both js/incomplete-sanitization alerts.

src/test/session-logic.test.ts
- The two integrity tests that scan the bundle for stray escaped
  backticks used /<script>...<\/script>/g which CodeQL flags as
  js/bad-tag-filter because it does not match upper-case <SCRIPT>.
  Switched to /<script\b[^>]*>...<\/script\s*>/gi. We never emit
  uppercase tags, so behaviour is unchanged; the alert is closed.

Verification
- npm run lint: clean (tsc --noEmit).
- npm run build: 290.9 KB bundle.
- npm test: 144 passing.

Co-Authored-By: Oz <oz-agent@warp.dev>
@tbitcs tbitcs merged commit 6fd9219 into develop May 4, 2026
3 checks passed
@tbitcs tbitcs deleted the fix/codeql-alerts branch May 4, 2026 13:19
tbitcs added a commit that referenced this pull request May 4, 2026
Bumps `package.json` from 0.10.0 to 0.10.1 and renames the existing
`[Unreleased]` CHANGELOG section to `[0.10.1] - 2026-05-04`. The
0.10.0 tag captured the multi-agent + BYOE work (PRs #45/#47/#48);
this point release rolls in the security hardening from #49 (17
CodeQL alerts closed) and the regex follow-up.

Validation:
- npm run lint: clean.
- npm test: 144 passing.

Co-Authored-By: Oz <oz-agent@warp.dev>
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