diff --git a/lib/web_push/connections.rb b/lib/web_push/connections.rb index 75370fe..df783fe 100644 --- a/lib/web_push/connections.rb +++ b/lib/web_push/connections.rb @@ -6,16 +6,24 @@ # same address for the same host. Net::HTTP reconnects to its pinned address, never to a new lookup of the host. class WebPush::Connections class ConnectionLost < StandardError; end + class StaleConnection < StandardError; end - # Net::HTTP checks an idle connection, and reconnects if the push service closed it, just before writing the - # request. Until then a dead connection can be replaced without sending the push twice. - module WriteTracking - attr_reader :request_written + # Where a request failed. Before writing it, Net::HTTP checks an idle connection (:checking) and connects again + # if the push service closed it (:connecting); then it writes (:sent). Only a failed check means a dead idle + # connection that a new one can replace without sending the push twice. + module Stages + attr_reader :stage - private def begin_transport(...) - @request_written = false - super.tap { @request_written = true } - end + private + def begin_transport(...) + @stage = :checking + super.tap { @stage = :sent } + end + + def connect(...) + @stage = :connecting if @stage == :checking + super + end end def initialize(keep_alive_timeout: 30) @@ -32,20 +40,17 @@ class WebPush::Connections address = [ http.address, http.port, http.ipaddr ] if idle = checkout(address) - response = begin - idle.request(request) - rescue IOError, SystemCallError, OpenSSL::SSL::SSLError => error - close(idle) - # The push service may have it: don't send it twice, and don't report a closed connection as a TLS failure - raise ConnectionLost, "#{error.class}: #{error.message}" if idle.request_written + begin + return send_over(idle, request, reused: true).tap { checkin(address, idle) } + rescue StaleConnection + # The push service had closed it: a new connection takes the push end - return response.tap { checkin(address, idle) } if response end - http.extend WriteTracking + http.extend Stages http.keep_alive_timeout = @keep_alive_timeout http.start - http.request(request).tap { checkin(address, http) } + send_over(http, request, reused: false).tap { checkin(address, http) } end def shutdown @@ -58,6 +63,17 @@ class WebPush::Connections end private + def send_over(http, request, reused:) + http.request(request) + rescue IOError, SystemCallError, OpenSSL::SSL::SSLError => error + close(http) + # Only a connection that was idle can have been dead already: on a new one the error is the push service's + raise StaleConnection if reused && http.stage == :checking + # The push service may have it: don't send it twice, and don't report a dropped connection as a TLS failure + raise ConnectionLost, "#{error.class}: #{error.message}" if http.stage == :sent + raise + end + def checkout(address) @mutex.synchronize do forget_after_fork diff --git a/test/lib/web_push/connections_test.rb b/test/lib/web_push/connections_test.rb index 70673c0..d465702 100644 --- a/test/lib/web_push/connections_test.rb +++ b/test/lib/web_push/connections_test.rb @@ -68,6 +68,35 @@ class WebPush::ConnectionsTest < ActiveSupport::TestCase end end + test "a push the service may have received on a new connection isn't reported as a TLS failure either" do + with_push_service(drop_request: ->(number) { number == 1 }) do |server| + error = assert_raises(WebPush::Connections::ConnectionLost) { @connections.request(pinned_connection(server), push_request) } + assert_not_kind_of OpenSSL::OpenSSLError, error + assert_equal 1, server.requests.size + end + end + + test "a TLS failure on a new connection before the push is written is reported as it is" do + failing_check = Module.new { private def begin_transport(*) = raise(OpenSSL::SSL::SSLError, "alert right after the handshake") } + + with_push_service do |server| + assert_raises(OpenSSL::SSL::SSLError) { @connections.request(pinned_connection(server).extend(failing_check), push_request) } + assert_empty server.requests + end + end + + test "a certificate for another name when reconnecting a cleanly closed connection is reported, even if a new one would work" do + certificates = { 2 => "other.test" } + with_push_service(hang_up_after_response: :close_notify, certificate: ->(connection) { certificates.fetch(connection, HOST) }) 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 + assert_equal 2, 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 }