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
110 changes: 110 additions & 0 deletions spec/std/http/cookie_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,29 @@ module HTTP
HTTP::Cookie.new("x", %(foo\rbar))
end
end

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")
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
end
end

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

describe "#name=" do
Expand All @@ -67,6 +90,40 @@ module HTTP
end
end
end

it "doesn't raise on invalid cookie with __Secure- prefix" do
cookie = HTTP::Cookie.new "x", "", secure: false

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 "doesn't raise on invalid cookie with __Host- prefix" do
cookie = HTTP::Cookie.new "x", "", domain: "example.com"

cookie.name = "__Host-x"
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
Expand Down Expand Up @@ -105,6 +162,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", "", secure: false
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", secure: false
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
57 changes: 55 additions & 2 deletions src/http/cookie.cr
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,41 @@ 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.
#
# 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)
@name = name
validate_value(value)
@value = value
raise IO::Error.new("Invalid max_age") if @max_age.try { |max_age| max_age < Time::Span.zero }

self.check_prefix
self.validate!
end

def secure : Bool
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
!!@secure
end

def secure=(@secure : Bool) : Bool
end

# Sets the name of this cookie.
Expand All @@ -50,6 +63,8 @@ module HTTP
def name=(name : String)
validate_name(name)
@name = name

self.check_prefix
end

private def validate_name(name)
Expand Down Expand Up @@ -140,6 +155,44 @@ 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! : 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
has_secure_prefix = @name.starts_with?("__Secure-")

!has_secure_prefix || (has_secure_prefix && self.secure)
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
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?)
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
end

private def check_prefix : Nil
if @name.starts_with?("__Host-") && @path.nil? && @domain.nil? && @secure.nil?
@path = "/"
@secure = true
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
end

if @name.starts_with?("__Secure-") && @secure.nil?
@secure = true
Blacksmoke16 marked this conversation as resolved.
Show resolved Hide resolved
end
end

# :nodoc:
module Parser
module Regex
Expand Down