[grid] stop forwarding se:remoteUrl past the Node that consumes it

[titusfortner]

🔗 Related Issues

Regression from #17790

💥 What does this PR do?

• Fixes WebDriver BiDi (and CDP/VNC) failing in Dynamic Grid — standalone-docker, node-docker, Kubernetes, and Relay — where the session starts but the WebSocket never connects, logging Failed to establish WebSocket endpoint. BiDi then degrades silently, so tests that only request it without using it still pass.
• Fixes IE sessions through the Grid failing to start at all with se:remoteUrl is an unknown extension capability for IE. Affects any driver that validates extension capabilities, with or without BiDi; Chrome, Firefox, and Edge ignore them, so IE surfaced it.
• Keeps what [grid] honor client-advertised se:remoteUrl for reachable BiDi/CDP/VNC URLs #17790 fixed: clients behind Docker port-mapping or a reverse proxy still get reachable BiDi/CDP/VNC URLs.
• se:remoteUrl now describes a single hop. It tells a Node how the client reached the Grid, and stops there rather than being forwarded to another Selenium server or a driver binary.

🔧 Implementation Notes

• Stripping does not undo [grid] honor client-advertised se:remoteUrl for reachable BiDi/CDP/VNC URLs #17790 because the client-facing URL and the Grid→browser URL are built at different hops. Only the first hop faces the client, and it still resolves se:remoteUrl. Deeper hops talk to targets they already reach by construction — each factory issues its new-session request over that same address — so their auto-detected address was never the problem.
• The multi-hop case was missed originally because the change modeled a single Node consuming the capability. In Dynamic Grid and Relay the chain has a second Selenium server, which applied se:remoteUrl as if the client had reached it. [grid] honor client-advertised se:remoteUrl for reachable BiDi/CDP/VNC URLs #17790 already stripped the capability from the response; it just needed stripping on the way in too.
• Stripping happens on the outbound NEW_SESSION command only, not on the capabilities merged into each ActiveSession. LocalNode.getSession() re-resolves the proxied URLs from the session's own capabilities, so removing it there would make getSession() disagree with newSession().
• stripPerHopCapabilities names the policy rather than exposing a generic removeCapability, so the capability name and the reason it must not travel live in one place instead of being repeated at four call sites. A generic remover would also sit oddly on SessionFactory, which is about creating sessions, and LocalNode is not one.
• The regression test lives in RelaySessionFactoryTest because Relay is the only delegating factory testable in-process; Docker and Kubernetes need a daemon or cluster. All four share the same helper.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: diagnosis, fix, and tests
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• Consider adding Dynamic Grid BiDi tests to docker-selenium and running them against nightly releases.
• If we are going to add or use se:* options in places it would make sense to ignore them in IEDriverServer, otherwise we must ensure none of them ever get sent to it.
• Separate pre-existing bug: setEnvVarsToContainer propagates Grid process config (SE_NODE_*) into browser containers, so a Grid configured with SE_NODE_GRID_URL reproduces this symptom by another route. Only image settings should propagate.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Grid: stop forwarding "se:remoteUrl" beyond the consuming Node

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Strip "se:remoteUrl" from outbound NEW_SESSION requests to downstream session endpoints.
• Prevent multi-hop Grid/Relay/Dynamic Grid from proxying BiDi/CDP/VNC back to itself.
• Add unit and relay regression tests to verify stripping while keeping local session caps.

Diagram

