update_option() calls sanitize_option() twice when option does not exist

[MikeSchinkel]

I just spent several hours tracking down an issue when using the Settings API where sanitize_option() is called twice which is unnecessary execution especially if sanitization includes calling an external API for username/password authorization (this was how another developer set it up for a plugin I was debugging.)

What happens is that a call to update_option() will call sanitize_option() and then if the option wasn't previously in the options table update_option() will delegate to add_option() which calls santize_option() a second time. This would normally be easy to workaround by first calling get_option() and testing for false and calling add_option() instead of update_option() if false, but not when the Settings API chooses how to call it.

I've looked at the problem and can envision several different ways to solve it such but don't know which the core developers would choose (or if they'd choose yet another option I haven't envisioned) so I didn't submit a patch:

• Adding a 3rd parameter $mode to sanitize_option() that identifies the mode ('add', 'edit', default = 'unknown') and thus allow the hook to ignore one of the options (this would be more backward compatible but would put the onus on the developer to know to do this),
• Adding a 5th parameter $bypass_sanitize to add_option() defaulted to false that is only passed as true in update_option() allowing update_option() to disable the call to sanitize_option() found in add_option() (this would be less backward compatible and hacky, but seemless to the developer)
• Adding a 3rd parameter $bypass_sanitize to update_option() defaulted to false that is only passed as true from /wp-admin/options.php allowing update_option() to bypass the call to sanitize_option() if an add_option() will happen (this would be less backward compatible and hacky, but seemless to the developer)
• Have /wp-admin/options.php test for no pre-existing option and then call add_option() or update_option(), respectively (this would be seemless to the developer but less backward compatible and not hacky, and it wouldn't handle the general case.)

So is this worth fixing to save other users and developers debugging time, and to reduce the chance of subtle errors in new code? If yes, how best to approach?

[MikeSchinkel]

As a quick update about how I worked around what was causing the problem, I moved the API authentication code into admin_init() and read and updated $_POST directly making sure to only do so on an update for this one plugin's admin settings page.

Just FYI for anyone else who might have the same issue prior to this getting fixed in core or if it does not get fixed in core.

[nacin]

I imagine the issue here is specifically related to the saving of a settings screen, and not usage of update_option() at the API level. In that case, it seems like the onus is on the developer to ensure such an option is initialized in the database. There are two schools of thought, but an option like this should certainly be initialized if that kind of side effect could occur.

If there were any possible route to an elegant solution here, I probably wouldn't be suggesting essentially a wontfix. But there isn't one, alas.

[totels]

Wouldn't it be easiest to just refactor and move the check for the old value and the call for add_option() before the sanitize_option() call in update_option()?

diff --git wp-includes/option.php wp-includes/option.php
index 722d1f3..4df07d8 100644
--- wp-includes/option.php
+++ wp-includes/option.php
@@ -224,17 +224,18 @@ function update_option( $option, $newvalue ) {
 	if ( is_object($newvalue) )
 		$newvalue = clone $newvalue;
 
-	$newvalue = sanitize_option( $option, $newvalue );
 	$oldvalue = get_option( $option );
+
+	if ( false === $oldvalue )
+		return add_option( $option, $newvalue );
+
+	$newvalue = sanitize_option( $option, $newvalue );
 	$newvalue = apply_filters( 'pre_update_option_' . $option, $newvalue, $oldvalue );
 
 	// If the new and old values are the same, no need to update.
 	if ( $newvalue === $oldvalue )
 		return false;
 
-	if ( false === $oldvalue )
-		return add_option( $option, $newvalue );
-
 	$notoptions = wp_cache_get( 'notoptions', 'options' );
 	if ( is_array( $notoptions ) && isset( $notoptions[$option] ) ) {
 		unset( $notoptions[$option] );}}}

[Denis-de-Bernardy]

Any odds this can get fixed in WP 3.9? (If so, do you need the above patch in diff format, or can you pick it up from there?)

[ericlewis]

Replying to Denis-de-Bernardy:

Any odds this can get fixed in WP 3.9? (If so, do you need the above patch in diff format, or can you pick it up from there?)

Given the Milestone is "Awaiting Review" on the ticket, it's not slated for 3.9 (or "Future Release" for that matter).

However, a diff as an attachment on this ticket is preferred rather than in a comment.

[Denis-de-Bernardy]

Fallback immediately on non-existent options in update_option() and update_site_option()

[Denis-de-Bernardy]

Aside on this: the pre_update_ filters and not-option wrangling don't get done consistently in update_option() and update_site_option(). Probably for a second ticket, but worth noting regardless.

[rmccue]

Just hit this myself.

Disagree with nacin that it's the developer's responsibility to check this. If I do something like register_setting( 'group', 'opt_name', 'nonidempotent_func' ), it doesn't make sense to me that the first save (that creates the option) should behave differently to the second save (that updates the option). As far as I'm concerned as the API user, I shouldn't need to worry whether it's using add_option or update_option internally.

Both totels and ddb's patches break backwards compatibility with the pre_update_option_{$option} and pre_update_option filters, which currently always get called when using update_option. However, both operate on the sanitized version of the value, so using them would be strange.

Choices I can think of, in preferred order:

1. Add extra parameter to add_option to skip sanitization: 21989.2.diff​ handles this
2. Skip the pre_update_option filters when adding options via update_option (should probably add pre_add_option_{$option} and pre_add_option filters as well; this also breaks BC with the filters): 21989.diff​ handles this

Thoughts? It'd be nice to fix this.

[rmccue]

Add new parameter to add_option

[Denis-de-Bernardy]

Replying to rmccue:

Both totels and ddb's patches break backwards compatibility with the pre_update_option_{$option} and pre_update_option filters, which currently always get called when using update_option. However, both operate on the sanitized version of the value, so using them would be strange.

Hold... Did you just seriously argue that maintaining backwards compat on broken was important? ;-)

Choices I can think of, in preferred order:

1. Add extra parameter to add_option to skip sanitization: 21989.2.diff​ handles this
2. Skip the pre_update_option filters when adding options via update_option (should probably add pre_add_option_{$option} and pre_add_option filters as well; this also breaks BC with the filters): 21989.diff​ handles this

Thoughts? It'd be nice to fix this.

I for one ink that adding a new argument borders on crazy, so I'd rather see my own patch get checked in.

As an aside, don't miss what I highlighted as I submitted it: the order of these filters and such differ in update_option and update_site_option -- and other options and transient related functions, for that matter.

[rmccue]

Replying to Denis-de-Bernardy:

Hold... Did you just seriously argue that maintaining backwards compat on broken was important? ;-)

The brokenness here only occurs for options with a sanitization callback; others are unaffected. Breaking a working hook for them here would be bad. :)

I for one ink that adding a new argument borders on crazy, so I'd rather see my own patch get checked in.

The new argument was so that we can maintain BC with the pre_update_option hooks rather than passing through early. With that said:

As an aside, don't miss what I highlighted as I submitted it: the order of these filters and such differ in update_option and update_site_option -- and other options and transient related functions, for that matter.

Interesting, I had missed that, apologies. update_site_option calls pre_update_option_{$option} but not pre_update_option. These should all be made consistent, obviously (that's probably a separate ticket, IMO).

update_site_option calls sanitize_option after pre_update_option_{$option}, so that's broken. Given that it's broken there, we might as well ignore the BC break and go with the filter-then-sanitize choice for consistency's sake. (Although multisite use with update_site_option is less common, filter-then-sanitize feels like the more correct way to do it.)

In which case, I endorse 21989.diff​. :) We should also fix these crazy filters.

[DrewAPicture]

I agree we should fix this, though not this late, and there doesn't seem to be a consensus. Let's try for it in 4.1 early.

[jaschaio]

It appears that this is still not fixed, as I just encountered the exact same problem while developing.

[Andy Schmidt]

Bug still occurs with 4.9.1.

Spent quite a while trying to figure out why a newly activated plugin, which had its settings (in this case a path to external code includes) configured for the first time, was complaining that functions from these required files had already been declared previously.

Sorry - it's completely unintuitive for a plug-in developer to have the validator be called TWICE, when the Options form was submitted ONCE.

[dd32]

Removing the version keyword, as it specifically refers to the first affected version (Which is something 8+ years ago I believe).

While this is a weird bug to run into, it's most definately not urgent based on the 5 years this ticket has existed without action & few reports.

Unfortunately other than adding a $already_sanitized parameter to add_option() as per 21989.2.diff​ I don't really see a backwards compatible route to take here - and I really don't like the idea of adding a sanitization bypass parameter in something this low down in the API.
Very tempted to suggest this as a wontfix myself and instead document this scenario with a warning to cache (on a per-page load basis) any expensive sanitization lookups.
I don't see an issue with running intval() multiple times, but wp_remote_post() on a sanitisation callback seems like it'd be a bit too heavy not to expect a problem.

[Andy Schmidt]

Replying to dd32:

Just want to document MY scenario that was impacted by this bug.

I was validating the correctness path to an external code base before storing that permanently as the "new" location.

So I first confirmed a syntactically valid relative path, then the actual existence of a certain file there, then (in an overabundance of caution) had a try/catch with an "include" of that new codebase path followed by a function call. Can you tell - I didn't want to switch to a non-working path?

