Fix hook commands that pass file URIs to spawn

[darrenoh]

Fixes microsoft/vscode#304555

Summary

• sanitize file:// tokens in hook command strings before invoking spawn
• preserve existing quoting and re-quote decoded paths that contain spaces
• add regression tests for file URIs, quoted file URIs, spaced paths, and non-file URI passthrough

Test plan

• Run NodeHookExecutor unit tests
• Run npm run typecheck

[darrenoh]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

This PR fixes hook command execution when commands include file:// URIs by rewriting those URI tokens into filesystem paths before running the command via spawn, and adds unit tests to prevent regressions in URI handling.

Changes:

• Sanitize file://... tokens inside ChatHookCommand.command before invoking child_process.spawn.
• Decode file URIs to filesystem paths and (re-)quote paths when needed (e.g., spaces).
• Add unit tests covering file URI sanitization, quoting preservation, and non-file URI passthrough.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/platform/chat/node/hookExecutor.ts	Sanitizes hook command strings by detecting file:// URI tokens, converting them to fsPath, and quoting when needed before spawn.
src/platform/chat/test/node/hookExecutor.spec.ts	Adds regression tests asserting the sanitized command string passed to spawn for file URIs and non-file URIs.

[Copilot]

quoteArgForShell wraps args in double quotes but does not escape $ or backticks. In /bin/sh -c ... (used when shell: true), $... and `...` are still expanded/executed inside double quotes, so a decoded file path containing these characters could change command behavior or allow command substitution. Consider switching to a POSIX-safe quoting strategy (e.g., single-quote with proper escaping) or explicitly escaping $ and backticks for the shell you invoke.

Suggested change

	return `"${arg.replace(/["\\]/g, '\\$&')}"`;
	if (!isWindows) {
	// POSIX-safe single-quote escaping: 'foo' -> 'foo', "foo'bar" -> 'foo'"'"'bar'
	let result = "'";
	for (const ch of arg) {
	if (ch === "'") {
	result += `'"'"'`;
	} else {
	result += ch;
	}
	}
	result += "'";
	return result;
	}
	// PowerShell-safe single-quote escaping: 'foo' -> 'foo', "foo'bar" -> 'foo''bar'
	return `'${arg.replace(/'/g, "''")}'`;

[Copilot]

executeCommand logs/errors refer to hookCommand.command, but _spawn now executes a sanitized command string. This can make diagnostics misleading (e.g., failures or timeouts will print the original file:// URI even though the executed command had a filesystem path/quoting). Consider logging/including the sanitized command (or both original + sanitized) anywhere the executed command is surfaced.

[Copilot]

The new URI-sanitization tests cover spaces and quoting, but don't cover decoded shell metacharacters (e.g., %24 -> $ or %60 -> backtick). Since the sanitized string is executed via spawn(..., { shell: ... }), add a regression test that asserts these characters are escaped/quoted so they cannot trigger shell expansion/command substitution.

[Copilot]

If the original URI token is already quoted (quote is non-empty), sanitizeHookCommand injects the decoded fsPath without escaping for that quote style. On POSIX, paths can contain " or ', which would break the surrounding quotes (and could change shell parsing). Consider escaping the decoded path for the specific quote character, or always re-quoting with a known-safe quoting routine.

[darrenoh]

@copilot open a new pull request to apply changes based on the comments in this thread