Merge pull request #1927 from microsoft/connor4312/msrc-33-cp

Cherry-pick MSRC 102702+103197

Author: connor4312

src/extension/tools/node/applyPatchTool.tsx

@@ -42,7 +42,7 @@ import { ToolName } from '../common/toolNames';
 import { ICopilotTool, ToolRegistry } from '../common/toolsRegistry';
 import { IToolsService } from '../common/toolsService';
 import { PATCH_PREFIX, PATCH_SUFFIX } from './applyPatch/parseApplyPatch';
-import { ActionType, Commit, DiffError, FileChange, identify_files_needed, InvalidContextError, InvalidPatchFormatError, processPatch } from './applyPatch/parser';
+import { ActionType, Commit, DiffError, FileChange, identify_files_added, identify_files_needed, InvalidContextError, InvalidPatchFormatError, processPatch } from './applyPatch/parser';
 import { EditFileResult, IEditedFile } from './editFileToolResult';
 import { canExistingFileBeEdited, createEditConfirmation, logEditToolResult, openDocumentAndSnapshot } from './editFileToolUtils';
 import { sendEditNotebookTelemetry } from './editNotebookTool';
@@ -577,7 +577,7 @@ export class ApplyPatchTool implements ICopilotTool<IApplyPatchToolParams> {
 	prepareInvocation(options: vscode.LanguageModelToolInvocationPrepareOptions<IApplyPatchToolParams>, token: vscode.CancellationToken): vscode.ProviderResult<vscode.PreparedToolInvocation> {
 		return this.instantiationService.invokeFunction(
 			createEditConfirmation,
-			identify_files_needed(options.input.input).map(f => URI.file(f)),
+			[...identify_files_needed(options.input.input), ...identify_files_added(options.input.input)].map(f => URI.file(f)),
 			() => '```\n' + options.input.input + '\n```',
 		);
 	}

src/extension/tools/node/editFileToolUtils.tsx

@@ -567,6 +567,68 @@ const platformConfirmationRequiredPaths = (
 			: []
 ).concat(allPlatformPatterns).map(p => glob.parse(p));
 
+/**
+ * Validates that a path doesn't contain suspicious characters that could be used
+ * to bypass security checks on Windows (e.g., NTFS Alternate Data Streams, invalid chars).
+ * Throws an error if the path is suspicious.
+ */
+export function assertPathIsSafe(fsPath: string, _isWindows = isWindows): void {
+	if (fsPath.includes('\0')) {
+		throw new Error(`Path contains null bytes: ${fsPath}`);
+	}
+
+	if (!_isWindows) {
+		return;
+	}
+
+	// Check for NTFS Alternate Data Streams (ADS)
+	const colonIndex = fsPath.indexOf(':', 2);
+	if (colonIndex !== -1) {
+		throw new Error(`Path contains invalid characters (alternate data stream): ${fsPath}`);
+	}
+
+	// Check for invalid Windows filename characters
+	const invalidChars = /[<>"|?*]/;
+	const pathAfterDrive = fsPath.length > 2 ? fsPath.substring(2) : fsPath;
+	if (invalidChars.test(pathAfterDrive)) {
+		throw new Error(`Path contains invalid characters: ${fsPath}`);
+	}
+
+	// Check for named pipes or device paths
+	if (fsPath.startsWith('\\\\.') || fsPath.startsWith('\\\\?')) {
+		throw new Error(`Path is a reserved device path: ${fsPath}`);
+	}
+
+	const reserved = /^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])(\.|$)/i;
+
+	// Check for trailing dots and spaces on path components (Windows quirk)
+	const parts = fsPath.split('\\');
+	for (const part of parts) {
+		if (part.length === 0) {
+			continue;
+		}
+
+		// Reserved device names. Would error on edit, but fail explicitly
+		if (reserved.test(part)) {
+			throw new Error(`Reserved device name in path: ${fsPath}`);
+		}
+
+		// Check for trailing dots or spaces
+		if (part.endsWith('.') || part.endsWith(' ')) {
+			throw new Error(`Path contains invalid trailing characters: ${fsPath}`);
+		}
+
+		// Check for 8.3 short filename pattern
+		const tildeIndex = part.indexOf('~');
+		if (tildeIndex !== -1) {
+			const afterTilde = part.substring(tildeIndex + 1);
+			if (afterTilde.length > 0 && /^\d/.test(afterTilde)) {
+				throw new Error(`Path appears to use short filename format (8.3 names): ${fsPath}. Please use the full path.`);
+			}
+		}
+	}
+}
+
 const enum ConfirmationCheckResult {
 	NoConfirmation,
 	NoPermissions,
@@ -612,6 +674,8 @@ function makeUriConfirmationChecker(configuration: IConfigurationService, worksp
 		let ok = true;
 		let fsPath = uri.fsPath;
 
+		assertPathIsSafe(fsPath);
+
 		if (platformConfirmationRequiredPaths.some(p => p(fsPath))) {
 			return ConfirmationCheckResult.SystemFile;
 		}
@@ -635,6 +699,8 @@ function makeUriConfirmationChecker(configuration: IConfigurationService, worksp
 		if (uri.scheme === Schemas.file) {
 			try {
 				const linked = await realpath(uri.fsPath);
+				assertPathIsSafe(linked);
+
 				if (linked !== uri.fsPath) {
 					toCheck.push(URI.file(linked));
 				}

src/extension/tools/node/test/editFileToolUtils.spec.ts

@@ -14,7 +14,7 @@ import { createTextDocumentData, IExtHostDocumentData, setDocText } from '../../
 import { URI } from '../../../../util/vs/base/common/uri';
 import { WorkspaceEdit } from '../../../../vscodeTypes';
 import { applyEdits as applyTextEdits } from '../../../prompt/node/intents';
-import { applyEdit, ContentFormatError, MultipleMatchesError, NoChangeError, NoMatchError } from '../editFileToolUtils';
+import { applyEdit, assertPathIsSafe, ContentFormatError, MultipleMatchesError, NoChangeError, NoMatchError } from '../editFileToolUtils';
 
 describe('replace_string_in_file - applyEdit', () => {
 	let workspaceEdit: WorkspaceEdit;
@@ -353,3 +353,52 @@ describe('replace_string_in_file - applyEdit', () => {
 		).toBe(output);
 	});
 });
+
+
+describe('assertPathIsSafe (Windows scenarios)', () => {
+	// Force Windows checks by passing true for _isWindows
+	test('accepts normal path', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\project\\file.txt', true)).not.toThrow();
+	});
+
+	test('rejects null byte', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\proje\0ct\\file.txt', true)).toThrow();
+	});
+
+	test('rejects ADS suffix', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\project\\file.txt:$I30:$INDEX_ALLOCATION', true)).toThrow();
+	});
+
+	test('rejects additional colon in component', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\file:name.txt', true)).toThrow();
+	});
+
+	test('rejects invalid characters', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\proj>ect\\file.txt', true)).toThrow();
+	});
+
+	test('rejects device path prefix \\?\\', () => {
+		// This should be treated as reserved device path
+		expect(() => assertPathIsSafe('\\\\?\\C:\\Users\\me\\file.txt', true)).toThrow();
+	});
+
+	test('rejects reserved device name component', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\CON\\file.txt', true)).toThrow();
+	});
+
+	test('rejects trailing dot in component', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\folder.\\file.txt', true)).toThrow();
+	});
+
+	test('rejects trailing space in component', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\folder \\file.txt', true)).toThrow();
+	});
+
+	test('rejects 8.3 short filename pattern', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\VSCODE~1\\settings.json', true)).toThrow();
+	});
+
+	test('allows tilde without digit', () => {
+		expect(() => assertPathIsSafe('C:\\Users\\me\\my~folder\\file.txt', true)).not.toThrow();
+	});
+});