[grid] honor client-advertised se:remoteUrl for reachable BiDi/CDP/VNC URLs#17790
Open
titusfortner wants to merge 2 commits into
Open
[grid] honor client-advertised se:remoteUrl for reachable BiDi/CDP/VNC URLs#17790titusfortner wants to merge 2 commits into
titusfortner wants to merge 2 commits into
Conversation
selenium-ci
added
B-grid
Contributor
PR Summary by QodoGrid: use client-advertised se:remoteUrl for reachable BiDi/CDP/VNC endpoints
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
39 rules 1. gridUrlSpecified() missing Javadoc
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…f address comparison
Comment on lines
+1577
to
+1580
| public Builder gridUrlSpecified(boolean configured) { | ||
| this.gridUrlSpecified = configured; | ||
| return this; | ||
| } |
Contributor
There was a problem hiding this comment.
1. gridurlspecified() missing javadoc 📘 Rule violation ✧ Quality
The new public method LocalNode.Builder.gridUrlSpecified(boolean configured) lacks a Javadoc block with @param/@return, violating the requirement for complete Javadoc on public API methods. This reduces API clarity and makes the builder contract harder to maintain.
Agent Prompt
## Issue description
A new public method was added without the required complete Javadoc (description + `@param` + `@return`).
## Issue Context
`LocalNode.Builder.gridUrlSpecified(boolean configured)` is public and should be documented per the project’s Javadoc requirements.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[1577-1580]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
176
to
180
| EventBus bus, | ||
| URI uri, | ||
| URI gridUri, | ||
| boolean gridUrlSpecified, | ||
| @Nullable HealthCheck healthCheck, |
Contributor
There was a problem hiding this comment.
2. localnode constructor signature changed 📘 Rule violation ≡ Correctness
The protected LocalNode(...) constructor now requires an additional boolean gridUrlSpecified parameter, which is a potentially breaking ABI/API change for any external subclasses calling super(...). Consider adding a backward-compatible overload delegating to the new constructor to preserve compatibility.
Agent Prompt
## Issue description
A new required parameter was added to a `protected` constructor, which can break downstream subclasses compiled against the previous signature.
## Issue Context
Even though call sites in-repo were updated, `protected` constructors are part of the subclassing surface and can impact external users.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[174-205]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1303
to
+1321
| // A configured grid-url always wins; only when the node falls back to its auto-detected address | ||
| // (which may be unreachable behind Docker/proxy) do we use the client-advertised se:remoteUrl. | ||
| private URI resolvePublicGridUri(Capabilities caps) { | ||
| if (gridUrlSpecified) { | ||
| return gridUri; | ||
| } | ||
| Object raw = caps.getCapability("se:remoteUrl"); | ||
| if (raw instanceof String && !((String) raw).isEmpty()) { | ||
| try { | ||
| URI uri = new URI((String) raw); | ||
| if (uri.getHost() != null) { | ||
| return uri; | ||
| } | ||
| } catch (URISyntaxException e) { | ||
| // Fall back to gridUri. | ||
| } | ||
| } | ||
| return gridUri; | ||
| } |
Contributor
There was a problem hiding this comment.
3. Remoteurl scheme not validated 🐞 Bug ≡ Correctness
LocalNode.resolvePublicGridUri will accept any se:remoteUrl URI that merely has a non-null host, even if it’s not an absolute http/https URL. This can produce incorrect ws/wss URL construction (and can break path handling) when the accepted URI is later consumed by rewrite().
Agent Prompt
### Issue description
`resolvePublicGridUri` currently accepts any `se:remoteUrl` string that parses as a `URI` with a non-null host. Because this URI is later used as the base for websocket URL generation, accepting scheme-less or non-http(s) URIs can lead to incorrect ws/wss selection and/or invalid path normalization.
### Issue Context
Downstream, `rewrite()` derives websocket scheme from `baseUri.getScheme()` and normalizes `baseUri.getPath()`.
### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[1303-1321]
### Suggested fix
- In `resolvePublicGridUri`, require an absolute URI with an explicit scheme of `http` or `https` (and a non-null host) before returning it.
- Optionally harden `rewrite()` to treat a null path as empty (defensive coding), but the key is to avoid returning invalid bases from `resolvePublicGridUri`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1577
to
+1581
| public Builder gridUrlSpecified(boolean configured) { | ||
| this.gridUrlSpecified = configured; | ||
| return this; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
4. Gridurlspecified footgun 🐞 Bug ⚙ Maintainability
LocalNode.Builder defaults gridUrlSpecified to false and requires callers to set it separately from the gridUri they pass. Any caller that supplies a configured/public gridUri but forgets to set gridUrlSpecified(true) will silently allow client se:remoteUrl to override what the caller intended to be authoritative.
Agent Prompt
### Issue description
The precedence decision in `resolvePublicGridUri` depends on `gridUrlSpecified`, but the builder API lets callers pass a `gridUri` while leaving `gridUrlSpecified` at its default `false`. This creates an easy-to-miss misconfiguration where an explicitly supplied public grid URI can be overridden by client-provided `se:remoteUrl`.
### Issue Context
`LocalNodeFactory` sets the flag correctly, but other builder call sites (tests/extensions) can pass a non-default `gridUri` without remembering to also set the boolean.
### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[1485-1505]
- java/src/org/openqa/selenium/grid/node/local/LocalNode.java[1577-1588]
### Suggested fix
Implement one of:
1) Make `gridUrlSpecified` derive from constructor inputs by default (e.g., initialize it in `Builder` based on whether `gridUri` differs from `uri`), while still allowing explicit override for the "equal URIs but configured" edge case.
2) Change the builder API to make the configured-vs-auto-detected nature explicit (e.g., introduce a dedicated `publicGridUri(URI configuredUri)` setter that both stores the URI and marks it configured, instead of a separate boolean).
ⓘ 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 9a20e5d |
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
2 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
Fixes #17782
Fixes #17788
💥 What does this PR do?
When a Grid is reached through Docker port-mapping or a reverse proxy, the BiDi/CDP/VNC URLs it hands back (
webSocketUrl,se:cdp,se:vnc) point at the Grid's own view of its address — e.g. a container-internalws://172.20.0.2:4444/...— which the client cannot reach, and causes attempted connections to error.The Grid currently addresses this with a
grid-urlconfiguration (--grid-url/SE_NODE_GRID_URL): when set to the externally-reachable address the Node uses it for the proxied URLs. Except:This PR has each bindiung advertise the url they actually reached the Grid on via a new
se:remoteUrlcapability, which the Node uses to build the proxied URLs. Additional connections to Docker then work with no extra configuration. Note that an explicitly configuredgrid-urlsetting still takes precedence.🔧 Implementation Notes
LocalNode.resolvePublicGridUri), so one server-side change makes the returned URLs reachable for every client; each binding only adds the small producer that sendsse:remoteUrl.resolvePublicGridUri: a configured public Grid URL (grid-url/ hub) is used as-is; otherwisese:remoteUrl; otherwise the Node's auto-detected address (previous behavior). So the capability only fills the gapgrid-urlleaves and never overrides operator intent.se:remoteUrlis not sent for local driver services (only for remote sessions), and when it is absent the behavior is unchanged for older clients.Host-header inference: the client is authoritative about its own reachable address, it is robust behind proxies, and it avoids changes to Grid routing.💡 Additional Considerations
https://user:key@host) are preserved in the resolved URL, identical to how an operator-configuredgrid-urlwith credentials already behaves. This keeps BiDi/CDP auth working against a basic-auth-protected Grid, where the returned websocket URL must carry the credentials. Under HTTPS these are protected in transit; the only residual is at-rest logging, consistent with the Grid already logging full capabilities today. Reducing that would be a separate, Grid-wide log-redaction change rather than mangling the URL the auth path depends on.ClientConfigto set local authentication and proxy behavior, in which case the connection URL carries no credentials andse:remoteUrlcarries none.🤖 AI assistance
🔄 Types of changes