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

Already on GitHub? Sign in to your account

Issue 23955: setcookie() to send Max-Age attribute #238

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

narfbg commented Dec 5, 2012

setcookie() should send not only the Expires, but also the Max-Age attribute.
Changed the session extension to send the Max-Age attribute as well.

As described in #23955, a user agent having an erroneously configured timezone would have issues when calculating when to expire the cookie.

As described in RFC6265, Max-Age must be implemented by user agents in the following manner:

  • Specifies delta time in seconds and when present.
  • Has precedence over Expires, when both are present.
  • User agents that don't support it should simply ignore it.

Given those characteristics, it only makes sense that both Expires and Max-Age are sent together.

This MIGHT also fix #43439, but I'm not really sure about it - should be checked.

Edit:

https://wiki.php.net/rfc/cookie_max-age

Contributor

weltling commented Dec 6, 2012

Reading the specs, both expires and max-age are optional:

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

Also, max-age can be negative.

I'd rather add a new arg to setcookie and let it up to user which combination of attributes he want's to send. That needs probably a small RFC and discussion.

Also - that attributes are case sensitive.

And even though - a test is always needed :)

Contributor

lstrojny commented Dec 6, 2012

Yes, this definitely needs an RFC. Besides that, it’s a good idea.

Contributor

narfbg commented Dec 6, 2012

Fixed the negative values - good point!

However, while adding another parameter to allow the user a choice of attributes would need an RFC, I don't see why this one would require that. The goal is to solve a problem, not just allow the user to solve it.
If both Expires and Max-Age are optional AND can be sent together - why the need to complicate it, if it has always sent Expires up to this point?

And yes - the attributes should be implemented in a case-sensitive way, that's why I've changed them. All of the RFCs that I read (the one linked in the PR description and those mentioned in the linked PHP bug IDs) mention them with an upper-case letter for each word in the attribute name.

Otherwise feel free to suggest any improvements on my code. I'm no C programmer and I've only written a few patches through the years. This one just seemed easy to do. :)

Contributor

weltling commented Dec 6, 2012

That's for instance what stands in the linked document.

If the attribute-name case-insensitively matches the string
"Expires", the user agent MUST process the cookie-av as follows.

The C coding looks fine so far, though i haven't tested yet.

The point about how to implement that is exactly what an RFC should solve. I get your idea not to complicate it now :) . Please look here for which RFCs already exist https://wiki.php.net/rfc . Once the RFC is there, give a note on the internals list. There will be sure more questions and improvements.

Also the tests are still a must :)

Contributor

narfbg commented Dec 6, 2012

So they say that the comparison is made in a case-insensitive manner (I'm assuming that, as nothing is mentioned about case-sensitive matches at all) - representing the attributes as they are specified in the RFC still doesn't hurt. I'm not getting what the argument here is all about - just trying to be consistent with the RFC. :)

I've read some of the RFCs at wiki.php.net, haven't used internals though. I have to post on both, you say?

I'll see what I can do about those tests. :)

Contributor

weltling commented Dec 6, 2012

Of course that capitals don't hurt, there was just not need for that and the patch would be cleaner.

First write an RFC (you'll need a wiki account probably), then expose it on internals. Everything happens for the first time somewhen :)

Contributor

narfbg commented Dec 7, 2012

My original code produced segmentation faults, now it works, with tests for setcookie() added. :)
I'll get on the RFC now.

Contributor

narfbg commented Dec 7, 2012

If only I had permissions to create pages on the wiki ...

Contributor

lstrojny commented Dec 15, 2012

That’s actually a turing test for people writing RFC. Can you find out how to get a Wiki account? :)

Contributor

narfbg commented Dec 15, 2012

I have one, the problem is that all pages are read-only and when I try to access a non-existent page it shows a suggestion that I can create it by clicking on a "Create this page" button, but no such button is shown anywhere.

When PHP 5.5.0 be officially stable released?

Contributor

narfbg commented Dec 19, 2012

I wrote about this on internals a week ago, btw: http://marc.info/?l=php-internals&m=135514355208843

Since the wiki appears to be read-only (again, I have an account), do I have to do anything else than just wait for it to gain attention?

@asifsaif90 This isn't the right place to ask, but they're still discussing the release schedule, so I don't think that anybody could answer you anyway.

Contributor

weltling commented Dec 20, 2012

Have you read the notes on the wiki registration page? There are some more steps needed to become write access.

Contributor

narfbg commented Dec 20, 2012

Oh, right ... thanks @weltling.
I had registered a few months ago and didn't need write access at the time, so I've completely ignored that part and didn't need to revisit the registration form again. I'll get on that now.

Contributor

narfbg commented Dec 28, 2012

Updated the description with a link to the RFC.

@smalyshev smalyshev commented on an outdated diff Dec 28, 2012

ext/session/session.c
@@ -1201,6 +1202,11 @@ static void php_session_send_cookie(TSRMLS_D) /* {{{ */
smart_str_appends(&ncookie, COOKIE_EXPIRES);
smart_str_appends(&ncookie, date_fmt);
efree(date_fmt);
+
+ char maxage[12];
+ sprintf(maxage, "%li", PS(cookie_lifetime));
@smalyshev

smalyshev Dec 28, 2012

