Validate option and transient name lengths

[chrisjean]

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.

[scribu]

Related: #13310

[nacin]

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>

[chrisbliss18]

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.

[toscho]

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

[nacin]

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.

[jamescollins]

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

[sc0ttkclark]

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

[nacin]

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.

[devesine]

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

Unit test that actually tests (probably) useful things added.

[dartiss]

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?

[sc0ttkclark]

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

[chrisbliss18]

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.

[khromov]

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.

[sc0ttkclark]

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.

[sc0ttkclark]

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

[sc0ttkclark]

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

[khromov]

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?

[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.

[chrisbliss18]

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 happens wanders into the ticket is to punt it or say that it won't happen.

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

[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.

[chrisbliss18]

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.

[sc0ttkclark]

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.

[khromov]

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!

[DrewAPicture]

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.

[DrewAPicture]

Moving to 4.0 for consideration.

[khromov]

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.

[MikeHansenMe]

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.

[jorbin]

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.

[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.

[toscho]

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

[voldemortensen]

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.

[MikeHansenMe]

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.

[khromov]

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.

[ChrisLTD]

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.

[MikeHansenMe]

open to suggestions on the function name

[MikeHansenMe]

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.

[MikeHansenMe]

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.

[khromov]

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

[MikeHansenMe]

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

[sc0ttkclark]

Max length increased to 191 which can alleviate this issue via #13310 as part of [34030]. I still wonder if we should keep this open though, as the silent fail may not be a good way for an API this important to fail.

[octalmage]

I definitely think set_transient should throw a warning if the key is too long, so in my opinion this issue is still relevant.

[khromov]

@octalmage

I'm split on whether a warning (ie. triggering _doing_it_wrong()) would be useful or not.

For end users, this warning would not be very useful. In my experience, plugins that set really long option/transient names usually involve some sort of dynamic component, so it may not show itself very easily.

On the other hand, it would make it much easier for end users to complain to the plugin author, making him aware of the issues.

[dustinbolton]

I also want to throw my hat in for getting this patched to throw the warning. This issue has bitten myself and other plugins' code I've had to troubleshoot. I bet the issue is more widespread than we know since it's invisible. While increasing the max length may reduce its occurrence, it is not a full solution.

Unexpected silent failures are very difficult to troubleshoot, and unless someone knows the internals of how WordPress works, they may have a very difficult time ever figuring out *why* this is breaking. In my opinion this is an 'oddity' of WordPress due to its inner workings and deserves a warning. It's certainly not intuitive that this would happen.

At best it will allow developers to discover the problem and head it off before an end user ever encounters it. At worst the end user will see the warning and contact the code author.

In my opinion a warning will result in better, more robust code in the WordPress ecosystem.

[MikeHansenMe]

trigger _doing_it_wrong to options and transients with long names

[MikeHansenMe]

15058.3.diff​ triggers _doing_it_wrong when the name is too long for options and transients. The message for each may still need some work but since this will only show when WP_DEBUG is true and it will continue to behave the same, I think this is an improvement.

[khromov]

@MikeHansenMe Shouldn't it be 191 <= instead of 191 <? (Max length is 191, inclusive)

Also, maybe a transient-specific error message would be more clear if we're talking about transients being saved as opposed to options?

[MikeHansenMe]

I think '191 <' is correct because if it is 192 it will trigger and if it is 191 it will save. There is a slightly different message for transients but open to any suggestions there.

[khromov]

You're right @MikeHansenMe - yoda conditions are hard. :)

I think the patch is fine, but it's likely a lot of people are going to start getting these messages once it gets merged.

[jdgrimes]

Replying to khromov:

You're right @MikeHansenMe - yoda conditions are hard. :)

@MikeHansenMe, I think we should flip these conditions. Since it isn't an == or === condition there is no reason for it to be a "yoda" condition. Flipping it around and using > instead of < will make it much more obvious what is going on.

[MikeHansenMe]

Adds number to message and uses mb_strlen to check char not bytes

[MikeHansenMe]

15058.4.diff​ uses mb_strlen instead of strlen to check char length instead of bytes. MySQL5+ varchar limits are based on char not bytes. Also added the number in the message per the discussion during bug scrub.

[chriscct7]

@MikeHansenMe Can you add some unit tests on attempting to add a option name that is too long?

[MikeHansenMe]

There is some in 15058.2.diff​ that can be modified to confirm _doing_it_wrong is getting triggered. I will get the patch updated.

[dartiss]

I would like to kick-start some further conversation on this topic, particularly around transients (rather than options).

The reason why I believe that this is now worthy of further discussion is that one particular point was missed - the two records created for an expiring transient are of different lengths. The prefix for these 2 records are _transient_ and _transient_timeout_. This means that the name for the timeout record is 8 characters less than the primary record (and the same for the network equivalents, although their maximum length will be 5 characters less). In this case, it's possible for the timeout record to be truncated and for the primary record not to be - the result is a potentially devastating situation where a non-expiring transient is created.

It's unlikely, though, with 256 characters to play with. But not impossible. I would like to see something implemented to protect against this - ideally against any truncation happening but, at minimum, to protect again the above scenario occurring.

From what I can tell from the conversation above, no changes have been made other than, via another ticket and for another reason, the field that the transient name is stored in was expanded from 64 characters to 256 characters. However, if the transient name is still too long for the field then it will "silently fail".

set_transient already returns a boolean response to indicate success of the transient being stored - a check of length against the minimum before truncation occurs and returning a false response would, to me, seem to be the most suitable solution. At worse case, if the user code isn't checking for this response, it will expect a transient to be stored and, when it's not there, will generate a new one - at least the data is fresh and I'd like to think this is the better result.

[swissspidy]

#45278 was marked as a duplicate.

[xedin.unknown]

If you want to use a standards-compliant API in your perhaps cross-platform ​modules, you may find ​wp-oop/transient-cache helpful. Besides the obvious benefits, it also protects you from using keys that would cause some of the problems described in this ticket.