Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,10 @@ function Format-MarkdownCell {
# `List<T>` that GitHub markdown would otherwise swallow as an HTML tag. The
# engine's own markers are emitted via AppendLine, not through this formatter,
# so escaping cells never disturbs them.
return (($Value -replace "\|", "\|") -replace "<", "&lt;" -replace ">", "&gt;").Trim()
# Collapse embedded newlines first: a malformed upstream title can contain a
# literal CR/LF (observed: ci-scan issue #35957), which would otherwise split
# the markdown table row across physical lines and break the rendered table.
return ((($Value -replace "[\r\n]+", " ") -replace "\|", "\|") -replace "<", "&lt;" -replace ">", "&gt;").Trim()
}

function Format-GitHubHandle {
Expand Down
78 changes: 63 additions & 15 deletions .github/skills/release-readiness/scripts/Get-ReleaseReadiness.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -2814,6 +2814,43 @@ function ConvertTo-LinkedPr {
return "[#$PrNumber]($RepoUrl/pull/$PrNumber)"
}

function Format-MarkdownTableCell {
<#
.SYNOPSIS
Sanitize an arbitrary (often upstream-controlled) string for safe use inside a
single Markdown table cell. Also used for the candidate-PR bulleted list, where
the newline collapse matters and `\|` renders as `|`.
.DESCRIPTION
Two hazards are neutralized so a hostile/malformed issue or PR title cannot
corrupt the rendered body:
1. Embedded CR/LF runs are collapsed to a single space, so the value cannot
split the row across physical lines (observed live: ci-scan issue #35957,
whose title contained a literal newline).
2. Literal `|` is escaped to `\|`, so a pipe in a title cannot open a new
column (common in PR/issue titles such as `[Android] A | B`).
Every SR markdown cell that embeds upstream-controlled text routes through this
single helper: the ci-scan rows, the Open-PRs / regression / Blocking / Cleanup /
ship-readiness-checks / Open-Fix-PRs tables, and the candidate-PR list.

The SR engine deliberately omits `<`/`>` escaping (unlike Get-PreviewReadiness.ps1's
Format-MarkdownCell, whose newline+pipe contract this otherwise mirrors), preserving
title fidelity like `List<T>`. That omission is safe because:
* Hash-freeze: SR emits its own semantic hash at the TOP of the body, so an
injected `<!-- ...hash... -->` lower in the body can never win the workflow's
`head -n1` extraction — it is structurally immune (the Preview engine, being
hash-less, is not, which is why `<>` escaping is load-bearing there).
* Human-notes forgery: the workflow matches the `<!-- release-readiness:human-notes:
begin/end -->` preservation markers with FULL-LINE-ANCHORED regex (`^\s*<!-- ... -->\s*$`).
An injected marker can therefore only fire if it lands ALONE on a physical line,
which requires an embedded newline to break out of its surrounding row/list text.
The newline collapse in (1) removes that capability, so a raw `<>` in a title
cannot forge an anchored marker and wipe Release Captain Notes.
#>
param([string]$Value)
if ([string]::IsNullOrEmpty($Value)) { return '' }
return (($Value -replace '[\r\n]+', ' ') -replace '\|', '\|').Trim()
}

function Format-CiScanIssueRows {
<#
.SYNOPSIS
Expand Down Expand Up @@ -2842,7 +2879,10 @@ function Format-CiScanIssueRows {
}
}
$issLink = "[#$($iss.number)]($RepoUrl/issues/$($iss.number))"
$title = ($iss.title -replace '\|', '\|').Trim()
# Sanitize the upstream ci-scan title for a single Markdown table cell:
# collapse embedded CR/LF (observed: #35957) and escape pipes. See
# Format-MarkdownTableCell for the full rationale (and why SR omits `<>`).
$title = Format-MarkdownTableCell $iss.title
[void]$sb.AppendLine("| $marker$issLink | $title | $ageDisplay |")
}
if ($Issues.Count -gt $MaxRows) {
Expand Down Expand Up @@ -3105,9 +3145,9 @@ function Format-MarkdownReport {
[void]$sb.AppendLine('| Area | Details | Next action |')
[void]$sb.AppendLine('|---|---|---|')
foreach ($b in $blockingItems) {
$area = ($b.area -replace '\|', '\|').Trim()
$details = ($b.details -replace '\|', '\|').Trim()
$action = ($b.action -replace '\|', '\|').Trim()
$area = Format-MarkdownTableCell $b.area
$details = Format-MarkdownTableCell $b.details
$action = Format-MarkdownTableCell $b.action
[void]$sb.AppendLine("| $area | $details | $action |")
}
[void]$sb.AppendLine()
Expand Down Expand Up @@ -3142,9 +3182,9 @@ function Format-MarkdownReport {
[void]$sb.AppendLine('| Area | Details | Next action |')
[void]$sb.AppendLine('|---|---|---|')
foreach ($c in $cleanupItems) {
$area = ($c.area -replace '\|', '\|').Trim()
$details = ($c.details -replace '\|', '\|').Trim()
$action = ($c.action -replace '\|', '\|').Trim()
$area = Format-MarkdownTableCell $c.area
$details = Format-MarkdownTableCell $c.details
$action = Format-MarkdownTableCell $c.action
[void]$sb.AppendLine("| $area | $details | $action |")
}
[void]$sb.AppendLine()
Expand Down Expand Up @@ -3245,11 +3285,11 @@ function Format-MarkdownReport {
[void]$sb.AppendLine('| Fix PR | Base | Regression issue | Status | Next action |')
[void]$sb.AppendLine('|---|---|---|---|---|')
foreach ($row in $openFixRows) {
$prCell = ($row.prCell -replace '\|', '\|').Trim()
$baseCell = ($row.baseCell -replace '\|', '\|').Trim()
$issCell = ($row.issCell -replace '\|', '\|').Trim()
$statCell = ($row.statusCell -replace '\|', '\|').Trim()
$actCell = ($row.actionCell -replace '\|', '\|').Trim()
$prCell = Format-MarkdownTableCell $row.prCell
$baseCell = Format-MarkdownTableCell $row.baseCell
$issCell = Format-MarkdownTableCell $row.issCell
$statCell = Format-MarkdownTableCell $row.statusCell
$actCell = Format-MarkdownTableCell $row.actionCell
[void]$sb.AppendLine("| $prCell | $baseCell | $issCell | $statCell | $actCell |")
}
[void]$sb.AppendLine()
Expand All @@ -3270,9 +3310,9 @@ function Format-MarkdownReport {
'CLEANUP' { '🧹 CLEANUP' }
default { "⚪ $($sc.Status)" }
}
$area = ($sc.Area -replace '\|', '\|').Trim()
$details = ($sc.Details -replace '\|', '\|').Trim()
$action = ($sc.NextAction -replace '\|', '\|').Trim()
$area = Format-MarkdownTableCell $sc.Area
$details = Format-MarkdownTableCell $sc.Details
$action = Format-MarkdownTableCell $sc.NextAction
[void]$sb.AppendLine("| $area | $statusEmoji | $details | $action |")
}
[void]$sb.AppendLine()
Expand Down Expand Up @@ -3368,6 +3408,12 @@ function Format-MarkdownReport {
foreach ($cp in $candidatePrs) {
$cpLink = ConvertTo-LinkedPr -PrNumber $cp.number -RepoUrl $RepoUrl
$cpTitle = if ($cp.title.Length -gt 80) { $cp.title.Substring(0, 80) + '...' } else { $cp.title }
# Collapse newlines (and escape pipes) even though this is a list, not a
# table: an upstream title with an embedded newline could otherwise push
# injected content (e.g. a forged `<!-- release-readiness:human-notes -->`
# marker) onto its own physical line. Markdown renders `\|` as `|` in a
# list, so escaping is harmless here.
$cpTitle = Format-MarkdownTableCell $cpTitle
[void]$sb.AppendLine("- $cpLink — $cpTitle (by $(Format-GitHubHandle $cp.author.login), updated $($cp.updatedAt))")
}
[void]$sb.AppendLine()
Expand All @@ -3381,6 +3427,7 @@ function Format-MarkdownReport {
[void]$sb.AppendLine('|---|---|---|---|---|---|')
foreach ($pr in $Data['openSrPrs']) {
$title = if ($pr.title.Length -gt 60) { $pr.title.Substring(0, 60) + '...' } else { $pr.title }
$title = Format-MarkdownTableCell $title
$draft = if ($pr.isDraft) { '✏️' } else { '' }
$rev = if ($pr.reviewDecision) { $pr.reviewDecision } else { '—' }
$prLink = ConvertTo-LinkedPr -PrNumber $pr.number -RepoUrl $RepoUrl
Expand Down Expand Up @@ -3434,6 +3481,7 @@ function Format-MarkdownReport {
# Stable sort: by issue number ascending
foreach ($it in ($items | Sort-Object issue)) {
$title = if ($it.title.Length -gt 50) { $it.title.Substring(0, 50) + '...' } else { $it.title }
$title = Format-MarkdownTableCell $title

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 [Suggestion] Defense-in-depth: Engine-controlled cells bypass sanitizer

$title correctly routes through Format-MarkdownTableCell, but recommendedAction (L3488), CI-pipeline cells (L3350), and Reverts table cells (L3380) are interpolated raw.

Safe today — All contain only engine-generated constants/issue numbers (L1914–2309), never upstream titles.

Future risk — If someone later embeds an upstream title in these cells, it would silently bypass sanitization and re-open row-split/pipe-injection vectors.

Optional mitigation: Route all table cells through Format-MarkdownTableCell, or add comments marking them as "engine-controlled, intentionally unsanitized."

(Opus finding, validated by Opus cross-poll, not contradicted by peers)

$prList = @($it.candidateFixPrs | ForEach-Object { ConvertTo-LinkedPr -PrNumber $_.number -RepoUrl $RepoUrl }) -join ', '
if (-not $prList) { $prList = '—' }
$issueLink = if ($RepoUrl) { "[#$($it.issue)]($RepoUrl/issues/$($it.issue))" } else { "#$($it.issue)" }
Expand Down
Loading
Loading