set_transient() allows invalid transient name

[jeremyescott]

Due to a typo/bug in my plugin code, I found that WordPress accepts empty strings, null, and false for the $transient arg, aka: the transient name, in set_transient() function which creates transients in the options database with values of simply _transient_ and _transient_timeout_.

That said... the transient created with an empty string continued to work (could be set and get and deleted). Because the typo in my code referenced a variable that held the transient name but was empty, the get, set, and delete function calls worked (annoyingly).

I did observer two issues...

1. In the event two developers cause the same mistake/error, their transients will collide with each other.

2. More importantly, I observed the empty string transient will not be cleaned up by the delete_expired_transients routine. (The Transients Manager plugin must use delete_expired_transients() as it could not delete the transient either.) I will submit a second ticket for this issue.

Upon review of the set_transient() and add_option() code, I observed several opportunities to improve, including:

• return false for empty $transient value
• return false for bool, non-scalar $transient values
• cast $transient as string
• return false for strings with more than 172 characters

These false returns will guide developers to fix issues with malformed $transient names.

I have a pull request to github ready to follow this ticket.

On set_transient( string $transient, mixed $value, int $timeout ) : bool:

• return false for empty $transient value
• return false for bool, non-scalar $transient values
• cast $transient as string
• return false for strings with more than 172 characters

Trac ticket: https://core.trac.wordpress.org/ticket/58903#ticket

[audrasjb]

Thanks for the patch and for providing unit tests!

Your have a couple coding standard issues in the unit test, and also the new test case needs a ticket reference. Commenting directly in the PR right now :)

[jeremyescott]

Thank you for the feedback! Updates were made to the Pull Request based on feedback.

[jeremyescott]

FYI: The other ticket for the related issue I discovered is #58904

[jeremyescott]

Following up on this...

I initially reported that a created transient with an empty string for the $transient [key] argument would not cause an issue with functionality, and that remains true.

However, I've observed now both Varnish and Redis caching, when installed to support WordPress, fall over when empty string name transients are attempted to be created.

[flixos90]

Thanks for the ticket and detailed description @jeremyescott!

I think you make some great points, and the PR looks solid to me.

The only thing that I'm less sure about is how to deal with the 172 character limit. Is this because of the database column type and its limitations? If so, I think this particular restriction would be better to discuss separately for whether we want to enforce it at the code level or not, because it applies in several different areas as well. For example, options would be subject to such a limit too, but I don't think the code enforces that. So I think that particular point would be better to discuss/explore separately from this ticket.

[jeremyescott]

Hi Felix. Thanks for the review. Super fun to go from meeting at WCUS'22 and hanging at the same table for contributor day to now having you read my code. haha.

The only thing that I'm less sure about is how to deal with the 172 character limit. Is this because of the database column type and its limitations? If so, I think this particular restriction would be better to discuss separately for whether we want to enforce it at the code level or not, because it applies in several different areas as well. For example, options would be subject to such a limit too, but I don't think the code enforces that. So I think that particular point would be better to discuss/explore separately from this ticket.

In preparing a response I originally felt indifferent to removing or keeping the 172 char check, but I now actually want to encourage it stay.

My original thought process was that since I was stepping in to add input validation and testing to set_transient() to prevent the problem I encountered, I may as well add validators for all the conditions listed in the docblock at one time.

Is this because of the database column type and its limitations?

I figured the DB was the limit, but I did not see the 191 char limit in the add_option() function and saw no mention/documentation.

So, I found the current (and I assume, longstanding) DB schema declares varchar(191) on option.name so if '_transient_timeout_' is 19 characters 191-19 = 172 and thus the transient limit.

Assuming WP is not using strict SQL, the DB will should simply truncate a too-long name. And this is why we should enforce the transient limit while not modifying the option name limit. Since transients are simply options with a prefix, and since two records are made for each transient...

Hypothetically we create a 173 character transient. WordPress makes two add_option calls:

_transient_timeout_173chars
_transient_173chars

The first record for the two-part transient will be truncated to 191 chars, dropping the 173rd character in the name, however the second record, since the prefix is 8 characters less, will not exceed the db varchar(191) limit and not be truncated. The result is the two parts of our transient have mismatched names and will not work. Ensuring a 172 char limit on a transient makes sure they both match and function as intended.

With that research, I actually favor keeping the 172 char check and I don't think we need to perform any checks on add_option() unless we want to.

[nicolefurlan]

We discussed this ticket in today's bug scrub. @hellofromTonya mentioned that this ticket might fall into an ongoing broader architectural discussion about input validation, and in the PR, there is discussion about seeking a broader solution for all transient functions.

Let's keep this discussion moving so that we can keep this ticket moving :)

[hellofromTonya]

I found that WordPress accepts empty strings, null, and false for the $transient arg

I did question if this falls into a broader discussion of input (data type and value coming into a function) validation. The transient name to be received by set_transient() is clearly identified as string. Sending it anything else (including null or false) is doing it wrong. The broader architectural discussion focuses on how to validate what is sent to Core functions. Rather than having isolated data type checks, a more structural architectural approach is also being considered across all of Core.

But then @joemcgill pointed out that the ​issue is more around an empty string being sent to it:

I think the issue is that the value could be an empty string and cause a bad experience. Edge case, for sure, but the updates to safeguard against this seem reasonable.

And in looking at the ​discussion in the patch, @flixos90 proposed an idea:

You add the limitations only in set_transient(), which I agree is the root of the problem. But additionally, wouldn't it make sense to enforce those limitations in the other transient functions too? Potentially we can introduce a new private function like wp_validate_transient_name() and reuse in the relevant places?

I tend to agree with his idea. Rather than fix the issue in only one place, instead put in a validation mechanism for all transient functions. But doing so might shift this ticket to an enhancement rather than a bugfix.

Looking at the description of the ticket:

I found that WordPress accepts empty strings, null, and false for the $transient arg

null and false are doing it wrong, but empty strings might be the value to fix.

Given this ticket is still being discussed and the patch may go through a redesign (per @flixos90 suggestion), I'll mark this ticket as 2nd-opinion, to denote it​needs more discussion and consideration to land on a solution.

[jeremyescott]

Should the patch be redesigned with this approach in mind, and the transient name checks applied to set_, delete_, and get_ functions? I'm happy to revise the patch with that goal in mind.

I tend to agree with his idea. Rather than fix the issue in only one place, instead put in a validation mechanism for all transient functions. But doing so might shift this ticket to an enhancement rather than a bugfix.

Both?

[hellofromTonya]

Replying to jeremyescott:

Should the patch be redesigned with this approach in mind, and the transient name checks applied to set_, delete_, and get_ functions? I'm happy to revise the patch with that goal in mind.

How about creating an alternative PR for fixing the empty string bug in each of the transient functions? ​See my thoughts here:

I think this fix only needs to deal with checking for empty string. The function is clearly documented that the transient name must be a string. Anything other data type, such as integer, boolean, or null, are doing it wrong.

If all it needs is to check for is_string() and empty() string, then thinking a separate validation function might be overkill.

Having the 2 different approaches gives contributors the opportunity to consider which one is better for Core now and in the long run.

[nicolefurlan]

@jeremyescott could you review #comment:12 and update your PR? We're looking to include this fix in 6.4 RC1 which is only one week away.

[jeremyescott]

I will get on this. Thanks for the ping!

[oglekler]

Because we are about to investigate another approach and have only 3 days before RC1, I am moving this ticket to the next milestone.

[rajinsharwar]

Punting this for Future Release. @jeremyescott If you would like to work on this and we have some updates recently, we can again punt this for 6.5 :)