diff --git a/.changeset/redact-ignored-npmrc-key-userinfo.md b/.changeset/redact-ignored-npmrc-key-userinfo.md new file mode 100644 index 0000000000..65294a07af --- /dev/null +++ b/.changeset/redact-ignored-npmrc-key-userinfo.md @@ -0,0 +1,6 @@ +--- +"@pnpm/config.reader": patch +"pnpm": patch +--- + +The warnings about ignored project `.npmrc` registry and auth settings no longer print the username and password of a URL-scoped key such as `//user:password@registry.example.com/:_authToken`. diff --git a/.changeset/redact-ignored-request-destination-userinfo.md b/.changeset/redact-ignored-request-destination-userinfo.md new file mode 100644 index 0000000000..45af32213e --- /dev/null +++ b/.changeset/redact-ignored-request-destination-userinfo.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +The warning about an ignored project `.npmrc` registry setting no longer prints the username and password of a URL-scoped key such as `//user:password@registry.example.com/${PATH}/:_authToken`. diff --git a/pnpm/crates/config/src/npmrc_auth/ini.rs b/pnpm/crates/config/src/npmrc_auth/ini.rs index 9600884f43..106670753e 100644 --- a/pnpm/crates/config/src/npmrc_auth/ini.rs +++ b/pnpm/crates/config/src/npmrc_auth/ini.rs @@ -261,6 +261,7 @@ impl NpmrcAuth { } pub(super) fn warn_ignored_request_destination_env(&mut self, key: &str) { + let key = redact_npm_auth_key(key); self.warnings.push(format!( "Ignored project-level request destination {key:?}: environment variables are not expanded in repository-controlled registry or proxy URLs.", )); diff --git a/pnpm/crates/config/src/npmrc_auth/tests/authorization.rs b/pnpm/crates/config/src/npmrc_auth/tests/authorization.rs index 558d1715af..11672a90a0 100644 --- a/pnpm/crates/config/src/npmrc_auth/tests/authorization.rs +++ b/pnpm/crates/config/src/npmrc_auth/tests/authorization.rs @@ -756,6 +756,23 @@ fn ignored_auth_warning_redacts_protocol_relative_userinfo() { assert!(!warning.contains("ss@registry")); } +#[test] +fn ignored_request_destination_warning_redacts_protocol_relative_userinfo() { + static_env!(Env, &[("REGISTRY_PATH", "npm")]); + + let auth = NpmrcAuth::from_project_ini::( + "//user:password@registry.example.com/${REGISTRY_PATH}/:_authToken=abc\n", + Path::new(""), + ); + let warning = auth.warnings + .iter() + .find(|warning| warning.contains("Ignored project-level request destination")) + .expect("ignored request destination warning"); + + assert!(warning.contains("//registry.example.com/${REGISTRY_PATH}/:_authToken")); + assert!(!warning.contains("user:password")); +} + #[test] fn from_ini_expands_auth_env_placeholder_without_warning() { static_env!(Env, &[("MY_TOKEN", "secret")]); diff --git a/pnpm11/config/reader/src/loadNpmrcFiles.ts b/pnpm11/config/reader/src/loadNpmrcFiles.ts index ab91a751df..0e868995fc 100644 --- a/pnpm11/config/reader/src/loadNpmrcFiles.ts +++ b/pnpm11/config/reader/src/loadNpmrcFiles.ts @@ -11,6 +11,7 @@ import { findLocalPrefix } from './findLocalPrefix.js' import { type JsonAuthResult, readGlobalConfigAuth, readJsonAuthEnv } from './jsonAuth.js' import { isNpmrcReadableKey } from './localConfig.js' import { parseCAFileContents } from './parseCAFileContents.js' +import { redactRegistryUrl } from './registryUrlUserinfo.js' import { rescopeUnscopedCreds } from './rescopeUnscopedCreds.js' export { findLocalPrefix } from './findLocalPrefix.js' @@ -412,14 +413,14 @@ function hasEnvPlaceholder (value: string): boolean { const DOCS_URL = 'https://pnpm.io/npmrc' function warnIgnoredRequestDestinationEnv (filePath: string, key: string, warnings: string[]): void { - warnings.push(`Ignored project-level request destination "${key}" in "${filePath}": ` + + warnings.push(`Ignored project-level request destination "${redactRegistryUrl(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. ' + `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}": ` + + warnings.push(`Ignored project-level auth setting "${redactRegistryUrl(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. ' + `See ${DOCS_URL}`) diff --git a/pnpm11/config/reader/src/registrySettings.ts b/pnpm11/config/reader/src/registrySettings.ts index ad7013e433..2c79aca96f 100644 --- a/pnpm11/config/reader/src/registrySettings.ts +++ b/pnpm11/config/reader/src/registrySettings.ts @@ -10,6 +10,7 @@ import normalizeRegistryUrl from 'normalize-registry-url' import type { OptionsFromRootManifest } from './getOptionsFromRootManifest.js' import { quoteAndJoin } from './quoteAndJoin.js' +import { redactRegistryUrl, registryUrlHasUserinfo } from './registryUrlUserinfo.js' import { assertBoolean, assertObjectSetting, assertString, assertStringArray } from './settingAssertions.js' const REGISTRY_SERVER_TYPES = new Set(['npm', 'artifactory']) @@ -236,61 +237,3 @@ function assertKnownRegistryFields (declaration: RegistryDeclaration, registry: function looksLikeRegistryUrl (key: string): boolean { return key.includes('://') || key.startsWith('//') } - -/** - * Whether the authority of `url` carries a `user:pass@` prefix. The authority - * ends at the first `/`, `?`, or `#`, so a later `@` in the path is not one. - * - * Both the full form and the scheme-less `//host/` form count. The latter is - * the shape `.npmrc` scopes settings with, so it is the one a user is most - * likely to reach for here. - */ -function registryUrlHasUserinfo (url: string): boolean { - return userinfoEnd(url) !== undefined -} - -/** - * The offset just past the `user:pass@` of `url`, or `undefined` when its - * authority carries none. Splitting it out keeps the detection and the - * redaction below agreeing on what the authority is. - */ -function userinfoEnd (url: string): number | undefined { - const authorityStart = authorityStartOf(url) - if (authorityStart === undefined) return undefined - const authority = url.slice(authorityStart) - const authorityEnd = authority.search(/[/?#]/) - const at = (authorityEnd === -1 ? authority : authority.slice(0, authorityEnd)).lastIndexOf('@') - return at === -1 ? undefined : authorityStart + at + 1 -} - -/** - * Where the authority of `url` begins, or `undefined` if it has none. - * - * The scheme is anchored at the start rather than found by searching for the - * first `://`: a `://` inside the path (`//host/a://b`) would otherwise be - * taken for the separator, and the real authority — credentials and all — - * would go unexamined. - */ -function authorityStartOf (url: string): number | undefined { - const schemeEnd = url.indexOf('://') - if (schemeEnd !== -1 && SCHEME.test(url.slice(0, schemeEnd))) return schemeEnd + '://'.length - if (url.startsWith('//')) return '//'.length - return undefined -} - -const SCHEME = /^[a-z][a-z0-9+.-]*$/i - -/** - * `url` with any `user:pass@` removed, safe to put in a message. - * - * {@link redactAndSanitize} only recognizes an authority after a `://`, and - * deliberately so: it runs over arbitrary prose, where a bare `//` is more - * often a comment or a path than a URL. Here the string is known to be a - * registry URL, so the scheme-less `//host/` form can be handled too. - */ -function redactRegistryUrl (url: string): string { - const authorityStart = authorityStartOf(url) - const end = userinfoEnd(url) - if (authorityStart === undefined || end === undefined) return redactAndSanitize(url) - return redactAndSanitize(`${url.slice(0, authorityStart)}${url.slice(end)}`) -} diff --git a/pnpm11/config/reader/src/registryUrlUserinfo.ts b/pnpm11/config/reader/src/registryUrlUserinfo.ts new file mode 100644 index 0000000000..c876f5fbaf --- /dev/null +++ b/pnpm11/config/reader/src/registryUrlUserinfo.ts @@ -0,0 +1,60 @@ +import { redactAndSanitize } from '@pnpm/error' + +/** + * Whether the authority of `url` carries a `user:pass@` prefix. The authority + * ends at the first `/`, `?`, or `#`, so a later `@` in the path is not one. + * + * Both the full form and the scheme-less `//host/` form count. The latter is + * the shape `.npmrc` scopes settings with, so it is the one a user is most + * likely to reach for here. + */ +export function registryUrlHasUserinfo (url: string): boolean { + return userinfoEnd(url) !== undefined +} + +/** + * The offset just past the `user:pass@` of `url`, or `undefined` when its + * authority carries none. Splitting it out keeps the detection and the + * redaction below agreeing on what the authority is. + */ +function userinfoEnd (url: string): number | undefined { + const authorityStart = authorityStartOf(url) + if (authorityStart === undefined) return undefined + const authority = url.slice(authorityStart) + const authorityEnd = authority.search(/[/?#]/) + const at = (authorityEnd === -1 ? authority : authority.slice(0, authorityEnd)).lastIndexOf('@') + return at === -1 ? undefined : authorityStart + at + 1 +} + +/** + * Where the authority of `url` begins, or `undefined` if it has none. + * + * The scheme is anchored at the start rather than found by searching for the + * first `://`: a `://` inside the path (`//host/a://b`) would otherwise be + * taken for the separator, and the real authority — credentials and all — + * would go unexamined. + */ +function authorityStartOf (url: string): number | undefined { + const schemeEnd = url.indexOf('://') + if (schemeEnd !== -1 && SCHEME.test(url.slice(0, schemeEnd))) return schemeEnd + '://'.length + if (url.startsWith('//')) return '//'.length + return undefined +} + +const SCHEME = /^[a-z][a-z0-9+.-]*$/i + +/** + * `url` with any `user:pass@` removed, safe to put in a message. + * + * {@link redactAndSanitize} only recognizes an authority after a `://`, and + * deliberately so: it runs over arbitrary prose, where a bare `//` is more + * often a comment or a path than a URL. Here the string is known to be a + * registry URL or a URL-scoped `.npmrc` key, so the scheme-less `//host/` + * form can be handled too. + */ +export function redactRegistryUrl (url: string): string { + const authorityStart = authorityStartOf(url) + const end = userinfoEnd(url) + if (authorityStart === undefined || end === undefined) return redactAndSanitize(url) + return redactAndSanitize(`${url.slice(0, authorityStart)}${url.slice(end)}`) +} diff --git a/pnpm11/config/reader/test/index.ts b/pnpm11/config/reader/test/index.ts index 15c56f2157..2dcb7a5384 100644 --- a/pnpm11/config/reader/test/index.ts +++ b/pnpm11/config/reader/test/index.ts @@ -1828,6 +1828,31 @@ test('project .npmrc does not expand env variables in scoped registry URLs or UR expect(urlScopedWarning).toContain('https://pnpm.io/npmrc') }) +test('ignored project .npmrc warnings do not print the userinfo of URL-scoped keys', async () => { + prepare() + + fs.writeFileSync('.npmrc', [ + '//user:password@registry.example.com/${PNPM_TEST_TOKEN}/:_authToken=token', + '//user:password@attacker.example/:_authToken=${PNPM_TEST_TOKEN}', + '', + ].join('\n'), 'utf8') + + const { warnings } = await getConfig({ + cliOptions: {}, + env: { ...env, PNPM_TEST_TOKEN: 'secret' }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(warnings).toEqual(expect.arrayContaining([ + expect.stringContaining('Ignored project-level request destination "//registry.example.com/${PNPM_TEST_TOKEN}/:_authToken"'), + expect.stringContaining('Ignored project-level auth setting "//attacker.example/:_authToken"'), + ])) + expect(warnings.join('\n')).not.toContain('user:password') +}) + test('project .npmrc does not expand env variables in auth values', async () => { prepare()