fix(security): require ToolAccessPolicy deny-list and protected-path policies - #1787
Open
Aaronontheweb wants to merge 4 commits into
Open
fix(security): require ToolAccessPolicy deny-list and protected-path policies#1787Aaronontheweb wants to merge 4 commits into
Aaronontheweb wants to merge 4 commits into
Conversation
…policies
ToolAccessPolicy took its shell deny-list (shellCommandPolicy) and its
protected-path policy (toolPathPolicy) as optional constructor parameters.
Both were consumed null-tolerantly:
- "_shellCommandPolicy is not null && ..." skips the hard-deny gate when null.
- "_toolPathPolicy?.CommandReferencesDeniedPath(...) == true" passes when null.
A policy built without those deps therefore silently allows commands the
deny-list and protected-path rules should block. Two in-repo fallbacks built
exactly that shape: the DispatchingToolExecutor 1-arg convenience constructor
and the SubAgentActor null-policy fallback. Production is not affected today
(SubAgentSpawner threads the fully-wired policy), but the optional parameters
leave a silent fail-open path for any future or edge caller.
Make both dependencies required so the type system guarantees they exist:
- ToolAccessPolicy rejects null with ArgumentNullException, moves the two
deps ahead of the optional parameters, and drops the null guards at the
consumption sites.
- Remove the DispatchingToolExecutor(registry) convenience constructor.
- Require a policy on SubAgentActor and SubAgentActor.CreateProps.
Update all construction sites; production wiring already passed both policies.
Add ToolAccessPolicyRequiredDependenciesTests: two contract tests that the
constructor rejects a null deny-list / protected-path policy, and a positive
control that the protected-path deny fires when the policy is present.
Aaronontheweb
marked this pull request as ready for review
August 7, 2026 01:06
…uards The deny-list and protected-path policies are required, non-nullable constructor parameters. With Nullable enabled and warnings-as-errors a null argument is a compile error at every call site, and the shell gate dereferences the deps directly, so a stray null from an NRT-oblivious path fails loudly (NullReferenceException) rather than silently skipping a check. The runtime ArgumentNullException.ThrowIfNull guards only moved that crash a few frames earlier, so drop them (ToolAccessPolicy, SubAgentActor, SubAgentActor.CreateProps). Trim the test to the one behavioral case that exercises real logic — the protected-path control is enforced and scoped once wired, the only coverage of shell_references_protected_path through AuthorizeInvocation. The null-rejection contract tests only compiled via null! (defeating the compiler) and re-tested the framework, not our code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ToolAccessPolicytook its two shell security controls as optional constructorparameters:
shellCommandPolicy(the hard deny-list), andtoolPathPolicy(the protected-path policy).Both were consumed with the null-tolerant pattern the constitution warns about:
if (_shellCommandPolicy is not null && …)— a null skips the hard-deny gate._toolPathPolicy?.CommandReferencesDeniedPath(…) == true— a null makes theprotected-path check evaluate to false and pass.
A
ToolAccessPolicybuilt without those deps therefore silently allows commandsthe deny-list and protected-path rules should block. Two in-repo fallback
constructions did exactly that:
DispatchingToolExecutorhad a 1-arg convenience constructor that built a policywith no
toolPathPolicy.SubAgentActorfell back to the same shape when no policy was injected.Production is not breached today —
SubAgentSpawnerthreads the fully-wired policyfrom
Program.csinto every sub-agent — but the optional parameters leave the dooropen for any future or edge caller to get a silently degraded policy. On a
security control, a null must fail loudly, not fail open.
Fix
Make the two security dependencies required so the type system, not a call-site
convention, guarantees they are present:
ToolAccessPolicynow takesshellCommandPolicyandtoolPathPolicyas requiredparameters (reordered ahead of the optional ones) and rejects null with
ArgumentNullException. The consumption sites drop their null guards.DispatchingToolExecutor(registry)convenience constructor is removed;callers must pass a policy.
SubAgentActorandSubAgentActor.CreatePropsrequire a policy — the nullfallback is gone.
Tests
ToolAccessPolicyRequiredDependenciesTestsadds:Constructor_requires_shell_command_policy— a null deny-list is rejected.Constructor_requires_tool_path_policy— a null protected-path policy is rejected.Shell_referencing_protected_path_is_denied_when_dependencies_are_wired— apositive control proving the protected-path deny fires when the policy is present
(the block the fallbacks silently lost).
The two contract tests fail before the fix (the constructor accepts null) and pass
after.
Scope
This PR is limited to the required-security-dependency fix. It came out of an
investigation into failing tool calls on self-hosted models; the other findings
from that investigation (approval re-drive of parallel "messy" tool batches, and a
history-ordering issue that lets a failed tool batch wedge a session on strict
OpenAI-compatible endpoints) are tracked separately and are not part of this change.