Allow RegExp pattern for targeting bundled files (e.g. chunk-XXXX.mjs) #821
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| return pathOrPattern === filePath; | ||
| } | ||
|
|
||
| return pathOrPattern.test(filePath); |
There was a problem hiding this comment.
Calling RegExp.test on potentially untrusted/complex patterns can introduce ReDoS risk (no validation or safeguards on provided RegExp).
Details
✨ AI Reasoning
1) The changes add support for RegExp matching via matchesPath which calls pathOrPattern.test(filePath).
2) This introduces the use of arbitrary regular expressions where previously only string equality was used, which can expose the code to ReDoS if untrusted or complex regexes are used.
3) This harms safety because a crafted pattern (or an overly complex pattern from configuration) can cause catastrophic backtracking when .test() is executed on long inputs.
4) Guarding or validating patterns or restricting allowed regex constructs would mitigate the risk; the change introduced the raw .test() invocation without such safeguards. Therefore this is a true issue introduced by the PR at the point where .test() is invoked.
🔧 How do I fix it?
Avoid nested quantifiers like (x+)+ and ambiguous patterns. Use atomic groups, possessive quantifiers, or rewrite complex regex patterns as simpler alternatives.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| */ | ||
| addFileInstrumentation(instruction: PackageFileInstrumentationInstruction) { | ||
| if (instruction.path instanceof RegExp) { | ||
| // Just accept RegExp paths as-is |
There was a problem hiding this comment.
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.
| * 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.
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
1) The new code assigns wrapped versions of request.formData/json/text without checking that those properties are actually functions; 2) If any of those properties is undefined or not callable at runtime, isWrapped(func) or createWrappedFunction(func, ...) may throw or behave incorrectly, breaking behavior; 3) This harms robustness because the wrapper assumes functions exist on the Request object but doesn't guard against missing/non-function properties; therefore it's a logic bug introduced by these changes (previous code did not perform these assignments);
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| return pathOrPattern.test(filePath); | ||
| } | ||
|
|
||
| function getIdentifier(pathOrPattern: string | RegExp) { |
There was a problem hiding this comment.
Does JS not call .toString() automatically?
const regex: RegExp = /abcd/i;
console.log(`test${regex}A42`);
// Prints 'test/abcd/iA42'new RegExp("abc")
/abc/
JSON.stringify(new RegExp("abc"))
'{}'
Seems like this works fine.
* 'main' of github.com:AikidoSec/node-RASP: Cleanup Dockerfile and .dockerignore Remove duplicate test case in imds.test.ts Fix detection of IMDS IPv4 addresses mapped to IPv6 Cleanup devDependencies for linked library dep Add React Router SSR ESM sample app
No description provided.