Skip to content

ci: exclude x.com and twitter.com from the docs link check - #612

Open
pablodeymo wants to merge 1 commit into
mainfrom
ci/lychee-exclude-x
Open

ci: exclude x.com and twitter.com from the docs link check#612
pablodeymo wants to merge 1 commit into
mainfrom
ci/lychee-exclude-x

Conversation

@pablodeymo

Copy link
Copy Markdown
Collaborator

🗒️ Description / Motivation

x.com answers the lychee link checker with 403 Forbidden intermittently, so the Link Check job failed on #611 for https://x.com/class_lambda in docs/introduction.md, a file that PR never touched, while the same job was green on main days earlier. The link is valid in a browser; this is the same anti-bot behavior the job already tolerates for eprint.iacr.org.

What Changed

.github/workflows/pr-main_mdbook.yml: two lychee --exclude patterns for x.com and twitter.com (which redirects to x.com), anchored to the URL origin so hosts merely ending in "x.com" are still checked. The comment next to the existing exclusion explains why.

Correctness / Behavior Guarantees

Only the project's two social links stop being checked; every other external link in docs/ is still validated. Internal links are unaffected (they are checked by mdbook-linkcheck2 in the build step).

Tests Added / Run

Patterns checked against every URL in docs/: they match https://x.com/class_lambda and https://twitter.com/ethlambda_lean only; https://linux.com/x and GitHub links are not matched. Workflow YAML validated. The Link Check job runs on this PR itself since the workflow file is in its path filter.

Related Issues / PRs

✅ Verification Checklist

No Rust changes; make fmt / make lint / make test not applicable.

x.com answers the lychee bot with 403 Forbidden intermittently, so the Link
Check job failed on PRs that never touched docs/introduction.md (#611 hit it on
https://x.com/class_lambda while the same job was green on main days earlier).
The links are valid in a browser; this is the same anti-bot behavior the job
already tolerates for eprint.iacr.org, so it gets the same exclusion, plus
twitter.com, which redirects there.

Both patterns are anchored to the URL origin so a host that merely ends in
"x.com" is still checked.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR diff focusing on the areas you specified. This is a small CI workflow change, but I'll examine it thoroughly.

File: .github/workflows/pr-main_mdbook.yml

Security & Correctness

Lines 88-89: The regex anchors ^https?://(www\.)?x\.com and ^https?://(www\.)?twitter\.com have a subtle issue — they don't anchor to the end of the URL ($).

  • Problem: This excludes x.com/anything and twitter.com/anything, which is likely intended. However, it also excludes x.com.evil.com and twitter.com.evil.com due to missing trailing anchor. Wait — actually re-reading: the ^ anchor at start prevents evil.com/x.com but the lack of $ means x.com.evil.com would match ^https?://(www\.)?x\.com only if it starts with that pattern... no, x.com.evil.com starts with x.com so ^https?://(www\.)?x\.com would match https://x.com.evil.com as https://x.com followed by .evil.com.

Actually, let me re-analyze: https://x.com.evil.com — the regex ^https?://(www\.)?x\.com matches https://x.com then .evil.com remains unmatched but the exclusion likely does prefix matching, so this would incorrectly exclude x.com.evil.com.

Suggestion: Add trailing anchor or path boundary:

--exclude '^https?://(www\.)?x\.com(/|$)'
--exclude '^https?://(www\.)?twitter\.com(/|$)'

Documentation Accuracy

Lines 81-85: The comment states "Both are anchored to the URL origin" but the regexes only have leading anchors, not trailing ones. This is slightly misleading — "anchored to the URL origin" suggests more precision than implemented.

Minor Issue

Line 87: The existing eprint\.iacr\.org exclusion (line 87) has the same pattern — no trailing anchor. For consistency, if you fix the new ones, consider whether this should also be tightened, though iacr.org is less spoofable than x.com.

Summary

Priority Issue Location
Medium Missing trailing regex anchor allows subdomain spoofing Lines 88-89
Low Comment overstates anchoring precision Lines 81-85

The change is functionally acceptable for a documentation link-checker (attack surface is low), but the regex precision doesn't match the comment's claims. Given this is CI infrastructure for a consensus client, defense-in-depth matters even in peripheral tooling.


Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. Low: .github/workflows/pr-main_mdbook.yml:90 and .github/workflows/pr-main_mdbook.yml:91 don’t terminate the host match. ^https?://(www\.)?x\.com also matches https://x.com.evil/... (and likewise for twitter.com), which contradicts the comment and could hide a bad or malicious docs link if one is introduced later. Tighten these to something host-bounded like ^https?://(www\.)?x\.com(:[0-9]+)?(/|$) and ^https?://(www\.)?twitter\.com(:[0-9]+)?(/|$).

Other than that, this looks fine. The PR only changes CI link-check exclusions, so I don’t see consensus-layer, memory-safety, or runtime performance risk in the actual client code path.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR 612 — Exclude x.com and twitter.com from docs link check

This is a small, well-scoped CI-only change (.github/workflows/pr-main_mdbook.yml, +6/-1). No Rust code is touched, so most of the consensus-layer review checklist doesn't apply. One correctness issue with the new patterns:

Correctness bug: regex isn't anchored to the host boundary

+          --exclude '^https?://(www\.)?x\.com'
+          --exclude '^https?://(www\.)?twitter\.com'

Both patterns anchor the start (^https?://(www\.)?) but never terminate the host match with / or end-of-string. Since lychee's --exclude does a substring/prefix match rather than requiring a full match, these patterns will also match hosts that merely start with x.com / twitter.com before more characters, e.g.:

  • https://x.comcast.net/...
  • https://x.company.example/...
  • https://twitter.com.phishing-site.io/...

That's the exact class of false-positive the PR description says it wants to avoid ("hosts merely ending in x.com are still checked") — the same problem exists symmetrically for hosts that start with the string plus extra characters. Any doc link to such a domain would silently stop being checked instead of being validated, undermining the PR's own "Correctness / Behavior Guarantees" claim that "every other external link in docs/ is still validated."

Suggested fix — require a path separator or end-of-string right after the domain:

--exclude '^https?://(www\.)?x\.com(/|$)'
--exclude '^https?://(www\.)?twitter\.com(/|$)'

Everything else looks fine

  • Anchoring with ^https?://(www\.)? correctly avoids matching arbitrary paths/hosts that merely end in x.com (e.g. https://linux.com/x), which is the case the PR explicitly tested against.
  • Placing the new excludes next to the existing eprint.iacr.org one with an updated comment explaining the anti-bot rationale is good practice and matches the existing pattern in the file.
  • Twitter's redirect-to-x.com is correctly treated as requiring its own separate exclude rather than assuming lychee follows the redirect before matching.
  • Scope is minimal and reversible — only two known social links stop being checked; no impact on internal link checking (mdbook-linkcheck2) or any other workflow behavior.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant