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

Fixes request body handling in this library #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timbastin
Copy link

Today I spend a lot of time debugging this library. The requirements I had to meet was to enable passkeys and password registration and login. I had a lot of issues because of the forwarded request body and content-type header.

Registering with a passkey resulted in a HTTP-Request originated from this library which had a Body in JSON-Format but a Content-Type Header of application/form-data. This, obviously, resulted in the ory kratos server not being able to parse the body correctly. I had to change the content-type header to application/json and the body to a stringified JSON-Object (https://github.com/ory/kratos/blob/master/selfservice/flow/registration/decoder.go#L32)[https://github.com/ory/kratos/blob/master/selfservice/flow/registration/decoder.go#L32]

I stumbled upon something which looks like a bug to me.

  1. The documentation states, one should disable the body parsing https://github.com/ory/integrations/blob/main/README.md#nextjs

    import { config, createApiHandler } from "@ory/integrations/next-edge"
    
    export { config } // this line
    
    export default createApiHandler({
    /* ... */
    })
  2. This is in direct contradiction to the code in the library. The createdApiHandler function stringifies the req.body (https://github.com/ory/integrations/blob/main/src/next-edge/index.ts#L115). Which is by definition undefined if the body parsing is disabled, since one has todo it manually.

    const response = await fetch(url, {
      method: req.method,
      headers,
      body:
        req.method !== "GET" && req.method !== "HEAD"
          ? JSON.stringify(req.body) // right here
          : null,
      redirect: "manual",
    })

There is a issue with quite some comments which is related to this problem: #29

The content type is zero, since the req.body is undefined. The solution to this ticket was to enable body parsing again by just stopping to export the config object. This is a workaround and not a solution to the problem: #29 (comment)

My Issue:

I did exactly like the comment asked me todo, but if you enable passkeys, you are actually supposed to send form-data and not a JSON-Object.

Since Body-Parsing is enabled again, nextjs is going to parse the form-data into a javascript object. The provided content-type header is application/form-data, which is correct. But know this library copies the application/form-data header and JSON.stringifies the request body, resulting in an invalid combination.

The fix is super simple: Just read the raw body and forward it. No json stringify. This keeps the content-type header and the body-format in sync. I think this is the intended behavior of the library, since the documentation states that the body parsing should be disabled.

@CLAassistant
Copy link

CLAassistant commented Nov 28, 2024

CLA assistant check
All committers have signed the CLA.

@timbastin
Copy link
Author

Pretty related to this one: #68

Nevertheless, just sending a buffer without inspecting is more future proof in my opinion. Otherwise one could end up with a lot of ternaries.

@timbastin
Copy link
Author

timbastin commented Nov 28, 2024

@aeneasr Reach out to me if you need help with maintaining this small library. We are using it in a big project and i am working with it on a daily basis.

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.

2 participants