FIX: Fix handling of multi-cookie responses

FIX: Correctly parse relative redirects using Addressable Also rename 'header' to 'redir_header' to deconflict with 'headers' parameter Add tests for redirect handling
discourse · Jun 29, 2020 · ac25fc8 · ac25fc8
1 parent 5fd30f8
commit ac25fc8
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 12 deletions.
diff --git a/lib/onebox/helpers.rb b/lib/onebox/helpers.rb
@@ -51,17 +51,14 @@ def self.fetch_response(location, limit = nil, domain = nil, headers = nil)
 
       raise Net::HTTPError.new('HTTP redirect too deep', location) if limit == 0
 
-      uri = URI(location)
-      uri = URI("#{domain}#{location}") if !uri.host
+      uri = Addressable::URI.parse(location)
+      uri = Addressable::URI.join(domain, uri) if !uri.host
 
       result = StringIO.new
-      Net::HTTP.start(uri.host, uri.port, use_ssl: uri.is_a?(URI::HTTPS)) do |http|
+      Net::HTTP.start(uri.host, uri.port, use_ssl: uri.normalized_scheme == 'https') do |http|
         http.open_timeout = Onebox.options.connect_timeout
         http.read_timeout = Onebox.options.timeout
-        if uri.is_a?(URI::HTTPS)
-          http.use_ssl = true
-          http.verify_mode = OpenSSL::SSL::VERIFY_NONE
-        end
+        http.verify_mode = OpenSSL::SSL::VERIFY_NONE  # Work around path building bugs
 
         headers ||= {}
 
@@ -76,10 +73,12 @@ def self.fetch_response(location, limit = nil, domain = nil, headers = nil)
         http.request(request) do |response|
 
           if cookie = response.get_fields('set-cookie')
-            header = { 'Cookie' => cookie.join }
+            # HACK: If this breaks again in the future, use HTTP::CookieJar from gem 'http-cookie'
+            # See test: it "does not send cookies to the wrong domain"
+            redir_header = { 'Cookie' => cookie.join('; ') }
           end
 
-          header = nil unless header.is_a? Hash
+          redir_header = nil unless redir_header.is_a? Hash
 
           code = response.code.to_i
           unless code === 200
@@ -88,7 +87,7 @@ def self.fetch_response(location, limit = nil, domain = nil, headers = nil)
               response['location'],
               limit - 1,
               "#{uri.scheme}://#{uri.host}",
-              header
+              redir_header
             )
           end
 

diff --git a/spec/lib/onebox/helpers_spec.rb b/spec/lib/onebox/helpers_spec.rb
@@ -44,6 +44,53 @@
     end
   end
 
+  describe "redirects" do
+    describe "redirect limit" do
+      before do
+        (1..6).each do |i|
+          FakeWeb.register_uri(:get, "https://httpbin.org/redirect/#{i}", location: "https://httpbin.org/redirect/#{i - 1}", body: "", status: [302, "Found"])
+        end
+        fake("https://httpbin.org/redirect/0", "<!DOCTYPE html><p>success</p>")
+      end
+
+      it "can follow redirects" do
+        expect(described_class.fetch_response("https://httpbin.org/redirect/2")).to match("success")
+      end
+
+      it "errors on long redirect chains" do
+        expect {
+          described_class.fetch_response("https://httpbin.org/redirect/6")
+        }.to raise_error(Net::HTTPError, /redirect too deep/)
+      end
+    end
+
+    describe "cookie handling" do
+      it "naively forwards cookies to the next request" do
+        FakeWeb.register_uri(:get, "https://httpbin.org/cookies/set/a/b",
+                             location: "/cookies",
+                             'set-cookie': "a=b; Path=/",
+                             status: [302, "Found"])
+        fake("https://httpbin.org/cookies", "success, cookie readback not implemented")
+
+        expect(described_class.fetch_response('https://httpbin.org/cookies/set/a/b')).to match("success")
+        expect(FakeWeb.last_request.to_hash['cookie'][0]).to match("a=b")
+      end
+
+      it "does not send cookies to the wrong domain" do
+        skip("unimplemented")
+
+        FakeWeb.register_uri(:get, "https://httpbin.org/cookies/set/a/b",
+                             location: "https://evil.com/show_cookies",
+                             'set-cookie': "a=b; Path=/",
+                             status: [302, "Found"])
+        fake("https://evil.com/show_cookies", "success, cookie readback not implemented")
+
+        described_class.fetch_response('https://httpbin.org/cookies/set/a/b')
+        expect(FakeWeb.last_request.to_hash['cookie']).to be_nil
+      end
+    end
+  end
+
   describe "user_agent" do
     before do
       fake("http://example.com/some-resource", body: 'test')

diff --git a/spec/support/html_spec_helper.rb b/spec/support/html_spec_helper.rb
@@ -2,10 +2,10 @@
 
 module HTMLSpecHelper
   def fake(uri, response, verb = :get)
-    FakeWeb.register_uri(verb, uri, response: header(response))
+    FakeWeb.register_uri(verb, uri, response: http_ok(response))
   end
 
-  def header(html)
+  def http_ok(html)
     "HTTP/1.1 200 OK\n\n#{html}"
   end