WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 4 months ago

#15058 new enhancement

Validate option and transient name lengths

Reported by: chrisjean Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Options, Meta APIs Keywords: has-patch
Focuses: Cc:

Description

The option_name column in the options table holds up to 64 characters yet there aren't any length checks to ensure that the length isn't exceeded. This leads to all sorts of odd behavior as the name will be truncated to fit, saving the option but not allowing it to be retrieved with the same name.

This issue affects all uses of the options system. A particular annoyance is transients. A transient that doesn't expire has a max name length of 53 characters yet a transient that does expire has a max name length of 45 characters. When attempting to save an expiring transient of name length between 46 and 53 characters, the transient will store but will be deleted before being used when get_transient is called due to the missing _transient_timeout_ option (since it was too long).

The core issues are that the functions don't return any type of failure condition on names that are too long and no warning is created. So, I've created a patch that addresses both of these issues.

The patch shows an example of how option and transient name length validation can be added. In order to allow users to modify the option_name column length, a new constant, WP_OPTION_LENGTH, is created with a default value of 64. This constant is checked for all length validation.

This is simply an example of how I'd like the validation to function. I'm not attached to the wording of the warnings or the name of the constant.

Attachments (5)

validate-option-length.diff (3.7 KB) - added by chrisbliss18 5 years ago.
15058.diff (484 bytes) - added by nacin 5 years ago.
15058-unit.diff (2.3 KB) - added by devesine 3 years ago.
15058.1.diff (2.2 KB) - added by MikeHansenMe 10 months ago.
15058.2.diff (3.6 KB) - added by MikeHansenMe 10 months ago.
open to suggestions on the function name

Download all attachments as: .zip

Change History (47)

comment:1 @scribu5 years ago

Related: #13310

comment:2 @chrisbliss185 years ago

  • Cc gaarai@… added

comment:3 @nacin5 years ago

We don't currently do sanity checks on the schema anywhere. If we are to do this, then it should be done across the board: #11959, #14649, etc. <http://wpdevel.wordpress.com/2010/09/13/cpt-enhancements/#comment-10882>

comment:4 @chrisbliss185 years ago

I'm all for adding these checks. Even as a seasoned WP developer, running into these issues can cause massive problems and wasted time in order to remember that many functions make the final option name longer and that it is being truncated.

I know that there are many more areas to be tackled. I could have gone deeper with my original patch as I made a full listing of all the areas where option names are modified before being stored. However, I didn't want to jump off the deep end when opening this topic for discussion.

I see that both #11959 and #14649 are awaiting patches. I can working on patches for those.

First, I'd like to know:

1) Is this something that will be added if I work on it? I have a history of working on things that won't get added due to one reason or another, and I'd rather not waste my time.

2) What is the best way to manage the workflow? Should the existing tickets relating to sanity checks be handled on an individual basis or would making a large batch ticket with a massive patch be preferred. The former (individual tickets/patches) makes more sense to me, but I'm not on the core team.

comment:5 @toscho5 years ago

@core developers: Any feedback? Im very interested to see a solution to this issue.

@nacin5 years ago

comment:6 @nacin5 years ago

  • Milestone changed from Awaiting Review to Future Release

Attached patch restricts set_transient() only.

The rest of them aren't necessary -- update_option( $length_65 ) will cause the option name to be truncated, but MySQL (at least 5.5.8) will not truncate the WHERE clause in get_option( $length_65 ).

But for transients, the additional timeout prefix will cause a transient that's between 45 and 53 to lack any expiration. Luckily, because they can expire, then this patch won't do much harm.

comment:7 @jamescollins4 years ago

In the mean time I've updated the Codex to make it clear that the transient key should be 45 characters or less:

http://codex.wordpress.org/Function_Reference/set_transient#Parameters
http://codex.wordpress.org/Transients_API#Saving_Transients

comment:8 follow-up: @sc0ttkclark3 years ago

  • Cc lol@… added

Why not md5 the transient key used if it's over X amount long?

comment:9 in reply to: ↑ 8 @nacin3 years ago

  • Keywords needs-unit-tests added

Replying to sc0ttkclark:

Why not md5 the transient key used if it's over X amount long?

That could make it more difficult to debug things, but it's not a bad idea.

This ticket could use unit tests to proceed.

comment:10 @devesine3 years ago

(Disregard that unit test, I added it by accident.)

Version 0, edited 3 years ago by devesine (next)

@devesine3 years ago

comment:11 @dartiss2 years ago

Just to correct one statement made by Chris - the transient name length is 45 characters, but only if you're not using network transients. If you use these, '_site' is prefixed to the name, meaning that the maximum length is now 40.

If we're going to limit and report on name length, would it make more sense to just make it a uniform 40 rather than have different limits for different transient types?

comment:12 @sc0ttkclark2 years ago

I vote uniformity, gives core 5 characters to use in the future for any future transient variations too.

comment:13 @chrisbliss182 years ago

A possible negative side effect of a uniform length limit is that it can create warnings for existing code that doesn't have any real issues. I'm not against the idea, but I wanted to make sure that this potential situation was noted.

comment:14 @khromov22 months ago

What happened to this issue?

This is a fairly big issue and I have stumbled upon plugins that keep trying to re-create transients on every page load as they can't check if they have been set properly.

comment:15 @sc0ttkclark22 months ago

I definitely think this needs to be moved forward, case and point -- it won't affect old plugins/themes since Transients are now (as of recently) cleared out on every WordPress upgrade anyways.

comment:16 @sc0ttkclark22 months ago

Anyone against a MD5 patch for those that go over the 40 character limit? If not, I'll put one forward this week.

comment:17 @sc0ttkclark22 months ago

I think for MD5, what we could do is check if chars are over 40, then substr all characters after the 8th one, and md5 it, so it remains within the 40 character limit. That way there's a nice blend of having the ability to see the transients in the DB when debugging, you'd see something like my_tests41c5e5a98711f4c6a8b52606c47f9f61 in place of my_testsomgomgomgomgomgomgomgomgomgomgomgomg

comment:18 follow-up: @khromov22 months ago

scott,

The md5 patch sounds pretty good.

Perhaps it's heresy, but why can't we alter option_name to something like VARCHAR(256) instead?

comment:19 in reply to: ↑ 18 ; follow-up: @jeremyfelt22 months ago

Replying to khromov:

Perhaps it's heresy, but why can't we alter option_name to something like VARCHAR(256) instead?

A ticket for this is open in #13310. Hashing that out first may make more sense.

comment:20 in reply to: ↑ 19 @chrisbliss1822 months ago

Replying to jeremyfelt:

Replying to khromov:

Perhaps it's heresy, but why can't we alter option_name to something like VARCHAR(256) instead?

A ticket for this is open in #13310. Hashing that out first may make more sense.

It would make sense, but that ticket is older than this one and it's clearly never going to get hashed out as no one in core is interested in dealing with it. The only time anyone with power to actually make the change happen wanders into the ticket, they punt it or say that it won't happen.

They have clearly binned this issue as "not a problem."

Last edited 22 months ago by chrisbliss18 (previous) (diff)

comment:21 follow-up: @sc0ttkclark22 months ago

Pushing to 256 is great, but still doesn't resolve things when the transient key is too long. Why it'd be that long, not sure, but the MD5 solution is the most versatile to support that, column change or not. I believe object caching, when it takes over transients etc, has a charcter limit of around 250, give or take a few, depending on what's running it. When it hits that limit, it truncates the key.

I think md5, which could be increased to a higher limit before it runs if option_name ever is expanded, is the best bet here for expanding the expected functionality of transients etc.

comment:22 in reply to: ↑ 21 @chrisbliss1822 months ago

Replying to sc0ttkclark:

Pushing to 256 is great, but still doesn't resolve things when the transient key is too long. Why it'd be that long, not sure, but the MD5 solution is the most versatile to support that, column change or not. I believe object caching, when it takes over transients etc, has a charcter limit of around 250, give or take a few, depending on what's running it. When it hits that limit, it truncates the key.

I think md5, which could be increased to a higher limit before it runs if option_name ever is expanded, is the best bet here for expanding the expected functionality of transients etc.

I agree that having a solution where "it just works" is ideal. Were you able to put together a patch?

At a minimum, I want it to always work reliably or to return an error and generate a notice when a problem will occur. As it is now, we don't get an error and it doesn't work, so it is a very bad flaw currently.

comment:23 @sc0ttkclark22 months ago

I want it to "just work". I'll work on a new patch with the md5 in mind, need to get my VVV unit testing environment setup so I can update the unit test accordingly too.

comment:24 @khromov22 months ago

I had an idea - why don't we add in filters in set/get/delete_option(); that lets you change the option name just before it is inserted, retreived or deleted?

Then it would be trivial to write a plugin that changes option names internally whichever way the user prefer. (For example using sc0ttkclarks solution).

Such a change (adding a few filters) is unlikely to break any unit tests.

Let me know what you think!

comment:25 @nacin19 months ago

  • Component changed from Validation to Options and Meta

comment:26 @DrewAPicture15 months ago

In 28735:

Improve inline documention for set_transient() and set_site_transient() to specify the 45- and 40-character limits for their respective transient name values.

Props edwin-at-studiojoyo.com for the original patch.
See #15058, #13310. Fixes #28467.

comment:27 @DrewAPicture15 months ago

  • Milestone changed from Future Release to 4.0

Moving to 4.0 for consideration.

comment:28 @ircbot14 months ago

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.

comment:29 @SergeyBiryukov14 months ago

  • Milestone changed from 4.0 to Future Release

comment:30 @khromov11 months ago

Here is another example where this limitation degrades performance and makes a plugin unusable.

https://wordpress.org/support/topic/currently-degrades-performance?replies=1

https://wordpress.org/plugins/widget-output-cache/

This is no weird edge case, there are probably hundreds of plugins that suffer from these issues.

@MikeHansenMe10 months ago

comment:31 @MikeHansenMe10 months ago

  • Keywords needs-unit-tests removed

Instead of failing if it is too long the patch I uploaded md5 the name AND prefixes it with md5. This works for options and transients. Unit tests included.

comment:32 @jorbin10 months ago

RE: unit tests - I think we should incorporate some Multi site unit tests as well. We also need to add unit tests around updating and deleting transiants.

RE: Code - I don't like that we are obscuring the _transient_ from the name. It makes it harder to immediately identify that the option is in fact a transient. I'd rather see us fix this in the transient code to prevent that.

comment:33 follow-up: @khromov10 months ago

I think MikeHansenMes patch adds unneeded complexity.

Imagine writing the Codex article for this, it'd sound pretty weird. Even truncating the names consistently would be a better way to tackle this issue.

comment:34 @toscho10 months ago

Please do not introduce more code duplication. Doing four times the same thing cries for separation.

comment:35 in reply to: ↑ 33 ; follow-up: @voldemortensen10 months ago

Replying to khromov:

I think MikeHansenMes patch adds unneeded complexity.

Imagine writing the Codex article for this, it'd sound pretty weird. Even truncating the names consistently would be a better way to tackle this issue.

khromov... 11 months ago you agreed that this kind of patch would be great...

comment:18

scott,

The md5 patch sounds pretty good.

Additionally, how do you propose we truncate things consistently? How would a theme/plugin know that its transient was truncated, and to be checking for a different value than the value they tried to set? It wouldn't make sense to cache something with the original value just to point to another value. That's too expensive.

comment:36 @MikeHansenMe10 months ago

Happy to write a few more unit tests. The current state is that it saves the option/transient but truncates it if too long. Then when you go to retrieve it it returns false because it(the long one) does not exist. The previous patch would return false, not allowing it to work. I am ok with that however wouldn't it be a better developer experience if it just worked? The reason I added the md5 to the end of the prefixes is to indicate that something else happened than the default. As for breaking it out. I am ok with that however it is only a few lines of code and it is not always the same number of characters.

comment:37 in reply to: ↑ 35 ; follow-up: @khromov10 months ago

Replying to voldemortensen:

khromov... 11 months ago you agreed that this kind of patch would be great...

That patch was different, it would use md5 to "fill in" the rest of the transient name if it was too long, so you would still know at least the first 13 characters, which helps in identifying which plugin the transient belongs to.

My position has been either using VARCHAR(256) or providing a filter (so people can use whatever prefixing scheme they want.)

Additionally, how do you propose we truncate things consistently? How would a theme/plugin know that its transient was truncated, and to be checking for a different value than the value they tried to set? It wouldn't make sense to cache something with the original value just to point to another value. That's too expensive.

The easiest way would be to truncate the transient name after it is passed to get_transient(). Nacin opposed this kind of change because schema sanity checks aren't used in other places, which is a good point.

comment:38 @ChrisLTD10 months ago

As a theme developer that's run into transient name length bugs a few times, I would prefer errors when attempting to save transients that are too long. Then each developer can decide what workaround is best for their project if they need long transient names. The Codex could have an example of the "preferred" workaround using an MD5 hash.

@MikeHansenMe10 months ago

open to suggestions on the function name

comment:39 @MikeHansenMe10 months ago

Updated the patch and added some tests for the option functions. I also abstracted the duplicate code. I do not care for the function name if anyone has a better suggestion. Also added the docs for the new function.

comment:40 in reply to: ↑ 37 @MikeHansenMe10 months ago

Replying to khromov:

My position has been either using VARCHAR(256) or providing a filter (so people can use whatever prefixing scheme they want.)

On this changing the scheme is an option. I actually added a filter to my patch until I realised that if one person changed the format another plugin would lose all of it's (long)entries. This would be perceived as WPs fault. I think it is best not to add a filter here.

Last edited 10 months ago by MikeHansenMe (previous) (diff)

comment:41 @khromov8 months ago

I've proposed a schema change and attached a patch over at #13310

comment:42 @MikeHansenMe4 months ago

Just dropping a note here that #13310 is marked 4.3 so if that gets commit. We can close this.

Note: See TracTickets for help on using tickets.