Spent days trying to figure out which of my hooks/actions/filters/timing etc. might cause the "include" to have happened previously somewhere earlier in my code, before I stumbled across this bug report.

Maybe the incident count is so low because it's so far-fetched and hard to pin-point; most people would just have to give up early and eliminate their (perfectly functioning!) code.

[ocean90]

#33582 was marked as a duplicate.

[ocean90]

#43327 was marked as a duplicate.

[littler.chicken]

I'll add that I've run into this recently as well. I've got a settings page with a group of repeatable fields. The last one of the group is hidden so that it can be cloned, and during sanitization, the last (empty) field is discarded. On the initial save, since the process is run twice, the next to last field is erroneously discarded.

I'm avoiding the issue for the moment by preemptively adding the option to the database if it does not already exist when the settings page is built, but this is less than ideal.

[Drivingralle]

Can we fix this in WordPress 5.4? Or close the ticket?
After over 7 years a clear decision would be a nice thing.

[joho68]

As per @Drivingralle's statement, can this at least be clearly stated in the Developer Docs somewhere, unless it already is and I missed it. I too ran into this when dealing with data that is "encoded" (json) at rest, and "decoded" (array -> text area) when in flight/use.

[48design]

We encountered this issue today as well...
+1 for this being looked into after more than 8 years now.

[PerS]

This is a performance issue, so I tagged it as such.

[chrisplaneta]

+1 to push this forward after 10+ years.

I'm running into this 10+ year old issue here:

Trac ticket: https://core.trac.wordpress.org/ticket/21989

...and wondering if something like this would be an acceptable fix. It works from what I can tell, but forgive me (and please correct me) if I'm out of protocol here, I've never opened a pull request before.

The TL;DR issue is that the value gets sanitized in update_option(), but then gets re-sanitized in add_option() if the option does not exist. In most situations it's not really an issue but in situations like the one that led me to this, sanitization functions that hash a password or encrypt something end up hashing the hash or double-encrypting prior to saving. This obviously renders hashed/encrypted data unusable, because any subsequent saves only sanitize/hash/encrypt the data once.

Can we not simply move the sanitization down in update_option() to prevent this? There's no output prior to that and since the filtered value may end up getting passed to add_action() and re-sanitized anyway, it seems safe to sanitize after the filters in update_option() as well... Besides, it seems safer regardless to sanitize after the filters, no?

Anyway this fixes the problem in my own use case, but perhaps I'm missing something.

[gregstorkan]

I'm sure I must be missing something but would it not work to just sanitize after the add_option() call inside update_option()? I don't know how to use SVN so I posted a ​pull request on GitHub but since add_option() would re-sanitize the value after it gets filtered anyway, why would it break the filters to sanitize after them in update_option() as well?

[costdev]

Curious, in add_option(), what if we wrap ​this line with a check on did_filter for the option?

if ( ! did_filter( "sanitize_option_{$option}" ) ) {
        $value = sanitize_option( $option, $value );
}

That way, if the value for the option has already been sanitized by the time it reaches add_option(), skip sanitizing it again.

​Here's a plugin with a basic test.

Without the above change:

• sanitize_option() runs twice.
• add_option() saves a_string_1_1.

With the above change:

• sanitize_option() runs once.
• add_option() saves a_string_1.
• All hooks that should fire continue to do so, with the expected value. (Confidence check me on this)

[Andy Schmidt]

Replying to costdev:

Curious, in add_option(), what if we wrap ​this line with a check on did_filter for the option?

Like this proposal!

[costdev]

Unfortunately, after further investigation and writing some PHPUnit tests, relying on a check to see if a filter has run isn't safe.

Why?

• Anyone can run sanitize_option() or apply_filters( "sanitize_option_{$option}" ) before accidentally passing an unsanitized value to add_option(). If sanitization in add_option() were to rely on whether "sanitize_option_{$option}" ran, it would proceed to add the unsafe value to the database.
• The same applies to any filter/action hook in update_option() as well.
• In short, it's possible to accidentally skip sanitizing if relying on whether a filter has run.

[gregstorkan]

There's clearly something I don't understand about the intended use of the pre_update_option and pre_update_option_{$option} filters, as it seems to me that sanitization should always be the last thing to happen to the value prior to it being saved... but I'll close my pull req and regardless, perhaps fixing the inconsistency of the filters is step 1 here anyway.

If the key is to come up with some way to be sure that the value passed to add_option() came from update_option(), and thus has been sanitized, then is there something update_option() could generate that add_option() could securely verify? Perhaps some kind of nonce that could be passed along with the value in an array?

Obviously I'm spitballing based on extremely limited knowledge but it does feel like there's a creative solution here somewhere.

