From bcadb3662bc53872f3826f5584dbbb6441859502 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sun, 15 Aug 2021 11:09:52 +0100 Subject: [PATCH] Server: Increase cookies security - set HttpOnly, Secure and SameSite flags --- packages/server/src/config.ts | 3 +++ packages/server/src/routes/index/login.test.ts | 7 ++++--- packages/server/src/routes/index/login.ts | 3 ++- packages/server/src/routes/index/logout.test.ts | 5 +++-- packages/server/src/routes/index/logout.ts | 3 ++- packages/server/src/routes/index/signup.test.ts | 3 ++- packages/server/src/routes/index/signup.ts | 3 ++- packages/server/src/routes/index/users.test.ts | 9 +++++---- packages/server/src/routes/index/users.ts | 3 ++- packages/server/src/utils/cookies.ts | 17 +++++++++++++++++ packages/server/src/utils/requestUtils.ts | 3 ++- packages/server/src/utils/testing/testUtils.ts | 3 ++- packages/server/src/utils/types.ts | 1 + 13 files changed, 47 insertions(+), 16 deletions(-) create mode 100644 packages/server/src/utils/cookies.ts diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index d9407a701..83829fba8 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -46,6 +46,8 @@ export interface EnvVariables { SUPPORT_NAME?: string; BUSINESS_EMAIL?: string; + + COOKIES_SECURE?: string; } let runningInDocker_: boolean = false; @@ -168,6 +170,7 @@ export async function initConfig(envType: Env, env: EnvVariables, overrides: any supportEmail, supportName: env.SUPPORT_NAME || appName, businessEmail: env.BUSINESS_EMAIL || supportEmail, + cookieSecure: env.COOKIES_SECURE === '1', ...overrides, }; } diff --git a/packages/server/src/routes/index/login.test.ts b/packages/server/src/routes/index/login.test.ts index ded8353f2..e888df3c6 100644 --- a/packages/server/src/routes/index/login.test.ts +++ b/packages/server/src/routes/index/login.test.ts @@ -1,5 +1,6 @@ import { Session } from '../../db'; import routeHandler from '../../middleware/routeHandler'; +import { cookieGet } from '../../utils/cookies'; import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, models, parseHtml, createUser } from '../../utils/testing/testUtils'; import { AppContext } from '../../utils/types'; @@ -52,7 +53,7 @@ describe('index_login', function() { const user = await createUser(1); const context = await doLogin(user.email, '123456'); - const sessionId = context.cookies.get('sessionId'); + const sessionId = cookieGet(context, 'sessionId'); const session: Session = await models().session().load(sessionId); expect(session.user_id).toBe(user.id); }); @@ -62,12 +63,12 @@ describe('index_login', function() { { const context = await doLogin('bad', '123456'); - expect(!context.cookies.get('sessionId')).toBe(true); + expect(!cookieGet(context, 'sessionId')).toBe(true); } { const context = await doLogin(user.email, 'bad'); - expect(!context.cookies.get('sessionId')).toBe(true); + expect(!cookieGet(context, 'sessionId')).toBe(true); } }); diff --git a/packages/server/src/routes/index/login.ts b/packages/server/src/routes/index/login.ts index dc400df47..64332b343 100644 --- a/packages/server/src/routes/index/login.ts +++ b/packages/server/src/routes/index/login.ts @@ -7,6 +7,7 @@ import config from '../../config'; import defaultView from '../../utils/defaultView'; import { View } from '../../services/MustacheService'; import limiterLoginBruteForce from '../../utils/request/limiterLoginBruteForce'; +import { cookieSet } from '../../utils/cookies'; function makeView(error: any = null): View { const view = defaultView('login', 'Login'); @@ -32,7 +33,7 @@ router.post('login', async (_path: SubPath, ctx: AppContext) => { const body = await formParse(ctx.req); const session = await ctx.joplin.models.session().authenticate(body.fields.email, body.fields.password); - ctx.cookies.set('sessionId', session.id); + cookieSet(ctx, 'sessionId', session.id); return redirect(ctx, `${config().baseUrl}/home`); } catch (error) { return makeView(error); diff --git a/packages/server/src/routes/index/logout.test.ts b/packages/server/src/routes/index/logout.test.ts index 015fc0e80..4b246bc4e 100644 --- a/packages/server/src/routes/index/logout.test.ts +++ b/packages/server/src/routes/index/logout.test.ts @@ -1,4 +1,5 @@ import routeHandler from '../../middleware/routeHandler'; +import { cookieGet } from '../../utils/cookies'; import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, models, createUserAndSession } from '../../utils/testing/testUtils'; describe('index_logout', function() { @@ -26,11 +27,11 @@ describe('index_logout', function() { }, }); - expect(context.cookies.get('sessionId')).toBe(session.id); + expect(cookieGet(context, 'sessionId')).toBe(session.id); expect(!!(await models().session().load(session.id))).toBe(true); await routeHandler(context); - expect(!context.cookies.get('sessionId')).toBe(true); + expect(!cookieGet(context, 'sessionId')).toBe(true); expect(!!(await models().session().load(session.id))).toBe(false); }); diff --git a/packages/server/src/routes/index/logout.ts b/packages/server/src/routes/index/logout.ts index 53237c394..148797bc4 100644 --- a/packages/server/src/routes/index/logout.ts +++ b/packages/server/src/routes/index/logout.ts @@ -4,12 +4,13 @@ import { RouteType } from '../../utils/types'; import { AppContext } from '../../utils/types'; import config from '../../config'; import { contextSessionId } from '../../utils/requestUtils'; +import { cookieSet } from '../../utils/cookies'; const router = new Router(RouteType.Web); router.post('logout', async (_path: SubPath, ctx: AppContext) => { const sessionId = contextSessionId(ctx, false); - ctx.cookies.set('sessionId', ''); + cookieSet(ctx, 'sessionId', ''); await ctx.joplin.models.session().logout(sessionId); return redirect(ctx, `${config().baseUrl}/login`); }); diff --git a/packages/server/src/routes/index/signup.test.ts b/packages/server/src/routes/index/signup.test.ts index 51b96cba5..319bbc4d6 100644 --- a/packages/server/src/routes/index/signup.test.ts +++ b/packages/server/src/routes/index/signup.test.ts @@ -3,6 +3,7 @@ import { NotificationKey } from '../../models/NotificationModel'; import { AccountType } from '../../models/UserModel'; import { getCanShareFolder, getMaxItemSize } from '../../models/utils/user'; import { MB } from '../../utils/bytes'; +import { cookieGet } from '../../utils/cookies'; import { execRequestC } from '../../utils/testing/apiUtils'; import { beforeAllDb, afterAllTests, beforeEachDb, models } from '../../utils/testing/testUtils'; import uuidgen from '../../utils/uuidgen'; @@ -50,7 +51,7 @@ describe('index_signup', function() { expect(getMaxItemSize(user)).toBe(10 * MB); // Check that the user is logged in - const session = await models().session().load(context.cookies.get('sessionId')); + const session = await models().session().load(cookieGet(context, 'sessionId')); expect(session.user_id).toBe(user.id); // Check that the notification has been created diff --git a/packages/server/src/routes/index/signup.ts b/packages/server/src/routes/index/signup.ts index d71331398..8e2d24e33 100644 --- a/packages/server/src/routes/index/signup.ts +++ b/packages/server/src/routes/index/signup.ts @@ -10,6 +10,7 @@ import { checkRepeatPassword } from './users'; import { NotificationKey } from '../../models/NotificationModel'; import { AccountType } from '../../models/UserModel'; import { ErrorForbidden } from '../../utils/errors'; +import { cookieSet } from '../../utils/cookies'; function makeView(error: Error = null): View { const view = defaultView('signup', 'Sign Up'); @@ -51,7 +52,7 @@ router.post('signup', async (_path: SubPath, ctx: AppContext) => { }); const session = await ctx.joplin.models.session().createUserSession(user.id); - ctx.cookies.set('sessionId', session.id); + cookieSet(ctx, 'sessionId', session.id); await ctx.joplin.models.notification().add(user.id, NotificationKey.ConfirmEmail); diff --git a/packages/server/src/routes/index/users.test.ts b/packages/server/src/routes/index/users.test.ts index ee610a04c..77db72c98 100644 --- a/packages/server/src/routes/index/users.test.ts +++ b/packages/server/src/routes/index/users.test.ts @@ -1,6 +1,7 @@ import { User } from '../../db'; import routeHandler from '../../middleware/routeHandler'; import { NotificationKey } from '../../models/NotificationModel'; +import { cookieGet } from '../../utils/cookies'; import { ErrorForbidden } from '../../utils/errors'; import { execRequest, execRequestC } from '../../utils/testing/apiUtils'; import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, createUserAndSession, models, parseHtml, checkContextError, expectHttpError } from '../../utils/testing/testUtils'; @@ -240,7 +241,7 @@ describe('index/users', function() { password: newPassword, password2: newPassword, }); - const sessionId = context.cookies.get('sessionId'); + const sessionId = cookieGet(context, 'sessionId'); expect(sessionId).toBeFalsy(); } @@ -253,7 +254,7 @@ describe('index/users', function() { password2: newPassword, token: token2, }); - const sessionId = context.cookies.get('sessionId'); + const sessionId = cookieGet(context, 'sessionId'); expect(sessionId).toBeFalsy(); } @@ -266,7 +267,7 @@ describe('index/users', function() { }); // Check that the user has been logged in - const sessionId = context.cookies.get('sessionId'); + const sessionId = cookieGet(context, 'sessionId'); const session = await models().session().load(sessionId); expect(session.user_id).toBe(user1.id); @@ -303,7 +304,7 @@ describe('index/users', function() { user1 = await models().user().load(user1.id); // Check that the user has been logged in - const sessionId = context.cookies.get('sessionId'); + const sessionId = cookieGet(context, 'sessionId'); expect(sessionId).toBeFalsy(); // Check that the email has been verified diff --git a/packages/server/src/routes/index/users.ts b/packages/server/src/routes/index/users.ts index 6dfdd65dd..2b2fe9a63 100644 --- a/packages/server/src/routes/index/users.ts +++ b/packages/server/src/routes/index/users.ts @@ -19,6 +19,7 @@ import { confirmUrl } from '../../utils/urlUtils'; import { cancelSubscriptionByUserId, updateSubscriptionType } from '../../utils/stripe'; import { createCsrfTag } from '../../utils/csrf'; import { formatDateTime } from '../../utils/time'; +import { cookieSet } from '../../utils/cookies'; export interface CheckRepeatPasswordInput { password: string; @@ -236,7 +237,7 @@ router.post('users/:id/confirm', async (path: SubPath, ctx: AppContext) => { await ctx.joplin.models.token().deleteByValue(userId, fields.token); const session = await ctx.joplin.models.session().createUserSession(userId); - ctx.cookies.set('sessionId', session.id); + cookieSet(ctx, 'sessionId', session.id); await ctx.joplin.models.notification().add(userId, NotificationKey.PasswordSet); diff --git a/packages/server/src/utils/cookies.ts b/packages/server/src/utils/cookies.ts new file mode 100644 index 000000000..201640207 --- /dev/null +++ b/packages/server/src/utils/cookies.ts @@ -0,0 +1,17 @@ +import config from '../config'; +import { AppContext } from './types'; + +export function cookieSet(ctx: AppContext, name: string, value: string) { + ctx.cookies.set(name, value, { + // Means that the cookies cannot be accessed from JavaScript + httpOnly: true, + // Can only be transferred over https + secure: config().cookieSecure, + // Prevent cookies from being sent in cross-site requests + sameSite: true, + }); +} + +export function cookieGet(ctx: AppContext, name: string) { + return ctx.cookies.get(name); +} diff --git a/packages/server/src/utils/requestUtils.ts b/packages/server/src/utils/requestUtils.ts index 99ffbfea4..01f10fd73 100644 --- a/packages/server/src/utils/requestUtils.ts +++ b/packages/server/src/utils/requestUtils.ts @@ -1,3 +1,4 @@ +import { cookieGet } from './cookies'; import { ErrorForbidden } from './errors'; import { AppContext } from './types'; @@ -61,7 +62,7 @@ export function headerSessionId(headers: any): string { export function contextSessionId(ctx: AppContext, throwIfNotFound = true): string { if (ctx.headers['x-api-auth']) return ctx.headers['x-api-auth']; - const id = ctx.cookies.get('sessionId'); + const id = cookieGet(ctx, 'sessionId'); if (!id && throwIfNotFound) throw new ErrorForbidden('Invalid or missing session'); return id; } diff --git a/packages/server/src/utils/testing/testUtils.ts b/packages/server/src/utils/testing/testUtils.ts index 8e4a4754b..cada643c9 100644 --- a/packages/server/src/utils/testing/testUtils.ts +++ b/packages/server/src/utils/testing/testUtils.ts @@ -21,6 +21,7 @@ import { initializeJoplinUtils } from '../joplinUtils'; import MustacheService from '../../services/MustacheService'; import uuidgen from '../uuidgen'; import { createCsrfToken } from '../csrf'; +import { cookieSet } from '../cookies'; // Takes into account the fact that this file will be inside the /dist directory // when it runs. @@ -211,7 +212,7 @@ export async function koaAppContext(options: AppContextTestOptions = null): Prom }; if (options.sessionId) { - appContext.cookies.set('sessionId', options.sessionId); + cookieSet(appContext, 'sessionId', options.sessionId); } return appContext as AppContext; diff --git a/packages/server/src/utils/types.ts b/packages/server/src/utils/types.ts index 7130a5f90..13c6fabe8 100644 --- a/packages/server/src/utils/types.ts +++ b/packages/server/src/utils/types.ts @@ -108,6 +108,7 @@ export interface Config { supportName: string; businessEmail: string; isJoplinCloud: boolean; + cookieSecure: boolean; } export enum HttpMethod {