Merge branch 'issue-174' into 'master'

altsvc improvements

Closes #174

See merge request honeyryderchuck/httpx!207

.rubocop.yml

@@ -28,7 +28,7 @@ AllCops:
     - 'test/extensions/response_pattern_match.rb'
 
 Metrics/ClassLength:
-  Max: 400
+  Enabled: false
 
 Metrics/MethodLength:
   Max: 200

lib/httpx/connection.rb

@@ -102,8 +102,8 @@ module HTTPX
           # origin came from an ORIGIN frame, we're going to verify the hostname with the
           # SSL certificate
           (@origins.size == 1 || @origin == uri.origin || (@io && @io.verify_hostname(uri.host)))
-        ) || match_altsvcs?(uri)
-      ) && @options == options
+        ) && @options == options
+      ) || (match_altsvcs?(uri) && match_altsvc_options?(uri, options))
     end
 
     def mergeable?(connection)
@@ -162,6 +162,14 @@ module HTTPX
         end
     end
 
+    def match_altsvc_options?(uri, options)
+      return @options == options unless @options.ssl[:hostname] == uri.host
+
+      dup_options = @options.merge(ssl: { hostname: nil })
+      dup_options.ssl.delete(:hostname)
+      dup_options == options
+    end
+
     def connecting?
       @state == :idle
     end

lib/httpx/plugins/response_cache/store.rb

@@ -37,6 +37,7 @@ module HTTPX::Plugins
             return unless request.headers.same_headers?(original_request.headers)
           else
             return unless vary.split(/ *, */).all? do |cache_field|
+              cache_field.downcase!
               !original_request.headers.key?(cache_field) || request.headers[cache_field] == original_request.headers[cache_field]
             end
           end

lib/httpx/session.rb

@@ -106,16 +106,21 @@ module HTTPX
     end
 
     def build_altsvc_connection(existing_connection, connections, alt_origin, origin, alt_params, options)
+      # do not allow security downgrades on altsvc negotiation
+      return if existing_connection.origin.scheme == "https" && alt_origin.scheme != "https"
+
       altsvc = AltSvc.cached_altsvc_set(origin, alt_params.merge("origin" => alt_origin))
 
       # altsvc already exists, somehow it wasn't advertised, probably noop
       return unless altsvc
 
-      connection = pool.find_connection(alt_origin, options) || build_connection(alt_origin, options)
+      alt_options = options.merge(ssl: options.ssl.merge(hostname: URI(origin).host))
+
+      connection = pool.find_connection(alt_origin, alt_options) || build_connection(alt_origin, alt_options)
       # advertised altsvc is the same origin being used, ignore
       return if connection == existing_connection
 
-      set_connection_callbacks(connection, connections, options)
+      set_connection_callbacks(connection, connections, alt_options)
 
       log(level: 1) { "#{origin} alt-svc: #{alt_origin}" }
 

sig/session.rbs

@@ -33,7 +33,7 @@ module HTTPX
 
     def set_connection_callbacks: (Connection, Array[Connection], Options) -> void
 
-    def build_altsvc_connection: (Connection, Array[Connection], URI::Generic, String, Hash[String, String], Options) -> Connection?
+    def build_altsvc_connection: (Connection existing_connection, Array[Connection] connections, URI::Generic alt_origin, String origin, Hash[String, String] alt_params, Options options) -> Connection?
 
     def build_requests: (verb | string, uri, options) -> Array[Request]
                       | (Array[[verb | string, uri, options]], options) -> Array[Request]

test/altsvc_test.rb

@@ -50,4 +50,18 @@ class AltSvcTest < Minitest::Test
             ["h3-Q043=:443", { "ma" => "2592000" }]],
            AltSvc.parse("quic=\":443\"; ma=2592000; v=\"46,43\",h3-Q046=\":443\"; ma=2592000,h3-Q043=\":443\"; ma=2592000").to_a
   end
+
+  def test_altsvc_clear_cache
+    AltSvc.cached_altsvc_set("http://www.example.com", { "origin" => "http://alt.example.com", "ma" => 2 })
+    entries = AltSvc.cached_altsvc("http://www.example.com")
+    assert !entries.empty?
+
+    req = Request.new(:get, "http://www.example.com/")
+    res = Response.new(req, 200, "2.0", { "alt-svc" => "clear" })
+
+    AltSvc.emit(req, res)
+
+    entries = AltSvc.cached_altsvc("http://www.example.com")
+    assert entries.empty?
+  end
 end

test/response_cache_store_test.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require_relative "test_helper"
+require "httpx/plugins/response_cache/store"
+
+class ResponseCacheStoreTest < Minitest::Test
+  include HTTPX
+
+  def test_store_cache
+    request = Request.new(:get, "http://example.com/")
+    response = cached_response(request)
+
+    assert store.lookup(request.uri) == response
+    assert store.cached?(request.uri)
+
+    request2 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain" })
+    assert store.lookup(request2.uri) == response
+  end
+
+  def test_prepare_vary
+    request = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain" })
+    cached_response(request, { "vary" => "Accept" })
+
+    request2 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/html" })
+    store.prepare(request2)
+    assert !request2.headers.key?("if-none-match")
+    request3 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain" })
+    store.prepare(request3)
+    assert request3.headers.key?("if-none-match")
+    request4 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain", "user-agent" => "Linux Bowser" })
+    store.prepare(request4)
+    assert request4.headers.key?("if-none-match")
+  end
+
+  def test_prepare_vary_asterisk
+    request = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain" })
+    cached_response(request, { "vary" => "*" })
+
+    request2 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/html" })
+    store.prepare(request2)
+    assert !request2.headers.key?("if-none-match")
+    request3 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain" })
+    store.prepare(request3)
+    assert request3.headers.key?("if-none-match")
+    request4 = Request.new(:get, "http://example.com/", headers: { "accept" => "text/plain", "user-agent" => "Linux Bowser" })
+    store.prepare(request4)
+    assert !request4.headers.key?("if-none-match")
+  end
+
+  private
+
+  def store
+    @store ||= Plugins::ResponseCache::Store.new
+  end
+
+  def cached_response(request, extra_headers = {})
+    response = Response.new(request, 200, "2.0", { "etag" => "ETAG" }.merge(extra_headers))
+    store.cache(request.uri, response)
+    response
+  end
+end

test/support/requests/plugins/response_cache.rb

@@ -42,25 +42,6 @@ module Requests
 
         # assert expired.body != uncached.body
       end
-
-      def test_plugin_response_cache_vary
-        skip
-        return unless origin.start_with?("https://")
-
-        vary_uri = "https://github.com/HoneyryderChuck/httpx"
-
-        HTTPX.plugin(:response_cache).wrap do |cache_client|
-          uncached = cache_client.get(vary_uri)
-          verify_status(uncached, 200)
-          cached = cache_client.get(vary_uri)
-          verify_status(cached, 304)
-
-          assert uncached.body == cached.body
-
-          uncached = cache_client.get(vary_uri, headers: { "accept" => "text/plain" })
-          verify_status(uncached, 200)
-        end
-      end
     end
   end
 end