Getting "Internal error occurred during message handling. Please check your implementation." after following recipe

[jhalborg]

Expected Behaviour
I am having an issue where my telemetry is flooded with errors thrown by graphql-ws for invalid (expired) tokens. I get the error

Internal error occurred during message handling. Please check your implementation. (this error)

I do not expect the server to throw an error in this case, as an invalid/expired token is not a server error, but a client error.

I tried looking through the docs/recipes, and tried updating our implementation to pretty much this recipe

which essentially defines a method for checking auth that can be called in onConnect/onSubscribe, which (according to the recipe) should just throw an error if verification fails. This is a fine approach, and I expected this error to just be returned to the client.

Actual Behaviour
When this method throws an error, however, the server logs the aforementioned error instead of just returning that error to the client.

Internal error occurred during message handling. Please check your implementation. Error: Some custom error

Debug Information
Simply set up the recipe. Something along the lines of

...
import { useServer } from 'graphql-ws/lib/use/ws';
...

const wsServer = new WebSocketServer({
    server,
    path: '/graphql',
  });

  const handleWsAuth = async ({ headers }: IncomingMessage) => {
    if (!headers?.authorization || !headers['correlation-id']) {
      throw new Error('Some custom error');
    }
    const user = await getCurrentUser(
      headers.authorization,
      headers['correlation-id'] as string
    );
    if (!user) {
      throw new Error('Some custom error');
    }
  };

  useServer(
    {
      schema,
      onConnect: async ({ extra: { request } }) => {
        // do your auth on every connect (recommended)
        await handleWsAuth(request);
      },
      context: async (ctx) => {
        const contextFromHeaders = await createContext(...);
        return {
          ...initContextCache(),
          ...contextFromHeaders,
        };
      },
    },
    wsServer
  );
})

[enisdenjo]

Errors thrown in any of the handlers are unexpected and shouldn't be handled by graphql-ws.

Please read the full recipe, you'll see below that the recipe implements a custom WS server for handling the instances of the Forbidden error.

graphql-ws/website/src/pages/recipes.mdx

Lines 817 to 833 in e45d6b1

	socket.on('message', async (event) => {
	try {
	// wait for the the operation to complete
	// - if init message, waits for connect
	// - if query/mutation, waits for result
	// - if subscription, waits for complete
	await cb(event.toString());
	} catch (err) {
	// all errors that could be thrown during the
	// execution of operations will be caught here
	if (err instanceof Forbidden) {
	// your magic
	} else {
	socket.close(CloseCode.InternalServerError, err.message);
	}
	}
	});

Alternatively, you can of course not throw in the handlers and gracefully close the connection.

[jhalborg]

Thank you for getting back to me so quickly enisdenjo. Sorry for having made an issue, didn't know the repo used discusssions - few do :D

Didn't see those lines, thanks for pointing me in the right direction.

Could any of the exposed onX be used for this purpose for simplicity, or do I have to actually create my own instance of useServer a la the recipe?

[jhalborg]

Alternatively, you can of course not throw in the handlers and gracefully close the connection

How would I do that inside of the onConnect method?

[jhalborg]

To be clear, my goal is to handle the case where the token used in a connection is expired simply and gracefully. Maybe I'm going about it all wrong?

[enisdenjo]

For graceful handling without implementing your own server, you could just use a try/catch when validating the token and return false from onConnect. Something like:

import { useServer } from 'graphql-ws/lib/use/ws';
import { validateAuth } from './my-auth';

const server = useServer(
  {
    schema,
    onConnect: async (ctx) => {
      try {
        await validateAuth(ctx.connectionParams);
      } catch {
        // returning false indicates that the user has been forbidden access
        // the server will close the socket with a `4403: Forbidden`
        return false;
      }
    },
  },
  wsServer,
);

This is also showcased in the "ws server and client auth usage with token expiration, validation and refresh" recipe.

[jhalborg]

@enisdenjo

Thank you again for your help! It is much appreciated <3

It seemed to solve some of the errors, but still have one left.

I use the context field in the useServer method:

  {
    schema,
    // https://github.com/enisdenjo/graphql-ws/discussions/535
    onConnect: async ({ connectionParams }) => {
      try {
        // do your auth on every connect (recommended)
        return canGetVerifiedUserFromToken(
          connectionParams as ClientHeaderShape
        );
      } catch (error) {
        // returning false indicates that the user has been forbidden access
        // the server will close the socket with a `4403: Forbidden`
        return false;
      }
    },
    context: async (ctx) => {
      const contextFromHeaders = await createContext(
        ioredis,
        launchDarklyClient,
        lowercaseKeys(ctx.connectionParams)
      );
      return {
        ...initContextCache(),
        ...contextFromHeaders,
      };
    },
  },
  wsServer
);

The context will also verify the user based on auth header, and inject the user into the context. It throws on the same logic, i.e. invalid/expired token, causing the Internal error occurred during message handling error.

One solution would be for the method to just return User | undefined, and update the context to have user?: User field - but this would pollute the rest of the code base/resolvers with if (!user) {} statements. Is there maybe a simpler way to handle the case of expired token in the context method? I would've assumed that the code would never reach that step now that we return false in onConnect?

Maybe I have to check the token in onSubscribe instead, and then it won't reach the context method? If so, what are the implications of doing that instead of in onConnect - I've seen a couple of places that doing it in onConnect is recommended.

Also, following the recipe , I get a lint error for Expected to return a value at the end of async method 'onSubscribe'. for this code:

onSubscribe: async ({ connectionParams, extra }) => {
      if (
        !(await canGetVerifiedUserFromToken(
          connectionParams as ClientHeaderShape
        ))
      ) {
        logger.debug({
          message:
            'Could not get/verify user from headers in websockets - onSubscribe - maybe token expired?',
        });
        return extra.socket.close(CloseCode.Forbidden, 'Forbidden');
      }
    },

What should I return from onSubscribe if I could actually get a valid user from the token?

[enisdenjo]

Maybe I have to check the token in onSubscribe instead, and then it won't reach the context method? If so, what are the implications of doing that instead of in onConnect - I've seen a couple of places that doing it in onConnect is recommended.

The difference is that onConnect is executed once per connection - on connection initialisation; while onSubscribe is executed every time the client requests an operation.

I personally prefer onConnect only because if it succeeds - there's no reason to re-authenticate the user on each subscription. A WebSocket connection cannot be hijacked - it's a long-lived TCP connection between the client and the server; meaning, as long as it's alive - you can know for a fact that you're communicating with the same client you authenticated in onConnect.

However, you might have a mechanism where you want to kick a user when the token expires. For that you might use onSubscribe; but me personally, I'd just have a setTimeout in onConnect that closes the socket after token's expiry...

Nevertheless, if you decide to stick with current validations - onSubscribe is much better than the context.

What should I return from onSubscribe if I could actually get a valid user from the token?

If you don't want to change the execution arguments, you can return nothing (no return statement or return void 0).

I get a lint error for Expected to return a value at the end of async method 'onSubscribe'.

That's a linter configuration local to you, nothing to do with graphql-ws itself.

[jhalborg]

Gotcha, thank you again! Decided to skip token validation in context creation for subscriptions, and just validate+verify in onConnect. Good suggestion about the timer for closing socket though!

And just to clarify, the linter error simply got me thinking about what should I be returning from the method, I was not complaining about my lint error to you :)

Have a great weekend

[enisdenjo]

Awesome! Glad you got it working. If you need any more help, feel free to open discussions or issues. 😄