search

nextauthjs / next-auth

Authentication for the Web.
https://authjs.dev
ISC License
22.87k stars 3.08k forks source link

Feature/nonce check type #4100

Closed james-bjss closed 1 year ago

james-bjss commented 2 years ago

Reasoning 💡

Adding the ability to generate and validate a nonce value when authenticating against an Open ID provider.

This resolves a "nonce missmatch" error raised when authenticating against an external IDP with Cognito.

Specifically this is to fix the following issue: https://github.com/nextauthjs/next-auth/discussions/3551#

Checklist 🧢

Affected issues 🎟

Fixes: #3551

Notes On Usage

const handler = NextAuth({ providers: [ CognitoProvider({ clientId: process.env.COGNITO_CLIENT_ID, clientSecret: process.env.COGNITO_CLIENT_SECRET, issuer: process.env.COGNITO_ISSUER, idToken: true, checks: 'nonce', }), ], })

export default handler

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/HggmkVHNf7hrWyf2YA3TjN518wT6
✅ Preview: https://next-auth-git-fork-james-bjss-feature-nonce-c-021620-nextauthjs.vercel.app

james-bjss commented 2 years ago

@balazsorban44 - If you require a test-cognito endpoint and credentials I can potentially set this up for you on my personal account, just give me some notice and I can set it up :)

james-bjss commented 2 years ago

Hi @balazsorban44 sorry to bug you, but would it be possible to give an indication if this fix is viable and something you may be willing to accept into the project?

I wouldn't normally chase up on PRs, but we are faced with a difficult decision on our project at the moment. Basically:

  1. Continue to use NextAuth with a fork containing this PR in the hope that the issue will be eventually be fixed.
  2. Start looking at refactoring to use Amplify libraries (which potentially brings with it other issues)

I realise this is a side-project and you are busy, but just looking for some confidence that a fix will be forthcoming if we chose Option1 even if its not immediate.

james-bjss commented 2 years ago

Hi @balazsorban44 sorry to bug you, but would it be possible to give an indication if this fix is viable and something you may be willing to accept into the project?

I wouldn't normally chase up on PRs, but we are faced with a difficult decision on our project at the moment. Basically:

  1. Continue to use NextAuth with a fork containing this PR in the hope that the issue will be eventually be fixed.
  2. Start looking at refactoring to use Amplify libraries (which potentially brings with it other issues)

I realise this is a side-project and you are busy, but just looking for some confidence that a fix will be forthcoming if we chose Option1 even if its not immediate.

@ThangHuuVu @ndom91 - Sorry to tag you but would you be able to answer the above?

balazsorban44 commented 2 years ago

Hi, I'm sorry, I'm currently taking a small break from the project to work through personal issues. Thanks for tagging the others.

I'll look into all PRs once I feel better. 💚🙏

james-bjss commented 2 years ago

Hi, I'm sorry, I'm currently taking a small break from the project to work through personal issues. Thanks for tagging the others.

I'll look into all PRs once I feel better. 💚🙏

No problem. Thanks for letting us know. Take care of yourself and hope you feel better soon!

ndom91 commented 2 years ago

Hi james, so from my side this looks like it makes a lot of sense and the PR looks clean as well. I dont want to speak for Balazs and we're waiting to release any versions on him anyway, but for the time being you could use something like patch-package to get your own copy up and running with this patch in it.

ndom91 commented 2 years ago

Thanks James! I'll mark this as "ready" so we can take a look as soon as we continue with cutting releases :+1:

kd-tony commented 2 years ago

@ndom91 @james-bjss We're also facing the same issues with this missing nouce check type. Any updates on this PR and when it will be a candidate for a release ?

james-bjss commented 2 years ago

@ndom91 @james-bjss We're also facing the same issues with this missing nouce check type. Any updates on this PR and when it will be a candidate for a release ?

Hi Sorry, I don't have any news I am also keen to see this merged too. We are currently using the code form this PR with patch-package method suggested above.

w0otness commented 2 years ago

can you share the patch-package you use?

ndom91 commented 2 years ago

can you share the patch-package you use?

Probably this one: https://www.npmjs.com/package/patch-package

lpanjwani commented 2 years ago

I have created a patch package on NPM until this PR has been merged.

https://www.npmjs.com/package/next-auth-patch-feature-nonce-check

w0otness commented 2 years ago

I currently use this patches/openid-client+5.1.6.patch which disables the nonce check and bypasses the nonce error, and the sign in process is seamless.

I'll try replacing the entire package with the patched one (though a gist containing the .patch would be preferable) to see if I get the extra protection of the nonce.

