How to avoid "Security - A prepared statement is generated from a nonconstant String" FindBugs Warning

Question

I am working on a project that has a piece of code like the one below:

String sql = "SELECT MAX(" + columnName + ") FROM " + tableName;                
PreparedStatement ps = connection.prepareStatement(sql);

Is there any way that I can change this code so that FindBugs stop giving me a "Security - A prepared statement is generated from a nonconstant String" warning ?

Please assume that this code is safe regarding SQL INJECTION since I can control elsewhere in the code the possible values for "tableName" and "columnName" (they do not come come directly from user input).

Solution 1

Do not concatenate the sql String by +. You can use

String sql = String.format("SELECT MAX(%s) FROM %s ", columnName, tableName);

This is slower than concatenating a String so you should initialize this static then this is not a problem.

I think using a StringBuilder will also fix this warning.

Another way you can avoid this warning is to add @SuppressWarnings("SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING") above that string (or the method/or the class).

You could also use a Filter File to define rules which should be excluded.

Solution 2

private static final String SQL = "SELECT MAX(?) FROM ?";
PreparedStatement ps = connection.prepareStatement(sql);
ps.preparedStatement.setInt(1,columnName);
ps.preparedStatement.setString(2,tableName);

if you are using prepared statement, then in parameter should be a final string and parameters should be added later using setInt, setString methods.

this will resolve the findbug warning.

Solution 3

Neither String.format nor StringBuilder (or StringBuffer) helped me.

Solution was "prepareStatement" isolation:

private PreparedStatement prepareStatement(Connection conn, String sql) throws SQLException {
    return conn.prepareStatement(sql);
}

Solution 4

Try using the following...

private static final String SQL = "SELECT MAX(%s) FROM %s";

And then using a String.format() call when you use it...

PreparedStatement ps = connection.prepareStatement(String.format(sql,columnName,tableName));

If that doesn't solve the problem, you can always ignore that check; turn it off in your FindBugs configuration.

If that doesn't work (or isn't an option), some IDEs (like IntelliJ) will also let you suprress warnings with either specially formatted comments or annotations.

Solution 5

It is possible to use concatenation to create your String. Doing so does not cause the Security warning. And is preferrable for clarity's sake when dealing with long SQL statements which would be better written split across many lines

Using variables to construct the String is what causes the security warning.

This will cause the warning :

String columnName = getName();
String tableName  = getTableName();
final String sql = "SELECT MAX(" + columnName + ") FROM " + tableName;
PreparedStatement ps = connection.prepareStatement(sql);

This will not not work :

String columnName = getName();
String tableName  = getTableName();
final String sql = "SELECT MAX(" + "?" + ")" +
                   "FROM " + "?";
PreparedStatement ps = connection.prepareStatement(sql);
ps.setString(1, columnName);
ps.setString(2, tableName);

It does not work because prepared statements only allow parameters to be bound for "values" bits of the SQL statement.

This is the solution that works :

private static final boolean USE_TEST_TABLE = true;
private static final boolean USE_RESTRICTED_COL = true;
private static final String TEST_TABLE = "CLIENT_TEST";
private static final String PROD_TABLE = "CLIENT";
private static final String RESTRICTED_COL ="AGE_COLLATED";
private static final String UNRESTRICTED_COL ="AGE";

....................

final String sql = "SELECT MAX(" +
        ( USE_RESTRICTED_COL ? RESTRICTED_COL : UNRESTRICTED_COL ) +  ")" +
        "FROM " +
        ( USE_TEST_TABLE ? TEST_TABLE : PROD_TABLE );
PreparedStatement ps = connectComun.prepareStatement(sql);

But it only works if you have to chose between two tables whose names are known at compile time. You could use compounded ternary operators for more than 2 cases but then it becomes unreadable.

The 1st case might be a security issue if getName() or getTableName() gets the name from untrusted sources.

It is quite possible to construct a secure SQL statement using variables if those variables have been previously verified. This is your case, but FindBugs can't figure it out. Findbugs is not able to know what sources are trusted or not.

But if you must use a column or table name from user or untrusted input then there is no way around it. You have to verify yourself such string and ignore the Findbugs warning with any of the methods proposed in other answers.

Conclusion : There is no perfect solution for the general case of this question.

View more solutions