diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index bf5e5f8..8e263b2 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/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 diff --git a/app/models/push/subscription.rb b/app/models/push/subscription.rb index f972941..bd78482 100644 --- a/app/models/push/subscription.rb +++ b/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 diff --git a/config/initializers/web_push.rb b/config/initializers/web_push.rb index ac96046..f0a28fa 100644 --- a/config/initializers/web_push.rb +++ b/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) diff --git a/lib/web_push/notification.rb b/lib/web_push/notification.rb index 2ad156c..932d2e2 100644 --- a/lib/web_push/notification.rb +++ b/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 diff --git a/lib/web_push/pool.rb b/lib/web_push/pool.rb index 2a0533a..aeccea5 100644 --- a/lib/web_push/pool.rb +++ b/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 diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index 0b90513..e69fc4a 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/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)) diff --git a/test/lib/web_push/persistent_request_test.rb b/test/lib/web_push/persistent_request_test.rb new file mode 100644 index 0000000..d570548 --- /dev/null +++ b/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 diff --git a/test/models/push/subscription_test.rb b/test/models/push/subscription_test.rb new file mode 100644 index 0000000..6dce213 --- /dev/null +++ b/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 diff --git a/test/models/room/push_test.rb b/test/models/room/push_test.rb index 7c5c590..8ecfdc6 100644 --- a/test/models/room/push_test.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index 9c1310c..5c4ac31 100644 --- a/test/test_helper.rb +++ b/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 diff --git a/test/test_helpers/dns_test_helper.rb b/test/test_helpers/dns_test_helper.rb new file mode 100644 index 0000000..86aa578 --- /dev/null +++ b/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