Opened 12 years ago
Last modified 6 months ago
#21989 accepted defect (bug)
update_option() calls sanitize_option() twice when option does not exist
Reported by: | MikeSchinkel | Owned by: | pbearne |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Options, Meta APIs | Keywords: | dev-feedback has-patch needs-testing |
Focuses: | performance | Cc: |
Description
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
tosanitize_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
toadd_option()
defaulted tofalse
that is only passed astrue
inupdate_option()
allowingupdate_option()
to disable the call tosanitize_option()
found inadd_option()
(this would be less backward compatible and hacky, but seemless to the developer) - Adding a 3rd parameter
$bypass_sanitize
toupdate_option()
defaulted tofalse
that is only passed astrue
from/wp-admin/options.php
allowingupdate_option()
to bypass the call tosanitize_option()
if anadd_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 calladd_option()
orupdate_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?
Attachments (4)
Change History (48)
#1
@
12 years ago
- Summary changed from update_option('example') calls sanitize_option('example') twice when false===get_option('example') to update_option() calls sanitize_option() twice when option does not exist
#3
@
12 years ago
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.
#4
@
11 years ago
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 a/wp-includes/option.php b/wp-includes/option.php index 722d1f3..4df07d8 100644 --- a/wp-includes/option.php +++ b/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] );
#6
follow-up:
↓ 7
@
11 years ago
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?)
#7
in reply to:
↑ 6
@
11 years ago
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.
@
11 years ago
Fallback immediately on non-existent options in update_option() and update_site_option()
#9
@
11 years ago
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.
#11
follow-up:
↓ 12
@
10 years ago
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:
- Add extra parameter to
add_option
to skip sanitization: 21989.2.diff handles this - Skip the
pre_update_option
filters when adding options viaupdate_option
(should probably addpre_add_option_{$option}
andpre_add_option
filters as well; this also breaks BC with the filters): 21989.diff handles this
Thoughts? It'd be nice to fix this.
#12
in reply to:
↑ 11
;
follow-up:
↓ 13
@
10 years ago
Replying to rmccue:
Both totels and ddb's patches break backwards compatibility with the
pre_update_option_{$option}
andpre_update_option
filters, which currently always get called when usingupdate_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:
- Add extra parameter to
add_option
to skip sanitization: 21989.2.diff handles this- Skip the
pre_update_option
filters when adding options viaupdate_option
(should probably addpre_add_option_{$option}
andpre_add_option
filters as well; this also breaks BC with the filters): 21989.diff handles thisThoughts? 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.
#13
in reply to:
↑ 12
@
10 years ago
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.
This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.
10 years ago
#15
@
10 years ago
- Keywords 4.1-early added
- Milestone changed from 4.0 to Future Release
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.
#17
follow-up:
↓ 19
@
8 years ago
It appears that this is still not fixed, as I just encountered the exact same problem while developing.
This ticket was mentioned in Slack in #core by parsmizban. View the logs.
7 years ago
#19
in reply to:
↑ 17
@
7 years ago
- Version set to 4.9.1
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.
#20
follow-ups:
↓ 21
↓ 44
@
7 years ago
- Version 4.9.1 deleted
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.
#21
in reply to:
↑ 20
@
7 years ago
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.
#24
@
6 years ago
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.
#25
@
5 years ago
Can we fix this in WordPress 5.4? Or close the ticket?
After over 7 years a clear decision would be a nice thing.
#26
@
4 years ago
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.
#27
@
4 years ago
We encountered this issue today as well...
+1 for this being looked into after more than 8 years now.
This ticket was mentioned in PR #3676 on WordPress/wordpress-develop by shoelaced.
2 years ago
#30
- Keywords needs-refresh removed
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.
#31
@
2 years ago
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?
#32
follow-up:
↓ 33
@
2 years ago
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. The value is a_string
, and sanitizing should append _1
.
Without the above change:
sanitize_option()
runs twice.add_option()
savesa_string_1_1
.
With the above change:
sanitize_option()
runs once.add_option()
savesa_string_1
.- All hooks that should fire continue to do so, with the expected value. (Confidence check me on this)
#33
in reply to:
↑ 32
@
2 years ago
Replying to costdev:
Curious, in
add_option()
, what if we wrap this line with a check ondid_filter
for the option?
Like this proposal!
#34
@
2 years ago
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()
orapply_filters( "sanitize_option_{$option}" )
before accidentally passing an unsanitized value toadd_option()
. If sanitization inadd_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.
#35
@
2 years ago
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.
#36
@
19 months ago
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
#37
@
9 months ago
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.
This ticket was mentioned in PR #6156 on WordPress/wordpress-develop by @lev0.
9 months ago
#39
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
9 months ago
#40
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
#41
@
8 months ago
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
#43
@
8 months ago
@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.
#44
in reply to:
↑ 20
@
6 months ago
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.
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.