Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance GraphQL request body checks to prevent 500 Error #726

Closed
karlrwjohnson opened this issue Jun 19, 2023 · 6 comments
Closed

Enhance GraphQL request body checks to prevent 500 Error #726

karlrwjohnson opened this issue Jun 19, 2023 · 6 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@karlrwjohnson
Copy link

An automated scanning tool at my organization found that when the request body is not well-formed, Spring GraphQL (tested v. 1.0.4) generates 500 Internal Server Error where 400 Bad Request is expected.

Consider the following example request:

POST /graphql
Content-Type: application/json

{
  "query": "query { foo }",
  "variables": "&/usr/bin/env&"
}

Obviously variables should be an object, not a string. One would expect this request to product a 400 Bad Request messsage, but instead the server returns 500 Internal Server Error:

{
    "timestamp": "2023-06-19T15:47:12.050+00:00",
    "status": 500,
    "error": "Internal Server Error",
    "message": "class java.lang.String cannot be cast to class java.util.Map (java.lang.String and java.util.Map are in module java.base of loader 'bootstrap')",
    "path": "/graphql"
}

Stack trace:

SEVERE: Servlet.service() for servlet [dispatcherServlet] in context with path [/] threw exception [Request processing failed; nested exception is java.lang.ClassCastException: class java.lang.String cannot be cast to class java.util.Map (java.lang.String and java.util.Map are in module java.base of loader 'bootstrap')] with root cause
java.lang.ClassCastException: class java.lang.String cannot be cast to class java.util.Map (java.lang.String and java.util.Map are in module java.base of loader 'bootstrap')
	at org.springframework.graphql.server.WebGraphQlRequest.<init>(WebGraphQlRequest.java:61)
	at org.springframework.graphql.server.webmvc.GraphQlHttpHandler.handleRequest(GraphQlHttpHandler.java:85)
	at org.springframework.web.servlet.function.support.HandlerFunctionAdapter.handle(HandlerFunctionAdapter.java:106)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1072)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:965)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1006)
	at org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:909)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:665)
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:883)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:750)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:209)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
        ...

The error occurs when WebGraphQLRequest extracts the variables key of the request via <T> T getKey(String key, Map<String, Object> body).

Only the presence of the key is checked, not its type. getKey() could expect a ParameterizedTypeReference<T> of the expected type, or the caller could use a library like Jackson to reinterpret the request body into a class with known-type keys.

As it stands, I'm looking at writing a custom filter to detect and validate GraphQL requests, but it seems like something that would be better done inside the library.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 19, 2023
@rstoyanchev rstoyanchev self-assigned this Jun 20, 2023
@rstoyanchev rstoyanchev added this to the 1.2.1 milestone Jun 20, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 20, 2023
@rstoyanchev rstoyanchev changed the title Validate GraphQL request bodies to prevent 500 Internal Server Error Enhance GraphQL request body checks to prevent 500 Error Jun 20, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 20, 2023

There is a fix for 1.2.1, also backported to 1.1.5 with #733. Both are due today.

@dfa1
Copy link

dfa1 commented Jun 20, 2023

@rstoyanchev did you mean 1.1.5?

@karlrwjohnson
Copy link
Author

@rstoyanchev Darn. I upgraded to 1.1.5, but I think it depends on Java 17

   > Could not resolve org.springframework.graphql:spring-graphql:1.1.5.
     Required by:
         project :
      > No matching variant of org.springframework.graphql:spring-graphql:1.1.5 was found. The consumer was configured to find an API of a library compatible with Java 11, packaged as a jar, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm' but:
          - Variant 'apiElements' capability org.springframework.graphql:spring-graphql:1.1.5 declares an API of a library, packaged as a jar, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm':
              - Incompatible because this component declares a component compatible with Java 17 and the consumer needed a component compatible with Java 11
          - Variant 'runtimeElements' capability org.springframework.graphql:spring-graphql:1.1.5 declares a runtime of a library, packaged as a jar, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm':
              - Incompatible because this component declares a component compatible with Java 17 and the consumer needed a component compatible with Java 11

My org is stuck on Java 11 right now for reasons outside my control.

I was hoping that Java 17/Spring Framework 6.0 corresponded to Spring-GraphQL 1.2.x

@bclozel
Copy link
Member

bclozel commented Jun 20, 2023

@dfa1 it's been backported as #733

@rstoyanchev
Copy link
Contributor

We can backport this to 1.0.x as well. I've created #735.

@dfa1
Copy link

dfa1 commented Jun 21, 2023

@dfa1 it's been backported as #733

yes, originally the comment mentioned 1.5.1 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants