-
-
Notifications
You must be signed in to change notification settings - Fork 679
fix(editor): execute oxc.path.server
in win32 with shell, when its the node_module binary
#14203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -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. | ||
// 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'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.