Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maxAge should translate to the max-age directive #58

Closed
fiatjaf opened this issue Jun 25, 2015 · 8 comments
Closed

maxAge should translate to the max-age directive #58

fiatjaf opened this issue Jun 25, 2015 · 8 comments
Assignees
Labels

Comments

@fiatjaf
Copy link

fiatjaf commented Jun 25, 2015

the maxAge option is misleading. It should translate to the max-age cookie directive, not expires with Date.now() base.

Date.now causes issues when maxAge is low (a few hours), since it uses the server date, and the client date may be different due to timezones.

@dougwilson
Copy link
Contributor

Can you please explain the following/show me how to reproduce this? The reason I ask is because the rationale doesn't really make sense to me, as the time zone is included in the expires value, so there wouldn't be time zone confusion.

Date.now causes issues when maxAge is low (a few hours), since it uses the server date, and the client date may be different due to timezones.

@dougwilson dougwilson self-assigned this Jun 25, 2015
@fiatjaf
Copy link
Author

fiatjaf commented Jun 26, 2015

Ok, that was my confusion. I was trying to set maxAge in seconds instead of milliseconds and assumed that the problems were coming from this timezone thing. You're right.

But why don't you allow the max-age cookie directive and instead translate it into a fancy expires?

@fiatjaf fiatjaf closed this as completed Jun 26, 2015
@dougwilson
Copy link
Contributor

Gotcha, good to hear there doesn't seem to be an issue with the date :) I mean, 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). AFAIK it uses expires instead because there are still lots of UAs that simply ignore the max-age attribute, and all the ones that do honor max-age also honor expires, so we basically always have to include expires, so what is the purpose of including both? You can get away with only max-age if you are OK with some of your users 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/

@baloo
Copy link

baloo commented Feb 6, 2018

the maxAge argument should indeed translate to max-age and not expires.
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.

Hope this makes sense.

@ruudud
Copy link

ruudud commented Feb 7, 2018

Incorrect UA clock is something we also observe quite often, and the root cause if often broken clock sync, not incorrect timezone.

From the link pasted:

Every browser that supports max-age will ignore the expires regardless of it’s value, and likewise, Internet Explorer will ignore the max-age and just use expires.

..indicating that just setting both shouldn't pose a problem. And It's only IE <=8 that doesn't support max-age.

I find it very strange that nobody has brought this up before in a module heavily in use because of eg. Koa.

Edit: Saw the discussion in #94 now, sorry for adding to the confusion. I still think it's a more sane default to send both.

@dougwilson
Copy link
Contributor

@ruudud I'm happy to accept a PR that just sets both. The other PR was closed without making that change, so you're most welcome to make one! I can make the change as well, but just wanted to put that out there if you wanted to get credit for the work :D

@ruudud
Copy link

ruudud commented Feb 13, 2018

Did a bit of digging now.. Doesn't seem like Google use the max-age directive for any of their cookies when logging in users.

I'm guessing the reason is that they just run a check client-side, comparing with their server clock, and if it's off by too much, warn the user or whatever. Seems like an OK solution if you ask me..

@jakeprime
Copy link

I've made a new PR for this (#107) as I need the ability to be able to use max-age due to differences I am seeing between the server clock and come clients' clocks.

I'm sending both values so this shouldn't cause a problem with any older browsers, I can't see any downside to this. What do we think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants