-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat: add PKCE support #941
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
Changes from 3 commits
7c269c4
87074d0
b97591b
83e1290
56e9bd8
3f75bf8
59387ab
1509a25
d358a7f
8100953
12ef15d
8247ec4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import session from './routes/session' | |
import pages from './pages' | ||
import adapters from '../adapters' | ||
import logger from '../lib/logger' | ||
import pkceChallenge from 'pkce-challenge' | ||
|
||
// To work properly in production with OAuth providers the NEXTAUTH_URL | ||
// environment variable must be set. | ||
|
@@ -217,6 +218,8 @@ async function NextAuth (req, res, userSuppliedOptions) { | |
redirect | ||
} | ||
|
||
const pkce = options.providers[provider]?.pkce ? pkceChallenge(64) : undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Length of Also, this is created for the entire app, meaning this will be the same for all the users. This makes the whole feature much less complicated, but I am not sure if this brings any security issues. If I remember our chat earlier @iaincollins, you said that in theory, we wouldn't even need to care about PKCE. Am I right, or we should find a way to create this per user? The problem is that in that case, the values must be saved somewhere, so the challenge and verifier can be passed to both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of saving the challenge and verifier, it could be saved in local storage (maybe session) to be cleaned up after a successful log in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure we would like to store anything sensitive this way, cause it is perceptible with potentially malicious code on the client. If I understand it correctly, Maybe the way you describe it IS the way we have to go, we will see. Thanks for weighing in! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem! I inspired myself from oidc-client-js which I'm currently using in a production spa. They use Via the spec every new authorization flow should use a new In any case I've set up a simple sample to generate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory we wouldn't actually need PKCE to be secure I think, since our secrets are all server-side and never accessed by the client. So this is why I went with my implementation. Although there might be something important I leave out. We will have a talk about this soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @balazsorban44 FWIW for the record this is exactly my current understanding as well. |
||
|
||
// If debug enabled, set ENV VAR so that logger logs debug messages | ||
if (options.debug === true) { process.env._NEXTAUTH_DEBUG = true } | ||
|
||
|
@@ -250,7 +253,7 @@ async function NextAuth (req, res, userSuppliedOptions) { | |
break | ||
case 'callback': | ||
if (provider && options.providers[provider]) { | ||
callback(req, res, options, done) | ||
callback(req, res, options, done, pkce?.code_verifier) | ||
} else { | ||
res.status(400).end(`Error: HTTP GET is not supported for ${url}`) | ||
return done() | ||
|
@@ -279,7 +282,7 @@ async function NextAuth (req, res, userSuppliedOptions) { | |
} | ||
|
||
if (provider && options.providers[provider]) { | ||
signin(req, res, options, done) | ||
signin({ req, res, options, done, codeChallenge: pkce.code_challenge }) | ||
} | ||
break | ||
case 'signout': | ||
|
@@ -297,7 +300,7 @@ async function NextAuth (req, res, userSuppliedOptions) { | |
return redirect(`${baseUrl}${basePath}/signin?csrf=true`) | ||
} | ||
|
||
callback(req, res, options, done) | ||
callback(req, res, options, done, pkce?.code_verifier) | ||
} else { | ||
res.status(400).end(`Error: HTTP POST is not supported for ${url}`) | ||
return done() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,22 +13,22 @@ import logger from '../../../lib/logger' | |
|
||
// @TODO Refactor to use promises and not callbacks | ||
// @TODO Refactor to use jsonwebtoken instead of jwt-decode & remove dependancy | ||
export default async (req, provider, csrfToken, callback) => { | ||
export default async function oAuthCallback (req, provider, csrfToken, callback, codeVerifier) { | ||
// The "user" object is specific to apple provider and is provided on first sign in | ||
// e.g. {"name":{"firstName":"Johnny","lastName":"Appleseed"},"email":"[email protected]"} | ||
let { oauth_token, oauth_verifier, code, user, state } = req.query // eslint-disable-line camelcase | ||
const client = oAuthClient(provider) | ||
|
||
if (provider.version && provider.version.startsWith('2.')) { | ||
if (provider.version?.startsWith('2.')) { | ||
// For OAuth 2.0 flows, check state returned and matches expected value | ||
// (a hash of the NextAuth.js CSRF token). | ||
// | ||
// This check can be disabled for providers that do not support it by | ||
// setting `state: false` as a option on the provider (defaults to true). | ||
// setting `state: false` as an option on the provider (defaults to true). | ||
if (!Object.prototype.hasOwnProperty.call(provider, 'state') || provider.state === true) { | ||
const expectedState = createHash('sha256').update(csrfToken).digest('hex') | ||
if (state !== expectedState) { | ||
return callback(new Error('Invalid state returned from oAuth provider')) | ||
return callback(new Error('Invalid state returned from OAuth provider')) | ||
} | ||
} | ||
|
||
|
@@ -87,7 +87,7 @@ export default async (req, provider, csrfToken, callback) => { | |
} | ||
) | ||
} else { | ||
// Use custom get() method for oAuth2 flows | ||
// Use custom get() method for OAuth2 flows | ||
client.get = _get | ||
|
||
client.get( | ||
|
@@ -100,7 +100,8 @@ export default async (req, provider, csrfToken, callback) => { | |
} | ||
) | ||
} | ||
} | ||
}, | ||
codeVerifier | ||
) | ||
} else { | ||
// Handle oAuth v1.x | ||
|
@@ -187,13 +188,17 @@ async function _getProfile (error, profileData, accessToken, refreshToken, provi | |
} | ||
|
||
// Ported from https://github.com/ciaranj/node-oauth/blob/a7f8a1e21c362eb4ed2039431fb9ac2ae749f26a/lib/oauth2.js | ||
async function _getOAuthAccessToken (code, provider, callback) { | ||
async function _getOAuthAccessToken (code, provider, callback, codeVerifier) { | ||
const url = provider.accessTokenUrl | ||
const setGetAccessTokenAuthHeader = (provider.setGetAccessTokenAuthHeader !== null) ? provider.setGetAccessTokenAuthHeader : true | ||
const params = { ...provider.params } || {} | ||
const params = new URLSearchParams({ ...provider.params } || {}) | ||
const headers = { ...provider.headers } || {} | ||
const codeParam = (params.grant_type === 'refresh_token') ? 'refresh_token' : 'code' | ||
|
||
if (provider.pkce) { | ||
params.code_verifier = codeVerifier | ||
balazsorban44 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (!params[codeParam]) { params[codeParam] = code } | ||
|
||
if (!params.client_id) { params.client_id = provider.clientId } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be removed and we can create our own, added to be able to get started a bit faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openid-client
has this built-in, we can utilize it when/if we finish #1105