Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) #411

geoffyoungs · 2019-11-07T12:41:42Z

Allow the following header to be parsed without throwing an exception:

Set-Cookie: name=value; Max-Age=315360000; domain=example.com; path=/; SameSite=Strict;; secure

It's not valid - https://tools.ietf.org/html/rfc6265#section-4.1.1 - but it
shouldn't result in a nil exception.

Background

We ran into an issue with a trusted client sending Set-Cookie headers
which were invalid and which we were proxying to the end user.

The headers were in the form:

Set-Cookie: name=value; Max-Age=315360000; domain=example.com; path=/; SameSite=Strict;; secure

When SecureHeaders::Cookie#parse encounters the ";;" it blows up because

      cookie.split(/[;,]\s?/).each do |pairs| # pairs == ""
        name, values = pairs.split("=", 2)    # pairs.split() => []
                                              # ie. name == values == nil
        name = CGI.unescape(name)             # raises exception

ie.

$ for rb in 2.0 2.1 2.2 2.3 2.4 2.5 2.6; do chruby-exec $rb -- ruby -r cgi -e 'p [RUBY_VERSION, "".split(/=/,2)]; p CGI.unescape(nil)'; done

["2.0.0", []]
/opt/rubies/2.0.0-p648/lib/ruby/2.0.0/cgi/util.rb:17:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError)
        from -e:1:in `<main>'
["2.1.10", []]
/opt/rubies/ruby-2.1.10/lib/ruby/2.1.0/cgi/util.rb:18:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError)
        from -e:1:in `<main>'
["2.2.6", []]
/opt/rubies/ruby-2.2.6/lib/ruby/2.2.0/cgi/util.rb:18:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError)
        from -e:1:in `<main>'
["2.3.6", []]
/opt/rubies/ruby-2.3.6/lib/ruby/2.3.0/cgi/util.rb:19:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError)
        from -e:1:in `<main>'
["2.4.0", []]
-e:1:in `unescape': no implicit conversion of nil into String (TypeError)
        from -e:1:in `<main>'
["2.5.0", []]
Traceback (most recent call last):
        1: from -e:1:in `<main>'
-e:1:in `unescape': no implicit conversion of nil into String (TypeError)
["2.6.5", []]
Traceback (most recent call last):
        1: from -e:1:in `<main>'
-e:1:in `unescape': no implicit conversion of nil into String (TypeError)

Has tests
Documentation updated


        Avoid throwing cookie headers when encountering an empty cookie-av

Allow the following header to be parsed without throwing an exception: Set-Cookie: name=value; Max-Age=315360000; domain=example.com; path=/; SameSite=Strict;; secure It's not valid - https://tools.ietf.org/html/rfc6265#section-4.1.1 - but it shouldn't result in a nil exception. Background: We ran into an issue with a trusted client sending Set-Cookie headers which were invalid and which we were proxying to the end user. The headers were in the form: Set-Cookie: name=value; Max-Age=315360000; domain=example.com; path=/; SameSite=Strict;; secure When SecureHeaders::Cookie#parse encounters the ";;" it blows up because ```ruby cookie.split(/[;,]\s?/).each do |pairs| # pairs == "" name, values = pairs.split("=", 2) # pairs.split() => [] # ie. name == values == nil name = CGI.unescape(name) # raises exception ``` ie. $ for rb in 2.0 2.1 2.2 2.3 2.4 2.5 2.6; do chruby-exec $rb -- ruby -r cgi -e 'p [RUBY_VERSION, "".split(/=/,2)]; p CGI.unescape(nil)'; done ["2.0.0", []] /opt/rubies/2.0.0-p648/lib/ruby/2.0.0/cgi/util.rb:17:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError) from -e:1:in `<main>' ["2.1.10", []] /opt/rubies/ruby-2.1.10/lib/ruby/2.1.0/cgi/util.rb:18:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError) from -e:1:in `<main>' ["2.2.6", []] /opt/rubies/ruby-2.2.6/lib/ruby/2.2.0/cgi/util.rb:18:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError) from -e:1:in `<main>' ["2.3.6", []] /opt/rubies/ruby-2.3.6/lib/ruby/2.3.0/cgi/util.rb:19:in `unescape': undefined method `tr' for nil:NilClass (NoMethodError) from -e:1:in `<main>' ["2.4.0", []] -e:1:in `unescape': no implicit conversion of nil into String (TypeError) from -e:1:in `<main>' ["2.5.0", []] Traceback (most recent call last): 1: from -e:1:in `<main>' -e:1:in `unescape': no implicit conversion of nil into String (TypeError) ["2.6.5", []] Traceback (most recent call last): 1: from -e:1:in `<main>' -e:1:in `unescape': no implicit conversion of nil into String (TypeError)

