fix: match Substack's own UI links on hostname, not substring - #23
Merged
Merged
Conversation
The filter that skips Substack chrome -- subscribe, comments, share -- tested `"substack.com" in link` against the whole URL. That also matches somebody else's address which merely mentions substack.com, such as a redirector like https://example.com/?next=https://x.substack.com/share, and the consequence is that the link is silently dropped from the scan. Quietly not checking a link is the opposite of what this tool is for, and nothing in the report would say it had happened. Now parsed: the host must be substack.com or a subdomain of it, and the segment must appear in the path. Deliberately `in` the path rather than `startswith`, because a comment link is ".../p/<slug>/comments" -- the segment sits inside the path, and switching to a prefix test would stop skipping them. Resolves the CodeQL py/incomplete-url-substring-sanitization alert on checker.py. Note the rule's "high" rating is generic to the query rather than to this use: nothing here gates auth or redirects, so the real cost was unchecked links rather than a vulnerability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4cfc0d43f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review catch on this PR, and it is the same bug this PR set out to fix, reintroduced one layer down. netloc can carry userinfo. For https://SUBSTACK.COM:443@evil.example/subscribe the request goes to evil.example, but taking everything before the first colon reads the host as substack.com -- so the link was silently skipped, which is exactly the failure mode being removed here. Verified against the previous commit: it returned True for that URL. parsed.hostname strips userinfo and port and lowercases, so it cannot be fooled that way. Three userinfo forms are now pinned as tests.
Applying the lesson from the previous commit to the rest of the package turned up the same mistake in three more places, all reading .netloc where .hostname was meant. Because netloc keeps the port, an explicit one silently defeated three documented features: --skip-domains wikipedia.org did not skip https://wikipedia.org:443/... --broken-domains dead.example did not flag https://dead.example.com:443/... triage --retired-hosts did not match https://old.example.com:443/... These fail toward not-matching, so unlike the previous commit there is no silent-skip of a third-party link -- the cost is a flag the user set quietly not applying. Verified by running each case before and after rather than reasoning about it. triage._plausible also read netloc, where userinfo could supply the dot its hostname check looks for. Tests pin both directions for all three: a port must not stop a listed host matching, and userinfo must not let an unlisted host borrow a listed name.
This repo's subject matter is URLs and it has now made the same host-matching mistake twice in one change -- once as a substring search over the raw link, once as netloc sliced by hand. There are four host comparisons in the package and a fifth is plausible, so it is worth writing the convention down rather than leaving it in commit messages. Includes the trap specifically: CodeQL flags the substring form, and slicing netloc silences that alert without fixing the problem, so a contributor following the alert can land where this PR started. Also notes that the two failure directions are not equally bad -- too loose silently drops a third-party link from the scan, too strict only stops a user's flag applying -- and lists the input forms to cover in tests.
ruff format also formats python fenced blocks inside markdown, which the one-line `if ...: ...` in the new section tripped.
Merged
jcddc83
added a commit
that referenced
this pull request
Sep 17, 2026
#23's fixes went in with careful commit messages and no CHANGELOG entry, which is the one place users actually look. Recording them, and cutting a patch release so a deployment can pin a version rather than a commit. Both fixes are user-visible: a third-party link could be silently dropped from the scan, and an explicit port quietly stopped --skip-domains, --broken-domains and --retired-hosts from applying.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The filter that skips Substack's own chrome — subscribe, comments, share — tested
"substack.com" in linkagainst the whole URL:So a third-party address that merely mentions substack.com matches too — a redirector like
https://example.com/?next=https://x.substack.com/share. The consequence is that the link is silently dropped from the scan. Quietly not checking a link is the opposite of this tool's job, and nothing in the report would tell you it happened.The fix
Parse it: the host must be
substack.comor a subdomain, and the segment must appear in the path.Deliberately
inthe path rather thanstartswith— a comment link is.../p/<slug>/comments, so the segment sits inside the path. A prefix test would have stopped skipping comment links, which is a behaviour change disguised as a tidy-up.On the "high" rating
This resolves CodeQL alert #5,
py/incomplete-url-substring-sanitization. The high severity is generic to the rule, not to this usage — nothing here gates authentication or redirects, so the real cost was unchecked links, not a vulnerability. Worth fixing on correctness grounds regardless.Verification
134 tests pass (16 new),
ruff check/ruff format --checkclean. The new tests pin both directions: real Substack UI links still skipped (including uppercase hosts and explicit ports), and lookalikes likemysubstack.comorhttps://example.com/how-to-share-on-substack.comno longer are.🤖 Generated with Claude Code