Skip to content

fix(config): avoid exporting persistent allow-scripts #9913

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.

Sign up for GitHub

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

Open
Fnine59 wants to merge 5 commits into
base: latest
Choose a base branch
from
+54 −1
Open

fix(config): avoid exporting persistent allow-scripts #9913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
473d216
fix(config): avoid exporting persistent allow-scripts
Fnine59 Aug 24, 2026
8c23b3e
Merge upstream/latest into fix/npm-allow-scripts-env-9912
Fnine59 Aug 26, 2026
843d75c
Merge upstream/latest into fix/npm-allow-scripts-env-9912
Fnine59 Aug 26, 2026
3a3a553
chore(config): add allow-scripts policy coverage
Fnine59 Aug 27, 2026
c06d865
chore(config): update docs snapshot
Fnine59 Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tap-snapshots/test/lib/docs.js.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ Each name is matched against a dependency's resolved identity, not against
the package's self-reported name. \`--ignore-scripts\` and
\`--dangerously-allow-all-scripts\` both override this setting.


This value is not exported to the environment for child processes.

#### \`allow-scripts-pending\`

Expand Down
16 changes: 16 additions & 0 deletions test/lib/utils/resolve-allow-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ t.test('--allow-scripts CLI flag is rejected in project-scoped installs', async
)
})

t.test('allow-scripts environment policy is rejected in project-scoped installs', async t => {
const mock = await mockNpm(t, {
prefixDir: {
'package.json': JSON.stringify({ name: 'p' }),
},
globals: {
'process.env.npm_config_allow_scripts': 'canvas',
},
})
const resolveAllowScripts = loadResolver(t)
await t.rejects(
resolveAllowScripts(mock.npm),
{ code: 'EALLOWSCRIPTS', message: /--allow-scripts is not allowed/ }
)
})

t.test('--allow-scripts CLI flag is accepted in global installs (RFC layer 1 wins)', async t => {
const mock = await mockNpm(t, {
prefixDir: {
Expand Down
1 change: 1 addition & 0 deletions workspaces/config/lib/definitions/definitions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ const definitions = {
default: '',
type: [String, Array],
hint: '<package-list>',
envExport: false,

@martinrrm martinrrm Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix. Marking allow-scripts  as envExport: false is the right shared solution: it prevents Config.load() / setEnvs() from turning file-backed policy into an environment-layer override before either npm run or Pacote Git preparation starts a child process. This addresses #9912 and actually fixes the persistent- .npmrc case in #9783 .

@martinrrm martinrrm Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Fnine59 Setting envExport: false causes Definition.describe() to append the non-export notice. test/lib/docs.js snapshots every generated config description, but the committed docs.js.test.cjs snapshot still lacks that notice for allow-scripts. Regenerate and commit tap-snapshots/test/lib/docs.js.test.cjs

TAP_SNAPSHOT=1 node test/lib/docs.js

@Fnine59 Fnine59 Aug 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated in c06d865: regenerated and committed the config description snapshot. Verified node test/lib/docs.js passes on Node 24.15.0 (6/6).

description: `
Comma-separated list of packages whose install-time lifecycle scripts
(\`preinstall\`, \`install\`, \`postinstall\`, and \`prepare\` for
Expand Down
36 changes: 36 additions & 0 deletions workspaces/config/test/set-envs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,39 @@ t.test('dont set configs marked as envExport:false', t => {
t.strictSame(env, { ...extras }, 'not exported, because envExport=false')
Comment thread
martinrrm marked this conversation as resolved.
t.end()
})

t.test('does not export persistent allow-scripts config', t => {
const { definitions, defaults } = mockDefinitions(t)
const userConf = Object.create(defaults)
userConf['allow-scripts'] = 'canvas'
const envConf = Object.create(userConf)
const cliConf = Object.create(envConf)
const env = {}
const config = {
list: [cliConf, envConf],
env,
defaults,
definitions,
execPath,
globalPrefix,
localPrefix,
npmPath,
npmBin,
}

setEnvs(config)
t.equal(
env.npm_config_allow_scripts,
undefined,
'persistent policy is reloaded instead of exported to lifecycle scripts'
)
envConf['allow-scripts'] = 'sharp'
env.npm_config_allow_scripts = 'sharp'
setEnvs(config)
t.equal(
env.npm_config_allow_scripts,
'sharp',
'an explicit environment policy remains inherited'
)
t.end()
Comment thread
martinrrm marked this conversation as resolved.
})