zirkelc / aws-sigv4-fetch

AWS SignatureV4 fetch function built on the official AWS SDK for JS
https://www.npmjs.com/package/aws-sigv4-fetch
MIT License
26 stars 5 forks source link

Non-GET requests don't work when passed as a single Request argument #19

Open sbking opened 2 months ago

sbking commented 2 months ago

When passing a non-GET Request object as a single argument, the method is automatically changed to "GET".

zirkelc commented 2 months ago

Hi @sbking thanks for reporting this. I'm going to look into this.

Do you have an example how you call it with a Request object?

sbking commented 2 months ago

Hi @zirkelc, I'm trying to use this with a REST API client generated by @hey-api/openapi-ts. Here is where they construct a new Request() and pass it to the provided fetch function:

https://github.com/hey-api/openapi-ts/blob/main/packages/client-fetch/src/index.ts#L62-L69

And a little example on MDN:

https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch#examples

Something like this is a minimal example:

const request = new Request("https://example.com", { method: "POST" });
fetch(request);
zirkelc commented 2 months ago

Hi @sbking, I have fixed this issue and released a new version v4.0.1.

Could you please check if it works for you?

sbking commented 2 months ago

@zirkelc Unfortunately now I'm getting this when sending a POST request:

TypeError: RequestInit: duplex option is required when sending a body.
zirkelc commented 2 months ago

@sbking I had the same issue, however, it looks like this is specific to Node.js implementation of fetch(). I can reproduce the same error on Node.js v20 and plain fetch(). Taking this example from Google Chrome:

(async () => {
  const stream = new ReadableStream({
    async start(controller) {
      controller.enqueue("This ");
      controller.enqueue("is ");
      controller.enqueue("a ");
      controller.enqueue("slow ");
      controller.enqueue("request.");
      controller.close();
    },
  }).pipeThrough(new TextEncoderStream());

  const request = new Request("https://example.com", {
    method: "POST",
    body: stream,
    // duplex: "half",
  });
  const response = await fetch(request);
  console.log(response);
})();

https://github.com/user-attachments/assets/efc1578e-d454-474a-bff2-34f7d35a2b0a

If your body is a stream, you have to set the duplex option to half. It is not required for string body though. Looking at you library, it applies some kind of serialization to the body:

https://github.com/hey-api/openapi-ts/blob/7e84960713160fc942c6e5a109e7d9687fb9dafc/packages/client-fetch/src/index.ts#L37-L39

So maybe it is streaming the body even though you supply only a string.

Can you set the duplex: "half" option as part of the RequestInit and see if it works? I know TypeScript is complaining that the option doesn't exist, but it surely does.

sbking commented 2 months ago

@zirkelc I don't really have control over RequestInit - the library gives me a Request object that already works with the vanilla Node 20 fetch, but not with this library's fetch

zirkelc commented 2 months ago

@sbking could you set up a small repository with some code so I can debug this issue on my side? No sensitive data or secret keys, just the code you are using and a few comments where I can insert my own AWS credentials to investigate it further

sbking commented 1 month ago

Hey @zirkelc I can try to set up a small reproduction repo when I have some time. For now, I'm using this instead which seems to be working perfectly. I believe the trick is the call to request.clone() which seems to respect whatever the original request's duplex option is:

export async function sigv4Fetch(request: Request) {
  const newRequest = request.clone();

  const url = new URL(request.url);
  const body = await request.text();
  const headers = {
    ...Object.fromEntries(request.headers.entries()),
    // host is required by AWS Signature V4: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
    host: url.host,
  };

  const smithyRequest = new HttpRequest({
    hostname: url.hostname,
    path: url.pathname,
    protocol: url.protocol,
    port: url.port ? Number(url.port) : undefined,
    username: url.username,
    password: url.password,
    method: request.method.toUpperCase(),
    body,
    query: Object.fromEntries(url.searchParams.entries()),
    fragment: url.hash,
    headers,
  });

  const signer = new SignatureV4({
    credentials: fromNodeProviderChain(),
    service: "lambda",
    region: process.env.AWS_REGION ?? "us-east-1",
    sha256: Sha256,
  });

  const signedRequest = await signer.sign(smithyRequest);

  for (const [key, value] of Object.entries(signedRequest.headers)) {
    newRequest.headers.set(key, value);
  }

  return fetch(newRequest);
}
zirkelc commented 1 month ago

Hi @sbking good that you found a workaround. I will investigate this further. I assume the signer.sign() somehow changes the body and you avoid that by cloning the request and simply copying the signed headers back.