Duplicate response headers overwritten, not appended

[sandydoo]

FastBoot provides us with an API to append headers to the response. If you set the same header more than once, FastBoot will store the values in an array. However, the Express middleware then loops through the flattened array of header entries and sets them using res.set(), which overwrites any duplicate headers with the last value.

fastboot-express-middleware/src/index.js

Lines 37 to 42 in 5a0981d

	let headers = result.headers;
	let statusMessage = result.error ? 'NOT OK ' : 'OK ';
	for (var pair of headers.entries()) {
	res.set(pair[0], pair[1]);
	}

Example

If you wanted to return several cookies, you might try the following:

let responseHeaders = this.get('fastboot.response.headers');
responseHeaders.append('set-cookie', 'first_cookie=...; path=/');
responseHeaders.append('set-cookie', 'second_cookie=...; path=/');

The first cookie gets overwritten by the second in the response.

In the wild

Both ember-simple-auth and ember-cookies seem to expect the cookies to be appended, not overwritten with the last value.

In ember-cookies: https://github.com/simplabs/ember-cookies/blob/0a0fb0e4d85b70041258829c5e7e2e4f1359ca7b/addon/services/cookies.js#L94-L105

  _writeFastBootCookie(name, value, options = {}) {
    let responseHeaders = this.get('_fastBoot.response.headers');
    let serializedCookie = this._serializeCookie(...arguments);


    if (!isEmpty(options.maxAge)) {
      options.maxAge *= 1000;
    }


    this._cacheFastBootCookie(...arguments);


    responseHeaders.append('set-cookie', serializedCookie);
  },

This innocuous code then produces subtle bugs in code like this in ember-simple-auth: https://github.com/simplabs/ember-simple-auth/blob/master/addon/session-stores/cookie.js#L246

this.get('_cookies').write(this.get('cookieName'), value, cookieOptions);
if (!isEmpty(expiration)) {
  let expirationCookieName = `${this.get('cookieName')}-expiration_time`;
  let cachedExpirationTime = this.get('_cookies').read(expirationCookieName);
  this.get('_cookies').write(expirationCookieName, this.get('cookieExpirationTime') || cachedExpirationTime, cookieOptions);
}

ember-simple-auth depends on the cookie store in FastBoot, so if you set an expiration date on your auth cookies, this expiration cookie will overwrite your auth cookie.

I assume this is not really the intended behaviour, especially for the append method of the fetch header API. I know Express previously provided all sorts of patches, including automatically appending duplicate header values (https://github.com/expressjs/express/wiki/Migrating-from-3.x-to-4.x#ressetheaderset-cookie-val).

The best solution I can come up with would be to handle the cookies with res.cookie here and let users pass in cookies via the FastBoot service. That's gonna require changes in 3 repos at least, so not sure how people feel about that.

We can filter out the cookie headers and set them all at once (which correctly sets multiple set-cookie headers), but we don't want to send more than one cookie with the same name. ember-simple-auth in particular will write the auth cookie 3-4 times for some reason. So, we'd have to parse the serialized cookies to check for duplicates – doesn't sound like a great idea.

[marcoow]

Right, that should probably just use res.cookie here.

[bobisjan]

I've encountered this issue recently, I see that #36 fixes this for Set-Cookie header, but all the "multivalue" headers should be supported?

Since both ember-cli and fastboot-app-server requires express#4.11+ than the response.append() could be used to fix the issue?

I'm able to provide a PR with the fix, if using response.append() would be appropriate. /cc @rwjblue

[rwjblue]

@bobisjan ya that seems good

[sandydoo]

Fixed in #60.