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

Close socket if handle_init returns an :error #18

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

maxsalven
Copy link

The current docs suggest returning {:error, %{}, socket} from handle_init.
https://github.com/geometerio/absinthe_graphql_ws/blob/b98d3deb15febd856725f69b7a54c38e3b67ab3b/lib/absinthe/graphql_ws/socket.ex#L176-L178

This ends up creating an invalid error message:
https://github.com/geometerio/absinthe_graphql_ws/blob/b98d3deb15febd856725f69b7a54c38e3b67ab3b/lib/absinthe/graphql_ws/transport.ex#L129-L130
https://github.com/geometerio/absinthe_graphql_ws/blob/b98d3deb15febd856725f69b7a54c38e3b67ab3b/lib/absinthe/graphql_ws/message/error.ex#L9-L11

The error message will be type: "error", id: %{}, payload: %{}, when the spec expects the id to be a string, and payload to be an array of graphql errors. The client currently throws an error when it sees this invalid message.

Even if you fix this, the spec does not expect an error message in response to connection_init, and the official client will also throw an error.

The reference implementation closes the connection with code 4403 if handling connection_init fails:
https://github.com/enisdenjo/graphql-ws/blob/799cfc7bbe0f6d1a5c90b4880f02c58c5c3a06d4/src/server.ts#L612-L614

https://github.com/enisdenjo/graphql-ws/blob/799cfc7bbe0f6d1a5c90b4880f02c58c5c3a06d4/src/__tests__/server.ts#L437-L458

This is critical for token refresh based workflows (e.g. the token you included in connectionParams on the client is stale, and you need to check for 4403 Forbidden to refresh it).

This PR matches the spec. I'm not sure how to adapt the tests to check for this, suggestions welcome!

@jHwls
Copy link

jHwls commented Dec 3, 2022

@maxsalven this is a good fix. Thank you! Just bumping in the hopes it gets picked up.

Might also be good to be able to pass a custom error code? vs just defaulting to 4403

@maartenvanvliet
Copy link
Contributor

+1 for this fix.

The graphql errors in the payload array is interesting. Since this is just protocol negotiation and no graphql has been invoked yet it does not make much sense to return graphql errors. If an empty array is passed ([]) as payload instead of {} does the client accept it?

@mdoverhag
Copy link

This would be a great improvement. I was mostly surprised to see the connection was left open when returning a {:error, %{error: "unauthorized"}, socket} in the handle_init/2 callback.

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

Successfully merging this pull request may close these issues.

4 participants