Skip to content

Commit

Permalink
fix: add regenerate method for session security (#3499)
Browse files Browse the repository at this point in the history
  • Loading branch information
czy88840616 authored Dec 14, 2023
1 parent 024a5c1 commit 74613d1
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 10 deletions.
7 changes: 6 additions & 1 deletion packages/session/src/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,15 @@ export interface ISession {
*/
manuallyCommit(): Promise<void>;

/**
* regenerate this session
*/
regenerate(callback?: () => void): void;

/**
* save this session no matter whether it is populated
*/
save(): void;
save(callback?: () => void): void;

/**
* allow to put any value on session object
Expand Down
16 changes: 11 additions & 5 deletions packages/session/src/lib/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class ContextSession {
* @api public
*/

async commit() {
async commit({ save = false, regenerate = false } = {}) {
const session = this.session;
const opts = this.opts;
const ctx = this.ctx;
Expand All @@ -237,7 +237,16 @@ export class ContextSession {
return;
}

const reason = this._shouldSaveSession();
if (regenerate) {
await this.remove();
if (this.store) this.externalKey = opts.genid && opts.genid(ctx);
}

// force save session when `session._requireSave` set
const reason =
save || regenerate || session._requireSave
? 'force'
: this._shouldSaveSession();
debug('should save session: %s', reason);
if (!reason) return;

Expand All @@ -253,9 +262,6 @@ export class ContextSession {
const prevHash = this.prevHash;
const session = this.session;

// force save session when `session._requireSave` set
if (session._requireSave) return 'force';

// do nothing if new and not populated
const json = session.toJSON();
if (!prevHash && !Object.keys(json).length) return '';
Expand Down
32 changes: 28 additions & 4 deletions packages/session/src/lib/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,19 @@ export class Session implements ISession {
* @api public
*/

save() {
this._requireSave = true;
save(callback) {
return this.commit({ save: true }, callback);
}

/**
* regenerate this session
*
* @param {Function} callback the optional function to call after regenerating the session
* @api public
*/

regenerate(callback) {
return this.commit({ regenerate: true }, callback);
}

/**
Expand All @@ -122,7 +133,20 @@ export class Session implements ISession {
* @api public
*/

async manuallyCommit() {
await this._sessCtx.commit();
manuallyCommit() {
return this.commit();
}

commit(options?, callback?) {
if (typeof options === 'function') {
callback = options;
options = {};
}
const promise = this._sessCtx.commit(options);
if (callback) {
promise.then(() => callback(), callback);
} else {
return promise;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { Configuration, Controller, Get } from '@midwayjs/core';
import * as koa from '../../../../../web-koa';
import * as assert from 'assert';

@Configuration({
imports: [
koa,
],
importConfigs: [
{
default: {
keys: '12345',
session: {
sameSite: 'none',
}
}
}
]
})
export class AutoConfiguration {
async onReady(container, app) {
const message = 'hi';
app.useMiddleware(async (ctx, next) => {
ctx.session = { message: 'hi' };
await next();
});

app.useMiddleware(async function(ctx, next) {
const sessionKey = ctx.cookies.get('MW_SESS', { signed: false });
if (sessionKey) {
await ctx.session.regenerate();
}
await next();
});

app.useMiddleware(async function(ctx) {
assert(ctx.session.message === message);
ctx.body = '';
});
}
}

@Controller('/')
export class HomeController {

@Get('/get')
async get(ctx) {
return ctx.session;
}
@Get('/set')
async set(ctx) {
ctx.session = ctx.query;
ctx.body = ctx.session;
}
}
23 changes: 23 additions & 0 deletions packages/session/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,27 @@ describe('test/index.test.ts', function () {

await close(app);
});

it('should change the session key, but not content', async () => {
const app = await createApp(join(__dirname, 'fixtures/change-session-key'));
const request = createHttpRequest(app);
let koaSession = null;
await request.get('/')
.expect(200)
.expect((res) => {
koaSession = res.headers['set-cookie'][0];
assert(koaSession.match(/MW_SESS=.*?;/));
});

await request.get('/')
.set('Cookie', koaSession)
.expect(200)
.expect((res) => {
const cookies = res.headers['set-cookie'][0];
assert(cookies.match(/MW_SESS=.*?;/));
assert(cookies.replace(/;.*/, '') !== koaSession.replace(/;.*/, ''));
});

await close(app);
});
});

0 comments on commit 74613d1

Please sign in to comment.