Skip to content

Apply npm hardening + npm audit fix - #27614

Open
HCL-JasonR wants to merge 2 commits into
emscripten-core:mainfrom
HCL-TECH-SOFTWARE:npm-hardening-emscripten
Open

Apply npm hardening + npm audit fix#27614
HCL-JasonR wants to merge 2 commits into
emscripten-core:mainfrom
HCL-TECH-SOFTWARE:npm-hardening-emscripten

Conversation

@HCL-JasonR

Copy link
Copy Markdown
Contributor

This locks down npm to only take packages that are over a week old and disables scripts, both of which are best practices against the recent Shai-Hulud tree of supply chain attacks.

Comment thread .npmrc Outdated
# to detect and report malicious releases.
# To override for an urgent update: npm install <pkg> --min-release-age=0
min-release-age=7

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove empty line

Comment thread package.json
"check": "prettier --check src/*.mjs tools/*.mjs --ignore-path src/pthread_esm_startup.mjs"
},
"allowScripts": {
"esbuild": false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this disallow esbuild? Is that not covered by the strict-allow-scripts=true blanket prohibition?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It fixes a warning that we're relying on the default.

npm warn install-scripts 1 package has install scripts not yet covered by allowScripts:
npm warn install-scripts esbuild@0.27.2 (postinstall: node install.js)

@sbc100

sbc100 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

From the CI failures it looks like emsdk is not shipping npm new enough?

@HCL-JasonR

Copy link
Copy Markdown
Contributor Author

From the CI failures it looks like emsdk is not shipping npm new enough?

CI error for reference:
npm ERR! notsup Required: {"npm":">=11.17.0"}
npm ERR! notsup Actual: {"npm":"10.5.0","node":"v21.7.3"}

https://app.circleci.com/pipelines/gh/emscripten-core/emscripten/54096/workflows/f458a6a6-dfd2-4474-b4d8-ce06da12657e/jobs/1288815

When I installed the latest emsdk (6.0.8), it pulled in node 24.19 + npm 11.19. The logs from my machine:
./emsdk install 6.0.8
Resolving SDK version '6.0.8' to 'sdk-releases-9d70dbe8860ccdd3595f6e6065d94bfb543ae955-64bit'
Installing SDK 'sdk-releases-9d70dbe8860ccdd3595f6e6065d94bfb543ae955-64bit'..
Installing tool 'node-24.19.0-64bit'..
Downloading: /local/emsdk/downloads/node-v24.19.0-linux-x64.tar.xz from https://storage.googleapis.com/webassembly/emscripten-releases-builds/deps/node-v24.19.0-linux-x64.tar.xz, 31633904 Bytes
Unpacking '/local/emsdk/downloads/node-v24.19.0-linux-x64.tar.xz' to '/local/emsdk/node/24.19.0_64bit'
Done installing tool 'node-24.19.0-64bit'.

@sbc100

sbc100 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Oh I see whats happening.

The CI windows machine end up using C:\Program Files\nodejs\npm.CMD ci since emsdk does not put its own version of node in the PATH if there is already a version present.

I'm not sure what the best solution here ... the idea is that emsdk users can have their own version node in their PATH but emscripten internally will always use the emsdk node.

I think mabye we could have bootstrap.py use the emsdk version node rather than one in the PATH...

@HCL-JasonR

Copy link
Copy Markdown
Contributor Author

The node version requirement was my biggest concern.

I'm not opposed to nudging people to update EOL releases, but I'm also not handling frontline tickets on this update breaking their build. Node 21 from the CI was last updated 2 years ago and is EOL (source: https://nodejs.org/en/about/previous-releases ).

I'd assume that a lot of people use the bundled emsdk when building a project with emscripten.

As far as bootstrap.py, I'm slightly less familiar with all of the code paths, but I don't know if we're going to run into version mismatch trouble with something like this from tools/link.py:
shared.check_call([*shared.get_npm_cmd('html-minifier-terser'), filename, '-o', filename, *opts], env=shared.env_with_node_in_path())

@sbc100

sbc100 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

boostrap fix is in #27617

sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 28, 2026
When `EMSDK_NODE` is present in the environment, prepend its directory
to `PATH` before running bootstrap actions.

By default, emsdk avoids prepending its own node binary to `PATH` if a
system node is already present in `PATH`. However, bootstrap actions
(such as `npm ci`) may fail if the system node is an older incompatible
version or lacks npm.

See: emscripten-core#27614
@HCL-JasonR

Copy link
Copy Markdown
Contributor Author

Would you prefer that I merge that into my branch to test, or wait until that PR gets merged?

@sbc100

sbc100 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

The latter

sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 28, 2026
When `EMSDK_NODE` is present in the environment, prepend its directory
to `PATH` before running bootstrap actions.

By default, emsdk avoids prepending its own node binary to `PATH` if a
system node is already present in `PATH`. However, bootstrap actions
(such as `npm ci`) may fail if the system node is an older incompatible
version or lacks npm.

See: emscripten-core#27614
sbc100 added a commit that referenced this pull request Aug 29, 2026
When `EMSDK_NODE` is present in the environment, prepend its directory
to `PATH` before running bootstrap actions.

By default, emsdk avoids prepending its own node binary to `PATH` if a
system node is already present in `PATH`. However, bootstrap actions
(such as `npm ci`) may fail if the system node is an older incompatible
version or lacks npm.

See: #27614
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.

3 participants