Opened 5 weeks ago
Last modified 3 weeks ago
#65055 reviewing enhancement
_pad_term_counts() uses string-concatenated SQL without prepared statement
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 7.1 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Taxonomy | Keywords: | has-patch has-unit-tests has-test-info |
| Focuses: | Cc: |
Description
$object_types values are not individually escaped via $wpdb->prepare(). They use esc_sql() only at the
get_taxonomy() call, but imploded directly into the query string. The array keys are integer IDs but are not cast.
This should use prepare() with placeholders.
Attachments (1)
Change History (11)
This ticket was mentioned in PR #11541 on WordPress/wordpress-develop by @rajeshcp.
5 weeks ago
#1
This ticket was mentioned in PR #11542 on WordPress/wordpress-develop by @rajeshcp.
5 weeks ago
#2
_pad_term_counts() uses string-concatenated SQL without prepared statement
$object_types values are not individually escaped via $wpdb->prepare(). They use esc_sql() only at the
get_taxonomy() call, but imploded directly into the query string. The array keys are integer IDs but are not cast.
This should use prepare() with placeholders.
Trac ticket: https://core.trac.wordpress.org/ticket/65055
Fixes #65055
## Use of AI Tools
#3
follow-up:
↓ 4
@
5 weeks ago
The current code isn't exploitable — esc_sql() does handle escaping correctly here, and the values involved are all server-side (database IDs and registered post types, not user input). But switching to $wpdb->prepare() is still the right call as a best-practice improvement. The patch looks good.
One small addition worth considering: _update_post_term_count() filters object_types through post_type_exists() before querying but _pad_term_counts() doesn't. Could be worth adding for consistency.
#4
in reply to:
↑ 3
;
follow-up:
↓ 5
@
5 weeks ago
Replying to abcd95:
The current code isn't exploitable — esc_sql() does handle escaping correctly here, and the values involved are all server-side (database IDs and registered post types, not user input). But switching to $wpdb->prepare() is still the right call as a best-practice improvement. The patch looks good.
One small addition worth considering: _update_post_term_count() filters object_types through post_type_exists() before querying but _pad_term_counts() doesn't. Could be worth adding for consistency.
@abcd95 I agree that aligning this with _update_post_term_count() is a great point for consistency. Filtering out unregistered post types would indeed make the query more robust.
I’ve drafted a refined version of the patch based on your suggestion:
function _update_post_term_count( $terms, $taxonomy ) {
// ...
$object_types = (array) $tax_obj->object_type;
foreach ( $object_types as $index => $object_type ) {
if ( ! post_type_exists( $object_type ) ) {
unset( $object_types[ $index ] );
}
}
// ...
}
function _pad_term_counts( &$terms, $taxonomy ) {
// Get the object and term IDs and stick them in a lookup table.
$tax_obj = get_taxonomy( $taxonomy );
+ $object_types = (array) $tax_obj->object_type;
+ // --- filter invalid post types ---
+ $object_types = array_filter( $object_types, 'post_type_exists' );
+
+ if ( empty( $object_types ) ) {
+ return; // if no valid post type,return directly
+ }
+ $object_types = esc_sql( $object_types );
- $object_types = esc_sql( $tax_obj->object_type );
- $results = $wpdb->get_results( "SELECT object_id, term_taxonomy_id FROM $wpdb->term_relationships INNER JOIN $wpdb->posts ON object_id = ID WHERE term_taxonomy_id IN (" . implode( ',', array_keys( $term_ids ) ) . ") AND post_type IN ('" . implode( "', '", $object_types ) . "') AND post_status = 'publish'" );
-
+ $results = $wpdb->get_results(
+ $wpdb->prepare(
+ "SELECT object_id, term_taxonomy_id FROM $wpdb->term_relationships INNER JOIN $wpdb->posts ON object_id = ID WHERE term_taxonomy_id IN (" . implode( ',', array_fill( 0, count( $term_ids ), '%d' ) ) . ') AND post_type IN (' . implode( ',', array_fill( 0, count( $object_types ), '%s' ) ) . ") AND post_status = 'publish'",
+ array_merge( array_keys( $term_ids ), $object_types )
+ )
+ );
foreach ( $results as $row ) {
$id = $term_ids[ $row->term_taxonomy_id ];
This seems to cover the concern perfectly. @rajeshcp, what do you think about incorporating this filter into the PR?
#5
in reply to:
↑ 4
;
follow-up:
↓ 6
@
5 weeks ago
- Keywords needs-testing removed
Replying to liaison:
Thanks @liaison! agreed on the alignment with _update_post_term_count(). The updated patch incorporating post_type_exists() filtering looks solid.
#6
in reply to:
↑ 5
@
5 weeks ago
Replying to abcd95:
Replying to liaison:
Thanks @liaison! agreed on the alignment with _update_post_term_count(). The updated patch incorporating post_type_exists() filtering looks solid.
Test Report
Summary:
I verified the fix using a zero-dependency test script to confirm the patch logic locally before implementing formal PHPUnit test cases for the GitHub CI suite. This script loads the core wp-includes/taxonomy.php directly while mocking the database and global environment.
Before Patch:
The logic uses direct string interpolation and includes unregistered/invalid post types in the query.
Method: wpdb::get_results (Direct string)
Captured Query: ... AND post_type IN ('post', 'ghost_type') ...
Result: ❌ FAILED: 'ghost_type' (unregistered) is present in the raw SQL string.
After Patch:
The logic now correctly filters post types via post_type_exists() and utilizes wpdb::prepare() for a safer, parameterized query.
Method: wpdb::prepare
Captured Arguments: 123, post
Result: ✅ SUCCESS: 'ghost_type' was successfully filtered, and only valid post types are passed to the prepare arguments.
test-65055-sql-filter.php
<?php <?php /** * Trac #65055: Zero-Dependency Test for Core wp-includes/taxonomy.php * Supports both patched (prepare) and unpatched (direct query) versions. */ define( 'ABSPATH', __DIR__ . '/' ); define( 'WPINC', 'wp-includes' ); /** * 1. Robust Mock_WPDB * Handles table name properties and captures both prepare/get_results. */ class Mock_WPDB { public $queries = []; public $prefix = 'wp_'; // Core table names used in _pad_term_counts public $term_relationships = 'wp_term_relationships'; public $posts = 'wp_posts'; public function prepare( $query, ...$args ) { // Flatten arguments if they were passed as an array $flat_args = ( is_array( $args[0] ) && count( $args ) === 1 ) ? $args[0] : $args; $this->queries[] = [ 'method' => 'prepare', 'query' => $query, 'args' => $flat_args ]; return $query; } public function get_results( $query ) { // If query was not recorded by prepare(), it's a direct string query $already_recorded = false; foreach ( $this->queries as $q ) { if ( $q['query'] === $query ) { $already_recorded = true; break; } } if ( ! $already_recorded ) { $this->queries[] = [ 'method' => 'get_results', 'query' => $query, 'args' => [] ]; } // Return empty result to satisfy the function return array(); } } $GLOBALS['wpdb'] = new Mock_WPDB(); /** * 2. Mock Global Functions (Stubbing) */ if ( ! function_exists( 'post_type_exists' ) ) { function post_type_exists( $post_type ) { return in_array( $post_type, array( 'post' ), true ); } } if ( ! function_exists( 'wp_installing' ) ) { function wp_installing() { return false; } } if ( ! function_exists( 'esc_sql' ) ) { function esc_sql( $data ) { return $data; } } if ( ! function_exists( 'get_option' ) ) { function get_option( $option, $default = false ) { // Force get_term_hierarchy to believe Term 123 has a child 999 if ( strpos( $option, '_children' ) !== false ) { return array( 123 => array( 999 ) ); } return array(); } } /** * 3. Load actual Core file */ if ( file_exists( ABSPATH . WPINC . '/taxonomy.php' ) ) { require_once ABSPATH . WPINC . '/taxonomy.php'; } else { die( "❌ Error: wp-includes/taxonomy.php not found. Check your ABSPATH.\n" ); } /** * 4. Setup Global State */ global $wp_taxonomies; $test_tax = new stdClass(); $test_tax->name = 'test_tax'; $test_tax->object_type = array( 'post', 'ghost_type' ); // Mixed valid/invalid $test_tax->hierarchical = true; $wp_taxonomies['test_tax'] = $test_tax; /** * 5. Execute Test */ echo "--- Starting Core _pad_term_counts() Test ---\n"; $term = new stdClass(); $term->term_id = 123; $term->term_taxonomy_id = 123; $term->parent = 0; $term->count = 0; $terms_array = array( $term ); _pad_term_counts( $terms_array, 'test_tax' ); /** * 6. Final Analysis */ $last_execution = end( $GLOBALS['wpdb']->queries ); if ( ! $last_execution ) { die( "❌ FAILED: No database activity detected.\n" ); } echo "\n[Execution Result]\n"; echo "Method: " . $last_execution['method'] . "\n"; if ( $last_execution['method'] === 'prepare' ) { $args = $last_execution['args']; echo "Arguments: " . implode( ', ', $args ) . "\n"; if ( in_array( 'ghost_type', $args, true ) ) { echo "❌ FAILED: 'ghost_type' remains in prepare arguments.\n"; } else { echo "✅ SUCCESS: 'ghost_type' filtered from prepare arguments.\n"; } } else { echo "Query: " . $last_execution['query'] . "\n"; if ( strpos( $last_execution['query'], 'ghost_type' ) !== false ) { echo "❌ FAILED: 'ghost_type' present in raw SQL string.\n"; } else { echo "✅ SUCCESS: 'ghost_type' not found in SQL string.\n"; } } echo "\n--- Test Completed ---\n";
This ticket was mentioned in PR #11579 on WordPress/wordpress-develop by @liaison.
4 weeks ago
#7
This commit updates _pad_term_counts() to use $wpdb->prepare() for all database queries, improving security and consistency. Additionally, it introduces a filter using post_type_exists() to ensure that only registered post types are queried, aligning the behavior with _update_post_term_count().Includes comprehensive unit tests to verify the logic and prevent regressions.Fixes #65055.See #11542.
Description
This PR builds upon the work by @rajeshcp in #11542. It implements the $wpdb->prepare() fix for #65055 and adds comprehensive unit tests to ensure the logic is robust.
While drafting the unit tests, I identified a scenario where _pad_term_counts() could query non-existent post types (see test_pad_term_counts_with_invalid_post_types). Following @abcd95's suggestion, I've aligned the logic with _update_post_term_count() by filtering via post_type_exists().
Key Changes:
Security: Refactored direct SQL string interpolation to use $wpdb->prepare() to prevent potential SQL injection and ensure standard database interaction.
Consistency: Integrated array_filter( $object_types, 'post_type_exists' ) before the query execution. This aligns _pad_term_counts() with the logic found in _update_post_term_count(), preventing queries from running against invalid or ghost post types that may still exist in the taxonomy's object_type array.
Robustness: Added an early exit if no valid post types remain after filtering, saving unnecessary database calls.
Unit Testing:
I have verified these changes with a zero-dependency local test script, confirming that the SQL arguments are correctly filtered and parameterized compared to the unpatched version.
Trac ticket: https://core.trac.wordpress.org/ticket/65055
Use of AI Tools
AI assistance: Yes
Tool(s): Gemini
Model(s): Gemini 3 Flash
Used for: Brainstorming the zero-dependency test script to isolate the core logic during local verification; assisting in drafting the technical description and PR documentation based on my local test results. All code changes and logic were manually implemented and verified by me.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
_pad_term_counts() uses string-concatenated SQL without prepared statement
$object_types values are not individually escaped via $wpdb->prepare(). They use esc_sql() only at the
Trac ticket: https://core.trac.wordpress.org/ticket/65055
Fixes #65055
## Use of AI Tools