[wp-trac] [WordPress Trac] #65055: _pad_term_counts() uses string-concatenated SQL without prepared statement

[WordPress Trac]

#65055: _pad_term_counts() uses string-concatenated SQL without prepared statement
-------------------------------------+------------------------------
 Reporter:  rajeshcp                 |       Owner:  rajeshcp
     Type:  defect (bug)             |      Status:  assigned
 Priority:  normal                   |   Milestone:  Awaiting Review
Component:  Database                 |     Version:  trunk
 Severity:  major                    |  Resolution:
 Keywords:  has-patch needs-testing  |     Focuses:
-------------------------------------+------------------------------

Comment (by liaison):

 Replying to [comment:3 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:
 {{{#!php
 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 ] );
         }
     }
     // ...
 }
 }}}


 {{{#!php

 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?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/65055#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform