From 5268aaee4f0ee67deec498081a09df50d229689f Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sat, 28 Dec 2024 21:33:05 +0100 Subject: [PATCH 1/8] deps: replace csurf with csrf-csrf --- package-lock.json | 74 ++++++++++++++++++----------------------------- package.json | 3 +- 2 files changed, 29 insertions(+), 48 deletions(-) diff --git a/package-lock.json b/package-lock.json index 78f8e3bef..5a9d9d3fd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,7 +29,7 @@ "codemirror": "5.65.18", "compression": "1.7.5", "cookie-parser": "1.4.7", - "csurf": "1.11.0", + "csrf-csrf": "3.1.0", "dayjs": "1.11.13", "dayjs-plugin-utc": "0.1.2", "debounce": "2.2.0", @@ -117,7 +117,6 @@ "@types/cls-hooked": "4.3.9", "@types/compression": "1.7.5", "@types/cookie-parser": "1.4.8", - "@types/csurf": "1.11.5", "@types/debounce": "1.2.4", "@types/ejs": "3.1.5", "@types/electron-squirrel-startup": "1.0.2", @@ -3807,16 +3806,6 @@ "@types/express": "*" } }, - "node_modules/@types/csurf": { - "version": "1.11.5", - "resolved": "https://registry.npmjs.org/@types/csurf/-/csurf-1.11.5.tgz", - "integrity": "sha512-5rw87+5YGixyL2W8wblSUl5DSZi5YOlXE6Awwn2ofLvqKr/1LruKffrQipeJKUX44VaxKj8m5es3vfhltJTOoA==", - "dev": true, - "license": "MIT", - "dependencies": { - "@types/express-serve-static-core": "*" - } - }, "node_modules/@types/d3": { "version": "7.4.3", "resolved": "https://registry.npmjs.org/@types/d3/-/d3-7.4.3.tgz", @@ -6922,19 +6911,40 @@ "node": ">=12.10" } }, - "node_modules/csrf": { + "node_modules/csrf-csrf": { "version": "3.1.0", - "resolved": "https://registry.npmjs.org/csrf/-/csrf-3.1.0.tgz", - "integrity": "sha512-uTqEnCvWRk042asU6JtapDTcJeeailFy4ydOQS28bj1hcLnYRiqi8SsD2jS412AY1I/4qdOwWZun774iqywf9w==", + "resolved": "https://registry.npmjs.org/csrf-csrf/-/csrf-csrf-3.1.0.tgz", + "integrity": "sha512-kZacFfFbdYFxNnFdigRHCzVAq019vJyUUtgPLjCtzh6jMXcWmf8bGUx/hsqtSEMXaNcPm8iXpjC+hW5aeOsRMg==", + "license": "ISC", "dependencies": { - "rndm": "1.2.0", - "tsscmp": "1.0.6", - "uid-safe": "2.1.5" + "http-errors": "^2.0.0" + } + }, + "node_modules/csrf-csrf/node_modules/http-errors": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-2.0.0.tgz", + "integrity": "sha512-FtwrG/euBzaEjYeRqOgly7G0qviiXoJWnvEH2Z1plBdXgbyjv34pHTSb9zoeHMyDy33+DWy5Wt9Wo+TURtOYSQ==", + "license": "MIT", + "dependencies": { + "depd": "2.0.0", + "inherits": "2.0.4", + "setprototypeof": "1.2.0", + "statuses": "2.0.1", + "toidentifier": "1.0.1" }, "engines": { "node": ">= 0.8" } }, + "node_modules/csrf-csrf/node_modules/toidentifier": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/toidentifier/-/toidentifier-1.0.1.tgz", + "integrity": "sha512-o5sSPKEkg/DIQNmH43V0/uerLrpzVedkUh8tGNvaeXpfpuwjKenlSox/2O/BTlZUtEe+JG7s5YhEz608PlAHRA==", + "license": "MIT", + "engines": { + "node": ">=0.6" + } + }, "node_modules/css-select": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/css-select/-/css-select-5.1.0.tgz", @@ -6976,29 +6986,6 @@ "node": ">=18" } }, - "node_modules/csurf": { - "version": "1.11.0", - "resolved": "https://registry.npmjs.org/csurf/-/csurf-1.11.0.tgz", - "integrity": "sha512-UCtehyEExKTxgiu8UHdGvHj4tnpE/Qctue03Giq5gPgMQ9cg/ciod5blZQ5a4uCEenNQjxyGuzygLdKUmee/bQ==", - "deprecated": "Please use another csrf package", - "dependencies": { - "cookie": "0.4.0", - "cookie-signature": "1.0.6", - "csrf": "3.1.0", - "http-errors": "~1.7.3" - }, - "engines": { - "node": ">= 0.8.0" - } - }, - "node_modules/csurf/node_modules/cookie": { - "version": "0.4.0", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.4.0.tgz", - "integrity": "sha512-+Hp8fLp57wnUSt0tY0tHEXh4voZRDnoIrZPqlo3DPiI4y9lwg/jqx+1Om94/W6ZaPDOUbnjOt/99w66zk+l1Xg==", - "engines": { - "node": ">= 0.6" - } - }, "node_modules/cytoscape": { "version": "3.30.4", "resolved": "https://registry.npmjs.org/cytoscape/-/cytoscape-3.30.4.tgz", @@ -15404,11 +15391,6 @@ "url": "https://github.com/sponsors/isaacs" } }, - "node_modules/rndm": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/rndm/-/rndm-1.2.0.tgz", - "integrity": "sha512-fJhQQI5tLrQvYIYFpOnFinzv9dwmR7hRnUz1XqP3OJ1jIweTNOd6aTO4jwQSgcBSFUB+/KHJxuGneime+FdzOw==" - }, "node_modules/roarr": { "version": "2.15.4", "resolved": "https://registry.npmjs.org/roarr/-/roarr-2.15.4.tgz", diff --git a/package.json b/package.json index 2f056ed91..245e9bdd1 100644 --- a/package.json +++ b/package.json @@ -74,7 +74,7 @@ "codemirror": "5.65.18", "compression": "1.7.5", "cookie-parser": "1.4.7", - "csurf": "1.11.0", + "csrf-csrf": "3.1.0", "dayjs": "1.11.13", "dayjs-plugin-utc": "0.1.2", "debounce": "2.2.0", @@ -159,7 +159,6 @@ "@types/cls-hooked": "4.3.9", "@types/compression": "1.7.5", "@types/cookie-parser": "1.4.8", - "@types/csurf": "1.11.5", "@types/debounce": "1.2.4", "@types/ejs": "3.1.5", "@types/electron-squirrel-startup": "1.0.2", From b7876107174bc364662ebdf7d1447aa4134b35d8 Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Mon, 30 Dec 2024 15:31:29 +0100 Subject: [PATCH 2/8] refactor: replace csurf with csrf-csrf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I've kept the identical same settings as before – however they are not *ideal* from what I read. More secure settings will need to be tested a bit more thoroughly first and will be a separate PR. --- src/routes/routes.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/routes/routes.ts b/src/routes/routes.ts index 81cec2c6d..63d85183d 100644 --- a/src/routes/routes.ts +++ b/src/routes/routes.ts @@ -9,7 +9,7 @@ import auth from "../services/auth.js"; import cls from "../services/cls.js"; import sql from "../services/sql.js"; import entityChangesService from "../services/entity_changes.js"; -import csurf from "csurf"; +import { doubleCsrf } from "csrf-csrf"; import { createPartialContentHandler } from "@triliumnext/express-partial-content"; import rateLimit from "express-rate-limit"; import AbstractBeccaEntity from "../becca/entities/abstract_becca_entity.js"; @@ -71,10 +71,15 @@ import etapiSpecialNoteRoutes from "../etapi/special_notes.js"; import etapiSpecRoute from "../etapi/spec.js"; import etapiBackupRoute from "../etapi/backup.js"; -const csrfMiddleware = csurf({ - cookie: { - path: "" // empty, so cookie is valid only for the current path - } +const { doubleCsrfProtection: csrfMiddleware } = doubleCsrf({ + getSecret: (req) => req.secret, + cookieOptions: { + path: "", // empty, so cookie is valid only for the current path + secure: false, + sameSite: false, + httpOnly: false, + }, + cookieName: "_csrf", }); const MAX_ALLOWED_FILE_SIZE_MB = 250; From d20a3bab2acf0f33019d31756cb9e7365850482b Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 5 Jan 2025 19:27:11 +0100 Subject: [PATCH 3/8] fix(csrfMiddleware): use sessionSecret instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit since `cookie-parser` is not configured with a secret, req.secret is not set and hence is `undefined`, which then is used as literal 'undefined' in the hashing function – making it less secure. Instead we can use the existing sessionSecret: the `csrf-csrf` developer confirmed in their Discord chat, that it would be ok to use the same secret here. --- src/routes/routes.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/routes/routes.ts b/src/routes/routes.ts index 63d85183d..f4f9314fe 100644 --- a/src/routes/routes.ts +++ b/src/routes/routes.ts @@ -15,6 +15,7 @@ import rateLimit from "express-rate-limit"; import AbstractBeccaEntity from "../becca/entities/abstract_becca_entity.js"; import NotFoundError from "../errors/not_found_error.js"; import ValidationError from "../errors/validation_error.js"; +import sessionSecret from "../services/session_secret.js"; // page routes import setupRoute from "./setup.js"; @@ -72,7 +73,7 @@ import etapiSpecRoute from "../etapi/spec.js"; import etapiBackupRoute from "../etapi/backup.js"; const { doubleCsrfProtection: csrfMiddleware } = doubleCsrf({ - getSecret: (req) => req.secret, + getSecret: () => sessionSecret, cookieOptions: { path: "", // empty, so cookie is valid only for the current path secure: false, From c36085e5801f991980f871e154e7c22287571692 Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 5 Jan 2025 19:37:28 +0100 Subject: [PATCH 4/8] chore: fix TS warning by type narrowing `req.csrfToken` might be undefined according to `csrf-csrf` provided types, so use type narrowing to make sure it exists, before calling it --- src/routes/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routes/index.ts b/src/routes/index.ts index 5115630a0..4a26d8387 100644 --- a/src/routes/index.ts +++ b/src/routes/index.ts @@ -19,7 +19,7 @@ function index(req: Request, res: Response) { const view = !utils.isElectron() && req.cookies["trilium-device"] === "mobile" ? "mobile" : "desktop"; - const csrfToken = req.csrfToken(); + const csrfToken = (typeof req.csrfToken === "function") ? req.csrfToken() : undefined; log.info(`Generated CSRF token ${csrfToken} with secret ${res.getHeader("set-cookie")}`); // We force the page to not be cached since on mobile the CSRF token can be From 59ecc614c227628bbb33003e303c6d2f44a68ba3 Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 12 Jan 2025 11:43:41 +0100 Subject: [PATCH 5/8] refactor: call logout route via JS required for csrf-csrf to correctly protect against CSRF, as it required the _csrf cookie AND the x-csrf-token HTTP header, the latter cannot be set via simple Form POST action using "../login" here, because "server" method is automatically prepending all paths with "/api", which we don't want here, as we want "/login" --- src/public/app/components/entrypoints.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/public/app/components/entrypoints.ts b/src/public/app/components/entrypoints.ts index b62cfdeb3..f75198cb7 100644 --- a/src/public/app/components/entrypoints.ts +++ b/src/public/app/components/entrypoints.ts @@ -114,11 +114,9 @@ export default class Entrypoints extends Component { utils.reloadFrontendApp(); } - logoutCommand() { - const $logoutForm = $('
').append($(``)); - - $("body").append($logoutForm); - $logoutForm.trigger("submit"); + async logoutCommand() { + await server.post("../logout"); + window.location.replace(`/login`); } backInNoteHistoryCommand() { From d1bd2d2812ae907979084ae87d0be61c52e5d83e Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 12 Jan 2025 13:13:59 +0100 Subject: [PATCH 6/8] refactor(routes/login): remove unused rendering of HTML --- src/routes/login.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/routes/login.ts b/src/routes/login.ts index cbb746c30..7b804896e 100644 --- a/src/routes/login.ts +++ b/src/routes/login.ts @@ -94,8 +94,7 @@ function verifyPassword(guessedPassword: string) { function logout(req: Request, res: Response) { req.session.regenerate(() => { req.session.loggedIn = false; - - res.redirect("login"); + res.sendStatus(200); }); } From 4cd18441e423265942aa17f9aac79c688c710dc4 Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 12 Jan 2025 13:16:26 +0100 Subject: [PATCH 7/8] deps: Update package-lock --- package-lock.json | 52 ----------------------------------------------- 1 file changed, 52 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5a9d9d3fd..2df4756a8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10830,42 +10830,6 @@ "dev": true, "license": "BSD-2-Clause" }, - "node_modules/http-errors": { - "version": "1.7.3", - "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.7.3.tgz", - "integrity": "sha512-ZTTX0MWrsQ2ZAhA1cejAwDLycFsd7I7nVtnkT3Ol0aqodaKW+0CTZDQ1uBv5whptCnc8e8HeRRJxRs0kmm/Qfw==", - "dependencies": { - "depd": "~1.1.2", - "inherits": "2.0.4", - "setprototypeof": "1.1.1", - "statuses": ">= 1.5.0 < 2", - "toidentifier": "1.0.0" - }, - "engines": { - "node": ">= 0.6" - } - }, - "node_modules/http-errors/node_modules/depd": { - "version": "1.1.2", - "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.2.tgz", - "integrity": "sha512-7emPTl6Dpo6JRXOXjLRxck+FlLRX5847cLKEn00PLAgc3g2hTZZgr+e4c2v6QpSmLeFP3n5yUo7ft6avBK/5jQ==", - "engines": { - "node": ">= 0.6" - } - }, - "node_modules/http-errors/node_modules/setprototypeof": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/setprototypeof/-/setprototypeof-1.1.1.tgz", - "integrity": "sha512-JvdAWfbXeIGaZ9cILp38HntZSFSo3mWg6xGcJJsd+d4aRMOqauag1C63dJfDw7OaMYwEbHMOxEZ1lqVRYP2OAw==" - }, - "node_modules/http-errors/node_modules/statuses": { - "version": "1.5.0", - "resolved": "https://registry.npmjs.org/statuses/-/statuses-1.5.0.tgz", - "integrity": "sha512-OpZ3zP+jT1PI7I8nemJX4AKmAX070ZkYPVWV/AaKTJl+tXCTGyVdC1a4SL8RUQYEwk/f34ZX8UTykN68FwrqAA==", - "engines": { - "node": ">= 0.6" - } - }, "node_modules/http-proxy-agent": { "version": "7.0.2", "resolved": "https://registry.npmjs.org/http-proxy-agent/-/http-proxy-agent-7.0.2.tgz", @@ -16921,14 +16885,6 @@ "node": ">=8.0" } }, - "node_modules/toidentifier": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/toidentifier/-/toidentifier-1.0.0.tgz", - "integrity": "sha512-yaOH/Pk/VEhBWWTlhI+qXxDFXlejDGcQipMlyxda9nthulaxLZUNcUqFxokp0vcYnvteJln5FNQDRrxj3YcbVw==", - "engines": { - "node": ">=0.6" - } - }, "node_modules/token-types": { "version": "5.0.1", "resolved": "https://registry.npmjs.org/token-types/-/token-types-5.0.1.tgz", @@ -17117,14 +17073,6 @@ "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", "license": "0BSD" }, - "node_modules/tsscmp": { - "version": "1.0.6", - "resolved": "https://registry.npmjs.org/tsscmp/-/tsscmp-1.0.6.tgz", - "integrity": "sha512-LxhtAkPDTkVCMQjt2h6eBVY28KCjikZqZfMcC15YBeNjkgUpdCfBu5HoiOTDu86v6smE8yOjyEktJ8hlbANHQA==", - "engines": { - "node": ">=0.6.x" - } - }, "node_modules/tsx": { "version": "4.19.2", "resolved": "https://registry.npmjs.org/tsx/-/tsx-4.19.2.tgz", From ea621ef8e16ec725cac72f55dad2c80573d1e12b Mon Sep 17 00:00:00 2001 From: Panagiotis Papadopoulos Date: Sun, 12 Jan 2025 13:30:02 +0100 Subject: [PATCH 8/8] chore(prettier): fix code style --- src/routes/routes.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/routes/routes.ts b/src/routes/routes.ts index f4f9314fe..d04718cd5 100644 --- a/src/routes/routes.ts +++ b/src/routes/routes.ts @@ -73,14 +73,14 @@ import etapiSpecRoute from "../etapi/spec.js"; import etapiBackupRoute from "../etapi/backup.js"; const { doubleCsrfProtection: csrfMiddleware } = doubleCsrf({ - getSecret: () => sessionSecret, - cookieOptions: { - path: "", // empty, so cookie is valid only for the current path - secure: false, - sameSite: false, - httpOnly: false, - }, - cookieName: "_csrf", + getSecret: () => sessionSecret, + cookieOptions: { + path: "", // empty, so cookie is valid only for the current path + secure: false, + sameSite: false, + httpOnly: false + }, + cookieName: "_csrf" }); const MAX_ALLOWED_FILE_SIZE_MB = 250;