search

Netflix / dgs-framework

GraphQL for Java with Spring Boot made easy.
https://netflix.github.io/dgs
Apache License 2.0
3.03k stars 286 forks source link

Stack Overflow (Criteria.parse) with JsonPath #1772

Closed martinvisser closed 5 months ago

martinvisser commented 6 months ago

According to https://github.com/json-path/JsonPath/issues/973 there is a high severity issue for JsonPath. Are there any plans to deal with this? JsonPath seems to be a dependency of the DGS client module.

paulbakker commented 6 months ago

Correct me if I'm wrong, but as far as I understand the issue wouldn't be exposed through DGS. We don't take user input directly that goes through JsonPath. It's only when using the DgsQueryExecutor, for example in tests, where a developer uses the APIs backed by jsonpath.

Once there is a patch for the library we'll update it, but I don't think there's any action to take now.

martinvisser commented 6 months ago

That's good to hear. I did notice however, that after fully excluding the dependency it does cause NoClassDefFoundError: com/jayway/jsonpath/Configuration, so it used outside of tests too.

java.lang.NoClassDefFoundError: com/jayway/jsonpath/Configuration
    at com.netflix.graphql.dgs.client.GraphQLResponse.<clinit>(GraphQLResponse.kt:131)
    at com.netflix.graphql.dgs.client.WebClientGraphQLClient.handleResponse(WebClientGraphQLClient.kt:146)
    at com.netflix.graphql.dgs.client.WebClientGraphQLClient.access$handleResponse(WebClientGraphQLClient.kt:42)
    at com.netflix.graphql.dgs.client.WebClientGraphQLClient$reactiveExecuteQuery$3.invoke(WebClientGraphQLClient.kt:136)
    at com.netflix.graphql.dgs.client.WebClientGraphQLClient$reactiveExecuteQuery$3.invoke(WebClientGraphQLClient.kt:136)
    at com.netflix.graphql.dgs.client.WebClientGraphQLClient.reactiveExecuteQuery$lambda$3(WebClientGraphQLClient.kt:136)
    ...
Caused by: java.lang.ClassNotFoundException: com.jayway.jsonpath.Configuration
    at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
    at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)

But I agree, looking at what's happening in GraphQLResponse, this is not the same as seen in that issue.

I'll convey the message as I was just forwarding the question from client.

paulbakker commented 6 months ago

It’s definitely there, but there is no code path to trigger the issue

On Tue, Jan 9, 2024 at 10:37 AM Martin Visser @.***> wrote:

That's good to hear. I did notice however, that after fully excluding the dependency it does cause NoClassDefFoundError: com/jayway/jsonpath/Configuration, so it used outside of tests too.

java.lang.NoClassDefFoundError: com/jayway/jsonpath/Configuration at com.netflix.graphql.dgs.client.GraphQLResponse.(GraphQLResponse.kt:131) at com.netflix.graphql.dgs.client.WebClientGraphQLClient.handleResponse(WebClientGraphQLClient.kt:146) at com.netflix.graphql.dgs.client.WebClientGraphQLClient.access$handleResponse(WebClientGraphQLClient.kt:42) at com.netflix.graphql.dgs.client.WebClientGraphQLClient$reactiveExecuteQuery$3.invoke(WebClientGraphQLClient.kt:136) at com.netflix.graphql.dgs.client.WebClientGraphQLClient$reactiveExecuteQuery$3.invoke(WebClientGraphQLClient.kt:136) at com.netflix.graphql.dgs.client.WebClientGraphQLClient.reactiveExecuteQuery$lambda$3(WebClientGraphQLClient.kt:136) ... Caused by: java.lang.ClassNotFoundException: com.jayway.jsonpath.Configuration at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)

But I agree, looking at what's happening in GraphQLResponse, this is not the same as seen in that issue.

I'll convey the message as I was just forwarding the question from client.

— Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/issues/1772#issuecomment-1883583922, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2XLASDFOTGTKXQ45LN6DYNWE5LAVCNFSM6AAAAABBSZ5TNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBTGU4DGOJSGI . You are receiving this because you commented.Message ID: @.***>

ntenherkel commented 6 months ago

If it's only used for test, why is the dependency not scoped for test in maven/gradle? I think if that can still be done, it should not be flagged anymore by Nexus IQ.

martinvisser commented 6 months ago

@ntenherkel But it isn't used only in test as my stack trace showed. The dependency is used in production code too. Maybe the path to that issue as stated in JsonPath isn't used in production code, but JsonPath itself is used outside test.

ntenherkel commented 6 months ago

Yes let me rephrase my suggestion. @paulbakker is it possible for the netflix developers to scope this dependency to test? Based on Martin's comment that would also mean some adjustments in the dgs-framework java/kotlin logic.

My feeling is that json-path is not playing an important role in the dgs-framework. The end result having a dependency less (that is poorly maintained, CVE already open for weeks with no sight on a fix) in the jar, is also worth the attempt if you ask me.

martinvisser commented 6 months ago

I do fully agree with that. If that would be possible, that would be a nice solution.

paulbakker commented 6 months ago

JsonPath is used in two places:

  1. DgsQueryExecutor - to parse out parts of a query from the result. This is typically used for testing. However it's part of the main API, so it should not be a test only dependency. I'm not too worried about it though, because there is no way for an outside user (someone sending a query) to specify the json path.
  2. The DGS client that you use to call other GraphQL services. That's the stacktrace @martinvisser posted. This could be a problem if you would take user input and pass that on directly as the jsonpath argument.

Like SQL injection, this issue is only a problem if user input is used directly to form the jsonpath argument, which we don't do in the framework itself. We can't make JsonPath a a test-only dependency, but I don't see how the CVE would affect a DGS in general.