Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@hono/auth-js] don't use request body #783

Closed
wants to merge 2 commits into from
Closed

Conversation

bordeo
Copy link

@bordeo bordeo commented Oct 18, 2024

auth-js don't need request body.
accessing request body prevent future usage by other hono handlers

auth-js don't need request body.
accessing request body prevent future usage by hono handler
Copy link

changeset-bot bot commented Oct 18, 2024

🦋 Changeset detected

Latest commit: 3d02cab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/auth-js Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yusukebe
Copy link
Member

Hi @bordeo Thank you for the PR.

@divyam234 Can you review this?

@divyam234
Copy link
Contributor

divyam234 commented Oct 21, 2024

@bordeo request body is always used in login action

body: new URLSearchParams({ ...opts, csrfToken, callbackUrl }),
see here. @yusukebe this pr should not be merged

@yusukebe
Copy link
Member

@divyam234 Thanks! I'll close this PR now.

@yusukebe yusukebe closed this Oct 23, 2024
@bordeo
Copy link
Author

bordeo commented Oct 23, 2024

I understand your point @divyam234, I'm gonna create a new PR with different approach because if we use verifyAuth and we try to read the request body we have the error.

@divyam234
Copy link
Contributor

Are you using bun?

@bordeo
Copy link
Author

bordeo commented Oct 23, 2024

yes. the problem isn't the runtime but the fact that request body can be used only once

@divyam234
Copy link
Contributor

divyam234 commented Oct 23, 2024

@bordeo This was added as a workaround for bun as req cloning was not working prior to 1.26 but now this can be replaced by simple request cloning like other runtimes.You can raise pr for that.

@divyam234
Copy link
Contributor

divyam234 commented Oct 23, 2024

import { test, expect } from "bun:test";

test("should clone request with a readable stream body correctly", async () => {
  const originalUrl = "https://example.com";
  const clonedUrl = "https://example1.com";
  const bodyContent = "test";

  const readableStream = new ReadableStream({
    start(controller) {
      controller.enqueue(new TextEncoder().encode(bodyContent));
      controller.close();
    }
  });

  const req = new Request(originalUrl, {
    method: "POST",
    body: readableStream,
    headers: {
      test: "test"
    }
  });

  const cloneRequest = new Request(clonedUrl, req);

  expect(cloneRequest.url).toBe(clonedUrl);

  expect(cloneRequest.method).toBe(req.method);

  expect(cloneRequest.headers.get("test")).toBe(req.headers.get("test"));

  const clonedBody = await cloneRequest.text();
  expect(clonedBody).toBe(bodyContent);
});

I had this test in one of my repo which was faling in earlier versions for bun but its passing now on latest version.Took them one year to fix this simple issue

@bordeo
Copy link
Author

bordeo commented Oct 23, 2024

yes your code is going to work because you are reading the body once with .text.
verifyAuth calls getAuthUser, which calls reqWithEnvUrl, which calls cloneRequest. However, cloneRequest is not using the proper method .clone but is instead creating a new Request object with the body read from the original request using request.blob.
Once you read from the original request, you can't read it again.
The original request can be used in the handler after verifyAuth to read body JSON params, for example, but attempting to read it throws an error because cloneRequest has already read it

@divyam234
Copy link
Contributor

#790 this pr will fix this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants