Skip to content

install: redact the registry URL in the manifest/tarball URL errors and keep namedRegistries credentials out of bun.lock #38997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+194 −18
Open

install: redact the registry URL in the manifest/tarball URL errors and keep namedRegistries credentials out of bun.lock #38997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions src/install/NetworkTask.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<u8> {
let mut out: Vec<u8> = Vec::new();
let _ = write!(out, "{}", redacted_npm_url(url));
out
}

impl NetworkTask {
pub(crate) fn for_manifest(
&mut self,
Expand Down Expand Up @@ -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),
),
);
Expand All @@ -487,40 +496,41 @@ impl NetworkTask {
bun_ast::Loc::EMPTY,
format_args!(
"Failed to join registry {} and package {} URLs",
quote(scope.url.href()),
quote(&redacted_registry),
quote(name),
),
);
}
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 {
log.add_warning_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)
),
);
}
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();
Expand All @@ -532,15 +542,17 @@ 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,
bun_ast::Loc::EMPTY,
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 {
Expand All @@ -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),
),
);
}
Expand Down Expand Up @@ -770,14 +782,15 @@ 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(
None,
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()),
),
);
Expand Down
6 changes: 4 additions & 2 deletions src/install/pnpm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
}
}
}
Expand Down Expand Up @@ -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
Expand Down
59 changes: 59 additions & 0 deletions test/cli/install/migration/pnpm-lock-v9.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down
102 changes: 102 additions & 0 deletions test/cli/install/redacted-config-logs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, Record<string, string>>) => JSON.stringify({ name: "app", ...deps });

const cases: {
title: string;
registry: string;
files: Record<string, string>;
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 = [
{
Expand Down
Loading