-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix: workspace incorrectly resolves node_modules path during script execution on windows (fixes #4564) #9078
base: master
Are you sure you want to change the base?
Conversation
Hi @BYK! I saw you had this issue assigned before. If you're not free, would you happen to know who would be best to talk with? I'm happy to do whatever is necessary to get this pushed through. I'm assuming it should be fairly straightforward in that we know that output I can't imagine a case where using the symlink path for the output env PATH would be more useful, but I wanted to check with the team first. Appreciate you taking the time to read and point me in the right direction! |
…ipt execution (fixes yarnpkg#4564)
@@ -195,7 +195,12 @@ export async function makeEnv( | |||
pathParts.unshift(path.join(config.workspaceRootFolder, binFolder)); | |||
} | |||
pathParts.unshift(path.join(config.linkFolder, binFolder)); | |||
pathParts.unshift(path.join(cwd, binFolder)); | |||
|
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.
We could narrow this by checking to see if os is windows first and/or stat to see if it's a symlink.
NotesNPXLooks like Alternatively, we can just go with the other route and make the cmd file resolve the real path, but I'd assume upstream fixes are best. Will wait on hearing back from the Yarn team. CommandsLooks like we need to also cover commands like Presumably, this is in I will hold on working further until I get a response from the team on what approach they prefer |
FYI team — Here is a workaround I created with tests that handles this issue by transforming the generated https://github.com/nonara/yarn-fix-bin-cmds Final cmd file looks something like this:
If this approach makes sense, I can revise this PR to do similar. FWIW, I believe this method should be compatible with Windows 7+. If need be, for compatibility, we can add a failsafe to make it execute using %~dp0 as a fallback in the absence of powershell |
Background Summary
The issue arises from the way Yarn handles symlink paths for executable files in node_modules.
When executables like rimraf.cmd are run from symlinked directories (like those under /node_modules/@pkg/a/node_modules/.bin), the paths resolve incorrectly relative to the repository root.
This problem is caused because Yarn adds the symlink path instead of the real, resolved path to the PATH environment variable. As a result, relative paths calculated in scripts point incorrectly, leading to failure in locating the correct files (e.g., prisma\build\index.js).
More Detail: #4564 (comment)
Severity & Notes
This issue has been a pervasive one, having been first reported over 7 years ago. I've run into it multiple times and the severity is such that it entirely cripples complex projects.
It appears to also be present in Yarn 3.
I have a fix submitted, but I'm also submitting multiple solution proposals. I'm happy to discuss these with the team and do whatever is most suitable.
I noticed that some patches are still being accepted for Yarn 1, which are not strictly security related, so I'm hopeful we can get this through as well.
With that said, I am also willing to address it in Yarn 3+ if need-be to get this merged, however, I'd love to get this merged in Yarn 1, as many of us still use it.
Issue
Solutions
Solution 1 : Modify execute-lifecycle-script.js
This would be the most ideal IMO, assuming there are no side-effects.
A simple change is to edit
makeEnv
to the following:Solution 2 : Modify cmd files
We can resolve symlinks in the generated command files.
Here is an example:
We'd want to ensure that this works with Win XP and up, but I believe it will.
If not, we can also do something more simple, like using cd to the path.
Questions for the Yarn Team
Questions I'd have for the Yarn team are:
a. Would it be better to just use realpath at a higher level?
b. Is there any potential downside to modifying it here?
Failing tests
Looks like I'm getting some failures:
This seems to be happening locally and on the latest master (before my changes) as well. Not sure what's going on here, as that package exists on npmjs registry, but if anyone has any guidance, I'm happy to make adjustments to ensure all passes.