Make WordPress Core

Opened 11 months ago

Last modified 5 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:

$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 (9)

#1 @SergeyBiryukov
11 months ago

  • Description modified (diff)

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

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

10 months ago

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

#4 @craigfrancis
10 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
8 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
8 months ago

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

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

5 months ago

#9 @audrasjb
5 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).

Note: See TracTickets for help on using tickets.