From 39aaf841c095cc894b3cc260ad542c107de3ccc0 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Mon, 19 Apr 2021 20:41:46 -0400 Subject: [PATCH 1/9] Validate cookie name prefixes --- spec/std/http/cookie_spec.cr | 62 ++++++++++++++++++++++++++++++++++++ src/http/cookie.cr | 8 +++++ 2 files changed, 70 insertions(+) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 46fb5508c540..72750b35b073 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -35,9 +35,19 @@ module HTTP expect_raises IO::Error, "Invalid cookie name" do HTTP::Cookie.new("", "") end + expect_raises IO::Error, "Invalid cookie name" do HTTP::Cookie.new("\t", "") end + + expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do + HTTP::Cookie.new("__Secure-foo", "bar") + end + + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + HTTP::Cookie.new("__Host-foo", "bar") + end + # more extensive specs on #name= end @@ -45,6 +55,7 @@ module HTTP expect_raises IO::Error, "Invalid cookie value" do HTTP::Cookie.new("x", %(foo\rbar)) end + # more extensive specs on #value= end end @@ -55,19 +66,64 @@ module HTTP expect_raises IO::Error, "Invalid cookie name" do cookie.name = "" end + expect_raises IO::Error, "Invalid cookie name" do cookie.name = "\t" end + expect_raises IO::Error, "Invalid cookie name" do cookie.name = "\r" end + expect_raises IO::Error, "Invalid cookie name" do cookie.name = "a\nb" end + expect_raises IO::Error, "Invalid cookie name" do cookie.name = "a\rb" end end + + it "raises on invalid cookie with __Secure- prefix" do + cookie = HTTP::Cookie.new "x", "" + + expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do + cookie.name = "__Secure-x" + end + + cookie.secure = true + + cookie.name = "__Secure-x" + cookie.name.should eq "__Secure-x" + end + + it "raises on invalid cookie with __Host- prefix" do + cookie = HTTP::Cookie.new "x", "", domain: "example.com" + + # Not secure + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.name = "__Host-x" + end + + cookie.secure = true + + # Invalid path + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.name = "__Host-x" + end + + cookie.path = "/" + + # Has domain + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.name = "__Host-x" + end + + cookie.domain = nil + + cookie.name = "__Host-x" + cookie.name.should eq "__Host-x" + end end describe "#value=" do @@ -76,21 +132,27 @@ module HTTP expect_raises IO::Error, "Invalid cookie value" do cookie.value = %(foo\rbar) end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = %(foo"bar) end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = "foo;bar" end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = "foo\\bar" end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = "foo\\bar" end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = "foo bar" end + expect_raises IO::Error, "Invalid cookie value" do cookie.value = "foo,bar" end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index c86a16bdf7e7..6d99a4375a94 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -58,6 +58,14 @@ module HTTP raise IO::Error.new("Invalid cookie name") end end + + # Enforce cookie prefix requirements as per https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.3. + case name + when .starts_with? "__Secure-" + raise ArgumentError.new "Invalid cookie name. Has '__Secure-' prefix, but is not secure." unless @secure + when .starts_with? "__Host-" + raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements." if !@secure || @path != "/" || !@domain.nil? + end end # Sets the value of this cookie. From 0bb6cd23fbb95ab19e658a1a34c3955465804029 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Mon, 19 Apr 2021 20:46:56 -0400 Subject: [PATCH 2/9] Doc update --- src/http/cookie.cr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 6d99a4375a94..45c7e8379a12 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -30,6 +30,7 @@ module HTTP # 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). + # Raises an `ArgumentError` if *name* has a security prefix but is invalid as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). def initialize(name : String, value : String, @path : String? = nil, @expires : Time? = nil, @domain : String? = nil, @secure : Bool = false, @http_only : Bool = false, @@ -43,6 +44,7 @@ module HTTP # Sets the name of this cookie. # # Raises `IO::Error` if the value is invalid as per [RFC 6265 §4.1.1](https://tools.ietf.org/html/rfc6265#section-4.1.1). + # Raises an `ArgumentError` if *name* has a security prefix but is invalid as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). def name=(name : String) validate_name(name) @name = name From 8b57f7808cc0b08f1d5f23ca7f5b7757b284310d Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Tue, 20 Apr 2021 21:34:00 -0400 Subject: [PATCH 3/9] Break validation out into public methods Better supports Cookie builder pattern --- spec/std/http/cookie_spec.cr | 104 ++++++++++++++++++++++------------- src/http/cookie.cr | 37 +++++++++---- 2 files changed, 94 insertions(+), 47 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 72750b35b073..e9a5e8bee5bc 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -40,14 +40,6 @@ module HTTP HTTP::Cookie.new("\t", "") end - expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do - HTTP::Cookie.new("__Secure-foo", "bar") - end - - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do - HTTP::Cookie.new("__Host-foo", "bar") - end - # more extensive specs on #name= end @@ -58,6 +50,16 @@ module HTTP # more extensive specs on #value= end + + it "raises on invalid cookie with prefix" do + expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do + HTTP::Cookie.new("__Secure-foo", "bar") + end + + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + HTTP::Cookie.new("__Host-foo", "bar") + end + end end describe "#name=" do @@ -84,45 +86,20 @@ module HTTP end end - it "raises on invalid cookie with __Secure- prefix" do + it "doesn't raise on invalid cookie with __Secure- prefix" do cookie = HTTP::Cookie.new "x", "" - expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do - cookie.name = "__Secure-x" - end - - cookie.secure = true - cookie.name = "__Secure-x" cookie.name.should eq "__Secure-x" + cookie.secure.should be_false end - it "raises on invalid cookie with __Host- prefix" do + it "doesn't raise on invalid cookie with __Host- prefix" do cookie = HTTP::Cookie.new "x", "", domain: "example.com" - # Not secure - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do - cookie.name = "__Host-x" - end - - cookie.secure = true - - # Invalid path - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do - cookie.name = "__Host-x" - end - - cookie.path = "/" - - # Has domain - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do - cookie.name = "__Host-x" - end - - cookie.domain = nil - cookie.name = "__Host-x" cookie.name.should eq "__Host-x" + cookie.secure.should be_false end end @@ -179,6 +156,59 @@ module HTTP it { HTTP::Cookie.new("empty-value", "").to_set_cookie_header.should eq "empty-value=" } end + + describe "#valid? & #validate!" do + it "raises on invalid cookie with __Secure- prefix" do + cookie = HTTP::Cookie.new "x", "" + cookie.name = "__Secure-x" + + cookie.valid?.should be_false + + expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do + cookie.validate! + end + + cookie.secure = true + + cookie.name = "__Secure-x" + cookie.name.should eq "__Secure-x" + cookie.valid?.should be_true + end + + it "raises on invalid cookie with __Host- prefix" do + cookie = HTTP::Cookie.new "x", "", domain: "example.com" + cookie.name = "__Host-x" + + cookie.valid?.should be_false + + # Not secure + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.validate! + end + + cookie.secure = true + cookie.valid?.should be_false + + # Invalid path + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.validate! + end + + cookie.path = "/" + cookie.valid?.should be_false + + # Has domain + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + cookie.validate! + end + + cookie.domain = nil + + cookie.name = "__Host-x" + cookie.name.should eq "__Host-x" + cookie.valid?.should be_true + end + end end describe Cookie::Parser do diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 45c7e8379a12..266945378403 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -30,7 +30,7 @@ module HTTP # 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). - # Raises an `ArgumentError` if *name* has a security prefix but is invalid as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). + # Raises `ArgumentError` if *name* has a security prefix but the requirements are not met as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). def initialize(name : String, value : String, @path : String? = nil, @expires : Time? = nil, @domain : String? = nil, @secure : Bool = false, @http_only : Bool = false, @@ -39,12 +39,12 @@ module HTTP @name = name validate_value(value) @value = value + self.validate! end # Sets the name of this cookie. # # Raises `IO::Error` if the value is invalid as per [RFC 6265 §4.1.1](https://tools.ietf.org/html/rfc6265#section-4.1.1). - # Raises an `ArgumentError` if *name* has a security prefix but is invalid as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). def name=(name : String) validate_name(name) @name = name @@ -60,14 +60,6 @@ module HTTP raise IO::Error.new("Invalid cookie name") end end - - # Enforce cookie prefix requirements as per https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.3. - case name - when .starts_with? "__Secure-" - raise ArgumentError.new "Invalid cookie name. Has '__Secure-' prefix, but is not secure." unless @secure - when .starts_with? "__Host-" - raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements." if !@secure || @path != "/" || !@domain.nil? - end end # Sets the value of this cookie. @@ -125,6 +117,31 @@ module HTTP end end + # Returns `false` if `#name` has a security prefix but the requirements are not met as per + # [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3), + # otherwise returns `true`. + def valid? : Bool + self.valid_secure_prefix? && self.valid_host_prefix? + end + + # Raises `ArgumentError` if `self` is not `#valid?`. + def validate! + raise ArgumentError.new "Invalid cookie name. Has '__Secure-' prefix, but is not secure." unless self.valid_secure_prefix? + raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements." unless self.valid_host_prefix? + end + + private def valid_secure_prefix? : Bool + has_secure_prefix = @name.starts_with?("__Secure-") + + !has_secure_prefix || (has_secure_prefix && @secure) + end + + private def valid_host_prefix? : Bool + has_host_prefix = @name.starts_with?("__Host-") + + !has_host_prefix || (has_host_prefix && @secure && "/" == @path && @domain.nil?) + end + # :nodoc: module Parser module Regex From 45d9b08f02e5519739926ee0cca988a0e4490704 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Sun, 4 Dec 2022 10:48:05 -0500 Subject: [PATCH 4/9] Automatically apply security prefix if all related properties are `nil` --- spec/std/http/cookie_spec.cr | 23 ++++++++++++++++++----- src/http/cookie.cr | 15 ++++++++++++++- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index c80d1f0f6228..ec804d4ab222 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -47,13 +47,26 @@ module HTTP end end - it "raises on invalid cookie with prefix" do - expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do - HTTP::Cookie.new("__Secure-foo", "bar") + describe "with a security prefix" do + it "raises on invalid cookie with prefix" do + expect_raises ArgumentError, "Invalid cookie name. Has '__Secure-' prefix, but is not secure." do + HTTP::Cookie.new("__Secure-foo", "bar", secure: false) + end + + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + HTTP::Cookie.new("__Host-foo", "bar", domain: "foo") + end end - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do - HTTP::Cookie.new("__Host-foo", "bar") + it "automatically makes the cookie secure if it has the __Secure- prefix and no explicit *secure* value is provided" do + HTTP::Cookie.new("__Secure-foo", "bar").secure.should be_true + end + + it "automatically configures the cookie if it has the __Host- prefix and no explicit values provided" do + cookie = HTTP::Cookie.new "__Host-foo", "bar" + cookie.secure.should be_true + cookie.domain.should be_nil + cookie.path.should eq "/" end end end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 2aa50a611164..69caf4172eaf 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -33,9 +33,10 @@ module HTTP # # 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). # Raises `ArgumentError` if *name* has a security prefix but the requirements are not met as per [RFC 6265 bis §4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07#section-4.1.3). + # Alternatively, if *name* has a security prefix, and the related properties are `nil`, the prefix will automatically be applied to the cookie. def initialize(name : String, value : String, @path : String? = nil, @expires : Time? = nil, @domain : String? = nil, - @secure : Bool = false, @http_only : Bool = false, + secure : Bool? = nil, @http_only : Bool = false, @samesite : SameSite? = nil, @extension : String? = nil, @max_age : Time::Span? = nil, @creation_time = Time.utc) validate_name(name) @@ -43,6 +44,18 @@ module HTTP validate_value(value) @value = value raise IO::Error.new("Invalid max_age") if @max_age.try { |max_age| max_age < Time::Span.zero } + + if @name.starts_with?("__Host-") && @path.nil? && @domain.nil? && secure.nil? + @path = "/" + secure = true + end + + if @name.starts_with?("__Secure-") && secure.nil? + secure = true + end + + @secure = secure || false + self.validate! end From bb8c551b2459228a6022a902c0243a1368b38d98 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Sun, 4 Dec 2022 10:50:17 -0500 Subject: [PATCH 5/9] Have `#validate!` return `self` --- spec/std/http/cookie_spec.cr | 1 - src/http/cookie.cr | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index ec804d4ab222..fdc1d2c64b9f 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -35,7 +35,6 @@ module HTTP expect_raises IO::Error, "Invalid cookie name" do HTTP::Cookie.new("", "") end - expect_raises IO::Error, "Invalid cookie name" do HTTP::Cookie.new("\t", "") end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 69caf4172eaf..bcca7c8c858a 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -163,9 +163,11 @@ module HTTP end # Raises `ArgumentError` if `self` is not `#valid?`. - def validate! + def validate! : self raise ArgumentError.new "Invalid cookie name. Has '__Secure-' prefix, but is not secure." unless self.valid_secure_prefix? raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements." unless self.valid_host_prefix? + + self end private def valid_secure_prefix? : Bool From 27b5c81d133bd07cd892c396a74c6205020a786a Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Fri, 9 Dec 2022 09:38:47 -0500 Subject: [PATCH 6/9] Handle automatically configuring the cookie within name= if related properties are unset --- spec/std/http/cookie_spec.cr | 24 +++++++++++++++++++--- src/http/cookie.cr | 39 +++++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index fdc1d2c64b9f..ffd218bfb534 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -92,7 +92,7 @@ module HTTP end it "doesn't raise on invalid cookie with __Secure- prefix" do - cookie = HTTP::Cookie.new "x", "" + cookie = HTTP::Cookie.new "x", "", secure: false cookie.name = "__Secure-x" cookie.name.should eq "__Secure-x" @@ -106,6 +106,24 @@ module HTTP cookie.name.should eq "__Host-x" cookie.secure.should be_false end + + it "automatically configures the cookie __Secure- prefix and related properties are unset" do + cookie = HTTP::Cookie.new "x", "" + + cookie.name = "__Secure-x" + cookie.name.should eq "__Secure-x" + cookie.secure.should be_true + end + + it "automatically configures the cookie __Host- prefix and related properties are unset" do + cookie = HTTP::Cookie.new "x", "" + + cookie.name = "__Host-x" + cookie.name.should eq "__Host-x" + cookie.secure.should be_true + cookie.path.should eq "/" + cookie.domain.should be_nil + end end describe "#value=" do @@ -147,7 +165,7 @@ module HTTP describe "#valid? & #validate!" do it "raises on invalid cookie with __Secure- prefix" do - cookie = HTTP::Cookie.new "x", "" + cookie = HTTP::Cookie.new "x", "", secure: false cookie.name = "__Secure-x" cookie.valid?.should be_false @@ -164,7 +182,7 @@ module HTTP end it "raises on invalid cookie with __Host- prefix" do - cookie = HTTP::Cookie.new "x", "", domain: "example.com" + cookie = HTTP::Cookie.new "x", "", domain: "example.com", secure: false cookie.name = "__Host-x" cookie.valid?.should be_false diff --git a/src/http/cookie.cr b/src/http/cookie.cr index bcca7c8c858a..6228033b57dc 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -20,13 +20,14 @@ module HTTP property path : String? property expires : Time? 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 + @secure : Bool? + def_equals_and_hash name, value, path, expires, domain, secure, http_only, samesite, extension # Creates a new `Cookie` instance. @@ -36,7 +37,7 @@ module HTTP # Alternatively, if *name* has a security prefix, and the related properties are `nil`, the prefix will automatically be applied to the cookie. def initialize(name : String, value : String, @path : String? = nil, @expires : Time? = nil, @domain : String? = nil, - secure : Bool? = nil, @http_only : Bool = false, + @secure : Bool? = nil, @http_only : Bool = false, @samesite : SameSite? = nil, @extension : String? = nil, @max_age : Time::Span? = nil, @creation_time = Time.utc) validate_name(name) @@ -45,18 +46,15 @@ module HTTP @value = value raise IO::Error.new("Invalid max_age") if @max_age.try { |max_age| max_age < Time::Span.zero } - if @name.starts_with?("__Host-") && @path.nil? && @domain.nil? && secure.nil? - @path = "/" - secure = true - end - - if @name.starts_with?("__Secure-") && secure.nil? - secure = true - end + self.check_prefix + self.validate! + end - @secure = secure || false + def secure : Bool + !!@secure + end - self.validate! + def secure=(@secure : Bool) : Bool end # Sets the name of this cookie. @@ -65,6 +63,8 @@ module HTTP def name=(name : String) validate_name(name) @name = name + + self.check_prefix end private def validate_name(name) @@ -173,13 +173,24 @@ module HTTP private def valid_secure_prefix? : Bool has_secure_prefix = @name.starts_with?("__Secure-") - !has_secure_prefix || (has_secure_prefix && @secure) + !has_secure_prefix || (has_secure_prefix && self.secure) end private def valid_host_prefix? : Bool has_host_prefix = @name.starts_with?("__Host-") - !has_host_prefix || (has_host_prefix && @secure && "/" == @path && @domain.nil?) + !has_host_prefix || (has_host_prefix && self.secure && "/" == @path && @domain.nil?) + end + + private def check_prefix : Nil + if @name.starts_with?("__Host-") && @path.nil? && @domain.nil? && @secure.nil? + @path = "/" + @secure = true + end + + if @name.starts_with?("__Secure-") && @secure.nil? + @secure = true + end end # :nodoc: From a0154966aa1db8f8ccec5cf20f76376904a57df3 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Sat, 10 Dec 2022 11:26:57 -0500 Subject: [PATCH 7/9] PR feedback --- spec/std/http/cookie_spec.cr | 19 +++++++++---------- src/http/cookie.cr | 21 +++++++++------------ 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index ffd218bfb534..747856ced6ff 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -52,7 +52,7 @@ module HTTP HTTP::Cookie.new("__Secure-foo", "bar", secure: false) end - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements of: secure: true, path: \"/\", domain: nil." do HTTP::Cookie.new("__Host-foo", "bar", domain: "foo") end end @@ -100,11 +100,13 @@ module HTTP end it "doesn't raise on invalid cookie with __Host- prefix" do - cookie = HTTP::Cookie.new "x", "", domain: "example.com" + cookie = HTTP::Cookie.new "x", "", path: "/foo" cookie.name = "__Host-x" cookie.name.should eq "__Host-x" - cookie.secure.should be_false + cookie.secure.should be_true + cookie.path.should eq "/foo" + cookie.valid?.should be_false end it "automatically configures the cookie __Secure- prefix and related properties are unset" do @@ -115,7 +117,7 @@ module HTTP cookie.secure.should be_true end - it "automatically configures the cookie __Host- prefix and related properties are unset" do + it "automatically configures the cookie __Host- prefix and related unset properties" do cookie = HTTP::Cookie.new "x", "" cookie.name = "__Host-x" @@ -175,9 +177,6 @@ module HTTP end cookie.secure = true - - cookie.name = "__Secure-x" - cookie.name.should eq "__Secure-x" cookie.valid?.should be_true end @@ -188,7 +187,7 @@ module HTTP cookie.valid?.should be_false # Not secure - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements of: secure: true, path: \"/\", domain: nil." do cookie.validate! end @@ -196,7 +195,7 @@ module HTTP cookie.valid?.should be_false # Invalid path - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements of: secure: true, path: \"/\", domain: nil." do cookie.validate! end @@ -204,7 +203,7 @@ module HTTP cookie.valid?.should be_false # Has domain - expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements." do + expect_raises ArgumentError, "Invalid cookie name. Does not meet '__Host-' prefix requirements of: secure: true, path: \"/\", domain: nil." do cookie.validate! end diff --git a/src/http/cookie.cr b/src/http/cookie.cr index 6228033b57dc..f6199cf42484 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -50,6 +50,7 @@ module HTTP self.validate! end + # Returns `true` if this cookie has the *Secure* flag. def secure : Bool !!@secure end @@ -165,31 +166,27 @@ module HTTP # Raises `ArgumentError` if `self` is not `#valid?`. def validate! : self raise ArgumentError.new "Invalid cookie name. Has '__Secure-' prefix, but is not secure." unless self.valid_secure_prefix? - raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements." unless self.valid_host_prefix? + raise ArgumentError.new "Invalid cookie name. Does not meet '__Host-' prefix requirements of: secure: true, path: \"/\", domain: nil." unless self.valid_host_prefix? self end private def valid_secure_prefix? : Bool - has_secure_prefix = @name.starts_with?("__Secure-") - - !has_secure_prefix || (has_secure_prefix && self.secure) + self.secure || !@name.starts_with?("__Secure-") end private def valid_host_prefix? : Bool - has_host_prefix = @name.starts_with?("__Host-") - - !has_host_prefix || (has_host_prefix && self.secure && "/" == @path && @domain.nil?) + !@name.starts_with?("__Host-") || (self.secure && "/" == @path && @domain.nil?) end private def check_prefix : Nil - if @name.starts_with?("__Host-") && @path.nil? && @domain.nil? && @secure.nil? - @path = "/" - @secure = true + if @name.starts_with?("__Host-") + @path = "/" if @path.nil? + @secure = true if @secure.nil? end - if @name.starts_with?("__Secure-") && @secure.nil? - @secure = true + if @name.starts_with?("__Secure-") + @secure = true if @secure.nil? end end From 2eb6f60cf5405880a526cfa302298e3ef1beb9f5 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Mon, 12 Dec 2022 09:30:41 -0500 Subject: [PATCH 8/9] Handle case where `@secure` ivar ends up as `nil` --- spec/std/http/cookie_spec.cr | 10 ++++++++++ src/http/cookie.cr | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 747856ced6ff..0caab46e8aad 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -180,6 +180,16 @@ module HTTP cookie.valid?.should be_true end + it "with a __Secure- prefix, but @secure is somehow `nil`" do + cookie = HTTP::Cookie.new "__Secure-x", "" + + cookie.valid?.should be_true + + pointerof(cookie.@secure).value = nil + + cookie.valid?.should be_true + end + it "raises on invalid cookie with __Host- prefix" do cookie = HTTP::Cookie.new "x", "", domain: "example.com", secure: false cookie.name = "__Host-x" diff --git a/src/http/cookie.cr b/src/http/cookie.cr index f6199cf42484..c8bf31e5c7f7 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -172,7 +172,7 @@ module HTTP end private def valid_secure_prefix? : Bool - self.secure || !@name.starts_with?("__Secure-") + @secure != false || !@name.starts_with?("__Secure-") end private def valid_host_prefix? : Bool From a9026940256c8f71db8dfa5e2af129be86edd25d Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Tue, 13 Dec 2022 16:52:54 -0500 Subject: [PATCH 9/9] Properly handle `@secure = nil` edge case --- spec/std/http/cookie_spec.cr | 2 +- src/http/cookie.cr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/std/http/cookie_spec.cr b/spec/std/http/cookie_spec.cr index 0caab46e8aad..218bfd9c608e 100644 --- a/spec/std/http/cookie_spec.cr +++ b/spec/std/http/cookie_spec.cr @@ -187,7 +187,7 @@ module HTTP pointerof(cookie.@secure).value = nil - cookie.valid?.should be_true + cookie.valid?.should be_false end it "raises on invalid cookie with __Host- prefix" do diff --git a/src/http/cookie.cr b/src/http/cookie.cr index c8bf31e5c7f7..f6199cf42484 100644 --- a/src/http/cookie.cr +++ b/src/http/cookie.cr @@ -172,7 +172,7 @@ module HTTP end private def valid_secure_prefix? : Bool - @secure != false || !@name.starts_with?("__Secure-") + self.secure || !@name.starts_with?("__Secure-") end private def valid_host_prefix? : Bool