Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-7rg2-qxmf-hhx9
Advisory fix 1
  • Loading branch information
AudreyBudryte2 committed Dec 9, 2021
2 parents 1a0c328 + 878876c commit 5ab67ff
Show file tree
Hide file tree
Showing 4 changed files with 242 additions and 16 deletions.
50 changes: 38 additions & 12 deletions lib/appSession.js
Expand Up @@ -15,14 +15,17 @@ const debug = require('./debug')('appSession');
const epoch = () => (Date.now() / 1000) | 0;
const MAX_COOKIE_SIZE = 4096;

const REASSIGN = Symbol('reassign');
const REGENERATED_SESSION_ID = Symbol('regenerated_session_id');

function attachSessionObject(req, sessionName, value) {
Object.defineProperty(req, sessionName, {
enumerable: true,
get() {
return value;
},
set(arg) {
if (arg === null || arg === undefined) {
if (arg === null || arg === undefined || arg[REASSIGN]) {
value = arg;
} else {
throw new TypeError('session object cannot be reassigned');
Expand All @@ -32,6 +35,17 @@ function attachSessionObject(req, sessionName, value) {
});
}

function regenerateSessionStoreId(req, config) {
if (config.session.store) {
req[REGENERATED_SESSION_ID] = config.session.genid(req);
}
}

function replaceSession(req, session, config) {
session[REASSIGN] = true;
req[config.session.name] = session;
}

module.exports = (config) => {
let current;

Expand Down Expand Up @@ -175,7 +189,7 @@ module.exports = (config) => {
};
}

setCookie(id, req, res, iat) {
setCookie(req, res, iat) {
setCookie(req, res, iat);
}
}
Expand All @@ -197,12 +211,14 @@ module.exports = (config) => {
res,
{ uat = epoch(), iat = uat, exp = calculateExp(iat, uat) }
) {
if (!req[sessionName] || !Object.keys(req[sessionName]).length) {
if (req[COOKIES][sessionName]) {
await this._destroy(id);
}
} else {
await this._set(id, {
const hasPrevSession = !!req[COOKIES][sessionName];
const replacingPrevSession = !!req[REGENERATED_SESSION_ID];
const hasCurrentSession = req[sessionName] && Object.keys(req[sessionName]).length;
if (hasPrevSession && (replacingPrevSession || !hasCurrentSession)) {
await this._destroy(id);
}
if (hasCurrentSession) {
await this._set(req[REGENERATED_SESSION_ID] || id, {
header: { iat, uat, exp },
data: req[sessionName],
});
Expand Down Expand Up @@ -233,7 +249,8 @@ module.exports = (config) => {
}
}

const store = config.session.store
const isCustomStore = !!config.session.store;
const store = isCustomStore
? new CustomStore(config.session.store)
: new CookieStore();

Expand Down Expand Up @@ -330,11 +347,13 @@ module.exports = (config) => {
attachSessionObject(req, sessionName, {});
}

const id = existingSessionValue || generateId(req);
if (isCustomStore) {
const id = existingSessionValue || generateId(req);

onHeaders(res, () => store.setCookie(id, req, res, { iat }));
onHeaders(res, () =>
store.setCookie(req[REGENERATED_SESSION_ID] || id, req, res, { iat })
);

if (store.set) {
const { end: origEnd } = res;
res.end = async function resEnd(...args) {
try {
Expand All @@ -349,8 +368,15 @@ module.exports = (config) => {
process.nextTick(() => next(e));
}
};
} else {
onHeaders(res, () =>
store.setCookie(req, res, { iat })
);
}

return next();
};
};

module.exports.regenerateSessionStoreId = regenerateSessionStoreId;
module.exports.replaceSession = replaceSession;
28 changes: 24 additions & 4 deletions middleware/auth.js
Expand Up @@ -10,6 +10,7 @@ const attemptSilentLogin = require('./attemptSilentLogin');
const TransientCookieHandler = require('../lib/transientHandler');
const { RequestContext, ResponseContext } = require('../lib/context');
const appSession = require('../lib/appSession');
const { regenerateSessionStoreId, replaceSession } = appSession;
const { decodeState } = require('../lib/hooks/getLoginState');

const enforceLeadingSlash = (path) => {
Expand Down Expand Up @@ -83,7 +84,7 @@ const auth = function (params) {
try {
const redirectUri = res.oidc.getRedirectUri();

let session;
let tokenSet;

try {
const callbackParams = client.callbackParams(req);
Expand All @@ -110,7 +111,7 @@ const auth = function (params) {
extras = { exchangeBody: config.tokenEndpointParams };
}

session = await client.callback(
tokenSet = await client.callback(
redirectUri,
callbackParams,
checks,
Expand All @@ -120,16 +121,35 @@ const auth = function (params) {
throw createError.BadRequest(err.message);
}

let session = Object.assign({}, tokenSet); // Remove non-enumerable methods from the TokenSet

if (config.afterCallback) {
session = await config.afterCallback(
req,
res,
Object.assign({}, session), // Remove non-enumerable methods from the TokenSet
session,
req.openidState
);
}

Object.assign(req[config.session.name], session);
if (req.oidc.isAuthenticated()) {
if (req.oidc.user.sub === tokenSet.claims().sub) {
// If it's the same user logging in again, just update the existing session.
Object.assign(req[config.session.name], session);
} else {
// If it's a different user, replace the session to remove any custom user
// properties on the session
replaceSession(req, session, config);
// And regenerate the session id so the previous user wont know the new user's session id
regenerateSessionStoreId(req, config);
}
} else {
// If a new user is replacing an anonymous session, update the existing session to keep
// any anonymous session state (eg. checkout basket)
Object.assign(req[config.session.name], session);
// But update the session store id so a previous anonymous user wont know the new user's session id
regenerateSessionStoreId(req, config);
}
attemptSilentLogin.resumeSilentLogin(req, res);

next();
Expand Down
14 changes: 14 additions & 0 deletions test/appSession.customStore.tests.js
Expand Up @@ -57,6 +57,7 @@ describe('appSession custom store', () => {
const store = new RedisStore({ client: redisClient, prefix: '' });
redisClient.asyncSet = promisify(redisClient.set).bind(redisClient);
redisClient.asyncGet = promisify(redisClient.get).bind(redisClient);
redisClient.asyncDbsize = promisify(redisClient.dbsize).bind(redisClient);

const conf = getConfig({
...defaultConfig,
Expand All @@ -72,6 +73,7 @@ describe('appSession custom store', () => {
await new Promise((resolve) => server.close(resolve));
}
if (redisClient) {
await new Promise((resolve) => redisClient.flushall(resolve));
await new Promise((resolve) => redisClient.quit(resolve));
}
});
Expand Down Expand Up @@ -116,6 +118,15 @@ describe('appSession custom store', () => {
});
});

it('should not populate the store when there is no session', async () => {
await setup();
await request.get('/session', {
baseUrl,
json: true,
});
assert.equal(await redisClient.asyncDbsize(), 0);
});

it('should get a new session', async () => {
await setup();
const jar = await login({ sub: '__foo_user__' });
Expand All @@ -126,6 +137,7 @@ describe('appSession custom store', () => {
});
assert.equal(res.statusCode, 200);
assert.deepEqual(res.body, { sub: '__foo_user__' });
assert.equal(await redisClient.asyncDbsize(), 1);
});

it('should destroy an existing session', async () => {
Expand Down Expand Up @@ -153,6 +165,7 @@ describe('appSession custom store', () => {
});
assert.isEmpty(loggedOutRes.body);
assert.isEmpty(jar.getCookies(baseUrl));
assert.equal(await redisClient.asyncDbsize(), 0);
});

it('uses custom session id generator when provided', async () => {
Expand All @@ -178,6 +191,7 @@ describe('appSession custom store', () => {
role: 'test',
userid: immId,
});
assert.equal(await redisClient.asyncDbsize(), 1);
});

it('should handle storage errors', async () => {
Expand Down

0 comments on commit 5ab67ff

Please sign in to comment.