feat: enhance cpu and memory exhausting protection #20
Conversation
…oved changelog updates
📝 WalkthroughWalkthroughA ResourceExhaustionRule was added to ast-guard and enabled in the AgentScript preset. Enclave VM sandboxing and memory/CPU protections were hardened (SafeObject, prototype freezes, codeGeneration disabled, memory-join enforcement). AST validation now runs before memory/sidecar transforms. Tests and docs were updated for dual AST+runtime defenses. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)libs/**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (8)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @libs/ast-guard/src/rules/index.ts:
- Line 23: The public export ResourceExhaustionRule (export * from
'./resource-exhaustion.rule') was added but missing documentation; update the
docs by adding an entry for ResourceExhaustionRule in
docs/draft/docs/libraries/ast-guard.mdx that describes the rule, its purpose,
usage, and any config options or examples, ensuring the new documentation
follows the existing format and headings used for other rule entries in that MDX
file.
In @libs/enclave-vm/src/memory-proxy.ts:
- Around line 159-160: Remove the unused variable declaration const
originalToString = Array.prototype.toString from memory-proxy.ts (it’s declared
at the same spot as originalJoin) and ensure no other code references
originalToString; keep originalJoin as-is since toString delegates to
this.join(','). After removal, run lint/type checks to confirm no
unused-variable remains.
🧹 Nitpick comments (10)
libs/enclave-vm/src/__tests__/literal-constructor-escape.spec.ts (2)
25-38: Consider usingtry/finallyorafterEachfor reliable cleanup.If any assertion fails,
enclave.dispose()won't be called, potentially leaking resources across test runs. This pattern is repeated throughout the file.Option 1: Use try/finally per test
it('should block Function constructor from array literal', async () => { const enclave = new Enclave(); + try { const code = ` const c = 'con' + 'struc' + 'tor'; const arr = []; const Fn = arr[c][c]; return Fn('return 42')(); `; const result = await enclave.run(code); expect(result.success).toBe(false); // Should fail at AST level (constructor obfuscation detected) or VM level (codeGeneration.strings disabled) expect(result.error?.message).toMatch(/code generation from strings|EvalError|AgentScript validation failed/i); + } finally { enclave.dispose(); + } });Option 2: Use describe-scoped variable with afterEach
describe('Array Literal Constructor Chain Attack', () => { let enclave: Enclave; afterEach(() => { enclave?.dispose(); }); it('should block Function constructor from array literal', async () => { enclave = new Enclave(); // ... test code ... }); });
208-244: Consider asserting on failure cases for completeness.The conditional assertions only verify behavior when
result.successis true. If the code fails for an unexpected reason (e.g., syntax error), the test would silently pass.🔎 Suggested improvement
const result = await enclave.run(code); // Either blocked by SecureProxy or frozen prototype if (result.success) { expect((result.value as { polluted: boolean }).polluted).toBe(false); + } else { + // Verify failure is security-related, not an unexpected error + expect(result.error?.message).toMatch(/Security violation|AgentScript validation failed|frozen|not extensible/i); } enclave.dispose();libs/enclave-vm/src/memory-proxy.ts (1)
165-197: Consider extracting shared join estimation logic to reduce duplication.The join size estimation and limit-checking logic (lines 165-196) is nearly identical to the implementation in
wrapArrayInstance(lines 53-84). This duplication could lead to maintenance drift if one is updated without the other.Consider extracting a shared helper function:
🔎 Proposed refactor
// Helper at module level function estimateJoinSize(arr: unknown[], separator: string): number { const arrLength = arr.length; if (arrLength > 10000) { return arrLength * (10 + separator.length) * 2; } let estimatedSize = 0; for (let i = 0; i < arrLength; i++) { const item = arr[i]; estimatedSize += (item == null ? 0 : String(item).length) + separator.length; } return estimatedSize * 2; } function checkJoinLimit(tracker: MemoryTracker, estimatedSize: number): void { const limit = tracker.getLimit(); if (limit > 0 && estimatedSize > limit) { throw new MemoryLimitError( `Array.join would exceed memory limit (estimated ${estimatedSize} bytes, limit ${limit} bytes)`, estimatedSize, limit, ); } }Then both
wrapArrayInstanceandcreateTrackedArrayMethodscan use these helpers.libs/enclave-vm/src/__tests__/function-gadget-attacks.spec.ts (1)
55-66: Consider addingSecurity violationto the error pattern for consistency.The error message regex only matches
AgentScript validation failed|code generation from strings, but other test files also acceptSecurity violationfor runtime proxy blocks.🔎 Proposed fix
function expectSecureResult( result: { success: boolean; value?: unknown; error?: { message?: string } }, expectedValue: unknown, ): void { if (!result.success) { // AST-level validation blocked the attack - this is valid and stronger security - expect(result.error?.message).toMatch(/AgentScript validation failed|code generation from strings/i); + expect(result.error?.message).toMatch(/Security violation|AgentScript validation failed|code generation from strings/i); return; } // Runtime blocking or sandbox execution expectSecureOutcome(result.value, expectedValue); }libs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.ts (1)
56-67: Helper signature differs from function-gadget-attacks.spec.ts - consider unifying.This
expectSecureResultacceptsexpectedValues: unknown[](array), while the one infunction-gadget-attacks.spec.tsacceptsexpectedValue: unknown(single value). Both are valid designs, but having consistent helper signatures across test files would improve maintainability.Consider extracting these helpers to a shared test utility file to ensure consistency across the test suite.
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (2)
142-159: Test description is misleading - it tests iteration limits, not timeout.The test name says "should enforce timeout or iteration limits" but the assertion specifically checks for iteration limit errors. Consider renaming for clarity or adding an actual timeout test.
🔎 Proposed fix
- it('should enforce timeout or iteration limits', async () => { - // This test verifies that long-running code is interrupted - // by either timeout OR iteration limits - both are valid protections + it('should enforce iteration limits on computation-heavy loops', async () => { + // This test verifies that long-running code is interrupted by iteration limits const enclave = new Enclave({ timeout: 100, maxIterations: 100 });
579-592: Weak assertion - test only verifies no crash, not security behavior.The assertion
expect(result).toBeDefined()only confirms the system didn't crash. Consider strengthening to verify the attack was actually mitigated.🔎 Proposed fix
const result = await enclave.run(code); // Main assertion: we get a result (didn't crash) - // The result may be success or failure depending on implementation + // Either blocked or succeeded within memory bounds expect(result).toBeDefined(); + if (result.success && typeof result.value === 'string') { + // If it succeeded, verify the string was created (test passed) + expect((result.value as string).length).toBeLessThanOrEqual(50 * 1024 * 1024); + } enclave.dispose();libs/ast-guard/src/rules/resource-exhaustion.rule.ts (3)
67-77: BigInt comparison with number may lose precision for very large values.The comparison
exponent > maxBigIntExponentcompares a BigInt with a number. While this works for values within safe integer range, consider making the comparison type-consistent.🔎 Proposed fix
// Check for large exponent if (node.right.type === 'Literal') { const exponent = node.right.bigint ? BigInt(node.right.bigint) : BigInt(node.right.value || 0); - if (exponent > maxBigIntExponent) { + if (exponent > BigInt(maxBigIntExponent)) { context.report({ code: 'RESOURCE_EXHAUSTION', message: `BigInt exponent ${exponent} exceeds maximum allowed (${maxBigIntExponent}). Large exponents can cause CPU exhaustion.`,
186-199: Missing__proto__check in VariableDeclarator handler.The
isSuspiciousStringConcatmethod checks for__proto__, but theVariableDeclaratorhandler only reportsconstructorandprototype. Consider adding__proto__for consistency.🔎 Proposed fix
VariableDeclarator: (node: any) => { if (!blockConstructorAccess) return; if (node.init && node.init.type === 'BinaryExpression') { const result = this.evaluateStringConcat(node.init); - if (result === 'constructor' || result === 'prototype') { + if (result === 'constructor' || result === 'prototype' || result === '__proto__') { context.report({ code: 'CONSTRUCTOR_ACCESS', message: `Variable assigned to "${result}" via string concatenation. This is a potential sandbox escape vector.`, location: this.getLocation(node), }); } } },
215-229: String concatenation evaluation is limited to simple literals - document the limitation.The
evaluateStringConcatmethod only evaluates direct string literal concatenations. Attackers could bypass this using template literals,String.fromCharCode, or variable references. This is acceptable given runtime protections exist, but consider documenting this limitation.🔎 Proposed fix
/** * Try to evaluate a string concatenation expression * Returns the result if it's a simple string concat, or null if too complex + * + * NOTE: This only handles direct string literal concatenation (e.g., 'con' + 'structor'). + * More complex obfuscation (template literals, String.fromCharCode, variable references) + * is handled by runtime protections (SecureProxy, codeGeneration.strings=false). */ private evaluateStringConcat(node: any): string | null {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
libs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/rules/index.tslibs/ast-guard/src/rules/resource-exhaustion.rule.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.tslibs/enclave-vm/src/__tests__/double-vm.security.spec.tslibs/enclave-vm/src/__tests__/enclave.constructor-attack.spec.tslibs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.tslibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/enclave-vm/src/__tests__/enclave.serialization.spec.tslibs/enclave-vm/src/__tests__/function-gadget-attacks.spec.tslibs/enclave-vm/src/__tests__/literal-constructor-escape.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/memory-proxy.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/enclave-vm/src/__tests__/enclave.serialization.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.tslibs/ast-guard/src/rules/index.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.tslibs/enclave-vm/src/__tests__/enclave.constructor-attack.spec.tslibs/enclave-vm/src/__tests__/double-vm.security.spec.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.tslibs/enclave-vm/src/memory-proxy.tslibs/enclave-vm/src/__tests__/function-gadget-attacks.spec.tslibs/enclave-vm/src/__tests__/literal-constructor-escape.spec.tslibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/ast-guard/src/rules/resource-exhaustion.rule.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/ast-guard/src/presets/agentscript.preset.ts
🧬 Code graph analysis (5)
libs/enclave-vm/src/memory-proxy.ts (2)
libs/enclave-vm/src/index.ts (1)
MemoryLimitError(109-109)libs/enclave-vm/src/memory-tracker.ts (1)
MemoryLimitError(40-58)
libs/enclave-vm/src/__tests__/literal-constructor-escape.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(157-739)
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(157-739)
libs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(157-739)
libs/ast-guard/src/presets/agentscript.preset.ts (1)
libs/ast-guard/src/rules/resource-exhaustion.rule.ts (1)
ResourceExhaustionRule(32-239)
🔇 Additional comments (44)
libs/enclave-vm/src/__tests__/literal-constructor-escape.spec.ts (6)
1-22: Well-documented attack vector and defense strategy.The header documentation clearly explains the constructor chain attack pattern and the two defense layers. This provides excellent context for understanding the test suite's purpose.
84-111: LGTM - Object literal attack vectors well covered.The tests correctly verify blocking of Function constructor access via object literals and the Object.prototype chain.
128-144: Double VM configuration is appropriate.The test correctly enables double VM mode with defaults and includes a
toolHandlerto exercise the full code path.
146-206: Good regression coverage for legitimate operations.These tests ensure the security hardening doesn't break normal array, object, string, and Promise operations - essential for preventing false positives.
247-272: Well-documented skip with appropriate test configuration.The comment clearly explains why the large Array.join test is skipped (single-VM mode requirement). The memory limit configuration and Jest timeout are reasonable.
274-299: LGTM - CPU exhaustion tests properly configured.The skipped test documents the platform-dependency clearly, and the reasonable BigInt operations test ensures normal use cases work.
libs/enclave-vm/src/memory-proxy.ts (4)
10-10: LGTM!Import correctly adds
MemoryLimitErrorwhich is used in the new join implementations for pre-allocation limit checking.
46-99: Well-designed proxy wrapper for array memory protection.The pre-allocation size check is a solid defense against memory exhaustion attacks like
new Array(12_000_000).join('x').Minor note: The separator length is added for each element (line 66), but
joinonly inserts N-1 separators for N elements. This results in a slight overestimate bysep.length * 2bytes, which is acceptable since overestimation is conservative for security purposes.
101-140: LGTM!Consistent wrapping of array instances across all creation paths (
apply,construct,Array.from,Array.of) ensures comprehensive memory tracking coverage.
200-203: LGTM!Clean delegation to the tracked
joinmethod ensurestoStringalso benefits from memory limit protection.libs/enclave-vm/src/adapters/vm-adapter.ts (2)
387-394: LGTM! Critical sandbox hardening.Adding
codeGeneration: { strings: false, wasm: false }at context creation time is the correct approach to prevent dynamic code generation attacks. This blocksnew Function()andeval()from string arguments, closing a significant sandbox escape vector via constructor chain traversal.
472-472: Good clarifying comment.The comment correctly notes that
codeGenerationmust be set atcreateContext()time, notrunInContext(). This is important documentation for future maintainers.libs/enclave-vm/src/enclave.ts (2)
320-344: LGTM! Correct validation ordering.Moving validation to run before sidecar/memory transforms is the right approach. This ensures malicious patterns like constructor obfuscation are detected in their original form, before they could be transformed into
__safe_concatcalls that might bypass detection.
346-354: LGTM! Transforms correctly deferred.Applying sidecar and memory-tracking transforms only after successful validation is correct. This provides both security benefits (early rejection of malicious code) and performance benefits (no wasted transform work on invalid code).
libs/enclave-vm/src/__tests__/double-vm.security.spec.ts (5)
1185-1198: LGTM! Test expectations updated for dual-layer security.The updated regex
/Security violation|AgentScript validation failed/correctly accommodates the dual-layer defense model where attacks can be blocked either at AST validation time or at runtime by the secure proxy. The comment update on line 1185 accurately reflects this.
1220-1222: Consistent with dual-layer security model.
1243-1245: Consistent with dual-layer security model.
1270-1272: Consistent with dual-layer security model.
1315-1317: Consistent with dual-layer security model.libs/enclave-vm/src/__tests__/enclave.serialization.spec.ts (1)
143-143: LGTM! Consistent test expectation update.The updated regex aligns with the dual-layer security model, where constructor access attempts can be blocked either during AST validation or at runtime by the secure proxy.
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (1)
192-199: LGTM! Consistent sandbox hardening in double VM wrapper.Adding
codeGeneration: { strings: false, wasm: false }to the parent VM context creation is consistent with the hardening applied invm-adapter.ts. This ensures the defense-in-depth model is maintained, with both parent and inner VMs protected against dynamic code generation attacks.libs/enclave-vm/src/adapters/worker-pool/worker-script.ts (1)
137-142: LGTM! Worker pool sandbox hardening complete.Adding
codeGeneration: { strings: false, wasm: false }to the worker pool's VM context completes the consistent hardening across all sandbox adapters (vm-adapter, double-vm-wrapper, and worker-script). This ensures that regardless of which execution path is used, dynamic code generation attacks are blocked.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (2)
715-719: LGTM! Strong sandbox hardening withcodeGenerationrestrictions.Disabling dynamic code generation via
strings: false, wasm: falseis an effective defense-in-depth measure that prevents constructor-chain-based sandbox escapes like[][c][c]('malicious code'). The inline comment clearly explains the security rationale.
844-844: LGTM! Helpful clarification comment.The comment accurately explains that
codeGenerationconstraints are configured at context creation time rather than during script execution. This aids code comprehension.libs/enclave-vm/src/__tests__/enclave.constructor-attack.spec.ts (2)
29-29: LGTM! Test expectations correctly updated for dual-layer defense.The regex pattern
/Security violation|AgentScript validation failed/appropriately accommodates both AST-level validation (viaResourceExhaustionRule) and runtime proxy blocking. This reflects the defense-in-depth approach where attacks may be blocked at either layer.Also applies to: 47-47, 64-64, 81-81, 98-98, 115-115, 135-135, 189-189, 211-211, 237-237
158-166: LGTM! Well-documented dual-outcome test.The conditional logic appropriately handles two valid security outcomes:
- AST validation blocks the suspicious string concatenation pattern
- Proxy invariant respected - JavaScript requires returning non-configurable properties
The inline comments clearly explain this is expected behavior, documenting the known limitation while still validating that the primary attack vectors remain blocked.
libs/ast-guard/src/presets/agentscript.preset.ts (2)
18-18: LGTM! Import correctly added.The
ResourceExhaustionRuleimport is properly placed alongside other validation rules.
434-446: LGTM! ResourceExhaustionRule properly integrated with sensible defaults.The configuration provides effective defense against CPU/memory exhaustion attacks:
maxBigIntExponent: 10000prevents exponential computation DoSmaxArraySize: 1000000andmaxStringRepeat: 100000prevent memory exhaustionblockConstructorAccess: truestrengthens the dual-layer defense against sandbox escapesblockBigIntExponentiation: falseallows legitimate BigInt operations below the thresholdThe inline comment clearly explains the rule's purpose, and the thresholds align with the library documentation provided.
libs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.ts (3)
129-129: LGTM! Consistent error pattern updates for dual-layer defense.The regex pattern correctly accommodates both AST validation and runtime blocking paths, consistent with the broader test suite updates.
Also applies to: 142-142, 314-314, 328-328
151-168: LGTM! Properly documented proxy invariant test with dual outcomes.The conditional logic appropriately handles AST-level blocking (new) vs. runtime proxy invariant behavior (existing limitation). The comments clearly explain why both outcomes are valid security responses.
174-198: LGTM! Tests appropriately isolate proxy-level behavior.The
validate: falseoption allows testing the runtime proxy configuration independently of AST validation. The comments clearly explain this testing strategy. However, note that this introduces a new public API parameter to the Enclave options.Note: Based on the coding guidelines, since this is a publishable SDK library under
libs/, the newvalidateparameter should be documented. Ensure there is a corresponding update indocs/draft/docs/**for this new public API option.Also applies to: 214-236
libs/enclave-vm/src/__tests__/enclave.security.spec.ts (2)
893-893: LGTM! Consistent dual-layer defense error patterns.The regex pattern correctly accommodates both AST validation and runtime proxy blocking, maintaining consistency across the test suite.
Also applies to: 911-911, 929-929, 1323-1323, 1344-1344
1321-1322: LGTM! Well-structured dual-outcome security tests.The conditional expectations appropriately validate security across both defense layers:
- AST validation blocks suspicious patterns early (preferred outcome)
- Runtime proxy blocks if AST validation passes (fallback defense)
The addition of
CONSTRUCTOR_ACCESSto the regex pattern at line 1435 correctly matches the error code fromResourceExhaustionRule. The comments accurately explain the dual-path security model.Also applies to: 1342-1343, 1371-1377, 1403-1409, 1432-1435
libs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts (3)
40-40: LGTM - Error pattern correctly updated for dual-layer security.The regex pattern
/Security violation|AgentScript validation failed/appropriately handles both runtime proxy blocking and AST-level validation outcomes.
513-537: Conditional test logic correctly handles both security outcomes.The test appropriately branches on
result.successto validate either:
- AST validation blocking (error message matches pattern)
- Runtime proxy allowing access in PERMISSIVE mode
The comment on lines 515-517 clearly documents why string concatenation attacks are blocked even in PERMISSIVE mode.
559-580: Test correctly isolates proxy configuration from AST validation.Disabling validation (
validate: false) to testsecureProxyConfigbehavior directly is a valid testing strategy that ensures the proxy layer can be tested independently.libs/enclave-vm/src/__tests__/function-gadget-attacks.spec.ts (2)
42-49: Helper function correctly identifies blocked values as valid security outcomes.The implementation is clear and the hardcoded blocked values array appropriately covers the test patterns used throughout the file.
380-388: Good defensive test structure handling both outcomes.The conditional check correctly validates either sandbox execution or AST blocking, with appropriate assertions for each path.
libs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.ts (2)
452-460: Thorough dual-outcome validation for async function constructor test.The test correctly handles both AST blocking (via error message check) and runtime sandbox isolation (via regex non-match for host access). The comment clearly explains the security expectations.
595-667: Excellent documentation of known security limitation.The test at line 595-651 explicitly documents that passing constructor functions (like
Map) as globals can expose the host's Function constructor. The warning comment and console.warn are valuable for security awareness.libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (2)
1-18: Excellent documentation header.The JSDoc clearly describes the defense layers (AST-level, runtime limits, VM timeout, worker watchdog), providing valuable context for understanding the test suite's purpose.
600-638: Sort attack test has appropriate timeout handling.The test correctly uses Jest's extended timeout (15000ms) and validates that either the attack completes within reasonable time or is blocked for security reasons. The conditional assertion logic is appropriate for this scenario.
libs/ast-guard/src/rules/resource-exhaustion.rule.ts (2)
1-18: Well-designed options interface with sensible defaults.The interface clearly documents each option with JSDoc comments, and the defaults balance security with usability.
32-47: Public API addition for libs/ - ensure docs are updated.As per coding guidelines, changes to public APIs in
libs/**should have matching documentation updates. Verify thatResourceExhaustionRuleandResourceExhaustionOptionsare documented appropriately.Based on coding guidelines, ensure there is a corresponding documentation update in
docs/draft/docs/**for this new public API.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts:
- Around line 785-799: The Object.freeze calls are being applied to the parent
VM globals; instead freeze the prototypes on the sandboxed context
(innerContext) so the inner VM’s prototypes are hardened—replace usages like
Object.freeze(Object.prototype) with
innerContext.Object.freeze(innerContext.Object.prototype) (and do the same for
Array.prototype, Function.prototype, String.prototype, Number.prototype,
Boolean.prototype, Date.prototype, Error.prototype, TypeError.prototype,
RangeError.prototype, SyntaxError.prototype, ReferenceError.prototype,
Promise.prototype) ensuring the freezes run inside innerContext rather than the
parent scope.
🧹 Nitpick comments (7)
docs/draft/docs/libraries/ast-guard.mdx (1)
80-134: Well-structured documentation for ResourceExhaustionRule.The new Built-in Rules section covers all the key elements per the coding guidelines: detected patterns, configuration options, example usage, and blocked attack examples.
One optional addition: consider adding a brief note linking this rule to the AgentScript preset mentioned earlier (lines 32-38), since the referenced guidance suggests documenting "how to instantiate rules for the AgentScript preset." A single sentence noting that
createAgentScriptPreset()includes this rule by default would help users understand when explicit configuration is needed versus when the preset handles it.libs/enclave-vm/src/memory-proxy.ts (2)
60-69: Size estimation includes one extra separator.The estimation adds
sep.lengthfor every element, butjoinonly insertsn-1separators fornelements. This results in a slight over-estimate (by one separator length). Since over-estimation is conservative and safe for limit checks, this is acceptable, but worth noting.🔎 More precise calculation (optional)
if (arrLength > 10000) { // Rough estimate for large arrays - estimatedSize = arrLength * (10 + sep.length) * 2; + estimatedSize = (arrLength * 10 + Math.max(0, arrLength - 1) * sep.length) * 2; } else { for (let i = 0; i < arrLength; i++) { const item = target[i]; - estimatedSize += (item == null ? 0 : String(item).length) + sep.length; + estimatedSize += (item == null ? 0 : String(item).length); + if (i < arrLength - 1) estimatedSize += sep.length; } estimatedSize = estimatedSize * 2; // UTF-16 }
164-196: Consider extracting shared join-estimation logic.The size estimation and limit-check logic in
wrapArrayInstance.join(lines 53-84) andcreateTrackedArrayMethods.join(lines 164-196) are nearly identical. Extracting a shared helper would reduce duplication and ensure consistent behavior if the estimation heuristics change.🔎 Example helper extraction
function estimateJoinSize(arr: unknown[], separator: string): number { const sep = separator; const arrLength = arr.length; if (arrLength > 10000) { return arrLength * (10 + sep.length) * 2; } let size = 0; for (let i = 0; i < arrLength; i++) { const item = arr[i]; size += (item == null ? 0 : String(item).length) + sep.length; } return size * 2; } function checkJoinLimit(tracker: MemoryTracker, estimatedSize: number): void { const limit = tracker.getLimit(); if (limit > 0 && estimatedSize > limit) { throw new MemoryLimitError( `Array.join would exceed memory limit (estimated ${estimatedSize} bytes, limit ${limit} bytes)`, estimatedSize, limit, ); } }Then both join implementations can call these helpers.
libs/enclave-vm/src/adapters/vm-adapter.ts (1)
376-444: Consider extracting shared constants to reduce duplication.
NODEJS_24_DANGEROUS_GLOBALSis duplicated verbatim inparent-vm-bootstrap.ts(lines 67-135). This creates a maintenance burden—if security levels are updated in one file, the other may become inconsistent.Consider extracting this to a shared constants file (e.g.,
security-constants.ts) and importing it in both locations.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (1)
830-874: SafeObject logic duplicated from vm-adapter.ts.The SafeObject creation (lines 841-874) duplicates the logic from
createSafeObjectinvm-adapter.ts. Since this is generated code, direct import isn't possible, but consider:
- Extracting the method lists (
DANGEROUS_OBJECT_STATIC_METHODS,safeObjectMethods) to shared constants- Generating the SafeObject code from a single source of truth to ensure consistency
This would reduce the risk of security fixes being applied to one location but not the other.
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (2)
471-504: Consider test timing reliability.This test relies on timing assertions with generous margins (500ms timeout, <15s execution, 20s test timeout). While the margins help prevent flakiness, the test could still be sensitive to CI environment variability. The high
maxIterations: 10000000is intentional to test timeout rather than iteration limits.Consider documenting why the specific timing values were chosen, or if the elapsed time assertion at line 500 is strictly necessary for the test's purpose.
898-906: Consider simplifying complex conditional assertion.The assertion logic at lines 901-905 is complex with substring checking and conditional expectations. This could be refactored for clarity.
Suggested simplification
- expect(['parsed', 'error: Maximum call stack size exceeded']).toContain( - result.value?.toString().substring(0, 6) === 'error:' - ? 'error: Maximum call stack size exceeded' - : result.value, - ); + const value = result.value?.toString() || ''; + expect(value === 'parsed' || value.startsWith('error:')).toBe(true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/draft/docs/libraries/ast-guard.mdxdocs/live/docs/libraries/ast-guard.mdxlibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/memory-proxy.ts
🧰 Additional context used
📓 Path-based instructions (3)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/enclave-vm/src/memory-proxy.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/live/docs/libraries/ast-guard.mdxdocs/draft/docs/libraries/ast-guard.mdx
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/libraries/ast-guard.mdx
🧬 Code graph analysis (3)
libs/enclave-vm/src/memory-proxy.ts (1)
libs/enclave-vm/src/memory-tracker.ts (1)
MemoryLimitError(40-58)
libs/enclave-vm/src/adapters/vm-adapter.ts (1)
libs/enclave-vm/src/types.ts (1)
SecurityLevel(24-24)
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (2)
libs/enclave-vm/src/index.ts (1)
SecurityLevel(28-28)libs/enclave-vm/src/types.ts (1)
SecurityLevel(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Performance
- GitHub Check: Test
🔇 Additional comments (19)
docs/live/docs/libraries/ast-guard.mdx (1)
83-137: Verify if direct edits to live docs follow the intended workflow.Per the coding guidelines,
docs/docs/**is described as "latest rendered docs, automation-only," and contributors are encouraged to add/update content indocs/draft/docs/**instead. The Codex workflow is meant to generate live docs from drafts combined with code diffs.If this file is being edited directly alongside the draft, please confirm this is intentional and won't be overwritten by automation. Otherwise, consider removing changes from this file and relying on the draft-to-live generation workflow.
libs/enclave-vm/src/memory-proxy.ts (5)
10-10: LGTM!The
MemoryLimitErrorimport is correctly added and aligns with the class definition inmemory-tracker.ts.
204-249: Arrays from prototype methods are not wrapped.Arrays created via
concat,slice,splice,map,filter,flatMap, andflatare tracked for size but not wrapped withwrapArrayInstance. This means calling.join()on these result arrays would use the untracked nativejoinmethod, potentially bypassing the memory limit pre-check.If an attacker can create a large array via one of these methods and then call
join()on it, the pre-allocation check inwrapArrayInstancewouldn't apply.Consider whether these methods should also return wrapped arrays:
concat: function (this: unknown[], ...items: unknown[]) { const result = originalConcat.apply(this, items); tracker.trackArray(result.length); - return result; + return wrapArrayInstance(result); },Note: This would require
wrapArrayInstanceto be accessible increateTrackedArrayMethods, possibly by passing it as a parameter or defining it at module scope.
198-202: LGTM!The
toStringimplementation correctly delegates to the trackedjoinmethod viathis.join(','), ensuring memory limits are enforced consistently. The previously flagged unusedoriginalToStringvariable has been removed.
64-67:String(item)during estimation may trigger getters ortoString()methods.Calling
String(item)on each array element during size estimation could invoke user-definedtoString()methods or Proxy getters. In adversarial scenarios, this might:
- Cause side effects before the limit check
- Allow expensive operations to run during estimation
- Trigger recursive memory allocations
Since this is sandbox code, verify that the execution context limits what
toString()can do, or consider using a more defensive approach for the rough estimate path.Also applies to: 176-178
45-139: Good addition of memory limit enforcement for array operations.The
wrapArrayInstancewrapper and its integration into all array creation paths (apply,construct,from,of) provide comprehensive protection against memory exhaustion viajoin()attacks. The pre-allocation limit check is a sound defense-in-depth approach.libs/enclave-vm/src/adapters/vm-adapter.ts (4)
275-362: Well-designed SafeObject mechanism for blocking property manipulation attacks.The
createSafeObjectfunction correctly blocks dangerous static methods likedefinePropertyandsetPrototypeOfwhile preserving safe methods. The implementation handles bothObject()andnew Object()call patterns.Minor note: The function is not exported, but the AI summary indicates it should be "exposed." If this is intended for use by other modules (e.g., double-vm bootstrap), consider exporting it to avoid code duplication.
484-496: LGTM!The SafeObject injection correctly uses the host's
Objectconstructor to create a clean, untampered safe version that shadows the VM's internalObjectglobal. Thewritable: false, configurable: falseproperties prevent runtime overrides.
549-556: LGTM!The
codeGeneration: { strings: false, wasm: false }setting is correctly placed atcreateContexttime. This provides V8-level protection againstnew Function()andeval()from strings, complementing the global removal as defense-in-depth.
564-574: LGTM!The guard
if (key in baseSandbox) continuecorrectly prevents overwriting the SafeObject that was already injected bysanitizeVmContext. This ensures the security-hardened Object global is preserved.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (3)
776-780: LGTM!The inner VM context is correctly created with
codeGeneration: { strings: false, wasm: false }to block dynamic code execution at the V8 level.
877-902: LGTM!SafeObject is correctly wrapped with
createSecureProxybefore injection intosafeGlobals, providing defense-in-depth by combining the SafeObject restrictions with the secure proxy's property blocking.
950-955: LGTM!The comment correctly clarifies that
codeGenerationis configured increateContext(), notrunInContext(). This documentation helps future maintainers understand the security architecture.libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (6)
1-58: LGTM: Well-documented test suite with good coverage of BigInt exponentiation limits.The documentation header clearly explains the defense layers, and the BigInt exponentiation tests provide good coverage of both blocked and allowed scenarios. The consistent use of
dispose()ensures proper cleanup.
600-668: LGTM: Thorough coverage of native sort attack vectors.The Sort Attack tests appropriately handle the complexity of testing native code execution, with conditional assertions that accept either successful completion or security blocking. The tests verify both AST-level detection (large arrays) and runtime defense (iteration counting in callbacks).
678-795: Excellent defense-in-depth testing for constructor escape vectors.The ATK-ESC-01 test suite comprehensively covers multiple constructor leak vectors (arrow functions, object methods, class methods, bound functions) and verifies both AST-level and runtime defenses. The test at lines 694-709 that explicitly disables validation to test runtime-only defense is particularly valuable.
1001-1186: Comprehensive serialization hijack coverage with good legitimate-use verification.The ATK-DATA-02 tests thoroughly verify that metaprogramming APIs (defineProperty, setPrototypeOf, etc.) are blocked while ensuring safe Object methods (keys, values, entries, freeze, isFrozen) remain available. The test at lines 1129-1156 using
toolHandleris particularly good at validating defense-in-depth.
1199-1479: Critical security verification: dangerous globals removal at VM level.The ATK-RECON-01 test suite provides essential verification that dangerous globals (Function, eval, Proxy, Reflect, etc.) are removed from the VM context even when AST validation is disabled. The tests at lines 1442-1477 comprehensively verify all dangerous globals are undefined while essential globals remain available. This is a critical security boundary test.
19-1479: Only 3 of 95 tests have custom timeouts—the suite structure is appropriate.The test suite contains 95 test cases, but only 3 explicitly configure Jest timeouts: one at 10000ms, one at 15000ms, and one at 20000ms (lines 159, 503, 638). The remaining 92 tests use Jest's default 5-second timeout. These custom timeouts are appropriately scoped to resource-exhaustion integration tests (worker pool termination, sort attacks, deep nesting) that require additional time for realistic attack scenarios. The jest.config.ts does not override the default timeout, and no performance concerns are documented in the CI workflow or recent commits.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts:
- Around line 785-850: The memory-safe prototype patching is executed after the
sanitization (via ${sanitizeContextCode}), which conflicts with vm-adapter's
requirement to run the patch BEFORE sanitizeVmContext/SafeObject replacement;
move the patch generation/run (the patchCode + new
vm.Script(...).runInContext(innerContext) block that patches
stringProto/arrayProto) so it executes prior to injecting
${sanitizeContextCode}, ensuring the injection runs while Object.getPrototypeOf
still refers to the realm intrinsics and before Object is replaced by
SafeObject.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/enclave-vm/src/double-vm/double-vm-wrapper.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts
🧬 Code graph analysis (2)
libs/enclave-vm/src/adapters/vm-adapter.ts (2)
libs/enclave-vm/src/index.ts (1)
SecurityLevel(28-28)libs/enclave-vm/src/types.ts (1)
SecurityLevel(24-24)
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(157-739)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Performance
- GitHub Check: Test
🔇 Additional comments (29)
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (16)
1-18: Excellent documentation and clear defense-layer explanation.The header comment provides a comprehensive overview of the test suite's purpose and the multi-layered defense strategy. This helps reviewers and maintainers understand the security model being tested.
24-58: Well-structured boundary testing for BigInt exponentiation.The tests properly cover both blocking cases (exceeding limits) and allowing cases (within limits), with correct disposal patterns and appropriate assertions.
60-86: LGTM! Comprehensive infinite loop detection.The tests cover the main infinite loop patterns (while(true), for(;;), while(1)) that should be blocked at the AST level.
93-140: Solid iteration limit enforcement testing.The tests verify runtime iteration counting across different loop types (for, for-of) with both blocking and allowing scenarios.
142-161: LGTM! Appropriate test for multi-layer protection.The test correctly verifies that iteration limits provide protection, with a safety timeout. The expectation aligns with the configured limits (iteration limit of 100 will be hit before timeout).
166-216: Thorough AST-level memory exhaustion detection.The tests properly verify that large allocations (arrays, string repeats, Array.join) are blocked at the AST level while reasonable sizes are allowed.
221-250: Runtime memory limit enforcement well-tested.The tests verify memory tracking works at runtime for exponential string growth while allowing normal operations within limits.
255-362: Comprehensive constructor obfuscation attack coverage.The tests thoroughly verify blocking of various constructor access patterns, including direct access, computed properties, string concatenation, and prototype/proto access. All major escape vectors are covered.
367-396: Appropriate code generation blocking tests.The tests correctly handle dual-layer defenses (AST and runtime) for Function constructor and eval, with flexible assertions that work for both blocking mechanisms.
401-466: Essential sanity checks for legitimate code paths.These tests are critical to ensure the security measures don't break normal, safe operations. Good coverage of common patterns (arrays, objects, strings, BigInt, iteration).
471-504: LGTM! Critical integration test for worker termination.This test verifies the watchdog timeout mechanism can forcefully terminate unresponsive workers, which is essential for preventing stuck processes. The timeout configuration (500ms worker timeout, 20s test timeout, <15s elapsed check) provides appropriate safety margins.
507-526: Clear unit tests for AST rule behavior.These focused unit tests specifically verify the ResourceExhaustionRule's BigInt detection logic, separate from the broader integration tests.
534-668: Excellent real-world attack scenario coverage.The ATK-MEM-01 tests cover multiple variations of the "Billion Laughs" memory bomb, including literal values, computed expressions, and iteration-based attacks. The Sort Attack tests verify protection against native code exploitation. The key assertion that the system doesn't crash (line 590) is particularly important.
678-1039: Comprehensive coverage of sophisticated attack vectors.The ATK-ESC-01 tests thoroughly cover constructor escape techniques with both AST and runtime defenses tested separately. The template literal injection tests (ATK-TPL-01) verify that transforms apply inside ${} expressions. The JSON bomb tests (ATK-JSON-02/03) appropriately focus on "no crash" assertions rather than just blocking, which is the right approach for native code attacks.
1047-1587: Exceptional comprehensive security testing with defense-in-depth validation.The ATK-BRIDGE-04 and ATK-DATA-02 tests thoroughly cover metaprogramming attack vectors (defineProperty, getters, setPrototypeOf, etc.) with excellent verification that safe Object methods remain functional (lines 1210-1235). The tool result defense test (lines 1237-1264) is particularly clever in testing runtime protection even when a tool returns a dangerous method name.
The ATK-RECON-01 suite is exemplary in its approach: using
validate: falseto specifically isolate and test VM-level defenses separate from AST validation, with clear comments explaining the dual-layer strategy (lines 1302-1306). The comprehensive checking of all dangerous globals (lines 1550-1585) provides strong verification that the VM context is properly sanitized.
1-1587: Outstanding comprehensive test suite for resource exhaustion prevention.This test file demonstrates exceptional security engineering practices:
Strengths:
Comprehensive Attack Coverage: Tests cover real-world attack scenarios (ATK-MEM-01 Billion Laughs, ATK-ESC-01 Constructor Leak, ATK-JSON-02/03 Parser Bombs, ATK-BRIDGE-04 Zombie Object, ATK-DATA-02 Serialization Hijack, ATK-RECON-01 Global Reconnaissance) with clear documentation of each attack vector.
Defense-in-Depth Validation: Tests systematically verify multiple layers (AST validation, runtime limits, VM context sanitization) both together and in isolation (using
validate: falseto test runtime defenses specifically).Proper Resource Management: Every test properly disposes the Enclave instance—no resource leaks detected.
Realistic Scenarios: Attack scenarios use actual exploitation techniques (string concatenation obfuscation, computed property access, template literal injection) rather than just testing APIs directly.
Sanity Checks: Includes tests verifying legitimate code still works (lines 401-466, 1210-1235), which is essential for avoiding false positives.
Flexible Assertions: Error message patterns accommodate dual failure paths (AST vs. runtime blocking), making tests robust across implementation changes.
Minor Observations:
- Test timeouts are appropriately generous (10s-20s) for integration tests involving worker pools
- The "no crash" assertions for JSON bombs (lines 590, 926, 948) are the correct approach for native code attacks where blocking may not be possible
- The use of
validate: falseis well-justified in comments (lines 1302-1306) to isolate VM-level defensesThis test suite provides excellent protection against regression and strong confidence in the security posture of the Enclave VM.
libs/enclave-vm/src/adapters/vm-adapter.ts (8)
275-293: LGTM! Well-documented dangerous methods list.The list of dangerous Object static methods is comprehensive and correctly focuses on static methods. The comment correctly notes that
__defineGetter__etc. are instance methods on Object.prototype, not static methods, so they're handled separately.
364-444: LGTM! Comprehensive dangerous globals expansion.The expanded
NODEJS_24_DANGEROUS_GLOBALSlists are well-organized by risk category and security level. The defense-in-depth approach is sound—even thoughcodeGeneration.strings=falseblocksnew Function(), removingFunctionentirely eliminates future bypass vectors.The categorization is clear:
- Code execution (Function, eval, globalThis)
- Metaprogramming (Proxy, Reflect)
- Memory/timing attacks (SharedArrayBuffer, Atomics, gc)
- Future APIs (ShadowRealm, WeakRef, FinalizationRegistry, etc.)
484-496: LGTM! SafeObject injection correctly shadows VM's Object.The SafeObject is correctly injected into the VM context after dangerous globals are removed. The use of
Object.definePropertywithwritable: falseandconfigurable: falseensures sandbox code cannot override this protection.
548-556: LGTM! Code generation restrictions correctly configured.The
codeGeneration: { strings: false, wasm: false }option is correctly set atvm.createContext()time, which prevents sandbox escape vianew Function()andeval()from strings, as well as WebAssembly attacks.
558-631: LGTM! Memory-safe prototype patching with correct ordering.The memory-safe prototype patches are correctly applied before
sanitizeVmContext, which is critical becausesanitizeVmContextreplaces the intrinsicObjectwithSafeObject. The patches need access to the intrinsicObject.getPrototypeOfto reach the VM realm's actualString.prototypeandArray.prototype.The pre-allocation checks for
repeat(),join(),padStart(), andpadEnd()provide effective defense against memory exhaustion attacks by estimating size before allocation.
639-642: LGTM! Correct skip logic to preserve SafeObject.The check
if (key in baseSandbox) continue;correctly prevents overwriting theSafeObjectthat was already added bysanitizeVmContext. This maintains the security boundary.
712-712: LGTM! Helpful clarification comment.The comment correctly notes that
codeGenerationis configured atcreateContext()time, notrunInContext(), which is an important distinction for developers maintaining this code.
304-362: SafeObject implementation is correct and intentional.The use of the host realm's
Objectinternally is by design and safe. The SafeObject wrapper controls all exposed methods and properly blocks property manipulation attacks:
- Dangerous static methods (defineProperty, setPrototypeOf, getOwnPropertyDescriptor, getOwnPropertyDescriptors) are blocked with helpful error messages
- Only safe methods (keys, values, entries, assign, freeze, etc.) are copied to SafeObject
- Object.create is restricted to disallow property descriptors as the second argument
- The prototype is correctly aliased from the VM context
Internal delegation to host Object (lines 311, 348) returns only converted primitives, never host realm objects. Tests confirm Object.defineProperty is properly blocked at runtime, preventing serialization hijack attacks (ATK-DATA-02) and prototype pollution. No escape vectors exist.
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (5)
56-135: Consistent dangerous globals list across VM layers.The
NODEJS_24_DANGEROUS_GLOBALSlists are consistent with the implementation invm-adapter.ts, which is good for defense-in-depth. Both the outer VM adapter and the parent VM bootstrap apply the same security restrictions.Note: This is code duplication, but appears necessary since this file generates bootstrap code that runs in a separate VM context.
775-780: LGTM! Consistent code generation restrictions.The
innerContextis correctly created withcodeGeneration: { strings: false, wasm: false }, consistent with the implementation invm-adapter.ts.
897-941: LGTM! SafeObject implementation consistent with vm-adapter.ts.The SafeObject implementation in the parent VM bootstrap matches the pattern in
vm-adapter.ts, blocking dangerous static methods while preserving safe operations.
948-948: LGTM! SafeObject correctly added to safeGlobals.The
SafeObjectis correctly wrapped increateSecureProxyand added to thesafeGlobalsmap, which will shadow the native Object ininnerContext.
1018-1018: LGTM! Helpful clarification comment.The comment correctly clarifies that
codeGenerationis configured atcreateContext()time, consistent withvm-adapter.ts.
…in both parent and inner VMs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts:
- Around line 776-783: The memory-safe prototype patching must run before
sanitization: locate the innerContext creation (var innerContext =
vm.createContext(...)) and the subsequent memory-safe prototype patching block
(the code that patches Object/Array/Function prototypes to prevent memory
tampering), and move that entire patching block so it executes immediately after
innerContext is created and before the injected ${sanitizeContextCode} runs;
keep the sanitizeContextCode invocation after the patching to match
vm-adapter.ts ordering and preserve expected behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/enclave-vm/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/enclave-vm/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts
🧬 Code graph analysis (1)
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (2)
libs/enclave-vm/src/index.ts (1)
SecurityLevel(28-28)libs/enclave-vm/src/types.ts (1)
SecurityLevel(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Performance
- GitHub Check: Test
🔇 Additional comments (8)
libs/enclave-vm/src/__tests__/enclave.infinite-loop-attacks.spec.ts (2)
485-506: LGTM! Test expectations correctly updated to reflect prototype freezing security enhancement.The updated expectations are correct. With frozen prototypes, the custom circular
toStringmethod is effectively ignored, and V8 falls back to the defaultObject.prototype.toString, which safely returns'[object Object]'without infinite recursion. This demonstrates that the attack vector is successfully neutralized at runtime.
508-530: LGTM! Test expectations correctly updated to reflect prototype freezing security enhancement.The updated expectations are correct. With frozen prototypes, the custom circular
valueOfmethod is effectively ignored, and V8 falls back to the defaultObject.prototype.valueOf. When the returned object is coerced to a number using the unary+operator, it correctly results inNaN. This demonstrates that the attack vector is successfully neutralized at runtime.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (6)
58-135: Documentation improvements look good.The enhanced comments clearly explain the security rationale for removing each dangerous global at different security levels. The defense-in-depth explanation for
codeGeneration.strings=falsecombined withFunctionremoval is particularly valuable.
785-850: Memory patching implementation is solid.The approach of getting prototypes from literals (
""and[]) correctly targets the realm's intrinsic prototypes. The pre-allocation memory checks forrepeat,join,padStart, andpadEndeffectively prevent memory exhaustion attacks like ATK-JSON-03.
852-895: Dual-context prototype freezing correctly implemented.The code now properly freezes prototypes in both contexts:
Parent VM prototypes (lines 863-875): Necessary because
SafeObject.prototypeis assigned to the parent VM'sObject.prototype(line 961), and user code accessingObject.prototypein the inner VM reaches this parent prototype via the SafeObject proxy.Inner VM prototypes (lines 877-895): Freezes the intrinsic prototypes used by literals (
"",[], etc.) withininnerContextby executing a freeze script inside that context.This defense-in-depth approach prevents prototype pollution attacks from both access paths.
926-970: SafeObject implementation is well-designed.The SafeObject wrapper correctly blocks dangerous static methods (
defineProperty,defineProperties,setPrototypeOf, etc.) while exposing a curated set of safe operations. The restrictedObject.createthat disallows property descriptors is particularly important for preventing property manipulation attacks like ATK-DATA-02.
977-977: Correct usage of SafeObject in innerContext.Replacing the standard
Objectwith the wrappedSafeObjectin the inner VM's global scope ensures that all user code accesses toObjectgo through the security layers: SafeObject restrictions → SecureProxy → frozen prototypes.
1047-1047: Helpful clarification comment.This comment correctly notes that the
codeGenerationrestrictions are configured duringcreateContext()(line 779), not at therunInContext()call site, preventing potential confusion.
…zation to prevent security vulnerabilities
Summary by CodeRabbit
New Features
Security Enhancements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.