diff --git a/src/lib/errors.js b/src/lib/errors.js index f73e977826..434342c290 100644 --- a/src/lib/errors.js +++ b/src/lib/errors.js @@ -1,8 +1,7 @@ -class UnknownError extends Error { +export class UnknownError extends Error { constructor (message) { super(message) this.name = 'UnknownError' - this.message = message } toJSON () { @@ -16,26 +15,25 @@ class UnknownError extends Error { } } -class CreateUserError extends UnknownError { +export class CreateUserError extends UnknownError { constructor (message) { super(message) this.name = 'CreateUserError' - this.message = message } } // Thrown when an Email address is already associated with an account // but the user is trying an OAuth account that is not linked to it. -class AccountNotLinkedError extends UnknownError { +export class AccountNotLinkedError extends UnknownError { constructor (message) { super(message) this.name = 'AccountNotLinkedError' - this.message = message } } -module.exports = { - UnknownError, - CreateUserError, - AccountNotLinkedError +export class OAuthCallbackError extends UnknownError { + constructor (message) { + super(message) + this.name = 'OAuthCallbackError' + } } diff --git a/src/providers/apple.js b/src/providers/apple.js index 70e783b98b..172b4ff244 100644 --- a/src/providers/apple.js +++ b/src/providers/apple.js @@ -24,6 +24,7 @@ export default (options) => { privateKey: null, keyId: null }, + protection: 'none', // REVIEW: Apple does not support state, as far as I know. Can we use "pkce" then? ...options } } diff --git a/src/providers/salesforce.js b/src/providers/salesforce.js index fd3fa533bc..56ddfb9d60 100644 --- a/src/providers/salesforce.js +++ b/src/providers/salesforce.js @@ -8,7 +8,7 @@ export default (options) => { accessTokenUrl: 'https://login.salesforce.com/services/oauth2/token', authorizationUrl: 'https://login.salesforce.com/services/oauth2/authorize?response_type=code', profileUrl: 'https://login.salesforce.com/services/oauth2/userinfo', - state: false, + protection: 'none', // REVIEW: Can we use "pkce" ? profile: (profile) => { return { ...profile, diff --git a/src/server/index.js b/src/server/index.js index 6cfdcb63c7..0c6f47b30d 100644 --- a/src/server/index.js +++ b/src/server/index.js @@ -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? + } + 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 diff --git a/src/server/lib/oauth/callback.js b/src/server/lib/oauth/callback.js index c74fcd0179..8b5edad513 100644 --- a/src/server/lib/oauth/callback.js +++ b/src/server/lib/oauth/callback.js @@ -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":"johnny.appleseed@nextauth.com"} - 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 { diff --git a/src/server/lib/pkce-handler.js b/src/server/lib/oauth/pkce-handler.js similarity index 65% rename from src/server/lib/pkce-handler.js rename to src/server/lib/oauth/pkce-handler.js index ca94c555d4..1741235b3d 100644 --- a/src/server/lib/pkce-handler.js +++ b/src/server/lib/oauth/pkce-handler.js @@ -1,7 +1,8 @@ import pkceChallenge from 'pkce-challenge' -import jwt from '../../lib/jwt' -import * as cookie from '../lib/cookie' -import logger from '../../lib/logger' +import * as cookie from '../cookie' +import jwt from '../../../lib/jwt' +import logger from '../../../lib/logger' +import { OAuthCallbackError } from '../../../lib/errors' const PKCE_LENGTH = 64 const PKCE_CODE_CHALLENGE_METHOD = 'S256' // can be 'plain', not recommended https://tools.ietf.org/html/rfc7636#section-4.2 @@ -16,7 +17,7 @@ export async function handleCallback (req, res) { } if (!(cookies.pkceCodeVerifier.name in req.cookies)) { - throw new Error('The code_verifier cookie was not found.') + throw new OAuthCallbackError('The code_verifier cookie was not found.') } const pkce = await jwt.decode({ ...req.options.jwt, @@ -24,11 +25,16 @@ export async function handleCallback (req, res) { maxAge: PKCE_MAX_AGE, encryption: true }) - cookie.set(res, cookies.pkceCodeVerifier.name, null, { maxAge: 0 }) // remove PKCE after it has been used req.options.pkce = pkce + logger.debug('OAUTH_CALLBACK_PROTECTION', 'Read PKCE verifier from cookie', { + code_verifier: pkce.code_verifier, + pkceLength: PKCE_LENGTH, + method: PKCE_CODE_CHALLENGE_METHOD + }) + cookie.set(res, cookies.pkceCodeVerifier.name, null, { maxAge: 0 }) // remove PKCE after it has been used } catch (error) { - logger.error('PKCE_ERROR', error) - return `${baseUrl}${basePath}/error?error=Configuration` + logger.error('CALLBACK_OAUTH_ERROR', error) + return res.redirect(`${baseUrl}${basePath}/error?error=OAuthCallback`) } } @@ -41,10 +47,18 @@ export async function handleSignin (req, res) { } // Started login flow, add generated pkce to req.options and (encrypted) code_verifier to a cookie const pkce = pkceChallenge(PKCE_LENGTH) - req.options.pkce = { + logger.debug('OAUTH_SIGNIN_PROTECTION', 'Created PKCE challenge/verifier', { + ...pkce, + pkceLength: PKCE_LENGTH, + method: PKCE_CODE_CHALLENGE_METHOD + }) + + provider.authorizationParams = { + ...provider.authorizationParams, code_challenge: pkce.code_challenge, code_challenge_method: PKCE_CODE_CHALLENGE_METHOD } + const encryptedCodeVerifier = await jwt.encode({ ...req.options.jwt, maxAge: PKCE_MAX_AGE, @@ -58,12 +72,11 @@ export async function handleSignin (req, res) { expires: cookieExpires.toISOString(), ...cookies.pkceCodeVerifier.options }) + logger.debug('OAUTH_SIGNIN_PROTECTION', 'Created PKCE code_verifier saved in cookie') } catch (error) { - logger.error('PKCE_ERROR', error) - return `${baseUrl}${basePath}/error?error=Configuration` + logger.error('SIGNIN_OAUTH_ERROR', error) + return res.redirect(`${baseUrl}${basePath}/error?error=OAuthSignin`) } } -export default { - handleSignin, handleCallback -} +export default { handleSignin, handleCallback } diff --git a/src/server/lib/oauth/state-handler.js b/src/server/lib/oauth/state-handler.js new file mode 100644 index 0000000000..f70fb6a7c7 --- /dev/null +++ b/src/server/lib/oauth/state-handler.js @@ -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 } diff --git a/src/server/lib/signin/oauth.js b/src/server/lib/signin/oauth.js index ca46f405d0..6b73f830a6 100644 --- a/src/server/lib/signin/oauth.js +++ b/src/server/lib/signin/oauth.js @@ -1,9 +1,8 @@ import oAuthClient from '../oauth/client' -import { createHash } from 'crypto' import logger from '../../../lib/logger' export default async function getAuthorizationUrl (req) { - const { provider, csrfToken, pkce } = req.options + const { provider } = req.options const client = oAuthClient(provider) if (provider.version?.startsWith('2.')) { @@ -11,11 +10,8 @@ export default async function getAuthorizationUrl (req) { let url = client.getAuthorizeUrl({ ...provider.authorizationParams, ...req.body.authorizationParams, - ...pkce, redirect_uri: provider.callbackUrl, - scope: provider.scope, - // A hash of the NextAuth.js CSRF token is used as the state - state: createHash('sha256').update(csrfToken).digest('hex') + scope: provider.scope }) // If the authorizationUrl specified in the config has query parameters on it diff --git a/www/docs/configuration/providers.md b/www/docs/configuration/providers.md index 645039ca9e..cb6452944b 100644 --- a/www/docs/configuration/providers.md +++ b/www/docs/configuration/providers.md @@ -114,25 +114,26 @@ providers: [ ### OAuth provider options -| Name | Description | Type | Required | -| :-----------------: | :---------------------------------------------------------: | :-----------------------------: | :------: | -| id | Unique ID for the provider | `string` | Yes | -| name | Descriptive name for the provider | `string` | Yes | -| type | Type of provider, in this case it should be `oauth` | `oauth`, `email`, `credentials` | Yes | -| version | OAuth version (e.g. '1.0', '1.0a', '2.0') | `string` | Yes | -| accessTokenUrl | Endpoint to retrieve an access token | `string` | Yes | -| authorizationUrl | Endpoint to request authorization from the user | `string` | Yes | -| clientId | Client ID of the OAuth provider | `string` | Yes | -| clientSecret | Client Secret of the OAuth provider | `string` | No | -| scope | OAuth access scopes (expects array or string) | `string` or `string[]` | No | -| params | Additional authorization URL parameters | `object` | No | -| requestTokenUrl | Endpoint to retrieve a request token | `string` | No | -| authorizationParams | Additional params to be sent to the authorization endpoint | `object` | No | -| profileUrl | Endpoint to retrieve the user's profile | `string` | No | -| profile | An callback returning an object with the user's info | `object` | No | -| idToken | Set to `true` for services that use ID Tokens (e.g. OpenID) | `boolean` | No | -| headers | Any headers that should be sent to the OAuth provider | `object` | No | -| protection | Additional security for OAuth login flows | `pkce` | No | +| Name | Description | Type | Required | +| :-----------------: | :--------------------------------------------------------------: | :-----------------------------: | :------: | +| id | Unique ID for the provider | `string` | Yes | +| name | Descriptive name for the provider | `string` | Yes | +| type | Type of provider, in this case it should be `oauth` | `oauth`, `email`, `credentials` | Yes | +| version | OAuth version (e.g. '1.0', '1.0a', '2.0') | `string` | Yes | +| accessTokenUrl | Endpoint to retrieve an access token | `string` | Yes | +| authorizationUrl | Endpoint to request authorization from the user | `string` | Yes | +| clientId | Client ID of the OAuth provider | `string` | Yes | +| clientSecret | Client Secret of the OAuth provider | `string` | No | +| scope | OAuth access scopes (expects array or string) | `string` or `string[]` | No | +| params | Additional authorization URL parameters | `object` | No | +| requestTokenUrl | Endpoint to retrieve a request token | `string` | No | +| authorizationParams | Additional params to be sent to the authorization endpoint | `object` | No | +| profileUrl | Endpoint to retrieve the user's profile | `string` | No | +| profile | An callback returning an object with the user's info | `object` | No | +| idToken | Set to `true` for services that use ID Tokens (e.g. OpenID) | `boolean` | No | +| headers | Any headers that should be sent to the OAuth provider | `object` | No | +| protection | Additional security for OAuth login flows (defaults to `state`) | `pkce`, `state`, `none` | No | +| state | Same as `protection: "state"`. Being deprecated, use protection. | `boolean` | No | ## Sign in with Email diff --git a/www/docs/warnings.md b/www/docs/warnings.md index 557c3eac72..fbc10a6943 100644 --- a/www/docs/warnings.md +++ b/www/docs/warnings.md @@ -67,4 +67,11 @@ To remedy this, simply return the url instead: ```js return "/some/url" -``` \ No newline at end of file +``` + + +#### STATE_OPTION_DEPRECATION +You provided `state: true` or `state: false` as a provider option. This is being deprecated in a later release in favour of `protection: "state"` and `protection: "none"` respectively. To remedy this warning: + +- If you use `state: true`, just simply remove it. The default is `protection: "state"` already.. +- If you use `state: false`, set `protection: "none"`. \ No newline at end of file