Skip to content

Dev/security audit mjhead - #677

Merged
Gawuww merged 11 commits into
release/3.6.5from
dev/security-audit-mjhead
Jul 30, 2026
Merged

Dev/security audit mjhead#677
Gawuww merged 11 commits into
release/3.6.5from
dev/security-audit-mjhead

Conversation

@MjHead

@MjHead MjHead commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@MjHead

MjHead commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jul 1, 2026

Copy link
Copy Markdown

@copilot resolve the merge conflicts in this pull request

Resolved in 4fc6250.

Copilot finished work on behalf of MjHead July 1, 2026 10:49
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 AI PR Review

Risk level: medium

Review

Summary

  • This PR implements several security-focused changes: single-use CSRF tokens (consume), server-side replacement token returned for AJAX continuations, client-side JS refresh of the CSRF hidden field after AJAX response, and a signed post-context signature to validate form-in-context post IDs. It also hardens the validation/parse pipeline for array payloads and fixes empty-value checks so zero-like values are validated correctly. Several unit tests were added (CSRF lifecycle, post-context behavior, validation pipeline). Build asset version hashes updated.

What I checked / notable files

  • modules/security/csrf/csrf-tools.php
    • Added consume() wrapper calling delete(). Good: provides explicit "consume" semantics for single-use tokens.
  • modules/security/csrf/module.php
    • handle_request(): now uses Csrf_Tools::consume() and stores client_id on the module instance.
    • handle_after_send(): signature changed to handle_after_send($handler, bool $is_success). It now generates and adds a replacement CSRF token to AJAX responses and removes the action after handling (to avoid double-run). Good improvement: ensures AJAX continuations get a fresh token.
    • Small nit: remove_action is called inside the handler to avoid re-entrance which is fine; priority is provided correctly.
  • includes/form-handler.php
    • Introduces $post_id_sig_key + get_post_context_signature(), set_current_post_context(), normalization helpers, and signature verification using wp_hash and hash_equals. Good: prevents tampering of context post ID in the POST payload and falls back to referrer-derived post when signature fails.
    • Uses wp_unslash + sanitize_text_field for reading the signature (phpcs suppression used). This is acceptable here because the value is unslashed POST data and is sanitized prior to use.
    • normalize_post_id() and resolve_referrer_post_id() help prevent legacy '-1' values resolving to a real post ID.
  • includes/blocks/render/form-hidden-fields.php
    • Hidden field for the post context signature is rendered. Default post ID changed from -1 to 0. Ensure consumers expecting -1 are considered (see tests added).
  • assets/src/frontend/main/submit/AjaxSubmit.js
    • Adds refreshCsrfToken(response._jfb_csrf_token) so after a successful/failed AJAX the frontend replaces any input[name="_jfb_csrf_token"] values for the same form ID. Good UX/security: continuation requests use fresh token.
    • Implementation iterates form nodes matching data-form-id. Reasonable and performant for common page sizes; if pages have many forms this is linear but acceptable.
  • modules/block-parsers/field-data-parser.php and parsers under modules/block-parsers/fields/
    • Introduces allows_array_value() default false and adds overrides for known array-capable field types (checkbox, select multiple, media, repeater, choices with allow_multiple). Good: prevents scalar fields from accepting arrays (mitigates malformed request attacks).
  • modules/validation/module.php
    • Replaced ! $parser->get_value() with Tools::is_empty() so '0' and '"0"' are not treated as empty. This prevents skipping validations for zero-like values. Good fix.
  • modules/validation/ssr/is-field-value-unique.php
    • Improved handling of nested paths, decoded field attrs, uses prepared SQL properly. New helper methods sanitize/normalize field paths. Good fix for nested repeater uniqueness checks.
  • tests/
    • Added tests for CSRF token lifecycle, FormHandler post-context behavior and Validation pipeline. Good coverage for new logic.

