diff --git a/.changeset/clever-warnings-guide.md b/.changeset/clever-warnings-guide.md new file mode 100644 index 0000000000..484e5ae3da --- /dev/null +++ b/.changeset/clever-warnings-guide.md @@ -0,0 +1,6 @@ +--- +"@pnpm/config.reader": patch +"pnpm": patch +--- + +Improved the warning printed when a project `.npmrc` uses an environment variable in a registry/proxy URL or in registry credentials. The message now explains why the setting was ignored and how to migrate it to a trusted source — for example by moving the line to the user-level `~/.npmrc` or running `pnpm config set "" ` — with a link to https://pnpm.io/npmrc. The `pnpm config set` example is only suggested when the key has no `${...}` placeholder, so the snippet is always safe to copy-paste. diff --git a/config/reader/src/loadNpmrcFiles.ts b/config/reader/src/loadNpmrcFiles.ts index d5a36f8552..ed86a6a269 100644 --- a/config/reader/src/loadNpmrcFiles.ts +++ b/config/reader/src/loadNpmrcFiles.ts @@ -265,12 +265,36 @@ function hasEnvPlaceholder (value: string): boolean { return /\$\{[^}]+\}/.test(value) } +const DOCS_URL = 'https://pnpm.io/npmrc' + +// The key embedded in the suggested `pnpm config set` command comes from a +// repository-controlled .npmrc. A shell expands `$(...)`, backticks and `$VAR` +// even inside double quotes, so suggesting a runnable command built from an +// arbitrary key would turn this warning into a copy-paste command-injection +// vector. Only emit the runnable example for keys made up entirely of +// shell-inert characters — which covers every real registry/auth key +// (`//host/:_authToken`, `@scope:registry`, `registry`, `https-proxy`, …). +const SHELL_SAFE_KEY = /^[\w@.:/-]+$/ + +function configSetExample (key: string): string { + return SHELL_SAFE_KEY.test(key) ? ` (for example, run: pnpm config set "${key}" )` : '' +} + function warnIgnoredRequestDestinationEnv (filePath: string, key: string, warnings: string[]): void { - warnings.push(`Ignored project-level request destination "${key}" in "${filePath}": environment variables are not expanded in repository-controlled registry or proxy URLs.`) + warnings.push(`Ignored project-level request destination "${key}" in "${filePath}": ` + + 'environment variables are not expanded in registry or proxy URLs that come from a project .npmrc, ' + + 'because that file is committed to the repository and a malicious value could redirect requests or leak secrets. ' + + 'Move this setting to a trusted source that pnpm still expands — put it in your user-level ~/.npmrc, ' + + `or set it with pnpm config set${configSetExample(key)}. ` + + `If the value is not secret, you can also write it literally in the project .npmrc. See ${DOCS_URL}`) } function warnIgnoredAuthValueEnv (filePath: string, key: string, warnings: string[]): void { - warnings.push(`Ignored project-level auth setting "${key}" in "${filePath}": environment variables are not expanded in repository-controlled registry credentials.`) + warnings.push(`Ignored project-level auth setting "${key}" in "${filePath}": ` + + 'environment variables are not expanded in registry credentials that come from a project .npmrc, ' + + 'because that file is committed to the repository and could leak the secret to an attacker-controlled registry. ' + + 'Move this credential to a trusted source that pnpm still expands — put the line in your user-level ~/.npmrc, ' + + `or set it with pnpm config set${configSetExample(key)}. See ${DOCS_URL}`) } // Rewrite any unscoped per-registry keys in `source` to their URL-scoped diff --git a/config/reader/test/index.ts b/config/reader/test/index.ts index 4807f59e60..4319ec7664 100644 --- a/config/reader/test/index.ts +++ b/config/reader/test/index.ts @@ -627,6 +627,11 @@ test('project .npmrc does not expand env variables in registry URLs', async () = expect(warnings).toEqual(expect.arrayContaining([ expect.stringContaining('Ignored project-level request destination "registry"'), ])) + // The warning should guide the user toward a trusted source and the docs. + const registryWarning = warnings.find((w) => w.includes('Ignored project-level request destination "registry"')) ?? '' + expect(registryWarning).toContain('~/.npmrc') + expect(registryWarning).toContain('pnpm config set "registry" ') + expect(registryWarning).toContain('https://pnpm.io/npmrc') }) test('project .npmrc does not expand env variables in scoped registry URLs or URL-scoped keys', async () => { @@ -653,6 +658,33 @@ test('project .npmrc does not expand env variables in scoped registry URLs or UR expect.stringContaining('Ignored project-level request destination "@scope:registry"'), expect.stringContaining('Ignored project-level request destination "//registry.example.com/${PNPM_TEST_TOKEN}/:_authToken"'), ])) + // When the key itself contains a ${...} placeholder, the warning must not + // embed it in a runnable `pnpm config set ""` command — a shell would + // expand the placeholder on copy-paste. + const urlScopedWarning = warnings.find((w) => w.includes('//registry.example.com/${PNPM_TEST_TOKEN}/:_authToken')) ?? '' + expect(urlScopedWarning).not.toContain('pnpm config set "') + expect(urlScopedWarning).toContain('~/.npmrc') +}) + +test('the warning never embeds a shell-unsafe key in a runnable pnpm config set command', async () => { + prepare() + + // A malicious repository could craft a key with shell metacharacters; the + // suggested copy-paste command must not become a command-injection vector. + fs.writeFileSync('.npmrc', '//$(touch pwned)`id`/:_authToken=${PNPM_TEST_TOKEN}\n', 'utf8') + + const { warnings } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + const unsafeWarning = warnings.find((w) => w.includes('$(touch pwned)')) ?? '' + expect(unsafeWarning).not.toBe('') + expect(unsafeWarning).not.toContain('pnpm config set "') }) test('project .npmrc does not expand env variables in auth values', async () => { @@ -702,6 +734,11 @@ test('project .npmrc does not expand env variables in auth values', async () => expect.stringContaining('Ignored project-level auth setting "cert"'), expect.stringContaining('Ignored project-level auth setting "key"'), ])) + // The warning should tell the user how to migrate the credential. + const authWarning = warnings.find((w) => w.includes('Ignored project-level auth setting "//attacker.example/:_authToken"')) ?? '' + expect(authWarning).toContain('pnpm config set "//attacker.example/:_authToken" ') + expect(authWarning).toContain('~/.npmrc') + expect(authWarning).toContain('https://pnpm.io/npmrc') }) test('project .npmrc does not expand env variables in proxy URLs', async () => {