diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f49bb5593..7c1735b2ef 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/README.md b/README.md index bc3452f0d9..4986bd1471 100644 --- a/README.md +++ b/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 | diff --git a/docs/documentation/head/connect.md b/docs/documentation/head/connect.md index 659ef498c8..9c8ebdf46f 100644 --- a/docs/documentation/head/connect.md +++ b/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. + ## Unix sockets diff --git a/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java b/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java index d07eaf5de9..01f98632d9 100644 --- a/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java +++ b/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(); } diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java b/pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java index fb4d92996b..d89d169ec3 100644 --- a/pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java +++ b/pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java @@ -151,9 +151,11 @@ public void handleError(SQLException newError) { } String queryString = ""; - 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; diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java index 21ace00455..8e983cff96 100644 --- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java +++ b/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 createQueryObject(Class ifc) throws SQLException { throw org.postgresql.Driver.notImplemented(this.getClass(), "createQueryObject(Class)"); } + public boolean getLogServerErrorDetail() { + return logServerErrorDetail; + } + @Override public boolean isWrapperFor(Class iface) throws SQLException { checkClosed(); diff --git a/pgjdbc/src/test/java/org/postgresql/jdbc/AbstractArraysTest.java b/pgjdbc/src/test/java/org/postgresql/jdbc/AbstractArraysTest.java index f892712b28..b2367fb9f3 100644 --- a/pgjdbc/src/test/java/org/postgresql/jdbc/AbstractArraysTest.java +++ b/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; + } } } diff --git a/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java b/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java index e5de8f248d..badf5a4cae 100644 --- a/pgjdbc/src/test/java/org/postgresql/test/core/LogServerMessagePropertyTest.java +++ b/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); + } }