diff --git a/package-lock.json b/package-lock.json index cb8b8d3..4efc268 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,6 +18,7 @@ "@vscode/test-electron": "^2.5.2", "@vscode/vsce": "^3.5.0", "eslint": "^9.25.1", + "parse5": "^7.3.0", "typescript": "^5.8.3" }, "engines": { diff --git a/package.json b/package.json index 6bcdf89..0614de0 100644 --- a/package.json +++ b/package.json @@ -208,6 +208,18 @@ "type": "boolean", "default": false, "description": "Enable the local router to convert OpenAI format to Anthropic format. Required for providers that use OpenAI-compatible APIs." + }, + "claudeCodeChat.ui.collapseLongCodeBlocks": { + "type": "boolean", + "default": true, + "description": "Collapse code blocks longer than the line threshold by default. They stay foldable via their header either way." + }, + "claudeCodeChat.ui.collapseCodeBlockLines": { + "type": "number", + "default": 20, + "minimum": 5, + "maximum": 500, + "description": "Number of lines a code block must exceed before it gets a collapsible header (5-500)." } } } @@ -221,7 +233,13 @@ "test": "vscode-test", "test:downloader": "npm run compile && mocha --ui tdd \"out/test/downloader*.test.js\" --reporter spec --timeout 360000", "test:downloader:unit": "npm run compile && mocha --ui tdd out/test/downloader.test.js --reporter spec", - "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec" + "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec", + "test:collapse-rules": "npm run compile && mocha --ui tdd out/test/collapse-rules.test.js --reporter spec", + "test:html-escape": "npm run compile && mocha --ui tdd out/test/html-escape.test.js --reporter spec", + "test:webview-attr-escape": "npm run compile && mocha --ui tdd out/test/webview-attr-escape.test.js --reporter spec", + "test:markdown-restore": "npm run compile && mocha --ui tdd out/test/markdown-restore.test.js --reporter spec", + "test:user-message-rawtext": "npm run compile && mocha --ui tdd out/test/user-message-rawtext.test.js --reporter spec", + "test:settings-batch": "npm run compile && mocha --ui tdd out/test/settings-batch.test.js --reporter spec" }, "devDependencies": { "@types/mocha": "^10.0.10", @@ -233,6 +251,7 @@ "@vscode/test-electron": "^2.5.2", "@vscode/vsce": "^3.5.0", "eslint": "^9.25.1", + "parse5": "^7.3.0", "typescript": "^5.8.3" } } diff --git a/src/collapse-rules.ts b/src/collapse-rules.ts new file mode 100644 index 0000000..cfeba44 --- /dev/null +++ b/src/collapse-rules.ts @@ -0,0 +1,37 @@ +// Pure threshold/line-count logic for the fork-issue-48 collapsible-code-blocks feature (upstream +// #151): decides whether a fenced code block parseSimpleMarkdown is about to render should +// start collapsed, based on its line count and the configured threshold. No vscode import, +// so this runs under plain mocha like html-escape/markdown-restore/settings-batch. +// +// Neither function is ever called directly by the webview's copy of this file -- it has +// none. Instead collapse-script.ts injects each function's own compiled source via +// .toString() into the page (see collapse-script.ts for why). That means EVERY +// default/helper these functions need must be declared INSIDE their bodies: .toString() +// only ever returns the function's own text, not the rest of this module, so a +// module-level const referenced from inside a function would be undefined in the browser. + +export interface CodeBlockCollapseInfo { lineCount: number; collapse: boolean; maxLines: number; } + +export function normalizeCollapseThreshold(value: unknown): number { + // Defaults/limits MUST live inside the function body: .toString() only returns + // this function's own text, not any module-level symbol. + const DEFAULT_LINES = 20; // kept in sync with claudeCodeChat.ui.collapseCodeBlockLines (package.json) + const MIN_LINES = 5; + const MAX_LINES = 500; + const n = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(n) || n <= 0) { return DEFAULT_LINES; } + const floored = Math.floor(n); + if (floored < MIN_LINES) { return MIN_LINES; } + if (floored > MAX_LINES) { return MAX_LINES; } + return floored; +} + +export function evaluateCodeBlockCollapse(code: string, configuredMaxLines: unknown): CodeBlockCollapseInfo { + const maxLines = normalizeCollapseThreshold(configuredMaxLines); + const text = typeof code === 'string' ? code : ''; + // The fence regex in parseSimpleMarkdown captures the newline BEFORE the closing + // fence: "a\nb\nc\n" is 3 lines, not 4. Normalize CRLF beforehand. + const normalized = text.replace(/\r\n/g, '\n').replace(/\n$/, ''); + const lineCount = normalized === '' ? 0 : normalized.split('\n').length; + return { lineCount: lineCount, collapse: lineCount > maxLines, maxLines: maxLines }; +} diff --git a/src/collapse-script.ts b/src/collapse-script.ts new file mode 100644 index 0000000..6ae1f2c --- /dev/null +++ b/src/collapse-script.ts @@ -0,0 +1,53 @@ +import { normalizeCollapseThreshold, evaluateCodeBlockCollapse } from './collapse-rules'; + +// Webview-side glue for the fork-issue-48 collapsible-code-blocks feature (upstream #151), injected +// into script.ts's getScript() template the same way getSkillsScript()/getPluginsScript() are +// (see plugins-script.ts). Two different things happen below and they must not be confused: +// +// 1. normalizeCollapseThreshold.toString() / evaluateCodeBlockCollapse.toString() are REAL, +// host-side template interpolations (like escapeAttr.toString() in html-escape.ts): +// they run in Node when getCollapseScript() is called, and splice each function's own +// *compiled* source into the returned string. collapse-rules.ts must stay fully +// self-contained for exactly this reason -- only its own text crosses into the browser, +// not the rest of that module. +// 2. Everything else below (markCodeBlockToggled, applyCodeBlockCollapseDefaults, +// toggleMessageCollapsed) is plain webview source written directly in this template +// literal. None of it happens to need a client-side "${...}" or a backtick, so nothing +// here needs the "\${"/"\`" escaping script.ts's own template literal requires elsewhere +// -- but if you add code that does, escape it the same way (see script.ts's +// parseSimpleMarkdown for examples). +const getCollapseScript = () => ` + // ─── Collapsible code blocks + per-message fold (fork-issue-48) ─── + ${normalizeCollapseThreshold.toString()} + ${evaluateCodeBlockCollapse.toString()} + + // Bound to the 's synchronous onclick, never the
's + // ontoggle -- toggle fires asynchronously and also for a programmatic .open + // assignment, which would make applyCodeBlockCollapseDefaults() below unable to + // tell a real user click from its own catch-up pass after the first run. + function markCodeBlockToggled(summaryEl) { + summaryEl.parentElement.setAttribute('data-user-toggled', '1'); + } + + // Catch-up pass: settingsData arrives AFTER the history replay + // (extension.ts _loadConversationHistory -> _sendReadyMessage -> _sendCurrentSettings), + // so blocks rendered from history always start out using the webview's hardcoded + // default. This re-applies the real collapseLongCodeBlocks setting to every block the + // user hasn't touched yet; called once settingsData actually arrives (see script.ts). + function applyCodeBlockCollapseDefaults() { + var els = document.querySelectorAll('details.code-block-collapsible:not([data-user-toggled="1"])'); + for (var i = 0; i < els.length; i++) { els[i].open = !collapseLongCodeBlocks; } + } + + // Manual per-message collapse via the caret button in .message-header + // (script.ts addMessage). Purely a CSS class toggle, no DOM removal -- see + // ui-styles.ts's ".message.collapsed" rules. + function toggleMessageCollapsed(messageDiv, btn) { + var collapsed = messageDiv.classList.toggle('collapsed'); + btn.textContent = collapsed ? '▸' : '▾'; + btn.title = collapsed ? 'Expand message' : 'Collapse message'; + btn.setAttribute('aria-expanded', collapsed ? 'false' : 'true'); + } +`; + +export default getCollapseScript; diff --git a/src/extension.ts b/src/extension.ts index 8fa37fb..3c1e1ef 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -8,6 +8,7 @@ import { startRouter, stopRouter, setModelConfig, setBaseUrl } from './router'; import { fetchAndResolveModels } from './model-updater'; import recommendedModels from './recommended-models.json'; import { downloadClaude, detectPlatform, DownloaderError } from './claudeDownloader'; +import { updateWithWorkspaceThenGlobalFallback } from './settings-batch'; // OpenCredits environment configuration let OPENCREDITS_API_URL = 'https://ccc.api.opencredits.ai'; @@ -3400,6 +3401,8 @@ class ClaudeChatProvider { 'executable.path': config.get('executable.path', ''), 'environment.variables': config.get>('environment.variables', {}), 'environment.disabled': config.get('environment.disabled', false), + 'ui.collapseLongCodeBlocks': config.get('ui.collapseLongCodeBlocks', true), + 'ui.collapseCodeBlockLines': config.get('ui.collapseCodeBlockLines', 20), 'isOpenCredits': this._isOpenCredits() }; @@ -3409,61 +3412,104 @@ class ClaudeChatProvider { }); } + // fork-issue-59: workspace-then-global fallback (same pattern _updateSettings already used for + // this key, via updateWithWorkspaceThenGlobalFallback). Before fork-issue-59 this only tried + // Workspace and swallowed the error into the console -- in a window with no + // workspace folder open that meant YOLO mode was never actually persisted, while the + // webview's "YOLO Mode enabled!" chat message (script.ts's enableYoloMode()) fired + // unconditionally client-side, independent of any response from here. That message + // is now gated on the 'yoloModeEnabled' reply below, sent only after a successful + // write; a double failure gets a 'yoloModeEnableFailed' reply plus a native error + // notification instead, and never a success confirmation. + // + // Follow-up: the outer try/catch below exists because this method is + // called fire-and-forget (extension.ts's message handler does + // `this._enableYoloMode();`, no `await`/`.catch`, see the switch above). Previously, + // only the two config.update() calls inside + // updateWithWorkspaceThenGlobalFallback could reject; now that this method's own + // logic (e.g. a settings-batch.ts that's out of sync with extension.ts after a + // partial deploy, so updateWithWorkspaceThenGlobalFallback itself is undefined) can + // also throw, an uncaught rejection here would silently swallow the click with none + // of fork-issue-59's reporting -- exactly the failure class fork-issue-59 exists to close. private async _enableYoloMode(): Promise { try { - // Update VS Code configuration to enable YOLO mode const config = vscode.workspace.getConfiguration('claudeCodeChat'); + const result = await updateWithWorkspaceThenGlobalFallback( + async () => { await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Workspace); }, + async () => { await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Global); } + ); - // Clear any global setting and set workspace setting - await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Workspace); - - - // Send updated settings to UI - this._sendCurrentSettings(); - - } catch (error) { - console.error('Error enabling YOLO mode:', error); + if (result.succeeded) { + // Send updated settings to UI + this._sendCurrentSettings(); + this._postMessage({ type: 'yoloModeEnabled' }); + } else { + this._reportYoloModeEnableFailure(result.globalError || result.workspaceError || 'Unknown error'); + } + } catch (error: any) { + this._reportYoloModeEnableFailure(error?.message || String(error)); } } + // Shared by _enableYoloMode's double-failure path and its outer catch: same + // treatment either way -- a caller must never see a silent no-op where the chat + // already claimed success. + private _reportYoloModeEnableFailure(message: string): void { + console.error('Error enabling YOLO mode:', message); + vscode.window.showErrorMessage(`Failed to enable YOLO mode: ${message}`); + this._postMessage({ type: 'yoloModeEnableFailed', error: message }); + } + private _saveInputText(text: string): void { this._draftMessage = text || ''; } private async _updateSettings(settings: { [key: string]: any }): Promise { const config = vscode.workspace.getConfiguration('claudeCodeChat'); + const failures: string[] = []; - try { - for (const [key, value] of Object.entries(settings)) { + // Each key gets its own try/catch so one failing key (e.g. a double + // workspace+global failure on permissions.yoloMode below) can never silently + // prevent the remaining keys in this settings batch from being applied. + for (const [key, value] of Object.entries(settings)) { + try { if (key === 'permissions.yoloMode') { - // YOLO mode: try workspace first, fall back to global - try { - await config.update(key, value, vscode.ConfigurationTarget.Workspace); - } catch { - await config.update(key, value, vscode.ConfigurationTarget.Global); + // fork-issue-59: YOLO mode: try workspace first, fall back to global (same + // helper _enableYoloMode uses). + const yoloResult = await updateWithWorkspaceThenGlobalFallback( + async () => { await config.update(key, value, vscode.ConfigurationTarget.Workspace); }, + async () => { await config.update(key, value, vscode.ConfigurationTarget.Global); } + ); + if (!yoloResult.succeeded) { + throw new Error(yoloResult.globalError || yoloResult.workspaceError || 'Unknown error'); } } else { // Other settings are global (user-wide) await config.update(key, value, vscode.ConfigurationTarget.Global); } + } catch (error: any) { + console.error(`Failed to update setting "${key}":`, error?.message || error); + failures.push(`${key}: ${error?.message || error}`); } + } - // Re-send settings so webview gets updated isOpenCredits flag, etc. - this._sendCurrentSettings(); + if (failures.length > 0) { + vscode.window.showErrorMessage(`Failed to update settings: ${failures.join('; ')}`); + } - // Update balance display based on new env vars - if (this._isOpenCredits() || this._getOpenCreditsKey()) { - this._sendOpenCreditsBalance(); - } else { - // Clear balance if no longer OpenCredits - this._postMessage({ - type: 'opencreditsBalance', - balance: null - }); - } - } catch (error: any) { - console.error('Failed to update settings:', error?.message || error); - vscode.window.showErrorMessage(`Failed to update settings: ${error?.message || 'Unknown error'}`); + // Re-send settings so webview gets updated isOpenCredits flag, etc. Runs even if + // some keys above failed, so the keys that did succeed are still reflected back. + this._sendCurrentSettings(); + + // Update balance display based on new env vars + if (this._isOpenCredits() || this._getOpenCreditsKey()) { + this._sendOpenCreditsBalance(); + } else { + // Clear balance if no longer OpenCredits + this._postMessage({ + type: 'opencreditsBalance', + balance: null + }); } } diff --git a/src/html-escape.ts b/src/html-escape.ts new file mode 100644 index 0000000..a819914 --- /dev/null +++ b/src/html-escape.ts @@ -0,0 +1,41 @@ +// Attribute escaping for the webview (fork-issue-49). script.ts' escapeHtml() serializes via +// textContent->innerHTML and therefore leaves " and ' UNTOUCHED -- for title="..."/data-*="..." +// that isn't enough (attribute breakout). This function is NOT called here: script.ts +// splices only its own compiled text into the page via .toString() (same pattern as +// collapse-rules.ts/markdown-restore.ts). So it MUST stay self-contained -- no +// module-level symbol, no import, no helper function outside the body. +export function escapeAttr(value: unknown): string { + const s = value === null || value === undefined ? '' : String(value); + // '&' must come first, otherwise the entities we just produced get re-decoded afterwards. + return s + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + +// fork-issue-61: escapeAttr() makes href=/src= breakout-safe but doesn't check the scheme -- a +// javascript:-link from third-party data (an MCP registry entry) stays clickable/live. +// safeHttpUrl() only lets http:/https: through, otherwise an empty string (the caller then +// omits the attribute/link entirely instead of rendering a dead attribute). The cleanup before +// the scheme check mirrors the first steps of the WHATWG URL parser: tab/newline/CR are +// stripped everywhere in the string (catches "java\tscript:"), leading/trailing C0 control +// characters and spaces are trimmed -- both are tricks browsers would otherwise let bypass a +// scheme check done via plain string comparison. Same self-containment rule as escapeAttr: +// no module-level symbol, no import, no helper function outside the body. +// NOTE: this is deliberately fail-closed even for relative ("/icons/x.png", +// "icon.png") and protocol-relative ("//cdn.example/i.png") URLs, which resolve to '' -- a +// scheme is a hard requirement here, no special case for those. Should a data source ever +// start supplying relative/protocol-relative icon URLs, the icon will silently fall back to +// the placeholder instead of loading -- not a bug, but a behavior change worth remembering. +export function safeHttpUrl(value: unknown): string { + let s = value === null || value === undefined ? '' : String(value); + s = s.replace(/[\t\n\r]/g, ''); + s = s.replace(/^[\x00-\x20]+/, '').replace(/[\x00-\x20]+$/, ''); + const schemeMatch = /^([a-zA-Z][a-zA-Z0-9+\-.]*):/.exec(s); + if (!schemeMatch) { return ''; } + const scheme = schemeMatch[1].toLowerCase(); + if (scheme !== 'http' && scheme !== 'https') { return ''; } + return s; +} diff --git a/src/markdown-restore.ts b/src/markdown-restore.ts new file mode 100644 index 0000000..709596f --- /dev/null +++ b/src/markdown-restore.ts @@ -0,0 +1,20 @@ +// Placeholder back-substitution for code blocks in parseSimpleMarkdown (fork-issue-55, a finding +// from the fork-issue-47 review). String.replace(placeholder, value) interprets "$&"/"$`"/"$'"/"$$" +// in the replacement string as substitution patterns -- a code block whose (already +// escaped) content happens to contain such a sequence (e.g. shell code with "$'...'") +// tears the surrounding HTML apart instead of appearing unchanged. This function uses +// function replacement (returning the value from a callback) instead of a plain string +// as the second argument, which sidesteps the substitution-pattern interpretation +// entirely. script.ts splices only the compiled function text into the page via +// .toString() (same pattern as html-escape.ts/collapse-rules.ts) -- so this function +// must stay self-contained: no module-level symbol, no import, no helper function +// outside the body. +export function restoreCodeBlockPlaceholders(html: string, codeBlockPlaceholders: string[]): string { + for (let i = 0; i < codeBlockPlaceholders.length; i++) { + const placeholder = '__CODEBLOCK_' + i + '__'; + const value = codeBlockPlaceholders[i]; + // Function replacement, NEVER a string directly as the 2nd argument (see comment above). + html = html.replace(placeholder, function () { return value; }); + } + return html; +} diff --git a/src/plugins-script.ts b/src/plugins-script.ts index e73cc49..922b874 100644 --- a/src/plugins-script.ts +++ b/src/plugins-script.ts @@ -53,7 +53,7 @@ const getPluginsScript = () => ` (desc ? '
' + escapeHtml(desc) + '
' : '') + '' + '
' + - '' + + '' + '
'; pluginsList.appendChild(item); }); @@ -72,7 +72,9 @@ const getPluginsScript = () => ` var displayName = formatPluginName(name); var desc = escapeHtml(plugin.description || 'No description'); var verified = plugin.verified; - var safeId = escapeHtml(plugin.installId || name).replace(/'/g, '''); + // fork-issue-57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left + // " unescaped and able to break out of the data-plugin-id attribute below. + var safeId = escapeAttr(plugin.installId || name); html += '
' + '
' + @@ -121,10 +123,10 @@ const getPluginsScript = () => ` '
' + escapeHtml(displayName) + '
' + '
' + verifiedHtml + '
' + '
' + - '' + + '' + '
' + '
' + escapeHtml(desc) + '
' + - '
View on GitHub
' + + '
View on GitHub
' + '
Adds ' + escapeHtml(installId) + ' to .claude/settings.json
' + ''; } diff --git a/src/script.ts b/src/script.ts index 4c949e2..7c53512 100644 --- a/src/script.ts +++ b/src/script.ts @@ -1,5 +1,8 @@ import getSkillsScript from './skills-script'; import getPluginsScript from './plugins-script'; +import getCollapseScript from './collapse-script'; +import { escapeAttr, safeHttpUrl } from './html-escape'; +import { restoreCodeBlockPlaceholders } from './markdown-restore'; const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https://ccc.api.opencredits.ai', opencreditsWebUrl: string = 'https://ccc.opencredits.ai', opencreditsPublishableKey: string = 'oc_pk_c43da4f9a9484ae484ad29bc97cc354f') => `` diff --git a/src/settings-batch.ts b/src/settings-batch.ts new file mode 100644 index 0000000..de7e4a6 --- /dev/null +++ b/src/settings-batch.ts @@ -0,0 +1,80 @@ +// fork-issue-59: permissions.yoloMode's workspace-then-global fallback, pulled into its own pure +// function so both _enableYoloMode (the inline "Enable Yolo Mode" chat button / menu +// item, fork-issue-52) and _updateSettings's existing per-key handling of the same setting key +// can share one implementation instead of keeping two copies of the same try/catch in +// extension.ts. No vscode import here, so this runs under plain mocha. Run with +// `npm run test:settings-batch`. +// +// Note: upstream (fork) also has an applySettingsBatch helper in this module for a +// separate, unrelated fix (fork-issue-56, whole-settings-batch loop resilience) that is not part +// of this security-hardening branch and is intentionally not included here. + +// Turns whatever a rejected update callback threw into a plain string, the same way the +// pre-fork-issue-59 code's 'err=' + (error?.message || error) string-concatenation did (Error +// instances and message-bearing objects use .message; anything else -- a thrown string, +// undefined, a plain object -- coerces the same way String() / template-literal +// interpolation would), so a caller in extension.ts never has to guard against +// a missing .message itself. +export function toErrorMessage(error: unknown): string { + if (typeof error === 'object' && error !== null && 'message' in error) { + const message = (error as { message: unknown }).message; + if (typeof message === 'string' && message) { + return message; + } + } + if (typeof error === 'string') { + return error; + } + return String(error); +} + +export interface WorkspaceThenGlobalFallbackResult { + succeeded: boolean; + // Which scope actually ended up holding the value on success, or which scope's + // error is the relevant "final" one when both attempts failed. + scope: 'workspace' | 'global'; + // Set whenever the workspace attempt was made and threw -- present both when the + // global fallback then succeeded (diagnostic-only) and when it also failed. + workspaceError?: string; + // Set only when the global fallback attempt itself threw. + globalError?: string; +} + +// fork-issue-59: permissions.yoloMode's workspace-then-global fallback, pulled into its own pure +// function so both _enableYoloMode (the inline "Enable Yolo Mode" chat button / menu +// item, fork-issue-52) and _updateSettings's existing per-key handling of the same setting key +// can share one implementation instead of keeping two copies of the same try/catch in +// extension.ts. extension.ts still owns the actual vscode.workspace config.update() +// calls via the injected callbacks. +// +// Before fork-issue-59, _enableYoloMode had no global fallback at all and its bare catch only +// logged to the extension host's console -- in a window with no workspace folder open, +// config.update(..., Workspace) throws, _sendCurrentSettings() (and therefore the +// webview) never found out, and the setting was silently never persisted. The chat +// already said "YOLO Mode enabled!" regardless, because that confirmation was fired +// client-side the moment the button was clicked, not gated on any response from the +// extension host. This function's `succeeded` flag exists specifically so a caller can +// tell "both attempts threw" apart from "it worked" and refuse to report success in +// that case. +export async function updateWithWorkspaceThenGlobalFallback( + updateWorkspace: () => Promise, + updateGlobal: () => Promise +): Promise { + try { + await updateWorkspace(); + return { succeeded: true, scope: 'workspace' }; + } catch (workspaceError) { + const workspaceMessage = toErrorMessage(workspaceError); + try { + await updateGlobal(); + return { succeeded: true, scope: 'global', workspaceError: workspaceMessage }; + } catch (globalError) { + return { + succeeded: false, + scope: 'global', + workspaceError: workspaceMessage, + globalError: toErrorMessage(globalError) + }; + } + } +} diff --git a/src/skills-script.ts b/src/skills-script.ts index 5fc8ec1..fc808cb 100644 --- a/src/skills-script.ts +++ b/src/skills-script.ts @@ -22,11 +22,13 @@ const getSkillsScript = () => ` var installs = skill.installs || 0; var source = skill.source || ''; var installsHtml = installs > 0 ? '' + (installs >= 1000 ? (Math.round(installs / 100) / 10) + 'k' : installs) + ' installs' : ''; - var safeId = escapeHtml(skill.id || name).replace(/'/g, '''); + // fork-issue-57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left + // " unescaped and able to break out of the data-skill-id attribute below. + var safeId = escapeAttr(skill.id || name); var rawUrl = skill.rawUrl || ''; var installsText = installs >= 1000 ? (Math.round(installs / 100) / 10) + 'k installs' : (installs > 0 ? installs + ' installs' : ''); - html += '
' + + html += '
' + '
' + '
' + escapeHtml(name.charAt(0).toUpperCase()) + '
' + '
' + @@ -76,7 +78,7 @@ const getSkillsScript = () => ` '
' + '
' + '' + - '' + + '' + '
' + '
' + ''; diff --git a/src/test/collapse-rules.test.ts b/src/test/collapse-rules.test.ts new file mode 100644 index 0000000..4dd0b0b --- /dev/null +++ b/src/test/collapse-rules.test.ts @@ -0,0 +1,108 @@ +// Unit tests for the fork-issue-48 collapsible-code-blocks logic (normalizeCollapseThreshold / +// evaluateCodeBlockCollapse). Pure (no vscode, no network, no filesystem access), so these +// run under plain mocha against the compiled out/ output -- same pattern as +// html-escape/markdown-restore/settings-batch. The first suite covers +// evaluateCodeBlockCollapse's line-counting edge cases (trailing newline, CRLF, empty +// input, a single very long line), the second covers normalizeCollapseThreshold's +// default/clamp behaviour, and the third is the splice-sandbox test that proves both +// functions still work with zero module context -- exactly how collapse-script.ts's +// .toString() splice runs them in the webview. Run with `npm run test:collapse-rules`. + +import * as assert from 'assert'; +import { evaluateCodeBlockCollapse, normalizeCollapseThreshold } from '../collapse-rules'; + +suite('collapse-rules: evaluateCodeBlockCollapse (line counting)', () => { + + test('3 lines at a threshold of 20 stay uncollapsed', () => { + const info = evaluateCodeBlockCollapse('a\nb\nc\n', 20); + assert.strictEqual(info.lineCount, 3); + assert.strictEqual(info.collapse, false); + }); + + test('exactly 20 lines at a threshold of 20 stay uncollapsed (strictly-greater-than collapses)', () => { + const twentyLines = new Array(20).fill('x').join('\n') + '\n'; + const info = evaluateCodeBlockCollapse(twentyLines, 20); + assert.strictEqual(info.lineCount, 20); + assert.strictEqual(info.collapse, false); + }); + + test('21 lines at a threshold of 20 collapses', () => { + const twentyOneLines = new Array(21).fill('x').join('\n') + '\n'; + const info = evaluateCodeBlockCollapse(twentyOneLines, 20); + assert.strictEqual(info.lineCount, 21); + assert.strictEqual(info.collapse, true); + }); + + test('a trailing newline does not count as an extra line ("a\\nb\\n" is 2 lines, not 3)', () => { + assert.strictEqual(evaluateCodeBlockCollapse('a\nb\n', 20).lineCount, 2); + }); + + test('CRLF line endings are normalized before counting ("a\\r\\nb\\r\\nc" is 3 lines)', () => { + assert.strictEqual(evaluateCodeBlockCollapse('a\r\nb\r\nc', 20).lineCount, 3); + }); + + test('an empty code block has lineCount 0 and never collapses', () => { + const info = evaluateCodeBlockCollapse('', 20); + assert.strictEqual(info.lineCount, 0); + assert.strictEqual(info.collapse, false); + }); + + test('a single very long line (5000 chars) does not collapse -- line-based only, not char-based', () => { + const info = evaluateCodeBlockCollapse('x'.repeat(5000), 20); + assert.strictEqual(info.lineCount, 1); + assert.strictEqual(info.collapse, false); + }); +}); + +suite('collapse-rules: normalizeCollapseThreshold (defaults/clamping)', () => { + + test('undefined falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(undefined), 20); + }); + + test('null falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(null), 20); + }); + + test('NaN falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(NaN), 20); + }); + + test('a non-numeric string falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold('abc'), 20); + }); + + test('0 falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(0), 20); + }); + + test('a negative number falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(-5), 20); + }); + + test('a value below the minimum clamps up to 5', () => { + assert.strictEqual(normalizeCollapseThreshold(3), 5); + }); + + test('a value inside the valid range passes through unchanged', () => { + assert.strictEqual(normalizeCollapseThreshold(7), 7); + }); + + test('a decimal value inside the valid range is floored', () => { + assert.strictEqual(normalizeCollapseThreshold(12.7), 12); + }); + + test('a value above the maximum clamps down to 500', () => { + assert.strictEqual(normalizeCollapseThreshold(9999), 500); + }); +}); + +suite('collapse-rules: toString() splice sandbox', () => { + + test('both functions still work when spliced into a Function body with zero module context, matching the real call', () => { + const spliced = new Function( + normalizeCollapseThreshold.toString() + '\n' + evaluateCodeBlockCollapse.toString() + + '\nreturn evaluateCodeBlockCollapse(arguments[0], arguments[1]);'); + assert.deepStrictEqual(spliced('a\nb\nc\nd', 2), evaluateCodeBlockCollapse('a\nb\nc\nd', 2)); + }); +}); diff --git a/src/test/html-escape.test.ts b/src/test/html-escape.test.ts new file mode 100644 index 0000000..7b71dd4 --- /dev/null +++ b/src/test/html-escape.test.ts @@ -0,0 +1,85 @@ +// Unit tests for the fork-issue-49 attribute escaper (escapeAttr). Pure (no vscode, no network, no +// DOM), so these run under plain mocha against the compiled out/ output -- same pattern as +// collapse-rules/markdown-restore/settings-batch. escapeHtml() in +// script.ts serialises through textContent->innerHTML and therefore leaves " and ' untouched, +// which is fine for element content but not for title="..."/data-*="..." attribute values -- +// escapeAttr() is the dedicated fix for that sink. The first suite covers plain character +// escaping (including the "&" -must-come-first ordering), the second covers edge cases +// (attribute breakout, empty/null/undefined/number input), and the third proves both that the +// escaping is information-preserving (roundtrip against a browser-style attribute decoder) and +// that the function still works with zero module context -- exactly how script.ts's +// .toString() splice runs it in the webview. Run with `npm run test:html-escape`. + +import * as assert from 'assert'; +import { escapeAttr } from '../html-escape'; + +suite('html-escape: escapeAttr (character escaping)', () => { + + test('" is escaped to " (the case escapeHtml does not cover)', () => { + assert.strictEqual(escapeAttr('"'), '"'); + }); + + test("' is escaped to '", () => { + assert.strictEqual(escapeAttr('\''), '''); + }); + + test('hi is escaped to <b>hi', () => { + assert.strictEqual(escapeAttr('hi'), '<b>hi'); + }); + + test('& is escaped to &', () => { + assert.strictEqual(escapeAttr('&'), '&'); + }); + + test('& is escaped to &amp; (entity text survives, no information loss)', () => { + assert.strictEqual(escapeAttr('&'), '&amp;'); + }); + + test('&&<<"" is escaped to &&<<"" (proves "&" runs first)', () => { + assert.strictEqual(escapeAttr('&&<<""'), '&&<<""'); + }); +}); + +suite('html-escape: escapeAttr (edge cases)', () => { + + test('an attribute-breakout attempt (" onmouseover="alert(1)) leaves no raw " in the result', () => { + const result = escapeAttr('" onmouseover="alert(1)'); + assert.ok(!result.includes('"'), 'result must not contain a raw "; got: ' + result); + }); + + test('an empty string stays an empty string', () => { + assert.strictEqual(escapeAttr(''), ''); + }); + + test('null and undefined both become an empty string', () => { + assert.strictEqual(escapeAttr(null), ''); + assert.strictEqual(escapeAttr(undefined), ''); + }); + + test('a number is stringified (42 -> "42")', () => { + assert.strictEqual(escapeAttr(42), '42'); + }); +}); + +suite('html-escape: escapeAttr (roundtrip / splice sandbox)', () => { + + test('roundtrip: decoding escapeAttr(x) the way a browser decodes an attribute value returns x unchanged', () => { + // & deliberately decoded LAST -- exactly the order a browser's attribute + // parser uses, and the mirror image of escapeAttr() encoding "&" FIRST. + function decodeAttr(s: string): string { + return s + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, '\'') + .replace(/&/g, '&'); + } + const x = 'echo "&"'; + assert.strictEqual(decodeAttr(escapeAttr(x)), x); + }); + + test('escapeAttr still works when spliced into a Function body with zero module context, matching the real call', () => { + const spliced = new Function(escapeAttr.toString() + '\nreturn escapeAttr(arguments[0]);'); + assert.strictEqual(spliced('a"&'), escapeAttr('a"&')); + }); +}); diff --git a/src/test/markdown-restore.test.ts b/src/test/markdown-restore.test.ts new file mode 100644 index 0000000..868dcdb --- /dev/null +++ b/src/test/markdown-restore.test.ts @@ -0,0 +1,86 @@ +// Unit tests for the fork-issue-55 code-block restore loop (restoreCodeBlockPlaceholders), the +// __CODEBLOCK_N__ half of parseSimpleMarkdown's placeholder dance. Pure (no vscode, no +// network, no DOM), so these run under plain mocha against the compiled out/ output -- +// same pattern as collapse-rules/html-escape/settings-batch. fork-issue-47's review found +// that the loop used html.replace(placeholder, str), a plain STRING as the 2nd argument -- +// String.replace treats "$&"/"$`"/"$'"/"$$" in a string replacement as substitution +// patterns, so a code block whose (already-escaped) content happens to contain one of +// those sequences tears the surrounding HTML apart instead of being reinserted unchanged. +// This fixes the code-block loop to use function-replacement instead, for exactly that +// reason. The first suite covers ordinary multi-placeholder restores, the second is the +// regression suite for each substitution pattern individually plus one +// combined case, and the third is the splice-sandbox test that proves the function still +// works with zero module context -- exactly how script.ts's .toString() splice runs it in +// the webview. Run with `npm run test:markdown-restore`. + +import * as assert from 'assert'; +import { restoreCodeBlockPlaceholders } from '../markdown-restore'; + +suite('markdown-restore: restoreCodeBlockPlaceholders (basic)', () => { + + test('a single placeholder is replaced by its code block value', () => { + const result = restoreCodeBlockPlaceholders('before __CODEBLOCK_0__ after', ['
x
']); + assert.strictEqual(result, 'before 
x
after'); + }); + + test('multiple placeholders are each restored to their own value, in order', () => { + const html = '__CODEBLOCK_0__ mid __CODEBLOCK_1__ mid __CODEBLOCK_2__'; + const result = restoreCodeBlockPlaceholders(html, ['
a
', '
b
', '
c
']); + assert.strictEqual(result, '
a
mid 
b
mid 
c
'); + }); + + test('an empty placeholders array leaves html unchanged', () => { + assert.strictEqual(restoreCodeBlockPlaceholders('

no code blocks here

', []), '

no code blocks here

'); + }); +}); + +suite('markdown-restore: restoreCodeBlockPlaceholders ($ substitution patterns, fork-issue-55)', () => { + + test('"$&" (matched-substring pattern) in the code block value survives unchanged, not the matched placeholder', () => { + const codeHtml = '
echo $& run in background
'; + const html = '

before

__CODEBLOCK_0__

after

'; + const result = restoreCodeBlockPlaceholders(html, [codeHtml]); + assert.strictEqual(result, '

before

' + codeHtml + '

after

'); + }); + + test('"$`" (pre-match pattern) in the code block value survives unchanged, not a copy of the preceding HTML', () => { + const codeHtml = '
VAR=$`date`
'; + const html = '

before

__CODEBLOCK_0__

after

'; + const result = restoreCodeBlockPlaceholders(html, [codeHtml]); + assert.strictEqual(result, '

before

' + codeHtml + '

after

'); + }); + + test('"$\'" (post-match pattern) in the code block value survives unchanged, not a copy of the following HTML', () => { + const codeHtml = '
MSG=$\'it worked\'
'; + const html = '

before

__CODEBLOCK_0__

after

'; + const result = restoreCodeBlockPlaceholders(html, [codeHtml]); + assert.strictEqual(result, '

before

' + codeHtml + '

after

'); + }); + + test('"$$" (escaped-dollar pattern) in the code block value does not collapse to a single "$"', () => { + const codeHtml = '
echo $$ # current PID
'; + const html = '

before

__CODEBLOCK_0__

after

'; + const result = restoreCodeBlockPlaceholders(html, [codeHtml]); + assert.strictEqual(result, '

before

' + codeHtml + '

after

'); + }); + + test('a code block combining "$&", "$`", "$\'" and "$$" survives the restore byte-for-byte', () => { + const dollarPatterns = 'a$&b a$`b a$\'b a$$b'; + const codeHtml = '
' + dollarPatterns + '
'; + const html = '

before

__CODEBLOCK_0__

after

'; + const result = restoreCodeBlockPlaceholders(html, [codeHtml]); + assert.strictEqual(result, '

before

' + codeHtml + '

after

'); + }); +}); + +suite('markdown-restore: toString() splice sandbox', () => { + + test('restoreCodeBlockPlaceholders still works when spliced into a Function body with zero module context, matching the real call', () => { + const spliced = new Function( + restoreCodeBlockPlaceholders.toString() + + '\nreturn restoreCodeBlockPlaceholders(arguments[0], arguments[1]);'); + const html = '

before

__CODEBLOCK_0__

after

'; + const placeholders = ['
a$&b a$`b a$\'b
']; + assert.deepStrictEqual(spliced(html, placeholders), restoreCodeBlockPlaceholders(html, placeholders)); + }); +}); diff --git a/src/test/settings-batch.test.ts b/src/test/settings-batch.test.ts new file mode 100644 index 0000000..7ed885b --- /dev/null +++ b/src/test/settings-batch.test.ts @@ -0,0 +1,67 @@ +// Unit tests for the fork-issue-59 updateWithWorkspaceThenGlobalFallback fix. Pure (no vscode, no +// network, no filesystem access), so these run under plain mocha against the compiled +// out/ output. Run with `npm run test:settings-batch`. +// +// Note: upstream (fork) also has applySettingsBatch tests in this file for a separate, +// unrelated fix (fork-issue-56, whole-settings-batch loop resilience) that is not part of this +// security-hardening branch and is intentionally not included here. + +import * as assert from 'assert'; +import { updateWithWorkspaceThenGlobalFallback } from '../settings-batch'; + +// fork-issue-59: updateWithWorkspaceThenGlobalFallback -- the pure decision logic behind +// permissions.yoloMode's workspace-then-global fallback, shared by _enableYoloMode and +// _updateSettings. Before fork-issue-59, _enableYoloMode had no fallback at all (a Workspace-scope +// write failing, e.g. no workspace folder open, meant the setting was silently never +// persisted while the webview still showed "YOLO Mode enabled!"); these tests pin the +// three outcomes a caller needs to distinguish: workspace succeeds, workspace fails but +// global saves it, and both fail (the case that must never be reported as success). +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (workspace succeeds)', () => { + + test('the workspace callback is used and global is never attempted', async () => { + let globalCalls = 0; + const result = await updateWithWorkspaceThenGlobalFallback( + async () => { /* succeeds */ }, + async () => { globalCalls++; } + ); + assert.deepStrictEqual(result, { succeeded: true, scope: 'workspace' }); + assert.strictEqual(globalCalls, 0, 'global must not be attempted when workspace already succeeded'); + }); +}); + +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (workspace throws, global fallback saves it)', () => { + + test('a workspace failure -- e.g. no workspace folder open -- falls back to global and still reports success', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject(new Error('Unable to write to Workspace Settings because no workspace is opened')), + async () => { /* global succeeds */ } + ); + assert.strictEqual(result.succeeded, true, 'fork-issue-59: this is exactly the case the old _enableYoloMode (no fallback at all) silently lost'); + assert.strictEqual(result.scope, 'global'); + assert.strictEqual(result.workspaceError, 'Unable to write to Workspace Settings because no workspace is opened'); + }); +}); + +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (both attempts throw)', () => { + + test('a double failure is reported as NOT succeeded, with both error messages preserved -- never treated as success', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject(new Error('workspace boom')), + () => Promise.reject(new Error('global boom')) + ); + assert.strictEqual(result.succeeded, false, 'fork-issue-59: a caller (e.g. the webview YOLO Mode enabled! message) must never treat this as success'); + assert.strictEqual(result.scope, 'global'); + assert.strictEqual(result.workspaceError, 'workspace boom'); + assert.strictEqual(result.globalError, 'global boom'); + }); + + test('non-Error rejections are still normalized to strings on both sides', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject('plain workspace error'), + () => Promise.reject(undefined) + ); + assert.strictEqual(result.succeeded, false); + assert.strictEqual(result.workspaceError, 'plain workspace error'); + assert.strictEqual(result.globalError, 'undefined'); + }); +}); diff --git a/src/test/user-message-rawtext.test.ts b/src/test/user-message-rawtext.test.ts new file mode 100644 index 0000000..b9ec09e --- /dev/null +++ b/src/test/user-message-rawtext.test.ts @@ -0,0 +1,383 @@ +// PoC/regression tests for fork-issue-63 (user messages rendered via parseSimpleMarkdown + innerHTML, so +// markdown syntax in the user's OWN typed text -- "**", "_", backticks, "#" lines, e.g. a prompt +// mentioning a path like src/_test_.ts -- got silently reinterpreted as formatting instead of +// showing up exactly as typed). +// +// Fix, revised after further review: the case 'userInput' handler now calls renderUserMessageContent +// instead of parseSimpleMarkdown. renderUserMessageContent keeps prose completely raw (only +// escapeHtml, never markdown-parsed) but still runs fenced ``` code blocks through the same +// extractCodeBlocks() machinery parseSimpleMarkdown itself uses -- an earlier, plain- +// textContent-only version of this fix also flattened code blocks (lost the fork-issue-48 +// collapse/copy-button/language-label treatment and fork-issue-62's data-raw-code), which was intentionally excluded from this behavior. +// Claude's/thinking's own messages are unchanged (parseSimpleMarkdown + innerHTML). +// +// These tests exercise the REAL case 'userInput': block's exact source text (extracted from the +// compiled webview output, not hand-copied) together with the REAL renderUserMessageContent, +// extractCodeBlocks and addMessage -- so they fail against both the pre-fork-issue-63 source (wraps +// everything in

// via parseSimpleMarkdown) and the intermediate plain- +// textContent fix (flattens fenced code blocks into literal text), and pass against the current +// source, without the test itself having to guess which state the repository is in. Run with +// `npm run test:user-message-rawtext`. + +import * as assert from 'assert'; +import * as vm from 'vm'; +import getScript from '../script'; +import { extractFunction, findAttrOn, textOn, tagExists } from './webview-dom-helpers'; + +function getEmittedScriptBody(): string { + const html = getScript(false); + const match = /]*)?>([\s\S]*?)<\/script>/.exec(html); + if (!match) { + throw new Error('getScript(false) did not contain a block'); + } + return match[1]; +} + +// Verifies an extractFunction() result actually starts/ends where expected and didn't drag in a +// trailing declaration (fork-issue-64, fixed by making extractFunction parser-based) -- kept as a +// belt-and-suspenders check before building on the extraction. +function assertCleanExtraction(name: string, src: string, expectedStart: string): void { + assert.ok(src.startsWith(expectedStart), 'extractFunction(' + name + ') did not start where expected; got: ' + src.slice(0, 80)); + assert.ok(src.trimEnd().endsWith('}'), 'extractFunction(' + name + ') did not end at a closing brace; got: ' + src.slice(-80)); + assert.ok( + !/\n\s*function\s+\w+\s*\(/.test(src.slice(expectedStart.length)), + 'extractFunction(' + name + ') appears to have dragged in a trailing function declaration (fork-issue-64); got length ' + src.length + ); +} + +// Extracts the exact statements inside `case '': ... break;` (quote-aware would be +// overkill here -- unlike extractFunction's braces, this case body contains no nested "break;" in +// any of the pre-fork-issue-63/intermediate/current source variants, only its own terminating one). +// Verified below via an explicit assertion, same spirit as assertCleanExtraction for +// extractFunction. +function extractCaseBlock(source: string, caseLabel: string): string { + const re = new RegExp('case \'' + caseLabel + '\':([\\s\\S]*?)\\n\\s*break;'); + const m = re.exec(source); + if (!m) { + throw new Error('case \'' + caseLabel + '\': block not found in emitted script'); + } + return m[1]; +} + +// Faithful-enough DOM stand-in for addMessage(): unlike webview-attr-escape.test.ts's +// FakeElement/FakeDiv (built for direct innerHTML-string assignment sinks), addMessage builds +// the message via createElement()+appendChild() trees, so outerHTML must recursively serialise +// children -- this class does that. Only the setters/methods addMessage's (and escapeHtml's, +// which parseSimpleMarkdown/renderUserMessageContent also call) path actually touches are +// implemented; anything else the emitted code assigns (onclick, title, ...) lands as an ordinary +// dynamic JS property at runtime -- harmless, and irrelevant to what these tests assert on. +class FakeNode { + readonly tagName: string; + private _attrs = new Map(); + private _children: FakeNode[] = []; + private _text: string | null = null; + private _rawHtml: string | null = null; + + constructor(tagName: string) { this.tagName = tagName; } + + set className(v: string) { this._attrs.set('class', v); } + get className(): string { return this._attrs.get('class') || ''; } + + setAttribute(name: string, value: string): void { this._attrs.set(name, String(value)); } + + appendChild(child: FakeNode): FakeNode { + this._children.push(child); + this._text = null; + this._rawHtml = null; + return child; + } + + // Node.textContent setter semantics: replaces all children with a single implicit text node. + set textContent(v: string) { + this._text = String(v); + this._children = []; + this._rawHtml = null; + } + + // addMessage assigns innerHTML a hand-built HTML string (parseSimpleMarkdown's/ + // renderUserMessageContent's output, or copyBtn's fixed SVG) -- stored verbatim, meant to BE + // parsed as markup, unlike textContent above. + set innerHTML(v: string) { + this._rawHtml = String(v); + this._text = null; + this._children = []; + } + + // Real DOM Text-node HTML serialisation escapes only & < > (not " or ') -- matches + // webview-attr-escape.test.ts's FakeDiv, and is what escapeHtml() (document.createElement + // ('div').textContent = ...; return div.innerHTML) relies on internally. + get innerHTML(): string { + if (this._rawHtml !== null) { return this._rawHtml; } + if (this._text !== null) { + return this._text.replace(/&/g, '&').replace(//g, '>'); + } + return this._children.map(c => c.outerHTML).join(''); + } + + get outerHTML(): string { + const attrs = Array.from(this._attrs.entries()) + .map(([k, v]) => ' ' + k + '="' + v.replace(/&/g, '&').replace(/"/g, '"') + '"') + .join(''); + return '<' + this.tagName + attrs + '>' + this.innerHTML + ''; + } +} + +interface FakeMessagesDiv { + scrollTop: number; + scrollHeight: number; + clientHeight: number; + lastAppended: FakeNode | undefined; + appendChild(child: FakeNode): FakeNode; +} + +interface UserInputPipelineSandbox { + addMessage(content: string, type: string): void; + parseSimpleMarkdown(markdown: string): string; + renderUserMessageContent(text: string): string; + runUserInputCase(message: { data: string; timestamp?: string }): void; +} + +// Splices together the REAL extracted addMessage/parseSimpleMarkdown/extractCodeBlocks/ +// renderUserMessageContent (same recipe fork-issue-62's loadCodeBlockSandbox already uses for +// parseSimpleMarkdown's own dependencies) and the REAL case 'userInput': block, wrapped +// as a callable function. Only addMessage's own free variables that are unrelated to fork-issue-63 (scroll +// position, the copy-button raw-text map, the processing indicator, the two error-only helpers) +// are stubbed -- see the inline comments on each stub below for exactly why each one is safe to +// skip rather than extract. No timestamp/formatMessageTimestamp here -- that's a separate, +// unrelated feature not part of this security-hardening branch; addMessage here takes only +// (content, type), matching the actual signature in this branch. +function loadUserInputPipelineSandbox(): { sandbox: UserInputPipelineSandbox; messagesDiv: FakeMessagesDiv } { + const body = getEmittedScriptBody(); + + const addMessageSrc = extractFunction(body, 'addMessage'); + assertCleanExtraction('addMessage', addMessageSrc, 'function addMessage('); + + const parseSimpleMarkdownSrc = extractFunction(body, 'parseSimpleMarkdown'); + assertCleanExtraction('parseSimpleMarkdown', parseSimpleMarkdownSrc, 'function parseSimpleMarkdown('); + + const renderUserMessageContentSrc = extractFunction(body, 'renderUserMessageContent'); + assertCleanExtraction('renderUserMessageContent', renderUserMessageContentSrc, 'function renderUserMessageContent('); + + const caseBody = extractCaseBlock(body, 'userInput'); + assert.ok(caseBody.includes('addMessage('), 'case \'userInput\': block does not call addMessage; got: ' + caseBody); + assert.ok(!/\bbreak\s*;/.test(caseBody), 'case \'userInput\': extraction appears to have swallowed more than one case (unexpected nested break;); got: ' + caseBody); + + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'normalizeCollapseThreshold'), + extractFunction(body, 'evaluateCodeBlockCollapse'), + // restoreCodeBlockPlaceholders (fork-issue-55): script.ts splices this in via + // `${restoreCodeBlockPlaceholders.toString()}` (build-time, see markdown-restore.ts), so it + // appears in the emitted body as an ordinary function declaration extractFunction can find -- + // both parseSimpleMarkdownSrc and renderUserMessageContentSrc below call it. + extractFunction(body, 'restoreCodeBlockPlaceholders'), + extractFunction(body, 'extractCodeBlocks'), + parseSimpleMarkdownSrc, + renderUserMessageContentSrc, + addMessageSrc, + // fork-issue-46's copy-button raw-text store -- addMessage only .set()s into it, never reads it + // back, so a real (empty) WeakMap is enough, no extraction needed. + 'let messageRawText = new WeakMap();', + // Scroll-position bookkeeping, entirely orthogonal to what/how the message text renders. + 'function shouldAutoScroll() { return false; }', + 'function scrollToBottomIfNeeded() { /* no-op */ }', + // Needs isProcessing/showProcessingIndicator (unrelated globals) -- addMessage only ever + // calls it, never inspects its result. + 'function moveProcessingIndicatorToLast() { /* no-op */ }', + // Only reached when type === "error" (isPermissionError) -- never for the type === "user" + // tests below. + 'function isPermissionError() { return false; }', + 'function runUserInputCase(message) {\n' + caseBody + '\n}', + ].join('\n'); + + const messagesDiv: FakeMessagesDiv = { + scrollTop: 0, + scrollHeight: 0, + clientHeight: 0, + lastAppended: undefined, + appendChild(child: FakeNode) { this.lastAppended = child; return child; } + }; + const sandbox: Record = { + collapseLongCodeBlocks: true, + collapseCodeBlockLines: 20, + document: { + getElementById(id: string) { + if (id !== 'messages') { throw new Error('unexpected document.getElementById(' + id + ')'); } + return messagesDiv; + }, + createElement(tag: string) { + if (tag !== 'div' && tag !== 'button' && tag !== 'span') { + throw new Error('unexpected document.createElement(' + tag + ')'); + } + return new FakeNode(tag); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as UserInputPipelineSandbox, messagesDiv }; +} + +// Runs the REAL case 'userInput': block (as compiled right now) against a mock incoming message +// and returns the resulting message element's outerHTML. Deliberately does NOT decide itself how +// the text gets rendered -- that's exactly what's compiled into caseBody, so this genuinely +// renders the pre-fork-issue-63 markup-wrapped output against the old source, the flattened-code-block +// output against the intermediate textContent-only fix, and the raw-text-with-real-code-blocks +// output against the current source, without the test having to know which one it's looking at. +function renderUserInputMessage(payload: string): string { + const { sandbox, messagesDiv } = loadUserInputPipelineSandbox(); + sandbox.runUserInputCase({ data: payload, timestamp: undefined }); + if (!messagesDiv.lastAppended) { + throw new Error('case \'userInput\' did not append a message for payload: ' + payload); + } + return messagesDiv.lastAppended.outerHTML; +} + +suite('webview user-message raw text rendering (fork-issue-63)', () => { + + test('a markdown-looking payload with no code fence renders as exactly that text -- no // elements', () => { + const payload = 'Please check **src/_test_.ts** and `foo_bar`'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'message-content must contain the exact raw text; got: ' + html); + assert.ok(!tagExists(html, 'strong'), 'must not render a element; got: ' + html); + assert.ok(!tagExists(html, 'em'), 'must not render an element; got: ' + html); + assert.ok(!tagExists(html, 'code'), 'must not render a element (no fenced block in this payload); got: ' + html); + }); + + test('an XSS payload with no code fence renders as inert text -- no element and no onerror attribute', () => { + const payload = ''; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'message-content must contain the exact raw text; got: ' + html); + assert.ok(!tagExists(html, 'img'), 'must not render an element; got: ' + html); + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'message-content must not carry an onerror attribute; got: ' + html); + }); + + test('a multi-line payload with no code fence keeps its line breaks exactly', () => { + const payload = 'first line\nsecond line\nthird line'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'line breaks must survive exactly; got: ' + html); + }); + + test('a plain payload with no special characters still renders visibly (no functional regression)', () => { + const payload = 'hello world'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload); + }); + + test('a fenced code block still renders as a real code-block-container (language label, copy button, exact data-raw-code), while the surrounding markdown-looking prose stays raw', () => { + const payload = 'Please **test** this:\n```js\nconst x = 1;\n```\nThanks `dir`'; + const html = renderUserInputMessage(payload); + + // The fence itself is consumed by the real code-block element; the prose on either side + // stays exactly as typed (including its own "**"/backtick markdown-looking syntax). + assert.strictEqual( + textOn(html, 'message-content'), + 'Please **test** this:\n\nThanks `dir`', + 'prose around the code block must stay raw and unwrapped; got: ' + html + ); + assert.ok(!tagExists(html, 'strong'), 'the prose\'s own "**test**" must not become a element; got: ' + html); + assert.ok(!tagExists(html, 'em'), 'must not render an element; got: ' + html); + + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'sanity: message-content itself must not carry an onerror attribute; got: ' + html); + const dataRawCode = findAttrOn(html, 'language-js', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-js code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code (fork-issue-62 -- getAttribute()-equivalent, escapeAttr-escaped); got: ' + html); + assert.strictEqual(textOn(html, 'code-block-language'), 'js', 'the code block\'s language label must read "js"; got: ' + html); + assert.ok(tagExists(html, 'button'), 'expected the code block\'s copy button (fork-issue-48/fork-issue-62 copyCodeBlock) to exist; got: ' + html); + }); + + test('an XSS payload INSIDE a fenced code block renders as inert text -- no element anywhere, data-raw-code holds the exact raw code', () => { + const payload = '```\n\n```'; + const html = renderUserInputMessage(payload); + + assert.ok(!tagExists(html, 'img'), 'must not render a live element inside the code block; got: ' + html); + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'sanity: message-content itself must not carry an onerror attribute; got: ' + html); + // No prose at all -- the payload is nothing but the fenced block. + assert.strictEqual(textOn(html, 'message-content'), '', 'a message that is only a code block must have no direct prose text; got: ' + html); + + const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, '\n', 'data-raw-code must hold the exact raw (unescaped-at-read-time) code; got: ' + html); + // The rendered .code-line text must be the decoded literal string too (proves each code + // line is escapeHtml()'d, not parsed as HTML, same guarantee as parseSimpleMarkdown's + // existing code-block rendering). + assert.strictEqual(textOn(html, 'code-line'), '', 'the rendered code line must be the exact literal text, not a parsed element; got: ' + html); + }); + + test('Claude\'s own messages are unaffected -- addMessage still renders pre-built HTML via innerHTML for type "claude"', () => { + const { sandbox, messagesDiv } = loadUserInputPipelineSandbox(); + sandbox.addMessage( + '

Please check src/_test_.ts

', + 'claude' + ); + if (!messagesDiv.lastAppended) { + throw new Error('addMessage did not append a message'); + } + const html = messagesDiv.lastAppended.outerHTML; + assert.ok(tagExists(html, 'strong'), 'claude messages must still render pre-parsed HTML markup; got: ' + html); + assert.strictEqual(textOn(html, 'message-content'), '', 'the text must live inside the

/, not as message-content\'s own direct text; got: ' + html); + }); + + test('Claude\'s own fenced code blocks (via the real parseSimpleMarkdown) still render correctly after the extractCodeBlocks refactor', () => { + const { sandbox } = loadUserInputPipelineSandbox(); + const rendered = sandbox.parseSimpleMarkdown('Please **test** this:\n```js\nconst x = 1;\n```\nThanks'); + // Claude's own prose IS markdown-parsed -- "**test**" becomes a real , unlike the + // user-message tests above. + assert.ok(tagExists(rendered, 'strong'), 'parseSimpleMarkdown must still render "**test**" as for Claude messages; got: ' + rendered); + const dataRawCode = findAttrOn(rendered, 'language-js', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-js code element; got: ' + rendered); + assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code; got: ' + rendered); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// PoC for the code-block restore fix: extractCodeBlocks' per-block HTML (data-raw-code attribute plus escaped +// code-line divs) does not strip "$"/"`"/"'" characters, so a code block whose content +// contains one of the String.replace(placeholder, string) substitution sequences +// ("$&"/"$`"/"$'"/"$$") reaches the placeholder-restore step still intact. Both +// parseSimpleMarkdown and renderUserMessageContent (fork-issue-63) restore their __CODEBLOCK_N__ +// placeholders via the shared restoreCodeBlockPlaceholders (fork-issue-55) -- a FUNCTION replacer, +// immune to this. These tests exercise the REAL, full pipeline (extractCodeBlocks + +// escapeHtml/escapeAttr + restoreCodeBlockPlaceholders, not a direct unit call to +// restoreCodeBlockPlaceholders as in markdown-restore.test.ts) through both entry points and +// prove the surrounding prose is not corrupted: a plain html.replace(placeholder, codeBlockHtml) +// here would splice codeBlockHtml's own preceding/following HTML ("$`"/"$'") or a literal "$" +// duplicate ("$$") into the restored output, which would show up as MARKER_BEFORE/MARKER_AFTER +// appearing more than once and/or a corrupted data-raw-code value. +// ───────────────────────────────────────────────────────────────────────── + +suite('webview code-block restore: "$" substitution patterns cannot corrupt surrounding HTML (fork-issue-70, full pipeline)', () => { + + // Covers all four String.replace substitution sequences at once: "$&" (matched substring), + // "$`" (pre-match), "$'" (post-match), "$$" (literal "$"). + const dollarPayload = 'echo $& run; VAR=$`date`; MSG=$\'ok\'; echo $$'; + + test('parseSimpleMarkdown: a code block containing "$&"/"$`"/"$\'"/"$$" does not duplicate the surrounding prose and round-trips byte-for-byte', () => { + const { sandbox } = loadUserInputPipelineSandbox(); + const markdown = 'MARKER_BEFORE\n\n```\n' + dollarPayload + '\n```\n\nMARKER_AFTER'; + const html = sandbox.parseSimpleMarkdown(markdown); + + assert.strictEqual(html.split('MARKER_BEFORE').length - 1, 1, 'MARKER_BEFORE must appear exactly once, not spliced into the code block; got: ' + html); + assert.strictEqual(html.split('MARKER_AFTER').length - 1, 1, 'MARKER_AFTER must appear exactly once, not spliced into the code block; got: ' + html); + + const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, dollarPayload + '\n', 'data-raw-code must contain the exact fenced code, dollar sequences untouched; got: ' + html); + assert.strictEqual(textOn(html, 'code-line'), dollarPayload, 'the rendered code line must be the exact literal text; got: ' + html); + }); + + test('renderUserMessageContent: a code block containing "$&"/"$`"/"$\'"/"$$" does not duplicate the surrounding raw-text prose and round-trips byte-for-byte', () => { + const { sandbox } = loadUserInputPipelineSandbox(); + const text = 'MARKER_BEFORE\n```\n' + dollarPayload + '\n```\nMARKER_AFTER'; + const html = sandbox.renderUserMessageContent(text); + + assert.strictEqual(html.split('MARKER_BEFORE').length - 1, 1, 'MARKER_BEFORE must appear exactly once, not spliced into the code block; got: ' + html); + assert.strictEqual(html.split('MARKER_AFTER').length - 1, 1, 'MARKER_AFTER must appear exactly once, not spliced into the code block; got: ' + html); + + const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, dollarPayload + '\n', 'data-raw-code must contain the exact fenced code, dollar sequences untouched; got: ' + html); + assert.strictEqual(textOn(html, 'code-line'), dollarPayload, 'the rendered code line must be the exact literal text; got: ' + html); + }); +}); diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts new file mode 100644 index 0000000..18a74b9 --- /dev/null +++ b/src/test/webview-attr-escape.test.ts @@ -0,0 +1,947 @@ +// PoC/regression tests for fork-issue-57 (HTML attribute / inline-handler injection via escapeHtml() +// used in attribute contexts). fork-issue-49's escapeHtml() serialises through +// textContent->innerHTML and therefore leaves " and ' untouched -- fine for element text +// content, but not for attribute values (title="...", data-*="...", src="...", href="...", +// value="...") or for a value embedded as a JS string argument inside an inline +// onclick="fn('...')" handler. escapeAttr() (also escapes " and ') plus the +// data-*/this.dataset.* pattern (see fork-issue-58's renderPermissions) is the fix for both sinks. +// +// formatFilePath/formatToolInputUI only exist inline inside script.ts's giant getScript() +// template literal -- never as an importable module, unlike escapeAttr/ +// evaluateCodeBlockCollapse -- so this suite extracts their exact +// source text from the ACTUAL getScript() output (out/script.js, i.e. the real emitted webview +// code, not a hand-copied version of the TS source) via extractFunction() (webview-dom-helpers.ts, +// parser-based since fork-issue-64), runs it in a vm sandbox with the one stub escapeHtml() needs +// (document.createElement), and parses the resulting HTML string with parse5 -- the same +// HTML5-spec parser class real browsers use -- to assert no attribute-breakout / inline-handler- +// breakage survives. Run with `npm run test:webview-attr-escape`. + +import * as assert from 'assert'; +import * as vm from 'vm'; +import * as parse5 from 'parse5'; +import getScript from '../script'; +import { extractFunction, findAttr, findAttrOn, textOn, tagExists } from './webview-dom-helpers'; + +function getEmittedScriptBody(): string { + const html = getScript(false); + const match = /]*)?>([\s\S]*?)<\/script>/.exec(html); + if (!match) { + throw new Error('getScript(false) did not contain a block'); + } + return match[1]; +} + +// escapeHtml()'s only external dependency is document.createElement('div') (textContent set, +// innerHTML read). Per the HTML fragment serialisation spec, a Text node's innerHTML escapes +// only & < > (not " or ') -- real browsers behave the same; this is a faithful minimal stub. +class FakeDiv { + private _text = ''; + set textContent(v: string) { this._text = String(v); } + get innerHTML(): string { + return this._text.replace(/&/g, '&').replace(//g, '>'); + } +} + +interface Sandbox { + formatFilePath(filePath: string): string; + formatToolInputUI(input: unknown): string; +} + +function loadSandbox(): Sandbox { + const body = getEmittedScriptBody(); + const src = ['escapeHtml', 'escapeAttr', 'formatFilePath', 'formatToolInputUI'] + .map(name => extractFunction(body, name)) + .join('\n'); + const sandbox: Record = { + document: { + createElement(tag: string) { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeDiv(); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return sandbox as unknown as Sandbox; +} + +suite('webview attribute escaping: formatFilePath / formatToolInputUI (fork-issue-57 PoC)', () => { + + test('an attribute-breakout file_path (a" onmouseover="alert(1)" zz=") produces no onmouseover attribute anywhere in the emitted element', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatToolInputUI({ file_path: payload }); + assert.ok(!findAttr(html, 'onmouseover'), 'emitted HTML must not contain an onmouseover attribute; got: ' + html); + }); + + test('the same payload also stays contained when passed straight through formatFilePath (the fork-issue-57 reference sink)', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatFilePath(payload); + assert.ok(!findAttr(html, 'onmouseover'), 'emitted HTML must not contain an onmouseover attribute; got: ' + html); + }); + + test('the title and data-file-path attributes decode back to the exact raw payload (properly escaped, not mangled)', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatFilePath(payload); + const title = findAttr(html, 'title'); + const dataFilePath = findAttr(html, 'data-file-path'); + assert.strictEqual(title && title.value, payload); + assert.strictEqual(dataFilePath && dataFilePath.value, payload); + }); + + test('a file path with an apostrophe (C:\\tmp\\it\'s\\a.txt) keeps the openFileInEditor handler syntactically intact', () => { + const sandbox = loadSandbox(); + const filePath = 'C:\\tmp\\it\'s\\a.txt'; + const html = sandbox.formatToolInputUI({ file_path: filePath }); + const onclick = findAttr(html, 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on the emitted element; got: ' + html); + // The path is never embedded as a JS string literal in the handler (which an + // apostrophe would break) -- it's read from the element's own dataset instead, so the + // handler text itself is a fixed, path-independent string. + assert.strictEqual(onclick!.value, 'openFileInEditor(this.dataset.filePath)'); + }); + + test('data-file-path round-trips the exact original apostrophe path on the div that actually owns the onclick handler (matching how el.dataset.filePath reads it)', () => { + const sandbox = loadSandbox(); + const filePath = 'C:\\tmp\\it\'s\\a.txt'; + const html = sandbox.formatToolInputUI({ file_path: filePath }); + // findAttrOn, not findAttr: formatFilePath's inner span also carries a data-file-path + // copy, but this.dataset.filePath in the onclick handler reads it from the outer + // div.diff-file-path specifically -- that's the element that must be checked. + const dataFilePath = findAttrOn(html, 'diff-file-path', 'data-file-path'); + assert.ok(dataFilePath, 'expected a data-file-path attribute on div.diff-file-path; got: ' + html); + assert.strictEqual(dataFilePath!.value, filePath); + }); + + test('a plain file path with no special characters still renders visibly (no functional regression)', () => { + const sandbox = loadSandbox(); + const html = sandbox.formatToolInputUI({ file_path: '/home/user/project/index.ts' }); + assert.ok(html.includes('index.ts'), 'expected the file name to appear in the rendered output; got: ' + html); + const dataFilePath = findAttrOn(html, 'diff-file-path', 'data-file-path'); + assert.strictEqual(dataFilePath && dataFilePath.value, '/home/user/project/index.ts'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-60 PoC: displayMCPServers() built the Edit/Delete buttons' onclick handlers by +// interpolating the raw server name (and, for Edit, JSON.stringify(config)) directly into a +// JS string literal inside an HTML attribute -- with NO escaping at all (not even the fork-issue-57-era +// escapeHtml() mistake). A server name of x'); alert(1); (' from a workspace's own .mcp.json +// (loaded with _scope: 'project', see extension.ts) broke straight out of +// editMCPServer('...', ...) and ran arbitrary JS in a CSP of default-src * 'unsafe-inline' +// 'unsafe-eval'. Fix: name (and scope, for Delete) move into data-* (escapeAttr) + +// this.dataset.*, same pattern as fork-issue-57/fork-issue-58. The config object itself can never be +// escapeAttr()'d sanely as a JS-object-literal attribute value, so it no longer touches an +// attribute at all -- it's kept in a webview-scope map (mcpServerConfigsByName) and +// editMCPServer looks it up by name. +// ───────────────────────────────────────────────────────────────────────── + +// Extracts a top-level "let NAME = ...;" declaration's exact source text (used for +// mcpServerConfigsByName/editingServerName, which extractFunction() can't grab -- they're not +// function declarations). +function extractDeclaration(source: string, name: string): string { + const match = new RegExp('let\\s+' + name + '\\s*=\\s*[^;]+;').exec(source); + if (!match) { + throw new Error('declaration for ' + name + ' not found in emitted script'); + } + return match[0]; +} + +// Minimal DOM stand-in for displayMCPServers/editMCPServer/updateServerForm. All three only +// ever call getElementById(id).{value,disabled,textContent,style.display,innerHTML,className}, +// document.createElement('div'), element.appendChild(child), element.insertAdjacentHTML(...) +// (cosmetic only -- edit-form h5 title, never security-relevant), and document.querySelector( +// ...) (also only cosmetic lookups in editMCPServer). A single auto-vivifying registry keyed +// by id lets assertions read back e.g. elements populated by editMCPServer after the call. +class FakeElement { + className = ''; + value = ''; + disabled = false; + style: Record = {}; + children: FakeElement[] = []; + private _innerHTML = ''; + // Mirrors FakeDiv above (escapeHtml()'s only external dependency): setting textContent + // replaces the content with an HTML-escaped text serialization, exactly what escapeHtml() + // reads back via innerHTML -- without this, escapeHtml() (used for .server-name/.server-type + // text) silently returns '' for every input inside this sandbox. displayMCPServers/ + // editMCPServer, by contrast, only ever WRITE innerHTML directly (a full template string) + // and never read it back through textContent, so a plain passthrough setter is enough there. + set textContent(v: string) { + this._innerHTML = String(v).replace(/&/g, '&').replace(//g, '>'); + } + get innerHTML(): string { return this._innerHTML; } + set innerHTML(v: string) { this._innerHTML = v; } + appendChild(child: FakeElement): FakeElement { this.children.push(child); return child; } + insertAdjacentHTML(): void { /* cosmetic only */ } + // fork-issue-61: renderAllModels()/renderDropdown() wire click handlers via + // listContainer.querySelectorAll(...).forEach(...) -- an empty array is enough here since + // none of these PoCs need the click wiring itself, only the innerHTML the sinks produce. + querySelectorAll(): FakeElement[] { return []; } +} + +class FakeDocument { + elementsById = new Map(); + getElementById(id: string): FakeElement { + let el = this.elementsById.get(id); + if (!el) { el = new FakeElement(); this.elementsById.set(id, el); } + return el; + } + createElement(tag: string): FakeElement { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeElement(); + } + querySelector(): FakeElement { + return new FakeElement(); + } +} + +interface McpSandbox { + displayMCPServers(servers: Record): void; + editMCPServer(name: string): void; + // Test-only inspection shim -- top-level "let" bindings (editingServerName, + // mcpServerConfigsByName) live in the vm Script's lexical scope, not as properties on the + // sandbox/global object, so they aren't readable from outside via plain property access + // (only top-level function/var declarations become global-object properties -- see + // loadSandbox() above, which already relies on that half of the same rule). __testState is + // never part of the real script.ts output; it only exists to expose those two "let"s here. + __testState(): { editingServerName: string | null; configsByName: Record }; +} + +function loadMcpSandbox(): { sandbox: McpSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractDeclaration(body, 'editingServerName'), + extractDeclaration(body, 'mcpServerConfigsByName'), + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'updateServerForm'), + extractFunction(body, 'displayMCPServers'), + extractFunction(body, 'editMCPServer'), + 'function __testState() { return { editingServerName: editingServerName, configsByName: mcpServerConfigsByName }; }', + ].join('\n'); + const sandbox: Record = { document }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as McpSandbox, document }; +} + +// Renders `servers` and returns the .mcp-server-item div's innerHTML for the given server name +// plus the sandbox/document for further assertions (mcpServerConfigsByName via __testState(), +// or calling editMCPServer(...) next). +function renderServerItem(servers: Record, name: string): { html: string; sandbox: McpSandbox; document: FakeDocument } { + const { sandbox, document } = loadMcpSandbox(); + sandbox.displayMCPServers(servers); + const serversList = document.getElementById('mcpServersList'); + const index = Object.keys(servers).indexOf(name); + const item = serversList.children[index]; + if (!item) { + throw new Error('displayMCPServers did not append an item for "' + name + '"; got ' + serversList.children.length + ' item(s)'); + } + return { html: item.innerHTML, sandbox, document }; +} + +suite('webview attribute escaping: displayMCPServers / editMCPServer (fork-issue-60 PoC)', () => { + + test('an attribute-breakout server name (x\'); alert(1); (\') cannot inject extra JS statements -- the onclick text is always the fixed, name-independent dataset call', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclicks = [...html.matchAll(/onclick="([^"]*)"/g)].map(m => m[1]); + assert.ok(onclicks.length > 0, 'expected at least one onclick attribute; got: ' + html); + for (const oc of onclicks) { + assert.ok( + oc === 'editMCPServer(this.dataset.serverName)' || oc === 'deleteMCPServer(this.dataset.serverName, this.dataset.serverScope)', + 'onclick handler must be exactly the fixed dataset-based call, got: ' + oc + ); + } + }); + + test('a server name with a double quote (x" onmouseover=...) stays contained -- no extra attribute breaks out', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + assert.ok(!findAttr(html, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + html); + }); + + test('the Edit button\'s onclick is exactly editMCPServer(this.dataset.serverName), independent of the name', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclick = findAttrOn(html, 'server-edit-btn', 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on .server-edit-btn; got: ' + html); + assert.strictEqual(onclick!.value, 'editMCPServer(this.dataset.serverName)'); + }); + + test('the Delete button\'s onclick is exactly deleteMCPServer(this.dataset.serverName, this.dataset.serverScope), independent of the name', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclick = findAttrOn(html, 'server-delete-btn', 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on .server-delete-btn; got: ' + html); + assert.strictEqual(onclick!.value, 'deleteMCPServer(this.dataset.serverName, this.dataset.serverScope)'); + }); + + test('data-server-name on both buttons round-trips the exact raw payload', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + const deleteName = findAttrOn(html, 'server-delete-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, payload); + assert.strictEqual(deleteName && deleteName.value, payload); + }); + + test('mcpServerConfigsByName holds the exact original config object after rendering (replaces the old JSON.stringify-in-attribute round-trip)', () => { + const payload = 'x\'); alert(1); (\''; + const config = { type: 'stdio', command: 'echo', args: ['a', 'b'] }; + const { sandbox } = renderServerItem({ [payload]: config }, payload); + assert.strictEqual(sandbox.__testState().configsByName[payload], config); + }); + + test('editMCPServer(name), called with the exact string the rendered data-server-name attribute decodes to, looks up the right config and does not throw', () => { + const payload = 'x\'); alert(1); (\''; + const config = { type: 'http', url: 'https://example.com/mcp' }; + const { html, sandbox, document } = renderServerItem({ [payload]: config }, payload); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, payload); + assert.doesNotThrow(() => sandbox.editMCPServer(editName!.value)); + assert.strictEqual(sandbox.__testState().editingServerName, payload); + assert.strictEqual(document.getElementById('serverType').value, 'http'); + assert.strictEqual(document.getElementById('serverUrl').value, 'https://example.com/mcp'); + }); + + test('a malicious config.type () renders as inert text in .server-type, not a live element', () => { + const payload = ''; + const { html } = renderServerItem({ srv: { type: payload } }, 'srv'); + assert.ok(!findAttr(html, 'onerror'), 'must not contain an onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'server-type'), payload.toUpperCase()); + }); + + test('a plain server name/config with no special characters still renders visibly (no functional regression)', () => { + const { html, sandbox } = renderServerItem({ 'my-server': { type: 'stdio', command: 'node' } }, 'my-server'); + assert.ok(html.includes('my-server'), 'expected the server name to appear in the rendered output; got: ' + html); + assert.strictEqual(textOn(html, 'server-type'), 'STDIO'); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, 'my-server'); + assert.strictEqual((sandbox.__testState().configsByName['my-server'] as { command: string }).command, 'node'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-61 Part A PoC: renderDropdown() (the model combo box built by initModelCombo()) and +// renderAllModels() (the "all models" modal) wrote model.id/model.name/model.owned_by +// straight into data-id=/data-model-id= attributes and innerHTML text -- with NO escaping at +// all (not even the fork-issue-57-era escapeHtml()-in-attribute-context mistake). The models come from +// fetch(OPENCREDITS_API_URL + '/v1/models'), a third-party HTTP endpoint outside our control. +// Fix: escapeAttr() for the data-id/data-model-id attribute values, escapeHtml() for the +// name/id/owned_by text nodes -- same split as fork-issue-57's fix. +// ───────────────────────────────────────────────────────────────────────── + +interface DropdownSandbox { + renderDropdown(query: string): void; +} + +function loadDropdownSandbox(models: unknown[]): { sandbox: DropdownSandbox; dropdown: FakeElement } { + const body = getEmittedScriptBody(); + const dropdown = new FakeElement(); + // fork-issue-64 (fixed): extractFunction('escapeAttr') used to also drag in + // safeHttpUrl/openFileInEditor/formatFilePath/toggleDiffExpansion/toggleResultExpansion -- + // escapeAttr's own /'/g regex literal desynced the old brace-matcher's naive quote tracking. + // extractFunction is now parser-based (webview-dom-helpers.ts) and returns exactly the + // escapeAttr function, nothing more. + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'renderDropdown'), + ].join('\n'); + // renderDropdown is a nested function (closure over initModelCombo's dropdown/input/combo + // locals) -- allModelsCache and dropdown are set directly as sandbox globals instead of + // being declared inside the vm script, which resolves the same way a free variable would. + // dropdown.querySelectorAll(...) returning [] means the mousedown-listener closure (which + // references input/combo) is created but never invoked, so those two stay undefined-but- + // unused without throwing. document.createElement('div') is escapeHtml()'s own stub (FakeDiv). + const sandbox: Record = { + allModelsCache: models, + dropdown, + document: { + createElement(tag: string) { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeDiv(); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as DropdownSandbox, dropdown }; +} + +suite('webview attribute escaping: renderDropdown model combo box (fork-issue-61 Part A PoC)', () => { + + test('an attribute-breakout model id stays contained -- no onmouseover attribute survives, and data-id round-trips the exact raw payload', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { sandbox, dropdown } = loadDropdownSandbox([{ id: payload, name: 'Model' }]); + sandbox.renderDropdown(''); + assert.ok(!findAttr(dropdown.innerHTML, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + dropdown.innerHTML); + const dataId = findAttrOn(dropdown.innerHTML, 'model-combo-option', 'data-id'); + assert.strictEqual(dataId && dataId.value, payload); + }); + + test('an payload in the model name renders as inert text, not a live element', () => { + const payload = ''; + const { sandbox, dropdown } = loadDropdownSandbox([{ id: 'm1', name: payload }]); + sandbox.renderDropdown(''); + assert.ok(!findAttr(dropdown.innerHTML, 'onerror'), 'must not contain an onerror attribute; got: ' + dropdown.innerHTML); + assert.strictEqual(textOn(dropdown.innerHTML, 'model-combo-option-name'), payload); + }); + + test('an attribute-breakout search query also stays contained in the "use as custom model" row', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { sandbox, dropdown } = loadDropdownSandbox([]); + sandbox.renderDropdown(payload); + assert.ok(!findAttr(dropdown.innerHTML, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + dropdown.innerHTML); + const dataId = findAttrOn(dropdown.innerHTML, 'model-combo-custom', 'data-id'); + assert.strictEqual(dataId && dataId.value, payload); + }); + + test('a plain model with no special characters still renders visibly (no functional regression)', () => { + const { sandbox, dropdown } = loadDropdownSandbox([{ id: 'gpt-4', name: 'GPT-4' }]); + sandbox.renderDropdown(''); + assert.ok(dropdown.innerHTML.includes('GPT-4'), 'expected the model name to appear; got: ' + dropdown.innerHTML); + const dataId = findAttrOn(dropdown.innerHTML, 'model-combo-option', 'data-id'); + assert.strictEqual(dataId && dataId.value, 'gpt-4'); + }); +}); + +interface AllModelsSandbox { + renderAllModels(models: unknown[]): void; +} + +function loadAllModelsSandbox(): { sandbox: AllModelsSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'renderAllModels'), + ].join('\n'); + const sandbox: Record = { document, currentModel: 'opus' }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as AllModelsSandbox, document }; +} + +suite('webview attribute escaping: renderAllModels "all models" modal (fork-issue-61 Part A PoC)', () => { + + test('an attribute-breakout model id stays contained -- no onmouseover attribute survives, and data-model-id round-trips the exact raw payload', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { sandbox, document } = loadAllModelsSandbox(); + sandbox.renderAllModels([{ id: payload, name: 'Model' }]); + const html = document.getElementById('allModelsList').innerHTML; + assert.ok(!findAttr(html, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + html); + const dataModelId = findAttrOn(html, 'all-models-item', 'data-model-id'); + assert.strictEqual(dataModelId && dataModelId.value, payload); + }); + + test('an payload in the model name renders as inert text', () => { + const payload = ''; + const { sandbox, document } = loadAllModelsSandbox(); + sandbox.renderAllModels([{ id: 'm1', name: payload }]); + const html = document.getElementById('allModelsList').innerHTML; + assert.ok(!findAttr(html, 'onerror'), 'must not contain an onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'all-models-item-name'), payload); + }); + + test('an payload in owned_by renders as inert text', () => { + const payload = ''; + const { sandbox, document } = loadAllModelsSandbox(); + sandbox.renderAllModels([{ id: 'm1', name: 'Model', owned_by: payload }]); + const html = document.getElementById('allModelsList').innerHTML; + assert.ok(!findAttr(html, 'onerror'), 'must not contain an onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'all-models-item-provider'), payload); + }); + + test('a plain model with no special characters still renders visibly (no functional regression)', () => { + const { sandbox, document } = loadAllModelsSandbox(); + sandbox.renderAllModels([{ id: 'gpt-4', name: 'GPT-4', owned_by: 'openai' }]); + const html = document.getElementById('allModelsList').innerHTML; + assert.ok(html.includes('GPT-4'), 'expected the model name to appear; got: ' + html); + const dataModelId = findAttrOn(html, 'all-models-item', 'data-model-id'); + assert.strictEqual(dataModelId && dataModelId.value, 'gpt-4'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-61 Part A follow-up: openCreditsModels is overwritten wholesale by +// resolveLatestModels() (model-updater.ts) from fetch(apiBaseUrl + '/v1/models') -- the SAME +// third-party endpoint as renderDropdown/renderAllModels above, just reached indirectly via +// extension.ts's 'updateRecommendedModels' postMessage -- and renderOpenCreditsModelCards() +// (the model-card grid shown by default whenever OpenCredits is enabled) runs unconditionally +// on that update, with no user interaction gating it. It wrote model.id/model.provider/ +// model.name straight into data-model-id=/data-provider=/innerHTML with NO escaping. +// ───────────────────────────────────────────────────────────────────────── + +interface ModelCardsSandbox { + renderOpenCreditsModelCards(): void; +} + +function loadModelCardsSandbox(openCreditsModels: unknown[]): { sandbox: ModelCardsSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'isModelMatch'), + extractFunction(body, 'getCreditsPricing'), + extractFunction(body, 'renderOpenCreditsModelCards'), + ].join('\n'); + const sandbox: Record = { + document, + openCreditsModels, + currentModel: 'opus', + pendingModelSelection: null, + hasOpenCreditsKey: false, + creditsPricingData: null, + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as ModelCardsSandbox, document }; +} + +suite('webview attribute escaping: renderOpenCreditsModelCards model-card grid (fork-issue-61 Part A follow-up PoC)', () => { + + test('an payload in model.name renders as inert text, not a live element (the review finding)', () => { + const payload = ''; + const { sandbox, document } = loadModelCardsSandbox([{ id: 'openai/gpt-9.9', name: payload, provider: 'openai' }]); + sandbox.renderOpenCreditsModelCards(); + const html = document.getElementById('opencreditsModelCards').innerHTML; + assert.ok(!findAttr(html, 'onerror'), 'must not contain a live onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'model-card-name'), payload); + }); + + test('an attribute-breakout model id/provider stays contained -- no onmouseover attribute survives, and data-model-id/data-provider round-trip the exact raw payloads', () => { + const idPayload = 'x" onmouseover="alert(1)" y="'; + const providerPayload = 'z" onmouseover="alert(2)" w="'; + const { sandbox, document } = loadModelCardsSandbox([{ id: idPayload, name: 'Model', provider: providerPayload }]); + sandbox.renderOpenCreditsModelCards(); + const html = document.getElementById('opencreditsModelCards').innerHTML; + assert.ok(!findAttr(html, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + html); + const dataModelId = findAttrOn(html, 'model-card', 'data-model-id'); + const dataProvider = findAttrOn(html, 'model-card', 'data-provider'); + assert.strictEqual(dataModelId && dataModelId.value, idPayload); + assert.strictEqual(dataProvider && dataProvider.value, providerPayload); + }); + + test('an payload in model.provider renders as inert text in .model-card-provider', () => { + const payload = ''; + const { sandbox, document } = loadModelCardsSandbox([{ id: 'm1', name: 'Model', provider: payload }]); + sandbox.renderOpenCreditsModelCards(); + const html = document.getElementById('opencreditsModelCards').innerHTML; + assert.ok(!findAttr(html, 'onerror'), 'must not contain an onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'model-card-provider'), payload); + }); + + test('a plain model with no special characters still renders visibly (no functional regression)', () => { + const { sandbox, document } = loadModelCardsSandbox([{ id: 'openai/gpt-4', name: 'GPT-4', provider: 'openai' }]); + sandbox.renderOpenCreditsModelCards(); + const html = document.getElementById('opencreditsModelCards').innerHTML; + assert.ok(html.includes('GPT-4'), 'expected the model name to appear; got: ' + html); + const dataModelId = findAttrOn(html, 'model-card', 'data-model-id'); + assert.strictEqual(dataModelId && dataModelId.value, 'openai/gpt-4'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-61 Part B PoC: renderMarketplace() and showMarketplaceDetail() already ran the MCP +// registry's icon/url values through escapeAttr() (post-fork-issue-57), making src=/href= itself +// ausbruchsicher -- but neither ever checked the URL SCHEME, so a "javascript:" (or oddly-cased +// / whitespace-obfuscated) src=/href= still rendered and would execute on click/load. The data +// comes from registry.modelcontextprotocol.io / mcp.agent-tooling.dev, publishable by anyone. +// Fix: safeHttpUrl() only lets http:/https: through; the caller then omits the attribute/link +// entirely (icon placeholder / no GitHub link) instead of rendering a dead attribute. +// ───────────────────────────────────────────────────────────────────────── + +function classExists(html: string, cssClass: string): boolean { + const frag = parse5.parseFragment(html); + let found = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (found) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { found = true; return; } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + return found; +} + +interface MarketplaceSandbox { + renderMarketplace(servers: unknown[], isLoading?: boolean): void; + showMarketplaceDetail(serverId: string): void; +} + +// marketplaceDisplayed/marketplaceCache/lastSearchQuery are plain "var" module-level state in +// script.ts, read as free variables by renderMarketplace/showMarketplaceDetail -- passed in as +// sandbox globals here (never declared inside the vm script itself), same technique as +// allModelsCache/currentModel/dropdown above. +function loadMarketplaceSandbox(marketplaceDisplayed: unknown[]): { sandbox: MarketplaceSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'safeHttpUrl'), + extractFunction(body, 'renderMarketplace'), + extractFunction(body, 'showMarketplaceDetail'), + ].join('\n'); + const sandbox: Record = { + document, + marketplaceDisplayed, + marketplaceCache: null, + lastSearchQuery: '' + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as MarketplaceSandbox, document }; +} + +suite('webview attribute escaping: MCP marketplace icon/link schema guard (fork-issue-61 Part B PoC)', () => { + + test('renderMarketplace: a "javascript:" icon URL is dropped -- no element, placeholder rendered instead', () => { + const { sandbox, document } = loadMarketplaceSandbox([]); + sandbox.renderMarketplace([{ id: 's1', name: 'Srv', icon: 'javascript:alert(1)' }]); + const html = document.getElementById('marketplaceGrid').innerHTML; + assert.ok(!tagExists(html, 'img'), 'must not render an element for a javascript: icon URL; got: ' + html); + assert.ok(classExists(html, 'marketplace-item-icon-placeholder'), 'expected the placeholder fallback instead; got: ' + html); + }); + + test('renderMarketplace: a plain https icon URL still renders normally (no functional regression)', () => { + const { sandbox, document } = loadMarketplaceSandbox([]); + sandbox.renderMarketplace([{ id: 's1', name: 'Srv', icon: 'https://example.com/icon.png' }]); + const html = document.getElementById('marketplaceGrid').innerHTML; + const src = findAttrOn(html, 'marketplace-item-icon', 'src'); + assert.strictEqual(src && src.value, 'https://example.com/icon.png'); + }); + + test('showMarketplaceDetail: a "javascript:" icon URL is dropped -- no element, placeholder rendered instead', () => { + const server = { id: 's1', name: 'Srv', icon: 'javascript:alert(1)', description: 'desc' }; + const { sandbox, document } = loadMarketplaceSandbox([server]); + sandbox.showMarketplaceDetail('s1'); + const html = document.getElementById('marketplaceGrid').innerHTML; + assert.ok(!tagExists(html, 'img'), 'must not render an element for a javascript: icon URL; got: ' + html); + assert.ok(classExists(html, 'marketplace-item-icon-placeholder'), 'expected the placeholder fallback instead; got: ' + html); + }); + + test('showMarketplaceDetail: a "javascript:" repository URL produces no GitHub link at all', () => { + const server = { id: 's1', name: 'Srv', url: 'javascript:alert(1)', description: 'desc' }; + const { sandbox, document } = loadMarketplaceSandbox([server]); + sandbox.showMarketplaceDetail('s1'); + const html = document.getElementById('marketplaceGrid').innerHTML; + assert.ok(!classExists(html, 'marketplace-detail-link'), 'must not render a GitHub link for a javascript: url; got: ' + html); + assert.ok(!findAttr(html, 'href'), 'must not contain an href attribute anywhere; got: ' + html); + }); + + test('showMarketplaceDetail: a mixed-case/whitespace-obfuscated "javascript:" repository URL is also blocked', () => { + const server = { id: 's1', name: 'Srv', url: 'Java\tScRiPt:alert(1)', description: 'desc' }; + const { sandbox, document } = loadMarketplaceSandbox([server]); + sandbox.showMarketplaceDetail('s1'); + const html = document.getElementById('marketplaceGrid').innerHTML; + assert.ok(!classExists(html, 'marketplace-detail-link'), 'must not render a GitHub link for an obfuscated javascript: url; got: ' + html); + assert.ok(!findAttr(html, 'href'), 'must not contain an href attribute anywhere; got: ' + html); + }); + + test('showMarketplaceDetail: a plain https repository URL still renders the GitHub link normally (no functional regression)', () => { + const server = { id: 's1', name: 'Srv', url: 'https://github.com/foo/bar', description: 'desc' }; + const { sandbox, document } = loadMarketplaceSandbox([server]); + sandbox.showMarketplaceDetail('s1'); + const html = document.getElementById('marketplaceGrid').innerHTML; + const href = findAttrOn(html, 'marketplace-detail-link', 'href'); + assert.strictEqual(href && href.value, 'https://github.com/foo/bar'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-61 Part C PoC: addEnvVariableRow() wrote key/value straight into value="..." with NO +// escaping at all. Self-XSS only (the values come from the user's own extension settings), but +// a '"' in a saved value truncates the rendered field instead of round-tripping. Fix: +// escapeAttr(), same pattern as the other fork-issue-57-era value="..." sinks. +// ───────────────────────────────────────────────────────────────────────── + +interface EnvRowSandbox { + addEnvVariableRow(key: string, value: string): void; +} + +function loadEnvRowSandbox(): { sandbox: EnvRowSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'addEnvVariableRow'), + ].join('\n'); + const sandbox: Record = { document }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as EnvRowSandbox, document }; +} + +suite('webview attribute escaping: addEnvVariableRow (fork-issue-61 Part C PoC)', () => { + + test('an attribute-breakout key stays contained -- no onmouseover attribute survives, and value="..." round-trips the exact raw payload', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { sandbox, document } = loadEnvRowSandbox(); + sandbox.addEnvVariableRow(payload, 'plain'); + const container = document.getElementById('env-variables-list'); + const html = container.children[0].innerHTML; + assert.ok(!findAttr(html, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + html); + const keyValue = findAttrOn(html, 'env-key', 'value'); + assert.strictEqual(keyValue && keyValue.value, payload); + }); + + test('a value containing a double quote no longer truncates the rendered field -- value="..." round-trips it exactly', () => { + const payload = 'sk-abc"123'; + const { sandbox, document } = loadEnvRowSandbox(); + sandbox.addEnvVariableRow('API_KEY', payload); + const container = document.getElementById('env-variables-list'); + const html = container.children[0].innerHTML; + const value = findAttrOn(html, 'env-value', 'value'); + assert.strictEqual(value && value.value, payload); + }); + + test('a plain key/value with no special characters still renders visibly (no functional regression)', () => { + const { sandbox, document } = loadEnvRowSandbox(); + sandbox.addEnvVariableRow('API_KEY', 'sk-abc123'); + const container = document.getElementById('env-variables-list'); + const html = container.children[0].innerHTML; + const keyValue = findAttrOn(html, 'env-key', 'value'); + const value = findAttrOn(html, 'env-value', 'value'); + assert.strictEqual(keyValue && keyValue.value, 'API_KEY'); + assert.strictEqual(value && value.value, 'sk-abc123'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-62 Part A PoC: copyCodeBlock() read data-raw-code via getAttribute() -- which the browser +// already entity-decodes during HTML parsing, since data-raw-code is built via escapeAttr(code) +// in parseSimpleMarkdown's codeBodyHtml assembly -- and then ran a SECOND, manual decode pass +// (.replace(/"/g,'"').replace(/</g,'<').replace(/>/g,'>').replace(/&/g,'&')) on +// top of the already-decoded value. That second pass is a no-op for ordinary special characters +// (a lone decoded '&'/'<'/'>'/'"'/''' doesn't spell out an entity reference), but corrupts any +// code block whose content literally contains one of the four entity texts (" / & / +// < / >) -- e.g. a snippet that itself demonstrates HTML-entity syntax. Fix: drop the +// second decode; getAttribute()'s result is already the exact original code. +// +// This suite calls the REAL, extracted parseSimpleMarkdown (not a hand-copied reconstruction of +// its codeBodyHtml assembly) to produce the data-raw-code attribute, reads it back through +// parse5 (which entity-decodes attribute values exactly like a real browser's getAttribute() +// would), and then runs the REAL, extracted copyCodeBlock against that value, asserting on what +// it hands to navigator.clipboard.writeText(...). What these tests establish is that the +// clipboard text is always exactly what getAttribute('data-raw-code') returns -- copyCodeBlock +// no longer transforms it at all. getAttribute()'s own value is not always byte-identical to the +// markdown input: the HTML parser normalises CR/CRLF to LF and NUL to U+FFFD while parsing the +// attribute (pre-existing browser/parse5 behaviour, unrelated to this fix, not exercised by the +// inputs below since none contain CR or NUL), and the fence regex in parseSimpleMarkdown +// captures the newline immediately before the closing ``` fence (see collapse-rules.ts's own +// comment on this) -- so for the plain inputs used here, getAttribute() returns `input + '\n'`. +// ───────────────────────────────────────────────────────────────────────── + +interface CodeBlockSandbox { + parseSimpleMarkdown(markdown: string): string; +} + +// The sandbox is kept to exactly the functions the data-raw-code path actually needs: +// escapeHtml, escapeAttr, normalizeCollapseThreshold, evaluateCodeBlockCollapse, +// extractCodeBlocks, parseSimpleMarkdown. extractCodeBlocks (fork-issue-63): parseSimpleMarkdown's +// fenced-code-block extraction moved into its own shared function (also used by +// renderUserMessageContent, see user-message-rawtext.test.ts) -- parseSimpleMarkdown now +// calls it instead of building the placeholder/collapse/copy-button HTML inline. +function loadCodeBlockSandbox(): CodeBlockSandbox { + const body = getEmittedScriptBody(); + // restoreCodeBlockPlaceholders (fork-issue-55): script.ts splices this in via + // `${restoreCodeBlockPlaceholders.toString()}` (build-time, see markdown-restore.ts), so it + // appears in the emitted body as an ordinary function declaration extractFunction can find, + // same as the others below -- parseSimpleMarkdown calls it to restore __CODEBLOCK_N__. + const src = ['escapeHtml', 'escapeAttr', 'normalizeCollapseThreshold', 'evaluateCodeBlockCollapse', 'restoreCodeBlockPlaceholders', 'extractCodeBlocks', 'parseSimpleMarkdown'] + .map(name => extractFunction(body, name)) + .join('\n'); + const sandbox: Record = { + collapseLongCodeBlocks: true, + collapseCodeBlockLines: 20, + document: { + createElement(tag: string) { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeDiv(); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return sandbox as unknown as CodeBlockSandbox; +} + +// Renders a single fenced ```text code block and returns the data-raw-code value parse5 reads +// off the element -- the getAttribute('data-raw-code') +// equivalent (parse5 entity-decodes attribute values during parsing, exactly like a real +// browser). findAttrOn (element-scoped), not findAttr: the language-fixed "language-text" class +// pins this to the actual element that carries data-raw-code, not any other element. +function renderCodeBlockRawAttr(code: string): string { + const sandbox = loadCodeBlockSandbox(); + const markdown = '```text\n' + code + '\n```'; + const html = sandbox.parseSimpleMarkdown(markdown); + const attr = findAttrOn(html, 'language-text', 'data-raw-code'); + if (!attr) { throw new Error('expected a data-raw-code attribute on the language-text code element; got: ' + html); } + return attr.value; +} + +// Extracts and runs the REAL copyCodeBlock() against a getAttribute('data-raw-code')-equivalent +// value, capturing what it hands to navigator.clipboard.writeText(...). +function runCopyCodeBlock(dataRawCode: string): string { + const body = getEmittedScriptBody(); + // fork-issue-64 (fixed): extractFunction is now parser-based and can no longer drag in trailing + // functions by misreading a regex literal as an unbalanced quote (copyCodeBlock's own decode + // chain has four /pattern/g regex literals). Kept as an explicit start/end/no-trailing- + // declaration check anyway, as a belt-and-suspenders regression guard for this one call site. + const src = extractFunction(body, 'copyCodeBlock'); + assert.ok(src.startsWith('function copyCodeBlock(codeId) {'), 'extractFunction(copyCodeBlock) did not start where expected; got: ' + src.slice(0, 80)); + assert.ok(src.trimEnd().endsWith('}'), 'extractFunction(copyCodeBlock) did not end at a closing brace; got: ' + src.slice(-80)); + assert.ok(!/\n\s*function\s+\w+\s*\(/.test(src.slice('function copyCodeBlock(codeId) {'.length)), 'extractFunction(copyCodeBlock) appears to have dragged in a trailing function declaration (fork-issue-64); got: ' + src); + let clipboardText: string | undefined; + const sandbox: Record = { + document: { + getElementById(_id: string) { + return { + getAttribute(name: string) { return name === 'data-raw-code' ? dataRawCode : null; }, + closest() { return { querySelector() { return null; } }; } + }; + } + }, + navigator: { + clipboard: { + writeText(text: string) { + clipboardText = text; + return { then(cb: () => void) { cb(); return { catch() { /* noop */ } }; } }; + } + } + }, + console + }; + vm.createContext(sandbox); + new vm.Script(src + '\ncopyCodeBlock("x");').runInContext(sandbox); + if (clipboardText === undefined) { throw new Error('copyCodeBlock never called navigator.clipboard.writeText'); } + return clipboardText; +} + +suite('webview attribute escaping: copyCodeBlock double-decode (fork-issue-62 Part A PoC)', () => { + + test('a code block literally containing """ -- the clipboard text matches getAttribute() exactly, no longer double-decoded (the corruption case)', () => { + const code = 'literal " entity'; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n', 'getAttribute() equivalent must already be the exact original code'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('a code block literally containing "&" -- the clipboard text matches getAttribute() exactly (the case)', () => { + const code = ''; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('a code block literally containing "<" and ">" -- the clipboard text matches getAttribute() exactly', () => { + const code = 'a <div> tag as text'; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('ordinary special characters (&, <, >, ", \') that do not spell out an entity -- the clipboard text matches getAttribute() exactly (no functional regression)', () => { + const code = 'const s = "a" + \'b\' & && d;'; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('a multi-line code block mixing real special characters and literal entity text -- the clipboard text matches getAttribute() exactly', () => { + const code = [ + 'function f(a, b) {', + ' // literal example: "quoted" and &amp;', + ' return a < b && b > a ? "yes" : \'no\';', + '}' + ].join('\n'); + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// fork-issue-64: extractFunction() (webview-dom-helpers.ts) used to find a function's source text via +// hand-rolled, quote-aware brace matching, which knew about strings and comments but not about +// regex literals. escapeAttr's own `.replace(/'/g, ''')` made the old scanner see `/` then +// `'` and misread the apostrophe as a string start, desyncing the brace count for everything +// after it -- pulling 2365 characters of unrelated trailing functions (safeHttpUrl, +// openFileInEditor, formatFilePath, toggleDiffExpansion, toggleResultExpansion) into the +// "escapeAttr" extraction, and duplicating formatFilePath into the vm sandbox. This was +// previously harmless only because escapeAttr sits at the front of that chunk and a later, +// correctly-extracted formatFilePath always overwrote the contaminated one -- reordering or +// editing toggleResultExpansion would have broken every test in this file. extractFunction is +// now parser-based (TypeScript's own parser) and structurally cannot have this failure mode; +// these tests pin that down for the exact case that triggered it. +// ───────────────────────────────────────────────────────────────────────── + +suite('extractFunction: regex literals containing a quote no longer desync extraction (fork-issue-64)', () => { + + test('extracting escapeAttr returns exactly its own body -- no trailing functions dragged in', () => { + const body = getEmittedScriptBody(); + const src = extractFunction(body, 'escapeAttr'); + assert.ok(src.startsWith('function escapeAttr('), 'expected the escapeAttr extraction to start with its own signature; got: ' + src.slice(0, 80)); + assert.ok(src.trimEnd().endsWith('}'), 'expected the escapeAttr extraction to end at a closing brace; got: ' + src.slice(-80)); + // The fork-issue-64 bug specifically dragged in these five trailing declarations (in this order) -- + // see the file header comment above. + for (const trailingName of ['safeHttpUrl', 'openFileInEditor', 'formatFilePath', 'toggleDiffExpansion', 'toggleResultExpansion']) { + assert.ok( + !src.includes('function ' + trailingName + '('), + 'escapeAttr extraction must not contain a trailing function ' + trailingName + '(...) declaration (fork-issue-64 regression); got: ' + src + ); + } + // General form of the same check: no OTHER top-level "function NAME(" declaration may + // appear anywhere inside the extracted text at all. + assert.ok( + !/\n\s*function\s+\w+\s*\(/.test(src.slice('function escapeAttr('.length)), + 'escapeAttr extraction appears to have dragged in a trailing function declaration (fork-issue-64 regression); got length ' + src.length + ); + // escapeAttr's own regex literals must still be present verbatim -- proves the fix didn't + // achieve a short extraction by truncating early instead of stopping at the right brace. + assert.ok(src.includes("replace(/'/g, ''')"), 'escapeAttr extraction is missing its own final .replace(/\'/g, \''\') call; got: ' + src); + }); + + test('extracting formatFilePath (declared after escapeAttr in the emitted script) still returns exactly its own body, not escapeAttr\'s', () => { + const body = getEmittedScriptBody(); + const src = extractFunction(body, 'formatFilePath'); + assert.ok(src.startsWith('function formatFilePath('), 'expected the formatFilePath extraction to start with its own signature; got: ' + src.slice(0, 80)); + assert.ok(!src.includes('function escapeAttr('), 'formatFilePath extraction must not contain escapeAttr\'s declaration; got: ' + src); + assert.ok(!src.includes('function toggleDiffExpansion('), 'formatFilePath extraction must not contain toggleDiffExpansion\'s declaration; got: ' + src); + }); + + test('a synthetic function with an apostrophe inside a regex literal in its body extracts correctly and does not swallow the next declaration', () => { + const source = [ + 'function withQuoteInRegex(s) {', + "\treturn s.replace(/'/g, 'X');", + '}', + '', + 'function nextFn() {', + '\treturn 1;', + '}' + ].join('\n'); + const src = extractFunction(source, 'withQuoteInRegex'); + assert.strictEqual(src, [ + 'function withQuoteInRegex(s) {', + "\treturn s.replace(/'/g, 'X');", + '}' + ].join('\n')); + }); +}); diff --git a/src/test/webview-dom-helpers.ts b/src/test/webview-dom-helpers.ts new file mode 100644 index 0000000..c54233a --- /dev/null +++ b/src/test/webview-dom-helpers.ts @@ -0,0 +1,134 @@ +// Shared extraction/DOM-inspection helpers for the webview vm-sandbox test suites +// (webview-attr-escape.test.ts, user-message-rawtext.test.ts). Both files extracted +// hand-copied duplicates of these five functions; pulled out here per fork-issue-64. +// +// fork-issue-64: extractFunction() used to find a function's source text via hand-rolled, quote-aware +// brace matching. That scanner knew about strings and // and /* */ comments, but not about +// regex literals -- escapeAttr's own `.replace(/'/g, ''')` made the scanner see `/` then +// `'` and misread the apostrophe as a string start, desyncing the brace count for everything +// after it and dragging 2365 chars of unrelated trailing functions (safeHttpUrl, +// openFileInEditor, formatFilePath, toggleDiffExpansion, toggleResultExpansion) into the +// "escapeAttr" extraction -- 362 chars of escapeAttr came back as 2727. +// A real parser doesn't have this failure mode, so this now asks TypeScript's own parser +// (already a devDependency) for the function declaration's exact span instead of re-implementing +// a JS tokenizer by hand. +import * as ts from 'typescript'; +import * as parse5 from 'parse5'; + +// Finds a "function NAME(...) { ... }" declaration anywhere in `source` (at any nesting depth -- +// e.g. renderDropdown is declared inside initModelCombo) and returns its exact source text, in +// document order (first match wins, same as the old regex-based scanner). Throws the same kind +// of message as before when nothing matches. +export function extractFunction(source: string, name: string): string { + const sourceFile = ts.createSourceFile('emitted-script.js', source, ts.ScriptTarget.Latest, true, ts.ScriptKind.JS); + let found: ts.FunctionDeclaration | undefined; + const visit = (node: ts.Node): void => { + if (found) { return; } + if (ts.isFunctionDeclaration(node) && node.name && node.name.text === name) { + found = node; + return; + } + ts.forEachChild(node, visit); + }; + visit(sourceFile); + if (!found) { + throw new Error('function ' + name + ' not found in emitted script'); + } + return source.slice(found.getStart(sourceFile), found.getEnd()); +} + +// First DFS match wins (document order) -- e.g. formatToolInputUI's output nests a +// span.file-path-truncated (from formatFilePath) inside a div.diff-file-path, and both currently +// carry a data-file-path attribute with the same value, so a "last match wins" walk would +// silently return the inner span's copy instead of the outer div's -- the one the div's own +// onclick="openFileInEditor(this.dataset.filePath)" actually reads. That would let a regression +// that drops data-file-path from the div alone (while leaving the span's copy intact) pass +// unnoticed. Use findAttrOn() below when a specific element matters. +export function findAttr(html: string, attrName: string): { name: string; value: string } | undefined { + const frag = parse5.parseFragment(html); + let found: { name: string; value: string } | undefined; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (found) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const a = el.attrs.find(x => x.name === attrName); + if (a) { found = a; return; } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + return found; +} + +// Scoped lookup: finds the first element carrying cssClass (document order) and returns +// attrName's value from THAT element specifically (undefined if the element lacks it) -- unlike +// findAttr(), this doesn't get confused by a same-named attribute on a nested element. +export function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined { + const frag = parse5.parseFragment(html); + let result: { name: string; value: string } | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + result = el.attrs.find(x => x.name === attrName); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result; +} + +// Concatenates the direct #text children of the first element carrying cssClass (document +// order); throws if no such element exists. If escaping were missing, a payload like +// '' would parse as a real element instead of literal text, so +// those characters would be MISSING from this concatenation -- the full raw payload +// round-tripping back as text is what proves the escaping worked. +export function textOn(html: string, cssClass: string): string { + const frag = parse5.parseFragment(html); + let result: string | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + result = (parent.childNodes || []) + .filter(n => n.nodeName === '#text') + .map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value) + .join(''); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result || ''; +} + +export function tagExists(html: string, tagName: string): boolean { + const frag = parse5.parseFragment(html); + let found = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (found) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.tagName === tagName) { found = true; return; } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + return found; +} diff --git a/src/ui-styles.ts b/src/ui-styles.ts index 76bd766..1be14fc 100644 --- a/src/ui-styles.ts +++ b/src/ui-styles.ts @@ -1141,6 +1141,28 @@ const styles = ` background-color: var(--vscode-list-hoverBackground); } + /* Manual per-message collapse (fork-issue-48, upstream #151) */ + .message-collapse-btn { + background: transparent; + border: none; + color: var(--vscode-descriptionForeground); + cursor: pointer; + padding: 2px 4px; + border-radius: 3px; + font-size: 10px; + line-height: 1; + opacity: 0.35; + transition: opacity 0.2s ease; + } + .message:hover .message-collapse-btn { opacity: 0.8; } + .message-collapse-btn:hover { opacity: 1; background-color: var(--vscode-list-hoverBackground); } + /* While collapsed, the handle stays permanently visible, otherwise nobody would find it again. */ + .message.collapsed .message-collapse-btn { opacity: 0.9; } + /* Hide everything except the header -- this also covers .message-content AND extra + blocks like .yolo-suggestion. */ + .message.collapsed > *:not(.message-header) { display: none; } + .message.collapsed .message-header { margin-bottom: 0; padding-bottom: 0; border-bottom: none; } + .message-icon { width: 18px; height: 18px; @@ -1183,6 +1205,20 @@ const styles = ` padding-left: 6px; } + /* fork-issue-63: user messages render their prose as raw text nodes (no

wrapper), so pre-wrap is + needed to keep newlines/multi-space visible; break-word stops long unbroken tokens (paths, + URLs) from overflowing. line-height: 1.6 matches .message p's (ui-styles.ts, ~line 3855), + which no longer applies here since the prose text isn't wrapped in

anymore -- without + this, user messages would fall back to .messages' line-height: 1.4 and look tighter than + before fork-issue-63 (a review finding). Code blocks inside a user message keep their own + .message-content pre.code-block { white-space: pre } untouched -- that rule targets the + 

 element directly, which always wins over inherited pre-wrap from this ancestor. */
+    .message.user .message-content {
+        white-space: pre-wrap;
+        word-wrap: break-word;
+        line-height: 1.6;
+    }
+
     /* Code blocks generated by markdown parser only */
     .message-content pre.code-block {
         background-color: var(--vscode-textCodeBlock-background);
@@ -1262,6 +1298,31 @@ const styles = `
         background: none;
     }
 
+    /* Collapsible long code blocks (fork-issue-48, upstream #151). The  keeps the
+       .code-block-header class so all existing and compact-mode rules still
+       apply unchanged; only marker/cursor/flow styling is added. */
+    details.code-block-container > summary.code-block-header {
+        cursor: pointer;
+        list-style: none;
+        user-select: none;
+        justify-content: flex-start;
+        gap: 6px;
+    }
+    details.code-block-container > summary.code-block-header::-webkit-details-marker { display: none; }
+    details.code-block-container > summary.code-block-header .code-copy-btn { margin-left: auto; }
+    .code-collapse-caret {
+        display: inline-block;
+        color: var(--vscode-descriptionForeground);
+        font-size: 9px;
+        transition: transform 0.15s ease;
+    }
+    details.code-block-container[open] > summary.code-block-header .code-collapse-caret { transform: rotate(90deg); }
+    .code-collapse-hint {
+        color: var(--vscode-descriptionForeground);
+        font-size: 10px;
+        opacity: 0.85;
+    }
+
     /* Inline code */
     .message-content code {
         background-color: var(--vscode-textCodeBlock-background);
diff --git a/src/ui.ts b/src/ui.ts
index 4ffcab7..d61b77e 100644
--- a/src/ui.ts
+++ b/src/ui.ts
@@ -414,6 +414,21 @@ const getHtml = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https
+

Appearance

+
+
+ + +
+
+ + +

+ Number of lines a code block must exceed before it gets a collapsible header (5-500). +

+
+
+