fix: enhance security by implementing safe error handling #29
Conversation
…ing prototype chain escape attacks
…and host with configurable payload limits
…and host with configurable payload limits
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoves many in-repo docs and README content; adds centralized safe-error utilities, a versioned string/direct tool-bridge with payload limits/sidecar support, return-value serialization-size checks, Array.fill memory guards, AST opt-in for dynamic fills, and expanded enclave-vm tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Sandbox as Sandbox VM
participant HostBridge as Host Tool Bridge
participant ToolHandler as Tool Handler
participant Sidecar as Sidecar (optional)
Sandbox->>HostBridge: __host_callToolBridge__(JSON request)
HostBridge->>HostBridge: Validate size & protocol version
HostBridge->>HostBridge: Parse & validate tool call
HostBridge->>ToolHandler: Execute tool handler
alt Tool execution succeeds
ToolHandler-->>HostBridge: Tool result
HostBridge->>HostBridge: Sanitize & check size
alt Result too large
HostBridge->>Sidecar: Store large payload → ref
HostBridge-->>Sandbox: JSON response with reference ID
else Result fits
HostBridge-->>Sandbox: JSON response with result
end
else Tool execution fails
ToolHandler-->>HostBridge: Error thrown
HostBridge->>HostBridge: Package error payload (truncated if needed)
HostBridge-->>Sandbox: JSON error response
end
sequenceDiagram
participant Inner as Inner VM / Sandbox
participant Adapter as VM Adapter
participant SerCheck as Serialization Checker
participant Host as Host
Inner->>Adapter: execute() -> value
Adapter->>SerCheck: checkSerializedSize(value, maxBytes)
alt within limit
SerCheck-->>Adapter: { ok: true, estimatedBytes }
Adapter->>Host: return value
else exceeds limit
SerCheck-->>Adapter: { ok: false, error }
Adapter->>Host: return MemoryLimitError-like result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 3
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/__tests__/constructor-obfuscation-attacks.spec.ts (1)
611-703: Make env restoration +enclave.dispose()robust to unexpected throws / assertion failures.Right now
process.envis restored infinally, butawait enclave.run(code)andenclave.dispose()sit outside that guard. Ifenclave.runthrows unexpectedly or an assertion throws, you can leak env state and/or not dispose the enclave. Wrap both in the sametry/finally.Proposed fix
const envKey = 'ENCLAVE_TEST_SECRET_35'; const secretValue = 'FAKE_SECRET_FOR_SECURITY_CHALLENGE'; const prevEnv = process.env[envKey]; process.env[envKey] = secretValue; const code = ` @@ `; - const result = await enclave.run(code); - - try { + let result; + try { + result = await enclave.run(code); // The attack should be blocked - it should NOT return the secret from host process.env // If the attack succeeds, it returns the env var string. // If blocked, it returns a "blocked:" message, undefined, or throws an error. if (result.success) { @@ } else { // Execution failed with error - this is also acceptable as long as it's not the secret expect(result.error?.message).not.toContain(secretValue); } } finally { // Restore env for other tests if (prevEnv === undefined) { delete process.env[envKey]; } else { process.env[envKey] = prevEnv; } + enclave.dispose(); } - - enclave.dispose();Also applies to: 627-631, 677-699
🤖 Fix all issues with AI agents
In @docs/docs.json:
- Around line 81-83: Update the three navigation path entries so they point to
the actual documentation locations: replace "docs/libraries/enclave" with
"docs/docs/libraries/enclave", "docs/libraries/ast-guard" with
"docs/docs/libraries/ast-guard", and "docs/libraries/vectoriadb" with
"docs/docs/libraries/vectoriadb" in the docs.json navigation array so they match
the real files (enclave.mdx, ast-guard.mdx, vectoriadb.mdx).
In @README.md:
- Line 16: README contains inconsistent documentation URLs: replace the
alternate doc link (https://agentfront.dev/docs/guides/enclave) with the
canonical URL (https://enclave.agentfront.dev) so all documentation links in
README point to the same canonical URL, and verify docs.json/any other config
uses https://enclave.agentfront.dev as the canonical docs URL.
- Line 3: The README references a missing image asset via the <img> tag src
"assets/logo.dark.svg" which causes a broken image; either add the missing file
named "logo.dark.svg" under the assets directory (so the existing <img> tag in
README.md resolves) or update the <img src="assets/logo.dark.svg"> entry in
README.md to point to an existing asset file/path or replace it with an
available logo filename.
🧹 Nitpick comments (3)
libs/enclave-vm/src/__tests__/safe-runtime-reference.spec.ts (1)
15-45: Keep testtoolBridgedefaults aligned with runtime normalization (avoid drift).The added
toolBridgeblock is sensible, but since it’s essentially “defaults”, consider referencing a shared default (if available) or add a short comment that this must track Enclave’s default normalization to prevent future divergence.Also applies to: 29-33
libs/enclave-vm/src/__tests__/tool-bridge.spec.ts (1)
25-56: Payload size enforcement tests are well-structured.Both request and response payload size tests verify the expected error messages. Consider also asserting on
result.error?.codefor more precise validation (e.g.,TOOL_BRIDGE_RESPONSE_TOO_LARGE/TOOL_BRIDGE_REQUEST_TOO_LARGE).💡 Optional: Add error code assertions for precision
expect(result.success).toBe(false); expect(result.error?.message).toMatch(/Tool response exceeds maximum size/i); + expect(result.error?.code).toBe('TOOL_BRIDGE_RESPONSE_TOO_LARGE');expect(result.success).toBe(false); expect(result.error?.message).toMatch(/Tool request exceeds maximum size/i); + expect(result.error?.code).toBe('TOOL_BRIDGE_REQUEST_TOO_LARGE');libs/enclave-vm/src/value-sanitizer.ts (1)
326-349: UTF-8 estimation doesn't handle 4-byte sequences (surrogate pairs).The UTF-8 byte estimation handles 1-3 byte sequences but misses 4-byte sequences for code points >= U+10000 (encoded as surrogate pairs in JavaScript strings). While rare in typical data, this could underestimate sizes for strings containing emoji or other supplementary characters.
🔧 Optional fix for surrogate pair handling
} else if (code < 2048) { bytes += 2; // 2-byte UTF-8 + } else if (code >= 0xd800 && code <= 0xdbff) { + // High surrogate - next char is low surrogate, together they form a 4-byte UTF-8 sequence + bytes += 4; + i++; // Skip the low surrogate } else { bytes += 3; // 3-byte UTF-8 }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
assets/logo.dark.svgis excluded by!**/*.svg,!**/*.svgassets/logo.light.svgis excluded by!**/*.svg,!**/*.svgdocs/draft/assets/banners/codecall-plugin/code-call-example.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/code-call-options.dark.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/code-call-options.light.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/code-call-plugin.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/codecall-overview-dark.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/codecall-overview-light.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/early-day-vs-today.dark.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/early-day-vs-today.light.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/security-layers.dark.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/security-layers.light.pngis excluded by!**/*.pngdocs/draft/assets/logo/banner.dark.pngis excluded by!**/*.pngdocs/draft/assets/logo/banner.light.pngis excluded by!**/*.pngdocs/draft/assets/logo/favicon-32x32.pngis excluded by!**/*.pngdocs/draft/assets/logo/favicon.dark.icois excluded by!**/*.icodocs/draft/assets/logo/favicon.svgis excluded by!**/*.svg,!**/*.svgdocs/draft/assets/logo/frontmcp.dark.pngis excluded by!**/*.pngdocs/draft/assets/logo/frontmcp.dark.svgis excluded by!**/*.svg,!**/*.svgdocs/draft/assets/logo/frontmcp.light.pngis excluded by!**/*.pngdocs/draft/assets/logo/frontmcp.light.svgis excluded by!**/*.svg,!**/*.svgdocs/draft/assets/logo/logo.dark.pngis excluded by!**/*.pngdocs/draft/assets/logo/logo.dark.svgis excluded by!**/*.svg,!**/*.svgdocs/draft/assets/logo/logo.light.pngis excluded by!**/*.pngdocs/draft/assets/logo/logo.light.svgis excluded by!**/*.svg,!**/*.svgdocs/favicon.icois excluded by!**/*.icodocs/live/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (35)
README.mddocs/docs.jsondocs/docs/getting-started/installation.mdxdocs/docs/getting-started/welcome.mdxdocs/docs/libraries/ast-guard.mdxdocs/docs/libraries/enclave.mdxdocs/docs/libraries/vectoriadb.mdxdocs/draft/docs.jsondocs/draft/docs/getting-started/installation.mdxdocs/draft/docs/getting-started/welcome.mdxdocs/draft/docs/libraries/ast-guard.mdxdocs/draft/docs/libraries/enclave.mdxdocs/draft/updates.mdxdocs/live/docs/libraries/vectoriadb.mdxdocs/live/snippets/card.jsxdocs/snippets/card.jsxdocs/style.cssdocs/updates.mdxlibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/rules/resource-exhaustion.rule.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/tool-bridge.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.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/safe-error.tslibs/enclave-vm/src/safe-runtime.tslibs/enclave-vm/src/secure-proxy.tslibs/enclave-vm/src/tool-bridge.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/value-sanitizer.ts
💤 Files with no reviewable changes (8)
- docs/draft/docs/getting-started/installation.mdx
- docs/draft/docs.json
- docs/draft/docs/getting-started/welcome.mdx
- docs/live/docs/libraries/vectoriadb.mdx
- docs/draft/docs/libraries/ast-guard.mdx
- docs/draft/docs/libraries/enclave.mdx
- docs/draft/updates.mdx
- docs/live/snippets/card.jsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use ESLint flat config with unused parameters and variables prefixed with underscore_
Prefer workspace import paths (e.g.,import { validate } from 'ast-guard') over deep cross-library relative imports
Files:
libs/ast-guard/src/rules/resource-exhaustion.rule.tslibs/enclave-vm/src/safe-error.tslibs/enclave-vm/src/tool-bridge.tslibs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/value-sanitizer.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/safe-runtime.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/secure-proxy.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
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/rules/resource-exhaustion.rule.tslibs/enclave-vm/src/safe-error.tslibs/enclave-vm/src/tool-bridge.tslibs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/value-sanitizer.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/safe-runtime.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/secure-proxy.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
docs/docs.json
⚙️ CodeRabbit configuration file
docs/docs.json: Mintlify navigation configuration. Important structure: - dropdowns[].dropdown == "Documentation":
- Contains a "versions" array.
- Latest version entry: "version": "vX.Y (latest)", "default": true, with
paths like "docs/getting-started/welcome" and "updates" (NO "v/X.Y" prefix).- Older versions: "version": "v0.2", "version": "v0.1", etc., with paths
like "docs/v/0.2/getting-started/welcome".- dropdowns[].dropdown == "Blog":
- Contains blog groups (e.g. "blog/november-2025/introducing-frontmcp").
Review guidelines: - Do NOT suggest editing the Blog dropdown unless the PR explicitly adds/changes blogs. - Ensure latest version uses non-versioned "docs/..." paths, and older versions
use "docs/v/{minor}/..." paths.- If you see changes that try to route archived versions away from docs/v/**,
flag that as incorrect.
Files:
docs/docs.json
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/docs.json
**/src/__tests__/**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under
src/__tests__/**/*.spec.tswith performance tests underlibs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts
Files:
libs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
🧠 Learnings (4)
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Enforce global coverage threshold for `enclave-vm` as configured in `libs/enclave-vm/jest.config.ts`
Applied to files:
README.mdlibs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Applies to libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts : Performance tests for enclave-vm should be placed under `libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts`
Applied to files:
README.mdlibs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Applies to **/src/__tests__/**/*.spec.ts : Place test files under `src/__tests__/**/*.spec.ts` with performance tests under `libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts`
Applied to files:
libs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/__tests__/resource-exhaustion.spec.tslibs/enclave-vm/src/__tests__/safe-runtime-reference.spec.tslibs/enclave-vm/src/__tests__/enclave.spec.tslibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.ts
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: PRs must include problem/solution description, test evidence (`yarn test` or `nx test <project>`), and security-impact notes for sandbox/VM changes
Applied to files:
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts
🧬 Code graph analysis (5)
libs/enclave-vm/src/tool-bridge.ts (2)
libs/enclave-vm/src/types.ts (1)
ExecutionContext(636-682)libs/enclave-vm/src/value-sanitizer.ts (1)
sanitizeValue(89-246)
libs/enclave-vm/src/__tests__/tool-bridge.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(163-756)
libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(163-756)
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (2)
libs/enclave-vm/src/value-sanitizer.ts (1)
checkSerializedSize(476-486)libs/enclave-vm/src/tool-bridge.ts (1)
createHostToolBridge(146-427)
libs/enclave-vm/src/secure-proxy.ts (1)
libs/enclave-vm/src/safe-error.ts (1)
createSafeError(18-75)
🪛 markdownlint-cli2 (0.18.1)
README.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (46)
docs/docs.json (5)
4-4: LGTM! Clear security messaging.The updated description effectively communicates the security focus of the project and aligns well with the PR's security hardening objectives.
6-9: Theme updates look good.The appearance and theme changes provide a consistent dark aesthetic for the documentation site.
48-53: Footer social links enhance discoverability.Good addition for community engagement.
33-36: No issues found. The Playground URL athttps://enclave.agentfront.devis live and properly accessible (HTTP 200), confirming the navigation link is correctly configured.
23-30: Metadata is correctly configured. The og:image URL athttps://enclave.agentfront.dev/og-image.pngis accessible and returns a valid PNG image, and the Twitter handle@agentfrontis correct.README.md (2)
22-35: Clear installation instructions and compelling benefits.The Install and Why Enclave sections effectively communicate the value proposition. The security claims (1000+ tests, 100% CVE coverage) are strong selling points.
Consider verifying that the security claims can be substantiated with test coverage reports or security audit documentation.
7-7: Static analysis hint can be ignored.The markdownlint tool flagged this line for using emphasis instead of a heading. However, this bold text is intentionally part of the centered header design, not a standalone heading. The current structure is appropriate for the visual layout.
libs/enclave-vm/src/__tests__/enclave.spec.ts (1)
429-447: LGTM: assertion now matches bridged tool error message.Also applies to: 443-445
libs/ast-guard/src/rules/resource-exhaustion.rule.ts (1)
7-31: The allowDynamicArrayFill option is correctly gated to runtime memory enforcement.The code properly couples the option to runtime protection: in
libs/enclave-vm/src/enclave.tsline 683, the agentscript preset enables it only whenthis.config.memoryLimit > 0. Since the defaultmemoryLimitis 1MB, runtime Array.prototype.fill patching is active by default, preventing memory exhaustion attacks. The static rule defaults to blocking dynamic fill (safe), but allows it when memoryLimit is configured, achieving the desired defense-in-depth.libs/enclave-vm/src/types.ts (1)
445-477: Enforcement is correctly implemented:mode: 'direct'is properly gated byacknowledgeInsecureDirect === trueat the Enclave constructor.The normalization and validation at Enclave constructor (lines 202-217 in enclave.ts) throws immediately if
mode='direct'without explicit acknowledgement. The enforced config then flows to all downstream consumers (double-vm-wrapper, vm-adapter) which receive the pre-validated configuration. Tests confirm the gate works correctly.libs/ast-guard/src/presets/agentscript.preset.ts (2)
220-233: Well-documented security option with safe default.The new
allowDynamicArrayFilloption is properly documented with clear guidance on when to enable it (only whenmemoryLimitis configured). The default offalseensures a secure-by-default posture for environments without runtime protection.
568-569: Correctly wired to ResourceExhaustionRule.The option is properly passed through with a safe default via nullish coalescing (
?? false).libs/enclave-vm/src/secure-proxy.ts (3)
19-19: Good: Centralized safe error import.Importing
createSafeErrorfrom the dedicated module ensures consistent, secure error handling across the codebase.
220-220: Security hardening: Safe error for Reflect.construct blocking.Using
createSafeErrorwith'SecurityError'name prevents prototype chain escape attacks that could occur if a regularErrorinstance crossed the sandbox boundary.
503-507: Consistent security error handling across all proxy traps.All blocked property access paths (get, set, defineProperty, getOwnPropertyDescriptor) now use
createSafeErrorwith'SecurityError'name. This ensures that even if an attacker triggers these security violations, the thrown error cannot be used as an escape vector.Also applies to: 524-528, 568-572, 589-593, 638-642
libs/enclave-vm/src/__tests__/tool-bridge.spec.ts (2)
1-6: Good test coverage for tool bridge security requirements.The test correctly verifies that
toolBridge.mode='direct'requires explicit acknowledgement, enforcing the security-by-default principle.
58-73: Defense-in-depth test for single-VM mode.Good verification that the host bridge global is not exposed when
doubleVmis disabled, preventing potential attack vectors.libs/enclave-vm/src/enclave.ts (3)
55-58: Reasonable default payload limit.5MB is a sensible default that balances usability with protection against oversized payloads.
200-219: Robust tool bridge configuration validation.The validation logic properly:
- Validates mode is one of the allowed values
- Ensures maxPayloadBytes is a positive finite number (handles
NaN,Infinity, negatives)- Enforces explicit acknowledgement for insecure direct mode
The security gate on direct mode is critical for preventing accidental exposure of the insecure path.
677-683: Correct integration of allowDynamicArrayFill with memoryLimit.Enabling
allowDynamicArrayFillonly whenmemoryLimit > 0ensures runtime protection is in place before allowing dynamic array sizes. This aligns with the documentation in the preset options.libs/enclave-vm/src/safe-error.ts (3)
1-12: Excellent threat model documentation.The header clearly explains the security rationale: preventing sandbox escape via prototype chain walks from host errors that cross the boundary. This documentation helps future maintainers understand why this module exists.
18-75: Robust safe error implementation.The implementation correctly addresses all escape vectors:
- Line 48:
Object.setPrototypeOf(error, null)- Critical for severing the actual[[Prototype]]chain- Lines 23-44: Null-prototype
SafeConstructorbreakserr.constructor.constructorchains- Lines 51-56, 58-63: Shadow properties for defense-in-depth
- Lines 66-71: Stack trace hidden to prevent host path leakage
- Line 73:
Object.freeze(error)prevents any modificationThis is a well-designed security primitive.
77-79: Convenient helper for TypeError.Simple delegation to
createSafeErrormaintains consistency.libs/enclave-vm/src/__tests__/resource-exhaustion.spec.ts (2)
272-320: Comprehensive dynamic Array.fill test coverage.The tests correctly verify:
- Dynamic sizes allowed when memoryLimit enables runtime protection
- Large allocations blocked at runtime
- Computed sizes within limits work correctly
Good coverage of the
allowDynamicArrayFillfeature interaction withmemoryLimit.
1658-1753: Thorough ATK-RSRC-340 rope string exhaustion tests.These tests effectively cover the serialization bomb attack vector where in-memory structures with shared string references explode during JSON serialization. Key test scenarios:
- Lines 1666-1697: Reference multiplication attack (500 refs × 5 copies)
- Lines 1699-1715: Baseline - reasonable repetition allowed
- Lines 1717-1733: Nested repetition attack
- Lines 1735-1752: Object-based variant
The assertions on
error.code === 'SERIALIZATION_LIMIT_EXCEEDED'provide precise validation.libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (4)
13-18: Clean import organization for new dependencies.Imports for
checkSerializedSize,createSafeError, andcreateHostToolBridgeare properly grouped with related functionality.
141-163: Critical security fix for Vector 340 (rope string exhaustion).This check prevents attacks where structures with many references to the same string appear small in memory but explode during serialization. The implementation:
- Line 149: Uses
Math.min(config.memoryLimit, 50MB)as a reasonable cap- Line 150: Leverages
checkSerializedSizefor estimation- Lines 153-162: Returns structured error with
SERIALIZATION_LIMIT_EXCEEDEDcodeThis is a well-placed defense that catches serialization bombs before they can cause resource exhaustion.
275-284: Defense-in-depth: Configurable host properties allow cleanup.Changing host-injected properties to
configurable: trueenables the parent VM bootstrap to delete these references after capturing them. This prevents access if child VM code escapes to the parent context.The inline comments clearly document this security rationale.
Also applies to: 290-291, 297-298, 304-305, 316-317
504-521: Tool bridge configuration propagated to bootstrap.The
toolBridgeModeandtoolBridgeMaxPayloadBytesare correctly extracted from config with appropriate defaults and passed to the bootstrap generator.libs/enclave-vm/src/safe-runtime.ts (2)
12-12: LGTM: Centralized safe error imports.Good refactor to centralize error creation utilities. This improves consistency and maintainability across the codebase.
478-486: Robust error message extraction.The defensive approach handles non-Error objects well. The fallback chain (checking for
.messagestring →Object.prototype.toString.call()→String()) covers edge cases where errors might be plain objects, null prototypes, or primitives.libs/enclave-vm/src/value-sanitizer.ts (2)
476-486: LGTM: Clean wrapper for size checking.The
checkSerializedSizefunction provides a clean API that catches exceptions and returns a discriminated union result. This is a good pattern for callers who want to check without handling exceptions.
363-366: Remove concern about misleading estimates—the function correctly assumes BigInt becomes a string.While
JSON.stringifydoes throw on BigInt,estimateSerializedSizedoesn't claim to estimate raw JSON serialization. It correctly predicts whatsafeJsonStringify(in tool-bridge.ts) will produce, which converts BigInt to string via thejsonReplacerreplacer function. The estimate ofString(value).length + 2for a quoted string is accurate for this code path.Note:
sanitizeValuepasses BigInt through unchanged as a primitive; the actual conversion to string happens in thejsonReplacerat serialization time, not upstream.Likely an incorrect or invalid review comment.
libs/enclave-vm/src/adapters/vm-adapter.ts (3)
832-859: Good addition: Array.fill memory protection.The patch correctly implements the ECMA-262 fill range calculation algorithm and tracks memory before allocation. This closes Vector 1110/1170 where
Array(largeSize).fill()could exhaust memory by converting sparse arrays to dense.The 8 bytes per element estimate is reasonable for object pointers in V8's tagged pointer representation.
900-1019: Comprehensive tool bridge implementation.The string-mode tool bridge provides good security boundaries:
- Captures and deletes host bridge reference (defense-in-depth)
- Conservative UTF-8 byte estimation (
length * 4)- Protocol version validation
- Input sanitization via JSON round-trip
- Proper error propagation with safe error types
One minor observation: the
estimateBytesfunction useslength * 4which is very conservative (UTF-8 max is 4 bytes per code point, but JavaScript strings are UTF-16 where most characters are 2 bytes max in UTF-8).
1136-1158: LGTM: Serialization size check prevents Vector 340.Good security fix. The check before returning prevents memory exhaustion from structures that appear small in-memory (due to string reference sharing) but explode during JSON serialization.
The cap at 50MB (
Math.min(config.memoryLimit, 50 * 1024 * 1024)) provides a sensible upper bound even for generous memory limits.libs/enclave-vm/src/tool-bridge.ts (4)
1-46: Well-structured utility functions.Good defensive utilities:
utf8ByteLengthuses Node's Buffer for accurate byte countsjsonReplacerhandles BigInt and non-finite numbers that would otherwise throw or produce invalid JSONsafeJsonStringifywraps with the replacertruncateForErrorMessageprevents oversized error messagesClean separation of concerns.
196-215: Potential off-by-one in tool call limit check.The stats increment happens at line 199 before the limit check at line 202. However, when
updateStatsis true, this incrementsstats.toolCallCountand then checks if it exceedsconfig.maxToolCalls.If
maxToolCallsis 10 and this is the 11th call,toolCallCountbecomes 11, and11 > 10correctly fails. This looks correct.However, note that in
safe-runtime.tsline 84-87, the increment happens first and then checks>(not>=). This means the check allows exactlymaxToolCallscalls, which is the expected behavior.
246-263: Good defensive sanitization of tool arguments.Sanitizing tool args with
allowDates: falseandallowErrors: falseis appropriate for the bridge since these types don't round-trip through JSON cleanly. The subsequent type check ensures the result is a plain object.
361-372: LGTM: Sidecar lift for large string results.Good optimization to store large string results in sidecar and return a reference ID. The silent fallback on storage failure is appropriate since the original value is still valid.
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (6)
33-43: Good API addition for tool bridge configuration.The new
toolBridgeModeandtoolBridgeMaxPayloadBytesoptions provide necessary configurability for the tool bridge feature. Default values (5MB payload, 'string' mode) are reasonable.
454-463: Correct fix: Use defineProperty for error.name.In strict mode, directly assigning to
error.namecan fail ifError.prototype.nameis frozen or non-writable. UsingObject.definePropertywith a try-catch is the correct defensive approach.
664-671: Good defense-in-depth: Delete host references after capture.Deleting
__host_*globals after capturing them prevents inner VM escape attempts from discovering these references. The try-catch handles cases where deletion might fail.
710-721: Critical security improvement: Proper prototype chain severance.Using
Object.setPrototypeOf(error, null)properly severs the prototype chain at the native level, not just shadowing__proto__. This is more robust than the previous approach and blocksObject.getPrototypeOf(error).constructor.constructorattacks.
1079-1135: Comprehensive tool bridge mode handling.The string mode implementation:
- Validates JSON serialization
- Checks payload size with conservative estimate (
length * 4)- Parses and validates response structure
- Properly handles success and error cases
- Uses createSafeError for all error paths
The fallback to direct mode (
hostCallTool(toolName, sanitizedArgs)) maintains backward compatibility.
1506-1524: Consistent Array.fill patch with vm-adapter.The Array.fill memory patch mirrors the implementation in
vm-adapter.ts, which is good for consistency. Both implementations:
- Calculate fill range per ECMA-262 spec
- Estimate 8 bytes per element
- Check against memory limit
- Track cumulative memory before allocation
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @libs/enclave-vm/src/value-sanitizer.ts:
- Around line 426-433: The RegExp branch in value size estimation is wrong:
instead of returning String(value).length + 2 it should mirror JSON.stringify
behavior (RegExp -> "{}"), so call currentPath.delete(value as object) and
return 2; update the RegExp handling in the same block as the Date check (the
instance check for RegExp) to delete the object from currentPath and return 2.
- Around line 372-375: The BigInt branch in sanitizeValue incorrectly returns a
quoted-string length instead of matching JSON.stringify behavior; update the
bigint handling in sanitizeValue (value-sanitizer.ts) to fail-fast by throwing a
TypeError (or a descriptive custom error) when type === 'bigint' so BigInt
values are rejected rather than treated as strings, and ensure any callers of
sanitizeValue catch or propagate that error appropriately.
- Around line 439-468: The code under the object iteration in
estimateSerializedSize incorrectly assumes key length as key.length + 3 and
ignores JSON string escaping; replace that naive calculation by estimating the
serialized size of the key as a JSON string (accounting for quotes and necessary
escape sequences) using the same string estimation logic used for values (or a
small helper like estimateSerializedKey) and add the resulting size (including
the surrounding quotes and colon) instead of key.length + 3; keep the existing
maxBytes check and currentPath cleanup behavior unchanged and reuse the existing
helper/logic so keys with quotes, backslashes or control characters are
correctly sized.
- Around line 324-358: The string-size estimator currently treats all control
chars (code < 32) as 2-byte escapes, undercounting JSON serialized size; update
the string branch that computes bytes for ASCII to treat only quote (code 34)
and backslash (code 92) as 2-byte escapes and treat other control characters
(code < 32) as 6 bytes (the "\uXXXX" form) so bytes += 6 for those codes; keep
the existing logic for multibyte UTF-8 and surrogate pairs, and ensure the check
that throws the Error using maxBytes still uses the corrected bytes value.
In @README.md:
- Line 7: The document uses bold markup "**Secure sandbox runtime for AI
agents**" instead of a proper Markdown heading; replace that bolded text with
the appropriate heading syntax (e.g., prepend one or more '#' characters such as
"## Secure sandbox runtime for AI agents") so it follows markdownlint MD036 and
proper heading hierarchy, ensuring the exact phrase "Secure sandbox runtime for
AI agents" becomes a heading rather than bold inline text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.mddocs/getting-started/installation.mdxdocs/getting-started/welcome.mdxdocs/libraries/ast-guard.mdxdocs/libraries/enclave.mdxdocs/libraries/vectoriadb.mdxlibs/enclave-vm/src/__tests__/constructor-obfuscation-attacks.spec.tslibs/enclave-vm/src/__tests__/tool-bridge.spec.tslibs/enclave-vm/src/value-sanitizer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/enclave-vm/src/tests/constructor-obfuscation-attacks.spec.ts
- libs/enclave-vm/src/tests/tool-bridge.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use ESLint flat config with unused parameters and variables prefixed with underscore_
Prefer workspace import paths (e.g.,import { validate } from 'ast-guard') over deep cross-library relative imports
Files:
libs/enclave-vm/src/value-sanitizer.ts
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/value-sanitizer.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: PRs must include problem/solution description, test evidence (`yarn test` or `nx test <project>`), and security-impact notes for sandbox/VM changes
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Enforce global coverage threshold for `enclave-vm` as configured in `libs/enclave-vm/jest.config.ts`
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Enforce global coverage threshold for `enclave-vm` as configured in `libs/enclave-vm/jest.config.ts`
Applied to files:
README.md
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Applies to libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts : Performance tests for enclave-vm should be placed under `libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts`
Applied to files:
README.md
🪛 markdownlint-cli2 (0.18.1)
README.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (8)
README.md (1)
1-50: Verify that PR description includes security-impact documentation and test coverage evidence.The README has been simplified appropriately for a quick-start guide. However, this PR implements significant security enhancements (centralized safe error handling, tool-bridge versioning, memory guards, serialization checks) that warrant security-impact notes in the PR description.
Based on learnings from prior reviews, sandbox/VM security changes must include:
- Problem statement and solution description
- Test evidence (e.g.,
yarn testoutput forenclave-vm)- Security-impact notes for the sandbox changes
Ensure these are documented in the pull request description on GitHub, as they're essential for reviewers to assess the security implications of the changes.
libs/enclave-vm/src/value-sanitizer.ts (7)
280-304: LGTM - Excellent security documentation.The documentation clearly explains the attack vector (Vector 340) and the distinction between circular and repeated references, which is critical for understanding the function's behavior.
318-320: LGTM - Correct JSON semantics.Properly handles
nullas 4 bytes andundefinedas omitted (0 bytes).
377-380: LGTM - Correct JSON behavior.Functions and symbols are properly handled as omitted (0 bytes), matching JSON.stringify behavior.
383-412: LGTM - Solid circular reference handling.The array handling correctly uses
currentPathto detect circular references and performs incremental size checks with fast-fail behavior. The cleanup pattern (removing fromcurrentPathbefore throwing) prevents memory leaks.
478-495: LGTM - Clean error handling wrapper.The wrapper provides a safe, non-throwing API for size checking. Good pattern for library consumers.
305-476: These are internal utility functions, not public APIs; documentation updates are not required.The
estimateSerializedSizeandcheckSerializedSizefunctions are internal implementation details used within the enclave-vm library (referenced in vm-adapter.ts and double-vm-wrapper.ts) and are not exported from the public API in index.ts. Test coverage is already comprehensive—resource-exhaustion.spec.ts includes multiple test cases covering ATK-RSRC-340 scenarios (rope string exhaustion, nested repetition, object structures). The global coverage thresholds are configured in jest.config.ts (40-45% for functions/lines/statements/branches). No public documentation updates are needed since these are internal utilities, not public API exports.Likely an incorrect or invalid review comment.
313-316: Clarify JSDoc and reconsider severity of depth limit behavior.The depth limit check returns a fixed estimate (20 bytes) rather than throwing, as stated in the JSDoc ("@throws Error if... depth limit"). While this creates potential for underestimation in extremely deep structures (depth > 2000), the actual attack vector (Vector 340) is caught by the
maxBytescheck that runs at each recursion level. The depth limit of 2000 is intentionally lenient to prevent stack overflow while still supporting legitimate deep structures.However, the JSDoc is inaccurate: it claims the function throws on depth limit violations, but it returns an estimate instead. Update the JSDoc to reflect this behavior or reconsider whether throwing would be more appropriate for consistency with
sanitizeValue()(which does throw at its depth limit on line 102).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libs/enclave-vm/src/value-sanitizer.ts:
- Around line 353-356: The current size-estimation logic returns a fixed 20 when
depth > maxDepth, which allows nested attackers to bypass protections; instead,
change the behavior in the size-estimation routine (the function that checks
`depth` against `maxDepth` in libs/enclave-vm/src/value-sanitizer.ts) to
raise/throw an error (or otherwise abort processing) when the depth limit is
exceeded, so deeply nested structures are rejected rather than conservatively
underestimated; update any callers of this function to handle the thrown error
(or propagate it) and ensure tests cover the new behavior.
- Around line 123-126: sanitizeValue currently returns BigInt primitives as-is
which will cause JSON.stringify to throw; update the primitive branch in
sanitizeValue to detect type === 'bigint' and return value.toString() (or
String(value)) instead of the raw BigInt, and adjust the comment to note BigInt
is converted to a string for JSON-safety; also add a brief guard or comment in
checkSerializedSize noting BigInt strings are used so size checks remain
accurate.
In @README.md:
- Around line 32-35: Update the "Why Enclave?" / README copy to either
substantiate the security metrics by linking to the supporting artifacts (e.g.,
SECURITY-AUDIT report, test-suite dashboard, CHANGELOG security section, or a
security whitepaper) or tone down the absolute claims to verifiable language;
specifically replace "1000+ security tests" and "100% CVE coverage" with either
a parenthetical link to the test/audit resources or a milder phrasing such as
"extensive security testing" and "broad CVE mitigations" so the README no longer
makes unreferenced quantitative claims.
🧹 Nitpick comments (1)
README.md (1)
39-46: Quick Start example doesn't demonstrate new security capabilities.The example shows basic arithmetic (
1 + 2) without exercising any of the security features being added in this PR (safe error handling, memory guards, serialization checks, etc.). Consider including a slightly more realistic example that:
- Demonstrates error handling behavior (how errors are safely surfaced), or
- Shows a pattern that would trigger memory guards or serialization checks, enabling users to validate the new protections.
This would help users immediately understand the value of the enhancements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdlibs/enclave-vm/src/value-sanitizer.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use ESLint flat config with unused parameters and variables prefixed with underscore_
Prefer workspace import paths (e.g.,import { validate } from 'ast-guard') over deep cross-library relative imports
Files:
libs/enclave-vm/src/value-sanitizer.ts
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/value-sanitizer.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: PRs must include problem/solution description, test evidence (`yarn test` or `nx test <project>`), and security-impact notes for sandbox/VM changes
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Enforce global coverage threshold for `enclave-vm` as configured in `libs/enclave-vm/jest.config.ts`
Applied to files:
README.md
⏰ 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). (3)
- GitHub Check: Test
- GitHub Check: Performance
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
libs/enclave-vm/src/value-sanitizer.ts (5)
280-318: LGTM! String size estimation is thorough and correct.The function properly accounts for JSON escaping, UTF-8 encoding, and surrogate pairs. The handling of unpaired surrogates is correct (3 bytes each), and the logic for control character escapes (
\uXXXX) is accurate.
399-425: LGTM! Array handling with circular reference detection is correct.The use of
currentPathSet to track ancestors correctly detects circular references without conflating repeated (non-circular) object references. The incremental size checking with fast-fail optimization is efficient, and cleanup before throwing is proper.
429-487: LGTM! Object handling with special type support is well-designed.The function correctly handles:
- Circular reference detection using the ancestor path pattern
- Special objects (Date → 26-byte ISO string, RegExp → "{}" = 2 bytes)
- Key serialization accounting for JSON escaping
- Getter trap protection with try-catch
- Incremental size validation with proper cleanup
493-510: LGTM! Wrapper function provides ergonomic error handling.The discriminated union return type allows callers to easily check success/failure and handle errors gracefully without exception handling.
320-491: Verify test coverage for security-critical size estimation.Given the security-critical nature of these functions (preventing memory exhaustion attacks), ensure comprehensive test coverage exists for:
- Deep nesting beyond
maxDepth- Circular reference detection
- Repeated references to large strings (Vector 340 mentioned in comments)
- Edge cases: surrogate pairs, control characters, special objects
- Fast-fail behavior when size limits exceeded
As per the repository learnings, please confirm test evidence (
yarn testornx test) and security-impact notes are included in the PR description.Based on learnings, security/sandbox changes require test evidence and impact documentation.
…o improve security
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libs/enclave-vm/README.md:
- Line 15: The README row currently reads "Security Tests | 1100+ tests, 100%
pass rate"; update that table cell to reflect the actual test coverage (e.g.,
"Security Tests | 1921 tests, 100% pass rate" or "Security Tests | 1900+ tests,
100% pass rate") to accurately reflect the added safe-error and tool-bridge
tests; edit the string "Security Tests | 1100+ tests, 100% pass rate" in
libs/enclave-vm/README.md to the new value.
In @libs/enclave-vm/src/value-sanitizer.ts:
- Around line 403-433: The array handling in estimateSerializedSize
underestimates sizes because array elements that are undefined, functions, or
symbols are treated via primitive branches as 0 bytes whereas JSON.stringify
coerces them to null (4 bytes); when iterating elements inside the Array.isArray
branch (and before calling estimateSerializedSize recursively), coerce any
element where value[i] === undefined || typeof value[i] === 'function' || typeof
value[i] === 'symbol' to null (or call estimateSerializedSize with null) so
those entries contribute the correct 4 bytes, keep the existing
circular-check/currentPath.add/currentPath.delete logic and the incremental
maxBytes check intact.
In @README.md:
- Line 3: The README image tag references "./assets/logo.dark.svg" which is
missing, causing a broken image; either add the missing SVG file at that path
and commit the assets directory (ensure the filename exactly matches
"logo.dark.svg"), or update the <img> src in README.md to point to an existing
logo asset (or a CDN) and commit that change so the reference resolves.
🧹 Nitpick comments (1)
libs/enclave-vm/src/value-sanitizer.ts (1)
371-490:maxBytesisn’t propagated into recursion; huge strings still fully scanned before failing.
You’re already doing incremental container checks, but passingmaxBytes=0into recursive calls prevents “fail-fast” for obviously-too-large leaves (e.g., a single giant string). PropagatingmaxBytesenables earlier aborts without changing correctness.Proposed refactor (pass maxBytes through)
- const elementSize = estimateSerializedSize(value[i], 0, depth + 1, maxDepth, currentPath); + const elementSize = estimateSerializedSize(value[i], maxBytes, depth + 1, maxDepth, currentPath); ... - const valueSize = estimateSerializedSize(propValue, 0, depth + 1, maxDepth, currentPath); + const valueSize = estimateSerializedSize(propValue, maxBytes, depth + 1, maxDepth, currentPath);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdlibs/enclave-vm/README.mdlibs/enclave-vm/src/value-sanitizer.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/README.mdlibs/enclave-vm/src/value-sanitizer.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use ESLint flat config with unused parameters and variables prefixed with underscore_
Prefer workspace import paths (e.g.,import { validate } from 'ast-guard') over deep cross-library relative imports
Files:
libs/enclave-vm/src/value-sanitizer.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: PRs must include problem/solution description, test evidence (`yarn test` or `nx test <project>`), and security-impact notes for sandbox/VM changes
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: PRs must include problem/solution description, test evidence (`yarn test` or `nx test <project>`), and security-impact notes for sandbox/VM changes
Applied to files:
README.mdlibs/enclave-vm/README.md
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Follow `SECURITY.md` for responsible disclosure when vulnerabilities are found
Applied to files:
README.md
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Enforce global coverage threshold for `enclave-vm` as configured in `libs/enclave-vm/jest.config.ts`
Applied to files:
README.mdlibs/enclave-vm/README.md
📚 Learning: 2026-01-08T18:40:23.667Z
Learnt from: CR
Repo: agentfront/enclave PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T18:40:23.667Z
Learning: Applies to libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts : Performance tests for enclave-vm should be placed under `libs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts`
Applied to files:
README.md
🧬 Code graph analysis (1)
libs/enclave-vm/src/value-sanitizer.ts (1)
libs/enclave-vm/src/index.ts (1)
estimateStringSize(112-112)
⏰ 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: Test
- GitHub Check: Performance
🔇 Additional comments (7)
libs/enclave-vm/README.md (1)
17-17: CVE language change is more accurate.The shift from a categorical claim to "Covers known vm2, isolated-vm, node-vm exploits" is more defensible and aligns with responsible security disclosures. This improves the README without overstating protections.
README.md (3)
42-48: Quick Start code example aligns with Enclave API.The TypeScript example correctly demonstrates:
- Enclave constructor with
securityLevelandtoolHandlertoolHandleras async function with tool name and args- Error handling for unknown tools
The code structure matches the library's documented API.
1-62: PR lacks explicit problem/solution description and security-impact notes.Per learnings, sandbox/VM security changes should include a clear problem statement, solution description, test evidence, and security-impact analysis. This root README now serves as the project entry point but omits these critical details.
While the library-specific README (
libs/enclave-vm/README.md) documents features, the PR itself should clarify:
- What security vulnerabilities or gaps this PR addresses
- How the new safe-error handling, tool-bridge, and serialization checks mitigate them
- Test coverage for the new security features
These details may be documented in the PR description. Please confirm that the PR description on GitHub includes problem/solution context and security-impact notes.
16-16: Documentation link is accessible and current.The documentation URL
https://enclave.agentfront.devis confirmed to be accessible and actively serves the EnclaveVM Playground documentation. No action needed.libs/enclave-vm/src/value-sanitizer.ts (3)
123-131: BigInt handling insanitizeValueis a good hardening step.
Convertingbigintto string before it can reach JSON serialization avoidsJSON.stringifythrowing.
507-517: These are internal VM utilities, not public API—but PR should document the security implications.The functions
checkSerializedSizeandestimateSerializedSizeare not part of the public barrel export (not re-exported inlibs/enclave-vm/src/index.ts), so they are internal utilities rather than public API. They are actively used within the VM adapter for security purposes (detecting serialization-based memory exhaustion attacks per the JSDoc).Per AGENTS.md guidelines, PRs with sandbox/VM changes should include a security-impact note in the problem/solution description and test evidence (
nx test enclave-vm). Ensure your PR description covers why these serialization size checks are necessary for resource isolation.
292-323: The export claim is incorrect—this function should remain private, and there is no build break.The function at line 292 in
value-sanitizer.tsis a private helper for estimating JSON serialized size. The exportedestimateStringSizeis inmemory-tracker.ts(line 70) and serves a different purpose (V8 heap memory estimation, not JSON size). Addingexportwould be wrong.However, the escaping accuracy issue is valid: the function treats all control characters as
\uXXXX(6 bytes), but JSON.stringify uses short escapes for common controls (\n,\r,\t,\b,\f—each 2 bytes). This causes overestimation in JSON size validation.To fix only the escaping accuracy (without the incorrect export):
Accurate escaping fix (without export)
function estimateStringSize(str: string): number { let bytes = 2; // quotes for (let i = 0; i < str.length; i++) { const code = str.charCodeAt(i); if (code < 128) { // ASCII: check if needs escaping if (code === 34 || code === 92) { // Quote and backslash: 2-byte escape (\" or \\) bytes += 2; + } else if ( + code === 0x08 || // \b + code === 0x09 || // \t + code === 0x0a || // \n + code === 0x0c || // \f + code === 0x0d // \r + ) { + // Common JSON short escapes + bytes += 2; } else if (code < 32) { // Control characters: 6-byte \uXXXX escape bytes += 6;Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.