Skip to content

[java] Prevent accessing BiDi internals from driver #17745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pujagani wants to merge 1 commit into from
Closed

[java] Prevent accessing BiDi internals from driver #17745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions java/src/org/openqa/selenium/bidi/BiDi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.function.Consumer;
import java.util.logging.Logger;
import org.openqa.selenium.Beta;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.internal.Require;

Expand All @@ -40,6 +41,16 @@ public class BiDi implements Closeable {
private final Connection connection;
private final Map<Event<?>, List<Long>> contextListenerIds = new ConcurrentHashMap<>();

public static BiDi from(WebDriver driver) {
Require.nonNull("WebDriver", driver);
if (!(driver instanceof HasBiDi)) {
throw new BiDiException("WebDriver does not support BiDi protocol");
}
return ((HasBiDi) driver)
.maybeGetBiDi()
.orElseThrow(() -> new BiDiException("Unable to create a BiDi connection"));
}
Comment on lines +44 to +52

@qodo-code-review qodo-code-review Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. bidi.from() missing javadoc 📘 Rule violation ✧ Quality

The new public API method BiDi.from(WebDriver) lacks a Javadoc block, and therefore does not
document its parameter and return value as required. This reduces API clarity and violates the
project’s public API documentation requirements.
Agent Prompt 
## Issue description
`BiDi.from(WebDriver)` is a newly added public API method but has no Javadoc, so it does not document purpose, `@param driver`, and `@return`.

## Issue Context
Public API methods must have Javadoc and include tags for parameters and non-void returns.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[44-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +44 to +52

@qodo-code-review qodo-code-review Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Bidi.from skips initialization 🐞 Bug ≡ Correctness

BiDi.from(WebDriver) calls HasBiDi.maybeGetBiDi() and throws when it’s empty, but the BiDi augmenter
(BiDiProvider) intentionally keeps BiDi lazy and returns Optional.empty until getBiDi() is called.
This makes BiDi.from() fail on freshly augmented RemoteWebDriver instances, breaking
RemoteWebDriver.script()/RemoteScript and the updated RemoteWebDriverBiDiTest path.
Agent Prompt 
### Issue description
`BiDi.from(WebDriver)` currently uses `HasBiDi.maybeGetBiDi()` which may legitimately be empty for augmented drivers (BiDi is lazily created). This causes `BiDi.from()` to throw even when BiDi is supported, breaking BiDi-based features on `Augmenter`-enhanced `RemoteWebDriver`.

### Issue Context
- The `BiDiProvider` augmentation returns `Optional.empty()` from `maybeGetBiDi()` until `getBiDi()` is invoked (lazy initialization).
- The PR updates production code (`RemoteScript`) and tests to call `BiDi.from(driver)`, so this is a real runtime path.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[44-52]

### Suggested fix approach
Update `BiDi.from(WebDriver)` to *initialize* the BiDi instance when needed, e.g.:
- Prefer calling `((HasBiDi) driver).getBiDi()` (with a local `@SuppressWarnings("deprecation")` if necessary) so lazy augmenters initialize.
- Or: `hasBiDi.maybeGetBiDi().orElseGet(() -> hasBiDi.getBiDi())` to keep the “already initialized” fast path.

This ensures `BiDi.from()` works with `Augmenter`-provided `HasBiDi` implementations and fixes the failing `RemoteScript`/remote BiDi flows.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


/**
* @deprecated Use constructor with timeout parameter: {@link #BiDi(Connection, Duration)}
*/
Expand Down
6 changes: 6 additions & 0 deletions java/src/org/openqa/selenium/bidi/HasBiDi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@

import java.util.Optional;
import org.openqa.selenium.Beta;
import org.openqa.selenium.WebDriver;

@Beta
public interface HasBiDi {

/**
* @deprecated Use {@link BiDi#from(WebDriver)} instead.
*/
@Deprecated(since = "4.24", forRemoval = true)
default BiDi getBiDi() {
return maybeGetBiDi()
.orElseThrow(() -> new BiDiException("Unable to create a BiDi connection"));
Comment on lines +27 to 33

@qodo-code-review qodo-code-review Bot Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. hasbidi.getbidi() javadoc incomplete 📘 Rule violation ✧ Quality

The modified (deprecated) public method HasBiDi.getBiDi() has Javadoc consisting only of an
@deprecated tag and omits an @return tag. This violates the requirement for complete Javadoc on
touched public methods.
Agent Prompt 
## Issue description
The touched public method `HasBiDi.getBiDi()` has tag-only Javadoc (`@deprecated` only) and does not include an `@return` tag even though it returns `BiDi`.

## Issue Context
Public methods changed in this PR must have complete Javadoc including at least one free-text sentence and `@return` for non-void returns.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/HasBiDi.java[27-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down
3 changes: 1 addition & 2 deletions java/src/org/openqa/selenium/remote/RemoteScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.bidi.BiDi;
import org.openqa.selenium.bidi.HasBiDi;
import org.openqa.selenium.bidi.log.ConsoleLogEntry;
import org.openqa.selenium.bidi.log.JavascriptLogEntry;
import org.openqa.selenium.bidi.module.LogInspector;
Expand All @@ -56,7 +55,7 @@ class RemoteScript implements Script {

public RemoteScript(WebDriver driver) {
this.driver = driver;
this.biDi = ((HasBiDi) driver).getBiDi();
this.biDi = BiDi.from(driver);
this.logInspector = new LogInspector(driver);
this.script = new org.openqa.selenium.bidi.module.Script(driver);
}
Expand Down
4 changes: 2 additions & 2 deletions java/test/org/openqa/selenium/bidi/BiDiSessionCleanUpTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ final class BiDiSessionCleanUpTest extends JupiterTestBase {
void shouldNotCloseBiDiSessionIfOneWindowIsClosed() {
localDriver = new WebDriverBuilder().get();
assumeThat(localDriver).isInstanceOf(HasBiDi.class);
BiDi biDi = ((HasBiDi) localDriver).getBiDi();
BiDi biDi = BiDi.from(localDriver);

BiDiSessionStatus status = biDi.getBidiSessionStatus();
assertThat(status).isNotNull();
Expand All @@ -59,7 +59,7 @@ void shouldNotCloseBiDiSessionIfOneWindowIsClosed() {
void shouldCloseBiDiSessionIfLastWindowIsClosed() {
localDriver = new WebDriverBuilder().get();
assumeThat(localDriver).isInstanceOf(HasBiDi.class);
BiDi biDi = ((HasBiDi) localDriver).getBiDi();
BiDi biDi = BiDi.from(localDriver);

BiDiSessionStatus status = biDi.getBidiSessionStatus();
assertThat(status).isNotNull();
Expand Down
2 changes: 1 addition & 1 deletion java/test/org/openqa/selenium/bidi/BiDiSessionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class BiDiSessionTest extends JupiterTestBase {

@Test
void shouldBeAbleToCreateABiDiSession() {
BiDi biDi = ((HasBiDi) driver).getBiDi();
BiDi biDi = BiDi.from(driver);

BiDiSessionStatus status = biDi.getBidiSessionStatus();
assertThat(status).isNotNull();
Expand Down
4 changes: 2 additions & 2 deletions java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
import org.openqa.selenium.UsernameAndPassword;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WindowType;
import org.openqa.selenium.bidi.BiDi;
import org.openqa.selenium.bidi.BiDiException;
import org.openqa.selenium.bidi.HasBiDi;
import org.openqa.selenium.bidi.browsingcontext.BrowsingContext;
import org.openqa.selenium.bidi.module.Network;
import org.openqa.selenium.testing.JupiterTestBase;
Expand Down Expand Up @@ -222,7 +222,7 @@ void canContinueWithAuthCredentials() {

assertThat(driver.findElement(By.tagName("h1")).getText()).isEqualTo("authorized");

((HasBiDi) driver).getBiDi().removeListener(callbackId);
BiDi.from(driver).removeListener(callbackId);
}
}

Expand Down
4 changes: 2 additions & 2 deletions java/test/org/openqa/selenium/firefox/FirefoxDriverConcurrentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ void multipleFirefoxInstancesWithBiDiEnabledCanRunSimultaneously() {

try {
driver1 = new WebDriverBuilder().get(options1);
BiDi biDi1 = ((FirefoxDriver) driver1).getBiDi();
BiDi biDi1 = BiDi.from(driver1);
assertThat(biDi1).isNotNull();

// Extract the BiDi websocket URL and port for the first instance
Expand All @@ -186,7 +186,7 @@ void multipleFirefoxInstancesWithBiDiEnabledCanRunSimultaneously() {
String port1 = webSocketUrl1.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1");

driver2 = new WebDriverBuilder().get(options2);
BiDi biDi2 = ((FirefoxDriver) driver2).getBiDi();
BiDi biDi2 = BiDi.from(driver2);
assertThat(biDi2).isNotNull();

// Extract the BiDi websocket URL and port for the second instance
Expand Down
10 changes: 4 additions & 6 deletions java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.openqa.selenium.WindowType;
import org.openqa.selenium.bidi.BiDi;
import org.openqa.selenium.bidi.BiDiSessionStatus;
import org.openqa.selenium.bidi.HasBiDi;
import org.openqa.selenium.bidi.browsingcontext.BrowsingContext;
import org.openqa.selenium.bidi.browsingcontext.NavigationResult;
import org.openqa.selenium.bidi.log.ConsoleLogEntry;
Expand Down Expand Up @@ -96,11 +95,10 @@ void tearDownDeployment() {
@Test
@NoDriverBeforeTest
void ensureBiDiSessionCreation() {
try (BiDi biDi = ((HasBiDi) localDriver).getBiDi()) {
BiDiSessionStatus status = biDi.getBidiSessionStatus();
assertThat(status).isNotNull();
assertThat(status.getMessage()).isNotEmpty();
}
BiDi biDi = BiDi.from(localDriver);
BiDiSessionStatus status = biDi.getBidiSessionStatus();
assertThat(status).isNotNull();
assertThat(status.getMessage()).isNotEmpty();
}

@Test
Expand Down
Loading