WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 months ago

#35379 assigned defect (bug)

update_network_option changes stored option value if sanitize_option detects error

Reported by: thomaswm Owned by: jeremyfelt
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Options, Meta APIs Keywords: has-patch has-unit-tests needs-testing
Focuses: multisite Cc:

Description

If you make a call like

update_network_option( null, "admin_email", "this.is.not.an.email.address" );

then the value of the admin_email network option will be changed to the value of the admin_email site option in the database. Expected behaviour would be that the option value in the database is not changed.

The reason is that update_network_option calls

sanitize_option( "admin_email", "this.is.not.an.email.address" );

and writes its return value to the database. However, sanitize_option will return the value of get_option( "admin_email" ), instead of get_network_option( null, "admin_email" ). See lines 3863ff. in formatting.php.

Attachments (7)

35379.diff (2.4 KB) - added by codex-m 22 months ago.
Patch
35379_unit_test.diff (2.8 KB) - added by codex-m 22 months ago.
Unit test
35379.2.diff (2.5 KB) - added by codex-m 22 months ago.
Refreshed version of the patch with latest changes on trunk
35379_unit_test.2.diff (2.9 KB) - added by codex-m 22 months ago.
Refreshed version of the unit test.
35379.3.diff (4.2 KB) - added by codex-m 20 months ago.
Updated patch -merged version ($context and $context_id illustrated)
35379.4.diff (4.2 KB) - added by codex-m 20 months ago.
Refreshed version of patch simplified
35379.5.diff (10.1 KB) - added by codex-m 19 months ago.
sanitize_network_option function

Download all attachments as: .zip

Change History (38)

#1 @jeremyfelt
2 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.5
  • Version changed from 4.4 to 3.0

Confirmed.

It looks like this was introduced in [14231] not too long after update_site_option() was made multisite aware. It originally affected admin_email and siteurl. In [18346], WPLANG got similar treatment. And as of [32791], some of the fallback logic was consolidated and it now appears to affect illegal_names as well. This applies to several other single site options, but not all of them share names with network options.

I guess our two options are to add a sanitize_network_option() or add context to sanitize_option(). I'm inclined to go the route of sanitize_network_option() so that we can also account for the other network options that do not match the names of site options.

#2 @codex-m
22 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

I manage a multisite and came across this issue. My attempt to solve this in my local multisite is passing a new parameter for sanitize_option. This parameter is the network ID which can be an integer when using for a specific network in a multisite. Or it can be TRUE to use the current network. By default, it's false for non-multisite implementations. See attached 35379.diff.

I wrote some unit for test (see attached 35379_unit_test.diff). Covering three network affected options (admin_email, siteurl and WPLANG). This test also detects the issue (if the patch is not applied and confirms the fix when patch is applied.

In addition, I ran the overall unit test before and after the patch and I don't see any side effects on this. Feel free to review ;) Thanks a lot!

@codex-m
22 months ago

Patch

@codex-m
22 months ago

Unit test

@codex-m
22 months ago

Refreshed version of the patch with latest changes on trunk

@codex-m
22 months ago

Refreshed version of the unit test.

#3 @codex-m
22 months ago

I refreshed the version of the patch with latest changes on trunk/unit tests (includes code cleanup. :)

#4 @codex-m
22 months ago

  • Keywords dev-feedback needs-testing added

#5 @jeremyfelt
22 months ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

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


21 months ago

#7 @chriscct7
21 months ago

  • Milestone changed from 4.5 to Future Release

Punting per discussion in Slack

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


20 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


20 months ago

#10 @jeremyfelt
20 months ago

Thank you for your patches on this ticket, @codex-m. Can you take a look at the work in progress on #35698? That has a slightly wider focus, but should help to resolve this one as well.

#11 @flixos90
20 months ago

When merging #35698 into this ticket, we should probably have sanitize_option() use the $network_id as third parameter (as it provides more value and is needed for this ticket) and then automatically detect the $context variable inside the function like

$context = $network_id ? 'network' : 'site';

Alternatively we could add both as a parameter, $context as third parameter and then $network_id as fourth. We might even name it $context_id then as this would provide flexibility for possible other contexts in the future, also for the regular option context.

Examples: A function like get_option() could then call it as

sanitize_option( $option, $value, 'site', get_current_blog_id() )

while get_network_option() would call it as

sanitize_option( $option, $value, 'network', $network_id )

@codex-m
20 months ago

Updated patch -merged version ($context and $context_id illustrated)

#12 @codex-m
20 months ago

Thanks @flixos90 / @jeremyfelt for the update. I merged the final patch from this ticket #35698. Attached is the final patch (35379.3.diff) which is now a combination of the two patches. Basically it now illustrates the concept of adding $context which will have a value of 'network' for multisite implementation. And also the $context_id which will have the value of network ID when used with multisite. As suggested, I illustrated $context_id instead of $network_id for a more flexible implementation.

I re-tested this manually and also using our automated unit testing (including the attached unit tests previously). This is working great and fixes the original issue being reported in this ticket. The unit test to cover these changes is 35379_unit_test.2.diff (previously attached).

Let me know what you think :)

Last edited 20 months ago by codex-m (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


20 months ago

#14 @flixos90
20 months ago

@codex-m The patch looks great I think. One thing that could be simplified in my opinion are lines 3654-3668 (initial check for $context_id in network mode). It's probably not needed to add a new variable $network_id there, we should be able to use the original $context_id throughout the entire function. Maybe something like this:

<?php
if ( 'network' === $context ) {
    if ( ! is_multisite() ) {
        $context = 'site'; // or we could throw a _doing_it_wrong() here
    } elseif ( ! $context_id ) {
        global $current_site;
        
        $context_id = $current_site->id;
    }
}

#15 @jeremyfelt
20 months ago

  • Milestone changed from Future Release to 4.6

@codex-m
20 months ago

Refreshed version of patch simplified

#16 @codex-m
20 months ago

@flixos90 Great, I agree :) I attached the refreshed and simplified version of the patch (35379.4.diff). Re-tested with 35379_unit_test.2.diff​ as well as some manual testing. Everything works good and the original issue is resolved. Thanks for the feedback. Cheers.

#17 follow-ups: @jeremyfelt
20 months ago

  • Keywords dev-feedback removed

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

#18 in reply to: ↑ 17 @flixos90
20 months ago

Replying to jeremyfelt:

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

I'd prefer to use one function for this cause. Especially with a look at #15691, a Settings API for the network admin should work in a similar manner like the regular one. There is too much parity between site vs network options to create separate functions. I don't see the point in writing an additional function for so little change in functionality. I think we should generally go with an additional $context (and when needed $context_id) parameter to support existing functionality for networks.

What we could of course do is create a sanitize_network_option() function which would call the other function, automatically passing the network context. It would make it a little more intuitive to use, but not sure if this is needed either.

#19 in reply to: ↑ 17 @codex-m
20 months ago

Replying to jeremyfelt:

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

Like @flixos90 I prefer to use only one function instead of using sanitize_network_option. It is because options in WordPress are dual-purpose and can be used both (single-site or multisite). Options name in single-site is the same as in the network and validated similarly. Using one function in this case would be much more user-friendly to me.

By using only one function and passing appropriate values in $context and $context_id, we can easily process these options in a network mode (even though these options are still usable on a single site mode and validated the same.).

Cheers.

Last edited 20 months ago by codex-m (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


19 months ago

#21 @flixos90
19 months ago

#35698 was marked as a duplicate.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


19 months ago

@codex-m
19 months ago

sanitize_network_option function

#23 follow-up: @codex-m
19 months ago

@jeremyfelt Here is my patch illustrating the use sanitize_network_option. It's now possible to sanitize network-specific options by putting them on their own function.

Since several of the WordPress default core options are used to both single-site and multisite-mode. I placed them in a new validating function called wp_validate_default_options which is called by both sanitize_option' and sanitize_network_option`. I implemented similar filters for consistency between single-site mode and multisite usage.

I tested this using 35379_unit_test.2.diff test to cover this issue as well as standard WordPress unit tests. Works fine. Let me know what you think ;) Cheers.

#24 in reply to: ↑ 23 @flixos90
19 months ago

Replying to codex-m:

@jeremyfelt Here is my patch illustrating the use sanitize_network_option. It's now possible to sanitize network-specific options by putting them on their own function.

I'm personally still in favor of the approach with one function that handles it. If we need sanitize_network_option(), we could make it a wrapper for that function. But it's great to get an example of what the alternate approach would look like, so now we have a better base to discuss.

I think that, even if we go with the approach of using separate functions, we should start with the initial idea. Your new patch introduces completely new behavior which would definitely take some additional time to review.

My proposal would be to follow the approach of having one flexible function for sanitization and aim for a 4.6 merge with that - and then possibly, in a follow-up ticket, continue to think about ways to split up these function into separate. I don't see any backwards-compatibility issues in coming up with outsourcing parts of the one function-approach / wrapping it in a sanitize_network_option() at a later point.

A minor note to your patch: I think we would still need the original sanitize_option filter to be present in the sanitize_network_option() function (for backwards compatibility, see https://core.trac.wordpress.org/ticket/35698#comment:2).

Last edited 19 months ago by flixos90 (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


18 months ago

#26 @flixos90
18 months ago

  • Milestone changed from 4.6 to Future Release

We need to think further on what the implementation of this ticket will look like. Especially given a possible #37181, we could improve the way we handle network options a lot (think register_meta() here) - although it's a pain in terms of naming conventions.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


16 months ago

#28 @flixos90
15 months ago

  • Milestone changed from Future Release to 4.7

Moving to milestone for consideration. It has a solid patch that would only require minor updates now, but we still need to find out whether this is the route to take.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


15 months ago

#30 @flixos90
15 months ago

  • Milestone changed from 4.7 to Future Release

Punting to a future release. As this is a blocker for a possible network settings API, I hope we can get to it in 4.8.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


3 months ago

Note: See TracTickets for help on using tickets.