[NOID] Fixex #4256: Obfuscate JDBC Password in query.log

neo4j-contrib · Dec 17, 2024 · 4956244 · 4956244
1 parent be96c18
commit 4956244
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 34 deletions.
diff --git 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
@@ -131,17 +131,7 @@ private Stream<RowResult> 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<RowResult> 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
@@ -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
@@ -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
@@ -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<String,Object> 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);
+        }
+    }
 }