Merge pull request #233 from AikidoSec/patch-url

Only discover endpoints/routes that exist

Author: willem-delbare

library/agent/Agent.test.ts

@@ -512,11 +512,9 @@ t.test("it sends hostnames and routes along with heartbeat", async () => {
   agent.onConnectHostname("aikido.dev", 80);
   agent.onConnectHostname("google.com", 443);
   agent.onRouteExecute("POST", "/posts/:id");
-  agent.onRouteExecute("OPTIONS", "/posts/:id");
   agent.onRouteExecute("POST", "/posts/:id");
   agent.onRouteExecute("GET", "/posts/:id");
   agent.onRouteExecute("GET", "/");
-  agent.onRouteExecute("HEAD", "/");
 
   api.clear();
 

library/agent/Agent.ts

@@ -412,12 +412,6 @@ export class Agent {
   }
 
   onRouteExecute(method: string, path: string) {
-    const excludedMethods = ["OPTIONS", "HEAD"];
-
-    if (excludedMethods.includes(method)) {
-      return;
-    }
-
     this.routes.addRoute(method, path);
   }
 

library/sources/HTTPServer.test.ts

@@ -171,9 +171,9 @@ t.test("it discovers routes", async () => {
   });
 
   await new Promise<void>((resolve) => {
-    server.listen(3318, () => {
+    server.listen(3340, () => {
       fetch({
-        url: new URL("http://localhost:3318/foo/bar"),
+        url: new URL("http://localhost:3340/foo/bar"),
         method: "GET",
         headers: {},
         timeoutInMS: 500,
@@ -195,6 +195,38 @@ t.test("it discovers routes", async () => {
   });
 });
 
+t.test(
+  "it does not discover route if server response is error code",
+  async () => {
+    const http = require("http");
+    const server = http.createServer((req, res) => {
+      res.statusCode = 404;
+      res.end();
+    });
+
+    await new Promise<void>((resolve) => {
+      server.listen(3341, () => {
+        fetch({
+          url: new URL("http://localhost:3341/not-found"),
+          method: "GET",
+          headers: {},
+          timeoutInMS: 500,
+        }).then(() => {
+          t.equal(
+            agent
+              .getRoutes()
+              .asArray()
+              .find((route) => route.path === "/not-found"),
+            undefined
+          );
+          server.close();
+          resolve();
+        });
+      });
+    });
+  }
+);
+
 t.test("it parses cookies", async () => {
   const http = require("http");
   const server = http.createServer((req, res) => {

library/sources/http-server/createRequestListener.ts

@@ -1,16 +1,11 @@
-import type {
-  IncomingMessage,
-  OutgoingMessage,
-  RequestListener,
-  ServerResponse,
-} from "http";
+import type { IncomingMessage, RequestListener, ServerResponse } from "http";
 import { Agent } from "../../agent/Agent";
 import { getContext, runWithContext } from "../../agent/Context";
-import { buildRouteFromURL } from "../../helpers/buildRouteFromURL";
 import { escapeHTML } from "../../helpers/escapeHTML";
 import { shouldRateLimitRequest } from "../../ratelimiting/shouldRateLimitRequest";
 import { contextFromRequest } from "./contextFromRequest";
 import { readBodyStream } from "./readBodyStream";
+import { shouldDiscoverRoute } from "./shouldDiscoverRoute";
 
 export function createRequestListener(
   listener: Function,
@@ -50,13 +45,23 @@ function callListenerWithContext(
 ) {
   const context = contextFromRequest(req, body, module);
 
-  if (context.route && context.method) {
-    agent.onRouteExecute(context.method, context.route);
-  }
-
   return runWithContext(context, () => {
     res.on("finish", () => {
       const context = getContext();
+
+      if (
+        context &&
+        context.route &&
+        context.method &&
+        shouldDiscoverRoute({
+          statusCode: res.statusCode,
+          route: context.route,
+          method: context.method,
+        })
+      ) {
+        agent.onRouteExecute(context.method, context.route);
+      }
+
       agent.getInspectionStatistics().onRequest();
       if (context && context.attackDetected) {
         agent.getInspectionStatistics().onDetectedAttack({

library/sources/http-server/shouldDiscoverRoute.test.ts

@@ -0,0 +1,148 @@
+import * as t from "tap";
+import { shouldDiscoverRoute } from "./shouldDiscoverRoute";
+
+t.test(
+  "it does not discover route if not found or method not allowed",
+  async () => {
+    t.same(
+      shouldDiscoverRoute({ statusCode: 404, route: "/", method: "GET" }),
+      false
+    );
+    t.same(
+      shouldDiscoverRoute({ statusCode: 405, route: "/", method: "GET" }),
+      false
+    );
+  }
+);
+
+t.test("it discovers route for all other status codes", async () => {
+  t.same(
+    shouldDiscoverRoute({ statusCode: 200, route: "/", method: "GET" }),
+    true
+  );
+  t.same(
+    shouldDiscoverRoute({ statusCode: 500, route: "/", method: "GET" }),
+    true
+  );
+  t.same(
+    shouldDiscoverRoute({ statusCode: 400, route: "/", method: "GET" }),
+    true
+  );
+  t.same(
+    shouldDiscoverRoute({ statusCode: 300, route: "/", method: "GET" }),
+    true
+  );
+  t.same(
+    shouldDiscoverRoute({ statusCode: 201, route: "/", method: "GET" }),
+    true
+  );
+});
+
+t.test("it does not discover route for OPTIONS or HEAD methods", async () => {
+  t.same(
+    shouldDiscoverRoute({ statusCode: 200, route: "/", method: "OPTIONS" }),
+    false
+  );
+  t.same(
+    shouldDiscoverRoute({ statusCode: 200, route: "/", method: "HEAD" }),
+    false
+  );
+});
+
+t.test(
+  "it does not discover route for OPTIONS or HEAD methods even with other status codes",
+  async () => {
+    t.same(
+      shouldDiscoverRoute({ statusCode: 404, route: "/", method: "OPTIONS" }),
+      false
+    );
+    t.same(
+      shouldDiscoverRoute({ statusCode: 405, route: "/", method: "HEAD" }),
+      false
+    );
+  }
+);
+
+t.test("it does not discover static files", async () => {
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/service-worker.js",
+      method: "GET",
+    }),
+    false
+  );
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/precache-manifest.10faec0bee24db502c8498078126dd53.js",
+      method: "POST",
+    }),
+    false
+  );
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/img/icons/favicon-16x16.png",
+      method: "GET",
+    }),
+    false
+  );
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/fonts/icomoon.ttf",
+      method: "GET",
+    }),
+    false
+  );
+});
+
+t.test("it allows html files", async () => {
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/index.html",
+      method: "GET",
+    }),
+    false
+  );
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/contact.html",
+      method: "GET",
+    }),
+    false
+  );
+});
+
+t.test("it allows files with extension of one character", async () => {
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/a.a",
+      method: "GET",
+    }),
+    true
+  );
+});
+
+t.test("it allows files with extension of 5 or more characters", async () => {
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/a.aaaaa",
+      method: "GET",
+    }),
+    true
+  );
+  t.same(
+    shouldDiscoverRoute({
+      statusCode: 200,
+      route: "/a.aaaaaa",
+      method: "GET",
+    }),
+    true
+  );
+});

library/sources/http-server/shouldDiscoverRoute.ts

@@ -0,0 +1,38 @@
+import { extname } from "path";
+
+const NOT_FOUND = 404;
+const METHOD_NOT_ALLOWED = 405;
+const ERROR_CODES = [NOT_FOUND, METHOD_NOT_ALLOWED];
+
+export function shouldDiscoverRoute({
+  statusCode,
+  route,
+  method,
+}: {
+  statusCode: number;
+  route: string;
+  method: string;
+}) {
+  const excludedMethods = ["OPTIONS", "HEAD"];
+
+  if (excludedMethods.includes(method)) {
+    return false;
+  }
+
+  if (ERROR_CODES.includes(statusCode)) {
+    return false;
+  }
+
+  let extension = extname(route);
+
+  if (extension && extension.startsWith(".")) {
+    // Remove the dot from the extension
+    extension = extension.slice(1);
+
+    if (extension.length >= 2 && extension.length <= 4) {
+      return false;
+    }
+  }
+
+  return true;
+}