fix(public-repo-hygiene): anchor category-2 host patterns on DNS-label boundaries and allow ports (BE-8729)

[mattmillerai]

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.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 11 minutes

Limit details: You’ve used the included review currently available. Your 97 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 043eb136-3b42-4e54-ad81-ab6489ac11e1

📥 Commits

Reviewing files that changed from the base of the PR and between 50ed30b and 5b0ea79.

📒 Files selected for processing (4)

• .github/public-repo-hygiene/README.md
• .github/public-repo-hygiene/check_public_repo_hygiene.py
• .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py
• .github/workflows/test-public-repo-hygiene.yml

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 5 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	2
⚪ Nit	2

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 2 — ledger: 5 prior finding(s) across 1 round(s) (0 never answered).

Found 7 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	5
⚪ Nit	1

Panel: 7/8 reviewers contributed findings.

Reviewers that did not contribute: gemini-3.1-pro:adversarial (error)

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 3 — ledger: 12 prior finding(s) across 2 round(s) (0 never answered).

Found 5 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	1
⚪ Nit	3

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 4 — ledger: 17 prior finding(s) across 3 round(s) (0 never answered).

Found 5 finding(s).

Severity	Count
🟡 Medium	2
🟢 Low	2
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 5 — ledger: 17 prior finding(s) across 3 round(s) (0 never answered).

Found 9 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	4
⚪ Nit	2

Panel: 8/8 reviewers contributed findings.