fix(harness): enforce parent DENY rules for spawned subagents - #2477
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
oss-maintainer
left a comment
There was a problem hiding this comment.
Review: fix(harness): enforce parent DENY rules for spawned subagents
Verdict: Approved ✅
Security Analysis
This PR fixes a real permission bypass where spawned subagents could execute tools explicitly denied by their parent. Three root causes are correctly addressed:
-
Type check gap — wraps a delegate but isn't itself a , so the old
instanceof ReActAgentcheck skipped propagation. Fixed by resolving the delegate viaharnessAgent.getDelegate(). -
Wrong target slot — Previous logic updated the default permission engine instead of the child's actual
(userId, sessionId)slot. Fixed by threadinguserId+childSessionIdthrough tomergeParentDenyRulesIntoSlot(). -
Stale
AgentState.permissionContext— Updating onlyPermissionEngineleft the state's context unchanged, allowing the runtime to treat it as trivial and bypass the engine. Fixed by the newinstallPermissionContext()helper that keeps state, engine cache, and persistence synchronized.
Design Quality
- Conservative merging: Only parent DENY rules are inherited; child's ALLOW/ASK rules, working directories, and mode are preserved. This is the correct security boundary — deny lists propagate, allow lists don't.
- Trivial child fallback: When a child has no explicit permissions (
isTrivial()), the merged context usesBYPASSmode, preserving backward compatibility forPASSTHROUGHtools while still enforcing explicit DENY rules. - Deduplication:
!targetRules.contains(rule)prevents duplicate rule accumulation on repeated spawn/send calls. - Three reapplication points: spawn, reuse, and
agent_send— covers all entry paths for parent→child interaction.
Minor Observations
"Permission denied by user"→"Permission denied by rules"is a good accuracy improvement for automatic rule rejections.- The
replacePermissionContext()public API onReActAgentis well-documented with clear Javadoc about in-flight call semantics. - 385 lines of tests covering HarnessAgent delegate, direct ReActAgent, slot isolation, persistence, dedup, and
inheritParentPermissions=false— thorough.
CI Status
Builds pending, CLA ✅. Looks good to merge once CI passes.
oss-maintainer
left a comment
There was a problem hiding this comment.
Review: fix(harness): enforce parent DENY rules for spawned subagents
Verdict: Approved ✅
Security Analysis
This PR fixes a real permission bypass where spawned subagents could execute tools explicitly denied by their parent. Three root causes are correctly addressed:
-
Type check gap —
HarnessAgentwraps aReActAgentdelegate but isn't itself aReActAgent, so the oldinstanceof ReActAgentcheck skipped propagation. Fixed by resolving the delegate viaharnessAgent.getDelegate(). -
Wrong target slot — Previous logic updated the default permission engine instead of the child's actual
(userId, sessionId)slot. Fixed by threadinguserId+childSessionIdthrough tomergeParentDenyRulesIntoSlot(). -
Stale
AgentState.permissionContext— Updating onlyPermissionEngineleft the state's context unchanged, allowing the runtime to treat it as trivial and bypass the engine. Fixed by the newinstallPermissionContext()helper that keeps state, engine cache, and persistence synchronized.
Design Quality
- Conservative merging: Only parent DENY rules are inherited; child's ALLOW/ASK rules, working directories, and mode are preserved. This is the correct security boundary — deny lists propagate, allow lists don't.
- Trivial child fallback: When a child has no explicit permissions (
isTrivial()), the merged context usesBYPASSmode, preserving backward compatibility forPASSTHROUGHtools while still enforcing explicit DENY rules. - Deduplication:
!targetRules.contains(rule)prevents duplicate rule accumulation on repeated spawn/send calls. - Three reapplication points: spawn, reuse, and
agent_send— covers all entry paths for parent→child interaction.
Minor Observations
"Permission denied by user"→"Permission denied by rules"is a good accuracy improvement for automatic rule rejections.- The
replacePermissionContext()public API onReActAgentis well-documented with clear Javadoc about in-flight call semantics. - 385 lines of tests covering HarnessAgent delegate, direct ReActAgent, slot isolation, persistence, dedup, and
inheritParentPermissions=false— thorough.
CI Status
Builds pending, CLA ✅. Looks good to merge once CI passes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Summary
Ensure spawned subagents cannot execute tools explicitly denied by their parent agent.
Problem
SubagentDeclaration.inheritParentPermissionsdefaults totrue, but parent DENY rules were not effective for automatically createdHarnessAgentsubagents.There were three causes:
HarnessAgentwraps aReActAgentdelegate but is not itself aReActAgent, so the existing type check skipped propagation.(userId, sessionId)slot.PermissionEngineleftAgentState.permissionContextunchanged, allowing the runtime to treat the context as trivial and bypass the engine.As a result, a subagent could execute a tool explicitly denied by its parent.
Changes
ReActAgentchildren andHarnessAgentdelegates.AgentState, the permission engine cache, and persisted state synchronized.inheritParentPermissions=false.Permission denied by rules.Only parent DENY rules are inherited. This change does not inherit parent ALLOW rules, ASK rules, or working directories.
For an initially trivial child context, BYPASS mode preserves the previous behavior for unmatched
PASSTHROUGHtools, while explicit DENY rules still take precedence.Tests
Added coverage for:
HarnessAgentdelegate.ReActAgentchildren.(userId, childSessionId)slot updates.agent_sendsynchronization.