fix: correct Decide precedence and remove sub-session scope escalation#3542
fix: correct Decide precedence and remove sub-session scope escalation#3542Piyush0049 wants to merge 9 commits into
Conversation
Sub-agents now inherit the exact Allow, Ask, and Deny rules from the parent session instead of bypassing them. This resolves a known prompt-injection loophole for background agents. Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>
|
Note: Learning from previous PRs, I also added a unit test to verify that this behavior works correctly and stays fixed in the future. You can run the test locally using this command: |
aheritier
added
kind/fix
Sayt-0
left a comment
Sayt-0
left a comment
There was a problem hiding this comment.
Thanks for tackling this. Inheriting the parent permissions in newSubSession is the right insertion point. Two points look blocking before this can land, plus a test gap; details are inline.
| area | concern |
|---|---|
| correctness | background agents run with ToolsApproved: true, and Decide short-circuits on the yolo flag before any checker runs, so the inherited Allow/Ask/Deny rules are never consulted on that path (the exact case the removed TODO described) |
| isolation | the parent PermissionsConfig pointer is shared with the child instead of cloned, unlike every other call site |
| tests | the new test asserts field copying only, not dispatch behaviour |
| opts = append(opts, session.WithAgentName(cfg.AgentName)) | ||
| } | ||
| if parent.Permissions != nil { | ||
| opts = append(opts, session.WithPermissions(parent.Permissions)) |
There was a problem hiding this comment.
This shares the parent's *PermissionsConfig (and its underlying slices) with the child instead of cloning it. Every other call site clones through clonePermissionsConfig (session.Clone, copySessionMetadata in branch.go, store.go). The aliasing is not benign: on the interactive transfer_task path, handleResume's ResumeTypeApproveTool case appends to sess.Permissions.Allow, so a child approving a tool would mutate the parent's permissions (and append to a shared slice). Suggest cloning here, for example by exposing the existing clonePermissionsConfig helper and passing session.WithPermissions(clone).
| @@ -482,10 +485,6 @@ func (r *LocalRuntime) CurrentAgentSubAgentNames() []string { | |||
| // authorises all tool calls made by the sub-agent when they approve | |||
| // run_background_agent. Callers should be aware that prompt injection in | |||
| // the sub-agent's context could exploit this gate-bypass. | |||
There was a problem hiding this comment.
Removing this TODO looks premature. RunAgent (background agents) sets ToolsApproved: true, and Decide (pkg/runtime/toolexec/permissions.go) returns Allow on the yolo flag before evaluating any checker. So the newly inherited parent Deny/Ask rules are never consulted for a background sub-session, which is precisely the bypass this TODO called out ("rather than a single shared ToolsApproved flag"). The warning just above ("could exploit this gate-bypass") still holds, so it now contradicts the TODO removal. Options: keep the TODO, or make Deny/ForceAsk win over the yolo flag in Decide (at least for an inherited session-level Deny) and cover it with a test.
| require.NotNil(t, s.Permissions) | ||
| assert.Equal(t, perms.Allow, s.Permissions.Allow) | ||
| assert.Equal(t, perms.Deny, s.Permissions.Deny) | ||
| assert.Equal(t, perms.Ask, s.Permissions.Ask) |
There was a problem hiding this comment.
This checks field copying but not that inheritance changes dispatch behaviour. Consider driving the approval path instead: a parent Deny of write_* should still deny in the child, and in particular the background-agent case (ToolsApproved: true) where the inherited Deny currently has no effect. That case would document the gap flagged in agent_delegation.go.
- Deep clone parent permissions into the child session to prevent aliasing - Re-order yolo check in Decide to ensure inherited Deny/ForceAsk overrides ToolsApproved: true - Enhance TestSubSessionInheritsPermissions to assert actual dispatch behavior Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>
|
@Sayt-0 Thank you for the review, I read it and have pushed an update addressing all three of your points:
All tests are passing locally. Do let me know if any change is required please. |
Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>
|
I just pushed a quick follow-up commit to fix the CI failures. |
|
HI @Piyush0049 can you resolve the conflicts ? ping me when its done |
|
Sayt-0 Sure |
…sions # Conflicts: # pkg/runtime/agent_delegation.go # pkg/runtime/agent_delegation_test.go
…sions # Conflicts: # pkg/runtime/toolexec/dispatcher.go
|
Sayt-0 I have resolved the merge conflicts. Please do let me know if any changes are required. |
Signed-off-by: piyush0049 <piyushjoshi81204@gmail.com>
|
I've pushed a commit to fix the CI timeout in The Side Note: While testing locally on Windows, I noticed a few pre-existing issues in |
|
The |
|
Thanks for re-running the jobs. Please do let me know if any other changes are required. |
There was a problem hiding this comment.
Thanks for the follow-up and for iterating on the earlier feedback. The isolation fix (PermissionsConfig.Clone()) is clean, and the race-prone double-cloning issue from the earlier round looks closed (verified locally with go test -race ./pkg/runtime/... ./pkg/session/..., no race, all green, matching CI).
One design point needs a decision before this can merge, plus a few smaller items, all left as inline comments.
| Area | Concern | Blocking |
|---|---|---|
| Scope / design | Decide() reordering makes Deny and ForceAsk win over --yolo globally, not just for inherited sub-sessions |
yes, needs a decision |
| Docs consistency | Several comments and one public doc page still describe the old ordering | yes |
| Diff hygiene | A few hunks are pure reformatting unrelated to the fix | no, nit |
| Test depth | New test exercises Decide() directly rather than the full dispatch path |
no, nit |
What's good:
PermissionsConfig.Clone(): exported, nil-safe, doc-commented, consistent withSession.ClonePermissions().TestSubSessionInheritsPermissionsis a real improvement over asserting field copies only.- No new allocations or performance concerns;
slices.Cloneis equivalent to the helper it replaces.
| } | ||
| } | ||
|
|
||
| if yoloApproved { |
There was a problem hiding this comment.
This reorders the approval pipeline for every session, not just sub-sessions with inherited permissions.
Deny winning over --yolo matches the contract PermissionsConfig has documented since it was introduced (pkg/config/latest/types.go, and every frozen version v3 to v11): "Deny: Tools matching these patterns are always rejected, even with --yolo".
ForceAsk winning over --yolo reverses commit 7351e4fa0 ("--yolo must accept any tool call"), which added TestYoloMode_OverridesForceAsk (renamed here to TestForceAskOverridesYoloMode, assertion flipped) specifically to lock in the opposite behavior, and documented it in pkg/permissions/permissions.go's CheckWithArgs comment: "Note that --yolo mode takes precedence over ForceAsk" (still present on main, now stale).
The doc comment on this function, line 61, still lists yolo as step 1 ahead of the checkers; it needs updating either way this lands.
Question: should this only reorder the Deny case and leave ForceAsk behind --yolo (matching the two contracts above), or is overriding ForceAsk too an intentional posture change? If the latter, pkg/permissions/permissions.go and docs/tools/background-agents/index.md:33 ("via YOLO mode or explicit allow rules") need updating in this PR.
There was a problem hiding this comment.
It was not an intentional posture change! I have updated the logic to only reorder the Deny case, leaving ForceAsk behind --yolo as originally intended.
Because this preserves the original contract, the external docs (pkg/permissions/permissions.go and index.md) remain accurate and didn't need updating. (I've also fixed the doc comment on Decide() to reflect this correctly).
| }, "shell", nil, false) | ||
|
|
||
| assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d) | ||
| assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "team"}, d) |
There was a problem hiding this comment.
Matches the documented PermissionsConfig contract for Deny, no objection to this one specifically. See the comment on pkg/runtime/toolexec/permissions.go about the related ForceAsk case that still needs a decision.
|
|
||
| func TestYoloMode_OverridesForceAsk(t *testing.T) { | ||
| // TestForceAskOverridesYoloMode verifies that ForceAsk permissions take precedence over the yolo flag. | ||
| func TestForceAskOverridesYoloMode(t *testing.T) { |
There was a problem hiding this comment.
This test (renamed from TestYoloMode_OverridesForceAsk) now expects the tool NOT to execute, same for TestSessionDenyOverridesYoloMode below. This is the ForceAsk-over---yolo change flagged on pkg/runtime/toolexec/permissions.go, holding these pending that decision.
Separately, sess.NonInteractive = true on line 2602 fixing the CI hang (askUser blocking on a resume channel with no listener) is correct and worth keeping regardless of the outcome above.
| } | ||
| if genai.EmitLegacyAttributes() { | ||
| delegationAttrs = append(delegationAttrs, | ||
| delegationAttrs = append( |
There was a problem hiding this comment.
This hunk (and the applyForceHandoff closing-paren move further down) is pure reformatting unrelated to the fix. Confirmed with golangci-lint fmt that neither the old nor the new layout is enforced by the formatter, so this is incidental noise. Suggest reverting both to keep the diff focused.
|
|
||
| parent := session.New(session.WithUserMessage("hello")) | ||
| childAgent := agent.New("worker", "a worker agent", | ||
| childAgent := agent.New( |
There was a problem hiding this comment.
Same as the note on agent_delegation.go: the agent.New(...) reflow here (and 3 more occurrences: lines 194, 414, 471) is unrelated to the fix and adds review noise. Suggest reverting.
| {Checker: checker, Source: "session permissions"}, | ||
| } | ||
|
|
||
| decision := toolexec.Decide(s.ToolsApproved, namedCheckers, "write_file", map[string]any{"path": "foo"}, false) |
There was a problem hiding this comment.
Nit, not blocking: this builds a permissions.Checker by hand from s.Permissions and calls toolexec.Decide directly. That's a real improvement over asserting field copies only, but it still bypasses the actual wiring in permissionCheckers / rt.processToolCalls. Consider complementing it with an end-to-end case through RunAgent/runCollecting with a real background sub-session, similar to TestDenyOverridesYoloMode, so the real dispatcher path is covered too.
Restores YOLO overriding ForceAsk while keeping Deny overriding YOLO. Reverts unrelated formatting noise in agent_delegation.go and tests. Adds end-to-end TestRunAgent_EndToEndPermissions to verify dispatch path inheritance.
|
Thanks for the review. I read it and have addressed all the points in the table:
Please do let me know if any other changes are required. |
Sayt-0
left a comment
Sayt-0
left a comment
There was a problem hiding this comment.
Thanks for iterating again. The design question from the previous round is now settled: making explicit Deny win over --yolo globally is accepted. It matches the documented "Deny patterns take priority" philosophy, and background agents inheriting ToolsApproved: true would otherwise carry dead-letter Deny rules. Keeping YOLO above ForceAsk restores the original contract, good call.
Verified locally on the branch: go build ./... and go test -race ./pkg/runtime/... ./pkg/session/... are green.
Remaining items before merge, all small; details inline.
| # | Area | Concern | Blocking |
|---|---|---|---|
| 1 | Stale comments | Two inline comments in runtime_test.go still state the old behavior ("--yolo takes precedence over deny") under tests that now assert the opposite |
yes |
| 2 | Docs consistency | Three doc lines still describe the old chain order (table below) | yes |
| 3 | Test robustness | TestRunAgent_EndToEndPermissions discards the RunAgent result, so it would pass vacuously if the run failed before any tool call |
yes |
| 4 | PR title/description | The newSubSession inheritance described in the PR body already landed on main via fd89580; the merge commit would tell the wrong story. Refresh both to describe the actual change (Decide precedence + Clone() refactor) |
yes |
| 5 | Diff hygiene | Two NewLocalRuntime( reflow-only hunks remain in agent_delegation_test.go |
no, nit |
| 6 | Test coverage | After the rename, no Decide-level case covers plain yolo-allow or yolo-over-ForceAsk |
no, nit |
Docs lines to update (files not in the diff, need a commit):
| File | Line | Current | Problem |
|---|---|---|---|
docs/configuration/hooks/index.md |
60 | "approval chain (yolo / permissions / readonly / ask)" | Deny permissions now precede yolo |
docs/configuration/hooks/index.md |
716 | "tool-approval chain (yolo / permissions / readonly / ...)" | same |
docs/features/cli/index.md |
31 | "--yolo: Auto-approve all tool calls" |
no longer "all": explicit deny rules still block |
Non-blocking observation, for the record: with first-checker-wins semantics, a session-level ForceAsk consulted before a team-level Deny still yields Allow under yolo. This is pre-existing layer-precedence behavior, unchanged by this PR.
What's good:
- The
Decide()reordering is minimal and keeps the function pure; returningReasonYoloon the ForceAsk-under-yolo path keepsapproval_sourceaudit labels consistent for hooks. PermissionsConfig.Clone()is nil-safe and removes the private helper cleanly across all six call sites.TestRunAgent_EndToEndPermissionsfinally exercises the real dispatch path, closing the test-depth gap from round one.
| func TestDenyOverridesYoloMode(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Test that --yolo flag takes precedence over deny permissions |
There was a problem hiding this comment.
Stale: this contradicts the renamed test and its assertion (Deny now wins over yolo). The doc comment above the function already states the behavior, so this line can go.
| // Test that --yolo flag takes precedence over deny permissions |
| func TestSessionDenyOverridesYoloMode(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Test that --yolo flag takes precedence over session-level deny |
There was a problem hiding this comment.
Same as above: stale comment stating the pre-PR behavior under a test that now asserts the opposite.
| // Test that --yolo flag takes precedence over session-level deny |
| rt.RunAgent(t.Context(), agenttool.RunParams{ | ||
| AgentName: "worker", | ||
| Task: "do something", | ||
| ParentSession: parentSession, | ||
| }) |
There was a problem hiding this comment.
The result is discarded, so require.False(executed) would pass vacuously if RunAgent failed before dispatch (wrong agent name, model error, ...). Asserting the result and the sub-session keeps the test honest. Verified locally, this exact block passes on this branch:
| rt.RunAgent(t.Context(), agenttool.RunParams{ | |
| AgentName: "worker", | |
| Task: "do something", | |
| ParentSession: parentSession, | |
| }) | |
| result := rt.RunAgent(t.Context(), agenttool.RunParams{ | |
| AgentName: "worker", | |
| Task: "do something", | |
| ParentSession: parentSession, | |
| }) | |
| require.Empty(t, result.ErrMsg, "RunAgent should succeed") | |
| var childSession *session.Session | |
| for _, item := range parentSession.Messages { | |
| if item.SubSession != nil { | |
| childSession = item.SubSession | |
| break | |
| } | |
| } | |
| require.NotNil(t, childSession, "parent must have a sub-session") |
| // Verify the gap flagged in PR review: even if ToolsApproved is true (yolo flag), | ||
| // the inherited Deny should correctly override the yolo flag during dispatch. |
There was a problem hiding this comment.
Nit: "the gap flagged in PR review" will not mean much to future readers; a self-contained phrasing ages better.
| // Verify the gap flagged in PR review: even if ToolsApproved is true (yolo flag), | |
| // the inherited Deny should correctly override the yolo flag during dispatch. | |
| // Even with ToolsApproved set (yolo), an inherited Deny must win during dispatch. |
| } | ||
|
|
||
| func TestDecide_YoloShortCircuits(t *testing.T) { | ||
| func TestDecide_DenyOverridesYolo(t *testing.T) { |
There was a problem hiding this comment.
Nit: after the rename, two paths of the new ordering have no Decide-level case left: plain yolo-allow (no checker match) and yolo-over-ForceAsk. Two one-liners would restore the exhaustive matrix promised by Decide's doc comment (both verified locally):
func TestDecide_YoloAllowsWhenNoCheckerMatches(t *testing.T) {
t.Parallel()
d := Decide(true, nil, "shell", nil, false)
assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d)
}
func TestDecide_YoloOverridesForceAsk(t *testing.T) {
t.Parallel()
d := Decide(true, []NamedChecker{
{Checker: newChecker(t, nil, []string{"shell"}, nil), Source: "team"},
}, "shell", nil, false)
assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d)
}| require.NoError(t, err) | ||
|
|
||
| sess := session.New(session.WithUserMessage("Test"), session.WithToolsApproved(true)) | ||
| sess.NonInteractive = true |
There was a problem hiding this comment.
Nit: with yolo winning over ForceAsk, askUser is never reached, so this line is purely defensive (fail fast instead of hanging CI if the behavior regresses). Worth one short comment, matching the style of the // mirror --exec mode note at line 2466.
| sess.NonInteractive = true | |
| sess.NonInteractive = true // fail fast instead of hanging on askUser if this regresses |
| rt, err := NewLocalRuntime( | ||
| t.Context(), tm, |
There was a problem hiding this comment.
Nit: reformat-only hunk, no semantic change. Reverting keeps the diff focused on the fix (same below at line 524).
| rt, err := NewLocalRuntime( | |
| t.Context(), tm, | |
| rt, err := NewLocalRuntime(t.Context(), tm, |
| rt, err := NewLocalRuntime( | ||
| t.Context(), tm, |
There was a problem hiding this comment.
Same reformat-only hunk as above.
| rt, err := NewLocalRuntime( | |
| t.Context(), tm, | |
| rt, err := NewLocalRuntime(t.Context(), tm, |
|
Thank you for the review. I read it and have pushed an update addressing your feedback:
|
Piyush0049
changed the title
|
I have also updated the PR title and description according to the review. Please do let me know if any other change is required. |
3 participants
What this PR does / why we need it:
This PR resolves permission override scoping issues between parent and child sessions and fixes the tool-approval chain precedence.
Previously, a successful sub-session would back-propagate its
ToolsApprovedboolean andPermissionsto the parent session. This created a scope escalation bug where a sub-agent could elevate the parent's permissions for the remainder of the parent's turn. Additionally, the tool execution pipeline didn't correctly prioritize explicitDenyrules over the--yoloflag.This PR fixes these issues by:
SetToolsApprovedandSetPermissions) when a child session completes, ensuring permissions only flow downwards.Decideprecedence logic so that explicitDenyrules correctly override--yolo, while keeping--yoloaboveForceAsk.PermissionsConfig.Clone()refactor to safely isolate parent and child session permissions without data races.Special notes for your reviewer:
TestRunAgent_EndToEndPermissionsto explicitly verify that the Allow, Ask, and Deny rules are correctly enforced along the real dispatch path.Decidetest matrix by adding tests for YOLO paths without checker matches and YOLO overridingForceAsk.docs/to correctly indicate that explicitDenyrules block tools even in YOLO mode.