From 84b84b5d665742375d8a98d1ea23f5d5622c8d4f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:06:47 +0000 Subject: [PATCH 1/5] http: fix use-after-free of the redirect URL on a retried request After a redirect, handle_response_metadata repoints client.url at a self-borrow into client.redirect, a Vec the HTTP-thread clone owns and frees in the final-callback teardown (on_async_http_callback_raw). The bitwise copy-back into the JS-thread AsyncHTTP (NetworkTask::notify) kept those dangling slices, so when bun install retried the request after a retryable failure (5xx or connection reset) on the redirect target, the retry connected through freed memory. ASAN: heap-use-after-free READ on the HTTP-client thread in URL::get_port <- HTTPThread::connect. In release builds the retries never reached the server at all and the install failed. Restore the caller-owned pre-redirect URL (and reset connected_url, which is derived from it) in the same teardown that frees the redirect buffer. This also makes a retry restart from the original URL rather than the last redirect hop, which is what the install retry intends. --- src/http/AsyncHTTP.rs | 6 ++ test/cli/install/bun-install-retry.test.ts | 115 +++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index b7a1a75ee0b2..acac9f24e9b0 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -769,7 +769,13 @@ impl<'a> AsyncHTTP<'a> { // its `Box` is reclaimed. Only the state the clone // built up itself during request processing is torn down. { + // After a redirect, `client.url` (and `connected_url`) self-borrow + // `client.redirect`, freed just below. The copy-back into the + // JS-thread original must not hand a retry those dangling slices. + let original_url = (*this).url.clone(); let client = &mut (*this).client; + client.url = original_url; + client.connected_url = URL::default(); // Clone-owned (allocated after `ptr::read`). drop(core::mem::take(&mut client.redirect)); drop(core::mem::take(&mut client.prev_redirect)); diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index 2538a2cc0a58..e5261d3243f1 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -40,6 +40,121 @@ afterEach(async () => { await dummyAfterEach(); }); +// Manifest request 302-redirects; the redirect target answers a retryable 500 +// once, then the real packument. The retry used to go out through a URL that +// was a dangling self-borrow into the first attempt's freed redirect buffer +// (ASAN: heap-use-after-free in URL::get_port on the HTTP-client thread), so +// the retries never reached the server. It must restart from the original URL. +it("retries a manifest whose redirect target 500s once", async () => { + const urls: string[] = []; + let redirectTargetHits = 0; + setHandler(async request => { + const { pathname } = new URL(request.url); + urls.push(pathname); + if (pathname === "/BaR") { + return new Response(null, { status: 302, headers: { Location: `${root_url}/redirected/BaR` } }); + } + if (pathname === "/redirected/BaR") { + if (redirectTargetHits++ === 0) { + return new Response("transient", { status: 500 }); + } + return Response.json({ + name: "BaR", + versions: { + "0.0.2": { name: "BaR", version: "0.0.2", dist: { tarball: `${root_url}/BaR-0.0.2.tgz` } }, + }, + "dist-tags": { latest: "0.0.2" }, + }); + } + if (pathname === "/BaR-0.0.2.tgz") { + return new Response(file(join(import.meta.dir, "bar-0.0.2.tgz"))); + } + return new Response("unexpected", { status: 404 }); + }); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { BaR: "0.0.2" } }), + ); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + expect(out).toContain("1 package installed"); + // The retry restarts from the original manifest URL, so the server sees the + // whole redirect chain a second time. + expect(urls).toEqual(["/BaR", "/redirected/BaR", "/BaR", "/redirected/BaR", "/BaR-0.0.2.tgz"]); + expect(await file(join(package_dir, "node_modules", "BaR", "package.json")).json()).toEqual({ + name: "bar", + version: "0.0.2", + }); + expect(exitCode).toBe(0); +}); + +// Same bug, sibling retry site (tarball downloads in runTasks): the tarball URL +// 302-redirects and the target 500s once before serving the archive. +it("retries a tarball whose redirect target 500s once", async () => { + const urls: string[] = []; + let redirectTargetHits = 0; + setHandler(async request => { + const { pathname } = new URL(request.url); + urls.push(pathname); + if (pathname === "/BaR") { + return Response.json({ + name: "BaR", + versions: { + "0.0.2": { name: "BaR", version: "0.0.2", dist: { tarball: `${root_url}/BaR-0.0.2.tgz` } }, + }, + "dist-tags": { latest: "0.0.2" }, + }); + } + if (pathname === "/BaR-0.0.2.tgz") { + return new Response(null, { status: 302, headers: { Location: `${root_url}/redirected/BaR-0.0.2.tgz` } }); + } + if (pathname === "/redirected/BaR-0.0.2.tgz") { + if (redirectTargetHits++ === 0) { + return new Response("transient", { status: 500 }); + } + return new Response(file(join(import.meta.dir, "bar-0.0.2.tgz"))); + } + return new Response("unexpected", { status: 404 }); + }); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { BaR: "0.0.2" } }), + ); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + expect(out).toContain("1 package installed"); + expect(urls).toEqual([ + "/BaR", + "/BaR-0.0.2.tgz", + "/redirected/BaR-0.0.2.tgz", + "/BaR-0.0.2.tgz", + "/redirected/BaR-0.0.2.tgz", + ]); + expect(await file(join(package_dir, "node_modules", "BaR", "package.json")).json()).toEqual({ + name: "bar", + version: "0.0.2", + }); + expect(exitCode).toBe(0); +}); + it("retries on 500", async () => { const urls: string[] = []; setHandler(dummyRegistry(urls, undefined, 4)); From 6f1f74a0e17468668099c5f63cb05fe9d7641a96 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:16:16 +0000 Subject: [PATCH 2/5] test: assert the install exit code before reading generated artifacts Matches the existing test in this file. A failed install now surfaces the exit code and process output instead of an ENOENT on node_modules. --- test/cli/install/bun-install-retry.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index e5261d3243f1..05df82d7691d 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -87,6 +87,7 @@ it("retries a manifest whose redirect target 500s once", async () => { expect(err).not.toContain("error:"); expect(err).toContain("Saved lockfile"); expect(out).toContain("1 package installed"); + expect(exitCode).toBe(0); // The retry restarts from the original manifest URL, so the server sees the // whole redirect chain a second time. expect(urls).toEqual(["/BaR", "/redirected/BaR", "/BaR", "/redirected/BaR", "/BaR-0.0.2.tgz"]); @@ -94,7 +95,6 @@ it("retries a manifest whose redirect target 500s once", async () => { name: "bar", version: "0.0.2", }); - expect(exitCode).toBe(0); }); // Same bug, sibling retry site (tarball downloads in runTasks): the tarball URL @@ -141,6 +141,7 @@ it("retries a tarball whose redirect target 500s once", async () => { expect(err).not.toContain("error:"); expect(err).toContain("Saved lockfile"); expect(out).toContain("1 package installed"); + expect(exitCode).toBe(0); expect(urls).toEqual([ "/BaR", "/BaR-0.0.2.tgz", @@ -152,7 +153,6 @@ it("retries a tarball whose redirect target 500s once", async () => { name: "bar", version: "0.0.2", }); - expect(exitCode).toBe(0); }); it("retries on 500", async () => { From 48158aeec3fd47046c1fcc63fd66bf3f0dab514e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:53:27 +0000 Subject: [PATCH 3/5] http: restore the pre-redirect headers and method on a retried request too A cross-origin redirect strips Authorization/Proxy-Authorization/Cookie/Host from client.header_entries in place (handle_response_metadata), and the same bitwise copy-back that leaked the dangling URL leaked the stripped list into the JS-thread AsyncHTTP. A retried request to an authorized registry whose redirect target failed transiently then went out without Authorization and got a 401. header_entries is bitwise-shared with the JS-thread original, so it must not be dropped or reallocated on the HTTP thread. It was cloned from request_headers at init and only ever shrinks, so restore it in place with clear_retaining_capacity + append_list_assume_capacity. Also restore client.method, which the same function may downgrade to GET for a redirect. --- src/http/AsyncHTTP.rs | 25 ++++++- test/cli/install/bun-install-retry.test.ts | 76 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index acac9f24e9b0..5403f46fed53 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -769,13 +769,32 @@ impl<'a> AsyncHTTP<'a> { // its `Box` is reclaimed. Only the state the clone // built up itself during request processing is torn down. { - // After a redirect, `client.url` (and `connected_url`) self-borrow - // `client.redirect`, freed just below. The copy-back into the - // JS-thread original must not hand a retry those dangling slices. + // `handle_response_metadata` rewrites per-hop request state in + // place: `client.url`/`connected_url` become self-borrows into + // `client.redirect` (freed just below), the method may be + // downgraded to GET, and a cross-origin hop strips auth headers + // from `client.header_entries`. The copy-back into the JS-thread + // original must not hand a re-scheduled attempt any of that. + // + // `header_entries` is bitwise-shared with the original (see the + // comment above), so it must never be dropped or reallocated + // here. It was cloned from `request_headers` at init and only + // ever shrinks (`ordered_remove`), so its capacity is enough. + debug_assert!( + (*this).client.header_entries.capacity() + >= (*this).request_headers.len() + ); + (*this).client.header_entries.clear_retaining_capacity(); + (*this) + .client + .header_entries + .append_list_assume_capacity(&(*this).request_headers); let original_url = (*this).url.clone(); + let original_method = (*this).method; let client = &mut (*this).client; client.url = original_url; client.connected_url = URL::default(); + client.method = original_method; // Clone-owned (allocated after `ptr::read`). drop(core::mem::take(&mut client.redirect)); drop(core::mem::take(&mut client.prev_redirect)); diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index 05df82d7691d..873d96dceda7 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -97,6 +97,82 @@ it("retries a manifest whose redirect target 500s once", async () => { }); }); +// A cross-origin redirect strips Authorization from the in-flight header list +// (per the fetch spec). That list is bitwise-shared with the task's AsyncHTTP, +// so the stripped state used to leak into the install retry and the +// re-requested registry URL went out unauthenticated (401). The retry must +// restore the original headers along with the original URL. +it("retries an authorized manifest whose cross-origin redirect target 500s once", async () => { + const token = "test-registry-token"; + const registryUrls: string[] = []; + const cdnAuth: (string | null)[] = []; + let cdnHits = 0; + // A second server on its own port stands in for the CDN the registry + // redirects to; a different port makes the redirect cross-origin. + await using cdn = Bun.serve({ + port: 0, + fetch(request) { + if (new URL(request.url).pathname !== "/cdn/BaR") { + return new Response("unexpected", { status: 404 }); + } + cdnAuth.push(request.headers.get("authorization")); + if (cdnHits++ === 0) { + return new Response("transient", { status: 500 }); + } + return Response.json({ + name: "BaR", + versions: { + "0.0.2": { name: "BaR", version: "0.0.2", dist: { tarball: `${root_url}/BaR-0.0.2.tgz` } }, + }, + "dist-tags": { latest: "0.0.2" }, + }); + }, + }); + setHandler(async request => { + const { pathname } = new URL(request.url); + registryUrls.push(pathname); + if (pathname === "/BaR") { + // The registry requires the token on every request, including the retry. + if (request.headers.get("authorization") !== `Bearer ${token}`) { + return new Response("missing authorization", { status: 401 }); + } + return new Response(null, { + status: 302, + headers: { Location: `http://localhost:${cdn.port}/cdn/BaR` }, + }); + } + if (pathname === "/BaR-0.0.2.tgz") { + return new Response(file(join(import.meta.dir, "bar-0.0.2.tgz"))); + } + return new Response("unexpected", { status: 404 }); + }); + await writeFile( + join(package_dir, "bunfig.toml"), + `[install]\ncache = false\nregistry = { url = "${root_url}/", token = "${token}" }\nsaveTextLockfile = false\n`, + ); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { BaR: "0.0.2" } }), + ); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + expect(out).toContain("1 package installed"); + expect(exitCode).toBe(0); + // Both registry hits carried the token (the handler 401s otherwise); the + // cross-origin CDN hops must NOT have (the spec strips it for that hop). + expect(registryUrls).toEqual(["/BaR", "/BaR", "/BaR-0.0.2.tgz"]); + expect(cdnAuth).toEqual([null, null]); +}); + // Same bug, sibling retry site (tarball downloads in runTasks): the tarball URL // 302-redirects and the target 500s once before serving the archive. it("retries a tarball whose redirect target 500s once", async () => { From e70d31d6dd7c53fd5d1c3c2d7cb93b5dd1b221cc Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:58:30 +0000 Subject: [PATCH 4/5] [autofix.ci] apply automated fixes --- src/http/AsyncHTTP.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index 5403f46fed53..d123d05dad48 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -781,8 +781,7 @@ impl<'a> AsyncHTTP<'a> { // here. It was cloned from `request_headers` at init and only // ever shrinks (`ordered_remove`), so its capacity is enough. debug_assert!( - (*this).client.header_entries.capacity() - >= (*this).request_headers.len() + (*this).client.header_entries.capacity() >= (*this).request_headers.len() ); (*this).client.header_entries.clear_retaining_capacity(); (*this) From 15c4a1ababda6087cd4286123e4dc377fcc4c957 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:05:34 +0000 Subject: [PATCH 5/5] http: keep the header-capacity invariant checked in release builds append_list_assume_capacity writes into the shared header allocation unchecked; a broken capacity invariant there would corrupt memory rather than just misbehave, so the assertion must survive release builds. Also trim the new test comments to the behavior they assert. --- src/http/AsyncHTTP.rs | 6 ++++-- test/cli/install/bun-install-retry.test.ts | 17 ++++++----------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index d123d05dad48..df4a620f9865 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -779,8 +779,10 @@ impl<'a> AsyncHTTP<'a> { // `header_entries` is bitwise-shared with the original (see the // comment above), so it must never be dropped or reallocated // here. It was cloned from `request_headers` at init and only - // ever shrinks (`ordered_remove`), so its capacity is enough. - debug_assert!( + // ever shrinks (`ordered_remove`), so its capacity is enough; + // the unchecked append below would corrupt the shared heap + // allocation otherwise, so keep this assertion in release too. + assert!( (*this).client.header_entries.capacity() >= (*this).request_headers.len() ); (*this).client.header_entries.clear_retaining_capacity(); diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index 873d96dceda7..b49d0c924750 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -40,11 +40,8 @@ afterEach(async () => { await dummyAfterEach(); }); -// Manifest request 302-redirects; the redirect target answers a retryable 500 -// once, then the real packument. The retry used to go out through a URL that -// was a dangling self-borrow into the first attempt's freed redirect buffer -// (ASAN: heap-use-after-free in URL::get_port on the HTTP-client thread), so -// the retries never reached the server. It must restart from the original URL. +// Manifest request 302-redirects and the redirect target answers a retryable +// 500 once. The install retry must restart from the original manifest URL. it("retries a manifest whose redirect target 500s once", async () => { const urls: string[] = []; let redirectTargetHits = 0; @@ -97,11 +94,9 @@ it("retries a manifest whose redirect target 500s once", async () => { }); }); -// A cross-origin redirect strips Authorization from the in-flight header list -// (per the fetch spec). That list is bitwise-shared with the task's AsyncHTTP, -// so the stripped state used to leak into the install retry and the -// re-requested registry URL went out unauthenticated (401). The retry must -// restore the original headers along with the original URL. +// A cross-origin redirect strips Authorization from the request (per the fetch +// spec). The install retry restarts from the original registry URL and must +// carry the original headers, including Authorization, again. it("retries an authorized manifest whose cross-origin redirect target 500s once", async () => { const token = "test-registry-token"; const registryUrls: string[] = []; @@ -173,7 +168,7 @@ it("retries an authorized manifest whose cross-origin redirect target 500s once" expect(cdnAuth).toEqual([null, null]); }); -// Same bug, sibling retry site (tarball downloads in runTasks): the tarball URL +// Sibling retry site (tarball downloads in runTasks): the tarball URL // 302-redirects and the target 500s once before serving the archive. it("retries a tarball whose redirect target 500s once", async () => { const urls: string[] = [];