Feature/harden proxy api#1
Open
crweiner wants to merge 7 commits into
Open
Conversation
Member
Author
|
@copilot, please review this PR |
… in Proxy.php and URL length check in MU plugin Agent-Logs-Url: https://github.com/crweiner/plausible-wordpress-plugin/sessions/b35d7e5f-5764-4ad1-9b52-a62192c6ac6b Co-authored-by: crweiner <23106097+crweiner@users.noreply.github.com>
I reviewed the PR and found two inconsistencies between the
Beyond those two fixes, the rest of the implementation looks solid:
One pre-existing note (not introduced by this PR): Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…es return a 404 so you cant assume an endpoint exists based on response
Added hardening to the host matching and also to ensure that all rout…
WalkthroughChanges modify mu-plugin/plausible-proxy-speed-module.php and src/Proxy.php to harden and conceal the proxy REST endpoint. A new MAX_REQUEST_BYTES (8192) is added. Request-path detection now uses parsed path prefix checks. Added request short-circuiting that returns a uniform 404-style 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
mu-plugin/plausible-proxy-speed-module.php (2)
107-135: Short-circuit sequence relies on a non-obviousexitinside the helper.Each check calls
send_rest_no_route()which only terminates becausesend_json_error()ends inexit. If that helper is ever refactored toreturnor throw, the subsequent checks below would continue executing and a subsequentheader()/echowould trip "headers already sent" / double output. Returning after the call (or converting toelseif) makes the contract explicit and robust.♻️ Proposed fix
- if ( $this->is_namespace_index_request() || ! $this->is_exact_proxy_endpoint_request() ) { - $this->send_rest_no_route(); - } - - if ( $this->get_request_method() !== 'POST' ) { - $this->send_rest_no_route(); - } - - if ( ! $this->has_json_content_type() ) { - $this->send_rest_no_route(); - } - - if ( ! $this->has_valid_provenance() ) { - $this->send_rest_no_route(); - } - - if ( $this->request_body_too_large() ) { - $this->send_rest_no_route(); - } - - if ( ! $this->has_valid_payload() ) { - $this->send_rest_no_route(); - } + if ( + $this->is_namespace_index_request() + || ! $this->is_exact_proxy_endpoint_request() + || $this->get_request_method() !== 'POST' + || ! $this->has_json_content_type() + || ! $this->has_valid_provenance() + || $this->request_body_too_large() + || ! $this->has_valid_payload() + ) { + $this->send_rest_no_route(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mu-plugin/plausible-proxy-speed-module.php` around lines 107 - 135, The maybe_short_circuit_request() function relies on send_rest_no_route() to terminate execution implicitly (send_json_error() currently calls exit); make this explicit and robust by returning immediately after each send_rest_no_route() call (or restructure the checks into an if/elseif chain) so subsequent checks won't run if a short-circuit response was sent; update maybe_short_circuit_request() to call send_rest_no_route() followed by return and keep references to send_rest_no_route() and send_json_error() for clarity.
82-90: Prefix check lacks a boundary — potential over-match.
strpos($path, '/wp-json/' . $namespace) === 0will also match siblings like/wp-json/{namespace}something/...if some future route happens to share the prefix. With the randomized namespace this is extremely unlikely to collide in practice, but it's cheap to make the check exact by requiring the next character to be/or end-of-string.♻️ Proposed tightening
- return strpos( $this->get_request_path(), '/wp-json/' . $namespace ) === 0; + $prefix = '/wp-json/' . $namespace; + $path = $this->get_request_path(); + + return $path === $prefix || strpos( $path, $prefix . '/' ) === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mu-plugin/plausible-proxy-speed-module.php` around lines 82 - 90, The prefix check in is_proxy_request currently allows over-matching; update is_proxy_request to compute $prefix = '/wp-json/'.$namespace, verify the request path starts with that prefix (using strpos or strncmp), then ensure the character immediately after the prefix is either a slash or there is no character (path length equals prefix length). Use get_request_path() and resources['namespace'] to locate values; return false if the prefix doesn't match or the next character is neither '/' nor end-of-string, otherwise return true.src/Proxy.php (1)
398-427: Consider validatingrevenueshape.
revenueis whitelisted but otherwise passes through tosend_event()unchecked. Plausible expects{ currency: string, amount: number|string }here — if you care about defense-in-depth against a misbehaving tracker or an attacker forging a same-origin request, a light shape check (array withcurrencystring and numericamount) would keep the forwarded body tight. Not blocking if you'd rather stay permissive and let Plausible reject upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Proxy.php` around lines 398 - 427, The payload validator currently allows a `revenue` field through unchecked; update has_valid_payload to also validate `revenue` (before returning true) when set: ensure isset($params['revenue']) implies is_array($params['revenue']) with keys 'currency' and 'amount', that 'currency' is a non-empty string and 'amount' is a numeric value (or numeric string) — reject otherwise; keep this check inside has_valid_payload so only well-shaped revenue objects are forwarded to send_event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mu-plugin/plausible-proxy-speed-module.php`:
- Around line 149-151: The get_request_path() helper currently only returns the
path from $this->request_uri which misses REST requests sent via query string
(e.g. ?rest_route=/namespace/v1/...) or when rest_url_prefix is customized;
update get_request_path() to first check for a $_GET['rest_route'] value and, if
present, return that route's path (normalized/trimmed of leading slash),
otherwise fall back to wp_parse_url($this->request_uri, PHP_URL_PATH) ?: ''. Use
rest_get_url_prefix() if you need to normalize or detect prefixed routes so
query-string REST requests are treated the same as pretty-permalink ones.
- Around line 305-315: The current checks call normalize_domain() on $data['d']
and other string functions without guarding types, which can cause TypeError on
non-strings; update the validation in plausible-proxy-speed-module.php to mirror
src/Proxy.php by requiring is_string() for 'n', 'd', and 'u' before any string
operations: for 'n' require is_string($data['n']) and $data['n'] !== '' and
strlen(...) <= 120; for 'd' require is_string($data['d']) before calling
normalize_domain($data['d']) and comparing to
normalize_domain($this->get_expected_domain()); for 'u' require
is_string($data['u']) before strlen(...) and before calling
$this->url_matches_home_host($data['u']); keep the same length limits and
url_matches_home_host/normalize_domain/get_expected_domain checks otherwise so
both layers accept/reject identical inputs.
- Around line 340-348: get_expected_domain() currently returns home_url()
(including scheme/path) and skips the plausible_analytics_settings filter,
causing mismatch with Helpers::get_domain() which normalizes to a bare domain;
update get_expected_domain() to obtain settings via
get_option('plausible_analytics_settings', []) (respecting existing
filter/defaults) and then either call Helpers::get_domain() to normalize the
domain or replicate its normalization (preg_replace to strip ^https?://(www\.)?)
so the function returns a bare domain like example.com matching
Helpers::get_domain() and avoiding validation divergence.
- Around line 197-214: The apply_filters call for
'plausible_analytics_proxy_require_same_origin' in has_valid_provenance is
effectively unused because no callback is registered; either register a default
filter callback during plugin initialization or remove the filter. Fix by adding
a single, documented add_filter registration in your MU-plugin bootstrap (or
plugin init hook) that returns the desired boolean (e.g., respects a defined
constant or option) so apply_filters in has_valid_provenance() and the
corresponding call in src/Proxy.php are honorably overridable, or alternatively
remove the filter usage from has_valid_provenance() and src/Proxy.php if you
decide not to support overriding.
In `@src/Proxy.php`:
- Around line 489-496: The current check uses empty( $data['response']['code'] )
which can trigger PHP 8 warnings if $data['response'] is a scalar; update the
guard to ensure $data['response'] is an array before accessing ['code'].
Specifically, in the block after $data = $response->get_data() change the
conditional to verify is_array($data) && isset($data['response']) &&
is_array($data['response']) && ! empty($data['response']['code']) (or the
logical inverse to return early), so wp_remote_retrieve_response_code($data) and
$response->set_status(...) only run when $data['response']['code'] is safely
accessible.
---
Nitpick comments:
In `@mu-plugin/plausible-proxy-speed-module.php`:
- Around line 107-135: The maybe_short_circuit_request() function relies on
send_rest_no_route() to terminate execution implicitly (send_json_error()
currently calls exit); make this explicit and robust by returning immediately
after each send_rest_no_route() call (or restructure the checks into an
if/elseif chain) so subsequent checks won't run if a short-circuit response was
sent; update maybe_short_circuit_request() to call send_rest_no_route() followed
by return and keep references to send_rest_no_route() and send_json_error() for
clarity.
- Around line 82-90: The prefix check in is_proxy_request currently allows
over-matching; update is_proxy_request to compute $prefix =
'/wp-json/'.$namespace, verify the request path starts with that prefix (using
strpos or strncmp), then ensure the character immediately after the prefix is
either a slash or there is no character (path length equals prefix length). Use
get_request_path() and resources['namespace'] to locate values; return false if
the prefix doesn't match or the next character is neither '/' nor end-of-string,
otherwise return true.
In `@src/Proxy.php`:
- Around line 398-427: The payload validator currently allows a `revenue` field
through unchecked; update has_valid_payload to also validate `revenue` (before
returning true) when set: ensure isset($params['revenue']) implies
is_array($params['revenue']) with keys 'currency' and 'amount', that 'currency'
is a non-empty string and 'amount' is a numeric value (or numeric string) —
reject otherwise; keep this check inside has_valid_payload so only well-shaped
revenue objects are forwarded to send_event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff7257bc-c655-482b-926a-3af00dd25f60
📒 Files selected for processing (2)
mu-plugin/plausible-proxy-speed-module.phpsrc/Proxy.php
Comment on lines
+149
to
+151
| private function get_request_path() { | ||
| return wp_parse_url( $this->request_uri, PHP_URL_PATH ) ?: ''; | ||
| } |
There was a problem hiding this comment.
Query-string REST routing bypasses MU short-circuit.
When pretty permalinks are disabled, WordPress accepts REST requests via /?rest_route=/{namespace}/v1/.... The path-only check here sees / and treats the request as non-proxy, so all the hardened short-circuit logic is skipped (request still hits src/Proxy.php::validate_proxy_request(), so this is not a security regression — just lost early-rejection / speed benefit). Similar note for sites that customize rest_url_prefix away from wp-json. Either document the pretty-permalinks assumption or also inspect $_GET['rest_route'].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mu-plugin/plausible-proxy-speed-module.php` around lines 149 - 151, The
get_request_path() helper currently only returns the path from
$this->request_uri which misses REST requests sent via query string (e.g.
?rest_route=/namespace/v1/...) or when rest_url_prefix is customized; update
get_request_path() to first check for a $_GET['rest_route'] value and, if
present, return that route's path (normalized/trimmed of leading slash),
otherwise fall back to wp_parse_url($this->request_uri, PHP_URL_PATH) ?: ''. Use
rest_get_url_prefix() if you need to normalize or detect prefixed routes so
query-string REST requests are treated the same as pretty-permalink ones.
Comment on lines
+197
to
+214
| private function has_valid_provenance() { | ||
| if ( ! apply_filters( 'plausible_analytics_proxy_require_same_origin', true ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| $origin = $_SERVER[ 'HTTP_ORIGIN' ] ?? ''; | ||
| $referer = $_SERVER[ 'HTTP_REFERER' ] ?? ''; | ||
|
|
||
| if ( $origin && $this->host_matches_home( $origin ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( $referer && $this->host_matches_home( $referer ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=php -C2 "plausible_analytics_proxy_require_same_origin"Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Find the constructor and where has_valid_provenance is called in the MU plugin
rg -n "function __construct|has_valid_provenance" mu-plugin/plausible-proxy-speed-module.php -A 5Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 719
🏁 Script executed:
#!/bin/bash
# Check the full context of the MU plugin to understand initialization
head -50 mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 970
🏁 Script executed:
#!/bin/bash
# Search for where the MU plugin class is instantiated
rg -n "new.*plausible" mu-plugin/plausible-proxy-speed-module.php -iRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check the init() method
sed -n '57,130p' mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 1650
🏁 Script executed:
#!/bin/bash
# Check context around instantiation at line 398
tail -20 mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Check if there are any hooks where the class is registered
rg -n "add_action|do_action|add_filter" mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Find send_rest_no_route method
rg -n "send_rest_no_route" mu-plugin/plausible-proxy-speed-module.php -A 5Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 901
🏁 Script executed:
#!/bin/bash
# Check WordPress mu-plugin loading documentation info - when are filters available?
# Let's also verify if there's any filter registration happening before instantiation
rg -n "^[^/]*apply_filters|^[^/]*add_filter" mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Let's verify the WordPress hook firing order - when init happens vs when plugins load
# Check if there are any comments about timing or hook order
rg -n "init|load|wp_" mu-plugin/plausible-proxy-speed-module.php | head -20Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Check if the regular plugin (src/Proxy.php) registers any callbacks before its filter use
rg -B20 "apply_filters.*plausible_analytics_proxy_require_same_origin" src/Proxy.php | head -40Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Let's see the full context around the Proxy.php usage of the same filter
sed -n '340,360p' src/Proxy.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 629
🏁 Script executed:
#!/bin/bash
# Check when exactly has_valid_provenance is called - need to understand is_proxy_request initialization
sed -n '49,105p' mu-plugin/plausible-proxy-speed-module.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 1308
🏁 Script executed:
#!/bin/bash
# Let's check if there's any way filters could be registered before the MU plugin runs
# Look for wp-config or other early loading mechanisms
git ls-files | grep -E "(wp-config|config|setup)" | head -10Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Verify when MU plugins are loaded relative to regular plugins and when filters can be registered
# Check if there's any documentation in the plugin about this
rg -n "mu.plugin|must.use|load|filter.*register" . -i --type=php --type=md | grep -i "mu\|must\|load\|early" | head -10Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 1118
🏁 Script executed:
#!/bin/bash
# Let's check when the main Plausible Analytics plugin is loaded and when it might register filters
rg -n "register_activation_hook|plugins_loaded|init" src/Plugin.php 2>/dev/null | head -20Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about the architecture
find . -name "README*" -o -name "*.md" | head -5 | xargs cat 2>/dev/null | head -50Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 2728
🏁 Script executed:
#!/bin/bash
# Check if the filter is ever registered with add_filter anywhere in the codebase
rg -n "add_filter.*plausible_analytics_proxy_require_same_origin" .Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Also search for any function that registers this filter
rg -n "plausible_analytics_proxy_require_same_origin" . --type=php | grep -v "apply_filters"Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Let's trace where Proxy.php is instantiated and when it runs
rg -n "new Proxy|Proxy::" src/ --type=php | head -20Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 804
🏁 Script executed:
#!/bin/bash
# Check if Proxy.php is loaded through the main plugin flow
rg -n "class Proxy\|function.*proxy" src/Plugin.php -A 3 | head -30Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Let's understand: when is Proxy.php actually used? On every request or only sometimes?
rg -B5 -A5 "has_valid_provenance" src/Proxy.php | head -50Repository: a8cteam51/plausible-wordpress-plugin
Length of output: 624
Filter has no registered callbacks and is effectively unused.
apply_filters( 'plausible_analytics_proxy_require_same_origin', true ) is applied on every proxy request in both the MU layer and src/Proxy.php, but the filter is never registered with a callback anywhere in the codebase. This means the default value of true is always used, making the filter dead code. If this filter is intended for users to override, either register default callbacks in the plugin initialization or document the requirement to hook it from wp-config.php or another MU plugin.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mu-plugin/plausible-proxy-speed-module.php` around lines 197 - 214, The
apply_filters call for 'plausible_analytics_proxy_require_same_origin' in
has_valid_provenance is effectively unused because no callback is registered;
either register a default filter callback during plugin initialization or remove
the filter. Fix by adding a single, documented add_filter registration in your
MU-plugin bootstrap (or plugin init hook) that returns the desired boolean
(e.g., respects a defined constant or option) so apply_filters in
has_valid_provenance() and the corresponding call in src/Proxy.php are honorably
overridable, or alternatively remove the filter usage from
has_valid_provenance() and src/Proxy.php if you decide not to support
overriding.
Comment on lines
+489
to
496
| $data = $response->get_data(); | ||
|
|
||
| if ( ! is_array( $data ) || empty( $data['response']['code'] ) ) { | ||
| return $response; | ||
| } | ||
|
|
||
| $response_code = wp_remote_retrieve_response_code( $data ); | ||
| $response->set_status( $response_code ); |
There was a problem hiding this comment.
Tighten the response subarray check to avoid PHP 8 warnings.
empty( $data['response']['code'] ) will emit a "cannot access offset on string" warning on PHP 8+ if $data['response'] happens to be a scalar rather than an array. Adding an is_array() guard on the intermediate offset keeps this fully warning-free and matches the intent of the existing is_array( $data ) check.
🛡️ Proposed fix
- if ( ! is_array( $data ) || empty( $data['response']['code'] ) ) {
+ if ( ! is_array( $data ) || ! isset( $data['response'] ) || ! is_array( $data['response'] ) || empty( $data['response']['code'] ) ) {
return $response;
}PHP 8 warning for array access with non-numeric string key on a scalar value
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Proxy.php` around lines 489 - 496, The current check uses empty(
$data['response']['code'] ) which can trigger PHP 8 warnings if
$data['response'] is a scalar; update the guard to ensure $data['response'] is
an array before accessing ['code']. Specifically, in the block after $data =
$response->get_data() change the conditional to verify is_array($data) &&
isset($data['response']) && is_array($data['response']) && !
empty($data['response']['code']) (or the logical inverse to return early), so
wp_remote_retrieve_response_code($data) and $response->set_status(...) only run
when $data['response']['code'] is safely accessible.
- Guard n/d/u with isset + is_string before string ops to avoid TypeError on non-string payloads (which would surface as 500s and defeat the uniform-404 design). - Use === '' instead of empty() for n and u so legitimate values like 0 aren't rejected. - Strip scheme and www. from the home_url() fallback in get_expected_domain() so it returns a bare domain matching Helpers::get_domain(), preventing silent rejection of valid payloads on sites without a configured domain_name setting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mu-plugin/plausible-proxy-speed-module.php (1)
89-90:⚠️ Potential issue | 🟡 MinorHandle query-string REST routing and custom REST prefixes in path detection.
This still only recognizes pretty-permalink paths under
/wp-json/.... Requests like/?rest_route=/{namespace}/v1/...(and installations with customrest_url_prefix) bypass MU short-circuiting, so hardening/speed benefits are skipped for those requests.💡 Proposed fix
+ private function get_rest_prefix() { + return '/' . trim( rest_get_url_prefix(), '/' ); + } + private function get_request_path() { - return wp_parse_url( $this->request_uri, PHP_URL_PATH ) ?: ''; + $rest_route = isset( $_GET['rest_route'] ) ? (string) wp_unslash( $_GET['rest_route'] ) : ''; + + if ( $rest_route !== '' ) { + $path = wp_parse_url( $rest_route, PHP_URL_PATH ) ?: $rest_route; + return '/' . ltrim( $path, '/' ); + } + + return wp_parse_url( $this->request_uri, PHP_URL_PATH ) ?: ''; } private function is_proxy_request() { $namespace = $this->resources[ 'namespace' ] ?? ''; @@ - return strpos( $this->get_request_path(), '/wp-json/' . $namespace ) === 0; + return strpos( $this->get_request_path(), $this->get_rest_prefix() . '/' . $namespace ) === 0; } @@ - return '/wp-json/' . $namespace . '/v1/' . $base . '/' . $endpoint; + return $this->get_rest_prefix() . '/' . $namespace . '/v1/' . $base . '/' . $endpoint; } @@ - return $this->get_request_path() === '/wp-json/' . ( $this->resources[ 'namespace' ] ?? '' ) . '/v1'; + return $this->get_request_path() === $this->get_rest_prefix() . '/' . ( $this->resources[ 'namespace' ] ?? '' ) . '/v1'; }#!/bin/bash # Verify current routing detection only uses REQUEST_URI path + hardcoded /wp-json prefix. rg -n --type=php -C3 "get_request_path|rest_route|rest_get_url_prefix|/wp-json/" mu-plugin/plausible-proxy-speed-module.phpAlso applies to: 149-151, 161-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mu-plugin/plausible-proxy-speed-module.php` around lines 89 - 90, The current path-detection only checks get_request_path() for a hardcoded '/wp-json/{namespace}' prefix (e.g. the strpos call) and therefore misses REST requests sent via the rest_route query string or sites with a custom rest_url_prefix; update the detection logic used in the methods that currently reference get_request_path()/'/wp-json/' (including the other similar checks around the same module like the code at the other occurrences) to also inspect $_GET['rest_route'] (normalize and trim leading slashes) and compare against rest_get_url_prefix() .'/'. $namespace (or build the expected prefix by joining rest_get_url_prefix() and $namespace) so both pretty permalink and query-string REST routes (and custom rest URL prefixes) are recognized and short-circuited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mu-plugin/plausible-proxy-speed-module.php`:
- Line 14: The MU plugin hardcodes MAX_REQUEST_BYTES (used in the constant,
request_body_too_large(), and get_request_body()) to 8192 which bypasses the
filter used in src/Proxy.php; change those sites to obtain the limit via the
same filter (plausible_analytics_proxy_max_body_bytes) instead of the hardcoded
constant so the MU layer respects configured values—replace direct uses of
MAX_REQUEST_BYTES with a call to the filter (defaulting to 8192) and ensure
request_body_too_large() and get_request_body() both use that filtered value.
---
Duplicate comments:
In `@mu-plugin/plausible-proxy-speed-module.php`:
- Around line 89-90: The current path-detection only checks get_request_path()
for a hardcoded '/wp-json/{namespace}' prefix (e.g. the strpos call) and
therefore misses REST requests sent via the rest_route query string or sites
with a custom rest_url_prefix; update the detection logic used in the methods
that currently reference get_request_path()/'/wp-json/' (including the other
similar checks around the same module like the code at the other occurrences) to
also inspect $_GET['rest_route'] (normalize and trim leading slashes) and
compare against rest_get_url_prefix() .'/'. $namespace (or build the expected
prefix by joining rest_get_url_prefix() and $namespace) so both pretty permalink
and query-string REST routes (and custom rest URL prefixes) are recognized and
short-circuited.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 748024a8-50f4-407e-9185-57ff5e2a6458
📒 Files selected for processing (2)
CHANGES.mdmu-plugin/plausible-proxy-speed-module.php
| */ | ||
|
|
||
| class PlausibleProxySpeed { | ||
| const MAX_REQUEST_BYTES = 8192; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify limit handling differs between MU layer and src/Proxy.php.
rg -n --type=php -C3 "MAX_REQUEST_BYTES|plausible_analytics_proxy_max_body_bytes|request_body_too_large|get_request_body" mu-plugin/plausible-proxy-speed-module.php src/Proxy.phpRepository: a8cteam51/plausible-wordpress-plugin
Length of output: 3444
Align MU plugin's request size limit with the filterable limit in src/Proxy.php.
The MU layer hardcodes MAX_REQUEST_BYTES to 8192 in three places:
- Line 14: constant definition
- Line 283-284:
request_body_too_large()method - Line 329-331:
get_request_body()method parameter
Meanwhile, src/Proxy.php applies the filter plausible_analytics_proxy_max_body_bytes (line 284), allowing customization. When this filter is modified, the MU plugin still rejects requests at 8192, silently bypassing the configured limit and causing valid requests to fail.
Update the MU plugin to use the same filterable approach as src/Proxy.php, ensuring consistent behavior across both validation layers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mu-plugin/plausible-proxy-speed-module.php` at line 14, The MU plugin
hardcodes MAX_REQUEST_BYTES (used in the constant, request_body_too_large(), and
get_request_body()) to 8192 which bypasses the filter used in src/Proxy.php;
change those sites to obtain the limit via the same filter
(plausible_analytics_proxy_max_body_bytes) instead of the hardcoded constant so
the MU layer respects configured values—replace direct uses of MAX_REQUEST_BYTES
with a call to the filter (defaulting to 8192) and ensure
request_body_too_large() and get_request_body() both use that filtered value.
Member
Author
|
No need to merge this PR into our own I've raised a PR with the Plausible repo: plausible#300 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR hardens the WordPress proxy endpoint used by the plugin’s “Bypass ad blockers” feature.
The goal is to make the proxy route:
This is not intended to make the endpoint secret from any client that fully loads the page. It is intended to reduce route enumeration and tighten request validation so operators can protect sites more safely.
Problem
The proxy endpoint is randomized, but in the current implementation it is still relatively easy to discover and probe:
permission_callbackThat makes the route easier to target than it needs to be, and can force site operators into blunt edge rules that may also block legitimate Plausible tracking.
Changes
src/Proxy.phprest_route_data404for namespace probing like/wp-json/<namespace>/v1/viarest_pre_dispatchpermission_callback => __return_truewith request validationContent-Type: application/jsonOriginorRefererdupmust be an array if presentmu-plugin/plausible-proxy-speed-module.php/wp-json/<namespace>prefixphp://inputfor body-size enforcement instead of trustingContent-Lengthsrc/Proxy.php, including relative URLs like/some-pageWhy these changes
1. Route discovery
The proxy route is randomized, but if it appears in
/wp-json/, it is still enumerable. Hiding it from REST discovery removes the most obvious public discovery surface.2. Namespace probing
Even if the root REST index is hidden, a known namespace should not expose a readable namespace index. Returning
404makes the route less self-documenting.3. Request validation
The proxy endpoint is expected to receive anonymous browser traffic, but it does not need to accept arbitrary malformed traffic. Tightening validation helps reject obvious junk early.
4. Early rejection in the MU layer
The MU speed module is the cheapest place to reject bad proxy traffic. The more validation that can happen there, the less unnecessary WordPress work happens under load.
Verified behaviors
With proxy enabled:
/wp-json/does not list the generated Plausible namespace/routeGET /wp-json/<namespace>/v1/returns404plausible.init({ endpoint: ... })202"u": "/test"also returns202Origin/Refererreturns404Origin/Refererreturns404dreturns400400400pas a non-array returns400404413Additional behavior checks
Notes
OPTIONS/ preflight requests now return404. This is acceptable for the intended same-origin browser use case, but worth being aware of if cross-origin proxy usage is ever expected.Resolves plausible#294
Summary by CodeRabbit
New Features
Bug Fixes