From 1060f68b7dece287dfbc3dcf80680ca2ac015ffd Mon Sep 17 00:00:00 2001 From: Chris Watson Date: Wed, 26 Jun 2019 01:26:10 -0700 Subject: [PATCH 1/9] Proper handling of max-age and expires for cookies --- spec/std/http/cookie_spec.cr | 76 ++++++++++++++++++++++++++++-------- src/http/cookie.cr | 36 +++++++++++------ 2 files changed, 85 insertions(+), 27 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 89330c8e419b..b11e22e5d517 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -194,36 +194,80 @@ module HTTP it "parse max-age as seconds from current time" do cookie = parse_set_cookie("a=1; max-age=10") - delta = cookie.expires.not_nil! - Time.utc - delta.should be > 9.seconds - delta.should be < 11.seconds + cookie.max_age.should eq 10.seconds cookie = parse_set_cookie("a=1; max-age=0") - delta = Time.utc - cookie.expires.not_nil! - delta.should be > 0.seconds - delta.should be < 1.seconds + cookie.max_age.should eq 0.seconds + end + end + + describe "expiration_time" do + it "sets expiration_time to be current when max-age=0" do + cookie = parse_set_cookie("bla=1; max-age=0") + (Time.now - cookie.expiration_time.not_nil!).should be <= 1.seconds + end + + it "sets expiration_time with old date" do + cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan 1970 00:00:00 -0000") + cookie.expiration_time.should eq Time.utc(1970, 1, 1, 0, 0, 0) + end + + it "sets future expiration_time with max-age" do + cookie = parse_set_cookie("bla=1; max-age=1") + (cookie.expiration_time.not_nil!).should be > Time.now + end + + it "sets future expiration_time with max-age and future cookie creation time" do + cookie = parse_set_cookie("bla=1; max-age=1") + cookie_expiration = cookie.expiration_time(time_reference: Time.now + 20.seconds).not_nil! + (Time.now + 20.seconds).should be < cookie_expiration + (Time.now + 21.seconds).should be >= cookie_expiration + end + + it "sets future expiration_time with expires" do + cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000") + (cookie.expiration_time.not_nil! >= Time.utc(2020, 1, 1, 0, 0, 0)).should be_true + end + + it "returns nil expiration_time when expires and max-age are not set" do + cookie = parse_set_cookie("bla=1") + cookie.expiration_time.should be_nil end end describe "expired?" do - it "by max-age=0" do - parse_set_cookie("bla=1; max-age=0").expired?.should eq true + it "expired when max-age=0" do + cookie = parse_set_cookie("bla=1; max-age=0") + cookie.expired?.should be_true + end + + it "expired with old expires date" do + cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan 1970 00:00:00 -0000") + cookie.expired?.should be_true end - it "by old date" do - parse_set_cookie("bla=1; expires=Thu, 01 Jan 1970 00:00:00 -0000").expired?.should eq true + it "not expired with future max-age" do + cookie = parse_set_cookie("bla=1; max-age=1") + cookie.expired?.should be_false end - it "not expired" do - parse_set_cookie("bla=1; max-age=1").expired?.should eq false + it "not expired with future expires" do + cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000") + cookie.expired?.should be_false end - it "not expired" do - parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000").expired?.should eq false + it "sets past expiration_time with max-age and future time reference" do + cookie = parse_set_cookie("bla=1; max-age=1") + cookie_expiration = cookie.expiration_time(time_reference: Time.now - 20.seconds).not_nil! + (Time.now - 20.seconds).should be < cookie_expiration + (Time.now - 18.seconds).should be > cookie_expiration + cookie_expired = cookie.expired?(time_reference: Time.now - 20.seconds) + cookie_expired.should be_true end - it "not expired" do - parse_set_cookie("bla=1").expired?.should eq false + it "not expired when max-age and expires are not provided" do + cookie = parse_set_cookie("bla=1") + cookie.expired?.should be_false end end end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 822fb9bc8064..5eb63812650b 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -16,6 +16,7 @@ module HTTP property value : String property path : String property expires : Time? + property max_age : Time::Span? property domain : String? property secure : Bool property http_only : Bool @@ -25,9 +26,11 @@ module HTTP def_equals_and_hash name, value, path, expires, domain, secure, http_only def initialize(@name : String, value : String, @path : String = "/", - @expires : Time? = nil, @domain : String? = nil, - @secure : Bool = false, @http_only : Bool = false, - @samesite : SameSite? = nil, @extension : String? = nil) + @expires : Time? = nil, @max_age : Time::Span? = nil, + @domain : String? = nil, @secure : Bool = false, + @http_only : Bool = false, @samesite : SameSite? = nil, + @extension : String? = nil) + @creation_time = Time.now @name = URI.unescape name @value = URI.unescape value end @@ -35,6 +38,7 @@ module HTTP def to_set_cookie_header path = @path expires = @expires + max_age = @max_age domain = @domain samesite = @samesite String.build do |header| @@ -42,6 +46,7 @@ module HTTP header << "; domain=#{domain}" if domain header << "; path=#{path}" if path header << "; expires=#{HTTP.format_time(expires)}" if expires + header << "; max-age=#{max_age.total_seconds}" if max_age header << "; Secure" if @secure header << "; HttpOnly" if @http_only header << "; SameSite=#{samesite}" if samesite @@ -53,9 +58,20 @@ module HTTP "#{@name}=#{URI.escape value}" end - def expired? - if e = expires - e < Time.utc + def expiration_time(time_reference = @creation_time) + if max_age = @max_age + time_reference + max_age + else + @expires + end + end + + # Returns the expiration status of this cookie as a `Bool` given a creation time + def expired?(time_reference = @creation_time) + if @max_age == 0.seconds + true + elsif (time = expiration_time(time_reference)) && time < Time.now + true else false end @@ -112,16 +128,14 @@ module HTTP match = header.match(SetCookieString) return unless match - expires = if max_age = match["max_age"]? - Time.utc + max_age.to_i.seconds - else - parse_time(match["expires"]?) - end + expires = parse_time(match["expires"]?) + max_age = match["max_age"]?.try(&.to_i64.seconds) Cookie.new( match["name"], match["value"], path: match["path"]? || "/", expires: expires, + max_age: max_age, domain: match["domain"]?, secure: match["secure"]? != nil, http_only: match["http_only"]? != nil, From a73e9a3a7b5db6a05fcd33729d029a062fb101c5 Mon Sep 17 00:00:00 2001 From: Chris Watson Date: Wed, 26 Jun 2019 02:49:36 -0700 Subject: [PATCH 2/9] Update spec wording --- spec/std/http/cookie_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index b11e22e5d517..e64525069817 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -192,7 +192,7 @@ module HTTP parse_set_cookie("a=1; domain=127.0.0.1; path=/; HttpOnly").domain.should eq "127.0.0.1" end - it "parse max-age as seconds from current time" do + it "parse max-age as Time::Span from current time" do cookie = parse_set_cookie("a=1; max-age=10") cookie.max_age.should eq 10.seconds From aaacd61d72ee346c98e68be4a832ba2abd73fc7a Mon Sep 17 00:00:00 2001 From: Chris Watson Date: Wed, 26 Jun 2019 08:20:31 -0700 Subject: [PATCH 3/9] Updated spec wording --- spec/std/http/cookie_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index e64525069817..874e4a1f773b 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -192,7 +192,7 @@ module HTTP parse_set_cookie("a=1; domain=127.0.0.1; path=/; HttpOnly").domain.should eq "127.0.0.1" end - it "parse max-age as Time::Span from current time" do + it "parse max-age as Time::Span" do cookie = parse_set_cookie("a=1; max-age=10") cookie.max_age.should eq 10.seconds From 602fe498855a73e6acdc6cf6c879ab50ec2eeb9b Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 17 Jul 2019 11:30:30 -0700 Subject: [PATCH 4/9] Set max-age to int64 --- src/http/cookie.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 5eb63812650b..2a1fef4318ba 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -46,7 +46,7 @@ module HTTP header << "; domain=#{domain}" if domain header << "; path=#{path}" if path header << "; expires=#{HTTP.format_time(expires)}" if expires - header << "; max-age=#{max_age.total_seconds}" if max_age + header << "; max-age=#{max_age.total_seconds.to_i}" if max_age header << "; Secure" if @secure header << "; HttpOnly" if @http_only header << "; SameSite=#{samesite}" if samesite From b9759b96940cd955d97800fb515ec7d22ec3f733 Mon Sep 17 00:00:00 2001 From: Chris Watson Date: Wed, 17 Jul 2019 11:35:45 -0700 Subject: [PATCH 5/9] Remove total_seconds and replace with max_age.to_i Co-Authored-By: Blacksmoke16 --- src/http/cookie.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 2a1fef4318ba..ef5a42ce18aa 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -46,7 +46,7 @@ module HTTP header << "; domain=#{domain}" if domain header << "; path=#{path}" if path header << "; expires=#{HTTP.format_time(expires)}" if expires - header << "; max-age=#{max_age.total_seconds.to_i}" if max_age + header << "; max-age=#{max_age.to_i}" if max_age header << "; Secure" if @secure header << "; HttpOnly" if @http_only header << "; SameSite=#{samesite}" if samesite From f9a0fe61d353c5ee7b4a12a74e337e61a7efb96a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 8 Dec 2020 16:15:23 +0100 Subject: [PATCH 6/9] Fix outstanding improvements --- spec/std/http/cookie_spec.cr | 27 ++++++++------------------- src/http/cookie.cr | 34 +++++++++++++++++++--------------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 36806b7b619a..c3fca8d02e9a 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -205,7 +205,8 @@ module HTTP describe "expiration_time" do it "sets expiration_time to be current when max-age=0" do cookie = parse_set_cookie("bla=1; max-age=0") - (Time.utc - cookie.expiration_time.not_nil!).should be <= 1.seconds + expiration_time = cookie.expiration_time.should_not be_nil + expiration_time.should be_close(Time.utc, 1.seconds) end it "sets expiration_time with old date" do @@ -220,14 +221,15 @@ module HTTP it "sets future expiration_time with max-age and future cookie creation time" do cookie = parse_set_cookie("bla=1; max-age=1") - cookie_expiration = cookie.expiration_time(time_reference: Time.utc + 20.seconds).not_nil! - (Time.utc + 20.seconds).should be < cookie_expiration - (Time.utc + 21.seconds).should be >= cookie_expiration + cookie_expiration = cookie.expiration_time.should_not be_nil + cookie_expiration.should be_close(Time.utc, 1.seconds) + + cookie.expired?(time_reference: cookie.creation_time + 1.second).should be_true end it "sets future expiration_time with expires" do cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000") - (cookie.expiration_time.not_nil! >= Time.utc(2020, 1, 1, 0, 0, 0)).should be_true + cookie.expiration_time.should eq Time.utc(2020, 1, 1, 0, 0, 0) end it "returns nil expiration_time when expires and max-age are not set" do @@ -253,23 +255,10 @@ module HTTP end it "not expired with future expires" do - cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan #{Time.utc.year + 1} 00:00:00 -0000") + cookie = parse_set_cookie("bla=1; expires=Thu, 01 Jan #{Time.utc.year + 2} 00:00:00 -0000") cookie.expired?.should be_false end - it "sets past expiration_time with max-age and future time reference" do - cookie = parse_set_cookie("bla=1; max-age=1") - cookie_expiration = cookie.expiration_time(time_reference: Time.utc - 20.seconds).not_nil! - (Time.utc - 20.seconds).should be < cookie_expiration - (Time.utc - 18.seconds).should be > cookie_expiration - cookie_expired = cookie.expired?(time_reference: Time.utc - 20.seconds) - cookie_expired.should be_true - end - - it "not expired" do - parse_set_cookie("bla=1; expires=Thu, 01 Jan #{Time.utc.year + 2} 00:00:00 -0000").expired?.should eq false - end - it "not expired when max-age and expires are not provided" do cookie = parse_set_cookie("bla=1") cookie.expired?.should be_false diff --git a/src/http/cookie.cr b/src/http/cookie.cr index db7a47e9a29f..22845b5103c5 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -19,23 +19,23 @@ module HTTP property value : String property path : String property expires : Time? - property max_age : Time::Span? property domain : String? property secure : Bool property http_only : Bool property samesite : SameSite? property extension : String? + property max_age : Time::Span? + getter creation_time : Time def_equals_and_hash name, value, path, expires, domain, secure, http_only - def initialize(@name : String, value : String, @path : String = "/", + def initialize(@name : String, @value : String, @path : String = "/", @expires : Time? = nil, @domain : String? = nil, @secure : Bool = false, @http_only : Bool = false, @samesite : SameSite? = nil, @extension : String? = nil, - @max_age : Time::Span? = nil) - @creation_time = Time.utc - @name = name - @value = value + *, + @max_age : Time::Span? = nil, @creation_time = Time.utc) + raise "Invalid max_age" if @max_age.try { |max_age| max_age < Time::Span.zero } end def to_set_cookie_header @@ -69,20 +69,24 @@ module HTTP URI.encode_www_form(value, io) end - def expiration_time(time_reference = @creation_time) + # Returns the expiration time of this cookie. + def expiration_time : Time? if max_age = @max_age - time_reference + max_age + @creation_time + max_age else @expires end end - # Returns the expiration status of this cookie as a `Bool` given a creation time - def expired?(time_reference = @creation_time) - if @max_age == 0.seconds - true - elsif (time = expiration_time(time_reference)) && time < Time.utc + # Returns the expiration status of this cookie as a `Bool`. + # + # *time_reference* can be passed to use a different reference time for + # comparison. Default is the current time (`Time.utc`). + def expired?(time_reference = Time.utc) : Bool + if @max_age.try &.zero? true + elsif expiration_time = self.expiration_time + expiration_time <= time_reference else false end @@ -146,12 +150,12 @@ module HTTP URI.decode_www_form(match["name"]), URI.decode_www_form(match["value"]), path: match["path"]? || "/", expires: expires, - max_age: max_age, domain: match["domain"]?, secure: match["secure"]? != nil, http_only: match["http_only"]? != nil, samesite: match["samesite"]?.try { |v| SameSite.parse? v }, - extension: match["extension"]? + extension: match["extension"]?, + max_age: max_age, ) end From 136a2ce3260a1b09e1024ed3577ec1c3e7739726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 8 Dec 2020 16:15:51 +0100 Subject: [PATCH 7/9] Deprecate positional arguments in Cookie constructor --- src/http/cookie.cr | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 22845b5103c5..87b6a088495e 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -29,11 +29,28 @@ module HTTP def_equals_and_hash name, value, path, expires, domain, secure, http_only - def initialize(@name : String, @value : String, @path : String = "/", + @[Deprecated("Use named arguments instead.")] + def self.new(_name : String, _value : String, _path : String = "/", + _expires : Time? = nil, _domain : String? = nil, + _secure : Bool = false, _http_only : Bool = false, + _samesite : SameSite? = nil, _extension : String? = nil) : self + new( + _name, _value, + path: _path, expires: _expires, domain: _domain, secure: _secure, + http_only: _http_only, samesite: _samesite, extension: _extension + ) + end + + def self.new(_name : String, _value : String) : self + new(name: _name, value: _value) + end + + def initialize(@name : String, @value : String, + *, + @path : String = "/", @expires : Time? = nil, @domain : String? = nil, @secure : Bool = false, @http_only : Bool = false, @samesite : SameSite? = nil, @extension : String? = nil, - *, @max_age : Time::Span? = nil, @creation_time = Time.utc) raise "Invalid max_age" if @max_age.try { |max_age| max_age < Time::Span.zero } end From 480f63f2f3c8cf12ac6b693350c18bf1a01808b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Wed, 31 Mar 2021 12:18:52 +0200 Subject: [PATCH 8/9] Revert initializer, should be a separate change --- src/http/cookie.cr | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index d3f1f5f4333b..1450c244b02e 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -29,28 +29,10 @@ module HTTP def_equals_and_hash name, value, path, expires, domain, secure, http_only, samesite, extension - @[Deprecated("Use named arguments instead.")] - def self.new(_name : String, _value : String, _path : String = "/", - _expires : Time? = nil, _domain : String? = nil, - _secure : Bool = false, _http_only : Bool = false, - _samesite : SameSite? = nil, _extension : String? = nil) : self - new( - _name, _value, - path: _path, expires: _expires, domain: _domain, secure: _secure, - http_only: _http_only, samesite: _samesite, extension: _extension - ) - end - - def self.new(_name : String, _value : String) : self - new(name: _name, value: _value) - end - # Creates a new `Cookie` instance. # # Raises `IO::Error` if *name* or *value* are invalid as per [RFC 6265 ยง4.1.1](https://tools.ietf.org/html/rfc6265#section-4.1.1). - def initialize(name : String, value : String, - *, - @path : String? = nil, + def initialize(name : String, value : String, @path : String? = nil, @expires : Time? = nil, @domain : String? = nil, @secure : Bool = false, @http_only : Bool = false, @samesite : SameSite? = nil, @extension : String? = nil, From c0d4d07fbfaa48ec3284246e6a8cac61f77c6229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Mon, 23 Aug 2021 14:22:03 +0200 Subject: [PATCH 9/9] Remove parenthesis --- spec/std/http/cookie_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 2e27678e55c8..597276b959f1 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -320,7 +320,7 @@ module HTTP it "sets future expiration_time with max-age" do cookie = parse_set_cookie("bla=1; max-age=1") - (cookie.expiration_time.not_nil!).should be > Time.utc + cookie.expiration_time.not_nil!.should be > Time.utc end it "sets future expiration_time with max-age and future cookie creation time" do