Refuse to run the destructive ps1 test runner outside a sandbox#343
Conversation
tests/run_all_tests.ps1 kills all psmux processes and wipes ~/.psmux (*.port, *.key, .psmux.conf, .psmuxrc) between tests. Run on a machine with a live psmux it silently destroys the user's running sessions and config. Gate it behind PSMUX_TEST_SANDBOX=1: the runner refuses (exit 2) with a clear explanation unless that opt-in is set. The Docker dev image sets the variable so the intended throwaway environment runs unchanged. Adds tests/test_runner_safety_gate.ps1, a static guard (it reads the runner, never executes it — safe anywhere, no binary needed) asserting the gate exists and appears before every destructive operation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
585187e to
9690208
Compare
|
I noticed I used the wrong committer email, so I rewrote under the proper identity. |
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in sandbox guard to the destructive PowerShell test runner to reduce the risk of wiping a contributor’s live psmux state when running tests/run_all_tests.ps1.
Changes:
- Adds
PSMUX_TEST_SANDBOX=1refusal logic totests/run_all_tests.ps1. - Sets
PSMUX_TEST_SANDBOXin the Docker dev image. - Adds a static regression guard for the runner safety gate.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/run_all_tests.ps1 |
Adds the sandbox opt-in check before runner setup. |
tests/test_runner_safety_gate.ps1 |
Adds static assertions that the safety gate exists and precedes destructive operations. |
docker/Dockerfile |
Marks the dev container as a sandbox so the gated runner still works there. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tarikguney
left a comment
There was a problem hiding this comment.
Thanks for the PR. Can you please review the comments and make appropriate changes.
|
Sure, I'll adjust accordingly. I intentionally omitted adding anything to the smoke test as it was not clear that this particular check merited being included, but if you want it there, I'll add it! |
The static guard located the gate via the first textual occurrence of PSMUX_TEST_SANDBOX, which is the explanatory comment rather than the executable `if ($env:PSMUX_TEST_SANDBOX ...)` check. That made the guard satisfiable by a comment naming the variable placed before destructive code, even if the real gate sat below it. Anchor on the if-statement instead so the position check reflects the actual abort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
run_all_tests.ps1 calls `& $PSMUX kill-server` in Clean-Server before the Stop-Process cleanup, but the static guard did not list kill-server as a destructive operation. A regression that moved the sandbox gate below the kill-server call would have passed unnoticed. Add kill-server to the blacklist so the position check covers it too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The guard only recognised Remove-Item calls whose literal target was "$env:USERPROFILE\.psmux...". Delete paths are commonly built from other $env: vars, so a target-specific pattern would not see them as destructive and a pre-gate delete could slip through. Match Remove-Item broadly instead; almost nothing runs before the gate, so flagging any pre-gate delete is the safe default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
[regex]::Match defaults to case-sensitive, but PowerShell keywords, $env: lookups and cmdlet/command names are case-insensitive. Differently cased code such as `remove-item` or `STOP-PROCESS` would therefore have gone unnoticed by the guard. Pass RegexOptions::IgnoreCase to both the gate-anchor match and the destructive-operation matches so casing cannot change the verdict. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The static guard lived as a standalone script that no automated path invoked, so a removed or moved sandbox gate in run_all_tests.ps1 would not be caught on a PR. CI already runs test_smoke_pr.ps1, so call the guard from there: it only reads the runner (no binary or session), runs before the session work, and a non-zero exit is reported as a smoke failure. Per review discussion, wiring it into the smoke suite keeps the check on the existing automated path and gives it local coverage too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
While addressing the review findings, I also tightened the static guard in two ways beyond the flagged points:
I verified each by temporarily mutating run_all_tests.ps1 to confirm the guard goes red for the right reason. |
|
Just FYI, all tests won't work only inside a sandbox. It requires a Windows Virtual machine for serious testing so we cannot force and assume docker is the sandbox. However the env variable is fine but just that the definition of a Sandbox is not a container but could be a Windows VM or machine to run it. |
This PR is an initial attempt to mitigate #342. Destructive tests are gated behind an environment variable.
tests/run_all_tests.ps1 kills all psmux processes and wipes ~/.psmux (*.port, *.key, .psmux.conf, .psmuxrc) between tests. Run by an unsuspecting user on a machine with a live psmux it silently destroys the user's running sessions and config.
Gate it behind PSMUX_TEST_SANDBOX=1: the runner refuses (exit 2) with a clear explanation unless that opt-in is set. The Docker dev image sets the variable so the intended throwaway environment runs unchanged.
Adds tests/test_runner_safety_gate.ps1, a static guard (it reads the runner, never executes it — safe anywhere, no binary needed) asserting the gate exists and appears before a blacklist of destructive operations.