Skip to content

replace raw null and surrogate code points during tokenization - #191

Merged
wRAR merged 2 commits into
scrapy:masterfrom
itecz26:preprocess-input-null-surrogate
Sep 16, 2026
Merged

wRAR merged 2 commits into
scrapy:masterfrom
itecz26:preprocess-input-null-surrogate

Conversation

@itecz26

@itecz26 itecz26 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

tokenize() consumes the selector string without the input-stream preprocessing step from css syntax level 3 §3.3, so a raw u+0000 or lone surrogate code point that appears directly in a string token (not as an escape) survives through attribute and :contains values into the final xpath. #164 and #189 only folded the escaped forms inside _replace_unicode (§4.3.7); the raw input path was never preprocessed.

this folds the raw u+0000 and surrogate forms to u+fffd at the entry of tokenize() with a length-preserving substitution so token positions stay correct, mirroring what the escape decoder already does. §3.3 only covers those code points, so this is a spec-conformance fix rather than a general lxml-compat one; other raw control characters are out of scope and still pass through.

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (4f6a271) to head (94b2bf7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #191   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files           3        3           
  Lines         966      968    +2     
  Branches      155      155           
=======================================
+ Hits          964      966    +2     
  Misses          1        1           
  Partials        1        1           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AdrianAtZyte

Copy link
Copy Markdown
Contributor

Nice catch, and I think the tokenizer entry is the right place for it.

A couple of things:

  • I would trim the comments. The one above the regex and the one inside tokenize() say the same thing, and both explain the bug that prompted the change rather than what the code does; a single line pointing at §3.3 is enough, and the rest reads better in the commit message. Same for the Before this, lxml rejected… part of the test comment.

  • The lxml framing only half holds, since a raw control character other than U+0000 still gets through:

    >>> from lxml.etree import XPath
    >>> XPath(GenericTranslator().css_to_xpath('*[aval="x\x01y"]'))
    ValueError: All strings must be XML compatible: Unicode or ASCII, no NULL bytes or control characters

    I think that is fine and out of scope here, since §3.3 only covers U+0000 and surrogates, but then the justification for this pull request is spec conformance, not the lxml error. Could you reword the description accordingly?

Everything passes locally for me otherwise.

@itecz26

itecz26 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

makes sense on both. trimmed the comments down to a single §3.3 line (regex + call site) and cut the lxml bit from the test comment. reworded the description too, since you're right that §3.3 only covers u+0000 and surrogates, so the raw control-char case is out of scope and this is really a conformance fix, not an lxml-compat one.

@wRAR
wRAR merged commit 698a590 into scrapy:master Sep 16, 2026
23 checks passed
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