hamidbjss commented 2 years ago

I currently use this patches/openid-client+5.1.6.patch which disables the nonce check and bypasses the nonce error, and the sign in process is seamless.

I'll try replacing the entire package with the patched one (though a gist containing the .patch would be preferable) to see if I get the extra protection of the nonce.

@w0otness I created a gist of the patch we use: https://gist.github.com/hamidbjss/b6408e48080f247edd22ec04a3b983e3

vercel[bot] commented 2 years ago

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

1 Ignored Deployment | Name | Status | Preview | Updated | | :--- | :----- | :------ | :------ | | **next-auth** | ⬜️ Ignored ([Inspect](https://vercel.com/nextauthjs/next-auth/nZszxUGcbHYw4gtUx95vhSnKMaP1)) | | Aug 16, 2022 at 8:20AM (UTC) |
iamursky commented 1 year ago

Any updates?

hamidbjss commented 1 year ago

@ThangHuuVu @balazsorban44 @ndom91 Hey, it's been some time since we last heard anything regarding this PR, wondering if you can provide an update on this please? :)

james-bjss commented 1 year ago

So I think this should be ready to be merged, thanks again @james-bjss! @balazsorban44 @ndom91 what do you think?

Thanks. Credit to @hamidbjss also, as we paired on this fix.

alaamub commented 1 year ago

@ThangHuuVu Thanks for the fix! how long does it usually take before releasing a new version of the library ?

mikejackowski commented 1 year ago

any chance on some update on when will this be live?

iamursky commented 1 year ago

any chance on some update on when will this be live?

Probably not, @ndom91 is in /dev/null 😁

hamidbjss commented 1 year ago

Created a patch for version 4.10.3 if it's of help to anyone next-auth+4.10.3.patch.

@balazsorban44 @ndom91 @ThangHuuVu @lluia really would be good if we could get this merged in please, it's a pain having to constantly create new patches.

revmischa commented 1 year ago

This error is blocking me from using a third-party IDP with Cognito nonce mismatch, expected undefined

james-bjss commented 1 year ago

This error is blocking me from using a third-party IDP with Cognito nonce mismatch, expected undefined

You can use the patch provided by @hamidbjss above until this is merged.

This PR seems to be in limbo though. Posts above do suggest that it's being considered as a candidate for a new release but it doesn't look like there is a roadmap for future releases and the only changes that are being merged currently are critical fixes and doc changes.

I understand the pressures of maintaining a popular OS project and am immensely grateful for this great library, but it's quite dissappointing when asked to raise a PR for a fix and it just languishes for 6months. We chose to stick with nextauth as we thought this would likely be fixed in the future.

@ndom91 would it be possible to get an update on why it has stalled? It's clear it is affecting a number of people who have chosen to use this lib (have seen this pop up on stack overflow too). Even if we could get a beta/branch build of the fix it would be nice. Or if it's concerns over the change, issues testing or that next release is not yet scheduled till x date etc.. Just some response to keep us updated.

Sorry for the nagging!

Also, for what its worth we have been using the patched version for many months now without issue.

james-bjss commented 1 year ago

Folks, we understand your frustration, and are sorry for the delayed action.

@james-bjss I just tested this PR locally with Google using nonce with and without state and pkce, everything works as expected. 🎉 I also added some comments, could you check and let me know what you think? Thanks again for the PR! Let's work together to get this one merged soon 🙌

Great. Thanks! Just going ahead and making these changes and we will test with Cognito + IDP (Azure AD) and will update on status.

james-bjss commented 1 year ago

Folks, we understand your frustration, and are sorry for the delayed action. @james-bjss I just tested this PR locally with Google using nonce with and without state and pkce, everything works as expected. 🎉 I also added some comments, could you check and let me know what you think? Thanks again for the PR! Let's work together to get this one merged soon 🙌

Great. Thanks! Just going ahead and making these changes and we will test with Cognito + IDP (Azure AD) and will update on status.

@ThangHuuVu - @hamidbjss and I have tested this against our Cognito setup and it works as-expected with those changes.

james-bjss commented 1 year ago

Thanks @ThangHuuVu and all. Sorry for all the nagging! This will be a huge help to our team. :star2: Have donated you all some beer money :)

mocon commented 1 year ago

Hey team, thanks for all your hard work on this! Will these changes be released in a new version?

kesoji commented 1 year ago

I'm all excited about it to be released 😊

james-bjss commented 1 year ago

This is now available in the latest release (though is absent from release notes). Will. Zap my fork in the next few days,

matewilk commented 1 year ago

This is now available in the latest release (though is absent from release notes). Will. Zap my fork in the next few days,

Hmm..that's interesting, today I installed:

"next-auth": "^4.12.3",

with the following configuration:

providers: [
    CognitoProvider({
      clientId: process.env.COGNITO_CLIENT_ID,
      clientSecret: process.env.COGNITO_CLIENT_SECRET,
      issuer: process.env.COGNITO_ISSUER,
      idToken: true,
      checks: "nonce",
    } as OAuthUserConfig<CognitoProfile>),
  ],

And I'm still getting:

providerId: 'cognito', message: 'nonce mismatch, expected undefined, got: 3C_ehdfrp6g ..... U8KNWxYvBf0f14FJYblKDvI0r-WZJ8'

Am I missing something?

kesoji commented 1 year ago

@matewilk I think checks should be array of string, not a string. Try checks: ["nonce"]

schorfES commented 1 year ago

The typings in typescript tell (assuming they are correct) that checks could be either a string literal or an array of string literals. Theoretically, both should work... I am using checks: "nonce" and don't see any of the errors @matewilk reported.

declare type ChecksType = "pkce" | "state" | "none" | "nonce";

// ...

export interface OAuthConfig<P> extends CommonProviderOptions, PartialIssuer {

  // ...

  checks?: ChecksType | ChecksType[];

  // ...

}
hamidbjss commented 1 year ago

@matewilk you should be able to provide an array or string literal.

Your config seems fine. I'd perhaps follow the usual troubleshooting steps:

  1. Clear cache in browser
  2. Remove node modules and re-install
  3. Regenerate the package-lock file and re-install node modules
mohammed-rashik-1403 commented 6 months ago

next-auth.session-token cookie is not set when I deployed to aws but works fine in vercel

mohammed-rashik-1403 commented 6 months ago

import NextAuth from 'next-auth'; import AzureADProvider from 'next-auth/providers/azure-ad'; import CognitoProvider from 'next-auth/providers/cognito';

/ Counsellor SSO login config file NextAuth with AzureADProvider / export const authOptions = { providers: [ CognitoProvider({ clientId: 241g55s6j05sqhc60pfv57if7t, clientSecret: null, issuer: https://cognito-idp.ap-southeast-1.amazonaws.com/ap-southeast-1_JtDXYUi6b, //idToken: true, client: { token_endpoint_auth_method: "none" }, checks: ["nonce","session-token"],

}),
AzureADProvider({
  clientId: `655d42ce-169d-45ed-88e0-7f23ac4ca837`,
  clientSecret: `nda8Q~GUCaBdZsm_5avtQvhRWZx7d1yZmUNooaAh`,
  tenantId: `c0c7ad79-cf13-4f39-8eeb-a1829aa096a6`,
  checks: 'both',
  authorization: {
    params: { scope: 'openid email profile User.Read  offline_access' },
  },
  httpOptions: { timeout: 10000 },
}),

], secret : "qJ8Zg4buf9bFLO2knTn40Tnqatfdm1BNVRQ0GFXCX6w=", callbacks: { async jwt({ token, user, account }) { console.log("tokeeeeeeeeeeeeeeeeee",token) console.log("tokeeeeeeaccounteeeeeeeeeeee",account) console.log("tokeeeeeeeusereeeeeeeeeee",user) return token;

},
async session({ session, token }) {
  if (session) {
    session.user = token.user;
    session.user = token.user;
    session.error = token.error;
    session.accessToken = token.accessToken;
  }
  return session;
},

async redirect({ url, baseUrl }) { return url.startsWith(baseUrl) ? url : baseUrl;

},

}, session: { jwt: true, useToken: true, cookie: { secure: true, sameSite: 'None', }, }, cookies: { sessionToken: { name: next-auth.session-token, options: { httpOnly: true, sameSite: "lax", path: "/", secure: true, domain: '.d3jsaq6594du80.cloudfront.net' } }, callbackUrl: { name: next-auth.callback-url, options: { httpOnly: true, sameSite: 'none', path: '/', secure: true } }, csrfToken: { name: 'next-auth.csrf-token', options: { httpOnly: true, sameSite: 'none', path: '/', secure: true } }, pkceCodeVerifier: { name: 'next-auth.pkce.code_verifier', options: { httpOnly: true, sameSite: 'none', path: '/', secure: true } }, state: { name: next-auth.state, options: { httpOnly: true, sameSite: 'none', path: "/", secure: true, }, }, nonce: { name: next-auth.nonce, options: { httpOnly: true, sameSite: "none", path: "/", secure: true, }, }, },

};

export default NextAuth(authOptions);

jwt callback was fine but session callback is not triiggered