feat: enhance security by clarifying host value boundaries and pinned property behavior in documentation - #76
Conversation
… property behavior in documentation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds static and runtime protection for laundered dangerous property keys, integrates computed-key guarding into ChangesSandbox escape defenses
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Enclave
participant ASTGuard
participant Sandbox
participant SecureProxy
Enclave->>ASTGuard: Transform dynamic computed member accesses
ASTGuard-->>Enclave: Return guarded code
Enclave->>Sandbox: Execute transformed code
Sandbox->>ASTGuard: Resolve computed key
ASTGuard-->>Sandbox: Return key or throw security error
Sandbox->>SecureProxy: Traverse wrapped references
SecureProxy-->>Sandbox: Return proxy or fail closed at recursion limit
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (1)
libs/ast/src/transforms/computed-member-guard.transform.ts (1)
114-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent no-op on non-Program AST leaves dangling
__ag_guardKeycalls.
guardComputedMemberKeyswraps computed keys and incrementsguardedCountunconditionally during the walk (lines 127-146), butinjectGuardHelperonly defines the helper ifast.type === 'Program'(line 167), otherwise it silently returns without injecting anything. If ever called with a non-Program node (the type signatureacorn.Nodedoesn't prevent this), every rewritten computed access would throw an unguardedReferenceError: __ag_guardKey is not definedat runtime instead of failing with the intended security error — a functional regression rather than a security one, but a confusing one to debug.♻️ Proposed defensive fix
export function guardComputedMemberKeys( - ast: acorn.Node, + ast: acorn.Program, parse: (source: string) => acorn.Node, options: ComputedMemberGuardOptions = { blockedKeys: DEFAULT_GUARD_BLOCKED_KEYS, throwOnBlocked: true }, ): ComputedMemberGuardResult { const blockedKeys = options.blockedKeys ?? DEFAULT_GUARD_BLOCKED_KEYS; + if (ast.type !== 'Program' || !Array.isArray((ast as any).body)) { + return { guardedCount: 0 }; + } // Nothing to enforce (e.g. a level that blocks no keys) — leave the code untouched. if (blockedKeys.length === 0) { return { guardedCount: 0 }; }🤖 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/ast/src/transforms/computed-member-guard.transform.ts` around lines 114 - 172, Ensure guardComputedMemberKeys only rewrites computed members when the input AST is a Program that can receive the helper, or otherwise fail explicitly before walking and incrementing guardedCount. Update the interaction between guardComputedMemberKeys and injectGuardHelper so every generated __ag_guardKey call has a corresponding injected helper, avoiding silent no-ops for non-Program nodes.
🤖 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/ast/src/rules/resource-exhaustion.rule.ts`:
- Around line 480-512: Update evaluateArrayJoin so Array.prototype.toString()
uses a comma separator, matching its runtime behavior for multi-element arrays;
retain the existing join handling, including its default comma and explicit
separator arguments.
In `@libs/core/src/__tests__/enclave.bind-depth-escape.spec.ts`:
- Around line 20-30: Extend the Enclave bind-depth test cases to include a chain
exceeding 256 .bind reads, and assert that the result fails with an error
matching /recursion depth/i. Update the assertion for this case so it
specifically verifies the recursion fail-closed policy rather than relying on
assertNoRce, while preserving the existing exploit checks for shorter chains.
In `@libs/core/src/double-vm/parent-vm-bootstrap.ts`:
- Around line 421-429: Replace the raw Error thrown by __ag_createSecureProxy
when the recursion depth exceeds MEMBRANE_MAX_RECURSION_DEPTH with the enclosing
IIFE’s existing __ag_createSafeError helper, preserving the current
security-violation message and fail-closed behavior.
In `@libs/core/src/secure-proxy.ts`:
- Around line 479-483: Validate configuredMaxDepth before applying Math.max in
the recursion-depth setup: reject non-finite values such as NaN and Infinity by
falling back to MIN_MEMBRANE_RECURSION_DEPTH, then clamp valid values so
maxDepth is always a finite fail-closed limit. Update the existing maxDepth
calculation near MIN_MEMBRANE_RECURSION_DEPTH without changing the option
precedence.
---
Nitpick comments:
In `@libs/ast/src/transforms/computed-member-guard.transform.ts`:
- Around line 114-172: Ensure guardComputedMemberKeys only rewrites computed
members when the input AST is a Program that can receive the helper, or
otherwise fail explicitly before walking and incrementing guardedCount. Update
the interaction between guardComputedMemberKeys and injectGuardHelper so every
generated __ag_guardKey call has a corresponding injected helper, avoiding
silent no-ops for non-Program nodes.
🪄 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: 539c3fdf-9cc8-4e01-88ec-e93e050d12f1
📒 Files selected for processing (15)
libs/ast/src/__tests__/resource-exhaustion-tainted-key.spec.tslibs/ast/src/index.tslibs/ast/src/rules/resource-exhaustion.rule.tslibs/ast/src/transforms/computed-member-guard.transform.tslibs/ast/src/transforms/index.tslibs/browser/src/adapters/inner-iframe-bootstrap.tslibs/core/src/__tests__/enclave.bind-depth-escape.spec.tslibs/core/src/__tests__/enclave.computed-key-guard.spec.tslibs/core/src/__tests__/enclave.host-reference-leak.spec.tslibs/core/src/__tests__/function-gadget-attacks.spec.tslibs/core/src/__tests__/resource-exhaustion.spec.tslibs/core/src/__tests__/secure-proxy.spec.tslibs/core/src/double-vm/parent-vm-bootstrap.tslibs/core/src/enclave.tslibs/core/src/secure-proxy.ts
…iptors and reflection methods
…orcing recursion depth limits
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/core/src/double-vm/parent-vm-bootstrap.ts (1)
421-453: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDepth-cap check runs before the primitive/type check, so a long property chain can throw on a totally safe primitive value.
In
__ag_createSecureProxy, thegettrap (439-453) recurses unconditionally for every property value (return __ag_createSecureProxy(value, depth + 1);), with no object/function guard first — unlike the corresponding get trap in the maincreateSecureProxy(944-953), which only recurses for object/function values. Combined with the depth-check being placed before the primitive-type check (422-429), a sufficiently long property/bind chain off__safe_concat/__safe_templatethat eventually resolves to a plain primitive (e.g..length,.name) will incorrectly throw aSecurityErrorfor that primitive instead of returning it — primitives can never carry the escape risk this cap defends against.♻️ Proposed fix
function __ag_createSecureProxy(obj, depth) { if (depth === undefined) depth = 0; if (!__ag_Proxy || !__ag_Reflect) return obj; + if (obj === null || (typeof obj !== 'object' && typeof obj !== 'function')) return obj; if (depth > ${MEMBRANE_MAX_RECURSION_DEPTH}) { throw __ag_createSafeError('Security violation: secure-proxy recursion depth exceeded.'); } - if (obj === null || (typeof obj !== 'object' && typeof obj !== 'function')) return obj;🤖 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/double-vm/parent-vm-bootstrap.ts` around lines 421 - 453, Update the get trap in __ag_createSecureProxy to recurse through __ag_createSecureProxy only when the retrieved value is an object or function; return primitive values directly. Preserve the existing depth-cap behavior for values that still require proxying, while ensuring safe primitives such as length or name do not trigger a recursion-depth error.libs/core/src/secure-proxy.ts (1)
671-722: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winAccessor descriptors still leak raw functions from
getOwnPropertyDescriptor
libs/core/src/secure-proxy.ts#L678-L721andlibs/core/src/double-vm/parent-vm-bootstrap.ts#L984-L1031only hardendescriptor.value;descriptor.get/descriptor.setstill return raw functions, so a descriptor read can bypass the membrane. Throw for non-configurable accessors, proxyget/setfor configurable ones, and add accessor-descriptor regression coverage inlibs/core/src/__tests__/secure-proxy.spec.ts#L658-L690.🤖 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 671 - 722, Accessor descriptors can expose raw getter/setter functions through getOwnPropertyDescriptor. Update the descriptor handling in libs/core/src/secure-proxy.ts lines 671-722 and libs/core/src/double-vm/parent-vm-bootstrap.ts lines 983-1031 to throw for non-configurable accessors and proxy get/set functions for configurable accessors, preserving proxy invariants. Add regression coverage in libs/core/src/__tests__/secure-proxy.spec.ts lines 658-690 for both accessor types and membrane enforcement.
🤖 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.
Outside diff comments:
In `@libs/core/src/double-vm/parent-vm-bootstrap.ts`:
- Around line 421-453: Update the get trap in __ag_createSecureProxy to recurse
through __ag_createSecureProxy only when the retrieved value is an object or
function; return primitive values directly. Preserve the existing depth-cap
behavior for values that still require proxying, while ensuring safe primitives
such as length or name do not trigger a recursion-depth error.
In `@libs/core/src/secure-proxy.ts`:
- Around line 671-722: Accessor descriptors can expose raw getter/setter
functions through getOwnPropertyDescriptor. Update the descriptor handling in
libs/core/src/secure-proxy.ts lines 671-722 and
libs/core/src/double-vm/parent-vm-bootstrap.ts lines 983-1031 to throw for
non-configurable accessors and proxy get/set functions for configurable
accessors, preserving proxy invariants. Add regression coverage in
libs/core/src/__tests__/secure-proxy.spec.ts lines 658-690 for both accessor
types and membrane enforcement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c6ea070-462d-4f29-82a6-b33124f72113
📒 Files selected for processing (3)
libs/core/src/__tests__/secure-proxy.spec.tslibs/core/src/double-vm/parent-vm-bootstrap.tslibs/core/src/secure-proxy.ts
…r descriptors and ensuring safe proxying of getter/setter functions
…r descriptors and ensuring safe proxying of getter/setter functions
Cherry-pick ConflictAutomatic cherry-pick to An issue has been created with manual instructions. Please resolve if this change should also be in |
1 participant
Summary by CodeRabbit
getOwnPropertyDescriptorto block descriptor-based escape paths.