Secure “Content Unlocking” Flow (Server-Verified Login Tokens) #93
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughClient JS defers finalization until PayButton close: onSuccess stores tx data, onClose validates server-side (with one retry) to obtain tokens and then completes login or unlock. Server adds token-backed validation endpoints and a Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant PayButton as PayButton Widget
participant Server
participant DB
rect rgb(200,220,255)
Note over Browser,Server: Login flow (high level)
Browser->>PayButton: User initiates login
PayButton->>Browser: onSuccess(tx) — store tx locally
Browser->>PayButton: onClose
Browser->>Server: POST validate_login_tx (wallet, tx_hash)
Server->>DB: find unused login row
alt success
DB-->>Server: login_token
Server-->>Browser: login_token
Browser->>Server: POST save_address (address, tx_hash, login_token)
else failure
Browser->>Browser: wait 3s then retry once
Browser->>Server: validate_login_tx (retry)
alt retry success
Server-->>Browser: login_token
Browser->>Server: POST save_address (address, tx_hash, login_token)
else
Browser-->>Browser: alert login failed
end
end
end
sequenceDiagram
participant Browser
participant PayButton as PayButton Widget
participant Server
participant DB
rect rgb(220,255,220)
Note over Browser,Server: Unlock/payment flow (high level)
Browser->>PayButton: User completes payment
PayButton->>Browser: onSuccess(tx) — store tx locally
Browser->>PayButton: onClose
Browser->>Server: POST validate_unlock_tx (wallet, post_id, tx_hash)
Server->>DB: find unused unlock row
alt success
DB-->>Server: unlock_token
Server-->>Browser: unlock_token
Browser->>Server: POST fetch_unlocked_content (post_id, unlock_token)
Server-->>Browser: unlocked content (update DOM, update URL `#unlocked` + cache-bust)
else failure
Browser->>Browser: wait 3s then retry once
Browser->>Server: validate_unlock_tx (retry)
alt retry success
Server-->>Browser: unlock_token
Browser->>Server: POST fetch_unlocked_content (post_id, unlock_token)
else
Browser-->>Browser: alert unlock failed
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![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
🧹 Nitpick comments (7)
assets/js/paybutton-paywall-cashtab-login.js (2)
77-99: Missing error handler for network failures.
jQuery.postcan fail due to network issues, but only the success callback is provided. If the request fails at the network level (timeout, connection error), the user receives no feedback.Consider adding a
.fail()handler:jQuery.post( PaywallAjax.ajaxUrl, { action: 'validate_login_tx', security: PaywallAjax.nonce, wallet_address: addrCopy, tx_hash: hashCopy }, function (resp) { if (resp && resp.success && resp.data && resp.data.login_token) { handleLogin(addrCopy, hashCopy, resp.data.login_token); } else { if (attempt === 1) { setTimeout(() => tryValidateLogin(2), 3000); } else { alert('⚠️ Login failed: Invalid or expired transaction.'); } } } - ); + ).fail(function() { + if (attempt === 1) { + setTimeout(() => tryValidateLogin(2), 3000); + } else { + alert('⚠️ Login failed: Network error. Please try again.'); + } + });
10-26: Missing error handling in handleLogin AJAX call.If
paybutton_save_addressfails (e.g., blacklisted address, network error), the user is not notified—the page simply doesn't reload.jQuery.post( PaywallAjax.ajaxUrl, { action: 'paybutton_save_address', security: PaywallAjax.nonce, address: address, tx_hash: txHash, login_token: loginToken }, function() { var baseUrl = location.href.split('?')[0]; var newUrl = baseUrl + '?t=' + Date.now(); window.history.replaceState(null, '', newUrl); location.reload(); } - ); + ).fail(function() { + alert('⚠️ Login could not be saved. Please try again.'); + });includes/class-paybutton-ajax.php (1)
241-260: Missingreturnafterwp_send_json_errorcalls.While
wp_send_json_errorcallswp_die()internally which terminates execution, the missing explicitreturnstatements (lines 243, 260) make the control flow less clear and could cause issues if WordPress behavior changes.if (!$address || !$tx_hash || !$login_token) { wp_send_json_error(['message' => 'Missing address, tx_hash, or login_token']); + return; } ... if (!$row) { wp_send_json_error(['message' => 'No validated login found for this token']); + return; }assets/js/paywalled-content.js (2)
108-148: Missing error handlers for nested AJAX calls.Similar to the login flow, network failures in
validate_unlock_txandmark_payment_successfuldon't notify the user. Themark_payment_successfulcall (lines 121-138) has no error handling at all.jQuery.ajax({ method: 'POST', url: PaywallAjax.ajaxUrl, data: { action: 'mark_payment_successful', ... }, success: function () { fetchUnlocked(); - } + }, + error: function () { + alert('⚠️ Could not confirm payment. Please refresh the page.'); + } });Also consider adding
.fail()to the outerjQuery.postforvalidate_unlock_tx.
42-73: No error handling in fetchUnlocked.If
fetch_unlocked_contentfails, the user sees no feedback and the paywall UI may remain in an inconsistent state.function fetchUnlocked() { jQuery.ajax({ method: 'POST', url: PaywallAjax.ajaxUrl, data: { action: 'fetch_unlocked_content', post_id: configData.postId, security: PaywallAjax.nonce }, success: function (resp) { if (resp && resp.success) { // ... existing success logic + } else { + console.error('Failed to fetch unlocked content'); } - } + }, + error: function () { + console.error('Network error fetching unlocked content'); + } }); }paybutton.php (1)
48-52: Consider caching the schema version to avoid repeated dbDelta calls.Running
create_tables()on everyplugins_loadedhook meansdbDelta()executes on every page load. WhiledbDeltais designed to be idempotent, it still performs schema comparisons which adds overhead.Consider storing a schema version in options and only running migrations when needed:
add_action( 'plugins_loaded', function() { $current_version = '4.0.0'; // or define as constant $stored_version = get_option('paybutton_db_version', '0'); if ( version_compare($stored_version, $current_version, '<') ) { if ( class_exists( 'PayButton_Activator' ) ) { PayButton_Activator::create_tables(); } update_option('paybutton_db_version', $current_version); } // ... rest of initialization }, 1);includes/class-paybutton-activator.php (1)
51-73: Inconsistent timestamp storage between tables.
paybutton_paywall_unlockedusesDATETIMEfortx_timestamp(line 51), whilepaybutton_loginsusesINT(11)(line 73). This inconsistency could cause confusion and require different handling in queries.Consider standardizing on one format.
INT(11)(Unix timestamp) is more compact and easier to compare, whileDATETIMEis more readable in database inspection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assets/js/paybutton-paywall-cashtab-login.js(4 hunks)assets/js/paywalled-content.js(1 hunks)includes/class-paybutton-activator.php(2 hunks)includes/class-paybutton-ajax.php(8 hunks)paybutton.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
includes/class-paybutton-ajax.php (1)
includes/class-paybutton-state.php (3)
set_address(97-134)add_article(187-227)get_address(139-150)
assets/js/paybutton-paywall-cashtab-login.js (2)
assets/js/paybutton-generator.js (1)
address(21-21)assets/js/paywalled-content.js (2)
addrCopy(102-102)hashCopy(103-103)
assets/js/paywalled-content.js (1)
assets/js/paybutton-paywall-cashtab-login.js (2)
addrCopy(74-74)hashCopy(75-75)
🪛 PHPMD (2.15.0)
includes/class-paybutton-ajax.php
306-306: Avoid unused local variables such as '$mysql_timestamp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (4)
includes/class-paybutton-ajax.php (1)
502-548: LGTM on token generation security.The use of
random_bytes(18)for token generation provides cryptographically secure randomness (~144 bits of entropy), making tokens unguessable. The base64url encoding is correctly implemented.assets/js/paywalled-content.js (1)
89-97: Good defensive handling of transaction data.The code safely handles potentially missing properties with fallbacks (
tx.inputAddresses && tx.inputAddresses.length > 0,tx.hash || '', etc.). This prevents runtime errors if the PayButton library returns incomplete data.includes/class-paybutton-activator.php (2)
65-84: New paybutton_logins table schema looks well-designed.The table includes appropriate indices for the common query patterns (lookup by
wallet_address + tx_hash, filtering byused, and token lookup). Thecreated_attimestamp with default is useful for auditing.
50-51: DECIMAL(20,2) is correct for XEC transactions.eCash (XEC) uses 2 decimal places, so the schema change from
DECIMAL(20,8)toDECIMAL(20,2)aligns perfectly with actual transaction precision requirements. No data loss or precision issues will occur.
There was a problem hiding this comment.
Pull request overview
This PR implements server-side verification for content unlocking and login flows, replacing the previous client-trust model with a secure token-based system. The PayButton server now validates transaction signatures before granting access, preventing malicious actors from bypassing paywalls through client-side manipulation.
Key changes:
- Introduced secure unlock/login tokens generated server-side after transaction verification
- Added validation endpoints that verify transactions before issuing one-time-use tokens
- Implemented retry logic with automatic fallback for transaction validation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| paybutton.php | Added automatic table creation on plugin load to ensure schema updates are applied |
| includes/class-paybutton-ajax.php | Implemented secure validation endpoints for login and unlock transactions with token generation and single-use enforcement |
| includes/class-paybutton-activator.php | Updated database schema to include token fields and usage tracking for both login and unlock tables |
| assets/js/paywalled-content.js | Refactored unlock flow to validate transactions server-side before marking payment successful |
| assets/js/paybutton-paywall-cashtab-login.js | Updated login flow to validate transactions and obtain server-issued tokens before setting login state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
includes/class-paybutton-ajax.php (3)
152-171: Debug logging statements should be removed or made conditional.The
error_logstatements on lines 152, 167-171 are still present. These should be removed or wrapped in aWP_DEBUGcheck before production.
510-537: TOCTOU race condition remains unaddressed.The SELECT-then-UPDATE pattern allows two concurrent requests to both pass the
used = 0check before either marks the row as used. The previous review suggested an atomicUPDATE ... WHERE used = 0pattern.
565-596: Same TOCTOU race condition asajax_validate_login_tx.This function has the identical SELECT-then-UPDATE race condition. Consider applying the atomic UPDATE pattern suggested in the previous review to both handlers.
🧹 Nitpick comments (5)
includes/class-paybutton-ajax.php (5)
240-259: Add explicitreturnstatements afterwp_send_json_errorcalls.While
wp_send_json_error()callsdie()internally, adding explicitreturnstatements improves code clarity and prevents potential issues if the function is mocked in tests or its behavior changes.if (!$address || !$tx_hash || !$login_token) { wp_send_json_error(['message' => 'Missing address, tx_hash, or login_token']); + return; } ... if (!$row) { wp_send_json_error(['message' => 'No validated login found for this token']); + return; }
263-263: Use strict comparison for consistency and safety.This
in_arraycall lacks the strict comparison flag, while the same check on line 334 usestrue. Use strict comparison consistently to prevent type juggling issues.- if ( in_array( $address, $blacklist ) ) { + if ( in_array( $address, $blacklist, true ) ) {
301-325: Add explicitreturnstatements afterwp_send_json_errorcalls.For consistency and defensive coding, add
returnstatements after the error responses on lines 303 and 325.if ( $post_id <= 0 || empty( $tx_hash ) || empty( $user_address ) || empty( $unlock_token ) ) { wp_send_json_error( array( 'message' => 'Missing required payment fields.' ), 400 ); + return; } ... if ( ! $row ) { wp_send_json_error( array( 'message' => 'No validated unlock found for this token.' ), 403 ); + return; }
502-519: Add explicitreturnstatements afterwp_send_json_errorcalls.Same pattern as other handlers—add
returnafter error responses on lines 504 and 519 for consistency.if (empty($wallet_address) || empty($tx_hash)) { wp_send_json_error('Missing data'); + return; } ... if (!$row) { wp_send_json_error('Login validation failed'); + return; }
557-580: Add explicitreturnstatements afterwp_send_json_errorcalls.Add
returnafter error responses on lines 559 and 580 for consistency with best practices.if (empty($wallet_address) || empty($tx_hash) || $post_id <= 0) { wp_send_json_error('Missing data'); + return; } ... if (!$row) { wp_send_json_error('Unlock validation failed'); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/class-paybutton-ajax.php(8 hunks)
🔇 Additional comments (2)
includes/class-paybutton-ajax.php (2)
46-52: LGTM!The new AJAX endpoint registrations follow the established pattern and correctly register both
wp_ajax_andwp_ajax_nopriv_hooks for the token validation handlers.
490-601: Security architecture for token-based validation is well-designed.The token generation uses
random_bytes(18)which provides 144 bits of entropy—sufficient for unguessable tokens. The validation chain (signature verification → row insertion → token generation → token consumption) correctly prevents client-side forgery once the race condition is addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
includes/class-paybutton-ajax.php (3)
152-171: Debug logging statements should be removed or made conditional.The
error_logcalls on lines 152, 167-171 are still present despite being flagged in a previous review. These should be removed or wrapped in aWP_DEBUGcheck before production.- error_log('[paybutton] login-branch addr=' . $user_address . ' tx=' . $tx_hash . ' ts=' . $tx_timestamp); if ( ! $exists ) { // insert logic... } - if ($wpdb->last_error) { - error_log('[paybutton] insert error: ' . $wpdb->last_error); - } else { - error_log('[paybutton] insert ok id=' . $wpdb->insert_id); - } + // Optionally log errors in debug mode + if ( defined('WP_DEBUG') && WP_DEBUG && $wpdb->last_error ) { + error_log('[paybutton] insert error: ' . $wpdb->last_error); + }
510-537: TOCTOU race condition between SELECT and UPDATE.As flagged in a previous review, two concurrent requests with the same
wallet_addressandtx_hashcould both pass theused = 0check before either marks the row asused = 1. This could result in duplicate tokens being issued.Use an atomic UPDATE that returns affected rows:
- $row = $wpdb->get_row($wpdb->prepare( - "SELECT id FROM {$table} - WHERE wallet_address = %s AND tx_hash = %s AND used = 0 - ORDER BY id DESC LIMIT 1", - $wallet_address, $tx_hash - )); - - if (!$row) { - wp_send_json_error('Login validation failed'); - } - $raw = random_bytes(18); $token = rtrim(strtr(base64_encode($raw), '+/', '-_'), '='); - $wpdb->update( - $table, - array( - 'used' => 1, - 'login_token' => $token, - ), - array('id' => (int)$row->id), - array('%d','%s'), - array('%d') - ); + // Atomic update: only succeeds if row exists AND used = 0 + $updated = $wpdb->query($wpdb->prepare( + "UPDATE {$table} + SET used = 1, login_token = %s + WHERE wallet_address = %s AND tx_hash = %s AND used = 0 + LIMIT 1", + $token, $wallet_address, $tx_hash + )); + + if (!$updated) { + wp_send_json_error('Login validation failed'); + }
565-596: Same TOCTOU race condition applies here.The same SELECT-then-UPDATE pattern creates a race condition. Apply the same atomic UPDATE fix as suggested for
ajax_validate_login_tx.- $row = $wpdb->get_row($wpdb->prepare( - "SELECT id FROM {$table} - WHERE pb_paywall_user_wallet_address = %s - AND tx_hash = %s - AND post_id = %d - AND used = 0 - ORDER BY id DESC - LIMIT 1", - $wallet_address, $tx_hash, $post_id - )); - - if (!$row) { - wp_send_json_error('Unlock validation failed'); - } - $raw = random_bytes(18); $token = rtrim(strtr(base64_encode($raw), '+/', '-_'), '='); - $wpdb->update( - $table, - array( - 'used' => 1, - 'unlock_token' => $token, - ), - array( 'id' => (int) $row->id ), - array( '%d', '%s' ), - array( '%d' ) - ); + $updated = $wpdb->query($wpdb->prepare( + "UPDATE {$table} + SET used = 1, unlock_token = %s + WHERE pb_paywall_user_wallet_address = %s + AND tx_hash = %s + AND post_id = %d + AND used = 0 + LIMIT 1", + $token, $wallet_address, $tx_hash, $post_id + )); + + if (!$updated) { + wp_send_json_error('Unlock validation failed'); + }
🧹 Nitpick comments (4)
assets/js/paywalled-content.js (2)
41-73: Consider adding error handling for content fetch failures.The
fetchUnlockedhelper silently ignores AJAX failures. If the content fetch fails after a successful payment validation, the user sees no feedback. Consider adding an error callback to inform the user.success: function (resp) { if (resp && resp.success) { // ... existing success logic } - } + }, + error: function () { + alert('⚠️ Failed to load unlocked content. Please refresh the page.'); + } });
66-67: Consider extracting duplicatedheaderOffsetconstant.The magic number
80for header offset appears in bothfetchUnlocked(line 66) and the page-load scroll handler (line 170). Consider extracting to a named constant for maintainability.// At the top of the file or within the document.ready scope: var HEADER_OFFSET = 80;Also applies to: 170-171
includes/class-paybutton-ajax.php (2)
240-259: Consider adding explicitreturnafterwp_send_json_errorcalls for clarity.While
wp_send_json_errorcallswp_die()internally, adding explicitreturnstatements improves code readability and makes control flow obvious.if (!$address || !$tx_hash || !$login_token) { wp_send_json_error(['message' => 'Missing address, tx_hash, or login_token']); + return; } // ... if (!$row) { wp_send_json_error(['message' => 'No validated login found for this token']); + return; }
100-100: Nested fallback is functional but could be clearer.The one-liner handles both array and string formats, which is correct. The past review suggestion to break this into explicit conditionals would improve readability but isn't essential.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/js/paywalled-content.js(1 hunks)includes/class-paybutton-ajax.php(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
includes/class-paybutton-ajax.php (1)
includes/class-paybutton-state.php (4)
PayButton_State(4-279)set_address(97-134)add_article(187-227)get_address(139-150)
assets/js/paywalled-content.js (1)
assets/js/paybutton-paywall-cashtab-login.js (2)
addrCopy(74-74)hashCopy(75-75)
🔇 Additional comments (5)
assets/js/paywalled-content.js (3)
37-39: Shared state comment updated for clarity.The comment on line 37 now correctly says "address" instead of the previous abbreviation. The shared state pattern using closures is appropriate for capturing transaction data across callbacks.
89-98: LGTM — transaction data capture is defensive and correct.The
onSuccesscallback correctly captures transaction data with appropriate fallbacks for missing fields. Deferring server validation toonClosealigns with the Cashtab login pattern referenced in the related snippets.
154-157: State clearing is correctly synchronized with async flow.The shared state is safely cleared after copies are made, preventing stale data from persisting across multiple unlock attempts.
includes/class-paybutton-ajax.php (2)
46-52: LGTM — New validation endpoints correctly registered.Both
validate_login_txandvalidate_unlock_txendpoints are properly registered for both logged-in and non-logged-in users, consistent with the plugin's pay-to-login model.
301-325: Token validation correctly enforces server-verified unlocks.The logic properly validates that the
unlock_tokenmatches a row marked asused = 1(meaningajax_validate_unlock_txalready validated it). This closes the bypass vulnerability described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
includes/class-paybutton-ajax.php (2)
510-537: TOCTOU race condition (duplicate of previous review).The SELECT-then-UPDATE pattern creates a race condition between concurrent requests. This was already flagged in a previous review with a suggested atomic UPDATE fix.
565-596: Same TOCTOU pattern applies here.As noted in the previous review comment on
ajax_validate_login_tx, the same atomic UPDATE fix should be applied to this function to prevent race conditions.
🧹 Nitpick comments (1)
includes/class-paybutton-ajax.php (1)
240-259: Consider adding explicitreturnafterwp_send_json_error()for consistency.While
wp_send_json_error()callsdie()internally, other error paths in this file (lines 80, 106, 265) include explicitreturnstatements. Adding them here improves code clarity and makes the control flow explicit.if (!$address || !$tx_hash || !$login_token) { wp_send_json_error(['message' => 'Missing address, tx_hash, or login_token']); + return; } ... if (!$row) { wp_send_json_error(['message' => 'No validated login found for this token']); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/class-paybutton-ajax.php(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
includes/class-paybutton-ajax.php (1)
includes/class-paybutton-state.php (4)
PayButton_State(4-279)set_address(97-134)add_article(187-227)get_address(139-150)
🔇 Additional comments (5)
includes/class-paybutton-ajax.php (5)
46-53: LGTM!The new AJAX endpoints are correctly registered for both authenticated and unauthenticated WordPress users, consistent with the existing pattern and necessary for the pay-to-login flow.
138-175: Solid login flow handling with proper idempotency.The detection of login transactions via
rawMessage, field validation, and idempotent insert pattern correctly prevent duplicates. The blacklist check is appropriately deferred tosave_address()where the login is finalized.
301-336: Token-based validation correctly secures the unlock flow.The validation against
used = 1and matchingunlock_tokenensures only server-verified transactions can complete the unlock. Same note as above applies regarding explicitreturnstatements after error responses (lines 303, 325, 336) for consistency.
524-525: Secure token generation approach.Using
random_bytes(18)with base64url encoding produces a cryptographically secure, unguessable 24-character token that effectively prevents replay attacks.
550-601: Unlock validation logic is correct.The three-part key (wallet + tx_hash + post_id) correctly identifies the specific unlock transaction, and the token generation mirrors the secure pattern used in login validation.
|
Ready to merge. Rebase on master before merging. |
This PR completes #82 (content unlocking part) by replacing the old “trust the browser/client” paywall logic with a fully server-verified unlock flow, similar to the new Cashtab login system (#90). Previously, a bad actor could unlock paywalled content by faking the mark_payment_successful / fetch_unlocked_content AJAX calls in the browser without ever sending a valid, signed payment. This change closes that gap and ensures unlock state is only granted based on data coming from the PayButton server, not just whatever the front-end/client claims.
Now, when a user pays to unlock a post, the PayButton server verifies the transaction signature and records an unlock row in paybutton_paywall_unlocked, attaching a one-time random token. The browser must then return that token (together with the tx hash and wallet address) when asking the server to confirm the unlock and fetch the content. Because the token is random, single-use, and only ever created after a real signed transaction, it’s no longer possible to fake or replay unlocks using only client-side tricks. This makes paywalled content unlocking reliable, tamper-resistant, and aligned with the secure Cashtab login flow.
Test Plan
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.