0

I'm using prepareStatement() to prevent sql injection. the problem I have now is that with the method below, I can do getConnection().prepareStatement() then build my query, but I'll need to try-catch on every single call to getConnection() and close it in finally block. createStatement().execute() seems to be better because I could have client to pass in the query then handle try-catch-finally in one place, but it won't prevent sql injection. what is the best practice usually? or is there any other way to build query that can prevent sql injection?

private static Connection getConnection() throws SQLException, URISyntaxException {
    URI dbUri = new URI(System.getenv("DATABASE_URL"));

    String username = dbUri.getUserInfo().split(":")[0];
    String password = dbUri.getUserInfo().split(":")[1];
    String dbUrl = "jdbc:postgresql://" + dbUri.getHost() + ':' + dbUri.getPort() + dbUri.getPath();

    return DriverManager.getConnection(dbUrl, username, password);
}
user1865027
  • 3,505
  • 6
  • 33
  • 71
  • 4
    You should be using well-proven connection pools, not creating your own. Check out the connection, pass it into the data access object, close all other JDBC resources in method scope in a finally block, and return the connection to the pool – duffymo Aug 08 '17 at 18:56
  • @duffymo i dont quite understand. its my first time writing java on server side. can you provide some example please – user1865027 Aug 08 '17 at 19:01
  • What will you deploy your application to? Tomcat? Spring Boot? JBOSS? All have built in connection pools. I'd recommend using them. – duffymo Aug 08 '17 at 19:01
  • @duffymo Tomcat. thanks. let me google it – user1865027 Aug 08 '17 at 19:04
  • Here's how you set up a JNDI data source connection pool in Tomcat: https://tomcat.apache.org/tomcat-8.0-doc/jndi-datasource-examples-howto.html – duffymo Aug 08 '17 at 19:04
  • 1
    Having to close all the resources (Connection, Statement, ResultSet) yourself results in a lot of error prone boiler plate code. So you should definitely have a look at Apache Commons DbUtils or Spring JDBC to do that. – Wilco Greven Aug 08 '17 at 19:25
  • I agree - I think Spring JdbcTemplate is the simplest, best solution to the problem I've seen. The best part is that you need not use all of Spring if you don't want to. It's possible to use JdbcTemplate and the bean factory without much overhead. – duffymo Aug 08 '17 at 20:07
  • I've converted my getConnection() method to return a connection from pool, but seems like I'll still need to close the connection in finally block. try-with doesn't work. – user1865027 Aug 08 '17 at 20:14
  • does that mean i still need to try-catch and close connection on every client method that calls getConnection()? – user1865027 Aug 08 '17 at 20:14
  • @duffymo so it looks like i cant manage all the close calls into one try-catch block and still able to prevent sql injection? – user1865027 Aug 08 '17 at 21:35
  • Nothing to do with SQL injection. – duffymo Aug 08 '17 at 21:38
  • @duffymo like i mentioned, I wanted to use `prepareStatement()` to avoid sql injection, but that means all the clients would need to close connection themselves. if i can do `createStatement().execute()`, I can have client to pass query then handle all the close calls in one place. – user1865027 Aug 08 '17 at 21:41
  • @duffymo but by doing `createStatement().execute()` will not prevent sql injection. – user1865027 Aug 08 '17 at 21:42
  • You ought to learn something about JDBC. It should be prepareStatement. Clients should not be passing in SQL. You should a separate data access layer that handles database operations. – duffymo Aug 08 '17 at 21:42

1 Answers1

1

I would use connection pool instead of creating connection and closing every time.

Couple of steps we should follow to create a connection pool using tomcat server

Step -1. Update TOMCAT_ROOT_DIR\conf\server.xml file with database connection details like as below:

<?xml version='1.0' encoding='utf-8'?>
...
  <GlobalNamingResources>
    ...
    <Resource name="jdbc/JCGExampleDB" 
              global="jdbc/JCGExampleDB"
              factory="org.apache.tomcat.jdbc.pool.DataSourceFactory"
              auth="Container"
              type="javax.sql.DataSource"              
              username="test"
              password="test"
              driverClassName="com.mysql.jdbc.Driver"
              description="JCG Example MySQL database."
              url="jdbc:mysql://localhost:3306/JCGExampleDB"
              maxTotal="10"
              maxIdle="10"
              maxWaitMillis="10000"
              removeAbandonedTimeout="300"            
              defaultAutoCommit="true" />
     ...
  </GlobalNamingResources>

Step -2: Using Spring's JdbcTemplate - You can write your database connection utility class and get connection like as below (This step can be done in multiple ways.. like context.xml file or web.xml file or as below)

@Bean
    public JdbcTemplate jdbcTemplate() {
        return new JdbcTemplate(dataSource());
    }

    @Bean
    public DataSource dataSource() {
        DataSource dataSource = new com.mchange.v2.c3p0.ComboPooledDataSource();
        try {
            JndiTemplate jndiTemplate = new JndiTemplate();
            dataSource = (DataSource)jndiTemplate.lookup("java:comp/env/jdbc/JCGExampleDB");
            } catch (NamingException e) {
            log.error("Unable to configure datasource: " + e.getStackTrace());
        }
        return dataSource;
    }

Edit1: Using Singleton Class - Without JdbcTemplate: You can get the connection from Singleton class whenever needed.

public class DatabaseConnectionManager {

    DataSource ds;

    public void init() {
        InitialContext initialContext = new InitialContext();
        ds = (javax.sql.DataSource)initialContext.lookup("jdbc/JCGExampleDB");
    }

    public Connection getConnection() {
        if(ds == null) init();

        return ds.getConnection();
    }
}

Hope this helps...

0190198
  • 1,667
  • 16
  • 23
  • I love your suggestion of JdbcTemplate, but it should be pointed out that it only makes sense if the OP is using Spring. – duffymo Aug 08 '17 at 20:07
  • im not using Spring. and i dont actually understand how connections are closed properly in your example. do i still need to close it manually on every call – user1865027 Aug 08 '17 at 20:30
  • @user1865027 - I updated my answer the way without using Spring. If you use the connection pool then pool will take care of opening&closing the connections based on the parameters you mentioned in tag. – 0190198 Aug 08 '17 at 20:45
  • @K.S thanks. i've tested connection pool. i still need to call close. i don't think I can avoid it? [mentioned here](https://stackoverflow.com/questions/4938517/closing-jdbc-connections-in-pool) – user1865027 Aug 08 '17 at 20:52
  • @user1865027 - Yes you should take the responsibility to close the resource if you get the connection using getConnection() from datasource. But messing the datasource by getConnection() externally is not advisable. – 0190198 Aug 10 '17 at 12:58
  • ok thanks. sounds like I'll have a lot of same try-catch blocks – user1865027 Aug 10 '17 at 17:25
  • @user1865027 - I would write a generic method which will take sqlQuery, array of parameters and call it where ever I want. And I will write connection closing logic in that method. – 0190198 Aug 10 '17 at 18:43
  • @K.S i thought about that too but it would be really trouble to identify the data type of each param since `prepareStatement` has different methods for different data type. for example, `setInt()`, `setString()` – user1865027 Aug 10 '17 at 18:48