Skip to content

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

Merged
ntohidi merged 2 commits into
unclecode:developfrom
Nalhin:fix-robots-parsing
Sep 22, 2026
Merged

ntohidi merged 2 commits 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

@Nalhin

Nalhin commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Great, thanks @SohamKukreti!

Can we get this landed?

@Nalhin

Nalhin commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@SohamKukreti I've addressed the nits

@ntohidi
ntohidi merged commit 64f124c into unclecode:develop Sep 22, 2026
@ntohidi

ntohidi commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Merged — thanks @Nalhin! The /*?/*?* rewrite is a neat fix: an empty query is the only thing urlunparse drops, so that one line covers the whole class of rules.

Digging into this turned up a second bug right next to it — the wildcard monkey patch breaks Allow: rules on Python 3.14, where the stdlib parser is already RFC 9309 compliant. Fixed separately in #2278.

ntohidi added a commit that referenced this pull request Sep 22, 2026
* fix(robots): don't patch robotparser on Python 3.14+

The wildcard monkey patch in utils.py overrides RuleLine.applies_to
unconditionally. Python 3.14 rewrote urllib.robotparser with native
wildcard, '$' and RFC 9309 longest-match support, where applies_to
returns the match *length* used to rank competing rules. The patch
returns a bool, so every wildcard rule collapses to the lowest
priority and Allow: overrides stop working:

    User-agent: *
    Disallow: /
    Allow: /public/*.html

denied /public/a.html on 3.14. Gate the patch to Python < 3.14, where
robotparser has no wildcard support and still needs it.

Follow-up to #2229, which fixed the 'Disallow: /*?' half of #2225.

Refs #2225

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(robots): don't rewrite bare-'?' rules on Python 3.14+ either

Review follow-up on the previous commit, which gated the RuleLine
monkey patch but left _preserve_bare_query running on every Python.

On 3.14 that rewrite is not just unnecessary, it is wrong. The stdlib
ranks rules by match length, and '/*?*' matches to end of string, so
it outranks a narrower competing Allow:

    User-agent: *
    Allow: /*?q=
    Disallow: /*?

/search?q=1 is allowed by the stdlib and denied after the rewrite, so
Crawl4AI skipped pages robots.txt permits. Gate the call with the same
sys.version_info < (3, 14) as the patch, and say so in the docstring,
which claimed the two forms were always equivalent.

Also move the RuleLine import inside the branch that uses it and drop
a duplicate 'import re'.

Refs #2225

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Nalhin
Nalhin deleted the fix-robots-parsing branch September 22, 2026 17:15
@Nalhin

Nalhin commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@ntohidi Thanks for getting this landed!

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.

3 participants