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

Support for cookie expiry date #1701

Closed
aberba opened this Issue Mar 1, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@aberba

aberba commented Mar 1, 2017

HTTPServerResponse.setCookie doesn't see to have a way to set expiry date for cookies. Am i missing something? This is necessary for persistent cookies.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 2, 2017

Member

setCookie returns a Cookie object, that has maxAge and expires properties. If those get set before the response gets written, the corresponding cookie field should be set accordingly.

Member

s-ludwig commented Mar 2, 2017

setCookie returns a Cookie object, that has maxAge and expires properties. If those get set before the response gets written, the corresponding cookie field should be set accordingly.

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 5, 2017

so like:

auto c = res.setCookie(...);
c.maxAge = 10000;
c.expires = ??

?

aberba commented Mar 5, 2017

so like:

auto c = res.setCookie(...);
c.maxAge = 10000;
c.expires = ??

?

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 8, 2017

Is there a reason setCookie() doesn't handle this functionality?

aberba commented Mar 8, 2017

Is there a reason setCookie() doesn't handle this functionality?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 8, 2017

Member

Should be c.expires = toRFC822DateTimeString(Clock.currTime(UTC()) + 10000.seconds);.

The API is currently definitely not ideal, especially the fact that .expires accepts a raw string. I'm not sure if letting setCookie handle everything is a good idea, but something like a Cookie.expire(Duration) method that sets both parameters properly should be added as a minimum.

Member

s-ludwig commented Mar 8, 2017

Should be c.expires = toRFC822DateTimeString(Clock.currTime(UTC()) + 10000.seconds);.

The API is currently definitely not ideal, especially the fact that .expires accepts a raw string. I'm not sure if letting setCookie handle everything is a good idea, but something like a Cookie.expire(Duration) method that sets both parameters properly should be added as a minimum.

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 9, 2017

Yeah. That will look more cleaner

aberba commented Mar 9, 2017

Yeah. That will look more cleaner

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 9, 2017

How is maxAge different from expires? Seems the cooking HTTP header only accepts expires value.

aberba commented Mar 9, 2017

How is maxAge different from expires? Seems the cooking HTTP header only accepts expires value.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 9, 2017

Member

The attributes do the same, "Expires" is the earlier one based on absolute dates and is supported also by older versions of IE, while "Max-Age" was introduced later and is simpler to use for typical applications. I don't have the exact list of supported browser versions in my head, but for IE it still makes sense to set the expires header.

Member

s-ludwig commented Mar 9, 2017

The attributes do the same, "Expires" is the earlier one based on absolute dates and is supported also by older versions of IE, while "Max-Age" was introduced later and is simpler to use for typical applications. I don't have the exact list of supported browser versions in my head, but for IE it still makes sense to set the expires header.

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 15, 2017

About implementing expire in a different function, have you see how php's set_cookie (name, value, path, msExpire, httpOnly, ...). Expire value is quite important and needed to make cookies useful iñ the first place. Else session can do same as cookies do currently. Encoding can come after expire in an overload of setCookie()

aberba commented Mar 15, 2017

About implementing expire in a different function, have you see how php's set_cookie (name, value, path, msExpire, httpOnly, ...). Expire value is quite important and needed to make cookies useful iñ the first place. Else session can do same as cookies do currently. Encoding can come after expire in an overload of setCookie()

@aberba

This comment has been minimized.

Show comment
Hide comment
@aberba

aberba Mar 15, 2017

I mean expire parameter in setCookie overload. Good idea?

aberba commented Mar 15, 2017

I mean expire parameter in setCookie overload. Good idea?

@aberba aberba closed this Mar 15, 2017

@s-ludwig s-ludwig reopened this Aug 12, 2017

s-ludwig added a commit that referenced this issue Aug 12, 2017

Add better means to set cookie expiry times. Fixes #1701.
This adds Duration/SysTime overloads for expires/maxAge properties and adds an expires method that sets both attributes at the same time. The members of the Cookie class also get documentation comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment