WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#15058 new enhancement

Validate option and transient name lengths

Reported by: chrisbliss18 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 (3)

validate-option-length.diff (3.7 KB) - added by chrisbliss18 4 years ago.
15058.diff (484 bytes) - added by nacin 3 years ago.
15058-unit.diff (2.3 KB) - added by devesine 16 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 scribu4 years ago

Related: #13310

comment:2 chrisbliss184 years ago

  • Cc gaarai@… added

comment:3 nacin4 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 chrisbliss184 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 toscho3 years ago

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

nacin3 years ago

comment:6 nacin3 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 jamescollins3 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: sc0ttkclark19 months ago

  • Cc lol@… added

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

comment:9 in reply to: ↑ 8 nacin17 months 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 devesine16 months ago

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

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

Last edited 16 months ago by devesine (previous) (diff)

devesine16 months ago

comment:11 dartiss11 months 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 sc0ttkclark11 months ago

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

comment:13 chrisbliss1811 months 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 khromov5 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 sc0ttkclark5 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 sc0ttkclark5 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 sc0ttkclark5 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: khromov5 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: jeremyfelt5 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 chrisbliss185 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 5 months ago by chrisbliss18 (previous) (diff)

comment:21 follow-up: sc0ttkclark5 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 chrisbliss185 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 sc0ttkclark5 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 khromov5 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 nacin3 months ago

  • Component changed from Validation to Options and Meta
Note: See TracTickets for help on using tickets.