FIX: Secure SSR callbacks with admin-managed registry

[Gawuww]

Issue - https://github.com/Crocoblock/issues-tracker/issues/20361

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

This PR replaces the per-form SSR allowlist with a single admin-managed registry and adds a bounded migration that imports candidate callback names as "pending" for admin review. It also adds UI for settings, an admin notice for migration results, and several migration/transaction fixes to preserve resume cursors. Overall the changes address the security issue (self-service allowlisting) and are thoughtfully implemented (time-boxed migrations, pending queue, denylist enforcement, masking callback names in public JSON).

What I checked

• Security: nonces/capabilities, input sanitization, denylist enforcement, safe call sites
• Performance: bounded migrations, DB queries, caching hot paths
• Backward compatibility: per-form allowlist retirement, migrations, filters
• Tests: added transaction and auto-migrator tests; removed/updated old tests

Issues, risks and suggestions (actionable)

1. Ensure capability + nonce enforcement for the settings endpoint (Ssr_Callbacks_Handler::on_get_request)

• File: includes/admin/tabs-handlers/ssr-callbacks-handler.php
• The method has inline phpcs notes asserting nonce verification in Base_Handler::on_raw_request(). Please double-check and confirm Base_Handler enforces both a nonce and a capability check (manage_options) for admin settings POST/GET requests. If Base_Handler does not strictly check manage_options then add an explicit current_user_can( 'manage_options' ) check before performing save/approve/reject and return wp_send_json_error/wp_die appropriately. This endpoint writes trust decisions — must be admin-only.

2. Verify admin registry ID mapping is robust and stable

• Files: modules/validation/advanced-rules/ssr-callback-registry.php (new), modules/validation/module.php (masking logic)
• The editor JSON masks custom callback names using Ssr_Callback_Registry::id_for_callback(). Ensure id_for_callback returns a stable, opaque ID (deterministic within a request/site, not reversible to a name without admin privileges) so that the frontend cannot be used to escalate trust. Also ensure the registry has an explicit inverse mapping (ID -> name) on the server to resolve the submitted rule IDs safely, and that it validates input before resolving.

3. sanitize_callback_name + validate_single_name coverage and behavior

• Files: modules/validation/advanced-rules/server-side-rule.php, ssr-callback-registry.php
• Good to see sanitize_callback_name introduced to make validation consistent. Confirm validate_single_name (used inside the registry) applies the same checks (sanitize, denylist, builtin check, function_exists). Add unit tests for malformed names, whitespace, unicode, and names that contain only word-characters but are uppercased to ensure lowercasing behavior is consistent.

4. Builtins / FIXED_SAFE: confirm safe semantics and compatibility

• File: modules/validation/advanced-rules/server-side-rule.php
• FIXED_SAFE short-list is executed with one argument and returns (bool). Please ensure these WP core functions are present in the minimum supported WP version (doc comment asserts that — double-check). Also document the rationale for executing these without registry approval. Consider whether any of them could be extended by plugins to do I/O (unlikely but mention risk). Add a unit test exercising these functions through SSR validation.

5. Migration/resume cursor correctness and transaction boundaries

• Files: includes/migrations/versions/version-3-6-5-3.php, includes/migrations/migration-incomplete-exception.php, includes/migrations/auto-migrator.php, includes/migrations/migrator.php
• The handling of a migration that issues its own COMMIT then throws Migration_Incomplete_Exception is subtle but appears correctly handled: Auto_Migrator now treats Migration_Incomplete_Exception as a special transient condition in REST/CLI entry points and does not let downstream migrations run in the same request. Ensure there is a comment in docs for operators explaining that an admin request may need to be re-run until the migration completes. Good unit coverage added — nice.

6. Blocked usages storage and post deletion

• Files: modules/validation/ssr/ssr-blocked-callback-usages.php, modules/validation/module.php
• The Ssr_Blocked_Callback_Usages option stores an array of usage records. For sites with many forms this could become large. Consider whether this should be stored as a separate custom table (out of scope here) or at least ensure autoload=false is used (update_option uses autoload=false already — good). Also ensure replace_for_form deduping/size remains acceptable on very large sites.

7. Escaping and JSON content returned to editor

• Files: includes/admin/tabs-handlers/ssr-callbacks-handler.php (on_load returns form_title as html_entity_decode of get_the_title()), modules/validation/module.php (localize_editor_config masking)
• Returning plain text titles and other strings in JSON that are later interpolated by Vue is fine when the JS framework escapes on render; your comment documents that. Still, ensure all strings in JSON are sanitized to prevent injection via stored post fields (you already use html_entity_decode — which is correct here) and that the JS side treats values as text. Good comments.

8. Multisite considerations

• The registry is site-scoped via get_option/update_option. If plugin is used in Multisite and admins expect network-wide control, consider whether a network-level option is desired. At minimum document that registry is per-site and that superadmins must manage each site separately. Also consider manage_network capability for multisite flows (out of scope but call out).

9. Tests and missing unit coverage

• Files: tests added/changed
• Good new tests for AutoMigrator transactional behavior. However add tests for:
  • Ssr_Callbacks_Handler permission/nonce refusal cases (403 on insufficient capability, invalid nonce)
  • Ssr_Callback_Registry::save_allowed_callbacks behavior (rejected reasons, function_exists behavior)
  • id_for_callback and resolution path (ensure submit/resolution mapping is correct)
  • sanitize_callback_name edge-cases and Unicode/locale

10. Backward compatibility note

• This is a behaviour change: per-form allowlists are retired and runtime validation now consults a global admin-managed registry + builtins + fixed-safe WP list + filter. Existing flows that relied on saving a form to "enable" a callback are changed — you correctly included a migration that imports candidates as pending and an admin notice. Still, this is a breaking security model change; ensure release notes and upgrade docs clearly explain the admin review step and how to approve legacy callbacks.

Minor nits

• Consistency: Server_Side_Rule::is_builtin_callback caches lowercase IDs; ensure all comparisons consistently lowercase before checking (I saw careful lowercase use, but double-check callers).
• Ssr_Registry_Migration_Notice::maybe_dismiss_notice checks current_user_can('manage_options') — good.

Conclusion

• Overall this looks like a good, careful security-focused refactor. The most important action is to verify and, if needed, harden capability/nonce checks for the admin settings endpoint (Ssr_Callbacks_Handler) and add unit tests covering the registry save/approve/reject API, id_for_callback mapping, and sanitize/validation edges. Also add developer/upgrader docs in the changelog or upgrade guide explaining the user-facing change (admin must approve pending callbacks).

Suggested changelog entry

- FIX: Secure SSR callbacks by replacing per-form allowlists with an admin-managed global registry and a pending-review migration; blocked functions are recorded and admins must approve imported callbacks in Settings (validation, migrations, and editor masking fixes)

[Copilot]

🟡 Changes recommended

Migration reliability, callback compatibility, and incomplete CLI reporting contain unresolved defects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Introduces an administrator-managed registry to prevent untrusted SSR validation callbacks from executing.

Changes:

• Replaces automatic per-form allowlisting with an explicit global registry.
• Adds resumable migrations, review notices, and blocked-callback reporting.
• Adds settings UI, editor guidance, and regression tests.

File summaries

File	Description
.gitignore	Tracks the new tests.
jet-form-builder.php	Bumps version to 3.6.5.3.
includes/admin/tabs-handlers/ssr-callbacks-handler.php	Handles registry administration.
includes/migrations/auto-migrator.php	Registers the new migration.
includes/migrations/migration-incomplete-exception.php	Defines resumable-migration signaling.
includes/migrations/migrator.php	Registers migration 3.6.5.3.
includes/migrations/versions/version-3-6-5-2.php	Documents time-boxed legacy migration behavior.
includes/migrations/versions/version-3-6-5-3.php	Migrates existing callbacks for review.
modules/cli/commands/upgrade-database.php	Handles incomplete CLI migrations.
modules/rest-api/endpoints/install-migrations-endpoint.php	Reports incomplete REST migrations.
modules/validation/advanced-rules/server-side-rule.php	Enforces the registry and safe callbacks.
modules/validation/advanced-rules/ssr-callback-allowlist.php	Retains bounded legacy migration support.
modules/validation/advanced-rules/ssr-callback-registry.php	Implements trusted and pending registries.
modules/validation/module.php	Integrates registry UI, notices, and usage tracking.
modules/validation/ssr/ssr-blocked-callback-usages.php	Records forms using denied callbacks.
modules/validation/ssr/ssr-registry-migration-notice.php	Prompts administrators to review imports.
assets/src/package/validation/components/AdvancedRuleModalItem.js	Links editors to callback settings.
assets/src/admin/pages/jfb-settings/SettingsPage.vue	Registers the settings tab.
assets/src/admin/pages/jfb-settings/tabs/ssr-callbacks/index.js	Exports the new tab.
assets/src/admin/pages/jfb-settings/tabs/ssr-callbacks/source.js	Defines tab labels and guidance.
assets/src/admin/pages/jfb-settings/tabs/ssr-callbacks/SsrCallbacksTab.vue	Implements registry review UI.
assets/build/editor/package.asset.php	Updates editor build metadata.
assets/build/editor/form.builder.asset.php	Updates form-builder build metadata.
assets/build/admin/pages/jfb-settings.js	Includes the compiled settings UI.
assets/build/admin/pages/jfb-settings.asset.php	Updates settings build metadata.
tests/wpunit/SsrCallbackAllowlistTest.php	Removes obsolete allowlist tests.
tests/wpunit/SsrCallbackRegistryTest.php	Tests registry enforcement and migration.
tests/wpunit/SsrCallbackMigrationBatchingTest.php	Tests resumable scanning.
tests/wpunit/AutoMigratorTransactionTest.php	Tests transaction-boundary behavior.
tests/wpunit/AutoMigratorTest.php	Expects the new automatic migration.

