Opened 8 years ago
Last modified 6 months ago
#38709 assigned defect (bug)
Unlimited query for invalid post names
Reported by: | david.binda | Owned by: | 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)
Change History (13)
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
3 years ago
@
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()?
#4
@
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.
@
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.
@
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
@
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
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
.