Reuse push connections pinned to the address the guard just approved

Since push delivery was pinned to the IP resolved and guarded for it, every push opens a new TCP and TLS connection: Net::HTTP::Persistent looks the host up itself and can't be pinned. The handshake is one or two extra round trips for every push. WebPush::Connections keeps the pinned connections open for 30 seconds and hands one out again only to a delivery whose own, fresh resolution returned the same address for the same host. Net::HTTP only ever reconnects to that address, so no request goes to an address the guard didn't just approve. A connection the push service closed while idle is replaced before the push is written; once it's written, a dropped connection raises ConnectionLost instead of sending the push twice or invalidating the subscription. A delivery without a resolved IP is no longer sent at all, and net-http-persistent goes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
2026-10-08 07:40:08 +09:00 · 2026-10-05 17:59:57 +02:00
parent 2393f01ba2
commit 53520d4444
8 changed files with 444 additions and 38 deletions
@@ -0,0 +1,127 @@
+require "test_helper"
+
+class WebPush::ConnectionsTest < ActiveSupport::TestCase
+  include PushServiceTestHelper
+
+  setup { @connections = WebPush::Connections.new }
+  teardown { @connections.shutdown }
+
+  test "a delivery to the same host and address reuses the open connection" do
+    with_push_service do |server|
+      2.times { |i| assert_kind_of Net::HTTPCreated, @connections.request(pinned_connection(server), push_request("/push/#{i}")) }
+
+      assert_equal 1, server.connections
+      assert_equal %w[ /push/0 /push/1 ], server.requests
+    end
+  end
+
+  test "a delivery to another address never reuses a connection" do
+    with_push_service do |server|
+      @connections.request(pinned_connection(server), push_request)
+
+      other = pinned_connection(server, "127.0.0.2")
+      other.expects(:start)
+      other.expects(:request).returns(:sent_to_the_other_address)
+      assert_equal :sent_to_the_other_address, @connections.request(other, push_request)
+      assert_equal 1, server.requests.size
+    end
+  end
+
+  test "a connection the push service closed while idle is replaced, and the push sent once" do
+    [ :fin, :close_notify ].each do |hang_up|
+      with_push_service(hang_up_after_response: hang_up) do |server|
+        @connections.request(pinned_connection(server), push_request("/push/0"))
+        assert server.hung_up?
+
+        assert_kind_of Net::HTTPCreated, @connections.request(pinned_connection(server), push_request("/push/1"))
+        assert_equal 2, server.connections
+        assert_equal %w[ /push/0 /push/1 ], server.requests
+      end
+    end
+  end
+
+  test "a certificate for another name is refused, and nothing is sent" do
+    with_push_service(certificate: ->(_) { "other.test" }) do |server|
+      assert_raises(OpenSSL::SSL::SSLError) { @connections.request(pinned_connection(server), push_request) }
+      assert_empty server.requests
+    end
+  end
+
+  test "a certificate for another name is refused when a dead connection is replaced too" do
+    with_push_service(hang_up_after_response: :fin, certificate: ->(connection) { connection == 1 ? HOST : "other.test" }) do |server|
+      @connections.request(pinned_connection(server), push_request)
+      assert server.hung_up?
+
+      assert_raises(OpenSSL::SSL::SSLError) { @connections.request(pinned_connection(server), push_request) }
+      assert_equal 1, server.requests.size
+    end
+  end
+
+  test "a push the service may have received is not sent again" do
+    with_push_service(drop_request: ->(number) { number == 2 }) do |server|
+      @connections.request(pinned_connection(server), push_request("/push/0"))
+
+      error = assert_raises(WebPush::Connections::ConnectionLost) { @connections.request(pinned_connection(server), push_request("/push/1")) }
+      assert_not_kind_of OpenSSL::OpenSSLError, error
+      assert_equal %w[ /push/0 /push/1 ], server.requests
+      assert_equal 1, server.connections
+    end
+  end
+
+  test "only new, direct TLS connections pinned to an address are pooled" do
+    with_push_service do |server|
+      unpinned = Net::HTTP.new(HOST, server.port, nil).tap { it.use_ssl = true }
+      proxied = Net::HTTP.new(HOST, server.port, "127.0.0.1", 3128).tap { it.ipaddr = IP; it.use_ssl = true }
+      plain = pinned_connection(server).tap { it.use_ssl = false }
+      started = pinned_connection(server).tap(&:start)
+
+      [ unpinned, proxied, plain, started ].each do |http|
+        assert_raises(ArgumentError) { @connections.request(http, push_request) }
+      end
+      assert_empty server.requests
+    ensure
+      started&.finish
+    end
+  end
+
+  test "a forked process opens its own connections" do
+    with_push_service do |server|
+      @connections.request(pinned_connection(server), push_request)
+
+      child = Process.pid + 1
+      Process.stubs(:pid).returns(child)
+      @connections.request(pinned_connection(server), push_request)
+      assert_equal 2, server.connections
+    end
+  end
+
+  test "a forked process shutting down leaves its parent's connections open" do
+    with_push_service do |server|
+      @connections.request(pinned_connection(server), push_request)
+
+      child = Process.pid + 1
+      Process.stubs(:pid).returns(child)
+      @connections.shutdown
+      assert_not server.hung_up?(within: 0.5)
+    end
+  end
+
+  test "idle connections are closed after the keep-alive timeout and on shutdown" do
+    connections = WebPush::Connections.new(keep_alive_timeout: 0)
+
+    with_push_service do |server|
+      other = Server.new
+      connections.request(pinned_connection(server), push_request)
+      connections.request(pinned_connection(other), push_request)
+      assert server.hung_up?
+
+      connections.shutdown
+      assert other.hung_up?
+
+      connections.request(pinned_connection(server), push_request)
+      assert server.hung_up?
+    ensure
+      other&.stop
+    end
+  end
+end
@@ -1,6 +1,8 @@
 require "test_helper"

 class WebPush::PersistentRequestTest < ActiveSupport::TestCase
