Forms: harden webhook URL validation — loopback, redirects, and an escape hatch - #52103
Forms: harden webhook URL validation — loopback, redirects, and an escape hatch#52103kraftbj wants to merge 3 commits into
Conversation
is_blocked_ip() covers ::1 but not 127.0.0.0/8, so a webhook pointed at
localhost was only ever blocked when the host resolver returned an AAAA
record for it. glibc does; macOS does not, which is why
test_send_webhooks_blocks_localhost_hostname passes in CI and fails on a
Mac. Same commit, same tree, different resolver.
Block 127.0.0.0/8 and 0.0.0.0/8 alongside the IPv6 checks already there.
That also revives the IPv4-mapped path, which was dead for loopback:
::ffff:127.0.0.1 recursed into is_blocked_ip('127.0.0.1') and got false.
Tighten the affected tests to assert webhook_skipped/blocked_ip and no
HTTP request, rather than a three-way || that passed on almost anything.
One of them had no pre_http_request stub at all and was making a real
network call.
Fixes FORMS-786
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
Blocking 127.0.0.0/8 is observable after all. wp_http_validate_url() keeps its whole IP check behind `if ( ! $same_host )`, so a site whose home host resolves to loopback could webhook to itself and core never looked. Add the plugins/jetpack changelog entry that behavior change earns, and stop claiming in the docblock that IPv4 private ranges are blocked when RFC1918 deliberately is not. Also block :: alongside ::1. The unspecified address, not ::1, is the IPv6 counterpart of the 0.0.0.0/8 rule added here, and connect() to :: reaches loopback. Switch the link-local check from magic long bounds to octet math. On 32-bit PHP ip2long() goes negative above 127.255.255.255, so the old `>= 2851995648` comparison never fired there and 169.254.0.0/16 was silently unblocked. Move the Azure literal above the ip2long() guard so it stays reachable regardless. Tests for 0.0.0.0/8 and :: — both were shipped unblocked and untested.
…p following redirects Four things the review turned up, all in the webhook SSRF path. Forms validates ahead of wp_safe_remote_request(), so core's own http_request_host_is_external escape hatch is never reached and a site that deliberately points a webhook at its own network has no recourse. Add jetpack_forms_webhook_blocked_ip as the equivalent. A webhook rejected at validation was skipped in get_enabled_webhooks() and left nothing behind, while a request-time failure wrote post meta and bumped the stat. Record validation failures the same way, so a misconfigured URL stays diagnosable. Stop following redirects. Core re-validates each hop, but it never blocks the Azure wire server and still permits http, so a 302 escaped the stricter checks we just ran -- and browser_redirect_compatibility turns a redirected POST into a bodyless GET regardless. Decode the IPv6 spellings that embed an IPv4 address before checking it. 64:ff9b::7f00:1 (NAT64) and 2002:7f00:1::1 (6to4) both reach 127.0.0.1 and both were allowed, which makes them loopback bypasses in a change whose whole point is blocking loopback. packages/ip already does this; see the PR for why the wholesale swap is a separate decision.
Fixes FORMS-786
Proposed changes
Started as one failing test and grew into the SSRF path around it.
Form_Webhooks::is_blocked_ip()blocked IPv6 loopback (::1), link-local, and ULA, but its IPv4 branch only covered169.254.0.0/16and the Azure wire server. Nothing blocked127.0.0.0/8.So
https://localhost/webhookcleared validation on thegethostbyname()result and was only ever stopped by the AAAA lookup further down. glibc answers::1forlocalhost; macOS returns nothing for it over DNS. That is whytest_send_webhooks_blocks_localhost_hostnameis green in CI and red on a Mac — same commit, same tree, different resolver.Blocking
127.0.0.0/8and0.0.0.0/8, plus::alongside::1.::rather than::1is the real IPv6 counterpart of0.0.0.0, andconnect()to it reaches loopback.64:ff9b::7f00:1(NAT64) and2002:7f00:1::1(6to4) both reach127.0.0.1and both were allowed — loopback bypasses in a change whose whole point is blocking loopback. The existing::ffff:handling was also dead for loopback, since it recursed intois_blocked_ip( '127.0.0.1' )and gotfalse.ip2long()returns a negative int above127.255.255.255, so>= 2851995648never fired there and169.254.0.0/16was silently unblocked. The Azure literal moved above theip2long()guard so it stays reachable.Don't follow redirects
Core re-validates each hop via
requests.before_redirect, butwp_http_validate_url()does not block168.63.129.16(the Azure wire server) anywhere and still permits plainhttp, so a 302 escaped the stricter checks we had just run on the configured URL. Core's ownbrowser_redirect_compatibilityalso turns a redirected POST into a bodyless GET, so following one was not delivering the webhook anyway.Give sites an escape hatch
Because Forms validates ahead of
wp_safe_remote_request(), core'shttp_request_host_is_externalfilter is never reached — a site that deliberately points a webhook at its own network had no recourse. New filter:Keep it diagnosable
A webhook rejected at validation was skipped in
get_enabled_webhooks()and left nothing behind, while a request-time failure wrote_jetpack_forms_webhook_errorand bumped the request stat. Validation failures now record the same way.Tests
Four existing tests asserted almost nothing —
test_send_webhooks_blocks_zero_padded_iphad nopre_http_requeststub at all, so it made a real network call and passed on the connection failure. Tightened those, and added coverage for0.0.0.0/8,::, the NAT64/6to4 spellings, the new filter, the error trail, and the redirect behavior.This is a behavior change, narrowly
An earlier attempt at this fix (#46526) was closed on the grounds that
wp_http_validate_url()already blocks127/8,10/8,172.16/12and192.168/16, making the addition redundant. True except for one case that review missed: core keeps its entire IP check insideif ( ! $same_host ). When the webhook host matches the site's ownhomehost, core does no IP filtering at all.So a site whose
homehost resolves to loopback, with a webhook pointed at that same host, previously fired and now does not — hence theplugins/jetpackchangelog entries, and the new filter for anyone who wants the old behavior back.On consolidating with
packages/ipAutomattic\Jetpack\IP\Utils::ip_is_public()already implements a superset of this logic, andautomattic/jetpack-ipis already in the Forms vendor tree transitively (via connection, status and sync). Delegating to it is tempting, and I measured what it would change across 32 representative addresses. It never unblocks anything, and after this PR it would newly block 11 more classes:10/8,172.16/12,192.168/16, and their::ffff:forms100.64/10(Alibaba metadata),192.0.0/24,192.88.99/24,198.18/15, multicast,240/4, broadcastThat is the RFC1918 decision below, plus six more ranges, arriving as a side effect of a refactor. Worth doing, but as its own change where the blast radius is the point rather than a footnote — tracked in FORMS-787. This PR takes the part that is unambiguously in scope, the embedded-IPv4 loopback decoding, and leaves the rest.
Deliberately out of scope
is_blocked_ip()still does not block RFC1918 at validation time, even though it blocks the IPv6 equivalentfc00::/7. Same-host aside, core covers those at request time, and blocking them here would newly break a local-dev site webhooking to its own private-IP host. That wants its own decision.Related product discussion/links
packages/ip: https://linear.app/a8c/issue/FORMS-787Does this pull request change what data or activity we track or use?
The
jetpack_forms_webhook_requeststat now counts validation-time rejections as errors, where before it only counted request-time failures. No new data is collected.Testing instructions
This one is platform-dependent, so run it on both if you can.
jetpack test php packages/forms. Natively, that iscomposer installintools/php-test-envand inprojects/packages/forms, thencomposer phpunitfrom the package.Form_Webhooks_Testcases should pass. On trunk,test_send_webhooks_blocks_localhost_hostnamefails on macOS and passes on Linux.https://localhost/webhook, submit an entry, and confirm nothing goes out and the entry records a webhook error. Then add thejetpack_forms_webhook_blocked_ipfilter above returningfalse, submit again, and confirm the request is attempted.