Skip to content

Fix 359#512

Merged
sorinmarta merged 6 commits into
masterfrom
fix/radio-meta-xss
Jun 25, 2026
Merged

Fix 359#512
sorinmarta merged 6 commits into
masterfrom
fix/radio-meta-xss

Conversation

@sorinmarta

@sorinmarta sorinmarta commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Validate submitted options and escape legacy radio values on display.
@sorinmarta sorinmarta changed the title Fix stored XSS in radio meta fields Fix 359 Jun 24, 2026
@sorinmarta sorinmarta added the run analysis Runs phpcs and phpunit label Jun 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@includes/fields/class-fieldtypes-radiobutton.php`:
- Around line 96-111: The radio input IDs in the field rendering logic are not
guaranteed to be unique because the ID built in the radio option loop only uses
normalize_name($label), which can collapse distinct labels into the same DOM id.
Update the ID generation in the radio rendering method to include an unambiguous
unique suffix from the current option (such as the loop index or option key) so
each input/label pair in this class remains distinct and the label[for]
attribute always points to the correct radio.
- Around line 133-140: The radio field save path in
class-fieldtypes-radiobutton::store_field_value is treating invalid submitted
values the same as empty values, so tampered input still reaches the parent
store logic and clears the stored meta. Update the flow to distinguish an
invalid submission (from convert_input() returning an empty string after bad
input) from a user intentionally clearing the field, and only call
parent::store_field_value() when the value is truly valid or intentionally
empty; otherwise preserve the existing saved choice and avoid deleting or
overwriting the _wpbdp[fields][id] meta.

In `@tests/wpunit/Fields/RadioButtonFieldTest.php`:
- Around line 99-111: The create_listing() helper currently only checks that
wp_insert_post() returned an int, which still allows 0 to pass and masks fixture
setup failures. Update create_listing() to validate that the returned listing ID
is a positive integer, or switch wp_insert_post() to return WP_Error on failure
and assert/fail accordingly so invalid listing IDs are never returned.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7cccafbf-f485-419c-83a5-3e28ed362ef4

📥 Commits

Reviewing files that changed from the base of the PR and between 5b30135 and 9f0ce9e.

📒 Files selected for processing (2)
  • includes/fields/class-fieldtypes-radiobutton.php
  • tests/wpunit/Fields/RadioButtonFieldTest.php

Comment thread includes/fields/class-fieldtypes-radiobutton.php
Comment thread includes/fields/class-fieldtypes-radiobutton.php
Comment thread tests/wpunit/Fields/RadioButtonFieldTest.php
@Strategy11 Strategy11 deleted a comment from coderabbitai Bot Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c4e03387-f74c-4209-a655-8f192c8709c3

📥 Commits

Reviewing files that changed from the base of the PR and between 9f0ce9e and b298af1.

⛔ Files ignored due to path filters (1)
  • pnpm-workspace.yaml is excluded by !**/*.yaml
📒 Files selected for processing (2)
  • includes/fields/class-fieldtypes-radiobutton.php
  • tests/wpunit/Fields/RadioButtonFieldTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • includes/fields/class-fieldtypes-radiobutton.php

Walkthrough

Updates radio-button field handling for meta-associated values by validating submitted options, tracking invalid submissions, changing meta storage/deletion behavior, and escaping displayed HTML values. Adds tests for valid, invalid, empty, special-character, and legacy stored values.

Changes

RadioButton field input validation, storage, and rendering

Layer / File(s) Summary
Input validation and stored-options helper
includes/fields/class-fieldtypes-radiobutton.php
Adds invalid-input tracking, normalizes stored options, and validates submitted radio values by association during convert_input().
Association-aware rendering and storage
includes/fields/class-fieldtypes-radiobutton.php
Updates checked-state rendering, derives radio ids and values from association type, conditionally stores or deletes meta values, and renders meta HTML values as escaped comma-separated text.
Meta radio field tests
tests/wpunit/Fields/RadioButtonFieldTest.php
Adds tests and helpers covering valid storage, special-character options, invalid submissions, empty submissions, preserved stored values, and legacy HTML escaping.

Sequence Diagram

sequenceDiagram
  participant ListingFields as $_POST['listingfields'][field_id]
  participant RadioButton as WPBDP_FieldTypes_RadioButton
  participant PostMeta as post meta
  ListingFields->>RadioButton: convert_input($field, $input)
  RadioButton->>RadioButton: get_stored_options()
  RadioButton-->>ListingFields: string value or empty string
  ListingFields->>RadioButton: store_field_value($field, $post_id, $value)
  RadioButton->>PostMeta: update_post_meta() or delete_post_meta()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is too generic to describe the actual change. Use a concise, specific title such as 'Fix radio values with special characters'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description links to the issue targeted by this PR and is related to the change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/radio-meta-xss

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 PHPStan (2.2.2)

PHPStan was skipped because the user-provided config is missing the required paths: directive.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@garretlaxton

Copy link
Copy Markdown
Contributor

Radio options containing a < character (e.g. Price < 100) silently fail to save when submitting a listing.

@sorinmarta

Copy link
Copy Markdown
Contributor Author

Thanks @garretlaxton, my last commit should fix that

@garretlaxton garretlaxton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this works great!

@sorinmarta
sorinmarta merged commit d88181a into master Jun 25, 2026
5 checks passed
@sorinmarta
sorinmarta deleted the fix/radio-meta-xss branch June 25, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run analysis Runs phpcs and phpunit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants