diff --git a/FAQ.md b/FAQ.md new file mode 100644 index 00000000..20115e38 --- /dev/null +++ b/FAQ.md @@ -0,0 +1,13 @@ +# FAQs + +## I'm getting the warning "Using 'form_post' for response_mode may cause issues for you logging in over http" on localhost + +If you use `form_post` response mode (the default for this library) you are relying on a cross-site POST request with cookies - these will only be attached to the POST request if they were set with `SameSite=None; Secure` properties. + +If your server is running over `http:` protocol, your cookie with the `Secure` property will not be attached under current browser SameSite behavior. + +However, there is [an exception](<(https://www.chromestatus.com/feature/5088147346030592)>) for "Lax+POST" that Chrome makes for such cookies for the first 2 minutes after they are stored. This means that your login requests will work in Chrome over `http` as long as the end-user takes less than 2 minutes to authenticate, otherwise it will fail. This special exception will be phased out in future Chrome releases. + +This should not be an issue in production, because your application will be running over `https` + +To resolve this, you should [run your local development server over https](https://auth0.com/docs/libraries/secure-local-development). diff --git a/README.md b/README.md index 9b776a36..d4941861 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ Express JS middleware implementing sign on for Express web apps using OpenID Con - [Architecture](./ARCHITECTURE.md) - [Contributing](#contributing) - [Troubleshooting](./TROUBLESHOOTING.md) +- [FAQs](./FAQ.md) - [Support + Feedback](#support--feedback) - [Vulnerability Reporting](#vulnerability-reporting) - [What is Auth0](#what-is-auth0) diff --git a/index.d.ts b/index.d.ts index 5d68677e..6a6daaf3 100644 --- a/index.d.ts +++ b/index.d.ts @@ -459,7 +459,7 @@ interface CookieConfigParams { /** * Marks the cookie to be used over secure channels only. * Passed to the [Response cookie](https://expressjs.com/en/api.html#res.cookie) as `secure`. - * Defaults to {@link Request.secure}. + * Defaults to the protocol of {@link ConfigParams.baseURL}. */ secure?: boolean; diff --git a/lib/appSession.js b/lib/appSession.js index ac0adee5..14e16bb1 100644 --- a/lib/appSession.js +++ b/lib/appSession.js @@ -91,10 +91,6 @@ module.exports = (config) => { const cookieOptions = { ...cookieConfig, expires: cookieConfig.transient ? 0 : new Date(exp * 1000), - secure: - typeof cookieConfig.secure === 'boolean' - ? cookieConfig.secure - : req.secure, }; delete cookieOptions.transient; diff --git a/lib/config.js b/lib/config.js index 17613ebb..aa135b59 100644 --- a/lib/config.js +++ b/lib/config.js @@ -2,6 +2,8 @@ const Joi = require('@hapi/joi'); const clone = require('clone'); const { defaultState: getLoginState } = require('./hooks/getLoginState'); +const isHttps = /^https:/i; + const paramsSchema = Joi.object({ secret: Joi.alternatives([ Joi.string().min(8), @@ -45,7 +47,23 @@ const paramsSchema = Joi.object({ .valid('Lax', 'Strict', 'None') .optional() .default('Lax'), - secure: Joi.boolean().optional(), + secure: Joi.when(Joi.ref('/baseURL'), { + is: Joi.string().pattern(isHttps), + then: Joi.boolean() + .default(true) + .custom((value, { warn }) => { + if (!value) warn('insecure.cookie'); + return value; + }) + .messages({ + 'insecure.cookie': + "Setting your cookie to insecure when over https is not recommended, I hope you know what you're doing.", + }), + otherwise: Joi.boolean().valid(false).default(false).messages({ + 'any.only': + 'Cookies set with the `Secure` property wont be attached to http requests', + }), + }), path: Joi.string().uri({ relativeOnly: true }).optional(), }) .default() @@ -74,7 +92,16 @@ const paramsSchema = Joi.object({ .optional() .unknown(true) .default(), - baseURL: Joi.string().uri().required(), + baseURL: Joi.string() + .uri() + .required() + .when(Joi.ref('authorizationParams.response_mode'), { + is: 'form_post', + then: Joi.string().pattern(isHttps).rule({ + warn: true, + message: `Using 'form_post' for response_mode may cause issues for you logging in over http, see https://github.com/auth0/express-openid-connect/blob/master/FAQ.md`, + }), + }), clientID: Joi.string().required(), clientSecret: Joi.string() .when( @@ -169,10 +196,13 @@ module.exports.get = function (params) { ...config, }; - const paramsValidation = paramsSchema.validate(config); - if (paramsValidation.error) { - throw new TypeError(paramsValidation.error.details[0].message); + const { value, error, warning } = paramsSchema.validate(config); + if (error) { + throw new TypeError(error.details[0].message); + } + if (warning) { + console.warn(warning.message); } - return paramsValidation.value; + return value; }; diff --git a/lib/transientHandler.js b/lib/transientHandler.js index 2a856da1..d952c073 100644 --- a/lib/transientHandler.js +++ b/lib/transientHandler.js @@ -94,7 +94,7 @@ class TransientCookieHandler { const { domain, path, secure } = this.sessionCookieConfig; const basicAttr = { httpOnly: true, - secure: typeof secure === 'boolean' ? secure : req.secure, + secure, domain, path, }; diff --git a/middleware/attemptSilentLogin.js b/middleware/attemptSilentLogin.js index 5c698d6e..88715e8f 100644 --- a/middleware/attemptSilentLogin.js +++ b/middleware/attemptSilentLogin.js @@ -14,7 +14,7 @@ const cancelSilentLogin = (req, res) => { } = weakRef(req.oidc); res.cookie(COOKIE_NAME, true, { httpOnly: true, - secure: typeof secure === 'boolean' ? secure : req.secure, + secure, domain, path, }); diff --git a/test/appSession.tests.js b/test/appSession.tests.js index bc093361..91cb875c 100644 --- a/test/appSession.tests.js +++ b/test/appSession.tests.js @@ -16,7 +16,7 @@ const defaultConfig = { clientID: '__test_client_id__', clientSecret: '__test_client_secret__', issuerBaseURL: 'https://op.example.com', - baseURL: 'https://example.org', + baseURL: 'http://example.org', secret: '__test_secret__', errorOnRequiredAuth: true, }; @@ -166,8 +166,10 @@ describe('appSession', () => { assert.equal(res.statusCode, 200); }); - it('should set the default cookie options', async () => { - server = await createServer(appSession(getConfig(defaultConfig))); + it('should set the default cookie options over http', async () => { + server = await createServer( + appSession(getConfig({ ...defaultConfig, baseURL: 'http://example.org' })) + ); const jar = request.jar(); await request.get('/session', { baseUrl, @@ -190,6 +192,25 @@ describe('appSession', () => { assert.approximately(Math.floor((expDate - now) / 1000), 86400, 5); }); + it('should set the default cookie options over https', async () => { + server = await createServer( + appSession( + getConfig({ ...defaultConfig, baseURL: 'https://example.org' }) + ) + ); + const jar = request.jar(); + await request.get('/session', { + baseUrl, + json: true, + jar, + headers: { + cookie: `appSession=${encrypted}`, + }, + }); + // Secure cookies not set over http + assert.isEmpty(jar.getCookies(baseUrl)); + }); + it('should set the custom cookie options', async () => { server = await createServer( appSession( diff --git a/test/attemptSilentLogin.tests.js b/test/attemptSilentLogin.tests.js index 17fa27c7..1046ea75 100644 --- a/test/attemptSilentLogin.tests.js +++ b/test/attemptSilentLogin.tests.js @@ -13,7 +13,7 @@ const baseUrl = 'http://localhost:3000'; const defaultConfig = { secret: '__test_session_secret__', clientID: '__test_client_id__', - baseURL: 'https://example.org', + baseURL: 'http://example.org', issuerBaseURL: 'https://op.example.com', }; diff --git a/test/callback.tests.js b/test/callback.tests.js index 18abd6e7..0561165a 100644 --- a/test/callback.tests.js +++ b/test/callback.tests.js @@ -19,7 +19,7 @@ const baseUrl = 'http://localhost:3000'; const defaultConfig = { secret: '__test_session_secret__', clientID: clientID, - baseURL: 'https://example.org', + baseURL: 'http://example.org', issuerBaseURL: 'https://op.example.com', authRequired: false, }; diff --git a/test/config.tests.js b/test/config.tests.js index f288cc5a..0369ed69 100644 --- a/test/config.tests.js +++ b/test/config.tests.js @@ -130,8 +130,28 @@ describe('get config', () => { }); }); - it('should set default app session configuration', () => { - const config = getConfig(defaultConfig); + it('should set default app session configuration for http', () => { + const config = getConfig({ + ...defaultConfig, + baseURL: 'http://example.com', + }); + assert.deepInclude(config.session, { + rollingDuration: 86400, + name: 'appSession', + cookie: { + sameSite: 'Lax', + httpOnly: true, + transient: false, + secure: false, + }, + }); + }); + + it('should set default app session configuration for https', () => { + const config = getConfig({ + ...defaultConfig, + baseURL: 'https://example.com', + }); assert.deepInclude(config.session, { rollingDuration: 86400, name: 'appSession', @@ -139,6 +159,7 @@ describe('get config', () => { sameSite: 'Lax', httpOnly: true, transient: false, + secure: true, }, }); }); @@ -177,6 +198,40 @@ describe('get config', () => { }); }); + it('should fail when the baseURL is http and cookie is secure', function () { + assert.throws(() => { + getConfig({ + ...defaultConfig, + baseURL: 'http://example.com', + session: { cookie: { secure: true } }, + }); + }, 'Cookies set with the `Secure` property wont be attached to http requests'); + }); + + it('should warn when the baseURL is https and cookie is not secure', function () { + getConfig({ + ...defaultConfig, + baseURL: 'https://example.com', + session: { cookie: { secure: false } }, + }); + sinon.assert.calledWith( + console.warn, + "Setting your cookie to insecure when over https is not recommended, I hope you know what you're doing." + ); + }); + + it('should warn when the baseURL is http and response_mode is form_post', function () { + getConfig({ + ...defaultConfig, + baseURL: 'http://example.com', + authorizationParams: { response_mode: 'form_post' }, + }); + sinon.assert.calledWith( + console.warn, + "Using 'form_post' for response_mode may cause issues for you logging in over http, see https://github.com/auth0/express-openid-connect/blob/master/FAQ.md" + ); + }); + it('should fail when the baseURL is invalid', function () { assert.throws(() => { getConfig({ diff --git a/test/logout.tests.js b/test/logout.tests.js index 10083273..f875c082 100644 --- a/test/logout.tests.js +++ b/test/logout.tests.js @@ -10,7 +10,7 @@ const request = require('request-promise-native').defaults({ const defaultConfig = { clientID: '__test_client_id__', - baseURL: 'https://example.org', + baseURL: 'http://example.org', issuerBaseURL: 'https://op.example.com', secret: '__test_session_secret__', authRequired: false, @@ -71,7 +71,7 @@ describe('logout route', async () => { assert.include( response.headers, { - location: 'https://example.org', + location: 'http://example.org', }, 'should redirect to the base url' ); @@ -92,7 +92,7 @@ describe('logout route', async () => { assert.include( response.headers, { - location: `https://op.example.com/session/end?post_logout_redirect_uri=https%3A%2F%2Fexample.org&id_token_hint=${makeIdToken()}`, + location: `https://op.example.com/session/end?post_logout_redirect_uri=http%3A%2F%2Fexample.org&id_token_hint=${makeIdToken()}`, }, 'should redirect to the identity provider' ); @@ -116,7 +116,7 @@ describe('logout route', async () => { response.headers, { location: - 'https://op.example.com/v2/logout?returnTo=https%3A%2F%2Fexample.org&client_id=__test_client_id__', + 'https://op.example.com/v2/logout?returnTo=http%3A%2F%2Fexample.org&client_id=__test_client_id__', }, 'should redirect to the identity provider' ); @@ -139,7 +139,7 @@ describe('logout route', async () => { assert.include( response.headers, { - location: 'https://example.org/after-logout-in-auth-config', + location: 'http://example.org/after-logout-in-auth-config', }, 'should redirect to postLogoutRedirect' ); diff --git a/test/requiresAuth.tests.js b/test/requiresAuth.tests.js index a386562c..25c8bee8 100644 --- a/test/requiresAuth.tests.js +++ b/test/requiresAuth.tests.js @@ -20,7 +20,7 @@ const baseUrl = 'http://localhost:3000'; const defaultConfig = { secret: '__test_session_secret__', clientID: '__test_client_id__', - baseURL: 'https://example.org', + baseURL: 'http://example.org', issuerBaseURL: 'https://op.example.com', }; diff --git a/test/setup.js b/test/setup.js index a06e18b9..dc182014 100644 --- a/test/setup.js +++ b/test/setup.js @@ -1,8 +1,12 @@ const nock = require('nock'); +const sinon = require('sinon'); const wellKnown = require('./fixture/well-known.json'); const certs = require('./fixture/cert'); +let warn; + beforeEach(function () { + warn = sinon.stub(global.console, 'warn'); nock('https://op.example.com', { allowUnmocked: true }) .persist() .get('/.well-known/openid-configuration') @@ -26,4 +30,5 @@ beforeEach(function () { afterEach(function () { nock.cleanAll(); + warn.restore(); }); diff --git a/test/transientHandler.tests.js b/test/transientHandler.tests.js index a3bebb58..47b3fa81 100644 --- a/test/transientHandler.tests.js +++ b/test/transientHandler.tests.js @@ -41,11 +41,21 @@ describe('transientHandler', function () { sinon.assert.calledWithMatch(res.cookie, '_test_key', re); }); - it('should use the req.secure property to automatically set cookies secure when on https', function () { - transientHandler.store('test_key', { secure: true }, res, { + it('should use the config.secure property to automatically set cookies secure', function () { + const transientHandlerHttps = new TransientCookieHandler({ + secret, + session: { cookie: { secure: true } }, + legacySameSiteCookie: true, + }); + const transientHandlerHttp = new TransientCookieHandler({ + secret, + session: { cookie: { secure: false } }, + legacySameSiteCookie: true, + }); + transientHandlerHttps.store('test_key', {}, res, { sameSite: 'Lax', }); - transientHandler.store('test_key', { secure: false }, res, { + transientHandlerHttp.store('test_key', {}, res, { sameSite: 'Lax', });