Don't reveal credentials in log and error output

README.md

@@ -110,7 +110,8 @@ The directory holding `tnsnames.ora` (and `ojdbc.properties`, if used) is taken
 
 Options 1-3 are handled by the Oracle JDBC driver. Option 4 is a fallback provided by utPLSQL-cli, used only when none of the others is set.
 
-In case you use a username containing `/` or a password containing `@` you should encapsulate it with double quotes `"`:
+A password may contain `@`: everything up to the last `@` is taken as the password, e.g. `utplsql run myUser/myP@ssword@connectstring`.
+A username containing `/` must be enclosed in double quotes `"`, and so may the password:
 ```
 utplsql run "my/Username"/"myP@ssword"@connectstring
 ```

src/main/java/org/utplsql/cli/Cli.java

@@ -4,7 +4,9 @@
 import org.slf4j.LoggerFactory;
 import picocli.CommandLine;
 
+import java.util.Arrays;
 import java.util.List;
+import java.util.stream.Collectors;
 
 public class Cli {
 
@@ -19,10 +21,19 @@
         System.exit(exitCode);
     }
 
+    /**
+     * @return the arguments separated by ", ", with the credentials of the connect string masked
+     */
+    static String maskedArgs(String... args) {
+        return Arrays.stream(args)
+                .map(ConnectionConfig::maskCredentials)
+                .collect(Collectors.joining(", "));
+    }
+
     static int runPicocliWithExitCode(String[] args) {
 
 
-        logger.debug("Args: "+String.join(", ", args));
+        logger.debug("Args: {}", maskedArgs(args));
 
         CommandLine commandLine = new CommandLine(UtplsqlPicocliCommand.class);
         commandLine.setTrimQuotes(true);

src/main/java/org/utplsql/cli/ConnectionConfig.java

@@ -7,9 +7,10 @@
 
     /**
      * Either {@code <user>/<password>@<connect>} or {@code /@<connect>}.
+     * An unquoted password extends to the last {@code @}, so it may contain {@code @} itself.
      */
     private static final Pattern CONNECT_STRING_PATTERN =
-            Pattern.compile("^(?:(\".+\"|[^/]+)/(\".+\"|[^@]+)|/)@(.*)$");
+            Pattern.compile("^(?:(\".+\"|[^/]+)/(\".+\"|.+)|/)@(.*)$");
 
     private final String user;
     private final String password;
@@ -26,6 +27,19 @@
         }
     }
 