Contributor

Please use snprintf or spprintf, never sprintf. Also, in this case probably smart_str_append_long would be appropriate.

@smalyshev smalyshev and 1 other commented on an outdated diff Dec 28, 2012

ext/standard/head.c
@@ -136,6 +136,11 @@ PHPAPI int php_setcookie(char *name, int name_len, char *value, int value_len, t
}
strlcat(cookie, dt, len + 100);
efree(dt);
+
+ char tsdelta[12];
+ sprintf(tsdelta, "%li", (long) difftime(expires, time(NULL)));
@smalyshev

smalyshev Dec 28, 2012

Contributor

Again, sprintf should not be used. snprintf or spprintf. Also, longest 64-bit value is 9,223,372,036,854,775,807 signed, 18,446,744,073,709,551,615 unsigned.

@narfbg

narfbg Dec 28, 2012

Contributor

A few lines above this, there is a sanity check preventing a year larger than 9999 to be supplied, so that limits the difference to ~253,202,544,000. Updated the length to 13, to account for the minus sign.

@smalyshev smalyshev and 2 others commented on an outdated diff Dec 28, 2012

ext/standard/head.c
@@ -144,18 +149,18 @@ PHPAPI int php_setcookie(char *name, int name_len, char *value, int value_len, t
}
if (path && path_len > 0) {
- strlcat(cookie, "; path=", len + 100);
+ strlcat(cookie, "; Path=", len + 100);
@smalyshev

smalyshev Dec 28, 2012

Contributor

Not sure why we need to mess with these?

@narfbg

narfbg Dec 28, 2012

Contributor

Just for consistency - those are all indeed case-insensitive, but RFCs all mention them with starting capital letters. I know it's irrelevant to the feature itself, but figured that since it's not a 300-line diff it won't be such a distraction.

@lstrojny

lstrojny Dec 28, 2012

Contributor

Let’s not change this. It might lead to strange behavior in browsers where browsers set it as a second cookie. Cookie handling in browsers in widely inconsistent and we should try hard not to break something by accident.

@smalyshev

smalyshev Dec 28, 2012

Contributor

I think it's better not to fix what isn't broken.

@narfbg

narfbg Dec 28, 2012

Contributor

OK, but "HttpOnly" for exaple is sent as "HttpOnly" by ext/session and "httponly" by setcookie() - which one do I keep?

@lstrojny

lstrojny Dec 29, 2012

Contributor

Both

@KalleZ KalleZ commented on an outdated diff Dec 28, 2012

ext/session/session.c
@@ -1201,6 +1202,11 @@ static void php_session_send_cookie(TSRMLS_D) /* {{{ */
smart_str_appends(&ncookie, COOKIE_EXPIRES);
smart_str_appends(&ncookie, date_fmt);
efree(date_fmt);
+
+ char maxage[12];
@KalleZ

KalleZ Dec 28, 2012

Contributor

The variable declaration should be at the top of the clause to be C-compatible, MSVC for example won't allow this

Contributor

narfbg commented Dec 29, 2012

OK, hopefully it's all good to go now.

Contributor

smalyshev commented Jan 2, 2013

looks ok now

Contributor

narfbg commented Jan 4, 2013

Great, I guess I'll be just waiting for it to get merged then. :)
Btw, could any of you guys link the RFC at wiki.php.net/rfc ? I don't have access to edit the list in there and I didn't get a response when I mailed webmaster@ about it.

Contributor

lstrojny commented Jan 4, 2013

@smalyshev will you merge or should I?

Comment on behalf of lstrojny at php.net:

Merged in 5.5 and master. Thanks for your contribution!

@php-pulls php-pulls closed this Jan 6, 2013

@narfbg narfbg deleted the unknown repository branch Jan 6, 2013

@narfbg narfbg referenced this pull request in bcit-ci/CodeIgniter Apr 4, 2013

Closed

Unstable CI Session #2384

I am sorry to say that, but his commit caused me some troubles.
The idea is great, but the way it was implemented is not complete.
You either allow some flag to disable it, or allow some way to define the current time stamp which this max-age can be calculated based on.
The problem is that, when I make a comparative test between two versions of my code, my code will maintain single value for the time stamp through over the two versions. But because this max-age addition, now, the headers are never matched between the two versions. And I need now to delete this part from all over the tests I do before comparing the two results.

Please, add some way to determine the time stamp of the max-age of the cookie, some way to disable its output completely, or any pretty solution to this matter.

Contributor

narfbg commented Nov 13, 2013

Tests ARE included in the patch and they are in pure PHP, predicting the value is easy.

Deleting it is much easier!
Actually I am talking about testing two PHP sites. Each one is returning html and headers. I do a comparative test between the two sites in millions of pages simulating the actual load while testing/comparing. Now, I need to modify all my tests to delete max-age cookie part before comparing. I found no way to make the cookie both numeric and dates values come in specific value if they are executed in different clock time.

In other way to explain, I set the time stamp of the request for both sites to be equal value, so all dates and functions will use that stamp for both sites. Previously, the cookie got its value only from the stamp I provide. Now, the max-age value is determined by subtracting the actual value (not the one I set and I found no way to control it) of now() time stamp from the time stamp I provide for expiration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment