vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 535 forks source link

GraphQLWS: `Complete` message type should check for the presence of the `id` field. #2524

Closed roksui closed 12 months ago

roksui commented 1 year ago

Version

4.5.0, 5.0.0-SNAPSHOT

Context

When a websocket client sends a Complete message to the websocket server, the Connectionhandler.ReadyState.unsubscribe method is called. However this method doesn't check if the id field is present in the message. This leads to a NullPointerException when evaluating the expression Subscription s = subscriptions.remove(msg.id());.

As per the graphql-ws protocol, the Complete message type must contain the id field, and if it doesn't the server should respond with a 4400: <error-message> and close the connection.

In ideal cases, the client implementations conform to the correct message format. However, I think the server needs to check for this exceptional case.

Do you have a reproducer?

GraphQLWSTestsServer inside the test package was used as the websocket server, and for the client, I wrote a test case at the end of the graphql-ws.test.js file. The client sends a Complete message after it has subscribed and a NPE is thrown on the server side.

Steps to reproduce

  1. Run io.vertx.ext.web.handler.graphql.GraphQLWSTestsServer.java.
  2. Inside the vertx-web\vertx-web-graphql\tests\graphql-ws\graphql-ws.test.js file, run the test function with the name "invalid complete message" at the bottom.
  3. On the server, check the console for confirming the NPE.

Extra

OS: Windows 10 JDK: zulu-17

tsegismont commented 12 months ago

Fixed by d3cf3a075