+  include PushServiceTestHelper
+
  ENDPOINT = "https://fcm.googleapis.com/fcm/send/test123"

  # The delivery must connect to the public IP resolved and guarded by
@@ -50,4 +52,72 @@ class WebPush::PersistentRequestTest < ActiveSupport::TestCase
    %w[ http_proxy https_proxy HTTP_PROXY HTTPS_PROXY ].each { |k| ENV.delete(k) }
    saved.each { |k, v| ENV[k] = v }
  end
+
+  test "deliveries through the pool to the same host and address share one connection" do
+    pool = WebPush::Pool.new(invalid_subscription_handler: nil)
+
+    with_push_service do |server|
+      2.times { deliver_to server, connection: pool.connection }
+
+      assert_equal 2, server.requests.size
+      assert_equal 1, server.connections
+    end
+  ensure
+    pool.shutdown
+  end
+
+  test "a pooled delivery checks the response like any other" do
+    pool = WebPush::Pool.new(invalid_subscription_handler: nil)
+
+    with_push_service(status: "410 Gone") do |server|
+      assert_raises(WebPush::ExpiredSubscription) { deliver_to server, connection: pool.connection }
+    end
+  ensure
+    pool.shutdown
+  end
+
+  test "pooled deliveries are pinned and ignore proxy env too" do
+    host = URI(ENDPOINT).host
+    saved = ENV.slice("http_proxy", "https_proxy", "HTTP_PROXY", "HTTPS_PROXY")
+    %w[ http_proxy https_proxy HTTP_PROXY HTTPS_PROXY ].each { |k| ENV[k] = "http://proxy.internal:3128" }
+
+    WebMock.disable_net_connect! allow: [ host ]
+
+    TCPSocket.expects(:open).with { |*args, **| args.first == host || args.first == "proxy.internal" }.never
+    TCPSocket.expects(:open).with { |*args, **| args.first == DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP && args[1] == 443 }.throws(:pinned_to_ip)
+
+    assert_throws :pinned_to_ip do
+      WebPush.payload_send \
+        message: "",
+        endpoint: ENDPOINT,
+        endpoint_ip: DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP,
+        p256dh: "", auth: "", vapid: {},
+        connection: WebPush::Connections.new,
+        urgency: "high"
+    end
+  ensure
+    %w[ http_proxy https_proxy HTTP_PROXY HTTPS_PROXY ].each { |k| ENV.delete(k) }
+    saved.each { |k, v| ENV[k] = v }
+  end
+
+  test "nothing is sent without a resolved endpoint IP" do
+    WebMock.disable_net_connect! allow: [ URI(ENDPOINT).host ]
+    TCPSocket.expects(:open).never
+
+    error = assert_raises(ArgumentError) do
+      WebPush.payload_send message: "", endpoint: ENDPOINT, p256dh: "", auth: "", vapid: {}, urgency: "high"
+    end
+    assert_equal "Push deliveries must be pinned to a resolved endpoint IP", error.message
+  end
+
+  private
+    def deliver_to(server, connection:)
+      WebPush.payload_send \
+        message: "",
+        endpoint: "https://#{PushServiceTestHelper::HOST}:#{server.port}/fcm/send/test123",
+        endpoint_ip: PushServiceTestHelper::IP,
+        p256dh: "", auth: "", vapid: {},
+        connection: connection,
+        urgency: "high"
+    end
 end