Review details

Suppressed comments (2)

includes/migrations/versions/version-3-6-5-3.php:90

• A failed SELECT is treated as an empty final page, so this migration can delete its progress, write an empty result, and be marked installed. The later successful option queries can also clear $wpdb->last_error before Base_Migration::run_up() checks it. Check the query error before handling empty( $form_ids ) so a transient DB failure is retried instead of silently skipping every remaining form.

			if ( empty( $form_ids ) ) {
				break;
			}

modules/validation/advanced-rules/ssr-callback-allowlist.php:136

• This has the same silent-completion failure mode as the registry scan: $wpdb->get_col() returns an empty value on an SQL error, and the subsequent option writes can clear $wpdb->last_error. Check the error here before treating the page as complete, otherwise the historical migration may be permanently stamped after skipping the remaining forms.

			if ( empty( $form_ids ) ) {
				break;
			}

• Files reviewed: 28/32 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

This PR introduces a secure, admin-managed registry for "Server-Side callback" validation (Ssr_Callback_Registry), retires the old per-form allowlist, adds a bounded, resumable migration (Version_3_6_5_3) to restore previously-used callbacks into the new registry, hardens the denylist in Server_Side_Rule, and adds admin UI (settings tab + migration notice) and tests around the migration transaction behavior. Overall the changes address the security gap described in issues-tracker #20361 and add sensible batching/time-budgeting to long-running scans. The migration/transaction handling and the new Migration_Incomplete_Exception handling in CLI/REST are good improvements.

What I checked and notable positive points

• Migration safety: Version_3_6_5_3 is time‑boxed, batched, persists resume cursors, and throws Migration_Incomplete_Exception so large sites can resume without timing out. Auto_Migrator was updated to not stamp the version when a migration yields and to expose is_migration_in_progress() so UI can avoid racing writes. Good.
• Transaction semantics: the PR accounts for migrations that commit their own progress before yielding and ensures Auto_Migrator stops further migrations when that happens. CLI and REST handlers properly surface the incomplete state so automation can re-run — good end-to-end consideration.
• Hardening: Server_Side_Rule NOT_ALLOWED list was substantially expanded (covers callback-injection primitives, file I/O, network, WP mutations). The registry and migration both check names are syntactically valid, not denylisted, not built-in, and function_exists() before trusting — good.
• UI: Admin settings tab for managing allowed callbacks and a migration notice to inform admins are useful and keep the admin in control; the editor-localization replaces custom callback names with opaque IDs in the public JSON blob (reduces exposure of callback names to unauthenticated clients) — good practice.
• Tests: Migration transaction tests were added to ensure resume cursor survives rollbacks and migrations resume correctly. Good coverage for the tricky transaction semantics.

Main concerns / required follow-ups

1. Ensure capability checks/nonce enforcement in Ssr_Callbacks_Handler::on_get_request

• File: includes/admin/tabs-handlers/ssr-callbacks-handler.php
• on_get_request() relies on a comment that Base_Handler::on_raw_request() already verifies the nonce. Please confirm (and document) that Base_Handler enforces both a valid nonce and a manage_options capability for this handler before allowing writes. If Base_Handler only verifies the nonce but not capabilities, add an explicit current_user_can( 'manage_options' ) check and return wp_send_json_error() when missing. Writing the registry must be limited to manage_options.

