Make WordPress Core

Opened 8 years ago

Last modified 6 months ago

#38709 assigned defect (bug)

Unlimited query for invalid post names

Reported by: davidbinda's profile david.binda Owned by: pbearne's profile pbearne
Milestone: Future Release Priority: normal
Severity: normal Version: 4.6.1
Component: Query Keywords: needs-patch
Focuses: performance Cc:

Description

In case a request which is being parsed as one which sets a "name" query var (eg.: http://localhost/wordpress-latest/2016/11/08/[ ) includes an invalid name ( the [ ), a query without any limit with empty post_name is triggered:

SELECT wp_posts.*
FROM wp_posts 
WHERE 1=1 
AND ( ( YEAR( wp_posts.post_date ) = 2016
AND MONTH( wp_posts.post_date ) = 11
AND DAYOFMONTH( wp_posts.post_date ) = 8 ) )
AND wp_posts.post_name = ''
AND wp_posts.post_type = 'post' 
ORDER BY wp_posts.post_date DESC

The problem with the query is that it matches all the drafts in the database, since they don't have the post_name set (is empty). This may have a performance implications for the database in case it contains a lot of drafts (or posts without the post_name set for any reason) as it queries all the posts.

The post_name is being set as empty due to sanitize_title_for_query in https://core.trac.wordpress.org/browser/tags/4.6.1/src/wp-includes/query.php#L2717

I have tested a more suitable validation function than trim ( eg.: sanitize_title ) in https://core.trac.wordpress.org/browser/tags/4.6.1/src/wp-includes/query.php#L1609 but it leads to archives being served on those invalid URLs, which does not seem like good fix.

Trying to set posts_per_page to 1 in case the name is parsed based on the rewrite rule (which seems like a good thing since we really should be interested in a single and unique post_name in such cases) hits a wall as the is_singular() returns true and the limit is not set even if the posts_per_page is present ( due to https://core.trac.wordpress.org/browser/tags/4.6.1/src/wp-includes/query.php#L3215 ).

So the fix might need to be more complicated than.

Attachments (4)

core.patch (595 bytes) - added by rebasaurus 23 months ago.
Maybe I'm oversimplifying it, but could the fix be just checking the value after it has ran through sanitize_title_for_query()?
class-wp-query.php (148.1 KB) - added by Khadreal 9 months ago.
The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. Forcing a limit when it's singular and sanitize the input kind of do the trick and resolve it.
query.patch (682 bytes) - added by Khadreal 9 months ago.
The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. Forcing a limit when it's singular and sanitize the input kind of do the trick and resolve it.
core.2.patch (938 bytes) - added by Khadreal 9 months ago.
The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. I set is_singular to false when the query name is empty and it's singular and not a preview. PS. My last two comments were mistake and I couldn't delete

Download all attachments as: .zip

Change History (13)

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


3 years ago

#2 @kirasong
3 years ago

  • Component changed from Permalinks to Query
  • Keywords needs-patch added

Hi @davidbinda!

Thanks so much for the report, and apologies for the extremely long time waiting for a response.

This ticket was discussed in a triage session today.

Investigating, @peterwilsoncc found that this does still seem to be an issue, and an EXPLAIN on that indicates it's using filesort so the indexes are missed.

Since this seems to be an issue coming from the Query component, going to move it there for further attention, and mark this as needs-patch.

@rebasaurus
23 months ago

Maybe I'm oversimplifying it, but could the fix be just checking the value after it has ran through sanitize_title_for_query()?

#3 @rebasaurus
23 months ago

  • Keywords has-patch added; needs-patch removed

cc @peterwilsoncc

#4 @joemcgill
15 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

@iCaleb and I were testing this ticket at WCUS 2023 contributor day and it would be great to get this fixed. The patch @rebasaurus added is very close, but doesn't result in the value actually getting sanitized in it's current form. What if we move the sanitization to above the if statement instead? The only downside is that we're unnecessarily running the sanitization on known empty name values, but in practice, seems like a much less impactful problem then what is occurring now.

@Khadreal
9 months ago

The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. Forcing a limit when it's singular and sanitize the input kind of do the trick and resolve it.

@Khadreal
9 months ago

The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. Forcing a limit when it's singular and sanitize the input kind of do the trick and resolve it.

This ticket was mentioned in Slack in #slackhelp by khadreal. View the logs.


9 months ago

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


9 months ago

@Khadreal
9 months ago

The patch suggested by @rebasaurus remove the query but doesn't resolve the issue as draft post still shows up. I set is_singular to false when the query name is empty and it's singular and not a preview. PS. My last two comments were mistake and I couldn't delete

This ticket was mentioned in PR #6534 on WordPress/wordpress-develop by @pbearne.


6 months ago
#7

  • Keywords has-patch added; needs-patch removed

#8 @pbearne
6 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to pbearne
  • Status changed from new to assigned

#9 @pbearne
6 months ago

We can't set is_singular to false as this brack a load of tests (functions)

So we are going to look at this a bit more

Note: See TracTickets for help on using tickets.