7

If not, is there a safer way than String concatenation?

Tried this ..

final String CREATE_SCHEMA_SQL = "CREATE SCHEMA IF NOT EXISTS ?";
List<String> schemas = // ["001", "002", "003"];

for (String schema : schemas) {

  try (Connection connection = dataSource.getConnection();
      PreparedStatement createSchemaStatement =
          connection.prepareStatement(CREATE_SCHEMA_SQL)) {

    createSchemaStatement.setString(1, schema);
    createSchemaStatement.executeUpdate();
    
  } catch (SQLException e) {
    throw new RuntimeException("Could not create tenant schema");
  }
}

Produces this error:

org.postgresql.util.PSQLException: ERROR: syntax error at or near "$1"
  Position: 29
    at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2674)
bobbyrne01
  • 6,295
  • 19
  • 80
  • 150
  • 3
    are the schemas user supplied? If they are fixed strings you supply you also know that concatenation is no issue. https://stackoverflow.com/a/43384505/995891 is the issue I think – zapl Oct 04 '22 at 22:58
  • Apparently you can call `toString` on a Postgres `PreparedStatement` and see what its rendered. https://stackoverflow.com/questions/7195855/logging-prepared-sql-statements-in-postgres-jdbc-driver – tgdavies Oct 04 '22 at 23:37
  • This may help: https://stackoverflow.com/questions/17648861/can-we-use-ddl-commands-in-a-prepared-statement-postgresql – tgdavies Oct 04 '22 at 23:44
  • Even if you're sanitizing your input, in general, this screams security vulnerability... – Isaiah van der Elst Oct 05 '22 at 02:34

2 Answers2

1

There are some constraints when a dynamic value is passed through a query and then the query is translated to be executed in database. Most database drivers do expect dynamic parameters only in specific places for security reasons as for example a dynamic parameter after the where clause, or after values in an insert script. The certain is that postgresql and most other drivers as well don't expect a dynamic parameter in the SQL for the creation of a schema.

String concatenation in SQL creation is known as bad practice as it could lead to some security issues, since all drivers expect the SQL passed to be static and safe while parameters are dynamic and are always checked if they contain something malicious. For this reason I would propose to avoid String concatenation to solve this issue and check the following approach.

Create the following function in your postgresql database.

  CREATE OR REPLACE FUNCTION create_custom_schema(s_name text) RETURNS void AS $$
        BEGIN
            EXECUTE 'CREATE SCHEMA IF NOT EXISTS' || quote_ident(s_name);
        END;
    $$ LANGUAGE plpgsql;

So we now have a function to do the job which accepts as parameter the schema name. Now the database driver would have no problem to accept a dynamic parameter for schema name and translate it correctly before executing the script.

Just note that functions are executed by the use of a subinterface of PreparedStatement which is the CallableStatement.

So your code now will be

    final String CREATE_SCHEMA_SQL = "{call create_custom_schema(?)}";
    List<String> schemas = // ["001", "002", "003"];
    
    for (String schema : schemas) {
    
      try (Connection connection = dataSource.getConnection();
          CallableStatement createSchemaStatement = connection.prepareCall(CREATE_SCHEMA_SQL)) {
    
           createSchemaStatement.setString(1, schema);
           createSchemaStatement.executeQuery();
        
      } catch (SQLException e) {
        throw new RuntimeException("Could not create tenant schema");
      }
    }
Panagiotis Bougioukos
  • 15,955
  • 2
  • 30
  • 47
0

Apart from using dynamic SQL on the server side, you could query the create statement using PostgreSQL's format function to encode the identifier.

For example:

private String getCreateSchemaStatement(Connection conn, String schemaName) throws SQLException {
    String sql = "SELECT format('CREATE SCHEMA IF NOT EXISTS %I', ?)";
    try (PreparedStatement stmt = conn.prepareStatement(sql)) {
        stmt.setString(1, schemaName);
        return getSingleResultString(stmt);
    }
}

private String getSingleResultString(PreparedStatement stmt) throws SQLException {
    try (ResultSet rs = stmt.executeQuery()) {
        String result = null;
        if (rs.next()) {
            result = rs.getString(1);
        }
        return result;
    }
}

Having this, you can just execute the statements:

public void createSchemas(List<String> schemaNames) throws SQLException {
    try (Connection conn = dataSource.getConnection();
         Statement stmt = conn.createStatement()) {
        for (String schemaName : schemaNames) {
            String sql = getCreateSchemaStatement(conn, schemaName);
            stmt.execute(sql);
        }
    }
}

Of course, this is not needed if the schema names are known to be safe.

Mihe
  • 2,270
  • 2
  • 4
  • 14