Skip to content

Commit

Permalink
Merge pull request #467 from AikidoSec/fix-express
Browse files Browse the repository at this point in the history
fix: Express handler wrapping
  • Loading branch information
hansott authored Dec 2, 2024
2 parents 31db21a + 8c43d18 commit 65132df
Show file tree
Hide file tree
Showing 7 changed files with 3,987 additions and 4 deletions.
55 changes: 55 additions & 0 deletions end2end/tests/node-red.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
const t = require("tap");
const { spawn } = require("child_process");
const { resolve } = require("path");
const timeout = require("../timeout");

const pathToApp = resolve(__dirname, "../../sample-apps/node-red");

t.test("it serves debug script", (t) => {
const server = spawn(`node_modules/.bin/node-red`, {
env: {
...process.env,
AIKIDO_DEBUG: "true",
AIKIDO_BLOCK: "true",
NODE_OPTIONS: "-r @aikidosec/firewall",
},
cwd: pathToApp,
});

server.on("close", () => {
t.end();
});

server.on("error", (err) => {
t.fail(err);
});

let stdout = "";
server.stdout.on("data", (data) => {
stdout += data.toString();
});

let stderr = "";
server.stderr.on("data", (data) => {
stderr += data.toString();
});

// Wait for the server to start
timeout(5000)
.then(() => {
return Promise.all([
fetch(`http://127.0.0.1:1880/debug/view/debug-utils.js`, {

Check failure

Code scanning / CodeQL

Download of sensitive file through insecure connection High test

Download
of sensitive file from
HTTP source
.
signal: AbortSignal.timeout(5000),
}),
]);
})
.then(([script]) => {
t.equal(script.status, 200);
})
.catch((error) => {
t.fail(error);
})
.finally(() => {
server.kill();
});
});
16 changes: 13 additions & 3 deletions library/helpers/wrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,19 @@ export function createWrappedFunction(
// .inspect("realpath", (args) => {...})
// We don't want to lose the original function's properties.
// Most of the functions we're wrapping don't have any properties, so this is a rare case.
for (const prop in original) {
if (original.hasOwnProperty(prop)) {
defineProperty(wrapped, prop, original[prop as keyof Function]);
// Inspired by https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-shimmer/src/shimmer.js#L8

Object.setPrototypeOf(wrapped, original);

const props = Object.getOwnPropertyDescriptors(original);
const keys = Reflect.ownKeys(props);

for (const key of keys) {
try {
// Define the property on the wrapped function, keeping the original property's attributes.
Object.defineProperty(wrapped, key as any, props[key as any]);
} catch (e) {
//

Check warning on line 54 in library/helpers/wrap.ts

View check run for this annotation

Codecov / codecov/patch

library/helpers/wrap.ts#L54

Added line #L54 was not covered by tests
}
}

Expand Down
29 changes: 29 additions & 0 deletions library/sources/Express.tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,26 @@ export function createExpressTests(expressPackageName: string) {

app.use(newRouter);

app.use("/", express.static(__dirname + "/fixtures/public/"));

const nestedApp = express();
nestedApp.set("trust proxy", true);
nestedApp.get("/", (req, res) => {
res.send(getContext());
});

app.use("/nested-app", nestedApp);

const nestedNestedApp = express();
nestedNestedApp.get("/2", (req, res) => {
res.send(getContext());
});
nestedApp.use(nestedNestedApp);

nestedApp.get("/foo", (req, res) => {
res.send("bar");
});

app.get("/", (req, res) => {
const context = getContext();

Expand Down Expand Up @@ -608,6 +621,12 @@ export function createExpressTests(expressPackageName: string) {
});
});

t.test("it supports static files", async (t) => {
const response = await request(getApp()).get("/test.txt");

t.same(response.text, "Testfile");
});

t.test("it supports nested app", async (t) => {
const response = await request(getApp()).get("/nested-app");

Expand All @@ -616,6 +635,16 @@ export function createExpressTests(expressPackageName: string) {
source: "express",
route: "/nested-app",
});

const response2 = await request(getApp()).get("/nested-app/foo");
t.same(response2.text, "bar");

const response3 = await request(getApp()).get("/nested-app/2");
t.match(response3.body, {
method: "GET",
source: "express",
route: "/nested-app/:number",
});
});

// Express instrumentation results in routes with no stack, crashing Ghost
Expand Down
2 changes: 1 addition & 1 deletion library/sources/express/wrapRequestHandler.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/* eslint-disable prefer-rest-params */
import type { RequestHandler } from "express";
import { runWithContext } from "../../agent/Context";
import { createWrappedFunction } from "../../helpers/wrap";
import { contextFromRequest } from "./contextFromRequest";
import { createWrappedFunction } from "../../helpers/wrap";

export function wrapRequestHandler(handler: RequestHandler): RequestHandler {
const fn = createWrappedFunction(handler, function wrap(handler) {
Expand Down
1 change: 1 addition & 0 deletions library/sources/fixtures/public/test.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Testfile
Loading

0 comments on commit 65132df

Please sign in to comment.