CGI::Cookie doesn't conform to RFC2109, 'Max-Age' ( Needs peer review ) [rt.cpan.org #50576]

[leejo]

https://rt.cpan.org/Ticket/Display.html?id=50576

From http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=64308:

Conforming to RFC2109, the 'Expires' field is deprecated in favour of
'Max-Age'. Please provide both fields to conform both "old"
specification and "modern" one.

E.g. this one should have also 'Max-Age' "field".

$ perl -MCGI::Cookie -le '$a=CGI::Cookie->new(q{-name} => 'ID',
q{-expires} => "+3M"); print $a->as_string()'
ID=; path=/; expires=Thu, 14-Jan-2010 19:51:06 GMT


Perl version: 5.10.1.
OS: Debian.

[leejo]

yanick@babyl.dyndns.org - 2009-10-17 14:50:55

http://jackyf.livejournal.com/ via RT wrote:

From http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=64308:

Conforming to RFC2109, the 'Expires' field is deprecated in favour of
'Max-Age'. Please provide both fields to conform both "old"
specification and "modern" one.

E.g. this one should have also 'Max-Age' "field".

$ perl -MCGI::Cookie -le '$a=CGI::Cookie->new(q{-name} => 'ID',
q{-expires} => "+3M"); print $a->as_string()'
ID=; path=/; expires=Thu, 14-Jan-2010 19:51:06 GMT

I've peeked into CGI::Cookie, and support for max-age seems to already 

be stubbed in. I'll try to dig a little deeper today and see in the
history if there's a reason why it's not active yet.

Cheers,
`/anick

[leejo]

YANICK - 2009-11-01 19:33:51

A tentative patch is at http://github.com/yanick/CGI.pm/tree/rt-50576

Points of discussion/caveats:

• By default, the cookie now prints both 'Expires' and 'Max-Age' fields.
  Their display can be controlled via the show_expires/show_max_age
  methods or the -show_expires/-show_max_age new() arguments.
• The time to live can now be set using max_age() or expires(). They
  both modify the same underlaying value.
• If the time to live is specified as a date string, the module
  Date::Parse is used to convert it into a timestamp.
• CGI.pm has not been modified to follow the same convention (yet).

[leejo]

ALEXMV - 2009-11-20 17:53:54

On Sun Nov 01 14:33:51 2009, YANICK wrote:

A tentative patch is at http://github.com/yanick/CGI.pm/tree/rt-50576

Mark asked me to peer-review this patchset in order to help get a
release of CGI.pm out the door. It looks pretty good to me, with one
caveat, below.

• By default, the cookie now prints both 'Expires' and 'Max-Age' fields.
  Their display can be controlled via the show_expires/show_max_age
  methods or the -show_expires/-show_max_age new() arguments.

I somewhat agree with Mark on this -- I'm not convinced that such
fine-grained control is necessary. In my mind, sending both is unlikely
to ever cause problems.

• If the time to live is specified as a date string, the module
  Date::Parse is used to convert it into a timestamp.

This is going to possibly cause problems -- Date::Parse isn't in core,
but CGI.pm is. The options which provide similar functions, which are
in core, are Time::Piece->strptime or Time::Local::timegm. Hauling in
Time::Piece seems heavier-weight, but the implementation will be easier.
Thoughts?

• Alex

[leejo]

yanick@babyl.dyndns.org - 2009-11-20 18:13:13

On November 20, 2009 12:53:55 pm Alex Vandiver via RT wrote:

• By default, the cookie now prints both 'Expires' and 'Max-Age' fields.
  Their display can be controlled via the show_expires/show_max_age
  methods or the -show_expires/-show_max_age new() arguments.

I somewhat agree with Mark on this -- I'm not convinced that such
fine-grained control is necessary. In my mind, sending both is unlikely
to ever cause problems.

I also agree.  I couldn't think of an example where having both would cause 

harm, buuut I thought that giving peeps the control if they so desire was the
ultimate insurance against gotchas. But I agree, we can always take that out.

• If the time to live is specified as a date string, the module
  Date::Parse is used to convert it into a timestamp.

This is going to possibly cause problems -- Date::Parse isn't in core,
but CGI.pm is.

Very good point.

The options which provide similar functions, which are
in core, are Time::Piece->strptime or Time::Local::timegm. Hauling in
Time::Piece seems heavier-weight, but the implementation will be easier.
Thoughts?

Time::Piece would be good. And this will only happen in the weird cases where 

someone would enter the expiry date as an absolute date instead of a delta.

I'll be offline for the next 3 days, but I'll try to bring in those changes 

asap. Or any of you can fork my github branch and play with it, if you so
desire. :-)

Thanks!
`/anick

[leejo]

yanick@babyl.dyndns.org - 2009-11-30 01:07:22

Alex Vandiver via RT wrote:

• By default, the cookie now prints both 'Expires' and 'Max-Age' fields.
  Their display can be controlled via the show_expires/show_max_age
  methods or the -show_expires/-show_max_age new() arguments.

I somewhat agree with Mark on this -- I'm not convinced that such
fine-grained control is necessary. In my mind, sending both is unlikely
to ever cause problems.

Both arguments removed.  If an expire/max-age has been specified, we 

put both fields in the header (and if nothing has been specified, we
output nothing).

• If the time to live is specified as a date string, the module
  Date::Parse is used to convert it into a timestamp.

This is going to possibly cause problems -- Date::Parse isn't in core,
but CGI.pm is. The options which provide similar functions, which are
in core, are Time::Piece->strptime or Time::Local::timegm. Hauling in
Time::Piece seems heavier-weight, but the implementation will be easier.
Thoughts?

I've switched Date::Parse for Time::Piece.  The big difference is that 

Time::Piece requires its string to be in a very specific format, whereas
D::P would do its damnedest to figure out what the user want. Before
we didn't have this problem because we would just output verbatim an
recognized string -- something we can't do anymore before we have to
compute the time delta for max-age. That shouldn't be too much of an
issue, hopefully. I expect that very few people use absolute dates for
their expire data.

`/anick

[leejo]

MARKSTOS - 2009-12-03 04:13:52

I've looked at some more. Some notes:

• The original bug report from the year 2000. CGI::Cookie got max-age
  support in September 2002, in the 2.83 release.
• When researching this, I came across an article I'd written about it
  myself: http://mark.stosberg.com/blog/2008/12/cookie-handling-in-
  titanium-catalyst-and-mojo.html
• The Max-Age support currently in CGI.pm can be considered incomplete
  in the sense that it has a "documentation bug". Using max-age is not
  documented, and there is nothing to inform the user that max-age is the
  header used in the spec, or advise about which header to use in terms of
  practical browser compatibility. Since CGI.pm is a low-level library for
  a number of projects, I have some inclination to provide users very fine
  grained support: If use you 'expires' you set the Expires header, and if
  you use "max_age", you set the Max-Age header. If you want them both,
  you should set them both. Clearly, setting just "Expires" works fine for
  pratical cases. The downstream bug report is only complaining that we
  don't technically follow the spec... it's not reporting that any known
  browser actually recognizes only Max-Age and not Expires.

This approach would be perfectly backwards compatible, and saves a bit
processing power to generate both headers when really only one is
desired. Any of the projects built-on CGI.pm can choose to provide a way
to set both headers at once if they want to.

What I'm proposing has the fine-grained control of Yanick's original
proposal, but I think is a simpler way to accomplish it.

I think always setting both headers might introduce backwards
compatibility issues in some cases simply by being a different behavior,
even if it shouldn't normally cause a problem.

[leejo]

MARKSTOS - 2010-12-26 03:03:05

Yanick,

I have taken a second try at the Max-Age support. It is available here:

https://github.com/markstos/CGI.pm/tree/max-age-cookie-take-2

I manually incorporated some of the ideas and code from the original
branch where it applied.

I started over because as the ticket reflects, the line of thinking
changed. Originally, we worked on managing the two headers together.

The current proposed approach is to manage them individually. This is
completely backwards compatible, and gives the flexibilities to users or
frameworks to use their own method manage the fields together if they
prefer.

If the changes and docs look OK to use, please go ahead and pull the
work to my "master" branch and let's consider this done.

[leejo]

MARKSTOS - 2011-01-24 01:48:15

( If anyone else besides Yanick wants to provide a peer review, that's
always welcome, too. )