Skip to content

ci: add --ignore-scripts to npm tool installs (Sonar) - #169

Merged
achoimet merged 1 commit into
mainfrom
chore/npm-install-ignore-scripts
Aug 11, 2026
Merged

ci: add --ignore-scripts to npm tool installs (Sonar)#169
achoimet merged 1 commit into
mainfrom
chore/npm-install-ignore-scripts

Conversation

@achoimet

Copy link
Copy Markdown
Member

SonarCloud flags npm installs in workflows that omit --ignore-scripts as major vulnerabilities ("Omitting --ignore-scripts allows lifecycle scripts to run during package installation") — this is what actually failed the platform nightly quality gate, not version pinning. Adds --ignore-scripts to the npm self-install (the npm package has no lifecycle scripts; verified 12.0.2 installs and runs correctly with the flag).

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @achoimet's task in 1m 38s —— View job


Review

  • Check for previously unresolved Claude review threads to resolve (none found)
  • Gather context (diff, workflow file, CLAUDE.md)
  • Review the change for bugs, security, performance, correctness
  • Post final review feedback

Summary: Small, well-scoped change — adds --ignore-scripts to the two remaining npm install -g npm@12.0.2 self-installs and switches the snyk install from --allow-scripts=snyk to --ignore-scripts, per Sonar's flag on unrestricted lifecycle-script execution during npm install. All npm install invocations in reusable-extension-ci.yml are now consistently using --ignore-scripts (verified via grep, lines 150, 151, 446, 447) — no missed spots.

Findings:

  • No bugs, security regressions, or performance issues introduced. Disabling lifecycle scripts on npm@12.0.2 and snyk reduces (rather than increases) the supply-chain attack surface, which is the intent here.
  • One thing worth a sanity check rather than a blocker: the snyk install line (.github/workflows/reusable-extension-ci.yml:151) only executes when startsWith(github.ref, 'refs/tags/'), i.e. on release tags — this PR's own CI run won't exercise that step, so the "lazily downloads its CLI binary on first run" fallback claimed in the updated comment can't be confirmed by this PR's checks. Worth keeping an eye on the next tagged release to make sure snyk test still succeeds without the postinstall step.
  • The previous --allow-scripts=snyk flag being replaced isn't a real npm CLI option (npm's only related config is the boolean ignore-scripts), so it likely had no effect previously — this change is the first time scripts are actually being suppressed for the snyk install. That's consistent with the PR's stated intent and not a regression, just worth being aware of when validating the release-time behavior.

No changes requested — looks good to merge from a correctness/security standpoint.

@achoimet
achoimet merged commit f331607 into main Aug 11, 2026
6 checks passed
@achoimet
achoimet deleted the chore/npm-install-ignore-scripts branch August 11, 2026 12:26
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 11, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant