[PR #232] [MERGED] feat(public-repo-hygiene): scan the PR title and description too (BE-9652) #239

New issue

Closed

opened 2026-10-26 22:37:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:40 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/232
Author: @mattmillerai
Created: 8/26/2026
Status: ✅ Merged
Merged: 8/26/2026
Merged by: @mattmillerai

Base: main ← Head: matt/hygiene-pr-text

📝 Commits (1)

7960247 feat(public-repo-hygiene): scan the PR title and description too (BE-9652)

📊 Changes

5 files changed (+393 additions, -6 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+31 -0)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+163 -6)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+126 -0)
📝 .github/workflows/public-repo-hygiene.yml (+57 -0)
📝 docs/callers/public-repo-hygiene.md (+16 -0)

📄 Description

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 the
check 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:

an internal collaboration-tool permalink pasted into a description, exposing a
workspace and a channel id;
a non-public org repo named in a body.

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_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

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 ticket
id 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_ref rather than the caller's checkout.

Passed as file paths, never argv; interpolated only into env:, never into
run:. A PR title is an attacker-controlled string, and ${{ … }} inside a
shell script is textual substitution before the shell sees it — $(…) 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
can'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

A repo named without the org prefix is not caught, on any surface. Category 3
keys on Comfy-Org/<name>; a bare name in prose is indistinguishable from an
ordinary 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.
PR text is only re-scanned when the caller's on: includes edited. GitHub's
default pull_request types don't fire on a title/body edit, so without it a PR
can 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 was
configured with — so the caller guide now carries it, with the types: line
marked 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 is
fixed 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 — clean
actionlint — only the two findings that already exist on this file (job.workflow_sha, SC2018/19)
Run end-to-end against the real leaked PR text: the permalink is caught, the prefixed repo reference is caught, (BE-9651) in the title is correctly ignored

Provenance

Written by @mattmillerai with Claude Code. Rebased by hand onto #231, which
refactored this checker into _line_findings + _path_findings while this was in
flight — the categories kwarg follows the url_suppressors pattern that PR
established rather than reintroducing a parallel structure.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/232 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/26/2026 **Status:** ✅ Merged **Merged:** 8/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/hygiene-pr-text` --- ### 📝 Commits (1) - [`7960247`](https://github.com/Comfy-Org/github-workflows/commit/7960247e0b5c7a48a74f2bbd520091fa42471bf5) feat(public-repo-hygiene): scan the PR title and description too (BE-9652) ### 📊 Changes **5 files changed** (+393 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+31 -0) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+163 -6) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+126 -0) 📝 `.github/workflows/public-repo-hygiene.yml` (+57 -0) 📝 `docs/callers/public-repo-hygiene.md` (+16 -0) </details> ### 📄 Description ## 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 the check 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: - an internal collaboration-tool permalink pasted into a description, exposing a workspace and a channel id; - a non-public org repo named in a body. 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_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 **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 ticket id 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_ref` rather than the caller's checkout. **Passed as file paths, never argv; interpolated only into `env:`, never into `run:`.** A PR title is an attacker-controlled string, and `${{ … }}` inside a shell script is textual substitution before the shell sees it — `$(…)` 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` can'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 - **A repo named without the org prefix is not caught, on any surface.** Category 3 keys on `Comfy-Org/<name>`; a bare name in prose is indistinguishable from an ordinary 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. - **PR text is only re-scanned when the caller's `on:` includes `edited`.** GitHub's default `pull_request` types don't fire on a title/body edit, so without it a PR can 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 was configured with — so the caller guide now carries it, with the `types:` line marked 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 is fixed 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` — clean - `actionlint` — only the two findings that already exist on this file (`job.workflow_sha`, SC2018/19) - Run end-to-end against the **real** leaked PR text: the permalink is caught, the prefixed repo reference is caught, `(BE-9651)` in the title is correctly ignored ## Provenance Written by @mattmillerai with Claude Code. Rebased by hand onto #231, which refactored this checker into `_line_findings` + `_path_findings` while this was in flight — the `categories` kwarg follows the `url_suppressors` pattern that PR established rather than reintroducing a parallel structure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:40 +00:00