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

Validate cookie name prefixes #10648

Merged
merged 10 commits into from
Dec 23, 2022
104 changes: 67 additions & 37 deletions spec/std/http/cookie_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
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

Expand Down Expand Up @@ -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"
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
37 changes: 27 additions & 10 deletions src/http/cookie.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down