diff --git a/.github/scripts/Apply-PRFinalize.Tests.ps1 b/.github/scripts/Apply-PRFinalize.Tests.ps1 index 5d3d6faa4717..0459aa630e3f 100644 --- a/.github/scripts/Apply-PRFinalize.Tests.ps1 +++ b/.github/scripts/Apply-PRFinalize.Tests.ps1 @@ -37,7 +37,8 @@ BeforeAll { 'Test-FinalizeIsNoOp', 'Get-FinalizeRecommendation', 'Merge-PreservedTitlePrefix', - 'Merge-PreservedBodyPreamble' + 'Merge-PreservedBodyPreamble', + 'New-ExclusiveTempFile' )) { $function = $ast.Find({ $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] -and @@ -317,3 +318,193 @@ Describe 'Security regressions — AzDO logging-command injection' { Should -Be '[iOS] new' } } + +Describe 'New-ExclusiveTempFile' { + BeforeAll { + $script:SandboxDir = Join-Path ([System.IO.Path]::GetTempPath()) "apply-prfinalize-tests-$([System.IO.Path]::GetRandomFileName())" + New-Item -ItemType Directory -Path $script:SandboxDir -Force | Out-Null + $script:OriginalAgentTemp = $env:AGENT_TEMPDIRECTORY + $env:AGENT_TEMPDIRECTORY = $script:SandboxDir + } + + AfterAll { + $env:AGENT_TEMPDIRECTORY = $script:OriginalAgentTemp + Remove-Item -LiteralPath $script:SandboxDir -Recurse -Force -ErrorAction SilentlyContinue + } + + It 'creates the file inside AGENT_TEMPDIRECTORY when it is set' { + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + try { + Test-Path -LiteralPath $path | Should -BeTrue + (Split-Path -Parent $path) | Should -Be $script:SandboxDir + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + } + } + + It 'produces a unique path on each call, so the name is not predictable' { + $a = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + $b = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + try { + $a | Should -Not -Be $b + } finally { + Remove-Item -LiteralPath $a -Force -ErrorAction SilentlyContinue + Remove-Item -LiteralPath $b -Force -ErrorAction SilentlyContinue + } + } + + # These pin the actual finding-#2 behaviour. The earlier version of this test was + # vacuous: it never planted a symlink at a path the helper would try, so a + # predictable-name Set-Content write-through implementation still passed it. The + # -NameGenerator seam forces known candidates so a symlink can be planted precisely. + It 'refuses a pre-planted symlink and leaves its target untouched' { + $secret = Join-Path $script:SandboxDir 'secret-existing.txt' + Set-Content -LiteralPath $secret -Value 'ORIGINAL' -Encoding UTF8 + + $planted = Join-Path $script:SandboxDir 'pr-finalize-body-123-forced0.md' + New-Item -ItemType SymbolicLink -Path $planted -Target $secret | Out-Null + + # $script: scope is required — a plain $i++ inside the scriptblock would mutate a + # local copy, so every attempt would re-request the planted name. + $script:ForcedIndex = 0 + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator { + $n = "forced$($script:ForcedIndex)"; $script:ForcedIndex++; $n + } + try { + # It must have skipped the planted path entirely... + $path | Should -Not -Be $planted + 'REPLACEMENT BODY' | Set-Content -LiteralPath $path -Encoding UTF8 + # ...so the symlink target is untouched, and the link is still a link. + (Get-Content -Raw -LiteralPath $secret).Trim() | Should -Be 'ORIGINAL' + (Get-Item -LiteralPath $planted).LinkType | Should -Be 'SymbolicLink' + (Get-Content -Raw -LiteralPath $path).Trim() | Should -Be 'REPLACEMENT BODY' + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + Remove-Item -LiteralPath $planted -Force -ErrorAction SilentlyContinue + } + } + + It 'refuses a dangling pre-planted symlink rather than creating its target' { + $missingTarget = Join-Path $script:SandboxDir 'never-created.txt' + $planted = Join-Path $script:SandboxDir 'pr-finalize-body-123-dangle0.md' + New-Item -ItemType SymbolicLink -Path $planted -Target $missingTarget | Out-Null + + $script:DangleIndex = 0 + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator { + $n = "dangle$($script:DangleIndex)"; $script:DangleIndex++; $n + } + try { + $path | Should -Not -Be $planted + 'REPLACEMENT BODY' | Set-Content -LiteralPath $path -Encoding UTF8 + # Writing through a dangling link would have created the target. + Test-Path -LiteralPath $missingTarget | Should -BeFalse + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + Remove-Item -LiteralPath $planted -Force -ErrorAction SilentlyContinue + } + } + + It 'throws instead of falling back to a predictable path when every candidate is occupied' { + $secret = Join-Path $script:SandboxDir 'secret-exhaust.txt' + Set-Content -LiteralPath $secret -Value 'ORIGINAL' -Encoding UTF8 + + $planted = 0..4 | ForEach-Object { + $link = Join-Path $script:SandboxDir "pr-finalize-body-123-exhaust$_.md" + New-Item -ItemType SymbolicLink -Path $link -Target $secret | Out-Null + $link + } + + try { + $script:ExhaustIndex = 0 + { New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator { + $n = "exhaust$($script:ExhaustIndex)"; $script:ExhaustIndex++; $n + } } | Should -Throw -ExpectedMessage '*after 5 attempts*' + # No fallback path was written, so the symlink target is still intact. + (Get-Content -Raw -LiteralPath $secret).Trim() | Should -Be 'ORIGINAL' + } finally { + $planted | ForEach-Object { Remove-Item -LiteralPath $_ -Force -ErrorAction SilentlyContinue } + } + } + + It 'tries exactly MaxAttempts candidates before giving up' { + $secret = Join-Path $script:SandboxDir 'secret-count.txt' + Set-Content -LiteralPath $secret -Value 'ORIGINAL' -Encoding UTF8 + $planted = 0..4 | ForEach-Object { + $link = Join-Path $script:SandboxDir "pr-finalize-body-123-count$_.md" + New-Item -ItemType SymbolicLink -Path $link -Target $secret | Out-Null + $link + } + + try { + $script:Calls = 0 + { New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' -NameGenerator { $n = "count$($script:Calls)"; $script:Calls++; $n } } | + Should -Throw + $script:Calls | Should -Be 5 + } finally { + $planted | ForEach-Object { Remove-Item -LiteralPath $_ -Force -ErrorAction SilentlyContinue } + } + } + + It 'surfaces a non-collision failure immediately instead of retrying it away' { + # A missing base directory can never be resolved by picking another name, so it must + # propagate rather than be masked by the generic "after N attempts" message. + $saved = $env:AGENT_TEMPDIRECTORY + $env:AGENT_TEMPDIRECTORY = $script:SandboxDir + try { + $script:Calls = 0 + { New-ExclusiveTempFile -Prefix 'missing-dir/nope/body' -NameGenerator { $script:Calls++; 'x' } } | + Should -Throw -ExpectedMessage '*Could not find a part of the path*' + $script:Calls | Should -Be 1 + } finally { + $env:AGENT_TEMPDIRECTORY = $saved + } + } + + It 'ignores AGENT_TEMPDIRECTORY when it points at a file rather than a directory' { + $saved = $env:AGENT_TEMPDIRECTORY + $asFile = Join-Path $script:SandboxDir 'not-a-directory.txt' + Set-Content -LiteralPath $asFile -Value 'x' -Encoding UTF8 + $env:AGENT_TEMPDIRECTORY = $asFile + try { + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + try { + Test-Path -LiteralPath $path | Should -BeTrue + (Split-Path -Parent $path) | Should -Not -Be $asFile + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + } + } finally { + $env:AGENT_TEMPDIRECTORY = $saved + } + } + + It 'falls back to the system temp directory when AGENT_TEMPDIRECTORY is unset' { + $saved = $env:AGENT_TEMPDIRECTORY + $env:AGENT_TEMPDIRECTORY = $null + try { + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + try { + Test-Path -LiteralPath $path | Should -BeTrue + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + } + } finally { + $env:AGENT_TEMPDIRECTORY = $saved + } + } + + It 'ignores AGENT_TEMPDIRECTORY when it points at a missing directory' { + $saved = $env:AGENT_TEMPDIRECTORY + $env:AGENT_TEMPDIRECTORY = Join-Path $script:SandboxDir 'does-not-exist' + try { + $path = New-ExclusiveTempFile -Prefix 'pr-finalize-body-123' + try { + Test-Path -LiteralPath $path | Should -BeTrue + } finally { + Remove-Item -LiteralPath $path -Force -ErrorAction SilentlyContinue + } + } finally { + $env:AGENT_TEMPDIRECTORY = $saved + } + } +} diff --git a/.github/scripts/Review-PR.ps1 b/.github/scripts/Review-PR.ps1 index 2814a1fc5d46..4aa2a325ca34 100644 --- a/.github/scripts/Review-PR.ps1 +++ b/.github/scripts/Review-PR.ps1 @@ -2413,7 +2413,11 @@ if ($env:SKIP_PR_FINALIZE_APPLY -eq 'true') { if ($DryRun) { $applyArgs.DryRun = $true } & $applyFinalizeScript @applyArgs } catch { - Write-Host " ⚠️ Failed to apply PR title/description (non-fatal): $_" -ForegroundColor Yellow + # Backstop, not a live path: the child sanitizes its own console output and its + # one throw carries no PR-derived text today. Kept because a future throw that + # quotes the recommendation would otherwise reach stdout unsanitized, and every + # other console sink in this script already goes through the sanitizer. + Write-Host " ⚠️ Failed to apply PR title/description (non-fatal): $(ConvertTo-AzdoSafeConsole "$_")" -ForegroundColor Yellow } } else { Write-Host " ⚠️ apply-pr-finalize.ps1 not found — skipping" -ForegroundColor Yellow diff --git a/.github/scripts/apply-pr-finalize.ps1 b/.github/scripts/apply-pr-finalize.ps1 index b7d317836529..fdd3851a31e2 100644 --- a/.github/scripts/apply-pr-finalize.ps1 +++ b/.github/scripts/apply-pr-finalize.ps1 @@ -254,6 +254,66 @@ function Merge-PreservedBodyPreamble { return "$preamble`n`n$RecommendedBody" } +function New-ExclusiveTempFile { + <# + .SYNOPSIS + Creates a temp file at a fresh, unpredictable path, never writing to one that exists. + .DESCRIPTION + `Set-Content` follows a pre-existing symlink and writes through to its target, so a + predictable temp path (pr-finalize-body-.md) is a write-through primitive if + anything can pre-create it. Reaching that requires arbitrary filesystem write as the + agent user, which already grants strictly more capability — but the fix is cheap, so + close it anyway rather than relying on that argument holding. + + Prefers the AzDO agent temp directory over the shared system temp when available. + Creating with New-Item (no -Force) refuses any path that already exists, including a + pre-planted or dangling symlink, so the write cannot be redirected. An occupied path + is skipped for a fresh random name; after $MaxAttempts the helper throws rather than + falling back to a predictable path, so exhaustion can never reopen the vector. + .PARAMETER NameGenerator + Test seam only. Lets a test force known candidate names so it can pre-plant a symlink + at the exact path the helper will try. Production uses random names. + .OUTPUTS + Full path to the newly created file. + #> + param( + [Parameter(Mandatory = $true)] + [string]$Prefix, + + [scriptblock]$NameGenerator = { [System.IO.Path]::GetRandomFileName() }, + + [int]$MaxAttempts = 5 + ) + + # -PathType Container so a stale AGENT_TEMPDIRECTORY pointing at a *file* falls back + # cleanly instead of failing later inside New-Item. + $baseDir = if ($env:AGENT_TEMPDIRECTORY -and (Test-Path -LiteralPath $env:AGENT_TEMPDIRECTORY -PathType Container)) { + $env:AGENT_TEMPDIRECTORY + } else { + [System.IO.Path]::GetTempPath() + } + + for ($attempt = 0; $attempt -lt $MaxAttempts; $attempt++) { + $candidate = Join-Path $baseDir "$Prefix-$(& $NameGenerator).md" + try { + $file = New-Item -ItemType File -Path $candidate -ErrorAction Stop + return $file.FullName + } catch [System.IO.DirectoryNotFoundException] { + # Derives from IOException, so it must be caught ahead of the collision case — + # a missing base directory will never resolve by picking another name. + throw + } catch [System.IO.IOException] { + # The path is occupied (regular file, or a pre-planted/dangling symlink). Skip it + # rather than write through, and try a different name. + continue + } + # Anything else (access denied, invalid path) is a real fault: let it surface + # unwrapped instead of being retried into a generic "after N attempts" message. + } + + throw "Could not create a temp file under '$baseDir' after $MaxAttempts attempts." +} + # ─── Main ─────────────────────────────────────────────────────────────────────── # Dot-sourced by the Pester suite to test the helpers above without executing the flow. if ($MyInvocation.InvocationName -eq '.') { return } @@ -340,8 +400,9 @@ if ($DryRun) { exit 0 } -$bodyFile = Join-Path ([System.IO.Path]::GetTempPath()) "pr-finalize-body-$PRNumber.md" +$bodyFile = $null try { + $bodyFile = New-ExclusiveTempFile -Prefix "pr-finalize-body-$PRNumber" $newBody | Set-Content -LiteralPath $bodyFile -Encoding UTF8 $ghArgs = @('pr', 'edit', "$PRNumber", '--repo', $Repo) @@ -359,7 +420,7 @@ try { } catch { Write-Host " ⚠️ Failed to apply the PR finalize recommendation (non-fatal): $(ConvertTo-AzdoSafeConsole "$_")" -ForegroundColor Yellow } finally { - Remove-Item -LiteralPath $bodyFile -Force -ErrorAction SilentlyContinue + if ($bodyFile) { Remove-Item -LiteralPath $bodyFile -Force -ErrorAction SilentlyContinue } } exit 0