From 6ac41d5be7032885551169ca3c9722b44b804857 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:40:35 +0000 Subject: [PATCH] install: redact registry URLs in the manifest/tarball URL errors and keep namedRegistries credentials out of bun.lock The messages NetworkTask prints when it cannot build a request URL out of the configured registry URL ("Failed to join registry ...", "Invalid package name ...: manifest URL ... is not on registry ...", "Registry URL must be http:// or https://", "Expected tarball URL to start with ...") formatted the registry href and the URL built from it verbatim, so a token written into the registry URL as a path segment ended up on stderr. Format them through redacted_npm_url like the other registry URL print sites; the URL is redacted into a buffer first and the buffer is passed to quote(), which escapes but does not redact. The pnpm migration's "fetching pnpm registry ... packages from " warning printed the namedRegistries URL from pnpm-workspace.yaml the same way. That URL was also stored as written, so user:password@ in it was copied into every tarball URL recorded in bun.lock (bun never sends URL credentials) and a named registry that was the configured registry spelled with credentials was not recognized as such. Read the value through NpmRegistry::from_url, as every other registry URL source does, so the credentials are split off and only the URL is kept; the warning is redacted as well for a token in the path. --- src/install/NetworkTask.rs | 45 +++++--- src/install/pnpm.rs | 6 +- .../install/migration/pnpm-lock-v9.test.ts | 59 ++++++++++ test/cli/install/redacted-config-logs.test.ts | 102 ++++++++++++++++++ 4 files changed, 194 insertions(+), 18 deletions(-) diff --git a/src/install/NetworkTask.rs b/src/install/NetworkTask.rs index 615177ac1646..8c2cef98922f 100644 --- a/src/install/NetworkTask.rs +++ b/src/install/NetworkTask.rs @@ -4,14 +4,15 @@ use core::sync::atomic::Ordering; use crate::bun_fs::{FileSystem, FilenameStore}; use bun_collections::HashMap; -use bun_core::{self, fmt::quote}; -use bun_core::{MutableString, strings}; +use bun_core::fmt::{quote, redacted_npm_url}; +use bun_core::{self, MutableString, strings}; use bun_http::{ self as http, AsyncHTTP, HTTPClientResult, HTTPClientResultCallback, HTTPVerboseLevel, HeaderBuilder, async_http::Options as AsyncHTTPOptions, }; use bun_threading::thread_pool::Batch; use bun_url::URL; +use std::io::Write as _; use crate::extract_tarball; use crate::npm::{self as npm, PackageManifest}; @@ -435,6 +436,13 @@ impl bun_core::output::ErrName for ForManifestError { } } +/// Redacted before `quote()`: the password scan only recognizes an unquoted URL. +fn redacted_url(url: &[u8]) -> Vec { + let mut out: Vec = Vec::new(); + let _ = write!(out, "{}", redacted_npm_url(url)); + out +} + impl NetworkTask { pub(crate) fn for_manifest( &mut self, @@ -471,13 +479,14 @@ impl NetworkTask { )); if tmp.tag() == bun_core::Tag::Dead { + let redacted_registry = redacted_url(scope.url.href()); if !is_optional { log.add_error_fmt( None, bun_ast::Loc::EMPTY, format_args!( "Failed to join registry {} and package {} URLs", - quote(scope.url.href()), + quote(&redacted_registry), quote(name), ), ); @@ -487,7 +496,7 @@ impl NetworkTask { bun_ast::Loc::EMPTY, format_args!( "Failed to join registry {} and package {} URLs", - quote(scope.url.href()), + quote(&redacted_registry), quote(name), ), ); @@ -495,14 +504,18 @@ impl NetworkTask { return Err(ForManifestError::InvalidURL); } + // This actually duplicates the string! So we defer deref the WTF managed one above. + let url_bytes = tmp.to_owned_slice().into_boxed_slice(); + if !(tmp.has_prefix_comptime(b"https://") || tmp.has_prefix_comptime(b"http://")) { + let redacted_manifest_url = redacted_url(&url_bytes); if !is_optional { log.add_error_fmt( None, bun_ast::Loc::EMPTY, format_args!( - "Registry URL must be http:// or https://\nReceived: \"{}\"", - *tmp + "Registry URL must be http:// or https://\nReceived: {}", + quote(&redacted_manifest_url) ), ); } else { @@ -510,17 +523,14 @@ impl NetworkTask { None, bun_ast::Loc::EMPTY, format_args!( - "Registry URL must be http:// or https://\nReceived: \"{}\"", - *tmp + "Registry URL must be http:// or https://\nReceived: {}", + quote(&redacted_manifest_url) ), ); } return Err(ForManifestError::InvalidURL); } - // This actually duplicates the string! So we defer deref the WTF managed one above. - let url_bytes = tmp.to_owned_slice().into_boxed_slice(); - { let joined = URL::parse(&url_bytes); let registry = scope.url.url(); @@ -532,6 +542,8 @@ impl NetworkTask { || joined.get_port_auto() != registry.get_port_auto() || !joined.pathname.starts_with(registry_dir) { + let redacted_manifest_url = redacted_url(&url_bytes); + let redacted_registry = redacted_url(scope.url.href()); if !is_optional { log.add_error_fmt( None, @@ -539,8 +551,8 @@ impl NetworkTask { format_args!( "Invalid package name {}: manifest URL {} is not on registry {}", quote(name), - quote(&url_bytes), - quote(scope.url.href()), + quote(&redacted_manifest_url), + quote(&redacted_registry), ), ); } else { @@ -550,8 +562,8 @@ impl NetworkTask { format_args!( "Invalid package name {}: manifest URL {} is not on registry {}", quote(name), - quote(&url_bytes), - quote(scope.url.href()), + quote(&redacted_manifest_url), + quote(&redacted_registry), ), ); } @@ -770,6 +782,7 @@ impl NetworkTask { }; if !(self.url_buf.starts_with(b"https://") || self.url_buf.starts_with(b"http://")) { + let redacted_tarball_url = redacted_url(&self.url_buf); // SAFETY: `pm.log` is the long-lived `*mut Log` the package // manager was constructed with. pm.log_mut().add_error_fmt( @@ -777,7 +790,7 @@ impl NetworkTask { bun_ast::Loc::EMPTY, format_args!( "Expected tarball URL to start with https:// or http://, got {} while fetching package {}", - quote(&self.url_buf), + quote(&redacted_tarball_url), quote(tarball.name.slice()), ), ); diff --git a/src/install/pnpm.rs b/src/install/pnpm.rs index e14532accf02..fd3e8ac2f597 100644 --- a/src/install/pnpm.rs +++ b/src/install/pnpm.rs @@ -242,7 +242,9 @@ fn read_named_registries( let key = prop.key.as_ref().expect("infallible: prop has key"); let value = prop.value.as_ref().expect("infallible: prop has value"); if let (Some(name_str), Some(url_str)) = (as_string(key), as_string(value)) { - registries.put(name_str, Box::from(url_str))?; + // Without its credentials: this URL is recorded in bun.lock as the tarball base. + let url = crate::bun_schema::api::NpmRegistry::from_url(url_str).url; + registries.put(name_str, url)?; } } } @@ -1310,7 +1312,7 @@ pub(crate) fn migrate_pnpm_lockfile<'a>( bun_core::warn!( "fetching pnpm registry \"{}\" packages from {}; add it to bunfig.toml or .npmrc if it needs authentication", bstr::BStr::new(registry_name), - bstr::BStr::new(url) + bun_core::fmt::redacted_npm_url(url) ); } &**url diff --git a/test/cli/install/migration/pnpm-lock-v9.test.ts b/test/cli/install/migration/pnpm-lock-v9.test.ts index b943197ac326..ce4264237ddb 100644 --- a/test/cli/install/migration/pnpm-lock-v9.test.ts +++ b/test/cli/install/migration/pnpm-lock-v9.test.ts @@ -387,6 +387,65 @@ snapshots: `); }); + test("credentials in a namedRegistries URL stay out of bun.lock and a token in its path is masked in the warning", async () => { + const password = "s3cret"; + const token = "npm_" + "a1b2c3d4e5f6g7h8i9j0k1l2m3n4o5p6q7r8"; + const { packageDir } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "package.json": JSON.stringify({ name: "named-registry-secrets", dependencies: { "no-deps": "^1.0.0" } }), + "pnpm-workspace.yaml": `namedRegistries:\n work: http://carol:${password}@127.0.0.1:1/${token}/\n`, + "pnpm-lock.yaml": registryQualifiedNoDepsLockfile("work"), + }, + }); + + const { stderr, exitCode } = await migrate(packageDir); + + expect(stderr).toContain( + 'warn: fetching pnpm registry "work" packages from http://127.0.0.1:1/***/; add it to bunfig.toml or .npmrc if it needs authentication', + ); + expect(stderr).not.toContain(password); + expect(stderr).not.toContain(token); + expect(stderr).toContain("migrated lockfile from pnpm-lock.yaml"); + expect(exitCode).toBe(0); + + // The token is part of the tarball location; the credentials are not. + const bunLock = await bunLockOf(packageDir); + expect(bunLock).toContain( + `["no-deps@1.0.1", "http://127.0.0.1:1/${token}/no-deps/-/no-deps-1.0.1.tgz", {}, "${NO_DEPS_1_0_1_INTEGRITY}"]`, + ); + expect(bunLock).not.toContain(password); + }); + + test("namedRegistries entry naming the configured registry with credentials in the URL needs no warning", async () => { + const { packageDir } = await verdaccio.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "package.json": JSON.stringify({ name: "named-registry-same-creds", dependencies: { "no-deps": "^1.0.0" } }), + "pnpm-workspace.yaml": `namedRegistries:\n work: ${verdaccio.registryUrl().replace("http://", "http://carol:s3cret@")}\n`, + "pnpm-lock.yaml": registryQualifiedNoDepsLockfile("work"), + }, + }); + + const { stderr, exitCode } = await migrate(packageDir); + + expect(stderr).not.toContain("pnpm registry"); + expect(stderr).not.toContain("warn:"); + expect(stderr).toContain("migrated lockfile from pnpm-lock.yaml"); + expect(exitCode).toBe(0); + + const bunLock = await bunLockOf(packageDir); + expect(bunLock).toContain(`"no-deps@1.0.1"`); + expect(bunLock).not.toContain("s3cret"); + expect(bunLock).not.toContain("work:"); + + const install = await run(packageDir, "install", "--frozen-lockfile"); + + expect(install.stderr).not.toContain("error:"); + expect(install.exitCode).toBe(0); + expect(nodeModulesPackages(packageDir)).toMatchInlineSnapshot(`"node_modules/no-deps/no-deps@1.0.1"`); + }); + test("two packages from one unknown registry warn once", async () => { const { packageDir } = await verdaccio.createTestDir({ bunfigOpts: { linker: "hoisted" }, diff --git a/test/cli/install/redacted-config-logs.test.ts b/test/cli/install/redacted-config-logs.test.ts index 6c49d43123b4..3edd3360c58d 100644 --- a/test/cli/install/redacted-config-logs.test.ts +++ b/test/cli/install/redacted-config-logs.test.ts @@ -141,6 +141,108 @@ test("bunfig password value is masked in config error output", async () => { expect(coloredExit).toBe(1); }); +// The config loaders split user:password@ off a registry URL, but a token +// written as a path segment stays part of it, and these messages echo that +// URL (or the manifest / tarball URL built from it) when no request can be +// made out of it. None of the cases below sends a request. +describe.concurrent("bun install masks the configured registry URL in the messages that echo it", () => { + const token = "npm_" + "a1b2c3d4e5f6g7h8i9j0k1l2m3n4o5p6q7r8"; + const packageJson = (deps: Record>) => JSON.stringify({ name: "app", ...deps }); + + const cases: { + title: string; + registry: string; + files: Record; + expected: string; + exitCode: number; + }[] = [ + { + title: "registry URL that is not a valid base URL (dependency)", + registry: `http://ex ample.org/${token}/`, + files: { "package.json": packageJson({ dependencies: { notapackage: "1.0.0" } }) }, + expected: `error: Failed to join registry "http://ex ample.org/***/" and package "notapackage" URLs\n`, + exitCode: 1, + }, + { + title: "registry URL that is not a valid base URL (optional dependency)", + registry: `http://ex ample.org/${token}/`, + files: { "package.json": packageJson({ optionalDependencies: { notapackage: "1.0.0" } }) }, + expected: `warn: Failed to join registry "http://ex ample.org/***/" and package "notapackage" URLs\n`, + exitCode: 0, + }, + // The alias makes ".." the name the manifest is requested for. It joins to + // the registry's parent directory, so the manifest URL no longer contains + // the token segment; the registry URL printed next to it does. + { + title: "manifest URL outside the registry directory (dependency)", + registry: `http://127.0.0.1:1/${token}/`, + files: { "package.json": packageJson({ dependencies: { innocent: "npm:..@1.0.0" } }) }, + expected: `error: Invalid package name "..": manifest URL "http://127.0.0.1:1/" is not on registry "http://127.0.0.1:1/***/"\n`, + exitCode: 1, + }, + { + title: "manifest URL outside the registry directory (optional dependency)", + registry: `http://127.0.0.1:1/${token}/`, + files: { "package.json": packageJson({ optionalDependencies: { innocent: "npm:..@1.0.0" } }) }, + expected: `warn: Invalid package name "..": manifest URL "http://127.0.0.1:1/" is not on registry "http://127.0.0.1:1/***/"\n`, + exitCode: 0, + }, + { + title: "registry URL with a non-http scheme (dependency)", + registry: `htp://127.0.0.1:1/${token}/`, + files: { "package.json": packageJson({ dependencies: { notapackage: "1.0.0" } }) }, + expected: `error: Registry URL must be http:// or https://\nReceived: "htp://127.0.0.1:1/***/notapackage"\n`, + exitCode: 1, + }, + { + title: "registry URL with a non-http scheme (optional dependency)", + registry: `htp://127.0.0.1:1/${token}/`, + files: { "package.json": packageJson({ optionalDependencies: { notapackage: "1.0.0" } }) }, + expected: `warn: Registry URL must be http:// or https://\nReceived: "htp://127.0.0.1:1/***/notapackage"\n`, + exitCode: 0, + }, + { + title: "tarball URL built from a registry URL with a non-http scheme", + registry: `htp://127.0.0.1:1/${token}/`, + files: { + "package.json": packageJson({ dependencies: { pkg: "1.0.0" } }), + // An empty URL in bun.lock stands for the configured registry's + // default tarball location, so no manifest is fetched first. + "bun.lock": JSON.stringify({ + lockfileVersion: 1, + workspaces: { "": { name: "app", dependencies: { pkg: "1.0.0" } } }, + packages: { pkg: ["pkg@1.0.0", "", {}, ""] }, + }), + }, + expected: `error: Expected tarball URL to start with https:// or http://, got "htp://127.0.0.1:1/***/pkg/-/pkg-1.0.0.tgz" while fetching package "pkg"\n`, + exitCode: 1, + }, + ]; + + for (const { title, registry, files, expected, exitCode } of cases) { + test(title, async () => { + using dir = tempDir("redacted-registry-token", { + ...files, + "bunfig.toml": `[install]\nregistry = ${JSON.stringify(registry)}\n`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "install"], + cwd: String(dir), + env: { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }, + stdout: "pipe", + stderr: "pipe", + }); + + const [out, err, exited] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(err).toContain(expected); + expect(err).not.toContain(token); + expect(out).not.toContain(token); + expect(exited).toBe(exitCode); + }); + } +}); + describe.concurrent("redact", async () => { const tests = [ {