diff --git a/README.md b/README.md index f3a2895..8f22984 100644 --- a/README.md +++ b/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 ``` diff --git a/src/main/java/org/utplsql/cli/Cli.java b/src/main/java/org/utplsql/cli/Cli.java index 3b14512..4a980a4 100644 --- a/src/main/java/org/utplsql/cli/Cli.java +++ b/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 @@ public static void main(String[] args) { 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); diff --git a/src/main/java/org/utplsql/cli/ConnectionConfig.java b/src/main/java/org/utplsql/cli/ConnectionConfig.java index d0ce618..180a268 100644 --- a/src/main/java/org/utplsql/cli/ConnectionConfig.java +++ b/src/main/java/org/utplsql/cli/ConnectionConfig.java @@ -7,9 +7,10 @@ public class ConnectionConfig { /** * Either {@code /@} or {@code /@}. + * 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 @@ public ConnectionConfig(String connectString) { } } + /** + * 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 @@ public String getConnectString() { return user + "/" + password + "@" + connect; } + /** + * @return the connect string with user and password replaced by asterisks, + * or {@code /@} for external authentication + */ + public String getMaskedConnectString() { + if (isExternalAuthentication()) { + return "/@" + connect; + } + return "****/****@" + connect; + } + public boolean isSysDba() { return user != null && (user.toLowerCase().endsWith(" as sysdba") diff --git a/src/main/java/org/utplsql/cli/datasource/TestedDataSourceProvider.java b/src/main/java/org/utplsql/cli/datasource/TestedDataSourceProvider.java index f871c03..7034144 100644 --- a/src/main/java/org/utplsql/cli/datasource/TestedDataSourceProvider.java +++ b/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 JDBC_URL_PREFIXES = List.of("jdbc:oracle:oci8:", "jdbc:oracle:thin:"); + private final ConnectionConfig config; - private final List 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() ? "/" : "****/****"; - } } diff --git a/src/test/java/org/utplsql/cli/CliArgsLoggingTest.java b/src/test/java/org/utplsql/cli/CliArgsLoggingTest.java new file mode 100644 index 0000000..8ccab49 --- /dev/null +++ b/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 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 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); + } +} diff --git a/src/test/java/org/utplsql/cli/ConnectionConfigTest.java b/src/test/java/org/utplsql/cli/ConnectionConfigTest.java index f32ad4d..79c3553 100644 --- a/src/test/java/org/utplsql/cli/ConnectionConfigTest.java +++ b/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)); + } }