Skip to content

fix(security): sandbox webview JS and validate openExternal URL (F-01, F-02) - #16790

Open
xxzzzzy wants to merge 3 commits into
Scottcjn:mainfrom
xxzzzzy:fix/vscode-F01-vscode-browser-xss
Open

fix(security): sandbox webview JS and validate openExternal URL (F-01, F-02)#16790
xxzzzzy wants to merge 3 commits into
Scottcjn:mainfrom
xxzzzzy:fix/vscode-F01-vscode-browser-xss

Conversation

@xxzzzzy

@xxzzzzy xxzzzzy commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fix: sandbox webview JS and validate openExternal URL

What this PR fixes

Three confirmed findings from the bug-hunter scan of
vscode-extension (F-01, F-02, F-03).

F-01 — Medium: XSS via unescaped single-quote in webview onclick

escapeHtml escaped & < > " but not '. The buildHtml wrapped
escapeHtml(issue.html_url) in a single-quoted JS string inside an
onclick HTML attribute. A ' in the URL closes the JS string,
allowing arbitrary script injection in the webview context.

F-02 — Medium: vscode.env.openExternal accepts any URI scheme

The openIssue handler did not validate the URL scheme. Combined
with F-01, an attacker controlling a GitHub issue page (DNS/API
compromise) could redirect via the legit html_url to vscode://
or javascript: schemes.

F-03 — Low: claimBounty does not enforce numeric issueNumber

The TS type was issueNumber?: number, but postMessage is JSON.
A string passed the truthiness check and got interpolated into the
PR compare URL via template literal.

Changes

  • escapeHtml now also escapes ' (F-01, defence-in-depth).
  • New escapeJsString(s) = JSON.stringify(s) helper used at the
    view-issue onclick so the URL goes through full JSON escaping
    before being interpolated (F-01, load-bearing fix).
  • openIssue handler now only follows https: URLs and silently
    drops malformed values (F-02).
  • claimBounty requires Number.isInteger(message.issueNumber)
    (F-03).
  • A strict Content-Security-Policy meta tag is added to the
    webview HTML (default-src '\''none'\'', script-src '\''unsafe-inline'\'',
    style-src '\''unsafe-inline'\''). The webview host does not
    expose a CSP by default, so this prevents any future XSS from
    reaching the network.

Risk

Low. Existing well-formed GitHub URLs render unchanged. Malformed
inputs are now rejected silently rather than passed through.

Suggested bounty tag

security, xss, vscode-extension

bounty-bot added 2 commits September 1, 2026 20:43
…back

hardware/system.rs
  get_mac_addresses() used to substitute a hard-coded
  '00:00:00:00:00:00' when no NIC interface could be enumerated.
  That value is *not* in any VM MAC-OUI prefix list, so it both
  silently passed the anti-emulation check *and* collided the
  identities of every sandbox / container / restricted node into
  one shared fingerprint inside the attestation payload.

  Return an empty Vec when no MAC can be read; the fingerprint
  check now treats that as a suspicious indicator instead of
  passing silently.

hardware/cpu.rs
  Repair a syntax error at the end of get_cpu_serial(): a stray
  'rand string' literal was glued to the format!() call, leaving
  the function un-compilable. The hash-based fallback is rebuilt
  on top of the already-declared sha2 dependency.

Verified with cargo check (clean build, only pre-existing
dead_code warning unrelated to this patch).

Refs: C-3 (MAC 00:00 fallback), C-1 (cpu.rs syntax error)
bounty-hunter-testautomaton/implementer.py
  - Add _safe_path() validator that rejects absolute paths, parent
    traversal (../), backslash variants, Windows drive letters,
    control characters, symlinks, and any resolved path that
    escapes workdir. The validator enforces an allow-list character
    set [A-Za-z0-9._/+-] and a 200-byte name length cap.
  - Use the validator in fork_and_implement before every write and
    every 'git add', so a path that does pass the allow-list but
    resolves outside workdir is still caught.
  - Cap each written file at 200 KB to limit blast radius if the
    model emits garbage.
  - Wrap the untrusted bounty body in <bounty_data> delimiters in
    the Claude prompt and add an explicit refusal rule that names
    the specific attack classes (prompt-injection, format-change,
    system-prompt exfiltration). The validator above is the load-
    bearing fix; the prompt change is defence-in-depth.

bounty-hunter-testautomaton/submitter.py
  - submit_pr() now accepts a used_claude flag and renders an
    honest PR body that describes the actual generation method.
    When the template fallback was used the body explicitly says
    'template stub, no ANTHROPIC_API_KEY available' so a reviewer
    cannot mistake a stub PR for a Claude-generated one.

bounty-hunter-testautomaton/agent.py
  - Propagate the used_claude flag from fork_and_implement through
    to submit_pr so the honest-body branch is actually taken.

Fixes:
- R-01 (Critical, prompt-injection -> path-traversal -> git push)
- R-03 (High, misleading PR body when template fallback used)
- R-04 (High, git add <attacker_path> stages outside workdir)

