Make WordPress Core

Opened 2 years ago

Last modified 2 months 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: Future Release 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:

<?php
$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 (16)

#1 @SergeyBiryukov
2 years ago

  • Description modified (diff)

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


23 months ago
#2

  • 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:


23 months ago
#3

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

#4 @craigfrancis
23 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
21 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
21 months ago

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

#7 @craigfrancis
21 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.


17 months ago

#9 @audrasjb
17 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
11 months ago

  • Milestone changed from Future Release to 6.4

#11 @SergeyBiryukov
11 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.


10 months ago

#13 @oglekler
9 months ago

  • Milestone changed from 6.4 to 6.5

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

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


5 months ago

#15 @audrasjb
5 months ago

  • Milestone changed from 6.5 to Future Release

Hey there, it looks like the needed change was released with WPCS 3.0.0.

However, let's move this ticket to Future Release as 6.5 beta 1 is pretty close, now.
@SergeyBiryukov if you believe this ticket is ready to ship, feel free to move it back to 6.5.

#16 @johnbillion
2 months ago

@craigfrancis Do you want to refresh this for 6.6? I'd love to remove this PHPCS config so unprepared queries trigger an error instead of a warning. Implementing %i gets us a step closer to that.

Note: See TracTickets for help on using tickets.