-
Notifications
You must be signed in to change notification settings - Fork 30
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 1 commit
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 |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -141,7 +160,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,31 @@ | ||
| import { Hooks } from "../agent/hooks/Hooks"; | ||
| import { Wrapper } from "../agent/Wrapper"; | ||
| import { wrapRequestBodyParsing } from "./react-router/wrapRequestBodyParsing"; | ||
|
|
||
| export class ReactRouter implements Wrapper { | ||
| wrap(hooks: Hooks) { | ||
| hooks | ||
| .addPackage("react-router") | ||
| .withVersion("^7.0.0") | ||
| .addFileInstrumentation({ | ||
| path: /^dist\/(production|development)\/chunk-[A-Z0-9]+\.mjs$/, | ||
| functions: [ | ||
| { | ||
| // We cannot patch the `Request` global (as Request is also used by fetch calls) | ||
| // We're interested in the Request object that gets passed to the server actions | ||
| // See https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/server-runtime/data.ts#L26 | ||
| nodeType: "FunctionDeclaration", | ||
| name: "stripRoutesParam", | ||
| operationKind: undefined, | ||
| modifyReturnValue: (_, returnValue) => { | ||
| if (returnValue instanceof Request) { | ||
hansott marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| wrapRequestBodyParsing(returnValue); | ||
| } | ||
|
|
||
| return returnValue; | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { getContext, updateContext } from "../../agent/Context"; | ||
| import { createWrappedFunction, isWrapped } from "../../helpers/wrap"; | ||
|
|
||
| /** | ||
| * Wrap the Request object's body parsing methods to update the context with the parsed body | ||
| * when any of the methods are called. | ||
| * This is needed because React Router uses the Fetch API Request object which has a stream | ||
| * body that can only be consumed once. We wrap the parsing methods to capture the result. | ||
| */ | ||
| export function wrapRequestBodyParsing(request: Request) { | ||
| request.formData = wrapBodyParsingFunction(request.formData); | ||
|
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. Wraps request.formData without verifying it's a function, which can cause runtime errors if the property is missing or not callable. Details✨ AI Reasoning 🔧 How do I fix it? More info - Comment |
||
| request.json = wrapBodyParsingFunction(request.json); | ||
| request.text = wrapBodyParsingFunction(request.text); | ||
| } | ||
|
|
||
| function wrapBodyParsingFunction<T extends Function>(func: T) { | ||
| if (isWrapped(func)) { | ||
| return func; | ||
| } | ||
|
|
||
| return createWrappedFunction(func, function parse(parser) { | ||
| return async function wrap() { | ||
| // @ts-expect-error No type for arguments | ||
| // eslint-disable-next-line prefer-rest-params | ||
| const returnValue = await parser.apply(this, arguments); | ||
|
|
||
| if (returnValue) { | ||
| const context = getContext(); | ||
| if (context) { | ||
| updateContext(context, "body", returnValue); | ||
| } | ||
| } | ||
|
|
||
| return returnValue; | ||
| }; | ||
| }) as T; | ||
| } | ||
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.