Added websocket transport support for query & mutation.

[klemens-morgenstern]

subscriptions-transport-ws supports websocket transport. I just modified the execute function which did the trick for me and seems to work with all the auth etc., too.

[benjie]

Auth is a little different for websockets so I had to disable queries/mutations so that it wasn’t a breaking change for people using pgSettings/etc in interesting ways. I wasn’t planning to add support until v5, but we could add this behind a flag if you like?

[klemens-morgenstern]

Ah, that makes way more sense; it looked a bit weird that it was just an exception. I guess my pgSettings is super boring.

Yes, a flag or plugin works for me.

Thanks for all the great work!

[benjie]

I need to think about the option name.

[benjie]

i’m not quite happy with this naming but my suggestions aren’t better. Let me ponder.

enableQueriesAndMutationsOverWebsockets
allOperationsOverWebsockets
fullWebsocketSupport
websocketExecute
websocketOperations
graphqlOverWebsocket
wsAllOps

[klemens-morgenstern]

Ah yes, the good ol' naming. Let me throw a few in.

• wsQueryMutation
• generalWebsocket
• generalWebsocketTransport
• gqlOverWebsocket

Alternatively, instead of a flag, the actual options could be like this (subscription being kept for compatibility but technically deprecated)

  websocketTransport?: boolean|"all"|"only-subscriptions";

with websocketTransport: true being equal to websocketTransport: "all", while subscriptions: true is equal to websocketTransport: "only-subscriptions".

[benjie]

This is better. Great suggestion. Lets drop the boolean and go with:

websocketOperations?: "all" | "subscriptions" | "none"

Internally it should default to "subscriptions". In addition to the changes in this PR we should also check it here:

https://github.com/graphile/postgraphile/blob/dd2b6b2b364efd015e46cd1faab4d37d2fd9fbad/src/postgraphile/http/createPostGraphileHttpRequestHandler.ts#L416

The reason is that the subscriptions flag currently opts you into a number of breaking changes that most people won't notice (e.g. it requires select grants on all primary keys), so this'd give a way to use subscriptions: true without enabling subscriptions... 👀

In future we can expand it to support query/mutation over websockets without allowing subscriptions themselves. We'll probably drop the subscriptions option from v5 as it'll be enabled by default.

[klemens-morgenstern]

Any further things I can improve?

[benjie]

Looking good! I'm going to need to spend some time analysing if there's any knock-on effects from this, but once the below suggestions are merged I think the ball's firmly in my court.

[klemens-morgenstern]

@benjie Just updated the build to pass the remaining format issue. Any thing else i can do to help you with this?

[benjie]

Leave it with me 👍

Dismissing review so I remember to review again

[russelgal]

Looks like there is a mutation error

  "errors": [
    {
      "errcode": "3B001",
      "extensions": {
        "exception": {
          "errcode": "3B001"
        }
      },
      "message": "savepoint \"graphql_mutation\" does not exist",
      "locations": [
        {
          "line": 1,
          "column": 42
        }
      ],
      "path": [
        "updateFormById"
      ]
    }
  ],
  "data": {
    "updateFormById": null
  }
}

[benjie]

Sorry this has taken so long to look at. There's a few minor things we need to tidy up, but my main concern is the security ramifications of opening websockets up like this. In particular CSRF-like attacks that can reuse user cookies to retrieve data from other sites. It's up to end users to protect their endpoints with websocketMiddlewares and Origin checks or CSRF tokens, but this will need documenting and really we should try and find a way to either enforce it or have the user confirm they've read that part of the documentation. What are your thoughts on this?

[klemens-morgenstern]

They all look fine to me. Regarding the security issue - tell me if I get this right: you can't even invalidate the websocket session once established. That is if the attacker hits the pgSettings and then keeps the websocket alive, his session won't be invalidated even if you were to reset everything establishing the connection.
If that is the case, this seems like a flaw in the websocket transport and would need to be addressed before mergint that.

[benjie]

That's one issue (which can be solved by using the session implementation in Graphile Starter which uses database-stored sessions which can be deleted and thus the RLS policies that pull from them would start failing); another is that websockets don't obey the same-origin policy so it's an easy to exploit attack vector if people don't set up their websocket connections well enough (this is still an issue with subscriptions, but it becomes significantly more dangerous when queries and mutations are allowed). There's also performance, stability and scalability concerns; to hear those in more depth I recommend you listen to the section of the last GraphQL Working Group starting here:

https://www.youtube.com/watch?v=FYF15RA9H3k&feature=youtu.be&t=3972

[benjie]

This was incorporated into #1338; now we're using a safer graphql websocket transport I'm happier opening this up 👍

Thanks for your work on this @klemens-morgenstern and sorry it took so long to resolve.