Skip to content
5 changes: 1 addition & 4 deletions src/install/PackageManager/PackageManagerOptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -696,10 +696,7 @@ impl Options {
self.scope.auth = Box::default();
self.scope.user = Box::default();
}
let href: Box<[u8]> = cli.registry.into();
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(cli.registry.into());
}

if let Some(cache_dir) = cli.cache_dir {
Expand Down
29 changes: 23 additions & 6 deletions src/install/npm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,24 @@ pub mod registry {
bun_semver::semver_string::Builder::string_hash(str)
}

/// Stores the configured registry URL and recomputes `url_hash`.
///
/// Manifest URLs are built from this href by the WHATWG parser
/// (`bun_url::join`), which rewrites every spelling it accepts
/// (`https:host/path`, `..` segments, unencoded characters, ...), so
/// the href is stored in that rewritten form: the `URL::parse` views
/// the same-origin checks compare against, the tarball URLs built by
/// concatenation and `url_hash` all have to agree with the requests.
/// A string the parser rejects is stored as written, so the join
/// fails on it and reports it unchanged. Credentials written into the
/// URL (`/:_authToken=...`) must be split off before this call: the
/// parser would percent-encode them.
Comment thread
robobun marked this conversation as resolved.
Outdated
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;
Expand Down Expand Up @@ -508,16 +526,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)
}
}

Expand Down
18 changes: 6 additions & 12 deletions src/install_jsc/npm_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ impl ManifestBindings {
#[bun_jsc::host_fn]
fn js_parse_manifest(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
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 _;
Expand Down Expand Up @@ -119,17 +119,11 @@ fn js_parse_manifest(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSV
}
};

// The `Scope.url` field
// is `OwnedURL`, which stores only the href buffer and re-derives components
// via `URL::parse` on demand. `load_by_file`/`read_all` only consult
// `scope.url_hash` and `scope.url.href().len()`, so copying the raw href is
// sufficient and drops the unsafe lifetime-extension hack the earlier draft
// needed.
let scope = npm::registry::Scope {
url_hash: npm::registry::Scope::hash(strings::without_trailing_slash(registry.slice())),
url: bun_url::OwnedURL::from_href(Box::from(registry.slice())),
..Default::default()
};
// `load_by_file` only consults `scope.url_hash` and `scope.url.href()`,
// which `set_url` derives the same way `bun install` did when it wrote
// the manifest.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut scope = npm::registry::Scope::default();
scope.set_url(Box::from(registry.slice()));

let maybe_package_manifest =
match npm::package_manifest::Serializer::load_by_file(&scope, &manifest_file) {
Expand Down
123 changes: 123 additions & 0 deletions test/cli/install/bun-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9036,6 +9036,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.
Expand Down Expand Up @@ -9125,6 +9128,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("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, string> | 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");
});
Comment thread
robobun marked this conversation as resolved.

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 () => {
Expand Down