diff --git a/src/install/PackageManager/PackageManagerOptions.rs b/src/install/PackageManager/PackageManagerOptions.rs index 6ff59e745914..10504ff5a1cd 100644 --- a/src/install/PackageManager/PackageManagerOptions.rs +++ b/src/install/PackageManager/PackageManagerOptions.rs @@ -644,10 +644,7 @@ impl Options { self.scope.auth = Box::default(); self.scope.user = Box::default(); } - let href = api_registry.url; - self.scope.url_hash = - Npm::registry::Scope::hash(bun_core::without_trailing_slash(&href)); - self.scope.url = bun_url::OwnedURL::from_href(href); + self.scope.set_url(api_registry.url); } } } diff --git a/src/install/audit_fix.rs b/src/install/audit_fix.rs index 0e72077642b7..505bd97a225c 100644 --- a/src/install/audit_fix.rs +++ b/src/install/audit_fix.rs @@ -87,6 +87,7 @@ pub struct UnmatchedAdvisory { } pub struct UnauditedRegistry { + /// The registry's href without URL credentials or a trailing slash. pub registry: Box<[u8]>, pub packages: Vec>, /// Status code or error name; empty when unknown. @@ -297,13 +298,13 @@ pub fn print_unaudited(groups: &[UnauditedRegistry]) { if group.reason.is_empty() { bun_core::warn!( "{} did not answer the audit request; skipped {}", - BStr::new(&group.registry), + bun_core::fmt::redacted_npm_url(&group.registry), BStr::new(&packages) ); } else { bun_core::warn!( "{} did not answer the audit request ({}); skipped {}", - BStr::new(&group.registry), + bun_core::fmt::redacted_npm_url(&group.registry), BStr::new(&group.reason), BStr::new(&packages) ); diff --git a/src/install/audit_fix/json.rs b/src/install/audit_fix/json.rs index 5c05b1bdce79..7e0a8dae8165 100644 --- a/src/install/audit_fix/json.rs +++ b/src/install/audit_fix/json.rs @@ -101,8 +101,14 @@ pub(super) fn write(plan: &FixPlan, outcome: Option<&FixOutcome>, dry_run: bool) out.extend_from_slice(b"],\"unaudited\":["); for (i, group) in plan.unaudited.iter().enumerate() { comma(&mut out, i); + let mut registry: Vec = Vec::new(); + let _ = write!( + registry, + "{}", + bun_core::fmt::redacted_npm_url(&group.registry) + ); out.extend_from_slice(b"{\"registry\":"); - s(&mut out, &group.registry); + s(&mut out, ®istry); out.extend_from_slice(b",\"packages\":["); for (j, package) in group.packages.iter().enumerate() { comma(&mut out, j); diff --git a/src/install/npm.rs b/src/install/npm.rs index 3cc06b469ab1..1d1482ab76e2 100644 --- a/src/install/npm.rs +++ b/src/install/npm.rs @@ -322,6 +322,13 @@ pub mod registry { bun_semver::semver_string::Builder::string_hash(str) } + /// Stores the WHATWG serialization (the base `bun_url::join` resolves against) so same-origin checks, concatenated tarball URLs and `url_hash` agree with the requests; credentials must already be split off. + pub fn set_url(&mut self, href: Box<[u8]>) { + self.url = URL::from_string(&bun_core::String::borrow_utf8(&href)) + .unwrap_or_else(|_| OwnedURL::from_href(href)); + self.url_hash = Self::hash(strings::without_trailing_slash(self.url.href())); + } + pub(crate) fn get_name(name: &[u8]) -> &[u8] { if name.is_empty() || name[0] != b'@' { return name; @@ -508,16 +515,15 @@ pub mod registry { registry_url }; - let url_hash = Self::hash(strings::without_trailing_slash(&final_href)); - - Ok(Scope { + let mut scope = Scope { name: name.into(), - url: OwnedURL::from_href(final_href), - url_hash, token: registry.token, auth, user, - }) + ..Default::default() + }; + scope.set_url(final_href); + Ok(scope) } } diff --git a/src/install_jsc/npm_jsc.rs b/src/install_jsc/npm_jsc.rs index 23fbca147c37..4fb0527626b1 100644 --- a/src/install_jsc/npm_jsc.rs +++ b/src/install_jsc/npm_jsc.rs @@ -82,7 +82,7 @@ impl ManifestBindings { #[bun_jsc::host_fn] fn js_parse_manifest(global: &JSGlobalObject, frame: &CallFrame) -> JsResult { use bstr::BStr; - use bun_core::{String as BunString, strings}; + use bun_core::String as BunString; use bun_install::npm; use bun_jsc::JsError; use std::io::Write as _; @@ -119,17 +119,8 @@ fn js_parse_manifest(global: &JSGlobalObject, frame: &CallFrame) -> JsResult &[u8] { fn report_non_json_response(registry: &[u8]) { Output::err_generic( - "{s} returned a non-JSON audit response", - (BStr::new(registry),), + "{f} returned a non-JSON audit response", + (bun_core::fmt::redacted_npm_url(registry),), ); } @@ -419,8 +419,9 @@ impl core::fmt::Display for SkipReason { fn unaudited(request: &AuditRequest, reason: &SkipReason) -> audit_fix::UnauditedRegistry { let mut reason_text: Vec = Vec::new(); write!(&mut reason_text, "{reason}").expect("unreachable"); + let registry = URL::parse(&request.registry.href).href_without_auth(); audit_fix::UnauditedRegistry { - registry: request.registry.href.clone(), + registry: Box::from(strings::without_trailing_slash(®istry)), packages: request .packages .iter() @@ -783,7 +784,7 @@ fn send_audit_request( reason => { bun_core::pretty_errorln!( "error: POST {} - {}", - BStr::new(&url_str), + bun_core::fmt::redacted_npm_url(&url_str), reason ); } diff --git a/test/cli/install/bun-audit.test.ts b/test/cli/install/bun-audit.test.ts index dc1378d7e8cf..b0197c013fb3 100644 --- a/test/cli/install/bun-audit.test.ts +++ b/test/cli/install/bun-audit.test.ts @@ -752,6 +752,144 @@ describe("`bun audit`", () => { }); }); +// Every audit line that names a registry prints it through the same redaction as the install error lines, which +// replaces an npm token or UUID anywhere in the URL with `***`; the skipped-registry record (the warning and the +// `unaudited` entries of `audit fix --json`) additionally leaves out credentials written into the URL itself. Most +// of these tests put the token in the registry path because that reaches the audit command from every config +// source, while `user:password@` is split out of the URL by some of them (`.npmrc`, bunfig registry strings) and +// kept by others (the bunfig object form used below, the env vars today). +describe("`bun audit` with a secret in the registry URL", () => { + const SECRET = "npm_" + "secret".padEnd(36, "0"); + const BULK_PATH = "/-/npm/v1/security/advisories/bulk"; + const NON_JSON = (registry: string) => `error: ${registry} returned a non-JSON audit response`; + + // `url` is what the project is configured with, `printed` is how every audit line must render it. + function secretRegistry(registry: Registry) { + return { url: `${registry.url}${SECRET}/`, printed: `${registry.url}***` }; + } + + // The bulk endpoint lives under the token path, so the registry answers every request the same way. + function registryAnswering(body: string, init?: ResponseInit) { + return Bun.serve({ port: 0, fetch: () => new Response(body, init) }); + } + + // `bun audit` only reads bun.lock, so the project never needs an install. + function project(dependencies: Record, extraFiles: Record = {}) { + return tempDir("audit-registry-secret-", { + "package.json": JSON.stringify({ name: "app", dependencies }), + "bun.lock": JSON.stringify({ + lockfileVersion: 1, + workspaces: { "": { name: "app", dependencies } }, + packages: Object.fromEntries( + Object.entries(dependencies).map(([name, version]) => [name, [`${name}@${version}`, "", {}, ""]]), + ), + }), + ...extraFiles, + }); + } + + async function auditAgainst(dir: string, defaultRegistry: string, ...args: string[]) { + await using proc = spawn({ + cmd: [bunExe(), "audit", ...args], + cwd: String(dir), + env: { ...bunEnv, NPM_CONFIG_REGISTRY: defaultRegistry }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + test.concurrent("the failed POST line masks the secret", async () => { + await using registry = registryAnswering("not found", { status: 404 }); + const { url, printed } = secretRegistry(registry); + using dir = project({ "no-deps": "1.0.0" }); + + const { stdout, stderr, exitCode } = await auditAgainst(dir, url); + expect(normalizeBunSnapshot(stderr)).toBe(`error: POST ${printed}${BULK_PATH} - 404`); + expect(normalizeBunSnapshot(stdout)).toBe("bun audit ()"); + expect(exitCode).toBe(1); + }); + + test.concurrent("the non-JSON response line masks the secret", async () => { + await using registry = registryAnswering("sign in"); + const { url, printed } = secretRegistry(registry); + using dir = project({ "no-deps": "1.0.0" }); + + const { stdout, stderr, exitCode } = await auditAgainst(dir, url); + expect(normalizeBunSnapshot(stderr)).toBe(NON_JSON(printed)); + expect(normalizeBunSnapshot(stdout)).toBe("bun audit ()"); + expect(exitCode).toBe(1); + }); + + // A body starting with `{` gets past the response check and is rejected when it is parsed instead; the report, + // --json and fix code paths each report that themselves. + test.concurrent("the unparsable response line masks the secret in every mode", async () => { + const body = "{ not json"; + await using registry = registryAnswering(body); + const { url, printed } = secretRegistry(registry); + using dir = project({ "no-deps": "1.0.0" }); + + const report = await auditAgainst(dir, url); + expect(normalizeBunSnapshot(report.stderr)).toBe(NON_JSON(printed)); + expect(normalizeBunSnapshot(report.stdout)).toBe("bun audit ()"); + expect(report.exitCode).toBe(1); + + const json = await auditAgainst(dir, url, "--json"); + expect(normalizeBunSnapshot(json.stderr)).toBe(NON_JSON(printed)); + expect(json.stdout).toBe(body + "\n"); + expect(json.exitCode).toBe(1); + + const fix = await auditAgainst(dir, url, "fix"); + expect(normalizeBunSnapshot(fix.stderr)).toBe(NON_JSON(printed)); + expect(normalizeBunSnapshot(fix.stdout)).toBe("bun audit fix ()"); + expect(fix.exitCode).toBe(1); + }); + + // `bun audit` and `bun audit fix --json` against a project whose only package comes from a scoped registry that + // answers 404, so both commands report that registry as skipped; `printed` is how it must be named. + async function expectSkippedRegistry(dir: string, printed: string) { + const skipped = skippedWarning(printed, "404", "@foo/bar"); + + const report = await auditAgainst(dir, registryHref(server)); + expect(normalizeBunSnapshot(report.stderr)).toBe(skipped); + expect(normalizeBunSnapshot(report.stdout)).toBe(AUDIT_HEADER + noVulnerabilities(0, "1 skipped")); + expect(report.exitCode).toBe(0); + + const fix = await auditAgainst(dir, registryHref(server), "fix", "--json"); + expect(normalizeBunSnapshot(fix.stderr)).toBe(skipped); + expect(JSON.parse(fix.stdout)).toStrictEqual({ + dryRun: false, + fixed: 0, + remaining: 0, + fixes: [], + blocked: [], + unfixable: [], + manifestUnavailable: [], + unmatched: [], + unaudited: [{ registry: printed, packages: ["@foo/bar"], reason: "404" }], + vulnerableAfterInstall: [], + }); + expect(fix.exitCode).toBe(0); + } + + test.concurrent("the skipped registry warning and the --json unaudited entry mask the secret", async () => { + await using scoped = registryAnswering("not found", { status: 404 }); + const { url, printed } = secretRegistry(scoped); + using dir = project({ "@foo/bar": "1.0.0" }, { ".npmrc": `@foo:registry=${url}\n` }); + + await expectSkippedRegistry(dir, printed); + }); + + test.concurrent("the skipped registry warning and the --json unaudited entry leave out URL credentials", async () => { + await using scoped = registryAnswering("not found", { status: 404 }); + const url = `${scoped.url.protocol}//alice:s3cret@${scoped.url.host}/`; + using dir = project({ "@foo/bar": "1.0.0" }, { "bunfig.toml": `[install.scopes]\nfoo = { url = "${url}" }\n` }); + + await expectSkippedRegistry(dir, registryHref(scoped)); + }); +}); + describe("`bun audit --prod`", () => { // pnpm#13605: an optional peer that only a devDependency brought in is not a production dependency. test.concurrent("bun audit --prod skips a dev-only optional peer of a production package", async () => { diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 279851e5dd27..1ff3c0107a89 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9194,6 +9194,9 @@ describe.concurrent("bun-install", () => { } else if (fails) { expect(err).toContain(`Failed to join registry "${regURL}" and package "notapackage" URLs`); } else { + // "failed to resolve" is also printed when Bun refuses the manifest + // URL it built, so make sure the registry URL itself was accepted. + expect(err).not.toContain("is not on registry"); expect(err).toContain("error: notapackage@0.0.2 failed to resolve"); } // fails either way, since notapackage is, well, not a real package. @@ -9283,6 +9286,126 @@ describe.concurrent("bun-install", () => { expect(await exited).toBe(0); }); + + // The manifest URL is built from the registry URL by the WHATWG parser, + // which accepts every spelling below and rewrites it to the canonical form. + // Everything else derived from the configured registry (the "is not on + // registry" check on that manifest URL, the same-origin check that decides + // whether a tarball request gets the Authorization header, the cache folder + // name) has to read the same canonical form, otherwise the install fails + // before or after the first request depending on the spelling. + describe.concurrent("spellings the WHATWG parser rewrites", () => { + const token = "registry-spelling-token"; + const tgz = join(import.meta.dir, "registry", "packages", "no-deps", "no-deps-1.0.0.tgz"); + + // Serves `no-deps@1.0.0` under whatever directory the manifest is + // requested from and records the path and Authorization header of every + // request. `configure` returns either extra project files or extra + // `bun install` arguments for the registry at `origin`. + async function installNoDeps(configure: (origin: string) => Record | string[]) { + const requests: { path: string; authorization: string | null }[] = []; + await using registry = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch(req, server) { + const { pathname } = new URL(req.url); + requests.push({ path: pathname, authorization: req.headers.get("authorization") }); + if (pathname.endsWith(".tgz")) { + return new Response(file(tgz)); + } + return Response.json({ + name: "no-deps", + "dist-tags": { latest: "1.0.0" }, + versions: { + "1.0.0": { + name: "no-deps", + version: "1.0.0", + dist: { tarball: `http://127.0.0.1:${server.port}${pathname}/-/no-deps-1.0.0.tgz` }, + }, + }, + }); + }, + }); + + const origin = `http://127.0.0.1:${registry.port}`; + const config = configure(origin); + const [files, args] = Array.isArray(config) ? [{}, config] : [config, []]; + using dir = tempDir("registry-url-spelling", { + "package.json": JSON.stringify({ name: "app", version: "1.0.0", dependencies: { "no-deps": "1.0.0" } }), + ...files, + }); + await using proc = spawn({ + cmd: [bunExe(), "install", ...args], + cwd: String(dir), + env: { ...env, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const cache = (await exists(join(String(dir), ".bun-cache"))) + ? await readdirSorted(join(String(dir), ".bun-cache")) + : []; + return { origin, cache, result: { requests, stdout, stderr, exitCode } }; + } + + // Both requests land in `directory` (the canonical form of the configured + // path) and both carry the same Authorization header. + function installedFrom(directory: string, authorization: string | null) { + return { + requests: [ + { path: `${directory}no-deps`, authorization }, + { path: `${directory}no-deps/-/no-deps-1.0.0.tgz`, authorization }, + ], + stdout: expect.stringContaining("1 package installed"), + stderr: expect.stringContaining("Saved lockfile"), + exitCode: 0, + }; + } + + const singleColon = (origin: string) => origin.replace("http://", "http:"); + + it.each([ + ["the scheme followed by a single colon", (origin: string) => `${singleColon(origin)}/npm/`, "/npm/"], + ["backslashes", (origin: string) => `${origin.replace("http://", "http:\\\\")}\\npm\\`, "/npm/"], + ["a dot segment", (origin: string) => `${origin}/npm/unused/../`, "/npm/"], + ["surrounding whitespace", (origin: string) => ` ${origin}/npm/ `, "/npm/"], + ["an unencoded space in the path", (origin: string) => `${origin}/npm dir/`, "/npm%20dir/"], + // Accepted before as well, but the tarball's same-origin check compared + // the scheme case-sensitively and withheld the token from the tarball. + ["an upper-case scheme", (origin: string) => `${origin.replace("http://", "HTTP://")}/npm/`, "/npm/"], + ])("bunfig.toml registry with %s", async (_, spell, directory) => { + const { result, cache } = await installNoDeps(origin => ({ + "bunfig.toml": Bun.TOML.stringify({ install: { registry: { url: spell(origin), token } } }), + })); + expect(result).toEqual(installedFrom(directory, `Bearer ${token}`)); + // The cache folder is named after the hostname read from the stored URL. + expect(cache).toContain("no-deps@1.0.0@@127.0.0.1@@@1"); + }); + + it(".npmrc registry= with the scheme followed by a single colon", async () => { + const { result } = await installNoDeps(origin => ({ ".npmrc": `registry=${singleColon(origin)}/npm/\n` })); + expect(result).toEqual(installedFrom("/npm/", null)); + }); + + it("--registry with a dot segment", async () => { + const { result } = await installNoDeps(origin => ["--registry", `${origin}/npm/unused/../`]); + expect(result).toEqual(installedFrom("/npm/", null)); + }); + + it("still refuses a name that joins to a URL outside the registry directory", async () => { + const { result, origin } = await installNoDeps(origin => ({ + "bunfig.toml": Bun.TOML.stringify({ install: { registry: { url: `${singleColon(origin)}/npm/`, token } } }), + "package.json": JSON.stringify({ name: "app", version: "1.0.0", dependencies: { "..": "1.0.0" } }), + })); + expect(result).toEqual({ + requests: [], + stdout: expect.stringContaining("bun install v1."), + // The error quotes the registry in the form the check compared against. + stderr: expect.stringContaining(`manifest URL "${origin}/" is not on registry "${origin}/npm/"`), + exitCode: 1, + }); + }); + }); }); it("should ensure read permissions of all extracted files", async () => {