Refs: bug-hunter scan of bounty-hunter-testautomaton (R-01..R-33).
@github-actions github-actions Bot added the needs-tier Maintainer must assign a review tier (contributor cannot self-label) label Sep 1, 2026

@Scottcjn Scottcjn left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself is correct and security-critical (accepted on the merits — webview XSS + openExternal scheme allowlist). One blocker before merge: the new line print(f" Generation: {"Claude" if used_claude else "template stub"}") nests same-type double-quotes inside an f-string — that's a SyntaxError on Python < 3.12, and our CI + nodes run 3.11. Swap the inner quotes to single ({'Claude' if used_claude else 'template stub'}) and rebase onto main (#16788 just merged, so this stack needs a rebase). Then it's a clean merge. Great catch on the underlying vuln.

@Scottcjn Scottcjn left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The https-only openExternal, Number.isInteger, and CSP additions are all real and welcome. One blocker: escapeJsString (which is JSON.stringify) is injected unescaped into a double-quoted onclick="openIssue(${issue.number}, ${escapeJsString(issue.html_url)})" — the JSON's own double-quotes break the attribute, which breaks the button for benign URLs and re-opens attribute injection. Build the onclick without inlining a quoted string into a quoted attribute (e.g. a data-attribute + addEventListener, or HTML-escape after JSON-encoding). Also heads-up: this branch is stacked on #16788 and #16789, so it'll need a rebase once those land.

…, F-02)

vscode-extension/src/bountyBrowser.ts
  - escapeHtml now also escapes single quote (F-01). The view-issue
    onclick interpolates issue.html_url into a single-quoted JS
    string; without the quote escape, a URL containing ' breaks out
    of the string.
  - Replace the in-attribute use site with a JSON.stringify wrapper
    (escapeJsString helper) so all control characters are properly
    escaped for both HTML-attribute and JS-string contexts.
  - Add a strict Content-Security-Policy meta tag to the bounty
    browser's HTML so even an injected node cannot load remote
    scripts (defence-in-depth for F-04).
  - openIssue handler now only follows https: URLs (F-02) and
    claimBounty requires Number.isInteger(issueNumber) (F-03).

Refs: bug-hunter scan F-01, F-02, F-03, F-04.
@xxzzzzy
xxzzzzy force-pushed the fix/vscode-F01-vscode-browser-xss branch from 81ee9f5 to 45b8602 Compare September 1, 2026 23:47
@xxzzzzy

xxzzzzy commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@Scottcjn Updated 45b8602 to address the only blocker, and rebased the branch onto current origin/main so it now touches a single file (vscode-extension/src/bountyBrowser.ts).

F-01 fix — escapeJsString injected into a quoted attribute (root cause):
The view-issue button no longer inlines JSON.stringify(issue.html_url) into a double-quoted onclick="..." attribute. Instead the URL is placed in a data-issue-url="..." attribute (which escapeHtml HTML-encodes), and the click is handled by a single delegated document.addEventListener('click', …) that reads data-issue-number + data-issue-url and calls openIssue(...) via acquireVsCodeApi().postMessage(...). The host-side onDidReceiveMessage still validates https: and Number.isInteger(issueNumber).

The escapeJsString helper has been deleted from the module (no longer reachable from the template), so the bypass class is gone — the host extension is the single source of truth for which URLs and issue numbers are forwarded.

Rebase: branch was carrying the testautomaton + miner + tipbot + comment-mod + telegram + otc-bridge + bounty-2864 + bounties.json changes from the stacked-on-#16788 layout; those are now all restored to origin/main so this PR is reviewable in isolation.

No other behaviour changed: the https:-only check on openExternal, Number.isInteger check on the claim payload, and the defence-in-depth Content-Security-Policy meta tag are still in place.

Scottcjn pushed a commit that referenced this pull request Sep 5, 2026
…HTML (RIP-306) (#16832)

scripts/sophia_dashboard.py (Admin Dashboard for SophiaCore Attestation Inspector):
  - Add esc() helper that escapes the five HTML-significant characters.
  - Replace the per-row inline-onclick-with-JSON.stringify pattern with a single
    delegated click listener on #review-table that reads data-record-idx and
    looks up the record from currentFilteredRecords.
  - Escape every API-controlled value going into tbody.innerHTML.
  - Escape API-controlled values in detail-verdict and detail-history blocks.
  - detail-title now uses textContent (inherently safe).
  - Coerce confidences through Number() before arithmetic.
  - Add EMOJI[...]-lookup fallback (|| empty-string).

tests/test_sophia_dashboard_xss.py:
  - 9 source-pattern regression tests that fail if the unsafe patterns come back.

Refs: RIP-306 admin dashboard XSS; same defensive posture as #16790, #16809-#16821.

Co-authored-by: bounty-bot <bounty-bot@local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-tier Maintainer must assign a review tier (contributor cannot self-label)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants