Tokenize export URLs #456
Tokenize export URLs #456
Conversation
WalkthroughJS was refactored to a state-driven, multi-step export flow with token persistence, cancellation, and download UI; PHP now generates and stores a 32-character token in a transient for completed exports, accepts token-based download requests, and deletes transients on cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Admin UI
participant JS as admin-export.js
participant AJAX as WP AJAX
participant PHP as csv-export.php
User->>UI: Submit export form
UI->>JS: onSubmit -> serialize params
JS->>AJAX: POST ajax_csv_export (initial)
AJAX->>PHP: ajax_csv_export handler
PHP-->>AJAX: { state, isDone?, token? }
AJAX-->>JS: response
alt isDone == false
JS->>JS: advanceExport(state) -> update UI
JS->>AJAX: POST ajax_csv_export (next step with state)
AJAX->>PHP: next step
PHP-->>AJAX: { state, ... }
AJAX-->>JS: response -> loop
else isDone == true
PHP->>PHP: set_transient("wpbdp_export_{token}", state)
PHP-->>AJAX: { isDone:true, token }
AJAX-->>JS: response -> show download UI
User->>UI: Click download
UI->>AJAX: GET ajax_csv_download?token={token}
AJAX->>PHP: ajax_csv_download -> lookup transient
PHP-->>AJAX: file stream or error
AJAX-->>User: download or error
end
opt cleanup/cancel
User->>UI: Click cleanup/cancel
UI->>JS: send cleanup with existing_token
JS->>AJAX: POST ajax_csv_export (cleanup, existing_token)
AJAX->>PHP: cleanup -> delete_transient("wpbdp_export_{token}")
PHP-->>AJAX: cleanup response
AJAX-->>JS: response -> update UI cancelled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (5)
includes/admin/csv-export.php (2)
89-90: Return token in all progress responses, not only when doneThis lets the frontend include existing_token in error/cancel cleanup earlier.
- $response['token'] = $state ? ( $state['done'] ? $state['token'] : '' ) : ''; + $response['token'] = $state ? $state['token'] : '';
169-175: Transient lifecycle: consider auto‑deleting on successful downloadTo avoid token reuse/stale transients, delete it once streaming begins.
- // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_readfile + // Optionally consume token to prevent reuse + if ( ! empty( $state['token'] ) ) { + delete_transient( 'wpbdp_export_' . $state['token'] ); + } + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_readfile readfile( $file_path );Note: Keep the manual “Cleanup” flow if repeat downloads are expected.
Also applies to: 120-151
assets/js/admin-export.js (3)
50-61: Include existing_token in cancel cleanupIf a token already exists, clear the transient too when canceling.
$.ajax( ajaxurl, { data: { action: 'wpbdp-csv-export', nonce: wpbdp_global.nonce, - state, + state, + existing_token: existingToken || '', cleanup: 1, }, type: 'POST',
162-179: Remove debug log and use reliable reload; add error handlingAvoid leaking to console and handle failures gracefully.
- console.log( existingToken ); $.ajax( ajaxurl, { data: { action: 'wpbdp-csv-export', nonce: wpbdp_global.nonce, state: lastState, existing_token: existingToken, cleanup: 1, }, type: 'POST', dataType: 'json', success( res ) { - location.href = ''; + window.location.reload(); }, + error() { + // Keep UI visible and show error banner + $( '.wpbdp-page-csv-export div.error' ).show(); + }, } );
153-157: Naming nit: “cancel-import” selectorIf this is indeed the export flow, consider renaming the class to “cancel-export” for clarity and future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
assets/js/admin-csv-import.min.jsis excluded by!**/*.min.js,!**/*.min.jsassets/js/admin-export.min.jsis excluded by!**/*.min.js,!**/*.min.jsassets/js/onboarding-wizard.min.jsis excluded by!**/*.min.js,!**/*.min.js
📒 Files selected for processing (2)
assets/js/admin-export.js(1 hunks)includes/admin/csv-export.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
assets/js/admin-export.js (1)
assets/js/admin-export.min.js (1)
progressBar(1-1)
includes/admin/csv-export.php (1)
includes/utils.php (1)
wpbdp_get_var(570-591)
🔇 Additional comments (1)
includes/admin/csv-export.php (1)
75-77: Token generation looks goodRandom 32-char, no special chars. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
includes/admin/csv-export.php (2)
61-64: Validate existing_token before deleting transient.The code accepts
existing_tokenfrom the request without format validation before using it indelete_transient. This should be restricted to 32-character alphanumeric tokens matching the generation pattern.Apply this diff to validate the token:
$existing_token = wpbdp_get_var( array( 'param' => 'existing_token' ), 'request' ); - if ( $existing_token ) { + if ( $existing_token && preg_match( '/^[A-Za-z0-9]{32}$/', $existing_token ) ) { delete_transient( 'wpbdp_export_' . $existing_token ); }
107-114: Validate token format and use strict base64 decoding.The code accepts any token value and decodes state without strict validation. Token should match the 32-character alphanumeric pattern, and
base64_decodeshould use strict mode.Apply this diff:
$token = wpbdp_get_var( array( 'param' => 'token' ), 'request' ); + if ( $token && ! preg_match( '/^[A-Za-z0-9]{32}$/', $token ) ) { + wp_die( esc_html__( 'Invalid token format.', 'business-directory-plugin' ) ); + } + $state_param = wpbdp_get_var( array( 'param' => 'state' ), 'request' ); if ( ! $token && ! $state_param ) { wp_die( esc_html__( 'Invalid download request.', 'business-directory-plugin' ) ); } - $state = $token ? get_transient( 'wpbdp_export_' . $token ) : json_decode( base64_decode( $state_param ), true ); + $state = $token ? get_transient( 'wpbdp_export_' . $token ) : json_decode( base64_decode( $state_param, true ), true );
🧹 Nitpick comments (1)
includes/admin/csv-export.php (1)
167-169: Add defensive check for token existence.If
$state['token']is missing,$tokenwill be undefined, leading to an invalid transient key. Add a guard to ensure the token exists.Apply this diff:
private function get_download_url( $state ) { + if ( empty( $state['token'] ) ) { + return ''; + } + $token = $state['token']; set_transient( 'wpbdp_export_' . $token, $state, HOUR_IN_SECONDS );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/admin/csv-export.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
includes/admin/csv-export.php (1)
includes/utils.php (1)
wpbdp_get_var(570-591)
🔇 Additional comments (1)
includes/admin/csv-export.php (1)
75-77: LGTM: Token generation logic is sound.The conditional generation ensures tokens persist across export steps while
wp_generate_password(32, false)provides sufficient entropy for secure token creation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
assets/js/admin-export.js (1)
12-35: Critical: Undefined variablestatein cleanup payload.Line 20 uses an undefined variable
state(ES6 property shorthand forstate: state). ThehandleErrorfunction is defined at module scope and only receives(msg, res)parameters—it has no access to astatevariable in its scope. This will cause aReferenceErrorat runtime when the cleanup AJAX call is attempted.The fix should reference
res.statewith a fallback tolastState, and should includeexisting_tokenwhen available for consistency with the cleanup pattern used elsewhere (line 169).Apply this diff to fix the issue:
const handleError = function ( msg, res ) { if ( msg ) $( '.wpbdp-page-csv-export div.error p' ).text( msg ); - if ( res && res.state ) { + const cleanupState = res && res.state ? res.state : lastState; + if ( cleanupState ) { $.ajax( ajaxurl, { data: { action: 'wpbdp-csv-export', nonce: wpbdp_global.nonce, - state, + state: cleanupState, + existing_token: existingToken || '', cleanup: 1, }, type: 'POST', + dataType: 'json', } ); }
🧹 Nitpick comments (4)
assets/js/admin-export.js (4)
50-60: Consider includingexisting_tokenin cancellation cleanup for consistency.While
existingTokenwill always benullduring cancellation (since exports can't be cancelled after completion), including it in the cleanup payload would align with the pattern used in the final cleanup (line 169) and provide defensive consistency if the flow changes in the future.$.ajax( ajaxurl, { data: { action: 'wpbdp-csv-export', nonce: wpbdp_global.nonce, state, + existing_token: existingToken || '', cleanup: 1, }, type: 'POST', dataType: 'json', success( res ) {}, } );
113-115: Consider passing error context tohandleError.The error callback on line 114 calls
handleError()with no arguments, providing no error message or response object for cleanup or user feedback. Consider passing an error message and any available response data.error() { - handleError(); + handleError( 'An unexpected error occurred during export.', null ); },
147-149: Consider passing error context tohandleError.Similar to line 114, the error callback calls
handleError()with no arguments. Consider providing an error message for better user feedback.error() { - handleError(); + handleError( 'Failed to initialize export. Please try again.', null ); },
162-178: Event handlers look good; token-based cleanup properly implemented.The cleanup handler correctly sends both
stateandexisting_tokento the server, properly implementing the token-based cleanup flow described in the PR objectives. The page refresh on success ensures a clean state.Optional: Consider more explicit redirect.
Line 175 uses
location.href = ''which relies on the browser defaulting to the current URL. For clarity, considerlocation.reload()or an explicit URL.success( res ) { - location.href = ''; + location.reload(); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/admin-export.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
assets/js/admin-export.js (2)
assets/js/admin-export.min.js (1)
progressBar(1-1)includes/admin/class-admin.php (1)
WPBDP_Admin(26-1704)
🔇 Additional comments (1)
assets/js/admin-export.js (1)
1-10: LGTM! Modern variable declarations and initialization.The module-level variables are properly scoped and will be accessible to all functions within the jQuery closure. The use of
constforprogressBarandletfor mutable state variables follows best practices.
shervElmi
left a comment
shervElmi
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Thanks @sorinmarta.
|
Thanks @shervElmi! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
includes/admin/csv-export.php (2)
61-64: Validate existing_token before deleting transientAccept only 32-char base62; ignore anything else to avoid deleting unintended keys.
- $existing_token = wpbdp_get_var( array( 'param' => 'existing_token' ), 'request' ); - if ( $existing_token ) { - delete_transient( 'wpbdp_export_' . $existing_token ); - } + $existing_token = wpbdp_get_var( array( 'param' => 'existing_token' ), 'request' ); + if ( is_string( $existing_token ) && preg_match( '/^[A-Za-z0-9]{32}$/', $existing_token ) ) { + delete_transient( 'wpbdp_export_' . $existing_token ); + }
138-146: Harden download input: validate token and decode state strictlyValidate token format and use strict base64 decoding with JSON error checks; fail closed on invalid input.
- $token = wpbdp_get_var( array( 'param' => 'token' ), 'request' ); + $token = wpbdp_get_var( array( 'param' => 'token' ), 'request' ); + if ( $token && ! preg_match( '/^[A-Za-z0-9]{32}$/', $token ) ) { + wp_die( esc_html__( 'Invalid token.', 'business-directory-plugin' ) ); + } $state_param = wpbdp_get_var( array( 'param' => 'state' ), 'request' ); - if ( ! $token && ! $state_param ) { + if ( ! $token && ! $state_param ) { wp_die( esc_html__( 'Invalid download request.', 'business-directory-plugin' ) ); } - $state = $token ? get_transient( 'wpbdp_export_' . $token ) : json_decode( base64_decode( $state_param ), true ); + if ( $token ) { + $state = get_transient( 'wpbdp_export_' . $token ); + } else { + $decoded = base64_decode( $state_param, true ); + if ( false === $decoded ) { + wp_die( esc_html__( 'Invalid download request.', 'business-directory-plugin' ) ); + } + $state = json_decode( $decoded, true ); + if ( JSON_ERROR_NONE !== json_last_error() ) { + wp_die( esc_html__( 'Invalid download request.', 'business-directory-plugin' ) ); + } + }Also applies to: 145-149
🧹 Nitpick comments (6)
includes/admin/csv-export.php (3)
91-116: Defensive reads from $state to avoid noticesIf any key is missing (edge cases), default safely.
- $response['state'] = base64_encode( wp_json_encode( $state ) ); - $response['count'] = count( $state['listings'] ); - $response['exported'] = $state['exported']; - $response['filesize'] = size_format( $state['filesize'] ); - $response['isDone'] = $state['done']; + $response['state'] = base64_encode( wp_json_encode( $state ) ); + $response['count'] = isset( $state['listings'] ) ? count( (array) $state['listings'] ) : 0; + $response['exported'] = isset( $state['exported'] ) ? (int) $state['exported'] : 0; + $response['filesize'] = isset( $state['filesize'] ) ? size_format( (int) $state['filesize'] ) : size_format( 0 ); + $response['isDone'] = ! empty( $state['done'] );Also applies to: 119-125
151-159: Option: one-time tokens — delete transient after successful lookupIf you want single-use tokens, delete the transient on download. This reduces storage and replay risk.
$export = WPBDP_CSVExporter::from_state( $state ); $file_path = $export->get_file_path(); $file_url = $export->get_file_url(); + if ( $token ) { + delete_transient( 'wpbdp_export_' . $token ); + }Confirm desired behavior (allow re-download vs one-time use).
197-206: Make token TTL filterableLet site owners adjust token lifetime without code edits.
- set_transient( 'wpbdp_export_' . $token, $state, HOUR_IN_SECONDS ); + $ttl = apply_filters( 'wpbdp_export_token_ttl', HOUR_IN_SECONDS, $state ); + set_transient( 'wpbdp_export_' . $token, $state, $ttl );assets/js/admin-export.js (3)
15-27: Fix unreachable lastState fallback in handleError; send JSON; optionally include existing_tokenCurrent guard requires res.state, so lastState is never used. Also add dataType for consistency and pass existing_token when available.
- if ( res && res.state ) { - const state = res.state ? res.state : lastState; + if ( ( res && res.state ) || lastState ) { + const state = res && res.state ? res.state : lastState; $.ajax( ajaxurl, { data: { action: 'wpbdp-csv-export', nonce: wpbdp_global.nonce, state, cleanup: 1, + // Include token if present (no-op when null). + existing_token: existingToken || undefined, }, type: 'POST', + dataType: 'json', } ); }
91-95: Avoid stripping whitespace from URLsdownload_url/fileurl should be valid; replacing whitespace is unnecessary and could mask encoding issues.
- const downloadUrl = ( - res.download_url || res.fileurl - ).replace( /\s/g, '' ); + const downloadUrl = res.download_url || res.fileurl;
176-178: Use location.reload() for clarityReload intent is clearer and avoids potential edge cases with empty href.
- location.href = ''; + location.reload();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/js/admin-export.js(1 hunks)includes/admin/csv-export.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
includes/admin/csv-export.php (3)
includes/utils.php (1)
wpbdp_get_var(570-591)assets/js/admin-export.js (1)
state(16-16)includes/admin/helpers/csv/class-csv-exporter.php (1)
get_file_url(289-298)
assets/js/admin-export.js (1)
assets/js/admin-export.min.js (1)
progressBar(1-1)
🔇 Additional comments (2)
includes/admin/csv-export.php (1)
75-77: Token generation OK; ensure consumer-side validation matcheswp_generate_password(32, false) yields A–Z, a–z, 0–9. Keep downstream validation (regex length/base62) consistent with this invariant. No change required; just noting the contract.
assets/js/admin-export.js (1)
155-159: No issue found—selector correctly matches export screen markupThe markup verification confirms the export template (csv-export.tpl.php line 188) uses
class="cancel-import", which matches the JavaScript selectora.cancel-importat lines 155-159 in admin-export.js. The handler will fire correctly.Likely an incorrect or invalid review comment.
fixes https://github.com/Strategy11/business-directory-premium/issues/327
This PR changes the way we create download URLs from adding the whole state to the URL to storing the token in a transient and passing the token as an identifier.
Also, while working on that I also applied Prettier to the admin-export.js file to modernize it a little. We could perhaps also refactor the old jQuery code into ES6 but I don't think it would make sense in a small update like this.