Guard push-subscription endpoints against SSRF (#265)

* Guard push-subscription endpoints against SSRF

Web push delivery POSTed to the endpoint URL a user supplied when
registering a subscription, with no scheme, host, or private-network
check -- unlike the OpenGraph unfurl path, which already routes through
the shared SSRF address policy (surfguard). Any authenticated user could
register a subscription whose endpoint pointed at an internal address and
have the server fetch it on every chat message: blind SSRF for internal
recon and reachability probing, plus a thread-pool DoS on the delivery
pool.

Validate the endpoint when the subscription is saved: it must be HTTPS,
its host must belong to a known browser push service (allowlist), and it
must resolve to a public IP. On every delivery, re-resolve the host and
pin the connection to that public IP so a later DNS rebind can't redirect
the request to an internal address. If no public IP resolves at delivery
time -- a rebind, or a subscription that predates this validation --
delivery is skipped rather than falling back to re-resolving the raw
host.

Adds model, controller, and delivery-pinning tests, plus a DNS stub
helper for deterministic resolution in tests.

* Route push endpoint guarding through RestrictedHTTP::PrivateNetworkGuard

The endpoint SSRF check already delegated its private-network classification
to surfguard, but called Surfguard.resolve_public_ips directly rather than
through RestrictedHTTP::PrivateNetworkGuard -- the hostname-in, address-out
shim #241 established as the app's single guarded-outbound entry point and
that Opengraph::Fetch resolves through. Route push resolution through the same
guard so once-campfire keeps one place that resolves and classifies outbound
addresses. Behavior is unchanged: the guard raises Violation when a permitted
host resolves only to blocked addresses (previously an empty list -> nil) and
propagates Surfguard::Unresolvable for a host that resolves to nothing; both
map to a nil endpoint IP, which fails validation and skips delivery. The
allowlist, HTTPS/443 constraints, and per-delivery IP pinning are unchanged.

* 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.

* Bound the web-push delivery queue (max_queue, not the ignored queue_size)

Concurrent::ThreadPoolExecutor takes :max_queue; :queue_size was silently
ignored, leaving the delivery backlog unbounded (max_queue: 0). A flood of
valid push subscriptions could accumulate queued deliveries without limit --
more acute now that each delivery task also resolves DNS. Using max_queue: 10000
activates the intended cap; overflow raises RejectedExecutionError under the
default :abort policy, which deliver_later already rescues (push is best-effort,
retried on the next message).

* Trim push-SSRF guard comments to match house style

Apply the review suggestions on the push-subscription SSRF guard: replace
the verbose rationale comments with terse one-liners (or drop them where
the code speaks for itself). No behavior change -- the Surfguard-shim
routing, per-delivery public-IP pin, and bounded delivery queue are
untouched.

app/controllers/users/push_subscriptions_controller.rb

@@ -6,12 +6,17 @@ class Users::PushSubscriptionsController < ApplicationController
 
   def create
     if subscription = @push_subscriptions.find_by(push_subscription_params)
-      subscription.touch
+      # Existing endpoints must pass current validations
+      if subscription.valid?
+        subscription.touch
+        head :ok
+      else
+        head :unprocessable_entity
+      end
     else
-      @push_subscriptions.create! push_subscription_params.merge(user_agent: request.user_agent)
+      subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent)
+      head subscription.persisted? ? :ok : :unprocessable_entity
     end
-
-    head :ok
   end
 
   def destroy

app/models/push/subscription.rb

@@ -1,7 +1,61 @@
+require "restricted_http/private_network_guard"
+
 class Push::Subscription < ApplicationRecord
+  PERMITTED_ENDPOINT_HOSTS = %w[
+    jmt17.google.com
+    fcm.googleapis.com
+    updates.push.services.mozilla.com
+    web.push.apple.com
+    notify.windows.com
+  ].freeze
+
   belongs_to :user
 
+  validates :endpoint, presence: true
+  validate :validate_endpoint_url
+
   def notification(**params)
-    WebPush::Notification.new(**params, badge: user.memberships.unread.count, endpoint: endpoint, p256dh_key: p256dh_key, auth_key: auth_key)
+    # Defer DNS lookup to the delivery worker to prevent rebinding
+    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
+
+  # Validate at point of use, not just when saved.
+  def resolved_endpoint_ip
+    RestrictedHTTP::PrivateNetworkGuard.resolve(endpoint_uri.host) if permitted_endpoint_uri?
+  rescue RestrictedHTTP::Violation, Surfguard::Unresolvable
+    nil
+  end
+
+  private
+    # Validate endpoint shape. Belt & suspenders.
+    def permitted_endpoint_uri?
+      endpoint_uri&.scheme == "https" && endpoint_uri.port == 443 && permitted_endpoint_host?
+    end
+
+    def endpoint_uri
+      URI.parse(endpoint) if endpoint.present?
+    rescue URI::InvalidURIError
+      nil
+    end
+
+    def validate_endpoint_url
+      if endpoint_uri.nil?
+        errors.add(:endpoint, "is not a valid URL")
+      elsif endpoint_uri.scheme != "https"
+        errors.add(:endpoint, "must use HTTPS")
+      elsif endpoint_uri.port != 443
+        errors.add(:endpoint, "must use the default HTTPS port")
+      elsif !permitted_endpoint_host?
+        errors.add(:endpoint, "is not a permitted push service")
+      elsif resolved_endpoint_ip.nil?
+        errors.add(:endpoint, "resolves to a private or invalid IP address")
+      end
+    end
+
+    def permitted_endpoint_host?
+      host = endpoint_uri&.host&.downcase
+      host.present? && PERMITTED_ENDPOINT_HOSTS.any? do |permitted|
+        host == permitted || host.end_with?(".#{permitted}")
+      end
+    end
 end

config/initializers/web_push.rb

@@ -17,7 +17,25 @@ end
 
 module WebPush::PersistentRequest
   def perform
-    if @options[:connection]
+    if endpoint_ip = @options[:endpoint_ip]
+      # Pin the connection to the public IP resolved (and guarded) by
+      # 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.
+      #
+      # 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?
+      http.open_timeout = @options[:open_timeout] unless @options[:open_timeout].nil?
+      http.read_timeout = @options[:read_timeout] unless @options[:read_timeout].nil?
+    elsif @options[:connection]
       http = @options[:connection]
     else
       http = Net::HTTP.new(uri.host, uri.port, *proxy_options)

lib/web_push/notification.rb

@@ -1,16 +1,18 @@
 class WebPush::Notification
-  def initialize(title:, body:, path:, badge:, endpoint:, 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, @p256dh_key, @auth_key = endpoint, p256dh_key, auth_key
+    @endpoint, @endpoint_ip_resolver, @p256dh_key, @auth_key = endpoint, endpoint_ip_resolver, p256dh_key, auth_key
   end
 
   def deliver(connection: nil)
-    WebPush.payload_send \
-      message: encoded_message,
-      endpoint: @endpoint, p256dh: @p256dh_key, auth: @auth_key,
-      vapid: vapid_identification,
-      connection: connection,
-      urgency: "high"
+    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,
+        vapid: vapid_identification,
+        connection: connection,
+        urgency: "high"
+    end
   end
 
   private

lib/web_push/pool.rb

@@ -3,7 +3,7 @@ class WebPush::Pool
   attr_reader :delivery_pool, :invalidation_pool, :connection, :invalid_subscription_handler
 
   def initialize(invalid_subscription_handler:)
-    @delivery_pool = Concurrent::ThreadPoolExecutor.new(max_threads: 50, queue_size: 10000)
+    @delivery_pool = Concurrent::ThreadPoolExecutor.new(max_threads: 50, max_queue: 10000)
     @invalidation_pool = Concurrent::FixedThreadPool.new(1)
     @connection = Net::HTTP::Persistent.new(name: "web_push", pool_size: 150)
     @invalid_subscription_handler = invalid_subscription_handler

test/controllers/users/push_subscriptions_controller_test.rb

@@ -3,10 +3,11 @@ require "test_helper"
 class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
   setup do
     sign_in :david
+    stub_web_push_dns_resolution
   end
 
   test "create new push subscription" do
-    subscription_params = { "endpoint" => "https://apple", "p256dh_key" => "123", "auth_key" => "456" }
+    subscription_params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" }
 
     post user_push_subscriptions_url,
       params: { push_subscription: subscription_params }, headers: { "HTTP_USER_AGENT" => "Mozilla/5.0" }
@@ -29,6 +30,44 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
     assert_response :ok
   end
 
+  test "rejects subscription with non-permitted endpoint" do
+    subscription_params = { "endpoint" => "https://attacker.example.com/steal", "p256dh_key" => "123", "auth_key" => "456" }
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: subscription_params }
+    end
+
+    assert_response :unprocessable_entity
+  end
+
+  test "rejects subscription with endpoint resolving to a private IP" do
+    stub_dns_resolution("169.254.169.254")
+    subscription_params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" }
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: subscription_params }
+    end
+
+    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

@@ -0,0 +1,53 @@
+require "test_helper"
+
+class WebPush::PersistentRequestTest < ActiveSupport::TestCase
+  ENDPOINT = "https://fcm.googleapis.com/fcm/send/test123"
+
+  # The delivery must connect to the public IP resolved and guarded by
+  # Push::Subscription, never re-resolve the raw endpoint host at connect time --
+  # otherwise a rebind between resolution and delivery reopens the SSRF. An empty
+  # message keeps the request past encryption and onto the socket we assert on.
+  test "pins delivery to endpoint_ip instead of re-resolving the host" do
+    host = URI(ENDPOINT).host
+    WebMock.disable_net_connect! allow: [ host ]
+
+    TCPSocket.expects(:open).with { |*args, **| args.first == host }.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
+  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

@@ -0,0 +1,152 @@
+require "test_helper"
+
+class Push::SubscriptionTest < ActiveSupport::TestCase
+  setup do
+    stub_web_push_dns_resolution
+  end
+
+  test "valid subscription with permitted endpoint" do
+    assert build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123").valid?
+  end
+
+  test "rejects endpoint with non-https scheme" do
+    subscription = build_subscription(endpoint: "http://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "must use HTTPS"
+  end
+
+  test "rejects endpoint with non-permitted host" do
+    subscription = build_subscription(endpoint: "https://attacker.example.com/webhook")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "is not a permitted push service"
+  end
+
+  test "rejects endpoint whose host only suffix-matches a permitted host" do
+    subscription = build_subscription(endpoint: "https://evilfcm.googleapis.com.attacker.example/webhook")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "is not a permitted push service"
+  end
+
+  test "rejects blank endpoint" do
+    subscription = build_subscription(endpoint: "")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "can't be blank"
+  end
+
+  test "rejects endpoint on a non-default port" do
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com:8443/fcm/send/abc123")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "must use the default HTTPS port"
+  end
+
+  test "rejects endpoint that resolves to private IP" do
+    stub_dns_resolution("192.168.1.1")
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address"
+  end
+
+  test "rejects endpoint that resolves to loopback IP" do
+    stub_dns_resolution("127.0.0.1")
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address"
+  end
+
+  test "rejects endpoint that resolves to link-local IP (AWS IMDS)" do
+    stub_dns_resolution("169.254.169.254")
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address"
+  end
+
+  test "rejects endpoint whose host resolves to nothing without raising" do
+    stub_dns_failure
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_nil subscription.resolved_endpoint_ip
+    assert_not subscription.valid?
+    assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address"
+  end
+
+  test "resolved_endpoint_ip returns the pinned public IP" do
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_equal DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, subscription.resolved_endpoint_ip
+  end
+
+  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"
+
+    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
+    stub_dns_resolution("10.0.0.5") # host now answers with a private address
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    assert_nil subscription.resolved_endpoint_ip
+    WebPush.expects(:payload_send).never
+    subscription.notification(title: "t", body: "b", path: "/").deliver
+  end
+
+  test "delivery is skipped for a non-permitted host even when it resolves publicly" do
+    # A row that predates endpoint validation: its host resolves to a public IP,
+    # but it is not a permitted push service, so delivery must not proceed.
+    subscription = build_subscription(endpoint: "https://attacker.example.com/collect")
+
+    assert_nil subscription.resolved_endpoint_ip
+    WebPush.expects(:payload_send).never
+    subscription.notification(title: "t", body: "b", path: "/").deliver
+  end
+
+  test "delivery is skipped for a permitted host on a non-default port" do
+    # A legacy/bypassed row: permitted host, resolves publicly, but port 22.
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com:22/fcm/send/abc123")
+
+    assert_nil subscription.resolved_endpoint_ip
+    WebPush.expects(:payload_send).never
+    subscription.notification(title: "t", body: "b", path: "/").deliver
+  end
+
+  test "delivery sends with the pinned endpoint_ip" do
+    subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123")
+
+    WebPush.expects(:payload_send).with(has_entry(endpoint_ip: DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP))
+    subscription.notification(title: "t", body: "b", path: "/").deliver
+  end
+
+  test "accepts all permitted push service domains" do
+    [
+      "https://fcm.googleapis.com/fcm/send/token123",
+      "https://jmt17.google.com/fcm/send/token123",
+      "https://updates.push.services.mozilla.com/wpush/v2/token123",
+      "https://web.push.apple.com/QaBC123",
+      "https://wns2-db5p.notify.windows.com/w/?token=abc123"
+    ].each do |endpoint|
+      subscription = build_subscription(endpoint: endpoint)
+      assert subscription.valid?, "Expected #{endpoint} to be valid, got: #{subscription.errors.full_messages}"
+    end
+  end
+
+  private
+    def build_subscription(endpoint:)
+      Push::Subscription.new(user: users(:david), endpoint: endpoint, p256dh_key: "test_key", auth_key: "test_auth")
+    end
+end

test/models/room/push_test.rb

@@ -3,6 +3,10 @@ require "test_helper"
 class Room::PushTest < ActiveSupport::TestCase
   include ActiveJob::TestHelper
 
+  setup do
+    stub_web_push_dns_resolution
+  end
+
   test "deliver new message to other room users with push subscriptions" do
     task_count = Push::Subscription.count - users(:david).push_subscriptions.count
     perform_enqueued_jobs only: Room::PushMessageJob do

test/test_helper.rb

@@ -17,7 +17,7 @@ class ActiveSupport::TestCase
   # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order.
   fixtures :all
 
-  include SessionTestHelper, MentionTestHelper, TurboTestHelper
+  include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper
 
   setup do
     ActionCable.server.pubsub.clear

test/test_helpers/dns_test_helper.rb

@@ -0,0 +1,21 @@
+module DnsTestHelper
+  WEB_PUSH_PUBLIC_TEST_IP = "142.250.185.206" # stable public IP for web push DNS stubs in tests
+
+  private
+    # Surfguard resolves through Resolv.getaddresses, which honours /etc/hosts and
+    # search domains and returns every address a host answers with.
+    def stub_dns_resolution(*ips)
+      Resolv.stubs(:getaddresses).returns(ips.map(&:to_s))
+    end
+
+    # A host that resolves to nothing: the resolver errors (timeout/NXDOMAIN),
+    # which Surfguard catches and reports as Unresolvable, distinct from a host
+    # that resolves only to blocked addresses.
+    def stub_dns_failure(error = Resolv::ResolvError)
+      Resolv.stubs(:getaddresses).raises(error)
+    end
+
+    def stub_web_push_dns_resolution
+      stub_dns_resolution(WEB_PUSH_PUBLIC_TEST_IP)
+    end
+end