diff --git a/.github/instructions/ci-copilot-pipeline-security.instructions.md b/.github/instructions/ci-copilot-pipeline-security.instructions.md index b77f09904152..8323345dc04a 100644 --- a/.github/instructions/ci-copilot-pipeline-security.instructions.md +++ b/.github/instructions/ci-copilot-pipeline-security.instructions.md @@ -25,9 +25,13 @@ Once the PR is merged into the worktree, the author controls every `.csproj`, `D 5. **Cross-phase signal files in `$(Agent.TempDirectory)`** (or `$TRUSTED`), never `$RepoRoot/...`. PR code can overwrite anything in the worktree, including a gate verdict. Readers must not silently fall back to a worktree path if the trusted one is missing. -6. **Strip `##vso[...]` from PR-controlled stdout.** Pipe through `tr -d '\r' | sed -E 's/##vso\[[^]]*\]//g'` — bare `sed` misses CRLF lines and the agent will execute the directive. +6. **Strip `##vso[...]` from PR-controlled stdout.** Pipe through `Out-SafePRSubprocessLine` (in `.github/scripts/shared/Write-SafeSubprocessOutput.ps1`), or for sites that need to preserve `-ForegroundColor` formatting, sanitize inline via `-replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1'`. Both forms apply identical non-anchored, case-insensitive substitution that defangs the prefix while keeping the payload visible. Bare `sed`/anchored-regex approaches MISS mid-line `##vso[` (the AzDO parser uses `IndexOf`, not `StartsWith`). The Pester suite enforces this via two AST audits of `Review-PR.ps1`: an inventory check that every `Write-Host`/`Write-Output`/`Write-Information`/`Out-Host`/`Out-Default` of a PR-derived variable is sanitizer-wrapped, and a count floor that catches wholesale deletion. See `.github/scripts/shared/Write-SafeSubprocessOutput.Tests.ps1`. -7. **`gh-aw` workflows.** Pin compiler version (≥ v0.68.4 strips `pull-requests: write` per `gh-aw#28767`). Regenerate `.lock.yml` with `gh aw compile` in the **same commit** as any `.md` frontmatter edit (stale lock ⇒ all dispatches fail). `workflow_dispatch` triggers must restore trusted `.github/` from main (see `Checkout-GhAwPr.ps1`). +7. **Launch PR-controlled `.ps1` scripts as `pwsh -NoProfile -File` SUBPROCESSES, never in-process `& $script`.** `BuildAndRunHostApp.ps1`, `Run-DeviceTests.ps1`, `Find-RegressionRisks.ps1`, etc. emit `Write-Host` of attacker-controlled content (device logcat, git-diff paths, gh-API response fields). In-process `& $script ... 2>&1` does NOT capture `Write-Host` — it writes to stream 6 (Information) which `2>&1` does not redirect. The child's `Write-Host` reaches the parent's host directly, bypassing the sanitizer. A subprocess child has its OWN host, so its `Write-Host` flows through its stdout and is captured by `2>&1` for sanitization. An AST regression guard in `Write-SafeSubprocessOutput.Tests.ps1` AST-walks `Review-PR.ps1` and `Invoke-UITestWithRetry.ps1` and fails on any in-process launch of a tracked runner script. Allowed exceptions: `& $script *>&1` (full-stream merge captures `Write-Host` too) and dispatched-via-`pwsh -File` forms. + +8. **Pester suite gates every change to `.github/scripts/**`.** `.github/workflows/security-scripts-pester.yml` runs the full suite on every PR touching the security surface. It enforces a minimum passed-count floor (catches accidental test deletion) and 0-skipped-tolerance (catches BeforeAll discovery errors). The `Review-PR.Phase-Audit.Tests.ps1` suite is the heart of this: it walks the closure of Gate-reachable code from `$runGate` in `Review-PR.ps1`, dot-sources every helper, and proves no write-class `gh` invocation can land in Gate. Make this workflow a REQUIRED check via repo settings → branch protection. + +9. **`gh-aw` workflows.** Pin compiler version (≥ v0.68.4 strips `pull-requests: write` per `gh-aw#28767`). Regenerate `.lock.yml` with `gh aw compile` in the **same commit** as any `.md` frontmatter edit (stale lock ⇒ all dispatches fail). `workflow_dispatch` triggers must restore trusted `.github/` from main (see `Checkout-GhAwPr.ps1`). 8. **No token republish.** Don't `setvariable` a token (visible to every later task, even with `issecret=true`). Don't write tokens to worktree files. Don't echo token names. @@ -49,4 +53,6 @@ git grep -nE 'Join-Path \$RepoRoot ".*\.(ps1|sh)"' .github/scripts .github/skill git grep -nA1 'checkout: self' eng/pipelines/ci-copilot.yml | grep -v persistCredentials git grep -nE 'Set-Content.*\$RepoRoot.*(gate-result|sentinel|verdict)' .github/scripts .github/skills git grep -nE 'sed.*##vso' eng/pipelines/ci-copilot.yml | grep -v 'tr -d' +# F1-CRIT subprocess wrap: any in-process `& $var.ps1` of a runner that emits attacker-controlled Write-Host +git grep -nE '& \$(uiTestRunner|deviceTestRunner|buildScript|regressionScript)\b' .github/scripts | grep -v 'pwsh -' ``` diff --git a/.github/scripts/Remove-StaleMauiBotComments.Tests.ps1 b/.github/scripts/Remove-StaleMauiBotComments.Tests.ps1 index d46d4e13109e..20a308f6164c 100644 --- a/.github/scripts/Remove-StaleMauiBotComments.Tests.ps1 +++ b/.github/scripts/Remove-StaleMauiBotComments.Tests.ps1 @@ -74,3 +74,97 @@ Describe 'Test-ShouldPreserveMauiBotArtifact' { Should -BeFalse } } + +Describe 'Hide-StaleMauiBotIssueComments — F3 author check on marker path' { + BeforeAll { + $script:hiddenIds = New-Object System.Collections.Generic.List[int] + Mock Invoke-GitHubMinimizeComment { + param($SubjectNodeId, $Classifier, $Reason, $DryRun) + if ($Reason -match 'comment (\d+)$') { + $script:hiddenIds.Add([int]$Matches[1]) + } + } + } + + BeforeEach { + $script:hiddenIds.Clear() + } + + It 'hides bot-authored comments that contain the AI Summary marker' { + Mock Get-GitHubIssueComments { + @( + [pscustomobject]@{ + id = 100; node_id = 'IC_bot' + user = [pscustomobject]@{ login = 'maui-bot' } + body = "`nbot summary" + } + ) + } + Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeAISummary -DryRun + $script:hiddenIds | Should -Contain 100 + } + + It 'does NOT hide a non-bot comment quoting the AI Summary marker (F3 fix)' { + Mock Get-GitHubIssueComments { + @( + [pscustomobject]@{ + id = 200; node_id = 'IC_user' + user = [pscustomobject]@{ login = 'contributor' } + body = 'Quoting `` for context.' + } + ) + } + Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeAISummary -DryRun + $script:hiddenIds | Should -Not -Contain 200 + $script:hiddenIds.Count | Should -Be 0 + } + + It 'does NOT hide a non-bot comment quoting the AI Gate legacy marker (F3 fix)' { + Mock Get-GitHubIssueComments { + @( + [pscustomobject]@{ + id = 300; node_id = 'IC_user2' + user = [pscustomobject]@{ login = 'someone-else' } + body = 'Reference: `` used to mark gate output.' + } + ) + } + Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeLegacyGate -DryRun + $script:hiddenIds | Should -Not -Contain 300 + $script:hiddenIds.Count | Should -Be 0 + } + + It 'still hides a bot comment that contains the AI Gate marker' { + Mock Get-GitHubIssueComments { + @( + [pscustomobject]@{ + id = 400; node_id = 'IC_bot2' + user = [pscustomobject]@{ login = 'github-actions[bot]' } + body = "`nGate output" + } + ) + } + Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeLegacyGate -DryRun + $script:hiddenIds | Should -Contain 400 + } + + It 'mixed batch: hides bot, leaves contributor with same marker' { + Mock Get-GitHubIssueComments { + @( + [pscustomobject]@{ + id = 1; node_id = 'IC_a' + user = [pscustomobject]@{ login = 'maui-bot' } + body = "`nbot" + }, + [pscustomobject]@{ + id = 2; node_id = 'IC_b' + user = [pscustomobject]@{ login = 'attacker' } + body = "`nuser quote" + } + ) + } + Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeAISummary -DryRun + $script:hiddenIds | Should -Contain 1 + $script:hiddenIds | Should -Not -Contain 2 + } +} diff --git a/.github/scripts/Review-PR.Tests.ps1 b/.github/scripts/Review-PR.Tests.ps1 index d4a4f50ebc3c..47e60435b591 100644 --- a/.github/scripts/Review-PR.Tests.ps1 +++ b/.github/scripts/Review-PR.Tests.ps1 @@ -51,6 +51,8 @@ BeforeAll { Invoke-Expression (Get-FunctionBody -ScriptText $content -FunctionName 'Get-CopilotCliUsageLineData') Invoke-Expression (Get-FunctionBody -ScriptText $content -FunctionName 'Get-CopilotOtelTokenMetrics') Invoke-Expression (Get-FunctionBody -ScriptText $content -FunctionName 'New-CopilotTokenUsageRecord') + Invoke-Expression (Get-FunctionBody -ScriptText $content -FunctionName 'Get-PhaseStateDir') + Invoke-Expression (Get-FunctionBody -ScriptText $content -FunctionName 'Restore-GateResultOrFailClosed') } Describe 'Copilot token usage helpers' { @@ -398,3 +400,214 @@ Describe 'Get-DotNetTestResults (console-scrape fallback)' { (Get-DotNetTestResults -Lines @()).Count | Should -Be 0 } } + +# ─── Phase-state security helpers (F1 hardening) ───────────────────────── +# These guard the cross-phase trust boundary: Gate writes verdict files, +# CopilotReview/Post restore them. Originally these defaulted to "SKIPPED" +# on missing file (fail open) and used the writable parent of +# $TrustedScriptsDir as the state dir. + +Describe 'Get-PhaseStateDir' { + BeforeAll { + $script:tmpRoot = Join-Path ([System.IO.Path]::GetTempPath()) "phase-state-$(New-Guid)" + New-Item -ItemType Directory -Force -Path $script:tmpRoot | Out-Null + } + AfterAll { + Remove-Item -Recurse -Force $script:tmpRoot -ErrorAction SilentlyContinue + } + + Context 'when -TrustedScriptsDir is provided' { + It 'returns a dedicated phase-state subdirectory, not the writable parent' { + $trusted = Join-Path $script:tmpRoot 'trusted-github' + New-Item -ItemType Directory -Force -Path $trusted | Out-Null + $d = Get-PhaseStateDir -TrustedScriptsDir $trusted -RepoRoot $script:tmpRoot -PRNumber '12345' + $d | Should -Be (Join-Path $script:tmpRoot 'phase-state') + $d | Should -Not -Be (Split-Path $trusted -Parent) + Test-Path $d | Should -BeTrue + } + It 'returns the same path on repeated calls (idempotent)' { + $trusted = Join-Path $script:tmpRoot 'trusted-github-2' + New-Item -ItemType Directory -Force -Path $trusted | Out-Null + $a = Get-PhaseStateDir -TrustedScriptsDir $trusted -RepoRoot $script:tmpRoot -PRNumber '1' + $b = Get-PhaseStateDir -TrustedScriptsDir $trusted -RepoRoot $script:tmpRoot -PRNumber '1' + $a | Should -Be $b + } + } + + Context 'when -TrustedScriptsDir is empty (local dev)' { + It 'falls back to a per-PR path under CustomAgentLogsTmp/PRState' { + $d = Get-PhaseStateDir -TrustedScriptsDir '' -RepoRoot $script:tmpRoot -PRNumber '42' + $d | Should -Match 'CustomAgentLogsTmp[/\\]PRState[/\\]42[/\\]PRAgent[/\\]phase-state$' + Test-Path $d | Should -BeTrue + } + } +} + +Describe 'Restore-GateResultOrFailClosed' { + BeforeEach { + $script:stateDir = Join-Path ([System.IO.Path]::GetTempPath()) "gate-restore-$(New-Guid)" + New-Item -ItemType Directory -Force -Path $script:stateDir | Out-Null + } + AfterEach { + Remove-Item -Recurse -Force $script:stateDir -ErrorAction SilentlyContinue + } + + It 'returns the gate verdict when the file exists with a valid value' { + 'PASSED' | Set-Content (Join-Path $script:stateDir 'gate-result.txt') -Encoding UTF8 + Restore-GateResultOrFailClosed -StateDir $script:stateDir | Should -Be 'PASSED' + } + + It 'accepts each whitelisted value' { + foreach ($v in @('PASSED','FAILED','SKIPPED','SKIP_NO_TESTS')) { + $v | Set-Content (Join-Path $script:stateDir 'gate-result.txt') -Encoding UTF8 + Restore-GateResultOrFailClosed -StateDir $script:stateDir | Should -Be $v + } + } + + It 'fails closed (throws) when the file is missing' { + { Restore-GateResultOrFailClosed -StateDir $script:stateDir } | + Should -Throw -ExpectedMessage '*failing closed*' + } + + It 'fails closed on an unrecognised verdict value (attacker tries arbitrary string)' { + 'GREEN' | Set-Content (Join-Path $script:stateDir 'gate-result.txt') -Encoding UTF8 + { Restore-GateResultOrFailClosed -StateDir $script:stateDir } | + Should -Throw -ExpectedMessage "*Invalid Gate result 'GREEN'*" + } + + It 'fails closed on an empty file' { + '' | Set-Content (Join-Path $script:stateDir 'gate-result.txt') -Encoding UTF8 -NoNewline + { Restore-GateResultOrFailClosed -StateDir $script:stateDir } | + Should -Throw -ExpectedMessage '*Invalid Gate result*' + } + + It 'trims surrounding whitespace before validating (real Set-Content adds trailing newline)' { + "PASSED`n" | Set-Content (Join-Path $script:stateDir 'gate-result.txt') -Encoding UTF8 -NoNewline + Restore-GateResultOrFailClosed -StateDir $script:stateDir | Should -Be 'PASSED' + } +} + +Describe 'Restore-GateResultOrFailClosed — F1.A HMAC verification' { + BeforeEach { + $script:stateDir = Join-Path ([System.IO.Path]::GetTempPath()) "gate-hmac-$(New-Guid)" + New-Item -ItemType Directory -Force -Path $script:stateDir | Out-Null + $script:resultFile = Join-Path $script:stateDir 'gate-result.txt' + $script:hmacFile = "$script:resultFile.hmac" + # 32-byte / 64-hex-char test key (matches `openssl rand -hex 32`). + $script:testKey = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef' + } + AfterEach { + Remove-Item -Recurse -Force $script:stateDir -ErrorAction SilentlyContinue + Remove-Item Env:GATE_HMAC_KEY -ErrorAction SilentlyContinue + } + + function script:Compute-TestHmac([string]$Path, [string]$KeyHex) { + $h = [System.Security.Cryptography.HMACSHA256]::HashData( + [System.Convert]::FromHexString($KeyHex), + [System.IO.File]::ReadAllBytes($Path)) + [System.Convert]::ToHexString($h).ToLowerInvariant() + } + + It 'returns verdict when HMAC is correct (param form)' { + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + (Compute-TestHmac -Path $script:resultFile -KeyHex $script:testKey) | Set-Content $script:hmacFile -Encoding UTF8 + Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey | + Should -Be 'PASSED' + } + + It 'returns verdict when HMAC is correct (env form)' { + 'FAILED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + (Compute-TestHmac -Path $script:resultFile -KeyHex $script:testKey) | Set-Content $script:hmacFile -Encoding UTF8 + $env:GATE_HMAC_KEY = $script:testKey + Restore-GateResultOrFailClosed -StateDir $script:stateDir | Should -Be 'FAILED' + } + + It 'skips HMAC verification when key is not supplied (local dev)' { + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + # No .hmac file, no env var — should still succeed + Restore-GateResultOrFailClosed -StateDir $script:stateDir | Should -Be 'PASSED' + } + + It 'fails closed when HMAC key is supplied but .hmac file is missing' { + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + { Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey } | + Should -Throw -ExpectedMessage '*HMAC key supplied but*missing*' + } + + It 'fails closed when the verdict file is tampered after HMAC was sealed (forgery attack)' { + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + (Compute-TestHmac -Path $script:resultFile -KeyHex $script:testKey) | Set-Content $script:hmacFile -Encoding UTF8 + # Simulate attacker overwriting verdict but unable to recompute HMAC + # without the key — old HMAC no longer matches the new file content. + 'FAILED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + { Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey } | + Should -Throw -ExpectedMessage '*HMAC verification failed*' + } + + It 'fails closed when the .hmac file is forged with wrong key' { + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + $wrongKey = 'ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff' + (Compute-TestHmac -Path $script:resultFile -KeyHex $wrongKey) | Set-Content $script:hmacFile -Encoding UTF8 + { Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey } | + Should -Throw -ExpectedMessage '*HMAC verification failed*' + } + + It 'accepts openssl-format HMAC output (lowercase hex, no newline normalisation)' { + # `openssl dgst -sha256 -hmac KEY file | awk '{print $NF}'` produces + # exactly 64 lowercase hex chars. Verify our verifier accepts that + # without leading whitespace, prefix, or trailing newline issues. + 'SKIPPED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + $h = Compute-TestHmac -Path $script:resultFile -KeyHex $script:testKey + # Set-Content -Encoding UTF8 normally appends newline → simulate that + # path too (the bash `echo ... > file` does the same). + Set-Content -Path $script:hmacFile -Value $h -Encoding UTF8 + Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey | + Should -Be 'SKIPPED' + } +} + +Describe 'Restore-GateResultOrFailClosed — openssl ↔ .NET HMAC cross-check' { + BeforeAll { + # Locate openssl. Hosted Linux/Mac CI agents have it on PATH. + # On Windows dev boxes we fall back to git-bash's openssl. + $script:openssl = (Get-Command openssl -ErrorAction SilentlyContinue)?.Source + if (-not $script:openssl) { + $candidates = @( + 'C:\Program Files\Git\usr\bin\openssl.exe', + 'C:\Program Files\Git\mingw64\bin\openssl.exe', + '/usr/bin/openssl', + '/opt/homebrew/bin/openssl' + ) + $script:openssl = $candidates | Where-Object { Test-Path $_ } | Select-Object -First 1 + } + } + + BeforeEach { + $script:stateDir = Join-Path ([System.IO.Path]::GetTempPath()) "gate-hmac-x-$(New-Guid)" + New-Item -ItemType Directory -Force -Path $script:stateDir | Out-Null + $script:resultFile = Join-Path $script:stateDir 'gate-result.txt' + $script:hmacFile = "$script:resultFile.hmac" + $script:testKey = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef' + } + AfterEach { + Remove-Item -Recurse -Force $script:stateDir -ErrorAction SilentlyContinue + } + + It 'a verdict file sealed via openssl with hexkey verifies under the .NET verifier' { + if (-not $script:openssl) { + Set-ItResult -Skipped -Because 'openssl not available in this environment' + return + } + # Reproduce the YAML pipeline byte-for-byte: write verdict, sign + # with `openssl dgst -sha256 -mac HMAC -macopt hexkey:KEY`, + # extract last token, write .hmac sibling. + 'PASSED' | Set-Content $script:resultFile -Encoding UTF8 -NoNewline + $opensslOut = & $script:openssl dgst -sha256 -mac HMAC -macopt "hexkey:$script:testKey" $script:resultFile + $hmac = ($opensslOut -split ' ')[-1].Trim() + $hmac | Should -Match '^[0-9a-f]{64}$' -Because 'openssl should produce 64 lowercase hex chars' + $hmac | Set-Content $script:hmacFile -Encoding UTF8 + + Restore-GateResultOrFailClosed -StateDir $script:stateDir -HmacKeyHex $script:testKey | + Should -Be 'PASSED' + } +} diff --git a/.github/scripts/Review-PR.ps1 b/.github/scripts/Review-PR.ps1 index 4dcbbda429b3..8af47d104f36 100644 --- a/.github/scripts/Review-PR.ps1 +++ b/.github/scripts/Review-PR.ps1 @@ -125,6 +125,66 @@ if (Test-Path $commentCleanupScript) { . $commentCleanupScript } +# Out-SafePRSubprocessLine neutralizes `##vso[…]` / `##[…]` AzDO logging +# commands in PR-controlled subprocess output before re-emit. +$sanitizerScript = Join-Path $ScriptsDir "shared/Write-SafeSubprocessOutput.ps1" +if (Test-Path $sanitizerScript) { + . $sanitizerScript +} + +# ─── Phase-state helpers ────────────────────────────────────────────────── +# Cross-phase state files (setup-complete sentinel, gate-result.txt, etc.) +# must live in a directory that hostile PR test code cannot rewrite between +# Gate and CopilotReview/Post. Previously these landed in the writable +# parent of `$TrustedScriptsDir`. We now use a dedicated `phase-state` +# subdirectory that CI re-chmods read-only after the Gate task completes. +function Get-PhaseStateDir([string]$TrustedScriptsDir, [string]$RepoRoot, [string]$PRNumber) { + $d = if ($TrustedScriptsDir) { + Join-Path (Split-Path $TrustedScriptsDir -Parent) 'phase-state' + } else { + Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/phase-state" + } + New-Item -ItemType Directory -Force -Path $d -ErrorAction SilentlyContinue | Out-Null + $d +} + +# Fail closed: missing/garbage verdict throws; previously defaulted to SKIPPED. +# When $env:GATE_HMAC_KEY is set, also verify gate-result.txt.hmac matches — +# the key is generated by the Gate bash task AFTER pwsh exits and propagated +# via AzDO secret variable, so PR-spawned daemons cannot forge a valid MAC. +function Restore-GateResultOrFailClosed { + [CmdletBinding()] + param( + [Parameter(Mandatory)][string]$StateDir, + [string]$HmacKeyHex = $env:GATE_HMAC_KEY + ) + $f = Join-Path $StateDir "gate-result.txt" + if (-not (Test-Path $f)) { + throw "Gate result file missing at '$f' — failing closed." + } + $raw = Get-Content $f -Raw + $r = if ($null -eq $raw) { '' } else { $raw.Trim() } + $valid = @('PASSED','FAILED','SKIPPED','SKIP_NO_TESTS') + if ($r -notin $valid) { + throw "Invalid Gate result '$r' at '$f' (expected one of: $($valid -join ', ')) — failing closed." + } + if (-not [string]::IsNullOrWhiteSpace($HmacKeyHex)) { + $hmacFile = "$f.hmac" + if (-not (Test-Path $hmacFile)) { + throw "HMAC key supplied but '$hmacFile' missing — failing closed (verdict file tampered)." + } + $expected = ((Get-Content $hmacFile -Raw) ?? '').Trim().ToLowerInvariant() + $keyBytes = [System.Convert]::FromHexString($HmacKeyHex) + $fileBytes = [System.IO.File]::ReadAllBytes($f) + $hashBytes = [System.Security.Cryptography.HMACSHA256]::HashData($keyBytes, $fileBytes) + $computed = [System.Convert]::ToHexString($hashBytes).ToLowerInvariant() + if ($computed -ne $expected) { + throw "Gate HMAC verification failed for '$f' — verdict file tampered after Gate sealed it." + } + } + $r +} + # Gate has GH_TOKEN in env so trusted code (Detect-TestsInDiff, Find-RegressionRisks, # detect-ui-test-categories) can fetch PR metadata via `gh` CLI. Any subprocess that # executes PR-controlled code (MSBuild targets, test code, source generators, host-app @@ -200,7 +260,7 @@ Write-Host " ✅ Copilot CLI: $copilotVersion" -ForegroundColor Green $prInfo = gh pr view $PRNumber --json title,state 2>$null | ConvertFrom-Json if (-not $prInfo) { Write-Error "PR #$PRNumber not found"; exit 1 } -Write-Host " ✅ PR: $($prInfo.title)" -ForegroundColor Green +Write-Host (" ✅ PR: " + ($prInfo.title -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1')) -ForegroundColor Green # ═════════════════════════════════════════════════════════════════════════════ # STEP 1: Branch Setup (Create Review Branch & Cherry-Pick PR) @@ -396,13 +456,7 @@ if ($DryRun) { # End of Setup phase — write sentinel and exit early if ($Phase -eq 'Setup') { # Sentinel signals to Tasks 2-4 that Setup completed successfully (PR merged). - $sentinelDir = if ($TrustedScriptsDir) { - Split-Path $TrustedScriptsDir -Parent - } else { - $d = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate" - New-Item -ItemType Directory -Force -Path $d | Out-Null - $d - } + $sentinelDir = Get-PhaseStateDir -TrustedScriptsDir $TrustedScriptsDir -RepoRoot $RepoRoot -PRNumber $PRNumber "OK" | Set-Content (Join-Path $sentinelDir "setup-complete") -Encoding UTF8 Write-Host "✅ Setup phase complete" -ForegroundColor Green if ($LogFile) { Stop-Transcript -ErrorAction SilentlyContinue | Out-Null } @@ -411,11 +465,7 @@ if ($Phase -eq 'Setup') { # ─── Sentinel check: verify Setup completed before running later phases ─── if ($Phase -and $Phase -ne 'Setup') { - $sentinelDir = if ($TrustedScriptsDir) { - Split-Path $TrustedScriptsDir -Parent - } else { - Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate" - } + $sentinelDir = Get-PhaseStateDir -TrustedScriptsDir $TrustedScriptsDir -RepoRoot $RepoRoot -PRNumber $PRNumber $sentinelFile = Join-Path $sentinelDir "setup-complete" if (-not (Test-Path $sentinelFile)) { Write-Error "Setup phase did not complete (sentinel not found at '$sentinelFile'). Cannot proceed with -Phase $Phase." @@ -1038,7 +1088,7 @@ function Invoke-CopilotStep { if ($DryRun) { Write-Host "[DRY RUN] Prompt:" -ForegroundColor Magenta - Write-Host $Prompt -ForegroundColor Gray + Write-Host ($Prompt -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1') -ForegroundColor Gray return 0 } @@ -1106,7 +1156,7 @@ function Invoke-CopilotStep { if ($event.data.model) { $modelName = $event.data.model Write-Host " ⚙️ Model: " -ForegroundColor DarkGray -NoNewline - Write-Host $modelName -ForegroundColor DarkCyan + Write-Host ($modelName -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1') -ForegroundColor DarkCyan } } 'assistant.turn_start' { @@ -1116,7 +1166,7 @@ function Invoke-CopilotStep { Write-Host " ┌─ Turn $turnCount " -ForegroundColor DarkGray -NoNewline Write-Host "[$elapsed]" -ForegroundColor DarkYellow -NoNewline if ($currentIntent) { - Write-Host " · $currentIntent" -ForegroundColor DarkCyan + Write-Host (" · " + ($currentIntent -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1')) -ForegroundColor DarkCyan } else { Write-Host "" } @@ -1132,7 +1182,7 @@ function Invoke-CopilotStep { if ($toolName -eq 'report_intent') { $currentIntent = $args_.intent ?? $currentIntent Write-Host " │ 🎯 " -ForegroundColor DarkGray -NoNewline - Write-Host $currentIntent -ForegroundColor Yellow + Write-Host ($currentIntent -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1') -ForegroundColor Yellow break } @@ -1163,9 +1213,9 @@ function Invoke-CopilotStep { } Write-Host " │ $icon " -ForegroundColor DarkGray -NoNewline - Write-Host $displayName -ForegroundColor Cyan -NoNewline + Write-Host ($displayName -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1') -ForegroundColor Cyan -NoNewline if ($detail) { - Write-Host " $detail" -ForegroundColor DarkGray + Write-Host (" " + ($detail -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1')) -ForegroundColor DarkGray } else { Write-Host "" } @@ -1186,7 +1236,7 @@ function Invoke-CopilotStep { $preview = $preview.Substring(0, 400) + "…" } Write-Host " │ 💬 " -ForegroundColor DarkGray -NoNewline - Write-Host $preview -ForegroundColor White + Write-Host ($preview -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1') -ForegroundColor White } } 'result' { @@ -1230,9 +1280,10 @@ function Invoke-CopilotStep { $modelName = [string]$cliLineData.model } - # Non-JSON line (e.g. stats) — pass through as-is + # Non-JSON line (e.g. stats) — sanitize the copilot CLI's + # PR-aware stdout before re-emit. if ($line.Trim()) { - Write-Host " $line" -ForegroundColor DarkGray + Write-Host (" " + ($line -replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1')) -ForegroundColor DarkGray } } } @@ -1299,7 +1350,7 @@ $detectScript = Join-Path $EngScriptsDir "detect-ui-test-categories.ps1" if (Test-Path $detectScript) { try { $detectOutput = & pwsh -NoProfile -File $detectScript -PrNumber "$PRNumber" 2>&1 - $detectOutput | ForEach-Object { Write-Host " $_" } + $detectOutput | Out-SafePRSubprocessLine -Prefix " " foreach ($line in $detectOutput) { $lineStr = $line.ToString() @@ -1334,6 +1385,17 @@ if (Test-Path $detectScript) { $catsForOutput = if ($uitestCategories -eq 'NONE') { 'NONE' } elseif ([string]::IsNullOrWhiteSpace($uitestCategories)) { 'ALL' } else { $uitestCategories } + # Defense-in-depth (mirrors the refresh-path guard ~L1683): the + # AzDO setvariable command is parsed by IndexOf — a `]` mid-value + # would terminate it. Validate the final string against the + # documented category-list grammar before re-emitting so a malicious + # detect script (or a future change that surfaces PR-controlled + # text into the category list) cannot inject a second ##vso + # command via this echo path. + if ($catsForOutput -notmatch '^(NONE|ALL|[A-Za-z0-9_,]+)$') { + Write-Host " ⚠️ Rejecting detected category list (invalid characters): $catsForOutput" -ForegroundColor Yellow + $catsForOutput = 'ALL' + } Write-Host "##vso[task.setvariable variable=detectedCategories;isOutput=true]$catsForOutput" Write-Host "##vso[task.setvariable variable=detectedPlatform;isOutput=true]$Platform" @@ -1377,7 +1439,12 @@ $regressionOutputDir = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber $regressionScript = Join-Path $ScriptsDir "Find-RegressionRisks.ps1" if (Test-Path $regressionScript) { try { - & $regressionScript -PRNumber $PRNumber -OutputDir $regressionOutputDir + # Launch as `pwsh -NoProfile -File` SUBPROCESS so the child's + # Write-Host writes to its own host (captured by 2>&1) instead of + # leaking directly to the parent AzDO log (Information stream is + # NOT captured by `2>&1` for in-process invocation). + & pwsh -NoProfile -File $regressionScript -PRNumber $PRNumber -OutputDir $regressionOutputDir 2>&1 | + Out-SafePRSubprocessLine -Prefix ' ' $regressionResult = if (Test-Path (Join-Path $regressionOutputDir "result.txt")) { (Get-Content (Join-Path $regressionOutputDir "result.txt") -Raw).Trim() } else { 'UNKNOWN' } @@ -1451,9 +1518,12 @@ if ($risksData -and ($risksData.result -eq 'REVERT' -or $risksData.result -eq 'O 'UITest' { if (Test-Path $uiTestRunner) { Write-Host " 🖥️ Running UI test via BuildAndRunHostApp.ps1 -Platform $regrPlatform -TestFilter `"$($t.Filter)`"" -ForegroundColor Cyan - $testOutput = Invoke-WithoutGhTokens { & $uiTestRunner -Platform $regrPlatform -TestFilter $t.Filter 2>&1 } + # `pwsh -File` SUBPROCESS so BuildAndRunHostApp's Write-Host + # of attacker-controlled device-log content can't bypass + # the 2>&1 capture into $testOutput. + $testOutput = Invoke-WithoutGhTokens { & pwsh -NoProfile -File $uiTestRunner -Platform $regrPlatform -TestFilter $t.Filter 2>&1 } $testExitCode = $LASTEXITCODE - $testOutput | Select-Object -Last 20 | ForEach-Object { Write-Host " $_" } + $testOutput | Select-Object -Last 20 | Out-SafePRSubprocessLine -Prefix ' ' } else { Write-Host " ⚠️ BuildAndRunHostApp.ps1 not found" -ForegroundColor Yellow $testExitCode = -1 @@ -1463,9 +1533,10 @@ if ($risksData -and ($risksData.result -eq 'REVERT' -or $risksData.result -eq 'O if (Test-Path $deviceTestRunner) { $dtProject = if ($t.Project) { $t.Project } else { 'Controls' } Write-Host " 📱 Running device test via Run-DeviceTests.ps1 -Project $dtProject -Platform $regrPlatform -TestFilter `"$($t.Filter)`"" -ForegroundColor Cyan - $testOutput = Invoke-WithoutGhTokens { & $deviceTestRunner -Project $dtProject -Platform $regrPlatform -TestFilter $t.Filter 2>&1 } + # `pwsh -File` SUBPROCESS — same rationale as above. + $testOutput = Invoke-WithoutGhTokens { & pwsh -NoProfile -File $deviceTestRunner -Project $dtProject -Platform $regrPlatform -TestFilter $t.Filter 2>&1 } $testExitCode = $LASTEXITCODE - $testOutput | Select-Object -Last 20 | ForEach-Object { Write-Host " $_" } + $testOutput | Select-Object -Last 20 | Out-SafePRSubprocessLine -Prefix ' ' } else { Write-Host " ⚠️ Run-DeviceTests.ps1 not found" -ForegroundColor Yellow $testExitCode = -1 @@ -1477,7 +1548,7 @@ if ($risksData -and ($risksData.result -eq 'REVERT' -or $risksData.result -eq 'O Write-Host " 🧪 Running: dotnet test $($t.ProjectPath) --filter `"$($t.Filter)`"" -ForegroundColor Cyan $testOutput = Invoke-WithoutGhTokens { dotnet test $resolvedProj --filter $t.Filter --logger "console;verbosity=minimal" 2>&1 } $testExitCode = $LASTEXITCODE - $testOutput | Select-Object -Last 20 | ForEach-Object { Write-Host " $_" } + $testOutput | Select-Object -Last 20 | Out-SafePRSubprocessLine -Prefix " " } else { Write-Host " ⚠️ No project path for unit test" -ForegroundColor Yellow $testExitCode = -1 @@ -1581,7 +1652,7 @@ Write-Host " 🔍 Detecting tests in PR #$PRNumber..." -ForegroundColor Cyan $testDetectScript = Join-Path $ScriptsDir "shared/Detect-TestsInDiff.ps1" if (Test-Path $testDetectScript) { $testDetectScript = (Resolve-Path $testDetectScript).Path - & pwsh -NoProfile -File $testDetectScript -PRNumber $PRNumber 2>&1 | ForEach-Object { Write-Host " $_" } + & pwsh -NoProfile -File $testDetectScript -PRNumber $PRNumber 2>&1 | Out-SafePRSubprocessLine -Prefix " " } else { Write-Host " ⚠️ Detect-TestsInDiff.ps1 not found at $testDetectScript" -ForegroundColor Yellow } @@ -1638,7 +1709,7 @@ for ($gateAttempt = 1; $gateAttempt -le $maxGateAttempts; $gateAttempt++) { # subprocess invocations internally to strip the token before PR code runs. $gateOutput = & pwsh -NoProfile -File "$verifyScript" -Platform $gatePlatform -PRNumber $PRNumber 2>&1 $gateExitCode = $LASTEXITCODE - $gateOutput | ForEach-Object { Write-Host " $_" } + $gateOutput | Out-SafePRSubprocessLine -Prefix " " # Check if this was an ENV ERROR (emulator timeout, ADB failure, etc.) $isEnvError = $false @@ -1822,13 +1893,7 @@ $gateLogTail } # Persist gate result so other phases can read it -$gateVerdictDir = if ($TrustedScriptsDir) { - Split-Path $TrustedScriptsDir -Parent -} else { - $d = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate" - New-Item -ItemType Directory -Force -Path $d | Out-Null - $d -} +$gateVerdictDir = Get-PhaseStateDir -TrustedScriptsDir $TrustedScriptsDir -RepoRoot $RepoRoot -PRNumber $PRNumber $gateResult | Set-Content (Join-Path $gateVerdictDir "gate-result.txt") -Encoding UTF8 Write-Host " 📄 Gate result persisted: $gateResult" -ForegroundColor Gray @@ -1848,12 +1913,22 @@ if ($risksData) { } } -# Persist detect script path and detected categories for Tier 3 refresh -if ($detectScript) { - $detectScript | Set-Content (Join-Path $gateVerdictDir "detect-script-path.txt") -Encoding UTF8 -} +# Persist detected UI test categories for Tier 3 refresh. The detect script +# *path* is intentionally NOT persisted — CopilotReview re-derives it from +# `$EngScriptsDir` so a tampered persisted path cannot redirect execution +# under COPILOT_GITHUB_TOKEN to attacker-controlled code. $uitestCategories | Set-Content (Join-Path $gateVerdictDir "uitest-categories.txt") -Encoding UTF8 +# Defense-in-depth: lock the phase-state directory read-only after Gate +# writes complete so attacker-controlled subprocess output cannot forge +# `gate-result.txt` (or any other state file) between phases on Linux/Mac +# CI agents. Restricted to CI runs ($TrustedScriptsDir is set) so local +# non-phased re-runs are not blocked by leftover read-only state files. +# On Windows / local dev the chmod is skipped entirely. +if (($IsLinux -or $IsMacOS) -and $TrustedScriptsDir) { + try { & chmod -R a-w $gateVerdictDir 2>$null } catch { } +} + } # end if (-not $skipGateAndTryFix) } # end if ($runGate) @@ -1863,19 +1938,13 @@ if ($runCopilotReview) { # Restore gate result from file when running in phased mode if ($Phase -eq 'CopilotReview') { - $gateVerdictDir = if ($TrustedScriptsDir) { - Split-Path $TrustedScriptsDir -Parent - } else { - Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate" - } - $gateVerdictFile = Join-Path $gateVerdictDir "gate-result.txt" - if (Test-Path $gateVerdictFile) { - $gateResult = (Get-Content $gateVerdictFile -Raw).Trim() - Write-Host " 📄 Restored gate result: $gateResult" -ForegroundColor Gray - } else { - $gateResult = "SKIPPED" - Write-Host " ⚠️ Gate result file not found — defaulting to SKIPPED" -ForegroundColor Yellow - } + $gateVerdictDir = Get-PhaseStateDir -TrustedScriptsDir $TrustedScriptsDir -RepoRoot $RepoRoot -PRNumber $PRNumber + # Fail closed if Gate did not produce a verdict — never default to SKIPPED. + # The function throws on missing file or invalid value; we let it propagate + # because running CopilotReview with a forged/missing verdict is the + # specific attack we are defending against. + $gateResult = Restore-GateResultOrFailClosed -StateDir $gateVerdictDir + Write-Host " 📄 Restored gate result: $gateResult" -ForegroundColor Gray # Restore regression data persisted by Gate phase $risksFile = Join-Path $gateVerdictDir "regression-risks.json" @@ -1898,12 +1967,11 @@ if ($Phase -eq 'CopilotReview') { } } - # Restore detect script path and UI test categories for Tier 3 refresh - $detectPathFile = Join-Path $gateVerdictDir "detect-script-path.txt" - $catsFile = Join-Path $gateVerdictDir "uitest-categories.txt" - if (Test-Path $detectPathFile) { - $detectScript = (Get-Content $detectPathFile -Raw).Trim() - } + # Re-derive detect script from the trusted directory rather than reading + # a persisted path (which an attacker could tamper to redirect execution + # to a malicious script run under COPILOT_GITHUB_TOKEN). + $detectScript = Join-Path $EngScriptsDir "detect-ui-test-categories.ps1" + $catsFile = Join-Path $gateVerdictDir "uitest-categories.txt" if (Test-Path $catsFile) { $uitestCategories = (Get-Content $catsFile -Raw).Trim() } @@ -2107,7 +2175,7 @@ if ($detectScript -and (Test-Path $detectScript) -and (Test-Path $aiCategoriesFi if (-not [string]::IsNullOrWhiteSpace($aiCategoriesArg)) { Write-Host " 🔁 Refreshing UI category detection with AI tier..." -ForegroundColor Cyan $refreshOutput = & pwsh -NoProfile -File $detectScript -PrNumber "$PRNumber" -AiCategories $aiCategoriesArg 2>&1 - $refreshOutput | ForEach-Object { Write-Host " $_" } + $refreshOutput | Out-SafePRSubprocessLine -Prefix " " $refreshedCategories = $uitestCategories foreach ($line in $refreshOutput) { @@ -2122,8 +2190,20 @@ if ($detectScript -and (Test-Path $detectScript) -and (Test-Path $aiCategoriesFi $refreshedForOutput = if ($refreshedCategories -eq 'NONE') { 'NONE' } elseif ([string]::IsNullOrWhiteSpace($refreshedCategories)) { 'ALL' } else { $refreshedCategories } - Write-Host "##vso[task.setvariable variable=detectedCategories;isOutput=true]$refreshedForOutput" - Write-Host " 🔁 Updated detectedCategories output: $refreshedForOutput" -ForegroundColor Green + # Defense-in-depth: the AzDO setvariable command is parsed by + # IndexOf — a `]` mid-value would terminate it. Validate the + # final string against the documented category-list grammar + # before re-emitting so a malicious detect script (or a + # tampered ai-categories.md) cannot inject a second + # ##vso command via this echo path. The detect script is + # trusted (\$EngScriptsDir) but the AI categories input is + # PR-derived. + if ($refreshedForOutput -notmatch '^(NONE|ALL|[A-Za-z0-9_,]+)$') { + Write-Host " ⚠️ Rejecting refreshed category list (invalid characters): $refreshedForOutput" -ForegroundColor Yellow + } else { + Write-Host "##vso[task.setvariable variable=detectedCategories;isOutput=true]$refreshedForOutput" + Write-Host " 🔁 Updated detectedCategories output: $refreshedForOutput" -ForegroundColor Green + } } $uitestOutputDir = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/uitests" @@ -2149,17 +2229,9 @@ if ($runPost) { # Restore gate result from file when running in phased mode if ($Phase -eq 'Post') { - $gateVerdictDir = if ($TrustedScriptsDir) { - Split-Path $TrustedScriptsDir -Parent - } else { - Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/gate" - } - $gateVerdictFile = Join-Path $gateVerdictDir "gate-result.txt" - if (Test-Path $gateVerdictFile) { - $gateResult = (Get-Content $gateVerdictFile -Raw).Trim() - } else { - $gateResult = "SKIPPED" - } + $gateVerdictDir = Get-PhaseStateDir -TrustedScriptsDir $TrustedScriptsDir -RepoRoot $RepoRoot -PRNumber $PRNumber + # Fail closed: see CopilotReview restore above. + $gateResult = Restore-GateResultOrFailClosed -StateDir $gateVerdictDir } # ─── Gate posting (moved here so only the Post task needs GH_TOKEN) ────── @@ -2256,8 +2328,20 @@ if (Test-Path $reviewScript) { $aiSummaryReviewId | Set-Content $reviewIdFile -Encoding UTF8 if (-not [string]::IsNullOrWhiteSpace($aiSummaryReviewNodeId)) { $aiSummaryReviewNodeId | Set-Content (Join-Path (Split-Path -Parent $reviewIdFile) "ai-summary-review-node-id.txt") -Encoding UTF8 - Write-Host "##vso[task.setvariable variable=aiSummaryReviewNodeId;isOutput=true]$aiSummaryReviewNodeId" + # Defense-in-depth: GitHub review node-ids are GraphQL-encoded + # base64-ish strings (e.g. `PRR_kwDOFB4dxc6...`). Validate the + # grammar before re-emitting through ##vso[task.setvariable] + # so a malicious post-ai-summary-comment.ps1 (trusted today) + # could not inject a second AzDO command via this echo path + # if it ever started surfacing PR-controlled text. + if ($aiSummaryReviewNodeId -match '^[A-Za-z0-9_\-]+$') { + Write-Host "##vso[task.setvariable variable=aiSummaryReviewNodeId;isOutput=true]$aiSummaryReviewNodeId" + } else { + Write-Host " ⚠️ Rejecting aiSummaryReviewNodeId (unexpected characters): $aiSummaryReviewNodeId" -ForegroundColor Yellow + } } + # $aiSummaryReviewId is already constrained to \d+ by the regex + # capture above, so it is safe to re-emit without an extra guard. Write-Host "##vso[task.setvariable variable=aiSummaryReviewId;isOutput=true]$aiSummaryReviewId" } else { Write-Host " ✅ PR review summary posted" -ForegroundColor Green diff --git a/.github/scripts/shared/Invoke-UITestWithRetry.ps1 b/.github/scripts/shared/Invoke-UITestWithRetry.ps1 index 9a0f0bd2f32a..5c1018ac2195 100644 --- a/.github/scripts/shared/Invoke-UITestWithRetry.ps1 +++ b/.github/scripts/shared/Invoke-UITestWithRetry.ps1 @@ -179,7 +179,20 @@ for ($attempt = 1; $attempt -le $MaxAttempts; $attempt++) { $envHit = $null Write-Host "▶ BuildAndRunHostApp.ps1 attempt $attempt/$MaxAttempts" -ForegroundColor Cyan - $lastOutput = & $buildScript @baseParams 2>&1 + # Launch BuildAndRunHostApp.ps1 as a `pwsh -NoProfile -File` SUBPROCESS, + # not in-process. BuildAndRunHostApp.ps1 has 19+ Write-Host emissions + # including the last 100 lines of the device log (`$recentLogs | ForEach-Object + # { Write-Host $_ }`) which is attacker-controlled (logcat / syslog content + # from a poisoned PR app). When this script is itself invoked IN-PROCESS + # (`& $retryScript` from ci-copilot.yml's deep-UI-test stage), an in-process + # `& $buildScript` would let that Write-Host write straight to the parent + # pwsh task's host (= the AzDO agent's stdout), BYPASSING `2>&1` capture + # (Write-Host targets the Information stream, which `2>&1` does not redirect). + # The `##vso[…]` payload would then reach the AzDO parser raw. Running as + # a subprocess gives the child its OWN host, so ALL of its output flows + # through its stdout and is captured here by `2>&1` into $lastOutput + # (which the caller writes to $LogFile / returns for sanitized re-emission). + $lastOutput = & pwsh -NoProfile -File $buildScript @baseParams 2>&1 $lastExit = $LASTEXITCODE if ($lastExit -eq 0) { break } diff --git a/.github/scripts/shared/Remove-StaleMauiBotComments.ps1 b/.github/scripts/shared/Remove-StaleMauiBotComments.ps1 index 1ca3cbe16a8e..7162a19d72e8 100644 --- a/.github/scripts/shared/Remove-StaleMauiBotComments.ps1 +++ b/.github/scripts/shared/Remove-StaleMauiBotComments.ps1 @@ -182,9 +182,14 @@ function Hide-StaleMauiBotIssueComments { continue } + # gate the marker path on bot authorship so a maintainer who + # quotes `` / `` in their own + # comment is not silently hidden on the next cleanup run. $matchesGeneratedMarker = - ($IncludeAISummary -and (Test-IsAISummaryCommentBody $body)) -or - ($IncludeLegacyGate -and $body.Contains($script:AiGateCommentMarker)) + (Test-IsMauiBotCommentAuthor $comment) -and ( + ($IncludeAISummary -and (Test-IsAISummaryCommentBody $body)) -or + ($IncludeLegacyGate -and $body.Contains($script:AiGateCommentMarker)) + ) $matchesBotOnlyContent = (Test-IsMauiBotCommentAuthor $comment) -and ( diff --git a/.github/scripts/shared/Write-SafeSubprocessOutput.Tests.ps1 b/.github/scripts/shared/Write-SafeSubprocessOutput.Tests.ps1 new file mode 100644 index 000000000000..1872a63bb14a --- /dev/null +++ b/.github/scripts/shared/Write-SafeSubprocessOutput.Tests.ps1 @@ -0,0 +1,1084 @@ +#Requires -Version 5.1 +#Requires -Modules @{ ModuleName='Pester'; ModuleVersion='5.0.0' } + +# Tests for shared/Write-SafeSubprocessOutput.ps1. Out-SafePRSubprocessLine +# must neutralize any `##vso[...]` / `##[...]` AzDO logging-command prefix +# in PR-controlled subprocess stdout (case-insensitive, mid-line, with +# leading whitespace / control chars / ANSI escapes, nested, multiple per +# line) while leaving the rest of the line intact, and handle $null / empty +# input without throwing. + +BeforeAll { + $script:HelperPath = Join-Path $PSScriptRoot 'Write-SafeSubprocessOutput.ps1' + . $script:HelperPath + + # Convenience: capture Write-Host output by running the helper inside + # a sub-shell that redirects Information stream to Output. Returns + # the emitted text as an array of strings (one per Write-Host call). + function Invoke-AndCapture { + param( + [Parameter(ValueFromPipeline = $true)] + [object]$InputObject, + + [string]$Prefix = '' + ) + begin { + $collected = New-Object System.Collections.Generic.List[object] + } + process { + $collected.Add($InputObject) + } + end { + # Use Information stream redirection — Write-Host writes to + # the Information stream in PS5+, and `6>&1` merges it into + # success output where we can capture it. + + # @() forces array context so a single-record result isn't + # unwrapped to a scalar string (which would make $result[0] + # return the first CHARACTER, not the first line). + $output = @($collected | Out-SafePRSubprocessLine -Prefix $Prefix 6>&1) + # 6>&1 produces InformationRecord objects; convert to plain strings. + $strings = @($output | ForEach-Object { + if ($_ -is [System.Management.Automation.InformationRecord]) { + $_.MessageData.ToString() + } else { + [string]$_ + } + }) + # Re-wrap with , (comma) so PowerShell doesn't unwrap a + # 1-element array on return. + return ,$strings + } + } + + function Test-LineIsNeutralized { + param([string]$EmittedLine) + # After sanitization, an AzDO parser must NOT see `##vso[` or `##[` + # at the start of a (whitespace-stripped) line — the parser uses + # IndexOf, so we also check no occurrence ANYWHERE in the line + # except inside the neutralization marker `##~SANITIZED~`. + + # Strategy: temporarily mask out our own sanitization marker, then + # search for any remaining `##vso[` or `##[`. + $masked = $EmittedLine -replace '##~SANITIZED~', '##XXX-MARKER-XXX' + return ($masked -notmatch '##vso\[' -and $masked -notmatch '##\[') + } +} + +Describe 'Out-SafePRSubprocessLine — benign passthrough' { + + It 'passes plain ASCII line through unchanged' { + $result = 'hello world' | Invoke-AndCapture + $result | Should -HaveCount 1 + $result[0] | Should -Be 'hello world' + } + + It 'passes empty string through' { + $result = '' | Invoke-AndCapture + $result | Should -HaveCount 1 + $result[0] | Should -Be '' + } + + It 'passes $null through without throwing' { + { $null | Invoke-AndCapture } | Should -Not -Throw + } + + It 'prepends Prefix to benign lines' { + $result = 'hello' | Invoke-AndCapture -Prefix ' ' + $result[0] | Should -Be ' hello' + } + + It 'passes line containing the word vso through (false-positive guard)' { + # The string "vso" by itself, not in a logging-command prefix, must + # not be touched. + $result = 'Connecting to vso.example.com ...' | Invoke-AndCapture + $result[0] | Should -Be 'Connecting to vso.example.com ...' + } + + It 'passes line containing single # marker through' { + $result = '#vso[ this is a comment' | Invoke-AndCapture + $result[0] | Should -Be '#vso[ this is a comment' + } + + It 'passes line with ## but no [ through' { + $result = '## section header' | Invoke-AndCapture + $result[0] | Should -Be '## section header' + } +} + +Describe 'Out-SafePRSubprocessLine — neutralizes AzDO task commands' { + + It 'neutralizes ##vso[task.setvariable variable=X]value' { + $result = '##vso[task.setvariable variable=detectedCategories;isOutput=true]NONE' | + Invoke-AndCapture + $result | Should -HaveCount 1 + Test-LineIsNeutralized $result[0] | Should -BeTrue + $result[0] | Should -BeLike '*detectedCategories*' # rest preserved + $result[0] | Should -BeLike '*NONE*' + } + + It 'neutralizes ##vso[task.complete result=Failed]done' { + $result = '##vso[task.complete result=Failed]done' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + $result[0] | Should -BeLike '*task.complete*' + } + + It 'neutralizes ##vso[task.uploadfile]/etc/passwd' { + $result = '##vso[task.uploadfile]/etc/passwd' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + $result[0] | Should -BeLike '*/etc/passwd*' + } + + It 'neutralizes ##vso[task.prependpath]/tmp/attacker' { + $result = '##vso[task.prependpath]/tmp/attacker' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vso[task.logissue type=error]forged' { + $result = '##vso[task.logissue type=error]forged' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — neutralizes ##[label] commands' { + + It 'neutralizes ##[error]forged-error' { + $result = '##[error]forged-error' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + $result[0] | Should -BeLike '*forged-error*' + } + + It 'neutralizes ##[warning]forged-warning' { + $result = '##[warning]forged-warning' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##[group]Some Group' { + $result = '##[group]Some Group' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — leading-whitespace evasions' { + + It 'neutralizes leading-spaces ##vso[...]' { + $result = ' ##vso[task.setvariable variable=X]Y' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes leading-tab ##vso[...]' { + $result = "`t##vso[task.setvariable variable=X]Y" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes leading-carriage-return ##vso[...]' { + $result = "`r##vso[task.setvariable variable=X]Y" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes leading-NUL-byte ##vso[...]' { + $result = "$([char]0)##vso[task.setvariable variable=X]Y" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes mixed leading whitespace + control chars' { + $result = " `t`r `t##vso[task.setvariable variable=X]Y" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — ANSI color evasions' { + + It 'neutralizes ANSI-prefixed ##vso[...] (ESC[31m)' { + $esc = [char]0x1b + $result = "$esc[31m##vso[task.setvariable variable=X]Y" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ANSI-prefixed ##[error]' { + $esc = [char]0x1b + $result = "$esc[1;33m##[error]forged" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ANSI reset + ANSI color + ##vso' { + $esc = [char]0x1b + $result = "$esc[0m$esc[31m##vso[task.complete result=Failed]done" | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — case variations' { + + It 'neutralizes ##VSO[...] (uppercase)' { + $result = '##VSO[task.setvariable variable=X]Y' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##Vso[...] (mixed case)' { + $result = '##Vso[task.setvariable variable=X]Y' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vSO[...] (random case)' { + $result = '##vSO[task.setvariable variable=X]Y' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — nesting / multiple commands per line' { + + It 'neutralizes multiple ##vso commands on one line' { + $line = '##vso[task.setvariable variable=X]Y ##vso[task.complete result=Failed]done' + $result = $line | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes nested ##vso[##vso[...]...] (recursive evasion)' { + # Some implementations only strip the first occurrence; we must + # strip ALL or the inner one becomes the outer after the first pass. + $line = '##vso[##vso[task.setvariable variable=X]Y]' + $result = $line | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vso AND ##[error] on same line' { + $line = '##vso[task.complete result=Failed]done ##[error]forged' + $result = $line | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } +} + +Describe 'Out-SafePRSubprocessLine — preserves visibility' { + + It 'leaves attacker payload visible (not silently dropped) so operators see it' { + $line = '##vso[task.setvariable variable=detectedCategories]NONE' + $result = $line | Invoke-AndCapture + # We MUST keep evidence the attack was attempted, otherwise an + # invisible drop hides the attack from incident response. + $result[0] | Should -BeLike '*detectedCategories*' + $result[0] | Should -BeLike '*NONE*' + $result[0] | Should -BeLike '*SANITIZED*' + } + + It 'preserves Prefix on sanitized lines' { + $result = '##vso[task.complete]done' | Invoke-AndCapture -Prefix ' >> ' + $result[0] | Should -BeLike ' >> *' + } +} + +Describe 'Out-SafePRSubprocessLine — pipeline behavior' { + + It 'processes a multi-line array correctly' { + $lines = @( + 'first benign line', + '##vso[task.setvariable variable=X]Y', + 'second benign line', + '##[error]forged', + 'third benign line' + ) + $result = $lines | Invoke-AndCapture + $result | Should -HaveCount 5 + $result[0] | Should -Be 'first benign line' + Test-LineIsNeutralized $result[1] | Should -BeTrue + $result[2] | Should -Be 'second benign line' + Test-LineIsNeutralized $result[3] | Should -BeTrue + $result[4] | Should -Be 'third benign line' + } + + It 'handles non-string input (e.g. integers) by stringifying' { + $result = @(42, 'text', 3.14) | Invoke-AndCapture + $result | Should -HaveCount 3 + $result[0] | Should -Be '42' + $result[1] | Should -Be 'text' + $result[2] | Should -Be '3.14' + } + + It 'integrates with realistic dotnet-test-style output' { + $stdout = @( + 'Build started ...', + ' Project.csproj -> bin/Debug/net9.0/Project.dll', + 'Test run for Project.dll (.NET 9.0)', + 'Microsoft (R) Test Execution Command Line Tool', + '##vso[task.setvariable variable=detectedCategories;isOutput=true]NONE', # poisoned + 'Passed! - Failed: 0, Passed: 1, Skipped: 0, Total: 1, Duration: 1 s', + '' + ) + $result = $stdout | Invoke-AndCapture -Prefix ' ' + $result | Should -HaveCount 7 + # Poisoned line was neutralized + Test-LineIsNeutralized $result[4] | Should -BeTrue + # Other lines unchanged (modulo prefix) + $result[0] | Should -Be ' Build started ...' + $result[6] | Should -Be ' ' + } +} + +# Mid-line IndexOf bypasses — these test the exact attack class that +# review found bypassed an earlier +# anchored-detection version of the helper. The AzDO agent parser uses +# `IndexOf('##vso[')` / `IndexOf('##[')`, NOT `StartsWith`, so any +# non-whitespace prefix (e.g. test framework's `[INFO]`, `[xUnit.net …]`, +# `PASS:`, etc.) leaves the dangerous prefix in the parser's reach. +# These cases MUST fail against any sanitizer that anchors detection to +# line start. See commit 03e49dbb3c and the follow-up fix commit. +Describe 'Out-SafePRSubprocessLine — mid-line IndexOf bypasses (regression for adversarial finding)' { + + It 'neutralizes mid-line ##vso[ after leading non-whitespace text' { + $result = 'PASS [cat] ##vso[task.setvariable variable=detectedCategories;isOutput=true]NONE' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vso[ after xUnit timing prefix `[xUnit.net 00:00:01.23]`' { + $result = '[xUnit.net 00:00:01.23] ##vso[task.complete result=Failed]x' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vso[ after `[INFO] ` prefix' { + $result = '[INFO] ##vso[task.uploadfile]/etc/shadow' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##[error] after leading text' { + $result = 'NUnit run 1/5: ##[error]forged' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes ##vso[ after deeper-nested log prefix with brackets' { + $result = '[2025-01-01 12:34:56] [Verbose] [Cat: UI] ##vso[task.prependpath]/tmp/x' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'neutralizes multiple mid-line dangerous prefixes on the SAME line' { + $result = 'header ##vso[task.complete result=Failed]a tail ##[error]b' | Invoke-AndCapture + Test-LineIsNeutralized $result[0] | Should -BeTrue + } + + It 'still passes a benign mid-line `vso` substring through' { + # Make sure widening the matcher doesn't cause false positives on + # the word `vso` (without the dangerous `##` prefix). + $result = 'Mention of vso somewhere in a sentence is fine.' | Invoke-AndCapture + $result[0] | Should -Be 'Mention of vso somewhere in a sentence is fine.' + } +} + +# ─────────────────────────────────────────────────────────────────────────── +# — PR-derived `Write-Host` inventory in Review-PR.ps1 + +# The helper is only useful if every PR-derived echo site in the parent +# script actually routes through it. Three independent reviewers +# plus our own grep audit found multiple +# raw `Write-Host $variable` sites in Review-PR.ps1 where the variable +# is fully attacker-controlled: +# * `$prInfo.title` — PR title (own grep audit) +# * `$Prompt` — DryRun branch # * `$currentIntent` — `report_intent` text # * `$displayName`/`$detail` — agent tool args (same) + +# Each was historically `Write-Host $var -ForegroundColor X`. The fix in +# routes them through `,$var | Out-SafePRSubprocessLine`. These +# tests fail-closed by AST-walking Review-PR.ps1 and asserting every +# `Write-Host` call's interpolated arguments do NOT contain any of the +# known-PR-derived variable names. New PR-derived variables MUST be +# added to $PrDerivedVars below and routed through the sanitizer (or +# explicitly exempted with a comment justifying why they're trusted). +# ─────────────────────────────────────────────────────────────────────────── +Describe 'Review-PR.ps1 Write-Host inventory — no raw PR-derived echo (+ )' { + + BeforeAll { + $script:ReviewPRPath = Join-Path $PSScriptRoot '..' 'Review-PR.ps1' | Resolve-Path + $tokens = $null + $errors = $null + $script:ReviewPRAst = [System.Management.Automation.Language.Parser]::ParseFile( + $script:ReviewPRPath, [ref]$tokens, [ref]$errors) + if ($errors -and $errors.Count -gt 0) { + throw "Review-PR.ps1 has parse errors: $($errors[0].Message)" + } + + # Variables KNOWN to carry PR-controlled content. Adding to this + # list is intentional — it forces every new echo site to be + # audited for sanitization routing. + $script:PrDerivedVars = @( + 'Prompt', # full agent prompt — embeds PR title / body / diff / labels / etc. + 'currentIntent', # agent's report_intent text, which can quote PR content + 'displayName', # derived from tool name; bounded today but defense-in-depth + 'detail', # PR-derived tool args (description / intent / command / pattern / query / path / prompt) + 'prInfo', # PR title (and any other field we ever pull from `gh pr view`) + 'content', # `assistant.message` text (PR-aware agent output) + 'textContent', # non-JSON catch branch (raw agent stdout) + # Variables previously documented but not actually enforced. + # `$line` and `$output` flow through pipeline-style sanitization + # at multiple sites; `$modelName` is derived from PR-scope label + # parsing; `$resp` is `gh api` response body, which echoes back + # the request payload on error and is PR-aware. + 'line', # generic line variable used in test/UI sanitization loops + 'output', # generic captured-output buffer in subprocess sites + 'modelName', # PR-label-derived model identifier + 'resp', # `gh api` response body (error path) + 'reviewOutput', # captured output of post-ai-summary-comment subprocess + # Subprocess-capture variables. Each holds the 2>&1 stream of + # a `pwsh -NoProfile -File` invocation that runs PR-controlled + # code OR a trusted helper whose stdin/stdout passes through + # PR-aware content (gh api responses, PR diffs, etc.). Seeded + # explicitly so the taint closure flags any future raw + # `Write-Host $` re-emit without relying on the helper + # call sites already being correctly wrapped. + 'gateOutput', # captured 2>&1 of verify-tests-fail.ps1 (Gate phase) + 'detectOutput', # captured 2>&1 of detect-ui-test-categories.ps1 + 'refreshOutput', # captured 2>&1 of detect-ui-test-categories.ps1 (AI refresh path) + 'testOutput', # captured 2>&1 of regression-test runners + 'runResult' # Invoke-UITestWithRetry return object whose .Output flows ##vso-bearing lines + ) + + # Compute the TAINT CLOSURE: any local variable whose value is + # derived (via assignment) from a PR-derived variable inherits the + # taint. Without this, a bypass like + # $preview = $content.Trim() + # Write-Host $preview # ← $preview not in PrDerivedVars, so audit misses + # passes the inventory check. The walker performs a fixed-point + # forward def-use closure over the WHOLE file: each pass adds any + # AssignmentStatementAst.Left variable whose RHS subtree contains + # a reference to an already-tainted variable. Converges in 1-5 + # passes typically. + $assignAsts = $script:ReviewPRAst.FindAll( + { param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] -and + $n.Left -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true) + $taintedVars = [System.Collections.Generic.HashSet[string]]::new() + foreach ($v in $script:PrDerivedVars) { [void]$taintedVars.Add($v) } + $changed = $true + $passLimit = 16 + while ($changed -and $passLimit -gt 0) { + $changed = $false + $passLimit-- + foreach ($a in $assignAsts) { + $lhsName = $a.Left.VariablePath.UserPath + if ($taintedVars.Contains($lhsName)) { continue } + $varsInRhs = $a.Right.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true) + foreach ($v in $varsInRhs) { + if ($taintedVars.Contains($v.VariablePath.UserPath)) { + [void]$taintedVars.Add($lhsName) + $changed = $true + break + } + } + } + } + + # Override: variables that are type-bounded to NON-string-tainted + # values by their assignment pattern. The closure is conservative + # (any def-use reference taints), but these names are assigned + # ONLY to numeric counts, fixed emoji literals from script-internal + # hashtable lookups, or other structurally-safe values. They cannot + # carry an attacker `##vso[…]` payload. + # Each entry is documented to the site that justifies its safety. + $knownSafeOverrides = @( + 'icon', # $icon = $toolIcons[$toolName] — script-internal hashtable with literal emoji values only + 'apiMs', # $apiMs = [math]::Round($usage.totalApiDurationMs / 1000, 1) — numeric + 'filesChanged', # $filesChanged = @($changes.filesModified).Count — integer + 'linesAdded', # $linesAdded = $changes.linesAdded — numeric stat + 'linesRemoved', # $linesRemoved = $changes.linesRemoved — numeric stat + 'toolCount', # $toolCount++ — integer counter + 'turnCount', # $turnCount++ — integer counter + 'exitCode', # $exitCode = $LASTEXITCODE — integer + 'elapsed' # $elapsed = $stopwatch.Elapsed.ToString("mm\:ss") — formatted-time string from .NET TimeSpan + ) + foreach ($n in $knownSafeOverrides) { [void]$taintedVars.Remove($n) } + $script:PrDerivedVarsClosure = $taintedVars + + # Cmdlets whose stdout reaches the AzDO agent log parser. All of + # these are equivalent attack surfaces for `##vso[…]` injection. + # `Write-Output` and `Write-Information` route to the same stdout + # stream the parser scans; we audit them just like `Write-Host`. + # `[Console]::WriteLine` is covered by a dedicated test below + # (different AST shape — InvokeMemberExpressionAst). + # added Out-Host and + # Out-Default. Both reach the host stream that AzDO parses for + # ##vso[...] exactly like Write-Host — a future refactor that + # routes captured stdout through `$cliOut | Out-Host` or + # `$x | Out-Default` would silently bypass the inventory. + # Not added: Write-Error / Write-Warning — these emit to a + # different channel (stderr) and the agent's ##[error]/##[warning] + # parsing is separate; we don't currently route subprocess output + # to those cmdlets, and adding them creates noise from intentional + # human-readable warnings/errors. + $script:EmitterCmdlets = @('Write-Host', 'Write-Output', 'Write-Information', 'Out-Host', 'Out-Default') + } + + It 'has no raw Write-Host/Write-Output/Write-Information of a known-PR-derived variable (any expression shape)' { + # the prior split tests only + # caught direct variable args, single-level member access, and + # ExpandableString interpolation. They MISSED these idioms that + # equally reach AzDO stdout: + # * Concat: Write-Host ("PR: " + $prInfo.title) + # * Multi-member: Write-Host $prInfo.author.login + # * Index access: Write-Host $prInfo['title'] + # * Sub-expr: Write-Host $($prInfo.title.ToUpper()) + # * Other emitters: Write-Output / Write-Information + # `Ast.FindAll` traverses into BinaryExpressionAst, MemberExpressionAst, + # IndexExpressionAst, SubExpressionAst, ParenExpressionAst, AND + # ExpandableStringExpressionAst.NestedExpressions (verified empirically), + # so a single FindAll on each command-element subtree closes all the + # above shapes uniformly. CommandElements.Count >= 1 also catches + # the bare pipeline-as-emitter shape (`$tainted | Out-Host`) — without + # this, the walker would miss any future refactor that pipes tainted + # data into an arity-zero emitter. + $emitterCalls = $script:ReviewPRAst.FindAll( + { + param($n) + $n -is [System.Management.Automation.Language.CommandAst] -and + $n.CommandElements.Count -ge 1 -and + $n.CommandElements[0] -is [System.Management.Automation.Language.StringConstantExpressionAst] -and + ($script:EmitterCmdlets -contains $n.CommandElements[0].Value) + }, + $true + ) + $violations = @() + + # For bare-pipe emitters (`$x | Out-Host`), the variable lives in the + # UPSTREAM pipeline element, not in the emitter's arguments. Walk + # pipeline ancestors of each emitter call and add upstream + # CommandExpressionAst values to the inspection set. + foreach ($call in @($emitterCalls)) { + # Walk up to the enclosing PipelineAst (if any). + $p = $call.Parent + while ($null -ne $p -and $p -isnot [System.Management.Automation.Language.PipelineAst]) { + $p = $p.Parent + } + if ($null -eq $p) { continue } + # If the emitter is not the first element, harvest preceding + # CommandExpressionAst nodes — those carry the tainted variable. + for ($i = 0; $i -lt $p.PipelineElements.Count; $i++) { + $el = $p.PipelineElements[$i] + if ($el -eq $call) { break } + if ($el -is [System.Management.Automation.Language.CommandExpressionAst]) { + $upstreamVars = $el.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true) + foreach ($v in $upstreamVars) { + $name = $v.VariablePath.UserPath + if ($script:PrDerivedVarsClosure.Contains($name) -and -not (& $isSanitized $v) -and -not (& $isSafeMemberAccess $v)) { + $violations += "L$($call.Extent.StartLineNumber): bare-pipe `$$name | $($call.CommandElements[0].Value) (no sanitizer)" + } + } + } + } + } + + # Helper: a variable reference is "safe member access" if its + # IMMEDIATE parent is a MemberExpression accessing a member that + # cannot carry the tainted string content — count, length, etc. + # `$failedTools.Count` evaluates to an integer; the AzDO host + # parser will never see the tainted strings inside the collection. + # Same for `.Length`, `.Keys`, `.PSObject.TypeNames`, etc. + $safeMemberNames = [System.Collections.Generic.HashSet[string]]::new( + [string[]]@('Count','Length','Keys','GetType','PSObject','Type'), + [System.StringComparer]::OrdinalIgnoreCase) + $isSafeMemberAccess = { + param($v) + $p = $v.Parent + if ($p -is [System.Management.Automation.Language.MemberExpressionAst] -and + $p.Expression -eq $v -and + $p.Member -is [System.Management.Automation.Language.StringConstantExpressionAst] -and + $safeMemberNames.Contains($p.Member.Value)) { + return $true + } + return $false + } + + # is a BinaryExpressionAst with the `-replace` (Ireplace) operator + # and the right-hand side is the canonical AzDO logging-command + # pattern (`(?i)##(vso\[|\[)`). The RHS of -replace is parsed as + # an ArrayLiteralAst (regex + replacement); the regex is element 0. + # Walks up the .Parent chain; cheap and syntactic. + $isSanitized = { + param($node) + $cur = $node.Parent + while ($null -ne $cur) { + if ($cur -is [System.Management.Automation.Language.BinaryExpressionAst] -and + $cur.Operator -eq [System.Management.Automation.Language.TokenKind]::Ireplace) { + $regex = $null + if ($cur.Right -is [System.Management.Automation.Language.ArrayLiteralAst] -and + $cur.Right.Elements.Count -ge 1 -and + $cur.Right.Elements[0] -is [System.Management.Automation.Language.StringConstantExpressionAst]) { + $regex = $cur.Right.Elements[0].Value + } elseif ($cur.Right -is [System.Management.Automation.Language.StringConstantExpressionAst]) { + $regex = $cur.Right.Value + } + if ($regex -and $regex -match '\(\?i\)##\(vso\\\[\|\\\[\)') { + return $true + } + } + # Also accept a pipeline whose downstream element is + # Out-SafePRSubprocessLine — the pipeline form. + if ($cur -is [System.Management.Automation.Language.PipelineAst]) { + foreach ($el in $cur.PipelineElements) { + if ($el -is [System.Management.Automation.Language.CommandAst] -and + $el.CommandElements.Count -ge 1 -and + $el.CommandElements[0] -is [System.Management.Automation.Language.StringConstantExpressionAst] -and + $el.CommandElements[0].Value -eq 'Out-SafePRSubprocessLine') { + return $true + } + } + } + $cur = $cur.Parent + } + return $false + } + + foreach ($call in $emitterCalls) { + $emitterName = $call.CommandElements[0].Value + for ($i = 1; $i -lt $call.CommandElements.Count; $i++) { + $arg = $call.CommandElements[$i] + # Colon-form parameters like `Write-Host -Object:$prInfo` + # parse as a single CommandParameterAst whose `.Argument` + # carries the variable. Walk the Argument subtree explicitly. + if ($arg -is [System.Management.Automation.Language.CommandParameterAst]) { + if ($arg.Argument) { + $varRefs = $arg.Argument.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true + ) + if ($arg.Argument -is [System.Management.Automation.Language.VariableExpressionAst]) { + $varRefs = @($arg.Argument) + @($varRefs) + } + foreach ($v in $varRefs) { + $name = $v.VariablePath.UserPath + if ($script:PrDerivedVarsClosure.Contains($name) -and -not (& $isSanitized $v) -and -not (& $isSafeMemberAccess $v)) { + $violations += "L$($call.Extent.StartLineNumber): $emitterName -$($arg.ParameterName):`$$name (colon-form bypass)" + } + } + } + continue + } + $varRefs = $arg.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true + ) + # Include the arg itself if it IS a VariableExpressionAst (FindAll + # may not return the root node depending on PS version). + if ($arg -is [System.Management.Automation.Language.VariableExpressionAst]) { + $varRefs = @($arg) + @($varRefs) + } + foreach ($v in $varRefs) { + $name = $v.VariablePath.UserPath + if ($script:PrDerivedVarsClosure.Contains($name) -and -not (& $isSanitized $v) -and -not (& $isSafeMemberAccess $v)) { + $shape = $arg.GetType().Name -replace 'ExpressionAst$','' + $violations += "L$($call.Extent.StartLineNumber): $emitterName ... `$$name (arg shape: $shape)" + } + } + } + } + if ($violations.Count -gt 0) { + throw ("Found $($violations.Count) raw $($script:EmitterCmdlets -join '/') of PR-derived variable(s) in Review-PR.ps1:`n " + + (($violations | Sort-Object -Unique) -join "`n ") + + "`nRoute these through `,`$expr | Out-SafePRSubprocessLine` to neutralize `##vso[...]` injection.") + } + $violations | Should -BeNullOrEmpty + } + + It 'has no `[Console]::WriteLine`/Write of a known-PR-derived variable (incl. Out/Error chains)' { + # + : + # [Console]::WriteLine and [Console]::Write also write to stdout, + # bypassing Write-Host entirely. The check only matched + # DIRECT `[Console]::Write` calls; the reviewer pointed out + # that `[Console]::Out.WriteLine($var)` and `[Console]::Error.WriteLine` + # also write to the same FD and bypass the check (chained + # MemberExpression — InvokeMember.Expression is a MemberExpressionAst, + # not a TypeExpressionAst). + + # Generalized: walk the InvokeMember.Expression chain back to its + # root TypeExpressionAst and check it is Console / System.Console. + function script:Get-InvokeMemberRootType { + param($ast) + $cur = $ast.Expression + while ($cur -is [System.Management.Automation.Language.MemberExpressionAst]) { + $cur = $cur.Expression + } + if ($cur -is [System.Management.Automation.Language.TypeExpressionAst]) { + return $cur.TypeName.FullName + } + return $null + } + + $consoleCalls = $script:ReviewPRAst.FindAll( + { + param($n) + $n -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and + $n.Member -is [System.Management.Automation.Language.StringConstantExpressionAst] -and + $n.Member.Value -in @('WriteLine','Write') -and + (script:Get-InvokeMemberRootType $n) -in @('Console','System.Console') + }, + $true + ) + $violations = @() + foreach ($call in $consoleCalls) { + foreach ($arg in $call.Arguments) { + $varRefs = $arg.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] }, + $true + ) + if ($arg -is [System.Management.Automation.Language.VariableExpressionAst]) { + $varRefs = @($arg) + @($varRefs) + } + foreach ($v in $varRefs) { + $name = $v.VariablePath.UserPath + if ($script:PrDerivedVarsClosure.Contains($name)) { + $violations += "L$($call.Extent.StartLineNumber): [Console]…$($call.Member.Value)(...`$$name...)" + } + } + } + } + if ($violations.Count -gt 0) { + throw ("Found $($violations.Count) raw [Console]…Write/WriteLine of PR-derived variable(s) in Review-PR.ps1:`n " + + (($violations | Sort-Object -Unique) -join "`n ") + + "`nRoute these through `,`$expr | Out-SafePRSubprocessLine` to neutralize `##vso[...]` injection.") + } + $violations | Should -BeNullOrEmpty + } + + It 'has sanitizer routing at every documented PR-derived site' { + # Smoke-check that the sanitization actually exists in the file. If + # someone refactors away ALL the sanitization calls, this fails fast + # before the next reviewer round. + # Two equivalent sanitizer shapes are counted: + # * `| Out-SafePRSubprocessLine` — pipeline form, used for + # subprocess stdout where line-by-line streaming is natural. + # * Inline `-replace '(?i)##(vso\[|\[)', '##~SANITIZED~$1'` — used + # where the call site wants to keep `-ForegroundColor` / + # `-NoNewline` formatting that the helper doesn't propagate. + # Both apply identical neutralization (same pattern, same global + # replacement). The floor catches wholesale deletion of either. + $text = Get-Content $script:ReviewPRPath -Raw + $pipelineCount = ([regex]::Matches($text, 'Out-SafePRSubprocessLine')).Count + $inlineCount = ([regex]::Matches($text, "-replace\s+'\(\?i\)##\(vso\\\[\|\\\[\)'")).Count + $count = $pipelineCount + $inlineCount + $count | Should -BeGreaterOrEqual 10 -Because 'every PR-derived stdout echo and every PR-reachable subprocess invocation must route through one of the two sanitizer shapes (pipeline helper or inline -replace)' + } + + It 'never pipes a child script invoked IN-PROCESS (`& $var`) through `Out-SafePRSubprocessLine` with bare `2>&1` ( F1-CRIT regression guard)' { + # — CRITICAL: + # The wrap pattern `& $script ... 2>&1 | Out-SafePRSubprocessLine` + # is BROKEN when the child script uses Write-Host. In-process `& $var` + # invocations run the child in the parent's runspace; the child's + # Write-Host writes to the parent's host directly, BYPASSING the + # pipeline redirection. Only stderr is captured by `2>&1`. So an + # attacker-controlled filename in `gh pr diff` echoed via Write-Host + # by Find-RegressionRisks.ps1 reaches the AzDO log raw. + + # — the original guard only covered the + # single-pipeline shape `& $var ... 2>&1 | Out-Safe`. Two additional + # capture shapes exist in Review-PR.ps1's UI/device test runner: + # 1. CAPTURE-THEN-PIPE + # $out = & $var ... 2>&1 + # $out | Out-SafePRSubprocessLine + # Same Write-Host bypass — the in-process Write-Host never made + # it into $out, so the sanitizer has nothing to sanitize. + # 2. SCRIPTBLOCK-WRAPPED (`Invoke-WithoutGhTokens { & $var ... 2>&1 }`) + # The `& $var` is now nested inside a scriptblock argument, so + # the outer pipeline scanner doesn't see it. But the same + # in-process semantics apply. + # Both shapes are detected here. + + # Safe alternatives: + # * Subprocess: `& pwsh -NoProfile -File $script ... 2>&1 | Out-SafePRSubprocessLine` + # (the child has its own host; its Write-Host writes to ITS stdout, + # which the parent's 2>&1 then captures) + # * Merged streams: `& $script ... *>&1 | Out-SafePRSubprocessLine` + # (captures all streams 1-6 including Information / Write-Host) + + # This test detects the dangerous shape and fails fast. + + # Helper: a CommandAst is a "bypass-shape invocation" iff it is + # `& [args...] 2>&1` (not `*>&1`, not `& pwsh ...`). + $isBypassInvocation = { + param($cmd) + if ($cmd -isnot [System.Management.Automation.Language.CommandAst]) { return $false } + if ($cmd.InvocationOperator -ne [System.Management.Automation.Language.TokenKind]::Ampersand) { return $false } + if ($cmd.CommandElements.Count -lt 1) { return $false } + if ($cmd.CommandElements[0] -isnot [System.Management.Automation.Language.VariableExpressionAst]) { return $false } + # `*>&1` (FromStream = All) merges Write-Host into stdout — safe. + foreach ($r in $cmd.Redirections) { + if ($r -is [System.Management.Automation.Language.MergingRedirectionAst] -and + $r.FromStream -eq [System.Management.Automation.Language.RedirectionStream]::All) { + return $false + } + } + return $true + } + + # Helper: a CommandAst is `Out-SafePRSubprocessLine`. + $isSanitizerCall = { + param($cmd) + ($cmd -is [System.Management.Automation.Language.CommandAst]) -and + $cmd.CommandElements.Count -ge 1 -and + $cmd.CommandElements[0] -is [System.Management.Automation.Language.StringConstantExpressionAst] -and + $cmd.CommandElements[0].Value -eq 'Out-SafePRSubprocessLine' + } + + $violations = @() + + # Shape 1: same-pipeline `& $var ... 2>&1 | Out-Safe`. Walk every + # multi-element pipeline. + $pipelines = $script:ReviewPRAst.FindAll( + { + param($n) + $n -is [System.Management.Automation.Language.PipelineAst] -and + $n.PipelineElements.Count -ge 2 + }, + $true + ) + foreach ($pl in $pipelines) { + $head = $pl.PipelineElements[0] + $usesHelper = $false + for ($i = 1; $i -lt $pl.PipelineElements.Count; $i++) { + if (& $isSanitizerCall $pl.PipelineElements[$i]) { $usesHelper = $true; break } + } + if (-not $usesHelper) { continue } + if (& $isBypassInvocation $head) { + $vn = $head.CommandElements[0].VariablePath.UserPath + $violations += "L$($head.Extent.StartLineNumber): same-pipeline `& `$$vn ... 2>&1 | Out-SafePRSubprocessLine — BYPASSES Write-Host" + } + } + + # Shape 2 (-1): capture-then-pipe across two statements OR inside + # an `Invoke-WithoutGhTokens { ... }` scriptblock. We walk every + # bypass-shape invocation in the file and check whether its emitted + # value flows into Out-SafePRSubprocessLine WITHOUT a sanitizer-equivalent + # in between. We approximate "flows into" as: + # * the bypass invocation is the RHS of an AssignmentStatementAst + # whose LHS variable is later piped to Out-SafePRSubprocessLine. + # This catches the L969/L981 pattern even when wrapped in a scriptblock. + $allBypassInvocations = $script:ReviewPRAst.FindAll( + { param($n) & $isBypassInvocation $n }, + $true + ) + foreach ($cmd in $allBypassInvocations) { + # Walk up to the enclosing AssignmentStatementAst (if any). The + # CommandAst is wrapped in PipelineAst > optionally CommandExpressionAst + # > ... > AssignmentStatementAst.Right. + $node = $cmd.Parent + $assign = $null + while ($null -ne $node) { + if ($node -is [System.Management.Automation.Language.AssignmentStatementAst]) { + $assign = $node + break + } + # Stop at function / scriptblock boundaries that are NOT + # `Invoke-WithoutGhTokens { ... }` argument scriptblocks — + # those are still in the same logical capture. + if ($node -is [System.Management.Automation.Language.FunctionDefinitionAst]) { break } + $node = $node.Parent + } + if ($null -eq $assign) { + # Bypass invocation NOT captured. The Shape-1 check above + # already handled inline-pipe; if we get here, the result + # is discarded or used in some other dangerous shape. + # Belt-and-suspenders: still flag if the enclosing scope + # later refers to no helper at all — but to keep the test + # focused, only Shape-1 + Shape-2 are flagged. + continue + } + if ($assign.Left -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } + $capVar = $assign.Left.VariablePath.UserPath + + # Find the scope where the assignment lives. For the L969/L981 + # case the assignment is at script-body scope; we then look for + # any pipeline `$capVar | Out-SafePRSubprocessLine` in the same + # FunctionDefinition (or script body). + $scopeNode = $assign.Parent + while ($null -ne $scopeNode -and + $scopeNode -isnot [System.Management.Automation.Language.FunctionDefinitionAst] -and + $scopeNode -isnot [System.Management.Automation.Language.ScriptBlockAst]) { + $scopeNode = $scopeNode.Parent + } + if ($null -eq $scopeNode) { continue } + + $consumerFound = $false + # forward def-use walk through scalar + # copies. If `$mid = $capVar` and later `$mid | Out-Safe...`, + # the consumer-search above misses it. Build a small watch-set + # of "tainted" variable names: start with $capVar, then add any + # variable bound to a tainted variable via a plain Equals + # AssignmentStatement. Cap hops to keep search bounded. + $watchSet = @{} + $watchSet[$capVar] = $true + for ($hop = 0; $hop -lt 4; $hop++) { + $added = $false + $copyAssigns = $scopeNode.FindAll( + { + param($n) + $n -is [System.Management.Automation.Language.AssignmentStatementAst] -and + $n.Operator -eq [System.Management.Automation.Language.TokenKind]::Equals -and + $n.Left -is [System.Management.Automation.Language.VariableExpressionAst] + }, + $true + ) + foreach ($ca in $copyAssigns) { + $lhsName = $ca.Left.VariablePath.UserPath + if ($watchSet.ContainsKey($lhsName)) { continue } + # RHS is a CommandExpression wrapping a VariableExpression + # whose name is already in $watchSet. + $rhsExpr = $null + if ($ca.Right -is [System.Management.Automation.Language.PipelineAst] -and + $ca.Right.PipelineElements.Count -eq 1 -and + $ca.Right.PipelineElements[0] -is [System.Management.Automation.Language.CommandExpressionAst]) { + $rhsExpr = $ca.Right.PipelineElements[0].Expression + } elseif ($ca.Right -is [System.Management.Automation.Language.CommandExpressionAst]) { + $rhsExpr = $ca.Right.Expression + } + if ($rhsExpr -is [System.Management.Automation.Language.VariableExpressionAst] -and + $watchSet.ContainsKey($rhsExpr.VariablePath.UserPath)) { + $watchSet[$lhsName] = $true + $added = $true + } + } + if (-not $added) { break } + } + + $consumerPipelines = $scopeNode.FindAll( + { + param($n) + $n -is [System.Management.Automation.Language.PipelineAst] -and + $n.PipelineElements.Count -ge 2 + }, + $true + ) + foreach ($p in $consumerPipelines) { + $first = $p.PipelineElements[0] + # First element must reference any tainted variable. + $refsCap = $false + if ($first -is [System.Management.Automation.Language.CommandExpressionAst] -and + $first.Expression -is [System.Management.Automation.Language.VariableExpressionAst] -and + $watchSet.ContainsKey($first.Expression.VariablePath.UserPath)) { + $refsCap = $true + } elseif ($first -is [System.Management.Automation.Language.CommandAst]) { + # Walk subtree of first element for tainted variable refs. + $vars = $first.FindAll( + { param($n) $n -is [System.Management.Automation.Language.VariableExpressionAst] -and + $watchSet.ContainsKey($n.VariablePath.UserPath) }, + $true + ) + if ($vars.Count -gt 0) { $refsCap = $true } + } + if (-not $refsCap) { continue } + # Any downstream element calls Out-SafePRSubprocessLine? + for ($i = 1; $i -lt $p.PipelineElements.Count; $i++) { + if (& $isSanitizerCall $p.PipelineElements[$i]) { $consumerFound = $true; break } + } + if ($consumerFound) { break } + } + + if ($consumerFound) { + $violations += "L$($cmd.Extent.StartLineNumber): capture-then-pipe `$$capVar = & `$$($cmd.CommandElements[0].VariablePath.UserPath) ... 2>&1; `$$capVar | Out-SafePRSubprocessLine — BYPASSES Write-Host" + } + } + + if ($violations.Count -gt 0) { + throw ("Found $($violations.Count) in-process child-script invocation(s) that BYPASS the sanitizer for Write-Host:`n " + + (($violations | Sort-Object -Unique) -join "`n ") + + "`nThe `2>&1` redirection does NOT capture in-process Write-Host (which writes to the parent's host directly). Use `& pwsh -NoProfile -File