Allow setting httpOnly

[vincent99]

There is currently no support for setting httpOnly on cookies in write(), with an explicit assert saying it's not useful.

While what it says is true and you won't be able to read them from inside Ember, there are still useful reasons you might want to set a httpOnly cookie, e.g. if it is generated by the client but only needs to be readable by a backend service/API.

(If you prefer to keep the assert, it could instead be an option like yesReallyHttpOnly passed into write() that causes it to allow writing one, I just need some way to do it)

[marcoow]

This is a good point actually. Is it even possible to set a HTTP-only cookie in the browser via document.cookie though?

Also, it might make sense to replace the assertion with a debug log.

[marcoow]

I'm not sure I understand what this change does. Is this necessary?

[vincent99]

I didn't really intend to put this in the same PR because I haven't added tests for it yet. So I can pull it out into a separate PR if you like.

But basically every time something writes a cookie right now it gets added to the header array, even if you've previously written a cookie of the same name. ember-simple-auth in particular does this several times during a fastboot request, which results in sending a bunch of redundant info to the client.

So if you do

cookies.write('key', 'value1');
cookies.write('key', 'value2');

The response headers are sent as

Set-Cookie: key=value1
Set-Cookie: key=value2

This change updates/replaces the existing header entry if there's already one for the name instead of always appending a new entry.

[vincent99]

It does appear that you can't set httpOnly in a browser, which makes sense. We're using this inside fastboot (copying the cookies from a login fetch() response back to the fastboot response so that the client gets them). So I could lave the assert but wrap it in a check for !isFastBoot?

[vincent99]

Updated to allow httpOnly only in fastboot, show the assert in a browser, test both sides appropriately, and pull out the redundant header stuff into a separate PR.

[marcoow]

Looks good 👍 I'd just restructure the tests a bit for simplicity.

[marcoow]

There's dedicated describes for both FastBoot and the browser actually: here and here. Can this be split up and moved into the respective describes?

[vincent99]

I was just trying to minimize the change keeping the options checking in itValidatesWriteOptions(), but sure, moved to 2 separate tests in "in the browser" and "in the FastBoot server".

[marcoow]

thanks 🎉