Backport PR2148 into 42.2.x (#2254)

* docs: Add description of connection property logServerErrorDetail

* fix: Avoid leaking error message details in JDBC batches when logServerErrorDetail is disabled

* test: Add a test case for logServerErrorDetail with JDBC batches

Co-authored-by: Frode Carlsen <frode.odde.carlsen@nav.no>
Co-authored-by: Sehrope Sarkuni <sehrope@jackdb.com>

CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 ### Added
 
 ### Fixed
+- Fix "Avoid leaking server error details through BatchUpdateException when logServerErrorDetail=false" [PR #2148](https://github.com/pgjdbc/pgjdbc/pull/2148) fixes Issue #2147
 
 [42.2.23] (2021-07-06 09:17:32 -0400)
 

README.md

@@ -125,6 +125,7 @@ In addition to the standard connection parameters the driver supports a number o
 | receiveBufferSize             | Integer | -1      | Socket read buffer size  |
 | loggerLevel                   | String  | null    | Logger level of the driver using java.util.logging. Allowed values: OFF, DEBUG or TRACE. |
 | loggerFile                    | String  | null    | File name output of the Logger, if set, the Logger will use a FileHandler to write to a specified file. If the parameter is not set or the file can't be created the ConsoleHandler will be used instead. |
+| logServerErrorDetail          | Boolean | true    | Allows server error detail (such as sql statements and values) to be logged and passed on in exceptions.  Setting to false will mask these errors so they won't be exposed to users, or logs. |
 | allowEncodingChanges          | Boolean | false   | Allow for changes in client_encoding |
 | logUnclosedConnections        | Boolean | false   | When connections that are not explicitly closed are garbage collected, log the stacktrace from the opening of the connection to trace the leak source |
 | binaryTransferEnable          | String  | ""      | Comma separated list of types to enable binary transfer. Either OID numbers or names |

docs/documentation/head/connect.md

@@ -540,7 +540,14 @@ Connection conn = DriverManager.getConnection(url);
     A limit during setting of property is 90% of max heap memory. All given values, which gonna be higher than limit, gonna lowered to the limit.
 
 	By default, maxResultBuffer is not set (is null), what means that reading of results gonna be performed without limits.
-
+
+* **logServerErrorDetail** == boolean
+
+    Whether to include server error details in exceptions and log messages (for example inlined query parameters). 
+	Setting to false will only include minimal, not sensitive messages.
+
+	By default this is set to true, server error details are propagated. This may include sensitive details such as query parameters.
+
 <a name="unix sockets"></a>
 ## Unix sockets
 

pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java

@@ -228,4 +228,11 @@ CachedQuery createQuery(String sql, boolean escapeProcessing, boolean isParamete
    * @throws SQLException if the class cannot be found or instantiated.
    */
   PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException;
+
+  /**
+   * Indicates if error details from server used in included in logging and exceptions.
+   *
+   * @return true if should be included and passed on to other exceptions
+   */
+  boolean getLogServerErrorDetail();
 }

pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java

@@ -151,9 +151,11 @@ public void handleError(SQLException newError) {
       }
 
       String queryString = "<unknown>";
-      if (resultIndex < queries.length) {
-        queryString = queries[resultIndex].toString(
-            parameterLists == null ? null : parameterLists[resultIndex]);
+      if (pgStatement.getPGConnection().getLogServerErrorDetail()) {
+        if (resultIndex < queries.length) {
+          queryString = queries[resultIndex].toString(
+             parameterLists == null ? null : parameterLists[resultIndex]);
+        }
       }
 
       BatchUpdateException batchException;

pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java

@@ -144,6 +144,8 @@ private enum ReadOnlyBehavior {
   private boolean readOnly = false;
   // Filter out database objects for which the current user has no privileges granted from the DatabaseMetaData
   private boolean  hideUnprivilegedObjects ;
+  // Whether to include error details in logging and exceptions
+  private final boolean logServerErrorDetail;
   // Bind String to UNSPECIFIED or VARCHAR?
   private final boolean bindStringAsVarchar;
 
@@ -297,6 +299,7 @@ public PgConnection(HostSpec[] hostSpecs,
     if (PGProperty.LOG_UNCLOSED_CONNECTIONS.getBoolean(info)) {
       openStackTrace = new Throwable("Connection was created at this point:");
     }
+    this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info);
     this.disableColumnSanitiser = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info);
 
     if (haveMinimumServerVersion(ServerVersion.v8_3)) {
@@ -1533,6 +1536,10 @@ public <T> T createQueryObject(Class<T> ifc) throws SQLException {
     throw org.postgresql.Driver.notImplemented(this.getClass(), "createQueryObject(Class<T>)");
   }
 
+  public boolean getLogServerErrorDetail() {
+    return logServerErrorDetail;
+  }
+
   @Override
   public boolean isWrapperFor(Class<?> iface) throws SQLException {
     checkClosed();

pgjdbc/src/test/java/org/postgresql/jdbc/AbstractArraysTest.java

@@ -942,5 +942,13 @@ public PGXmlFactoryFactory getXmlFactoryFactory() throws SQLException {
     public boolean hintReadOnly() {
       return false;
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public boolean getLogServerErrorDetail() {
+      return false;
+    }
   }
 }

pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java

@@ -15,6 +15,7 @@
 import org.junit.Test;
 
 import java.sql.Connection;
+import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.Properties;
 
@@ -34,15 +35,22 @@ public class LogServerMessagePropertyTest {
    * create a temp table with a primary key, run two inserts to generate
    * a duplicate key error, and finally return the exception message.
    */
-  private static String testViolatePrimaryKey(Properties props) throws SQLException {
+  private static String testViolatePrimaryKey(Properties props, boolean batch) throws SQLException {
     Connection conn = TestUtil.openDB(props);
     Assume.assumeTrue(TestUtil.haveMinimumServerVersion(conn, ServerVersion.v9_1));
     try {
       TestUtil.execute(CREATE_TABLE_SQL, conn);
-      // First insert should work
-      TestUtil.execute(INSERT_SQL, conn);
-      // Second insert should throw a duplicate key error
-      TestUtil.execute(INSERT_SQL, conn);
+      if (batch) {
+        PreparedStatement stmt = conn.prepareStatement(INSERT_SQL);
+        stmt.addBatch();
+        stmt.addBatch();
+        stmt.executeBatch();
+      } else {
+        // First insert should work
+        TestUtil.execute(INSERT_SQL, conn);
+        // Second insert should throw a duplicate key error
+        TestUtil.execute(INSERT_SQL, conn);
+      }
     } catch (SQLException e) {
       Assert.assertEquals("SQL state must be for a unique violation", PSQLState.UNIQUE_VIOLATION.getState(), e.getSQLState());
       return e.getMessage();
@@ -54,6 +62,10 @@ private static String testViolatePrimaryKey(Properties props) throws SQLExceptio
     return null;
   }
 
+  private static String testViolatePrimaryKey(Properties props) throws SQLException {
+    return testViolatePrimaryKey(props, false);
+  }
+
   private static void assertMessageContains(String message, String text) {
     if (message.toLowerCase().indexOf(text.toLowerCase()) < 0) {
       Assert.fail(String.format("Message must contain text '%s': %s", text, message));
@@ -97,4 +109,36 @@ public void testWithLogServerErrorDetailDisabled() throws SQLException {
     assertMessageDoesNotContain(message, "Detail:");
     assertMessageDoesNotContain(message, SECRET_VALUE);
   }
+
+  @Test
+  public void testBatchWithDefaults() throws SQLException {
+    Properties props = new Properties();
+    String message = testViolatePrimaryKey(props, true);
+    assertMessageContains(message, PRIMARY_KEY_NAME);
+    assertMessageContains(message, "Detail:");
+    assertMessageContains(message, SECRET_VALUE);
+  }
+
+  /**
+   * NOTE: This should be the same as the default case as "true" is the default.
+   */
+  @Test
+  public void testBatchExplicitlyEnabled() throws SQLException {
+    Properties props = new Properties();
+    props.setProperty(PGProperty.LOG_SERVER_ERROR_DETAIL.getName(), "true");
+    String message = testViolatePrimaryKey(props, true);
+    assertMessageContains(message, PRIMARY_KEY_NAME);
+    assertMessageContains(message, "Detail:");
+    assertMessageContains(message, SECRET_VALUE);
+  }
+
+  @Test
+  public void testBatchWithLogServerErrorDetailDisabled() throws SQLException {
+    Properties props = new Properties();
+    props.setProperty(PGProperty.LOG_SERVER_ERROR_DETAIL.getName(), "false");
+    String message = testViolatePrimaryKey(props, true);
+    assertMessageContains(message, PRIMARY_KEY_NAME);
+    assertMessageDoesNotContain(message, "Detail:");
+    assertMessageDoesNotContain(message, SECRET_VALUE);
+  }
 }