Skip to content

Commit

Permalink
[App] Add CSRF token-based mitigation (#187)
Browse files Browse the repository at this point in the history
* [*.test] Refactor logInAgent to a common place
* [Deps] npm i --save-dev @types/lusca
* [App] Default CSRF protection on Express app
* [CSRF] Echo back the CSRF token when logging in
* [CSRF] Use the _csrf value that Lusca places in res.locals
* [CSRF] Echo back CSRF token on all forms served through EJS
* [tRPC] Echo the CSRF token through the x-csrf-token header
* [ES] Support NODE_ENV=test (at least on Posix)
* [CSRF] Disable in testing environment
  • Loading branch information
dchege711 authored Jul 1, 2024
1 parent 464e421 commit 67ac066
Show file tree
Hide file tree
Showing 17 changed files with 130 additions and 118 deletions.
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
10 changes: 10 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down
5 changes: 3 additions & 2 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'.",
);
}

Expand Down
2 changes: 1 addition & 1 deletion src/models/LogInUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ export async function deleteAccount(userIDInApp: number): Promise<void> {
export function deleteAllAccounts(
usernamesToSpare = [config.PUBLIC_USER_USERNAME],
): Promise<number> {
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`,
);
Expand Down
4 changes: 2 additions & 2 deletions src/models/MongooseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
12 changes: 12 additions & 0 deletions src/public/src/trpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<AppRouter>({
links: [
httpBatchLink({
url: "/trpc",
headers() {
return {
"x-csrf-token": getCSRFToken(),
};
},
}),
],
});
Expand Down
31 changes: 1 addition & 30 deletions src/routes/AuthenticationRoutes.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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() {
Expand Down
31 changes: 1 addition & 30 deletions src/routes/InAppRoutes.test.ts
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down
15 changes: 10 additions & 5 deletions src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ 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 {
IS_DEV,
IS_PROD,
IS_TEST,
IS_TS_NODE,
MONGO_URI,
PORT,
Expand Down Expand Up @@ -60,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,
}),
Expand All @@ -76,14 +78,17 @@ 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
*
* TODO: Enable CSRF protection
*/
// app.use(lusca.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");
Expand Down
40 changes: 40 additions & 0 deletions src/tests/supertest.utils.ts
Original file line number Diff line number Diff line change
@@ -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);
}
23 changes: 12 additions & 11 deletions src/views/partials/forms/login.ejs
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
<form method="post" action="<%= LOGIN %>">
<label for="username"> Username or Email Address: </label>
<input class="w3-input" type="text" name="username_or_email" required/>
<input type="hidden" name="_csrf" value="<%= _csrf %>">
<label for="username"> Username or Email Address: </label>
<input class="w3-input" type="text" name="username_or_email" required />

<label for="password"> Password: </label>
<input class="w3-input" type="password" name="password" minlength="8" required/>
<label for="password"> Password: </label>
<input class="w3-input" type="password" name="password" minlength="8" required />

<button class="w3-button w3-center w3-green" type="submit">Log In</button>
<button class="w3-button w3-center w3-green" type="submit">Log In</button>

<p>
Do not have an account? <a href="<%= REGISTER_USER %>">Sign up</a>
</p>
<p>
Do not have an account? <a href="<%= REGISTER_USER %>">Sign up</a>
</p>

<p>
Forgot password? <a href="<%= RESET_PASSWORD %>">Reset password</a>
</p>
<p>
Forgot password? <a href="<%= RESET_PASSWORD %>">Reset password</a>
</p>

</form>
11 changes: 6 additions & 5 deletions src/views/partials/forms/reset_password.ejs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<form method="post">
<label for="email"> Type your new password: </label>
<input class="w3-input" type="password" name="password_1" minlength="8" required/>
<input type="hidden" name="_csrf" value="<%= _csrf %>">
<label for="email"> Type your new password: </label>
<input class="w3-input" type="password" name="password_1" minlength="8" required />

<label for="username"> Re-type your new password: </label>
<input class="w3-input" type="password" name="password_2" minlength="8" required/>
<label for="username"> Re-type your new password: </label>
<input class="w3-input" type="password" name="password_2" minlength="8" required />

<button class="w3-button w3-center w3-green" type="submit">Reset Password</button>
<button class="w3-button w3-center w3-green" type="submit">Reset Password</button>
</form>
11 changes: 6 additions & 5 deletions src/views/partials/forms/reset_password_request.ejs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<form method="POST">
<label for="email"> Type the email address associated with your account: </label>
<input class="w3-input" type="email" name="email" required/>
<input type="hidden" name="_csrf" value="<%= _csrf %>">
<label for="email"> Type the email address associated with your account: </label>
<input class="w3-input" type="email" name="email" required />

<button class="w3-button w3-center w3-green" type="submit">
Request Password Reset
</button>
<button class="w3-button w3-center w3-green" type="submit">
Request Password Reset
</button>
</form>
10 changes: 5 additions & 5 deletions src/views/partials/forms/send_validation_url.ejs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<form method="post">
<input type="hidden" name="_csrf" value="<%= _csrf %>">
<h5>Request a validation URL for your <%= APP_NAME %> Account</h5>

<h5>Request a validation URL for your <%= APP_NAME %> Account</h5>
<label for="email"> Type your email address below: </label>
<input class="w3-input" type="email" name="email" required />

<label for="email"> Type your email address below: </label>
<input class="w3-input" type="email" name="email" required/>

<button class="w3-button w3-center w3-green" type="submit">Send Validation URL</button>
<button class="w3-button w3-center w3-green" type="submit">Send Validation URL</button>

</form>
25 changes: 11 additions & 14 deletions src/views/partials/forms/sign_up.ejs
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
<form method="post">
<input type="hidden" name="_csrf" value="<%= _csrf %>">
<label for="email">Email Address: </label>
<input class="w3-input" type="email" name="email" required />

<label for="email">Email Address: </label>
<input class="w3-input" type="email" name="email"
required/>
<label for="username"> Choose an alphanumeric username: </label>
<input class="w3-input" type="text" name="username" pattern="[_\-A-Za-z0-9]+" required />

<label for="username"> Choose an alphanumeric username: </label>
<input class="w3-input" type="text" name="username"
pattern="[_\-A-Za-z0-9]+" required/>
<label for="password"> Choose a password: </label>
<input class="w3-input" type="password" name="password" id="signup_password" minlength="8" required />

<label for="password"> Choose a password: </label>
<input class="w3-input" type="password" name="password"
id="signup_password" minlength="8" required/>
<button class="w3-button w3-center w3-green" type="submit">Sign Up</button>

<button class="w3-button w3-center w3-green" type="submit">Sign Up</button>

<p>
Already have an account? <a href="<%= LOGIN %>">Log In</a>
</p>
<p>
Already have an account? <a href="<%= LOGIN %>">Log In</a>
</p>

</form>
Loading

0 comments on commit 67ac066

Please sign in to comment.