Merge pull request #94 from IABTechLab/aul-uid2-4448-sanitize-paths-f…

…or-metrics Sanitize paths for Prometheus
IABTechLab · Nov 14, 2024 · 37f277d · 37f277d
2 parents ac9e0f6 + 0b1d802
commit 37f277d
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 48 deletions.
diff --git a/src/app.ts b/src/app.ts
@@ -33,7 +33,7 @@ app.use(
   makeMetricsApiMiddleware({
     port: 9082,
     isNormalizePathEnabled: true,
-    discardUnmatched: false,
+    discardUnmatched: true,
   }),
 );
 

diff --git a/src/middleware/metrics.ts b/src/middleware/metrics.ts
@@ -8,6 +8,14 @@ import UrlPattern from 'url-pattern';
 
 import logger from '../utils/logging';
 
+const withoutTrailingSlash = (path: string) => {
+  // Express routes don't include a trailing slash unless it's actually just `/` path, then it stays
+  if (path !== '/' && path.endsWith('/')) {
+    return path.slice(0, -1);
+  }
+  return path;
+};
+
 type Options = {
   port?: number,
   isNormalizePathEnabled?: boolean,
@@ -32,69 +40,46 @@ const setupMetricService = ({ port }: Options = { port: 9082 }) => {
   });
 };
 
-const captureAllRoutes = ({ urlPatternMaker }: Options, app: express.Express) => {
-  const allRoutes = listEndpoints(app).filter(
-    (route) => route.path !== '/*' && route.path !== '*',
-  );
-
-  return allRoutes.map((route) => {
-    const path = route.path.endsWith('/') ? route.path.replace(/\/$/, '') : route.path;
-
-    logger.log('debug', `Route found: ${route.path} - ${JSON.stringify(route)}`);
-    // route.pattern = route.path; // TODO remove?
-
-    // NOTE: urlPatternMaker has to create an UrlPattern compatible object.
-    const pattern = urlPatternMaker ? urlPatternMaker(route.path) : new UrlPattern(route.path, {
-      segmentNameCharset: 'a-zA-Z0-9_-',
-    }).toString();
-
-    if (!pattern) {
-      logger.log('debug', 'skipping route due to empty path');
-    }
-
-    return { ...route, path: pattern || path };
-  });
+type Route = {
+  methods: string[],
+  path: string,
+  pattern: UrlPattern
 };
-
 const makeMetricsApiMiddleware = (options: Options = {}) => {
   const { isNormalizePathEnabled, discardUnmatched, createServer } = options;
-  const allRoutes = [] as { methods: string[], path: string, pattern?: UrlPattern }[];
+  let allRoutes: Route[];
 
+  // This function takes a path request and returns a path that should go into the `path` label of the metric
+  // The goal here is to not have an infinite number of paths so get rid of path variables and unknown paths
   const normalizePath: NormalizePathFn = (req, _opts) => {
     if (!isNormalizePathEnabled) {
       return req.url;
     }
 
-    const path = (() => {
-      const parsedPath = url.parse(req.originalUrl || req.url).pathname || req.url;
-      if (parsedPath.endsWith('/')) {
-        // Remove trailing slash
-        return parsedPath.replace(/\/$/, '');
-      }
-      return parsedPath;
-    })();
-
-    const pattern = allRoutes.filter((route) => {
-      if (route.methods.indexOf(req.method) === -1) {
+    const parsedPath = url.parse(req.originalUrl || req.url).pathname || req.url;
+    const path = withoutTrailingSlash(parsedPath);
+
+    const matchingRoute = allRoutes.find((route) => {
+      if (!route.methods.includes(req.method)) {
         return false;
       }
 
+      // match will be null if it doesn't fit the pattern and will be some object depending on path params if it does
+      // eg path /abc/123 will match route /abc/:id with object {id: 123} but will return null for route /xyz/:id
       try {
-        if (route.path.match(path)) {
-          return true;
-        }
+        const match: null | object = route.pattern.match(path);
+        return match !== null;
       } catch (e: unknown) {
         logger.error('Error: something went wrong.');
         return false;
       }
-      return false;
-    })[0]?.pattern;
+    });
 
-    if (discardUnmatched && !pattern) {
-      return '';
+    if (discardUnmatched && !matchingRoute) {
+      return 'unmatched-url';
     }
 
-    return pattern?.stringify() || path || '';
+    return matchingRoute?.path ?? 'no-path';
   };
 
   const metricsMiddleware = promBundle({
@@ -113,11 +98,18 @@ const makeMetricsApiMiddleware = (options: Options = {}) => {
   }
 
   const handler: RequestHandler = (req, res, next) => {
-    if (allRoutes.length === 0) {
+    if (!allRoutes) {
+      // Scrape all the paths registered with express on the first recording of metrics. These paths will later be used
+      // to ensure we don't use unknown paths (ie spam calls) in metric labels and don't overwhelm Prometheus.
+      // Unfortunately, this doesn't include static paths since they are not registered with Express. If desired,
+      // we could add them by recursively listing /public.
       try {
-        captureAllRoutes(options, req.app as express.Express).forEach((route) => {
-          allRoutes.push(route);
-        });
+        allRoutes = listEndpoints(req.app as express.Express)
+          .filter((route) => route.path !== '/*' && route.path !== '*')
+          .map((route) => ({
+            ...route,
+            pattern: new UrlPattern(route.path),
+          }));
       } catch (e) {
         logger.log('error', `unable to capture route for prom-metrics: ${e}`);
       }