Address push-SSRF review: defer DNS off enqueue path, disable proxy on pinned path, revalidate on re-registration

- Resolve the guarded endpoint IP lazily inside WebPush::Notification#deliver
  (on the bounded delivery worker) instead of eagerly when the notification is
  built on the serial enqueue path, so a slow resolver can't stall the push job
  before any delivery starts. resolved_endpoint_ip only reads the already-loaded
  endpoint attribute, so it is safe off the AR connection.
- Pin the delivery socket with an explicit nil proxy address so http_proxy/
  https_proxy can't route the request through a proxy that re-resolves the host
  and defeats the ipaddr pin.
- Revalidate an existing subscription on re-registration so a row predating
  endpoint validation gets the same 422 as a fresh create instead of being kept
  alive by touch.
- Regression tests: resolution deferred to delivery, pin survives proxy env,
  legacy invalid row rejected with 422.

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

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

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?

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"

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))

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

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