test: Guest helper windows unit tests#28
Conversation
|
Skipping CI for Draft Pull Request. |
kubevirt-prow
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds a containerized Pester test suite for the Windows guest-helper (guest-helpers/windows/filerestore.bat) and refactors the script so its logic is callable/testable from PowerShell while preserving the .bat entrypoint behavior.
Changes:
- Introduce
make test-scripts/make test-scripts-windowsto run Windows guest-helper Pester tests in a container. - Add Pester test harness/module plus a README-style
TESTING.mddescribing how tests work. - Refactor
filerestore.batinto functions (plus wrapper functions for native executables) to enable mocking and unit testing, and add an SSH command validation helper.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds containerized Pester-based script test targets and related variables. |
| guest-helpers/windows/TESTING.md | Documents how to run and understand the Windows guest-helper script tests. |
| guest-helpers/windows/test/TestHelper.psm1 | Adds helpers to extract the PowerShell portion of the polyglot .bat and stub Windows-only cmdlets. |
| guest-helpers/windows/test/filerestore.Tests.ps1 | Adds Pester coverage for argument parsing, disk/partition logic, BitLocker, junction handling, and SSH command restriction behavior. |
| guest-helpers/windows/filerestore.bat | Refactors into functions for testability and adds wrappers to allow Pester mocking. |
| .gitignore | Ignores testResults.xml produced by test runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (-not $env:SSH_ORIGINAL_COMMAND) { return $null } | ||
| if ($env:SSH_ORIGINAL_COMMAND -notmatch '^"?C:\\Program Files\\filerestore\\filerestore\.bat"?(\s|$)') { | ||
| Write-Host "ERROR: Only filerestore.bat commands are allowed" -ForegroundColor Red | ||
| return 'rejected' | ||
| } | ||
| $arguments = $env:SSH_ORIGINAL_COMMAND -replace '^"?C:\\Program Files\\filerestore\\filerestore\.bat"?\s*', '' | ||
| return $arguments |
There was a problem hiding this comment.
I think this is a theorethical concern with close to zero risk. I'd prefer not to change it at this time
ema-aka-young
changed the title
ema-aka-young
changed the title
kubevirt-prow
Bot
added
release-note-none
kubevirt-prow
Bot
removed
the
do-not-merge/work-in-progress
kubevirt-prow
Bot
added
the
needs-rebase
kubevirt-prow
Bot
removed
the
needs-rebase
ema-aka-young
force-pushed
the
guest-helper-windows-tests
branch
from
12e44cf to
394f27c
Compare
| test-scripts: test-scripts-linux ## Run guest-helper script tests | ||
|
|
| function Test-SshCommand { | ||
| if (-not $env:SSH_ORIGINAL_COMMAND) { return $null } | ||
| if ($env:SSH_ORIGINAL_COMMAND -notmatch '^"?C:\\Program Files\\filerestore\\filerestore\.bat"?(\s|$)') { | ||
| Write-Host "ERROR: Only filerestore.bat commands are allowed" -ForegroundColor Red | ||
| return 'rejected' | ||
| } | ||
| $arguments = $env:SSH_ORIGINAL_COMMAND -replace '^"?C:\\Program Files\\filerestore\\filerestore\.bat"?\s*', '' | ||
| return $arguments | ||
| } |
| # Create a junction point to redirect $MountPath to the drive | ||
| if (Test-Path $MountPath) { Remove-Junction -Path $MountPath } | ||
| New-Junction -Path $MountPath -Target "${driveLetter}:\" | ||
| if (-not (Test-Path $MountPath)) { | ||
| Write-Host "ERROR: Failed to create junction from $MountPath to ${driveLetter}:\" | ||
| return 1 | ||
| } |
| # When dot-sourced (e.g. by Pester), load functions but skip main logic | ||
| if ($MyInvocation.InvocationName -eq '.') { return } |
| if ($SourcePath -match '^\\\\') { | ||
| Write-Host "ERROR: UNC paths are not supported for --source-path" | ||
| return 1 | ||
| } | ||
| $RelativePath = $SourcePath -replace '^[A-Za-z]:\\', '' | ||
| $RelativePath = $RelativePath.TrimStart('\') | ||
| $BackupPath = Join-Path $MountPath $RelativePath |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
guest-helpers/windows/filerestore.bat (1)
223-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fail fast on UNC
--source-pathbefore mounting.The UNC rejection (Line 229-232) runs inside the try block, after the disk has already been brought online, unlocked, and junction-mounted. Since this check doesn't depend on the mount at all, validating it during argument parsing would avoid an unnecessary mount/unmount cycle for a request that's guaranteed to fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guest-helpers/windows/filerestore.bat` around lines 223 - 232, Move the UNC --source-path validation out of the try/finally flow in filerestore.bat so it happens during argument parsing or before any disk mount/unlock work begins. The current check inside the automatic mode block still allows the mount lifecycle to run first; update the handling around the SourcePath validation to fail fast with the same error message and return code before any side effects occur.guest-helpers/windows/test/filerestore.Tests.ps1 (1)
329-336: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd cleanup assertion once junction-failure disk cleanup is fixed.
This test only checks
ExitCode -eq 1. Once the missing disk-offline cleanup on junction failure (seefilerestore.batLines 207-213) is addressed, extend this test to assertShould -Invoke Set-Disk -ParameterFilter { $IsOffline -eq $true }so the fix is regression-locked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guest-helpers/windows/test/filerestore.Tests.ps1` around lines 329 - 336, The junction-failure test in filerestore.Tests.ps1 only asserts the nonzero exit code, but it should also cover the disk-offline cleanup path once the filerestore failure handling is fixed. Update the existing `It 'exits 1 when junction creation fails'` case around `Invoke-TestScript`, `New-Junction`, and `Set-Disk` to add an assertion that `Set-Disk` is invoked with `-ParameterFilter { $IsOffline -eq $true }`, so the cleanup behavior is regression-locked alongside the exit code check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@guest-helpers/windows/filerestore.bat`:
- Around line 36-38: The `New-Junction` and `Remove-Junction` wrapper functions
are interpolating `$Path` and `$Target` directly into `cmd /c` commands, which
allows shell metacharacters to break out of the quoted arguments. Update these
helpers to avoid `cmd.exe` entirely by using PowerShell-native junction
operations (for example, `New-Item -ItemType Junction` and `Remove-Item -Force`)
and keep the same wrapper function names so the call sites continue to work
safely.
In `@Makefile`:
- Around line 180-192: The Makefile has two `test-scripts` target definitions,
which leaves redundant and potentially divergent prerequisites. Remove the stale
duplicate `test-scripts: test-scripts-linux` rule and keep the consolidated
`test-scripts` target that depends on both `test-scripts-windows` and
`test-scripts-linux`, using the existing `test-scripts`, `test-scripts-windows`,
and `test-scripts-linux` targets as the reference points.
---
Nitpick comments:
In `@guest-helpers/windows/filerestore.bat`:
- Around line 223-232: Move the UNC --source-path validation out of the
try/finally flow in filerestore.bat so it happens during argument parsing or
before any disk mount/unlock work begins. The current check inside the automatic
mode block still allows the mount lifecycle to run first; update the handling
around the SourcePath validation to fail fast with the same error message and
return code before any side effects occur.
In `@guest-helpers/windows/test/filerestore.Tests.ps1`:
- Around line 329-336: The junction-failure test in filerestore.Tests.ps1 only
asserts the nonzero exit code, but it should also cover the disk-offline cleanup
path once the filerestore failure handling is fixed. Update the existing `It
'exits 1 when junction creation fails'` case around `Invoke-TestScript`,
`New-Junction`, and `Set-Disk` to add an assertion that `Set-Disk` is invoked
with `-ParameterFilter { $IsOffline -eq $true }`, so the cleanup behavior is
regression-locked alongside the exit code check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f9f287b-13fe-497c-95b8-0a047a1b5dd5
📒 Files selected for processing (5)
Makefileguest-helpers/windows/TESTING.mdguest-helpers/windows/filerestore.batguest-helpers/windows/test/TestHelper.psm1guest-helpers/windows/test/filerestore.Tests.ps1
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
ema-aka-young
force-pushed
the
guest-helper-windows-tests
branch
from
394f27c to
fc5acf1
Compare
- Introduced Pester tests for the Windows guest helper script (`filerestore.bat`). - Updated Makefile to include a new target for running Windows tests. - Created `TESTING.md` for Windows guest helper with instructions on running tests. - Refactored `filerestore.bat` to support testing and improve argument parsing. - Added `TestHelper.psm1` for shared functions used in Pester tests. - Implemented comprehensive Pester tests covering various scenarios for the Windows guest helper. Signed-off-by: Emanuele Prella <eprella@redhat.com>
ema-aka-young
force-pushed
the
guest-helper-windows-tests
branch
from
fc5acf1 to
6717f19
Compare
| if ($SourcePath -match '^\\\\') { | ||
| Write-Host "ERROR: UNC paths are not supported for --source-path" | ||
| return 1 | ||
| } | ||
| $RelativePath = $SourcePath -replace '^[A-Za-z]:\\', '' |
| function script:Invoke-TestScript { | ||
| param( | ||
| [string[]]$Arguments = @(), | ||
| [hashtable]$Env = @{} | ||
| ) | ||
| try { | ||
| foreach ($key in $Env.Keys) { | ||
| Set-Item "env:$key" $Env[$key] | ||
| } | ||
| return @{ ExitCode = (Invoke-FileRestore $Arguments) } | ||
| } catch { | ||
| throw "Invoke-FileRestore threw unexpectedly: $_" | ||
| } finally { | ||
| foreach ($key in $Env.Keys) { | ||
| Remove-Item "env:$key" -ErrorAction SilentlyContinue | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
guest-helpers/windows/filerestore.bat (1)
269-284: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftCritical: SSH forced-command bypass via
cmd /cre-injection ofSSH_ORIGINAL_COMMAND.
Test-SshCommandonly validates that the command starts with the fixedfilerestore.batpath (Line 82); everything after that prefix is returned unmodified as$sshResult. That raw, attacker-supplied string is then re-interpolated intocmd /c "<path> $sshResult"(Line 279). Sincecmd /cparses the entire resulting line, any shell metacharacters in$sshResult(&,|,&&,>, etc.) execute as additional commands — e.g. an SSH client sending"...\filerestore.bat" restore & calc.exesails through the prefix check and then runscalc.exe(or any attacker payload) via the reinvokedcmd /c. This completely defeats the SSH forced-command restriction and grants arbitrary command execution as the sshd service account — a strictly worse outcome than the previously-discussed (and accepted) junction-wrapper interpolation risk, since it requires no--mount-pathcontrol at all.Avoid re-invoking through
cmd /cwith a reconstructed string entirely. Since the functions are already loaded in this same process, tokenize$sshResult(respecting quotes, without shell-operator interpretation) and callInvoke-FileRestoredirectly instead of shelling out:🔒 Proposed fix — avoid cmd.exe reinterpretation
if ($null -ne $sshResult) { $env:SSH_ORIGINAL_COMMAND = $null - cmd /c "`"C:\Program Files\filerestore\filerestore.bat`" $sshResult" - exit $LASTEXITCODE + # Tokenize like a native command line (quote-aware) without invoking a + # shell, so `&`, `|`, etc. in SSH_ORIGINAL_COMMAND cannot be interpreted + # as command separators. + $parsedArgs = [regex]::Matches($sshResult, '"[^"]*"|\S+') | + ForEach-Object { $_.Value.Trim('"') } + exit (Invoke-FileRestore $parsedArgs) }Please verify the tokenization approach against real-world
--mount-path/--source-pathvalues that may contain quoted spaces before landing this fix, since it changes how the SSH forced-command path parses arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guest-helpers/windows/filerestore.bat` around lines 269 - 284, The SSH forced-command path in Test-SshCommand and the entry-point block is vulnerable because $sshResult is re-injected into cmd /c, allowing shell metacharacters to be executed as extra commands. Remove the cmd /c round-trip in the entry point and dispatch directly to Invoke-FileRestore after safely tokenizing $sshResult as arguments, preserving quoted paths without letting cmd.exe reinterpret them. Keep the Test-SshCommand prefix check, but ensure the main flow uses the parsed argument list rather than a reconstructed command string.
🧹 Nitpick comments (2)
Makefile (1)
180-191: 🧹 Nitpick | 🔵 TrivialNetwork dependency on every CI run.
test-scripts-windowscallsInstall-Module Pester -RequiredVersion $(PESTER_VERSION) ...on every invocation, so CI reliability now depends on PowerShell Gallery availability with no local caching/fallback. Consider caching the module (e.g., a persisted volume or pre-baked image) if this test target ends up running frequently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 180 - 191, The test-scripts-windows target currently installs Pester inside the Invoke-Pester container on every run, creating an unnecessary network dependency. Update the Makefile target so it reuses a cached or preinstalled Pester module instead of always calling Install-Module, for example by using a persisted volume or a pre-baked $(PWSH_IMAGE); keep the change localized to test-scripts-windows and related test-scripts behavior.guest-helpers/windows/test/filerestore.Tests.ps1 (1)
1-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gap: SSH dispatch/reinvocation entry point is untested.
All tests call
Test-SshCommandorInvoke-FileRestoredirectly; none exercise the actual entry-point block infilerestore.bat(Lines 269-284) that reinvokes the script viacmd /cwith the extracted SSH arguments. That's exactly the code path flagged as a critical command-injection risk infilerestore.bat— once fixed, it would be valuable to add a test (e.g., mocking the dispatch call) asserting that shell metacharacters inSSH_ORIGINAL_COMMANDare not given special interpretation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guest-helpers/windows/test/filerestore.Tests.ps1` around lines 1 - 89, The SSH dispatch entry point is not covered by these tests, so the reinvocation path can still mishandle injected shell metacharacters. Add a Pester test around the actual entry-point logic in filerestore.bat by invoking the script through its SSH command path and mocking the cmd /c reinvocation, then assert the extracted arguments are passed through as plain data and not reinterpreted. Use the existing Test-SshCommand and Invoke-FileRestore helpers as the basis, but target the top-level dispatch block that calls cmd /c.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@guest-helpers/windows/filerestore.bat`:
- Around line 269-284: The SSH forced-command path in Test-SshCommand and the
entry-point block is vulnerable because $sshResult is re-injected into cmd /c,
allowing shell metacharacters to be executed as extra commands. Remove the cmd
/c round-trip in the entry point and dispatch directly to Invoke-FileRestore
after safely tokenizing $sshResult as arguments, preserving quoted paths without
letting cmd.exe reinterpret them. Keep the Test-SshCommand prefix check, but
ensure the main flow uses the parsed argument list rather than a reconstructed
command string.
---
Nitpick comments:
In `@guest-helpers/windows/test/filerestore.Tests.ps1`:
- Around line 1-89: The SSH dispatch entry point is not covered by these tests,
so the reinvocation path can still mishandle injected shell metacharacters. Add
a Pester test around the actual entry-point logic in filerestore.bat by invoking
the script through its SSH command path and mocking the cmd /c reinvocation,
then assert the extracted arguments are passed through as plain data and not
reinterpreted. Use the existing Test-SshCommand and Invoke-FileRestore helpers
as the basis, but target the top-level dispatch block that calls cmd /c.
In `@Makefile`:
- Around line 180-191: The test-scripts-windows target currently installs Pester
inside the Invoke-Pester container on every run, creating an unnecessary network
dependency. Update the Makefile target so it reuses a cached or preinstalled
Pester module instead of always calling Install-Module, for example by using a
persisted volume or a pre-baked $(PWSH_IMAGE); keep the change localized to
test-scripts-windows and related test-scripts behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 164063e0-4a3b-4179-8316-a8242986ea24
📒 Files selected for processing (6)
Makefileguest-helpers/linux/TESTING.mdguest-helpers/windows/TESTING.mdguest-helpers/windows/filerestore.batguest-helpers/windows/test/TestHelper.psm1guest-helpers/windows/test/filerestore.Tests.ps1
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
✅ Files skipped from review due to trivial changes (1)
- guest-helpers/linux/TESTING.md
What this PR does / why we need it:
Add Pester unit tests for the Windows guest helper script (filerestore.bat) and refactor the script to make it testable.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
The script was restructured from top-level procedural code into functions to enable unit testing. The runtime behavior is unchanged: the entrypoint at the bottom of the file calls the same logic in the same order.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: