Skip to content

fix: preventing disallow: /*? from blocking the whole website. - #2229

Open
Nalhin wants to merge 1 commit into
unclecode:developfrom
Nalhin:fix-robots-parsing
Open

Nalhin wants to merge 1 commit into
unclecode:developfrom
Nalhin:fix-robots-parsing

Conversation

@Nalhin

@Nalhin Nalhin commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #2225 - disallow: /*? in robots.txt blocks the whole website.

List of files changed and why

Adding logic in robots parser to append "*" after "?" so that urls ending with "?" get parsed correctly by the lib.

How Has This Been Tested?

Unit tests for this specific edge case.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@SohamKukreti

Copy link
Copy Markdown
Collaborator

Verified locally and this looks good to merge.

What I checked

  • Cherry-picked the commit onto current develop (clean).
  • Confirmed the root cause: on Python 3.9–3.12, RuleLine.__init__ does urlunparse(urlparse(path)), which drops a trailing empty ?. /*? is stored as /%2A, and the wildcard patch then turns that into ^/.*. Python 3.13 switched to normalize_path(), which keeps the ?, so the bug only affects 3.9–3.12.
  • tests/unit/test_robots_query_rules.py: 22 passed on 3.10 and 3.13 with the fix. Without the fix: 10 failed on 3.10 (the 7 helper tests plus the 3 plain-URL tests), 7 failed on 3.13 (helper tests only, as expected since 3.13 has no bug).
  • Live check against https://www.wired.com/robots.txt (User-agent: * / Disallow: /*?) on Python 3.10: before the fix, arun("https://www.wired.com/", check_robots_txt=True) returned 403 "Access denied by robots.txt"; after the fix it crawls, and /search?q=ai is still denied.
  • Rewrite is semantically a no-op: robots rules are prefix matches, so /x? and /x?* match the same set, and rules ending in $ are never touched. Ran the helper over six other live robots files; only lines ending in a bare ? change.
  • Full tests/unit suite: 102 passed on 3.10.

Two small nits (non-blocking)

  1. test_preserve_bare_query_keeps_document_structure asserts that the trailing newline is dropped. That pins an incidental side effect of "\n".join(...) rather than intended behavior; if the helper is ever changed to preserve the newline, the test would fail for no real reason. Suggest dropping the first assert and keeping only the .splitlines() comparison.

  2. Part 4b in tests/general/test_robot_parser.py binds a second server to a hardcoded port 8081. If anything else is listening there the test errors with "address already in use". The existing part 4 already does this with 8080, so this isn't new, but for the new server it'd be nicer to bind to port 0 and read the assigned port back from the site/runner.

Not caused by this PR, noting for a future follow-up

urllib.robotparser applies the first matching rule, not the longest. With wired.com's real file (Disallow: /*? followed by Allow: /*?page), /story/x/?page=2 is denied both before and after this change, on 3.10 and 3.13. RFC 9309 / Google use longest-match.

Thanks for the fix @Nalhin

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.

2 participants