feat: pass subscription Headers as connection_init payload

[andreialecu]

Fixes #2181

This matches the behavior of graphql-playground by allowing to use Headers to configure the payload of the connection_init frame.

[changeset-bot]

🦋 Changeset detected

Latest commit: a76456c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@graphiql/toolkit	Minor
@graphiql/react	Patch
graphiql	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: andreialecu / name: Andrei Alecu (c52ebbf)

[andreialecu]

For more context about why this is required.

Apollo used to use graphql-playground, where this was supported.

Mercurius uses GraphiQL (which I recently bumped in mercurius-js/mercurius#855).

Mercurius allows enabling graphiql via a boolean flag like graphiql: true.

GraphiQL is just a generic tool as far as Mercurius is concerned. It's not possible to hook into graphiql's initialization script to pass connection options there. And even if it were, generating Authorization tokens and other required headers would be too complicated and potentially a security risk.

Passing headers manually should be supported. Otherwise, testing subscriptions in real-life applications built with Mercurius is almost impossible.

[acao]

This same PR has been declined before because it would cause unexpected behavior where the same headers are not shared between http and websockets requests. Users should be at least allowed to opt in to such behavior.

Please see create graphiql docs for instructions , or the related github discussion, on how to pass headers to http or websockets requests, in seperate places, when creating a graphiql fetcher

[acao]

wsConnectionParams.connectionParams.headers is what you probably want. See graphql-ws for documentation.

[andreialecu]

Unfortunately, you don't seem to want to take the time to understand this issue.

What you're proposing is not a solution to the issue presented in #2181. This is not about connection headers, and they cannot be defined statically at startup in any case. The connection payload needs to be configurable the same way HTTP headers are.

This same PR has been declined before because it would cause unexpected behavior where the same headers are not shared between http and websockets requests.

Link? There's no way to send both a subscription and regular query at the same time, so I don't understand how this can even be a problem.

[andreialecu]

Here's the relevant code in graphql-playground.

https://github.com/graphql/graphql-playground/blob/3a09b2e15e12cd1794126e9354e6b2154849f778/packages/graphql-playground-react/src/state/sessions/fetchingSagas.ts#L72-L74

[acao]

@andreialecu can you show me where in the graphql-ws spec this is prescribed?

A number users with playground have reported this as a bug, because it forces behaviour that is not the same with every implementation. users report bugs that the http headers are being passed as connection initialisation params accidentally. if you can show me where this is prescribed behavior in the proposed protocol spec, I will make it the default behavior

Playground is not a language/protocol reference implementation, but we are. Forcing such an opinionated behavior, even if common, will cause people to believe this implementation detail is part of the protocol spec.

This is already configurable using static values. You already have multiple ways to configure this behavior, both statically and programatically

[acao]

Programatically:

const headers = {
  "Authorization" : "bearer 1234"
}

const fetcher = createGraphiQLFetcher({
  url,
  headers,
  wsClient: createClient({
    url: subscriptionUrl,
    keepAlive: 2000,
    headers,
  }),
});

Statically:

const headers = {
  "Authorization" : "bearer 1234"
}

const fetcher = createGraphiQLFetcher({
  url,
  headers,
  wsConnectionParams: { headers }
});

[andreialecu]

It is NOT possible to send actual HTTP headers as part of websocket connections. The websocket browser API does not have such functionality.
whatwg/websockets#16

Here's confirmation of this by the graphql-ws author as well: enisdenjo/graphql-ws#267 (comment)

The only way to send what are essentially headers are via the connection_init frame, and this is already well known and documented pretty much everywhere in the GraphQL ecosystem as such. By passing the headers into connectionParams.

Here is one example: https://stackoverflow.com/questions/46992147/updating-authorization-header-in-websocket-graphql

Every answer you will find for this question will suggest the same pattern.

[acao]

Ok then,

Programatically:

const headers = {
  "Authorization" : "bearer 1234"
}

const fetcher = createGraphiQLFetcher({
  url,
  headers,
  wsClient: createClient({
    url: subscriptionUrl,
    keepAlive: 2000,
    ...headers,
  }),
});

Statically:

const headers = {
  "Authorization" : "bearer 1234"
}

const fetcher = createGraphiQLFetcher({
  url,
  headers,
  wsConnectionParams: headers
});

[acao]

@andreialecu your PR doesn't do anything besides add to the existing and configurable wsConnectionParams object. It is completely redundant. I'm sorry. Please review the code of conduct.

[andreialecu]

Please read my comments again:

#2719 (comment)
#2719 (comment)

[acao]

const headers = {
  "Authorization" : "bearer 1234"
}

const fetcher = createGraphiQLFetcher({
  url,
  headers,
  wsConnectionParams: headers
});

This already does the same exact thing your PR does. Please quit spamming

[andreialecu]

@andreialecu your PR doesn't do anything besides add to the existing and configurable wsConnectionParams object. It is completely redundant. I'm sorry. Please review the code of conduct.

@acao, since you seem to be having a bad day and I already wasted too much time trying to explain this, it will be my last message on the subject.

In the first comment I mentioned, which you haven't read, I said that Mercurius bundles GraphiQL as a convenience. What you suggested is impossible to configure because Mercurius does not allow customizing GraphiQL, nor should it.

So essentially, you're shutting this down from Mercurius users, and for others, you're suggesting to hardcode headers into the initialization script. Good solution there /s.

I'll try to see if I can send a PR to Mercurius to provide the option to use a sane GQL IDE.

Thank you.

[acao]

@andreialecu this is up to mercurius to expose the wsConnectionParams option. The client is already configurable in the way you want. It is up to Matteo & the mercius folks to allow you to configure these params. If they want to automatically pass http headers as connection params as you and other playground users want to, they are free to change their lovely server to do that.

[andreialecu]

I only wish you had an open mind and were open to understand graphql subscriptions - because it's obvious you don't use them.

enisdenjo/graphql-ws#263
https://github.com/enisdenjo/graphql-ws#auth-token

There's nothing that can be done by Mercurius here.

Passing wsConnectionParams does not make any sense. By the same logic, you should remove the Headers pane for regular HTTP connections as well and just provide a programmatic way to supply them.

[thomasheyenbrock]

Hey folks, I just saw that this discussion escalated and wanted to chime in to see if I can moderate. Let's see if we can find some common ground here!

@acao, you mentioned that a change like this would "cause unexpected behavior where the same headers are not shared between http and websockets requests". @andreialecu correctly pointed out that adding HTTP on websocket connections does not work in browsers. So to me it seems that we can't be 100% consistent in any case as this is just not possible with the given browser APIs. What exactly did you have in mind here when you said "unexpected"?

I looked up the code for createGraphiQLFetcher and here's my understanding of it:

• When handling subscriptions we are currently not doing anything with the fetcherOpts argument, so no matter the contents of the headers editor, the subscription ws-connection will always be the same.
• @acao was correct to point out that you can use the wsConnectionParams option when calling createGraphiQLFetcher to set the payload of the connection_init message.
• @andreialecu I'll try to phrase your counter-argument here: When using Mercurius you don't know any auth tokens yet when creating the fetcher, so you need a way for users to pass in their token "on runtime" via the GraphiQL UI. That's why the wsConnectionParams option does not work out for you. Did I put that correctly?

Coming back to @acao point of other users being confused about the headers editor contents showing up in the connection_init event: I don't see why we could not make that opt-in like you proposed. Basically adding a new option to createGraphiQLFetcher that toggles this behavior:

const fetcher = createGraphiQLFetcher({
  url: "https://my.graphql.api",
  subscriptionUrl: "ws://my.graphql.api",
  // If there are some "static" headers that should be sent with _every_ subscription, then define them here
  wsConnectionParams: { foo: "bar" },
  // By setting this to `true`, the fetcher will also pass the headers from the editor with the `connection_init` payload.
  // When not specifying this option the headers are not added (which is the current behavior)
  // (Naming of this new property TBD)
  passHeadersOnWsConnect: true
});

@andreialecu would the above approach work for the way you integrate GraphiQL in Mercurius?

[andreialecu]

Hey Thomas. Thank you for chiming in here. Yes, your understanding is correct.

Right now, headers do absolutely nothing for subscriptions, so it's not like this is a breaking change. It's essentially a fix, bringing some purpose to the subscription Headers pane. The old graphql playground got this one right.

I want to note that I don't see the need to put this behind an extra flag. I feel it adds unnecessary complexity without providing any additional value.

Either way, not having this feature built-in results in gimping subscription support for any real-life applications.

And as you correctly noted, it's often impossible to know the correct headers before starting the app.

And in addition, in a multi-tenant app I'm working on, I need to pass different tenant ids via connection options. It's just incredibly senseless to try to work with this any other way than dynamically via the UI.

[andreialecu]

Regarding previous reports from confused users: Please point me to any such threads. I'd be curious to see under which circumstances this confuses anyone.

The only threads I found were about users expecting them to be sent as actual HTTP headers on the WebSocket request. This expectation is simply invalid from a web standards point of view, as you also correctly pointed out. The same misunderstanding as @acao had.

Again: It's impossible to send actual HTTP headers for WebSockets, it's just not something any web browser supports.

[loa]

To make it easier for our users we have wrapped graphiql and control the auth-token in headers and wsConnectionParams.

We still want users to be able to override the auth-token through the graphiql headers panel, to test service accounts etc. This causes an inconsistency today since the headers panel will only override the values of headers for our queries/http-post queries but not wsConnectionParams for subscriptions/websocket connections.

It would be very helpful for our organization if request headers panel also applied to wsConnectionParams.

[dpangier]

We have exactly the same issue with gqlgen

[acao]

@loa that makes sense

@andreialecu as before, just make this something the user can control. Either an opt in or opt our behavior. I’m wary of making it the default, but either way, users should be able to opt out of this behavior as playground users have asked

[andreialecu]

users should be able to opt out of this behavior as playground users have asked

I have already mentioned in a previous comment that the only thing playground users asked about was why actual headers are not sent to the WebSocket request. It's the 5th time I mention this is simply impossible in this thread.

These are all such reports of this misunderstanding of how WebSockets work:
graphql/graphql-playground#1305
graphql/graphql-playground#1296
graphql/graphql-playground#853
graphql/graphql-playground#769

Please point me to a single thread of someone asking to opt out of the only real way to use what are essentially headers with WebSockets.

I could put it behind a flag, but you're creating more work downstream to other packages that use GraphiQL for no real gain. No one would ever want to opt out of this.

It's only going to waste users' time when they realize they need it and there's some obscure flag that needs to be turned on to get it working.

Please provide a single instance of proof for this opt-out requirement.

[thomasheyenbrock]

Thanks @andreialecu and @acao for being open to continue this discussion and reopening this PR 🙏

I'll just try to sum up the current state:

• We reached agreement that adding the contents of the headers editor to the payload of the init_connection message is something that makes sense.
• The remaining point of argument is whether this should be the default behavior for a fetcher created with createGraphiQLFetcher or if users need to opt-in using a flag.

I don't feel strongly about this. Since we reached partial agreement we should get some changes in that address this. Making it opt-in initially seems like the "safer" option, and we could at the same time continue a discussion about changing the default behavior. @andreialecu would you be up to changing the implementation of your PR or is this a hard no for you?

@acao if you still have some time and you can dig up some previous playground issues, that would be awesome! (I know you want to take a break so don't feel pressured to do so ❤️ )

[andreialecu]

It's not a hard no from me, but I feel there's no justification for making it opt-in or opt-out. I keep hearing about user reports, but no one can pinpoint a credible one. I did my best to search, and I couldn't find anything.

I feel that adding a flag for this has no positive effect. On the contrary, it adds extra maintenance costs to the code and further confusion for users down the line when they will first run into this issue.

Whoever wants to opt-out of this can leave the Headers pane empty.

If they're not attempting to use this specific feature, there's no point in having anything in the Headers pane. Currently, it does nothing.

[loa]

I don't have any strong feelings regarding flag or no, happy that it will be possible to get "feature parity" between http/post and websocket in graphiql.

A note though, from my understanding GraphQL is trying to be "protocol agnostic". With graphql-ws library currently being the "standard" for subscriptions I find it weird that the "request header panel" in GraphiQL only sets http requests headers and not graphql-ws connection params (which is the standard way to send headers in graphql-ws).

I agree with @andreialecu that this should be the default.

GraphiQL is an amazing project, thank you all for your hard work ❤️

[acao]

@loa our original goal was to have a network configuration sidebar tab where you define headers, ws connection params, url, and additional http and ws options. the headers tab was just a hold over til we get there. in most cases, the headers/connection params should be configured per project (schema, etc) and not per tab IMHO. Sure, maybe you need to change the content type for some requests, but it would be more ideal to have a default setting as well

thus why i created @graphiql/toolkit and createGraphiQLFetcher, as our proposed design in 2020 was to build this network tab as a plugin for the GraphiQLApp component, and then generate the props for GraphiQL component as an internal detail of the service layer component.

The idea was that GraphiQL remains the spec compliant component, whereas GraphiQLApp would be like playground and allow more opinionated architecture and complexity. The goal therein being that it's much easier to add new language and protocol spec features to graphiql if it's separated from all the complexity of a full-scale app for day to day use

Update:

Now that we have @graphiql/react, perhaps this pattern is no longer needed, and instead, I propose we use toolkit libraries internally in GraphiQL such as createFetcher() or the createRouter() @harshithpabbati was hoping to make for handling query params. I wish I'd thought of this for 2.0.0. So, with this approach, a user can provide fetcher or fetcherOpts, for example. It would be non-breaking to introduce in a minor version if you think of it...

[acao]

@loa we are not protocol agnostic, we are a reference implementation for the official language features and protocols for graphql, and the most advanced RFCs

https://github.com/graphql/graphql-over-http/blob/main/rfcs/GraphQLOverWebSocket.md

[andreialecu]

@acao, thank you for the thought-out response.

About the RFC:

https://github.com/graphql/graphql-over-http/blob/main/rfcs/GraphQLOverWebSocket.md

The reference implementation does specify the payload field for connection_init.

That payload is the only known/recommended way to pass authorization or other "header" information for graphql subscriptions (besides cookies). It's essentially equivalent to headers for regular HTTP operations.

The definition of HTTP Headers is:

HTTP headers let the client and the server pass additional information with an HTTP request or response

Since WebSocket requests are long-lived, the only request is at connection time. The way to pass additional information at that time is via payload.

I understand it's not spelled precisely that way in the RFC, but all online literature on the subject treats the GQL socket's connection options as interchangeable with HTTP headers. Their only purpose is to send additional information with the WebSocket connection's request.

https://www.apollographql.com/docs/react/data/subscriptions/#5-authenticate-over-websocket-optional
https://www.apollographql.com/docs/react/networking/authentication/#header

The "Headers" pane could probably be renamed to "Headers / Connection Options", but that seems unnecessary. You already cannot put a subscription, query, or mutation in the same tab. Alternatively, it could show a tooltip or help icon on the Headers tab that clarifies it works as connection options for subscriptions.

In addition, setting Headers individually per tab may be good to have in some instances. @loa already mentioned being able to test multiple service accounts quickly. I also have similar needs.

This PR is a 2 line change that gets the best of both worlds with the least friction IMO and is already a practice that most other IDEs conform to.

[acao]

@andreialecu indeed, this is what i meant by asking "is it in the spec?" originally. thank you for your well thought out response. for the record, I think this PR is fine, just talking about next steps and how to solve some of these issues upstream

Sure, maybe you need to change the content type for some requests, but it would be more ideal to have a default setting as well.

yes, you may need to change the header or tokens - which could be configured as separate environments as "user personas" as we had envisioned in 2020 with the sidebar tab. A user even had a PR for a network sidebar in 2019 that worked for a single environment, but we were in the midst of the conversion to typescript, and then the 2.x context/hooks rewrite, so we put it off until 2.x. Imagine if we had just merged the sidebar and the headers tab!

This is why i think we need both for this to be practical IMO. Environment based default configurations, and per-operation doc header tabs. What I want us to avoid is trying to solve the problem of user configurable global configurations with the per-operation(s) doc credential tabs.

The sidebar network tab will answer the complaints of graphiql and playground users who want to set default headers and connection params, so that when you choose the environment you want to execute your query against, it swaps out to use the connection params for that environment, and the headers tab per operation(s) doc only appends or overrides that. It's much more practical for most cases. I highly suggest checking out tools like Insomnia, Altair, Postman etc that implement this.

[thomasheyenbrock]

Alright, it seems to me that we can move ahead with this change! Thanks @acao for adding more context to the discussion, I believe this change is fine now and won't cause major problems down the road if we decide to invest more into a plugin that provides extended functionality.

@andreialecu I only have one thought regarding the implementation.

[acao]

merged! thank you @andreialecu !