Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve client IP resolution for Stream logs #1459

Merged
merged 23 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
4d086cc
Make the plugin responsible for resolving request IPs
kasparsd Oct 9, 2023
868957b
Make the logger rely on the IP resolved by the plugin
kasparsd Oct 9, 2023
970121b
Add a notice to plugin settings in case request IP can't be determined
kasparsd Oct 9, 2023
5578066
Document the IP resolver logic
kasparsd Oct 9, 2023
54150d6
Add an example
kasparsd Oct 9, 2023
2a00ddf
Limit the notice to the Stream admin pages
kasparsd Oct 9, 2023
c637256
Ensure we fail gracefully when the WP core helper is not available
kasparsd Oct 9, 2023
fb0f093
Fix docblock syntax
kasparsd Oct 9, 2023
c10f996
Make it accessible
kasparsd Oct 9, 2023
d18e872
Add basic tests
kasparsd Oct 9, 2023
3d11efd
Test for invalid IP
kasparsd Oct 9, 2023
d9fa985
Use the local helper for consistency
kasparsd Oct 9, 2023
4afe6ec
Update readme.txt
kasparsd Oct 9, 2023
f0a5e20
Apply suggestions from code review
kasparsd Oct 16, 2023
a136099
Apply suggestions from code review
kasparsd Oct 16, 2023
560baf1
Skip any formatting for simplicity
kasparsd Oct 16, 2023
94e17bd
Describe the output types
kasparsd Oct 16, 2023
9daaaa8
Account for multiple IPs in the forwarded header
kasparsd Oct 16, 2023
1b9068d
Show the notice if client IP missing
kasparsd Oct 16, 2023
38683c1
Don’t even attempt to use the unsafe option
kasparsd Oct 16, 2023
f6c799b
We no longer default to a fallback
kasparsd Oct 16, 2023
85bdc4d
Add the changelog
kasparsd Oct 16, 2023
9bcc490
Add noticeable warning regarding HTTP_* spoofing
schlessera Oct 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions classes/class-admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,12 @@ public function init() {
$this->network = new Network( $this->plugin );
$this->live_update = new Live_Update( $this->plugin );
$this->export = new Export( $this->plugin );

// Check if the host has configured the `REMOTE_ADDR` correctly.
$client_ip = $this->plugin->get_client_ip_address();
if ( empty( $client_ip ) && $this->is_stream_screen() ) {
$this->notice( __( 'Stream plugin can\'t determine a reliable client IP address! Please update the hosting environment to set the $_SERVER[\'REMOTE_ADDR\'] variable or use the wp_stream_client_ip_address filter to specify the verified client IP address!', 'stream' ) );
}
}

/**
Expand Down Expand Up @@ -541,9 +547,10 @@ public function is_stream_screen() {
return true;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return ( Alerts::POST_TYPE === $screen->post_type );
}

return false;
Expand Down
26 changes: 4 additions & 22 deletions classes/class-log.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,6 @@ class Log {
*/
public $plugin;

/**
* Hold Current visitors IP Address.
*
* @var string
*/
private $ip_address;


/**
* Previous Stream record ID, used for chaining same-session records
*
Expand All @@ -42,12 +34,6 @@ class Log {
public function __construct( $plugin ) {
$this->plugin = $plugin;

// Support proxy mode by checking the `X-Forwarded-For` header first.
$ip_address = wp_stream_filter_input( INPUT_SERVER, 'HTTP_X_FORWARDED_FOR', FILTER_VALIDATE_IP );
$ip_address = $ip_address ? $ip_address : wp_stream_filter_input( INPUT_SERVER, 'REMOTE_ADDR', FILTER_VALIDATE_IP );

$this->ip_address = $ip_address;

// Ensure function used in various methods is pre-loaded.
if ( ! function_exists( 'is_plugin_active_for_network' ) ) {
require_once ABSPATH . '/wp-admin/includes/plugin.php';
Expand Down Expand Up @@ -87,9 +73,11 @@ public function log( $connector, $message, $args, $object_id, $context, $action,
return false;
}

$ip_address = $this->plugin->get_client_ip_address();

$user = new \WP_User( $user_id );

if ( $this->is_record_excluded( $connector, $context, $action, $user ) ) {
if ( $this->is_record_excluded( $connector, $context, $action, $user, $ip_address ) ) {
return false;
}

Expand Down Expand Up @@ -140,7 +128,7 @@ function ( $var ) {
'connector' => (string) $connector,
'context' => (string) $context,
'action' => (string) $action,
'ip' => (string) $this->ip_address,
'ip' => (string) $ip_address,
schlessera marked this conversation as resolved.
Show resolved Hide resolved
'meta' => (array) $stream_meta,
);

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

if ( is_null( $ip ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$ip = $this->ip_address;
} else {
$ip = wp_stream_filter_var( $ip, FILTER_VALIDATE_IP );
}

if ( ! empty( $user->roles ) ) {
$roles = array_values( $user->roles );
$role = $roles[0];
Expand Down
19 changes: 19 additions & 0 deletions classes/class-plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ class Plugin {
*/
public $locations = array();

