Make WordPress Core

Opened 15 months ago

Last modified 7 days ago

#56091 assigned enhancement

Using %i for table/field names in wpdb::prepare()

Reported by: craigfrancis's profile craigfrancis Owned by: craigfrancis's profile craigfrancis
Milestone: 6.5 Priority: low
Severity: minor Version: 6.1
Component: Database Keywords: has-patch early
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Now wpdb::prepare() supports %i for Identifiers (e.g. table/field names), via commit [53575], and ticket #52506.

Queries within WP Core should use this, to ensure variables are always quoted, and avoid static analysis tools flagging unescaped SQL input (a non-literal-string) for the $query parameter:

$wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s", $post_type );

$wpdb->prepare( "SELECT ID FROM %i WHERE post_type = %s", $wpdb->posts, $post_type );

I'll write a patch for the first set, but I suspect there will be a lot of changes, and they should be checked carefully.

Change History (13)

#1 @SergeyBiryukov
15 months ago

  • Description modified (diff)

This ticket was mentioned in PR #3016 on WordPress/wordpress-develop by craigfrancis.

14 months ago

  • Keywords has-patch added; needs-patch removed

First patch, only affecting SQL that is used directly by $wpdb->get_col(), $wpdb->get_var(), etc.

Trac ticket: 56091

craigfrancis commented on PR #3016:

14 months ago

Created PR 2072 for WordPress-Coding-Standards (ref "Unsupported placeholder used in $wpdb->prepare()").

#4 @craigfrancis
14 months ago

First patch: Parameterise some table names.

It will help Static Analysis / Sniffs, especially when IN(%...d) is supported (via #54042), as makes it easy to check all variables are escaped correctly.

But I found some of these changes make the SQL a bit harder to read, as it can take a second to work out which table is being used (maybe that will get easier once I'm used to it?)

This patch only affects a few queries. I do have more, where I've been able to get most of WordPress working with the $table_prefix set to 'wp ' (with a space), but some changes get a bit complicated, and I doubt many (any?) developers need a $table_prefix that contains characters other than [a-zA-Z0-9_].

A different approach, specifically for $table_prefix, would use the literal-string type (supported in Psalm and PHPStan). This ensures a property/variable only contains a "developer defined string", and it supports concatenation.

This means properties like $wpdb->posts could use this type, and continue to be concatenated directly into the SQL string without escaping (ish). Then static analsysis tools (that trace variables from source to sink) could check the first argument to wpdb::prepare() is still a literal-string. Admittedly this won't work for variables that cannot be defined as a literal-string, or when they contain any non-permitted characters, or any of the ~286 reserved words (which is why we need %i).

#5 @davidbaumwald
12 months ago

  • Keywords early added
  • Milestone changed from 6.1 to 6.2

This still needs some work, and I think this should go in early in a cycle.

Moving this to 6.2 for continued work after r53575 has some time out in the wild.

#6 @uzumymw
12 months ago

Is it impossible to fight with this change for version 6.1?

#7 @craigfrancis
12 months ago

  • Summary changed from Use %i for table/field names in wpdb::prepare() to Using %i for table/field names in wpdb::prepare()

Hi @uzumymw, and anyone who's interested, I agree with David, this patch needs to be done carefully, and with discussion.

Just to note, "%i" will be available in 6.1 (assuming I've not made a massive mistake somewhere), where it ensures variable identifiers (e.g. table/field names) are escaped correctly... but this patch is about using "%i" in core, and in some cases I think it can make the code harder to read, for little to no benefit (I want to avoid those situations).

I'm happy to chat about this on WP Slack, or email (craig [at] craigfrancis [dot] co [dot] uk).

This ticket was mentioned in Slack in #core by audrasjb. View the logs.

9 months ago

#9 @audrasjb
9 months ago

  • Milestone changed from 6.2 to Future Release

As per today's bug scrub, it looks like this ticket still needs review/testing. Moving to Future Release. @craigfrancis @davidbaumwald feel free to move it back to 6.2 if you think it can be committed in time (early tickets should be committed by next week).

#10 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 6.4

#11 @SergeyBiryukov
2 months ago

This came up in a coding session with @afercia, @aristath, and @poena, where we tried to address the existing WPCS warnings for missing or incorrect usage of $wpdb->prepare() in core, so I'm moving this to 6.4 for review.

We have also noticed that the %i placeholder is not currently recognized by WPCS, as seen on the PR:

Unsupported placeholder used in $wpdb->prepare(). Found: "%i".

This appears to be fixed in an upcoming version of WPCS.

In our testing, this silences the error for now, until WPCS is updated in core:

// The placeholder ignores can be removed when %i is supported by WPCS.
// phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.UnsupportedPlaceholder, WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.

7 weeks ago

#13 @oglekler
7 days ago

  • Milestone changed from 6.4 to 6.5

Because this is an early ticket, I am moving it into 6.5 milestone.

Note: See TracTickets for help on using tickets.