Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.sun.net.httpserver.HttpExchange;
import com.sun.net.httpserver.HttpServer;
import io.appium.java_client.AppiumClientConfig;
import io.appium.java_client.android.AndroidDriver;
import io.appium.java_client.android.options.UiAutomator2Options;
import org.junit.jupiter.api.AfterEach;
Expand All @@ -37,6 +38,7 @@
import java.io.IOException;
import java.io.OutputStream;
import java.net.InetSocketAddress;
import java.net.URI;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.List;
Expand Down Expand Up @@ -116,7 +118,7 @@
}

private AndroidDriver newAppiumDriver() throws IOException {
return new AndroidDriver(new URL("http://127.0.0.1:" + server.getAddress().getPort()),

Check warning on line 121 in selenium-bridge/src/test/java/io/appium/java_client/selenium/SeleniumBridgeTest.java

View workflow job for this annotation

GitHub Actions / JDK 21 build (stable)

[deprecation] URL(String) in URL has been deprecated

Check warning on line 121 in selenium-bridge/src/test/java/io/appium/java_client/selenium/SeleniumBridgeTest.java

View workflow job for this annotation

GitHub Actions / JDK 21 build (snapshot)

[deprecation] URL(String) in URL has been deprecated

Check warning on line 121 in selenium-bridge/src/test/java/io/appium/java_client/selenium/SeleniumBridgeTest.java

View workflow job for this annotation

GitHub Actions / JDK 25 build (snapshot)

[deprecation] URL(String) in URL has been deprecated

Check warning on line 121 in selenium-bridge/src/test/java/io/appium/java_client/selenium/SeleniumBridgeTest.java

View workflow job for this annotation

GitHub Actions / JDK 25 build (stable)

[deprecation] URL(String) in URL has been deprecated
new UiAutomator2Options().setDeviceName("Android Emulator"));
}

Expand Down Expand Up @@ -261,4 +263,23 @@

assertThrows(BiDiException.class, seleniumDriver::getHandle);
}

@Test
void propagatesDirectConnectServerUrlToSeleniumClientConfig() throws IOException {
extraCapabilities = "\"appium:directConnectProtocol\":\"https\","
+ "\"appium:directConnectHost\":\"203.0.113.1\","
+ "\"appium:directConnectPort\":4443,"
+ "\"appium:directConnectPath\":\"/wd/hub\",";
var clientConfig = AppiumClientConfig.defaultConfig()
.baseUri(URI.create("http://127.0.0.1:" + server.getAddress().getPort()))
.directConnect(true);
var appiumDriver = new AndroidDriver(clientConfig,
new UiAutomator2Options().setDeviceName("Android Emulator"));

var seleniumDriver = SeleniumBridge.asRemoteWebDriver(appiumDriver);

assertEquals(URI.create("https://203.0.113.1:4443/wd/hub"),
seleniumDriver.getClientConfig().baseUri());
seleniumDriver.closeBiDi();
}
}
18 changes: 15 additions & 3 deletions src/main/java/io/appium/java_client/AppiumDriver.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import io.appium.java_client.remote.options.BaseOptions;
import io.appium.java_client.service.local.AppiumDriverLocalService;
import io.appium.java_client.service.local.AppiumServiceBuilder;
import lombok.Getter;
import org.jspecify.annotations.Nullable;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.OutputType;
Expand Down Expand Up @@ -64,8 +64,7 @@ public class AppiumDriver extends AppiumRemoteWebDriver implements

private static final ErrorHandler ERROR_HANDLER = new ErrorHandler(new ErrorCodesMobile());
// frequently used command parameters
@Getter
private final URL remoteAddress;
private final @Nullable URL remoteAddress;
private final ExecuteMethod executeMethod;

/**
Expand Down Expand Up @@ -221,6 +220,19 @@ public AppiumDriver(URL remoteSessionAddress, String platformName, String automa
setSessionId(sessionAddress.getId());
}

/**
* Gets the remote server address to which commands are sent.
*
* @return the remote server URL, or {@code null} if not configured
*/
@Nullable
public URL getRemoteAddress() {

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.

[nit] getRemoteAddress() now reads from the executor, so the remoteAddress field is only a fallback for a non-AppiumCommandExecutor. Both constructors only ever create or accept an AppiumCommandExecutor, so the field and the instanceof branch look dead. Dropping them would make it clear where the value comes from.

var executor = getCommandExecutor();
return executor instanceof AppiumCommandExecutor
? ((AppiumCommandExecutor) executor).getAddressOfRemoteServer()
: remoteAddress;
}

@Override
public ExecuteMethod getExecuteMethod() {
return executeMethod;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ public class AppiumCommandExecutor implements CommandExecutor, Closeable {

private final Optional<AppiumDriverLocalService> serviceOptional;
@Getter
private final AppiumClientConfig appiumClientConfig;
private final @Nullable URL remoteServer;
private volatile AppiumClientConfig appiumClientConfig;
private volatile @Nullable URL remoteServer;
private final Map<String, AppiumCommandInfo> additionalCommands;
@Getter
private final Factory httpClientFactory;
private HttpClient client;
private volatile HttpClient client;
private @Nullable CommandCodec commandCodec;
private @Nullable ResponseCodec responseCodec;

Expand All @@ -78,10 +78,14 @@ public AppiumCommandExecutor(
@Nullable Factory httpClientFactory,
AppiumClientConfig appiumClientConfig) {
this.additionalCommands = new HashMap<>(requireNonNull(additionalCommands, "Additional commands"));
this.appiumClientConfig = requireNonNull(appiumClientConfig, "HTTP client configuration");
AppiumClientConfig effectiveConfig = requireNonNull(appiumClientConfig, "HTTP client configuration");
if (service != null && effectiveConfig.baseUri() == null) {
effectiveConfig = effectiveConfig.baseUrl(service.getUrl());
}
this.appiumClientConfig = effectiveConfig;
this.httpClientFactory = ofNullable(httpClientFactory).orElseGet(Factory::createDefault);
this.client = this.httpClientFactory.createClient(appiumClientConfig);
this.remoteServer = appiumClientConfig.baseUrl();
this.client = this.httpClientFactory.createClient(effectiveConfig);
this.remoteServer = effectiveConfig.baseUrl();
this.serviceOptional = ofNullable(service);
}

Expand Down Expand Up @@ -168,7 +172,7 @@ public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands,
*/
public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands,
AppiumDriverLocalService service, AppiumClientConfig appiumClientConfig) {
this(additionalCommands, service, Factory.createDefault(), appiumClientConfig);
this(additionalCommands, requireNonNull(service), Factory.createDefault(), appiumClientConfig);

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.

[minor] Adding requireNonNull(service) here changes behavior. The delegated 4-arg constructor accepts a null service (it wraps it in ofNullable), so a caller passing null to this overload used to get a working executor and now gets an NPE. Is that break intended? If so, it's worth a note in the PR description. It isn't part of the directConnect fix.

}

/**
Expand Down Expand Up @@ -240,9 +244,12 @@ public HttpClient getClient() {
* and other non-special addresses are not rejected by this check.
*/
protected void overrideServerUrl(URL serverUrl) {
DirectConnectUrlSafety.requireSafeOverrideTarget(serverUrl);
HttpClient newClient = getHttpClientFactory().createClient(appiumClientConfig.baseUrl(serverUrl));
HttpClient oldClient = this.client;
DirectConnectUrlSafety.requireSafeOverrideTarget(requireNonNull(serverUrl, "Server URL"));

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.

[minor] appiumClientConfig, remoteServer and client are now three separate volatile writes. A thread calling getClient() or getAddressOfRemoteServer() during an override can see the new URL with the old client. volatile doesn't make the group atomic. remoteServer is also just appiumClientConfig.baseUrl() cached, which is the kind of duplicated state that caused this bug. Could getAddressOfRemoteServer() read appiumClientConfig.baseUrl() directly, and could the config and client sit in one immutable holder behind a single volatile reference (or this swap be synchronized)?

AppiumClientConfig updatedConfig = appiumClientConfig.baseUrl(serverUrl);
HttpClient newClient = getHttpClientFactory().createClient(updatedConfig);
final HttpClient oldClient = this.client;
this.appiumClientConfig = updatedConfig;
this.remoteServer = updatedConfig.baseUrl();
this.client = newClient;
oldClient.close();
}
Expand Down Expand Up @@ -292,7 +299,7 @@ private void setDirectConnect(Response response) throws SessionNotCreatedExcepti
return;
}

if (!directConnect.getProtocol().equals("https")) {
if (!"https".equalsIgnoreCase(directConnect.getProtocol())) {
throw new SessionNotCreatedException(
String.format("The given protocol '%s' as the direct connection url returned by "
+ "the remote server is not accurate. Only 'https' is supported.",
Expand Down
46 changes: 37 additions & 9 deletions src/main/java/io/appium/java_client/remote/DirectConnect.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Stream;

import static io.appium.java_client.internal.CapabilityHelpers.APPIUM_PREFIX;
Expand Down Expand Up @@ -65,11 +64,13 @@ private String getDirectConnectValue(Map<String, ?> responseValue, String key) {
}

/**
* Returns true if the {@link DirectConnect} instance member has nonnull values.
* @return true if each connection information has a nonnull value
* Returns true if the {@link DirectConnect} instance member has valid values.
* @return true if protocol, host and port are non-blank and path is non-null
*/
public boolean isValid() {
return Stream.of(this.protocol, this.path, this.host, this.port).noneMatch(Objects::isNull);
return this.path != null
&& Stream.of(this.protocol, this.host, this.port)
.allMatch(value -> value != null && !value.isBlank());
}

/**
Expand All @@ -78,15 +79,42 @@ public boolean isValid() {
* @throws MalformedURLException if the built url was invalid
*/
public URL getUrl() throws MalformedURLException {
String newUrlCandidate = String.format("%s://%s:%s%s", this.protocol, this.host, this.port, this.path);
String formattedHost = formatHost(this.host);

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.

[nit] getUrl() formats the host and path before checking isValid(), only so the error message can include the candidate. That forces formatHost and normalizePath to handle null, and normalizePath returns the string "null". Checking isValid() first and formatting after would remove those placeholder branches.

String normalizedPath = normalizePath(this.path);
String newUrlCandidate = String.format("%s://%s:%s%s",
this.protocol, formattedHost, this.port, normalizedPath);
if (!isValid()) {
throw createMalformedUrlException(newUrlCandidate);
}

try {
return new URL(newUrlCandidate);
} catch (MalformedURLException e) {
throw new MalformedURLException(
String.format("The remote server returned an invalid value to build the direct connect URL: %s",
newUrlCandidate)
);
throw createMalformedUrlException(newUrlCandidate);
}
}

private static String formatHost(@Nullable String host) {
if (host != null && !host.startsWith("[") && host.indexOf(':') != host.lastIndexOf(':')) {
return "[" + host + "]";
}
return String.valueOf(host);
}

private static String normalizePath(@Nullable String path) {
if (path == null) {
return "null";
}
if (path.isEmpty() || path.startsWith("/")) {
return path;
}
return "/" + path;
}

private static MalformedURLException createMalformedUrlException(String candidate) {
return new MalformedURLException(
String.format("The remote server returned an invalid value to build the direct connect URL: %s",
candidate)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,71 @@ void hasInvalidDirectConnect() {
Map<String, String> responseValue = new HashMap<>();
DirectConnect directConnect = new DirectConnect(responseValue);
assertFalse(directConnect.isValid());
assertThrowsExactly(MalformedURLException.class, directConnect::getUrl);
}

@Test
void formatsBareIpv6HostAndPreservesBracketedIpv6Host() throws MalformedURLException {
Map<String, String> bareIpv6 = Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", "2001:db8::1",
"appium:directConnectPort", "4443",
"appium:directConnectPath", "/wd/hub"
);
DirectConnect bare = new DirectConnect(bareIpv6);
assertTrue(bare.isValid());
assertEquals("https://[2001:db8::1]:4443/wd/hub", bare.getUrl().toString());

Map<String, String> bracketedIpv6 = Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", "[2001:db8::1]",
"appium:directConnectPort", "4443",
"appium:directConnectPath", "/wd/hub"
);
DirectConnect bracketed = new DirectConnect(bracketedIpv6);
assertTrue(bracketed.isValid());
assertEquals("https://[2001:db8::1]:4443/wd/hub", bracketed.getUrl().toString());
}

@Test
void normalizesEmptyAndRelativeDirectConnectPath() throws MalformedURLException {
DirectConnect emptyPath = new DirectConnect(Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", "203.0.113.1",
"appium:directConnectPort", "4443",
"appium:directConnectPath", ""
));
assertTrue(emptyPath.isValid());
assertEquals("https://203.0.113.1:4443", emptyPath.getUrl().toString());

DirectConnect relativePath = new DirectConnect(Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", "203.0.113.1",
"appium:directConnectPort", "4443",
"appium:directConnectPath", "wd/hub"
));
assertTrue(relativePath.isValid());
assertEquals("https://203.0.113.1:4443/wd/hub", relativePath.getUrl().toString());
}

@Test
void rejectsSingleColonHostAndBlankFields() {
DirectConnect embeddedPortHost = new DirectConnect(Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", "evil.example.com:8443",
"appium:directConnectPort", "4443",
"appium:directConnectPath", "/wd/hub"
));
assertTrue(embeddedPortHost.isValid());
assertThrowsExactly(MalformedURLException.class, embeddedPortHost::getUrl);

DirectConnect blankHost = new DirectConnect(Map.of(
"appium:directConnectProtocol", "https",
"appium:directConnectHost", " ",
"appium:directConnectPort", "4443",
"appium:directConnectPath", ""
));
assertFalse(blankHost.isValid());
assertThrowsExactly(MalformedURLException.class, blankHost::getUrl);
}
}
Loading
Loading