Make WordPress Core

Opened 7 years ago

Last modified 4 weeks ago

#38709 new defect (bug)

Unlimited query for invalid post names

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


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

I have tested a more suitable validation function than trim ( eg.: sanitize_title ) in 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 ).

So the fix might need to be more complicated than.

Attachments (1)

core.patch (595 bytes) - added by rebasaurus 9 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()?

Download all attachments as: .zip

Change History (5)

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

2 years ago

#2 @mikeschroder
2 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.

9 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
9 months ago

  • Keywords has-patch added; needs-patch removed

cc @peterwilsoncc

#4 @joemcgill
4 weeks 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.

Note: See TracTickets for help on using tickets.