0

I'm in a situation where I have a parent class

class Parent<T extends Entity> {
    ...
    public List<T> selectSomething() {
         String sql = "SELECT " + getColumns() + " FROM " + getTable() + " WHERE ...";     
         TypedQuery<T> query = this.entityManager.createQuery(sql, T.class);
         return query.getResultList();
    }


}

(For sake of simplicity, there is only a method described but in practice there are more with the same format)

And many child classes :

class Child1 extends Parent<EntityChild1> { ... }
class Child2 extends Parent<EntityChild2> { ... }
...
class Child50 extends Parent<EntityChild50> { ... }

My issue is that this piece of code in the Parent is detected as a security issue by tools like SonarQube for possible SQL injection, and this warning is a blocking point that I need to correct or bypass.

I precise the both method getColumns() and getTables() are 100% controlled by me and I do know for sure that there is no way for a user to interfere in any way with the returned values of those methods. So even if it's detected as SQL injection, I do know it's not one.

To fix this, I can't just use a prepared statement since the whole point of a prepared statement is to prevent to inject SQL code, so you can't use them for dynamic columns name or table name.

I'm considering two options :

  1. Make the selectSomething method from the Parent abstract and reimplement it in each child with a fixed string for the request directly containing the table name and columns name.
    This would work fine and fix the warning, but due to the high number of Child it makes the code much less maintainable and evolutive (especially considering that the number of Child class could increase over time).

  2. Simply let it as is and use a @SupressWarning annotation in the selectSomething method from the Parent this make the tools like SonarQube ignore this.
    Again, it's a solution that would work but which is not very clean and filling code with many @SupressWarning is a bad practice.

Is there any good way to handle this situation ? If not, is there any reason I should stick with a solution above the other ?

Xiidref
  • 1,456
  • 8
  • 20
  • 1
    Is getColumns always returning the same result? You might be able to turn it into a static final Map and avoid these issues, having extra peace of mind that only a subset of approved strings can be returned. Not to mention the obvious performance boost. – Pedro Cavalheiro Feb 09 '23 at 15:28
  • @PedroCavalheiro Sadly, it's not a fixed result for each Child class, it will remain the same most of the time but can occasionally change with other columns under a few specific conditions, however this could be a solution at least for the table name part. – Xiidref Feb 09 '23 at 15:40
  • You have implemented SQL Injection... intentionally? I would suggest you get rid of that piece of code, and use a safe ORM instead. – The Impaler Feb 09 '23 at 15:47
  • 1
    "...there is no way for a user to interfere in any way with the returned values of those methods..." -- well, yes, today. The next developer that works wit the code next year won't know about this and could happily inject bad data in them. Be safe, don't concatenate SQL statements and use parameterized queries instead. – The Impaler Feb 09 '23 at 15:50
  • 3
    Parameterized queries will not help with things like dynamic column selection or table names, those are not bindable in jdbc. My personal way of handling these is to have `.sql` files with the query parameters already made out, and then loading those files at runtime (so you reference a query name instead of putting SQL into Java code). However, for the situation OP faces, short of files for every permutation this is the only way. An ORM or dynamic query builder will end up doing this under the hood anyhow, so it's a question of how to isolate these so user input doesn't make it there. – Rogue Feb 09 '23 at 16:05
  • 1
    @TheImpaler Normally I'd agree with this, but in this case the parametrized part concern the columns and table name, which can't be passed using prepared statements. However, the part of considering what could the next developer do is still a good point that I didn't consider. – Xiidref Feb 09 '23 at 16:09

1 Answers1

2

Reflect whether this isn't a design flaw on your inheritance model. If it isn't, well, stick with the solution that's less likely to introduce errors.

Warnings are just that, warnings, you may proceed with caution. Having a lot of code duplication is more likely to introduce errors, in my personal experience. But make sure the methods are actually 100% controlled by you. Make the possible security concerns clear in the documentation, both for you and future maintainers. That documentation will serve to avoid having it being falsely flagged as a security flaw and to help prevent someone from introducing that same security flaw. As referenced in the comments, pass as many things as you can to a Set or another data structure that maps a specific input to a specific output, with special attention to the special output, both for performance reasons and some peace of mind.

See Variable column names using prepared statements for a similar question.

Pedro Cavalheiro
  • 187
  • 1
  • 9