[lev0]

11 years old and this just bit me. Crashed a plugin settings save after install, and never saved any data.

update_option() already uses the $notoptions cache below the diversion to add_option(), which will contain the name of a new, non-existent option after the get_option() call. If the get_option() call was moved above the sanitize_option(), and that cache was read immediately after that, then that cache could be checked first, and the function diverted to add_option() before the sanitize_option(). The issue with that solution is that new options would no longer go through the pre_update_option_{$option} and pre_update_option filters, but options added with add_option() don't either.

Something like this: https://core.trac.wordpress.org/attachment/ticket/21989/no-double-handling-2.patch

[inf3rno]

I wonder how this wasn't fixed in the past 11 years. I reported something similar, maybe a duplicate. https://core.trac.wordpress.org/ticket/60595#ticket I spent an hour on it. You are playing with developers time...

I think the main problem here that we have only a single callback for type conversion, sanitization, API call, encryption, etc... Sanitization should be idempotent in theory, but we have a lot of other features which are not idempotent. Either we need a guarantee to call sanitize_callback only once or we need other callbacks which are called only once.

[inf3rno]

#60595 was marked as a duplicate.

A possible solution to the gotcha of relying on the behaviour of update_option(), which register_setting() does:

If the option does not exist, it will be created.

It is not obvious to developers that their option will, currently, be sanitised twice in that case. This can easily lead to data corruption, plugin crashes, etc.

This patch is a little bit of a hack inasmuch as any other underscore-prefixed private function, but I don't see another DRY way to bypass only that processing which has already been done.

Trac ticket: https://core.trac.wordpress.org/ticket/21989

Can also confirm that new unit test fails on parent commit of this PR:

$ vendor/bin/phpunit --verbose tests/phpunit/tests/option/updateOption.php 
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.10-2ubuntu1
Configuration: /home/roy/path/to/wordpress-develop/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

.........F                                                        10 / 10 (100%)

Time: 00:00.051, Memory: 42.50 MB

There was 1 failure:

1) Tests_Option_UpdateOption::test_stored_sanitized_value_from_update_of_nonexistent_option_should_be_same_as_that_from_add_option
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'x_y'
+'x_y_y'

/home/roy/path/to/wordpress-develop/tests/phpunit/tests/option/updateOption.php:249

[pbearne]

Trying to make sense of this ticket

We need to call sanitize_option before the filters pre_update_option_{$option} and pre_update_option so that they can adjust the value
as there are not match filters in add_option()
Which feels like there should be

So one option could be
If the option not found call add_option() with the raw value early and add the filters to that function

Throughts?

Paul

[lev0]

@pbearne did you look at & test my ​PR? Most proposed solutions – including yours – pose a hazard because of ​Hyrum's Law. WordPress certainly meets its "sufficient number of users" requirement.

The minimum amount of double processing you can get with your suggested earlier diversion is avoiding the is_scalar() and empty() (to ensure an option name). This would be sufficient to stop the double sanitise, except it would also bypass the pre_update_option* and (within the $old_value = get_option( $option );) the extra default_option_{$option} and pre_option* filters that occur prior to the current diversion.

The sequence in add_option() is not compatible, and Hyrum tells us we should expect devs to rely on those filters. In order for your suggestion to work and maintain bc, the lost filtering would have to be duplicated in add_option() but made to only run conditionally so that it would not change explicit add_option() calls. I'm not sure it's even feasible to get the same order of operations trying to do it that way.

I understand there may be objections to the creation of another private use function in the PR, but I can't see another way to fix this without either duplicate code or a bc break. I'm happy to be proven wrong.

[lev0]

I disagree with dd32's assessment:

While this is a weird bug to run into, it's most definately not urgent based on the 5 years this ticket has existed without action & few reports.

If someone analysed all plugins using (the widely-recommended) register_setting(), I'm sure they'd find a miniscule proportion anticipate that the sanitize_callback can be called twice. That's just one way to hit this bug.

Developers probably have no idea their plugins are affected, because the issue disappears as soon as the option is created (even with an invalid value). I don't know about other devs, but usually when I create an option, it's assigned a value so I can test it, and it stays. I might change the value but I rarely delete it. I put more complex configs into single array options, and HTML form structures normally aren't 1:1 with the option structures, so unless I protect input from being parsed twice, it's guaranteed to get corrupted.

In contrast, every new user of an affected plugin can hit this on the first install, where the option will not exist. This makes a crappy first impression, and unduly reflects poorly on the author. You install a plugin, go to its settings page, carefully complete the form, and whether it saves correctly or not is a gamble.

Finally some interest and it's stalled again. 12 years is a long time.