From 67e2459cb3277fd6f2a8af10993302e8233eb1c8 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 14:37:33 -0700 Subject: [PATCH 1/9] [*.test] Refactor logInAgent to a common place --- src/routes/AuthenticationRoutes.test.ts | 31 +------------------ src/routes/InAppRoutes.test.ts | 31 +------------------ src/tests/supertest.utils.ts | 40 +++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 60 deletions(-) create mode 100644 src/tests/supertest.utils.ts diff --git a/src/routes/AuthenticationRoutes.test.ts b/src/routes/AuthenticationRoutes.test.ts index 69653325..9948dfea 100644 --- a/src/routes/AuthenticationRoutes.test.ts +++ b/src/routes/AuthenticationRoutes.test.ts @@ -1,8 +1,6 @@ -import { expect } from "chai"; import { StatusCodes } from "http-status-codes"; import request from "supertest"; -import { authenticateUser } from "../models/LogInUtilities"; import { HOME, LOGIN, @@ -14,34 +12,7 @@ import { VERIFY_ACCOUNT, } from "../paths"; import { app } from "./../server"; -import { dummyAccountDetails } from "../tests/DummyAccountUtils"; - -const authDetails = { - username_or_email: dummyAccountDetails.email, - password: dummyAccountDetails.password, -}; - -async function logInAgent() { - const agent = request.agent(app); - - // Check that the user does exist. - const user = await authenticateUser(authDetails); - expect(user).to.not.be.null; - - // Login, and follow redirect to `HOME`. - const result = await agent - .post(LOGIN) - .send(authDetails) - .redirects(1); - - expect(result.status).to.equal(StatusCodes.OK); - expect(result.type).to.equal("text/html"); - - const finalURL = new URL(result.request.url); - expect(finalURL.pathname).to.equal(HOME); - - return Promise.resolve(agent); -} +import { logInAgent } from "../tests/supertest.utils"; describe(ROOT, function() { it("should show the login form if not authenticated", function() { diff --git a/src/routes/InAppRoutes.test.ts b/src/routes/InAppRoutes.test.ts index 42fb3a62..0a7279af 100644 --- a/src/routes/InAppRoutes.test.ts +++ b/src/routes/InAppRoutes.test.ts @@ -1,38 +1,9 @@ -import { expect } from "chai"; import { StatusCodes } from "http-status-codes"; import request from "supertest"; -import { authenticateUser } from "../models/LogInUtilities"; import { ACCOUNT, HOME, LOGIN, WIKI } from "../paths"; import { app } from "../server"; -import { dummyAccountDetails } from "../tests/DummyAccountUtils"; - -const authDetails = { - username_or_email: dummyAccountDetails.email, - password: dummyAccountDetails.password, -}; - -async function logInAgent() { - const agent = request.agent(app); - - // Check that the user does exist. - const user = await authenticateUser(authDetails); - expect(user).to.not.be.null; - - // Login, and follow redirect to `HOME`. - const result = await agent - .post(LOGIN) - .send(authDetails) - .redirects(1); - - expect(result.status).to.equal(StatusCodes.OK); - expect(result.type).to.equal("text/html"); - - const finalURL = new URL(result.request.url); - expect(finalURL.pathname).to.equal(HOME); - - return Promise.resolve(agent); -} +import { logInAgent } from "../tests/supertest.utils"; describe(HOME, function() { it("requires authentication", function() { diff --git a/src/tests/supertest.utils.ts b/src/tests/supertest.utils.ts new file mode 100644 index 00000000..c85ec181 --- /dev/null +++ b/src/tests/supertest.utils.ts @@ -0,0 +1,40 @@ +import { expect } from "chai"; +import { StatusCodes } from "http-status-codes"; +import request from "supertest"; + +import { + authenticateUser, + AuthenticateUserParam, +} from "../models/LogInUtilities"; +import { HOME, LOGIN } from "../paths"; +import { app } from "../server"; +import { dummyAccountDetails } from "./DummyAccountUtils"; + +/** + * Log in a user using `supertest` and return the agent. By default, logs in the + * dummy account. + */ +export async function logInAgent(authDetails: AuthenticateUserParam = { + username_or_email: dummyAccountDetails.email, + password: dummyAccountDetails.password, +}) { + const agent = request.agent(app); + + // Check that the user does exist. + const user = await authenticateUser(authDetails); + expect(user).to.not.be.null; + + // Login, and follow redirect to `HOME`. + const result = await agent + .post(LOGIN) + .send(authDetails) + .redirects(1); + + expect(result.status).to.equal(StatusCodes.OK); + expect(result.type).to.equal("text/html"); + + const finalURL = new URL(result.request.url); + expect(finalURL.pathname).to.equal(HOME); + + return Promise.resolve(agent); +} From 8cf1a756ef34e257206db1074841ad1c1f2f3b97 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 15:52:18 -0700 Subject: [PATCH 2/9] [Deps] npm i --save-dev @types/lusca --- package-lock.json | 10 ++++++++++ package.json | 1 + 2 files changed, 11 insertions(+) diff --git a/package-lock.json b/package-lock.json index 2a115223..f3477e96 100644 --- a/package-lock.json +++ b/package-lock.json @@ -48,6 +48,7 @@ "@types/express-session": "^1.18.0", "@types/express-sslify": "^1.2.5", "@types/katex": "^0.16.7", + "@types/lusca": "^1.7.5", "@types/markdown-it": "^14.1.1", "@types/mocha": "^10.0.1", "@types/mongoose": "^5.11.97", @@ -3223,6 +3224,15 @@ "integrity": "sha512-sVDA58zAw4eWAffKOaQH5/5j3XeayukzDk+ewSsnv3p4yJEZHCCzMDiZM8e0OUrRvmpGZ85jf4yDHkHsgBNr9Q==", "dev": true }, + "node_modules/@types/lusca": { + "version": "1.7.5", + "resolved": "https://registry.npmjs.org/@types/lusca/-/lusca-1.7.5.tgz", + "integrity": "sha512-l49gAf8pu2iMzbKejLcz6Pqj+51H2na6BgORv1ElnE8ByPFcBdh/eZ0WNR1Va/6ZuNSZa01Hoy1DTZ3IZ+y+kA==", + "dev": true, + "dependencies": { + "@types/express": "*" + } + }, "node_modules/@types/markdown-it": { "version": "14.1.1", "resolved": "https://registry.npmjs.org/@types/markdown-it/-/markdown-it-14.1.1.tgz", diff --git a/package.json b/package.json index 42cf9c05..c15c44fc 100644 --- a/package.json +++ b/package.json @@ -93,6 +93,7 @@ "@types/express-session": "^1.18.0", "@types/express-sslify": "^1.2.5", "@types/katex": "^0.16.7", + "@types/lusca": "^1.7.5", "@types/markdown-it": "^14.1.1", "@types/mocha": "^10.0.1", "@types/mongoose": "^5.11.97", From 841bfcd3a2e7320a5a9249e767f66473e277ab5a Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 15:53:53 -0700 Subject: [PATCH 3/9] [App] Default CSRF protection on Express app A lot of tests are failing --- src/server.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server.ts b/src/server.ts index 9c63c890..d511cf74 100644 --- a/src/server.ts +++ b/src/server.ts @@ -13,6 +13,7 @@ import cookieParser from "cookie-parser"; import express, { Request, Response } from "express"; import session, { MemoryStore } from "express-session"; import { HTTPS } from "express-sslify"; +import { csrf } from "lusca"; import { join } from "path"; import { @@ -83,7 +84,7 @@ app.use(cookieParser()); * * TODO: Enable CSRF protection */ -// app.use(lusca.csrf()); +app.use(csrf()); app.set("views", join(__dirname, "views")); app.set("view engine", "ejs"); From 83f06a59df09619d81dd5a21f3b0afebd7d5c02e Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 18:06:20 -0700 Subject: [PATCH 4/9] [CSRF] Echo back the CSRF token when logging in --- src/@types/augmentations.d.ts | 2 ++ src/controllers/ControllerUtilities.ts | 7 +++++++ src/server.ts | 2 +- src/views/partials/forms/login.ejs | 23 ++++++++++++----------- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/@types/augmentations.d.ts b/src/@types/augmentations.d.ts index a8947486..869ff9bc 100644 --- a/src/@types/augmentations.d.ts +++ b/src/@types/augmentations.d.ts @@ -16,6 +16,8 @@ declare module "express" { message?: string; user?: AuthenticateUser; }; + /** Set by lusca.csrf. */ + csrfToken?: () => string; } interface Response { diff --git a/src/controllers/ControllerUtilities.ts b/src/controllers/ControllerUtilities.ts index 9370e4a1..4e70a51e 100644 --- a/src/controllers/ControllerUtilities.ts +++ b/src/controllers/ControllerUtilities.ts @@ -16,6 +16,7 @@ interface TemplateVariables { BASE_URL: string; LOGGED_IN: boolean; SEARCH_ENDPOINT_URL?: string; + csrfToken: string; abbreviatedCards?: Array>; account_info?: AuthenticateUser; message: string; @@ -34,10 +35,16 @@ export function getDefaultTemplateVars( const message = req?.session?.message || ""; if (req?.session) { req.session.message = ""; } + let csrfToken = ""; + if (req !== null && req.csrfToken !== undefined) { + csrfToken = req.csrfToken(); + } + return { APP_NAME: config.APP_NAME, BASE_URL: config.BASE_URL, LOGGED_IN: req?.session?.user !== undefined, + csrfToken, message, ...allPaths, }; diff --git a/src/server.ts b/src/server.ts index d511cf74..09496dba 100644 --- a/src/server.ts +++ b/src/server.ts @@ -77,7 +77,7 @@ app.use(cookieParser()); * * With csrf enabled, the CSRF token must be in the payload when modifying data * or the client will receive a 403 Forbidden. To send the token the client - * needs to echo back the _csrf value you received from the previous request. + * needs to echo back the _csrf value received from the previous request. * Furthermore, parsers must be registered before lusca. * * [1]: https://github.com/krakenjs/lusca#readme diff --git a/src/views/partials/forms/login.ejs b/src/views/partials/forms/login.ejs index b58a445b..f05af46c 100644 --- a/src/views/partials/forms/login.ejs +++ b/src/views/partials/forms/login.ejs @@ -1,18 +1,19 @@
- - + + + - - + + - + -

- Do not have an account? Sign up -

+

+ Do not have an account? Sign up +

-

- Forgot password? Reset password -

+

+ Forgot password? Reset password +

From da7b45a4b1a7da792f1ffe9f3c2ce35f9d797af3 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 18:22:55 -0700 Subject: [PATCH 5/9] [CSRF] Use the _csrf value that Lusca places in res.locals Filed #186 to track app-wide improvements w.r.t. res.locals and app.locals. [1]: https://expressjs.com/en/5x/api.html#res.locals --- src/@types/augmentations.d.ts | 2 -- src/controllers/ControllerUtilities.ts | 7 ------- src/views/partials/forms/login.ejs | 2 +- 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/@types/augmentations.d.ts b/src/@types/augmentations.d.ts index 869ff9bc..a8947486 100644 --- a/src/@types/augmentations.d.ts +++ b/src/@types/augmentations.d.ts @@ -16,8 +16,6 @@ declare module "express" { message?: string; user?: AuthenticateUser; }; - /** Set by lusca.csrf. */ - csrfToken?: () => string; } interface Response { diff --git a/src/controllers/ControllerUtilities.ts b/src/controllers/ControllerUtilities.ts index 4e70a51e..9370e4a1 100644 --- a/src/controllers/ControllerUtilities.ts +++ b/src/controllers/ControllerUtilities.ts @@ -16,7 +16,6 @@ interface TemplateVariables { BASE_URL: string; LOGGED_IN: boolean; SEARCH_ENDPOINT_URL?: string; - csrfToken: string; abbreviatedCards?: Array>; account_info?: AuthenticateUser; message: string; @@ -35,16 +34,10 @@ export function getDefaultTemplateVars( const message = req?.session?.message || ""; if (req?.session) { req.session.message = ""; } - let csrfToken = ""; - if (req !== null && req.csrfToken !== undefined) { - csrfToken = req.csrfToken(); - } - return { APP_NAME: config.APP_NAME, BASE_URL: config.BASE_URL, LOGGED_IN: req?.session?.user !== undefined, - csrfToken, message, ...allPaths, }; diff --git a/src/views/partials/forms/login.ejs b/src/views/partials/forms/login.ejs index f05af46c..b97eb9a9 100644 --- a/src/views/partials/forms/login.ejs +++ b/src/views/partials/forms/login.ejs @@ -1,5 +1,5 @@
- + From df74d7ba09c802cba260653c9e26f14194824883 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 19:14:07 -0700 Subject: [PATCH 6/9] [CSRF] Echo back CSRF token on all forms served through EJS --- src/views/partials/forms/reset_password.ejs | 11 ++++---- .../partials/forms/reset_password_request.ejs | 11 ++++---- .../partials/forms/send_validation_url.ejs | 10 ++++---- src/views/partials/forms/sign_up.ejs | 25 ++++++++----------- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/views/partials/forms/reset_password.ejs b/src/views/partials/forms/reset_password.ejs index 53d68428..cdf61d12 100644 --- a/src/views/partials/forms/reset_password.ejs +++ b/src/views/partials/forms/reset_password.ejs @@ -1,9 +1,10 @@ - - + + + - - + + - + diff --git a/src/views/partials/forms/reset_password_request.ejs b/src/views/partials/forms/reset_password_request.ejs index d246a68f..78600f60 100644 --- a/src/views/partials/forms/reset_password_request.ejs +++ b/src/views/partials/forms/reset_password_request.ejs @@ -1,8 +1,9 @@
- - + + + - +
diff --git a/src/views/partials/forms/send_validation_url.ejs b/src/views/partials/forms/send_validation_url.ejs index eb149d02..598cdb76 100644 --- a/src/views/partials/forms/send_validation_url.ejs +++ b/src/views/partials/forms/send_validation_url.ejs @@ -1,10 +1,10 @@
+ +
Request a validation URL for your <%= APP_NAME %> Account
-
Request a validation URL for your <%= APP_NAME %> Account
+ + - - - - +
diff --git a/src/views/partials/forms/sign_up.ejs b/src/views/partials/forms/sign_up.ejs index fe8bcdae..4407aea7 100644 --- a/src/views/partials/forms/sign_up.ejs +++ b/src/views/partials/forms/sign_up.ejs @@ -1,21 +1,18 @@
+ + + - - + + - - + + - - + - - -

- Already have an account? Log In -

+

+ Already have an account? Log In +

From 861c71c2c6f55ad75a3f1c6615b2c4bd13d377a4 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 20:09:47 -0700 Subject: [PATCH 7/9] [tRPC] Echo the CSRF token through the x-csrf-token header --- src/public/src/trpc.ts | 12 ++++++++++++ src/views/partials/header.ejs | 13 +++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/public/src/trpc.ts b/src/public/src/trpc.ts index 2f18ade7..c864a2c5 100644 --- a/src/public/src/trpc.ts +++ b/src/public/src/trpc.ts @@ -3,12 +3,24 @@ import { inferRouterInputs, inferRouterOutputs } from "@trpc/server"; import type { AppRouter } from "../../server.js"; +function getCSRFToken() { + return document.head.querySelector("meta[name='_csrf']")?.getAttribute( + "content", + ) + || ""; +} + // Pass `AppRouter` as generic here. This lets the `trpc` object know what // procedures are available on the server and their input/output types. export const trpc = createTRPCClient({ links: [ httpBatchLink({ url: "/trpc", + headers() { + return { + "x-csrf-token": getCSRFToken(), + }; + }, }), ], }); diff --git a/src/views/partials/header.ejs b/src/views/partials/header.ejs index 5450bcf9..9c87ac0b 100644 --- a/src/views/partials/header.ejs +++ b/src/views/partials/header.ejs @@ -1,5 +1,6 @@ + @@ -7,10 +8,10 @@ From 465884b86535e7b8d6eb8537f78f156676e918aa Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 20:43:56 -0700 Subject: [PATCH 8/9] [ES] Support NODE_ENV=test (at least on Posix) --- .vscode/launch.json | 2 +- package.json | 2 +- src/config.ts | 5 +++-- src/models/LogInUtilities.ts | 2 +- src/models/MongooseClient.ts | 4 ++-- src/server.ts | 3 ++- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index c37fcaf7..e94caed6 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -17,7 +17,7 @@ "type": "node-terminal" }, { - "command": "npx ts-mocha --config ./.mocharc.yml --timeout 0", + "command": "NODE_ENV=test npx ts-mocha --config ./.mocharc.yml --timeout 0", "name": "test:server", "request": "launch", "type": "node-terminal" diff --git a/package.json b/package.json index c15c44fc..e01edba7 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "lint:fix": "eslint --fix .", "single_test": "npm run build && ts-mocha", "test": "npm run test:server && npm run test:client", - "test:server": "npm run mocha_tests", + "test:server": "NODE_ENV=test npm run mocha_tests", "test:client": "cd src/public && npm run test && cd ../../", "mocha_tests": "nyc ts-mocha --config ./.mocharc.yml", "webpack": "webpack --config ./webpack.config.cjs" diff --git a/src/config.ts b/src/config.ts index 3f71395c..2a65537b 100644 --- a/src/config.ts +++ b/src/config.ts @@ -8,13 +8,14 @@ export const PORT = process.env.PORT || 5000; export const NODE_ENV = process.env.NODE_ENV || ""; export const IS_PROD = NODE_ENV === "production"; export const IS_DEV = NODE_ENV === "development"; +export const IS_TEST = NODE_ENV === "test"; export const IS_TS_NODE = !!process[Symbol.for("ts-node.register.instance")] || process.env.TS_NODE_DEV !== undefined; -if (!IS_DEV && !IS_PROD) { +if (!IS_DEV && !IS_PROD && !IS_TEST) { throw Error( - "Please set the NODE_ENV environment variable to either 'production' or 'development'.", + "Please set the NODE_ENV environment variable to either 'production', 'development', or 'test'.", ); } diff --git a/src/models/LogInUtilities.ts b/src/models/LogInUtilities.ts index 569351e9..e9a6fe4d 100644 --- a/src/models/LogInUtilities.ts +++ b/src/models/LogInUtilities.ts @@ -609,7 +609,7 @@ export async function deleteAccount(userIDInApp: number): Promise { export function deleteAllAccounts( usernamesToSpare = [config.PUBLIC_USER_USERNAME], ): Promise { - if (config.NODE_ENV !== "development") { + if (!(config.IS_DEV || config.IS_TEST)) { return Promise.reject( `Deleting all accounts isn't allowed in the ${config.NODE_ENV} environment`, ); diff --git a/src/models/MongooseClient.ts b/src/models/MongooseClient.ts index b3d1c044..4c545f4c 100644 --- a/src/models/MongooseClient.ts +++ b/src/models/MongooseClient.ts @@ -16,11 +16,11 @@ import { MongoMemoryServer } from "mongodb-memory-server"; import { connect, connection, disconnect } from "mongoose"; -import { IS_DEV, MONGO_URI } from "../config"; +import { IS_DEV, IS_TEST, MONGO_URI } from "../config"; // Already 5 by default, but I might need to increase it one day... let mongoServer: MongoMemoryServer | null = null; -if (IS_DEV) { +if (IS_DEV || IS_TEST) { (async () => { mongoServer = await MongoMemoryServer.create(); await connect(mongoServer.getUri()); diff --git a/src/server.ts b/src/server.ts index 09496dba..2dceb663 100644 --- a/src/server.ts +++ b/src/server.ts @@ -19,6 +19,7 @@ import { join } from "path"; import { IS_DEV, IS_PROD, + IS_TEST, IS_TS_NODE, MONGO_URI, PORT, @@ -61,7 +62,7 @@ app.use(session({ }, resave: false, name: "c13u-study-buddy", - store: IS_DEV ? new MemoryStore() : MongoStore.create({ + store: (IS_DEV || IS_TEST) ? new MemoryStore() : MongoStore.create({ mongoUrl: MONGO_URI, touchAfter: 24 * 3600, }), From 88b6a8a1507d38211275c2f9a493056f58679369 Mon Sep 17 00:00:00 2001 From: Chege Gitau Date: Sun, 30 Jun 2024 21:00:50 -0700 Subject: [PATCH 9/9] [CSRF] Disable in testing environment --- src/server.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/server.ts b/src/server.ts index 2dceb663..a53690d9 100644 --- a/src/server.ts +++ b/src/server.ts @@ -82,10 +82,13 @@ app.use(cookieParser()); * Furthermore, parsers must be registered before lusca. * * [1]: https://github.com/krakenjs/lusca#readme - * - * TODO: Enable CSRF protection */ -app.use(csrf()); +if (!IS_TEST) { + app.use(csrf()); +} else { + // Provide a fake CSRF token as the EJS templates expect it. + app.locals._csrf = "csrf_has_been_disabled_for_testing"; +} app.set("views", join(__dirname, "views")); app.set("view engine", "ejs");