From 9a6e23639b10b49e96216d549e836018c42fcf7b Mon Sep 17 00:00:00 2001 From: Brainslug Date: Tue, 14 May 2024 14:01:40 +0200 Subject: [PATCH] Prevent "invalid token" from being blocking (#22459) * Throw a consistent invalid credentials error and remove invalid session cookies on the response * updated tests * prettier * Added tests for cookie clearing * prettier * Update api/src/middleware/authenticate.test.ts Co-authored-by: Pascal Jufer * Update api/src/middleware/authenticate.test.ts Co-authored-by: Pascal Jufer * Update api/src/middleware/authenticate.ts Co-authored-by: Pascal Jufer --------- Co-authored-by: Pascal Jufer --- api/src/middleware/authenticate.test.ts | 102 ++++++++++++++++++++++-- api/src/middleware/authenticate.ts | 20 ++++- api/src/utils/verify-session-jwt.ts | 4 +- 3 files changed, 114 insertions(+), 12 deletions(-) diff --git a/api/src/middleware/authenticate.test.ts b/api/src/middleware/authenticate.test.ts index d9ff6c0fa35ee..67718ec80b8f6 100644 --- a/api/src/middleware/authenticate.test.ts +++ b/api/src/middleware/authenticate.test.ts @@ -1,9 +1,8 @@ -import { useEnv } from '@directus/env'; import { InvalidCredentialsError } from '@directus/errors'; import type { Request, Response } from 'express'; import jwt from 'jsonwebtoken'; import type { Knex } from 'knex'; -import { afterEach, beforeEach, expect, test, vi } from 'vitest'; +import { afterEach, expect, test, vi } from 'vitest'; import getDatabase from '../database/index.js'; import emitter from '../emitter.js'; import '../types/express.d.ts'; @@ -13,14 +12,20 @@ vi.mock('../database/index'); // This is required because logger uses global env which is imported before the tests run. Can be // reduce to just mock the file when logger is also using useLogger everywhere @TODO -vi.mock('@directus/env', () => ({ useEnv: vi.fn().mockReturnValue({}) })); - -beforeEach(() => { - vi.mocked(useEnv).mockReturnValue({ +vi.mock('@directus/env', () => ({ + useEnv: vi.fn().mockReturnValue({ SECRET: 'test', EXTENSIONS_PATH: './extensions', - }); -}); + SESSION_COOKIE_NAME: 'directus_session', + // needed for constants.ts top level mocking + REFRESH_TOKEN_COOKIE_DOMAIN: '', + REFRESH_TOKEN_TTL: 0, + REFRESH_TOKEN_COOKIE_SECURE: false, + SESSION_COOKIE_DOMAIN: '', + SESSION_COOKIE_TTL: 0, + SESSION_COOKIE_SECURE: false, + }), +})); afterEach(() => { vi.clearAllMocks(); @@ -29,6 +34,7 @@ afterEach(() => { test('Short-circuits when authenticate filter is used', async () => { const req = { ip: '127.0.0.1', + cookies: {}, get: vi.fn(), } as unknown as Request; @@ -48,6 +54,7 @@ test('Short-circuits when authenticate filter is used', async () => { test('Uses default public accountability when no token is given', async () => { const req = { ip: '127.0.0.1', + cookies: {}, get: vi.fn((string) => { switch (string) { case 'user-agent': @@ -108,6 +115,7 @@ test('Sets accountability to payload contents if valid token is passed', async ( const req = { ip: '127.0.0.1', + cookies: {}, get: vi.fn((string) => { switch (string) { case 'user-agent': @@ -184,6 +192,7 @@ test('Throws InvalidCredentialsError when static token is used, but user does no const req = { ip: '127.0.0.1', + cookies: {}, get: vi.fn((string) => { switch (string) { case 'user-agent': @@ -207,6 +216,7 @@ test('Throws InvalidCredentialsError when static token is used, but user does no test('Sets accountability to user information when static token is used', async () => { const req = { ip: '127.0.0.1', + cookies: {}, get: vi.fn((string) => { switch (string) { case 'user-agent': @@ -266,3 +276,79 @@ test('Sets accountability to user information when static token is used', async expect(req.accountability).toEqual(expectedAccountability); expect(next).toHaveBeenCalledTimes(1); }); + +test('Invalid session token responds with error and clears the cookie', async () => { + const req = { + ip: '127.0.0.1', + cookies: { + directus_session: 'session-token', + }, + get: vi.fn((string) => { + switch (string) { + case 'user-agent': + return 'fake-user-agent'; + case 'origin': + return 'fake-origin'; + default: + return null; + } + }), + token: 'session-token', + } as unknown as Request; + + const res = { + clearCookie: vi.fn(), + } as unknown as Response; + + const next = vi.fn(); + + vi.mocked(getDatabase).mockReturnValue({ + select: vi.fn().mockReturnThis(), + from: vi.fn().mockReturnThis(), + leftJoin: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + first: vi.fn().mockResolvedValue(null), + } as unknown as Knex); + + await expect(handler(req, res, next)).rejects.toEqual(new InvalidCredentialsError()); + expect(res.clearCookie).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledTimes(0); +}); + +test('Invalid query token responds with error but does not clear the session cookie', async () => { + const req = { + ip: '127.0.0.1', + cookies: { + directus_session: 'session-token', + }, + get: vi.fn((string) => { + switch (string) { + case 'user-agent': + return 'fake-user-agent'; + case 'origin': + return 'fake-origin'; + default: + return null; + } + }), + token: 'static-token', + } as unknown as Request; + + const res = { + clearCookie: vi.fn(), + } as unknown as Response; + + const next = vi.fn(); + + vi.mocked(getDatabase).mockReturnValue({ + select: vi.fn().mockReturnThis(), + from: vi.fn().mockReturnThis(), + leftJoin: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + first: vi.fn().mockResolvedValue(null), + } as unknown as Knex); + + await expect(handler(req, res, next)).rejects.toEqual(new InvalidCredentialsError()); + expect(next).toHaveBeenCalledTimes(0); + expect(res.clearCookie).toHaveBeenCalledTimes(0); +}); diff --git a/api/src/middleware/authenticate.ts b/api/src/middleware/authenticate.ts index f9e0a794cfda8..bc2710c5af972 100644 --- a/api/src/middleware/authenticate.ts +++ b/api/src/middleware/authenticate.ts @@ -6,11 +6,16 @@ import emitter from '../emitter.js'; import asyncHandler from '../utils/async-handler.js'; import { getAccountabilityForToken } from '../utils/get-accountability-for-token.js'; import { getIPFromReq } from '../utils/get-ip-from-req.js'; +import { ErrorCode, isDirectusError } from '@directus/errors'; +import { useEnv } from '@directus/env'; +import { SESSION_COOKIE_OPTIONS } from '../constants.js'; /** * Verify the passed JWT and assign the user ID and role to `req` */ -export const handler = async (req: Request, _res: Response, next: NextFunction) => { +export const handler = async (req: Request, res: Response, next: NextFunction) => { + const env = useEnv(); + const defaultAccountability: Accountability = { user: null, role: null, @@ -45,7 +50,18 @@ export const handler = async (req: Request, _res: Response, next: NextFunction) return next(); } - req.accountability = await getAccountabilityForToken(req.token, defaultAccountability); + try { + req.accountability = await getAccountabilityForToken(req.token, defaultAccountability); + } catch (err) { + if (isDirectusError(err, ErrorCode.InvalidCredentials) || isDirectusError(err, ErrorCode.InvalidToken)) { + if (req.cookies[env['SESSION_COOKIE_NAME'] as string] === req.token) { + // clear the session token if ended up in an invalid state + res.clearCookie(env['SESSION_COOKIE_NAME'] as string, SESSION_COOKIE_OPTIONS); + } + } + + throw err; + } return next(); }; diff --git a/api/src/utils/verify-session-jwt.ts b/api/src/utils/verify-session-jwt.ts index 3a6bdeef2b49c..720df4c62632c 100644 --- a/api/src/utils/verify-session-jwt.ts +++ b/api/src/utils/verify-session-jwt.ts @@ -1,5 +1,5 @@ import getDatabase from '../database/index.js'; -import { InvalidTokenError } from '@directus/errors'; +import { InvalidCredentialsError } from '@directus/errors'; import type { DirectusTokenPayload } from '../types/index.js'; /** @@ -21,6 +21,6 @@ export async function verifySessionJWT(payload: DirectusTokenPayload) { .first(); if (!session) { - throw new InvalidTokenError(); + throw new InvalidCredentialsError(); } }