Skip to content

Commit 21984db

Browse files
committed
fix(exec): avoid double prompts for approved wrappers
1 parent 9173e4e commit 21984db

2 files changed

Lines changed: 164 additions & 9 deletions

File tree

src/OpenClaw.Shared/Capabilities/SystemCapability.cs

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -396,12 +396,17 @@ private async Task<NodeInvokeResponse> HandleRunAsync(NodeInvokeRequest request)
396396
if (_approvalPolicy != null)
397397
{
398398
var approval = _approvalPolicy.Evaluate(fullCommand, shell);
399-
if (!await EnsureApprovedAsync(fullCommand, shell, approval))
399+
var approvalCheck = await EnsureApprovedAsync(fullCommand, shell, approval);
400+
if (!approvalCheck.Allowed)
400401
{
401402
Logger.Warn($"system.run DENIED: {fullCommand} ({approval.Reason})");
402403
return Error($"Command denied by exec policy: {approval.Reason}");
403404
}
404405

406+
var outerApprovalCoversNestedTargets =
407+
approvalCheck.PromptDecisionKind != null ||
408+
IsExactAllowRuleForCommand(approval, fullCommand);
409+
405410
var parseResult = ExecShellWrapperParser.Expand(fullCommand, shell);
406411
if (!string.IsNullOrWhiteSpace(parseResult.Error))
407412
{
@@ -412,7 +417,18 @@ private async Task<NodeInvokeResponse> HandleRunAsync(NodeInvokeRequest request)
412417
foreach (var target in parseResult.Targets)
413418
{
414419
var innerApproval = _approvalPolicy.Evaluate(target.Command, target.Shell);
415-
if (!await EnsureApprovedAsync(target.Command, target.Shell, innerApproval))
420+
if (outerApprovalCoversNestedTargets && !IsExplicitDeny(innerApproval))
421+
{
422+
if (!innerApproval.Allowed)
423+
{
424+
Logger.Info(
425+
$"system.run nested approval covered by approved wrapper: {target.Command} ({innerApproval.Reason})");
426+
}
427+
continue;
428+
}
429+
430+
var innerApprovalCheck = await EnsureApprovedAsync(target.Command, target.Shell, innerApproval);
431+
if (!innerApprovalCheck.Allowed)
416432
{
417433
Logger.Warn($"system.run DENIED: {target.Command} ({innerApproval.Reason})");
418434
return Error($"Command denied by exec policy: {innerApproval.Reason}");
@@ -448,17 +464,17 @@ private async Task<NodeInvokeResponse> HandleRunAsync(NodeInvokeRequest request)
448464
}
449465
}
450466

451-
private async Task<bool> EnsureApprovedAsync(
467+
private async Task<ExecApprovalCheckResult> EnsureApprovedAsync(
452468
string command,
453469
string? shell,
454470
ExecApprovalResult approval,
455471
CancellationToken cancellationToken = default)
456472
{
457473
if (approval.Allowed)
458-
return true;
474+
return new ExecApprovalCheckResult(true, null);
459475

460476
if (approval.Action != ExecApprovalAction.Prompt || _promptHandler == null || _approvalPolicy == null)
461-
return false;
477+
return new ExecApprovalCheckResult(false, null);
462478

463479
var decision = await _promptHandler.RequestAsync(new ExecApprovalPromptRequest
464480
{
@@ -471,7 +487,7 @@ private async Task<bool> EnsureApprovedAsync(
471487
if (decision.Kind == ExecApprovalPromptDecisionKind.Deny)
472488
{
473489
Logger.Warn($"system.run DENIED by prompt: {command} ({decision.Reason})");
474-
return false;
490+
return new ExecApprovalCheckResult(false, decision.Kind);
475491
}
476492

477493
if (decision.Kind == ExecApprovalPromptDecisionKind.AlwaysAllow)
@@ -494,12 +510,28 @@ private async Task<bool> EnsureApprovedAsync(
494510
}
495511

496512
Logger.Info($"system.run APPROVED by prompt: {command} ({decision.Kind})");
497-
return true;
513+
return new ExecApprovalCheckResult(true, decision.Kind);
498514
}
499515

500516
private static bool CanPersistExactAllowRule(string command) =>
501517
!string.IsNullOrWhiteSpace(command) &&
502518
command.IndexOfAny(['*', '?']) < 0;
519+
520+
private static bool IsExactAllowRuleForCommand(ExecApprovalResult approval, string command) =>
521+
approval.Allowed &&
522+
approval.Action == ExecApprovalAction.Allow &&
523+
!string.IsNullOrWhiteSpace(approval.MatchedPattern) &&
524+
approval.MatchedPattern.IndexOfAny(['*', '?']) < 0 &&
525+
string.Equals(approval.MatchedPattern, command, StringComparison.OrdinalIgnoreCase);
526+
527+
private static bool IsExplicitDeny(ExecApprovalResult approval) =>
528+
!approval.Allowed &&
529+
approval.Action == ExecApprovalAction.Deny &&
530+
!string.IsNullOrWhiteSpace(approval.MatchedPattern);
531+
532+
private readonly record struct ExecApprovalCheckResult(
533+
bool Allowed,
534+
ExecApprovalPromptDecisionKind? PromptDecisionKind);
503535

504536
private NodeInvokeResponse HandleExecApprovalsGet()
505537
{

tests/OpenClaw.Shared.Tests/SystemRunTests.cs

Lines changed: 125 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,42 @@ public async Task SystemRun_WithPromptPolicy_AllowsOnce_WhenUserApproves()
430430
}
431431
}
432432

433+
[Fact]
434+
public async Task SystemRun_WithPromptPolicy_PromptsOnceForShellWrapper_WhenUserApprovesOnce()
435+
{
436+
var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}");
437+
Directory.CreateDirectory(tempDir);
438+
439+
try
440+
{
441+
var logger = new ExecTestLogger();
442+
var policy = new ExecApprovalPolicy(tempDir, logger);
443+
policy.SetRules(Array.Empty<ExecApprovalRule>(), ExecApprovalAction.Prompt);
444+
var runner = new FakeCommandRunner();
445+
var prompt = new FakePromptHandler(ExecApprovalPromptDecision.AllowOnce());
446+
var cap = new SystemCapability(logger);
447+
cap.SetCommandRunner(runner);
448+
cap.SetApprovalPolicy(policy);
449+
cap.SetPromptHandler(prompt);
450+
451+
var res = await cap.ExecuteAsync(new NodeInvokeRequest
452+
{
453+
Id = "prompt-wrapper-1",
454+
Command = "system.run",
455+
Args = Parse("""{"command":"cmd /c echo hello"}""")
456+
});
457+
458+
Assert.True(res.Ok, res.Error);
459+
Assert.NotNull(runner.LastRequest);
460+
Assert.Equal(1, prompt.CallCount);
461+
Assert.Empty(policy.Rules);
462+
}
463+
finally
464+
{
465+
try { Directory.Delete(tempDir, true); } catch { }
466+
}
467+
}
468+
433469
[Fact]
434470
public async Task SystemRun_WithPromptPolicy_PersistsExactAllowRule_WhenUserAlwaysAllows()
435471
{
@@ -464,6 +500,88 @@ public async Task SystemRun_WithPromptPolicy_PersistsExactAllowRule_WhenUserAlwa
464500
}
465501
}
466502

503+
[Fact]
504+
public async Task SystemRun_WithPromptPolicy_AlwaysAllowWrapperPersistsSingleRule_AndCoversRepeat()
505+
{
506+
var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}");
507+
Directory.CreateDirectory(tempDir);
508+
509+
try
510+
{
511+
var logger = new ExecTestLogger();
512+
var policy = new ExecApprovalPolicy(tempDir, logger);
513+
policy.SetRules(Array.Empty<ExecApprovalRule>(), ExecApprovalAction.Prompt);
514+
var runner = new FakeCommandRunner();
515+
var prompt = new FakePromptHandler(ExecApprovalPromptDecision.AlwaysAllow());
516+
var cap = new SystemCapability(logger);
517+
cap.SetCommandRunner(runner);
518+
cap.SetApprovalPolicy(policy);
519+
cap.SetPromptHandler(prompt);
520+
521+
var req = new NodeInvokeRequest
522+
{
523+
Id = "prompt-wrapper-2",
524+
Command = "system.run",
525+
Args = Parse("""{"command":"cmd /c echo repeat"}""")
526+
};
527+
528+
var first = await cap.ExecuteAsync(req);
529+
Assert.True(first.Ok, first.Error);
530+
Assert.Equal(1, prompt.CallCount);
531+
Assert.Single(policy.Rules);
532+
Assert.Equal("cmd /c echo repeat", policy.Rules[0].Pattern);
533+
534+
var second = await cap.ExecuteAsync(req);
535+
Assert.True(second.Ok, second.Error);
536+
Assert.Equal(1, prompt.CallCount);
537+
}
538+
finally
539+
{
540+
try { Directory.Delete(tempDir, true); } catch { }
541+
}
542+
}
543+
544+
[Fact]
545+
public async Task SystemRun_WithPromptPolicy_StillDeniesExplicitBlockedShellWrapperPayload()
546+
{
547+
var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}");
548+
Directory.CreateDirectory(tempDir);
549+
550+
try
551+
{
552+
var logger = new ExecTestLogger();
553+
var policy = new ExecApprovalPolicy(tempDir, logger);
554+
policy.SetRules(
555+
new[]
556+
{
557+
new ExecApprovalRule { Pattern = "del *", Action = ExecApprovalAction.Deny }
558+
},
559+
ExecApprovalAction.Prompt);
560+
var runner = new FakeCommandRunner();
561+
var prompt = new FakePromptHandler(ExecApprovalPromptDecision.AllowOnce());
562+
var cap = new SystemCapability(logger);
563+
cap.SetCommandRunner(runner);
564+
cap.SetApprovalPolicy(policy);
565+
cap.SetPromptHandler(prompt);
566+
567+
var res = await cap.ExecuteAsync(new NodeInvokeRequest
568+
{
569+
Id = "prompt-wrapper-3",
570+
Command = "system.run",
571+
Args = Parse("""{"command":"cmd /c del C:\\important.txt"}""")
572+
});
573+
574+
Assert.False(res.Ok);
575+
Assert.Contains("denied", res.Error!, StringComparison.OrdinalIgnoreCase);
576+
Assert.Null(runner.LastRequest);
577+
Assert.Equal(1, prompt.CallCount);
578+
}
579+
finally
580+
{
581+
try { Directory.Delete(tempDir, true); } catch { }
582+
}
583+
}
584+
467585
[Fact]
468586
public async Task SystemRun_WithPromptPolicy_Denies_WhenUserDenies()
469587
{
@@ -525,10 +643,15 @@ public FakePromptHandler(ExecApprovalPromptDecision decision)
525643
_decision = decision;
526644
}
527645

646+
public int CallCount { get; private set; }
647+
528648
public Task<ExecApprovalPromptDecision> RequestAsync(
529649
ExecApprovalPromptRequest request,
530-
CancellationToken cancellationToken = default) =>
531-
Task.FromResult(_decision);
650+
CancellationToken cancellationToken = default)
651+
{
652+
CallCount++;
653+
return Task.FromResult(_decision);
654+
}
532655
}
533656
}
534657

0 commit comments

Comments
 (0)