diff --git a/api/jest.utils.ts b/api/jest.utils.ts index 6ddb7b706ac..2afbaba0115 100644 --- a/api/jest.utils.ts +++ b/api/jest.utils.ts @@ -3,6 +3,7 @@ import request from 'supertest'; import { build } from './src/app'; import { createUserInput } from './src/utils/create-user'; import { examJson } from './__mocks__/exam'; +import { CSRF_COOKIE, CSRF_HEADER } from './src/plugins/csrf'; type FastifyTestInstance = Awaited>; @@ -25,7 +26,7 @@ const requests = { }; export const getCsrfToken = (setCookies: string[]): string | undefined => { - const csrfSetCookie = setCookies.find(str => str.includes('csrf_token')); + const csrfSetCookie = setCookies.find(str => str.includes(CSRF_COOKIE)); const [csrfCookie] = csrfSetCookie?.split(';') ?? []; const [_key, csrfToken] = csrfCookie?.split('=') ?? []; @@ -72,7 +73,7 @@ export function superRequest( const csrfToken = (setCookies && getCsrfToken(setCookies)) ?? ''; if (sendCSRFToken) { - void req.set('CSRF-Token', csrfToken); + void req.set(CSRF_HEADER, csrfToken); } return req; } diff --git a/api/src/app.ts b/api/src/app.ts index 17d3d7473af..0177fbf8814 100644 --- a/api/src/app.ts +++ b/api/src/app.ts @@ -1,5 +1,4 @@ import fastifyAccepts from '@fastify/accepts'; -import fastifyCsrfProtection from '@fastify/csrf-protection'; import fastifySwagger from '@fastify/swagger'; import fastifySwaggerUI from '@fastify/swagger-ui'; import type { TypeBoxTypeProvider } from '@fastify/type-provider-typebox'; @@ -26,6 +25,7 @@ import security from './plugins/security'; import auth from './plugins/auth'; import bouncer from './plugins/bouncer'; import errorHandling from './plugins/error-handling'; +import csrf, { CSRF_COOKIE, CSRF_HEADER } from './plugins/csrf'; import notFound from './plugins/not-found'; import * as publicRoutes from './routes/public'; import * as protectedRoutes from './routes/protected'; @@ -88,33 +88,7 @@ export const build = async ( await fastify.register(cors); await fastify.register(cookies); - - 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, - cookieOpts: { signed: false, sameSite: 'strict' } - }); - - // All routes except signout should add a CSRF token to the response - fastify.addHook('onRequest', (_req, reply, done) => { - const isSignout = _req.url === '/signout' || _req.url === '/signout/'; - - if (!isSignout) { - const token = reply.generateCsrf(); - void reply.setCookie('csrf_token', token, { - sameSite: 'strict', - signed: false, - // it needs to be read by the client, so that it can be sent in the - // header of the next request: - httpOnly: false - }); - } - done(); - }); + await fastify.register(csrf); const provider = EMAIL_PROVIDER === 'ses' ? new SESProvider() : new NodemailerProvider(); @@ -136,11 +110,11 @@ export const build = async ( requestInterceptor: req => { const csrfTokenCookie = document.cookie .split(';') - .find(str => str.includes('csrf_token')); + .find(str => str.includes(CSRF_COOKIE)); const [_key, csrfToken] = csrfTokenCookie?.split('=') ?? []; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (csrfToken) req.headers['csrf-token'] = csrfToken.trim(); + if (csrfToken) req.headers[CSRF_HEADER] = csrfToken.trim(); return req; } } diff --git a/api/src/plugins/cookies.ts b/api/src/plugins/cookies.ts index 6611855da74..337b48d3a85 100644 --- a/api/src/plugins/cookies.ts +++ b/api/src/plugins/cookies.ts @@ -7,6 +7,7 @@ import { COOKIE_SECRET, FREECODECAMP_NODE_ENV } from '../utils/env'; +import { CSRF_COOKIE, CSRF_SECRET_COOKIE } from './csrf'; export { type CookieSerializeOptions } from '@fastify/cookie'; @@ -68,8 +69,8 @@ const cookies: FastifyPluginCallback = (fastify, _options, done) => { void fastify.decorateReply('clearOurCookies', function () { void this.clearCookie('jwt_access_token'); - void this.clearCookie('_csrf'); - void this.clearCookie('csrf_token'); + void this.clearCookie(CSRF_SECRET_COOKIE); + void this.clearCookie(CSRF_COOKIE); }); done(); diff --git a/api/src/plugins/csrf.test.ts b/api/src/plugins/csrf.test.ts new file mode 100644 index 00000000000..675beba0469 --- /dev/null +++ b/api/src/plugins/csrf.test.ts @@ -0,0 +1,107 @@ +import Fastify, { type FastifyInstance } from 'fastify'; + +import { COOKIE_DOMAIN } from '../utils/env'; +import cookies from './cookies'; +import csrf, { CSRF_COOKIE, CSRF_SECRET_COOKIE } from './csrf'; + +// eslint-disable-next-line @typescript-eslint/no-unsafe-return +jest.mock('../utils/env', () => ({ + ...jest.requireActual('../utils/env'), + COOKIE_DOMAIN: 'www.example.com', + FREECODECAMP_NODE_ENV: 'production' +})); + +async function setupServer() { + const fastify = Fastify(); + await fastify.register(cookies); + await fastify.register(csrf); + // @ts-expect-error - @fastify/csrf-protection needs to update their types + // eslint-disable-next-line @typescript-eslint/unbound-method + fastify.addHook('onRequest', fastify.csrfProtection); + + fastify.get('/', (_req, reply) => { + void reply.send({ foo: 'bar' }); + }); + return fastify; +} + +describe('CSRF protection', () => { + let fastify: FastifyInstance; + beforeEach(async () => { + fastify = await setupServer(); + }); + it('should receive a new CSRF token with the expected properties', async () => { + const response = await fastify.inject({ + method: 'GET', + url: '/' + }); + const newCookies = response.cookies; + const csrfTokenCookie = newCookies.find( + cookie => cookie.name === CSRF_COOKIE + ); + + const { value, ...rest } = csrfTokenCookie!; + + // The value is a random string - it's enough to check that it's not empty + expect(value).toHaveLength(52); + + expect(rest).toStrictEqual({ + name: CSRF_COOKIE, + path: '/', + sameSite: 'Strict', + domain: COOKIE_DOMAIN, + secure: true + }); + }); + + it('should return 403 if the _csrf secret is missing', async () => { + const response = await fastify.inject({ + method: 'GET', + url: '/' + }); + + expect(response.statusCode).toEqual(403); + // The response body is determined by the error-handling plugin, so we don't + // check it here. + }); + + it('should return 403 if the csrf_token is invalid', async () => { + const response = await fastify.inject({ + method: 'GET', + url: '/', + cookies: { + _csrf: 'foo', + csrf_token: 'bar' + } + }); + expect(response.statusCode).toEqual(403); + }); + + it('should allow the request if the csrf_token is valid', async () => { + const csrfResponse = await fastify.inject({ + method: 'GET', + url: '/' + }); + + const csrfTokenCookie = csrfResponse.cookies.find( + cookie => cookie.name === CSRF_COOKIE + ); + const csrfSecretCookie = csrfResponse.cookies.find( + cookie => cookie.name === CSRF_SECRET_COOKIE + ); + + const res = await fastify.inject({ + method: 'GET', + url: '/', + cookies: { + _csrf: csrfSecretCookie!.value + }, + headers: { + 'csrf-token': csrfTokenCookie!.value + } + }); + + expect(res.json()).toEqual({ foo: 'bar' }); + expect(res.statusCode).toEqual(200); + }); +}); diff --git a/api/src/plugins/csrf.ts b/api/src/plugins/csrf.ts new file mode 100644 index 00000000000..dd7f97964ad --- /dev/null +++ b/api/src/plugins/csrf.ts @@ -0,0 +1,48 @@ +import type { FastifyPluginCallback } from 'fastify'; +import fastifyCsrfProtection from '@fastify/csrf-protection'; + +import fp from 'fastify-plugin'; + +export const CSRF_COOKIE = 'csrf_token'; +export const CSRF_HEADER = 'csrf-token'; +export const CSRF_SECRET_COOKIE = '_csrf'; + +/** + * Plugin for preventing CSRF attacks. + * + * @param fastify The Fastify instance. + * @param _options Options passed to the plugin via `fastify.register(plugin, options)`. + * @param done Callback to signal that the logic has completed. + */ +const csrf: FastifyPluginCallback = (fastify, _options, done) => { + 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_HEADER] as string, + cookieOpts: { signed: false, sameSite: 'strict' } + }); + + // All routes except signout should add a CSRF token to the response + fastify.addHook('onRequest', (_req, reply, done) => { + const isSignout = _req.url === '/signout' || _req.url === '/signout/'; + + if (!isSignout) { + const token = reply.generateCsrf(); + void reply.setCookie(CSRF_COOKIE, token, { + sameSite: 'strict', + signed: false, + // it needs to be read by the client, so that it can be sent in the + // header of the next request: + httpOnly: false + }); + } + done(); + }); + + done(); +}; + +export default fp(csrf, { dependencies: ['cookies'] }); diff --git a/api/src/routes/protected/settings.test.ts b/api/src/routes/protected/settings.test.ts index 34952be9623..bbc9b0e9377 100644 --- a/api/src/routes/protected/settings.test.ts +++ b/api/src/routes/protected/settings.test.ts @@ -53,52 +53,6 @@ const updateErrorResponse = { describe('settingRoutes', () => { setupServer(); - // 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 - - // 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 superRequest('/update-my-profileui', { - method: 'PUT' - }); - - expect(response.body).toEqual({ - message: 'flash.generic-error', - type: 'danger' - }); - expect(response.statusCode).toEqual(403); - }); - - it('should return 403 if the csrf_token is invalid', async () => { - const response = await superRequest('/update-my-profileui', { - method: 'PUT' - }).set('Cookie', ['_csrf=foo', 'csrf-token=bar']); - - expect(response.body).toEqual({ - message: 'flash.generic-error', - type: 'danger' - }); - expect(response.statusCode).toEqual(403); - }); - - it('should receive a new CSRF token + secret in the response', async () => { - const response = await superRequest('/update-my-profileui', { - method: 'PUT' - }); - - const newCookies = response.get('Set-Cookie'); - expect(newCookies).toEqual( - expect.arrayContaining([ - expect.stringContaining('_csrf'), - expect.stringContaining('csrf_token') - ]) - ); - }); - }); - describe('Authenticated user', () => { let superPut: ReturnType; let superGet: ReturnType; diff --git a/api/src/routes/public/signout.ts b/api/src/routes/public/signout.ts index 6a96d436eba..a7367255144 100644 --- a/api/src/routes/public/signout.ts +++ b/api/src/routes/public/signout.ts @@ -16,9 +16,7 @@ export const signoutRoute: FastifyPluginCallback = ( done ) => { fastify.get('/signout', async (req, reply) => { - void reply.clearCookie('jwt_access_token'); - void reply.clearCookie('csrf_token'); - void reply.clearCookie('_csrf'); + void reply.clearOurCookies(); const { returnTo } = getRedirectParams(req); await reply.redirect(returnTo); diff --git a/api/src/server.test.ts b/api/src/server.test.ts index aa27bd59a0a..751dae951a8 100644 --- a/api/src/server.test.ts +++ b/api/src/server.test.ts @@ -1,5 +1,5 @@ import { setupServer, superRequest } from '../jest.utils'; -import { HOME_LOCATION, COOKIE_DOMAIN } from './utils/env'; +import { HOME_LOCATION } from './utils/env'; jest.mock('./utils/env', () => { // eslint-disable-next-line @typescript-eslint/no-unsafe-return @@ -12,26 +12,6 @@ jest.mock('./utils/env', () => { describe('server', () => { setupServer(); - describe('CSRF protection', () => { - it('should receive a new CSRF token with the expected properties', async () => { - const response = await superRequest('/status/ping', { 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('should have OWASP recommended headers', async () => { const res = await superRequest('/', { method: 'GET' });