Status: validate the visitor IP address - #51349
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Package] Status
[Status] In Progress
[Tests] Includes Tests
labels
|
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! Beta plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. 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. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Inspect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Classic Theme helper plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Paypal Payment buttons plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcloud Sso plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Agents Manager plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Stats Data plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
Bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 1 file.
|
filter_var() with no filter applies FILTER_DEFAULT, which does not sanitize. REMOTE_ADDR is now validated as an IP address, and the forwarded headers, which may hold lists, are sanitized as text.
a5efbac to
1b5ff1f
Compare
Nikschavan
added
[Status] Needs Review
There was a problem hiding this comment.
Pull request overview
This PR hardens visitor IP handling in the automattic/jetpack-status package by properly validating REMOTE_ADDR as an IP address and sanitizing forwarded header values as plain text (to allow legitimate comma-separated lists), addressing plugin review-tooling concerns.
Changes:
- Sanitize forwarded IP-related headers using
sanitize_text_field()instead offilter_var()default behavior. - Validate
$_SERVER['REMOTE_ADDR']withFILTER_VALIDATE_IP, returning an empty string when invalid. - Extend the PHPUnit bootstrap with a
sanitize_text_field()polyfill and add tests covering the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/packages/status/tests/php/Visitor_Test.php | Adds new unit tests for Visitor::get_ip() validation/sanitization behavior. |
| projects/packages/status/tests/php/bootstrap.php | Adds a sanitize_text_field() workalike for the test environment. |
| projects/packages/status/src/class-visitor.php | Updates get_ip() to validate REMOTE_ADDR and sanitize forwarded headers. |
| projects/packages/status/changelog/stats-343-status-sanitize-visitor-ip | Adds a changelog entry describing the behavioral change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A header that does not hold a single address now falls through to the next one, so a junk value in an earlier header cannot hide a valid address in a later one.
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
What's holding me up is the forwarded-header fall-through. A comma-separated X-Forwarded-For is the normal shape on any site behind a CDN or load balancer, and it now resolves to REMOTE_ADDR — the proxy — rather than the visitor. That's a live behavior change for self-hosted sites, it reaches WordPress.com through roughly twenty call sites, and it lands in every plugin bundling jetpack-status on the next release with no filter or option to opt out of it.
You flagged this yourself and said it needs its own change. I'd push back a little: the in-between state is worse than either end. Before, WordPress.com received a list it could parse. After, it receives a confidently wrong single address with no signal anything was dropped. IP_Utils::clean_ip() already handles the list, ports, brackets, and ::ffff: forms, so doing it here is small.
Two housekeeping items that apply either way: the plugins shipping this package need their own changelog entries (the package CHANGELOG never reaches users), and the description still describes the sanitize_text_field approach that 6b86c47 replaced.
I can push these myself if that's easier — the list handling, the plugin changelogs, and the description. Say the word and you can review the diff instead of writing it.
| // @todo Some of these might actually be lists of IPs (e.g. HTTP_X_FORWARDED_FOR) or something else entirely (HTTP_VIA). | ||
| return filter_var( wp_unslash( $_SERVER[ $key ] ) ); | ||
| // @todo Some of these might actually be lists of IPs (e.g. HTTP_X_FORWARDED_FOR) or something else entirely (HTTP_VIA). Those fail validation and fall through to the next header. | ||
| $ip = filter_var( wp_unslash( $_SERVER[ $key ] ), FILTER_VALIDATE_IP ); |
There was a problem hiding this comment.
The realistic proxy case regresses here. Before this PR a comma-separated X-Forwarded-For was forwarded verbatim; now it fails validation, falls through the remaining headers, and lands on REMOTE_ADDR. On a self-hosted site behind its own proxy chain that's the load balancer, not the visitor, and it ships that way to WordPress.com from about twenty call sites — connection's proxy trait, stats, forms, scan, licensing, activity-log, plans, my-jetpack, and a dozen spots in core-rest-api-endpoints.
Automattic\Jetpack\IP\Utils::clean_ip() already does this work: projects/packages/ip/src/class-utils.php:59 handles the comma list, " unless " suffixes, IPv4:port, bracketed IPv6, and ::ffff: mapping, then validates. Minimal version without taking on the dependency:
foreach ( explode( ',', (string) wp_unslash( $_SERVER[ $key ] ) ) as $candidate ) {
$ip = filter_var( trim( $candidate ), FILTER_VALIDATE_IP );
if ( false !== $ip ) {
return $ip;
}
}Leftmost-wins matches the order proxies append in. It's client-supplied and spoofable, but so was the entire raw header before this change, so it isn't a new trust boundary.
There was a problem hiding this comment.
Taken the clean_ip() route rather than the snippet, since every plugin that ships status already bundles ip except Beta, and Modules::activate() has called IP_Utils::get_ip() since 2023 — so this just makes an existing runtime dependency explicit. The loop still splits on commas here, as clean_ip() only handles one entry; leftmost valid wins. Pushed in 54c60de.
| return filter_var( wp_unslash( $_SERVER[ $key ] ) ); | ||
| // @todo Some of these might actually be lists of IPs (e.g. HTTP_X_FORWARDED_FOR) or something else entirely (HTTP_VIA). Those fail validation and fall through to the next header. | ||
| $ip = filter_var( wp_unslash( $_SERVER[ $key ] ), FILTER_VALIDATE_IP ); | ||
| if ( false !== $ip ) { |
There was a problem hiding this comment.
Beyond the list case, a few single-address shapes that show up in the wild fail FILTER_VALIDATE_IP and now fall through: a port suffix (5.6.7.8:41234, which Azure App Service emits), bracketed IPv6 ([2001:db8::1]:443), ::ffff:-mapped IPv4, and plain leading whitespace.
Worth noting Jetpack now has two IP resolvers that disagree about the same request — IP_Utils::get_ip() normalizes all of those, this one rejects them. Routing through clean_ip() here would keep them from drifting further.
There was a problem hiding this comment.
Covered by the same change — each candidate and REMOTE_ADDR go through clean_ip(). Tests for all three shapes plus leading whitespace. The two resolvers now agree on single-address parsing; what they still differ on is which headers they read, and that is a bigger question than this PR.
There was a problem hiding this comment.
This did increase the size of the PR due to bunch of composer.lock changes - all plugins already were shipping this automattic/jetpack-ip dependency though - this change does not introduce a new dependency
github-actions
Bot
added
[Plugin] Backup
Please feel free to push to the PR if you have any suggestions 👍🏼 |
Proposed changes
filter_var()with no filter argument appliesFILTER_DEFAULT, which neither sanitizes nor validates.Visitor::get_ip()used it in both of its return paths, so whatever a header held was returned as-is.Automattic\Jetpack\IP\Utils::clean_ip(), which strips a port suffix, IPv6 brackets and the::ffff:IPv4 mapping and then validates. A value that does not resolve to an address yields an empty string.automattic/jetpack-ipmoves fromrequire-devtorequirein the status package.Modules::activate()has calledIP_Utils::get_ip()since 2023, so this makes an existing runtime dependency explicit; every plugin shipping status already bundlesipexcept Beta, which now does.minor, as the value existing callers receive changes.composer.lockdiffs are bookkeeping: each lock re-records thestatuspackage'srequirelist. Only Beta's lock adds a package.Flagged by the WordPress.org plugin review tooling while the standalone Jetpack Stats plugin was under review.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No new data is collected. Two behaviour changes for callers that record the visitor IP:
Testing instructions
( new Visitor() )->get_ip()still returns the expected address.$_SERVER['REMOTE_ADDR']to a value that is not an IP address and confirm the method returns an empty string; set it to1.2.3.4:8080and confirm it returns1.2.3.4.$_SERVER['HTTP_X_FORWARDED_FOR']to5.6.7.8, 9.10.11.12and callget_ip( true ); confirm it returns5.6.7.8.jp test php packages/status.