UnreachableBrowserException logs the command parameter details only i…

…n debug mode (#11328) * UnreachableBrowserException logs the command parameter details only in debug mode * [java] Refining PR --------- Co-authored-by: Diego Molina <diemol@gmail.com> Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
SeleniumHQ · Jul 24, 2023 · 9ff4c2b · 9ff4c2b
1 parent bc3b548
commit 9ff4c2b
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 24 deletions.
diff --git a/java/maven_deps.bzl b/java/maven_deps.bzl
@@ -95,6 +95,7 @@ def selenium_java_deps():
             "org.hamcrest:hamcrest:2.2",
             "org.hsqldb:hsqldb:2.7.1",
             "org.mockito:mockito-core:4.11.0",
+            "org.mockito:mockito-inline:4.11.0",
             "org.slf4j:slf4j-api:2.0.7",
             "org.slf4j:slf4j-jdk14:2.0.7",
             "org.apache.logging.log4j:log4j-core:2.20.0",

diff --git a/java/maven_install.json b/java/maven_install.json
@@ -1,7 +1,7 @@
 {
   "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL",
-  "__INPUT_ARTIFACTS_HASH": 1835346973,
-  "__RESOLVED_ARTIFACTS_HASH": 831155937,
+  "__INPUT_ARTIFACTS_HASH": 1647168181,
+  "__RESOLVED_ARTIFACTS_HASH": -409306628,
   "artifacts": {
     "com.beust:jcommander": {
       "shasums": {
@@ -902,6 +902,13 @@
       },
       "version": "4.11.0"
     },
+    "org.mockito:mockito-inline": {
+      "shasums": {
+        "jar": "ee52e1c299a632184fba274a9370993e09140429f5e516e6c5570fd6574b297f",
+        "sources": "ee52e1c299a632184fba274a9370993e09140429f5e516e6c5570fd6574b297f"
+      },
+      "version": "4.11.0"
+    },
     "org.objenesis:objenesis": {
       "shasums": {
         "jar": "02dfd0b0439a5591e35b708ed2f5474eb0948f53abf74637e959b8e4ef69bfeb",
@@ -1475,6 +1482,9 @@
       "net.bytebuddy:byte-buddy-agent",
       "org.objenesis:objenesis"
     ],
+    "org.mockito:mockito-inline": [
+      "org.mockito:mockito-core"
+    ],
     "org.ow2.asm:asm-analysis": [
       "org.ow2.asm:asm-tree"
     ],
@@ -3764,6 +3774,8 @@
       "org.junit.platform:junit-platform-suite-engine:jar:sources",
       "org.mockito:mockito-core",
       "org.mockito:mockito-core:jar:sources",
+      "org.mockito:mockito-inline",
+      "org.mockito:mockito-inline:jar:sources",
       "org.objenesis:objenesis",
       "org.objenesis:objenesis:jar:sources",
       "org.opentest4j:opentest4j",
@@ -4071,6 +4083,8 @@
       "org.junit.platform:junit-platform-suite-engine:jar:sources",
       "org.mockito:mockito-core",
       "org.mockito:mockito-core:jar:sources",
+      "org.mockito:mockito-inline",
+      "org.mockito:mockito-inline:jar:sources",
       "org.objenesis:objenesis",
       "org.objenesis:objenesis:jar:sources",
       "org.opentest4j:opentest4j",

diff --git a/java/src/org/openqa/selenium/internal/Debug.java b/java/src/org/openqa/selenium/internal/Debug.java
@@ -23,8 +23,7 @@
 /** Used to provide information about whether Selenium is running under debug mode. */
 public class Debug {
 
-  private static boolean IS_DEBUG;
-
+  private static final boolean IS_DEBUG;
   static {
     boolean debugFlag =
         ManagementFactory.getRuntimeMXBean().getInputArguments().stream()

diff --git a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java
@@ -74,6 +74,7 @@
 import org.openqa.selenium.devtools.HasDevTools;
 import org.openqa.selenium.interactions.Interactive;
 import org.openqa.selenium.interactions.Sequence;
+import org.openqa.selenium.internal.Debug;
 import org.openqa.selenium.internal.Require;
 import org.openqa.selenium.logging.LocalLogs;
 import org.openqa.selenium.logging.LoggingHandler;
@@ -542,7 +543,14 @@ protected Response execute(CommandPayload payload) {
                 "Error communicating with the remote browser. It may have died.", e);
       }
       populateWebDriverException(toThrow);
-      toThrow.addInfo("Command", command.toString());
+      // Showing full command information when user is debugging
+      // Avoid leaking user/pwd values for authenticated Grids.
+      if (toThrow instanceof UnreachableBrowserException && !Debug.isDebugging()) {
+        toThrow.addInfo("Command", "[" + command.getSessionId() + ", " + command.getName()
+                                   + " " + command.getParameters().keySet() + "]");
+      } else {
+        toThrow.addInfo("Command", command.toString());
+      }
       throw toThrow;
     } finally {
       Thread.currentThread().setName(currentName);

diff --git a/java/test/org/openqa/selenium/remote/BUILD.bazel b/java/test/org/openqa/selenium/remote/BUILD.bazel
@@ -33,6 +33,7 @@ java_test_suite(
         artifact("com.google.guava:guava"),
         artifact("org.junit.jupiter:junit-jupiter-api"),
         artifact("org.mockito:mockito-core"),
+        artifact("org.mockito:mockito-inline"),
     ] + JUNIT5_DEPS,
 )
 

diff --git a/java/test/org/openqa/selenium/remote/RemoteWebDriverUnitTest.java b/java/test/org/openqa/selenium/remote/RemoteWebDriverUnitTest.java
@@ -45,6 +45,8 @@
 import java.util.logging.Level;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
 import org.openqa.selenium.Alert;
 import org.openqa.selenium.By;
 import org.openqa.selenium.Cookie;
@@ -58,6 +60,7 @@
 import org.openqa.selenium.WebDriverException;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.WindowType;
+import org.openqa.selenium.internal.Debug;
 import org.openqa.selenium.virtualauthenticator.VirtualAuthenticator;
 import org.openqa.selenium.virtualauthenticator.VirtualAuthenticatorOptions;
 
@@ -698,13 +701,69 @@ void canHandleGeneralExceptionThrownByCommandExecutor() {
         .withMessageContaining(String.format("Session ID: %s", fixture.driver.getSessionId()))
         .withMessageContaining(String.format("%s", fixture.driver.getCapabilities()))
         .withMessageContaining(
-            String.format("Command: [%s, getCurrentUrl {}]", fixture.driver.getSessionId()))
+            String.format("Command: [%s, getCurrentUrl []]", fixture.driver.getSessionId()))
         .havingCause()
         .withMessage("BOOM!!!");
 
     fixture.verifyCommands(new CommandPayload(DriverCommand.GET_CURRENT_URL, emptyMap()));
   }
 
+  @Test
+  void canHandleGeneralExceptionInNonDebugModeThrownByCommandExecutor() {
+    try (MockedStatic<Debug> debugMock = Mockito.mockStatic(Debug.class)) {
+      final ImmutableMap<String, String> parameters = ImmutableMap.of(
+        "url", "https://user:password@somedomain.com", "token", "12345Secret");
+      final CommandPayload commandPayload = new CommandPayload(DriverCommand.GET, parameters);
+      debugMock.when(Debug::isDebugging).thenReturn(false);
+      WebDriverFixture fixture = new WebDriverFixture(
+        new ImmutableCapabilities(
+          "browserName", "cheese", "platformName", "WINDOWS"),
+        echoCapabilities, exceptionResponder);
+      assertThatExceptionOfType(UnreachableBrowserException.class)
+        .isThrownBy(() -> fixture.driver.execute(commandPayload))
+        .withMessageStartingWith("Error communicating with the remote browser. It may have died.")
+        .withMessageContaining("Build info: ")
+        .withMessageContaining(
+          "Driver info: org.openqa.selenium.remote.RemoteWebDriver")
+        .withMessageContaining(String.format(
+          "Session ID: %s", fixture.driver.getSessionId()))
+        .withMessageContaining(String.format(
+          "%s", fixture.driver.getCapabilities()))
+        .withMessageContaining(String.format(
+          "Command: [%s, get [url, token]]", fixture.driver.getSessionId()))
+        .havingCause()
+        .withMessage("BOOM!!!");
+    }
+  }
+
+  @Test
+  void canHandleGeneralExceptionInDebugModeThrownByCommandExecutor() {
+    try (MockedStatic<Debug> debugMock = Mockito.mockStatic(Debug.class)) {
+      final ImmutableMap<String, String> parameters = ImmutableMap.of(
+        "url", "https://user:password@somedomain.com", "token", "12345Secret");
+      final CommandPayload commandPayload = new CommandPayload(DriverCommand.GET, parameters);
+      debugMock.when(Debug::isDebugging).thenReturn(true);
+      WebDriverFixture fixture = new WebDriverFixture(
+        new ImmutableCapabilities(
+          "browserName", "cheese", "platformName", "WINDOWS"),
+        echoCapabilities, exceptionResponder);
+      assertThatExceptionOfType(UnreachableBrowserException.class)
+        .isThrownBy(() -> fixture.driver.execute(commandPayload))
+        .withMessageStartingWith("Error communicating with the remote browser. It may have died.")
+        .withMessageContaining("Build info: ")
+        .withMessageContaining(
+          "Driver info: org.openqa.selenium.remote.RemoteWebDriver")
+        .withMessageContaining(String.format(
+          "Session ID: %s", fixture.driver.getSessionId()))
+        .withMessageContaining(String.format(
+          "%s", fixture.driver.getCapabilities()))
+        .withMessageContaining(String.format(
+          "Command: [%s, get %s]", fixture.driver.getSessionId(), parameters))
+        .havingCause()
+        .withMessage("BOOM!!!");
+    }
+  }
+
   @Test
   void canHandleWebDriverExceptionReturnedByCommandExecutor() {
     WebDriverFixture fixture =

diff --git a/java/test/org/openqa/selenium/remote/RemoteWebElementTest.java b/java/test/org/openqa/selenium/remote/RemoteWebElementTest.java
@@ -30,13 +30,16 @@
 import com.google.common.collect.ImmutableMap;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
 import org.openqa.selenium.By;
 import org.openqa.selenium.Dimension;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.NoSuchElementException;
 import org.openqa.selenium.Point;
 import org.openqa.selenium.Rectangle;
 import org.openqa.selenium.WebDriverException;
+import org.openqa.selenium.internal.Debug;
 
 @Tag("UnitTests")
 class RemoteWebElementTest {
@@ -91,33 +94,33 @@ void canHandleWebDriverExceptionThrownByCommandExecutor() {
 
   @Test
   void canHandleGeneralExceptionThrownByCommandExecutor() {
-    WebElementFixture fixture =
-        new WebElementFixture(
-            new ImmutableCapabilities("browserName", "cheese", "platformName", "WINDOWS"),
-            echoCapabilities,
-            exceptionResponder);
+    try (MockedStatic<Debug> debugMock = Mockito.mockStatic(Debug.class)) {
+      debugMock.when(Debug::isDebugging).thenReturn(true);
+      WebElementFixture fixture = new WebElementFixture(
+        new ImmutableCapabilities("browserName", "cheese", "platformName", "WINDOWS"),
+        echoCapabilities, exceptionResponder);
 
-    assertThatExceptionOfType(WebDriverException.class)
+      assertThatExceptionOfType(WebDriverException.class)
         .isThrownBy(fixture.element::click)
         .withMessageStartingWith("Error communicating with the remote browser. It may have died.")
         .withMessageContaining("Build info: ")
-        .withMessageContaining("Driver info: org.openqa.selenium.remote.RemoteWebDriver")
-        .withMessageContaining(String.format("Session ID: %s", fixture.driver.getSessionId()))
-        .withMessageContaining(String.format("%s", fixture.driver.getCapabilities()))
         .withMessageContaining(
-            String.format(
-                "Command: [%s, clickElement {id=%s}]",
-                fixture.driver.getSessionId(), fixture.element.getId()))
-        .withMessageContaining(
-            String.format(
-                "Element: [[RemoteWebDriver: cheese on windows (%s)] -> id: test]",
-                fixture.driver.getSessionId()))
+          "Driver info: org.openqa.selenium.remote.RemoteWebDriver")
+        .withMessageContaining(String.format(
+          "Session ID: %s", fixture.driver.getSessionId()))
+        .withMessageContaining(String.format(
+          "%s", fixture.driver.getCapabilities()))
+        .withMessageContaining(String.format(
+          "Command: [%s, clickElement {id=%s}]", fixture.driver.getSessionId(), fixture.element.getId()))
+        .withMessageContaining(String.format(
+          "Element: [[RemoteWebDriver: cheese on windows (%s)] -> id: test]", fixture.driver.getSessionId()))
         .havingCause()
         .withMessage("BOOM!!!");
 
-    fixture.verifyCommands(
+      fixture.verifyCommands(
         new CommandPayload(
-            DriverCommand.CLICK_ELEMENT, ImmutableMap.of("id", fixture.element.getId())));
+          DriverCommand.CLICK_ELEMENT, ImmutableMap.of("id", fixture.element.getId())));
+    }
   }
 
   @Test