Apollo-server: wrong behavie when there are same keys in responseInit.headers

[anlijiu]

I use koajs/session for auth.

There should be "set-cookie": ["koa:sess=xxx;...", "koa:sess.sig=xxx;..."] in the response header, not "set-cookie": "koa:sess=xxx;..., koa:sess.sig=xxx;..." .

And of course the auth doesn't work.

After digging into the code , I think this line in runHttpQuery.ts should be for (const [name, value] of Object.entries(response.http.headers.raw())) {

Am I right ? Sorry for my poor english...

[abernix]

Thanks for reporting this issue! Your explanation seems possible, especially if there's a stringified array appearing in the headers (which seems to be what you're suggesting?).

Do you mind putting together a runnable reproduction that anyone can easily run to see the problem? It can be either a GitHub repository or an example built using the Apollo Server CodeSandbox template or by "re-mixing" the Apollo Server Glitch template.

If you aren't able to supply a runnable reproduction, we're happy to re-open this later on when one can be provided, but building reproductions takes a large portion of core contributor's and issue triager's time. Without a reproduction, we'll have to close this issue in the next 7 days. Hope you understand!

Regardless, thanks again for filing the issue since, reproduction or not, this information could be useful for a future visitor!

[anlijiu]

runnable reproduction: https://ft5pl.sse.codesandbox.io/

code: https://codesandbox.io/s/apollo-server-ft5pl

you will see "fake-set-cookie: sess:1111, sess.sig:1111" in the http response header, which should be
fake-set-cookie: sess:1111
fake-set-cookie: sess.sig:1111

[yurist38]

Any updates on this one? I clearly see that it's not possible to send multiple cookies now, faced this issue in my project too. Please take a look at the problem! Thanks!

[abernix]

@yurist38 Would you be able to consider looking at the reproduction and seeing what the solution might look like? Some preliminary research based on the reproduction would be great to share here. For starters, I believe that this could be isolated to the Koa integration.

In other words, I think it's possible that the Koa integration needs to join the array, whereas the Express integration might be doing this automatically (possibly thanks to Express, itself?)

[yurist38]

@abernix I found something I'd like to share.
If you look at the reproduction (here is my fork of the one from @anlijiu with express instead of koa) you will see that Headers implementation comes from node-fetch. I looked at the implementation of its append method and found out that according to the method description it's working correctly. This method supposed to append a value, not a header itself. So it does his job.
But another problem I see when I use set method: it overrides an existing header if there is one. In the case of Set-Cookie header (at least), we should be allowed to set multiple headers, that's the correct way of handling set-cookie as far as I understand...

[b2a3e8]

@yurist38 I can confirm your findings.
Also there are no issues or references I could find at node-fetch that indicate that they're planing to support multiple headers with the same name.

Because I'm facing this issue in my projects too, I thought about possible workarounds:

• First I tried using different casing in the header name (for example: set-cookie, Set-Cookie, SET-COOKIE, sET-cookie,...), but that doesnt work because node-fetch avoids header duplication due to casing.
• The only other thing in my mind is setting different headers like Set-Cookie-A, Set-Cookie-B, ... and then changing all those headers to Set-Cookie in a reverse proxy, aws api gateway or whatever is in front of your apollo server. Although I didnt try it, I think it will work but its very hacky.

[b2a3e8]

Btw, I created a package for easily setting response headers in apollo server.

[kgoggin]

I bumped into this, too. Just to clarify the use case, when using cookie-based auth it's common to set two cookies to mitigate against CSRF attacks (known as the double-submit cookie pattern). I do think this would be a good thing for Apollo Server to eventually support, but I fear that doing so would be a breaking change.

As @yurist38 pointed out, Apollo Server is leaning on node-fetch's Headers class which doesn't allow for setting the same header key more than once (which would appear to be against the official fetch standard which specifically mentions:

A header list is essentially a specialized multimap. An ordered list of key-value pairs with potentially duplicate keys.

Furthermore the Header class appears to stringify any value passed to set, such that ["foo", "bar"] will turn into "foo, bar". So, not only can we not call set twice with the same key, we can't pass something more complicated to set.

I'm running apollo-server-lambda, and Lambda has an API for setting multi-value headers via the mutliValueHeaders key in the response object. So, my current work-around is to:

1. Stringify an array of cookie values (using JSON.stringify so that I can parse it back out again)
2. Proxy the Apollo handler to check the value for set-cookie and parse it back into an array, and then set mutltiValueHeaders instead of plain headers.

Here's what that looks like for anyone curious:

const handleManyCookies = (headers = {}) => {
  if (headers["set-cookie"]) {
    try {
      const value = JSON.parse(headers["set-cookie"]);
      if (Array.isArray(value)) {
        return {
          multiValueHeaders: {
            ...headers,
            "set-cookie": value,
          },
        };
      }
    } catch (err) {
      return { headers };
    }
  }

  return { headers };
};

const proxiedHandler = (event, context, cb) => {
  const apolloHandler = server.createHandler({
    cors: {
      origin: "*",
      credentials: true,
    },
  });

  return apolloHandler(event, context, (err, response = {}) => {
    if (err) {
      cb(err);
    } else {
      const { headers, ...responseData } = response;
      const newHeaders = handleManyCookies(headers);

      cb(null, {
        ...responseData,
        ...newHeaders,
      });
    }
  });
};

I'd be wary of suggesting a way to address all of this in apollo-server because it feels like a pretty substantial change would be needed to the underlying way it handles headers. Even if node-fetch released support for multi-value headers tomorrow, there'd still need to be changes made in this library for correctly parsing that API change and applying it correctly to the various server plugins. To me, this feels like something the core team needs to decide if it wants to support (IMHO, it really ought to) and then come up with an approach that isn't dependent on node-fetch (an Apollo-specific headers implementation).

Hope all this is helpful both to anyone else who lands here looking for a way around this, and to the core team in deciding how to move forward with this issue.