maxAge should not be translated to expires

[baloo]

The expires argument includes the timezone, but the UA clock might
be offset by a couple of hours (incorrect timezone), which would
end-up in the following situation:

• UTC is 2018-01-15 10:00 + 00:00
  Client has localtime: 2018-01-15 10:00 - 08:00 (8h in the future)
• Server set cookie with maxAge = 30min
• Server send:
  Set-Cookie: foo=bar; expires=2018-01-15T10:30:00+00:00
• UA immediately expires the cookie
• UA send a new request without cookie.

Sadly incorrect UA clock is a common problem (at least for us),
maxAge is a correct option (as the time is calculated relative
to UA clock), but this is not an available option in this library.

Fix #58

[dougwilson]

Hi @baloo thanks for your pull request! So long story here is that this was an adopted module, so I cannot speak for the history (and of course changing it would be a breaking change and would need to be evaluated first). From my understanding it uses expires instead of max-age because there are clients that simply ignore the max-age attribute, and all the ones that do honor max-age also honor expires. I assume only expires is generated because it is les bytes. If max-age is sent, some of clients silently ending up with session-length cookies, which seems too surprising.

Here's also a blog about the two if it helps: http://mrcoles.com/blog/cookies-max-age-vs-expires/

Is there a way to make this pull request backwards-compatible in any way? Otherwise we'll need to hold this PR for the next major. Some backwards-compatible possibilities I'm thinking of:

1. What if both expires and max-age are sent? Will that cause issues on the clients in which the time zone is wrong in your scenario, or will those clients honor max-age over expires, thus fixing the issue?
2. If (1) is out, can this be an opt-in switch?

[dougwilson]

Waiting response from OP 👍

[dougwilson]

Another thing I noticed in the change is that prior to this PR, if both maxAge and expires are provided, maxAge would take precedence and determine the output of the Set-Cookie header. Now they will both be there, and potentially have different values. To ease upgrades if this isn't to be held until the next major version, the condition of overriding expires if maxAge is set should be kept intact 👍 But if the desire is to hold this for the next major, then I don't think the behavior needs to be put back (though you may still feel otherwise -- major versions are allowed to be incompatible -- so you can change it regardless).

[baloo]

To my knowledge rfc6265 is the document that defines the cookies semantics.

To reply to your questions:

• What if both expires and max-age are sent? Will that cause issues on the clients in which the time zone is wrong in your scenario, or will those clients honor max-age over expires, thus fixing the issue?
  RFC reads:

   If a cookie has both the Max-Age and the Expires attribute, the Max-
   Age attribute has precedence and controls the expiration date of the
   cookie.  If a cookie has neither the Max-Age nor the Expires
   attribute, the user agent will retain the cookie until "the current
   session is over" (as defined by the user agent).

Maybe just ignore expires if maxAge is specified. What would you think?

• Some UA ignores max-age:
  To my knownledge, only Internet Explorer and Edge are problematic. I plan to fix this in my application by encoding expires (server generated) along with the value of the cookie. Should the cookie be expired, the server will then be able to remove the cookie "server-side".

Another option would be to have a "maxAgeRfc" option and let maxAge to its current implementation.

Let me know what you think.

[dougwilson]

Unfortinately that RFC was not even drafted until April 2011 and web browsers existed before than. If Cookies were as simple as reading an RFC this would be really easy, but they all seem to have their own quirks. We need to test this in real web browser to validate. I was wondering for at least your specific scenario you described if you could test the result of a Set-Cookie with both headers and see if it honored the max-age over the expires so we'd know if sending both would actually work for you before committing to the change.

Anyway, I leave the changes up to you 👍 This PR is fine as-is, but is not backwards-compatible and thus I cannot give you a timeline in which I will accept it. If you're willing to wait, no need to make any changes. If you want it to land now, then it just needs to be backwards-compatible in a way you see fit. My thoughts on it I already gave. Let me know which way you want to go :)

[baloo]

Well, AFAIK the first rfc describing cookie (and max-age) was rfc2109 (February 1997). Expires was not even an option back then. RFC6265 is just the current document that is authoritative on it.

I'd prefer to have a fix now in cookies (as that would ease things up for me) so any change that could make it happen is welcome.

[dougwilson]

Great, looking forward to the changes 👍 After you push this, please ping me here because GitHub doesn't always alert me to when new changes are pushed up.

[baloo]

@dougwilson I've gone down the maxAgeRfc road. Let me know what you think.

[dougwilson]

What was the reason to not just send both? Does that not work? Because if it does, that would allow us to give the fix to all users of the module without breaking or them changing their code, seems like a win-win. I would like to hear why we cannot go down that road.

Also, need to add docs 👍

[dougwilson]

Also, don't forget to add tests in test/test.js as well, to test the header that's getting set. You don't need to duplicate the tests into restify/http/express, as the new test is consolidating those three into one 👍

[baloo]

Sending both header could work, but that gives little control to the user which I think is wrong.

[dougwilson]

The tests in this file should be testing the underlying properties of the cookie object for the most part, where the other tests (really just test.js now) testing the serialized header. You can test the header here too if you like, but at least you'll want to test the populated properties 👍

[dougwilson]

Sending both header could work, but that gives little control to the user which I think is wrong.

I mean, it's starting to sound like we're disagreeing here. I'd rather go that route of the breaking change route. The current proposed maxAgeRfc route I do not like at all, at least how it's implemented. It's just adding more complication than necessary. The RFCs you said were authoritive and they say that expires would be ignored. Why can't max-age be included in addition? Everything seems to say than when both are there, the clients that support max-age will use it and ignore expires and the clients that only understand expires (or broken ones like IE6-8 that do understand both) will use expires.

I mean, if you want just full control of every attribute without the module trying to help provide a balance of working everywhere, why not just use the module https://github.com/jshttp/cookie over this one?

[dougwilson]

If you're really looking to go down the maxAgeRfc road and can articulate why, exactly, we should go that road vs including both attributes, the maxAgeRfc should probably be implemented as a Boolean option to the Cookies instance, because it would seem like you'd just want your cookies to work that way, but have to change the name of the attribute on all your cookie set calls though-out your code.

[baloo]

I don't want my server to emit expires at all, but you are probably correct I should use a lower-level module. Because IE does not support max-age, if expires is still too short and the UA clock is incorrect, the cookie will be removed, giving me no-chance of having it back.
The IE behavior with max-age but no expires (making it a session cookie) is fine with me. A cookie expiring too soon is not.

[dougwilson]

The IE behavior with max-age but no expires (making it a session cookie) is fine with me. A cookie expiring too soon is not.

Right, ultimately it seems both choices are bad, in different ways. And it's very hard to detect when either one of these conditions occur. It would seem like the right thing to do is to force neither option on the module consumer and instead maybe we make it such that the consumer of this module must choose which type of loss is the acceptable for their application.

[baloo]

Definitely and what I'm trying to achieve is very particular, so using a cookie module is probably a better solution for me.