Refactor/dedupe cookie/session logic (transloadit#4420)

* refactor/dedupe cookie logic so that we also send sameSite for session cookies this might fix the issue where sessions are recreated for every single request * "fix" test * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> --------- Co-authored-by: Antoine du Hamel <[email protected]>
docsend · Apr 24, 2023 · 354cc30 · 354cc30
1 parent fdb8293
commit 354cc30
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 20 deletions.
diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js
@@ -48,9 +48,11 @@ module.exports.verifyEncryptedToken = (token, secret) => {
   }
 }
 
-const addToCookies = (res, token, companionOptions, authProvider, prefix) => {
+const getCookieName = (authProvider) => `uppyAuthToken--${authProvider}`
+
+function getCookieOptions (companionOptions) {
   const cookieOptions = {
-    maxAge: 1000 * EXPIRY, // would expire after one day (24 hrs)
+    maxAge: 1000 * EXPIRY,
     httpOnly: true,
   }
 
@@ -64,10 +66,12 @@ const addToCookies = (res, token, companionOptions, authProvider, prefix) => {
   if (companionOptions.cookieDomain) {
     cookieOptions.domain = companionOptions.cookieDomain
   }
-  // send signed token to client.
-  res.cookie(`${prefix}--${authProvider}`, token, cookieOptions)
+
+  return cookieOptions
 }
 
+module.exports.getCookieOptions = getCookieOptions
+
 /**
  *
  * @param {object} res
@@ -76,7 +80,10 @@ const addToCookies = (res, token, companionOptions, authProvider, prefix) => {
  * @param {string} authProvider
  */
 module.exports.addToCookies = (res, token, companionOptions, authProvider) => {
-  addToCookies(res, token, companionOptions, authProvider, 'uppyAuthToken')
+  const cookieOptions = getCookieOptions(companionOptions)
+
+  // send signed token to client.
+  res.cookie(getCookieName(authProvider), token, cookieOptions)
 }
 
 /**
@@ -86,14 +93,10 @@ module.exports.addToCookies = (res, token, companionOptions, authProvider) => {
  * @param {string} authProvider
  */
 module.exports.removeFromCookies = (res, companionOptions, authProvider) => {
-  const cookieOptions = {
-    maxAge: 1000 * EXPIRY, // would expire after one day (24 hrs)
-    httpOnly: true,
-  }
-
-  if (companionOptions.cookieDomain) {
-    cookieOptions.domain = companionOptions.cookieDomain
-  }
+  // https://expressjs.com/en/api.html
+  // Web browsers and other compliant clients will only clear the cookie if the given options is
+  // identical to those given to res.cookie(), excluding expires and maxAge.
+  const cookieOptions = getCookieOptions(companionOptions)
 
-  res.clearCookie(`uppyAuthToken--${authProvider}`, cookieOptions)
+  res.clearCookie(getCookieName(authProvider), cookieOptions)
 }
diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js
@@ -11,6 +11,7 @@ const logger = require('../server/logger')
 const redis = require('../server/redis')
 const companion = require('../companion')
 const { getCompanionOptions, generateSecret, buildHelpfulStartupMessage } = require('./helper')
+const { getCookieOptions } = require('../server/helpers/jwt')
 
 /**
  * Configures an Express app for running Companion standalone
@@ -116,12 +117,7 @@ module.exports = function server (inputCompanionOptions) {
     sessionOptions.store = new RedisStore({ client: redisClient, prefix: process.env.COMPANION_REDIS_EXPRESS_SESSION_PREFIX || 'sess:' })
   }
 
-  if (process.env.COMPANION_COOKIE_DOMAIN) {
-    sessionOptions.cookie = {
-      domain: process.env.COMPANION_COOKIE_DOMAIN,
-      maxAge: 24 * 60 * 60 * 1000, // 1 day
-    }
-  }
+  sessionOptions.cookie = getCookieOptions(companionOptions)
 
   // Session is used for grant redirects, so that we don't need to expose secret tokens in URLs
   // See https://github.com/transloadit/uppy/pull/1668

diff --git a/packages/@uppy/companion/test/__tests__/preauth.js b/packages/@uppy/companion/test/__tests__/preauth.js
@@ -10,6 +10,7 @@ jest.mock('../../src/server/helpers/jwt', () => {
     },
     addToCookies: () => {},
     removeFromCookies: () => {},
+    getCookieOptions: () => {},
   }
 })
-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,7 @@ jest.mock('../../src/server/helpers/jwt', () => { @@
         },
         addToCookies: () => {},
         removeFromCookies: () => {},
+        getCookieOptions: () => {},
       }
     })
@@ Expand Down @@