From c3c912db074ffc46612792ed7651e86cb7f6f256 Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Thu, 18 May 2023 13:36:40 +0200 Subject: [PATCH] feat(api): add csrf protection (#50275) Co-authored-by: Sboonny --- api/jest.utils.test.ts | 19 +++ api/jest.utils.ts | 24 +-- api/package.json | 1 + api/src/app.ts | 47 +++++- api/src/routes/auth.ts | 4 +- api/src/routes/settings.test.ts | 259 ++++++++++++++++++++------------ api/src/routes/settings.ts | 4 + api/src/server.test.ts | 32 +++- api/src/utils/env.ts | 2 + api/tsconfig.json | 3 +- pnpm-lock.yaml | 15 ++ 11 files changed, 287 insertions(+), 123 deletions(-) create mode 100644 api/jest.utils.test.ts diff --git a/api/jest.utils.test.ts b/api/jest.utils.test.ts new file mode 100644 index 00000000000..03e074b43de --- /dev/null +++ b/api/jest.utils.test.ts @@ -0,0 +1,19 @@ +import { getCsrfToken } from './jest.utils'; + +const fakeCookies = [ + '_csrf=123; Path=/; HttpOnly; SameSite=Strict', + 'csrf_token=abc-123; Path=/', + 'sessionId=CV-abc.123; Path=/; Expires=Wed, 03 May 2023 16:29:53 GMT; HttpOnly' +]; + +describe('getCsrfToken', () => { + test('returns csrf token if there is one', () => { + expect(getCsrfToken(fakeCookies)).toEqual('abc-123'); + }); + + test('returns undefined if there is no csrf token', () => { + expect( + getCsrfToken(['_csrf=123; Path=/; HttpOnly; SameSite=Strict']) + ).toBeUndefined(); + }); +}); diff --git a/api/jest.utils.ts b/api/jest.utils.ts index 04369cdc7b8..ff5a88a5563 100644 --- a/api/jest.utils.ts +++ b/api/jest.utils.ts @@ -2,31 +2,17 @@ import request from 'supertest'; import { build } from './src/app'; +type FastifyTestInstance = Awaited>; + declare global { // eslint-disable-next-line no-var - var fastifyTestInstance: Awaited> | undefined; + var fastifyTestInstance: FastifyTestInstance; } type Options = { sendCSRFToken: boolean; }; -// TODO: remove this function and use superRequest instead -export function superPut( - resource: string, - setCookies: string[], - opts?: Options -): request.Test { - return superRequest( - resource, - { - method: 'PUT', - setCookies - }, - opts - ); -} - /* eslint-disable @typescript-eslint/naming-convention */ const requests = { GET: (resource: string) => request(fastifyTestInstance?.server).get(resource), @@ -72,7 +58,7 @@ export function superRequest( } export function setupServer(): void { - let fastify: Awaited> | undefined; + let fastify: FastifyTestInstance; beforeAll(async () => { fastify = await build(); await fastify.ready(); @@ -83,6 +69,6 @@ export function setupServer(): void { afterAll(async () => { // Due to a prisma bug, this is not enough, we need to --force-exit jest: // https://github.com/prisma/prisma/issues/18146 - await fastifyTestInstance?.close(); + await fastifyTestInstance.close(); }); } diff --git a/api/package.json b/api/package.json index 429bfa224d6..68be1cd55b0 100644 --- a/api/package.json +++ b/api/package.json @@ -6,6 +6,7 @@ "dependencies": { "@fastify/cookie": "^8.3.0", "@fastify/middie": "8.3", + "@fastify/csrf-protection": "6.3.0", "@fastify/session": "^10.1.1", "@fastify/swagger": "^8.3.1", "@fastify/swagger-ui": "^1.5.0", diff --git a/api/src/app.ts b/api/src/app.ts index 7f85f830d0c..15ae8dc1743 100644 --- a/api/src/app.ts +++ b/api/src/app.ts @@ -14,8 +14,9 @@ import MongoStore from 'connect-mongo'; import type { TypeBoxTypeProvider } from '@fastify/type-provider-typebox'; import fastifySwagger from '@fastify/swagger'; import fastifySwaggerUI from '@fastify/swagger-ui'; -import fastifySentry from './plugins/fastify-sentry'; +import fastifyCsrfProtection from '@fastify/csrf-protection'; +import fastifySentry from './plugins/fastify-sentry'; import cors from './plugins/cors'; import jwtAuthz from './plugins/fastify-jwt-authz'; import security from './plugins/security'; @@ -29,6 +30,7 @@ import prismaPlugin from './db/prisma'; import { AUTH0_AUDIENCE, AUTH0_DOMAIN, + COOKIE_DOMAIN, FREECODECAMP_NODE_ENV, MONGOHQ_URL, SESSION_SECRET, @@ -66,6 +68,33 @@ export const build = async ( await fastify.register(cors); await fastify.register(fastifyCookie); + + // @ts-expect-error @fastify/csrf-protection is overly restrictive, here. It + // requires an hmacKey if getToken is provided, but that should only be a + // requirement if the getUserInfo function is provided. + void fastify.register(fastifyCsrfProtection, { + // TODO: consider signing cookies. We don't on the api-server, but we could + // as an extra layer of security. + + ///Ignore all other possible sources of CSRF + // tokens since we know we can provide this one + getToken: req => req.headers['csrf-token'] as string + }); + + // All routes should add a CSRF token to the response + fastify.addHook('onRequest', (_req, reply, done) => { + const token = reply.generateCsrf(); + // Path is necessary to ensure that only one cookie is set and it is valid + // for all routes. + void reply.setCookie('csrf_token', token, { + path: '/', + sameSite: 'strict', + domain: COOKIE_DOMAIN, + secure: FREECODECAMP_NODE_ENV === 'production' + }); + done(); + }); + // @ts-expect-error - @fastify/session's types are not, yet, compatible with // express-session's types await fastify.register(fastifySession, { @@ -101,7 +130,21 @@ export const build = async ( security: [{ session: [] }] } }); - void fastify.register(fastifySwaggerUI); + void fastify.register(fastifySwaggerUI, { + uiConfig: { + // Convert csrf_token cookie to csrf-token header + requestInterceptor: req => { + const csrfTokenCookie = document.cookie + .split(';') + .find(str => str.includes('csrf_token')); + const [_key, csrfToken] = csrfTokenCookie?.split('=') ?? []; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (csrfToken) req.headers['csrf-token'] = csrfToken.trim(); + return req; + } + } + }); fastify.log.info(`Swagger UI available at ${API_LOCATION}/documentation`); } diff --git a/api/src/routes/auth.ts b/api/src/routes/auth.ts index b8a2db9fc9d..2036cbffebf 100644 --- a/api/src/routes/auth.ts +++ b/api/src/routes/auth.ts @@ -108,7 +108,7 @@ export const devLoginCallback: FastifyPluginCallback = ( _options, done ) => { - fastify.get('/dev-callback', async (req, _res) => { + fastify.get('/dev-callback', async req => { const email = 'foo@bar.com'; const { id } = await findOrCreateUser(fastify, email); @@ -122,7 +122,7 @@ export const devLoginCallback: FastifyPluginCallback = ( export const auth0Routes: FastifyPluginCallback = (fastify, _options, done) => { fastify.addHook('onRequest', fastify.authenticate); - fastify.get('/callback', async (req, _res) => { + fastify.get('/callback', async req => { const email = await getEmailFromAuth0(req); const { id } = await findOrCreateUser(fastify, email); diff --git a/api/src/routes/settings.test.ts b/api/src/routes/settings.test.ts index 8279583476a..24bd2a670d2 100644 --- a/api/src/routes/settings.test.ts +++ b/api/src/routes/settings.test.ts @@ -1,6 +1,6 @@ import request from 'supertest'; -import { build } from '../app'; +import { setupServer, superRequest } from '../../jest.utils'; const baseProfileUI = { isLocked: false, @@ -26,42 +26,88 @@ const profileUI = { }; describe('settingRoutes', () => { - let fastify: undefined | Awaited>; + setupServer(); - beforeAll(async () => { - fastify = await build(); - await fastify.ready(); - }, 20000); + // TODO: rather than testing every single route, create a single test for the + // rest api plugin that checks that the appropriate hooks are added to all - afterAll(async () => { - await fastify?.close(); + // This only tests one route, but all of the routes in the settings plugin add + // the same hooks. So if this suite passes, the other routes should be + // protected. + describe('CSRF protection', () => { + it('should return 403 if the _csrf secret is missing', async () => { + const response = await request(fastifyTestInstance.server).put( + '/update-my-profileui' + ); + + expect(response.statusCode).toEqual(403); + expect(response.body).toEqual({ + code: 'FST_CSRF_MISSING_SECRET', + error: 'Forbidden', + message: 'Missing csrf secret', + statusCode: 403 + }); + }); + + it('should return 403 if the csrf_token is invalid', async () => { + const response = await request(fastifyTestInstance.server) + .put('/update-my-profileui') + .set('Cookie', ['_csrf=foo', 'csrf-token=bar']); + + expect(response.statusCode).toEqual(403); + expect(response.body).toEqual({ + code: 'FST_CSRF_INVALID_TOKEN', + error: 'Forbidden', + message: 'Invalid csrf token', + statusCode: 403 + }); + }); + + it('should receive a new CSRF token + secret in the response', async () => { + const response = await request(fastifyTestInstance.server).put( + '/update-my-profileui' + ); + + const newCookies = response.get('Set-Cookie'); + expect(newCookies).toEqual( + expect.arrayContaining([ + expect.stringContaining('_csrf'), + expect.stringContaining('csrf_token') + ]) + ); + }); }); describe('Authenticated user', () => { - let cookies: string[]; + let setCookies: string[]; + // Authenticate user beforeAll(async () => { - await fastify?.prisma.user.updateMany({ + await fastifyTestInstance.prisma.user.updateMany({ where: { email: 'foo@bar.com' }, data: { profileUI: baseProfileUI } }); - const res = await request(fastify?.server).get('/auth/dev-callback'); - cookies = res.get('Set-Cookie'); + const res = await request(fastifyTestInstance.server).get( + '/auth/dev-callback' + ); + setCookies = res.get('Set-Cookie'); }); describe('/update-my-profileui', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-my-profileui') - .set('Cookie', cookies) - .send({ profileUI }); + const response = await superRequest('/update-my-profileui', { + method: 'PUT', + setCookies + }).send({ + profileUI + }); - const user = await fastify?.prisma.user.findFirst({ + const user = await fastifyTestInstance.prisma.user.findFirst({ where: { email: 'foo@bar.com' } }); - expect(response?.statusCode).toEqual(200); - expect(response?.body).toEqual({ + expect(response.statusCode).toEqual(200); + expect(response.body).toEqual({ message: 'flash.privacy-updated', type: 'success' }); @@ -69,40 +115,40 @@ describe('settingRoutes', () => { }); test('PUT ignores invalid keys', async () => { - const response = await request(fastify?.server) - .put('/update-my-profileui') - .set('Cookie', cookies) - .send({ - profileUI: { - ...profileUI, - invalidKey: 'invalidValue' - } - }); + const response = await superRequest('/update-my-profileui', { + method: 'PUT', + setCookies + }).send({ + profileUI: { + ...profileUI, + invalidKey: 'invalidValue' + } + }); - const user = await fastify?.prisma.user.findFirst({ + const user = await fastifyTestInstance.prisma.user.findFirst({ where: { email: 'foo@bar.com' } }); - expect(response?.statusCode).toEqual(200); + expect(response.statusCode).toEqual(200); expect(user?.profileUI).toEqual(profileUI); }); test('PUT returns 400 status code with missing keys', async () => { - const response = await request(fastify?.server) - .put('/update-my-profileui') - .set('Cookie', cookies) - .send({ - profileUI: { - isLocked: true, - showName: true, - showPoints: false, - showPortfolio: true, - showTimeLine: false - } - }); + const response = await superRequest('/update-my-profileui', { + method: 'PUT', + setCookies + }).send({ + profileUI: { + isLocked: true, + showName: true, + showPoints: false, + showPortfolio: true, + showTimeLine: false + } + }); - expect(response?.statusCode).toEqual(400); - expect(response?.body).toEqual({ + expect(response.statusCode).toEqual(400); + expect(response.body).toEqual({ code: 'FST_ERR_VALIDATION', error: 'Bad Request', message: `body/profileUI must have required property 'showAbout'`, @@ -113,35 +159,39 @@ describe('settingRoutes', () => { describe('/update-my-theme', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-my-theme') - .set('Cookie', cookies) - .send({ theme: 'night' }); + const response = await superRequest('/update-my-theme', { + method: 'PUT', + setCookies + }).send({ + theme: 'night' + }); - expect(response?.statusCode).toEqual(200); + expect(response.statusCode).toEqual(200); - expect(response?.body).toEqual({ + expect(response.body).toEqual({ message: 'flash.updated-themes', type: 'success' }); }); test('PUT returns 400 status code with invalid theme', async () => { - const response = await request(fastify?.server) - .put('/update-my-theme') - .set('Cookie', cookies) - .send({ theme: 'invalid' }); + const response = await superRequest('/update-my-theme', { + method: 'PUT', + setCookies + }).send({ + theme: 'invalid' + }); - expect(response?.statusCode).toEqual(400); + expect(response.statusCode).toEqual(400); }); }); describe('/update-my-keyboard-shortcuts', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-my-keyboard-shortcuts') - .set('Cookie', cookies) - .send({ keyboardShortcuts: true }); + const response = await superRequest('/update-my-keyboard-shortcuts', { + method: 'PUT', + setCookies + }).send({ keyboardShortcuts: true }); expect(response?.statusCode).toEqual(200); @@ -152,21 +202,21 @@ describe('settingRoutes', () => { }); test('PUT returns 400 status code with invalid shortcuts setting', async () => { - const response = await request(fastify?.server) - .put('/update-my-keyboard-shortcuts') - .set('Cookie', cookies) - .send({ keyboardShortcuts: 'invalid' }); + const response = await superRequest('/update-my-keyboard-shortcuts', { + method: 'PUT', + setCookies + }).send({ keyboardShortcuts: 'invalid' }); - expect(response?.statusCode).toEqual(400); + expect(response.statusCode).toEqual(400); }); }); describe('/update-my-quincy-email', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-my-quincy-email') - .set('Cookie', cookies) - .send({ sendQuincyEmail: true }); + const response = await superRequest('/update-my-quincy-email', { + method: 'PUT', + setCookies + }).send({ sendQuincyEmail: true }); expect(response?.statusCode).toEqual(200); @@ -177,10 +227,10 @@ describe('settingRoutes', () => { }); test('PUT returns 400 status code with invalid sendQuincyEmail', async () => { - const response = await request(fastify?.server) - .put('/update-my-quincy-email') - .set('Cookie', cookies) - .send({ sendQuincyEmail: 'invalid' }); + const response = await superRequest('/update-my-quincy-email', { + method: 'PUT', + setCookies + }).send({ sendQuincyEmail: 'invalid' }); expect(response?.statusCode).toEqual(400); }); @@ -188,10 +238,10 @@ describe('settingRoutes', () => { describe('/update-my-honesty', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-my-honesty') - .set('Cookie', cookies) - .send({ isHonest: true }); + const response = await superRequest('/update-my-honesty', { + method: 'PUT', + setCookies + }).send({ isHonest: true }); expect(response?.statusCode).toEqual(200); @@ -202,10 +252,10 @@ describe('settingRoutes', () => { }); test('PUT returns 400 status code with invalid honesty', async () => { - const response = await request(fastify?.server) - .put('/update-my-honesty') - .set('Cookie', cookies) - .send({ isHonest: false }); + const response = await superRequest('/update-my-honesty', { + method: 'PUT', + setCookies + }).send({ isHonest: false }); expect(response?.statusCode).toEqual(400); }); @@ -213,10 +263,10 @@ describe('settingRoutes', () => { describe('/update-privacy-terms', () => { test('PUT returns 200 status code with "success" message', async () => { - const response = await request(fastify?.server) - .put('/update-privacy-terms') - .set('Cookie', cookies) - .send({ quincyEmails: true }); + const response = await superRequest('/update-privacy-terms', { + method: 'PUT', + setCookies + }).send({ quincyEmails: true }); expect(response?.statusCode).toEqual(200); @@ -227,10 +277,10 @@ describe('settingRoutes', () => { }); test('PUT returns 400 status code with non-boolean data', async () => { - const response = await request(fastify?.server) - .put('/update-privacy-terms') - .set('Cookie', cookies) - .send({ quincyEmails: '123' }); + const response = await superRequest('/update-privacy-terms', { + method: 'PUT', + setCookies + }).send({ quincyEmails: '123' }); expect(response?.statusCode).toEqual(400); expect(response?.body).toEqual({ @@ -244,24 +294,37 @@ describe('settingRoutes', () => { }); describe('Unauthenticated User', () => { - test('PUT /update-my-profileui returns 401 status code for un-authenticated users', async () => { - const response = await request(fastify?.server).put( - '/update-my-profileui' - ); + let setCookies: string[]; - expect(response?.statusCode).toEqual(401); + // Get the CSRF cookies from an unprotected route + beforeAll(async () => { + const res = await request(fastifyTestInstance.server).get('/'); + setCookies = res.get('Set-Cookie'); + }); + + test('PUT /update-my-profileui returns 401 status code for un-authenticated users', async () => { + const response = await superRequest('/update-my-profileui', { + method: 'PUT', + setCookies + }); + + expect(response.statusCode).toEqual(401); }); test('PUT /update-my-theme returns 401 status code for un-authenticated users', async () => { - const response = await request(fastify?.server).put('/update-my-theme'); + const response = await superRequest('/update-my-theme', { + method: 'PUT', + setCookies + }); - expect(response?.statusCode).toEqual(401); + expect(response.statusCode).toEqual(401); }); test('PUT /update-privacy-terms returns 401 status code for un-authenticated users', async () => { - const response = await request(fastify?.server).put( - '/update-privacy-terms' - ); + const response = await superRequest('/update-privacy-terms', { + method: 'PUT', + setCookies + }); expect(response?.statusCode).toEqual(401); }); diff --git a/api/src/routes/settings.ts b/api/src/routes/settings.ts index 1f24230308b..3ccc8784bcf 100644 --- a/api/src/routes/settings.ts +++ b/api/src/routes/settings.ts @@ -8,6 +8,10 @@ export const settingRoutes: FastifyPluginCallbackTypebox = ( _options, done ) => { + // The order matters here, since we want to reject invalid cross site requests + // before checking if the user is authenticated. + // eslint-disable-next-line @typescript-eslint/unbound-method + fastify.addHook('onRequest', fastify.csrfProtection); fastify.addHook('onRequest', fastify.authenticateSession); fastify.put( diff --git a/api/src/server.test.ts b/api/src/server.test.ts index 9a04d682935..b7c81a68692 100644 --- a/api/src/server.test.ts +++ b/api/src/server.test.ts @@ -1,8 +1,38 @@ import { setupServer, superRequest } from '../jest.utils'; -import { HOME_LOCATION } from './utils/env'; +import { HOME_LOCATION, COOKIE_DOMAIN } from './utils/env'; + +jest.mock('./utils/env', () => { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return { + ...jest.requireActual('./utils/env'), + // eslint-disable-next-line @typescript-eslint/naming-convention + COOKIE_DOMAIN: '.freecodecamp.org' + }; +}); describe('server', () => { setupServer(); + + describe('CSRF protection', () => { + it('should receive a new CSRF token with the expected properties', async () => { + const response = await superRequest('/', { method: 'GET' }); + const newCookies = response.get('Set-Cookie'); + const csrfTokenCookie = newCookies.find(cookie => + cookie.includes('csrf_token') + ); + + expect(csrfTokenCookie).toEqual( + expect.stringContaining('SameSite=Strict') + ); + expect(csrfTokenCookie).toEqual( + expect.stringContaining(`Domain=${COOKIE_DOMAIN}`) + ); + expect(csrfTokenCookie).toEqual(expect.stringContaining('Path=/')); + // Since we're not mocking FREECODECAMP_NODE_ENV to production, there's no + // point checking if it is secure (it won't be in testing). + }); + }); + describe('GET /', () => { test('have a 200 response', async () => { const res = await superRequest('/', { method: 'GET' }); diff --git a/api/src/utils/env.ts b/api/src/utils/env.ts index e74938fd224..342621afada 100644 --- a/api/src/utils/env.ts +++ b/api/src/utils/env.ts @@ -33,6 +33,7 @@ assert.ok(process.env.FCC_ENABLE_SWAGGER_UI); assert.ok(process.env.FCC_ENABLE_DEV_LOGIN_MODE); if (process.env.FREECODECAMP_NODE_ENV !== 'development') { + assert.ok(process.env.COOKIE_DOMAIN); assert.ok(process.env.PORT); assert.ok(process.env.MONGOHQ_URL); assert.ok(process.env.SENTRY_DSN); @@ -70,3 +71,4 @@ export const SENTRY_DSN = process.env.SENTRY_DSN === 'dsn_from_sentry_dashboard' ? '' : process.env.SENTRY_DSN; +export const COOKIE_DOMAIN = process.env.COOKIE_DOMAIN || 'localhost'; diff --git a/api/tsconfig.json b/api/tsconfig.json index 9e31e075255..e16ba099ba4 100644 --- a/api/tsconfig.json +++ b/api/tsconfig.json @@ -7,6 +7,7 @@ "forceConsistentCasingInFileNames": true, "esModuleInterop": true, "resolveJsonModule": true, - "noEmit": true + "noEmit": true, + "noUncheckedIndexedAccess": true } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8ff1005a9a3..becbed8fdb8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -167,6 +167,9 @@ importers: '@fastify/cookie': specifier: ^8.3.0 version: 8.3.0 + '@fastify/csrf-protection': + specifier: 6.3.0 + version: 6.3.0 '@fastify/middie': specifier: '8.3' version: 8.3.0 @@ -6236,6 +6239,18 @@ packages: fastify-plugin: 4.5.0 dev: false + /@fastify/csrf-protection@6.3.0: + resolution: {integrity: sha512-RktewyPbVMSbKrRKmn0VzKfqtRp+0mYodAz7tbJMmz1QJFWFeP5F5mS8Ym1djaM0EFqCBItIceJizeLOD+UILA==} + dependencies: + '@fastify/csrf': 6.2.0 + '@fastify/error': 3.2.0 + fastify-plugin: 4.5.0 + dev: false + + /@fastify/csrf@6.2.0: + resolution: {integrity: sha512-DvBvhhAjTh4JxH7u/HAaXY6uawrEA3lGrJNTDF2R0sojwXdmFF50an2nnnHMBIsABRyNh9TueZjzgznRCe74yA==} + dev: false + /@fastify/deepmerge@1.3.0: resolution: {integrity: sha512-J8TOSBq3SoZbDhM9+R/u77hP93gz/rajSA+K2kGyijPpORPWUXHUpTaleoj+92As0S9uPRP7Oi8IqMf0u+ro6A==} dev: false