feat(public-repo-hygiene): scan the PR title and description too (BE-9652) - #232
Merged
Conversation
…9652) PR text is a leak surface with no guard at all. It is published on a public repo the moment it is typed, and no file scan can ever see it -- the checker walks tracked files, and a PR title and body are neither. Both motivating leaks are real and happened on a public PR of THIS repo: an internal collaboration-tool permalink pasted into a description (exposing a workspace and a channel id), and a non-public org repo named in a body. Neither is caught anywhere today. This adds PR title and description as a third surface, alongside a file's contents and (since #231) its tracked path. All three go through the one `_line_findings` matcher rather than a fork of it, for the reason #231 already gives: a second matcher is a second place to forget an allowlist entry. CATEGORIES 2 AND 3 ONLY -- NEVER CATEGORY 1 The ticket category is deliberately excluded from this surface, and that is a policy decision, not an oversight. This org's commit convention REQUIRES a `(BE-####)` Linear suffix on the PR title, so a ticket id in a public PR title is deliberate org-wide practice rather than a leak: half of this repo's own recent merged PR titles carry one. Scanning for it here would fail roughly every second PR on every enrolled repo, and a required check that fires on CORRECT behaviour does not get fixed -- it gets switched off, taking the two categories that catch real leaks with it. A test pins this, in both directions: the same string is still flagged in a tracked file. TWO PROPERTIES THAT ARE SECURITY, NOT STYLE Read from the EVENT, never from a workflow input. A caller that could supply the text being judged could supply different text -- the same reasoning that loads the checker from `workflows_ref` rather than the caller's checkout. Passed to the checker as FILE PATHS, never as argv values, and interpolated only into an `env:` value, never into a `run:` body. A PR title and body are attacker-controlled strings; `${{ ... }}` inside a shell script is textual substitution before the shell sees it, so `$(...)` in a PR title would execute on the runner. The workflow writes them from `env:` to RUNNER_TEMP -- outside the scanned tree -- and passes `--scan-text 'LABEL=PATH'`. Surface findings are reported BEFORE file findings so `MAX_FINDINGS_TOTAL` cannot truncate away the highest-signal, cheapest-to-fix ones. A surface named but unreadable FAILS (exit 2) rather than scanning an empty string: a green run over text nobody read is the silent pass this checker exists to deny. TWO LIMITATIONS, DOCUMENTED RATHER THAN IMPLIED A repo named WITHOUT the org prefix is not caught on any surface -- a bare name in prose is indistinguishable from an ordinary word without a list of non-public names, and that list is exactly what cannot live in a public repo. Of the three real leaks above, this catches two. PR text is only re-scanned when the caller's `on:` includes `edited`; GitHub's default `pull_request` types do not fire on a title or body edit, so without it a PR can go green and then have a link pasted in. The reusable cannot enforce this -- `on:` belongs to the caller and a workflow cannot read the trigger types it was configured with -- so the caller guide now carries it, with the `types:` line marked load-bearing in the copy-pasteable example. Verified: 196 tests pass (184 upstream + 12 new), workflow-pins lint clean, actionlint reports only the two pre-existing findings on this file, and the checker was run end to end against the real leaked PR text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 122 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
The leak checker reads every file in the repo. But a pull request's title and
description aren't files — they're published on the public repo the moment you
type them, and nothing was looking at them.
So you could commit a clean repo, pass the check green, and paste an internal
link straight into the PR description with nobody noticing. This makes the
checker read those two boxes as well.
It skips ticket numbers there on purpose, because our own commit rules tell you
to put
(BE-####)in a PR title — flagging that would fail half our PRs and thecheck would just get turned off.
Why
PR text is a leak surface with no guard at all. Both motivating cases are real
and happened on a public PR of this repo:
workspace and a channel id;
Neither is caught anywhere today — the checker walks tracked files, and PR text is
not one.
What this does
Adds PR title and description as a third surface, next to a file's contents and
(since #231) its tracked path. All three go through the one
_line_findingsmatcher rather than a fork of it, for the reason #231 already gives: a second
matcher is a second place to forget an allowlist entry.
Categories 2 and 3 only — never category 1
This is the design decision worth reviewing. The ticket category is excluded
from this surface deliberately.
Our commit convention requires a
(BE-####)suffix on the PR title, so a ticketid in a public PR title is org-wide practice, not a leak — half of this repo's
own recent merged PR titles carry one. Scanning for it here would fail roughly
every second PR on every enrolled repo, and a required check that fires on
correct behaviour doesn't get fixed; it gets switched off, taking the two
categories that catch real leaks with it.
A test pins this in both directions — the same string is still flagged in a
tracked file, so it's a per-surface policy, not a hole in the category.
Two properties that are security, not style
Read from the event, never from a workflow input. A caller that could supply
the text being judged could supply different text — the same reasoning that
loads the checker from
workflows_refrather than the caller's checkout.Passed as file paths, never argv; interpolated only into
env:, never intorun:. A PR title is an attacker-controlled string, and${{ … }}inside ashell script is textual substitution before the shell sees it —
$(…)in a PRtitle would execute on the runner. The workflow writes them from
env:toRUNNER_TEMP(outside the scanned tree) and passes--scan-text 'LABEL=PATH'.Surface findings are reported before file findings so
MAX_FINDINGS_TOTALcan't truncate away the highest-signal, cheapest-to-fix ones. A surface named but
unreadable fails (exit 2) rather than scanning an empty string — a green run
over text nobody read is the silent pass this checker exists to deny.
Two limitations, documented rather than implied
keys on
Comfy-Org/<name>; a bare name in prose is indistinguishable from anordinary word without a list of non-public names — and that list is precisely what
cannot live in a public repo. Of the three real leaks above, this catches two.
on:includesedited. GitHub'sdefault
pull_requesttypes don't fire on a title/body edit, so without it a PRcan go green and then have a link pasted in. The reusable can't enforce this —
on:belongs to the caller, and a workflow can't read the trigger types it wasconfigured with — so the caller guide now carries it, with the
types:linemarked load-bearing in the copy-pasteable example.
Behaviour change for existing callers
The scan is on by default whenever the event carries a PR — there's no opt-out
input, deliberately: a blanket off-switch for a whole category is weaker than the
existing
exclude_paths, and unlike a tracked file, a false positive here isfixed by editing a PR description, which takes seconds. Roster is currently two
public SDK repos, so the blast radius is small — but this needs a caller bump to
take effect.
Verification
python3 -m unittest discover -s .github/public-repo-hygiene/tests— 196 passed (184 upstream + 12 new)python3 .github/workflow-pins/check_workflow_pins.py— cleanactionlint— only the two findings that already exist on this file (job.workflow_sha, SC2018/19)(BE-9651)in the title is correctly ignoredProvenance
Written by @mattmillerai with Claude Code. Rebased by hand onto #231, which
refactored this checker into
_line_findings+_path_findingswhile this was inflight — the
categorieskwarg follows theurl_suppressorspattern that PRestablished rather than reintroducing a parallel structure.
🤖 Generated with Claude Code