2. Confirm existence and properties of id_for_callback and mapping stability

• File references: modules/validation/* and usage in Module::add_validation_block
• Module::add_validation_block replaces custom callback names in the localized editor blob with Ssr_Callback_Registry::id_for_callback( $callback_name ). I couldn't see the complete Ssr_Callback_Registry implementation in the truncated diff; ensure:
  • id_for_callback() exists and returns a stable, opaque ID (not trivially reversible) per callback name.
  • The mapping must be deterministic across requests so editor/client code can refer to the same ID repeatedly.
  • The server-side validation must be able to resolve the ID back to the registered callback at runtime (or the client must not be able to influence the server by sending an ID in a way that bypasses server-side resolution). If this mapping relies on transient states, make it persistent.

3. Verify front-end exposure and reverse mapping security

• The localized JSON must not expose raw custom callback names to unauthenticated users. The code replaces names for non-builtins, which is good, but confirm that no other path leaks raw names (e.g. REST endpoints, AJAX responses, block attributes rendered into public HTML). Also confirm that the opaque ID cannot be used to synthesize a request that makes the server call an arbitrary function — server must always resolve the callback name from the saved post_content or the trusted registry, never from a client-provided ID.

4. Ssr_Callbacks_Handler: capability and input sanitization

• File: includes/admin/tabs-handlers/ssr-callbacks-handler.php
• The POST input is wp_unslash()'d and later sent to Ssr_Callback_Registry::save_allowed_callbacks(). Ensure Ssr_Callback_Registry::validate_single_name() is strict (it appears to use Server_Side_Rule::sanitize_callback_name()). Also add an explicit capability check at the top of on_get_request() as mentioned above, to be fail-safe even if Base_Handler changes later.

5. Option size and scalability of Ssr_Blocked_Callback_Usages

• File: modules/validation/ssr/ssr-blocked-callback-usages.php
• The PR stores Ssr_Blocked_Callback_Usages in a single option (jet_fb_ssr_blocked_callback_usages). For sites with many forms and many blocked usages this option could grow large and impact autoloaded option reads (update_option(..., false) sets autoload false which helps). Still, consider long-term scalability: if you expect large numbers, a dedicated DB table or a site‑transient + paginated UI might be better. At minimum document expected sizes and that OPTION is non-autoloaded (I see update_option(..., false) used — good). Also ensure get_option() calls for this option are not in performance sensitive request paths.

6. Multisite and network-activated plugin considerations

• The migration and registry use site options (get_option/update_option). If the plugin is network‑activated and per-site forms exist, ensure migration runs per-site (seems to be triggered on admin_init in the capable admin request). Consider documenting expected behavior for network-activated installs. If a network admin needs to manage callbacks network-wide, current approach is per-site; if you intended network scope, note that this is currently per-site.

7. Tests coverage gaps / more unit tests suggested

• Tests cover db transaction/resume behavior which is the highest risk area. Additional tests are recommended:
  • Authorization/nonce tests ensuring only manage_options can save via Ssr_Callbacks_Handler.
  • Tests ensuring Ssr_Callback_Registry::id_for_callback and the editor-localized replacements are consistent and that the server resolves IDs safely on submission.
  • Tests for Ssr_Blocked_Callback_Usages storage and that remove/replace_for_form is invoked on delete_post to avoid stale entries.
  • Tests verifying that denylisted functions in Server_Side_Rule::NOT_ALLOWED cannot be executed via SSR rule even if present in registry (i.e. ensure validate_single_name rejects NOT_ALLOWED names for both save_allowed_callbacks and import_trusted_callbacks).

8. Review denylist changes

• File: modules/validation/advanced-rules/server-side-rule.php
• The denylist was expanded considerably. This seems appropriate given the security review, but it is a behavioral change: some previously-working SSR callbacks may now be permanently disallowed (or require admin to add them manually if they were present during migration and passed validation). Make sure to communicate in release notes which classes of functions are denied. Consider ensuring specific WP Core functions (e.g. get_option) were intentionally denylisted and that users understand the consequence.

9. Minor: ensure consistent escaping and i18n

• Most strings are internationalized and outputs escaped in admin notice. Good.
• In Ssr_Callbacks_Handler::get_blocked_usages() html_entity_decode(get_the_title()) is used to avoid double-encoding in Vue; this is reasonable but ensure titles are later escaped in templates — the Vue binding will do escaping. Document the rationale (already commented). Good.

10. Code style / standards

• Generally follows WPCS and standards. Ensure any new PHPCS rules in CI pick up the new files. I noticed some long docblocks and in-code comments — acceptable for complex migration logic.

Summary recommendation

This is a valuable security hardening and the migration approach appears careful and well thought out. Before merging, please:

• Add or confirm explicit capability checks in Ssr_Callbacks_Handler::on_get_request() (current_user_can( 'manage_options' )). Verify Base_Handler covers it and document that dependency if you rely on it.
• Verify Ssr_Callback_Registry contains the id_for_callback / server-side resolution implementations and that the ID mapping is deterministic and safe.
• Add tests for the authorization path and for the id->callback resolution and ensure denied functions cannot be executed by SSR rules.
• Consider documenting multisite behavior and potential option-size implications for very large sites (or plan a follow-up to move blocked-usages into a more scalable store if necessary).

If these are addressed, I think this is ready to merge.

Suggested changelog entry

- FIX: Secure "Server-Side callback" validation by introducing an admin-managed registry, safe migration of legacy callbacks, expanded denylist, and a settings tab to manage allowed callbacks (SSR validation, migration, admin UI)

[Copilot]

🟡 Changes recommended

The migration bypasses administrator approval, while blocked-usage storage has stale-state, concurrency, and scalability defects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

includes/migrations/versions/version-3-6-5-3.php:224

• This is not actually a batched write: for up to 200 forms, replace_for_form() rereads, filters, sanitizes, and rewrites the entire growing option once per form. The work also occurs after the time-budget check, so a large blocked-usage list can make the supposedly bounded migration overrun badly. Merge the batch into one in-memory snapshot and persist once, or store usages per form.

		foreach ( $batch_blocked as $form_id => $form_blocked ) {
			// Every form in the batch is visited exactly once, so this form's current set
			// of blocked usages (possibly empty) fully replaces whatever was recorded for
			// it before — including nothing, if a prior migration run or a later manual fix
			// already cleared it. This is the same call `Module::

• Files reviewed: 31/35 changed files
• Comments generated: 3
• Review effort level: Balanced

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Overall: This PR significantly improves security around the Server-Side callback (SSR) validation rule by replacing the per-form self-service allowlist with a single admin-managed registry, expanding the denylist, and adding a robust, time-boxed migration to import previously-used callbacks. The changes are well-structured, include transaction/exception handling, CLI/REST handling for the time-boxed migration, and unit tests that exercise the important transaction/resume scenarios. Good attention to sanitization and the migration edge-cases.

What I inspected closely (high-level):

• New admin settings tab: assets/src/admin/pages/jfb-settings/tabs/ssr-callbacks/* and modified AdvancedRuleModalItem.js
• New admin handler: includes/admin/tabs-handlers/ssr-callbacks-handler.php
• New registry and migration: modules/validation/advanced-rules/ssr-callback-registry.php and includes/migrations/versions/version-3-6-5-3.php (+ Migration_Incomplete_Exception, auto-migrator changes)
• Denylist expansion and sanitize rules: modules/validation/advanced-rules/server-side-rule.php
• Blocked usage tracking and migration notice: modules/validation/ssr/* and hooks in modules/validation/module.php
• Tests and CLI/REST adjustments for Migration_Incomplete_Exception

Positive notes:

• Security: Centralizing admin-managed allowed callbacks closes the self-service approval vector. Validation + function_exists checks before adding names are present. Denylist expansion is extensive and addresses callback-injection & destructive primitives.
• Migration: Time-boxed batching, per-batch commits, a resume cursor option and Migration_Incomplete_Exception propagation are thoughtful and necessary for large sites. Auto_Migrator and Migrator updates handle incomplete batches correctly for REST and CLI flows.
• Data safety: Switching blocked-usage storage to per-form post meta avoids a read-modify-write single-option race. Tests added to validate transaction behavior and resume logic.
• UX: Admin notice, settings tab and link from the editor to Settings are helpful.

Potential issues, recommendations and questions (please address or confirm):

1. Ensure capability checks / nonce verification for the settings save endpoint

• File: includes/admin/tabs-handlers/ssr-callbacks-handler.php (on_get_request)
• Concern: on_get_request relies on a comment that nonce is verified in Base_Handler::on_raw_request(). It saves registry entries (update_option). Please confirm Base_Handler enforces both a valid nonce AND the proper capability (manage_options) for this tab endpoint. If Base_Handler does not check capabilities, add an explicit current_user_can( 'manage_options' ) check and return wp_send_json_error( ... ) with appropriate status. This is a high-value endpoint (change live execution behavior) and must be admin-only.

2. Escape / encoding considerations for form titles passed to JS

• File: includes/admin/tabs-handlers/ssr-callbacks-handler.php (get_blocked_usages)
• You intentionally decode get_the_title() and send raw title to the Vue tab so Vue's escaping works correctly; that's reasonable. Confirm the front-end renders via text interpolation only (no v-html). Keep this comment; good.

3. Settings save: server-side validation and rejection messages

• File: modules/validation/advanced-rules/ssr-callback-registry.php (save_allowed_callbacks / validate_single_name)
• Good validation pipeline. Ensure any returned rejection messages are localized and safe for display. Current API returns human-readable reasons; ensure these are passed through esc_html on output in the settings UI.

4. WP Multisite behaviour

• Files: ssr-callback-registry.php (update_option), ssr callbacks handler, migration
• The registry is stored with update_option() per-site. For multisite/network-activated installs, consider whether registry should be network-wide (update_site_option) or leave per-site. Document the intended behavior. If network admins need to centrally manage callbacks, provide a note or a follow-up to add network-level controls.

5. Potential heavy WP_Query on the settings tab for sites with many forms

• File: modules/validation/ssr/ssr-blocked-callback-usages.php (get_usages)
• This builds a WP_Query with 'posts_per_page' => -1 and meta_key search. On sites with many forms this may be memory heavy. Consider paginating results in the settings UI or limiting the query and loading more via AJAX. At minimum, add a comment / TODO and guard the admin UI if count is huge.

6. get_usages() performance and DB indexing

• Because you rely on meta_key existence queries, on very large wp_postmeta tables this query may be slow. Consider a batched query or an alternative indexable approach if you expect many forms. You do use 'fields' => 'ids' and 'no_found_rows' which helps.

7. Tests for admin authorization and settings endpoint

• Tests were added for migrations and auto-migrator transaction behaviour (good). I did not see tests that exercise the admin-save path for the registry endpoint (capability/nonce/accept/reject flows). Add unit tests that assert only manage_options users may POST to the handler, and that invalid names are rejected as expected.

8. Ensure no backward-compatibility surprises for add-ons

• You retired Ssr_Callback_Allowlist collection hooks and removed automatic per-form allowlist creation on save. This is intentional (security), but addons/snippets/third-party code that relied on per-form allowlist meta or on the old OPTION_KEY might be impacted. You left constants and minimal shell for migration compatibility which helps. Add a short dev note in docs/UPGRADE.md describing: "Saving a form no longer grants an arbitrary callback permission — admins must add callbacks in Settings or rely on the migration import. Per-form allowlist meta is retained only for migration compatibility."

9. Minor code hygiene / small suggestions

• File: modules/validation/ssr/ssr-blocked-callback-usages.php (clear())
  • You call $wpdb->delete( $wpdb->postmeta, array( 'meta_key' => self::META_KEY ) ); and then wp_cache_delete( 'last_changed', 'posts' ); Consider also calling wp_cache_flush() or cleaning post cache where appropriate; however this is acceptable since you only remove meta rows during migration down.
• File: modules/validation/module.php (hooks added)
  • Good to see refresh hooks for reusable blocks. Confirm that the find_forms_referencing_reusable_block() implementation used by refresh_blocked_callback_usages_for_referencing_forms is efficient and uses a meta or cached reverse lookup; if it's a full-scan, it might be expensive when saving a reusable block shared by many forms.

10. Front-end/editor: link to settings

• File: assets/src/package/validation/components/AdvancedRuleModalItem.js
  • You show a Notice advising editors that custom functions must be added in Settings with a link. That improves discoverability. Ensure ssrCallbacksSettingsUrl is localized for all contexts. Good use of rel/noreferrer.

Summary of actionable items I recommend before merge:

• Explicitly confirm / add a current_user_can( 'manage_options' ) check for Ssr_Callbacks_Handler::on_get_request (if Base_Handler does not already enforce it). Add an early return with wp_send_json_error if capability missing.
• Add unit tests for the settings save endpoint that assert capability and nonce checks are enforced and that invalid lines are rejected.
• Consider changing Ssr_Blocked_Callback_Usages::get_usages to paginate (or at least guard extremely large results) to avoid memory spikes for sites with many forms; document the expected scale/limits for the settings UI.
• Add note in upgrade docs about the behaviour change: saving a form no longer auto-trusts a callback.
• Consider whether the registry should support network-level (multisite) configuration or document current per-site scope.

Given the sensitivity of this surface (it controls code execution that can run server-side) the approach taken is sound and carefully implemented. The remaining items are follow-ups (mainly around explicit capability checks, additional tests, and large-site performance for the settings UI).

Suggested changelog entry

- FIX: Secure Server-Side callback validation by introducing an admin-managed registry and safe migration; expand SSR denylist and add robust time-boxed migration to restore previously-used callbacks (validation/migrations/settings).

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary
This PR implements a security-hardening change for the Server-Side callback (SSR) validation rule: it retires the per-form self-service allowlist and introduces a single, admin-managed site-wide registry (Ssr_Callback_Registry) plus UI (settings tab), migration machinery to restore previously-used names safely, and supporting runtime bookkeeping to list forms that relied on denylisted callbacks. It also expands the denylist in Server_Side_Rule, adds time-boxed, resumable migrations, and introduces tests that exercise the auto-migrator transaction behavior.

What I like

• Security-first approach: moving from per-form automatic trust to an admin-managed registry closes the self-service approval vector that allowed a form save to implicitly approve arbitrary callbacks.
• Migration is carefully thought-out: time-bounded scanning, persistent resume cursor, per-batch writes, and special Migration_Incomplete_Exception to signal a legitimate mid-scan yield. Auto_Migrator and consumers (WP-CLI and REST endpoint) are updated to handle the incomplete signal.
• Race conditions addressed: moving blocked-usage storage to per-post meta (Ssr_Blocked_Callback_Usages) prevents the read-modify-write races the single option approach suffered from.
• Appropriate tests covering the auto-migrator transactional behaviour were added (AutoMigratorTransactionTest) and AutoMigratorTest updated.
• UI UX: editor warns authors that custom functions must be added via Settings, and the admin settings tab includes a migration-in-progress state and a blocked forms list.

Files / locations reviewed (high level)

• modules/validation/advanced-rules/server-side-rule.php — denylist expanded; added wp_delete_auto_drafts to block a zero-arg destructive function.
• modules/validation/advanced-rules/ssr-callback-registry.php — new central registry implementation and save/import methods.
• includes/migrations/versions/version-3-6-5-3.php — new time-budgeted migration to import previously-used callback names into registry.
• includes/migrations/auto-migrator.php, migrator.php, migration-incomplete-exception.php — auto-migrator and migrator updates to include/run the new migration and to detect incomplete migrations.
• includes/admin/tabs-handlers/ssr-callbacks-handler.php + assets — settings tab backend/frontend.
• modules/validation/ssr/ssr-blocked-callback-usages.php and ssr-registry-migration-notice.php — new classes to hold blocked usages and to show admin notices.
• modules/validation/module.php — installs/uninstalls tab handler and lifecycle hooks to keep blocked usages in sync.

Security review / comments

• Capability checks / nonces:
  • The code comments state nonce verification occurs in Base_Handler::on_raw_request() and Ssr_Callbacks_Handler::on_get_request is relying on that (see phpcs:ignore in the file). Please confirm Base_Handler indeed enforces a manage_options capability (or otherwise restricts to the appropriate admin capability) in addition to nonce checks. This endpoint modifies a saved trusted list; it must be restricted to manage_options.
  • Ssr_Registry_Migration_Notice::maybe_dismiss_notice correctly calls check_admin_referer() before acting and verifies current_user_can('manage_options'). Good.
• Input validation / sanitization:
  • Ssr_Callback_Registry::save_allowed_callbacks() delegates per-line validation to validate_single_name() which uses Server_Side_Rule::sanitize_callback_name() and other checks; thats good. The handler calls wp_unslash() and documents that sanitization occurs inside the registry save function. This is acceptable but ensure validate_single_name reports a human-friendly reason for each rejected line (it appears to, via $rejected array). Good.
• Output escaping:
  • Ssr_Callbacks_Handler::get_blocked_usages() uses html_entity_decode( get_the_title() ) before sending in JSON and explains why. That’s sensible because Vue will escape on display. Ensure the edit_url and field/name values are encoded on output in the front-end templates (the Vue component should use esc_url/escaped text; I saw the admin Vue code using normal link interpolation — Vue escapes by default, but verify the component does not use v-html or other raw-output approaches for these values).
• Denylist changes:
  • The denylist in Server_Side_Rule was significantly expanded (many functions added). This is expected for security but it can cause runtime differences (forms that used previously-allowed functions will now be blocked). The migration attempts to restore previously-used names where safe, but administrators should be informed. The migration notice class provides that.

Performance & scalability

• The new migration (Version_3_6_5_3) is carefully batched and time-boxed and persists resume state. It uses $wpdb->get_col with a prepared query and LIMIT, and flushes batch writes per BATCH_SIZE. That is appropriate.
• Ssr_Blocked_Callback_Usages::get_usages() uses a WP_Query with meta_key filter to find forms with the per-post meta — that will trigger a meta_key lookup which can be slow on very big databases. However, it's only used to show the admin settings tab (not in hot runtime paths), and posts_per_page = -1 is used to fetch them all for display. On extremely large sites with many forms this could still be heavy; consider paginating the settings tab or using a lightweight admin list+AJAX pagination if necessary. Right now the approach is acceptable for typical usage.

Backward compatibility

• Behaviour change: editors saving a form can no longer implicitly approve custom callbacks — this is an intentional security breaking-change. The migration tries to restore previously used names automatically while rejecting denylisted/absent names. That’s a good middle ground to preserve backwards compatibility where safe. The PR also records and surfaces permanently-blocked usages so admins can fix forms.
• The Ssr_Callback_Allowlist class is retired but left as a shell for backward compatibility with the historical Version_3_6_5_2 migration that references its constants. The comments make this explicit; good.

Multisite

• The registry and migration write to per-site options (update_option). If the plugin needs a network-wide registry in multisite setups, this behavior may need to be revisited. If per-site is intended, OK. Please confirm expected behaviour in multisite.

Testing gaps / suggestions

• There are good tests for the auto-migrator transaction behavior. A few additional tests would be valuable:
  • Unit tests for Ssr_Callback_Registry::save_allowed_callbacks() and import_trusted_callbacks() validating accept/reject reasons for various input lines (bad characters, denylisted names, built-in callbacks, non-existent functions).
  • Tests for Ssr_Callbacks_Handler endpoint to assert capability requirement and that a non-privileged user cannot save changes.
  • A test asserting that the settings tab JSON includes migrationInProgress when Auto_Migrator::is_migration_in_progress() is true and that saving while migrationInProgress returns the expected error.
  • Front-end test to ensure edit_url and form titles render correctly and do not expose HTML entities raw.

Small code suggestions

• Double-check Base_Handler::on_raw_request() enforces manage_options; if not, add explicit current_user_can('manage_options') to Ssr_Callbacks_Handler::on_get_request() before processing.
• In Ssr_Callbacks_Handler::on_get_request() you call wp_send_json_error() / wp_send_json_success(). Consider setting appropriate HTTP response codes where useful (e.g. 202 for migration-in-progress?) — not required, but can be helpful for API clients.
• Ssr_Blocked_Callback_Usages::get_usages() returns all posts with meta and then iterates to build the list. If the number of affected forms is large, the admin UI could be slow; consider server-side pagination in the Settings UI as a future improvement.

Overall recommendation
This PR is a solid and careful security-focused refactor that addresses the root trust model problem properly, with well-thought migration and race handling. The code shows attention to DB transaction semantics and practical deploy-time realities. Before merging I recommend confirming the capability enforcement assumption referenced in the comments (Base_Handler::on_raw_request() must require manage_options) and adding a couple of small unit tests for the registry validation and the settings endpoint capability check.

Specific lines/files to review by author before merge

• includes/admin/tabs-handlers/ssr-callbacks-handler.php::on_get_request — confirm capability and refer to Base_Handler behaviour in code (add explicit current_user_can('manage_options') if not already enforced upstream).
• modules/validation/advanced-rules/ssr-callback-registry.php::validate_single_name (not fully visible in diff) — ensure reject messages are clear and that function_exists() and builtin callback checks are enforced.
• modules/validation/ssr/ssr-blocked-callback-usages.php::get_usages — consider pagination for the admin UI on large sites.

Suggested tests to add prior to merge

• Registry validation unit tests (accept/reject reasons).
• Endpoint permission test that only manage_options can save settings.

Good job overall — careful work that balances security, migration UX, and data integrity.

Suggested changelog entry

- FIX: Secure Server-Side callback handling — introduce an admin-managed allowlist, block unsafe callbacks by default, and add a time-boxed migration to restore previously-used safe callbacks (settings tab and migration notice added).