feat: implement memory tracking and limit enforcement for VM execution #16
Conversation
✅ 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. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds end-to-end memory tracking and enforcement across Enclave: new MemoryTracker and sizing helpers, tracked String/Array proxies and AST transforms, runtime integrations in safe runtime, single/worker/double-VM adapters, worker-slot/script adjustments, secure-proxy throw-on-blocked behavior, and extensive test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant E as Enclave
participant T as MemoryTracker
participant R as SafeRuntime
participant V as VM (worker/double)
U->>E: execute(code, options{memoryLimit})
E->>T: create/start (if memoryLimit>0)
E->>E: applyMemoryTrackingTransforms(code) (if no sidecar)
E->>R: createSafeRuntime({memoryTracker: T})
R->>V: run(user code in VM)
V->>T: report allocations (strings/arrays) during execution
alt limit exceeded
T-->>V: throw MemoryLimitError
V-->>E: propagate structured MEMORY_LIMIT_EXCEEDED error
else within limit
V-->>E: return result
end
E->>T: getSnapshot()
T-->>E: peakTrackedBytes
E-->>U: {result, stats: {memoryUsage}}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @libs/enclave-vm/src/enclave.ts:
- Line 98: Update the JSDoc for the memoryLimit property in types.ts to reflect
the new default of 1MB (1 * 1024 * 1024) instead of 128MB, explicitly note that
this change was intentional and is a breaking change for users upgrading from
prior versions, and add a short entry to the project release notes or migration
guide documenting the change and pointing out that memoryLimitPerWorker in the
worker pool still defaults to 128MB (so users should adjust that separately if
needed); locate the memoryLimit JSDoc near the memoryLimit property declaration
in types.ts and update the default value text and add a one-line breaking-change
note, then add the release-notes entry referencing the tests (e.g.,
memory-limit.spec.ts) that confirm the 1MB default.
In @libs/enclave-vm/src/index.ts:
- Around line 105-114: Add a dedicated API section for the Memory Tracker
exports (MemoryTracker, MemoryLimitError, MemoryTrackerConfig,
MemoryUsageSnapshot, estimateStringSize, estimateArraySize, estimateObjectSize)
in the docs: document each symbol’s purpose and TypeScript signature, list
configuration options available on MemoryTrackerConfig, show concise examples
for instantiating MemoryTracker, recording/tracking objects, requesting
snapshots (MemoryUsageSnapshot) and handling MemoryLimitError, and include short
usage snippets demonstrating the size estimator helpers and common patterns
(start/stop, snapshot comparison, and error handling).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/enclave-vm/src/adapters/worker-pool/worker-slot.ts (1)
242-248: Public API breaking change:markForRecycleparameter removal requires documentation update.
WorkerSlotis exported as public API, and removing thereasonparameter frommarkForRecycle()is a breaking change for external consumers. Per the coding guidelines forlibs/**, public API changes must include matching documentation updates indocs/draft/docs/**, which are currently missing. All internal call sites are compatible with the new signature.
🧹 Nitpick comments (8)
eslint.config.mjs (1)
8-8: Consider linting test files with relaxed rules instead of excluding them entirely.Excluding all test and spec files from linting may miss legitimate issues (e.g., incorrect assertions, unused variables in tests). A common alternative is to keep tests linted but with relaxed rules for test-specific patterns.
If this is intentional to reduce noise from test-specific patterns, this is acceptable.
libs/enclave-vm/src/adapters/worker-pool/worker-slot.ts (1)
175-191: Path detection may match unintended directories.The check
__dirname.includes('/src/')could match paths containing/src/in other locations (e.g.,/some-src/or nested paths like/node_modules/@scope/src/). Consider using a more precise pattern.🔎 Proposed fix for more precise path matching
- // Check if we're running from src (ts-jest tests) - if (__dirname.includes('/src/')) { - // Replace /src/ with /dist/src/ to find compiled JS - const distPath = __dirname.replace('/src/', '/dist/src/'); + // Check if we're running from src (ts-jest tests) + // Match /src/ only when it's followed by adapters/worker-pool (our expected path) + if (__dirname.includes('/src/adapters/worker-pool')) { + // Replace /src/ with /dist/src/ to find compiled JS + const distPath = __dirname.replace('/src/', '/dist/src/'); return path.join(distPath, 'worker-script.js'); }libs/enclave-vm/src/__tests__/worker-pool-adapter.spec.ts (3)
79-95: Missing cleanup in async test.This test creates an enclave but doesn't dispose it in a finally block. If the test fails, the worker pool won't be cleaned up, potentially causing resource leaks or interference with other tests.
🔎 Proposed fix to add cleanup
it('should execute async code with await', async () => { const toolHandler: ToolHandler = async () => { await new Promise((r) => setTimeout(r, 10)); return { done: true }; }; const enclave = createWorkerEnclave({ toolHandler }); - const result = await enclave.run(` - async function __ag_main() { - const response = await callTool('asyncOp', {}); - return response.done ? 'async complete' : 'failed'; - } - `); - - expect(result.success).toBe(true); - expect(result.value).toBe('async complete'); + try { + const result = await enclave.run(` + async function __ag_main() { + const response = await callTool('asyncOp', {}); + return response.done ? 'async complete' : 'failed'; + } + `); + + expect(result.success).toBe(true); + expect(result.value).toBe('async complete'); + } finally { + enclave.dispose(); + } });
97-109: Missing cleanup in error handling test.Same issue - enclave not disposed in finally block.
111-133: Missing cleanup in multiple tests.Several tests in this block (
should return primitives correctly) don't dispose the enclave. Consider extracting a shared enclave instance withbeforeEach/afterEachor wrapping each test in try/finally.libs/enclave-vm/src/__tests__/memory-limit.spec.ts (1)
344-346: Fragile Node.js version detection.Parsing the version string with
slice(1).split('.')[0]works but could fail with unexpected version formats. Consider usingprocess.versions.nodeor a more robust parser.🔎 Proposed fix for more robust version parsing
- const nodeVersion = parseInt(process.version.slice(1).split('.')[0], 10); - const skipDueToNodeVersion = nodeVersion >= 24; + // Parse major version more robustly + const nodeVersion = parseInt(process.versions.node.split('.')[0], 10); + const skipDueToNodeVersion = Number.isNaN(nodeVersion) || nodeVersion >= 24;libs/enclave-vm/src/memory-proxy.ts (1)
176-185: Pre-check in repeat() uses incorrect size estimation.The pre-check calculates
estimateStringSize(this) * count, butestimateStringSizealready includes the 40-byte overhead per string. The actual result is one string withthis.length * countcharacters, so the size should bethis.length * count * 2 + 40, not(this.length * 2 + 40) * count.This causes the pre-check to over-estimate, which is conservative (may reject valid operations) but not a correctness issue.
🔎 Proposed fix for accurate size estimation
repeat: function (this: string, count: number) { // Pre-check to avoid allocating before tracking - const estimatedSize = estimateStringSize(this) * count; + // Result is one string with (this.length * count) characters + const estimatedSize = this.length * count * 2 + 40; if (tracker.getLimit() > 0 && estimatedSize > tracker.getLimit()) { tracker.track(estimatedSize); // This will throw } const result = originalRepeat.call(this, count); tracker.trackString(result); return result; },libs/enclave-vm/src/memory-tracker.ts (1)
258-263: formatBytes doesn't handle edge cases.Negative values would display incorrectly (e.g.,
-1024would show as-1024Binstead of being formatted). This is likely fine sincetrackedBytesshould never be negative, but consider adding a guard.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
eslint.config.mjslibs/ast-guard/src/transforms/concat.transform.tslibs/enclave-vm/src/__tests__/memory-limit.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/worker-pool-adapter.spec.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/adapters/worker-pool/execution-queue.tslibs/enclave-vm/src/adapters/worker-pool/memory-monitor.tslibs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.tslibs/enclave-vm/src/adapters/worker-pool/worker-slot.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/index.tslibs/enclave-vm/src/memory-proxy.tslibs/enclave-vm/src/memory-tracker.tslibs/enclave-vm/src/safe-runtime.tslibs/enclave-vm/src/types.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/ast-guard/src/transforms/concat.transform.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/safe-runtime.tslibs/enclave-vm/src/index.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/__tests__/memory-limit.spec.tslibs/enclave-vm/src/memory-tracker.tslibs/enclave-vm/src/__tests__/worker-pool-adapter.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/memory-proxy.tslibs/enclave-vm/src/adapters/worker-pool/memory-monitor.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/adapters/worker-pool/execution-queue.tslibs/enclave-vm/src/adapters/worker-pool/worker-slot.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.ts
🧬 Code graph analysis (3)
libs/enclave-vm/src/enclave.ts (1)
libs/ast-guard/src/transforms/concat.transform.ts (2)
transformConcatenation(65-116)transformTemplateLiterals(131-200)
libs/enclave-vm/src/__tests__/memory-limit.spec.ts (2)
libs/enclave-vm/src/index.ts (4)
MemoryTracker(107-107)estimateStringSize(111-111)estimateArraySize(112-112)MemoryLimitError(108-108)libs/enclave-vm/src/memory-tracker.ts (4)
MemoryTracker(126-264)estimateStringSize(70-75)estimateArraySize(87-91)MemoryLimitError(40-58)
libs/enclave-vm/src/memory-proxy.ts (2)
libs/enclave-vm/src/index.ts (2)
MemoryTracker(107-107)estimateStringSize(111-111)libs/enclave-vm/src/memory-tracker.ts (3)
MemoryTracker(126-264)start(144-148)estimateStringSize(70-75)
🔇 Additional comments (42)
libs/ast-guard/src/transforms/concat.transform.ts (1)
137-153: LGTM! Correctly preserves tagged template expressions.The two-pass approach properly identifies and skips tagged templates (e.g.,
html`...`), which must pass the raw template to the tag function rather than be transformed into__safe_template__calls. Using a Set with direct node references is efficient and correct sinceacorn-walktraverses the same AST objects.libs/enclave-vm/src/adapters/worker-pool/memory-monitor.ts (2)
10-11: LGTM!Import cleanup is consistent with the simplified
markForRecycle()signature.
156-162: LGTM!The simplified
markForRecycle()call aligns with the updated signature inworker-slot.ts. The memory-exceeded event is still emitted (Line 158) before marking for recycle, so the reason is communicated through the event system rather than the method parameter.libs/enclave-vm/src/adapters/worker-pool/worker-script.ts (4)
141-147: LGTM!The wrapped code correctly calls
__ag_main()if defined, aligning with the Enclave's AST transformation that wraps user code in an async__ag_mainfunction. The fallback toundefinedwhen__ag_mainis not defined is appropriate.
292-296: LGTM!Exposing console under both
__safe_consoleandconsoleprovides backward compatibility while supporting the AST transformer's prefixed naming convention.
311-316: LGTM!Adding
__safe_prefixed error types enables the AST transformer to rewrite error constructor references while still allowing the sandbox to resolve them correctly.
298-334: Memory tracking is handled differently in the worker pool adapter and does not require memory-tracked constructors.The worker pool uses OS-level memory limiting via the
--max-old-space-sizeNode.js flag (set inworker-slot.ts:155) combined with external RSS-based monitoring inMemoryMonitor, rather than the memory-tracked proxy approach used in the VM adapter. ThememoryLimitconfiguration is intentionally not serialized and passed to workers—it controls the worker process heap limit instead. This architectural choice differs from the VM adapter but is appropriate for the worker pool's OS-level isolation model.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (4)
47-49: LGTM!Adding
memoryLimittoParentVmBootstrapOptionsenables memory-aware code generation for the double-VM path.
196-198: LGTM!The
memoryLimitconstant is correctly emitted into the generated bootstrap code, making it available at runtime for memory tracking decisions.
508-527: LGTM! Numeric addition fast path and incremental memory tracking.The numeric addition fast path (Lines 509-512) correctly follows JavaScript
+semantics. For string concatenation, tracking onlyrightLen * 2bytes is correct for incremental tracking since the left operand's memory was already accounted for in prior operations.
574-584:__host_memory_track__is properly injected into parent VM context.The function is defined in
double-vm-wrapper.tsviaObject.defineProperty()on the parent context, with appropriate handling for both enabled and disabled memory tracking scenarios.libs/enclave-vm/src/adapters/worker-pool/execution-queue.ts (1)
152-155: LGTM!Replacing the length pre-check with a direct
shift()followed by an undefined guard is safer and eliminates the non-null assertion (!). The behavior remains identical while being more defensive.libs/enclave-vm/src/types.ts (2)
391-399: Documentation clearly explains memory tracking semantics.The expanded documentation for
memoryUsageeffectively communicates that this is an estimation based on allocation tracking (String/Array), not actual V8 heap usage. The guidance about using worker_threads for precise isolation is helpful.
434-443: Documentation clearly explains memoryLimit enforcement.The documentation properly describes that memoryLimit enables allocation tracking rather than V8 heap limits, notes the overhead (~5-10%), and provides appropriate guidance for precise isolation via worker_threads.
libs/enclave-vm/src/adapters/vm-adapter.ts (4)
361-374: Memory tracker initialization is correct.The conditional creation of MemoryTracker when
memoryLimit > 0is appropriate. The configuration disables object tracking (line 369) which is a reasonable optimization given the noted overhead. The call tostart()on line 374 properly initiates tracking before execution.
382-382: Memory tracker correctly passed to safe runtime.Passing the
memoryTrackertocreateSafeRuntimeenables allocation monitoring within the sandbox, which is the correct integration point for memory tracking.
475-479: Memory usage captured in both success and error paths.The implementation correctly captures peak tracked memory in both success (lines 475-479) and error (lines 493-497) paths, ensuring stats are complete regardless of execution outcome.
Also applies to: 493-497
499-514: MemoryLimitError handling provides good error context.The special-case handling for
MemoryLimitErrorreturns a structured error with:
- Descriptive name and message
- Error code
MEMORY_LIMIT_EXCEEDED- Data with
usedBytesandlimitBytesThis provides clear actionable information for debugging memory limit issues.
libs/enclave-vm/src/__tests__/safe-runtime-reference.spec.ts (1)
53-66: Tests correctly validate JavaScript + operator semantics.The updated test cases properly verify that
__safe_concatfollows JavaScript's + operator behavior when no sidecar is configured:
- Numeric addition:
1 + 2 === 3✓- Boolean coercion:
true + false === 1✓- Null/undefined:
null + undefined === NaN✓- String concatenation with numbers ✓
This aligns with the implementation described in the relevant code snippets from
safe-runtime.ts.libs/enclave-vm/src/enclave.ts (2)
323-327: Memory tracking transform logic is correct.The conditional application of memory tracking transforms is well-structured:
- Applied only when
memoryLimit > 0(line 323)- Applied only when no sidecar is configured (avoids duplicate transforms)
- Follows the same pattern as sidecar transforms (lines 321-322)
460-486: Memory tracking transform implementation is sound.The
applyMemoryTrackingTransformsmethod correctly:
- Parses code into AST (lines 471-474)
- Applies concatenation transform (line 478)
- Applies template literal transform (line 482)
- Regenerates code (line 485)
The implementation pattern mirrors
applySidecarTransforms(lines 497-522), ensuring consistency.libs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.ts (3)
188-192: State change handler correctly updated for new signature.The handler now receives only
newState(line 188), matching the updated event signature described in the AI summary. The recycling check on line 189 works correctly with the single parameter.
243-243: markForRecycle() called with simplified signature.The call to
markForRecycle()without a reason parameter aligns with the simplified API mentioned in the AI summary. This removes unused metadata while preserving the recycling trigger functionality.
276-299: handleMemoryExceeded simplified appropriately.The method signature now takes only
slotId(line 276), removing unusedusageandlimitparameters. The implementation correctly:
- Retrieves the slot (line 277)
- Checks if executing (line 281)
- Increments termination counter (line 282)
- Terminates and cleans up (lines 283-297)
libs/enclave-vm/src/adapters/worker-pool/worker-slot.ts (1)
153-159: LGTM - Conditional memory flag for Node.js 24+ compatibility.Good fix for handling Node.js 24+ stricter worker permissions. The conditional check ensures backward compatibility with earlier versions while avoiding failures on newer runtimes.
libs/enclave-vm/src/__tests__/worker-pool-adapter.spec.ts (1)
463-478: Good test for preserving JavaScript numeric semantics.This test verifies that the memory tracking transforms don't inadvertently break numeric addition (e.g., by coercing numbers to strings). Important regression test.
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (3)
70-82: Memory tracker configuration disables object tracking.The tracker is created with
trackObjects: false, which means object allocations won't be tracked. This may be intentional for performance, but large object graphs could still exhaust memory without triggering limits. Consider documenting this limitation.
136-148: Good structured error handling for MemoryLimitError.The error includes useful diagnostic data (
usedBytes,limitBytes) and a consistent error code. This enables callers to programmatically handle memory limit violations.
227-245: No-op callback pattern for disabled tracking.Good approach to always define
__host_memory_track__(even as no-op) to avoid conditional checks in parent VM code. This simplifies the parent VM bootstrap logic.libs/enclave-vm/src/__tests__/memory-limit.spec.ts (3)
10-57: Comprehensive unit tests for MemoryTracker.Good coverage of basic tracking, string/array size tracking, and reset behavior. The tests verify both accumulation and peak tracking correctly.
60-96: Good limit enforcement tests with edge cases.Tests cover the limit exceeded case, error details verification, and the unlimited (0) case. The
fail()pattern in the try-catch ensures the error is actually thrown.
529-549: Good regression test for numeric addition semantics.This test ensures that the memory tracking transforms don't break JavaScript's
+operator semantics for numbers. Important for correctness validation.libs/enclave-vm/src/safe-runtime.ts (3)
271-275: Critical numeric fast path for performance.Good optimization - checking both operands are numbers before any string operations ensures numeric addition isn't penalized by memory tracking overhead.
326-337: Potential over-tracking for mixed-type concatenation.For cases like
"hello" + 123, the entire result string is tracked (result.length * 2), not just the growth. This could over-count memory if the left operand was already tracked. However, this is a conservative approach that ensures no allocations are missed.
530-547: Memory-tracked constructors correctly integrated.When
memoryTrackeris provided, the String and Array constructors are wrapped with tracking proxies. This ensures all allocations through these constructors are monitored.libs/enclave-vm/src/memory-proxy.ts (3)
21-34: String constructor proxy tracks allocations correctly.Both
apply(callingString(value)) andconstruct(new String(value)) are handled. ThevalueOf()call on line 30 correctly gets the primitive string from the String object.
118-126: Splice tracking may over-count allocations.The splice wrapper tracks both the removed items array (line 121) and the net growth (lines 122-124). However, if
deleteCountis provided but items are not added, this still works correctly. The logic looks sound.
187-201: padStart/padEnd only track when string grows.Good optimization - only tracking when the result is longer than the original avoids unnecessary overhead for no-op padding operations.
libs/enclave-vm/src/memory-tracker.ts (4)
40-58: Well-implemented custom Error class.Proper inheritance from Error with
Error.captureStackTracefor V8 stack traces. Thecodeproperty is readonly and set directly, making it easy to check programmatically. Good pattern for custom errors.
70-75: String size estimation follows V8 memory model.The 2-bytes-per-character (UTF-16) plus 40-byte overhead is a reasonable approximation for V8's string storage. The comment explains the short-string optimization that's not accounted for.
156-176: Correct limit enforcement with peak tracking.The implementation correctly:
- Ignores non-positive allocations
- Updates peak before checking limit (so peak reflects the violating allocation)
- Only checks limit when limit > 0
217-219: Release correctly floors at zero.Using
Math.max(0, ...)ensures tracked bytes never go negative, which could cause incorrect limit checks. This aligns with the test at lines 111-120 in the spec file.
… security violation handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (1)
577-603: Minor: Variable shadowing may cause confusion.
leftIsRefandrightIsRefare declared twice withvar(lines 550-551 and 580-581). While this works due tovarhoisting, it can be confusing. Consider using different variable names in the second block for clarity.🔎 Suggested rename for clarity
// For mixed types (one string, one non-string), check for references first // then use native + for proper ToPrimitive handling if (typeof left === 'string' || typeof right === 'string') { // Check if the string operand(s) are references BEFORE coercion - var leftIsRef = typeof left === 'string' && refIdPattern.test(left); - var rightIsRef = typeof right === 'string' && refIdPattern.test(right); + var leftHasRef = typeof left === 'string' && refIdPattern.test(left); + var rightHasRef = typeof right === 'string' && refIdPattern.test(right); - if (leftIsRef || rightIsRef) { + if (leftHasRef || rightHasRef) { // Reference detected - check if composites are allowed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
libs/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__/runtime-attack-vectors.spec.tslibs/enclave-vm/src/__tests__/secure-proxy.spec.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/secure-proxy.tslibs/enclave-vm/src/types.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/__tests__/enclave.constructor-attack.spec.tslibs/enclave-vm/src/__tests__/function-gadget-attacks.spec.tslibs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/__tests__/secure-proxy.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/secure-proxy.tslibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.tslibs/enclave-vm/src/__tests__/double-vm.security.spec.tslibs/enclave-vm/src/__tests__/enclave.serialization.spec.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
🔇 Additional comments (38)
libs/enclave-vm/src/__tests__/enclave.serialization.spec.ts (3)
21-44: LGTM - Test correctly updated for new security model.The test appropriately verifies that non-blocked properties (like
data) remain accessible while confirming the host environment is not polluted. The explicit assertions onresult.successandresult.valueimprove test clarity.
71-92: LGTM - Consistent with updated security model.The test correctly verifies that nested non-blocked properties remain accessible. The approach is consistent with the other updated tests in this file.
119-146: Verify documentation and error message implementation for breaking behavior change.The test validates throwing an error when accessing blocked
constructorproperties, which changes from previous silent filtering. Before proceeding:
- Confirm the
'Security violation'error message is actually implemented in the proxy handler (could not be located in codebase)- Ensure a changelog or migration guide documents this breaking change, particularly since changes to
libs/**public APIs require a matchingdocs/draft/docs/**update per coding guidelines- Verify the error message will appear in developer-facing documentation
The test logic itself is sound, but the implementation claims require verification.
libs/enclave-vm/src/__tests__/double-vm.security.spec.ts (5)
1006-1018: LGTM: Test correctly validates throwing behavior on constructor access.The test has been properly updated to expect a thrown security violation error when accessing constructor via computed properties on tool results, replacing the previous successful-but-blocked behavior. The updated comment accurately describes the secure proxy's new throwing semantics.
1041-1042: LGTM: Consistent error handling for proto access.Test expectations correctly updated to match the new security violation throwing behavior.
1064-1065: LGTM: Promise constructor blocking correctly enforced.Test properly validates that accessing Promise constructor via computed properties now throws a security violation.
1080-1092: LGTM: Test correctly reflects immediate throw on first violation.The test and comments properly document that accessing blocked properties throws immediately, preventing subsequent attack steps. The updated code correctly expects the first computed property access to fail with a security violation.
1136-1137: LGTM: Chained escape prevention validated.Test correctly expects security violation when attempting to access constructor through chained tool call results.
libs/enclave-vm/src/__tests__/enclave.nodejs24-security.spec.ts (3)
128-129: LGTM: Runtime proxy protection correctly enforced.Test properly validates that constructor access via computed properties on wrapped globals now throws a security violation.
193-201: LGTM: PERMISSIVE mode behavior correctly documented.The updated comment clearly explains that PERMISSIVE mode has
throwOnBlocked: false, so it returns undefined instead of throwing. This is an important distinction from other security levels and is properly tested.
301-302: LGTM: Combined attack vectors correctly blocked.Tests properly validate that multiple obfuscation techniques and prototype pollution attempts all fail with security violations.
Also applies to: 312-316
libs/enclave-vm/src/__tests__/function-gadget-attacks.spec.ts (3)
748-756: LGTM: String constructor access correctly blocked.Test properly validates that the String global is wrapped by SecureProxy and throws when constructor is accessed via computed properties. The comment clearly explains the expected behavior.
1255-1268: LGTM: Object global properly distinguished from sandbox objects.The updated test clearly validates that the Object global (wrapped by SecureProxy) throws a security violation when constructor is accessed, improving clarity over the previous undefined-check approach.
1270-1284: LGTM: Important distinction between wrapped globals and sandbox objects.This new test correctly validates a critical security model distinction:
- Wrapped globals (Object, Array, etc.) block constructor access to prevent host escape
- Sandbox-created objects ({}) allow constructor access, which leads to the sandbox's isolated Function constructor
This is the correct defense-in-depth approach.
libs/enclave-vm/src/secure-proxy.ts (5)
336-339: LGTM: Cache key correctly includes throwOnBlocked.The cache key generation correctly includes
throwOnBlockedstate to ensure different proxies are created for different throwing behaviors. The optimization to only include whentrueis reasonable.
444-445: LGTM: Correct precedence for throwOnBlocked resolution.The option resolution correctly prioritizes explicit
options.throwOnBlockedoverlevelConfig, with a safe default offalse.
492-497: LGTM: get handler correctly implements throwing behavior.The implementation properly:
- Respects JavaScript proxy invariants for non-configurable properties
- Calls
onBlockedcallback before throwing- Throws clear, descriptive error messages
- Falls back to
undefinedwhenthrowOnBlockedis falseAlso applies to: 512-517
555-560: LGTM: set handler consistently implements throwing behavior.The set handler correctly throws when attempting to set blocked properties, maintaining consistency with the get handler implementation.
575-580: LGTM: Comprehensive throwing behavior across all relevant proxy traps.Both
definePropertyandgetOwnPropertyDescriptorhandlers correctly implement the throwing behavior, ensuring blocked properties are protected across all access patterns.Also applies to: 618-628
libs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts (2)
39-40: LGTM: Comprehensive and systematic test updates for throwing behavior.All constructor obfuscation attack tests have been correctly updated to expect security violation errors instead of silent blocking. The updates are consistent across various obfuscation techniques (string concatenation, character codes, escapes, type coercion, etc.) and correctly validate that the SecureProxy now throws on blocked property access.
The test coverage remains comprehensive, testing all documented attack vectors against wrapped globals.
Also applies to: 53-54, 65-66, 78-79, 90-91, 110-111, 125-126, 138-139, 177-178, 194-195, 247-248, 281-282, 299-300, 312-313, 324-325, 339-340, 352-353, 386-387, 421-422, 439-440
527-545: LGTM: PERMISSIVE mode behavior correctly validated.The PERMISSIVE mode tests correctly demonstrate the nuanced behavior:
blockConstructor: false- constructor is accessibleblockPrototype: true-__proto__is still blockedthrowOnBlocked: false- returnsundefinedinstead of throwingThis validates that PERMISSIVE mode provides a less restrictive but still somewhat protected environment.
libs/enclave-vm/src/__tests__/secure-proxy.spec.ts (1)
443-486: LGTM!The test configurations are correctly updated to include the new
throwOnBlocked: trueflag, aligning with the updatedSecureProxyLevelConfiginterface. The tests properly exercise thebuildBlockedPropertiesFromConfigfunction with complete configuration objects.libs/enclave-vm/src/__tests__/enclave.constructor-attack.spec.ts (2)
27-31: LGTM!The test expectations are correctly updated to verify that constructor access via computed properties now throws a security violation error instead of returning
undefined. This aligns with the newthrowOnBlocked: truedefault behavior.
134-137: LGTM!The
__proto__access test correctly expects the security violation error message to contain both "Security violation" and "proto", ensuring proper error identification.libs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.ts (2)
68-71: LGTM!Test expectations are correctly updated to verify that computed property attacks now result in explicit security violation errors.
1181-1184: LGTM!Tool result attack tests correctly expect security violations when attempting to access
constructoron proxied tool results.libs/enclave-vm/src/types.ts (5)
80-92: LGTM!The
throwOnBlockedfield is well-documented with clear explanation of its purpose, default value, and security implications. The documentation correctly notes that setting it tofalseis for backward compatibility.
197-217: LGTM!STRICT security level appropriately sets
throwOnBlocked: trueto make security violations explicit and detectable.
314-334: LGTM!PERMISSIVE level correctly sets
throwOnBlocked: falsefor development/debugging purposes, consistent with its documented use case. The comment accurately explains this is for returningundefinedinstead of throwing.
406-416: LGTM!The enhanced documentation for
memoryUsageclearly explains that this is an estimate based on allocation tracking (String and Array), not actual V8 heap usage, and appropriately recommendsworker_threadsfor precise isolation.
449-459: LGTM!The
memoryLimitdocumentation correctly explains the enforcement mechanism (allocation tracking with ~5-10% overhead) and sets appropriate expectations about precision.libs/enclave-vm/src/__tests__/enclave.security.spec.ts (2)
881-895: LGTM!Object introspection attack tests are correctly updated to expect security violation errors. The added comments clearly explain that SecureProxy blocks constructor access at runtime by throwing an error.
1308-1327: LGTM!Sandbox escape prevention tests correctly verify that
__proto__access via string concatenation throws a security violation error, both with Double VM enabled and disabled. This provides good coverage for the defense-in-depth strategy.libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (5)
47-53: LGTM!New options
memoryLimitandthrowOnBlockedare clearly documented and appropriately optional with sensible defaults (0 for unlimited memory, true for throwing on blocked properties).
197-205: LGTM!The constants are correctly injected into the generated bootstrap code, making them available for the proxy traps and concatenation logic.
249-256: LGTM!The
gettrap correctly implements the throw-on-blocked pattern with a descriptive error message that explains why the property is blocked.
605-615: LGTM on the default path logic.The fallback to JavaScript's native
+operator correctly handles all edge cases (objects withToPrimitive, booleans, null, undefined) and appropriately tracks memory only for string results.
539-575: Remove the suggested defensive check—__host_memory_track__is always injected.The code does not need the
typeof __host_memory_track__ === 'function'guard. Thedouble-vm-wrapper.tsinitialization (lines 230 and 239) always injects__host_memory_track__into the context: either as an actual tracking function or as a no-op function when memory tracking is disabled. Since it's guaranteed to exist, the current guardif (memoryLimit > 0)is sufficient and safe.Likely an incorrect or invalid review comment.
…curity violation handling and document breaking changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.