graph TD
  C(["Client"]) --> N["Grid Node"] --> F["SessionFactory impl"] --> S[["stripPerHopCapabilities\n(removes se:remoteUrl)"]] --> R["Downstream driver / Selenium server"]
  F --> A[("ActiveSession")]

  subgraph Legend
    direction LR
    _cli(["Client"]) ~~~ _svc["Component"] ~~~ _fn[["Helper"]] ~~~ _as[("State")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Carry per-hop metadata via headers (not capabilities)

• ➕ Avoids W3C extension-capability validation issues entirely (e.g., IE)
• ➕ Clarifies that the value is transport metadata, not desired capabilities
• ➖ Larger protocol-surface change; requires plumbing through multiple HTTP layers
• ➖ Harder to support across all client/server combinations without compatibility risks

2. Strip per-hop capabilities at request ingress, then store separately

• ➕ Guarantees downstream never sees per-hop capabilities regardless of factory
• ➕ Keeps a single sanitization point
• ➖ Requires adding a new internal field to preserve "se:remoteUrl" for later URL reconstruction
• ➖ More refactoring across Node/session lifecycle than this targeted fix

Recommendation: The chosen approach (strip only on outbound NEW_SESSION inside factories, while preserving the original capabilities for ActiveSession) is the best minimal fix: it corrects multi-hop forwarding behavior without changing how Nodes reconstruct client-reachable BiDi/CDP/VNC/VNC URLs. The header-based or internal-metadata approaches are cleaner conceptually but would be significantly more invasive.

Files changed (7) +165 / -4

Bug fix (5) +33 / -4

SessionFactory.javaAdd per-hop capability stripping helper for session forwarding +17/-0

Add per-hop capability stripping helper for session forwarding

• Introduces SessionFactory.stripPerHopCapabilities to remove "se:remoteUrl" before forwarding a new-session request upstream. This prevents downstream servers/drivers from misinterpreting the client-to-Grid address or rejecting the extension capability.

java/src/org/openqa/selenium/grid/node/SessionFactory.java

DriverServiceSessionFactory.javaStrip "se:remoteUrl" before sending NEW_SESSION to local driver service +4/-1

Strip "se:remoteUrl" before sending NEW_SESSION to local driver service

• Wraps the NEW_SESSION command to use stripPerHopCapabilities(capabilities). Prevents driver binaries that validate capabilities from failing due to forwarded Grid-only metadata.

java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java

DockerSessionFactory.javaStrip "se:remoteUrl" before forwarding NEW_SESSION into Docker container +4/-1

Strip "se:remoteUrl" before forwarding NEW_SESSION into Docker container

• Sanitizes desired capabilities for the downstream NEW_SESSION call so container-hosted drivers don't see Grid hop metadata. Keeps merged capabilities (including original values) for local session behavior.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java

KubernetesSessionFactory.javaStrip "se:remoteUrl" before forwarding NEW_SESSION into Kubernetes job +4/-1

Strip "se:remoteUrl" before forwarding NEW_SESSION into Kubernetes job

• Updates the downstream NEW_SESSION request to remove per-hop capabilities prior to protocol handshake. Avoids misrouting/validation issues in multi-hop Grid setups.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java

RelaySessionFactory.javaStrip "se:remoteUrl" before forwarding NEW_SESSION to relay endpoint +4/-1

Strip "se:remoteUrl" before forwarding NEW_SESSION to relay endpoint

• Ensures relay factories do not forward the client-to-Grid remote URL to the remote end, preventing it from advertising WebSocket endpoints pointing back at the Node. The returned ActiveSession capabilities still include the original value for local URL reconstruction.

java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

Tests (2) +132 / -0

SessionFactoryTest.javaUnit test coverage for stripPerHopCapabilities behavior +67/-0

Unit test coverage for stripPerHopCapabilities behavior

• Adds focused tests verifying that "se:remoteUrl" is removed, other capabilities remain unchanged, and capability objects are returned as-is when no stripping is required.

java/test/org/openqa/selenium/grid/node/SessionFactoryTest.java

RelaySessionFactoryTest.javaRegression test: relay must not forward "se:remoteUrl" downstream +65/-0

Regression test: relay must not forward "se:remoteUrl" downstream

• Adds an in-process relay test that captures the forwarded /session payload to ensure "se:remoteUrl" is omitted. Also verifies the resulting ActiveSession retains "se:remoteUrl" locally for building client-reachable BiDi/CDP/VNC URLs.

java/test/org/openqa/selenium/grid/node/relay/RelaySessionFactoryTest.java

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Copilot]

Pull request overview

This PR fixes a Grid regression where the client-advertised se:remoteUrl capability was being forwarded past the Selenium Node/factory that consumes it, causing downstream Selenium servers (Dynamic Grid/Relay) to mis-resolve proxied BiDi/CDP/VNC URLs and causing strict drivers (notably IE) to reject session creation due to an unknown extension capability.

Changes:

• Introduces SessionFactory.stripPerHopCapabilities(...) to remove hop-scoped capabilities (currently se:remoteUrl) from outbound NEW_SESSION requests sent to downstream servers/drivers.
• Applies the stripping consistently across delegating session factories (Relay, Docker, Kubernetes, DriverService) when constructing the outbound NEW_SESSION command.
• Adds unit/regression tests to ensure se:remoteUrl is stripped from forwarded payloads while remaining available locally for Node-side URL resolution.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
java/src/org/openqa/selenium/grid/node/SessionFactory.java	Adds a centralized helper to strip hop-scoped capabilities (removes se:remoteUrl) before forwarding new-session requests downstream.
java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java	Uses the helper when creating the outbound NEW_SESSION request to the relay/Appium endpoint.
java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java	Uses the helper when creating the outbound NEW_SESSION request to the containerized driver service.
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java	Uses the helper when creating the outbound NEW_SESSION request to the K8s pod driver service.
java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java	Uses the helper when creating the outbound NEW_SESSION request to local driver services (prevents strict drivers from rejecting se:remoteUrl).
java/test/org/openqa/selenium/grid/node/SessionFactoryTest.java	Adds unit coverage for the stripping helper (removal of se:remoteUrl, preservation of other caps, no-op behavior).
java/test/org/openqa/selenium/grid/node/relay/RelaySessionFactoryTest.java	Adds a regression test asserting se:remoteUrl is not forwarded to the remote end while remaining present in the Node-local merged capabilities.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.