fixed scripts on windows environments #4174
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@dpuhlmann you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
PullRequest Breakdown
Reviewable lines of change
+ 6
- 5
100% JSONGenerated lines of change
+ 8
- 1
Type of change
Fix - These changes are likely to be fixing a bug or issue.
| 1 Message | |
|---|---|
| 📚 | It looks like the description for this pull request is either blank or very short. Adding a high-level summary will help our reviewers provide better feedback. Feel free to include questions for PullRequest reviewers and make specific feedback requests. |
There was a problem hiding this comment.
cross-env should not be used any more. It is EOL as per kentcdodds/cross-env#257. What issue are you trying to solve with this PR?
Reviewed with ❤️ by PullRequest
According to the linked issue cross-env is in maintenance mode not EOL, which should still make it perfectly fine to include. Also there seem to be no good alternatives. |
There was a problem hiding this comment.
This use of cross-env looks good. I don't see any issues here.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
cross-env should not be used any more. It is EOL as per kentcdodds/cross-env#257. What issue are you trying to solve with this PR?
Reviewed with ❤️ by PullRequest
According to the linked issue cross-env is in maintenance mode not EOL, which should still make it perfectly fine to include. Also there seem to be no good alternatives.
That post is a little out of date with regards to that which is why I had said that it is now EOL. The repo was marked as read-only in 2021, a year after the post was made, making it EOL and a security risk to include, since any security issues may never be properly disclosed nor fixed.
Okay, i see your point. But how relevant is that for including it as a dev dependency? It will only be used at build time and not distributed with the package. |
There was a problem hiding this comment.
cross-env should not be used any more. It is EOL as per kentcdodds/cross-env#257. What issue are you trying to solve with this PR?
Reviewed with ❤️ by PullRequestAccording to the linked issue cross-env is in maintenance mode not EOL, which should still make it perfectly fine to include. Also there seem to be no good alternatives.
That post is a little out of date with regards to that which is why I had said that it is now EOL. The repo was marked as read-only in 2021, a year after the post was made, making it EOL and a security risk to include, since any security issues may never be properly disclosed nor fixed.
Okay, i see your point. But how relevant is that for including it as a dev dependency? It will only be used at build time and not distributed with the package.
Also what would be the alternative? Just not supporting development on Windows anymore doesn't sound like a good option.
Security issues are just as relevant for development, if not more so, because the consequences can be worse if takeover of the developer's machine or access to their GitHub account can be achieved. What exactly is broken on Windows today? Is it mitigated by using WSL?
First to what was (is) broken: Regarding WSL as an alternative: For security risks: Cross-env is talking as input scripts from package.json and process variables. For the scripts I have to already trust them when executing them and they go through the normal contribution process of the library. So they are not more dangerous then any other piece of code in this package. Or to put it the other way around: If someone is able to inject malicous code into the library (or package scripts) cross-env is the more minor problem. |
There was a problem hiding this comment.
cross-env should not be used any more. It is EOL as per kentcdodds/cross-env#257. What issue are you trying to solve with this PR?
Reviewed with ❤️ by PullRequestAccording to the linked issue cross-env is in maintenance mode not EOL, which should still make it perfectly fine to include. Also there seem to be no good alternatives.
That post is a little out of date with regards to that which is why I had said that it is now EOL. The repo was marked as read-only in 2021, a year after the post was made, making it EOL and a security risk to include, since any security issues may never be properly disclosed nor fixed.
Okay, i see your point. But how relevant is that for including it as a dev dependency? It will only be used at build time and not distributed with the package.
Also what would be the alternative? Just not supporting development on Windows anymore doesn't sound like a good option.Security issues are just as relevant for development, if not more so, because the consequences can be worse if takeover of the developer's machine or access to their GitHub account can be achieved. What exactly is broken on Windows today? Is it mitigated by using WSL?
First to what was (is) broken:
Any script from package.json that sets NODE_OPTIONS and/or NODE_ENV will not work in CMD/PowerShell (nor in Git Bash, which on some occasions can help with windows comand line problems). The affected sripts include "build" and "test" which make local development nearly impossible.Regarding WSL as an alternative:
This might be an option for some developers (if it works there, which I do not know). For myself this is not a possibility due to me beeing on a company machine with limited permissions, especially regarding installation of software.For security risks: Cross-env is talking as input scripts from package.json and process variables. For the scripts I have to already trust them when executing them and they go through the normal contribution process of the library. So they are not more dangerous then any other piece of code in this package. Or to put it the other way around: If someone is able to inject malicous code into the library (or package scripts) cross-env is the more minor problem.
For the process variables that is even more true: If an attacker is able to manipulate the process environment on my machine I already have an enormous problem, where again the use cross-env is of secondary concern.
Or do you see any other realistic attack vectors that would be introduced by the use of cross-env?
The problem comes from if someone is able to take over the account of the abandoned package and publish malicious code, which has happened on the past with similar packages, more than once, every year for the last few years, and because this is library code, there is no protection from this kind of attack since people are doing dev work on the library using npm install rather than npm ci with a committed lock file. Use of abandoned packages is always a bad idea from a security perspective.
Do any contributors to this library actually use Windows and not Docker or WSL?
There was a problem hiding this comment.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.
I'm not entirely convinced by this argument, since all they are just helpers or ways to execute commands; have you considered some of the alternatives:
I'm not strongly against adding cross-env back, I think the security implications are marginal - but I do think different solutions probably work better than pulling in a unmaintained dependency; but as it's a dev dependency I'm not hugely concerned. Will this unblock you to contribute to this repository or are you adding this fix just because it fixes a problem you're aware of? |
You can use |
|
Looks like dotenvx should be sufficient. |
Any script from package.json that sets NODE_OPTIONS and/or NODE_ENV will not work in CMD/PowerShell (nor in Git Bash, which on some occasions can help with windows comand line problems). The affected sripts include "build" and "test" which make local development nearly impossible.