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
Changes from 1 commit
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
Next Next commit
Make the plugin responsible for resolving request IPs
kasparsd authored and schlessera committed Oct 16, 2023
commit 4d086cc4302bfdfc63f0ff4e28aff66d9d8bc3c5
54 changes: 54 additions & 0 deletions classes/class-plugin.php
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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?
  2. Are these three types properly stored in the database?
  3. Are these three types properly rendered in all UI dialogs and reports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

*/
protected $client_ip_address;

/**
* Class constructor
*/
@@ -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 );

@@ -315,4 +325,48 @@ public function is_mustuse() {

return false;
}

/**
* Get the IP address for the current request.
*
* @return false|null|string
*/
public function get_client_ip_address() {
return apply_filters( 'wp_stream_client_ip_address', $this->client_ip_address );
}

/**
* Get the client IP address from the HTTP request headers.
*
* There is no guarantee that this is the real IP address of the client.
*
* @return string|null
*/
protected function get_unsafe_client_ip_address() {
// List of $_SERVER keys that could contain the client IP address.
$address_headers = array(
'HTTP_X_FORWARDED_FOR',
'HTTP_FORWARDED_FOR',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

);

foreach ( $address_headers as $header ) {
if ( ! empty( $_SERVER[ $header ] ) ) {
$header_client_ip = $_SERVER[ $header ];

// Account for multiple IPs in case of multiple proxies.
if ( false !== strpos( $header_client_ip, ',' ) ) {
$header_client_ips = explode( ',', $header_client_ip );
$header_client_ip = $header_client_ips[0];
}

$client_ip = filter_var( trim( $header_client_ip ), FILTER_VALIDATE_IP );

if ( ! empty( $client_ip ) ) {
return $client_ip;
}
}
}

return null;
}
}