Open
sbushmelev
opened this issue 1 month ago
•
6 comments
💥 What does this PR do?
Refactoring to 17 java style
🔄 Types of changes
Cleanup (formatting, renaming)
PR Type
Enhancement
Description
Modernize Java code to Java 17 style conventions
Convert switch statements to enhanced switch expressions
Reorganize imports to follow Java conventions
Refactor string formatting to use text blocks
Improve code indentation and formatting consistency
Diagram Walkthrough
flowchart LR
A["Java Code"] -->|"Switch to enhanced switch expressions"| B["Modern Switch Syntax"]
A -->|"Reorganize imports"| C["Proper Import Order"]
A -->|"Use text blocks"| D["Multi-line String Formatting"]
A -->|"Fix indentation"| E["Consistent Code Style"]
B --> F["Java 17 Compliant Code"]
C --> F
D --> F
E --> F
Below is a summary of compliance checks for this PR:
Security Compliance
⚪
Information disclosure
Description: The new WebDriverException message is a multi-line text block that ends with a newline, which may inadvertently leak thread names and IDs verbatim to logs; consider whether exposing thread identifiers in exception messages is acceptable for your threat model. ThreadGuard.java [86-92]
Referred Code
String.format(
"""
Thread safety error; this instance of WebDriver was constructed on
thread %s (id %d) and is being accessed by thread %s (id %d)
This is not permitted and *will* cause undefined behaviour
""", threadName, threadId, currentThread.getName(), currentThread.getId()));
}
Ensure clicking links with javascript in the href triggers the JavaScript as it did in version 2.47.1, including on Firefox 42.0 (32-bit) on a 64-bit machine.
Provide compatibility so that behavior remains consistent across versions 2.48.0/2.48.2 where regression was observed.
Include any necessary handling in WebDriver to correctly trigger href-based JavaScript alerts during click().
Diagnose and resolve "Error: ConnectFailure (Connection refused)" occurring when instantiating multiple ChromeDriver instances on Ubuntu 16.04.4 with Chrome 65 and ChromeDriver 2.35 using Selenium 3.9.0.
Ensure subsequent ChromeDriver instantiations after the first do not fail with connection refused errors.
Provide a stable initialization flow for repeated ChromeDriver creation without console errors.
Codebase Duplication Compliance
⚪
Codebase context is not defined
Follow the guide to enable codebase context checks.
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code
Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting
Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance.
Status: No auditing: The new code modifies error messaging and proxy creation but does not add or enhance any audit logging for critical actions, making it unclear whether required audit trails are maintained.
Referred Code
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
try {
if (Thread.currentThread().getId() != threadId) {
Thread currentThread = Thread.currentThread();
throw new WebDriverException(
String.format(
"""
Thread safety error; this instance of WebDriver was constructed on
thread %s (id %d) and is being accessed by thread %s (id %d)
This is not permitted and *will* cause undefined behaviour
""", threadName, threadId, currentThread.getName(), currentThread.getId()));
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Input validation: The enhanced switch continues to read multiple JSON fields without visible validation or sanitization, leaving uncertainty about handling of malformed or unexpected input.
Referred Code
case "context" -> id = input.read(String.class);
case "url" -> url = input.read(String.class);
case "children" -> children = input.read(LIST_OF_BROWSING_CONTEXT_INFO);
case "parent" -> parentBrowsingContext = input.read(String.class);
case "clientWindow" -> clientWindow = input.read(String.class);
case "originalOpener" -> originalOpener = input.read(String.class);
case "userContext" -> userContext = input.read(String.class);
default -> input.skipValue();
The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.
After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.
We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us
To preserve the original single-line error message format, use .stripIndent() and line continuation characters (</code>) within the text block to remove unwanted whitespace and newlines.
String.format(
"""
- Thread safety error; this instance of WebDriver was constructed on
- thread %s (id %d) and is being accessed by thread %s (id %d)
- This is not permitted and *will* cause undefined behaviour
- """, threadName, threadId, currentThread.getName(), currentThread.getId())
+ Thread safety error; this instance of WebDriver was constructed on \
+ thread %s (id %d) and is being accessed by thread %s (id %d). \
+ This is not permitted and *will* cause undefined behaviour.
+ """.stripIndent(), threadName, threadId, currentThread.getName(), currentThread.getId())
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that the refactoring to a text block unintentionally alters the error message format by adding newlines and whitespace, and it provides a valid solution using .stripIndent() and line continuations to restore the original single-line format.
Low
Learned best practice
Sanitize thread names in messages
Sanitize thread names before inserting them into the formatted message to prevent control characters or unexpected formatting from being injected into logs or UIs.
+String safeThreadName = currentThread.getName() == null ? "<unknown>" : currentThread.getName().replaceAll("\\p{Cntrl}", "?");
+String safeConstructedName = threadName == null ? "<unknown>" : threadName.replaceAll("\\p{Cntrl}", "?");
throw new WebDriverException(
- String.format(
- """
- Thread safety error; this instance of WebDriver was constructed on
- thread %s (id %d) and is being accessed by thread %s (id %d)
- This is not permitted and *will* cause undefined behaviour
- """, threadName, threadId, currentThread.getName(), currentThread.getId()));
+ String.format(
+ """
+ Thread safety error; this instance of WebDriver was constructed on
+ thread %s (id %d) and is being accessed by thread %s (id %d)
+ This is not permitted and *will* cause undefined behaviour
+ """, safeConstructedName, threadId, safeThreadName, currentThread.getId()));
Why:
Relevant best practice - Validate and sanitize external inputs when constructing messages to avoid unintended formatting or injection; avoid embedding raw text into format strings that use placeholders.
Low
[ ] More <!-- /improve --more_suggestions=true -->
@sbushmelev Thank you. Unfortunately, currently Selenium must support Java 11.
But hopefully soon we will release Selenium 5.x which will run on Java17+.
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
