From faa10da86b34ba449ace6fcff0795170a31f6b1d Mon Sep 17 00:00:00 2001 From: David Mehren Date: Mon, 8 Jun 2020 15:27:31 +0200 Subject: [PATCH 1/5] Set all cookies with sameSite: strict Modern browsers do not support (or will stop supporting) sameSite: none (or no sameSite attribute) without the Secure flag. As we don't want everyone to be able to make requests with our cookies anyway, this commit sets sameSite to strict. See https://developer.mozilla.org/de/docs/Web/HTTP/Headers/Set-Cookie/SameSite Signed-off-by: David Mehren --- public/js/index.js | 3 ++- public/js/lib/common/login.js | 6 ++++-- public/js/lib/editor/index.js | 24 ++++++++++++++++-------- public/js/locale.js | 3 ++- src/lib/app.ts | 3 ++- 5 files changed, 26 insertions(+), 13 deletions(-) diff --git a/public/js/index.js b/public/js/index.js index ca28c3c89..603802338 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -1594,7 +1594,8 @@ function toggleNightMode () { store.set('nightMode', !isActive) } else { Cookies.set('nightMode', !isActive, { - expires: 365 + expires: 365, + sameSite: 'strict' }) } } diff --git a/public/js/lib/common/login.js b/public/js/lib/common/login.js index 28e5b4703..931c115f4 100644 --- a/public/js/lib/common/login.js +++ b/public/js/lib/common/login.js @@ -19,11 +19,13 @@ export function resetCheckAuth () { export function setLoginState (bool, id) { Cookies.set('loginstate', bool, { - expires: 365 + expires: 365, + sameSite: 'strict' }) if (id) { Cookies.set('userid', id, { - expires: 365 + expires: 365, + sameSite: 'strict' }) } else { Cookies.remove('userid') diff --git a/public/js/lib/editor/index.js b/public/js/lib/editor/index.js index 3cbdd8e9e..a8b25e20d 100644 --- a/public/js/lib/editor/index.js +++ b/public/js/lib/editor/index.js @@ -344,12 +344,14 @@ export default class Editor { const setType = () => { if (this.editor.getOption('indentWithTabs')) { Cookies.set('indent_type', 'tab', { - expires: 365 + expires: 365, + sameSite: 'strict' }) type.text('Tab Size:') } else { Cookies.set('indent_type', 'space', { - expires: 365 + expires: 365, + sameSite: 'strict' }) type.text('Spaces:') } @@ -360,11 +362,13 @@ export default class Editor { var unit = this.editor.getOption('indentUnit') if (this.editor.getOption('indentWithTabs')) { Cookies.set('tab_size', unit, { - expires: 365 + expires: 365, + sameSite: 'strict' }) } else { Cookies.set('space_units', unit, { - expires: 365 + expires: 365, + sameSite: 'strict' }) } widthLabel.text(unit) @@ -432,7 +436,8 @@ export default class Editor { const setKeymapLabel = () => { var keymap = this.editor.getOption('keyMap') Cookies.set('keymap', keymap, { - expires: 365 + expires: 365, + sameSite: 'strict' }) label.text(keymap) this.restoreOverrideEditorKeymap() @@ -480,7 +485,8 @@ export default class Editor { } this.editor.setOption('theme', theme) Cookies.set('theme', theme, { - expires: 365 + expires: 365, + sameSite: 'strict' }) checkTheme() @@ -525,7 +531,8 @@ export default class Editor { this.editor.setOption('mode', mode) } Cookies.set('spellcheck', mode === 'spell-checker', { - expires: 365 + expires: 365, + sameSite: 'strict' }) checkSpellcheck() @@ -570,7 +577,8 @@ export default class Editor { ) if (overrideBrowserKeymap.is(':checked')) { Cookies.set('preferences-override-browser-keymap', true, { - expires: 365 + expires: 365, + sameSite: 'strict' }) this.restoreOverrideEditorKeymap() } else { diff --git a/public/js/locale.js b/public/js/locale.js index 71c0f99fb..670370d40 100644 --- a/public/js/locale.js +++ b/public/js/locale.js @@ -25,7 +25,8 @@ $('select.ui-locale option[value="' + lang + '"]').attr('selected', 'selected') locale.change(function () { Cookies.set('locale', $(this).val(), { - expires: 365 + expires: 365, + sameSite: 'strict' }) window.location.reload() }) diff --git a/src/lib/app.ts b/src/lib/app.ts index 3aa576637..bfc97410f 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -181,7 +181,8 @@ app.use(session({ saveUninitialized: true, // always create session to ensure the origin rolling: true, // reset maxAge on every response cookie: { - maxAge: config.sessionLife + maxAge: config.sessionLife, + sameSite: 'strict' }, store: sessionStore })) From fb7787814320edbed0ab4e96406e2fa3c0ff61cd Mon Sep 17 00:00:00 2001 From: David Mehren Date: Mon, 8 Jun 2020 15:29:27 +0200 Subject: [PATCH 2/5] Disable unneeded 'io' cookie. According to https://github.com/socketio/socket.io/issues/2276 this cookie is not used for anything. To avoid browser warnings about the sameSite attribute, we disable it here. Signed-off-by: David Mehren --- src/lib/app.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/app.ts b/src/lib/app.ts index bfc97410f..b4a921df1 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -64,7 +64,7 @@ if (config.useSSL) { } // socket io -const io = SocketIO(server) +const io = SocketIO(server, { cookie: false }) io.engine.ws = new WebSocket.Server({ noServer: true, perMessageDeflate: false From 8406f75bb781ba9e7876b3015cf3bb9cf4704205 Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Mon, 8 Jun 2020 15:11:17 +0200 Subject: [PATCH 3/5] Ensure session cookies are secure While HSTS should take care of most of this, setting cookies to be secure, and only applied on same site helps to improve situations where for whatever reason, downgrade attacks are still a thing. This patch adds the `sameSite` and `secure` to the session cookie and this way prevent all accidents where a browser may doesn't support HSTS or HSTS is intentionally dropped. Reference: https://www.npmjs.com/package/express-session#cookiesecure Signed-off-by: Sheogorath --- src/lib/app.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/app.ts b/src/lib/app.ts index b4a921df1..93a0399b9 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -182,7 +182,8 @@ app.use(session({ rolling: true, // reset maxAge on every response cookie: { maxAge: config.sessionLife, - sameSite: 'strict' + sameSite: 'strict', + secure: config.useSSL || config.protocolUseSSL || false }, store: sessionStore })) From 3ae999024f308bf6fc8bca004863bc570211b803 Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Wed, 10 Jun 2020 12:21:11 +0200 Subject: [PATCH 4/5] Fix broken cookie handling due to missing proxy awareness We enabled the `secure` flag for various cookies in previous commits. This caused setups behind reverse proxies to drop cookies as the nodejs instance wasn't aware of the fact that it was able to hand out secure commits using an insecure connection (between the codimd instance and the reverse proxy). This patch makes express, the webserver framework we use, aware of proxies and this way re-enabled the handing out of cookies. Not only the cookie monster will enjoy, but also functionality like authentication and real-time editing will return as intended. References: https://www.npmjs.com/package/express-session#cookiesecure https://github.com/codimd/server/commit/383d791a50919bb9890a3f3f797ecc95125ab8bf Signed-off-by: Sheogorath --- src/lib/app.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/lib/app.ts b/src/lib/app.ts index 93a0399b9..0fb01c2a1 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -63,6 +63,13 @@ if (config.useSSL) { server = http.createServer(app) } +// if we manage to provide HTTPS domains, but don't provide TLS ourselves +// obviously a proxy is involded. In order to make sure express is aware of +// this, we provide the option to trust proxies here. +if (!config.useSSL && config.protocolUseSSL) { + app.set('trust proxy', 1) +} + // socket io const io = SocketIO(server, { cookie: false }) io.engine.ws = new WebSocket.Server({ From c67214b7d01d73aa22c4294d696c1676bbd6f34b Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Wed, 10 Jun 2020 15:08:39 +0200 Subject: [PATCH 5/5] Relax cookie restrictions to 'lax' to allow frontend to work Our frontend requests the `/me` pathname in order to determine whether it's logged in or not. Due to the fact that the sameSite attribute of the session cookie was set to `strict` in a previous commit, the session token was no longer sent along with HTTP calls initiated by JS. This is due to the RFCs definition of "safe" HTTP calls in RFC7231. The bug triggers the UI to show up like an unauthenticated user, even after a successful login. In order to debug it a look into the send cookies to the `/me` turned out to be very enlightening. The fix this patch implements is rather simple, it replaces the sameSite attribute to `lax` which enables the cookies for those requests again. Some older and mobile clients were unaffected by this due to the lack of implementations of sameSite policies. References: https://tools.ietf.org/html/rfc7231#section-4.2.1 https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-5.3.7.1 https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite https://github.com/codimd/server/commit/e77e7b165ac4920290015ec4b95e651730009edc Signed-off-by: Sheogorath --- src/lib/app.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/app.ts b/src/lib/app.ts index 0fb01c2a1..982664236 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -189,7 +189,7 @@ app.use(session({ rolling: true, // reset maxAge on every response cookie: { maxAge: config.sessionLife, - sameSite: 'strict', + sameSite: 'lax', secure: config.useSSL || config.protocolUseSSL || false }, store: sessionStore