Security review / concerns

  • CSRF field constant consistency: The backend returns the replacement token under Csrf_Tools::FIELD. Frontend expects response._jfb_csrf_token (AjaxSubmit.js reads response?._jfb_csrf_token). Ensure Csrf_Tools::FIELD indeed equals the string "_jfb_csrf_token" (or that responses use the same key). If the constant differs the frontend refresh will not run. Please verify the constant value in Csrf_Tools.
  • Atomicity / race conditions: Csrf_Tools::consume() delegates to delete() which returns deleted rows. The DB-level delete should be atomic enough for single-use semantics. Make sure Csrf_Token_Model::delete() uses proper WHERE token+client and limits to 1 row. (I did not see the model implementation in the diff.) Tests show expected lifecycle passes.
  • Clearing of tokens prior to consume: In Module::handle_request() there is a call to Csrf_Token_Model::clear() (intended to clear old tokens) followed by Csrf_Tools::consume(). Ensure clear() does not remove the token you are about to consume (clear() should only remove expired/old tokens). If clear() deletes all tokens unconditionally that would break consume. I assume clear() targets expirations, but please double-check.
  • Signature entropy and scheme: get_post_context_signature() uses wp_hash( $data, 'nonce' ). That is OK, but document or centralize the hashing key/salt usage. Using wp_hash with 'nonce' is acceptable; hash_equals protects timing attacks.
  • POST reading: set_current_post_context() calls wp_unslash($_POST[ $this->post_id_sig_key ] ?? '' ) and sanitize_text_field. The phpcs suppression for nonce verification is present; this is OK as it's not a WordPress nonce but a custom signature. Make sure any other uses of raw POST are similarly sanitized.

Backward compatibility

  • Form post context: behavior changed — the form_post_id is now validated with signature and on failure falls back to referrer post ID. That is safer but could change behavior for integrators that intentionally supply post ids without signature. The fallback and tests cover likely cases but call out to ensure integrators are aware of the stricter checks.
  • Hidden post id default changed from -1 to 0 in form-hidden-fields.php. Tests check legacy negative id behavior, but verify that any external code checking for -1 specifically is updated or accounted for.
  • API/Action signature changed: Module->handle_after_send() now accepts two args. It was added to add_action with 2 accepted args, so this is internally consistent. But if other code calls the method directly with old signature, that could be an issue. The method is a module internal handler so low risk.

Performance

  • AjaxSubmit.refreshCsrfToken() queries document for all 'form.jet-form-builder[data-form-id]' and loops fields to update CSRF inputs. For pages with very many forms this could be slightly heavy but still O(number of forms * inputs-per-form). Acceptable in practice. If you expect pages with hundreds of forms consider limiting the selector to forms with the same dataset form id or using a more direct mapping.
  • Database queries in SSR unique check are prepared and use IN (...) with placeholders. For very large numbers of record IDs this could be heavy but that behaviour is unchanged from prior code (just extended to nested fields). Consider limiting the search scope or adding indexes if this becomes hot.

Tests

  • Good unit tests added for CSRF lifecycle, post context signature handling, and validation pipeline. This is excellent and increases confidence.
  • Missing / recommended tests:
    • A small integration test verifying the full AJAX flow where server returns the replacement token and the frontend refreshCsrfToken updates the hidden input (simulate Ajax response containing Csrf_Tools::FIELD and inspect DOM). This ensures constant names match and frontend/server integration is correct.
    • A test ensuring Csrf_Token_Model::clear() does not inadvertently delete the token being consumed (if clear() is time-based, an integration test with a token near expiration might surface issues).

Minor code/style

  • Assets build hash changes are expected from builds; all JS/PHP changes follow WPCS in the changed code snippets (sanitization/escaping applied where needed). Good.
  • Small whitespace/no-newline adjustments in JS exports (no functional change).

Recommendation

  • Approve after verifying the Csrf_Tools::FIELD constant matches the frontend key (response._jfb_csrf_token) and confirming Csrf_Token_Model::clear() only removes expired tokens. Add an integration test for the AJAX CSRF refresh path.

Overall: changes are security-minded and well-covered by tests. A few small integration checks will make this change safer for production rollout.

Suggested changelog entry

- FIX: Improve CSRF token lifecycle and post-context signature verification for form submissions (single-use tokens, AJAX token refresh, server-side consume/replace, and signed post-context validation)

@github-actions

Copy link
Copy Markdown

