[PR #208] [MERGED] fix(public-repo-hygiene): anchor category-2 host patterns on DNS-label boundaries and allow ports (BE-8729) #216

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8729-hygiene-host-boundaries

📝 Commits (6)

33b9f9e fix(public-repo-hygiene): anchor category-2 host patterns on DNS-label boundaries and allow ports (BE-8729)
85ee416 fix(public-repo-hygiene): close the port-backtracking, empty-port and case-folding holes in the category-2 anchors (BE-8729)
ce8aa39 fix(public-repo-hygiene): repair the right anchor's prose-colon regression, reject userinfo, and state the UTS-46 trade honestly (BE-8729)
d6f7966 fix(public-repo-hygiene): stop the host-only anchor consuming the port, closing the userinfo family and the prose-colon regressions (BE-8729)
17827bb fix(public-repo-hygiene): bound the userinfo run by class and length, closing a quadratic scan and two prose misses (BE-8729)
5b0ea79 fix(public-repo-hygiene): drop the non-port suffix alternative and repair the unreadable-file crash (BE-8729)

📊 Changes

4 files changed (+781 additions, -11 deletions)

View changed files

📝 .github/public-repo-hygiene/README.md (+66 -2)
📝 .github/public-repo-hygiene/check_public_repo_hygiene.py (+192 -9)
📝 .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (+491 -0)
📝 .github/workflows/test-public-repo-hygiene.yml (+32 -0)

📄 Description

ELI-5

The leak guard has a rule like "flag any link to posthog.com/project/…". To decide where the host name starts and stops it used \b, the regex word boundary — and \b knows nothing about domain names. A hyphen is not a word character, so evil-posthog.com/project/1 looked to it like a perfectly good posthog.com, and half the rules had no left-hand check at all, so fooslack.com/archives/x and foonotion.so/page were "internal links" too. Meanwhile a real internal link only had to add :443 — notion.so:443/team/page — and every rule that expected a / right after the host went silent. This teaches the rules what a domain-name boundary actually is: a dot in front is a genuine subdomain edge and still matches, a letter/digit/hyphen in front means a different domain entirely, and an explicit port is no longer an escape hatch.

What changed

INTERNAL_MARKER_RES (check_public_repo_hygiene.py) — two new shared fragments and every host pattern rebuilt on them:
- _HOST_L = (?<![A-Za-z0-9-]) — a left anchor on the real DNS-label alphabet. It deliberately permits a preceding dot, because a dot is the one character that can precede a host and still leave it that host. (?<![A-Za-z0-9.-]) is the tempting-but-wrong tightening: it would reject every subdomain-prefixed positive the existing fixtures pin (www.notion.so, comfy.notion.site, comfy.slack.com/archives, us.posthog.com/project). There is an in-file comment saying so.
- _PORT = (?::\d*)? — an explicit port between host and path. The digits are *, not +: port = *DIGIT in RFC 3986, so the empty port https://notion.so:/page is a valid URL for the same host and was the same one-token bypass :443 was.
- _HOST_FLAGS = re.IGNORECASE | re.ASCII on the eight host patterns (incident-\d+ is not a host and keeps plain re.IGNORECASE). The ignore-case flag alone folds Unicode, and review was right that this is two boundaries, not one:
  - U+0131 / U+0130 are different hosts. Python matches both against i, so lınear.app/x and notİon.so/page read as the real hosts — but UTS-46 leaves U+0131 alone and maps U+0130 to i + a combining dot, so neither resolves anywhere near the real service. Flagging them was a false positive; re.ASCII removes it, restoring the invariant REPO_REF_RE already scopes its own flag to keep.
  - U+017F / U+212A are the same host. UTS-46 maps them to s and k, so ſlack.com/archives/C123 really does resolve to slack.com. re.ASCII turns those into misses. That is a deliberate scope call — an obfuscated spelling, in the same family as the punycode/percent-encoded/defanged spellings already listed under "Known limitations" — and the code comment, the test name and the README all now say so rather than claiming a domain nobody else can reach.
- app.slack.com is the only host-only pattern (nothing requires a / after it), so it loses \b for a right anchor of its own — and, after four review rounds, it does not consume _PORT at all and carries only three alternatives:
```
(?!\.?[A-Za-z0-9-]|@|[.:][A-Za-z0-9._~%:-]{0,64}@)
```
  An optional, greedy port sitting in front of a negative lookahead backtracks — the engine hands digits back one at a time until the lookahead is satisfied — so every alternative had to be correct for every possible port split. That cost two rounds: round 2 shipped a hole (app.slack.com:443.evil.com matched anyway), its fix shipped a regression (app.slack.com:general went silent), and that fix relocated it (app.slack.com:2FA). A search() only needs a boolean, so nothing has to be consumed; one lookahead reads the whole tail, the port never enters the match, and each alternative is independent:
  - \.?[A-Za-z0-9-] — a following label (app.slack.com.evil.com), what \b used to accept. A dot is allowed when what follows is not a label, so app.slack.com./x still matches.
  - @ — the userinfo delimiter; in https://app.slack.com@evil.com/ the real host is evil.com, the canonical phishing shape.
  - [.:][A-Za-z0-9._~%:-]{0,64}@ — the same with userinfo in between (:@, :secret@, .@, and :443.evil.com@), all of which an adjacent-only @ left flagged. The class is the characters a real credential uses, not "anything but a URL delimiter": the first cut, [^\s/?#@]*, crossed commas, quotes and braces, so any later @ on the line silenced the host. It is length-bounded because an unbounded run made a long single line quadratic to scan. Both bounds cost an over-flag, and both are now named in "Known limitations" and pinned: a colon-chained run still reaches a later @ (: has to stay in the class, since :user:pass@ is real userinfo), and userinfo past 64 characters is out of the run's reach. A leading admin@app.slack.com still matches. Every other pattern's required / directly after host+port already blocks the suffix shape — notion.so.evil.com/page and linear.app.evil.com/x are non-matches before and after this change, and that is asserted.
  - There is no fourth alternative for a non-numeric "port". Rounds 2–4 carried :\d+\.[A-Za-z0-9-] to reject app.slack.com:443.evil.com as "a port the host continues past". Round 5 removed it: port = *DIGIT, so WHATWG's port state fails on the ., the URL does not parse at all, urlsplit(…).hostname is app.slack.com, and curl and Go's net/url both error on the port. No mainstream parser resolves that line to evil.com, so the only readable host on it is the internal one — suppressing it was a miss, not a false-positive fix, and it also caused the :2.5 / :1.0.1 prose misses that shared the shape. The genuine phishing form, app.slack.com:443.evil.com@evil.com, is rejected by the userinfo alternative and needed nothing here (asserted both ways).
Tests — new cases in InternalMarkerCategoryTest: 5 third-party lookalike hosts + 2 same-namespace neighbours, 3 ported URLs, 3 empty-port URLs, 8 userinfo shapes (5 rejected, 3 prose forms that must survive), 2 different-host homoglyphs, 2 UTS-46-mapped spellings pinned as out-of-scope misses, the IDN-neighbour limitation (both boundaries), the host-only trailing-root-label match, 7 host-only right-boundary shapes, the four non-numeric-port lines that must now be flagged plus their two userinfo forms that must not, the colon-chained userinfo miss, both sides of the 64-character bound, and the label-adjacency misses. test_each_marker_is_flagged is untouched and green, which is what pins the subdomain-prefixed positives. CoverageReportingTest gains test_an_unopenable_file_warns_instead_of_crashing (see below).
Smoke steps — test-public-repo-hygiene.yml gains lookalike hosts (a homoglyph domain and two userinfo URLs) in the clean-repo fixture, and ported-URL + empty-port cases in the per-category dirty-repo fixture. Both were verified to fail against the pre-change checker (lookalikes: old exit 1 / new exit 0; ported URL: old exit 0 / new exit 1), so they are real pins rather than decoration. Round 5 moved the bare app.slack.com:443.evil.com/x line out of the clean fixture — it is a real reference and is now correctly flagged — and replaced it with its userinfo form, which is still rejected.
Docs — the checker README's category-2 row and its "Known limitations" section.
One unrelated crash, fixed in passing (round 5). _read_text's open()-failure path returned a five-tuple where the docstring, every sibling branch and its only caller use four, so any unreadable tracked file (EACCES on a mode-000 blob, EMFILE, an I/O error) raised ValueError: too many values to unpack (expected 4) and exited 1 — which public-repo-hygiene.yml renders as internal-only references found, with no finding listed, on a repo that is in fact clean. It predates this branch (landed with the centralization). The existing coverage test reaches the lstat failure path only, which is why nothing caught it; the new test uses a mode-000 regular file so lstat succeeds and only the read fails, and it reproduces the ValueError when the fifth value is put back.

Falsification, not just unit tests

Tests I author only assert my own premise, and the user-facing effect of this diff is that six line shapes stop being findings. So the premise — "these are unrelated third-party domains, not the internal services" — was checked read-only against DNS rather than assumed:

host	A record	SOA
`slack.com`	`54.71.95.193`	`ns-1493.awsdns-58.org.`
`fooslack.com`	—	— (no zone)
`posthog.com`	`216.150.1.1`	`julissa.ns.cloudflare.com.`
`evil-posthog.com`	—	— (no zone)
`linear.app`	`172.64.147.211`	`tim.ns.cloudflare.com.`
`my-linear.app`	—	— (no zone)
`notion.so`	`208.103.161.1`	`dana.ns.cloudflare.com.`
`foonotion.so`	—	— (no zone)
`app.slack.com`	`35.81.85.251`	—
`app.slack.com.evil.com`	—	— (no zone)

Not one of the six lookalikes resolves at all, and none is a delegation of the real service's zone. They are separate registrable names, which is exactly what the DNS label alphabet says they are: labels are dot-separated and made of letters, digits and hyphens, so a letter/digit/hyphen in front of posthog.com is part of a different label, while a dot in front is a delegation edge. The direction the diff moves detection is therefore: strictly fewer lookalike-third-party-host lines flagged (false positives removed), strictly more ported-URL lines flagged (false negatives closed).

Corpus sweep — both halves

Old and new pattern sets run over the pre-change main tree, all 175 tracked files (this repo is deliberately not self-enrolled, so this is a stress corpus rather than a clean one):

Category-2 findings: 22 → 22. Zero lines change verdict. Neither the false-positive class nor the port class occurs in this tree, so the evidence for the change is the FP/FN matrix and the DNS check above, not the corpus. Saying "0 changed" out loud matters more than a number that flattered the diff would.
23 lines merely mention one of these hosts; 5 of them are correctly non-findings under the new set (bare host, or a public marketing path like posthog.com/docs).
The half this PR does NOT fix, measured with the same sweep: a host written with a trailing root label — https://notion.so./page, the same host to any resolver — still walks past every category-2 pattern, because they require / (or a port then /) immediately after the host. Same for obfuscated spellings: percent-encoding (%2E), punycode (xn--), defanging (notion[.]so). All of these were misses before this change too and are unchanged by it; corpus occurrences on this tree: 0. They are now written down in the checker README's "Known limitations" rather than left implicit. Deliberately not fixed here: the ticket specifies an exact pattern set validated as a unit upstream, and quietly widening it would leave the shipped set unvalidated.

Detection parity with the SDK scripts

The 18/18 findings-parity proof from BE-8654 no longer describes this branch, by design and for the second time (PR #207 shipped a detection change the same way and is the precedent). Direction of divergence, stated plainly: the checker now flags strictly fewer lookalike-third-party-host lines and strictly more ported-URL lines than the two per-repo scripts the centralization retired. Neither SDK copy is touched here — editing them is what would invalidate the parity proof that licenses their deletion.

Judgment calls / criteria not met as written

Change 2 of the ticket (add UTF-16/UTF-32 to TICKET_ALLOWLIST) is not in this diff, because it is already satisfied on main by a better mechanism. The ticket was written from a spike snapshot that predates the BE-8654 review, which added TICKET_ALLOWED_PREFIXES (CVE, CWE, PEP, RFC, ISO, UTF). UTF-16 and UTF-32 therefore already clear by prefix, and test_public_identifier_namespaces_clear_by_prefix already pins both by name. Adding two exact entries next to UTF-8 would be dead list content. The ticket's follow-on request — update the comment at test_an_excluded_path_may_carry_a_work_tree_encoding_attribute and restore its fixture to canonical UTF-16 casing — is likewise already done on main; the comment there already cites TICKET_ALLOWED_PREFIXES and the fixture already reads UTF-16.
The ticket's requested UTF-99 negative control was deliberately not added. It contradicts the shipped design: UTF-99 splits to the prefix UTF, which is allowlisted, so it clears and is not a finding. Asserting otherwise would pin a behaviour this checker does not have and never agreed to have. The analogous control that is correct for a prefix allowlist already exists — test_an_allowlisted_prefix_does_not_clear_a_longer_acronym (CVEX-1234, ISOP-99).
The ticket's stated baseline of Ran 74 tests is stale — main is at 97 (OK (skipped=1)), for the same snapshot reason. This branch is 100.
One known over-match, kept on purpose. The ASCII left anchor lets an IDN neighbour clear it, so https://énotion.so/page — a different registrable domain — reports as notion.so. The blunt fix, a second lookbehind rejecting any non-ASCII character, would also silence a real link written straight after a curly quote, an em dash or CJK prose, and a missed leak costs more than an extra finding in a leak guard. Pinned by test_an_idn_neighbour_still_reports_as_the_real_host and written down in the README so changing it is deliberate.
Rounds 2–4 were wrong about app.slack.com:443.evil.com, in both directions, and round 5 settles it. Round 2 let it match and this body defended that as a safe over-flag; rounds 3–4 rejected it with :\d+\.[A-Za-z0-9-] and this body defended that, calling the resulting :2.5 miss "a miss for a miss, in the direction that flags less". Both readings were wrong, and the second was wrong twice: a non-numeric port makes the URL unparseable, so the line's only readable host is the internal one and suppressing it is a plain miss — and removing an alternative from a negative lookahead can only ever flag MORE, never less, which is the same rule stated twenty lines below it in the same file. The alternative is gone; both directions are now asserted (the bare line flagged, the userinfo form rejected).
Five findings are documented and pinned rather than re-tuned. The colon-chained userinfo run, the 64-character bound's over-flag past the boundary, ASCII _ as a host character, the right-hand half of the IDN-neighbour limitation, and label characters adjacent to a three-label host (my-app.slack.com, app.slack.com-hosted). Each is a real over-flag or miss in the safe direction whose only fix trades it for the opposite error; every regex tweak in this PR has cost a review round, so they are written into README "Known limitations" and pinned by tests instead of re-tuned blind. The last one also scopes the file's own rationale: "a letter, digit or hyphen in front means a different registrable name" is exact for the two-label patterns and over-claims for the three-label ones, where it is the same customer-sub-domain shape the README already documents for Datadog.
Two detection gaps are documented rather than closed, on the same reasoning as the trailing-root-label case. A backslash separator (WHATWG normalizes \ to / for special schemes, so https://notion.so\page is host notion.so) and a Datadog custom organization sub-domain (comfyapp.datadoghq.com/dashboard/1 — Datadog hands customers their own <name>.datadoghq.com, so unlike *.google.com that namespace is not vendor-only) are both real misses. Neither is the accidental-paste shape this guard is scoped to, and both would widen what the shipped pattern set matches beyond what was validated. They are in README "Known limitations" so a deliberate widening has something to start from. The Datadog case also cost a fixture: myapp.datadoghq.com was removed from SAME_NAMESPACE_HOSTS rather than relabelled, because pinning it as a correct non-finding would assert that a custom-sub-domain dashboard should be missed.

Unexercised artifacts

The ticket's provenance points at a Linear spike issue and a PR review-thread comment; I hold no access to either. The ticket text as handed to me is the whole of what I had, and everything in it was independently re-verified above rather than taken on trust — including the two claims that turned out to be stale.
The two per-repo SDK scripts live in other repositories and are intentionally untouched, so no parity re-run was performed against them. Ending that parity is the point.

Verification

python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py' — 114 passed, 0 failed, 1 skipped (110 before round 5). Five review rounds; every round is recorded in the in-file comments rather than only in this body. main baseline is 97 with the same single skip (pre-existing: APFS rejects the non-UTF-8 filename that test builds; it is Linux-only by design).
All 13 run: smoke steps from test-public-repo-hygiene.yml executed locally under bash -e — all pass, including the two new cases.
Both new smoke cases re-run against the pre-change checker to prove they pin something: lookalike fixture old exit 1 → new exit 0; ported-URL fixture old exit 0 → new exit 1.
27-case FP/FN matrix run directly against the compiled round-5 host-only pattern (17 positives incl. every prose-colon shape rounds 2–3 lost and the four non-numeric-port lines round 5 recovered; 10 negatives incl. the full userinfo family, both userinfo forms of the non-numeric port, and the two label-adjacency misses) — 0 mismatches. Earlier rounds ran a 37-case and then a 52-case matrix over the whole pattern set.
Round-4-vs-round-5 corpus sweep over the same 175 tracked files — 19 → 19 category-2 findings, and specifically zero lines newly silent, which is the direction that would matter.
The new _read_text regression test re-run against a copy with the fifth tuple value restored — reproduces ValueError: too many values to unpack (expected 4, got 5), so it pins the fix rather than merely passing beside it.
Old-vs-new corpus sweep over 175 tracked files of the pre-change main tree — 22 → 22 category-2 findings, 0 lines changed.
Read-only DNS resolution of all 10 hosts in the table above.
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, no default, every ref checkout guarded.
python3 .github/agents-md-integrity/check_agents_md.py --root . fails on AGENTS.md being 305 lines against a 200-line ceiling. Pre-existing and unrelated — identical failure on unmodified main, this diff does not touch AGENTS.md, and this repo carries no self-enrolled agents-md caller.

Refs BE-8729 — deliberately not Closes. The body above names an unfixed detection gap (the trailing-root-label host, plus percent-encoded / punycode / defanged spellings) on INTERNAL_MARKER_RES, the exact artifact this ticket names, and two of the ticket's own acceptance items were resolved as already-done-or-contradicted rather than implemented as written. The ticket should stay open to carry that residual and to confirm those two dispositions.

Provenance

Authored by: agent-work loop
Verified: public-repo-hygiene unittest suite: 114 passed, 0 failed, 1 skipped (pre-existing macOS filesystem skip; main baseline 97, branch was 110 before round 5); all 14 run: steps of test-public-repo-hygiene.yml replayed locally under bash -e, all pass, including the moved clean-repo lookalike fixture and the ported/empty-port dirty fixtures; a 27-case FP/FN matrix run directly against the compiled round-5 host-only pattern, 0 mismatches (17 positives incl. every prose-colon shape rounds 2–3 lost, admin@app.slack.com, the four comma/quote/brace/NBSP prose forms, and the four non-numeric-port lines round 5 recovered; 10 negatives incl. app.slack.com.evil.com, the five userinfo shapes, both userinfo forms of :443.evil.com, and the two label-adjacency misses); a round-4-vs-round-5 corpus sweep over 175 tracked files of main (19 → 19 category-2 findings, zero lines newly silent in either direction); and the new _read_text regression test re-run against a copy with the five-tuple restored, which reproduces ValueError: too many values to unpack (expected 4, got 5). Earlier revisions additionally ran 37-case and 52-case FP/FN matrices, an old-vs-new corpus sweep (22 → 22, 0 changed), read-only DNS resolution of 10 real-vs-lookalike hosts, and the workflow-pins lint (11 workflows).
Deviations: the ticket's Change 2 (add UTF-16/UTF-32 to TICKET_ALLOWLIST) was not implemented because it is already satisfied on main by TICKET_ALLOWED_PREFIXES, and its requested UTF-99 negative control was not added because it contradicts that shipped design; both are argued above. Change 1 and all of the requested test coverage shipped as specified. Across five review rounds, twenty-six findings: eighteen fixed in code and eight deliberately documented as limitations rather than fixed — the IDN neighbour on both boundaries, ASCII _ as a host character, the backslash separator, the Datadog custom organization sub-domain, label characters adjacent to a three-label host, the colon-chained userinfo run, and the 64-character userinfo bound's over-flag. Each trades a rare error for its opposite, or widens the shipped pattern set past what was validated; all are in README "Known limitations" with the reasoning, and every one is pinned by a test so changing it is deliberate. Round 5 also reversed a round-3/4 code decision (the :\d+\.[A-Za-z0-9-] alternative) and fixed one pre-existing crash in _read_text that is outside this PR's stated scope — a one-token typo whose blast radius is a false "references found" failure on any repo with an unreadable tracked file.

_{🔄 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/208 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/22/2026 **Status:** ✅ Merged **Merged:** 8/22/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8729-hygiene-host-boundaries` --- ### 📝 Commits (6) - [`33b9f9e`](https://github.com/Comfy-Org/github-workflows/commit/33b9f9e213f36b87ddf2f55e943c80c7dea6b047) fix(public-repo-hygiene): anchor category-2 host patterns on DNS-label boundaries and allow ports (BE-8729) - [`85ee416`](https://github.com/Comfy-Org/github-workflows/commit/85ee4161304ecffb5cfe8849b7cbffbd50dfd806) fix(public-repo-hygiene): close the port-backtracking, empty-port and case-folding holes in the category-2 anchors (BE-8729) - [`ce8aa39`](https://github.com/Comfy-Org/github-workflows/commit/ce8aa39b96a714e30c90101410467dccb2dd48ba) fix(public-repo-hygiene): repair the right anchor's prose-colon regression, reject userinfo, and state the UTS-46 trade honestly (BE-8729) - [`d6f7966`](https://github.com/Comfy-Org/github-workflows/commit/d6f7966ddd7eee27d339f5b66a7f0b3ea1a46b59) fix(public-repo-hygiene): stop the host-only anchor consuming the port, closing the userinfo family and the prose-colon regressions (BE-8729) - [`17827bb`](https://github.com/Comfy-Org/github-workflows/commit/17827bb0e6c324f91dd9792060ddde410c0113c9) fix(public-repo-hygiene): bound the userinfo run by class and length, closing a quadratic scan and two prose misses (BE-8729) - [`5b0ea79`](https://github.com/Comfy-Org/github-workflows/commit/5b0ea795fa53a4a38f016bec4efc129e6189ffef) fix(public-repo-hygiene): drop the non-port suffix alternative and repair the unreadable-file crash (BE-8729) ### 📊 Changes **4 files changed** (+781 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `.github/public-repo-hygiene/README.md` (+66 -2) 📝 `.github/public-repo-hygiene/check_public_repo_hygiene.py` (+192 -9) 📝 `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (+491 -0) 📝 `.github/workflows/test-public-repo-hygiene.yml` (+32 -0) </details> ### 📄 Description ## ELI-5 The leak guard has a rule like "flag any link to `posthog.com/project/…`". To decide where the host name starts and stops it used `\b`, the regex word boundary — and `\b` knows nothing about domain names. A hyphen is not a word character, so `evil-posthog.com/project/1` looked to it like a perfectly good `posthog.com`, and half the rules had no left-hand check at all, so `fooslack.com/archives/x` and `foonotion.so/page` were "internal links" too. Meanwhile a real internal link only had to add `:443` — `notion.so:443/team/page` — and every rule that expected a `/` right after the host went silent. This teaches the rules what a domain-name boundary actually is: a dot in front is a genuine subdomain edge and still matches, a letter/digit/hyphen in front means a different domain entirely, and an explicit port is no longer an escape hatch. ## What changed - **`INTERNAL_MARKER_RES` (`check_public_repo_hygiene.py`)** — two new shared fragments and every host pattern rebuilt on them: - `_HOST_L = (?<![A-Za-z0-9-])` — a left anchor on the real DNS-label alphabet. It deliberately **permits a preceding dot**, because a dot is the one character that can precede a host and still leave it *that* host. `(?<![A-Za-z0-9.-])` is the tempting-but-wrong tightening: it would reject every subdomain-prefixed positive the existing fixtures pin (`www.notion.so`, `comfy.notion.site`, `comfy.slack.com/archives`, `us.posthog.com/project`). There is an in-file comment saying so. - `_PORT = (?::\d*)?` — an explicit port between host and path. The digits are `*`, not `+`: `port = *DIGIT` in RFC 3986, so the empty port `https://notion.so:/page` is a valid URL for the same host and was the same one-token bypass `:443` was. - `_HOST_FLAGS = re.IGNORECASE | re.ASCII` on the eight **host** patterns (`incident-\d+` is not a host and keeps plain `re.IGNORECASE`). The ignore-case flag alone folds **Unicode**, and review was right that this is two boundaries, not one: - U+0131 / U+0130 are **different hosts**. Python matches both against `i`, so `lınear.app/x` and `notİon.so/page` read as the real hosts — but UTS-46 leaves U+0131 alone and maps U+0130 to `i` + a combining dot, so neither resolves anywhere near the real service. Flagging them was a false positive; `re.ASCII` removes it, restoring the invariant `REPO_REF_RE` already scopes its own flag to keep. - U+017F / U+212A are the **same host**. UTS-46 *maps* them to `s` and `k`, so `ſlack.com/archives/C123` really does resolve to `slack.com`. `re.ASCII` turns those into misses. That is a deliberate scope call — an obfuscated spelling, in the same family as the punycode/percent-encoded/defanged spellings already listed under "Known limitations" — and the code comment, the test name and the README all now say so rather than claiming a domain nobody else can reach. - `app.slack.com` is the only **host-only** pattern (nothing requires a `/` after it), so it loses `\b` for a right anchor of its own — and, after four review rounds, it **does not consume `_PORT` at all** and carries only three alternatives: ``` (?!\.?[A-Za-z0-9-]|@|[.:][A-Za-z0-9._~%:-]{0,64}@) ``` An optional, greedy port sitting in front of a negative lookahead **backtracks** — the engine hands digits back one at a time until the lookahead is satisfied — so every alternative had to be correct for every possible port split. That cost two rounds: round 2 shipped a hole (`app.slack.com:443.evil.com` matched anyway), its fix shipped a regression (`app.slack.com:general` went silent), and *that* fix relocated it (`app.slack.com:2FA`). A `search()` only needs a boolean, so nothing has to be consumed; one lookahead reads the whole tail, the port never enters the match, and each alternative is independent: - `\.?[A-Za-z0-9-]` — a following label (`app.slack.com.evil.com`), what `\b` used to accept. A dot is allowed when what follows is not a label, so `app.slack.com./x` still matches. - `@` — the userinfo delimiter; in `https://app.slack.com@evil.com/` the real host is `evil.com`, the canonical phishing shape. - `[.:][A-Za-z0-9._~%:-]{0,64}@` — the same with userinfo in between (`:@`, `:secret@`, `.@`, and `:443.evil.com@`), all of which an adjacent-only `@` left flagged. The class is the characters a real credential uses, not "anything but a URL delimiter": the first cut, `[^\s/?#@]*`, crossed commas, quotes and braces, so any later `@` on the line silenced the host. It is length-bounded because an unbounded run made a long single line quadratic to scan. Both bounds cost an over-flag, and both are now named in "Known limitations" and pinned: a **colon-chained** run still reaches a later `@` (`:` has to stay in the class, since `:user:pass@` is real userinfo), and userinfo past 64 characters is out of the run's reach. A leading `admin@app.slack.com` still matches. Every other pattern's required `/` directly after host+port already blocks the suffix shape — `notion.so.evil.com/page` and `linear.app.evil.com/x` are non-matches before and after this change, and that is asserted. - **There is no fourth alternative for a non-numeric "port".** Rounds 2–4 carried `:\d+\.[A-Za-z0-9-]` to reject `app.slack.com:443.evil.com` as "a port the host continues past". Round 5 removed it: `port = *DIGIT`, so WHATWG's port state fails on the `.`, the URL does not parse at all, `urlsplit(…).hostname` is `app.slack.com`, and curl and Go's `net/url` both error on the port. No mainstream parser resolves that line to `evil.com`, so the only readable host on it is the internal one — suppressing it was a **miss**, not a false-positive fix, and it also caused the `:2.5` / `:1.0.1` prose misses that shared the shape. The genuine phishing form, `app.slack.com:443.evil.com@evil.com`, is rejected by the userinfo alternative and needed nothing here (asserted both ways). - **Tests** — new cases in `InternalMarkerCategoryTest`: 5 third-party lookalike hosts + 2 same-namespace neighbours, 3 ported URLs, 3 empty-port URLs, 8 userinfo shapes (5 rejected, 3 prose forms that must survive), 2 different-host homoglyphs, 2 UTS-46-mapped spellings pinned as out-of-scope misses, the IDN-neighbour limitation (both boundaries), the host-only trailing-root-label match, 7 host-only right-boundary shapes, the four non-numeric-port lines that must now be **flagged** plus their two userinfo forms that must not, the colon-chained userinfo miss, both sides of the 64-character bound, and the label-adjacency misses. `test_each_marker_is_flagged` is **untouched** and green, which is what pins the subdomain-prefixed positives. `CoverageReportingTest` gains `test_an_unopenable_file_warns_instead_of_crashing` (see below). - **Smoke steps** — `test-public-repo-hygiene.yml` gains lookalike hosts (a homoglyph domain and two userinfo URLs) in the clean-repo fixture, and ported-URL + empty-port cases in the per-category dirty-repo fixture. Both were verified to **fail against the pre-change checker** (lookalikes: old exit 1 / new exit 0; ported URL: old exit 0 / new exit 1), so they are real pins rather than decoration. Round 5 moved the bare `app.slack.com:443.evil.com/x` line **out** of the clean fixture — it is a real reference and is now correctly flagged — and replaced it with its userinfo form, which is still rejected. - **Docs** — the checker README's category-2 row and its "Known limitations" section. - **One unrelated crash, fixed in passing (round 5).** `_read_text`'s `open()`-failure path returned a **five**-tuple where the docstring, every sibling branch and its only caller use four, so any unreadable tracked file (EACCES on a mode-000 blob, EMFILE, an I/O error) raised `ValueError: too many values to unpack (expected 4)` and exited 1 — which `public-repo-hygiene.yml` renders as *internal-only references found*, with no finding listed, on a repo that is in fact clean. It predates this branch (landed with the centralization). The existing coverage test reaches the `lstat` failure path only, which is why nothing caught it; the new test uses a mode-000 **regular** file so `lstat` succeeds and only the read fails, and it reproduces the `ValueError` when the fifth value is put back. ## Falsification, not just unit tests Tests I author only assert my own premise, and the user-facing effect of this diff is that six line shapes **stop** being findings. So the premise — "these are unrelated third-party domains, not the internal services" — was checked read-only against DNS rather than assumed: | host | A record | SOA | |---|---|---| | `slack.com` | `54.71.95.193` | `ns-1493.awsdns-58.org.` | | `fooslack.com` | — | — (no zone) | | `posthog.com` | `216.150.1.1` | `julissa.ns.cloudflare.com.` | | `evil-posthog.com` | — | — (no zone) | | `linear.app` | `172.64.147.211` | `tim.ns.cloudflare.com.` | | `my-linear.app` | — | — (no zone) | | `notion.so` | `208.103.161.1` | `dana.ns.cloudflare.com.` | | `foonotion.so` | — | — (no zone) | | `app.slack.com` | `35.81.85.251` | — | | `app.slack.com.evil.com` | — | — (no zone) | Not one of the six lookalikes resolves at all, and none is a delegation of the real service's zone. They are separate registrable names, which is exactly what the DNS label alphabet says they are: labels are dot-separated and made of letters, digits and hyphens, so a letter/digit/hyphen in front of `posthog.com` is *part of a different label*, while a dot in front is a delegation edge. The direction the diff moves detection is therefore: strictly **fewer** lookalike-third-party-host lines flagged (false positives removed), strictly **more** ported-URL lines flagged (false negatives closed). ## Corpus sweep — both halves Old and new pattern sets run over the pre-change `main` tree, all **175** tracked files (this repo is deliberately not self-enrolled, so this is a stress corpus rather than a clean one): - **Category-2 findings: 22 → 22.** Zero lines change verdict. Neither the false-positive class nor the port class occurs in this tree, so the evidence for the change is the FP/FN matrix and the DNS check above, **not** the corpus. Saying "0 changed" out loud matters more than a number that flattered the diff would. - 23 lines merely mention one of these hosts; 5 of them are correctly non-findings under the new set (bare host, or a public marketing path like `posthog.com/docs`). - **The half this PR does NOT fix**, measured with the same sweep: a host written with a trailing **root label** — `https://notion.so./page`, the same host to any resolver — still walks past every category-2 pattern, because they require `/` (or a port then `/`) immediately after the host. Same for obfuscated spellings: percent-encoding (`%2E`), punycode (`xn--`), defanging (`notion[.]so`). All of these were misses before this change too and are unchanged by it; corpus occurrences on this tree: **0**. They are now written down in the checker README's "Known limitations" rather than left implicit. Deliberately not fixed here: the ticket specifies an exact pattern set validated as a unit upstream, and quietly widening it would leave the shipped set unvalidated. ## Detection parity with the SDK scripts The 18/18 findings-parity proof from BE-8654 **no longer describes this branch**, by design and for the second time (PR #207 shipped a detection change the same way and is the precedent). Direction of divergence, stated plainly: the checker now flags **strictly fewer** lookalike-third-party-host lines and **strictly more** ported-URL lines than the two per-repo scripts the centralization retired. Neither SDK copy is touched here — editing them is what would invalidate the parity proof that licenses their deletion. ## Judgment calls / criteria not met as written - **Change 2 of the ticket (add `UTF-16`/`UTF-32` to `TICKET_ALLOWLIST`) is not in this diff, because it is already satisfied on `main` by a better mechanism.** The ticket was written from a spike snapshot that predates the BE-8654 review, which added `TICKET_ALLOWED_PREFIXES` (`CVE`, `CWE`, `PEP`, `RFC`, `ISO`, **`UTF`**). `UTF-16` and `UTF-32` therefore already clear by prefix, and `test_public_identifier_namespaces_clear_by_prefix` already pins both by name. Adding two exact entries next to `UTF-8` would be dead list content. The ticket's follow-on request — update the comment at `test_an_excluded_path_may_carry_a_work_tree_encoding_attribute` and restore its fixture to canonical `UTF-16` casing — is likewise **already done on `main`**; the comment there already cites `TICKET_ALLOWED_PREFIXES` and the fixture already reads `UTF-16`. - **The ticket's requested `UTF-99` negative control was deliberately not added.** It contradicts the shipped design: `UTF-99` splits to the prefix `UTF`, which *is* allowlisted, so it clears and is **not** a finding. Asserting otherwise would pin a behaviour this checker does not have and never agreed to have. The analogous control that *is* correct for a prefix allowlist already exists — `test_an_allowlisted_prefix_does_not_clear_a_longer_acronym` (`CVEX-1234`, `ISOP-99`). - **The ticket's stated baseline of `Ran 74 tests` is stale** — `main` is at 97 (`OK (skipped=1)`), for the same snapshot reason. This branch is 100. - **One known over-match, kept on purpose.** The ASCII left anchor lets an **IDN neighbour** clear it, so `https://énotion.so/page` — a different registrable domain — reports as `notion.so`. The blunt fix, a second lookbehind rejecting any non-ASCII character, would also silence a *real* link written straight after a curly quote, an em dash or CJK prose, and a missed leak costs more than an extra finding in a leak guard. Pinned by `test_an_idn_neighbour_still_reports_as_the_real_host` and written down in the README so changing it is deliberate. - **Rounds 2–4 were wrong about `app.slack.com:443.evil.com`, in both directions, and round 5 settles it.** Round 2 let it match and this body defended that as a safe over-flag; rounds 3–4 rejected it with `:\d+\.[A-Za-z0-9-]` and this body defended *that*, calling the resulting `:2.5` miss "a miss for a miss, in the direction that flags less". Both readings were wrong, and the second was wrong twice: a non-numeric port makes the URL unparseable, so the line's only readable host is the internal one and suppressing it is a plain miss — and *removing* an alternative from a **negative** lookahead can only ever flag MORE, never less, which is the same rule stated twenty lines below it in the same file. The alternative is gone; both directions are now asserted (the bare line flagged, the userinfo form rejected). - **Five findings are documented and pinned rather than re-tuned.** The colon-chained userinfo run, the 64-character bound's over-flag past the boundary, ASCII `_` as a host character, the right-hand half of the IDN-neighbour limitation, and label characters adjacent to a **three**-label host (`my-app.slack.com`, `app.slack.com-hosted`). Each is a real over-flag or miss in the safe direction whose only fix trades it for the opposite error; every regex tweak in this PR has cost a review round, so they are written into README "Known limitations" and pinned by tests instead of re-tuned blind. The last one also scopes the file's own rationale: "a letter, digit or hyphen in front means a different **registrable** name" is exact for the two-label patterns and over-claims for the three-label ones, where it is the same customer-sub-domain shape the README already documents for Datadog. - **Two detection gaps are documented rather than closed, on the same reasoning as the trailing-root-label case.** A backslash separator (WHATWG normalizes `\` to `/` for special schemes, so `https://notion.so\page` is host `notion.so`) and a Datadog **custom organization sub-domain** (`comfyapp.datadoghq.com/dashboard/1` — Datadog hands customers their own `<name>.datadoghq.com`, so unlike `*.google.com` that namespace is not vendor-only) are both real misses. Neither is the accidental-paste shape this guard is scoped to, and both would widen what the shipped pattern set matches beyond what was validated. They are in README "Known limitations" so a deliberate widening has something to start from. The Datadog case also cost a fixture: `myapp.datadoghq.com` was **removed** from `SAME_NAMESPACE_HOSTS` rather than relabelled, because pinning it as a correct non-finding would assert that a custom-sub-domain dashboard *should* be missed. ## Unexercised artifacts - The ticket's provenance points at a Linear spike issue and a PR review-thread comment; I hold no access to either. The ticket text as handed to me is the whole of what I had, and everything in it was independently re-verified above rather than taken on trust — including the two claims that turned out to be stale. - The two per-repo SDK scripts live in other repositories and are intentionally untouched, so no parity re-run was performed against them. Ending that parity is the point. ## Verification - `python3 -m unittest discover -s .github/public-repo-hygiene/tests -p 'test_*.py'` — **114 passed, 0 failed, 1 skipped** (110 before round 5). Five review rounds; every round is recorded in the in-file comments rather than only in this body. `main` baseline is 97 with the same single skip (pre-existing: APFS rejects the non-UTF-8 filename that test builds; it is Linux-only by design). - All **13** `run:` smoke steps from `test-public-repo-hygiene.yml` executed locally under `bash -e` — all pass, including the two new cases. - Both new smoke cases re-run against the pre-change checker to prove they pin something: lookalike fixture old exit 1 → new exit 0; ported-URL fixture old exit 0 → new exit 1. - 27-case FP/FN matrix run directly against the compiled round-5 host-only pattern (17 positives incl. every prose-colon shape rounds 2–3 lost and the four non-numeric-port lines round 5 recovered; 10 negatives incl. the full userinfo family, both userinfo forms of the non-numeric port, and the two label-adjacency misses) — 0 mismatches. Earlier rounds ran a 37-case and then a 52-case matrix over the whole pattern set. - Round-4-vs-round-5 corpus sweep over the same 175 tracked files — **19 → 19** category-2 findings, and specifically **zero lines newly silent**, which is the direction that would matter. - The new `_read_text` regression test re-run against a copy with the fifth tuple value restored — reproduces `ValueError: too many values to unpack (expected 4, got 5)`, so it pins the fix rather than merely passing beside it. - Old-vs-new corpus sweep over 175 tracked files of the pre-change `main` tree — 22 → 22 category-2 findings, 0 lines changed. - Read-only DNS resolution of all 10 hosts in the table above. - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, no default, every ref checkout guarded. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` fails on `AGENTS.md` being 305 lines against a 200-line ceiling. **Pre-existing and unrelated** — identical failure on unmodified `main`, this diff does not touch `AGENTS.md`, and this repo carries no self-enrolled agents-md caller. Refs BE-8729 — deliberately not `Closes`. The body above names an unfixed detection gap (the trailing-root-label host, plus percent-encoded / punycode / defanged spellings) on `INTERNAL_MARKER_RES`, the exact artifact this ticket names, and two of the ticket's own acceptance items were resolved as already-done-or-contradicted rather than implemented as written. The ticket should stay open to carry that residual and to confirm those two dispositions. ## Provenance - **Authored by:** agent-work loop - **Verified:** public-repo-hygiene unittest suite: 114 passed, 0 failed, 1 skipped (pre-existing macOS filesystem skip; main baseline 97, branch was 110 before round 5); all 14 `run:` steps of `test-public-repo-hygiene.yml` replayed locally under `bash -e`, all pass, including the moved clean-repo lookalike fixture and the ported/empty-port dirty fixtures; a 27-case FP/FN matrix run directly against the compiled round-5 host-only pattern, 0 mismatches (17 positives incl. every prose-colon shape rounds 2–3 lost, `admin@app.slack.com`, the four comma/quote/brace/NBSP prose forms, and the four non-numeric-port lines round 5 recovered; 10 negatives incl. `app.slack.com.evil.com`, the five userinfo shapes, both userinfo forms of `:443.evil.com`, and the two label-adjacency misses); a round-4-vs-round-5 corpus sweep over 175 tracked files of main (19 → 19 category-2 findings, zero lines newly silent in either direction); and the new `_read_text` regression test re-run against a copy with the five-tuple restored, which reproduces `ValueError: too many values to unpack (expected 4, got 5)`. Earlier revisions additionally ran 37-case and 52-case FP/FN matrices, an old-vs-new corpus sweep (22 → 22, 0 changed), read-only DNS resolution of 10 real-vs-lookalike hosts, and the workflow-pins lint (11 workflows). - **Deviations:** the ticket's Change 2 (add `UTF-16`/`UTF-32` to `TICKET_ALLOWLIST`) was not implemented because it is already satisfied on main by `TICKET_ALLOWED_PREFIXES`, and its requested `UTF-99` negative control was not added because it contradicts that shipped design; both are argued above. Change 1 and all of the requested test coverage shipped as specified. Across five review rounds, twenty-six findings: eighteen fixed in code and eight deliberately documented as limitations rather than fixed — the IDN neighbour on both boundaries, ASCII `_` as a host character, the backslash separator, the Datadog custom organization sub-domain, label characters adjacent to a three-label host, the colon-chained userinfo run, and the 64-character userinfo bound's over-flag. Each trades a rare error for its opposite, or widens the shipped pattern set past what was validated; all are in README "Known limitations" with the reasoning, and every one is pinned by a test so changing it is deliberate. Round 5 also **reversed** a round-3/4 code decision (the `:\d+\.[A-Za-z0-9-]` alternative) and fixed one pre-existing crash in `_read_text` that is outside this PR's stated scope — a one-token typo whose blast radius is a false "references found" failure on any repo with an unreadable tracked file. --- <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:28 +00:00