spring-data-relational icon indicating copy to clipboard operation
spring-data-relational copied to clipboard

Add SpEL support for table and column names

Open funky-eyes opened this issue 3 years ago • 11 comments

We're from the [Seata] https://github.com/seata/seata community, we're a highly available, high-performance distributed transaction component, and currently our server side is considering moving from using jdbc to spring-data-r2dbc, but we've found some issues Our transaction information is stored in mysql, oracle and other databases, because we support the resource isolation of transactions by configuring different table names, and then assembling them into corresponding sqls to use, which makes us unable to use @Table to set a fixed table name, perhaps we can set the right through environment.resolvePlaceholders() or open doSelect table If this requirement is reasonable, I am willing to complete this job

funky-eyes avatar Sep 09 '22 14:09 funky-eyes

I'm highly sceptical of this since it seems likely to open us up to SQL injection.

https://xkcd.com/327/

schauder avatar Sep 13 '22 08:09 schauder

I'm highly sceptical of this since it seems likely to open us up to SQL injection.

https://xkcd.com/327/

I don't think there is an SQL injection problem with @Table("${dynamic_table:user_01})
Environment is determined by the application developer and there should be no possibility of SQL injection

funky-eyes avatar Sep 14 '22 02:09 funky-eyes

Environment is determined by the application developer

That is exactly the problem. A developer can use any variable and sooner than later will use untrusted data. And since bind variables can't be used, it has to be done by String concatenation, which pretty much the definition of a SQL injection problem.

schauder avatar Sep 14 '22 07:09 schauder

If it is because the environment variable is set by the developer, then the @Table can also be set by the developer and becomes a point of existence for sql injection. Is there a greater risk associated with methods like update or delete intable? @schauder

funky-eyes avatar Sep 14 '22 09:09 funky-eyes

If it is because the environment variable is set by the developer, then the https://github.com/table can also be set by the developer and becomes a point of existence for sql injection.

No, the Table annotation is safe because its values have to be compile time constants. Therefore they are trusted values by definition. This is not about the developer doing something malicious. It is about untrusted data (input from the user, data from a database ...) ending up as part of a sql statement.

schauder avatar Sep 14 '22 09:09 schauder

If it is because the environment variable is set by the developer, then the https://github.com/table can also be set by the developer and becomes a point of existence for sql injection.

No, the Table annotation is safe because its values have to be compile time constants. Therefore they are trusted values by definition. This is not about the developer doing something malicious. It is about untrusted data (input from the user, data from a database ...) ending up as part of a sql statement.

I still don't quite understand, even the parameters of properties, env, -D are set by the developer, who will attack their own application? I just wish this feature could be more flexible?

funky-eyes avatar Sep 14 '22 09:09 funky-eyes

Assuming I have multiple shards in my database, how do I aggregate the results of select * from table1 and select * from table2 when I use spring-data-r2dbc?

funky-eyes avatar Sep 14 '22 09:09 funky-eyes

unless I go to use DatabaseClient

funky-eyes avatar Sep 14 '22 09:09 funky-eyes

for example java -jar -DTABLE=abc app.jar or export TABLE=abc java -jar app.jar

@Table("${TABLE:cba}")

funky-eyes avatar Sep 14 '22 09:09 funky-eyes

We came up with this plan:

Have SpEL support for names provided in @Table and @Column annotations but run the results through a sanitizer. The sanitizer will be a bean implementing a simple interface and the default implementation out of the box by Spring Data will be rather restrictive. Something like only accept digits, ASCII letters and _. Stuff that doesn't get accepted will throw an exception.

If someone really wants to allow strange names they can do so be providing a custom sanitizer. Of course it is then up to them to make sure the whole is still safe.

schauder avatar Oct 06 '22 09:10 schauder

The feature should also apply to reference and key columns specified via @MappedCollection

schauder avatar Oct 07 '22 11:10 schauder

@schauder

Hi,How long before the feature is expected to be available

funky-eyes avatar Oct 10 '22 07:10 funky-eyes

We don't have schedule for this. And to be honest I don't consider it high priority.

Of course if there would be a good pull request ...

schauder avatar Oct 10 '22 07:10 schauder

We don't have schedule for this. And to be honest I don't consider it high priority.

Of course if there would be a good pull request ...

@schauder @mp911de Can I overload the following method. createPersistentEntity(TypeInformation,NamingStrategy)->createPersistentEntity(TypeInformation,NamingStrategy,ApplicationContext) or public JdbcMappingContext(NamingStrategy namingStrategy) -> public JdbcMappingContext(NamingStrategy namingStrategy,ApplicationContext)

This gives me a way to pass the ApplicationContext into the RelationalPersistentEntityImpl for support of spel expressions I'd be happy to submit a relevant pull request

funky-eyes avatar Oct 18 '22 01:10 funky-eyes

I'm not sure what method you want to overload, my IDE doesn't find any with that signature.

schauder avatar Oct 18 '22 07:10 schauder

I'm not sure what method you want to overload, my IDE doesn't find any with that signature.

@schauder https://github.com/spring-projects/spring-data-relational/blob/main/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/JdbcMappingContext.java public JdbcMappingContext(NamingStrategy namingStrategy) -> public JdbcMappingContext(NamingStrategy namingStrategy,ApplicationContext) or https://github.com/spring-projects/spring-data-commons/blob/main/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java createPersistentEntity

funky-eyes avatar Oct 18 '22 10:10 funky-eyes

I don't think we want to overload that. PersistentEntity and PersistentProperty are static objects, in the sense that they are created once in the lifetime of an application and then don't change. It feels off to have it reference a context that may include request scoped variables. I therefor would prefer their methods that provide column/table names to take a SpelQueryContext.

@mp911de your thoughts on this?

schauder avatar Oct 18 '22 12:10 schauder

@schauder Maybe replace ApplicationContext with Environment, holding it just to resolveRequiredPlaceholders for tableName and schemaName in @Table when creating PersistentEntity

funky-eyes avatar Oct 18 '22 13:10 funky-eyes

Maybe MappingContext would be a good choice too

funky-eyes avatar Oct 18 '22 13:10 funky-eyes