UseLiteral in SimpleQuerydslR2dbcFragment and ReactiveQuerydslR2dbcPredicateExecutor

[humanitas03]

SimpleQuerydslR2dbcFragment and ReactiveQuerydslR2dbcPredicateExecutor do not use prepared statement bindings but literals.

is there any reason for setUseLiterals(true)?

https://github.com/infobip/infobip-spring-data-querydsl/blob/c1b6926919520b70c8fa2f40e59ad2afca3de489/infobip-spring-data-r2dbc-querydsl/src/main/java/com/infobip/spring/data/r2dbc/SimpleQuerydslR2dbcFragment.java#L67

https://github.com/infobip/infobip-spring-data-querydsl/blob/ead317f5fb782178913a90798c420be00c5f8e12/infobip-spring-data-r2dbc-querydsl/src/main/java/com/infobip/spring/data/r2dbc/ReactiveQuerydslR2dbcPredicateExecutor.java#L138

[lpandzic]

Because, as far as I know, there is no layer that would translate querydsl model into prepared statement bindings.

[lpandzic]

Ok, so after some investigation and help from @mp911de I can confirm my initial suspicion. Querydsl generates sql which is escaped with marker ? which is the JDBC way of things. R2DBC on the other hand focuses on what each database uses - https://github.com/spring-projects/spring-framework/blob/main/spring-r2dbc/src/main/java/org/springframework/r2dbc/core/binding/BindMarkersFactoryResolver.java#L119-L128 this code probably best illustrates this.

For update method I've got as far as

    @Override
    @Transactional
    public Mono<Integer> update(Function<SQLUpdateClause, SQLUpdateClause> update) {
        SQLUpdateClause clause = sqlQueryFactory.update(path);
        List<SQLBindings> bindings = update.apply(clause).getSQL();
        String sql = bindings.stream()
                             .map(SQLBindings::getSQL)
                             .collect(Collectors.joining(" "));
        GenericExecuteSpec spec = bindParameters(databaseClient.sql(sql), bindings);
        return spec.fetch()
                   .rowsUpdated()
                   .as(TransactionalOperator.create(reactiveTransactionManager)::transactional);
    }

    GenericExecuteSpec bindParameters(GenericExecuteSpec spec, List<SQLBindings> bindings) {

        int index = 0;
        GenericExecuteSpec mutableSpec = spec;
        for (SQLBindings binding : bindings) {
            for (Object nullFriendlyBinding : binding.getNullFriendlyBindings()) {
                mutableSpec = mutableSpec.bind(index++, nullFriendlyBinding);
            }
        }
        return mutableSpec;
    }

but this isn't really doable without parsing the query and replacing jdbc markers with database specific markers.

What's your use case, @humanitas03 ? Is this a SQL injection concern or something else?

[humanitas03]

@lpandzic Yes, I'm concerned about SQL Injection. Even if there are no risks for injection under the current version of library except for the Query exposure, I'm worrying about some security issues since plain query is now moving through a network. Therefore, in case, is it possible for you to allow us to use useliteral in an optional way?

[lpandzic]

Therefore, in case, is it possible for you to allow us to use useliteral in an optional way?

The library doesn't work without useLiteral, that is why it was set in the first place.

[lpandzic]

I finally did some investigation on how to avoid using set literal true and while testing sql injection I was unable to sql inject anything through parameters - parameters are escaped. https://github.com/infobip/infobip-spring-data-querydsl/commit/c225c9aec1fe1a50e2266a4409f79cf7dc4c1b97 here's the test - I did various variations but while debugging the parameter is always escaped. I did post start a discussion on querydsl side to be certain - https://github.com/querydsl/querydsl/discussions/3558.

Regarding the network bit I'm not sure I follow, what makes you believe that binding markers would be any more secure than anything else in a network call? Both the query and the parameters still need to cross the wire and it's up to the network security layer/configuration to make sure neither is visible.

[lpandzic]

I'm closing this unless responses in the querydsl discussion provide arguments that literal is subject to sql injection.

[lpandzic]

After discussion with @jwgmeligmeyling - https://github.com/querydsl/querydsl/discussions/3558 I've decided to reopen this issue and remove setLiteral use for good.