Make WordPress Core

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#43056 closed defect (bug) (fixed)

Notice in redirect_guess_404_permalink() when post type is an array

Reported by: junaidbhura's profile junaidbhura Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Canonical Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

When manipulating URLs, it sometimes becomes necessary to update the query using the pre_get_posts hook.

If we set the post type to an array in the pre_get_posts hook like so:

$query->set( 'post_type', array( 'post', 'page', 'my_cpt' ) );

On a 404 page, we get a notice when WP_DEBUG is set to true:

Notice: wpdb::prepare was called incorrectly. The query only expected one placeholder, but an array of multiple placeholders was sent.

This is caused by the following code in redirect_guess_404_permalink():

$where .= $wpdb->prepare(" AND post_type = %s", get_query_var('post_type'));

This can be fixed by looking for an array and updating the query.

Attachments (5)

43056.diff (853 bytes) - added by junaidbhura 7 years ago.
canonical.php.patch (973 bytes) - added by Enchiridion 6 years ago.
43056.2.diff (949 bytes) - added by Enchiridion 6 years ago.
Small optimization added
43056.3.diff (865 bytes) - added by junaidbhura 4 years ago.
43056.4.diff (1.9 KB) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (25)

@junaidbhura
7 years ago

#1 @junaidbhura
7 years ago

  • Keywords has-patch added

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


7 years ago

#3 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 5.0

#4 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.0 to 5.1

#5 @pento
6 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to Future Release
  • Version trunk deleted

I suspect 43056.diff will introduce SQL injection issues. wpdb:prepare() won't put quotes around each element of the array being sent to it when replacing into the %s.

#6 @Enchiridion
6 years ago

This issue has been bugging me too. I've updated the patch with SQL escaping.

#7 @laternastudio
6 years ago

Would love to see this released soon!

#8 @Enchiridion
6 years ago

  • Keywords needs-refresh removed

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


6 years ago

@Enchiridion
6 years ago

Small optimization added

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


4 years ago

#11 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.6

#12 @peterwilsoncc
4 years ago

I've taken a look at 43056.2.diff

The escaping looks good with esc_url, the post types are verified as legitimate in class-wp.php.

You'll notice that at the moment, the canonical guess function uses get_query_var( 'post_type' ) calls multiple times. This is to act as a safety switch so that the value of a variable isn't changed by mistake -- this can lead to problems ;)

Rather than putting the value in a variable, I think calling get_query_var( 'post_type' ) each time would an improvement.

@junaidbhura
4 years ago

#13 @junaidbhura
4 years ago

Nice catch @peterwilsoncc ! I've updated and attached a new diff, could you please take a look?

This ticket was mentioned in PR #480 on WordPress/wordpress-develop by peterwilsoncc.


4 years ago
#14

  • Keywords has-unit-tests added

#15 @peterwilsoncc
4 years ago

  • Owner set to peterwilsoncc
  • Status changed from new to assigned

@junaidbhura I've modified your patch slightly in PR 480 on the GitHub repo:

  • The escaping was a little out, $wpdb->prepare() can't be used for IN so the query would have being incorrect, it would have looked for a post type of an apparently random concatenation of the post types. I've replaced that with the counter-intuitive method of correctly escaping LIKEs.
  • I've added some unit tests too for array formatted post types. I've used the publicly queryable post types in a query string rather than the pre_get_posts filter example in your initial report.

#16 @junaidbhura
4 years ago

Thanks @peterwilsoncc ! I've added a couple of comments on your PR. Could you please take a look?

#17 @peterwilsoncc
4 years ago

43056.4.diff is the final state of the PR with tests and @junaidbhura's code

#18 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49200:

Canonical: Support multiple post types in redirect_guess_404_permalink().

Prevent redirect_guess_404_permalink() from throwing a notice when multiple post types are included in the post_type query.

Props junaidbhura.
Fixes #43056.

#20 @junaidbhura
4 years ago

Sweet thanks @peterwilsoncc !

Note: See TracTickets for help on using tickets.