From c3e434d0694166e1abde22d797686d80edbf30de Mon Sep 17 00:00:00 2001 From: Aron Roberts Date: Mon, 25 Mar 2013 20:24:11 -0700 Subject: [PATCH] CSPACE-5943: Use prepared statements, rather than dynamically-build SQL that accepts user-supplied input. --- .../AuthorityItemDocumentModelHandler.java | 4 + .../services/common/storage/JDBCTools.java | 48 ++++++++-- .../storage/PreparedStatementBuilder.java | 37 ++++++++ .../client/java/RepositoryJavaClientImpl.java | 88 ++++++------------- 4 files changed, 111 insertions(+), 66 deletions(-) create mode 100644 services/common/src/main/java/org/collectionspace/services/common/storage/PreparedStatementBuilder.java diff --git a/services/authority/service/src/main/java/org/collectionspace/services/common/vocabulary/nuxeo/AuthorityItemDocumentModelHandler.java b/services/authority/service/src/main/java/org/collectionspace/services/common/vocabulary/nuxeo/AuthorityItemDocumentModelHandler.java index d54ca7a6f..007dded89 100644 --- a/services/authority/service/src/main/java/org/collectionspace/services/common/vocabulary/nuxeo/AuthorityItemDocumentModelHandler.java +++ b/services/authority/service/src/main/java/org/collectionspace/services/common/vocabulary/nuxeo/AuthorityItemDocumentModelHandler.java @@ -153,6 +153,10 @@ public abstract class AuthorityItemDocumentModelHandler public void setInAuthority(String inAuthority) { this.inAuthority = inAuthority; } + + public String getInAuthority() { + return this.inAuthority; + } /** Subclasses may override this to customize the URI segment. */ public String getAuthorityServicePath() { diff --git a/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java b/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java index 8f58bd635..09b662ee4 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java +++ b/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java @@ -30,6 +30,7 @@ import javax.sql.DataSource; import java.sql.DatabaseMetaData; import java.sql.Connection; import java.sql.DriverManager; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; @@ -55,6 +56,8 @@ public class JDBCTools { public static String NUXEO_MANAGER_DATASOURCE_NAME = "NuxeoMgrDS"; public static String NUXEO_READER_DATASOURCE_NAME = "NuxeoReaderDS"; public static String NUXEO_USER_NAME = "nuxeo"; + public static String SQL_WILDCARD = "%"; + // // Private constants // @@ -175,14 +178,13 @@ public class JDBCTools { conn = getConnection(dataSourceName, repositoryName); stmt = conn.createStatement(); - RowSetFactory rowSetFactory = RowSetProvider.newFactory(); + RowSetFactory rowSetFactory = RowSetProvider.newFactory(); CachedRowSet crs = rowSetFactory.createCachedRowSet(); stmt = conn.createStatement(); try (ResultSet resultSet = stmt.executeQuery(sql)) { crs.populate(resultSet); } - stmt.close(); // also closes resultSet return crs; } catch (SQLException sqle) { SQLException tempException = sqle; @@ -193,11 +195,43 @@ public class JDBCTools { throw new RuntimeException("SQL problem in executeQuery: ", sqle); } finally { try { + if (stmt != null) { + stmt.close(); + } if (conn != null) { conn.close(); } - if (stmt != null) { - stmt.close(); + } catch (SQLException sqle) { + logger.debug("SQL Exception closing statement/connection in executeQuery: " + sqle.getLocalizedMessage()); + return null; + } + } + } + + public static CachedRowSet executePreparedQuery(final PreparedStatementBuilder builder, + String dataSourceName, String repositoryName, String sql) throws Exception { + Connection conn = null; + PreparedStatement ps = null; + try { + conn = getConnection(dataSourceName, repositoryName); + RowSetFactory rowSetFactory = RowSetProvider.newFactory(); + CachedRowSet crs = rowSetFactory.createCachedRowSet(); + ps = builder.build(conn); + // FIXME: transition this log statement to DEBUG level when appropriate + if (logger.isInfoEnabled()) { + logger.info("prepared statement=" + ps.toString()); + } + try (ResultSet resultSet = ps.executeQuery()) { + crs.populate(resultSet); + } + return crs; + } finally { + try { + if (ps != null) { + ps.close(); + } + if (conn != null) { + conn.close(); } } catch (SQLException sqle) { logger.debug("SQL Exception closing statement/connection in executeQuery: " + sqle.getLocalizedMessage()); @@ -233,12 +267,12 @@ public class JDBCTools { throw new RuntimeException("SQL problem in executeUpdate: " + msg, sqle); } finally { try { - if (conn != null) { - conn.close(); - } if (stmt != null) { stmt.close(); } + if (conn != null) { + conn.close(); + } } catch (SQLException sqle) { logger.debug("SQL Exception closing statement/connection in executeUpdate: " + sqle.getLocalizedMessage()); return -1; diff --git a/services/common/src/main/java/org/collectionspace/services/common/storage/PreparedStatementBuilder.java b/services/common/src/main/java/org/collectionspace/services/common/storage/PreparedStatementBuilder.java new file mode 100644 index 000000000..7b8f09d67 --- /dev/null +++ b/services/common/src/main/java/org/collectionspace/services/common/storage/PreparedStatementBuilder.java @@ -0,0 +1,37 @@ +package org.collectionspace.services.common.storage; + +import java.sql.Connection; +import java.sql.PreparedStatement; +/** + * Per http://stackoverflow.com/a/7127189 + */ + +import java.sql.SQLException; + +public class PreparedStatementBuilder +{ + private String sql; + + public PreparedStatementBuilder(final String sql) { + this.sql = sql; + } + + protected void preparePrepared(final PreparedStatement preparedStatement) + throws SQLException { + // This virtual method lets us declare how, when we generate our + // PreparedStatement, we want it to be set up. + + // Note that at the time this method is overridden, the + // PreparedStatement has not yet been created. + } + + public PreparedStatement build(final Connection conn) + throws SQLException + { + // Fetch the PreparedStatement + final PreparedStatement returnable = conn.prepareStatement(sql); + // Perform setup directives + preparePrepared(returnable); + return returnable; + } +} \ No newline at end of file diff --git a/services/common/src/main/java/org/collectionspace/services/nuxeo/client/java/RepositoryJavaClientImpl.java b/services/common/src/main/java/org/collectionspace/services/nuxeo/client/java/RepositoryJavaClientImpl.java index 19f7aeccd..60c2af0dd 100644 --- a/services/common/src/main/java/org/collectionspace/services/nuxeo/client/java/RepositoryJavaClientImpl.java +++ b/services/common/src/main/java/org/collectionspace/services/nuxeo/client/java/RepositoryJavaClientImpl.java @@ -18,6 +18,7 @@ package org.collectionspace.services.nuxeo.client.java; import java.io.Serializable; import java.sql.Connection; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; @@ -42,6 +43,7 @@ import org.collectionspace.services.common.context.ServiceContext; import org.collectionspace.services.common.query.QueryContext; import org.collectionspace.services.common.repository.RepositoryClient; import org.collectionspace.services.common.storage.JDBCTools; +import org.collectionspace.services.common.storage.PreparedStatementBuilder; import org.collectionspace.services.lifecycle.TransitionDef; import org.collectionspace.services.nuxeo.util.NuxeoUtils; @@ -100,7 +102,7 @@ public class RepositoryJavaClientImpl implements RepositoryClient queryParams = ctx.getQueryParams(); - String partialTerm = queryParams.getFirst(IQueryManager.SEARCH_TYPE_PARTIALTERM); + final String partialTerm = queryParams.getFirst(IQueryManager.SEARCH_TYPE_PARTIALTERM); // FIXME: Replace this placeholder with an appropriate per-authority value // obtained from the relevant document handler - String termInfoGroupTableName = "loctermgroup"; + final String termGroupTableName = "loctermgroup"; + + // AuthorityItemDocModelHandler authHandler = (AuthorityItemDocModelHandler) handler; // FIXME: Replace this placeholder query with an actual query from CSPACE-5945 - // FIXME: Consider using a prepared statement here + + // IMPORTANT FIXME: Guard against SQL injection attacks, since partialTerm + // is obtained from user-supplied query parameters + // See, for example: http://stackoverflow.com/a/7127189 String sql = - "SELECT DISTINCT hierarchy.id as id " // For debugging add: ", ltg.termdisplayname" + "SELECT DISTINCT hierarchy.id as id " + " FROM hierarchy " + " LEFT JOIN hierarchy h1 " + " ON h1.parentid = hierarchy.id " - + " AND h1.name = 'locations_common:locTermGroupList' " - + " LEFT JOIN loctermgroup ltg " - + " ON ltg.id = h1.id " - + " WHERE (ltg.termdisplayname ILIKE '" + partialTerm + "%')"; - - // Make sure autocommit is off. See: - // http://jdbc.postgresql.org/documentation/80/query.html#query-with-cursor - // http://docs.oracle.com/javase/7/docs/api/java/sql/ResultSet.html - // connection.setAutoCommit(false); - - // FIXME: Add exception handling and 'finally' blocks to ensure we close resources - // FIXME: Identify whether we can piggyback on existing JDBC method(s) in common.storage, - // and if so, add whatever additional functionality may be required to those method(s) - // FIXME: Add pagination handling - - /* - Statement st = connection.createStatement(); - // Enable use of the cursor for pagination - st.setFetchSize(50); - List docIds = new ArrayList(); - ResultSet rs = st.executeQuery(sql); - String id; - while (rs.next()) { - id = rs.getString("id"); - if (Tools.notBlank(id)) { - docIds.add(id); - } - } - rs.close(); - - // Close the statement. - st.close(); - */ + + " LEFT JOIN ? tg " + + " ON tg.id = h1.id " + + " WHERE tg.termdisplayname ILIKE ?"; + + PreparedStatementBuilder partialTermMatchStatementBuilder = new PreparedStatementBuilder(sql){ + @Override + protected void preparePrepared(PreparedStatement preparedStatement) + throws SQLException + { + preparedStatement.setString(1, termGroupTableName); + preparedStatement.setString(2, partialTerm + JDBCTools.SQL_WILDCARD); + }}; List docIds = new ArrayList(); - CachedRowSet crs = null; - try { - crs = JDBCTools.executeQuery(dataSourceName, repositoryName, sql); + try (CachedRowSet crs = JDBCTools.executePreparedQuery(partialTermMatchStatementBuilder, + dataSourceName, repositoryName, sql)) { // If the response to the query is null or contains zero rows, // return an empty list of document models @@ -982,20 +965,8 @@ public class RepositoryJavaClientImpl implements RepositoryClient