Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chunked cookies should not exceed browser max #237

Merged
merged 3 commits into from
Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions lib/appSession.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -90,19 +96,17 @@ module.exports = (config) => {
res,
{ uat = epoch(), iat = uat, exp = calculateExp(iat, uat) }
) {
const cookieOptions = {
...cookieConfig,
expires: cookieConfig.transient ? 0 : new Date(exp * 1000),
};
delete cookieOptions.transient;
const cookies = req[COOKIES];
const { transient: cookieTransient , ...cookieOptions } = cookieConfig;
cookieOptions.expires = cookieTransient ? 0 : new Date(exp * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was the existing logic, but I noticed it's not correct.

If you want a session cookie expires should not be defined (it shouldn't be set to 0)

If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes

See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was the existing logic, but I noticed it's not correct.

If you want a session cookie expires should not be defined (it shouldn't be set to 0)

If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes

See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidpatrick I know this was the existing logic, but I noticed it's not correct.

If you want a session cookie expires should not be defined (it shouldn't be set to 0)

If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes

See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271

Would you mind fixing it and adding a regression test? Either in this PR or another one before you do the release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidpatrick - actually, aplogies, ignore me - I've just tested it and I get a sessoion cookie fine - so don't worry


// 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
if (!req[sessionName] || !Object.keys(req[sessionName]).length) {
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,
Expand All @@ -115,25 +119,44 @@ module.exports = (config) => {
'found session, creating signed session cookie(s) with name %o(.i)',
sessionName
);

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 / cookieChunkSize);

if (chunkCount > 1) {
debug('cookie size greater than %d, chunking', CHUNK_BYTE_SIZE);
debug('cookie size greater than %d, chunking', cookieChunkSize);
for (let i = 0; i < chunkCount; i++) {
const chunkValue = value.slice(
i * CHUNK_BYTE_SIZE,
(i + 1) * CHUNK_BYTE_SIZE
i * cookieChunkSize,
(i + 1) * cookieChunkSize
);

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
});
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
81 changes: 81 additions & 0 deletions test/appSession.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,87 @@ 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();

await request.post('session', {
baseUrl,
jar,
json: {
sub: '__test_sub__',
random: crypto.randomBytes(8000).toString('base64'),
},
});

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 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();
Expand Down