Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions openspec/changes/simplify-shell-policy-evaluator/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@

## 7. Remove obsolete structure

- [ ] 7.1 Remove dead coordinator branches, duplicate coverage mutation, duplicate path helpers, and duplicate prompt-scope logic.
- [ ] 7.2 Keep the required public compatibility adapter isolated from new typed policy code.
- [ ] 7.3 Record the separate generic approval API work that can remove the compatibility adapter.
- [x] 7.1 Remove dead coordinator branches, duplicate coverage mutation, duplicate path helpers, and duplicate prompt-scope logic.
- [x] 7.2 Keep the required public compatibility adapter isolated from new typed policy code.
- [x] 7.3 Record the separate generic approval API work that can remove the compatibility adapter ([#1944](https://github.com/netclaw-dev/netclaw/issues/1944)).
- [ ] 7.4 Verify production policy contains no new executable names or executable-private argument rules.
- [ ] 7.5 Verify no call-local parser occurrence, command text, path, or secret crosses actor or persistence boundaries.
- [ ] 7.6 Run API and durable-contract comparisons against the frozen baseline.
Expand Down
25 changes: 25 additions & 0 deletions src/Netclaw.Actors.Tests/Tools/ScopedShellSafeVerbPolicyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Netclaw.Configuration;
using Netclaw.Security;
using Netclaw.Tools;
using ShellSyntaxTree;
using Xunit;

namespace Netclaw.Actors.Tests.Tools;
Expand Down Expand Up @@ -339,6 +340,30 @@ public void Path_shaped_data_under_safe_root_does_not_create_new_authority()
Assert.True(policy.AllShortCircuit(candidates, _projectDir, ctx));
}

[Fact]
public void PowerShell_compatibility_paths_use_posix_host_roots()
{
if (OperatingSystem.IsWindows())
return;

var policy = new ScopedShellSafeVerbPolicy(
SafeVerbList.FromVerbs(ApprovalShell.PowerShell, ["Get-ChildItem"]));
var ctx = PersonalContext(projectDir: _projectDir);
var matcher = new ShellApprovalMatcher(
ShellExecutionEnvironment.CreatePowerShell(
"C:\\PowerShell\\pwsh.exe",
PwshDialect.PowerShell7));
var candidates = matcher.ExtractCandidates(
new ToolName("shell_execute"),
new Dictionary<string, object?>
{
["Command"] = "Get-ChildItem -LiteralPath .\\data.txt",
["WorkingDirectory"] = _projectDir
});

Assert.True(policy.AllShortCircuit(candidates, _projectDir, ctx));
}

[Fact]
public void Prefix_collision_does_not_match_reviewed_phrase()
{
Expand Down
22 changes: 1 addition & 21 deletions src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -438,27 +438,7 @@ void ISessionScratchRetryAwareExecutor.MarkSessionScratchRetry(
private static ToolAuthorizationDecision CompleteAuthorizationDecision(
ToolAccessDecision accessDecision,
IReadOnlyList<ToolApprovalMatch> approvalMatches)
{
if (accessDecision.NeedsApproval)
{
return ToolAuthorizationDecision.RequiresApproval(
accessDecision.ApprovalContext
?? throw new InvalidOperationException("Approval decision missing approval context."),
approvalMatches);
}

if (!accessDecision.Allowed)
{
return ToolAuthorizationDecision.Deny(
accessDecision.DenyReason
?? throw new InvalidOperationException("Denied decision missing a deny reason."));
}

return ToolAuthorizationDecision.Allow(
accessDecision.AllowReason
?? throw new InvalidOperationException("Allowed decision missing an allow reason."),
approvalMatches);
}
=> ToolAuthorizationDecision.From(accessDecision, approvalMatches);

private static bool TryGetExactUnapprovedCandidates(
ToolApprovalCheckResult result,
Expand Down
119 changes: 31 additions & 88 deletions src/Netclaw.Actors/Tools/ScopedShellSafeVerbPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,18 @@ public bool CanShortCircuitAfterProjectDeclaration(
.ToArray();
foreach (var candidate in candidates)
{
if (!IsReviewedDiagnostic(candidate, candidate.SourceOccurrence, prospectiveRoots))
var resolvedPaths = ResolveCompatibilityPaths(
candidate,
candidate.SourceOccurrence,
fullCwd);
if (!IsReviewedDiagnostic(
candidate,
candidate.SourceOccurrence,
prospectiveRoots,
resolvedPaths))
{
return false;
}

var effectiveDirectory = candidate.Directory ?? fullCwd;
if (string.IsNullOrWhiteSpace(effectiveDirectory))
Expand Down Expand Up @@ -128,50 +138,15 @@ private bool IsReviewedDiagnostic(
ApprovalCandidate candidate,
CommandOccurrence? sourceOccurrence,
IReadOnlyList<string> safeRoots,
string? workingDirectoryOverride = null)
{
if (candidate is not
{
Shell: { } shell,
VerbTokens: { }
}
|| sourceOccurrence is null
|| ShellRedirectPolicyFacts.HasFileWritingRedirect(sourceOccurrence)
|| !_safeVerbs.TryMatchReviewedDiagnostic(
shell,
candidate.VerbTokens,
out var matchedTokenCount))
{
return false;
}

if (sourceOccurrence.Arguments.Any(argument =>
argument.Element.PrecedingVerbElementCount < matchedTokenCount))
{
return false;
}

return AllPossibleAuthoredPathsStayWithinRoots(
sourceOccurrence,
shell,
safeRoots,
workingDirectoryOverride);
}

private bool IsReviewedDiagnostic(
ApprovalCandidate candidate,
ShellPolicyCandidatePathFacts pathFacts,
IReadOnlyList<string> safeRoots,
ShellPolicyResolvedPathView? resolvedPaths)
{
var sourceOccurrence = pathFacts.SourceOccurrence;
if (candidate is not
{
Shell: { } shell,
VerbTokens: { }
}
|| sourceOccurrence is null
|| HasFileWritingRedirect(pathFacts)
|| HasFileWritingRedirect(resolvedPaths)
|| !_safeVerbs.TryMatchReviewedDiagnostic(
shell,
candidate.VerbTokens,
Expand Down Expand Up @@ -214,7 +189,7 @@ internal bool ShortCircuitsCausalIntent(
ShellPathStyle.Posix)
|| !IsReviewedDiagnostic(
candidate,
pathFacts,
pathFacts.SourceOccurrence,
[intentPath.Value],
pathFacts.Intent))
{
Expand All @@ -233,8 +208,9 @@ internal bool ShortCircuits(
ToolInvocationContext context)
{
var safeRoots = ResolveSafeSpaceRoots(context);
var resolvedPaths = ResolveCompatibilityPaths(candidate, sourceOccurrence);
if (safeRoots.Count == 0
|| !IsReviewedDiagnostic(candidate, sourceOccurrence, safeRoots))
|| !IsReviewedDiagnostic(candidate, sourceOccurrence, safeRoots, resolvedPaths))
{
return false;
}
Expand Down Expand Up @@ -265,7 +241,7 @@ internal bool ShortCircuits(
if (safeRoots.Count == 0
|| !IsReviewedDiagnostic(
candidate,
pathFacts,
pathFacts.SourceOccurrence,
safeRoots,
pathFacts.Real)
|| pathFacts.RealScope is not
Expand Down Expand Up @@ -305,8 +281,8 @@ or NotSupportedException
}
}

private static bool HasFileWritingRedirect(ShellPolicyCandidatePathFacts pathFacts)
=> pathFacts.Real?.Facts.Any(static fact =>
private static bool HasFileWritingRedirect(ShellPolicyResolvedPathView? resolvedPaths)
=> resolvedPaths?.Facts.Any(static fact =>
fact.Source is
{
Origin: ShellPolicyPathOrigin.Redirect,
Expand Down Expand Up @@ -340,7 +316,7 @@ private static bool AllAuthoredPathsStayWithinRoots(
!safeRoots.Any(root => IsSafePath(
path.Value,
root,
pathStyle))))
path.PathStyle))))
{
return false;
}
Expand Down Expand Up @@ -391,56 +367,23 @@ private static bool AllEffectivePathsStayWithinIntent(
return true;
}

private static bool AllPossibleAuthoredPathsStayWithinRoots(
CommandOccurrence occurrence,
ApprovalShell shell,
IReadOnlyList<string> safeRoots,
string? workingDirectoryOverride)
private static ShellPolicyResolvedPathView? ResolveCompatibilityPaths(
ApprovalCandidate candidate,
CommandOccurrence? occurrence,
string? workingDirectoryOverride = null)
{
if (candidate.Shell is not { } shell || occurrence is null)
return null;

var workingDirectory = workingDirectoryOverride
?? (occurrence.WorkingDirectory is ShellValueDomain.Exact exact
? exact.Value
: null);
var pathStyle = shell == ApprovalShell.Bash
? ShellPathStyle.Posix
: ShellPathStyle.Windows;

foreach (var argument in occurrence.Arguments)
{
if (argument.AuthoredPathShape == ShellPathShape.Unknown)
continue;
if (argument.AuthoredPathShape == ShellPathShape.Posix
&& pathStyle != ShellPathStyle.Posix
|| argument.AuthoredPathShape == ShellPathShape.Windows
&& pathStyle != ShellPathStyle.Windows)
{
return false;
}

IReadOnlyList<string> possiblePaths = argument.AuthoredValue switch
{
ShellValueDomain.Exact value => [value.Value],
ShellValueDomain.FiniteSet values => values.Values,
_ => []
};
if (possiblePaths.Count == 0)
return false;

foreach (var possiblePath in possiblePaths)
{
var resolved = ShellTokenizer.NormalizePathToken(
possiblePath,
workingDirectory,
pathStyle);
if (string.IsNullOrWhiteSpace(resolved)
|| !safeRoots.Any(root => IsSafePath(resolved, root)))
{
return false;
}
}
}

return true;
var pathStyle = OperatingSystem.IsWindows()
? ShellPathStyle.Windows
: ShellPathStyle.Posix;
return ShellPolicyOccurrencePathFacts.Create(occurrence)
.Resolve(workingDirectory, pathStyle);
}

private static bool IsSafePath(string path, string root)
Expand Down
28 changes: 2 additions & 26 deletions src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -187,33 +187,9 @@ private static ToolAuthorizationDecision Complete(
ToolAccessDecision decision,
IReadOnlyList<ToolApprovalMatch> approvalMatches,
ShellPolicyDecisionTraceBuilder trace)
{
if (decision.NeedsApproval)
{
return CompleteWithTrace(
ToolAuthorizationDecision.RequiresApproval(
decision.ApprovalContext
?? throw new InvalidOperationException("Approval decision missing approval context."),
approvalMatches),
trace);
}

if (!decision.Allowed)
{
return CompleteWithTrace(
ToolAuthorizationDecision.Deny(
decision.DenyReason
?? throw new InvalidOperationException("Denied decision missing a deny reason.")),
trace);
}

return CompleteWithTrace(
ToolAuthorizationDecision.Allow(
decision.AllowReason
?? throw new InvalidOperationException("Allowed decision missing an allow reason."),
approvalMatches),
=> CompleteWithTrace(
ToolAuthorizationDecision.From(decision, approvalMatches),
trace);
}

private static ToolAuthorizationDecision CompleteWithTrace(
ToolAuthorizationDecision decision,
Expand Down
Loading
Loading