Unverified Commit ffcdecd8 authored by Hong Minhee's avatar Hong Minhee
Browse files

Fix TypeError: unusable in doubleKnock redirect handling 

Fixed inconsistent request body handling in the second redirect path
of doubleKnock() function. Changed from request.clone().body to
await request.clone().arrayBuffer() to prevent body consumption issues
that caused "TypeError: unusable" errors during HTTP signature retry
attempts with redirects.

Also replaced destructuring with explicit property copying to ensure
proper Request constructor behavior.

Closes https://github.com/fedify-dev/fedify/issues/294



Co-Authored-By: default avatarClaude <noreply@anthropic.com>
parent df9a24f0

CHANGES.md

+7 −0
Original line number Diff line number Diff line
@@ -8,6 +8,13 @@ Version 1.6.6 


To be released.



 -  Fixed `TypeError: unusable` error that occurred when `doubleKnock()`

    encountered redirects during HTTP signature retry attempts.

    [[#294], [#295]]



[#294]: https://github.com/fedify-dev/fedify/issues/294

[#295]: https://github.com/fedify-dev/fedify/pull/295





Version 1.6.5

-------------

fedify/sig/http.test.ts

+58 −0
Original line number Diff line number Diff line
@@ -1877,3 +1877,61 @@ test("verifyRequest() [rfc9421] error handling for invalid signature base creati 
    "Verification should fail gracefully for malformed signature inputs",

  );

});



test("doubleKnock() regression test for TypeError: unusable bug #294", async () => {

  // This test reproduces the bug where request.clone().body in the second redirect

  // handling path causes "TypeError: unusable" when the body is consumed before

  // subsequent clone() calls in signRequest functions.



  fetchMock.spyGlobal();



  let requestCount = 0;



  // Mock server that:

  // 1. Returns 401 for first spec (triggers retry with different spec)

  // 2. Returns 302 redirect for second spec (triggers redirect handling)

  // 3. Returns 200 for final destination

  fetchMock.post("https://example.com/inbox-retry-redirect", (_cl) => {

    requestCount++;



    if (requestCount === 1) {

      // First request: reject to trigger retry with different spec

      return new Response("Unauthorized", { status: 401 });

    } else if (requestCount === 2) {

      // Second request: redirect to trigger the problematic redirect handling

      return Response.redirect("https://example.com/final-destination", 302);

    }



    return new Response("Should not reach here", { status: 500 });

  });



  // Mock final destination

  fetchMock.post("https://example.com/final-destination", () => {

    return new Response("Success", { status: 200 });

  });



  const request = new Request("https://example.com/inbox-retry-redirect", {

    method: "POST",

    body: "Test activity content",

    headers: {

      "Content-Type": "application/activity+json",

    },

  });



  // This should trigger the bug: 401 -> retry -> 302 -> TypeError: unusable

  // because the second redirect path uses request.clone().body instead of

  // await request.clone().arrayBuffer()

  const response = await doubleKnock(

    request,

    {

      keyId: rsaPublicKey2.id!,

      privateKey: rsaPrivateKey2,

    },

  );



  // The test should pass after the fix

  assertEquals(response.status, 200);

  assertEquals(requestCount, 2, "Should make 2 requests before redirect");



  fetchMock.hardReset();

});

fedify/sig/http.ts

+21 −3
Original line number Diff line number Diff line
@@ -1322,11 +1322,29 @@ export async function doubleKnock( 
      response.headers.has("Location")

    ) {

      const location = response.headers.get("Location")!;

      // IMPORTANT: Use arrayBuffer() instead of .body to prevent "TypeError: unusable"

      // When using .body (ReadableStream), subsequent clone() calls in signRequest functions

      // will fail because the stream has already been consumed. Using arrayBuffer() ensures

      // the body can be safely cloned for HTTP signature generation.

      const body = request.method !== "GET" && request.method !== "HEAD"

        ? request.clone().body

        : null;

        ? await request.clone().arrayBuffer()

        : undefined;

      return doubleKnock(

        new Request(location, { ...request, body }),

        new Request(location, {

          // Explicitly copy all Request properties instead of using destructuring

          // to ensure proper Request constructor behavior and preserve all properties

          method: request.method,

          headers: request.headers,

          body,

          redirect: "manual",

          signal: request.signal,

          mode: request.mode,

          credentials: request.credentials,

          referrer: request.referrer,

          referrerPolicy: request.referrerPolicy,

          integrity: request.integrity,

          keepalive: request.keepalive,

        }),

        identity,

        options,

      );