Fix wp engine issue - #22
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds anonymous metering across server, storage, endpoint, frontend, and client paths. Posts receive render modes, anonymous subjects use server-side ledgers, protected samples can be released through AJAX, and paywall counters can display remaining views. ChangesAnonymous metering runtime
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/js/src/metering.js`:
- Line 68: The metering template hydration only replaces the first {count}
token, which can diverge from the PHP rendering path. Update the logic in
metering.js where node.textContent is assigned so it replaces every occurrence
of {count}, matching the behavior of the server-side str_replace and ensuring
templates with multiple placeholders render consistently.
- Around line 52-54: The `record` helper in `metering.js` is overwriting an
existing `cfg.postId` timestamp every time it runs, which should only happen for
first-time counts. Update `record` so it preserves the existing value when
`views[cfg.postId]` is already present, and only assigns `nowSeconds()` for new
post IDs before calling `persist(views)`. Use the existing `record_and_check()`
flow and `cfg.postId` lookup to keep the original view timestamp intact.
In `@wordpress/wp-content/plugins/memberful-wp/src/content_filter.php`:
- Around line 261-272: Wrap the state mutation in
memberful_wp_render_metered_body() around the apply_filters('the_content', ...)
call with a try/finally so the original $GLOBALS['post'], wp_reset_postdata(),
and memberful_metering_releasing_post_id(0) are always restored even if a
shortcode/block/plugin throws. Use the existing
memberful_wp_render_metered_body() and memberful_metering_releasing_post_id()
symbols to keep the release state and global post isolated for later rendering
in the same request.
🪄 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: Repository: TheCodeCompany/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7ab6923-e0da-4855-b4b6-28f50157a9b6
📒 Files selected for processing (9)
wordpress/wp-content/plugins/memberful-wp/js/src/blocks/metering-countdown/render.phpwordpress/wp-content/plugins/memberful-wp/js/src/metering.jswordpress/wp-content/plugins/memberful-wp/src/content_filter.phpwordpress/wp-content/plugins/memberful-wp/src/metering.phpwordpress/wp-content/plugins/memberful-wp/src/metering/access.phpwordpress/wp-content/plugins/memberful-wp/src/metering/frontend.phpwordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.phpwordpress/wp-content/plugins/memberful-wp/stylesheets/metering.csswordpress/wp-content/plugins/memberful-wp/webpack.config.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wordpress/wp-content/plugins/memberful-wp/js/src/metering.js (1)
79-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHydrate every countdown block, not just the first.
querySelector()updates only one[data-memberful-countdown], so additional countdown blocks on the page stay stale after client hydration.const hydrateCountdown = (remaining) => { const nodes = document.querySelectorAll('[data-memberful-countdown]'); if (!nodes.length) { return; } const count = String(Math.max(0, remaining)); nodes.forEach((node) => { const template = node.getAttribute('data-memberful-template') || ''; node.textContent = template.replace(/\{count\}/g, count); node.hidden = false; }); };As per path instructions, “Review for possible edge cases and unexpected behaviour.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/js/src/metering.js` around lines 79 - 86, The hydrateCountdown helper currently uses document.querySelector(), so only the first [data-memberful-countdown] element is updated during hydration. Update hydrateCountdown in metering.js to target all matching nodes, iterate over each one, and apply the same template replacement and hidden=false behavior so every countdown block stays in sync after client-side hydration.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/frontend.php`:
- Around line 41-45: The inline runtime config in memberful_metering frontend
setup should fail closed when JSON encoding fails instead of outputting a false
value. Update the logic around memberful_metering_runtime_config() and
wp_json_encode() so the encoded result is stored first, checked for false, and
the wp_add_inline_script() call is skipped on failure, matching the guarded
behavior used in memberful_metering_prepaint().
---
Outside diff comments:
In `@wordpress/wp-content/plugins/memberful-wp/js/src/metering.js`:
- Around line 79-86: The hydrateCountdown helper currently uses
document.querySelector(), so only the first [data-memberful-countdown] element
is updated during hydration. Update hydrateCountdown in metering.js to target
all matching nodes, iterate over each one, and apply the same template
replacement and hidden=false behavior so every countdown block stays in sync
after client-side hydration.
🪄 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: Repository: TheCodeCompany/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51799867-0a03-41d9-93df-8540eae16491
📒 Files selected for processing (5)
wordpress/wp-content/plugins/memberful-wp/js/src/metering.jswordpress/wp-content/plugins/memberful-wp/src/content_filter.phpwordpress/wp-content/plugins/memberful-wp/src/metering/access.phpwordpress/wp-content/plugins/memberful-wp/src/metering/frontend.phpwordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wordpress/wp-content/plugins/memberful-wp/src/metering/access.php (1)
152-195: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSerialize anonymous ledger updates before releasing protected content.
Concurrent requests for one subject can all read the same under-limit ledger, each return
allowed, and overwrite one another. Since subjects with an active ledger bypass the cookieless cap, repeated bursts can exceedanonymous_limit.Use a per-subject lock or atomic compare-and-set around the complete read/prune/check/write operation, failing closed when synchronization cannot be acquired. This requires coordinated storage changes, so a local suggestion is not safe.
As per path instructions, review possible edge cases and unexpected behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/src/metering/access.php` around lines 152 - 195, Serialize the complete anonymous flow in Memberful_Metering_Access::record_and_check for each subject: acquire a per-subject lock or atomic compare-and-set before read/prune/check/write, hold it through the decision, and release it on every return path. Fail closed with allowed=false when synchronization cannot be acquired, while preserving the existing user-based path and ledger semantics; update Memberful_Metering_Storage with the required coordinated locking/CAS primitives and handle stale-lock, cleanup, and failure cases safely.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php`:
- Around line 176-185: Update is_rate_limited so it increments and checks the
per-IP counter first, immediately returning true when RATE_PER_IP is exceeded;
only requests within the per-IP limit should increment and evaluate the global
counter against RATE_GLOBAL.
- Around line 160-167: Update the origin comparison around the source/site URL
parsing so it validates and compares scheme, host, and effective port rather
than hosts alone. Restrict schemes to HTTP/HTTPS, reject missing or invalid
origins, apply default ports based on each scheme, and return true whenever any
origin component differs; preserve the existing same-origin false result.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php`:
- Around line 64-87: Update get_or_create_subject so it always recalculates the
expiry, signs the existing or newly generated subject with that expiry, sends
the refreshed cookie, and updates $_COOKIE[ self::COOKIE_NAME ]. Preserve
subject generation only when read_subject returns null, and review behavior for
invalid period_days and existing cookie values.
- Around line 204-220: Update Metering storage’s incr_counter method to enforce
fixed-window fallback counters by clamping $ttl to at least 1 and appending the
current floor(time() / $ttl) bucket to $key before cache or transient
operations. Preserve the existing increment and expiry behavior while ensuring
continuous traffic creates a new counter at each window boundary instead of
extending one indefinitely.
---
Outside diff comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/access.php`:
- Around line 152-195: Serialize the complete anonymous flow in
Memberful_Metering_Access::record_and_check for each subject: acquire a
per-subject lock or atomic compare-and-set before read/prune/check/write, hold
it through the decision, and release it on every return path. Fail closed with
allowed=false when synchronization cannot be acquired, while preserving the
existing user-based path and ledger semantics; update Memberful_Metering_Storage
with the required coordinated locking/CAS primitives and handle stale-lock,
cleanup, and failure cases safely.
🪄 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: Repository: TheCodeCompany/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63f570eb-162d-4a75-b859-f755ad108649
📒 Files selected for processing (5)
wordpress/wp-content/plugins/memberful-wp/js/src/metering.jswordpress/wp-content/plugins/memberful-wp/src/metering/access.phpwordpress/wp-content/plugins/memberful-wp/src/metering/frontend.phpwordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.phpwordpress/wp-content/plugins/memberful-wp/src/metering/storage.php
💤 Files with no reviewable changes (1)
- wordpress/wp-content/plugins/memberful-wp/js/src/metering.js
| $source_host = wp_parse_url( $source, PHP_URL_HOST ); | ||
| $site_host = wp_parse_url( home_url(), PHP_URL_HOST ); | ||
|
|
||
| if ( ! is_string( $source_host ) || ! is_string( $site_host ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| return strtolower( $source_host ) !== strtolower( $site_host ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Compare complete origins, not only hosts.
Different schemes or ports are cross-origin but currently pass this check.
| $source_host = wp_parse_url( $source, PHP_URL_HOST ); | |
| $site_host = wp_parse_url( home_url(), PHP_URL_HOST ); | |
| if ( ! is_string( $source_host ) || ! is_string( $site_host ) ) { | |
| return true; | |
| } | |
| return strtolower( $source_host ) !== strtolower( $site_host ); | |
| $source_url = wp_parse_url( $source ); | |
| $site_url = wp_parse_url( home_url() ); | |
| if ( ! is_array( $source_url ) || ! is_array( $site_url ) ) { | |
| return true; | |
| } | |
| $source_scheme = isset( $source_url['scheme'] ) ? strtolower( $source_url['scheme'] ) : ''; | |
| $site_scheme = isset( $site_url['scheme'] ) ? strtolower( $site_url['scheme'] ) : ''; | |
| $source_host = isset( $source_url['host'] ) ? strtolower( $source_url['host'] ) : ''; | |
| $site_host = isset( $site_url['host'] ) ? strtolower( $site_url['host'] ) : ''; | |
| if ( | |
| ! in_array( $source_scheme, array( 'http', 'https' ), true ) | |
| || ! in_array( $site_scheme, array( 'http', 'https' ), true ) | |
| || '' === $source_host | |
| || '' === $site_host | |
| ) { | |
| return true; | |
| } | |
| $source_port = isset( $source_url['port'] ) ? (int) $source_url['port'] : ( 'https' === $source_scheme ? 443 : 80 ); | |
| $site_port = isset( $site_url['port'] ) ? (int) $site_url['port'] : ( 'https' === $site_scheme ? 443 : 80 ); | |
| return $source_scheme !== $site_scheme | |
| || $source_host !== $site_host | |
| || $source_port !== $site_port; |
As per path instructions, review possible edge cases and unexpected behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php`
around lines 160 - 167, Update the origin comparison around the source/site URL
parsing so it validates and compares scheme, host, and effective port rather
than hosts alone. Restrict schemes to HTTP/HTTPS, reject missing or invalid
origins, apply default ports based on each scheme, and return true whenever any
origin component differs; preserve the existing same-origin false result.
Source: Path instructions
| /** | ||
| * Return the subject id for this request, minting and setting the cookie when absent. | ||
| * | ||
| * MUST be called only from an uncached context (admin-ajax, login) — never while rendering a cacheable page, or the | ||
| * Set-Cookie would either be dropped by the cache or baked into shared HTML. | ||
| * | ||
| * @param int $period_days Rolling-window length; also drives the cookie's expiry. | ||
| * | ||
| * @return string | ||
| */ | ||
| public static function get_or_create_subject( int $period_days ): string { | ||
| $subject = self::read_subject(); | ||
|
|
||
| if ( null === $subject ) { | ||
| $subject = bin2hex( random_bytes( self::SUBJECT_BYTES ) ); | ||
| $expires = time() + ( max( 1, $period_days ) * DAY_IN_SECONDS ); | ||
| $signed = self::sign( $subject, $expires ); | ||
|
|
||
| self::send_cookie( $signed, $expires ); | ||
| $_COOKIE[ self::COOKIE_NAME ] = $signed; | ||
| } | ||
|
|
||
| return self::apply_subject_filter( $subject ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Refresh the subject expiry when recording later views.
The cookie expires period_days after its first creation, even when newer ledger entries remain active. A subject created on day 1 and used on day 29 resets on day 31, bypassing the rolling window.
| /** | |
| * Return the subject id for this request, minting and setting the cookie when absent. | |
| * | |
| * MUST be called only from an uncached context (admin-ajax, login) — never while rendering a cacheable page, or the | |
| * Set-Cookie would either be dropped by the cache or baked into shared HTML. | |
| * | |
| * @param int $period_days Rolling-window length; also drives the cookie's expiry. | |
| * | |
| * @return string | |
| */ | |
| public static function get_or_create_subject( int $period_days ): string { | |
| $subject = self::read_subject(); | |
| if ( null === $subject ) { | |
| $subject = bin2hex( random_bytes( self::SUBJECT_BYTES ) ); | |
| $expires = time() + ( max( 1, $period_days ) * DAY_IN_SECONDS ); | |
| $signed = self::sign( $subject, $expires ); | |
| self::send_cookie( $signed, $expires ); | |
| $_COOKIE[ self::COOKIE_NAME ] = $signed; | |
| } | |
| return self::apply_subject_filter( $subject ); | |
| } | |
| /** | |
| * Return the subject id for this request, minting it when absent and refreshing the signed cookie expiry. | |
| * | |
| * MUST be called only from an uncached context (admin-ajax, login) — never while rendering a cacheable page, or the | |
| * Set-Cookie would either be dropped by the cache or baked into shared HTML. | |
| * | |
| * `@param` int $period_days Rolling-window length; also drives the cookie's expiry. | |
| * | |
| * `@return` string | |
| */ | |
| public static function get_or_create_subject( int $period_days ): string { | |
| $subject = self::read_subject(); | |
| if ( null === $subject ) { | |
| $subject = bin2hex( random_bytes( self::SUBJECT_BYTES ) ); | |
| } | |
| $expires = time() + ( max( 1, $period_days ) * DAY_IN_SECONDS ); | |
| $signed = self::sign( $subject, $expires ); | |
| self::send_cookie( $signed, $expires ); | |
| $_COOKIE[ self::COOKIE_NAME ] = $signed; | |
| return self::apply_subject_filter( $subject ); | |
| } |
As per path instructions, review possible edge cases and unexpected behaviour.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 74-87: get_or_create_subject accesses the super-global variable $_COOKIE. (undefined)
(Superglobals)
[error] 74-87: The method get_or_create_subject is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 74-87: The parameter $period_days is not named in camelCase. (undefined)
(CamelCaseParameterName)
🪛 PHPStan (2.2.5)
[warning] 79-79: Constant DAY_IN_SECONDS not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php` around
lines 64 - 87, Update get_or_create_subject so it always recalculates the
expiry, signs the existing or newly generated subject with that expiry, sends
the refreshed cookie, and updates $_COOKIE[ self::COOKIE_NAME ]. Preserve
subject generation only when read_subject returns null, and review behavior for
invalid period_days and existing cookie values.
Source: Path instructions
| public static function incr_counter( string $key, int $ttl ): int { | ||
| if ( wp_using_ext_object_cache() ) { | ||
| wp_cache_add( $key, 0, self::CACHE_GROUP, $ttl ); | ||
| $new = wp_cache_incr( $key, 1, self::CACHE_GROUP ); | ||
| if ( is_int( $new ) ) { | ||
| return $new; | ||
| } | ||
| // Object cache present but without a working incr: fall through to the transient counter. A given backend is | ||
| // consistent about incr support, so reads and writes stay on one store (read_counter no longer exists to split | ||
| // them). This never returns a constant that would disable enforcement. | ||
| } | ||
|
|
||
| // Transient counter: atomic where transients are backed by a persistent object cache; best-effort (approximate | ||
| // under heavy concurrency) on hosts with no object cache, where WordPress exposes no atomic increment primitive. | ||
| // Acceptable for a soft meter — the per-subject ledger cap still bounds each individual visitor regardless. | ||
| $value = (int) get_transient( $key ) + 1; | ||
| set_transient( $key, $value, $ttl ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep fallback counters fixed-window.
Passing $ttl to every set_transient() call refreshes its expiry, so continuous traffic can keep the global endpoint blocked indefinitely. WordPress confirms that updating a transient with an expiration updates its expiry. (developer.wordpress.org)
Bucket the key so old counters become irrelevant at the window boundary:
| public static function incr_counter( string $key, int $ttl ): int { | |
| if ( wp_using_ext_object_cache() ) { | |
| wp_cache_add( $key, 0, self::CACHE_GROUP, $ttl ); | |
| $new = wp_cache_incr( $key, 1, self::CACHE_GROUP ); | |
| if ( is_int( $new ) ) { | |
| return $new; | |
| } | |
| // Object cache present but without a working incr: fall through to the transient counter. A given backend is | |
| // consistent about incr support, so reads and writes stay on one store (read_counter no longer exists to split | |
| // them). This never returns a constant that would disable enforcement. | |
| } | |
| // Transient counter: atomic where transients are backed by a persistent object cache; best-effort (approximate | |
| // under heavy concurrency) on hosts with no object cache, where WordPress exposes no atomic increment primitive. | |
| // Acceptable for a soft meter — the per-subject ledger cap still bounds each individual visitor regardless. | |
| $value = (int) get_transient( $key ) + 1; | |
| set_transient( $key, $value, $ttl ); | |
| public static function incr_counter( string $key, int $ttl ): int { | |
| $ttl = max( 1, $ttl ); | |
| $key .= '_' . (string) floor( time() / $ttl ); | |
| if ( wp_using_ext_object_cache() ) { |
As per path instructions, follow WordPress best practices.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 204-223: The method incr_counter is not named in camelCase. (undefined)
(CamelCaseMethodName)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php` around
lines 204 - 220, Update Metering storage’s incr_counter method to enforce
fixed-window fallback counters by clamping $ttl to at least 1 and appending the
current floor(time() / $ttl) bucket to $key before cache or transient
operations. Preserve the existing increment and expiry behavior while ensuring
continuous traffic creates a new counter at each window boundary instead of
extending one indefinitely.
Source: Path instructions
There was a problem hiding this comment.
@av3nger this seems to be a legit issue. Refreshing the TTL on every increment, and over-limit IPs still bump the global counter. Once the global sample bucket passes 600/min it never expires while traffic continues, so all anonymous visitors may get 429 and no protected content is released until the endpoint sees a full minute of no requests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c5c1c76. Configure here.
| && Memberful_Metering_Access::RENDER_FREE_METER === Memberful_Metering_Access::current_anon_mode( $post->ID ) | ||
| ) { | ||
| return $content; | ||
| } |
There was a problem hiding this comment.
Divider teaser skipped by ID check
Medium Severity
The if condition preserving the paywall divider marker for metered content fails due to a strict equality (===) comparison between $post->ID (string) and get_queried_object_id() (int). This type mismatch incorrectly strips the divider, preventing the above-divider teaser from showing in the metered paywall.
Triggered by team rule: Code Review Guidelines
Reviewed by Cursor Bugbot for commit c5c1c76. Configure here.
| array( | ||
| 'released' => false, | ||
| 'remaining' => 0, | ||
| 'synced' => $synced, |
There was a problem hiding this comment.
Cap exemption needs fragile marker
Medium Severity
Cookieless-cap exemption now depends on a separate protected_release transient plus a non-empty ledger, instead of the ledger alone. On hosts that evict object-cache transients under pressure (including WP Engine), the marker can disappear while the ledger remains, so returning visitors are treated as cookieless again and burn the hourly cap—even after a prior successful protected release.
Additional Locations (2)
Triggered by team rule: Code Review Guidelines
Reviewed by Cursor Bugbot for commit c5c1c76. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php (1)
232-243: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA single over-limit IP can still exhaust the per-op global bucket.
Both counters are incremented unconditionally before checking either threshold. Once an IP exceeds
RATE_PER_IP, its continued requests still bump$global_key, letting it drive the op's global counter pastRATE_GLOBALand 429 every other visitor for that operation.🛠️ Suggested fix (short-circuit before the global increment)
private static function is_rate_limited( string $op ): bool { $bucket = ( 'record_public' === $op ) ? 'public' : 'sample'; $ip_key = 'mbf_mtr_rl_' . $bucket . '_' . hash( 'sha256', self::client_ip() . wp_salt( 'auth' ) ); $global_key = 'mbf_mtr_rl_global_' . $bucket; - // Increment first, then compare the returned value: concurrent requests get distinct atomic counts, so they cannot - // all observe an under-limit value and slip through. Over-limit requests still increment, which keeps them blocked. - $ip_hits = Memberful_Metering_Storage::incr_counter( $ip_key, MINUTE_IN_SECONDS ); - $global_hits = Memberful_Metering_Storage::incr_counter( $global_key, MINUTE_IN_SECONDS ); - - return ( $ip_hits > self::RATE_PER_IP || $global_hits > self::RATE_GLOBAL ); + $ip_hits = Memberful_Metering_Storage::incr_counter( $ip_key, MINUTE_IN_SECONDS ); + if ( $ip_hits > self::RATE_PER_IP ) { + return true; + } + + $global_hits = Memberful_Metering_Storage::incr_counter( $global_key, MINUTE_IN_SECONDS ); + + return $global_hits > self::RATE_GLOBAL; }As per path instructions, review possible edge cases and unexpected behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php` around lines 232 - 243, Update is_rate_limited so it checks the per-IP counter immediately after incrementing it and returns true when RATE_PER_IP is exceeded, before incrementing the operation’s global counter. Only requests still under the per-IP limit should increment and evaluate global_hits, preserving rate limiting for both thresholds without allowing one IP to consume the global bucket.Source: Path instructions
wordpress/wp-content/plugins/memberful-wp/src/metering/access.php (1)
355-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated post-eligibility guard across three methods.
The
publish+is_post_publicly_viewable+!post_password_requiredcheck is repeated verbatim inis_releasable_sample(),evaluate_sample(), and the loop insiderecord_public_views(). A future tightening (e.g. adding a sticky/expiry check) applied to one call site but missed in another would silently create an inconsistent access decision.♻️ Suggested extraction
+ /** + * Whether a post is currently publicly readable content (published, publicly viewable, not password protected). + * + * `@param` WP_Post|null $post Post to check. + * + * `@return` bool + */ + private static function is_publicly_readable( ?WP_Post $post ): bool { + return ( $post instanceof WP_Post ) + && 'publish' === $post->post_status + && is_post_publicly_viewable( $post ) + && ! post_password_required( $post ); + }Then replace each
! ( $post instanceof WP_Post ) || 'publish' !== $post->post_status/! is_post_publicly_viewable( $post ) || post_password_required( $post )pair with! self::is_publicly_readable( $post ).Also applies to: 424-437, 299-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/src/metering/access.php` around lines 355 - 366, Extract the shared published/publicly-viewable/not-password-protected eligibility logic into an `is_publicly_readable()` helper, then update `is_releasable_sample()`, `evaluate_sample()`, and the `record_public_views()` loop to use `! self::is_publicly_readable( $post )` while preserving their existing behavior.
♻️ Duplicate comments (2)
wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php (1)
202-222: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCompare full origin (scheme + host + port), not host alone.
A source URL with a different scheme or port than
home_url()still passes this check, since onlyPHP_URL_HOSTis compared.🔒 Suggested fix
private static function is_cross_origin(): bool { $source = ''; if ( ! empty( $_SERVER['HTTP_ORIGIN'] ) ) { $source = (string) wp_unslash( $_SERVER['HTTP_ORIGIN'] ); } elseif ( ! empty( $_SERVER['HTTP_REFERER'] ) ) { $source = (string) wp_unslash( $_SERVER['HTTP_REFERER'] ); } if ( '' === $source ) { return false; } - $source_host = wp_parse_url( $source, PHP_URL_HOST ); - $site_host = wp_parse_url( home_url(), PHP_URL_HOST ); - - if ( ! is_string( $source_host ) || ! is_string( $site_host ) ) { - return true; - } - - return strtolower( $source_host ) !== strtolower( $site_host ); + $source_url = wp_parse_url( $source ); + $site_url = wp_parse_url( home_url() ); + + if ( ! is_array( $source_url ) || ! is_array( $site_url ) ) { + return true; + } + + $source_scheme = isset( $source_url['scheme'] ) ? strtolower( $source_url['scheme'] ) : ''; + $site_scheme = isset( $site_url['scheme'] ) ? strtolower( $site_url['scheme'] ) : ''; + $source_host = isset( $source_url['host'] ) ? strtolower( $source_url['host'] ) : ''; + $site_host = isset( $site_url['host'] ) ? strtolower( $site_url['host'] ) : ''; + + if ( '' === $source_host || '' === $site_host ) { + return true; + } + + $source_port = isset( $source_url['port'] ) ? (int) $source_url['port'] : ( 'https' === $source_scheme ? 443 : 80 ); + $site_port = isset( $site_url['port'] ) ? (int) $site_url['port'] : ( 'https' === $site_scheme ? 443 : 80 ); + + return $source_scheme !== $site_scheme + || $source_host !== $site_host + || $source_port !== $site_port; }As per path instructions, review possible edge cases and unexpected behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php` around lines 202 - 222, Update is_cross_origin() to compare the complete parsed origin of the source URL and home_url(), including scheme, host, and port, rather than comparing only PHP_URL_HOST. Normalize comparisons consistently, preserve the existing true result when either URL cannot be parsed, and ensure differing schemes or ports are treated as cross-origin.Source: Path instructions
wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php (1)
253-272: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFixed-window counters still extend on every hit — same issue as before.
set_transient( $key, $value, $ttl )resets the transient's expiry on every call. Under continuous traffic against the same IP/global key, the window never rolls over — a blocked caller stays blocked indefinitely instead of resetting each minute/hour, and this backs bothis_rate_limited()andcookieless_over_cap()insample_endpoint.php.🛠️ Suggested fix (fixed time-bucket key)
public static function incr_counter( string $key, int $ttl ): int { + $ttl = max( 1, $ttl ); + $key .= '_' . (string) floor( time() / $ttl ); + if ( wp_using_ext_object_cache() ) { wp_cache_add( $key, 0, self::CACHE_GROUP, $ttl ); $new = wp_cache_incr( $key, 1, self::CACHE_GROUP );As per path instructions, review possible edge cases and unexpected behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php` around lines 253 - 272, Update Metering storage::incr_counter so transient counters use a fixed time-bucket key or otherwise preserve the original window expiry instead of resetting the TTL on every increment. Ensure repeated hits to the same counter roll over at the configured fixed window, while retaining the existing object-cache increment path and return-value behavior used by is_rate_limited() and cookieless_over_cap().Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress/wp-content/plugins/memberful-wp/views/metering/settings.php`:
- Around line 69-75: Update the metering settings output to use late escaping:
replace the current printf/wp_kses_post pattern with sprintf applied to the
translated message and URL, then pass the complete result to wp_kses_post before
echoing it. Preserve the existing translation string, translator comment, and
memberful_wp_plugin_global_marketing_url() value.
---
Outside diff comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/access.php`:
- Around line 355-366: Extract the shared
published/publicly-viewable/not-password-protected eligibility logic into an
`is_publicly_readable()` helper, then update `is_releasable_sample()`,
`evaluate_sample()`, and the `record_public_views()` loop to use `!
self::is_publicly_readable( $post )` while preserving their existing behavior.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php`:
- Around line 232-243: Update is_rate_limited so it checks the per-IP counter
immediately after incrementing it and returns true when RATE_PER_IP is exceeded,
before incrementing the operation’s global counter. Only requests still under
the per-IP limit should increment and evaluate global_hits, preserving rate
limiting for both thresholds without allowing one IP to consume the global
bucket.
---
Duplicate comments:
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.php`:
- Around line 202-222: Update is_cross_origin() to compare the complete parsed
origin of the source URL and home_url(), including scheme, host, and port,
rather than comparing only PHP_URL_HOST. Normalize comparisons consistently,
preserve the existing true result when either URL cannot be parsed, and ensure
differing schemes or ports are treated as cross-origin.
In `@wordpress/wp-content/plugins/memberful-wp/src/metering/storage.php`:
- Around line 253-272: Update Metering storage::incr_counter so transient
counters use a fixed time-bucket key or otherwise preserve the original window
expiry instead of resetting the TTL on every increment. Ensure repeated hits to
the same counter roll over at the configured fixed window, while retaining the
existing object-cache increment path and return-value behavior used by
is_rate_limited() and cookieless_over_cap().
🪄 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: Repository: TheCodeCompany/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd979c77-e5f8-4d70-ae21-6c07475a95d1
📒 Files selected for processing (17)
wordpress/wp-content/plugins/memberful-wp/js/metering.test.jswordpress/wp-content/plugins/memberful-wp/js/src/metering.jswordpress/wp-content/plugins/memberful-wp/js/src/paywall-builder.jswordpress/wp-content/plugins/memberful-wp/package.jsonwordpress/wp-content/plugins/memberful-wp/readme.txtwordpress/wp-content/plugins/memberful-wp/src/content_filter.phpwordpress/wp-content/plugins/memberful-wp/src/metering.phpwordpress/wp-content/plugins/memberful-wp/src/metering/access.phpwordpress/wp-content/plugins/memberful-wp/src/metering/sample_endpoint.phpwordpress/wp-content/plugins/memberful-wp/src/metering/storage.phpwordpress/wp-content/plugins/memberful-wp/src/paywall/config.phpwordpress/wp-content/plugins/memberful-wp/src/paywall/preview.phpwordpress/wp-content/plugins/memberful-wp/src/paywall/renderer.phpwordpress/wp-content/plugins/memberful-wp/src/paywall/sanitizer.phpwordpress/wp-content/plugins/memberful-wp/stylesheets/paywall.csswordpress/wp-content/plugins/memberful-wp/views/metering/settings.phpwordpress/wp-content/plugins/memberful-wp/views/paywall/builder-panel.php
💤 Files with no reviewable changes (1)
- wordpress/wp-content/plugins/memberful-wp/src/metering.php
| <?php | ||
| printf( | ||
| /* translators: %s: URL of the paywall / global marketing settings screen. */ | ||
| wp_kses_post( __( 'Metering is on, but no paywall content is configured, so metered posts show a generic default message. <a href="%s">Set up your paywall</a> to customise what non-members see.', 'memberful' ) ), | ||
| esc_url( memberful_wp_plugin_global_marketing_url() ) | ||
| ); | ||
| ?> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Apply late escaping for WordPress VIP compliance.
WordPress VIP coding standards highly recommend "late escaping" when combining translation strings with variables. By escaping the final output of sprintf() rather than just the format string inside printf(), we ensure that the entire output is safely sanitized right before it's echoed out to the screen.
| <?php | |
| printf( | |
| /* translators: %s: URL of the paywall / global marketing settings screen. */ | |
| wp_kses_post( __( 'Metering is on, but no paywall content is configured, so metered posts show a generic default message. <a href="%s">Set up your paywall</a> to customise what non-members see.', 'memberful' ) ), | |
| esc_url( memberful_wp_plugin_global_marketing_url() ) | |
| ); | |
| ?> | |
| echo wp_kses_post( | |
| sprintf( | |
| /* translators: %s: URL of the paywall / global marketing settings screen. */ | |
| __( 'Metering is on, but no paywall content is configured, so metered posts show a generic default message. <a href="%s">Set up your paywall</a> to customise what non-members see.', 'memberful' ), | |
| esc_url( memberful_wp_plugin_global_marketing_url() ) | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress/wp-content/plugins/memberful-wp/views/metering/settings.php` around
lines 69 - 75, Update the metering settings output to use late escaping: replace
the current printf/wp_kses_post pattern with sprintf applied to the translated
message and URL, then pass the complete result to wp_kses_post before echoing
it. Preserve the existing translation string, translator comment, and
memberful_wp_plugin_global_marketing_url() value.
Source: Path instructions
Merge resolution reviewHead The conflict resolution is sound. All seven conflicted files kept both sides, and the two places where the branches genuinely collided (server-rendered vs. cache-safe client-rendered) were reasoned through rather than resolved mechanically. The findings below are mostly consequences of those adaptations, not dropped code. Everything below was reproduced on a live install with metering enabled, Already verified, no need to repeat
In scope for this PR1. Free-registration upsell and free-view counter never reach protected metered posts
The base branch gated the upsell on
Same site, same anonymous request, registered limit above anonymous limit:
Fix: add 2. Countdown block never appears on a released protected sample
On a protected post the only countdown element sits inside So Fix: scope the lookup to the released container instead of 3. Countdown templates render as literal tags for anonymous visitors
The reader sees the angle brackets. This pre-existed on the head branch for Fix: 4. Counter fields are not gated, and the checkbox is styled as a heading
The base branch introduced In the same block the bare Separately, Fix: wrap both fields in 5. Cosmetic and changelog
Not this PR, worth separate ticketsContrast helper picks the less legible option in a band of colours. Every brand colour with luminance in Free-registration button cannot be previewed. Button shape fallback mismatch. Merge risk ahead, worth reading before this branch goes to
|
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
| public static function incr_counter( string $key, int $ttl ): int { | ||
| if ( wp_using_ext_object_cache() ) { | ||
| wp_cache_add( $key, 0, self::CACHE_GROUP, $ttl ); | ||
| $new = wp_cache_incr( $key, 1, self::CACHE_GROUP ); | ||
| if ( is_int( $new ) ) { | ||
| return $new; | ||
| } | ||
| // Object cache present but without a working incr: fall through to the transient counter. A given backend is | ||
| // consistent about incr support, so reads and writes stay on one store (read_counter no longer exists to split | ||
| // them). This never returns a constant that would disable enforcement. | ||
| } | ||
|
|
||
| // Transient counter: atomic where transients are backed by a persistent object cache; best-effort (approximate | ||
| // under heavy concurrency) on hosts with no object cache, where WordPress exposes no atomic increment primitive. | ||
| // Acceptable for a soft meter — the per-subject ledger cap still bounds each individual visitor regardless. | ||
| $value = (int) get_transient( $key ) + 1; | ||
| set_transient( $key, $value, $ttl ); |
There was a problem hiding this comment.
@av3nger this seems to be a legit issue. Refreshing the TTL on every increment, and over-limit IPs still bump the global counter. Once the global sample bucket passes 600/min it never expires while traffic continues, so all anonymous visitors may get 429 and no protected content is released until the endpoint sees a full minute of no requests.
| add_filter("rss_enclosure", "__return_empty_string"); | ||
|
|
||
| $memberful_marketing_content = memberful_marketing_content( $post->ID ); | ||
| $rendered_marketing_content = memberful_wp_resolve_paywall_content( (int) $post->ID ); |
There was a problem hiding this comment.
@av3nger Default paywall message is over-scoped. It's in the general gate, so it applies to every protected post, not just metered ones. Existing teaser-only paywalls will gain an unexpected "Subscribe · Sign in" block on upgrade. The changelog already says it's scoped to metered posts.
Suggested fix: gate it on the metering render modes, or keep it broad and update the changelog.
| /* --------------------------------------------------------- | ||
| Metering runtime (cache-safe front-end gate) | ||
|
|
||
| Free posts keep their body in the cached HTML and toggle to the paywall client-side once the meter trips. Protected |
There was a problem hiding this comment.
@av3nger let's remove any comments like this on frontend facing files please, just so we don't give away all our secrets 😄
Also, if metering.css doesn't load the article stays visible with the paywall stacked under it (I confirmed this on my local test). Could we use the hidden attribute like the protected path does?
2 participants
Note
High Risk
Large change to paywall/metering enforcement, anonymous identity, and content release over ajax; mistakes could leak protected content or miscount views under cache and concurrency.
Overview
Anonymous metered pages are now edge-cacheable by serving the same HTML to every logged-out visitor and enforcing limits in the browser plus an uncacheable
admin-ajaxpath, instead of baking per-visitor counts into PHP responses (the WP Engine / full-page cache failure mode).Storage and enforcement split: anonymous view counts move from a signed cookie payload to a server-side transient ledger keyed by an opaque, HttpOnly subject cookie minted only on ajax (not on cacheable page views). Public metered posts use localStorage for immediate enforcement with background
record_publicsync; protected samples stay gated in cached HTML until thesampleop releases full body HTML from the server ledger. Login-time anonymous cookie merge is removed; anonymous and registered allowances stay independent per the readme.New surface area:
metering.jsruntime with prepaint anti-flash CSS, countdown block client hydration, default paywall when marketing is empty, optional paywall free-view counter ({limit}), rate limits / cookieless-release cap on the sample endpoint, and Node tests for the runtime sync behavior. Admin copy now describes cache-safe behavior for anonymous visitors.Reviewed by Cursor Bugbot for commit 01ab0e3. Bugbot is set up for automated code reviews on this repo. Configure here.