Improve client IP resolution for Stream logs #1459

kasparsd · 2023-10-09T12:39:05Z

Fixes #1456.

Rely on $_SERVER['REMOTE_ADDR'] as the canonical source for the request origin IP.
Show a notice in admin if $_SERVER['REMOTE_ADDR'] is not specified.
~~Fallback to $_SERVER['HTTP_X_FORWARDED_FOR'] only if $_SERVER['REMOTE_ADDR'] not present.~~

Checklist

Project documentation has been updated to reflect the changes in this pull request, if applicable.
I have tested the changes in the local development environment (see contributing.md).
I have added phpunit tests.

Release Changelog

Fix: Prefer $_SERVER['REMOTE_ADDR'] for the source of user IP and fallback to $_SERVER['HTTP_X_FORWARDED_FOR'] only in case REMOTE_ADDR is not available. Introduce a wp_stream_client_ip_address filter to adjust the trusted client IP.

kasparsd · 2023-10-09T12:40:01Z

classes/class-admin.php

-		if ( is_admin() && Alerts::POST_TYPE === $screen->post_type ) {
-			return true;
+		if ( is_admin() && function_exists( 'get_current_screen' ) ) {
+			$screen = get_current_screen();


Account for calls to this method when get_current_screen() is not available.

kasparsd · 2023-10-09T12:41:16Z

classes/class-log.php

+
+		// Fallback to unsafe IP extracted from the request HTTP headers.
+		if ( empty( $ip_address ) ) {
+			$ip_address = $this->plugin->get_unsafe_client_ip_address();


Keep the previous behaviour for now. Consider removing this fallback in the future releases (as mentioned in the changelog).

I was wondering why you opted to do two separate calls in here, as opposed to having the fallback be part of get_client_ip_address() instead.

I thought the latter would have been easier for removing the fallback behavior at a later date. Also, it would allow get_unsafe_client_ip_address() to be private instead so we don't create an unwanted public interface for this.

The reason is we want to check just the "reliable" IP address in some instances with no fallbacks (when showing the admin notice, for example). This also clearly identifies the places where the use of get_unsafe_client_ip_address() should be wrapped into a conditional eventually.

kasparsd · 2023-10-09T12:42:13Z

classes/class-log.php

@@ -174,12 +167,6 @@ public function is_record_excluded( $connector, $context, $action, $user = null,
 			$user = wp_get_current_user();
 		}

-		if ( is_null( $ip ) ) {


We expect all of the data arriving here to be sanitized and checked already.

kasparsd · 2023-10-09T12:43:08Z

classes/class-plugin.php

@@ -138,6 +145,9 @@ public function __construct() {
 		// Load logger class.
 		$this->log = apply_filters( 'wp_stream_log_handler', new Log( $this ) );

+		// Set the IP address for the current request.
+		$this->client_ip_address = wp_stream_filter_input( INPUT_SERVER, 'REMOTE_ADDR', FILTER_VALIDATE_IP );


Make the main plugin class responsible for knowing properties of the current request. Expose the knowledge via a helper method.

schlessera · 2023-10-09T15:24:01Z

classes/class-plugin.php

+			'HTTP_X_FORWARDED_FOR',
+			'HTTP_FORWARDED_FOR',


RFC 7239 proposes a new standard 'HTTP_FORWARDED': https://datatracker.ietf.org/doc/html/rfc7239

It is still a comma-separated list, but contains prefixes in front of the IP address and other directives, like so:

Forwarded: for=192.0.2.43,for=198.51.100.17;by=203.0.113.60;proto=http;host=example.com

Not sure it's worthwhile adding support for that upcoming standard as of yet, though, as it doesn't seem to be widely supported => https://c960657.github.io/forwarded.html

Yeah, let's leave this to a separate pull request.

classes/class-admin.php

schlessera · 2023-10-09T15:43:03Z

classes/class-plugin.php

+					$header_client_ip  = $header_client_ips[0];
+				}
+
+				$client_ip = wp_stream_filter_var( trim( $header_client_ip ), FILTER_VALIDATE_IP );


Note for future reference:
The known drawbacks of FILTER_VALIDATE_IP can be ignored here, as it goes through a special polyfill and ends up being implemented via WP_Http::is_ip_address() instead of via PHP's internal filter_var.

readme.txt

schlessera · 2023-10-09T15:44:50Z

readme.txt

+`add_filter(
+    'wp_stream_client_ip_address',
+    function( $client_ip ) {
+        // Trust the X-Forwarded-For header.
+        if ( ! empty( $_SERVER['HTTP_X_FORWARDED_FOR'] ) ) {
+            return $_SERVER['HTTP_X_FORWARDED_FOR'];
+        }
+
+        return $client_ip;
+    }
+);`


Note 100% sure about the readme.txt markdown compatibility, but shouldn't this section use triple backtick notation instead?

schlessera · 2023-10-09T15:58:15Z

classes/class-plugin.php

+	/**
+	 * IP address for the current request to be associated with the log entry.
+	 *
+	 * @var null|false|string


What's the intention behind having three different types here?

From what I can gather in the PR here, we have:

valid IP address gets returned as a string

invalid IP address gets returned as a false

no IP address gets returned as null

As mixed types always offer extra surface for bugs, just covering the bases here:

Are we distinguishing between these three types in other parts of the code? I.e., does an invalid IP address trigger anything differently than a missing IP address?

Are these three types properly stored in the database?

Are these three types properly rendered in all UI dialogs and reports?

This is just mirroring the output types of filter_input(). This is the canonical state of the REMOTE_ADDR value and the methods can use these values as needed.

classes/class-log.php

schlessera · 2023-10-09T16:03:59Z

classes/class-log.php

+		$ip_address = $this->plugin->get_client_ip_address();
+
+		// Fallback to unsafe IP extracted from the request HTTP headers.
+		if ( empty( $ip_address ) ) {


The empty( $ip_address ) will be true for both the case where no IP address was provided as well for when an invalid IP address is provided. Is that the intended behavior?

It is a bit tricky here -- we're attempting to preserve the HTTP_X_FORWARDED_FOR usage from before but now it will only happen if REMOTE_ADDR is not configured which might not be the case making this an equally breaking change.

I'm thinking we should probably do a breaking change release and make it consistent immediately to avoid confusion.

schlessera · 2023-10-09T16:08:46Z

classes/class-log.php

+
+		// Fallback to unsafe IP extracted from the request HTTP headers.
+		if ( empty( $ip_address ) ) {
+			$ip_address = $this->plugin->get_unsafe_client_ip_address();


I was wondering why you opted to do two separate calls in here, as opposed to having the fallback be part of get_client_ip_address() instead.

I thought the latter would have been easier for removing the fallback behavior at a later date. Also, it would allow get_unsafe_client_ip_address() to be private instead so we don't create an unwanted public interface for this.

schlessera · 2023-10-09T16:10:06Z

classes/class-plugin.php

+	 *
+	 * @return string|null
+	 */
+	public function get_unsafe_client_ip_address() {


As mentioned elsewhere, if this fallback behavior would be called immediately while initially setting $this->client_ip_address, we wouldn't need to make it public and not have it part of the public interface.

kasparsd · 2023-10-16T07:57:55Z

@schlessera I feel like attempting the fallback behaviour might not be in our best interest here because it would actually be different from the previous behaviour. Let's make it a breaking change instead (does this warrant a 4.0.0 release?) and encourage everyone to use the newly introduced filter for adjusting the client IP address on environments where either REMOTE_ADDR is reporting wrong.

calvinalkan · 2023-10-16T14:51:54Z

@schlessera I feel like attempting the fallback behaviour might not be in our best interest here because it would actually be different from the previous behaviour. Let's make it a breaking change instead (does this warrant a 4.0.0 release?) and encourage everyone to use the newly introduced filter for adjusting the client IP address on environments where either REMOTE_ADDR is reporting wrong.

My 2C.

The fallback behaviour is insecure. People wil shoot themselves in the foot.
If PHP can't trust it's server to set REMOTE_ADDR correctly, or at all, all hope is lost. This should not be a consideration.
Yes, breaking change, 4.0.0

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

kasparsd commented Oct 9, 2023

View reviewed changes

kasparsd requested a review from schlessera October 9, 2023 13:56

schlessera suggested changes Oct 9, 2023

View reviewed changes

schlessera force-pushed the add/event-source-ip branch 2 times, most recently from 178177e to b42c4c0 Compare October 16, 2023 16:46

kasparsd and others added 23 commits October 16, 2023 18:55

Make the plugin responsible for resolving request IPs

4d086cc

Make the logger rely on the IP resolved by the plugin

868957b

Add a notice to plugin settings in case request IP can't be determined

970121b

Document the IP resolver logic

5578066

Add an example

54150d6

Limit the notice to the Stream admin pages

2a00ddf

Ensure we fail gracefully when the WP core helper is not available

c637256

Fix docblock syntax

fb0f093

Make it accessible

c10f996

Add basic tests

d18e872

Test for invalid IP

3d11efd

Use the local helper for consistency

d9fa985

Update readme.txt

4afe6ec

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

Apply suggestions from code review

f0a5e20

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

Apply suggestions from code review

a136099

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

Skip any formatting for simplicity

560baf1

Describe the output types

94e17bd

Account for multiple IPs in the forwarded header

9daaaa8

Show the notice if client IP missing

1b9068d

Don’t even attempt to use the unsafe option

38683c1

We no longer default to a fallback

f6c799b

Add the changelog

85bdc4d

Add noticeable warning regarding HTTP_* spoofing

9bcc490

schlessera force-pushed the add/event-source-ip branch from b42c4c0 to 9bcc490 Compare October 16, 2023 16:56

schlessera approved these changes Oct 24, 2023

View reviewed changes

schlessera merged commit 7fcbb08 into develop Oct 24, 2023
2 of 3 checks passed

schlessera deleted the add/event-source-ip branch October 24, 2023 12:23

schlessera added this to the 4.0.0 milestone Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve client IP resolution for Stream logs #1459

Improve client IP resolution for Stream logs #1459

kasparsd commented Oct 9, 2023 •

edited by schlessera

kasparsd Oct 9, 2023

kasparsd Oct 9, 2023

schlessera Oct 9, 2023

kasparsd Oct 16, 2023

kasparsd Oct 9, 2023

kasparsd Oct 9, 2023

schlessera Oct 9, 2023

kasparsd Oct 16, 2023

schlessera Oct 9, 2023

schlessera Oct 9, 2023

schlessera Oct 9, 2023

kasparsd Oct 16, 2023

schlessera Oct 9, 2023

kasparsd Oct 16, 2023

schlessera Oct 9, 2023

schlessera Oct 9, 2023

kasparsd commented Oct 16, 2023

calvinalkan commented Oct 16, 2023

Improve client IP resolution for Stream logs #1459

Improve client IP resolution for Stream logs #1459

Conversation

kasparsd commented Oct 9, 2023 • edited by schlessera

Checklist

Release Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasparsd commented Oct 16, 2023

calvinalkan commented Oct 16, 2023

kasparsd commented Oct 9, 2023 •

edited by schlessera