Merge pull request #453 from AikidoSec/express-ghost

Make non-owned props of express wrapped functions accessible

library/helpers/fetch.ts

@@ -52,16 +52,16 @@ async function request({
 
 export async function fetch({
   url,
-  method,
-  headers,
+  method = "GET",
+  headers = {},
   body = "",
-  timeoutInMS,
+  timeoutInMS = 5000,
 }: {
   url: URL;
-  method: string;
-  headers: Record<string, string>;
+  method?: string;
+  headers?: Record<string, string>;
   body?: string;
-  timeoutInMS: number;
+  timeoutInMS?: number;
 }): Promise<{ body: string; statusCode: number }> {
   const abort = new AbortController();
 

library/sources/Express.test.ts

@@ -5,6 +5,7 @@ import { Express } from "./Express";
 import { FileSystem } from "../sinks/FileSystem";
 import { HTTPServer } from "./HTTPServer";
 import { createTestAgent } from "../helpers/createTestAgent";
+import { fetch } from "../helpers/fetch";
 
 // Before require("express")
 const agent = createTestAgent({
@@ -124,6 +125,20 @@ function getApp(userMiddleware = true) {
   // A middleware that is used as a route
   app.use("/api/*path", apiMiddleware);
 
+  const newRouter = express.Router();
+  newRouter.get("/nested-router", (req, res) => {
+    res.send(getContext());
+  });
+
+  app.use(newRouter);
+
+  const nestedApp = express();
+  nestedApp.get("/", (req, res) => {
+    res.send(getContext());
+  });
+
+  app.use("/nested-app", nestedApp);
+
   app.get("/", (req, res) => {
     const context = getContext();
 
@@ -554,3 +569,91 @@ t.test("it preserves original function name in Layer object", async () => {
     1
   );
 });
+
+t.test("it supports nested router", async () => {
+  const response = await request(getApp()).get("/nested-router");
+
+  t.match(response.body, {
+    method: "GET",
+    source: "express",
+    route: "/nested-router",
+  });
+});
+
+t.test("it supports nested app", async (t) => {
+  const response = await request(getApp()).get("/nested-app");
+
+  t.match(response.body, {
+    method: "GET",
+    source: "express",
+    route: "/nested-app",
+  });
+});
+
+// Express instrumentation results in routes with no stack, crashing Ghost
+// https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2271
+// https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2294
+t.test(
+  "it keeps handle properties even if router is patched before instrumentation does it",
+  async () => {
+    const { createServer } = require("http") as typeof import("http");
+    const expressApp = express();
+    const router = express.Router();
+
+    let routerLayer: { name: string; handle: { stack: any[] } } | undefined =
+      undefined;
+
+    const CustomRouter: (...p: Parameters<typeof router>) => void = (
+      req,
+      res,
+      next
+    ) => router(req, res, next);
+
+    router.use("/:slug", (req, res, next) => {
+      // On express v4, the router is available as `app._router`
+      // On express v5, the router is available as `app.router`
+      // @ts-expect-error stack is private
+      const stack = req.app.router.stack as any[];
+      routerLayer = stack.find((router) => router.name === "CustomRouter");
+      return res.status(200).send("bar");
+    });
+
+    // The patched router now has express router's own properties in its prototype so
+    // they are not accessible through `Object.keys(...)`
+    // https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192
+    Object.setPrototypeOf(CustomRouter, router);
+    expressApp.use(CustomRouter);
+
+    // supertest acts weird with the custom router, so we need to create a server manually
+    const server = createServer(expressApp);
+    await new Promise<void>((resolve) => {
+      server.listen(0, resolve);
+    });
+
+    if (!server) {
+      throw new Error("server not found");
+    }
+
+    const address = server.address();
+
+    if (typeof address === "string") {
+      throw new Error("address is a string");
+    }
+
+    const response = await fetch({
+      url: new URL(`http://localhost:${address!.port}/foo`),
+    });
+    t.same(response.body, "bar");
+    server.close();
+
+    if (!routerLayer) {
+      throw new Error("router layer not found");
+    }
+
+    t.ok(
+      // @ts-expect-error handle is private
+      routerLayer.handle.stack.length === 1,
+      "router layer stack is accessible"
+    );
+  }
+);

library/sources/express/wrapRequestHandler.ts

@@ -1,37 +1,62 @@
+/* eslint-disable prefer-rest-params */
 import type { RequestHandler } from "express";
 import { runWithContext } from "../../agent/Context";
+import { createWrappedFunction } from "../../helpers/wrap";
 import { contextFromRequest } from "./contextFromRequest";
 
 export function wrapRequestHandler(handler: RequestHandler): RequestHandler {
-  const fn: RequestHandler = (req, res, next) => {
-    const context = contextFromRequest(req);
+  const fn = createWrappedFunction(handler, function wrap(handler) {
+    return function wrap(this: RequestHandler) {
+      if (arguments.length === 0) {
+        return handler.apply(this);
+      }
 
-    return runWithContext(context, () => {
-      return handler(req, res, next);
-    });
-  };
+      const context = contextFromRequest(arguments[0]);
+
+      return runWithContext(context, () => {
+        return handler.apply(this, arguments);
+      });
+    };
+  }) as RequestHandler;
+
+  // Some libraries/apps have properties on the handler functions that are not copied by our createWrappedFunction function
+  // (createWrappedFunction only copies properties when hasOwnProperty is true)
+  // Let's set up a proxy to forward the property access to the original handler
+  // e.g. https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192
+  for (const key in handler) {
+    if (handler.hasOwnProperty(key)) {
+      continue;
+    }
 
-  if (handler.name) {
-    preserveFunctionName(fn, handler.name);
+    Object.defineProperty(fn, key, {
+      get() {
+        // @ts-expect-error Types unknown
+        return handler[key];
+      },
+      set(value) {
+        // @ts-expect-error Types unknown
+        handler[key] = value;
+      },
+    });
   }
 
+  // For some libraries/apps it's important to preserve the function name
+  // e.g. Ghost looks up a middleware function by name in the router stack
+  preserveLayerName(fn, handler.name);
+
   return fn;
 }
 
 /**
- * Preserve the original function name
- * e.g. Ghost looks up a middleware function by name in the router stack
- *
  * Object.getOwnPropertyDescriptor(function myFunction() {}, "name")
- *
  * {
  *   value: 'myFunction',
  *   writable: false,
  *   enumerable: false,
  *   configurable: true
  * }
  */
-function preserveFunctionName(wrappedFunction: Function, originalName: string) {
+function preserveLayerName(wrappedFunction: Function, originalName: string) {
   try {
     Object.defineProperty(wrappedFunction, "name", {
       value: originalName,