Security hardening for ci-copilot (#35677 stack): sanitize PR stdout, fail-closed Gate state, HMAC verdict, Phase-Audit

.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 -'
 ```

.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 = "<!-- AI Summary -->`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 `<!-- AI Summary -->` 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: `<!-- AI Gate -->` 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 = "<!-- AI Gate -->`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 = "<!-- AI Summary -->`nbot"
+                },
+                [pscustomobject]@{
+                    id = 2; node_id = 'IC_b'
+                    user = [pscustomobject]@{ login = 'attacker' }
+                    body = "<!-- AI Summary -->`nuser quote"
+                }
+            )
+        }
+        Hide-StaleMauiBotIssueComments -PRNumber 1 -IncludeAISummary -DryRun
+        $script:hiddenIds | Should -Contain 1
+        $script:hiddenIds | Should -Not -Contain 2
+    }
+}

.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'
+    }
+}