`Rack::Protection::CookieTossing` test fail with Rack 3.1.3

[dentarg]

From https://github.com/sinatra/sinatra/actions/runs/9565078027/job/26367227204

Failures:

  1) Rack::Protection::CookieTossing with default reaction adds the correct Set-Cookie header
     Failure/Error: expect(last_response.headers['Set-Cookie']).to eq(expected_header)

       expected: ["rack.%2573ession=; domain=example.org; path=/; expires=Thu, 01 Jan [19](https://github.com/sinatra/sinatra/actions/runs/9565078027/job/26367227204#step:4:20)70 00:00:00 GMT", "rack.%2573e...0 GMT", "rack.session=; domain=example.org; path=/some/path; expires=Thu, 01 Jan 1970 00:00:00 GMT"]
            got: ["rack.%73ession=; domain=example.org; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT", "rack.%73essio...0 GMT", "rack.session=; domain=example.org; path=/some/path; expires=Thu, 01 Jan 1970 00:00:00 GMT"]

       (compared using ==)
     # ./spec/lib/rack/protection/cookie_tossing_spec.rb:40:in `block (3 levels) in <top (required)>'

[dentarg]

Intentional change @ioquatix @jeremyevans?

[dentarg]

This is the test:

https://github.com/sinatra/sinatra/blob/5640495babcb4cfd69ba650b293660b7446402da/rack-protection/spec/lib/rack/protection/cookie_tossing_spec.rb#L29-L42

Added in 2016: https://github.com/sinatra/sinatra/commit/cd5028b5c961cd1beca01409a2cd1a3ee818da37 (https://github.com/sinatra/rack-protection/pull/113)

[dentarg]

Looks like https://github.blog/2013-04-09-yummy-cookies-across-domains/ is the origin of this protection. I have only scrolled the blog post, not read it.

[jeremyevans]

I believe it is intentional that Rack no longer URI encodes the % in the cookie in Rack 3.1+. @ioquatix is that correct?

[ioquatix]

Yes, that's correct.

[dentarg]

Are you able to help me out here, should we just change the test case here or should the implementation account for the fact that Rack no longer encodes % in this scenario?

(Again, I have not spend so much time to understand this ... (yet))

[ioquatix]

Previously, Rack was escaping and unescaping both cookie keys and values. Bear in mind that this is a bit tricky, as there are two places where this happens:

1. Rack receives cookies from the client and should unescape them. Rack also sends cookies to the client and should escape them.
2. Rack::Test (and similar) can receive cookies as a "fake client" and unescapes them.

The reason why I mention this is because (2) is not fixed in "(and similar)" cases, i.e. people whom have assumed the Rack behaviour and hard coded a specific interpretation (as may be the case in the tests here).

Let me also say clearly that browsers don't unescape cookie keys, so Rack was basically just deciding to escape the cookie key because it could, and escaping more characters than were necessary and the same for unescaping. But browsers don't understand those keys, e.g. browsers don't unescape them.

So, I'm going to assume, a long long time ago, Rack probably didn't escape and unescape cookie keys. Then someone probably complained that set_cookie(key, value) didn't work when key contained invalid characters. So someone probably decided to add escape/unescape to allow for this.

Not only does this break document.cookies, i.e. set_cookie("my cookie", ...) could be encoded with as my+cookie or my%20cookie, it also caused a security incident where someone could force a cookie key like __%73ecure-foo from the client, which doesn't follow the semantics for secure cookies, but DOES decode into one that looks like it should on the server CGI.unescape("__%73ecure-foo") -> "__secure-foo" (see the note on https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie starting with "Note: Some have a specific semantic").

Even if we could fix this, we don't really know if in the future there will be other specific cookie names added... so using unescape on cookie keys is a bad idea.

This was "fixed" by removing the unescape step. However, as shown in the example here, this just leads to a situation where it's impossible to use those cookies.

Therefore the correct solution is for Rack to raise an error when you try to use an invalid cookie key. Therefore your code should simply not test for this, you should not try to use invalid characters - by definition it's invalid to do so.

Hope that clarifies it.

[dentarg]

Thanks for all the info (still need to spend more time on this, like reading the Rails PR you link to).

Therefore the correct solution is for Rack to raise an error when you try to use an invalid cookie key

Shouldn't this test raise an error when using rack head? Looks like it doesn't: https://github.com/sinatra/sinatra/actions/runs/9565078027/job/26367227701#step:4:17

[ioquatix]

Only if it hits this code path: https://github.com/rack/rack/blob/800e53fbe15b3424b7a8946b067bf6f2e648d5a8/lib/rack/utils.rb#L281-L284

% is not an invalid character.

[dentarg]

So with https://github.com/rack/rack/commit/a71dfd79d8f49decbe5a1db703dd234df30ba6cc (Rack 3.1.0+), Rack::Utils.escape is no longer called on the cookie key (when it is valid).

