feat(public-repo-hygiene): scan the tracked path string, not just file contents - #231
Merged
Merged
Conversation
…e contents A tracked entry publishes three strings, and the checker only read two of them. A tree holding `docs/Comfy-Org/<a-private-repo>/placeholder.md` names that repo to anyone who browses or clones it — and the run reported clean, because the reference regexes were applied to file CONTENTS and symlink target strings but never to the tracked PATH. Factor the per-line matcher out of `_file_findings` into a shared `_line_findings(location, line, ticket_allowlist, owner_span)` and run it over `rel` in the scan loop, right after the exclusion check and before `check_file`. Deliberately ONE matcher, not two: the allowlists, the model-host and markdown-label suppressors and both caller-side knobs (`ticket_allowlist:`, `exclude_paths:`) reach the new surface for free, and there is no second place to forget an allowlist entry. - Runs for EVERY non-excluded entry, including the ones `check_file` declines (binary, non-UTF-8, gitlink, FIFO, unreadable) — the path is published whatever the entry type — and does not make such an entry count as SCANNED. - `owner_span` is always None on a path: a path string is not a CODEOWNERS owner line, even the path OF a CODEOWNERS file. - Findings are labelled `<path> (tracked path):` rather than `<path>:<lineno>:`, because "rename the file" and "edit the file" are different fixes. Merged under the same per-run MAX_FINDINGS_TOTAL cap. - No boundary change: `/` is not in the repo-reference lookbehind, so `docs/Comfy-Org/x/y.md` matches while `aComfy-Org/x` does not, and `\b` fires at `/` and `-`. - `_emit` now lists findings BEFORE the zero-coverage verdict, since the two can co-occur for the first time: a repo of nothing but binaries can leak in its own file listing while `scanned` stays 0. The exit code is unchanged (still 2 there); the finding is no longer swallowed. False-positive risk measured, not assumed: all 11,415 tracked paths of nine Comfy-Org public repos through the real machinery produce zero findings, and the existing 165-test corpus stays green unchanged. Overhead is ~4 us per path.
|
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 (6)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…rface, cap path findings, read surrogates as name (BE-9399) Review round 1 of the tracked-path scan: - The model-host prefix and markdown-link-label suppressors read URL / markdown syntax a path does not have, so inheriting them let a tree park a private name under an hf.co/ directory and stay green. Gated via _line_findings(url_suppressors=False) on the path surface; allowlists, ticket knobs, the npm-scope crossing and the homoglyph handling still reach both surfaces. A vendored mirror now over-flags at the root as well as nested (fail-closed, documented, corpus count still zero). - A lone surrogate (category Cs, from surrogateescape-decoded paths) is now a name continuation in _nonascii_tail, so Comfy-Org/ComfyUI\xff-x can no longer clear as the bare allowlisted prefix. - Path findings go through _path_findings with the same per-file cap, PARTIAL_FINDINGS accounting and warning as contents; a file counts once per partial kind whichever surface earned it. - Documented rather than changed: an allowlisted name with a file extension over-flags (stripping would fail open), lowercase ticket ids in a path are a miss, category-2 hosts need a following slash, and exit 2 may now carry findings. exclude_paths' contents-coverage cost is stated in both docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mattmillerai
added a commit
that referenced
this pull request
Aug 26, 2026
…9652) (#232) 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>
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.
ELI-5
The leak checker reads what is inside your files. It never read the file names. So a public repo could contain a directory literally called
docs/Comfy-Org/<a-private-repo>/, with a completely empty file in it, and the check went green — while GitHub happily showed that private repo's name to anyone browsing the repository. This PR makes the checker read the tracked path as well as the contents, using the exact same matcher so every allowlist and every false-positive suppressor applies to both.What changed
_file_findings's per-line body is factored out into a shared_line_findings(location, line, ticket_allowlist, owner_span), andrun_checksnow runs it over the tracked path itself — right after the exclusion check, beforecheck_file. Deliberately one matcher, not two:TICKET_ALLOWLIST/TICKET_ALLOWED_PREFIXES, the--ticket-allowextension, the model-host and markdown-link-label suppressors, the homoglyph handling and the.git/period strips all reach the new surface for free, and there is no second place to forget an allowlist entry.Details worth knowing at review time:
check_filedeclines (binary, non-UTF-8, submodule gitlink, LFS stub, FIFO, unreadable). The path is published in the tree whatever the entry type. It does not make such an entry count asSCANNED:— that number is files read as text.owner_spanis alwaysNoneon a path. A path string is not a CODEOWNERS owner line, even the path of a CODEOWNERS file. I passed the precomputed span rather than anis_codeownersflag because deriving the span also needs the line number (the BOM-at-offset-0 rule), and a line number means nothing on a path —owner_span=Nonereads as the fact it is, wherelineno=1would have been a lie in the signature. Same behaviour either way.exclude_paths:covers the new surface, because the existing_is_excludedcontinuesits above the path scan. An excluded path is not scanned as a path either, and still reports its skipped-file count so the hole stays named in the log.<path> (tracked path):rather than<path>:<lineno>:— "rename the file" and "edit the file" are different fixes. Merged into the same per-runMAX_FINDINGS_TOTALcap./is not inREPO_REF_RE's left lookbehind, sodocs/Comfy-Org/x/y.mdmatches whileaComfy-Org/xdoes not;\bfires at both/and-, sonotes/TEAM-1234/plan.mdandTEAM-1234-notes.mdboth match. Same token rules as content, verified empirically rather than assumed._emitnow lists findings before the zero-coverage verdict. Until this PR, findings impliedscanned >= 1; now a repo of nothing but binaries can leak in its own file listing whilescannedstays0, and the old ordering returned exit 2 having printed the finding nowhere — sending the operator after a configuration problem instead of the leak. Exit codes are byte-for-byte unchanged (still 2 there); only the output is added. Pinned bytest_a_path_finding_is_listed_even_when_nothing_was_scanned.Rollout is the ordinary one: callers pin
workflows_refby SHA, so the new surface reaches them through the normalbump-public-repo-hygiene-callers.ymlflow.False-positive risk — measured, not asserted
Every tracked path of nine Comfy-Org public repos, fed through the real
_line_findings:Enabling this reddens no caller CI. This repo's own working tree (176 tracked paths, and it is deliberately not self-enrolled) also produces 0 path findings. Overhead is ~4 µs per path (47 ms across 11,187 paths), against a content scan that reads up to 5 MiB per file.
And the portion this does not cover, swept with the same tool: the checker reads the work tree, so a path that once existed and was renamed or deleted stays in every clone and is still not examined — nor are branch and tag names. Across the same nine repos that is 3,337 history-only paths, which likewise produce 0 would-be findings today. Real gap, empirically empty, now written down as a known limitation rather than left implicit. (Caveat: measured over the refs present in the local clones, which may not carry every remote branch.)
Verification
python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'— 178 passed, 0 failed (1 skipped, pre-existing; 165 → 178, +13 new). The pre-existing 165 pass unchanged, which is the zero-new-false-positive claim.run:step of.github/workflows/test-public-repo-hygiene.ymlextracted and executed locally — all 13 smoke steps pass, exit 0.python3 .github/workflow-pins/check_workflow_pins.py— OK, 11 workflows, 0 exempt.python3 .github/agents-md-integrity/check_agents_md.py --root .— passed (1 pre-existing CODEOWNERS warning).bash .github/bump-callers/tests/test_paths_contract.sh— 70 passed, 0 failed.New tests (13): a private repo name in a path fails; an allowlisted repo name in a path passes; ticket-shaped directory component and filename prefix both fail; built-in and caller-extended acronyms in a path pass; the
(tracked path)vs:<lineno>labels distinguish the two surfaces when a name appears in both;aComfy-Org/xdoes not flag (left-boundary pin); an internal-marker host in a path flags; the npm-scope and model-host suppressors carry over; the nested-model-host over-flag is pinned;exclude_paths:suppresses and still counts; a path finding fires on a binary entry and on a dangling symlink whose body is never read; and the zero-coverage output case above.Residual
.github/lint/README.mdKNOWN LIMITATION 8 is not updated. That file does not exist onmain— it arrives with PR ci(lint): fail on org repo literals not on a committed allowlist #224, which is still open. Its limitation-8 sentence "neither checker scans the tracked path itself" becomes stale the moment this merges, and ci(lint): fail on org repo literals not on a committed allowlist #224's branch will need that sentence corrected on rebase. I could not edit it here without touching another open PR's branch, so it is called out rather than done. Whichever of the two lands second owns the fix.models/hf.co/Comfy-Org/<model>/config.jsonis reported while the same mirror at the tree root is suppressed:MODEL_HOST_PREFIX_RE's left anchor rejects a preceding/, and every segment of a path has one. This is the over-flag direction (a leak guard should be wrong that way), oneexclude_paths:entry clears it, and the shape occurs zero times across the 11,415 paths measured — so it is pinned by a test and documented as a known limitation rather than narrowed. Narrowing it would mean loosening a suppressor, which fails open; that is a deliberate non-goal here and would want its own change with its own evidence.git ls-files, but it is the honest remaining hole and is now stated in both READMEs.bump-public-repo-hygiene-callers.ymlmoves their pinned SHA. Nothing here does that, and no caller CI changes until it happens.Provenance
_line_findingstakes a precomputedowner_spanrather than anis_codeownersflag plus a line number (same behaviour, no fakelineno=1on a path — rationale above);_emitoutput ordering changed so a finding is never swallowed by the zero-coverage verdict (exit codes unchanged);.github/lint/README.mdleft alone because PR ci(lint): fail on org repo literals not on a committed allowlist #224 has not merged. Review round 1 (cursor-review panel, 9 threads): the two URL-syntax suppressors (model-host prefix, markdown link label) are now gated OFF the path surface viaurl_suppressors=False— a path has no authority, so they failed open there; a vendored mirror therefore over-flags at the root as well as nested. A surrogateescape code point (Cs) now continues a name in_nonascii_tail. Path findings are capped andPARTIAL-counted through_path_findings. Documented rather than changed:Comfy-Org/ComfyUI.mdover-flags, lowercase ticket ids in paths miss, category-2 hosts need a following/, exit 2 may carry findings, andexclude_paths:costs contents coverage.