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

@@ -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.

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) {

src/lib/middleware/reject-double-slashes-in-path.ts

@@ -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();
-    }
-};

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);
 });