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

Set cookie max Expires to 400 days #6413

Closed
wants to merge 1 commit into from
Closed

Conversation

paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Aug 23, 2022

Description
Based on Replacement to RFC 6265, the max Expires SHOULD NOT be greater than 400 days.

Expires attributes that are greater than the limit MUST be reduced to the limit.

Thus, browsers crop the expires limit to this amount.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Aug 23, 2022
@kenjis
Copy link
Member

kenjis commented Aug 23, 2022

It is draft-ietf-httpbis-rfc6265bis-latest, not RFC 6265.

The method name withNeverExpiring() will not match the behavior.
It is better to make deprecated the method and add a new one.

@paulbalandan
Copy link
Member Author

I stand corrected. The referenced internet draft is not RFC 6265, but is intended to obsolete and replace RFC 6265.

@MGatner
Copy link
Member

MGatner commented Aug 23, 2022

Authors:
L. Chen, Ed.
Google LLC
S. Englehardt, Ed.
Mozilla
M. West, Ed.
Google LLC
J. Wilander, Ed.
Apple, Inc

That's got some sign-off! This was just published (8/15) - anyone know the voting/implementation lifecycle of RFCs like this?

@paulbalandan
Copy link
Member Author

How is this a breaking change? I checked with Microsoft Edge today, and seems it is using the 400 days as expires of the request cookie. Well, aside from the tests expecting the expires of withNeverExpiring() to obviously change (but we don't have tests for that at the moment), the browser's request cookie will not have the expires at 5 years.

  1. Change PHP code
diff --git a/app/Config/Routes.php b/app/Config/Routes.php
index ff2ac645c..4b1899517 100644
--- a/app/Config/Routes.php
+++ b/app/Config/Routes.php
@@ -36,6 +36,7 @@ $routes->set404Override();
 // We get a performance increase by specifying the default
 // route since we don't have to scan directories.
 $routes->get('/', 'Home::index');
+$routes->get('/yard', 'Home::test');
 
 /*
  * --------------------------------------------------------------------
diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php
index 7f867e95f..d425bb207 100644
--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -2,10 +2,20 @@

 namespace App\Controllers;

+use CodeIgniter\Cookie\Cookie;
+
 class Home extends BaseController
 {
     public function index()
     {
-        return view('welcome_message');
+        $this->response->setCookie((new Cookie('cookie', 'monsters'))->withNeverExpiring());
+        return redirect()->withCookies()->to('/yard');   
+
+        // return view('welcome_message');
+    }
+
+    public function test()
+    {
+        var_dump($_COOKIE);
     }
 }
  1. Run php spark serve
    image

  2. Check Developer Tools > Network. Reload page. Check for the request cookie "cookie" and look for its Expires value.
    image

  3. Verify date shown.
    image

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

This is Firefox. At the moment, it has the expires at 5 years.
Screenshot 2022-08-25 17 20 46

@kenjis
Copy link
Member

kenjis commented Aug 25, 2022

Do we need the withNeverExpiring() method?
Why don't we just make it deprecated?

@MGatner
Copy link
Member

MGatner commented Aug 25, 2022

Yes let's deprecate it. If we have a feature that inherently creates differing behavior across browsers that's a detractor. I also don't like that this is called "never expiring" but sets for five years.

@paulbalandan?

@kenjis
Copy link
Member

kenjis commented Sep 2, 2022

See #6463

@kenjis kenjis closed this Sep 2, 2022
@paulbalandan paulbalandan deleted the cookie-expires-limit branch September 3, 2022 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants