Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 5 additions & 0 deletions dotnet/src/webdriver/Remote/HttpCommandExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ public HttpCommandExecutor(Uri addressOfRemoteServer, TimeSpan timeout, bool ena
/// </summary>
public string UserAgent { get; set; }

/// <summary>
/// Gets the address of the remote end this executor connects to.
/// </summary>
internal Uri RemoteServerUri => this.remoteServerUri;

/// <summary>
/// Gets the repository of objects containing information about commands.
/// </summary>
Expand Down
5 changes: 5 additions & 0 deletions dotnet/src/webdriver/WebDriver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,11 @@ protected void StartSession(ICapabilities capabilities)
{
Dictionary<string, object> matchCapabilities = this.GetCapabilitiesDictionary(capabilities);

if (this.CommandExecutor is Remote.HttpCommandExecutor httpExecutor)
{
matchCapabilities["se:remoteUrl"] = httpExecutor.RemoteServerUri.AbsoluteUri;
}

List<object> firstMatchCapabilitiesList = new List<object>();
firstMatchCapabilitiesList.Add(matchCapabilities);

Expand Down
46 changes: 39 additions & 7 deletions java/src/org/openqa/selenium/grid/node/local/LocalNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public class LocalNode extends Node implements Closeable {
private final EventBus bus;
private final URI externalUri;
private final URI gridUri;
private final boolean gridUrlSpecified;
private final Duration heartbeatPeriod;
private final HealthCheck healthCheck;
private final int maxSessionCount;
Expand Down Expand Up @@ -175,6 +176,7 @@ protected LocalNode(
EventBus bus,
URI uri,
URI gridUri,
boolean gridUrlSpecified,
@Nullable HealthCheck healthCheck,
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
int maxSessionCount,
int drainAfterSessionCount,
Expand All @@ -200,6 +202,7 @@ protected LocalNode(

this.externalUri = Require.nonNull("Remote node URI", uri);
this.gridUri = Require.nonNull("Grid URI", gridUri);
this.gridUrlSpecified = gridUrlSpecified;
this.maxSessionCount =
Math.min(Require.positive("Max session count", maxSessionCount), factories.size());
this.heartbeatPeriod = heartbeatPeriod;
Expand Down Expand Up @@ -1221,10 +1224,12 @@ private Session createExternalSession(
Capabilities toUse =
ImmutableCapabilities.copyOf(requestCapabilities.merge(other.getCapabilities()));

URI baseUri = resolvePublicGridUri(toUse);

// Add se:cdp if necessary to send the cdp url back
if ((isSupportingCdp || toUse.getCapability("se:cdp") != null) && cdpEnabled) {
String cdpPath = String.format("/session/%s/se/cdp", other.getId());
toUse = new PersistentCapabilities(toUse).setCapability("se:cdp", rewrite(cdpPath));
toUse = new PersistentCapabilities(toUse).setCapability("se:cdp", rewrite(cdpPath, baseUri));
} else {
// Remove any se:cdp* from the response, CDP is not supported nor enabled
MutableCapabilities cdpFiltered = new MutableCapabilities();
Expand Down Expand Up @@ -1259,7 +1264,7 @@ private Session createExternalSession(
toUse =
new PersistentCapabilities(toUse)
.setCapability("se:gridWebSocketUrl", uri)
.setCapability("webSocketUrl", rewrite(bidiPath));
.setCapability("webSocketUrl", rewrite(bidiPath, baseUri));
} else {
// Remove any "webSocketUrl" from the response, BiDi is not supported nor enabled
MutableCapabilities bidiFiltered = new MutableCapabilities();
Expand All @@ -1278,23 +1283,43 @@ private Session createExternalSession(
boolean isVncEnabled = toUse.getCapability("se:vncLocalAddress") != null;
if (isVncEnabled) {
String vncPath = String.format("/session/%s/se/vnc", other.getId());
toUse = new PersistentCapabilities(toUse).setCapability("se:vnc", rewrite(vncPath));
toUse = new PersistentCapabilities(toUse).setCapability("se:vnc", rewrite(vncPath, baseUri));
}

return new Session(other.getId(), externalUri, other.getStereotype(), toUse, Instant.now());
}

private URI rewrite(String path) {
private URI rewrite(String path, URI baseUri) {
try {
String scheme = "https".equals(gridUri.getScheme()) ? "wss" : "ws";
path = NodeOptions.normalizeSubPath(gridUri.getPath()) + path;
String scheme = "https".equals(baseUri.getScheme()) ? "wss" : "ws";
path = NodeOptions.normalizeSubPath(baseUri.getPath()) + path;
return new URI(
scheme, gridUri.getUserInfo(), gridUri.getHost(), gridUri.getPort(), path, null, null);
scheme, baseUri.getUserInfo(), baseUri.getHost(), baseUri.getPort(), path, null, null);
} catch (URISyntaxException e) {
throw new RuntimeException(e);
}
}

// 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;
}
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

@Override
public NodeStatus getStatus() {
Set<Slot> slots =
Expand Down Expand Up @@ -1457,6 +1482,7 @@ public static class Builder {
private final Secret registrationSecret;
private final List<SessionSlot> factories;
private final List<NodeCommandInterceptor> interceptors = new ArrayList<>();
private boolean gridUrlSpecified = false;
private int maxSessions = NodeOptions.DEFAULT_MAX_SESSIONS;
private int drainAfterSessionCount = NodeOptions.DEFAULT_DRAIN_AFTER_SESSION_COUNT;
private boolean cdpEnabled = NodeOptions.DEFAULT_ENABLE_CDP;
Expand Down Expand Up @@ -1548,12 +1574,18 @@ public Builder addInterceptor(NodeCommandInterceptor interceptor) {
return this;
}

public Builder gridUrlSpecified(boolean configured) {
this.gridUrlSpecified = configured;
return this;
}
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

Comment on lines +1579 to +1583

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.

Remediation recommended

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The only non-test caller of LocalNode.builder is LocalNodeFactory, and it derives both values from the same source in the same place

Additionally forgetting the flag only matters if a session request also carries se:remoteUrl

public LocalNode build() {
return new LocalNode(
tracer,
bus,
uri,
gridUri,
gridUrlSpecified,
healthCheck,
maxSessions,
drainAfterSessionCount,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public static Node create(Config config) {
serverOptions.getExternalUri(),
nodeOptions.getPublicGridUri().orElseGet(serverOptions::getExternalUri),
secretOptions.getRegistrationSecret())
.gridUrlSpecified(nodeOptions.getPublicGridUri().isPresent())
.maximumConcurrentSessions(nodeOptions.getMaxSessions())
.sessionTimeout(sessionTimeout)
.drainAfterSessionCount(nodeOptions.getDrainAfterSessionCount())
Expand Down
11 changes: 11 additions & 0 deletions java/src/org/openqa/selenium/remote/RemoteWebDriver.java
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,20 @@ protected void setSessionId(String opaqueKey) {
sessionId = new SessionId(opaqueKey);
}

private Capabilities addRemoteUrl(Capabilities capabilities) {
URI baseUri = clientConfig.baseUri();
if (baseUri == null) {
return capabilities;
}
MutableCapabilities withRemoteUrl = new MutableCapabilities(capabilities);
withRemoteUrl.setCapability("se:remoteUrl", baseUri.toString());
return withRemoteUrl;
}

protected void startSession(Capabilities capabilities) {
checkNonW3CCapabilities(capabilities);
checkChromeW3CFalse(capabilities);
capabilities = addRemoteUrl(capabilities);

Comment thread
titusfortner marked this conversation as resolved.
try {
Response response = execute(DriverCommand.NEW_SESSION(singleton(capabilities)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,14 @@ private Set<String> getClobberedCapabilities() {
private NewSessionPayload getPayload() {
Map<String, Object> roughPayload = new TreeMap<>(metadata);

Map<String, Object> alwaysMatch = new TreeMap<>(additionalCapabilities);
URI baseUri = getBaseUri();
if (baseUri != null) {
alwaysMatch.put("se:remoteUrl", baseUri.toString());
}

Map<String, Object> w3cCaps = new TreeMap<>();
w3cCaps.put("alwaysMatch", additionalCapabilities);
w3cCaps.put("alwaysMatch", alwaysMatch);
if (!requestedCapabilities.isEmpty()) {
w3cCaps.put("firstMatch", requestedCapabilities);
}
Expand Down
171 changes: 171 additions & 0 deletions java/test/org/openqa/selenium/grid/node/NodeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,177 @@ void shouldCreateASessionIfTheCorrectCapabilitiesArePassedToIt() {
assertThat(sessionResponse.getSession()).isNotNull();
}

@Test
void usesClientReachableAddressForProxiedUrlsWhenRemoteUrlProvided() {
Capabilities request =
new ImmutableCapabilities(
"browserName",
"cheese",
"se:vncLocalAddress",
"localhost:5900",
"se:remoteUrl",
"http://localhost:9999");

Either<WebDriverException, CreateSessionResponse> response =
local.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://localhost:9999/session/" + session.getId() + "/se/vnc");
}

@Test
void preservesCredentialsFromClientAdvertisedRemoteUrl() {
Capabilities request =
new ImmutableCapabilities(
"browserName",
"cheese",
"se:vncLocalAddress",
"localhost:5900",
"se:remoteUrl",
"http://user:secret@localhost:9999");

Either<WebDriverException, CreateSessionResponse> response =
local.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://user:secret@localhost:9999/session/" + session.getId() + "/se/vnc");
}

@Test
void ignoresRemoteUrlWhenPublicGridUrlIsConfigured() throws URISyntaxException {
URI configuredGridUri = new URI("http://grid.example:4444");

class Handler extends Session implements HttpHandler {
private Handler(Capabilities capabilities) {
super(new SessionId(UUID.randomUUID()), uri, stereotype, capabilities, Instant.now());
}

@Override
public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
return new HttpResponse();
}
}

LocalNode node =
LocalNode.builder(tracer, bus, uri, configuredGridUri, registrationSecret)
.gridUrlSpecified(true)
.add(caps, new TestSessionFactory((id, c) -> new Handler(c)))
.build();

Capabilities request =
new ImmutableCapabilities(
"browserName",
"cheese",
"se:vncLocalAddress",
"localhost:5900",
"se:remoteUrl",
"http://localhost:9999");

Either<WebDriverException, CreateSessionResponse> response =
node.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://grid.example:4444/session/" + session.getId() + "/se/vnc");
}

@Test
void preservesCredentialsFromConfiguredPublicGridUrl() throws URISyntaxException {
URI configuredGridUri = new URI("http://user:secret@grid.example:4444");

class Handler extends Session implements HttpHandler {
private Handler(Capabilities capabilities) {
super(new SessionId(UUID.randomUUID()), uri, stereotype, capabilities, Instant.now());
}

@Override
public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
return new HttpResponse();
}
}

LocalNode node =
LocalNode.builder(tracer, bus, uri, configuredGridUri, registrationSecret)
.gridUrlSpecified(true)
.add(caps, new TestSessionFactory((id, c) -> new Handler(c)))
.build();

Capabilities request =
new ImmutableCapabilities(
"browserName",
"cheese",
"se:vncLocalAddress",
"localhost:5900",
"se:remoteUrl",
"http://localhost:9999");

Either<WebDriverException, CreateSessionResponse> response =
node.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://user:secret@grid.example:4444/session/" + session.getId() + "/se/vnc");
}

@Test
void ignoresRemoteUrlWhenGridUrlIsConfiguredEvenIfItEqualsNodeAddress() {
// Edge case: grid-url is explicitly configured to the same value as the node's own externalUri.
// The explicit configuration must still win over a client-advertised se:remoteUrl.
class Handler extends Session implements HttpHandler {
private Handler(Capabilities capabilities) {
super(new SessionId(UUID.randomUUID()), uri, stereotype, capabilities, Instant.now());
}

@Override
public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
return new HttpResponse();
}
}

LocalNode node =
LocalNode.builder(tracer, bus, uri, uri, registrationSecret)
.gridUrlSpecified(true)
.add(caps, new TestSessionFactory((id, c) -> new Handler(c)))
.build();

Capabilities request =
new ImmutableCapabilities(
"browserName",
"cheese",
"se:vncLocalAddress",
"localhost:5900",
"se:remoteUrl",
"http://localhost:9999");

Either<WebDriverException, CreateSessionResponse> response =
node.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://localhost:1234/session/" + session.getId() + "/se/vnc");
}

@Test
void fallsBackToGridAddressForProxiedUrlsWithoutRemoteUrl() {
Capabilities request =
new ImmutableCapabilities("browserName", "cheese", "se:vncLocalAddress", "localhost:5900");

Either<WebDriverException, CreateSessionResponse> response =
local.newSession(createSessionRequest(request));
assertThatEither(response).isRight();

Session session = response.right().getSession();
assertThat(String.valueOf(session.getCapabilities().getCapability("se:vnc")))
.isEqualTo("ws://localhost:1234/session/" + session.getId() + "/se/vnc");
}

@Test
void shouldRetryIfNoMatchingSlotIsAvailable() {
Node local =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,24 @@ void shouldAllowMetaDataToBeSet() {
assertThat(seen).isTrue();
}

@Test
void advertisesRemoteUrlToTheServer() {
AtomicReference<Object> seen = new AtomicReference<>();

RemoteWebDriver.builder()
.oneOf(new FirefoxOptions())
.address("http://localhost:34576")
.connectingWith(
config ->
req -> {
seen.set(listCapabilities(req).get(0).getCapability("se:remoteUrl"));
return CANNED_SESSION_RESPONSE;
})
.build();

assertThat(seen.get()).isEqualTo("http://localhost:34576");
}

@Test
void doesNotAllowFirstMatchToBeUsedAsAMetadataNameAsItIsConfusing() {
RemoteWebDriverBuilder builder = RemoteWebDriver.builder();
Expand Down
Loading
Loading