feat(bug-detectors): replace RCE substring check with global canary#873
feat(bug-detectors): replace RCE substring check with global canary#873
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Remote Code Execution bug detector to avoid false positives from substring checks by introducing a global canary on globalThis and ensuring it’s also visible inside Jest’s vm.Context sandbox.
Changes:
- Replace source-text substring detection for
eval/Functionwith a global canary-based detection mechanism. - Update before-hooks to provide only fuzzing guidance (
guideTowardsContainment) instead of reporting findings directly. - Expand and rename fuzz targets/tests to cover canary-triggering cases and “safe” (non-triggering) cases, including string-literal and string-coercible bodies.
Reviewed changes
Copilot reviewed 61 out of 63 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/bug-detectors/remote-code-execution/tests.fuzz.js | Renames/reorganizes fuzz test cases to distinguish canary-triggering vs safe scenarios. |
| tests/bug-detectors/remote-code-execution/fuzz.js | Updates fuzz entrypoints to execute code that should (or should not) trigger the canary for eval and Function. |
| tests/bug-detectors/remote-code-execution.test.js | Aligns CLI + Jest integration tests with the new entrypoints and expected outcomes. |
| packages/bug-detectors/internal/remote-code-execution.ts | Implements the global canary, propagates it to Jest vmContext, and changes hooks to guidance-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The current design is better than looking for "jaz_zer" substring in the source text for const propertyName = "jaz_zer";
void globalThis[propertyName];
console.log("can be called just fine");I think that the current “if anything reads globalThis.jaz_zer, report RCE.” check is too borad. How about we create a cannary call, e.g., |
|
I think we should make it configurable and allow both (as in either or) defaulting to the current implementation that marks access as RCE. And the reasoning is that it is:
var fn = eval(my_awesome_sanitizer("..."));
...
// much later, after lots of fuzzing
fn(); // actual callIn case it's a false-positive, the users can be informed that it's possible to switch to a more strict RCE detection that requires an actual canary execution. |
f0dfebc to
1918f67
Compare
Keep Jest transpilation independent from project references so the build remains the single source of type-checking truth.
Reenable previously skipped test that calls multiple done-callbacks
Mock parent-directory scans without recursing through the spy so the missing-package test exercises the corpus lookup failure.
Reset regular-expression state before each detector-name check so global patterns disable detectors consistently.
Run afterEach callbacks before clearing callback findings so deferred detector reports are delivered through done(error).
Match suppressions against the stack text users see so detectors can share the same low-surprise ignore rules.
Use an owned canary to distinguish heuristic reads from confirmed execution and avoid substring-only reports that confuse string literals with code execution.
Let users silence expected traversal callsites with the same shown-stack rules used by code-injection findings.
Use the same filesystem locations as the spawned fuzz targets so repeated direct runs do not fail on stale SAFE directories.
The old detector checked whether the target string appeared in eval/Function source text. This false-positived on safely quoted occurrences (e.g. Handlebars' lookupProperty(d, "jaz_zer")).
Install a canary function on globalThis that fires only when attacker-controlled code actually executes. The before-hooks now provide fuzzing guidance only (guideTowardsContainment). The canary is propagated to the Jest vm.Context sandbox, following the same pattern as the prototype-pollution detector.