Webview XSS hardening: HTML/attribute escaping for untrusted sinks, URL scheme guard, safe code-block restore, regression test suites

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"
   }
 }

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 };
+}

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 <summary>'s synchronous onclick, never the <details>'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;

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<string>('executable.path', ''),
 			'environment.variables': config.get<Record<string, string>>('environment.variables', {}),
 			'environment.disabled': config.get<boolean>('environment.disabled', false),
+			'ui.collapseLongCodeBlocks': config.get<boolean>('ui.collapseLongCodeBlocks', true),
+			'ui.collapseCodeBlockLines': config.get<number>('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<void> {
 		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<void> {
 		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
+			});
 		}
 	}
 

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, '&lt;')
+		.replace(/>/g, '&gt;')
+		.replace(/"/g, '&quot;')
+		.replace(/'/g, '&#39;');
+}
+
+// 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;
+}

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;
+}