Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion editors/vscode/client/PathValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*
* The check for malicious characters is not needed, but it's an additional layer of security.
* When using `shell: true` in `LanguageClient.ServerOptions`, it can be vulnerable to command injection.
* Even though we are not using `shell: true`, it's a good practice to validate the input.
* We are using `shell: true` only on Windows when the paths ends with `node_modules/.bin/oxc_language_server`.
*/
export function validateSafeBinaryPath(binary: string): boolean {
// Check for path traversal (including Windows variants)
Expand Down
15 changes: 12 additions & 3 deletions editors/vscode/client/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,7 @@ export async function activate(context: ExtensionContext) {
outputChannel,
);

async function findBinary(): Promise<string> {
let bin = configService.getUserServerBinPath();
async function findBinary(bin: string | undefined): Promise<string> {
if (workspace.isTrusted && bin) {
try {
await fsPromises.access(bin);
Expand All @@ -155,10 +154,20 @@ export async function activate(context: ExtensionContext) {
);
}

const command = await findBinary();
const userDefinedBinary = configService.getUserServerBinPath();
const command = await findBinary(userDefinedBinary);
const run: Executable = {
command: command!,
options: {
// On Windows we need to run the binary in a shell to be able to execute the shell npm bin script.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this because windows can’t execute this script directly? I think there’s another alternative to spawn a cmd prompt with the argument to execute the script, then you don’t need shell: true.

Not sure if it matters or if there’s any reason to prefer one or the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does not handle the sh shebang in the file. Calling the CMD file or ps1 file inside node_modules/.bin/ will not work. This is the only solution or searching for the .exe file.
But searching for the .exe file will create more problems with the location of the .exe.
Your examples in the issue did not check for different package versions. Calling the wrapper script in node_modules/.bin/ "should" be the right way.

// This is only needed when the user explicitly configures the binary to point to the npm bin script.
// The extension is shipped with the `.exe` file, we don't need to run it in a shell.
// Searching for the right `.exe` file inside `node_modules/` is not reliable as it depends on
// the package manager used (npm, yarn, pnpm, etc) and the package version.
// The npm bin script is a shell script that points to the actual binary.
// Security: We validated the userDefinedBinary in `configService.getUserServerBinPath()`.
shell: process.platform === 'win32' && command === userDefinedBinary &&
userDefinedBinary?.endsWith('node_modules\\.bin\\oxc_language_server'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t personally think this (or the other path validations) are necessary.

If the user has opted into the trusted workspace, they have consented to these settings. I don’t think the extension is responsible for validating what the path might look like or what the contents of the evaluated file might be.

It has potential to make it harder for users to use, for what I would consider little to no real security benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

A user would trust the extension more than a freshly opened workspace. Even when a user has trusted a workspace by accident (when the workspace is inside a parent folder, which is already been trusted). Opening the extension to execute untrusted binaries / shell script should still be validated.
Maybe not all restrictions are needed and can frustrate users to set up the oxc.path.server correctly.
But I will take the cost for better end-user security.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what you’re trying to solve is possibly a gap in the trusted workspace feature. Instead of trusting an entire workspace, you want users to trust specific values within the workspace. It doesn’t feel like the responsibility of the extension.

I think the definition of setting the oxc.path.server “correctly” is subjective. If I want to use whatever characters in my path, that shouldn’t be prevented by the extension.

I understand the desire for security though. I don’t think this is a blocker by any means, just wanted to bring it up as something to consider.

env: {
...process.env,
RUST_LOG: process.env.RUST_LOG || 'info',
Expand Down
55 changes: 45 additions & 10 deletions editors/vscode/tests/ConfigService.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,55 @@ suite('ConfigService', () => {
await Promise.all(keys.map(key => conf.update(key, undefined)));
});

testSingleFolderMode('resolves relative server path with workspace folder', async () => {
const service = new ConfigService();
const nonDefinedServerPath = service.getUserServerBinPath();
const getWorkspaceFolderPlatformSafe = () => {
let workspace_path = WORKSPACE_FOLDER.uri.path;
if (process.platform === 'win32') {
workspace_path = workspace_path.replaceAll('/', '\\');
if (workspace_path.startsWith('\\')) {
workspace_path = workspace_path.slice(1);
}
}
return workspace_path;
};

strictEqual(nonDefinedServerPath, undefined);
suite('getUserServerBinPath', () => {
testSingleFolderMode('resolves relative server path with workspace folder', async () => {
const service = new ConfigService();
const nonDefinedServerPath = service.getUserServerBinPath();

await conf.update('path.server', '/absolute/oxc_language_server');
const absoluteServerPath = service.getUserServerBinPath();
strictEqual(nonDefinedServerPath, undefined);

strictEqual(absoluteServerPath, '/absolute/oxc_language_server');
await conf.update('path.server', '/absolute/oxc_language_server');
const absoluteServerPath = service.getUserServerBinPath();

await conf.update('path.server', './relative/oxc_language_server');
const relativeServerPath = service.getUserServerBinPath();
strictEqual(absoluteServerPath, '/absolute/oxc_language_server');

strictEqual(relativeServerPath, WORKSPACE_FOLDER.uri.path + '/relative/oxc_language_server');
await conf.update('path.server', './relative/oxc_language_server');
const relativeServerPath = service.getUserServerBinPath();

let workspace_path = getWorkspaceFolderPlatformSafe();
strictEqual(relativeServerPath, `${workspace_path}/relative/oxc_language_server`);
});

testSingleFolderMode('returns undefined for unsafe server path', async () => {
const service = new ConfigService();
await conf.update('path.server', '../unsafe/oxc_language_server');
const unsafeServerPath = service.getUserServerBinPath();

strictEqual(unsafeServerPath, undefined);
});

testSingleFolderMode('returns backslashes path on Windows', async () => {
if (process.platform !== 'win32') {
return;
}
const service = new ConfigService();
await conf.update('path.server', './relative/oxc_language_server');
const relativeServerPath = service.getUserServerBinPath();
let workspace_path = getWorkspaceFolderPlatformSafe();

strictEqual(workspace_path[1], ':', 'The test workspace folder must be an absolute path with a drive letter on Windows');
strictEqual(relativeServerPath, `${workspace_path}\\relative\\oxc_language_server`);
});
});
});
Loading