Skip to content

Improve PowerShell execution-region approvals - #1857

Merged
Aaronontheweb merged 1 commit into
devfrom
test/sst-v03-acceptance-matrix
Aug 10, 2026
Merged

Improve PowerShell execution-region approvals#1857
Aaronontheweb merged 1 commit into
devfrom
test/sst-v03-acceptance-matrix

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Collaborator

Summary

  • add the missing Bash substitution and PowerShell subexpression or execution-region cases to the reviewed approval matrix
  • account for an opaque PowerShell script-block host argument only when ShellSyntaxTree exposes the exact known command-owned region through complete body occurrences
  • require the host and every body command to match approval policy independently
  • keep empty bodies, unknown receivers, incomplete occurrences, and unknown or future region metadata strict
  • synchronize the positive and fail-closed behavior into the canonical tool-approval OpenSpec

Why

ShellSyntaxTree deliberately represents a known PowerShell callback script block twice: the host clause retains the opaque authored argument, while the executable body is projected into command occurrences. Netclaw was treating the opaque host argument as unresolved even after it had independently analyzed every body occurrence. That prevented valid stored grants from being reused.

The correction is narrow. It correlates the exact parser-owned ClauseElement by reference, exempts only its joined DynamicSkip argument, and requires a complete descendant occurrence with known execution-region facts. Body commands remain separate approval units.

Evidence

The reviewed matrix grows from 225 to 244 cases:

  • Bash: 193 to 199
  • PowerShell 7: 23 to 36
  • Windows PowerShell 5.1: unchanged at 5

The new PowerShell cases prove that both grants are required:

  • host and body grants: allowed
  • host grant only: Remove-Item still requires approval
  • body grant only: ForEach-Object still requires approval
  • unknown receiver: remains complex and cannot reuse either grant

Validation

  • Release build: 0 warnings, 0 errors
  • full solution tests before the content-preserving rebase: 6,793 passed, 15 expected skips, 0 failed
  • post-rebase Netclaw.Security.Tests: 815 passed
  • post-rebase approval matrix: 244 passed
  • strict OpenSpec validation: 77 passed, 0 failed
  • Slopwatch strict analysis: 0 issues
  • copyright headers, scoped formatting, and git diff --check: passed
  • three adversarial review passes: all findings corrected; final disposition has no blockers
  • patch ID is unchanged across the rebase onto the current Migrate shell policy to ShellSyntaxTree alpha.6 #1855 head

Delivery sequence

This PR is intentionally stacked on #1855 so its CI can run while the parent native smoke jobs finish. It must be retargeted to dev after #1855 merges; auto-merge will be enabled only after that retargeting.

@Aaronontheweb
Aaronontheweb changed the base branch from feat/sst-alpha6-consumer to dev August 10, 2026 20:02
@Aaronontheweb

Copy link
Copy Markdown
Collaborator Author

Delivery note: I retargeted this PR to dev before #1855 merged because the required workflows only run for dev-targeted pull requests. Auto-merge remains disabled. The head contains #1855 current content, so CI tests the intended combined tree; after #1855 merges, the visible diff will collapse to the single reviewed matrix commit.

Comment on lines +373 to +384
foreach (var frame in command.Ancestry)
{
if (frame is
{
Region: CommandAncestryRegion.ExecutionRegion,
Ancestor: ExecutionRegionSyntax region
}
&& IsKnownCommandArgumentRegion(region))
{
arguments.Add(region.HostArgument!);
}
}
@Aaronontheweb
Aaronontheweb force-pushed the test/sst-v03-acceptance-matrix branch from e0f57ac to dab95ac Compare August 10, 2026 20:59
@Aaronontheweb
Aaronontheweb enabled auto-merge (squash) August 10, 2026 20:59
@Aaronontheweb

Copy link
Copy Markdown
Collaborator Author

Cross-platform snapshot follow-up: the first macOS and Windows runs exposed Verify built-in temp-path scrubbing, not a policy difference. The authored Bash case intentionally contains /tmp; Linux normalized it to {TempPath}, while the other hosts preserved it. The final fix scopes VerifySettings.DisableScrubbers() to this deterministic review-table snapshot and records the literal command exactly. A broader string replacement was rejected during adversarial review because it could hide future command changes. On the rebased current head, Netclaw.Security.Tests passes 815/815 and the full shell approval matrix passes 244/244.

@Aaronontheweb
Aaronontheweb force-pushed the test/sst-v03-acceptance-matrix branch from dab95ac to 02e93b0 Compare August 10, 2026 22:50
@Aaronontheweb
Aaronontheweb merged commit b5de8d4 into dev Aug 10, 2026
29 of 30 checks passed
@Aaronontheweb
Aaronontheweb deleted the test/sst-v03-acceptance-matrix branch August 10, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant