-
Notifications
You must be signed in to change notification settings - Fork 22
Allow RegExp pattern for targeting bundled files (e.g. chunk-XXXX.mjs) #821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f16eb61
43e107b
afa1ca5
a6e608c
0b389dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| import { getRandomPort } from "./utils/get-port.mjs"; | ||
| import { spawnSync, spawn } from "node:child_process"; | ||
| import { resolve } from "node:path"; | ||
| import { timeout } from "./utils/timeout.mjs"; | ||
| import { test, before } from "node:test"; | ||
| import { equal, fail, match, doesNotMatch } from "node:assert"; | ||
|
|
||
| const pathToAppDir = resolve( | ||
| import.meta.dirname, | ||
| "../../sample-apps/react-router-pg" | ||
| ); | ||
|
|
||
| const port = await getRandomPort(); | ||
| const port2 = await getRandomPort(); | ||
|
|
||
| before(() => { | ||
| const { stderr, status } = spawnSync("npm", ["run", "build"], { | ||
| cwd: pathToAppDir, | ||
| }); | ||
|
|
||
| if (status !== 0) { | ||
| throw new Error(`Failed to build: ${stderr.toString()}`); | ||
| } | ||
| }); | ||
|
|
||
| test("it blocks request in blocking mode", async () => { | ||
| const server = spawn( | ||
| `node_modules/.bin/react-router-serve`, | ||
| ["./build/server/index.js"], | ||
| { | ||
| cwd: pathToAppDir, | ||
| env: { | ||
| ...process.env, | ||
| AIKIDO_DEBUG: "true", | ||
| AIKIDO_BLOCK: "true", | ||
| NODE_OPTIONS: "-r @aikidosec/firewall/instrument", | ||
| PORT: port, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| try { | ||
| server.on("error", (err) => { | ||
| 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 | ||
| await timeout(2000); | ||
|
|
||
| const formData1 = new FormData(); | ||
| formData1.append("catname", "Kitty'); DELETE FROM cats_5;-- H"); | ||
|
|
||
| const formData2 = new FormData(); | ||
| formData2.append("catname", "Miau"); | ||
|
|
||
| const [sqlInjection, normalAdd] = await Promise.all([ | ||
| fetch(`http://127.0.0.1:${port}/add-cat`, { | ||
| method: "POST", | ||
| body: formData1, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(5000), | ||
| }), | ||
| fetch(`http://127.0.0.1:${port}/add-cat`, { | ||
| method: "POST", | ||
| body: formData2, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(5000), | ||
| }), | ||
| ]); | ||
|
|
||
| equal(sqlInjection.status, 500); | ||
| equal(normalAdd.status, 302); // Redirect after successful add | ||
| match(stdout, /Starting agent/); | ||
| match(stderr, /Zen has blocked an SQL injection/); | ||
| } finally { | ||
| server.kill(); | ||
| } | ||
| }); | ||
|
|
||
| test("it does not block request in monitoring mode", async () => { | ||
| const server = spawn( | ||
| `node_modules/.bin/react-router-serve`, | ||
| ["./build/server/index.js"], | ||
| { | ||
| cwd: pathToAppDir, | ||
| env: { | ||
| ...process.env, | ||
| AIKIDO_DEBUG: "true", | ||
| AIKIDO_BLOCK: "false", | ||
| NODE_OPTIONS: "-r @aikidosec/firewall/instrument", | ||
| PORT: port2, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| try { | ||
| server.on("error", (err) => { | ||
| 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 | ||
| await timeout(2000); | ||
|
|
||
| const formData1 = new FormData(); | ||
| formData1.append("catname", "Kitty'); DELETE FROM cats_5;-- H"); | ||
|
|
||
| const formData2 = new FormData(); | ||
| formData2.append("catname", "Miau"); | ||
|
|
||
| const [sqlInjection, normalAdd] = await Promise.all([ | ||
| fetch(`http://127.0.0.1:${port2}/add-cat`, { | ||
| method: "POST", | ||
| body: formData1, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(5000), | ||
| }), | ||
| fetch(`http://127.0.0.1:${port2}/add-cat`, { | ||
| method: "POST", | ||
| body: formData2, | ||
| redirect: "manual", | ||
| signal: AbortSignal.timeout(5000), | ||
| }), | ||
| ]); | ||
|
|
||
| equal(sqlInjection.status, 302); // Redirect even with SQL injection | ||
| equal(normalAdd.status, 302); | ||
| match(stdout, /Starting agent/); | ||
| doesNotMatch(stderr, /Zen has blocked an SQL injection/); | ||
| } finally { | ||
| server.kill(); | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ export function setPackagesToInstrument(_packages: Package[]) { | |
| return versionedPackage | ||
| .getFileInstrumentationInstructions() | ||
| .map((file) => { | ||
| const fileIdentifier = `${pkg.getName()}.${file.path}.${versionedPackage.getRange()}`; | ||
| const fileIdentifier = `${pkg.getName()}.${getIdentifier(file.path)}.${versionedPackage.getRange()}`; | ||
| if (file.accessLocalVariables?.cb) { | ||
| fileCallbackInfo.set(fileIdentifier, { | ||
| pkgName: pkg.getName(), | ||
|
|
@@ -57,7 +57,7 @@ export function setPackagesToInstrument(_packages: Package[]) { | |
| versionRange: versionedPackage.getRange(), | ||
| identifier: fileIdentifier, | ||
| functions: file.functions.map((func) => { | ||
| const identifier = `${pkg.getName()}.${file.path}.${func.name}.${func.nodeType}.${versionedPackage.getRange()}`; | ||
| const identifier = `${pkg.getName()}.${getIdentifier(file.path)}.${func.name}.${func.nodeType}.${versionedPackage.getRange()}`; | ||
|
|
||
| // If bindContext is set to true, but no modifyArgs is defined, modifyArgs will be set to a stub function | ||
| // The reason for this is that the bindContext logic needs to modify the arguments | ||
|
|
@@ -117,6 +117,25 @@ export function shouldPatchPackage(name: string): boolean { | |
| return packages.has(name); | ||
| } | ||
|
|
||
| function matchesPath( | ||
| pathOrPattern: string | RegExp, | ||
| filePath: string | ||
| ): boolean { | ||
| if (typeof pathOrPattern === "string") { | ||
| return pathOrPattern === filePath; | ||
| } | ||
|
|
||
| return pathOrPattern.test(filePath); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling RegExp.test on potentially untrusted/complex patterns can introduce ReDoS risk (no validation or safeguards on provided RegExp). Details✨ AI Reasoning 🔧 How do I fix it? More info - Comment |
||
| } | ||
|
|
||
| function getIdentifier(pathOrPattern: string | RegExp) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does JS not call .toString() automatically? const regex: RegExp = /abcd/i;
console.log(`test${regex}A42`);
// Prints 'test/abcd/iA42' |
||
| if (typeof pathOrPattern === "string") { | ||
| return pathOrPattern; | ||
| } | ||
|
|
||
| return pathOrPattern.toString(); | ||
| } | ||
|
|
||
| export function getPackageFileInstrumentationInstructions( | ||
| packageName: string, | ||
| version: string, | ||
|
|
@@ -128,7 +147,8 @@ export function getPackageFileInstrumentationInstructions( | |
| } | ||
|
|
||
| return instructions.find( | ||
| (f) => f.path === filePath && satisfiesVersion(f.versionRange, version) | ||
| (f) => | ||
| matchesPath(f.path, filePath) && satisfiesVersion(f.versionRange, version) | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -141,7 +161,7 @@ export function shouldPatchFile( | |
| return false; | ||
| } | ||
|
|
||
| return instructions.some((f) => f.path === filePath); | ||
| return instructions.some((f) => matchesPath(f.path, filePath)); | ||
| } | ||
|
|
||
| export function getFunctionCallbackInfo( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import * as t from "tap"; | ||
| import { formDataToPlainObject } from "./formDataToPlainObject"; | ||
|
|
||
| t.test( | ||
| "simple", | ||
| { | ||
| skip: !globalThis.FormData | ||
| ? "This Node.js version does not support FormData yet" | ||
| : false, | ||
| }, | ||
| async (t) => { | ||
| const formData = new FormData(); | ||
| formData.append("abc", "123"); | ||
| formData.append("another", "42"); | ||
| formData.append("hello", "world"); | ||
|
|
||
| t.same(formDataToPlainObject(formData), { | ||
| abc: "123", | ||
| another: "42", | ||
| hello: "world", | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| t.test( | ||
| "with arrays", | ||
| { | ||
| skip: !globalThis.FormData | ||
| ? "This Node.js version does not support FormData yet" | ||
| : false, | ||
| }, | ||
| async (t) => { | ||
| const formData = new FormData(); | ||
| formData.append("abc", "123"); | ||
| formData.append("arr", "1"); | ||
| formData.append("arr", "2"); | ||
| formData.append("arr", "3"); | ||
|
|
||
| t.same(formDataToPlainObject(formData), { | ||
| abc: "123", | ||
| arr: ["1", "2", "3"], | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| t.test( | ||
| "binary data", | ||
| { | ||
| skip: | ||
| !globalThis.FormData || !globalThis.File | ||
| ? "This Node.js version does not support FormData or File yet" | ||
| : false, | ||
| }, | ||
| async (t) => { | ||
| const formData = new FormData(); | ||
| formData.append("abc", "123"); | ||
| formData.append("arr", "2"); | ||
| formData.append("arr", "3"); | ||
| formData.append( | ||
| "file", | ||
| new File(["hello"], "hello.txt", { type: "text/plain" }) | ||
| ); | ||
|
|
||
| t.same(formDataToPlainObject(formData), { | ||
| abc: "123", | ||
| arr: ["2", "3"], | ||
| }); | ||
| } | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| export function formDataToPlainObject(formData: FormData) { | ||
| const object: Map<string, unknown> = new Map(); | ||
| formData.forEach((value, key) => { | ||
| if (typeof value !== "string") { | ||
| return; | ||
| } | ||
|
|
||
| if (object.has(key)) { | ||
| // If the key already exists, treat it as an array | ||
| const entry = object.get(key); | ||
|
|
||
| if (Array.isArray(entry)) { | ||
| // If it's already an array, just push the new value | ||
| entry.push(value); | ||
| return; | ||
| } | ||
|
|
||
| // Convert it to an array | ||
| object.set(key, [object.get(key), value]); | ||
| return; | ||
| } | ||
|
|
||
| object.set(key, value); | ||
| }); | ||
|
|
||
| return Object.fromEntries(object); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment 'Just accept RegExp paths as-is' explains what the code does; replace it with why RegExp paths are allowed (e.g., to support bundled chunk filenames like 'chunk-XXXX.mjs').
Details
✨ AI Reasoning
1) The added comment on line 69 simply restates what the immediately following code does (accept RegExp paths) rather than explaining why this special-case exists or its intended effect (e.g., supporting bundled chunk filenames).
2) This harms maintainability because such "what" comments add little value and can become stale; a "why" comment would guide future maintainers.
3) The issue is limited in scope to a small, recent change and is appropriate to fix within this PR by replacing the comment.
4) Fixing it improves long-term clarity without requiring refactor.
5) The change is a single new comment, so it's straightforward to improve.
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment
@AikidoSec feedback: [FEEDBACK]to get better review comments in the future.