Avoid stripping data from radio button/checkbox options#2192
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant Field
participant Sanitizer
User->>Field: Submits form input
Field->>Sanitizer: Sanitize input value
Sanitizer->>Field: Apply sanitization
Field->>Field: Check if sanitized value matches options
Field-->>User: Preserve original value if matches options
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldType.php (1)
1454-1458: Consider a unified approach for sanitizing + preserving option values
This block duplicates the pattern of storing$unsanitized_value, then sanitizing$valuebefore callingmaintain_option_values(). You might consider extracting this into a helper method shared by bothget_display_value()andsanitize_value()to reduce repetition and ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldType.php(2 hunks)
🔇 Additional comments (1)
classes/models/fields/FrmFieldType.php (1)
1658-1664: Verify type handling when sanitizing with HTML
In sanitize_value(), $value can be an array or a scalar. Please ensure that callers correctly handle these types and that no data is lost or inadvertently exposed.
✅ Verification successful
Let me analyze the code further to understand the type handling.
Based on the analysis of the code, I can now provide a final response:
Type handling in sanitize_value is properly implemented
The implementation safely handles both scalar and array values:
FrmAppHelper::sanitize_value()recursively processes arrays through its array check- For scalar values,
sanitize_with_html()applies appropriate sanitization based on user role:- Admin users:
wp_kses_post(preserves safe HTML) - Regular users:
strip_most_html(only allows basic HTML tags like b, br, p, etc.)
- Admin users:
- The test cases confirm proper handling of different input types and expected sanitization behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script searches all occurrences of sanitize_value to verify type usage.
rg -A 3 'sanitize_value\('
Length of output: 8060
Script:
#!/bin/bash
# Check the test cases to understand type handling
ast-grep --pattern 'test_sanitize_value() {
$$$
}'
Length of output: 54
Script:
#!/bin/bash
# Let's check the test file content directly
rg -A 20 "test_sanitize_value" "tests/phpunit/fields/test_FrmFieldType.php"
Length of output: 569
Script:
#!/bin/bash
# Let's check the implementation of sanitize_with_html and strip_most_html
rg -A 10 "sanitize_with_html|strip_most_html" "classes/helpers/FrmAppHelper.php"
Length of output: 889
Crabcyborg
deleted the
avoid_stripping_data_from_radio_button_and_checkbox_options
branch
1 participant
Fixes https://github.com/Strategy11/formidable-pro/issues/5559
Pre-release
formidable-6.16.4b.zip
Fix verified by Dimitris