[PR #217] [MERGED] fix(public-repo-hygiene): stop reading model-host URLs as GitHub repo refs #224

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/hygiene-model-host-namespace

📝 Commits (2)

db57600 fix(public-repo-hygiene): stop reading model-host URLs as GitHub repo refs
5259d36 fix(public-repo-hygiene): anchor the model-host skip to a URL authority

📊 Changes

2 files changed (+254 additions, -0 deletions)

View changed files

📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+117 -0)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+137 -0)

📄 Description

Summary

Comfy-Org/<name> under huggingface.co is a different namespace from the same spelling under github.com, but the repo matcher has no host anchor, so it read one out of a Hugging Face URL exactly as it read one out of a GitHub URL. Every model URL was then tested against a GitHub-repo allowlist it could never be in and reported as a leak.

In comfy-cli that was 19 of 29 findings, all public Comfy-Org model weights the CLI ships download URLs for (Qwen-Image_ComfyUI, stable-diffusion-v1-5-archive, ace_step_1.5_ComfyUI_files, and others). It recurs with every model that ships, and that kind of churn is what gets a required check switched off.

Details

REPO_REF_RE is untouched. It is a deliberately dumb tokenizer whose boundaries and case scoping are load-bearing, and Python needs a fixed-width lookbehind, which cannot cover hf.co, ports, and the api/models/ route in one pattern. The host test goes in the match loop instead:

MODEL_HOST_PREFIX_RE matches a model-host URL prefix, anchored on the left by _AUTHORITY_L.
Prefix end offsets are computed at most once per line, the same way owner_span is, so this stays linear rather than searching backwards from every match — and lazily, on the first Comfy-Org/ match, so a line without one allocates nothing.
The skip is checked before the homoglyph branch, whose remedy ("rewrite the name in ASCII") is wrong advice for a model repo whose name is not ours to rewrite.

Host-scoped, not name-scoped, on purpose: putting Hugging Face names in PUBLIC_COMFY_ORG_REPOS would also silently clear any future github.com/Comfy-Org repo that took the same name, which is a default-deny hole that outlives the reference that opened it.

Anchoring the suppression

The first revision reused _HOST_L, the DNS-label lookbehind INTERNAL_MARKER_RES uses. That was wrong, and the review caught it. _HOST_L is tuned for detection, where over-matching costs one extra finding; gating a suppression on it inverts the failure direction, so every gap it documents as an accepted trade became fail-open. It bars neither _, nor a non-ASCII character, nor /, so evil_huggingface.co/Comfy-Org/<private>, the leading-non-ASCII IDN neighbour, and internal.example/mirror/hf.co/Comfy-Org/<private> all cleared.

_AUTHORITY_L replaces it: the match must be preceded by a delimiter or start the line, and the scheme separator is consumed by an optional (?:(?:https?:)?//)? rather than tolerated. / is deliberately not a delimiter, which is what stops a path segment from passing as an authority.

The path alternation is likewise narrowed to routes that resolve. api/ now requires a resource segment (HF routes /api/{models,datasets,spaces}/<owner>, never /api/<owner>); path segments are (?-i:...)-scoped, because re.IGNORECASE belongs to the host, where DNS really is case-insensitive, and it was reaching the path and clearing /MODELS/; and collections/ is added, an owner-first route whose absence left the same false-positive class this change exists to remove. Both narrowings fail closed.

Residual ambiguity, accepted

A huggingface.co/Comfy-Org/<private-github-repo> URL now clears, so a name can be hidden behind a host prefix that does not resolve to it. The npm-scope crossing rests on the same reasoning — the guard is against an accidental paste, and a deliberately fabricated URL naming a private repo is not one — but it is a narrower exception and is documented as such: that crossing still requires the name to be in PUBLIC_COMFY_ORG_REPOS, while this branch skips before any allowlist test. What holds the line here is the host anchor, not the name.

Testing

153 tests pass. Every new test in this round fails against the previous pattern, so none is vacuous; the one exception is test_model_host_url_shapes_that_still_anchor, which passes in both states by design as the no-regression guard (bare host at line start, protocol-relative, markdown link, angle brackets, quoted, host case + port).

Verified against a 24-case skip/deny corpus, including all three fail-open vectors from review. Confirmed the edits introduce no new findings on a self-scan of this repo beyond the new test fixtures themselves, which follow the existing fixture pattern.

End to end against comfy-cli, findings go 29 to 10. The 10 that remain are genuine private-repo references plus two markdown link texts, which are bare names and correctly still denied.

ELI5

The checker's job is to notice when a public repo mentions something private. It spots those by looking for the text Comfy-Org/something.

The problem is that Comfy-Org is a name on two different websites. On GitHub it is where the code lives, and some of those repos are private. On Hugging Face it is where the published model files live, and those are all public and meant to be downloaded. The checker only knew about the GitHub meaning, so every download link for a public model looked to it like someone leaking a private repo name. comfy-cli ships a lot of those links, so most of its reported problems were not problems at all.

The fix teaches the checker to look at which website a mention is attached to. If the text sits right after a Hugging Face address, it is a model file and gets skipped. If it is anywhere else, including written on its own in a sentence, nothing changes and it is still checked as before.

The catch, which review found, is that "sits right after a Hugging Face address" has to mean a real address. The first version accepted anything that merely ended in the right letters, so a made-up host like evil_huggingface.co, or the real name buried in someone else's web address, would have been enough to silence the checker. Now the address has to be at the start of a link, not just somewhere inside one.

The reason for keying on the website rather than just adding those model names to the approved list: an approved name stays approved forever. If someone later made a private GitHub repo with the same name as one of these models, the old approach would have waved it straight through.

Provenance

Authored by: agent-work loop

Verified: 153 unit tests pass (python3 -m unittest discover -s .github/public-repo-hygiene/tests); the cursor-review, agents-md-integrity and groom suites also pass unchanged. Each new test was confirmed to fail against the pre-fix pattern. A 24-case skip/deny corpus was checked directly against the compiled regex, and a before/after self-scan of this repo confirmed no new findings outside the added fixtures.

Deviations: None. All five review findings were fixed rather than deferred; no follow-ups filed.

_{🔄 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/217 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `fix/hygiene-model-host-namespace` --- ### 📝 Commits (2) - [`db57600`](https://github.com/Comfy-Org/github-workflows/commit/db57600cfd104f54aca59330458c4e061fcdba0d) fix(public-repo-hygiene): stop reading model-host URLs as GitHub repo refs - [`5259d36`](https://github.com/Comfy-Org/github-workflows/commit/5259d3646febcfce1dde1ab57636c512464cefc6) fix(public-repo-hygiene): anchor the model-host skip to a URL authority ### 📊 Changes **2 files changed** (+254 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+117 -0) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+137 -0) </details> ### 📄 Description ## Summary `Comfy-Org/<name>` under `huggingface.co` is a different namespace from the same spelling under `github.com`, but the repo matcher has no host anchor, so it read one out of a Hugging Face URL exactly as it read one out of a GitHub URL. Every model URL was then tested against a GitHub-repo allowlist it could never be in and reported as a leak. In comfy-cli that was 19 of 29 findings, all public Comfy-Org model weights the CLI ships download URLs for (`Qwen-Image_ComfyUI`, `stable-diffusion-v1-5-archive`, `ace_step_1.5_ComfyUI_files`, and others). It recurs with every model that ships, and that kind of churn is what gets a required check switched off. ## Details `REPO_REF_RE` is untouched. It is a deliberately dumb tokenizer whose boundaries and case scoping are load-bearing, and Python needs a fixed-width lookbehind, which cannot cover `hf.co`, ports, and the `api/models/` route in one pattern. The host test goes in the match loop instead: - `MODEL_HOST_PREFIX_RE` matches a model-host URL prefix, anchored on the left by `_AUTHORITY_L`. - Prefix end offsets are computed at most once per line, the same way `owner_span` is, so this stays linear rather than searching backwards from every match — and lazily, on the first `Comfy-Org/` match, so a line without one allocates nothing. - The skip is checked before the homoglyph branch, whose remedy ("rewrite the name in ASCII") is wrong advice for a model repo whose name is not ours to rewrite. Host-scoped, not name-scoped, on purpose: putting Hugging Face names in `PUBLIC_COMFY_ORG_REPOS` would also silently clear any future `github.com/Comfy-Org` repo that took the same name, which is a default-deny hole that outlives the reference that opened it. ### Anchoring the suppression The first revision reused `_HOST_L`, the DNS-label lookbehind `INTERNAL_MARKER_RES` uses. That was wrong, and the review caught it. `_HOST_L` is tuned for **detection**, where over-matching costs one extra finding; gating a **suppression** on it inverts the failure direction, so every gap it documents as an accepted trade became fail-open. It bars neither `_`, nor a non-ASCII character, nor `/`, so `evil_huggingface.co/Comfy-Org/<private>`, the leading-non-ASCII IDN neighbour, and `internal.example/mirror/hf.co/Comfy-Org/<private>` all cleared. `_AUTHORITY_L` replaces it: the match must be preceded by a delimiter or start the line, and the scheme separator is **consumed** by an optional `(?:(?:https?:)?//)?` rather than tolerated. `/` is deliberately not a delimiter, which is what stops a path segment from passing as an authority. The path alternation is likewise narrowed to routes that resolve. `api/` now requires a resource segment (HF routes `/api/{models,datasets,spaces}/<owner>`, never `/api/<owner>`); path segments are `(?-i:...)`-scoped, because `re.IGNORECASE` belongs to the host, where DNS really is case-insensitive, and it was reaching the path and clearing `/MODELS/`; and `collections/` is added, an owner-first route whose absence left the same false-positive class this change exists to remove. Both narrowings fail **closed**. ### Residual ambiguity, accepted A `huggingface.co/Comfy-Org/<private-github-repo>` URL now clears, so a name can be hidden behind a host prefix that does not resolve to it. The npm-scope crossing rests on the same reasoning — the guard is against an accidental paste, and a deliberately fabricated URL naming a private repo is not one — but it is a **narrower** exception and is documented as such: that crossing still requires the name to be in `PUBLIC_COMFY_ORG_REPOS`, while this branch skips before any allowlist test. What holds the line here is the host anchor, not the name. ## Testing 153 tests pass. Every new test in this round fails against the previous pattern, so none is vacuous; the one exception is `test_model_host_url_shapes_that_still_anchor`, which passes in both states by design as the no-regression guard (bare host at line start, protocol-relative, markdown link, angle brackets, quoted, host case + port). Verified against a 24-case skip/deny corpus, including all three fail-open vectors from review. Confirmed the edits introduce no new findings on a self-scan of this repo beyond the new test fixtures themselves, which follow the existing fixture pattern. End to end against comfy-cli, findings go 29 to 10. The 10 that remain are genuine private-repo references plus two markdown link texts, which are bare names and correctly still denied. ## ELI5 The checker's job is to notice when a public repo mentions something private. It spots those by looking for the text `Comfy-Org/something`. The problem is that Comfy-Org is a name on two different websites. On GitHub it is where the code lives, and some of those repos are private. On Hugging Face it is where the published model files live, and those are all public and meant to be downloaded. The checker only knew about the GitHub meaning, so every download link for a public model looked to it like someone leaking a private repo name. comfy-cli ships a lot of those links, so most of its reported problems were not problems at all. The fix teaches the checker to look at which website a mention is attached to. If the text sits right after a Hugging Face address, it is a model file and gets skipped. If it is anywhere else, including written on its own in a sentence, nothing changes and it is still checked as before. The catch, which review found, is that "sits right after a Hugging Face address" has to mean a *real* address. The first version accepted anything that merely ended in the right letters, so a made-up host like `evil_huggingface.co`, or the real name buried in someone else's web address, would have been enough to silence the checker. Now the address has to be at the start of a link, not just somewhere inside one. The reason for keying on the website rather than just adding those model names to the approved list: an approved name stays approved forever. If someone later made a private GitHub repo with the same name as one of these models, the old approach would have waved it straight through. ## Provenance **Authored by:** agent-work loop **Verified:** 153 unit tests pass (`python3 -m unittest discover -s .github/public-repo-hygiene/tests`); the cursor-review, agents-md-integrity and groom suites also pass unchanged. Each new test was confirmed to fail against the pre-fix pattern. A 24-case skip/deny corpus was checked directly against the compiled regex, and a before/after self-scan of this repo confirmed no new findings outside the added fixtures. **Deviations:** None. All five review findings were fixed rather than deferred; no follow-ups filed. --- <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:31 +00:00