CLAassistant · 2019-11-07T12:42:26Z

All committers have signed the CLA.

oreoshake · 2019-11-12T19:09:20Z

Hi @geoffyoungs, thanks for the PR! My first instinct was to suggest a monkey patch for this one-off, non-standard behavior if you're unable to affect the upstream data.

I wanted to see if good 'ole rack parsing would also allow this. I was under the impression that is where the code was copied from, but it appears that we might be able to remove this cookie parsing code entirely in favor of something from rack itself.

https://github.com/rack/rack/blob/18365e63d054b65e5f1b0b69e4c95ad83fadc71d/lib/rack/utils.rb

    def parse_cookies_header(header)
      # According to RFC 2109:
      #   If multiple cookies satisfy the criteria above, they are ordered in
      #   the Cookie header such that those with more specific Path attributes
      #   precede those with less specific.  Ordering with respect to other
      #   attributes (e.g., Domain) is unspecified.
      cookies = parse_query(header, ';,') { |s| unescape(s) rescue s }
      cookies.each_with_object({}) { |(k, v), hash| hash[k] = Array === v ? v.first : v }
    end

    def parse_query(qs, d = nil, &unescaper)
      Rack::Utils.default_query_parser.parse_query(qs, d, &unescaper)
    end
    module_function :parse_query

~~What's interesting, is that Rack wouldn't be able to detect the secure flag:~~

It is detecting it, a name without a value is still valid 🤦‍♂

irb(main):001:0> Rack::Utils.default_query_parser.parse_query("Set-Cookie: name=value; Max-Age=315360000; domain=example.com; path=/; SameSite=Strict;; secure", ";,") { |s| puts s; s }
Set-Cookie: name
value
Max-Age
315360000
domain
example.com
path
/
SameSite
Strict
secure
=> {"Set-Cookie: name"=>"value", "Max-Age"=>"315360000", "domain"=>"example.com", "path"=>"/", "SameSite"=>"Strict", "secure"=>nil}

Would you be willing to update the code to use this standard method? I believe it would parse things safely and correctly set the desired flags.

geoffyoungs changed the title ~~Avoid throwing cookie headers when encountering an empty cookie-av~~ Avoid throwing nil errors parsing cookie headers when encountering an empty cookie-av Nov 8, 2019

geoffyoungs changed the title ~~Avoid throwing nil errors parsing cookie headers when encountering an empty cookie-av~~ Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) Nov 8, 2019

Aug	SEP	Oct
	12
2019	2020	2021

github / secure_headers

Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) #411

Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) #411

geoffyoungs commented Nov 7, 2019

CLAassistant commented Nov 7, 2019 •

edited

oreoshake commented Nov 12, 2019 •

edited

github / secure_headers

Join GitHub today

Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) #411

Handle invalid Set-Cookie headers more gracefully (in the presence of a blank cookie-av value) #411

Conversation

geoffyoungs commented Nov 7, 2019

Background

CLAassistant commented Nov 7, 2019 • edited

oreoshake commented Nov 12, 2019 • edited

CLAassistant commented Nov 7, 2019 •

edited

oreoshake commented Nov 12, 2019 •

edited