Avoid leaking server error details through BatchUpdateException when logServerErrorDetail=false

CHANGELOG.md

@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 - Rework OSGi bundle activator so it does not rely on exception message to check DataSourceFactory presence PR [#507](https://github.com/pgjdbc/pgjdbc/pull/507)
+- 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.20] (2021-04-19)
 
@@ -53,6 +54,7 @@ is executed a second time it will fail.
 - Fix DatabaseMetaData.getTablePrivileges() to include views, materialized views, and foreign tables [PR #2049](https://github.com/pgjdbc/pgjdbc/pull/2049)
 - Fix Resolve ParseError in PGtokenizer fixes #2050
 - Fix return metadata privileges for views and foreign tables
+- DatabaseMetaData.getTables returns columns in UPPER case as per the spec [PR #2092](https://github.com/pgjdbc/pgjdbc/pull/2092) fixes Issue #830
 
 ## [42.2.18]
 ### Fixed

README.md

@@ -104,6 +104,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

@@ -564,9 +564,16 @@ Connection conn = DriverManager.getConnection(url);
 
 	Specifies the highest number of rows which can be calculated by `adaptiveFetch`.
     Requires `adaptiveFetch` set to true to work.
-        
+
     By default, maximum of rows calculated by `adaptiveFetch` is -1, which is understood as infinite.
-
+
+* **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)) {
@@ -1534,6 +1537,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

@@ -959,5 +959,12 @@ public boolean getAdaptiveFetch() {
       throw new UnsupportedOperationException();
     }
 
+    /**
+     * {@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);
+  }
 }