/**
* IP address for the current request to be associated with the log entry.
*
* @var null|false|string Valid IP address, null if not set, false if invalid.
*/
protected $client_ip_address;

/**
* Class constructor
*/
Expand Down Expand Up @@ -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 );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Load settings and connectors after widgets_init and before the default init priority.
add_action( 'init', array( $this, 'init' ), 9 );

Expand Down Expand Up @@ -315,4 +325,13 @@ public function is_mustuse() {

return false;
}

/**
* Get the IP address for the current request.
*
* @return false|null|string Valid IP address, null if not set, false if invalid.
*/
public function get_client_ip_address() {
return apply_filters( 'wp_stream_client_ip_address', $this->client_ip_address );
}
}
2 changes: 1 addition & 1 deletion includes/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* @param int $filter The ID of the filter to apply.
* @param mixed $options Associative array of options or bitwise disjunction of flags. If filter accepts options, flags can be provided in "flags" field of array.
*
* @return Value of the requested variable on success, FALSE if the filter fails, or NULL if the $variable_name is not set.
* @return mixed|false|null Value of the requested variable on success, FALSE if the filter fails, or NULL if the $variable_name is not set.
*/
function wp_stream_filter_input( $type, $variable_name, $filter = null, $options = array() ) {
return call_user_func_array( array( '\WP_Stream\Filter_Input', 'super' ), func_get_args() );
Expand Down
39 changes: 37 additions & 2 deletions readme.txt
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,43 @@ With Stream’s powerful logging, you’ll have the valuable information you nee
* WP-CLI command for querying records


= Known Issues
== Configuration ==

Most of the plugin configuration is available under the "Stream" → "Settings" page in the WordPress dashboard.


= Request IP Address =

The plugin expects the `$_SERVER['REMOTE_ADDR']` variable to contain the verified IP address of the current request. On hosting environments with PHP processing behind reverse proxies or CDNs the actual client IP is passed to PHP through request HTTP headers such as `X-Forwarded-For` and `True-Client-IP` which can't be trusted without an additional layer of validation. Update your server configuration to set the `$_SERVER['REMOTE_ADDR']` variable to the verified client IP address.

As a workaround, you can use the `wp_stream_client_ip_address` filter to adapt the IP address:

`add_filter(
'wp_stream_client_ip_address',
function( $client_ip ) {
// Trust the first IP in the X-Forwarded-For header.
// ⚠️ Note: This is inherently insecure and can easily be spoofed!
if ( ! empty( $_SERVER['HTTP_X_FORWARDED_FOR'] ) ) {
$forwarded_ips = explode( ',' $_SERVER['HTTP_X_FORWARDED_FOR'] );

if ( filter_var( $forwarded_ips[0], FILTER_VALIDATE_IP ) ) {
return $forwarded_ips[0];
}
}

return $client_ip;
}
);`

⚠️ **WARNING:** The above is an insecure workaround that you should only use when you fully understand what this implies. Relying on any variable with the `HTTP_*` prefix is prone to spoofing and cannot be trusted!


== Known Issues ==

* We have temporarily disabled the data removal feature through plugin uninstallation, starting with version 3.9.3. We identified a few edge cases that did not behave as expected and we decided that a temporary removal is preferable at this time for such an impactful and irreversible operation. Our team is actively working on refining this feature to ensure it performs optimally and securely. We plan to reintroduce it in a future update with enhanced safeguards.


= Contribute =
== Contribute ==

There are several ways you can get involved to help make Stream better:

Expand Down Expand Up @@ -103,6 +134,10 @@ Track changes to posts when using the block editor.

== Changelog ==

= NEXT =

- Breaking: Use only `$_SERVER['REMOTE_ADDR']` as the reliable client IP address for event logs. This might cause incorrectly reported event log IP addresses on environments where PHP is behind a proxy server or CDN. Use the `wp_stream_client_ip_address` filter to set the correct client IP address (see `readme.txt` for instructions) or configure the hosting environment to report the correct IP address in `$_SERVER['REMOTE_ADDR']`.

= 3.10.0 - October 9, 2023 =

- Fix: Improve PHP 8.1 compatibility by updating `filter_*()` calls referencing `FILTER_SANITIZE_STRING` (issue [#1422](https://github.com/xwp/stream/pull/1422)).
Expand Down
4 changes: 4 additions & 0 deletions tests/tests/test-class-plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,8 @@ public function test_get_version() {
$version = $this->plugin->get_version();
$this->assertNotEmpty( $version );
}

public function test_get_client_ip_address() {
$this->assertEquals( $_SERVER['REMOTE_ADDR'], $this->plugin->get_client_ip_address() );
}
}
Loading