feat: enhance security by implementing recursive SecureProxy wrapping for custom globals and adding AST validation presets #14
Conversation
… for custom globals and adding AST validation presets
📝 WalkthroughWalkthroughThe PR adds configurable AST validation presets, recursively secures user-provided globals with SecureProxy (including bound functions and nested objects), and introduces extensive runtime and function-gadget sandbox-escape test suites; README and SECURITY-AUDIT metrics/docs updated to reflect v2.0.0 and expanded test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)libs/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (7)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/SECURITY-AUDIT.md (1)
452-453: Inconsistent test count in conclusion section.The conclusion states "100% test pass rate (690/690 passing)" but the header at line 5-6 indicates "1184 security tests" with "1184/1184 passing". Update the conclusion to reflect the correct test count.
🔎 Proposed fix
-- ✅ **100% test pass rate** (690/690 passing) +- ✅ **100% test pass rate** (1184/1184 passing)
🧹 Nitpick comments (1)
libs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.ts (1)
567-623: Important known limitation documentation test.This test at line 567 correctly documents a critical security consideration: passing constructor functions (like
Map) as globals exposes the host's Function constructor. The test doesn't fail - it documents the behavior with aconsole.warn.Consider using a more structured approach to document this limitation rather than
console.warnin test output, which may be noisy or missed.🔎 Alternative approach
if (!isBlockedOrNoAccess) { // This confirms the limitation: host access IS possible // When Map is passed as global, Function constructor is host's - console.warn('⚠️ KNOWN LIMITATION: Passing Map as global allows host access:', result.value); + // Note: This is expected behavior - documented limitation + // See SECURITY-AUDIT.md section "Constructor Functions as Globals" }The limitation is already well-documented in the SECURITY-AUDIT.md file, so the runtime warning may be redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/enclave-vm/README.mdlibs/enclave-vm/SECURITY-AUDIT.mdlibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/enclave-vm/src/__tests__/function-gadget-attacks.spec.tslibs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/index.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/enclave-vm/src/index.tslibs/enclave-vm/src/types.tslibs/enclave-vm/src/__tests__/function-gadget-attacks.spec.tslibs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/README.mdlibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/enclave-vm/src/enclave.tslibs/enclave-vm/src/adapters/vm-adapter.tslibs/enclave-vm/SECURITY-AUDIT.md
🧬 Code graph analysis (3)
libs/enclave-vm/src/types.ts (1)
libs/enclave-vm/src/index.ts (1)
AstPreset(29-29)
libs/enclave-vm/src/enclave.ts (2)
libs/enclave-vm/src/types.ts (1)
AstPreset(41-41)libs/ast-guard/src/presets/agentscript.preset.ts (1)
createAgentScriptPreset(178-424)
libs/enclave-vm/src/adapters/vm-adapter.ts (2)
libs/enclave-vm/src/index.ts (1)
createSecureProxy(92-92)libs/enclave-vm/src/secure-proxy.ts (1)
createSecureProxy(399-627)
🪛 markdownlint-cli2 (0.18.1)
libs/enclave-vm/SECURITY-AUDIT.md
719-719: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
726-726: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
732-732: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
737-737: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
741-741: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
744-744: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
747-747: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
751-751: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
755-755: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
759-759: Table column count
Expected: 4; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (16)
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (1)
240-251: LGTM! Enhanced recursive proxying strengthens security.The recursive wrapping of both bound methods and nested objects prevents deep property chain attacks. The depth limit (max 10) provides adequate protection against stack overflow while allowing reasonable nesting.
Key benefits:
- Blocks attacks via bound function constructor access
- Prevents prototype pollution through nested objects
- Maintains internal slot access for built-in objects like Promise
libs/enclave-vm/src/index.ts (1)
29-29: New public API export looks good.The AstPreset type addition is properly documented in types.ts with clear examples and preset descriptions.
Based on coding guidelines, when public APIs change in libs/, there should be a matching docs/draft/docs/ update. If documentation hasn't been prepared yet, please ensure it's added before release.
libs/enclave-vm/README.md (1)
15-16: Documentation updates reflect expanded test coverage.The metrics correctly reflect the PR's enhanced security testing, with test count more than doubling (516+ → 1184+) and attack vector coverage increasing significantly (81+ → 150+).
libs/enclave-vm/src/adapters/vm-adapter.ts (1)
388-400: Enhanced custom globals security looks good.Wrapping user-provided globals with
createSecureProxyeffectively prevents prototype chain attacks through custom objects. The conditional logic correctly bypasses primitives which don't need protection.Minor observation: The
as anycast on line 394 is necessary but consider using a more specific type if possible for better type safety.libs/enclave-vm/src/types.ts (2)
26-41: Well-designed AstPreset type with excellent documentation.The type follows TypeScript best practices and provides clear guidance through comprehensive JSDoc. The preset names are intuitive and align well with the existing SecurityLevel pattern.
714-738: Excellent API design and documentation for the preset option.The new
presetfield is:
- Non-breaking (optional)
- Well-documented with clear examples
- Aligned with existing patterns
- Provides good developer guidance
The note about custom globals compatibility is particularly helpful.
libs/enclave-vm/src/__tests__/enclave.security.spec.ts (1)
1303-1426: Well-structured security test suite for custom globals.The new test suite thoroughly covers critical security scenarios for custom globals with both Double VM enabled and disabled modes. The tests properly:
- Verify
__proto__access blocking via string concatenation- Test prototype chain attack prevention
- Validate constructor access blocking on nested objects
- Consistently use
securityLevel: 'STRICT'for appropriate test conditions- Properly dispose enclave instances after each test
libs/enclave-vm/src/enclave.ts (2)
14-21: LGTM - New preset imports are well-organized.The imports for the preset factories are cleanly grouped and align with the new multi-preset architecture.
224-228: LGTM - Clean preset selection implementation.The preset selection with fallback to 'agentscript' is straightforward and maintains backward compatibility.
libs/enclave-vm/src/__tests__/function-gadget-attacks.spec.ts (3)
1-29: Excellent documentation header explaining the security model.The file header clearly explains the three-layer security approach (AST Layer, Runtime Layer, Sandbox Context) and the attack categories being tested. This is valuable documentation for future maintainers.
30-283: Comprehensive primitive constructor chain tests.The tests thoroughly cover constructor chain attacks via String, Number, Boolean, Array, Object, and RegExp primitives. The tests correctly verify that even when the Function constructor is accessible through sandbox-created objects, it runs in sandbox context without host access.
Good practice: Tests use appropriate presets (PERMISSIVE for RegExp tests where needed).
1286-1339: Coverage summary provides good test documentation.The coverage summary test that documents all attack categories and verifies vector counts (10 categories, 30+ vectors) is a useful pattern for maintaining test completeness.
libs/enclave-vm/src/__tests__/runtime-attack-vectors.spec.ts (4)
1-51: Excellent security model documentation.The header comment provides crucial context about the three-layer security model (wrapped objects, sandbox-created objects, AST validation) and clearly explains what each layer protects. This is essential documentation for understanding the test expectations.
751-803: Critical host isolation verification for Error.prepareStackTrace.Good security practice: the test at line 771 verifies that the host's
Error.prepareStackTraceis NOT modified by sandbox code, preventing a potential sandbox escape vector. The test correctly stores and compares the original value.
1477-1529: Critical process.env isolation tests.These tests are essential for verifying that custom globals with
process.env-like structures cannot be exploited to access real host environment variables. The tests correctly verify:
__proto__chain attack is blocked- Only provided env vars are visible, not real ones like
PATHorHOME
1537-1601: Coverage summary provides test completeness verification.The coverage summary documents 8 categories and 35+ vectors, providing a useful reference for test scope and maintaining coverage over time.
Summary by CodeRabbit
New Features
Security Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.