[java] Fix By.className()/By.id() misescaping non-ASCII leading digits#17815
[java] Fix By.className()/By.id() misescaping non-ASCII leading digits#17815diemol wants to merge 2 commits into
Conversation
PreW3CLocator.cssEscape (used by By.className/By.id, duplicated in
W3CHttpCommandCodec) used Character.isDigit() to detect a leading digit
needing CSS escaping. That accepts any Unicode decimal digit, not just
ASCII 0-9, so a leading non-ASCII digit (e.g. Arabic-Indic U+0665) got
rewritten using its numeric value as if it were the ASCII digit of the
same value, producing the same selector as an unrelated ASCII-digit
class/id (By.className("٥foo") collided with By.className("5foo")).
Per the CSS Syntax spec, "digit" is ASCII 0-9 only; non-ASCII code
points are already valid identifier-start characters and need no
escaping, so the fix narrows the check to the ASCII range instead of
adding new escaping logic.
PR Summary by QodoFix CSS escaping for non-ASCII leading digits in By.className()/By.id()
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1. Non-ASCII source literal
|
W3CHttpCommandCodec carries a byte-identical copy of the leading-digit CSS-escape logic fixed in By.java, but had no test file exercising it at all. ByTest/RemotableByTest only cover the By.java copy, since By.toJson() pre-escapes client-side before anything reaches a command codec, so a regression in this copy alone (e.g. from a future de-duplication refactor) would go unnoticed. Add W3CHttpCommandCodecTest exercising encode() for the "class name"/ "id" locator strategies directly, mirroring the ByTest cases. Verified these tests fail against the pre-fix implementation and reproduce the exact collision from the original report.
|
Added a second commit: Added |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| void ensureLeadingNonAsciiDigitClassNameIsNotMisescapedAsADifferentAsciiDigit() { | ||
| // U+0665 (Arabic-Indic digit five) has numeric value 5, but is not an ASCII digit. | ||
| // It must be passed through as-is, not (mis)escaped to the same selector as an ASCII '5'. | ||
| Map<String, Object> arabicIndicFive = encodeFindElement("class name", "٥foo"); | ||
| Map<String, Object> asciiFive = encodeFindElement("class name", "5foo"); | ||
|
|
||
| assertThat(arabicIndicFive) | ||
| .containsEntry("using", "css selector") | ||
| .containsEntry("value", ".٥foo"); | ||
| assertThat(arabicIndicFive.get("value")).isNotEqualTo(asciiFive.get("value")); |
There was a problem hiding this comment.
1. Non-ascii source literal 🐞 Bug ☼ Reliability
W3CHttpCommandCodecTest embeds the Arabic-Indic digit ٥ directly in Java string literals; if the compiler reads sources using a non-UTF-8 default encoding, this file may fail to compile or the literal may be decoded incorrectly. Using \u0665 escapes makes the test encoding-agnostic and aligns with existing test patterns in the repo.
Agent Prompt
### Issue description
`W3CHttpCommandCodecTest` contains raw non-ASCII characters (`٥`) in Java string literals. If source encoding is not consistently UTF-8, this can break compilation or alter the test input.
### Issue Context
The Bazel Java wrapper in this repo does not obviously enforce a `-encoding` flag, and other tests use `\uXXXX` escapes for non-ASCII strings.
### Fix Focus Areas
- java/test/org/openqa/selenium/remote/codec/w3c/W3CHttpCommandCodecTest.java[56-65]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| private Map<String, Object> encodeFindElement(String strategy, Object value) { | ||
| HttpRequest request = | ||
| codec.encode(new Command(sessionId, DriverCommand.FIND_ELEMENT(strategy, value))); | ||
| return json.toType(string(request), MAP_TYPE); | ||
| } |
There was a problem hiding this comment.
2. Deprecated string helper used 🐞 Bug ⚙ Maintainability
The new test calls deprecated Contents.string(HttpMessage) to read the request body, which adds avoidable deprecation usage and future maintenance risk. HttpRequest already exposes contentAsString(), so the test can parse the JSON body without the deprecated helper.
Agent Prompt
### Issue description
`W3CHttpCommandCodecTest` uses deprecated `Contents.string(HttpMessage)`.
### Issue Context
`Contents.string(HttpMessage<?>)` is annotated `@Deprecated` and explicitly recommends using `HttpMessage#contentAsString()`.
### Fix Focus Areas
- java/test/org/openqa/selenium/remote/codec/w3c/W3CHttpCommandCodecTest.java[20-23]
- java/test/org/openqa/selenium/remote/codec/w3c/W3CHttpCommandCodecTest.java[68-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ba21930 |
2 participants
🔗 Related Issues
Fixes #17805
💥 What does this PR do?
PreW3CLocator.cssEscape(used byBy.className()/By.id(), and duplicated inW3CHttpCommandCodec) usedCharacter.isDigit(...)to decide whether a leading character needs CSS's leading-digit escape.Character.isDigitis true for any Unicode decimal digit, not just ASCII0-9, so a leading non-ASCII digit (e.g. Arabic-Indic٥, U+0665) was rewritten using its numeric value as if it were the ASCII digit of that value. That collides with a completely different class/id:By.className("٥foo")andBy.className("5foo")both produced the selector.\35 foo.The fix narrows the check to the ASCII range (
'0'-'9') in both copies ofcssEscape, and adds regression tests toByTest.javacovering both the existing ASCII-digit escape and the non-ASCII-digit collision.🔧 Implementation Notes
Per the CSS Syntax spec,
digitis defined as ASCIIU+0030-U+0039only. Non-ASCII code points are already valid identifier-start characters (ident-start code point) and require no escaping at all — they can be emitted as-is, including as the first character of the identifier. So the minimal, spec-correct fix is to stop misclassifying non-ASCII digits as needing the ASCII digit-escape, rather than adding new escaping logic for them.I considered implementing a full CSS
serialize-an-identifier/CSS.escape()port, but that's unnecessary here: the existingCSS_ESCAPEregex already leaves non-ASCII characters untouched, so restricting the digit check to ASCII is sufficient and keeps the diff minimal.🤖 AI assistance
trunkcode and against the CSS Syntax/CSSOM specs (via jshell and spec review), authored the one-line fix in bothBy.javaandW3CHttpCommandCodec.java, and wrote the regression tests.💡 Additional Considerations
None — this is a minimal, self-contained fix with no behavior change for existing ASCII-digit or non-digit inputs.
🔄 Types of changes