diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index 65fe9d2..f4bae3d 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -6,8 +6,16 @@ class Users::PushSubscriptionsController < ApplicationController def create if subscription = @push_subscriptions.find_by(push_subscription_params) - subscription.touch - head :ok + # Re-validate on re-registration: a row that predates endpoint validation + # (or was inserted around it) must get the same 422 as a fresh create + # rather than being kept alive by touch. Delivery already fails closed for + # such a row; this keeps both create paths on one contract. + if subscription.valid? + subscription.touch + head :ok + else + head :unprocessable_entity + end else subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent) head subscription.persisted? ? :ok : :unprocessable_entity diff --git a/app/models/push/subscription.rb b/app/models/push/subscription.rb index b317821..9c93fa1 100644 --- a/app/models/push/subscription.rb +++ b/app/models/push/subscription.rb @@ -21,7 +21,12 @@ class Push::Subscription < ApplicationRecord validate :validate_endpoint_url def notification(**params) - WebPush::Notification.new(**params, badge: user.memberships.unread.count, endpoint: endpoint, endpoint_ip: resolved_endpoint_ip, p256dh_key: p256dh_key, auth_key: auth_key) + # Pass the guarded-resolution as a callable, not an already-resolved IP: the + # notification is built here on the serial enqueue path (for the unread badge + # count and other AR reads), but the DNS lookup must happen later, on the + # bounded delivery worker. resolved_endpoint_ip only reads the already-loaded + # endpoint attribute, so invoking it off-thread needs no AR connection. + WebPush::Notification.new(**params, badge: user.memberships.unread.count, endpoint: endpoint, endpoint_ip_resolver: method(:resolved_endpoint_ip), p256dh_key: p256dh_key, auth_key: auth_key) end # The public address to pin this delivery to, or nil when the endpoint is not a diff --git a/config/initializers/web_push.rb b/config/initializers/web_push.rb index 30f9ec7..f0a28fa 100644 --- a/config/initializers/web_push.rb +++ b/config/initializers/web_push.rb @@ -22,7 +22,14 @@ module WebPush::PersistentRequest # Push::Subscription so delivery can't be rebound to a private address # between resolution and connect. Bypasses the shared persistent pool, # which would re-resolve the host itself. - http = Net::HTTP.new(uri.host, uri.port) + # + # The explicit nil proxy address disables proxy discovery from + # http_proxy/https_proxy. An egress proxy would open the TCP connection + # itself and re-resolve the endpoint host, so http.ipaddr would no longer + # pin the destination and the DNS-rebinding guarantee would be lost. This + # path is already committed to a direct connection (it bypasses the pool); + # push delivery to public vendor endpoints goes direct. + http = Net::HTTP.new(uri.host, uri.port, nil) http.ipaddr = endpoint_ip http.use_ssl = true http.ssl_timeout = @options[:ssl_timeout] unless @options[:ssl_timeout].nil? diff --git a/lib/web_push/notification.rb b/lib/web_push/notification.rb index f1ba1df..7d21bcb 100644 --- a/lib/web_push/notification.rb +++ b/lib/web_push/notification.rb @@ -1,19 +1,26 @@ class WebPush::Notification - def initialize(title:, body:, path:, badge:, endpoint:, endpoint_ip:, p256dh_key:, auth_key:) + def initialize(title:, body:, path:, badge:, endpoint:, endpoint_ip_resolver:, p256dh_key:, auth_key:) @title, @body, @path, @badge = title, body, path, badge - @endpoint, @endpoint_ip, @p256dh_key, @auth_key = endpoint, endpoint_ip, p256dh_key, auth_key + @endpoint, @endpoint_ip_resolver, @p256dh_key, @auth_key = endpoint, endpoint_ip_resolver, p256dh_key, auth_key end - # @endpoint_ip is the public address Push::Subscription resolved and guarded for - # this delivery. When it is nil the host resolved to nothing or to a blocked - # (private) address, so we skip delivery rather than let the request fall back - # to re-resolving the raw host -- which is what would reopen the SSRF for a - # subscription that slipped in before endpoint validation existed. + # @endpoint_ip_resolver resolves and guards the endpoint's public address -- + # Push::Subscription's allowlist plus surfguard's private-network classification + # -- returning the IP to pin, or nil. It is invoked here, on the bounded + # delivery worker, rather than when the notification is built on the serial + # enqueue path, so a slow or stalled resolver can't hold up the push job before + # any delivery starts (one blocking lookup per recipient, serialized, would + # otherwise multiply a resolver timeout by the room's subscriber count). + # + # nil means the host resolved to nothing or to a blocked (private) address, so + # we skip delivery rather than let the request fall back to re-resolving the + # raw host -- which is what would reopen the SSRF for a subscription that + # slipped in before endpoint validation existed. def deliver(connection: nil) - if @endpoint_ip + if endpoint_ip = @endpoint_ip_resolver.call WebPush.payload_send \ message: encoded_message, - endpoint: @endpoint, endpoint_ip: @endpoint_ip, p256dh: @p256dh_key, auth: @auth_key, + endpoint: @endpoint, endpoint_ip: endpoint_ip, p256dh: @p256dh_key, auth: @auth_key, vapid: vapid_identification, connection: connection, urgency: "high" diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index 449840e..e69fc4a 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/test/controllers/users/push_subscriptions_controller_test.rb @@ -51,6 +51,23 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest assert_response :unprocessable_entity end + test "re-registering a legacy invalid subscription is rejected with 422" do + # A row that predates endpoint validation (saved without validation, as a + # sink planted before this shipped could be). Re-POSTing its params must hit + # the same 422 as a fresh create, not be kept alive by touch. + legacy = users(:david).push_subscriptions.build \ + endpoint: "https://attacker.example.com/steal", p256dh_key: "123", auth_key: "456" + legacy.save!(validate: false) + + assert_no_difference -> { Push::Subscription.count } do + post user_push_subscriptions_url, params: { + push_subscription: { endpoint: "https://attacker.example.com/steal", p256dh_key: "123", auth_key: "456" } + } + end + + assert_response :unprocessable_entity + end + test "destroy a push subscription via dev mode" do assert_difference -> { Push::Subscription.count }, -1 do delete user_push_subscription_url(push_subscriptions(:david_chrome)) diff --git a/test/lib/web_push/persistent_request_test.rb b/test/lib/web_push/persistent_request_test.rb index c0f4742..d570548 100644 --- a/test/lib/web_push/persistent_request_test.rb +++ b/test/lib/web_push/persistent_request_test.rb @@ -23,4 +23,31 @@ class WebPush::PersistentRequestTest < ActiveSupport::TestCase urgency: "high" end end + + # An egress proxy would open the TCP connection itself and re-resolve the + # endpoint host, defeating the ipaddr pin. The pinned path must ignore + # http_proxy/https_proxy and connect straight to the resolved public IP. + test "ignores proxy env so the pin can't be routed through a re-resolving proxy" 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 == "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: {}, + 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 end diff --git a/test/models/push/subscription_test.rb b/test/models/push/subscription_test.rb index de5bb51..6dce213 100644 --- a/test/models/push/subscription_test.rb +++ b/test/models/push/subscription_test.rb @@ -83,11 +83,18 @@ class Push::SubscriptionTest < ActiveSupport::TestCase assert_equal DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, subscription.resolved_endpoint_ip end - test "notification carries the resolved endpoint IP for delivery pinning" do + test "endpoint resolution is deferred from the enqueue path to the delivery worker" do + lookups = 0 + # A side-effecting matcher lets us count resolver calls without a real lookup. + Resolv.stubs(:getaddresses).with { |*| lookups += 1; true }.returns([ DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP ]) + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") notification = subscription.notification(title: "t", body: "b", path: "/") + assert_equal 0, lookups, "building the notification must not resolve DNS on the serial enqueue path" - assert_equal DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, notification.instance_variable_get(:@endpoint_ip) + WebPush.stubs(:payload_send) + notification.deliver + assert_operator lookups, :>, 0, "delivery must resolve and pin the endpoint IP on the worker" end test "delivery is skipped when the endpoint no longer resolves to a public IP" do