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

[Gawuww]

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

[github-actions]

🤖 AI PR Review

Risk level: high

Review

Summary

• This PR tightens the Secure payment amount migration and validation pipelines: it bumps the scan schema, adds logic to detect safe static price sources (calculated/hidden/option fields) and automatically enables protection for strictly verifiable static sources, and hardens the calculated-expression parser by separating parsing/validation and ensuring non-finite results are rejected.

What I like

• Added validation path for calculated expressions (parse_tree() + validate_node()) which makes reasoning about expression safety clearer (file: modules/gateways/trusted-price-resolver-expression-parser.php).
• More conservative scan logic: existing forms are preserved, but strictly verifiable static price sources can be opted-in automatically (file: modules/gateways/secure-price-notice.php).
• User-facing notice wording updated and still escaped with esc_html_e() (good).

Potential issues / things to address

1. Possible side-effectful method call in read-path (high risk)

   • In Secure_Price_Notice::get_affected_form() you call $this->enable_price_protection( $form_id, $settings ) inside a predicate used to decide whether a form is considered affected. The method name implies it may mutate gateway settings. Running side-effecting code during a read/display step is risky and can cause unexpected changes during admin page render. If enable_price_protection() mutates state it must not be called here. If it is purely a pure/boolean check, rename it to is_price_protection_enabled_for_form() or document that it has no side effects and is idempotent. (file: modules/gateways/secure-price-notice.php)

2. Unused import / coding standards

   • You added use JFB_Modules\Post_Type\Meta\Gateways_Meta; at the top of secure-price-notice.php but I don't see it used in the shown diff. Remove unused imports to satisfy WPCS and avoid confusion.

3. Trusted_Price_Resolver constants and class availability

   • Secure_Price_Notice references Trusted_Price_Resolver::MAX_FORMULA_LENGTH and ::MAX_MACRO_TOKENS. Ensure the class and these constants exist and are autoloadable in this namespace. If it's in the same namespace this is fine, otherwise add a use statement or full namespace.

4. Behavior change: scan schema bump and automatic enabling (backward compatibility)

   • SCAN_SCHEMA_VERSION bumped from '1' to '2'. Please confirm migration behaviour: previously stored notices that lack the new schema are handled via notice_uses_current_scan_schema(); the code avoids scanning tracked_ids unless schema matches. Ensure this upgrade path doesn't silently enable protection for forms that should stay unchanged. Document this behavior in the changelog and consider an admin-facing audit log or clear notice so admins can review which forms were auto-enabled.

5. Performance: Block scanning for large forms/sites

   • The code uses Block_Helper::get_blocks_by_post( $form_id, true, true ) and then traverses blocks to find price fields. For large forms or many forms this can be expensive. The scanning is batched (SCAN_BATCH_SIZE = 30) so the impact might be acceptable, but consider caching block results per form during the scan loop or limiting operations performed during admin page render. (file: modules/gateways/secure-price-notice.php)

6. Regex / macro parsing complexity

   • You added CALCULATED_MACRO_PATTERN and preg_match_all() in is_safe_calculated_formula(). Make sure Trusted_Price_Resolver::MAX_MACRO_TOKENS is set to a reasonable limit to avoid catastrophic backtracking or high memory usage with maliciously long inputs. The code already checks substr_count( $formula, '%' ) / 2 > MAX_MACRO_TOKENS which helps; ensure MAX_MACRO_TOKENS is documented/tested.

7. Expression evaluation edge-cases and error messages

   • The expression parser now throws when evaluation produces non-finite values — good. Add unit tests for edge cases: division by zero, pow() with huge exponents, NaN/INF propagation, and unexpected identifiers. Also add tests that validate() rejects syntactically valid but semantically problematic trees.

8. is_safe_static_calculated_field recursion and circular references

   • The implementation attempts to avoid circular references by tracking $resolving_fields — good. Make sure the limit on macro tokens and formula length is sufficient to prevent DoS via deep recursion chains. Consider adding a max dependency graph traversal depth.

9. Missing tests for high-risk changes

   • This PR changes payment protection behavior and adds a new grammar/validation layer for calculated price expressions. Add unit/integration tests for:
     • Trusted price resolver: validate(), parse(), error conditions, supported constants and math calls.
     • Secure price migration: ensure forms with supported static sources are auto-enabled and unsupported ones remain disabled.
     • get_affected_form behaviour to ensure no mutation occurs during inspection.

Suggestions / small improvements

• Rename enable_price_protection() if it's a pure predicate to avoid confusion (see point 1).
• Remove unused use statements.
• Add inline comments/docblocks documenting why some calculated fields are accepted (e.g. no ^ operator, no dynamic containers) so future maintainers understand constraints.
• Consider logging (or exposing in UI) which forms were auto-enabled by the migration so admins can verify.

Files touched (callouts)

• modules/gateways/secure-price-notice.php
  • Watch for potential side effects in get_affected_form() and remove unused imports.
  • Performance: Block_Helper::get_blocks_by_post usage and recursion in find_price_field_blocks/is_safe_calculated_formula.
• modules/gateways/trusted-price-resolver-expression-parser.php
  • Good separation of parse/validate and additional node validation. Add unit tests for validate_node/assert_supported_math_call and non-finite results.
• modules/post-type/module.php
  • Comment change mentioning the migration approach — OK.

Overall

• Functionality and security improvements are in the right direction, especially stricter expression validation and refusing non-finite results. However the potential side-effectful call in a read-path and the schema bump that may auto-enable protections for some forms are high-impact changes: verify enable_price_protection() is side-effect-free or move mutation to an explicit migration step. Add unit tests around the parser and the migration to reduce risk.

Suggested changelog entry

- IMPROVE: Automatically enable Secure payment amount for strictly verified static price sources in existing forms (gateways); add stricter validation for calculated price expressions.