Make WordPress Core

Opened 13 years ago

Last modified 3 years ago

#15058 new enhancement

Validate option and transient name lengths

Reported by: chrisjean's profile chrisjean Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Options, Meta APIs Keywords: has-patch needs-unit-tests
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 (7)

validate-option-length.diff (3.7 KB) - added by chrisbliss18 13 years ago.
15058.diff (484 bytes) - added by nacin 13 years ago.
15058-unit.diff (2.3 KB) - added by devesine 11 years ago.
15058.1.diff (2.2 KB) - added by MikeHansenMe 9 years ago.
15058.2.diff (3.6 KB) - added by MikeHansenMe 9 years ago.
open to suggestions on the function name
15058.3.diff (1.7 KB) - added by MikeHansenMe 8 years ago.
trigger _doing_it_wrong to options and transients with long names
15058.4.diff (1.8 KB) - added by MikeHansenMe 8 years ago.
Adds number to message and uses mb_strlen to check char not bytes

Download all attachments as: .zip

Change History (65)

#1 @scribu
13 years ago

Related: #13310

#2 @chrisbliss18
13 years ago

  • Cc gaarai@… added

#3 @nacin
13 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>

#4 @chrisbliss18
13 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.

#5 @toscho
13 years ago

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

@nacin
13 years ago

#6 @nacin
13 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.

#7 @jamescollins
13 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

#8 follow-up: @sc0ttkclark
12 years ago

  • Cc lol@… added

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

#9 in reply to: ↑ 8 @nacin
11 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.

#10 @devesine
11 years ago

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

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

Last edited 11 years ago by devesine (previous) (diff)

@devesine
11 years ago

#11 @dartiss
11 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?

#12 @sc0ttkclark
11 years ago

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

#13 @chrisbliss18
11 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.

#14 @khromov
10 years 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.

#15 @sc0ttkclark
10 years 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.

#16 @sc0ttkclark
10 years ago

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

#17 @sc0ttkclark
10 years 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

#18 follow-up: @khromov
10 years 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?

#19 in reply to: ↑ 18 ; follow-up: @jeremyfelt
10 years 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.

#20 in reply to: ↑ 19 @chrisbliss18
10 years 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 10 years ago by chrisbliss18 (previous) (diff)

#21 follow-up: @sc0ttkclark
10 years 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.

#22 in reply to: ↑ 21 @chrisbliss18
10 years 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.

#23 @sc0ttkclark
10 years 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.

#24 @khromov
10 years 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!

#25 @nacin
10 years ago

  • Component changed from Validation to Options and Meta

#26 @DrewAPicture
10 years 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.

#27 @DrewAPicture
10 years ago

  • Milestone changed from Future Release to 4.0

Moving to 4.0 for consideration.

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


10 years ago

#29 @SergeyBiryukov
10 years ago

  • Milestone changed from 4.0 to Future Release

#30 @khromov
9 years 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.

@MikeHansenMe
9 years ago

#31 @MikeHansenMe
9 years 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.

#32 @jorbin
9 years 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.

#33 follow-up: @khromov
9 years 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.

#34 @toscho
9 years ago

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

#35 in reply to: ↑ 33 ; follow-up: @voldemortensen
9 years 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.

#36 @MikeHansenMe
9 years 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.

#37 in reply to: ↑ 35 ; follow-up: @khromov
9 years 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.

#38 @ChrisLTD
9 years 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.

@MikeHansenMe
9 years ago

open to suggestions on the function name

#39 @MikeHansenMe
9 years 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.

#40 in reply to: ↑ 37 @MikeHansenMe
9 years 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 9 years ago by MikeHansenMe (previous) (diff)

#41 @khromov
9 years ago

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

#42 @MikeHansenMe
9 years ago

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

#43 @sc0ttkclark
9 years ago

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.

#44 @octalmage
8 years ago

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

#45 @khromov
8 years ago

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

#46 @dustinbolton
8 years ago

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
8 years ago

trigger _doing_it_wrong to options and transients with long names

#47 @MikeHansenMe
8 years ago

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.

#48 @khromov
8 years ago

@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?

#49 follow-up: @MikeHansenMe
8 years ago

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.

#50 in reply to: ↑ 49 ; follow-up: @khromov
8 years ago

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.

#51 in reply to: ↑ 50 @jdgrimes
8 years ago

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.

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


8 years ago

@MikeHansenMe
8 years ago

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

#53 @MikeHansenMe
8 years ago

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.

#54 @chriscct7
8 years ago

  • Keywords needs-unit-tests added

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

#55 @MikeHansenMe
8 years ago

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.

Last edited 8 years ago by MikeHansenMe (previous) (diff)

#56 @dartiss
5 years ago

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.

#57 @swissspidy
5 years ago

#45278 was marked as a duplicate.

#58 @xedin.unknown
4 years ago

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.

Note: See TracTickets for help on using tickets.