feat: enhance security by preventing raw host object leaks in proxy and enclave - #74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change prevents host-owned object and function references from escaping through pinned properties. Host-global wrapping preserves ownership across nested values and calls, with documentation and tests covering invariants, primitives, constructors, custom globals, and realm intrinsics. ChangesHost Reference Boundary
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostGlobals as hostConfig.globals
participant ParentVM as parent VM
participant Sandbox as sandbox code
HostGlobals->>ParentVM: inject globals in host mode
ParentVM->>ParentVM: recursively wrap reachable values
Sandbox->>ParentVM: access pinned property
ParentVM-->>Sandbox: return primitive or throw SecurityError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (4)
libs/core/src/__tests__/enclave.host-reference-leak.spec.ts (2)
287-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
dispose()is only reached on the happy path.Every test in this block calls
enclave.dispose()as the last statement, so a failed expectation leaks the enclave (and its worker pool) for the remainder of the run, which can cascade into unrelated timeouts. Track the instance in aletand dispose inafterEach, or wrap the body intry/finally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/src/__tests__/enclave.host-reference-leak.spec.ts` around lines 287 - 294, Update the tests using enclaveWithHostGlobals so enclave disposal is guaranteed even when assertions fail. Track each Enclave instance for cleanup in afterEach, or wrap each test body in try/finally, while preserving the existing test behavior and ensuring the worker pool is always disposed.
309-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert why the PoC failed, not just that it failed.
expect(result.success).toBe(false)passes for any failure — a syntax error, a timeout, or a future refactor that breaks the fixture would keep this green while the membrane regressed. Pin the error to the denial.💚 Proposed assertion
const result = await enclave.run(code); assertNoRce(result); expect(result.success).toBe(false); + expect(result.error?.message).toMatch(/blocked/i); enclave.dispose();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/src/__tests__/enclave.host-reference-leak.spec.ts` around lines 309 - 312, Strengthen the assertion in the enclave run test by verifying that the failed result contains the expected membrane-denial error, rather than only checking result.success is false. Use the result’s existing error field and assert the specific denial message or identifying error code, while preserving the existing assertNoRce and disposal behavior.libs/core/src/__tests__/secure-proxy.spec.ts (1)
413-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name attributes the throw to the wrong defense, and the
freezeis a no-op.A class's
prototypeis already non-writable and non-configurable, soObject.freeze(Widget.prototype)changes nothing about what is asserted. More importantly,createSecureProxydefaults tothrowOnBlocked: false, so the blocked-property path would returnundefinedhere — the throw comes from the new descriptor branch, not "the blocked-property path". Renaming keeps the regression suite honest about which control is under test.♻️ Proposed rename
- it('should not leak a frozen class prototype through the blocked-property path', () => { + it('should not leak a class prototype via the non-configurable descriptor path', () => { class Widget { value = 1; } - Object.freeze(Widget.prototype); const proxy = createSecureProxy(Widget) as unknown as Record<string, unknown>; - // `prototype` on a class is non-configurable and non-writable, so the old invariant - // concession returned it raw — from there `.constructor.constructor` is host Function. + // A class `prototype` is non-configurable and non-writable, so the old invariant + // concession returned it raw — from there `.constructor.constructor` is host Function. expect(() => proxy['prototype']).toThrow(/blocked/i); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/src/__tests__/secure-proxy.spec.ts` around lines 413 - 423, Rename the test around createSecureProxy to describe the descriptor/invariant defense rather than the blocked-property path, and remove the redundant Object.freeze(Widget.prototype) setup. Preserve the assertion that accessing proxy['prototype'] throws, since the default throwOnBlocked behavior is not the control under test.libs/core/src/secure-proxy.ts (1)
493-499: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDescriptor lookup now runs on every
get.
Object.getOwnPropertyDescriptorallocates a fresh object per property read on a hot path (recursive proxying already multiplies reads). Consider short-circuiting forSymbol.toStringTag/well-known symbols, or only performing the lookup when the value is object/function — though note the current ordering is what makes the invariant check sound, so any reordering needs care.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/src/secure-proxy.ts` around lines 493 - 499, Reduce descriptor-allocation overhead in the proxy get handling around Object.getOwnPropertyDescriptor by short-circuiting safe Symbol.toStringTag or other well-known symbol reads before the lookup, while preserving the required non-configurable, non-writable property invariant checks for other values. Ensure any fast path cannot expose an object or function across the proxy barrier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/src/secure-proxy.ts`:
- Around line 500-509: Update the documentation under docs/enclave/** for the
exported createSecureProxy API to describe that accessing non-configurable,
non-writable properties containing objects or functions now throws a
SecurityError instead of returning the value. Keep the documentation aligned
with the behavior implemented around descriptor handling.
- Around line 499-510: Update the get-trap logic around descriptor handling in
secure-proxy.ts so non-configurable, non-writable object/function properties are
denied only when the value is host-owned, matching the double-VM path. Preserve
returning exact primitive values and ordinary reads of frozen sandbox-owned
objects; reuse the existing ownership check rather than rejecting every
object-valued descriptor.
---
Nitpick comments:
In `@libs/core/src/__tests__/enclave.host-reference-leak.spec.ts`:
- Around line 287-294: Update the tests using enclaveWithHostGlobals so enclave
disposal is guaranteed even when assertions fail. Track each Enclave instance
for cleanup in afterEach, or wrap each test body in try/finally, while
preserving the existing test behavior and ensuring the worker pool is always
disposed.
- Around line 309-312: Strengthen the assertion in the enclave run test by
verifying that the failed result contains the expected membrane-denial error,
rather than only checking result.success is false. Use the result’s existing
error field and assert the specific denial message or identifying error code,
while preserving the existing assertNoRce and disposal behavior.
In `@libs/core/src/__tests__/secure-proxy.spec.ts`:
- Around line 413-423: Rename the test around createSecureProxy to describe the
descriptor/invariant defense rather than the blocked-property path, and remove
the redundant Object.freeze(Widget.prototype) setup. Preserve the assertion that
accessing proxy['prototype'] throws, since the default throwOnBlocked behavior
is not the control under test.
In `@libs/core/src/secure-proxy.ts`:
- Around line 493-499: Reduce descriptor-allocation overhead in the proxy get
handling around Object.getOwnPropertyDescriptor by short-circuiting safe
Symbol.toStringTag or other well-known symbol reads before the lookup, while
preserving the required non-configurable, non-writable property invariant checks
for other values. Ensure any fast path cannot expose an object or function
across the proxy barrier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e12c6cc-e476-4067-bb77-a690875e882d
📒 Files selected for processing (4)
libs/core/src/__tests__/enclave.host-reference-leak.spec.tslibs/core/src/__tests__/secure-proxy.spec.tslibs/core/src/double-vm/parent-vm-bootstrap.tslibs/core/src/secure-proxy.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/enclave/core-libraries/enclave-browser/security-architecture.mdx`:
- Line 143: Correct the malformed sentence in the security guidance by replacing
“Hand sandboxed code plain serialized data” with wording that states sandboxed
code should receive only plain serialized data. Preserve the surrounding
explanation unchanged.
- Around line 141-143: Update the browser implementation’s supplied get trap to
enforce the documented boundary for non-configurable, non-writable own data
properties: return primitive values unchanged, but throw SecurityError for
object or function values before returning descriptor.value, regardless of
throwOnBlocked. Align the behavior in the get-trap logic of
inner-iframe-bootstrap with the security-architecture documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b5d832b-2c35-4d57-a8b7-d099d03534a3
📒 Files selected for processing (3)
docs/enclave/core-libraries/enclave-browser/security-architecture.mdxlibs/core/src/__tests__/enclave.host-reference-leak.spec.tslibs/core/src/__tests__/secure-proxy.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/core/src/tests/secure-proxy.spec.ts
- libs/core/src/tests/enclave.host-reference-leak.spec.ts
… property behavior in documentation
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
1 participant
Summary by CodeRabbit
SecurityError, while primitive pinned values still work.