Skip to content

Conversation

Fdawgs
Copy link
Member

@Fdawgs Fdawgs commented Sep 17, 2025

After the recent supply chain attacks that use install scripts, we should enable this everywhere.

This was already enabled in the main fastify repo as part of fastify/fastify#6108.
If this is approved and merged then I will do a batch of PRs to the rest of the repos.

Checklist

Signed-off-by: Frazer Smith <[email protected]>
@Fdawgs Fdawgs requested a review from mcollina September 17, 2025 08:22
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

In a lot of repositories we use @fastify/pre-commit to set up the pre-commit script (not sure here). We should ideally white-list that.

@Fdawgs
Copy link
Member Author

Fdawgs commented Sep 17, 2025

lgtm

In a lot of repositories we use @fastify/pre-commit to set up the pre-commit script (not sure here). We should ideally white-list that.

Is there a way to whitelist? Only thing I can thing of is running npm rebuild @fastify/pre-commit

If not, maybe we just need to remove @fastify/pre-commit? From looking at where it is used it runs lint and test, both of which the CI workflows run anyway.

@Fdawgs Fdawgs requested a review from a team September 17, 2025 09:16
@jsumners
Copy link
Member

We should ideally white-list that.

The only way I see to enable that is https://github.com/LavaMoat/LavaMoat/tree/main/packages/allow-scripts.

I'm not so convinced the pre-commit script is worth it any longer. In my view, it was meant to assure a commit is "clean" before it gets suggested as a change in a PR. But a lot of people just clone, edit, and PR. They never even install dependencies and simply rely on CI to do all of the work. Personally, I end up skipping it more often than not with -n because it's an annoying step that probably won't work due to all of the not-JavaScript stuff.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

RodrigoDornelles pushed a commit to fastify/fastify-opensearch that referenced this pull request Sep 18, 2025
Fdawgs added a commit to fastify/fastify-cli that referenced this pull request Sep 18, 2025
Fdawgs added a commit to fastify/gh-issues-finder that referenced this pull request Sep 21, 2025
See fastify/deepmerge#78. This is a batch PR created by a script. Please review prior to merging.

Signed-off-by: Frazer Smith <[email protected]>
Fdawgs added a commit to fastify/gh-issues-finder that referenced this pull request Sep 21, 2025
See fastify/deepmerge#78. This is a batch PR created by a script. Please review prior to merging.

Signed-off-by: Frazer Smith <[email protected]>
Fdawgs added a commit to Fdawgs/node-unrtf that referenced this pull request Sep 23, 2025
Fdawgs added a commit to Fdawgs/node-unrtf that referenced this pull request Sep 23, 2025
Fdawgs added a commit to Fdawgs/redact that referenced this pull request Oct 15, 2025
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.

4 participants