http: fix use-after-free of the redirect URL on a retried request

src/http/AsyncHTTP.rs

@@ -769,7 +769,32 @@ impl<'a> AsyncHTTP<'a> {
                 // its `Box<AsyncHTTP>` is reclaimed. Only the state the clone
                 // built up itself during request processing is torn down.
                 {
+                    // `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));

test/cli/install/bun-install-retry.test.ts

@@ -40,6 +40,197 @@ 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");
+  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"]);
+  expect(await file(join(package_dir, "node_modules", "BaR", "package.json")).json()).toEqual({
+    name: "bar",
+    version: "0.0.2",
+  });
+});
+
+// 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 () => {
+  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(exitCode).toBe(0);
+  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",
+  });
+});
+
 it("retries on 500", async () => {
   const urls: string[] = [];
   setHandler(dummyRegistry(urls, undefined, 4));