Send max-age header as well as expires if it is set

[jakeprime]

It can be necessary to use max-age rather than rely on expires, when the client and server clocks are not in sync.

Older versions of IE will ignore the max-age and use the expires value. Any browser which supports max-age will use it in preference of expires if both are present.

[baloo]

Thank you for fixing this!

For reference (rfc6265):

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).

[jakeprime]

I've realised that the maxAge value was previously treated as being in milliseconds. However the spec states that it should be in seconds.

rfc6265:

The Max-Age attribute indicates the maximum lifetime of the cookie,
represented as the number of seconds until the cookie expires

Unfortunately this is a breaking change for anyone using maxAge instead of expires. I'm not sure what the best thing to do about that is, but it doesn't seem right to keep using milliseconds for that?

[baloo]

last commit is changing semantics of the maxAge parameters regarding the expires. This would confuse users of this lib when upgrading and might break code, I would suggest dividing by 1000 when issuing the max-age instead.

[jakeprime]

@baloo Yes you're right, it doesn't make sense to change it to seconds now, I've reverted that. I see this has been discussed before (#65 (comment)).

I understand why it makes sense to be able to set the age in milliseconds, as that is the norm in Javascript, but it is unfortunate that the variable has the same name as the attribute in the cookies spec which is defined as being in seconds. Still, best not add something that will break anyone's projects.

[fionnbharra]

@dougwilson will this be included in the next release?

[lerarosalene]

@dougwilson any plans on accepting this PR?

[dougwilson]

It looks like this is ultimately a duplicate of #94 , and I believe the OP was aware of this by the thread post in #58 . This PR is just on hold because it is not backwards-compatible, as the thread in #94 discusses. If the desire is to land it in our current line, ideally this change would be opt-in instead of on (and not even the ability to disable) by default.

But of course, to be clear, the current PR contents are I think fine, as long as the desire here it to land it in a new major version.

[jakeprime]

@dougwilson I believe this is a fully backwards-compatible change. There is no change in the way the cookie is set, and as far as I can tell the only functional change would be in the case where a cookie is set using max-age only and the UA clock is different to the server, which would currently result in an incorrect expiry.

[dougwilson]

So the non-backwards-compatible change is outlined in the previous PR #94 ; the TL;DR is that different user agents will resolve a mis-match between expires and max-age differently, with some preferring expires over max-age and some preferring max-age over expires. Introducing this as a backwards-compatible change will introduce this change into folk's web apps who are relying on semver to keep things functioning but this will result in changes to how the browsers are interpreting the ultimate cookie expiration set by those servers.

[lerarosalene]

different user agents will resolve a mis-match between expires and max-age differently

@dougwilson it is only about old IE browsers and, moreover, it's only the case if server clock and browser clock are out of sync. Thus, it fixes this situation in new browsers and not fixes it in IE.

[dougwilson]

I understand that, but also I don't know every user agent out there (hint: there are many more user agents than just web browser a user would use, and even those are numerous). Just think about it from this module's contract: you can see in the tests, this module has been promising to users that max-age would explicitly not be set. This changes that contract we have with existing users of this module. The not setting of max-age was no accident, which could thus be argued as a bug fix, but rather a purposeful choice. We are, though, here to make the purposeful choice to start including it, but the contract of module consumers changes with such a change.

[panva]

it's only the case if server clock and browser clock are out of sync

Isn't that the case like - almost always...