[js] Ensure BiDi is not exposed on driver - #17829
Closed
pujagani wants to merge 84 commits into
Closed
Conversation
* [js] parse BiDi CDDL in generate_bidi.mjs, drop py/private merge_cddl dependency
… custom Firefox webExtension options (SeleniumHQ#17840) * [build] add local CDDL overlays for webExtension.install SeleniumHQ#1140 extension point and moz vendor params * [js] implement in schema not in merged cddl file that is sent to cddl2ts * [rb] generate a browser-scoped webExtension vendor subclass (WebExtension::Moz)
…C URLs (SeleniumHQ#17790) * [grid] honor client-advertised se:remoteUrl in the Node with grid-url precedence * [grid] advertise se:remoteUrl from the language bindings * [grid] fix se:remoteUrl on URL constructor, https scheme casing, and builder capability conflict * [grid] strip se:remoteUrl from returned session caps * [grid] use only the origin of se:remoteUrl for proxied URLs
…SE_BIDI_STRICT to escalate (SeleniumHQ#17844)
…licy in AGENTS.md
…Q#17838) * [rust] locate Chrome and Edge in known install directories * [rust] honor --skip-browser-in-path and keep WebView2 out of Edge browser lookup
* [dotnet] throw instead of warn for CDP on Firefox * [py] raise for CDP methods on Firefox * [rb] remove CDP extensions from Firefox so methods are unavailable
… match severity (SeleniumHQ#17848) * [rb] match log entries by id and severity, add missing log ids, drop logger stubs * [rb] match message assertions against entries carrying multiple ids
* [rb] glob spec sources into //rb/spec:spec for rubocop and clear hidden offenses * [rb] remove unnecessary rb_library from rb_integration_test now that //rb/spec:spec globs sources
…date primitives outbound in Ruby (SeleniumHQ#17852) * [build] bump cddl to 0.21.1 so float BiDi ranges type as number, not integer * [build] hoist nullable inline enums to named enums so their vocabulary is validated * [rb] validate outbound BiDi primitive types at construction
…eleniumHQ#17853) * [build] drop preserveExtras schema signal; extensibility alone gates wire extras * [rb] retain wire extras on every extensible BiDi type, not only re-sendable ones * [rb] keep extensible extras without warning and reject extras shadowing declared fields
…e map (SeleniumHQ#17855) * [rb] raise typed WebDriver errors for BiDi from a generated error-code map * [rb] declare RBS only for BiDi-only error classes to avoid redeclaring classic ones
…DEO_SESSION_SUBFOLDER (SeleniumHQ#17856) Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
…SeleniumHQ#17859) * [rb] add exception-aware pending_if guard matching * [rb] keep pending_exception_guard spec deterministic under SKIP_PENDING
…LE is set (SeleniumHQ#17858) [dotnet][java][py][rb] pass --enable-chrome-logs unless CHROME_LOG_FILE is set or on Windows (SeleniumHQ#16201)
…eleniumHQ#17862) * [rb] add add_chromium_option/add_firefox_option escape hatches and merge hand-built vendor options * [rb] normalize vendor option names to string keys
…eleniumHQ#17861) * [rb] validate BiDi outbound ref fields against their declared type * [rb] reject a non-variant object at a scalar-arm BiDi union * [rb] validate BiDi union bare-scalar arms against their schema literals
…17863) * [rb] capture log matcher output under SE_DEBUG and keep service specs env-agnostic * [rb] drop the SE_DEBUG output lock in the spec log-capture helper
[js] normalize empty custom locator results Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
* [rb] scaffold BiDi protocol integration specs from schema * [build] run edge-bidi targets in ci-ruby full suite * [rb] raise BiDi SerializationError on wire deserialization mismatches * [rb] guard bidi protocol specs for unsupported browser commands and timeouts * [rb] skip unsupported BiDi modules on Safari with per-module support probes * [rb] enable strict BiDi serialization for all bidi specs via the test environment
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
SeleniumHQ#17909) [build] trim no-op gh-cache triggers and prune CodeQL caches from PR runs
…#17910) Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
[py] disable nightly TestPyPI publishing and add twine --verbose
[rb] fix Safari set_viewport spec guard and update Homebrew before STP install
…g last release (SeleniumHQ#17921) * [build] fix release mirror push target and poll Maven Central when verifying Java
…mHQ#17920)" (SeleniumHQ#17924) This reverts commit 7f32b68.
…into bidi-access-js
pujagani
marked this pull request as ready for review
August 18, 2026 07:01
Contributor
PR Summary by QodoDeprecate direct JavaScript WebDriver BiDi connection access
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Contributor
Code Review by Qodo
1. Raw BiDi remains exposed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1790
to
+1792
| WebDriver.prototype.getBidi = util.deprecate(function () { | ||
| return getBidiConnection(this) | ||
| }, 'WebDriver#getBidi() is deprecated. Use a composed BiDi module instead, e.g. Network.create(driver) or ' + "require('selenium-webdriver/bidi/network'). See docs/decisions/17670-bidi-implementation-boundaries.md.") |
Contributor
There was a problem hiding this comment.
1. Deprecation bypasses project logger 📘 Rule violation ✧ Quality
WebDriver#getBidi uses Node's util.deprecate instead of WebDriver.logger.deprecate and supplies no unique deprecation ID. This bypasses the required standardized deprecation mechanism.
Agent Prompt
## Issue description
`WebDriver#getBidi` emits its deprecation through `util.deprecate` rather than `WebDriver.logger.deprecate`, and the notice has no unique ID.
## Issue Context
Keep the existing guidance to composed BiDi modules, but emit it through the project-standard deprecation logger with a stable, non-empty identifier.
## Fix Focus Areas
- javascript/selenium-webdriver/lib/webdriver.js[1790-1792]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| * `require('selenium-webdriver/bidi/network')`. | ||
| * @function | ||
| * @name WebDriver#getBidi | ||
| * @returns {Promise<import('../bidi')>} |
Contributor
There was a problem hiding this comment.
2. getbidi return undocumented 📘 Rule violation ✧ Quality
The public WebDriver#getBidi JSDoc has an @returns type but no description of the returned value. Public API JSDoc requires a typed return tag with non-empty descriptive text.
Agent Prompt
## Issue description
The new JSDoc for public `WebDriver#getBidi` uses `@returns {Promise<import('../bidi')>}` without the required return-value description.
## Issue Context
Retain the existing type and add concise text explaining that the promise resolves to the session's BiDi connection.
## Fix Focus Areas
- javascript/selenium-webdriver/lib/webdriver.js[1778-1789]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,39 @@ | |||
| diff --git a/ruby/private/utils.bzl b/ruby/private/utils.bzl | |||
Contributor
There was a problem hiding this comment.
3. Third-party file added 📘 Rule violation § Compliance
The PR adds third_party/bazel/rules_ruby_windows_batch_crlf.patch, despite the checklist prohibiting additions or modifications under third_party/. The underlying integration or vendoring configuration must be changed outside that directory instead.
Agent Prompt
## Issue description
A new patch file is committed under `third_party/`, which is a prohibited path for direct additions or modifications.
## Issue Context
Move the required Windows batch handling to project-owned source or vendoring configuration outside `third_party/`, then remove this generated/vendor patch from the PR.
## Fix Focus Areas
- third_party/bazel/rules_ruby_windows_batch_crlf.patch[1-39]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| }, | ||
| "engines": { | ||
| "node": ">= 20.0.0" | ||
| "node": ">= 22.0.0" |
Contributor
There was a problem hiding this comment.
4. Pr contains unrelated changes 📘 Rule violation ✧ Quality
The diff spans hundreds of files across JavaScript, Java, .NET, Python, Ruby, Rust, build infrastructure, protocol schemas, and dependencies, far beyond hiding the JavaScript BiDi connection. It also changes unrelated user-visible policy such as the minimum Node.js version, making the PR difficult to review or revert safely.
Agent Prompt
## Issue description
The PR includes a large collection of unrelated release, dependency, runtime-policy, protocol, and cross-language changes alongside the JavaScript BiDi change.
## Issue Context
Rebase the branch onto the intended target and retain only the narrowly related JavaScript BiDi implementation, documentation, build entries, and regression tests. Submit unrelated accumulated changes separately.
## Fix Focus Areas
- javascript/selenium-webdriver/package.json[3-51]
- javascript/selenium-webdriver/CHANGES.md[1-20]
- third_party/bazel/rules_ruby_windows_batch_crlf.patch[1-39]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1790
to
+1792
| WebDriver.prototype.getBidi = util.deprecate(function () { | ||
| return getBidiConnection(this) | ||
| }, 'WebDriver#getBidi() is deprecated. Use a composed BiDi module instead, e.g. Network.create(driver) or ' + "require('selenium-webdriver/bidi/network'). See docs/decisions/17670-bidi-implementation-boundaries.md.") |
Contributor
There was a problem hiding this comment.
5. Raw bidi remains exposed 🐞 Bug ≡ Correctness
Assigning getBidi to WebDriver.prototype leaves the raw transport publicly discoverable and callable, contrary to the PR's stated boundary. Recommended composed modules still call _driver.getBidi(), so ordinary module use also invokes the deprecated API and emits its warning.
Agent Prompt
## Issue description
Remove the raw BiDi transport accessor from the public `WebDriver` prototype and prevent composed BiDi modules from invoking a deprecated public API.
## Issue Context
`WebDriver.prototype.getBidi` still exposes the transport, while composed modules such as Network and Browser depend on it. Introduce an internal connection mechanism and migrate those modules before removing the prototype accessor.
## Fix Focus Areas
- javascript/selenium-webdriver/lib/webdriver.js[1778-1792]
- javascript/selenium-webdriver/lib/bidi_connection.js[38-55]
- javascript/selenium-webdriver/bidi/network.js[93-95]
- javascript/selenium-webdriver/bidi/browser.js[29-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+798
to
+799
| // Close the BiDi websocket connection, if one was ever opened | ||
| closeBidiConnection(this) |
Contributor
There was a problem hiding this comment.
6. Bidi cleanup bypassed or unawaited 🐞 Bug ☼ Reliability
quit() reaches the added cleanup only when onQuit_ is absent, and even then it does not return or await the asynchronous close promise. Consequently await driver.quit() can finish without completing BiDi cleanup, while drivers with an onQuit_ callback skip explicit cleanup entirely.
Agent Prompt
## Issue description
Ensure WebDriver quit always performs and awaits BiDi connection cleanup, including when an `onQuit_` callback is configured.
## Issue Context
The finalizer returns early for `onQuit_`, and the new asynchronous close call is otherwise invoked without returning its promise. Browser-specific local drivers configure `onQuit_` to stop their services.
## Fix Focus Areas
- javascript/selenium-webdriver/lib/webdriver.js[775-800]
- javascript/selenium-webdriver/lib/bidi_connection.js[68-79]
- javascript/selenium-webdriver/bidi/index.js[318-337]
- javascript/selenium-webdriver/chromium.js[640-654]
- javascript/selenium-webdriver/firefox.js[580-604]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1349
to
+1351
| String.format( | ||
| "Ignoring unusable se:remoteUrl '%s'; using %s for proxied URLs", | ||
| value, gridUri)); |
Contributor
There was a problem hiding this comment.
7. Remote credentials logged 🐞 Bug ⛨ Security
When se:remoteUrl is unusable, resolvePublicGridUri logs its complete untrusted value, including any embedded username or password. JavaScript now sends the configured remote URL verbatim as this capability, allowing malformed or unsupported credential-bearing URLs to disclose secrets in Grid logs.
Agent Prompt
## Issue description
Prevent credentials embedded in `se:remoteUrl` from being written to Grid logs when the URL cannot be used.
## Issue Context
The JavaScript builder advertises the complete configured server URL. Grid's invalid-value warning interpolates that raw capability after URI validation fails or rejects the scheme.
## Fix Focus Areas
- javascript/selenium-webdriver/index.js[661-664]
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[1329-1353]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| describe '#send_keys' do | ||
| it 'sends keys to the active element', pending_if: {browser: %i[safari safari_preview]} do | ||
| it 'sends keys to the active element', pending_if: {browser_family: :safari} do |
Contributor
There was a problem hiding this comment.
1. pending_if guards omit reasons 📘 Rule violation ▣ Testability
Multiple modified Ruby integration-test guards omit the mandatory reason, leaving skipped or pending tests without an explicit explanation. This affects pending_if, skip_if, and flaky guards across several integration specs.
Agent Prompt
## Issue description
Modified integration-test guards omit the required explicit `reason` value.
## Issue Context
Every integration guard that supports a reason must provide a string literal or numeric issue identifier explaining why the test is guarded.
## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/action_builder_spec.rb[28-160]
- rb/spec/integration/selenium/webdriver/driver_spec.rb[33-356]
- rb/spec/integration/selenium/webdriver/element_spec.rb[32-351]
- rb/spec/integration/selenium/webdriver/fedcm_spec.rb[70-70]
- rb/spec/integration/selenium/webdriver/manager_spec.rb[82-166]
- rb/spec/integration/selenium/webdriver/navigation_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/target_locator_spec.rb[171-171]
- rb/spec/integration/selenium/webdriver/timeout_spec.rb[89-89]
- rb/spec/integration/selenium/webdriver/window_spec.rb[116-129]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| * | ||
| * @param {...string} paths The paths to the extension XPI files to install. | ||
| * @return {!Options} A self reference. | ||
| * @deprecated Use {@link Driver#installAddon} instead. |
Contributor
There was a problem hiding this comment.
2. addextensions lacks runtime warning 📘 Rule violation ◔ Observability
Options#addExtensions is newly documented as deprecated, but its body emits no warning naming the method and replacement API. Callers therefore receive no runtime migration guidance as required for deprecated APIs.
Agent Prompt
## Issue description
`Options#addExtensions` is marked deprecated only in JSDoc and emits no runtime deprecation warning.
## Issue Context
The warning should be emitted at most once, identify `Options#addExtensions`, and recommend `Driver#installAddon`. Use the project-standard deprecation mechanism rather than an ad-hoc print or log statement.
## Fix Focus Areas
- javascript/selenium-webdriver/firefox.js[315-324]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (pending === undefined) { | ||
| return | ||
| } | ||
| connections.delete(driver) |
Contributor
There was a problem hiding this comment.
3. Bidi reopens during shutdown 🐞 Bug ☼ Reliability
closeBidiConnection() deletes the cached entry before the existing connection finishes closing, so concurrent BiDi access creates a replacement connection. That replacement is not tracked by the active cleanup operation and can remain open after driver shutdown.
Agent Prompt
## Issue description
`closeBidiConnection()` removes the driver's cache entry before the pending connection has closed. A concurrent `getBidiConnection()` therefore creates a second websocket that is outside the active cleanup and may leak after shutdown.
## Issue Context
Preserve a closing/tombstone state for the driver so access cannot create another connection while shutdown is underway. Only clear lifecycle state when doing so cannot permit a connection to reopen for a quitting driver.
## Fix Focus Areas
- javascript/selenium-webdriver/lib/bidi_connection.js[38-42]
- javascript/selenium-webdriver/lib/bidi_connection.js[68-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 6288b65 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
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.
🔗 Related Issues
Related to #17814
💥 What does this PR do?
Ensure that BiDi connection is not accessible on driver.
🔧 Implementation Notes
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes