Add extra_data column and contrast data collection (DB 1.0.8) - #1802
Add extra_data column and contrast data collection (DB 1.0.8)#1802pattonwebz wants to merge 4 commits into
Conversation
Extracts the infrastructure-only portion of PR #1589: adds `extra_data text NULL` to `wp_accessibility_checker`, stores JSON metadata during scanner inserts, and collects color contrast values (fg/bg color, ratio, font size/weight) from axe-core results for `color_contrast_failure` violations. Output/display side (highlighter swatches, admin HTML attributes, IssueDetailsModal changes) is intentionally excluded and remains in PR #1589. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an Changesextra_data persistence pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an extra_data column to the database schema (version 1.0.8) to store arbitrary JSON metadata, such as color contrast values, alongside accessibility violations. It updates the Insert_Rule_Data class, the REST API, and the frontend page scanner to process and store this new metadata. The review feedback identifies three critical issues: a potential Stored XSS vulnerability due to unsanitized $extra_data being stored in the database, potential JSON corruption from applying sanitize_text_field() directly to a JSON string, and a MySQL syntax error in the migration script caused by using IF NOT EXISTS in an ALTER TABLE statement (which is redundant as dbDelta already handles this).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 'rule' => $rule, | ||
| 'ruletype' => $ruletype, | ||
| 'object' => esc_attr( $rule_obj ), | ||
| 'extra_data' => $extra_data ? wp_json_encode( $extra_data ) : null, |
There was a problem hiding this comment.
The $extra_data array is encoded and stored directly in the database on updates without any sanitization. Since this data originates from the REST API (user input), this creates a Stored XSS vulnerability if a user with edit permissions submits a malicious payload.
To prevent this, we should recursively sanitize the $extra_data array before JSON encoding it.
'extra_data' => $extra_data ? ( function( $data ) {
array_walk_recursive( $data, function( &$val ) {
$val = sanitize_text_field( $val );
} );
return wp_json_encode( $data );
} )( $extra_data ) : null,There was a problem hiding this comment.
Fixed. A private static encode_extra_data( ?array $data ): ?string helper was added that recursively walks the array and applies sanitize_text_field() to every string value before JSON-encoding. It is called in both the UPDATE and INSERT paths so all writes go through the same sanitization gate.
Introduced in f27bd5e ("Address CodeRabbit/Gemini review comments") and further refined in 967245f (made private static) and 0c61427 (rubber-duck review: INSERT and UPDATE paths now both call encode_extra_data() on the raw PHP array, not on a pre-encoded JSON string).
| 'rule' => sanitize_text_field( $rule_data['rule'] ), | ||
| 'ruletype' => sanitize_text_field( $rule_data['ruletype'] ), | ||
| 'object' => esc_attr( $rule_data['object'] ), | ||
| 'extra_data' => isset( $rule_data['extra_data'] ) ? sanitize_text_field( $rule_data['extra_data'] ) : null, |
There was a problem hiding this comment.
Using sanitize_text_field() directly on a JSON string is highly problematic. It strips HTML tags, converts < characters, and removes line breaks, which will corrupt the JSON structure if any values contain HTML-like characters (such as CSS selectors containing > or future manual descriptions containing HTML).
Instead, we should decode the JSON, recursively sanitize the individual string values, and then re-encode it.
'extra_data' => isset( $rule_data['extra_data'] ) ? ( function( $json ) {
$decoded = json_decode( $json, true );
if ( is_array( $decoded ) ) {
array_walk_recursive( $decoded, function( &$val ) {
$val = sanitize_text_field( $val );
} );
return wp_json_encode( $decoded );
}
return null;
} )( $rule_data['extra_data'] ) : null,References
- When ensuring a variable is an array, prefer using an
if...thenconditional check over(array)type casting to maintain consistency with existing code patterns. - When the contract for a variable is a specific type (e.g., an array of commands), prefer a stricter type check (e.g.,
is_array) over a more general one (e.g.,is_iterable) to enforce that contract.
There was a problem hiding this comment.
Fixed. The original approach of calling sanitize_text_field() on an already-encoded JSON string is gone. The encode_extra_data() helper now receives the raw PHP array, sanitizes string values within it, and then encodes — so sanitize_text_field never sees a JSON string. In the sanitized-copy block (INSERT path), the field is decoded from whatever the filter returned before being passed to encode_extra_data(), handling the edge case where a filter sets the field to a JSON string.
Fixed in f27bd5e ("Address CodeRabbit/Gemini review comments"), with the encode-before-filter design flaw corrected in 0c61427 ("Fix bugs found in rubber-duck review").
| // Add extra_data column for storing arbitrary JSON metadata alongside issues. | ||
| if ( version_compare( $db_version, '1.0.8', '<' ) ) { | ||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.DirectDatabaseQuery.SchemaChange, WordPress.DB.PreparedSQLPlaceholders.UnsupportedIdentifierPlaceholder -- One-time schema migration; %i is a valid identifier placeholder since WP 6.2. | ||
| $wpdb->query( $wpdb->prepare( 'ALTER TABLE %i ADD COLUMN IF NOT EXISTS extra_data text NULL', $table_name ) ); | ||
| } |
There was a problem hiding this comment.
MySQL does not support the IF NOT EXISTS clause for ALTER TABLE ADD COLUMN (this is a MariaDB-only extension). Running this query on standard MySQL servers will result in a database syntax error, preventing the database upgrade from completing successfully.
Since dbDelta( $sql ) is already called on line 81 with the updated schema containing extra_data text NULL, dbDelta will automatically and safely add the column if it does not exist. Therefore, this manual ALTER TABLE query is both redundant and broken, and should be removed.
// Schema updates for 1.0.8 (adding extra_data column) are handled automatically by dbDelta() above.There was a problem hiding this comment.
Fixed. The ALTER TABLE $table_name ADD COLUMN IF NOT EXISTS extra_data text NULL block was removed entirely. As the comment correctly identified, dbDelta() already handles this: it compares the declared CREATE TABLE SQL (which now includes the extra_data column) against the actual table structure and issues a plain ADD COLUMN only when the column is absent. Since extra_data is nullable, no data backfill is required — existing rows automatically receive NULL.
Removed in f27bd5e ("Address CodeRabbit/Gemini review comments").
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@admin/class-insert-rule-data.php`:
- Line 70: The `extra_data` value is being JSON-encoded without consistent
recursive sanitization across both insert and update paths in
`class-insert-rule-data.php`. Centralize normalization and sanitization in the
rule data flow (for example, in the insert/update handling methods around
`extra_data`) so rescans and new inserts both pass through the same safe
preprocessing before `wp_json_encode`. Ensure the shared sanitizing step is
applied before persisting `extra_data` in every path that writes it, not just in
one branch.
In `@admin/class-update-database.php`:
- Around line 93-97: The migration in class-update-database.php uses an ALTER
TABLE ADD COLUMN IF NOT EXISTS for extra_data, which is not safe on MySQL 5.7.
Update the upgrade path inside the db_version check in the database updater to
first detect whether the extra_data column already exists, then run a plain ADD
COLUMN only when it is missing. Keep the change within the existing
wpdb->prepare / $wpdb->query flow in the database migration logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02db9734-3d49-407c-9224-d5e090382438
📒 Files selected for processing (5)
accessibility-checker.phpadmin/class-insert-rule-data.phpadmin/class-update-database.phpincludes/classes/class-rest-api.phpsrc/pageScanner/index.js
- Remove ALTER TABLE ADD COLUMN IF NOT EXISTS block: MySQL 5.7 does not support this syntax (it's MariaDB-only). dbDelta() already handles adding the extra_data column when upgrading from < 1.0.8, so the explicit ALTER is redundant. - Fix sanitize_text_field() on JSON string: applying that function directly to an encoded JSON value corrupts it (strips < and > characters). Replace with a private encode_extra_data() helper that recursively sanitizes string values before encoding and is called in both the insert and update paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- encode_extra_data: handle wp_json_encode() returning false (returns null instead of propagating false into DB as empty string) - encode_extra_data: make private static since it doesn't reference $this; update call sites to use self:: - UPDATE SQL: remove stray double space before WHERE clause Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- extra_data in UPDATE path: switch from raw wpdb->prepare('%s', null) to
wpdb->update(), which routes through process_fields() and emits SQL NULL for
null values. prepare('%s', null) coerces null to '' causing a divergence between
first-scan INSERT (NULL) and rescan UPDATE ('').
- extra_data pre-encoded before filter: keep extra_data as a raw PHP array in
$rule_data so edac_filter_insert_rule_data receives native PHP types, consistent
with every other field. Encoding now happens at point of persistence. The sanitize
block handles the edge case where a filter sends a JSON string.
- index.js: add console.warn when color-contrast check is found but has no data,
so silent drops are visible during debugging.
- class-update-database.php: add comment explaining why 1.0.8 has no migration
guard (dbDelta handles it), consistent with the 1.0.5 and 1.0.7 comments.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
extra_data text NULLcolumn towp_accessibility_checkerInsert_Rule_Data::insert()with an optional$extra_dataarray parameter; JSON-encoded before storage, updated on rescanscolor_contrast_failureviolations and sends it asextraDatain the scan payloadset_post_scan_results()readsviolation['extraData']and passes it through toinsert()Intentionally excluded from this PR (remains in PR #1589):
src/frontendHighlighterApp/index.js,.scss)data-extra-dataHTML attribute output (admin/class-ajax.php,admin/class-frontend-highlight.php)IssueDetailsModalchangesThis is a prerequisite for the Manual Issues feature, which will store its user-authored fields (
manual_title,manual_description,manual_why_it_matters,manual_how_to_fix,screenshot_id) as JSON keys insideextra_datarather than adding dedicated columns.Test plan
extra_datacolumn is added on plugin loadextra_datacolumn is populated with JSON containingfgColor,bgColor,contrastRatio,expectedContrastRatio,fontSize,fontWeightextra_datais updated, not duplicatedextra_data = NULL🤖 Generated with Claude Code
Summary by CodeRabbit