diff --git a/java/src/org/openqa/selenium/bidi/BiDiProvider.java b/java/src/org/openqa/selenium/bidi/BiDiProvider.java index a43046a1e13e7..3dc25de87d781 100644 --- a/java/src/org/openqa/selenium/bidi/BiDiProvider.java +++ b/java/src/org/openqa/selenium/bidi/BiDiProvider.java @@ -93,7 +93,7 @@ private Optional getBiDiUrl(Capabilities caps) { try { return new URI(uri); } catch (URISyntaxException e) { - return null; + throw new BiDiException("Invalid BiDi URL: " + uri, e); } }); } diff --git a/java/src/org/openqa/selenium/bidi/Connection.java b/java/src/org/openqa/selenium/bidi/Connection.java index 9b667eee627dc..cc684d6af03e4 100644 --- a/java/src/org/openqa/selenium/bidi/Connection.java +++ b/java/src/org/openqa/selenium/bidi/Connection.java @@ -276,14 +276,18 @@ public void onText(CharSequence data) { try { handle(data); } catch (Exception e) { - throw new BiDiException("Unable to process: " + data, e); + throw new BiDiException("Unable to process BiDi response: " + data, e); } }); } @Override public void onClose(int code, String reason) { - LOG.fine("BiDi connection websocket closed"); + LOG.log( + Level.FINE, + () -> + String.format( + "BiDi connection websocket closed (code: %s, reason: \"%s\")", code, reason)); underlyingSocketClosed.set(true); } } @@ -303,7 +307,7 @@ private void handle(CharSequence data) { } else if (raw.get("method") instanceof String && raw.get("params") instanceof Map) { handleEventResponse(raw); } else { - LOG.warning(() -> "Unhandled type:" + data); + LOG.warning(() -> "Unhandled type BiDi response type: " + data); } } @@ -340,11 +344,9 @@ private void handleEventResponse(Map rawDataMap) { LOG.log( getDebugLogLevel(), () -> - "Method" - + rawDataMap.get("method") - + "called with" - + eventCallbacks.size() - + "callbacks available"); + String.format( + "Method %s called with %s callbacks available", + rawDataMap.get("method"), eventCallbacks.size())); Lock lock = callbacksLock.readLock(); // A waiting writer will block a reader to enter the lock, even if there are currently other // readers holding the lock. TryLock will bypass the waiting writers and acquire the read lock. diff --git a/java/src/org/openqa/selenium/netty/server/WebSocketMessageHandler.java b/java/src/org/openqa/selenium/netty/server/WebSocketMessageHandler.java index 709b8ce1c005e..7dd969dd7c25b 100644 --- a/java/src/org/openqa/selenium/netty/server/WebSocketMessageHandler.java +++ b/java/src/org/openqa/selenium/netty/server/WebSocketMessageHandler.java @@ -21,11 +21,14 @@ import io.netty.channel.SimpleChannelInboundHandler; import io.netty.util.AttributeKey; import java.util.function.Consumer; +import java.util.logging.Level; +import java.util.logging.Logger; import org.openqa.selenium.internal.Require; +import org.openqa.selenium.remote.http.CloseMessage; import org.openqa.selenium.remote.http.Message; class WebSocketMessageHandler extends SimpleChannelInboundHandler { - + private static final Logger LOG = Logger.getLogger(WebSocketMessageHandler.class.getName()); private final AttributeKey> key; public WebSocketMessageHandler(AttributeKey> key) { @@ -39,6 +42,10 @@ protected void channelRead0(ChannelHandlerContext ctx, Message msg) { } Consumer handler = ctx.channel().attr(key).get(); + if (handler == null && msg instanceof CloseMessage && ((CloseMessage) msg).code() == -1) { + LOG.log(Level.FINE, "Failed to handle websocket message \"{0}\" - handler is null", msg); + return; + } ctx.executor() .execute( diff --git a/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java b/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java index cd4c841de0404..d4917da5a3859 100644 --- a/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java +++ b/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java @@ -233,10 +233,11 @@ public void channelInactive(ChannelHandlerContext ctx) throws Exception { Consumer consumer = ctx.channel().attr(key).getAndSet(null); if (consumer != null) { + CloseMessage channelGotInactive = new CloseMessage(1001, "channel got inactive"); try { - consumer.accept(new CloseMessage(1001, "channel got inactive")); - } catch (Exception ex) { - LOG.log(Level.FINE, "failed to send the close message, code: 1001", ex); + consumer.accept(channelGotInactive); + } catch (RuntimeException ex) { + LOG.log(Level.FINE, ex, () -> "failed to send " + channelGotInactive); } } } diff --git a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java index a2c2f3a12a9e5..c7e9a3559322b 100644 --- a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java +++ b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java @@ -93,6 +93,7 @@ import org.openqa.selenium.remote.http.ConnectionFailedException; import org.openqa.selenium.remote.http.Contents; import org.openqa.selenium.remote.http.HttpClient; +import org.openqa.selenium.remote.http.jdk.ConnectionException; import org.openqa.selenium.remote.service.DriverCommandExecutor; import org.openqa.selenium.remote.tracing.TracedHttpClient; import org.openqa.selenium.remote.tracing.Tracer; @@ -628,6 +629,11 @@ protected Response execute(CommandPayload payload) { } } else if (e instanceof WebDriverException) { toThrow = (WebDriverException) e; + } else if (e instanceof ConnectionException) { + ConnectionException cause = (ConnectionException) e; + toThrow = + new UnreachableBrowserException( + "Error communicating with the remote browser at " + cause.uri(), cause); } else { toThrow = new UnreachableBrowserException( diff --git a/java/src/org/openqa/selenium/remote/http/CloseMessage.java b/java/src/org/openqa/selenium/remote/http/CloseMessage.java index 56487d9ed8d53..01c86c4719f64 100644 --- a/java/src/org/openqa/selenium/remote/http/CloseMessage.java +++ b/java/src/org/openqa/selenium/remote/http/CloseMessage.java @@ -38,4 +38,9 @@ public int code() { public String reason() { return reason; } + + @Override + public String toString() { + return String.format("%s{code=%d, reason=%s}", getClass().getSimpleName(), code, reason); + } } diff --git a/java/src/org/openqa/selenium/remote/http/jdk/ConnectionException.java b/java/src/org/openqa/selenium/remote/http/jdk/ConnectionException.java new file mode 100644 index 0000000000000..767d41991b574 --- /dev/null +++ b/java/src/org/openqa/selenium/remote/http/jdk/ConnectionException.java @@ -0,0 +1,34 @@ +// Licensed to the Software Freedom Conservancy (SFC) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The SFC licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.openqa.selenium.remote.http.jdk; + +import java.io.UncheckedIOException; +import java.net.ConnectException; + +public class ConnectionException extends UncheckedIOException { + private final String uri; + + public ConnectionException(String message, String uri, ConnectException cause) { + super(message, cause); + this.uri = uri; + } + + public String uri() { + return uri; + } +} diff --git a/java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java b/java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java index 4f6a24df553b8..a9120c69328bc 100644 --- a/java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java +++ b/java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java @@ -23,6 +23,7 @@ import java.io.InputStream; import java.io.UncheckedIOException; import java.net.Authenticator; +import java.net.ConnectException; import java.net.PasswordAuthentication; import java.net.ProtocolException; import java.net.Proxy; @@ -443,18 +444,18 @@ private HttpResponse execute0(HttpRequest req) throws UncheckedIOException { long start = System.currentTimeMillis(); BodyHandler responseBodyHandler = BodyHandlers.ofInputStream(); + URI rawUri = messages.getRawUri(req); + HttpMethod method = req.getMethod(); try { - HttpMethod method = req.getMethod(); - URI rawUri = messages.getRawUri(req); - // We need a custom handling of redirects to: // - increase the maximum number of retries to 100 // - avoid a downgrade of POST requests, see the javadoc of j.n.h.HttpClient.Redirect // - not run into https://bugs.openjdk.org/browse/JDK-8304701 for (int i = 0; i < 100; i++) { if (Thread.interrupted()) { - throw new InterruptedException("http request has been interrupted"); + throw new InterruptedException( + String.format("http request has been interrupted: %s", describe(req))); } java.net.http.HttpRequest request = messages.createRequest(req, method, rawUri); @@ -469,26 +470,8 @@ private HttpResponse execute0(HttpRequest req) throws UncheckedIOException { case 302: case 307: case 308: - URI location = - rawUri.resolve( - response - .headers() - .firstValue("location") - .orElseThrow( - () -> - new ProtocolException( - "HTTP " - + response.statusCode() - + " without 'location' header set"))); - - if ("https".equalsIgnoreCase(rawUri.getScheme()) - && !"https".equalsIgnoreCase(location.getScheme())) { - throw new SecurityException("Downgrade from secure to insecure connection."); - } else if ("wss".equalsIgnoreCase(rawUri.getScheme()) - && !"wss".equalsIgnoreCase(location.getScheme())) { - throw new SecurityException("Downgrade from secure to insecure connection."); - } - + URI location = rawUri.resolve(getLocationHeader(response, method, rawUri)); + checkNotDowngrade(rawUri, location); rawUri = location; continue; default: @@ -496,14 +479,20 @@ private HttpResponse execute0(HttpRequest req) throws UncheckedIOException { } } - throw new ProtocolException("Too many redirects: 101"); + throw new ProtocolException(String.format("Too many redirects: 101 (%s)", describe(req))); } catch (HttpTimeoutException e) { - throw new TimeoutException(e); + throw new TimeoutException( + String.format("Timeout when executing request (%s)", describe(req)), e); + } catch (ConnectException e) { + throw new ConnectionException( + String.format("Connection error (%s)", describe(req)), maskUrlCredentials(rawUri), e); } catch (IOException e) { - throw new UncheckedIOException(e); + throw new UncheckedIOException( + String.format("Failed to execute request (%s)", describe(req)), e); } catch (InterruptedException e) { Thread.currentThread().interrupt(); - throw new WebDriverException(e.getMessage(), e); + throw new WebDriverException( + String.format("%s when executing request (%s)", e.getMessage(), describe(req)), e); } finally { LOG.log( Level.FINE, @@ -512,6 +501,67 @@ private HttpResponse execute0(HttpRequest req) throws UncheckedIOException { } } + private void checkNotDowngrade(URI from, URI to) { + if (isDowngradeFrom("https", from, to) || isDowngradeFrom("wss", from, to)) { + throw new SecurityException( + String.format("Downgrade from secure to insecure connection (%s -> %s)", from, to)); + } + } + + private boolean isDowngradeFrom(String protocol, URI from, URI to) { + return protocol.equalsIgnoreCase(from.getScheme()) + && !protocol.equalsIgnoreCase(to.getScheme()); + } + + private String describe(HttpRequest req) { + String uri = maskUrlCredentials(messages.getRawUri(req)); + HttpMethod method = req.getMethod(); + return String.format("%s %s", method, uri); + } + + static String maskUrlCredentials(String uri) { + try { + return maskUrlCredentials(new URI(uri)); + } catch (URISyntaxException invalidUri) { + return uri; + } + } + + private static String maskUrlCredentials(URI u) { + if (u.getUserInfo() == null) { + return u.toString(); + } + try { + return new URI( + u.getScheme(), + "***", + u.getHost(), + u.getPort(), + u.getPath(), + u.getQuery(), + u.getFragment()) + .toString(); + } catch (URISyntaxException e) { + return u.toString(); + } + } + + private String getLocationHeader( + java.net.http.HttpResponse response, HttpMethod method, URI uri) + throws ProtocolException { + return response + .headers() + .firstValue("location") + .orElseThrow( + () -> { + String message = + String.format( + "HTTP response with status %d but without \"location\" header (%s %s)", + response.statusCode(), method, maskUrlCredentials(uri)); + return new ProtocolException(message); + }); + } + @Override public CompletableFuture> sendAsyncNative( java.net.http.HttpRequest request, java.net.http.HttpResponse.BodyHandler handler) { diff --git a/java/test/logging.properties b/java/test/logging.properties new file mode 100644 index 0000000000000..fe7ba8aee5c4e --- /dev/null +++ b/java/test/logging.properties @@ -0,0 +1,6 @@ +handlers=java.util.logging.ConsoleHandler +java.util.logging.ConsoleHandler.level = FINE +.level=INFO + +#Uncomment to enable DEBUG logs for some specific package: +#org.openqa.selenium.bidi.level=FINE diff --git a/java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java b/java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java index 2c7703684067f..fe507998dfbab 100644 --- a/java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java +++ b/java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java @@ -17,14 +17,18 @@ package org.openqa.selenium.grid.router; +import static java.util.Objects.requireNonNull; +import static java.util.logging.Level.FINE; +import static java.util.logging.Level.INFO; import static org.assertj.core.api.Assertions.assertThat; import java.io.StringReader; -import java.util.Objects; +import java.net.URL; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.logging.Logger; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -49,12 +53,15 @@ import org.openqa.selenium.testing.drivers.Browser; class RemoteWebDriverBiDiTest extends JupiterTestBase { + private static final Logger LOG = Logger.getLogger(RemoteWebDriverBiDiTest.class.getName()); private Deployment deployment; + private URL remoteUrl; @BeforeEach void setup() { - Browser browser = Objects.requireNonNull(Browser.detect()); + Browser browser = requireNonNull(Browser.detect()); + LOG.log(FINE, () -> String.format("Starting grid server for %s...", browser)); deployment = DeploymentTypes.STANDALONE.start( @@ -65,23 +72,29 @@ void setup() { + "selenium-manager = false\n" + "driver-implementation = " + String.format("\"%s\"", browser.displayName())))); - } + remoteUrl = deployment.getServer().getUrl(); + LOG.log(INFO, () -> String.format("Started grid server for %s: %s", browser, remoteUrl)); - private void createDriver() { - Browser browser = Objects.requireNonNull(Browser.detect()); - localDriver = new RemoteWebDriver(deployment.getServer().getUrl(), browser.getCapabilities()); + localDriver = new RemoteWebDriver(remoteUrl, browser.getCapabilities()); localDriver = new Augmenter().augment(localDriver); } @AfterEach void tearDownDeployment() { - Safely.safelyCall(() -> deployment.tearDown()); + if (localDriver != null) { + localDriver.quit(); + } + + if (deployment != null) { + LOG.log(FINE, () -> String.format("Stopping grid server %s ...", remoteUrl)); + Safely.safelyCall(() -> deployment.tearDown()); + LOG.log(INFO, () -> String.format("Stopped grid server %s", remoteUrl)); + } } @Test @NoDriverBeforeTest void ensureBiDiSessionCreation() { - createDriver(); try (BiDi biDi = ((HasBiDi) localDriver).getBiDi()) { BiDiSessionStatus status = biDi.getBidiSessionStatus(); assertThat(status).isNotNull(); @@ -92,7 +105,6 @@ void ensureBiDiSessionCreation() { @Test @NoDriverBeforeTest void canListenToLogs() throws ExecutionException, InterruptedException, TimeoutException { - createDriver(); try (LogInspector logInspector = new LogInspector(localDriver)) { CompletableFuture future = new CompletableFuture<>(); logInspector.onConsoleEntry(future::complete); @@ -117,7 +129,6 @@ void canListenToLogs() throws ExecutionException, InterruptedException, TimeoutE @Test @NoDriverBeforeTest void canNavigateToUrl() { - createDriver(); BrowsingContext browsingContext = new BrowsingContext(localDriver, WindowType.TAB); String url = appServer.whereIs("/bidi/logEntryAdded.html"); diff --git a/java/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java b/java/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java index f6b5489a1910f..9c85840c167f3 100644 --- a/java/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java +++ b/java/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java @@ -17,6 +17,10 @@ package org.openqa.selenium.remote.http.jdk; +import static org.assertj.core.api.Assertions.assertThat; +import static org.openqa.selenium.remote.http.jdk.JdkHttpClient.maskUrlCredentials; + +import org.junit.jupiter.api.Test; import org.openqa.selenium.remote.http.HttpClient; import org.openqa.selenium.remote.internal.HttpClientTestBase; @@ -26,4 +30,23 @@ class JdkHttpClientTest extends HttpClientTestBase { protected HttpClient.Factory createFactory() { return new JdkHttpClient.Factory(); } + + @Test + void maskUrlCredentials_hidesUserInfoInUrl() { + assertThat(maskUrlCredentials("http://user:pass@my.grid.com:4444/wd/hub")) + .isEqualTo("http://***@my.grid.com:4444/wd/hub"); + assertThat(maskUrlCredentials("https://vi:se@myshop.com/wd/hub?email=foo@bar.ee")) + .isEqualTo("https://***@myshop.com/wd/hub?email=foo@bar.ee"); + assertThat(maskUrlCredentials("https://0:@myshop.com/wd/hub")) + .isEqualTo("https://***@myshop.com/wd/hub"); + } + + @Test + void maskUrlCredentials_withoutUserInfo() { + assertThat(maskUrlCredentials("http://localhost:49112/wd/hub")) + .isEqualTo("http://localhost:49112/wd/hub"); + assertThat(maskUrlCredentials("https://localhost/wd/hub")) + .isEqualTo("https://localhost/wd/hub"); + assertThat(maskUrlCredentials("")).isEqualTo(""); + } }