+    /**
+     * Masks the credentials of a connect string, e.g. for logging.
+     *
+     * @param value any string, e.g. a command line argument
+     * @return the value as returned by {@link #getMaskedConnectString()} for a connect string, otherwise the unchanged value
+     */
+    public static String maskCredentials(String value) {
+        if (value == null || !CONNECT_STRING_PATTERN.matcher(value).matches()) {
+            return value;
+        }
+        return new ConnectionConfig(value).getMaskedConnectString();
+    }
+
     private String stripEnclosingQuotes(String value) {
         if (value.length() > 1
                 && value.startsWith("\"")
@@ -63,6 +77,17 @@
         return user + "/" + password + "@" + connect;
     }
 
+    /**
+     * @return the connect string with user and password replaced by asterisks,
+     * or {@code /@<connect>} for external authentication
+     */
+    public String getMaskedConnectString() {
+        if (isExternalAuthentication()) {
+            return "/@" + connect;
+        }
+        return "****/****@" + connect;
+    }
+
     public boolean isSysDba() {
         return user != null &&
                 (user.toLowerCase().endsWith(" as sysdba")

src/main/java/org/utplsql/cli/datasource/TestedDataSourceProvider.java

@@ -16,23 +16,18 @@
 
 public class TestedDataSourceProvider {
 
-    interface ConnectStringPossibility {
-        String getConnectString(ConnectionConfig config);
-
-        String getMaskedConnectString(ConnectionConfig config);
-    }
-
     private static final Logger logger = LoggerFactory.getLogger(TestedDataSourceProvider.class);
+    /**
+     * JDBC URL prefixes tried in this order: thick (OCI) driver first, then thin driver
+     */
+    private static final List<String> JDBC_URL_PREFIXES = List.of("jdbc:oracle:oci8:", "jdbc:oracle:thin:");
+
     private final ConnectionConfig config;
-    private final List<ConnectStringPossibility> possibilities = new ArrayList<>();
     private final int maxConnections;
 
     public TestedDataSourceProvider(ConnectionConfig config, int maxConnections) {
         this.config = config;
         this.maxConnections = maxConnections;
-
-        possibilities.add(new ThickConnectStringPossibility());
-        possibilities.add(new ThinConnectStringPossibility());
     }
 
     public DataSource getDataSource() throws SQLException {
@@ -55,14 +50,15 @@ private void setThickOrThinJdbcUrl(InitializableOracleDataSource ds) throws SQLE
             ds.setPassword(config.getPassword());
         }
 
-        for (ConnectStringPossibility possibility : possibilities) {
-            logger.debug("Try connecting {}", possibility.getMaskedConnectString(config));
-            ds.setURL(possibility.getConnectString(config));
+        for (String jdbcUrlPrefix : JDBC_URL_PREFIXES) {
+            String maskedUrl = jdbcUrlPrefix + config.getMaskedConnectString();
+            logger.debug("Try connecting {}", maskedUrl);
+            ds.setURL(jdbcUrlPrefix + "@" + config.getConnect());
             try (Connection ignored = ds.getConnection()) {
-                logger.info("Use connection string {}", possibility.getMaskedConnectString(config));
+                logger.info("Use connection string {}", maskedUrl);
                 return;
             } catch (Error | Exception e) {
-                errors.add(possibility.getMaskedConnectString(config) + ": " + e.getMessage());
+                errors.add(maskedUrl + ": " + e.getMessage());
                 lastException = e;
             }
         }
@@ -101,32 +97,4 @@ private void setInitSqlFrom_NLS_LANG(InitializableOracleDataSource ds) {
             }
         }
     }
-
-    private static class ThickConnectStringPossibility implements ConnectStringPossibility {
-        @Override
-        public String getConnectString(ConnectionConfig config) {
-            return "jdbc:oracle:oci8:@" + config.getConnect();
-        }
-
-        @Override
-        public String getMaskedConnectString(ConnectionConfig config) {
-            return "jdbc:oracle:oci8:" + maskedCredentials(config) + "@" + config.getConnect();
-        }
-    }
-
-    private static class ThinConnectStringPossibility implements ConnectStringPossibility {
-        @Override
-        public String getConnectString(ConnectionConfig config) {
-            return "jdbc:oracle:thin:@" + config.getConnect();
-        }
-
-        @Override
-        public String getMaskedConnectString(ConnectionConfig config) {
-            return "jdbc:oracle:thin:" + maskedCredentials(config) + "@" + config.getConnect();
-        }
-    }
-
-    private static String maskedCredentials(ConnectionConfig config) {
-        return config.isExternalAuthentication() ? "/" : "****/****";
-    }
 }

src/test/java/org/utplsql/cli/CliArgsLoggingTest.java

@@ -0,0 +1,60 @@
+package org.utplsql.cli;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * The command line arguments are logged at debug level; the credentials of the connect string must not be (issue #172).
+ */
+class CliArgsLoggingTest {
+
+    private final Logger cliLogger = (Logger) LoggerFactory.getLogger(Cli.class);
+    private final ListAppender<ILoggingEvent> appender = new ListAppender<>();
+    private Level originalLevel;
+
+    @BeforeEach
+    void captureCliLog() {
+        originalLevel = cliLogger.getLevel();
+        cliLogger.setLevel(Level.DEBUG);
+        appender.start();
+        cliLogger.addAppender(appender);
+    }
+
+    @AfterEach
+    void restoreCliLog() {
+        cliLogger.detachAppender(appender);
+        cliLogger.setLevel(originalLevel);
+    }
+
+    @Test
+    void maskedArgs() {
+        assertEquals("run, ****/****@//localhost:1521/FREEPDB1, --debug",
+                Cli.maskedArgs("run", "app/Sup3rSecretPw@//localhost:1521/FREEPDB1", "--debug"));
+    }
+
+    @Test
+    void passwordIsNotLogged() {
+        // "run -h" only prints the usage, so no database is needed
+        Cli.runPicocliWithExitCode(new String[]{"run", "app/Sup3rSecretPw@//localhost:1521/FREEPDB1", "-h"});
+
+        List<String> messages = appender.list.stream()
+                .map(ILoggingEvent::getFormattedMessage)
+                .toList();
+
+        assertTrue(messages.contains("Args: run, ****/****@//localhost:1521/FREEPDB1, -h"), () -> "Logged: " + messages);
+        assertFalse(messages.stream().anyMatch(message -> message.contains("Sup3rSecretPw")), () -> "Logged: " + messages);
+    }
+}

src/test/java/org/utplsql/cli/ConnectionConfigTest.java

@@ -2,6 +2,7 @@
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.ValueSource;
 
 import static org.junit.jupiter.api.Assertions.*;
@@ -106,4 +107,45 @@ void parseExternalAuthenticationWithEzConnect() {
     void rejectInvalidConnectString(String connectString) {
         assertThrows(IllegalArgumentException.class, () -> new ConnectionConfig(connectString));
     }
+
+    @Test
+    void parseUnquotedPasswordWithAt() {
+        ConnectionConfig info = new ConnectionConfig("test/p@ss@w0rd@MY_TNS_ALIAS");
+
+        assertEquals("test", info.getUser());
+        assertEquals("p@ss@w0rd", info.getPassword());
+        assertEquals("MY_TNS_ALIAS", info.getConnect());
+    }
+
+    @ParameterizedTest
+    @CsvSource(delimiter = '|', value = {
+            "test/pw@my.local.host/service              | ****/****@my.local.host/service",
+            "test/pw@//my.local.host:1521/service       | ****/****@//my.local.host:1521/service",
+            "sys as sysdba/pw@MY_TNS_ALIAS              | ****/****@MY_TNS_ALIAS",
+            "test/\"p@ssw0rd=\"@MY_TNS_ALIAS            | ****/****@MY_TNS_ALIAS",
+            "\"User/Mine@=\"/pw@MY_TNS_ALIAS            | ****/****@MY_TNS_ALIAS",
+            "test/p@ss@MY_TNS_ALIAS                     | ****/****@MY_TNS_ALIAS",
+            "/@MY_TNS_ALIAS                             | /@MY_TNS_ALIAS"
+    })
+    void maskCredentials(String connectString, String expected) {
+        assertEquals(expected, ConnectionConfig.maskCredentials(connectString));
+        assertEquals(expected, new ConnectionConfig(connectString).getMaskedConnectString());
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "run",
+            "--debug",
+            "-p=app.test_pkg",
+            "-f=ut_documentation_reporter",
+            "MY_TNS_ALIAS"
+    })
+    void maskCredentialsLeavesOtherValuesUnchanged(String value) {
+        assertEquals(value, ConnectionConfig.maskCredentials(value));
+    }
+
+    @Test
+    void maskCredentialsOfNull() {
+        assertNull(ConnectionConfig.maskCredentials(null));
+    }
 }