You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
New feature (non-breaking change which adds functionality and tests!)
PR Type
Enhancement
Description
Implements emulation.setTimezoneOverride BiDi command for Java
Creates reusable AbstractOverrideParameters base class for override operations
Refactors SetGeolocationOverrideParameters to extend new abstract class
Adds comprehensive tests for timezone override in contexts and user contexts
Diagram Walkthrough
flowchart LR
A["OverrideParameters<br/>Interface"] --> B["AbstractOverrideParameters<br/>Base Class"]
B --> C["SetGeolocationOverrideParameters<br/>Refactored"]
B --> D["SetTimezoneOverrideParameters<br/>New"]
C --> E["Emulation<br/>setTimezoneOverride Method"]
D --> E
E --> F["SetTimezoneOverrideTest<br/>New Tests"]
Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance.
Status: No auditing: The new setTimezoneOverride operation performs a critical environment change without emitting any audit/log record, but logging patterns for this component are not visible in the diff.
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Partial validation: Context selection is validated, but there is no validation of the timezone value itself (null/empty/format), which may rely on external spec enforcement.
Referred Code
@OverridepublicMap<String, Object> toMap() {
// Validate that either contexts or userContexts is setif (!map.containsKey("contexts") && !map.containsKey("userContexts")) {
thrownewIllegalStateException("Must specify either contexts or userContexts");
}
returnnewHashMap<>(map);
}
Description: The change stores a direct reference to the provided params map instead of an immutable copy, allowing external mutation after command creation which could lead to parameter tampering or race conditions if the map is shared across threads. Command.java [47-53]
Follow the guide to enable codebase context checks.
Custom Compliance
🟢
Generic: Secure Error Handling
Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging.
Status: Passed
Generic: Secure Logging Practices
Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data.
Status: Passed
🔴
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation
Status: Missing validation: The constructor accepts a timezone string without validating null/empty or format, risking invalid inputs propagating to the command.
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Mutable params: Replacing the immutable Map.copyOf with a mutable map reference can allow external mutation of command parameters, potentially introducing security and reliability issues.
✅ Return a defensive copy of mapSuggestion Impact:The commit updated the return statement to create and return a new HashMap from the internal map, implementing the defensive copy as suggested.
code diff:
- return map;+ return new HashMap<>(map);
Return a defensive copy of the internal map in the toMap() method to prevent external modification and maintain encapsulation.
Why: The suggestion correctly identifies a significant encapsulation issue in the new AbstractOverrideParameters class where returning a direct reference to the internal map allows bypassing validation logic. Providing a defensive copy is crucial for maintaining the object's integrity.
Medium
Possible issue
✅ Create a defensive copy of parametersSuggestion Impact:The commit changed the assignment of params to wrap a new HashMap copy with Collections.unmodifiableMap, exactly implementing the defensive copy and immutability as suggested.
code diff:
+ this.params =+ Collections.unmodifiableMap(+ new java.util.HashMap<>(Require.nonNull("Command parameters", params)));
To ensure the Command object is immutable, create a defensive, unmodifiable copy of the params map that allows null values.
Why: The PR removed Map.copyOf() which made the params map immutable but disallowed nulls. This suggestion correctly identifies that the new implementation is mutable from the outside and proposes a solution that restores immutability while still allowing null values, improving the robustness of the Command object.
Medium
Learned best practice
Validate timezone constructor input
Validate timezone input (allow null for reset, otherwise non-empty) to prevent invalid values from reaching the BiDi command.
public SetTimezoneOverrideParameters(String timezone) {
+ if (timezone != null && timezone.isBlank()) {+ throw new IllegalArgumentException("Timezone must be null (to reset) or a non-empty string");+ }
map.put("timezone", timezone);
}
Apply / Chat
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - Guard external API and I/O operations with targeted validation to surface clear errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
💥 What does this PR do?
Implements
emulation.setTimezoneOverridefor Selenium Java🔧 Implementation Notes
Implemented as per W3C spec
💡 Additional Considerations
🔄 Types of changes
PR Type
Enhancement
Description
Implements
emulation.setTimezoneOverrideBiDi command for JavaCreates reusable
AbstractOverrideParametersbase class for override operationsRefactors
SetGeolocationOverrideParametersto extend new abstract classAdds comprehensive tests for timezone override in contexts and user contexts
Diagram Walkthrough
File Walkthrough
OverrideParameters.java
New interface for override parametersjava/src/org/openqa/selenium/bidi/emulation/OverrideParameters.java
contexts(),userContexts(), andtoMap()methodsAbstractOverrideParameters.java
Abstract base class for override parametersjava/src/org/openqa/selenium/bidi/emulation/AbstractOverrideParameters.java
OverrideParametersinterfacecontexts()anduserContexts()methods
userContexts
toMap()with validation ensuring at least one is specifiedSetTimezoneOverrideParameters.java
New timezone override parameters classjava/src/org/openqa/selenium/bidi/emulation/SetTimezoneOverrideParameters.java
AbstractOverrideParametersfor timezone override"+05:30")
Emulation.java
Add setTimezoneOverride method to Emulationjava/src/org/openqa/selenium/bidi/emulation/Emulation.java
setTimezoneOverride()method to Emulation classemulation.setTimezoneOverrideBiDi command with parametersSetGeolocationOverrideParameters.java
Refactor to use abstract base classjava/src/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideParameters.java
AbstractOverrideParametersinstead of duplicatingcode
contexts(),userContexts(), andtoMap()implementations
Command.java
Remove immutable copy of command parametersjava/src/org/openqa/selenium/bidi/Command.java
paramsassignment to removeMap.copyOf()wrapperSetGeolocationOverrideTest.java
Rename test class for clarityjava/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java
EmulationTesttoSetGeolocationOverrideTestSetTimezoneOverrideTest.java
New tests for timezone override functionalityjava/test/org/openqa/selenium/bidi/emulation/SetTimezoneOverrideTest.java