From e53d942c7632f8948889a083592e6154da94ac0a Mon Sep 17 00:00:00 2001 From: Tim Kellogg Date: Thu, 21 Apr 2022 22:41:52 -0400 Subject: [PATCH 1/3] Recognize cookies set by redirect When a redirect response arrives, the browser will recognize `Set-Cookie` headers the same as any other request/response. This PR enables the Redirector to correctly follow the cookie hand-offs. I encountered this situation out in the wild. This PR, as-is, seems to fix the issue. I haven't bothered to find the relevant spec. --- lib/http/redirector.rb | 33 ++++++++++++++++++++++++- spec/lib/http/redirector_spec.rb | 41 +++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/lib/http/redirector.rb b/lib/http/redirector.rb index 5d2de095..b3af9fc2 100644 --- a/lib/http/redirector.rb +++ b/lib/http/redirector.rb @@ -59,13 +59,44 @@ def perform(request, response) @response.flush # XXX(ixti): using `Array#inject` to return `nil` if no Location header. - @request = redirect_to(@response.headers.get(Headers::LOCATION).inject(:+)) + @request = redirect_to(@response.headers.get(Headers::LOCATION).inject(:+)) + self.class.update_cookies(@response, @request) @response = yield @request end @response end + class << self + # Used internally to update cookies between redirects. If a redirct response contains + # a Set-Cookie header(s), the following request should have that cookie set. + # + # The `request` parameter is modified (no return value). + def update_cookies(response, request) + request_cookie_header = request.headers["Cookie"] + cookies = + if request_cookie_header + HTTP::Cookie.cookie_value_to_hash(request_cookie_header) + else + {} + end + cookies = overwrite_cookies(response.cookies, cookies) + + request.headers[Headers::COOKIE] = cookies.map { |k, v| "#{k}=#{v}" }.join("; ") + end + + def overwrite_cookies(from, into_h) + from.each do |cookie| + if cookie.value == "" + into_h.delete(cookie.name) + else + into_h[cookie.name] = cookie.value + end + end + into_h + end + end + private # Check if we reached max amount of redirect hops diff --git a/spec/lib/http/redirector_spec.rb b/spec/lib/http/redirector_spec.rb index 110f4ec1..f7445782 100644 --- a/spec/lib/http/redirector_spec.rb +++ b/spec/lib/http/redirector_spec.rb @@ -33,6 +33,45 @@ def redirect_response(status, location) end end + describe "#update_cookies" do + subject { described_class.update_cookies } + + def set_cookie(name, value) + "#{name}=#{value}; path=/; httponly; secure; SameSite=none; Secure" + end + + it "creates a new cookie" do + req = HTTP::Request.new :verb => :post, :uri => "http://example.com" + res = simple_response(301) + res.headers["Set-Cookie"] = set_cookie("foo", 1) + described_class.update_cookies(res, req) + + expect(req.headers["Cookie"]).to eq "foo=1" + end + + it "overwrites an existing cookie" do + req = HTTP::Request.new :verb => :post, :uri => "http://example.com", :headers => { + "Cookie" => "foo=0" + } + res = simple_response(301) + res.headers["Set-Cookie"] = set_cookie("foo", "1") + described_class.update_cookies(res, req) + + expect(req.headers["Cookie"]).to eq "foo=1" + end + + it "unsets an existing cookie" do + req = HTTP::Request.new :verb => :post, :uri => "http://example.com", :headers => { + "Cookie" => "foo=0" + } + res = simple_response(301) + res.headers["Set-Cookie"] = set_cookie("foo", "") + described_class.update_cookies(res, req) + + expect(req.headers["Cookie"]).to eq "" + end + end + describe "#perform" do let(:options) { {} } let(:redirector) { described_class.new options } @@ -400,7 +439,7 @@ def redirect_response(status, location) describe "changing verbs during redirects" do let(:options) { {:strict => false} } let(:post_body) { HTTP::Request::Body.new("i might be way longer in real life") } - let(:cookie) { "dont eat my cookies" } + let(:cookie) { "dont=eat my cookies" } def a_dangerous_request(verb) HTTP::Request.new( From d59f654e41a3ef323ed01dd7634af7ca77ddee08 Mon Sep 17 00:00:00 2001 From: Tim Kellogg Date: Sat, 23 Apr 2022 16:10:24 -0400 Subject: [PATCH 2/3] Carry cookies to the final response My previous commit only carried cookies between redirect requests and not to the final response. This corrects that. All tests are now at the higher level, and the code change is smaller, overall. --- lib/http/redirector.rb | 51 +++++++++++------------- spec/lib/http/redirector_spec.rb | 67 +++++++++++++------------------- 2 files changed, 49 insertions(+), 69 deletions(-) diff --git a/lib/http/redirector.rb b/lib/http/redirector.rb index b3af9fc2..b6b12345 100644 --- a/lib/http/redirector.rb +++ b/lib/http/redirector.rb @@ -49,6 +49,7 @@ def perform(request, response) @request = request @response = response @visited = [] + collect_cookies while REDIRECT_CODES.include? @response.status.code @visited << "#{@request.verb} #{@request.uri}" @@ -60,44 +61,38 @@ def perform(request, response) # XXX(ixti): using `Array#inject` to return `nil` if no Location header. @request = redirect_to(@response.headers.get(Headers::LOCATION).inject(:+)) - self.class.update_cookies(@response, @request) @response = yield @request + collect_cookies end @response end - class << self - # Used internally to update cookies between redirects. If a redirct response contains - # a Set-Cookie header(s), the following request should have that cookie set. - # - # The `request` parameter is modified (no return value). - def update_cookies(response, request) - request_cookie_header = request.headers["Cookie"] - cookies = - if request_cookie_header - HTTP::Cookie.cookie_value_to_hash(request_cookie_header) - else - {} - end - cookies = overwrite_cookies(response.cookies, cookies) - - request.headers[Headers::COOKIE] = cookies.map { |k, v| "#{k}=#{v}" }.join("; ") - end + private - def overwrite_cookies(from, into_h) - from.each do |cookie| - if cookie.value == "" - into_h.delete(cookie.name) - else - into_h[cookie.name] = cookie.value - end + # Carry cookies from one response to the next. Carrying cookies to the next response ends up + # carrying them to the next request as well. + # + # Note that this isn't part of the IETF standard, but all major browsers support setting cookies + # on redirect: https://blog.dubbelboer.com/2012/11/25/302-cookie.html + def collect_cookies + # it seems that @response.cookies instance is reused between responses, so we have to "clone" + @cookie_jar ||= HTTP::CookieJar.new + + # Overwrite previous cookies + @response.cookies.each do |cookie| + if cookie.value == "" + @cookie_jar.delete(cookie) + else + @cookie_jar.add(cookie) end - into_h end - end - private + # I wish we could just do @response.cookes = @cookie_jar + @cookie_jar.each do |cookie| + @response.cookies.add(cookie) + end + end # Check if we reached max amount of redirect hops # @return [Boolean] diff --git a/spec/lib/http/redirector_spec.rb b/spec/lib/http/redirector_spec.rb index f7445782..7d7f278e 100644 --- a/spec/lib/http/redirector_spec.rb +++ b/spec/lib/http/redirector_spec.rb @@ -11,8 +11,12 @@ def simple_response(status, body = "", headers = {}) ) end - def redirect_response(status, location) - simple_response status, "", "Location" => location + def redirect_response(status, location, set_cookie = {}) + res = simple_response status, "", "Location" => location + set_cookie.each do |name, value| + res.headers.add("Set-Cookie", "#{name}=#{value}; path=/; httponly; secure; SameSite=none; Secure") + end + res end describe "#strict" do @@ -33,45 +37,6 @@ def redirect_response(status, location) end end - describe "#update_cookies" do - subject { described_class.update_cookies } - - def set_cookie(name, value) - "#{name}=#{value}; path=/; httponly; secure; SameSite=none; Secure" - end - - it "creates a new cookie" do - req = HTTP::Request.new :verb => :post, :uri => "http://example.com" - res = simple_response(301) - res.headers["Set-Cookie"] = set_cookie("foo", 1) - described_class.update_cookies(res, req) - - expect(req.headers["Cookie"]).to eq "foo=1" - end - - it "overwrites an existing cookie" do - req = HTTP::Request.new :verb => :post, :uri => "http://example.com", :headers => { - "Cookie" => "foo=0" - } - res = simple_response(301) - res.headers["Set-Cookie"] = set_cookie("foo", "1") - described_class.update_cookies(res, req) - - expect(req.headers["Cookie"]).to eq "foo=1" - end - - it "unsets an existing cookie" do - req = HTTP::Request.new :verb => :post, :uri => "http://example.com", :headers => { - "Cookie" => "foo=0" - } - res = simple_response(301) - res.headers["Set-Cookie"] = set_cookie("foo", "") - described_class.update_cookies(res, req) - - expect(req.headers["Cookie"]).to eq "" - end - end - describe "#perform" do let(:options) { {} } let(:redirector) { described_class.new options } @@ -128,6 +93,26 @@ def set_cookie(name, value) expect(res.to_s).to eq "http://example.com/123" end + it "returns cookies in response" do + req = HTTP::Request.new :verb => :head, :uri => "http://example.com" + hops = [ + redirect_response(301, "http://example.com/1", {"foo" => "42"}), + redirect_response(301, "http://example.com/2", {"bar" => "53", "deleted" => "foo"}), + redirect_response(301, "http://example.com/3", {"baz" => "64", "deleted" => ""}), + redirect_response(301, "http://example.com/4", {"baz" => "65"}), + simple_response(200, "bar") + ] + + res = redirector.perform(req, hops.shift) { hops.shift } + expect(res.to_s).to eq "bar" + cookies = res.cookies.cookies.map { |c| [c.name, c.value] }.to_h + puts cookies + expect(cookies["foo"]).to eq "42" + expect(cookies["bar"]).to eq "53" + expect(cookies["baz"]).to eq "65" + expect(cookies["deleted"]).to eq nil + end + context "following 300 redirect" do context "with strict mode" do let(:options) { {:strict => true} } From c01009ddae9a1bd9823c8d1f88741e9f4f4e8892 Mon Sep 17 00:00:00 2001 From: Tim Kellogg Date: Sat, 23 Apr 2022 22:34:38 -0400 Subject: [PATCH 3/3] Carry cookies from response to the next request I had previously thought it happened automatically, but I guess not. --- lib/http/redirector.rb | 42 ++++++++++++++++++++++++-------- spec/lib/http/redirector_spec.rb | 39 +++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/lib/http/redirector.rb b/lib/http/redirector.rb index b6b12345..ea890e96 100644 --- a/lib/http/redirector.rb +++ b/lib/http/redirector.rb @@ -49,7 +49,8 @@ def perform(request, response) @request = request @response = response @visited = [] - collect_cookies + collect_cookies_from_request + collect_cookies_from_response while REDIRECT_CODES.include? @response.status.code @visited << "#{@request.verb} #{@request.uri}" @@ -61,8 +62,11 @@ def perform(request, response) # XXX(ixti): using `Array#inject` to return `nil` if no Location header. @request = redirect_to(@response.headers.get(Headers::LOCATION).inject(:+)) + unless cookie_jar.empty? + @request.headers.set(Headers::COOKIE, cookie_jar.cookies.map { |c| "#{c.name}=#{c.value}" }.join("; ")) + end @response = yield @request - collect_cookies + collect_cookies_from_response end @response @@ -70,26 +74,44 @@ def perform(request, response) private + # All known cookies. On the original request, this is only the original cookies, but after that, + # Set-Cookie headers can add, set or delete cookies. + def cookie_jar + # it seems that @response.cookies instance is reused between responses, so we have to "clone" + @cookie_jar ||= HTTP::CookieJar.new + end + + def collect_cookies_from_request + request_cookie_header = @request.headers["Cookie"] + cookies = + if request_cookie_header + HTTP::Cookie.cookie_value_to_hash(request_cookie_header) + else + {} + end + + cookies.each do |key, value| + cookie_jar.add(HTTP::Cookie.new(key, value, :path => @request.uri.path, :domain => @request.host)) + end + end + # Carry cookies from one response to the next. Carrying cookies to the next response ends up # carrying them to the next request as well. # # Note that this isn't part of the IETF standard, but all major browsers support setting cookies # on redirect: https://blog.dubbelboer.com/2012/11/25/302-cookie.html - def collect_cookies - # it seems that @response.cookies instance is reused between responses, so we have to "clone" - @cookie_jar ||= HTTP::CookieJar.new - + def collect_cookies_from_response # Overwrite previous cookies @response.cookies.each do |cookie| if cookie.value == "" - @cookie_jar.delete(cookie) + cookie_jar.delete(cookie) else - @cookie_jar.add(cookie) + cookie_jar.add(cookie) end end - # I wish we could just do @response.cookes = @cookie_jar - @cookie_jar.each do |cookie| + # I wish we could just do @response.cookes = cookie_jar + cookie_jar.each do |cookie| @response.cookies.add(cookie) end end diff --git a/spec/lib/http/redirector_spec.rb b/spec/lib/http/redirector_spec.rb index 7d7f278e..8a3abd2d 100644 --- a/spec/lib/http/redirector_spec.rb +++ b/spec/lib/http/redirector_spec.rb @@ -103,16 +103,51 @@ def redirect_response(status, location, set_cookie = {}) simple_response(200, "bar") ] - res = redirector.perform(req, hops.shift) { hops.shift } + request_cookies = [ + {"foo" => "42"}, + {"foo" => "42", "bar" => "53", "deleted" => "foo"}, + {"foo" => "42", "bar" => "53", "baz" => "64"}, + {"foo" => "42", "bar" => "53", "baz" => "65"} + ] + + res = redirector.perform(req, hops.shift) do |request| + req_cookie = HTTP::Cookie.cookie_value_to_hash(request.headers["Cookie"] || "") + expect(req_cookie).to eq request_cookies.shift + hops.shift + end expect(res.to_s).to eq "bar" cookies = res.cookies.cookies.map { |c| [c.name, c.value] }.to_h - puts cookies expect(cookies["foo"]).to eq "42" expect(cookies["bar"]).to eq "53" expect(cookies["baz"]).to eq "65" expect(cookies["deleted"]).to eq nil end + it "returns original cookies in response" do + req = HTTP::Request.new :verb => :head, :uri => "http://example.com" + req.headers.set("Cookie", "foo=42; deleted=baz") + hops = [ + redirect_response(301, "http://example.com/1", {"bar" => "64", "deleted" => ""}), + simple_response(200, "bar") + ] + + request_cookies = [ + {"foo" => "42", "bar" => "64"}, + {"foo" => "42", "bar" => "64"} + ] + + res = redirector.perform(req, hops.shift) do |request| + req_cookie = HTTP::Cookie.cookie_value_to_hash(request.headers["Cookie"] || "") + expect(req_cookie).to eq request_cookies.shift + hops.shift + end + expect(res.to_s).to eq "bar" + cookies = res.cookies.cookies.map { |c| [c.name, c.value] }.to_h + expect(cookies["foo"]).to eq "42" + expect(cookies["bar"]).to eq "64" + expect(cookies["deleted"]).to eq nil + end + context "following 300 redirect" do context "with strict mode" do let(:options) { {:strict => true} }