Skip to content
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: propagate --ignore-scripts to yarn v1 dependency bootstrap if enableScripts=false #6259

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

legobeat
Copy link

@legobeat legobeat commented Apr 30, 2024

What's the problem this PR addresses?

This passes --ignore-scripts for the bootstrapping of Yarn Classic packages if enableScripts==false.

How did you fix it?

  • Read settings from configuration
  • Iif configured to disable lifecycle scripts: Extend arguments passed to yarn v1 with --ignore-scripts .

Checklist

  • I have set the packages that need to be released for my changes to be effective.
    • Note: I marked all as patch to make the check pass locally but I'm not certain they are actually all required. Could use some guidance in case this needs adjusting.
  • I will check that all automated PR checks pass before the PR gets reviewed.

  • Add test coverage for new branch by adding new test-case

    • Might need some assistance in adding test. I am not familiar enough with the usage of git workspaces here.

@legobeat legobeat marked this pull request as ready for review April 30, 2024 21:51
@legobeat legobeat force-pushed the yarn-v1-pass-ignore-scripts branch from 6e9a032 to 6d78713 Compare May 2, 2024 22:42
@arcanis
Copy link
Member

arcanis commented May 6, 2024

I'm not 100% convinced this is the right approach. While it makes sense to do that, it would only work for Yarn Classic - not for npm, neither pnpm. It would also only affect one setting.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

@legobeat
Copy link
Author

legobeat commented May 6, 2024

I'm not 100% convinced this is the right approach. While it makes sense to do that, it would only work for Yarn Classic - not for npm, neither pnpm. It would also only affect one setting.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

Good point that this PR is addressing the Yarn Classic case and is therefore only a partial resolution of #6258 in its current state.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

Is that not precisely what enableScripts is intended for?

If the user relies on enableScripts as a security mechanism to disable lifecycle scripts, it's not worth much if all it takes for a malicious/compromised package to unlock its scripts is to ship a lockfile...

(I also believe Yarn Classic has some mechanism to override this via environment variables, in case anyone explicitly wants to e.g. enable install scripts for all dependencies except those with yarn v1 lockfiles in the package root?).

@legobeat legobeat force-pushed the yarn-v1-pass-ignore-scripts branch from 6d78713 to 5c7e7b9 Compare May 6, 2024 08:52
@naugtur
Copy link

naugtur commented May 7, 2024

I would argue (and the file name scriptUtils.ts is kinda supporting me here) that setting enableScripts=false should prevent the whole process of building the external package from happening.

  • People set enableScripts=false for security reasons, not for speed or convenience
  • enableScripts=false already breaks more prominent things than this
  • when enableScripts=false is set the expectation is no scripts would run, which is not the case here - this functionality does run not only for local folders but also git protocol dependencies.

@legobeat legobeat force-pushed the yarn-v1-pass-ignore-scripts branch from 5c7e7b9 to d264d3b Compare May 7, 2024 08:43
@legobeat
Copy link
Author

legobeat commented May 7, 2024

@naugtur That makes sense. The docs say:

If false, Yarn will not execute the postinstall scripts from third-party packages when installing the project (workspaces will still see their postinstall scripts evaluated, as they're assumed to be safe if you're running an install within them)

This does not seem to include git dependencies.

This is being implemented in #6280 (which would make this PR redundant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants