mirror of
https://github.com/basecamp/once-campfire.git
synced 2026-09-13 20:12:06 +09:00
Jeremy Daer
94a48aacb6
* 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.
84 lines
3.1 KiB
Ruby
84 lines
3.1 KiB
Ruby
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
|
|
WebPush.expects(:payload_send).times(task_count)
|
|
rooms(:hq).messages.create! body: "This is from earth", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(task_count)
|
|
end
|
|
|
|
test "notifies subscribed users" do
|
|
perform_enqueued_jobs only: Room::PushMessageJob do
|
|
WebPush.expects(:payload_send).times(2)
|
|
rooms(:designers).messages.create! body: "This is from earth", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(2)
|
|
|
|
perform_enqueued_jobs only: Room::PushMessageJob do
|
|
WebPush.expects(:payload_send).times(3)
|
|
rooms(:designers).messages.create! body: "Hey #{mention_attachment_for(:kevin)}", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(5)
|
|
end
|
|
|
|
test "does not notify for connected rooms" do
|
|
memberships(:kevin_designers).connected
|
|
|
|
perform_enqueued_jobs only: Room::PushMessageJob do
|
|
WebPush.expects(:payload_send).times(2)
|
|
rooms(:designers).messages.create! body: "Hey @kevin", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(2)
|
|
end
|
|
|
|
test "does not notify for invisible rooms" do
|
|
memberships(:kevin_designers).update! involvement: "invisible"
|
|
|
|
perform_enqueued_jobs only: Room::PushMessageJob do
|
|
WebPush.expects(:payload_send).times(2)
|
|
rooms(:designers).messages.create! body: "Hey @kevin", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(2)
|
|
end
|
|
|
|
test "destroys invalid subscriptions" do
|
|
memberships(:kevin_designers).update! involvement: "invisible"
|
|
|
|
assert_difference -> { Push::Subscription.count }, -2 do
|
|
perform_enqueued_jobs only: Room::PushMessageJob do
|
|
WebPush.expects(:payload_send).times(2).raises(WebPush::ExpiredSubscription.new(Struct.new(:body).new, "example.com"))
|
|
rooms(:designers).messages.create! body: "Hey @kevin", client_message_id: "earth", creator: users(:david)
|
|
end
|
|
wait_for_web_push_delivery_pool_tasks(2)
|
|
wait_for_invalidation_pool_tasks(2)
|
|
end
|
|
end
|
|
|
|
private
|
|
def wait_for_web_push_delivery_pool_tasks(count)
|
|
wait_for_pool_tasks(Rails.configuration.x.web_push_pool.delivery_pool, count)
|
|
end
|
|
|
|
def wait_for_invalidation_pool_tasks(count)
|
|
wait_for_pool_tasks(Rails.configuration.x.web_push_pool.invalidation_pool, count)
|
|
end
|
|
|
|
def wait_for_pool_tasks(pool, count)
|
|
start = Time.now
|
|
timeout = 0.2
|
|
while pool.completed_task_count < count
|
|
raise "Timeout waiting for pool tasks to complete" if Time.now - start > timeout
|
|
sleep timeout / 10.0
|
|
end
|
|
end
|
|
end
|