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

fix(core): handle Request -> Response regressions #5991

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

balazsorban44
Copy link
Member

@balazsorban44 balazsorban44 commented Dec 8, 2022

#4769 was a major internal refactoring that caused some bugs to surface:

@vercel
Copy link

vercel bot commented Dec 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
next-auth ✅ Ready (Inspect) Visit Preview Dec 8, 2022 at 11:30PM (UTC)

@github-actions github-actions bot added the core Refers to `@auth/core` label Dec 8, 2022
@balazsorban44 balazsorban44 marked this pull request as draft December 8, 2022 15:25
@balazsorban44 balazsorban44 temporarily deployed to Preview December 8, 2022 15:26 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🎉 Experimental release published 📦️ on npm!

@ammmze
Copy link
Contributor

ammmze commented Dec 8, 2022

I'm still having some issues with unstable_getServerSession after switching to [email protected].

I get an error page with the error FetchError: invalid json response body at reason: Unexpected token E in JSON at position 0.

To debug further, I modified the unstable_getServerSession function in node_modules adding the following:

console.log('request url =>', urlOrError);
console.log('response body =>', response.body.toString());

The result of these console logs are:

request url => URL {
  href: 'http://localhost:3000/sso/api/auth/api/auth/session',
  origin: 'http://localhost:3000',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'localhost:3000',
  hostname: 'localhost',
  port: '3000',
  pathname: '/sso/api/auth/api/auth/session',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
response body => Error: This action with HTTP GET is not supported by NextAuth.js

It looks like the url path is a bit wonky. The path should be /sso/api/auth/session. For some additional context, I am running my application with a base path configured in NextJS of /sso. And as a result I have NEXTAUTH_URL configured as http://localhost:3000/sso/api/auth (as described here).

Edit: Looks like duplication in the path isn't necessarily the cause. I temporarily changed the path in the getURL call from /api/auth/session to just /session so the result is the correct path. However, I still get the same error response body from the auth handler (Error: This action with HTTP GET is not supported by NextAuth.js).

Edit 2: But it does look like the base path is likely the main trigger here. I jumped in the AuthHanderInternal and added a console.log({method, action}); which resulted in { method: 'GET', action: 'auth' }. So it looks like it is picking up the wrong action.

@ammmze
Copy link
Contributor

ammmze commented Dec 8, 2022

Edit 2: But it does look like the base path is likely the main trigger here. I jumped in the AuthHanderInternal and added a console.log({method, action}); which resulted in { method: 'GET', action: 'auth' }. So it looks like it is picking up the wrong action.

Looks like one option is to update these lines in the getURL method with the following to strip any path from the host variable (which in my case is coming from the NEXTAUTH_URL:

    if (host.startsWith("http://") || host.startsWith("https://")) {
      return new URL(`${host.replace(/(https?:\/\/[^/]+)\/?.*/, '$1')}${url}`)
    }
    return new URL(`https://${host.replace(/([^/]+)\/?.*/, '$1')}${url}`)

@ammmze
Copy link
Contributor

ammmze commented Dec 8, 2022

Edit 2: But it does look like the base path is likely the main trigger here. I jumped in the AuthHanderInternal and added a console.log({method, action}); which resulted in { method: 'GET', action: 'auth' }. So it looks like it is picking up the wrong action.

Looks like one option is to update these lines in the getURL method with the following to strip any path from the host variable (which in my case is coming from the NEXTAUTH_URL:

    if (host.startsWith("http://") || host.startsWith("https://")) {
      return new URL(`${host.replace(/(https?:\/\/[^/]+)\/?.*/, '$1')}${url}`)
    }
    return new URL(`https://${host.replace(/([^/]+)\/?.*/, '$1')}${url}`)

Hmm...though another issue that crops up from stripping the path from the host is that anywhere that we call that getURL with the path we then lose our base path which can lead to some invalid redirects. So I think we also need some updates to that toInternalRequest method that was added that extracts things like the action, which in gist is effectively doing action = url.pathname.split("/").slice(3)[0]. This works fine without a base path, but then add the base path and we get the wrong action...

❯ node
Welcome to Node.js v16.14.2.
Type ".help" for more information.
> '/api/auth/signin'.split('/').slice(3)
[ 'signin' ]
> '/sso/api/auth/signin'.split('/').slice(3)
[ 'auth', 'signin' ]

@ammmze
Copy link
Contributor

ammmze commented Dec 8, 2022

I think we also need some updates to that toInternalRequest method

Perhaps we should do something like this in there? So basically first we take just the part of the path starting with the last /api/auth and then do the split and slice. This will give us the consistency we need in extracting the "nextauth" array.

const nextauth = url.pathname.substring(url.pathname.lastIndexOf("/api/auth")).split("/").slice(3);

@balazsorban44
Copy link
Member Author

balazsorban44 commented Dec 8, 2022

The custom basepath handling is on my todo list, it's not checked off in the description as you can see in #5991 (comment) 👍

Based on your URL http://localhost:3000/sso/api/auth/api/auth/session it's that.

Thanks!

@balazsorban44
Copy link
Member Author

I'll merge this PR for now to fix the two other issues, I did not manage to fix this today, postponing the custom base path fix tomorrow, will follow up in a separate PR.

@balazsorban44 balazsorban44 marked this pull request as ready for review December 8, 2022 23:28
@balazsorban44 balazsorban44 merged commit 5c4a9a6 into main Dec 8, 2022
@balazsorban44 balazsorban44 deleted the fix/request-response-regressions branch December 8, 2022 23:29
@ammmze
Copy link
Contributor

ammmze commented Dec 9, 2022

I'll merge this PR for now to fix the two other issues, I did not manage to fix this today, postponing the custom base path fix tomorrow, will follow up in a separate PR.

Sounds good! Thank you!

@noclat
Copy link

noclat commented Dec 9, 2022

Thanks guys, literally spent the complete past day just trying to debug CredentialsProvider (jwt ofc) not creating session cookie in prod and had absolutely no clue why, and woke up to this fix. At least I know the full NextAuth doc by heart now.

ThangHuuVu pushed a commit to ThangHuuVu/next-auth that referenced this pull request Dec 12, 2022
* fix(core): properly construct url (nextauthjs#5984)

* chore(release): bump package version(s) [skip ci]

* fix(core): add protocol if missing

* fix(core): throw error if no action can be determined

* test(core): fix test

* chore(release): bump package version(s) [skip ci]

* chore(docs): add new tutorial (nextauthjs#5604)

Co-authored-by: Nico Domino <[email protected]>

* fix(core): handle `Request` -> `Response` regressions  (nextauthjs#5991)

* fix(next): don't override `Content-Type` by `unstable_getServerSession`

* fix(core): handle `,` while setting `set-cookie`

* chore(release): bump package version(s) [skip ci]

* fix(sequelize): increase sequelize `id_token` column length (nextauthjs#5929)

Co-authored-by: Nico Domino <[email protected]>

* fix(core): correct status code when returning redirects (nextauthjs#6004)

* fix(core): correctly set status when returning redirect

* update tests

* forward other headers

* update test

* remove default 200 status

* fix(core): host detection/NEXTAUTH_URL (nextauthjs#6007)

* rename `host` to `origin` internally

* rename `userOptions` to `authOptions` internally

* use object for `headers` internally

* default `method` to GET

* simplify `unstable_getServerSession`

* allow optional headers

* revert middleware

* wip getURL

* revert host detection

* use old `detectHost`

* fix/add some tests wip

* move more to core, refactor getURL

* better type auth actions

* fix custom path support (w/ api/auth)

* add `getURL` tests

* fix email tests

* fix assert tests

* custom base without api/auth, with trailing slash

* remove parseUrl from assert.ts

* return 400 when wrong url

* fix tests

* refactor

* fix protocol in dev

* fix tests

* fix custom url handling

* add todo comments

* chore(release): bump package version(s) [skip ci]

* update lock file

* fix(next): correctly bundle next-auth/middleware
fixes nextauthjs#6025

* fix(core): preserve incoming set cookies (nextauthjs#6029)

* fix(core): preserve `set-cookie` by the user

* add test

* improve req/res mocking

* refactor

* fix comment typo

* chore(release): bump package version(s) [skip ci]

* make logos optional

* sync with `next-auth`

* clean up `next-auth/edge`

* sync

Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Thomas Desmond <[email protected]>
Co-authored-by: Nico Domino <[email protected]>
Co-authored-by: Cyril Perraud <[email protected]>
balazsorban44 added a commit that referenced this pull request Dec 13, 2022
* WIP use `Request` and `Response` for core

* bump Next.js

* rename ts types

* refactor

* simplify

* upgrade Next.js

* implement body reader

* use `Request`/`Response` in `next-auth/next`

* make linter happy

* revert

* fix tests

* remove workaround for middleware return type

* return session in protected api route example

* don't export internal handler

* fall back host to localhost

* refactor `getBody`

* refactor `next-auth/next`

* chore: add `@edge-runtime/jest-environment`

* fix tests, using Node 18 as runtime

* fix test

* remove patch

* upgrade/add dependencies

* type and default import on one line

* don't import all adapters by default in dev

* simplify internal endpoint config

Instead of passing url and params around as a string and an object,
we parse them into a `URL` instance.

* assert if both endpoint and issuer config is missing

* allow internal redirect to be `URL`

* mark clientId as always internally, fix comments

* add web-compatible authorization URL handling

* fix type

* fix neo4j build

* remove new-line

* reduce file changes in the PR

* simplify types

* refactor `crypto` usage

In Node.js, inject `globalThis.crypto` instead of import

* add `next-auth/web`

* refactor

* send header instead of body to indicate redirect response

* fix eslint

* fix tests

* chore: upgrade dep

* fix import

* refactor: more renames

* wip core

* support OIDC

* remove `openid-client`

* temprarily remove duplicate logos

* revert

* move redirect logic to core

* feat: add sveltekit auth

* wip fix css

* revert Logo component

* output ESM

* fix logout

* deprecate OAuth 1,  simplify internals, improve defaults

* refactor providers, test facebook

* fix providers

* target es2020

* fix CSS

* fix AuthHandler, add getServerSession

* update lock file

* make logos optional

* sync with `next-auth`

* clean up `next-auth/edge`

* sync

* Sync (#2)

* fix(core): properly construct url (#5984)

* chore(release): bump package version(s) [skip ci]

* fix(core): add protocol if missing

* fix(core): throw error if no action can be determined

* test(core): fix test

* chore(release): bump package version(s) [skip ci]

* chore(docs): add new tutorial (#5604)

Co-authored-by: Nico Domino <[email protected]>

* fix(core): handle `Request` -> `Response` regressions  (#5991)

* fix(next): don't override `Content-Type` by `unstable_getServerSession`

* fix(core): handle `,` while setting `set-cookie`

* chore(release): bump package version(s) [skip ci]

* fix(sequelize): increase sequelize `id_token` column length (#5929)

Co-authored-by: Nico Domino <[email protected]>

* fix(core): correct status code when returning redirects (#6004)

* fix(core): correctly set status when returning redirect

* update tests

* forward other headers

* update test

* remove default 200 status

* fix(core): host detection/NEXTAUTH_URL (#6007)

* rename `host` to `origin` internally

* rename `userOptions` to `authOptions` internally

* use object for `headers` internally

* default `method` to GET

* simplify `unstable_getServerSession`

* allow optional headers

* revert middleware

* wip getURL

* revert host detection

* use old `detectHost`

* fix/add some tests wip

* move more to core, refactor getURL

* better type auth actions

* fix custom path support (w/ api/auth)

* add `getURL` tests

* fix email tests

* fix assert tests

* custom base without api/auth, with trailing slash

* remove parseUrl from assert.ts

* return 400 when wrong url

* fix tests

* refactor

* fix protocol in dev

* fix tests

* fix custom url handling

* add todo comments

* chore(release): bump package version(s) [skip ci]

* update lock file

* fix(next): correctly bundle next-auth/middleware
fixes #6025

* fix(core): preserve incoming set cookies (#6029)

* fix(core): preserve `set-cookie` by the user

* add test

* improve req/res mocking

* refactor

* fix comment typo

* chore(release): bump package version(s) [skip ci]

* make logos optional

* sync with `next-auth`

* clean up `next-auth/edge`

* sync

Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Thomas Desmond <[email protected]>
Co-authored-by: Nico Domino <[email protected]>
Co-authored-by: Cyril Perraud <[email protected]>

* merge

* clean up sveltekit auth handler

* upgrade playground to latest

* upgrade sveltekit auth to latest

* Some more refactoring

* feat: extract type to core and reuse in sveltekit

* remove uuid

* make secret required in dev

* remove todo comments

* pass through OAuth client options

* generate declaration map

* default env secret to AUTH_SECRET

* temporary Headers fix

* move pages to lib

* move errors to lib

* move pages/index to lib

* move routes to lib

* move init to lib

* move styles to lib

* move types to lib

* move utils to lib

* fix imports

* update ignore/clean patterns

* fix imports

* update styles ts

* update gitignore

* update exports field

* revert `next-auth`

* remove extra tsconfig files

* remove `private` from package.json

* revert

* feat sveltekit

* commit

* remove unused file, expose type

* remove nextauth_url, memoize locals.getSession

* move to dependency

* fix

* format

* fix post build

* simplify

* fix lock file

* add packages/frameworks

* update package.json

* update gitignore

* Delete .gitignore

* Update types.ts

* Update tsconfig.dev.json

* skip test

* format

* skip format/lint

Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Thomas Desmond <[email protected]>
Co-authored-by: Nico Domino <[email protected]>
Co-authored-by: Cyril Perraud <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core`
Projects
None yet
3 participants