🤖 AI PR Review

Risk level: low

Review

Summary

  • This PR updates many built assets (assets/build/**) and adds a small but important frontend behavior in assets/src/frontend/main/submit/AjaxSubmit.js: if the server returns a _jfb_csrf_token in the AJAX submission response, the code will update all matching form hidden _jfb_csrf_token inputs for forms with the same data-form-id.

Positive

  • Refreshing the CSRF token client-side after a successful AJAX response is a good security-improvement pattern (reduces token reuse window and helps support multiple forms on the same page).

Files / Changes of interest

  • assets/src/frontend/main/submit/AjaxSubmit.js
    • Adds reading response._jfb_csrf_token and a refreshCsrfToken(csrfToken) helper that updates input[name="_jfb_csrf_token"] for forms matching the form id.
    • Minor trailing-newline / export whitespace change.
  • Many assets under assets/build/** and package.asset.php files were rebuilt/updated. These appear to be generated bundles corresponding to the source changes.

Security review

  • Good: The new flow only updates CSRF token values when the server provides a token in the response. That token is server-provided and same-origin via AJAX — this avoids token reuse after a submission. This is aligned with the repository's security guidelines.
  • Consideration: Ensure the server actually provides a fresh _jfb_csrf_token on responses that require rotation. This client-side logic assumes the server will issue the token. If not present it is a no-op (which is safe).
  • Suggestion: Consider validating token shape/length before applying it (defensive), e.g. ensure token is a string and non-empty. Not strictly necessary, but safer in case of malformed responses.

Functional / compatibility review

  • The implementation loops over document forms with selector 'form.jet-form-builder[data-form-id]' and updates input[name="_jfb_csrf_token"]. That should work for most setups.
  • Edge cases to test:
    • Multiple forms on the same page: ensure only forms with matching data-form-id are updated.
    • Forms rendered dynamically (e.g. via AJAX or in repeaters): ensure the updated token is applied to newly created DOM nodes (the code updates current DOM only).
    • Forms inside iframes or cross-origin contexts: querySelectorAll won't reach into iframes — this is expected; document-level update is appropriate for same-document forms.
    • Some codepaths or themes might render CSRF token as value attribute only once or store token elsewhere — verify all places where token is read by server are updated.

Performance

  • The DOM traversal performed by refreshCsrfToken is small (selecting only .jet-form-builder forms) and only runs when the server returns a token; this is acceptable.

Backward compatibility

  • This is an additive, optional client-side improvement. If the server does not return _jfb_csrf_token the behavior is unchanged.
  • The many generated asset changes are expected but ensure consumers relying on exact bundles are okay with new versions.

Multisite / other considerations

  • No multisite-specific issues identified. If tokens are per-site/per-form, updating only same data-form-id is correct.

Testing / missing tests

  • I don't see new automated tests. Please add or manually test the following scenarios before merging:
    • Submit form via AJAX -> server returns _jfb_csrf_token -> inputs updated, subsequent submissions use the new token.
    • Two forms on the page with different data-form-id: only the matching form receives the updated token.
    • Forms inside repeaters or dynamic templates: ensure tokens are updated where expected, or document any limitations.
    • Graceful behavior if server returns no token.

Minor code suggestions

  • In AjaxSubmit.refreshCsrfToken, consider an early type check: if ( typeof csrfToken !== 'string' || !csrfToken ) return; — defensive but optional.
  • Consider updating 'value' and also attribute (e.g. field.setAttribute('value', csrfToken)) if some consumers inspect HTML attributes rather than DOM properties. Usually field.value is enough.

Build artifacts

  • This PR modifies many built JS and asset manifest files. Please confirm that the built artifacts were regenerated from the source and that the source-to-build mapping is correct (i.e. the repository should include source changes alongside updated bundles). If the repo prefers not to include built bundles in PRs, consider that process.

Recommendation

  • Overall: approve after adding basic tests (manual or automated) for multi-form and dynamic-form scenarios and optionally adding a small defensive check for token type/emptiness.

Suggested changelog entry

- IMPROVE: Refresh frontend CSRF token after AJAX form submissions to reduce token reuse and improve security for multi-form pages (frontend submit)

@github-actions

Copy link
Copy Markdown

🤖 AI PR Review

Risk level: medium

Review

Summary

  • This PR updates a large number of built asset files (packed/minified JS and asset .php files with new versions). No PHP source files or unminified sources are included in the diff.

Primary concerns & findings

  1. Built assets changed without source context (high process risk)

    • The PR replaces many compiled bundles under assets/build/** (admin, editor, frontend). Replacing minified/compiled output without accompanying source changes or a build log makes it impossible to verify what changed at the source level. This is a release-process / supply-chain risk: any change in built files must be accompanied by (a) the corresponding source changes, (b) a reproducible build script/commit or CI artifact proving the build, and (c) a short description of intentional changes. Please provide the unminified source commits (and ideally the build step / tool versions) that produced these artifacts so reviewers can verify functionality and security.
  2. Use of eval() in calculated field (security)

    • File: assets/build/frontend/calculated.field.js
    • Function: convertMillisToDateString uses const millis = eval(millisInput);
    • Risk: eval() can execute arbitrary code and is dangerous if the formula/millisInput can be influenced by an attacker (e.g. via form settings, dynamic formulas, or untrusted storage). Even if this was present previously, it must be called out in a security audit PR. Recommend replacing eval with a safe parser, e.g. Number(), parseInt/parseFloat, or a restricted expression evaluator (or only allow numeric timestamps). Document the trust model for formulas and sanitize/validate inputs before eval if it must remain.
  3. Dynamic REST / AJAX calls require server-side nonce and capability checks (sanity check)

    • Several admin bundles add the nonce to AJAX request payloads when window.JetFBPageConfigPackage.nonce exists. Ensure all corresponding server-side handlers verify the nonce and perform capability checks for sensitive actions (plugin install/activate/deactivate, license handling, settings changes). This PR touches only JS, but reviewers must confirm server-side endpoints already validate nonces and capabilities.
  4. Optional chaining / modern syntax in built files

    • Multiple built files use optional chaining (e.g. window?.JetFBPageConfigPackage?.nonce). That is fine for modern browsers but ensure the build target/transpilation is consistent with the plugin's supported browser list. Since these are built files, confirm the source and build toolchain targets (Babel/webpack) are correct and tested on min supported browsers.
  5. Media/upload & file handling

    • Files under assets/build/frontend/media.field.js and restrictions updated. File upload handling (creating File objects from fetch blobs, preview URLs, and using DataTransfer) looks complex. Confirm server-side upload restrictions are enforced (MIME types, max size, capability checks) and that file URLs used in previews are sanitized when inserted into the DOM. There are helper functions that sanitize strings (replace & < > etc.) — good, but ensure anywhere innerHTML is used is safe.
  6. Frontend validation & SSR rules

    • Advanced validation files and SSR validation code appear in bundles. Ensure server handlers used by SSR validation endpoints exist and verify nonces/capabilities as needed. Confirm long-running network calls (fetch) have sensible timeouts (client-side) or server-side rate-limiting.

Performance & scalability notes

  • The PR touches many UI/build bundles; I did not find obvious performance anti-patterns in the minified code, but please validate via performance testing on large forms (many fields / conditional logic) and on multisite setups. The admin pages (addons/settings) also add potentially heavy operations like plugin install/update checks — ensure they are asynchronous and not blocked on large sync tasks.

Backward compatibility

  • Because only built assets changed, assume no API contract changes, but please ensure:
    • Gutenberg block attributes / block JSON remain unchanged.
    • REST endpoints and their payloads are compatible with previous versions (no breaking changes).
    • Public JS globals used by third-party add-ons (e.g. JetFBComponents, JetFBMixins) were not removed or renamed.

Missing tests / artifacts

  • This PR contains no tests or changelog in code. For a risk / security PR that updates many built files please:
    • Provide unit/integration tests for: calculated fields (including date parsing), SSR validation, file upload and restrictions, conditional block behavior.
    • Provide the build script / commit SHA for the unminified sources used to produce these artifacts (or CI build artifact). Without it we cannot easily verify the changes.

Actionable recommendations

  1. Provide the corresponding source commits and the reproducible build output (CI artifact) that produced these asset files.
  2. Replace eval(millisInput) with a safe parse flow (e.g. Number/moment/Date parsing) or explicitly validate the input before eval and document trust assumptions.
  3. Confirm all AJAX endpoints used here verify nonces and user capabilities server-side; call this out in the PR description.
  4. Add/extend automated tests for the critical areas mentioned above (calculated fields, SSR validation, file uploads and restrictions, conditional visibility). Add a short QA checklist for manual testing (multi-step forms, conditional blocks, media uploads, license/install flows).
  5. If this is a security audit release: include a small SUMMARY in the PR body explaining what was audited and what changes were made to the source (not only built files).

Files called out (examples)

  • assets/build/frontend/calculated.field.js — convertMillisToDateString uses eval() (replace or strongly justify)
  • assets/build/admin/package.js — uses window?.JetFBPageConfigPackage?.nonce; ensure build/compatibility
  • assets/build/frontend/media.field.js and assets/build/frontend/media.field.restrictions.js — file handling and size/ext restrictions; ensure server-side checks and sanitize preview HTML
  • Many asset .asset.php files — version bumps only; ensure these correspond to real builds

Overall assessment

  • The change set is primarily a wholesale rebuild of frontend/admin/editor bundles. That in itself is not a problem, but shipping compiled assets without the associated source modifications and verification increases supply-chain risk. The eval usage in the calculated-field code is the most concrete security issue to address or justify.

I can re-review once you provide the unminified source changes, build logs/CI artifact, and address the eval usage or explain the trust model and mitigation.

Suggested changelog entry

- IMPROVE: Rebuilt admin/editor/frontend JavaScript assets after security audit (updated bundles and asset hashes).

@MjHead
MjHead changed the base branch from main to release/3.6.4 July 20, 2026 10:37
@github-actions

Copy link
Copy Markdown

🤖 AI PR Review

Risk level: medium

Review

Summary

  • This PR updates many built JS assets under assets/build/* (admin, editor, frontend) and bumps asset versions. No PHP source files were changed — only compiled assets and their package/version files.

Main concerns

  1. Unsafe dynamic code execution in frontend/calculated.field.js
  • File: assets/build/frontend/calculated.field.js
  • Issue: convertMillisToDateString() uses eval(millisInput). Using eval on any input that can be influenced (formulas, dynamic parameters, templates) is a high-risk XSS/remote code execution vector. I also see CalculatedFormula usage that forces functions; any runtime evaluation must be tightly controlled.
  • Recommendation: remove eval. Accept only numeric millis values or use safe parsers like Number(), Date.parse(), or a vetted expression evaluator (e.g. mathjs) with a restricted context. If formula language requires JS evaluation, use a sandboxed parser and whitelist allowed operations. Add unit tests with malicious inputs to confirm safe behavior.
  1. innerHTML / template injection (XSS) in media field previews
  • Files: assets/build/frontend/media.field.js, assets/build/frontend/media.field.restrictions.js
  • Issue: createPreview and related functions build HTML using innerHTML and template strings with file names/URLs. Although file data is normally from user uploads, any scenario where a template or file URL could contain unsafe content may lead to XSS. There are helper escaping functions (u) but ensure they are used consistently for all inserted values and in all code paths.
  • Recommendation: prefer textContent/property assignment for text nodes instead of innerHTML when possible. Ensure any HTML inserted is sanitized (use wp_kses on server-rendered templates) and confirm escaping is applied to every user-controllable value (file names, URLs). Add automated tests to simulate malicious file names and confirm escaping prevents script execution.
  1. MutationObservers and DOM re-insertion logic (behavioral risk)
  • Files: media.field.js, conditional.block.js, multi.step.js
  • Issue: code removes and re-inserts nodes (conditional blocks) and uses MutationObservers in multiple places. Re-insertion logic must preserve event listeners/state (e.g., jQuery UI sortable, input masks, Vue/React-managed areas). This may break third-party integrations or lead to duplicate listeners.
  • Recommendation: review reinitChildren() and showBlockDom() logic to ensure it re-attaches or avoids duplicating handlers; prefer toggling visibility (and preserving state) where feasible. Add integration tests for common third-party scripts (inputmask, datepickers, select2) across show/hide cycles.
  1. Use of fetch/wp.apiFetch / AJAX and nonces
  • Files: several admin bundles (assets/build/admin/package.js, pages jfb-addons.js, jfb-settings.js)
  • Observations: most outgoing AJAX calls include nonce (window.JetFBPageConfig.nonce or JetFBPageConfigPackage.nonce). Good. Ensure server-side handlers verify those nonces and current_user_can() where appropriate.
  • Recommendation: verify all endpoints referenced in bundles (e.g. jfb_addon_* actions, jfb_license_action, jet_fb_save_tab__*) perform wp_verify_nonce() and capability checks. Add unit/acceptance tests for permissioned actions.
  1. Eval-like behavior and server-side SSR validation
  • Files: assets/build/frontend/advanced.reporting.js, assets/build/frontend/conditional.block.js, assets/build/frontend/calculated.field.js
  • Observations: SSR validation and calculated field evaluation call remote endpoints (REST / admin-ajax) and sometimes evaluate patterns. Make sure all server-side endpoints sanitize inputs and do not reflect unsanitized values back into client-side HTML without escaping.
  • Recommendation: add server-side input validation and output escaping (wp_kses, esc_html) for any messages or templates used by frontend.

Performance / scalability notes

  • Large JS bundles were changed — ensure HTTP caching/asset versioning reflected in updated asset PHP files (I see version bumps). That's fine; verify cache invalidation on deployment.
  • Some code uses repeated querySelector or iterates DOM in hot paths (e.g. conditional block reinitChildren scanning the tree). Consider throttling/debouncing where appropriate (some debounce present). Add tests for large forms (100+ fields) focusing on load & show/hide behavior.

Backward compatibility

  • Only built assets changed. No public PHP APIs appear modified. Still, any small behavioral change in condition evaluation, calculated formulas, or media previews may impact forms relying on previous quirks. Recommend a compatibility smoke test covering:
    • Calculated fields (expressions, numeric/date outputs)
    • Multi-step navigation (enter key, next/prev, validation)
    • Conditional blocks show/hide/disable
    • Media uploads and existing uploaded files display

Tests to add (high priority)

  • Security tests for calculated field input (attempt to inject JS or expressions that use eval) — confirm no arbitrary code runs.
  • XSS tests for media field file names/URLs/templates (both in editor and when macros insert remote URLs).
  • Permission tests for admin AJAX endpoints referenced by admin bundles to ensure nonce + capability checks server-side.
  • Large-form performance test that toggles conditional blocks and multi-step pages repeatedly.

Minor suggestions

  • In convertMillisToDateString(), improve naming and avoid eval. For formatting, consider a small deterministic formatter that maps tokens to Date methods without runtime eval.
  • Ensure all inserted HTML uses escaping helpers (there are functions like u — verify coverage across all code paths).

Conclusion

  • This PR is primarily a rebuild of bundled assets and includes potentially dangerous runtime evaluation (eval). The major security risk is eval usage in calculated.field.js and similar dynamic evaluation points. Please remove eval or restrict/validate inputs and add tests to ensure the frontend is not vulnerable to code injection.
  • After addressing eval/innerHTML sanitization and adding tests, this can be merged.

Suggested changelog entry

- FIX: Harden calculated field evaluation and media field rendering in frontend JS (security audit, rebuilt assets)

@Gawuww
Gawuww changed the base branch from release/3.6.4 to release/3.6.5 July 30, 2026 12:07
@github-actions

Copy link
Copy Markdown

🤖 AI PR Review

Risk level: medium

Review

Summary

  • This PR updates many built frontend/editor/compatibility assets and adds a CSRF token refresh on Ajax form submit (assets/src/frontend/main/submit/AjaxSubmit.js). Most other changes are build/formatting tweaks in compatibility bundles.

What I checked

  • Security: added refresh of _jfb_csrf_token after Ajax response (AjaxSubmit.js)
  • Performance & scalability: how CSRF update is implemented and any new DOM scanning
  • Backwards compatibility: whether public APIs/attribute names change
  • General quality: many changes are built/minified assets — verify source changes exist and nothing important was removed

Findings / Issues / Suggestions

  1. assets/src/frontend/main/submit/AjaxSubmit.js
  • Good: The client now updates the _jfb_csrf_token value from response._jfb_csrf_token when provided. This helps support rotating CSRF tokens and avoids broken subsequent submissions.
  • Security: Make sure the server-side actually emits response._jfb_csrf_token and that server-side endpoints validate the same token. Front-end change alone is harmless, but token rotation must also be validated server-side — please confirm the server accepts and verifies _jfb_csrf_token for subsequent submissions.
  • Performance: Current implementation queries all forms on the page and iterates them:
    const forms = document.querySelectorAll('form.jet-form-builder[data-form-id]');
    for ( const formNode of forms ) { if (+formNode.dataset.formId !== formId) continue; ... }
    Suggestion: replace this with a targeted selector to avoid scanning all forms, e.g.:
    const formNode = document.querySelector(form.jet-form-builder[data-form-id="${formId}"]);
    then update inputs for that single form. This will be much cheaper on pages with many forms or in large admin pages.
  • Type comparison: The code uses +formNode.dataset.formId !== formId. This coerces dataset to number; prefer consistent type handling. Either cast both sides to Number or compare strings explicitly (String(formId) === formNode.dataset.formId). This avoids subtle bugs if formId comes as a string.
  • Input selector: the selector looks for input[name="_jfb_csrf_token"]. Ensure this is the canonical name used everywhere. If the token input could be present with different attributes (e.g. data-name or different placement) consider a more robust approach or document the required markup.
  • Safety: the token is injected into input.value only — that's safe. Double-check server does not reflect the token into any HTML without escaping (to avoid reflected XSS) — tokens should be opaque strings.
  1. Browser compatibility / JS language features
  • Many bundles use optional chaining (?.) and other modern JS. Confirm the plugin's supported browser matrix allows these features (or ensure transpilation/polyfills are applied). If you support older browsers that lack optional chaining, this could break them.
  1. Built assets vs source
  • The PR modifies many built/minified files (assets/build/* and compatibility/* build files). Please ensure the corresponding source (assets/src/*) changes are present in the PR or are planned; committing only built assets without updating sources or build scripts makes future maintenance harder.
  1. Multisite / caching considerations
  • Rotating CSRF tokens can interact with full-page caching and public pages. If tokens are user/session-specific and server rotates them per request, cached HTML (with an old token) will be served to other users and cause failures. Ensure the token rotation strategy is compatible with widely-used caching setups or document the behavior.
  1. Tests and QA
  • There are no automated tests included for the new client behavior. Please add a small integration/UI test (or manual test steps) verifying:
    • Ajax submit returns _jfb_csrf_token and subsequent submissions use updated token
    • Behavior when token is absent (no-op)
    • Behavior on pages with multiple forms (only the relevant form updated)
  1. Misc
  • Several compatibility bundles contain only formatting/arrow/parentheses changes. I did not see functional regressions, but these touched many files — run integration smoke tests for common integrations (Bricks, JetBooking, JetAppointment, JetEngine) and multi-step forms.

Conclusion / Acceptance criteria

  • Before merging please:
    1. Confirm server-side support: server sends response._jfb_csrf_token and verifies it on subsequent requests.
    2. Update AjaxSubmit.refreshCsrfToken to target a single form (use querySelector with the specific data-form-id) and normalize type comparisons.
    3. Ensure source files are updated (not only built artifacts) or include a note why build artifacts were committed.
    4. Add (or describe) tests / QA steps for the new behavior.

If those are addressed I’m OK with this change. The CSRF refresh addition is a positive security/UX improvement, but the scanning approach and built-only diffs need small fixes and confirmation of server-side support.

Suggested changelog entry

- IMPROVE: frontend - refresh form CSRF token after Ajax submit when server returns a new _jfb_csrf_token (improves support for rotating tokens)

@Gawuww
Gawuww merged commit 8f3537f into release/3.6.5 Jul 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants