From 52904ee0383f8360225821159b9b39ccad2beadf Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Sat, 27 May 2023 16:29:54 +0200 Subject: [PATCH] fix: reject unauthorized client requests (#3881) If apiTokens are enabled breaks middleware chain with a 401 if no token is found for requests to client and frontend apis. Previously the middleware allowed the chain to process. Removes the regex search for multiple slashes, and instead configures the apiTokenMiddleware to reject unauthorized requests. --- src/lib/app.ts | 2 -- src/lib/middleware/api-token-middleware.ts | 20 ++++++++++++++----- .../reject-double-slashes-in-path.ts | 11 ---------- .../leading-slashes-are-stripped.e2e.test.ts | 14 +++++-------- 4 files changed, 20 insertions(+), 27 deletions(-) delete mode 100644 src/lib/middleware/reject-double-slashes-in-path.ts diff --git a/src/lib/app.ts b/src/lib/app.ts index 346c952641..6c1308b8db 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -29,7 +29,6 @@ import maintenanceMiddleware from './middleware/maintenance-middleware'; import { unless } from './middleware/unless-middleware'; import { catchAllErrorHandler } from './middleware/catch-all-error-handler'; import NotFoundError from './error/notfound-error'; -import { rejectDoubleSlashesInPath } from './middleware/reject-double-slashes-in-path'; export default async function getApp( config: IUnleashConfig, @@ -93,7 +92,6 @@ export default async function getApp( if (config.enableOAS && services.openApiService) { services.openApiService.useDocs(app); } - app.use(`${baseUriPath}/`, rejectDoubleSlashesInPath); // Support CORS preflight requests for the frontend endpoints. // Preflight requests should not have Authorization headers, // so this must be handled before the API token middleware. diff --git a/src/lib/middleware/api-token-middleware.ts b/src/lib/middleware/api-token-middleware.ts index 0fd689f98e..3c1b3046ad 100644 --- a/src/lib/middleware/api-token-middleware.ts +++ b/src/lib/middleware/api-token-middleware.ts @@ -4,7 +4,7 @@ import { IUnleashConfig } from '../types/option'; import { IAuthRequest } from '../routes/unleash-types'; const isClientApi = ({ path }) => { - return path && path.startsWith('/api/client'); + return path && path.indexOf('/api/client') > -1; }; const isProxyApi = ({ path }) => { @@ -15,16 +15,18 @@ const isProxyApi = ({ path }) => { // Handle all our current proxy paths which will redirect to the new // embedded proxy endpoint return ( - path.startsWith('/api/proxy') || - path.startsWith('/api/development/proxy') || - path.startsWith('/api/production/proxy') || - path.startsWith('/api/frontend') + path.indexOf('/api/proxy') > -1 || + path.indexOf('/api/development/proxy') > -1 || + path.indexOf('/api/production/proxy') > -1 || + path.indexOf('/api/frontend') > -1 ); }; export const TOKEN_TYPE_ERROR_MESSAGE = 'invalid token: expected a different token type for this endpoint'; +export const NO_TOKEN_WHERE_TOKEN_WAS_REQUIRED = + 'This endpoint requires an API token. Please add an authorization header to your request with a valid token'; const apiAccessMiddleware = ( { getLogger, @@ -64,6 +66,14 @@ const apiAccessMiddleware = ( return; } req.user = apiUser; + } else if (isClientApi(req) || isProxyApi(req)) { + // If we're here, we know that api token middleware was enabled, otherwise we'd returned a no-op middleware + // We explicitly only protect client and proxy apis, since admin apis are protected by our permission checker + // Reject with 401 + res.status(401).send({ + message: NO_TOKEN_WHERE_TOKEN_WAS_REQUIRED, + }); + return; } } } catch (error) { diff --git a/src/lib/middleware/reject-double-slashes-in-path.ts b/src/lib/middleware/reject-double-slashes-in-path.ts deleted file mode 100644 index 5d5388ddb0..0000000000 --- a/src/lib/middleware/reject-double-slashes-in-path.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { RequestHandler } from 'express'; - -const MULTIPLE_SLASHES = /\/\/+/; - -export const rejectDoubleSlashesInPath: RequestHandler = (req, res, next) => { - if (req.path.match(MULTIPLE_SLASHES)) { - res.status(404).send(); - } else { - next(); - } -}; diff --git a/src/test/e2e/api/auth/leading-slashes-are-stripped.e2e.test.ts b/src/test/e2e/api/auth/leading-slashes-are-stripped.e2e.test.ts index 0feb8533e0..82646ea790 100644 --- a/src/test/e2e/api/auth/leading-slashes-are-stripped.e2e.test.ts +++ b/src/test/e2e/api/auth/leading-slashes-are-stripped.e2e.test.ts @@ -19,7 +19,7 @@ beforeAll(async () => { authentication: { enableApiToken: true, type: IAuthType.DEMO }, }); appWithBaseUrl = await setupAppWithAuth(stores, { - server: { baseUriPath: '/demo' }, + server: { unleashUrl: 'http://localhost:4242', basePathUri: '/demo' }, authentication: { enableApiToken: true, type: IAuthType.DEMO }, }); }); @@ -31,17 +31,13 @@ afterAll(async () => { test('Access to /api/client/features are refused no matter how many leading slashes', async () => { await app.request.get('/api/client/features').expect(401); - await app.request.get('/////api/client/features').expect(404); - await app.request.get('//api/client/features').expect(404); -}); - -test('Multiple slashes anywhere in the path is not a URL that exists', async () => { - await app.request.get('/api/admin///projects/default/features').expect(404); - await app.request.get('/api/client///features').expect(404); + await app.request.get('/////api/client/features').expect(401); + await app.request.get('//api/client/features').expect(401); }); test('multiple slashes after base path is also rejected with 404', async () => { - await appWithBaseUrl.request.get('/demo///api/client/features').expect(404); + await appWithBaseUrl.request.get('/demo///api/client/features').expect(401); + await appWithBaseUrl.request.get('/demo//api/client/features').expect(401); await appWithBaseUrl.request.get('/demo/api/client/features').expect(401); });