Improves Git security with untrusted workspaces

extensions/git/src/git.ts

@@ -63,9 +63,11 @@ function parseVersion(raw: string): string {
 	return raw.replace(/^git version /, '');
 }
 
-function findSpecificGit(path: string, onLookup: (path: string) => void): Promise<IGit> {
+function findSpecificGit(path: string, onValidate: (path: string) => boolean): Promise<IGit> {
 	return new Promise<IGit>((c, e) => {
-		onLookup(path);
+		if (!onValidate(path)) {
+			return e('git not found');
+		}
 
 		const buffers: Buffer[] = [];
 		const child = cp.spawn(path, ['--version']);
@@ -75,7 +77,7 @@ function findSpecificGit(path: string, onLookup: (path: string) => void): Promis
 	});
 }
 
-function findGitDarwin(onLookup: (path: string) => void): Promise<IGit> {
+function findGitDarwin(onValidate: (path: string) => boolean): Promise<IGit> {
 	return new Promise<IGit>((c, e) => {
 		cp.exec('which git', (err, gitPathBuffer) => {
 			if (err) {
@@ -85,7 +87,9 @@ function findGitDarwin(onLookup: (path: string) => void): Promise<IGit> {
 			const path = gitPathBuffer.toString().replace(/^\s+|\s+$/g, '');
 
 			function getVersion(path: string) {
-				onLookup(path);
+				if (!onValidate(path)) {
+					return e('git not found');
+				}
 
 				// make sure git executes
 				cp.exec('git --version', (err, stdout) => {
@@ -117,33 +121,31 @@ function findGitDarwin(onLookup: (path: string) => void): Promise<IGit> {
 	});
 }
 
-function findSystemGitWin32(base: string, onLookup: (path: string) => void): Promise<IGit> {
+function findSystemGitWin32(base: string, onValidate: (path: string) => boolean): Promise<IGit> {
 	if (!base) {
 		return Promise.reject<IGit>('Not found');
 	}
 
-	return findSpecificGit(path.join(base, 'Git', 'cmd', 'git.exe'), onLookup);
+	return findSpecificGit(path.join(base, 'Git', 'cmd', 'git.exe'), onValidate);
 }
 
-function findGitWin32InPath(onLookup: (path: string) => void): Promise<IGit> {
+function findGitWin32InPath(onValidate: (path: string) => boolean): Promise<IGit> {
 	const whichPromise = new Promise<string>((c, e) => which('git.exe', (err, path) => err ? e(err) : c(path)));
-	return whichPromise.then(path => findSpecificGit(path, onLookup));
+	return whichPromise.then(path => findSpecificGit(path, onValidate));
 }
 
-function findGitWin32(onLookup: (path: string) => void): Promise<IGit> {
-	return findSystemGitWin32(process.env['ProgramW6432'] as string, onLookup)
-		.then(undefined, () => findSystemGitWin32(process.env['ProgramFiles(x86)'] as string, onLookup))
-		.then(undefined, () => findSystemGitWin32(process.env['ProgramFiles'] as string, onLookup))
-		.then(undefined, () => findSystemGitWin32(path.join(process.env['LocalAppData'] as string, 'Programs'), onLookup))
-		.then(undefined, () => findGitWin32InPath(onLookup));
+function findGitWin32(onValidate: (path: string) => boolean): Promise<IGit> {
+	return findSystemGitWin32(process.env['ProgramW6432'] as string, onValidate)
+		.then(undefined, () => findSystemGitWin32(process.env['ProgramFiles(x86)'] as string, onValidate))
+		.then(undefined, () => findSystemGitWin32(process.env['ProgramFiles'] as string, onValidate))
+		.then(undefined, () => findSystemGitWin32(path.join(process.env['LocalAppData'] as string, 'Programs'), onValidate))
+		.then(undefined, () => findGitWin32InPath(onValidate));
 }
 
-export async function findGit(hint: string | string[] | undefined, onLookup: (path: string) => void): Promise<IGit> {
-	const hints = Array.isArray(hint) ? hint : hint ? [hint] : [];
-
+export async function findGit(hints: string[], onValidate: (path: string) => boolean): Promise<IGit> {
 	for (const hint of hints) {
 		try {
-			return await findSpecificGit(hint, onLookup);
+			return await findSpecificGit(hint, onValidate);
 		} catch {
 			// noop
 		}
@@ -151,9 +153,9 @@ export async function findGit(hint: string | string[] | undefined, onLookup: (pa
 
 	try {
 		switch (process.platform) {
-			case 'darwin': return await findGitDarwin(onLookup);
-			case 'win32': return await findGitWin32(onLookup);
-			default: return await findSpecificGit('git', onLookup);
+			case 'darwin': return await findGitDarwin(onValidate);
+			case 'win32': return await findGitWin32(onValidate);
+			default: return await findSpecificGit('git', onValidate);
 		}
 	} catch {
 		// noop

extensions/git/src/main.ts

@@ -34,8 +34,30 @@ export async function deactivate(): Promise<any> {
 }
 
 async function createModel(context: ExtensionContext, outputChannel: OutputChannel, telemetryReporter: TelemetryReporter, disposables: Disposable[]): Promise<Model> {
-	const pathHint = workspace.getConfiguration('git').get<string | string[]>('path');
-	const info = await findGit(pathHint, path => outputChannel.appendLine(localize('looking', "Looking for git in: {0}", path)));
+	const pathValue = workspace.getConfiguration('git').get<string | string[]>('path');
+	let pathHints = Array.isArray(pathValue) ? pathValue : pathValue ? [pathValue] : [];
+
+	const { isTrusted, workspaceFolders = [] } = workspace;
+	const excludes = isTrusted ? [] : workspaceFolders.map(f => path.normalize(f.uri.fsPath).replace(/[\r\n]+$/, ''));
+
+	if (!isTrusted && pathHints.length !== 0) {
+		// Filter out any non-absolute paths
+		pathHints = pathHints.filter(p => path.isAbsolute(p));
+	}
+
+	const info = await findGit(pathHints, gitPath => {
+		outputChannel.appendLine(localize('validating', "Validating found git in: {0}", gitPath));
+		if (excludes.length === 0) {
+			return true;
+		}
+
+		const normalized = path.normalize(gitPath).replace(/[\r\n]+$/, '');
+		const skip = excludes.some(e => normalized.startsWith(e));
+		if (skip) {
+			outputChannel.appendLine(localize('skipped', "Skipped found git in: {0}", gitPath));
+		}
+		return !skip;
+	});
 
 	const askpass = await Askpass.create(outputChannel, context.storagePath);
 	disposables.push(askpass);

extensions/git/src/model.ts

@@ -147,23 +147,23 @@ export class Model implements IRemoteSourceProviderRegistry, IPushErrorHandlerRe
 		await Promise.all((workspace.workspaceFolders || []).map(async folder => {
 			const root = folder.uri.fsPath;
 			const children = await new Promise<string[]>((c, e) => fs.readdir(root, (err, r) => err ? e(err) : c(r)));
-			const promises = children
-				.filter(child => child !== '.git')
-				.map(child => this.openRepository(path.join(root, child)));
+			const subfolders = new Set(children.filter(child => child !== '.git').map(child => path.join(root, child)));
 
-			const folderConfig = workspace.getConfiguration('git', folder.uri);
-			const paths = folderConfig.get<string[]>('scanRepositories') || [];
+			const scanPaths = (workspace.isTrusted ? workspace.getConfiguration('git', folder.uri) : config).get<string[]>('scanRepositories') || [];
+			for (const scanPath of scanPaths) {
+				if (scanPath !== '.git') {
+					continue;
+				}
 
-			for (const possibleRepositoryPath of paths) {
-				if (path.isAbsolute(possibleRepositoryPath)) {
+				if (path.isAbsolute(scanPath)) {
 					console.warn(localize('not supported', "Absolute paths not supported in 'git.scanRepositories' setting."));
 					continue;
 				}
 
-				promises.push(this.openRepository(path.join(root, possibleRepositoryPath)));
+				subfolders.add(path.join(root, scanPath));
 			}
 
-			await Promise.all(promises);
+			await Promise.all([...subfolders].map(f => this.openRepository(f)));
 		}));
 	}
 
@@ -226,6 +226,10 @@ export class Model implements IRemoteSourceProviderRegistry, IPushErrorHandlerRe
 	}
 
 	private async onDidChangeVisibleTextEditors(editors: readonly TextEditor[]): Promise<void> {
+		if (!workspace.isTrusted) {
+			return;
+		}
+
 		const config = workspace.getConfiguration('git');
 		const autoRepositoryDetection = config.get<boolean | 'subFolders' | 'openEditors'>('autoRepositoryDetection');
 
@@ -251,20 +255,33 @@ export class Model implements IRemoteSourceProviderRegistry, IPushErrorHandlerRe
 	}
 
 	@sequentialize
-	async openRepository(path: string): Promise<void> {
-		if (this.getRepository(path)) {
+	async openRepository(repoPath: string): Promise<void> {
+		if (this.getRepository(repoPath)) {
 			return;
 		}
 
-		const config = workspace.getConfiguration('git', Uri.file(path));
+		const config = workspace.getConfiguration('git', Uri.file(repoPath));
 		const enabled = config.get<boolean>('enabled') === true;
 
 		if (!enabled) {
 			return;
 		}
 
+		if (!workspace.isTrusted) {
+			// Check if the folder is a bare repo: if it has a file named HEAD && `rev-parse --show -cdup` is empty
+			try {
+				fs.accessSync(path.join(repoPath, 'HEAD'), fs.constants.F_OK);
+				const result = await this.git.exec(repoPath, ['-C', repoPath, 'rev-parse', '--show-cdup'], { log: false });
+				if (result.stderr.trim() === '' && result.stdout.trim() === '') {
+					return;
+				}
+			} catch {
+				// If this throw, we should be good to open the repo (e.g. HEAD doesn't exist)
+			}
+		}
+
 		try {
-			const rawRoot = await this.git.getRepositoryRoot(path);
+			const rawRoot = await this.git.getRepositoryRoot(repoPath);
 
 			// This can happen whenever `path` has the wrong case sensitivity in
 			// case insensitive file systems
@@ -286,7 +303,7 @@ export class Model implements IRemoteSourceProviderRegistry, IPushErrorHandlerRe
 			await repository.status();
 		} catch (ex) {
 			// noop
-			this.outputChannel.appendLine(`Opening repository for path='${path}' failed; ex=${ex}`);
+			this.outputChannel.appendLine(`Opening repository for path='${repoPath}' failed; ex=${ex}`);
 		}
 	}