Guard the proxy key contract against hand-written expectations - #926
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes contradictory FTP proxy expectations while preserving hostile-environment coverage.
Changes:
- Corrects
FTP_PROXYexpectations. - Adds constants-driven managed-key coverage.
- Extends WSLc proxy E2E checks to both FTP key casings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/core/wxc_common/tests/proxy_env_spec.rs |
Corrects and strengthens proxy environment tests. |
tests/configs/wslc_network_proxy.json |
Adds hostile FTP proxy inputs and output diagnostics. |
tests/scripts/run_wslc_proxy_test.ps1 |
Verifies both FTP proxy variables are replaced. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`cooperative_env_scrubs_all_caller_supplied_proxy_keys` asserted FTP_PROXY was absent while a sibling test asserted every PROXY_SET_KEYS entry was present at the proxy URL. microsoft#923 corrected the assertion; nothing yet stops the next hand-written per-key expectation from drifting the same way. `every_managed_key_lands_in_the_state_its_constants_dictate` derives the expected state of all ten managed keys from PROXY_SET_KEYS and PROXY_NEUTRALIZE_KEYS rather than naming keys by hand, under hostile caller input for every key. No existing test covered that combination: the scrub test supplies hostile values but checks three keys, and the set test checks every key but starts from an empty caller environment. Extends the WSLc proxy e2e to the FTP family for the same reason. The assertion is case-sensitive because PowerShell `-notmatch` is not, which would let the upper-case entry satisfy both checks and leave a lower-case attacker value untested. Refs microsoft#922 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: acf3853b-fbe3-41ab-870a-9274a698cc0c
b9eba2d to
8c8a66d
Compare
wslc_readonly_mount.json cats /mnt/c/workspace/test.txt, and nothing on the WSLc path ever wrote that file, so the test failed on a missing fixture rather than on a mount defect -- the write half was correctly blocked. The LXC and bubblewrap suites already seed the same fixture before their equivalent test (run_lxc_filesystem_test.sh:21, run_bwrap_filesystem_test.sh:25). WSLc was the odd one out. Full suite now 23/23 passed with 2 documented tar skips, and the state-aware suite 57/57, on WSL 2.9.4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: acf3853b-fbe3-41ab-870a-9274a698cc0c
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/scripts/run_wslc_all_tests.ps1:229
- The fixture now makes the command reach its write probe, but the harness still checks only
Read succeeded. If the readonly mount is accidentally writable, the command emitsWrite succeeded (unexpected)and this suite reports PASS. Require the blocked-write marker so the setup actually detects a readonly-mount defect.
Set-Content -Path $readonlyFixture -Value "test content" -Encoding ascii
| $readonlyFixtureDir = "C:\workspace" | ||
| $readonlyFixture = Join-Path $readonlyFixtureDir "test.txt" | ||
| if (-not (Test-Path $readonlyFixtureDir)) | ||
| { | ||
| $null = New-Item -ItemType Directory -Path $readonlyFixtureDir -Force | ||
| } | ||
| Set-Content -Path $readonlyFixture -Value "test content" -Encoding ascii |
There was a problem hiding this comment.
is this a good suggestion? Darren Hoehna (@dhoehna)?
There was a problem hiding this comment.
That is a good idea. I'll get that in
There was a problem hiding this comment.
Done in 4606b6f.
To answer the question about which scripts those are, since it was not obvious: every WSLc script in this suite that owns an on-disk fixture already uses a fixed, purpose-named root plus try/finally cleanup, and run_wslc_all_tests.ps1 was the only one that did not. They are run_wslc_denied_masking_test.ps1:50-61,107-108 (C:\wslcmask), run_wslc_object_test.ps1:49-58,87-88 (C:\objtest), run_wslc_most_specific_test.ps1:48-57,85-86 (C:\wslcmsp), run_wslc_dotdot_alias_test.ps1:53-62,94-95 (C:\ddttest), and run_wslc_state_aware_tests.ps1:550-552,627-633 (C:\mxc_wslc_sa_test). I followed the masking one, which is the closest match since it seeds files with Set-Content too.
The fixture now lives in C:\wslcro, created and removed by a Remove-ReadonlyFixture helper with a pre-clean before the try and cleanup in the finally, so it also goes away on the failure path. C:\workspace was a bad place for it specifically because the prerequisites at lines 26-33 tell the developer to put alpine.tar and alpine-docker-save.tar there, so it is a directory holding real files.
wslc_readonly_mount.json moved with it, both readonlyPaths and the two /mnt/c/... paths in commandLine. The guest path is derived from the host path by windows_path_to_container_path (src/backends/wslc/common/src/policy_mapping.rs:39-65), so C:\wslcro maps to exactly /mnt/c/wslcro. Cleanup now also removes readonly_write_test.txt, the probe file a wrongly-writable mount would leave behind.
One decision worth flagging rather than hiding: moving the fixture also dropped the New-Item that created C:\workspace, because hosting test.txt was its only job. wslc_filesystem.json still mounts C:\workspace and again relies on the developer providing it, which is what it does on main and what the prerequisites already state. If you would rather the suite create that directory itself, say so and I will put it back as its own line with its own reason.
There was a problem hiding this comment.
Follow-up in f5301bb, which finishes the job properly.
My first pass moved only the readonly fixture and left wslc_filesystem.json mounting C:\workspace. That was wrong, and the question that exposed it was "why does a developer have to supply something for a test?" Nothing in the repo creates C:\workspace -- setup-wslc.ps1 does not -- so wslc_filesystem.json passed only on a machine where someone had already made that directory by hand for the tar prerequisites. On a clean machine it fails with FAIL: /mnt/c/workspace not visible. That was true on main too; it was just latent.
Both filesystem configs now mount roots the harness owns outright, C:\wslcfs and C:\wslcro, created and removed in one try/finally. The suite supplies its own mount targets and touches C:\workspace not at all.
The one thing that stays a developer prerequisite is C:\workspace\alpine.tar for the tar-import tests, because it comes out of docker export and the harness cannot produce it. Those already skip cleanly when it is absent (run_wslc_all_tests.ps1:117-124), which is the right behavior for a prerequisite that genuinely cannot be automated.
Re-ran both configs from a clean state with neither root present: wslc_filesystem.json printed all three PASS lines and exited 0, wslc_readonly_mount.json read test content from /mnt/c/wslcro/test.txt and had its write probe refused with Read-only file system, and both roots were gone afterwards.
The suite seeded C:\workspace\test.txt, which is the directory this script's own prerequisites tell developers to fill with alpine.tar, so a run overwrote a real file and left it overwritten. The fixture now lives in C:\wslcro, created and removed by the harness in try/finally the way the other WSLc fixture scripts already do (run_wslc_denied_masking_test.ps1:56-61,107-108, run_wslc_object_test.ps1:53-58,87-88). Cleanup also removes the probe file a wrongly-writable mount would leave behind. wslc_readonly_mount.json moves with it: readonlyPaths and both guest paths in commandLine, which windows_path_to_container_path derives as /mnt/c/wslcro (policy_mapping.rs:39-65). Dropping the fixture from C:\workspace also drops the New-Item that created it, whose only job was to host test.txt. wslc_filesystem.json still mounts C:\workspace and again relies on the developer providing it, as it does on main and as the header prerequisites state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2ffc3254-b392-4298-a0f9-dbdf1d36fa55
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/scripts/run_wslc_all_tests.ps1:243
- This still passes when the readonly mount is writable: both the successful write branch and the
||blocked-write branch exit 0, while this assertion checks only the earlier read marker. Assert the blocked-write marker as well so the fixture test detects the mount defect it is intended to cover.
$null = $results.Add((Run-WslcTest "wslc_readonly_mount.json" -OutputContains "Read succeeded"))
wslc_filesystem.json mounted C:\workspace read-write and used it as cwd, and nothing in the repo ever creates that directory, so the test passed only on a machine where the developer had already made it by hand for the tar prerequisites. On a clean machine it failed with "FAIL: /mnt/c/workspace not visible". It now mounts C:\wslcfs, which the harness creates and removes in the same try/finally as the readonly fixture, so the suite supplies its own mount targets. Verified from a clean state with neither root present: wslc_filesystem.json printed all three PASS lines and exited 0, wslc_readonly_mount.json read its fixture and had its write probe refused with "Read-only file system", and both roots were gone afterwards. The tar imports still read C:\workspace\alpine.tar, which is a genuine developer prerequisite the harness cannot produce, and those tests already skip when it is absent (run_wslc_all_tests.ps1:117-124). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2ffc3254-b392-4298-a0f9-dbdf1d36fa55
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/configs/wslc_readonly_mount.json:6
- A writable “readonly” mount still passes this test: when the redirect succeeds, the
&&chain printsWrite succeeded (unexpected)and exits 0, while the harness only requires exit 0 plusRead succeeded. Make the successful-write branch fail so this config actually detects a mount defect.
"commandLine": "cat /mnt/c/wslcro/test.txt && echo 'Read succeeded' && echo 'write test' > /mnt/c/wslcro/readonly_write_test.txt && echo 'Write succeeded (unexpected)' || echo 'Write blocked (expected for readonly)'"
Problem
cooperative_env_scrubs_all_caller_supplied_proxy_keysassertedFTP_PROXYwas absent from the result while
cooperative_env_sets_every_set_key_to_the_proxy_urlasserted every
PROXY_SET_KEYSentry was present at the proxy URL. Bothcannot hold. #923 corrected the assertion, so
mainis green — but nothingprevents the next hand-written per-key expectation from drifting away from
the constant that drives the behavior in exactly the same way.
What this adds
every_managed_key_lands_in_the_state_its_constants_dictatederives theexpected post-state of all ten managed keys from
PROXY_SET_KEYSandPROXY_NEUTRALIZE_KEYSrather than naming keys by hand, and it does sounder hostile caller input for every key.
No existing test covered that combination. The scrub test supplies hostile
values but checks three keys; the set test checks every key but starts from
an empty caller environment.
The WSLc proxy e2e gains the FTP family for the same reason. The assertion
is case-sensitive (
-cnotmatch) because PowerShell-notmatchis not,which would let the upper-case entry satisfy both checks and leave a
lower-case attacker value untested.
Scope
In:
proxy_env_spec.rs(additive only),wslc_network_proxy.json,run_wslc_proxy_test.ps1,run_wslc_all_tests.ps1,wslc_readonly_mount.json,wslc_filesystem.json. Out:proxy_env.rs— thebehavior is correct and is not touched.
Also makes the WSLc filesystem tests supply their own fixtures. Both configs
mounted
C:\workspace, which nothing in the repo creates, andwslc_readonly_mount.jsoncat atest.txtthat nothing wrote. So thereadonly test failed on a missing fixture rather than on a mount defect, and
the filesystem test passed only on a machine where the developer had already
made
C:\workspaceby hand for the tar prerequisites; on a clean machine itfailed with
FAIL: /mnt/c/workspace not visible.They now mount
C:\wslcfsandC:\wslcro, which the harness creates andremoves in
try/finallythe way every other WSLc fixture script does(
run_wslc_denied_masking_test.ps1:50-61,107-108).windows_path_to_container_pathderives the guest paths from the host roots(
policy_mapping.rs:39-65). Cleanup also removes the probe file awrongly-writable readonly mount would leave behind.
C:\workspaceis where the prerequisites tell the developer to keepalpine.tar, so the suite no longer writes to it or creates it. The tarimports still read it, which is a genuine prerequisite the harness cannot
produce, and those tests already skip when it is absent
(
run_wslc_all_tests.ps1:117-124).The redundant half of this PR was dropped. #923 landed the same assertion
fix an hour earlier and asserts both
FTP_PROXYandftp_proxywhere thisbranch asserted only the upper-case one, so this now sits on top of #923
rather than competing with it.
Verification
wxc_common786 passed / 0 failed on Windows, 706 on Linux under WSL.proxy_env.rsfails it atproxy_env_spec.rs:221. Reverted;proxy_env.rsis absent from the diff.
cargo clippy -p wxc_common --all-targets -- -D warningsclean.against
api.github.com/zenand the diff touches no LXC file.The new FTP assertion ran against a live container and the attacker-supplied
FTP_PROXY/ftp_proxyvalues were scrubbed and replaced with the proxyURL. This required upgrading the host to WSL 2.9.4; the suite could not run
at all before.
were re-run afterwards from a clean state with neither root present.
wslc_filesystem.jsonprinted all three PASS lines and exited 0.wslc_readonly_mount.jsonreadtest contentfrom/mnt/c/wslcro/test.txt,printed
Read succeeded, had its write probe refused withRead-only file system, and exited 0. Both roots were gone afterwards andC:\workspacewas untouched.