[SDK-2155] Default cookie.secure config to the protocol of baseURL (#159

) * Default the `cookie.secure` config to the protocol of the `baseUrl` config * Update FAQ * Apply suggestions from code review Co-authored-by: Filip Skokan <[email protected]> Co-authored-by: Filip Skokan <[email protected]>
auth0 · Dec 7, 2020 · df1d82b · df1d82b
1 parent 91c5c2e
commit df1d82b
Show file tree

Hide file tree

Showing 15 changed files with 160 additions and 29 deletions.
diff --git a/FAQ.md 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -130,15 +130,36 @@ 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',
       cookie: {
         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
@@ -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
@@ -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
@@ -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
@@ -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',
       });