Repository: pgjdbc/pgjdbc
Pull request #1579 (2019-10-11 21:16:48)

Add connection property to limit server error detail in exception exceptions
This patch keeps the old behavior but adds a new connection property to disable error detail. It defaults to "true" (the old behavior).

RFC for:

* A better name for the property?
* Whether we really need the single-argument constructor for `PSQLException(ServerErrorMessage)`? (i.e. is the constructor part of our public API, if not remove it to ensure we don't accidentally create it without the option)
* Whether the non-sensitive message should include more than just the SEVERITY + MESSAGE?
* Should we go to the next level and instead add a class-name style property that can customize the error, i.e. instantiate it as an object and call `.getMessage(ServerErrorMessage)` to get the text? (Seems like overkill but would give users option to format things however they'd like)

Consider this patch a WIP / proof-of-concept until those are addressed.

Also would need some tests to actually try out enabling and disabling things and verifying they're not included.

Also I did a quick compile check and ran a handful of tests but not the entire suite yet. I doubt the error messages break anything but if so we'll need to address those too...

AppVeyorBot: :x: [Build pgjdbc 1.0.470 failed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28055357) (commit https://github.com/pgjdbc/pgjdbc/commit/e15f280779 by @sehrope)
davecramer: This one looks better. Seems that the datasource also needs a getter/setter.

Thanks for working on this.
AppVeyorBot: :white_check_mark: [Build pgjdbc 1.0.474 completed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28065487) (commit https://github.com/pgjdbc/pgjdbc/commit/6afde19dc2 by @sehrope)
davecramer: @sehrope is this ready ?
sehrope: Never got any feedback to change the naming etc so I suppose it's fine. I'll try to add at least one test for it today and then it should be good.
davecramer: Excellent, thanks
sehrope: Rebased and added a test for the new property. Works on later versions of PG, hopefully it passes on all the older versions too. Tried to keep the error message checking generic enough that it should. Let's see what Travis says...
AppVeyorBot: :white_check_mark: [Build pgjdbc 1.0.506 completed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28837719) (commit https://github.com/pgjdbc/pgjdbc/commit/b7ba864181 by @sehrope)
sehrope: Looks like it was good on everything except 8.4 as the older server versions do not include the full details in the error response. I force pushed an update to skip test unless it's 9.1+. Once it passes should be good to merge.
AppVeyorBot: :white_check_mark: [Build pgjdbc 1.0.507 completed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28841389) (commit https://github.com/pgjdbc/pgjdbc/commit/73d0369da9 by @sehrope)
davecramer: @sehrope I can't seem to find any code that actually looks for the version?
sehrope: @davecramer Because it's not... I added the check on line 38 but didn't wrap it in an `assumeTrue(...)` to actually skip the test. I'll fix it in a min.
AppVeyorBot: :x: [Build pgjdbc 1.0.510 failed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28856322) (commit https://github.com/pgjdbc/pgjdbc/commit/b837e71f3f by @sehrope)
sehrope: So Travis passes and it correctly skips 8.4, but looks like it failed on some unrelated race condition on AppVeyor: https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28856322/job/cyyur72crn2t7ohl#L306

I say this is good to go and will take a peek at that unrelated race condition some time soon.

Patch:
diff --git a/pgjdbc/src/main/java/org/postgresql/PGProperty.java b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
index b342b44c..9e99dc7a 100644
--- a/pgjdbc/src/main/java/org/postgresql/PGProperty.java
+++ b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
@@ -172,6 +172,12 @@ public enum PGProperty {
   LOG_UNCLOSED_CONNECTIONS("logUnclosedConnections", "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"),
 
+  /**
+   * Whether to include full server error detail in exception messages.
+   */
+  LOG_SERVER_ERROR_DETAIL("logServerErrorDetail", "true",
+      "Include full server error detail in exception messages. If disabled then only the error itself will be included."),
+
   /**
    * Enable optimization that disables column name sanitiser.
    */
diff --git a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java
index fbe5c498..1a74a7f7 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java
@@ -45,6 +45,7 @@ public abstract class QueryExecutorBase implements QueryExecutor {
   private final PreferQueryMode preferQueryMode;
   private AutoSave autoSave;
   private boolean flushCacheOnDeallocate = true;
+  protected final boolean logServerErrorDetail;
 
   // default value for server versions that don't report standard_conforming_strings
   private boolean standardConformingStrings = false;
@@ -70,6 +71,7 @@ public abstract class QueryExecutorBase implements QueryExecutor {
     String preferMode = PGProperty.PREFER_QUERY_MODE.get(info);
     this.preferQueryMode = PreferQueryMode.of(preferMode);
     this.autoSave = AutoSave.of(PGProperty.AUTOSAVE.get(info));
+    this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info);
     this.cachedQueryCreateAction = new CachedQueryCreateAction(this);
     statementCache = new LruCache<Object, CachedQuery>(
         Math.max(0, PGProperty.PREPARED_STATEMENT_CACHE_QUERIES.getInt(info)),
diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
index 662cc263..1efcd527 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
@@ -517,7 +517,7 @@ public class ConnectionFactoryImpl extends ConnectionFactory {
             ServerErrorMessage errorMsg =
                 new ServerErrorMessage(pgStream.receiveErrorString(elen - 4));
             LOGGER.log(Level.FINEST, " <=BE ErrorMessage({0})", errorMsg);
-            throw new PSQLException(errorMsg);
+            throw new PSQLException(errorMsg, PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info));
 
           case 'R':
             // Authentication request.
@@ -647,7 +647,8 @@ public class ConnectionFactoryImpl extends ConnectionFactory {
                   org.postgresql.gss.MakeGSS.authenticate(pgStream, host, user, password,
                       PGProperty.JAAS_APPLICATION_NAME.get(info),
                       PGProperty.KERBEROS_SERVER_NAME.get(info), usespnego,
-                      PGProperty.JAAS_LOGIN.getBoolean(info));
+                      PGProperty.JAAS_LOGIN.getBoolean(info),
+                      PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info));
                 }
                 break;
 
diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
index 0b889022..48ca673b 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@@ -2500,7 +2500,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
       LOGGER.log(Level.FINEST, " <=BE ErrorMessage({0})", errorMsg.toString());
     }
 
-    PSQLException error = new PSQLException(errorMsg);
+    PSQLException error = new PSQLException(errorMsg, this.logServerErrorDetail);
     if (transactionFailCause == null) {
       transactionFailCause = error;
     } else {
diff --git a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
index 64e9f04b..f4be8197 100644
--- a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
+++ b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
@@ -928,6 +928,22 @@ public abstract class BaseDataSource implements CommonDataSource, Referenceable
     PGProperty.LOG_UNCLOSED_CONNECTIONS.set(properties, enabled);
   }
 
+  /**
+   * @return true if driver should log include detail in server error messages
+   * @see PGProperty#LOG_SERVER_ERROR_DETAIL
+   */
+  public boolean getLogServerErrorDetail() {
+    return PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(properties);
+  }
+
+  /**
+   * @param enabled true if driver should include detail in server error messages
+   * @see PGProperty#LOG_SERVER_ERROR_DETAIL
+   */
+  public void setLogServerErrorDetail(boolean enabled) {
+    PGProperty.LOG_SERVER_ERROR_DETAIL.set(properties, enabled);
+  }
+
   /**
    * @return assumed minimal server version
    * @see PGProperty#ASSUME_MIN_SERVER_VERSION
diff --git a/pgjdbc/src/main/java/org/postgresql/gss/GssAction.java b/pgjdbc/src/main/java/org/postgresql/gss/GssAction.java
index fa27e30d..f747fd4a 100644
--- a/pgjdbc/src/main/java/org/postgresql/gss/GssAction.java
+++ b/pgjdbc/src/main/java/org/postgresql/gss/GssAction.java
@@ -32,15 +32,17 @@ class GssAction implements PrivilegedAction<Exception> {
   private final String kerberosServerName;
   private final boolean useSpnego;
   private final GSSCredential clientCredentials;
+  private final boolean logServerErrorDetail;
 
   GssAction(PGStream pgStream, GSSCredential clientCredentials, String host, String user,
-      String kerberosServerName, boolean useSpnego) {
+      String kerberosServerName, boolean useSpnego, boolean logServerErrorDetail) {
     this.pgStream = pgStream;
     this.clientCredentials = clientCredentials;
     this.host = host;
     this.user = user;
     this.kerberosServerName = kerberosServerName;
     this.useSpnego = useSpnego;
+    this.logServerErrorDetail = logServerErrorDetail;
   }
 
   private static boolean hasSpnegoSupport(GSSManager manager) throws GSSException {
@@ -111,7 +113,7 @@ class GssAction implements PrivilegedAction<Exception> {
 
               LOGGER.log(Level.FINEST, " <=BE ErrorMessage({0})", errorMsg);
 
-              return new PSQLException(errorMsg);
+              return new PSQLException(errorMsg, logServerErrorDetail);
             case 'R':
               LOGGER.log(Level.FINEST, " <=BE AuthenticationGSSContinue");
               int len = pgStream.receiveInteger4();
diff --git a/pgjdbc/src/main/java/org/postgresql/gss/MakeGSS.java b/pgjdbc/src/main/java/org/postgresql/gss/MakeGSS.java
index 84c2f35f..b3b856b4 100644
--- a/pgjdbc/src/main/java/org/postgresql/gss/MakeGSS.java
+++ b/pgjdbc/src/main/java/org/postgresql/gss/MakeGSS.java
@@ -28,7 +28,8 @@ public class MakeGSS {
   private static final Logger LOGGER = Logger.getLogger(MakeGSS.class.getName());
 
   public static void authenticate(PGStream pgStream, String host, String user, String password,
-      String jaasApplicationName, String kerberosServerName, boolean useSpnego, boolean jaasLogin)
+      String jaasApplicationName, String kerberosServerName, boolean useSpnego, boolean jaasLogin,
+      boolean logServerErrorDetail)
           throws IOException, SQLException {
     LOGGER.log(Level.FINEST, " <=BE AuthenticationReqGSS");
 
@@ -58,7 +59,7 @@ public class MakeGSS {
         sub = lc.getSubject();
       }
       PrivilegedAction<Exception> action = new GssAction(pgStream, gssCredential, host, user,
-          kerberosServerName, useSpnego);
+          kerberosServerName, useSpnego, logServerErrorDetail);
 
       result = Subject.doAs(sub, action);
     } catch (Exception e) {
diff --git a/pgjdbc/src/main/java/org/postgresql/util/PSQLException.java b/pgjdbc/src/main/java/org/postgresql/util/PSQLException.java
index 370ce8a9..0e3cd476 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/PSQLException.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/PSQLException.java
@@ -20,7 +20,11 @@ public class PSQLException extends SQLException {
   }
 
   public PSQLException(ServerErrorMessage serverError) {
-    super(serverError.toString(), serverError.getSQLState());
+    this(serverError, true);
+  }
+
+  public PSQLException(ServerErrorMessage serverError, boolean detail) {
+    super(detail ? serverError.toString() : serverError.getNonSensitiveErrorMessage(), serverError.getSQLState());
     this.serverError = serverError;
   }
 
diff --git a/pgjdbc/src/main/java/org/postgresql/util/ServerErrorMessage.java b/pgjdbc/src/main/java/org/postgresql/util/ServerErrorMessage.java
index d44b464d..7b3f62c2 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/ServerErrorMessage.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/ServerErrorMessage.java
@@ -143,6 +143,19 @@ public class ServerErrorMessage implements Serializable {
     return Integer.parseInt(s);
   }
 
+  String getNonSensitiveErrorMessage() {
+    StringBuilder totalMessage = new StringBuilder();
+    String message = mesgParts.get(SEVERITY);
+    if (message != null) {
+      totalMessage.append(message).append(": ");
+    }
+    message = mesgParts.get(MESSAGE);
+    if (message != null) {
+      totalMessage.append(message);
+    }
+    return totalMessage.toString();
+  }
+
   public String toString() {
     // Now construct the message from what the server sent
     // The general format is:


Test patch:
diff --git a/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java b/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java
new file mode 100644
index 00000000..daab6935
--- /dev/null
+++ b/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.test.core;
+
+import org.postgresql.PGProperty;
+import org.postgresql.core.ServerVersion;
+import org.postgresql.test.TestUtil;
+
+import org.junit.Assert;
+import org.junit.Assume;
+import org.junit.Test;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.Properties;
+
+public class LogServerMessagePropertyTest {
+  private static final String PRIMARY_KEY_NAME = "lms_test_pk";
+  private static final String CREATE_TABLE_SQL =
+      "CREATE TABLE pg_temp.lms_test ("
+      + "  id text, "
+      + "  CONSTRAINT " + PRIMARY_KEY_NAME + " PRIMARY KEY (id)"
+      + ")";
+  private static final String SECRET_VALUE = "some_secret_value";
+  private static final String INSERT_SQL =
+      "INSERT INTO pg_temp.lms_test (id) VALUES ('" + SECRET_VALUE + "')";
+  private static final String UNIQUE_VIOLATION_SQL_STATE = "23505";
+
+  /**
+   * Creates a connection with the additional properties, use it to
+   * 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 {
+    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);
+    } catch (SQLException e) {
+      Assert.assertEquals("SQL state must be for a unique violation", UNIQUE_VIOLATION_SQL_STATE, e.getSQLState());
+      return e.getMessage();
+    } finally {
+      conn.close();
+    }
+    // Should never get here:
+    Assert.fail("A duplicate key exception should have occurred");
+    return null;
+  }
+
+  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));
+    }
+  }
+
+  private static void assertMessageDoesNotContain(String message, String text) {
+    if (message.toLowerCase().indexOf(text.toLowerCase()) >= 0) {
+      Assert.fail(String.format("Message must not contain text '%s': %s", text, message));
+    }
+  }
+
+  @Test
+  public void testWithDefaults() throws SQLException {
+    Properties props = new Properties();
+    String message = testViolatePrimaryKey(props);
+    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 testWithExplicitlyEnabled() throws SQLException {
+    Properties props = new Properties();
+    props.setProperty(PGProperty.LOG_SERVER_ERROR_DETAIL.getName(), "true");
+    String message = testViolatePrimaryKey(props);
+    assertMessageContains(message, PRIMARY_KEY_NAME);
+    assertMessageContains(message, "Detail:");
+    assertMessageContains(message, SECRET_VALUE);
+  }
+
+  @Test
+  public void testWithLogServerErrorDetailDisabled() throws SQLException {
+    Properties props = new Properties();
+    props.setProperty(PGProperty.LOG_SERVER_ERROR_DETAIL.getName(), "false");
+    String message = testViolatePrimaryKey(props);
+    assertMessageContains(message, PRIMARY_KEY_NAME);
+    assertMessageDoesNotContain(message, "Detail:");
+    assertMessageDoesNotContain(message, SECRET_VALUE);
+  }
+}
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
index 01d9fbf9..7dc50232 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
@@ -15,6 +15,7 @@ import org.postgresql.core.v3.V3ParameterListTests;
 import org.postgresql.jdbc.DeepBatchedInsertStatementTest;
 import org.postgresql.jdbc.PrimitiveArraySupportTest;
 import org.postgresql.test.core.JavaVersionTest;
+import org.postgresql.test.core.LogServerMessagePropertyTest;
 import org.postgresql.test.core.NativeQueryBindLengthTest;
 import org.postgresql.test.core.OptionsPropertyTest;
 import org.postgresql.test.util.ExpressionPropertiesTest;
@@ -67,6 +68,7 @@ import org.junit.runners.Suite;
     JavaVersionTest.class,
     JBuilderTest.class,
     LoginTimeoutTest.class,
+    LogServerMessagePropertyTest.class,
     LruCacheTest.class,
     MiscTest.class,
     NativeQueryBindLengthTest.class,