npm / cli

the package manager for JavaScript
https://docs.npmjs.com/cli/
Other
8.53k stars 3.2k forks source link

[BUG] project config still loaded if userconfig sets global=true #5797

Open isaacs opened 3 years ago

isaacs commented 3 years ago

What / Why

https://github.com/npm/config/pull/23 will make npm not load the project config if global=true in cli or env.

However, if global=true is set in the userconfig, then the project config is still loaded.

If this is an unsupported case, then it should be documented. If not, then... I guess project config would have to be unloaded, since it can change the location of the userconfig? So, you could have a project config that sets userconfig=/path/to/userconfig/a, and then /path/to/userconfig/a contains global=true, which means we shouldn't load the project config, so now we loaded the wrong userconfig?

Maybe project config shouldn't be able to set the userconfig/globalconfig options?

Failing test showing the issue (but really, this just highlights that it's a weird paradox loop, not that this test should be made to pass, necessarily)

diff --git a/test/index.js b/test/index.js
index 17f0879..7d9ff5c 100644
--- a/test/index.js
+++ b/test/index.js
@@ -122,7 +122,11 @@ prefix = ${path}/global
 user-config-from-builtin = true
 foo = from-custom-userconfig
 globalconfig = ${path}/global/etc/npmrc
-`
+`,
+      '.npmrc-userconfig-sets-global-true': `
+globalconfig = ${path}/global/etc/npmrc
+global = true
+`,
     },
     project: {
       node_modules: {},
@@ -190,6 +194,22 @@ loglevel = yolo
     t.equal(config.sources.get(source), 'project', 'sources has project')
   })

+  t.test('dont load project config if global set by user config', async t => {
+    const config = new Config({
+      npmPath: `${path}/npm`,
+      env: {},
+      argv: [process.execPath, __filename, `--userconfig=${path}/user/.npmrc-userconfig-sets-global-true`],
+      cwd: `${path}/project`,
+      shorthands,
+      definitions,
+    })
+
+    await config.load()
+    const source = config.data.get('project').source
+    t.equal(source, '(global mode enabled, ignored)', 'data has placeholder')
+    t.equal(config.sources.get(source), 'project', 'sources has project')
+  })
+
   t.test('dont load project config if location is global', async t => {
     const config = new Config({
       npmPath: `${path}/npm`,
isaacs commented 3 years ago

Also, what happens if the project config sets global=true?

Is global just something that shouldn't ever be set by a config file?

Should project config be allowed to set userconfig/globalconfig? The userconfig setting the globalconfig is a thing I know some users depend on, and seems much less hazardous.

The point is: if "global=true + loaded a project config" is a combination we don't want, then there's some more prickly surface area to cover here.