From fb49523aeb9be09f9d777a974a3c2bd50916de63 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 20 May 2021 11:45:40 -0700 Subject: [PATCH 1/3] Chunked cookies should not exceed browser max --- lib/appSession.js | 15 ++++++++++----- test/appSession.tests.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/lib/appSession.js b/lib/appSession.js index ae1baa4e..e64e463e 100644 --- a/lib/appSession.js +++ b/lib/appSession.js @@ -14,7 +14,7 @@ const { encryption: deriveKey } = require('./hkdf'); const debug = require('./debug')('appSession'); const epoch = () => (Date.now() / 1000) | 0; -const CHUNK_BYTE_SIZE = 4000; +const MAX_COOKIE_SIZE = 4096; function attachSessionObject(req, sessionName, value) { Object.defineProperty(req, sessionName, { @@ -115,20 +115,25 @@ module.exports = (config) => { 'found session, creating signed session cookie(s) with name %o(.i)', sessionName ); + const emptyCookie = cookie.serialize(`${sessionName}.0`, '', cookieOptions); + const chunkSize = MAX_COOKIE_SIZE - emptyCookie.length; + const value = encrypt(JSON.stringify(req[sessionName]), { iat, uat, exp, }); - const chunkCount = Math.ceil(value.length / CHUNK_BYTE_SIZE); + const chunkCount = Math.ceil(value.length / chunkSize); + if (chunkCount > 1) { - debug('cookie size greater than %d, chunking', CHUNK_BYTE_SIZE); + debug('cookie size greater than %d, chunking', chunkSize); for (let i = 0; i < chunkCount; i++) { const chunkValue = value.slice( - i * CHUNK_BYTE_SIZE, - (i + 1) * CHUNK_BYTE_SIZE + i * chunkSize, + (i + 1) * chunkSize ); + const chunkCookieName = `${sessionName}.${i}`; res.cookie(chunkCookieName, chunkValue, cookieOptions); } diff --git a/test/appSession.tests.js b/test/appSession.tests.js index f0329c9c..5e462518 100644 --- a/test/appSession.tests.js +++ b/test/appSession.tests.js @@ -122,6 +122,36 @@ describe('appSession', () => { }); }); + it('should limit total cookie size to 4096 Bytes', async () => { + const path = + '/some-really-really-really-really-really-really-really-really-really-really-really-really-really-long-path'; + + server = await createServer(appSession(getConfig({ ...defaultConfig, session: { cookie: { path } } }))); + + const jar = request.jar(); + const random = crypto.randomBytes(8000).toString('base64'); + + await request.post('session', { + baseUrl, + jar, + json: { + sub: '__test_sub__', + random, + }, + }); + + + const cookies = jar + .getCookies(`${baseUrl}${path}`) + .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); + + assert.exists(cookies); + assert.equal(cookies['appSession.0'].length, 4096); + assert.equal(cookies['appSession.1'].length, 4096); + assert.equal(cookies['appSession.2'].length, 4096); + assert.isTrue(cookies['appSession.3'].length <= 4096) + }); + it('should handle unordered chunked cookies', async () => { server = await createServer(appSession(getConfig(defaultConfig))); const jar = request.jar(); From 4d2d97bd525dbf72266d241c16ffc1822ca51287 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 20 May 2021 12:47:06 -0700 Subject: [PATCH 2/3] Clean up cookies when switching chunking vs single --- lib/appSession.js | 19 +++++++++++- test/appSession.tests.js | 65 +++++++++++++++++++++++++++++++++++----- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/lib/appSession.js b/lib/appSession.js index e64e463e..06f9c268 100644 --- a/lib/appSession.js +++ b/lib/appSession.js @@ -90,6 +90,7 @@ module.exports = (config) => { res, { uat = epoch(), iat = uat, exp = calculateExp(iat, uat) } ) { + const cookies = req[COOKIES]; const cookieOptions = { ...cookieConfig, expires: cookieConfig.transient ? 0 : new Date(exp * 1000), @@ -102,7 +103,7 @@ module.exports = (config) => { debug( 'session was deleted or is empty, clearing all matching session cookies' ); - for (const cookieName of Object.keys(req[COOKIES])) { + for (const cookieName of Object.keys(cookies)) { if (cookieName.match(`^${sessionName}(?:\\.\\d)?$`)) { res.clearCookie(cookieName, { domain: cookieOptions.domain, @@ -137,8 +138,24 @@ module.exports = (config) => { const chunkCookieName = `${sessionName}.${i}`; res.cookie(chunkCookieName, chunkValue, cookieOptions); } + if (sessionName in cookies) { + debug('replacing non chunked cookie with chunked cookies'); + res.clearCookie(sessionName, { + domain: cookieConfig.domain, + path: cookieConfig.path + }); + } } else { res.cookie(sessionName, value, cookieOptions); + for (const cookieName of Object.keys(cookies)) { + debug('replacing chunked cookies with non chunked cookies'); + if (cookieName.match(`^${sessionName}\\.\\d$`)) { + res.clearCookie(cookieName, { + domain: cookieConfig.domain, + path: cookieConfig.path + }); + } + } } } } diff --git a/test/appSession.tests.js b/test/appSession.tests.js index 5e462518..ade92183 100644 --- a/test/appSession.tests.js +++ b/test/appSession.tests.js @@ -125,24 +125,20 @@ describe('appSession', () => { it('should limit total cookie size to 4096 Bytes', async () => { const path = '/some-really-really-really-really-really-really-really-really-really-really-really-really-really-long-path'; - server = await createServer(appSession(getConfig({ ...defaultConfig, session: { cookie: { path } } }))); - const jar = request.jar(); - const random = crypto.randomBytes(8000).toString('base64'); await request.post('session', { baseUrl, jar, json: { sub: '__test_sub__', - random, + random: crypto.randomBytes(8000).toString('base64'), }, }); - - + const cookies = jar - .getCookies(`${baseUrl}${path}`) + .getCookies(`${baseUrl}${path}`) .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); assert.exists(cookies); @@ -152,6 +148,61 @@ describe('appSession', () => { assert.isTrue(cookies['appSession.3'].length <= 4096) }); + it('should clean up single cookie when switching to chunked', async () => { + server = await createServer(appSession(getConfig(defaultConfig))); + const jar = request.jar(); + jar.setCookie(`appSession=foo`, baseUrl) + + const firstCookies = jar + .getCookies(baseUrl) + .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); + assert.property(firstCookies, 'appSession') + + await request.post('session', { + baseUrl, + jar, + json: { + sub: '__test_sub__', + random: crypto.randomBytes(8000).toString('base64'), + }, + }); + + const cookies = jar + .getCookies(baseUrl) + .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); + + assert.property(cookies, 'appSession.0') + assert.notProperty(cookies, 'appSession') + }); + + it('should clean up chunked cookies when switching to single cookie', async () => { + server = await createServer(appSession(getConfig(defaultConfig))); + const jar = request.jar(); + jar.setCookie(`appSession.0=foo`, baseUrl) + jar.setCookie(`appSession.1=foo`, baseUrl) + + const firstCookies = jar + .getCookies(baseUrl) + .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); + assert.property(firstCookies, 'appSession.0') + assert.property(firstCookies, 'appSession.1') + + await request.post('session', { + baseUrl, + jar, + json: { + sub: '__test_sub__', + }, + }); + + const cookies = jar + .getCookies(baseUrl) + .reduce((obj, value) => Object.assign(obj, { [value.key]: value + '' }), {}); + + assert.property(cookies, 'appSession') + assert.notProperty(cookies, 'appSession.0') + }); + it('should handle unordered chunked cookies', async () => { server = await createServer(appSession(getConfig(defaultConfig))); const jar = request.jar(); From fb24d084f0894af33b52d23e9baa8f8d5e27e674 Mon Sep 17 00:00:00 2001 From: David Date: Fri, 28 May 2021 12:36:05 -0700 Subject: [PATCH 3/3] requested changes --- lib/appSession.js | 23 ++++++++++++----------- package.json | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/appSession.js b/lib/appSession.js index 06f9c268..12e199d8 100644 --- a/lib/appSession.js +++ b/lib/appSession.js @@ -49,6 +49,12 @@ module.exports = (config) => { rollingDuration, } = config.session; + const { transient: emptyTransient , ...emptyCookieOptions } = cookieConfig; + emptyCookieOptions.expires = emptyTransient ? 0 : new Date(); + + const emptyCookie = cookie.serialize(`${sessionName}.0`, '', emptyCookieOptions); + const cookieChunkSize = MAX_COOKIE_SIZE - emptyCookie.length; + let keystore = new JWKS.KeyStore(); secrets.forEach((secretString, i) => { @@ -91,11 +97,8 @@ module.exports = (config) => { { uat = epoch(), iat = uat, exp = calculateExp(iat, uat) } ) { const cookies = req[COOKIES]; - const cookieOptions = { - ...cookieConfig, - expires: cookieConfig.transient ? 0 : new Date(exp * 1000), - }; - delete cookieOptions.transient; + const { transient: cookieTransient , ...cookieOptions } = cookieConfig; + cookieOptions.expires = cookieTransient ? 0 : new Date(exp * 1000); // session was deleted or is empty, this matches all session cookies (chunked or unchunked) // and clears them, essentially cleaning up what we've set in the past that is now trash @@ -116,8 +119,6 @@ module.exports = (config) => { 'found session, creating signed session cookie(s) with name %o(.i)', sessionName ); - const emptyCookie = cookie.serialize(`${sessionName}.0`, '', cookieOptions); - const chunkSize = MAX_COOKIE_SIZE - emptyCookie.length; const value = encrypt(JSON.stringify(req[sessionName]), { iat, @@ -125,14 +126,14 @@ module.exports = (config) => { exp, }); - const chunkCount = Math.ceil(value.length / chunkSize); + const chunkCount = Math.ceil(value.length / cookieChunkSize); if (chunkCount > 1) { - debug('cookie size greater than %d, chunking', chunkSize); + debug('cookie size greater than %d, chunking', cookieChunkSize); for (let i = 0; i < chunkCount; i++) { const chunkValue = value.slice( - i * chunkSize, - (i + 1) * chunkSize + i * cookieChunkSize, + (i + 1) * cookieChunkSize ); const chunkCookieName = `${sessionName}.${i}`; diff --git a/package.json b/package.json index ff1b39d9..d9a243b9 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "scripts": { "lint": "eslint .", "start:example": "node ./examples/run_example.js", - "test": "mocha", + "test": "mocha --max-http-header-size=16384", "test:ci": "nyc --reporter=lcov npm test", "docs": "typedoc --options typedoc.js index.d.ts", "test:end-to-end": "mocha end-to-end"