From f1caee28ad0d7d6bb7c81aab3fc51da939adc87d Mon Sep 17 00:00:00 2001 From: vga91 Date: Tue, 17 Dec 2024 10:44:17 +0100 Subject: [PATCH 1/3] [NOID] Fixex #4256: Obfuscate JDBC Password in query.log --- .../main/java/apoc/load/util/JdbcUtil.java | 4 ++ full/src/main/java/apoc/load/Jdbc.java | 38 ++++++++----------- full/src/test/java/apoc/load/JdbcTest.java | 36 +++++++++++++++++- .../src/test/java/apoc/load/LoadLdapTest.java | 10 +---- .../test/java/apoc/util/ExtendedTestUtil.java | 24 ++++++++++++ 5 files changed, 78 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/apoc/load/util/JdbcUtil.java b/core/src/main/java/apoc/load/util/JdbcUtil.java index 7858bdcd6b..9ef3d78925 100644 --- a/core/src/main/java/apoc/load/util/JdbcUtil.java +++ b/core/src/main/java/apoc/load/util/JdbcUtil.java @@ -89,5 +89,9 @@ public static String getSqlOrKey(String sqlOrKey) { return sqlOrKey.contains(" ") ? sqlOrKey : Util.getLoadUrlByConfigFile(LOAD_TYPE, sqlOrKey, "sql").orElse("SELECT * FROM " + sqlOrKey); + } + + public static String obfuscateJdbcUrl(String query) { + return query.replaceAll("(jdbc:[^:]+://)([^\\s\\\"']+)", "$1*******"); } } diff --git a/full/src/main/java/apoc/load/Jdbc.java b/full/src/main/java/apoc/load/Jdbc.java index 2bd2d4cd2d..d9bbfc69fc 100644 --- a/full/src/main/java/apoc/load/Jdbc.java +++ b/full/src/main/java/apoc/load/Jdbc.java @@ -131,17 +131,7 @@ private Stream executeQuery( throw sqle; } } catch (Exception e) { - log.error(String.format("Cannot execute SQL statement `%s`.%nError:%n%s", query, e.getMessage()), e); - String errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s"; - if (e.getMessage().contains("No suitable driver")) - errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s%n%s"; - throw new RuntimeException( - String.format( - errorMessage, - query, - e.getMessage(), - "Please download and copy the JDBC driver into $NEO4J_HOME/plugins,more details at https://neo4j-contrib.github.io/neo4j-apoc-procedures/#_load_jdbc_resources"), - e); + throw logsErrorAndThrowsException(e, query, log); } } @@ -182,20 +172,22 @@ private Stream executeUpdate( throw sqle; } } catch (Exception e) { - log.error(String.format("Cannot execute SQL statement `%s`.%nError:%n%s", query, e.getMessage()), e); - String errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s"; - if (e.getMessage().contains("No suitable driver")) - errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s%n%s"; - throw new RuntimeException( - String.format( - errorMessage, - query, - e.getMessage(), - "Please download and copy the JDBC driver into $NEO4J_HOME/plugins,more details at https://neo4j-contrib.github.io/neo4j-apoc-procedures/#_load_jdbc_resources"), - e); + throw logsErrorAndThrowsException(e, query, log); } } - + + private static RuntimeException logsErrorAndThrowsException(Exception e, String query, Log log) { + String errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s"; + String exceptionMsg = e.getMessage(); + if(e.getMessage().contains("No suitable driver")) { + errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s%n%s"; + exceptionMsg = obfuscateJdbcUrl(e.getMessage()); + } + Exception ex = new Exception(exceptionMsg); + log.error(String.format("Cannot execute SQL statement `%s`.%nError:%n%s", query, exceptionMsg), ex); + return new RuntimeException(String.format(errorMessage, query, exceptionMsg, "Please download and copy the JDBC driver into $NEO4J_HOME/plugins, more details at https://neo4j-contrib.github.io/neo4j-apoc-procedures/#_load_jdbc_resources"), ex); + } + static void closeIt(Log log, AutoCloseable... closeables) { for (AutoCloseable c : closeables) { try { diff --git a/full/src/test/java/apoc/load/JdbcTest.java b/full/src/test/java/apoc/load/JdbcTest.java index 43d3fd798a..ccdd459c06 100644 --- a/full/src/test/java/apoc/load/JdbcTest.java +++ b/full/src/test/java/apoc/load/JdbcTest.java @@ -19,10 +19,13 @@ package apoc.load; import static apoc.ApocConfig.apocConfig; +import static apoc.util.ExtendedTestUtil.assertFails; +import static apoc.util.ExtendedTestUtil.getLogFileContent; import static apoc.util.MapUtil.map; import static apoc.util.TestUtil.testCall; import static apoc.util.TestUtil.testResult; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import apoc.periodic.Periodic; import apoc.util.TestUtil; @@ -39,17 +42,25 @@ import java.util.Map; import org.junit.*; import org.junit.rules.ExpectedException; +import org.junit.rules.TemporaryFolder; import org.junit.rules.TestName; +import org.neo4j.configuration.GraphDatabaseSettings; +import org.neo4j.dbms.api.DatabaseManagementService; +import org.neo4j.graphdb.GraphDatabaseService; import org.neo4j.graphdb.QueryExecutionException; import org.neo4j.internal.helpers.collection.Iterators; import org.neo4j.internal.helpers.collection.MapUtil; +import org.neo4j.test.TestDatabaseManagementServiceBuilder; import org.neo4j.test.rule.DbmsRule; import org.neo4j.test.rule.ImpermanentDbmsRule; public class JdbcTest extends AbstractJdbcTest { @Rule - public DbmsRule db = new ImpermanentDbmsRule(); + public TemporaryFolder STORE_DIR = new TemporaryFolder(); + + private GraphDatabaseService db; + private DatabaseManagementService dbms; private Connection conn; @@ -63,6 +74,9 @@ public class JdbcTest extends AbstractJdbcTest { @Before public void setUp() throws Exception { + dbms = new TestDatabaseManagementServiceBuilder(STORE_DIR.getRoot().toPath()).build(); + db = dbms.database(GraphDatabaseSettings.DEFAULT_DATABASE_NAME); + apocConfig().setProperty("apoc.jdbc.derby.url", "jdbc:derby:derbyDB"); apocConfig().setProperty("apoc.jdbc.test.sql", "SELECT * FROM PERSON"); apocConfig().setProperty("apoc.jdbc.testparams.sql", "SELECT * FROM PERSON WHERE NAME = ?"); @@ -92,7 +106,7 @@ public void tearDown() throws SQLException { System.clearProperty("derby.connection.requireAuthentication"); System.clearProperty("derby.user.apoc"); - db.shutdown(); + dbms.shutdown(); } @Test @@ -133,6 +147,24 @@ public void testLoadJdbcParams() throws Exception { (row) -> assertResult(row)); } + @Test + public void testExceptionAndLogWithObfuscatedUrl() { + String url = "jdbc:ajeje://localhost:3306/data_mart?user=root&password=root"; + String errorMsgWithObfuscatedUrl = "No suitable driver found for jdbc:ajeje://*******"; + + // obfuscated exception + assertFails(db, "CALL apoc.load.jdbc($url,'SELECT * FROM PERSON WHERE NAME = ?',['John'])", + Map.of("url", url), + errorMsgWithObfuscatedUrl + ); + + // obfuscated log in `debug.log` + String fileContent = getLogFileContent(); + assertTrue("Actual log content is: " + fileContent, + fileContent.contains(errorMsgWithObfuscatedUrl) + ); + } + @Test public void testLoadJdbcParamsWithConfigLocalDateTime() throws Exception { testCall( diff --git a/full/src/test/java/apoc/load/LoadLdapTest.java b/full/src/test/java/apoc/load/LoadLdapTest.java index ec6e6ebe49..3151646cbf 100644 --- a/full/src/test/java/apoc/load/LoadLdapTest.java +++ b/full/src/test/java/apoc/load/LoadLdapTest.java @@ -19,6 +19,7 @@ package apoc.load; import static apoc.ApocConfig.apocConfig; +import static apoc.util.ExtendedTestUtil.getLogFileContent; import static apoc.util.TestUtil.testCall; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -117,15 +118,6 @@ public void testLoadLDAPWithApocConfigWithoutDotBeforeLdapKey() { assertTrue(getLogFileContent().contains(logWarn)); } - private static String getLogFileContent() { - try { - File logFile = new File(FileUtils.getLogDirectory(), "debug.log"); - return Files.readString(logFile.toPath()); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - private void testWithStringConfigCommon(String key) { // set a config `key=localhost:port dns pwd` String ldapValue = diff --git a/full/src/test/java/apoc/util/ExtendedTestUtil.java b/full/src/test/java/apoc/util/ExtendedTestUtil.java index dc3a8ba34c..bbebb0b88f 100644 --- a/full/src/test/java/apoc/util/ExtendedTestUtil.java +++ b/full/src/test/java/apoc/util/ExtendedTestUtil.java @@ -1,8 +1,14 @@ package apoc.util; +import static apoc.util.TestUtil.testCall; import static apoc.util.TestUtil.testCallAssertions; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.neo4j.test.assertion.Assert.assertEventually; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; import java.util.Collections; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -67,4 +73,22 @@ public static void testResultEventually( timeout, TimeUnit.SECONDS); } + + public static void assertFails(GraphDatabaseService db, String query, Map params, String expectedErrMsg) { + try { + testCall(db, query, params, r -> fail("Should fail due to " + expectedErrMsg)); + } catch (Exception e) { + String actualErrMsg = e.getMessage(); + assertTrue("Actual err. message is: " + actualErrMsg, actualErrMsg.contains(expectedErrMsg)); + } + } + + public static String getLogFileContent() { + try { + File logFile = new File(FileUtils.getLogDirectory(), "debug.log"); + return Files.readString(logFile.toPath()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } } From 1e77ce47ebf2c35c25bd698a46d1193e41f188c0 Mon Sep 17 00:00:00 2001 From: vga91 Date: Wed, 18 Dec 2024 10:23:08 +0100 Subject: [PATCH 2/3] [NOID] format changes --- core/src/main/java/apoc/load/util/JdbcUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/apoc/load/util/JdbcUtil.java b/core/src/main/java/apoc/load/util/JdbcUtil.java index 9ef3d78925..1bbb555bb3 100644 --- a/core/src/main/java/apoc/load/util/JdbcUtil.java +++ b/core/src/main/java/apoc/load/util/JdbcUtil.java @@ -89,7 +89,7 @@ public static String getSqlOrKey(String sqlOrKey) { return sqlOrKey.contains(" ") ? sqlOrKey : Util.getLoadUrlByConfigFile(LOAD_TYPE, sqlOrKey, "sql").orElse("SELECT * FROM " + sqlOrKey); - } + } public static String obfuscateJdbcUrl(String query) { return query.replaceAll("(jdbc:[^:]+://)([^\\s\\\"']+)", "$1*******"); From d5ee5c032abc4ae0ed8cee5a1ed2d972f1c64409 Mon Sep 17 00:00:00 2001 From: vga91 Date: Wed, 18 Dec 2024 10:47:58 +0100 Subject: [PATCH 3/3] [NOID] removed duplicated test and format changes --- full/src/main/java/apoc/load/Jdbc.java | 14 ++++++++---- full/src/test/java/apoc/load/JdbcTest.java | 21 ++++++++---------- .../src/test/java/apoc/load/LoadLdapTest.java | 4 ---- .../apoc/periodic/PeriodicExtendedTest.java | 22 ------------------- .../test/java/apoc/util/ExtendedTestUtil.java | 3 ++- 5 files changed, 21 insertions(+), 43 deletions(-) diff --git a/full/src/main/java/apoc/load/Jdbc.java b/full/src/main/java/apoc/load/Jdbc.java index d9bbfc69fc..6204a91770 100644 --- a/full/src/main/java/apoc/load/Jdbc.java +++ b/full/src/main/java/apoc/load/Jdbc.java @@ -175,19 +175,25 @@ private Stream executeUpdate( throw logsErrorAndThrowsException(e, query, log); } } - + private static RuntimeException logsErrorAndThrowsException(Exception e, String query, Log log) { String errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s"; String exceptionMsg = e.getMessage(); - if(e.getMessage().contains("No suitable driver")) { + if (e.getMessage().contains("No suitable driver")) { errorMessage = "Cannot execute SQL statement `%s`.%nError:%n%s%n%s"; exceptionMsg = obfuscateJdbcUrl(e.getMessage()); } Exception ex = new Exception(exceptionMsg); log.error(String.format("Cannot execute SQL statement `%s`.%nError:%n%s", query, exceptionMsg), ex); - return new RuntimeException(String.format(errorMessage, query, exceptionMsg, "Please download and copy the JDBC driver into $NEO4J_HOME/plugins, more details at https://neo4j-contrib.github.io/neo4j-apoc-procedures/#_load_jdbc_resources"), ex); + return new RuntimeException( + String.format( + errorMessage, + query, + exceptionMsg, + "Please download and copy the JDBC driver into $NEO4J_HOME/plugins, more details at https://neo4j-contrib.github.io/neo4j-apoc-procedures/#_load_jdbc_resources"), + ex); } - + static void closeIt(Log log, AutoCloseable... closeables) { for (AutoCloseable c : closeables) { try { diff --git a/full/src/test/java/apoc/load/JdbcTest.java b/full/src/test/java/apoc/load/JdbcTest.java index ccdd459c06..f7db48046a 100644 --- a/full/src/test/java/apoc/load/JdbcTest.java +++ b/full/src/test/java/apoc/load/JdbcTest.java @@ -51,14 +51,12 @@ import org.neo4j.internal.helpers.collection.Iterators; import org.neo4j.internal.helpers.collection.MapUtil; import org.neo4j.test.TestDatabaseManagementServiceBuilder; -import org.neo4j.test.rule.DbmsRule; -import org.neo4j.test.rule.ImpermanentDbmsRule; public class JdbcTest extends AbstractJdbcTest { @Rule public TemporaryFolder STORE_DIR = new TemporaryFolder(); - + private GraphDatabaseService db; private DatabaseManagementService dbms; @@ -76,7 +74,7 @@ public class JdbcTest extends AbstractJdbcTest { public void setUp() throws Exception { dbms = new TestDatabaseManagementServiceBuilder(STORE_DIR.getRoot().toPath()).build(); db = dbms.database(GraphDatabaseSettings.DEFAULT_DATABASE_NAME); - + apocConfig().setProperty("apoc.jdbc.derby.url", "jdbc:derby:derbyDB"); apocConfig().setProperty("apoc.jdbc.test.sql", "SELECT * FROM PERSON"); apocConfig().setProperty("apoc.jdbc.testparams.sql", "SELECT * FROM PERSON WHERE NAME = ?"); @@ -153,16 +151,15 @@ public void testExceptionAndLogWithObfuscatedUrl() { String errorMsgWithObfuscatedUrl = "No suitable driver found for jdbc:ajeje://*******"; // obfuscated exception - assertFails(db, "CALL apoc.load.jdbc($url,'SELECT * FROM PERSON WHERE NAME = ?',['John'])", + assertFails( + db, + "CALL apoc.load.jdbc($url,'SELECT * FROM PERSON WHERE NAME = ?',['John'])", Map.of("url", url), - errorMsgWithObfuscatedUrl - ); - - // obfuscated log in `debug.log` + errorMsgWithObfuscatedUrl); + + // obfuscated log in `debug.log` String fileContent = getLogFileContent(); - assertTrue("Actual log content is: " + fileContent, - fileContent.contains(errorMsgWithObfuscatedUrl) - ); + assertTrue("Actual log content is: " + fileContent, fileContent.contains(errorMsgWithObfuscatedUrl)); } @Test diff --git a/full/src/test/java/apoc/load/LoadLdapTest.java b/full/src/test/java/apoc/load/LoadLdapTest.java index 3151646cbf..e7b8e04f16 100644 --- a/full/src/test/java/apoc/load/LoadLdapTest.java +++ b/full/src/test/java/apoc/load/LoadLdapTest.java @@ -26,14 +26,10 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import apoc.util.FileUtils; import apoc.util.TestUtil; import com.novell.ldap.LDAPEntry; import com.novell.ldap.LDAPSearchResults; import com.unboundid.ldap.sdk.LDAPConnection; -import java.io.File; -import java.io.IOException; -import java.nio.file.Files; import java.util.List; import java.util.Map; import org.junit.AfterClass; diff --git a/full/src/test/java/apoc/periodic/PeriodicExtendedTest.java b/full/src/test/java/apoc/periodic/PeriodicExtendedTest.java index 5bfd1aab08..d706b725ad 100644 --- a/full/src/test/java/apoc/periodic/PeriodicExtendedTest.java +++ b/full/src/test/java/apoc/periodic/PeriodicExtendedTest.java @@ -32,7 +32,6 @@ import apoc.nlp.gcp.GCPProcedures; import apoc.nodes.NodesExtended; import apoc.util.TestUtil; -import java.sql.SQLException; import java.util.Collections; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -222,27 +221,6 @@ public void testIterateErrors() { }); } - @Test - public void testIterateJDBC() { - TestUtil.ignoreException( - () -> { - testResult( - db, - "CALL apoc.periodic.iterate('call apoc.load.jdbc(\"jdbc:mysql://localhost:3306/northwind?user=root\",\"customers\")', 'create (c:Customer) SET c += $row', {batchSize:10,parallel:true})", - result -> { - Map row = Iterators.single(result); - assertEquals(3L, row.get("batches")); - assertEquals(29L, row.get("total")); - }); - - testCall( - db, - "MATCH (p:Customer) return count(p) as count", - row -> assertEquals(29L, row.get("count"))); - }, - SQLException.class); - } - @Test public void testRock_n_roll_while() { // setup diff --git a/full/src/test/java/apoc/util/ExtendedTestUtil.java b/full/src/test/java/apoc/util/ExtendedTestUtil.java index bbebb0b88f..91f712677d 100644 --- a/full/src/test/java/apoc/util/ExtendedTestUtil.java +++ b/full/src/test/java/apoc/util/ExtendedTestUtil.java @@ -74,7 +74,8 @@ public static void testResultEventually( TimeUnit.SECONDS); } - public static void assertFails(GraphDatabaseService db, String query, Map params, String expectedErrMsg) { + public static void assertFails( + GraphDatabaseService db, String query, Map params, String expectedErrMsg) { try { testCall(db, query, params, r -> fail("Should fail due to " + expectedErrMsg)); } catch (Exception e) {