The key we are testing with here is valid:

irb(main):001> VALID_COOKIE_KEY = /\A[!#$%&'*+\-\.\^_`|~0-9a-zA-Z]+\z/.freeze
=> /\A[!#$%&'*+\-\.\^_`|~0-9a-zA-Z]+\z/
irb(main):002> key = "rack.%73ession"
=> "rack.%73ession"
irb(main):003> key =~ VALID_COOKIE_KEY ? true : false
=> true

Before Rack 3.1.0, a cookie with such key round-tripped to a cookie like

irb(main):006> Rack::Utils.escape("rack.%73ession")
=> "rack.%2573ession"

Here's my full testing:

rack 2.1.3

$ rack='2.1.3' ruby app.rb
== Sinatra (v2.0.8.1) has taken the stage on 4567 for development with backup from Puma
Puma starting in single mode...
* Version 4.3.12 (ruby 3.2.4-p170), codename: Mysterious Traveller
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://127.0.0.1:4567
* Listening on tcp://[::1]:4567
Use Ctrl-C to stop
received cookies: {"rack.session"=>"foo"}

$ curl --header 'Cookie: rack.%73ession=foo' --head http://localhost:4567
HTTP/1.1 200 OK
Content-Type: text/html;charset=utf-8
Set-Cookie: rack.%2573ession=Rack%3A%3ARELEASE+2.1.3
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Content-Length: 2

rack 2.1.4 (https://github.com/advisories/GHSA-j6w9-fv6q-3q52 fixed)

$ rack='2.1.4' ruby app.rb
== Sinatra (v2.0.8.1) has taken the stage on 4567 for development with backup from Puma
Puma starting in single mode...
* Version 4.3.12 (ruby 3.2.4-p170), codename: Mysterious Traveller
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://127.0.0.1:4567
* Listening on tcp://[::1]:4567
Use Ctrl-C to stop
received cookies: {"rack.%73ession"=>"foo"}

$ curl --header 'Cookie: rack.%73ession=foo' --head http://localhost:4567
HTTP/1.1 200 OK
Content-Type: text/html;charset=utf-8
Set-Cookie: rack.%2573ession=Rack%3A%3ARELEASE+2.1.4
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Content-Length: 2

rack 3.0.10

$ rack='3.0.10' ruby app.rb
== Sinatra (v4.0.0) has taken the stage on 4567 for development with backup from Puma
Puma starting in single mode...
* Puma version: 6.4.2 (ruby 3.2.4-p170) ("The Eagle of Durango")
*  Min threads: 0
*  Max threads: 5
*  Environment: development
*          PID: 59997
* Listening on http://127.0.0.1:4567
* Listening on http://[::1]:4567
Use Ctrl-C to stop
received cookies: {"rack.%73ession"=>"foo"}

$ curl --header 'Cookie: rack.%73ession=foo' --head http://localhost:4567
HTTP/1.1 200 OK
set-cookie: rack.%2573ession=Rack%3A%3ARELEASE+3.0.10
content-type: text/html;charset=utf-8
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
Content-Length: 2

rack 3.1.7

$ rack='3.1.7' ruby app.rb
== Sinatra (v4.0.0) has taken the stage on 4567 for development with backup from Puma
Puma starting in single mode...
* Puma version: 6.4.2 (ruby 3.2.4-p170) ("The Eagle of Durango")
*  Min threads: 0
*  Max threads: 5
*  Environment: development
*          PID: 60436
* Listening on http://127.0.0.1:4567
* Listening on http://[::1]:4567
Use Ctrl-C to stop
received cookies: {"rack.%73ession"=>"foo"}

$ curl --header 'Cookie: rack.%73ession=foo' --head http://localhost:4567
HTTP/1.1 200 OK
set-cookie: rack.%73ession=Rack%3A%3ARELEASE+3.1.7
content-type: text/html;charset=utf-8
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
Content-Length: 2

app.rb

#!/usr/bin/env ruby

# frozen_string_literal: true

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  rack_v = ENV.fetch("rack")
  sinatra_v = ENV.fetch("sinatra", nil)
  rack_2 = rack_v.to_s.start_with?("2.1")
  puma_v = rack_2 ? "< 5" : nil

  gem "rackup" unless rack_2
  gem "puma", puma_v
  gem "sinatra", sinatra_v, require: "sinatra/base"
  gem "rack", rack_v
end

class App < Sinatra::Base
  get "/" do
    puts "received cookies: #{request.cookies.inspect}"
    "OK"
  end

  after do
    response.set_cookie "rack.%73ession", "Rack::RELEASE #{Rack::RELEASE}"
  end

  run! if app_file == $0
end

[ioquatix]

Yes, that's correct, the cookie is escaped in the response but not unescaped in the request, leading to an impossible to use cookie key. See https://github.com/rack/rack/pull/2189 for an example of this kind of failure.