Skip to content

Support /mnt/ and \\wsl.localhost\ and \\wsl$\ style links on Windows frontends#169592

Merged
Tyriar merged 3 commits intomainfrom
tyriar/wsl_links
Dec 21, 2022
Merged

Support /mnt/ and \\wsl.localhost\ and \\wsl$\ style links on Windows frontends#169592
Tyriar merged 3 commits intomainfrom
tyriar/wsl_links

Conversation

@Tyriar
Copy link
Member

@Tyriar Tyriar commented Dec 20, 2022

Fixes #148062

image
image

@Tyriar Tyriar added this to the January 2023 milestone Dec 20, 2022
@Tyriar Tyriar self-assigned this Dec 20, 2022
@Tyriar Tyriar requested a review from aeschli December 20, 2022 00:54
@Tyriar
Copy link
Member Author

Tyriar commented Dec 20, 2022

@aeschli a review would be appreciated since you're fairly familiar with WSL

@aeschli
Copy link
Contributor

aeschli commented Dec 20, 2022

Here's the code I use in the WSL extension.

  • It has some more backward compatibility things checking for older WSL verisons, but after all these years I think we can skip these
  • It avoids backsplash escaping issues by using env variables to make the conversion
export function executeWslPath(pathToConvert: string, toLinux: boolean, distro: string): string {
	if (toLinux) {
		let commandLine = 'echo "$VSCODE_WSL_PATH"';
		const options = {
			'WSLENV': 'VSCODE_WSL_PATH/up',
			'VSCODE_WSL_PATH': pathToConvert
		};
		return executeCommand(commandLine, options, distro).trim();
	} else {
		let commandLine = `wslpath -w "$VSCODE_WSL_PATH"`;
		const options = {
			'WSLENV': 'VSCODE_WSL_PATH/u',
			'VSCODE_WSL_PATH': pathToConvert
		};
		return executeCommand(commandLine, options, distro).trim();
	}
}

export function executeCommand(commandLine: string, env: Record<string, string>, distro: string): string {
	const wslExecPath = getWSLExecutablePath();
	if (!wslExecPath) {
		throw new Error(localize('executeCommand.noWSL', 'wsl command not found\n\nMake sure WSL is installed:\nhttps://aka.ms/vscode-remote/wsl/install-wsl'));
	}
	const options = getExecSyncOptions(env);
	let args = [];
	if (isExecAndDistroArgumentSupported()) {
		args.push('-d', distro, '-e', 'sh', '-c', commandLine);
	} else if (path.basename(wslExecPath).toLowerCase() === 'wsl.exe') {
		args.push('sh', '-c', commandLine);
	} else {
		args.push('-c', commandLine);
	}
	return cp.execFileSync(wslExecPath, args, options as cp.ExecFileSyncOptions).toString();
}

export function getWSLExecutablePath(): string | undefined {
	let useWSLexe = getWindowsBuildNumber() >= 16299;
	const is32ProcessOn64Windows = process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432');
	const systemRoot = process.env['SystemRoot'];
	if (systemRoot) {
		return path.join(systemRoot, is32ProcessOn64Windows ? 'Sysnative' : 'System32', useWSLexe ? 'wsl.exe' : 'bash.exe');
	}
	return undefined;
}

export function isExecAndDistroArgumentSupported(): boolean {
	// https://docs.microsoft.com/en-us/windows/wsl/release-notes#build-17666
	return getWindowsBuildNumber() >= 17666; // part of September 2018 Update (1809)
}

export function getExecSyncOptions(env: Record<string, string> = {}): cp.ExecSyncOptions & { env: NodeJS.ProcessEnv; stdio: cp.StdioOptions } {
	return { env, stdio: [null, null, null] };
}

return null;
// If the link looks like a /mnt/ WSL path and this is a Windows frontend, use the backend
// to get the resolved path from the wslpath util.
if (isWindows && link.match(/^\/mnt\/[a-z]/i) && this._processManager.backend) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially all links that start or contain / can be paths from the WSL subsystem.
/home/me/test.txt -> \\wsl$\Ubuntu\home\me\test.txt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sort of thing would have to be a separate feature request. It should just work out of the box in a remote wsl workspace or if you have the local wsl folder open, but to reliably know what distro to do we would want to have more knowledge about what's actually running in the terminal. That's possible but out of the scope of this change imo.

Copy link
Contributor

@aeschli aeschli Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, understand that you don't know the distro, especially in a local window, when the user uses wsl.exe himself. And just looking for /mnt/ and calling wsl.exe of the default distro will probably work, but just to say, distros can change the windows root from /mnt/ to something else
In a remote WSL window all that should not really be necessary. The link can be detected as a linux path /mnt/... and opened as such (ill open and editor on '/mnt/...`)

@aeschli
Copy link
Contributor

aeschli commented Dec 20, 2022

Your changes look ok, I just put the request changes so you can review my comments.
Looks like escapeNonWindowsPath bans some characters that are not forbidden in paths. In lInux the only character that is forbidden is /. On Windows it's more but not $. That could be a problem with \\wsl$\...
I'd suggest to use the same code I use in the extension.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 20, 2022

escapeNonWindowsPath is an aggressive function for preventing script injection across many shells, sure it doesn't work with some paths but it's what the terminal uses to sanitize paths for security.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 20, 2022

Although we may not need to worry about injection when using execFile vs exec 🤔, I hesitant as I don't want to introduce a security issue is all

@aeschli
Copy link
Contributor

aeschli commented Dec 20, 2022

execFile is safer. But it's important to run it with a full absolute path. Just invoking bash.exe can be a problem.
(Note, the WSL code does not use exec)

@Tyriar Tyriar requested a review from aeschli December 20, 2022 21:42
@Tyriar Tyriar merged commit e7f57b2 into main Dec 21, 2022
@Tyriar Tyriar deleted the tyriar/wsl_links branch December 21, 2022 16:24
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terminal links should support WSL folders

2 participants