Fixex #4256: Obfuscate JDBC Password in query.log (#4261)

vga91 · web-flow · commit 32192dbcfee8 · 2024-12-06T15:37:02.000+01:00
diff --git a/extended/src/main/java/apoc/load/Jdbc.java b/extended/src/main/java/apoc/load/Jdbc.java
@@ -14,16 +14,31 @@
 
 import java.math.BigDecimal;
 import java.math.BigInteger;
-import java.sql.*;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import java.sql.Types;
 import java.time.LocalDateTime;
 import java.time.OffsetDateTime;
 import java.time.OffsetTime;
 import java.time.ZoneId;
-import java.util.*;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Spliterator;
+import java.util.Spliterators;
+import java.util.UUID;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
-import static apoc.load.util.JdbcUtil.*;
+import static apoc.load.util.JdbcUtil.getConnection;
+import static apoc.load.util.JdbcUtil.getSqlOrKey;
+import static apoc.load.util.JdbcUtil.getUrlOrKey;
+import static apoc.load.util.JdbcUtil.obfuscateJdbcUrl;
 
 /**
  * @author mh
@@ -96,10 +111,7 @@ private Stream<RowResult> executeQuery(String urlOrKey, String tableOrSelect, Ma
                 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);
         }
     }
 
@@ -134,13 +146,24 @@ private Stream<RowResult> executeUpdate(String urlOrKey, String query, Map<Strin
                 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/extended/src/main/java/apoc/load/util/JdbcUtil.java b/extended/src/main/java/apoc/load/util/JdbcUtil.java
@@ -81,4 +81,8 @@ public static String getUrlOrKey(String urlOrKey) {
     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/extended/src/test/java/apoc/load/JdbcTest.java b/extended/src/test/java/apoc/load/JdbcTest.java
@@ -5,13 +5,19 @@
 import apoc.util.TestUtil;
 import apoc.util.Util;
 import apoc.util.collection.Iterators;
-import org.junit.*;
-import org.junit.jupiter.api.AfterAll;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
 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.test.rule.DbmsRule;
-import org.neo4j.test.rule.ImpermanentDbmsRule;
+import org.neo4j.test.TestDatabaseManagementServiceBuilder;
 
 import java.lang.reflect.InvocationTargetException;
 import java.sql.Connection;
@@ -25,15 +31,21 @@
 import java.util.Map;
 
 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;
 
 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;
 
@@ -47,6 +59,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 = ?");
@@ -77,11 +92,6 @@ public void tearDown() throws SQLException {
         System.clearProperty("derby.user.apoc");
     }
 
-    @AfterAll
-    public void tearDownAll() {
-        db.shutdown();
-    }
-
     @Test
     public void testLoadJdbc() throws Exception {
         testCall(db, "CALL apoc.load.jdbc('jdbc:derby:derbyDB','PERSON')",
@@ -111,6 +121,21 @@ 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` 
+        assertTrue(getLogFileContent().contains(errorMsgWithObfuscatedUrl));
+    }
+
     @Test
     public void testLoadJdbcParamsWithConfigLocalDateTime() throws Exception {
         testCall(db, "CALL apoc.load.jdbc('jdbc:derby:derbyDB','SELECT * FROM PERSON WHERE NAME = ?',['John'])",
diff --git a/extended/src/test/java/apoc/load/LoadLdapTest.java b/extended/src/test/java/apoc/load/LoadLdapTest.java
@@ -1,7 +1,6 @@
 package apoc.load;
 
 
-import apoc.util.FileUtils;
 import apoc.util.TestUtil;
 import com.unboundid.ldap.sdk.LDAPConnection;
 import com.unboundid.ldap.sdk.SearchResult;
@@ -18,13 +17,11 @@
 import org.zapodot.junit.ldap.EmbeddedLdapRule;
 import org.zapodot.junit.ldap.EmbeddedLdapRuleBuilder;
 
-import java.io.File;
-import java.io.IOException;
-import java.nio.file.Files;
 import java.util.List;
 import java.util.Map;
 
 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;
@@ -96,15 +93,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 = "%s %s %s".formatted(
diff --git a/extended/src/test/java/apoc/util/ExtendedTestUtil.java b/extended/src/test/java/apoc/util/ExtendedTestUtil.java
@@ -12,6 +12,9 @@
 import org.neo4j.internal.helpers.collection.Iterators;
 import org.neo4j.test.assertion.Assert;
 
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -141,4 +144,13 @@ public static void assertFails(GraphDatabaseService db, String query, Map<String
             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);
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -81,4 +81,8 @@ public static String getUrlOrKey(String urlOrKey) {`
`81`	`81`	`public static String getSqlOrKey(String sqlOrKey) {`
`82`	`82`	`return sqlOrKey.contains(" ") ? sqlOrKey : Util.getLoadUrlByConfigFile(LOAD_TYPE, sqlOrKey, "sql").orElse("SELECT * FROM " + sqlOrKey);`
`83`	`83`	`}`
	`84`	`+`
	`85`	`+ public static String obfuscateJdbcUrl(String query) {`
	`86`	`+ return query.replaceAll("(jdbc:[^:]+://)([^\\s\\\"']+)", "$1*******");`
	`87`	`+ }`
`84`	`88`	`}`