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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIddleware should accept custom JWT decode method to correctly read custom-signed JWT #4181

Closed
hinsxd opened this issue Mar 14, 2022 · 5 comments 路 Fixed by #4210
Closed

MIddleware should accept custom JWT decode method to correctly read custom-signed JWT #4181

hinsxd opened this issue Mar 14, 2022 · 5 comments 路 Fixed by #4210
Labels
enhancement New feature or request good first issue Good issue to take for first time contributors

Comments

@hinsxd
Copy link
Contributor

hinsxd commented Mar 14, 2022

Description 馃摀

const token = await getToken({ req: req as any })

Middleware is calling getToken directly without providing any decode methods. By getToken() uses jwtDecrypt from jose package, and it will probably throws error when the JWT is not signed in the same way. It will throw error when we provide custom JWT encode/decode inside [...nextauth].ts

There should be a way to synchronize / share settings between [...nextauth].ts and _middleware.ts

How to reproduce 鈽曪笍

// [...nextauth].ts
import jwt from "jsonwebtoken";
...

  jwt: {
    encode: async ({ secret, token }) => {
      // Do other stuff
      return jwt.sign(token as any, secret, { algorithm: "HS256" });
    },
    decode: async ({ secret, token }) => {
      return jwt.verify(token as string, secret, {
        algorithms: ["HS256"],
      }) as any;
    },
  },
// Any _middleware.ts
export { default } from "next-auth/middleware"

Contributing 馃檶馃徑

No, I am afraid I cannot help regarding this

@hinsxd hinsxd added enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Mar 14, 2022
@balazsorban44 balazsorban44 added good first issue Good issue to take for first time contributors and removed triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Mar 15, 2022
@balazsorban44
Copy link
Member

balazsorban44 commented Mar 15, 2022

Yeah, I think we could add jwt.decode to the Middleware API, and forward it to getToken (the idea is to keep the API the same as NextAuthOptions). 馃憤 I don't think encode is used anywhere? 馃

@hinsxd
Copy link
Contributor Author

hinsxd commented Mar 16, 2022

Yes I think so. As long as the middleware is only used to read session and protect pages, decode would be enough.

Some extra thoughts:
Is it possible to declare all the settings in next.config.js or a separate config files, so both api routes and middleware can grab the config? Sounds like a better option than asking user to abstract the decode function themselves and put it somewhere. We can have some opinionated file structures

@hinsxd
Copy link
Contributor Author

hinsxd commented Mar 17, 2022

@balazsorban44 I have made PR to address this issure. Feel free to have a look, thanks!

@agustif
Copy link

agustif commented Mar 20, 2022

Some extra thoughts: Is it possible to declare all the settings in next.config.js or a separate config files, so both api routes and middleware can grab the config? Sounds like a better option than asking user to abstract the decode function themselves and put it somewhere. We can have some opinionated file structures

What I've seen in the codebase is declaring a const that you export in nextauth file itself called authOptions
https://github.com/nextauthjs/next-auth/blob/001354eaa8053427d996d2e7c0b3eba69e0552cb/apps/dev/pages/api/auth/%5B...nextauth%5D.ts
I've copied it myself, maybe that's good enough for now? Adding an auth.config.js in the future might be useful to make it easier from scratch to use appropiate config that can be reused across the codbase or different repos/projects.

@hinsxd
Copy link
Contributor Author

hinsxd commented Mar 21, 2022

Agreed. Lets keep everything simple unless a rewrite / breaking change is going to be introduced.

@stale stale bot added the stale Did not receive any activity for 60 days label May 25, 2022
@nextauthjs nextauthjs deleted a comment from stale bot May 31, 2022
@stale stale bot removed the stale Did not receive any activity for 60 days label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good issue to take for first time contributors
Projects
None yet
3 participants