-
Notifications
You must be signed in to change notification settings - Fork 4k
feat(provider): re-add state, expand protection provider options #1184
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
3d876a5
refactor: move OAuthCallbackError to errors file
balazsorban44 fb2dd83
refactor: improve pkce handling
balazsorban44 4c236d6
feat(provider): re-introduce state to provider options
balazsorban44 4ca03d1
docs(provider): mention protection options "state" and "none"
balazsorban44 2b14dca
docs(provider): document state property deprecation
balazsorban44 947c881
fix: only add code_verifier param if protection is pkce
balazsorban44 b1e4f19
docs: explain state deprecation better
balazsorban44 e1d5b60
chore: unify string
balazsorban44 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,8 @@ import * as routes from './routes' | |
| import renderPage from './pages' | ||
| import csrfTokenHandler from './lib/csrf-token-handler' | ||
| import createSecret from './lib/create-secret' | ||
| import * as pkce from './lib/pkce-handler' | ||
| import * as pkce from './lib/oauth/pkce-handler' | ||
| import * as state from './lib/oauth/state-handler' | ||
|
|
||
| // To work properly in production with OAuth providers the NEXTAUTH_URL | ||
| // environment variable must be set. | ||
|
|
@@ -63,6 +64,13 @@ async function NextAuthHandler (req, res, userOptions) { | |
| const providers = parseProviders({ providers: userOptions.providers, baseUrl, basePath }) | ||
| const provider = providers.find(({ id }) => id === providerId) | ||
|
|
||
| if (provider && | ||
| provider.type === 'oauth' && provider.version?.startsWith('2') && | ||
| (!provider.protection && provider.state !== false) | ||
| ) { | ||
| provider.protection = 'state' // Default to state, as we did in 3.1 REVIEW: should we use "pkce" or "none" as default? | ||
|
Member
Author
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. @iaincollins should we default to something else than "state"? |
||
| } | ||
|
|
||
| const maxAge = 30 * 24 * 60 * 60 // Sessions expire after 30 days of being idle | ||
|
|
||
| // Parse database / adapter | ||
|
|
@@ -145,9 +153,8 @@ async function NextAuthHandler (req, res, userOptions) { | |
| return render.signout() | ||
| case 'callback': | ||
| if (provider) { | ||
| const error = await pkce.handleCallback(req, res) | ||
| if (error) return res.redirect(error) | ||
|
|
||
| if (await pkce.handleCallback(req, res)) return | ||
| if (await state.handleCallback(req, res)) return | ||
| return routes.callback(req, res) | ||
| } | ||
| break | ||
|
|
@@ -184,9 +191,8 @@ async function NextAuthHandler (req, res, userOptions) { | |
| case 'signin': | ||
| // Verified CSRF Token required for all sign in routes | ||
| if (csrfTokenVerified && provider) { | ||
| const error = await pkce.handleSignin(req, res) | ||
| if (error) return res.redirect(error) | ||
|
|
||
| if (await pkce.handleSignin(req, res)) return | ||
| if (await state.handleSignin(req, res)) return | ||
| return routes.signin(req, res) | ||
| } | ||
|
|
||
|
|
@@ -204,9 +210,8 @@ async function NextAuthHandler (req, res, userOptions) { | |
| return res.redirect(`${baseUrl}${basePath}/signin?csrf=true`) | ||
| } | ||
|
|
||
| const error = await pkce.handleCallback(req, res) | ||
| if (error) return res.redirect(error) | ||
|
|
||
| if (await pkce.handleCallback(req, res)) return | ||
| if (await state.handleCallback(req, res)) return | ||
| return routes.callback(req, res) | ||
| } | ||
| break | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,16 @@ | ||
| import { createHash } from 'crypto' | ||
| import { decode as jwtDecode } from 'jsonwebtoken' | ||
| import oAuthClient from './client' | ||
| import logger from '../../../lib/logger' | ||
| class OAuthCallbackError extends Error { | ||
| constructor (message) { | ||
| super(message) | ||
| this.name = 'OAuthCallbackError' | ||
| this.message = message | ||
| } | ||
| } | ||
| import { OAuthCallbackError } from '../../../lib/errors' | ||
|
|
||
| export default async function oAuthCallback (req) { | ||
| const { provider, csrfToken, pkce } = req.options | ||
| const { provider, pkce } = req.options | ||
| const client = oAuthClient(provider) | ||
|
|
||
| if (provider.version?.startsWith('2.')) { | ||
| // The "user" object is specific to the Apple provider and is provided on first sign in | ||
| // e.g. {"name":{"firstName":"Johnny","lastName":"Appleseed"},"email":"[email protected]"} | ||
| let { code, user, state } = req.query // eslint-disable-line camelcase | ||
| // For OAuth 2.0 flows, check state returned and matches expected value | ||
| // (a hash of the NextAuth.js CSRF token). | ||
| // | ||
| // Apple does not support state verification. | ||
| if (provider.id !== 'apple') { | ||
| const expectedState = createHash('sha256').update(csrfToken).digest('hex') | ||
| if (state !== expectedState) { | ||
| throw new OAuthCallbackError('Invalid state returned from OAuth provider') | ||
| } | ||
| } | ||
| let { code, user } = req.query // eslint-disable-line camelcase | ||
|
|
||
| if (req.method === 'POST') { | ||
| try { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { createHash } from 'crypto' | ||
| import logger from '../../../lib/logger' | ||
| import { OAuthCallbackError } from '../../../lib/errors' | ||
|
|
||
| /** | ||
| * For OAuth 2.0 flows, if the provider supports state, | ||
| * check if state matches the one sent on signin | ||
| * (a hash of the NextAuth.js CSRF token). | ||
| */ | ||
| export async function handleCallback (req, res) { | ||
| const { csrfToken, provider, baseUrl, basePath } = req.options | ||
| try { | ||
| if (provider.protection !== 'state') { // Provider does not support state, nothing to do. | ||
| return | ||
| } | ||
|
|
||
| const { state } = req.query | ||
| const expectedState = createHash('sha256').update(csrfToken).digest('hex') | ||
|
|
||
| logger.debug( | ||
| 'OAUTH_CALLBACK_PROTECTION', | ||
| 'Comparing received and expected state', | ||
| { state, expectedState } | ||
| ) | ||
| if (state !== expectedState) { | ||
| throw new OAuthCallbackError('Invalid state returned from OAuth provider') | ||
| } | ||
| } catch (error) { | ||
| logger.error('STATE_ERROR', error) | ||
| return res.redirect(`${baseUrl}${basePath}/error?error=OAuthCallback`) | ||
| } | ||
| } | ||
|
|
||
| /** Adds CSRF token to the authorizationParams. */ | ||
| export async function handleSignin (req, res) { | ||
| const { provider, baseUrl, basePath, csrfToken } = req.options | ||
| try { | ||
| if (provider.protection !== 'state') { // Provider does not support state, nothing to do. | ||
| return | ||
| } | ||
|
|
||
| if ('state' in provider) { | ||
| logger.warn( | ||
| 'STATE_OPTION_DEPRECATION', | ||
| 'The `state` provider option is being replaced with `protection`. See the docs.' | ||
| ) | ||
| } | ||
|
|
||
| // A hash of the NextAuth.js CSRF token is used as the state | ||
| const state = createHash('sha256').update(csrfToken).digest('hex') | ||
|
|
||
| provider.authorizationParams = { ...provider.authorizationParams, state } | ||
| logger.debug( | ||
| 'OAUTH_CALLBACK_PROTECTION', | ||
| 'Added state to authorization params', | ||
| { state } | ||
| ) | ||
| } catch (error) { | ||
| logger.error('SIGNIN_OAUTH_ERROR', error) | ||
| return res.redirect(`${baseUrl}${basePath}/error?error=OAuthSignin`) | ||
| } | ||
| } | ||
|
|
||
| export default { handleSignin, handleCallback } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
@mmahalwy since you added this provider (#1027) can you answer me if Salesforce supports PKCE? https://tools.ietf.org/html/rfc7636