[PR #325] [MERGED] fix(lint): NUL-delimit the fallback org-repo-literal scan so a newline in a path cannot fabricate a record #328

New issue

Closed

opened 2026-10-26 22:38:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/325
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 9/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15443-nul-delimited-fallback-scan

📝 Commits (2)

4d9eec5 fix(lint): NUL-delimit the fallback scan so a newline in a path cannot fabricate a record
142c64b fix(lint): C-quote the fallback label exactly as git does, and fail closed on an unopenable re-scan

📊 Changes

3 files changed (+313 additions, -79 deletions)

View changed files

📝 .github/lint/README.md (+12 -19)
📝 .github/lint/check-org-repo-literals.sh (+129 -37)
📝 .github/workflows/test-org-repo-literals.yml (+172 -23)

📄 Description

ELI-5

This repo has a small guard script that scans tracked files for leaked private
Comfy-Org/<repo> names. It has two ways to scan: a fast git grep path (used
inside a git checkout) and a plain grep -r fallback (used when the target
isn't a git work tree).

The fallback had a hole. It fed one big grep -rIoiEn blob into a loop that
splits each line into file:line:match. If a tracked file's own name contained
a newline, that one match arrived as two lines — and the leading half looked
enough like a real file:line:match record to sail past the two "does this
record look sane?" checks, so the real hit got pinned to a filename that doesn't
exist (and could even be silently cleared against the allowlist). The git grep
path never had this problem because git C-quotes such a path.

This PR makes the fallback do what git does: list the matching files
NUL-delimited, then scan each file one at a time on stdin under a C-quoted
--label. Now a newline / tab / \ / " in a path is reported as one
finding with a C-quoted location, identical on both scan paths.

What changed

check-org-repo-literals.sh (the fallback hits= line). Replaced the
single grep -rIoiEn … . with: enumerate matching files with
grep -rIliE --null, piped straight into a while read -r -d '' loop
(never stored in a variable — bash $(…) drops NUL bytes, which would run the
loop zero times and fail open), and scan each file on stdin with
grep -oiEnH --label="$label". $label is C-quoted (\→\\, "→\",
LF→\n, TAB→\t, wrapped in ") exactly the way git grep quotes a path, so
the per-hit loop body is unchanged and both paths emit identical records.
- grep -l exit 1 (no match) flows through the existing scan_status logic
  as before; a per-file exit above 1 (e.g. an unreadable file) is
  propagated so the existing "scan failed (exit N)" refusal still fires; a
  per-file exit 1 on the re-scan is tolerated.
- Used --null, not -Z (-Z is --decompress on BSD grep).
Per-hit parse refusals reframed. The non-numeric-line-field and
org-prefix-shape cases are kept as cheap parse invariants, but their
comments and error messages no longer describe a newline-in-a-path split
(unreachable now on either path). Dropped the two "BEST-EFFORT" sentences.
Docs. Deleted KNOWN LIMITATIONS item 12 and the matching README bullet;
updated the else-branch comment and the git-path core.quotePath comment to
say both paths now C-quote the same four bytes / print bytes ≥ 0x80 raw;
rewrote the README paragraph that called the refusals best-effort.
Smoke tests (test-org-repo-literals.yml). The two fallback fixtures that
expected exit 2 now expect exit 1 with exactly one C-quoted
unapproved: line (e.g. "a:bc\nd.md":1:, where \n is two literal
characters). Added: the three spike fixtures from the source investigation under one root (exit 1,
three findings, every ::error file= quoted, no file=a, / file=y.md); the
same three paths with an allowlisted body (exit 0 — pins that no fragment
survives to be cleared); a name with a tab, ", \ and a leading - (pins
the C-quoting and the --/< "$path" handling); and a NUL-safety fixture
(two matching files, one newline-named → two findings, guarding against a
future refactor that stores the file list in a variable).

Verification

shellcheck -x .github/lint/check-org-repo-literals.sh — clean.
All seven steps of test-org-repo-literals.yml extracted and run locally
(with GITHUB_WORKSPACE set) — green, including every new/flipped fixture.
Empirically confirmed: fallback a:bc\nd.md → exit 1, one finding at
"a:bc\nd.md":1:; the three spike fixtures → exit 1, three C-quoted findings,
no fabricated file=a/y.md; allowlisted body → exit 0; unreadable sibling →
exit 2 ("scan failed"); NUL-safety → two findings; git-path parity — the
same newline-named file in a git tree yields the identical "a:bc\nd.md":1:
location.
Repo-wide checks: check_workflow_pins.py OK, whole-tree
check-org-repo-literals.sh scans clean on the real tree, check_agents_md.py
passes (its two warnings are pre-existing and unrelated — AGENTS.md untouched).
Precondition met: the script exists on the default branch (the #224 change is
merged), so this is not the "script absent — stop" case.

Residual

The source spike's findings/prototype could not be read (no access to that
investigation from this environment). This PR implements the recommended plan
verbatim and verifies it empirically instead; the "verified prototype" it
references was not re-opened.
The performance numbers in the code comment are quoted from the source
ticket, not re-measured here (300 files × 5 hits 9.4s→9.6s; 1 file × 2000
hits 12.1s→12.2s). Functional correctness is verified; the exact timings are
not reproduced. The change adds one grep fork per matching file, so the
per-hit cost in KNOWN LIMITATIONS 11 is unchanged.
Cross-shell/cross-grep compatibility is exercised only on the CI surface
(ubuntu-latest, GNU grep 3.x). --null/--label and the bash pattern
substitutions are documented as portable to BSD grep / ugrep and bash 3.2, but
BSD grep and macOS /bin/bash were not run locally.

Provenance

Authored by: agent-work loop
Verified: shellcheck clean; all 7 workflow smoke-test steps green locally;
git-path parity and every ticket-named behavior reproduced by hand; repo-wide
lints pass.
Deviations: none from the ticket's plan. Added a couple of extra
belt-and-suspenders assertions in the flipped num_dir fixture
(file=a,/:oo must not appear).

_{🔄 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/325 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15443-nul-delimited-fallback-scan` --- ### 📝 Commits (2) - [`4d9eec5`](https://github.com/Comfy-Org/github-workflows/commit/4d9eec55e87b707e039d8fae25a6da1991826be7) fix(lint): NUL-delimit the fallback scan so a newline in a path cannot fabricate a record - [`142c64b`](https://github.com/Comfy-Org/github-workflows/commit/142c64bd758b130ac35f865fcdd570daa99d6446) fix(lint): C-quote the fallback label exactly as git does, and fail closed on an unopenable re-scan ### 📊 Changes **3 files changed** (+313 additions, -79 deletions) <details> <summary>View changed files</summary> 📝 `.github/lint/README.md` (+12 -19) 📝 `.github/lint/check-org-repo-literals.sh` (+129 -37) 📝 `.github/workflows/test-org-repo-literals.yml` (+172 -23) </details> ### 📄 Description ## ELI-5 This repo has a small guard script that scans tracked files for leaked private `Comfy-Org/<repo>` names. It has two ways to scan: a fast `git grep` path (used inside a git checkout) and a plain `grep -r` **fallback** (used when the target isn't a git work tree). The fallback had a hole. It fed one big `grep -rIoiEn` blob into a loop that splits each line into `file:line:match`. If a tracked *file's own name* contained a newline, that one match arrived as **two** lines — and the leading half looked enough like a real `file:line:match` record to sail past the two "does this record look sane?" checks, so the real hit got pinned to a filename that doesn't exist (and could even be silently cleared against the allowlist). The `git grep` path never had this problem because git C-quotes such a path. This PR makes the fallback do what git does: list the matching files **NUL-delimited**, then scan each file one at a time on stdin under a C-quoted `--label`. Now a newline / tab / `\` / `"` in a path is reported as **one** finding with a C-quoted location, identical on both scan paths. ## What changed - **`check-org-repo-literals.sh` (the fallback `hits=` line).** Replaced the single `grep -rIoiEn … .` with: enumerate matching files with `grep -rIliE --null`, piped **straight** into a `while read -r -d ''` loop (never stored in a variable — bash `$(…)` drops NUL bytes, which would run the loop zero times and fail *open*), and scan each file on stdin with `grep -oiEnH --label="$label"`. `$label` is C-quoted (`\`→`\\`, `"`→`\"`, LF→`\n`, TAB→`\t`, wrapped in `"`) exactly the way `git grep` quotes a path, so the per-hit loop body is unchanged and both paths emit identical records. - `grep -l` exit 1 (no match) flows through the existing `scan_status` logic as before; a per-file exit **above** 1 (e.g. an unreadable file) is propagated so the existing "scan failed (exit N)" refusal still fires; a per-file exit 1 on the re-scan is tolerated. - Used `--null`, not `-Z` (`-Z` is `--decompress` on BSD grep). - **Per-hit parse refusals reframed.** The non-numeric-line-field and org-prefix-shape `case`s are kept as cheap parse **invariants**, but their comments and error messages no longer describe a newline-in-a-path split (unreachable now on either path). Dropped the two "BEST-EFFORT" sentences. - **Docs.** Deleted KNOWN LIMITATIONS item 12 and the matching README bullet; updated the `else`-branch comment and the git-path `core.quotePath` comment to say both paths now C-quote the same four bytes / print bytes ≥ 0x80 raw; rewrote the README paragraph that called the refusals best-effort. - **Smoke tests (`test-org-repo-literals.yml`).** The two fallback fixtures that expected **exit 2** now expect **exit 1** with exactly one C-quoted `unapproved:` line (e.g. `"a:bc\nd.md":1:`, where `\n` is two literal characters). Added: the three spike fixtures from the source investigation under one root (exit 1, three findings, every `::error file=` quoted, no `file=a,` / `file=y.md`); the same three paths with an allowlisted body (exit 0 — pins that no fragment survives to be cleared); a name with a tab, `"`, `\` and a leading `-` (pins the C-quoting and the `--`/`< "$path"` handling); and a NUL-safety fixture (two matching files, one newline-named → **two** findings, guarding against a future refactor that stores the file list in a variable). ## Verification - `shellcheck -x .github/lint/check-org-repo-literals.sh` — clean. - All seven steps of `test-org-repo-literals.yml` extracted and run locally (with `GITHUB_WORKSPACE` set) — green, including every new/flipped fixture. - Empirically confirmed: fallback `a:bc\nd.md` → exit 1, one finding at `"a:bc\nd.md":1:`; the three spike fixtures → exit 1, three C-quoted findings, no fabricated `file=a`/`y.md`; allowlisted body → exit 0; unreadable sibling → exit 2 ("scan failed"); NUL-safety → two findings; **git-path parity** — the same newline-named file in a git tree yields the identical `"a:bc\nd.md":1:` location. - Repo-wide checks: `check_workflow_pins.py` OK, whole-tree `check-org-repo-literals.sh` scans clean on the real tree, `check_agents_md.py` passes (its two warnings are pre-existing and unrelated — AGENTS.md untouched). - Precondition met: the script exists on the default branch (the #224 change is merged), so this is not the "script absent — stop" case. ## Residual - **The source spike's findings/prototype could not be read** (no access to that investigation from this environment). This PR implements the recommended plan verbatim and verifies it empirically instead; the "verified prototype" it references was not re-opened. - **The performance numbers in the code comment are quoted from the source ticket, not re-measured here** (300 files × 5 hits 9.4s→9.6s; 1 file × 2000 hits 12.1s→12.2s). Functional correctness is verified; the exact timings are not reproduced. The change adds one grep fork per *matching file*, so the per-hit cost in KNOWN LIMITATIONS 11 is unchanged. - **Cross-shell/cross-grep compatibility is exercised only on the CI surface** (ubuntu-latest, GNU grep 3.x). `--null`/`--label` and the bash pattern substitutions are documented as portable to BSD grep / ugrep and bash 3.2, but BSD grep and macOS `/bin/bash` were not run locally. ## Provenance - **Authored by:** agent-work loop - **Verified:** shellcheck clean; all 7 workflow smoke-test steps green locally; git-path parity and every ticket-named behavior reproduced by hand; repo-wide lints pass. - **Deviations:** none from the ticket's plan. Added a couple of extra belt-and-suspenders assertions in the flipped `num_dir` fixture (`file=a,`/`:oo` must not appear). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:33 +00:00