WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 6 weeks ago

#42069 closed defect (bug) (fixed)

Saving metadata fails (randomly) if equal value already exists

Reported by: JVel Owned by: kadamwhite
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.8.2
Component: REST API Keywords: has-patch has-unit-tests fixed-5.0
Focuses: rest-api Cc:

Description

When using js rest api to save page.

Duplicate value test in wp-includes/rest-api/fields/class-wp-rest-meta-fields.php line 303 fails and causes error "Could not update meta value in database."

if ( $old_value[0] === $meta_value ) {

Problem seems to be that $old_value is not going through wp_slash but new $meta_value is.

I get it working by changing code to test against $value instead.

if ( $old_value[0] === $value ) {

Attachments (13)

42069.1.diff (3.9 KB) - added by dcavins 11 months ago.
Improve reliability of updating meta via REST API.
42069.2.diff (4.6 KB) - added by dcavins 8 months ago.
Update tests to use new REST fake server. Add test of single-slash case.
42069.2.2.diff (4.6 KB) - added by dcavins 8 months ago.
Update tests to use new REST fake server. Add test of single-quote case.
42069.diff (3.2 KB) - added by flixos90 7 months ago.
42069.1.2.diff (3.8 KB) - added by MattGeri 6 months ago.
42069.3.diff (4.0 KB) - added by dcavins 5 months ago.
Correct problem in test setup; use string typecast to improve equivalency check.
42069.4.diff (3.5 KB) - added by boonebgorges 4 months ago.
42069.5.diff (4.2 KB) - added by dcavins 4 months ago.
Add JSON string test.
42069.5.2.diff (4.2 KB) - added by dcavins 4 months ago.
Use better meta name from Boone's patch.
42069.6.diff (3.0 KB) - added by boonebgorges 3 months ago.
42069.7.diff (4.2 KB) - added by TimothyBlynJacobs 3 months ago.
42069.8.diff (4.3 KB) - added by TimothyBlynJacobs 3 months ago.
42069.9.diff (4.7 KB) - added by boonebgorges 3 months ago.

Download all attachments as: .zip

Change History (48)

#1 @pilou69
16 months ago

Having an issue with this too in 4.8.2.

It's not random, it always happens when the meta is not modified and has any characters that must be slashed.

We found 3 ways to work around this:

  1. Apply modification above, but it overrides the core :(
    if ( $old_value[0] === $value ) {
    
  1. Register meta fields as single => false. But that seems very ugly.
  1. Create custom endpoint:

create a class extending WP_REST_Meta_Fields, and override update_meta_value function with patch applied above

extend WP_REST_Posts_Controller (WP_REST_{$type}_Controller),)

override in you custom endpoint controller $this->meta with your Class with the patch

@dcavins
11 months ago

Improve reliability of updating meta via REST API.

#2 @dcavins
11 months ago

  • Keywords has-patch added

I too ran into this problem today and was able to identify two cases where WP_REST_Meta_Fields::update_meta_value() was failing to interpret the values as identical, but update_metadata() was returning false (because the values were considered equal by that function), so the REST request was getting a 500 error back. These two cases are problematic:

  • Text containing characters that are slashed or converted to entities, like " or <.
  • Booleans, like a checkbox, that, when unchecked, pass the integer 0.

In the first case, the comparison in WP_REST_Meta_Fields::update_meta_value() tries to strict compare &lt; to <, and, in the second case, the string '0' (all saved meta is strings) is compared to the incoming sanitized value of (int) 0.

I've created a diff with tests showing these failures and a proposed patch to improve the comparison in WP_REST_Meta_Fields::update_meta_value() so that it catches more cases that update_metadata() or $wpdb->query() will consider equal.

If there are other cases that I can write tests for, please describe the situation in detail in your comments. I'd like to find all failures and make sure that the fix works for all of them.

Thanks for your comments!

#3 @dcavins
11 months ago

Also note that #42810 is related.

#4 follow-up: @MattGeri
8 months ago

I'm encountering this issue too. I've looked at the patch and my suggestion would be that you remove the sanitization from the old value as it's already unslashed and sanitized. Sanitization and unslashing should only be applied to $meta_value.

So the conditional could read something like this, to satisfy all the usecases and tests:

if ( $old_value[0] === sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) ) {

Let me know your thoughts and if you agree but can't make the changes to the patch, I'd be happy to.

#5 in reply to: ↑ 4 @dcavins
8 months ago

Replying to MattGeri: Thanks for your comments. Your change is close to what TimothyBlynJacobs suggested in https://core.trac.wordpress.org/attachment/ticket/42810/42810.diff

Can you offer a use case that fails in his previous patch, so that we can explain the necessity of adding wp_unslash()?

I'm fuzzy on the details, but it seems like one of my test cases was failing with the previous patch, probably the '0' (from saved meta) === 0 (incoming sanitized POST) case, which is why I added the sanitize step to the saved value as well. It's easy to test, but hard to remember something from a few months ago.

Thanks!

#6 @MattGeri
8 months ago

No worries. I think it was actually the other test that would have been failing.

The issue is that $old_value[0] gets returned unslashed from get_metadata, but $meta_value is wrapped in slashes earlier in the method. So it's trying to compare slashed against unslashed data, which the other solution does not take in to account.

The data in meta.php > update_metadata() gets unslashed on line 167 this is before the same comparison as the rest function happens to check if an update should occur.

Edit: So an example usecase would be text with a single quote in it.

Last edited 8 months ago by MattGeri (previous) (diff)

@dcavins
8 months ago

Update tests to use new REST fake server. Add test of single-slash case.

#7 @dcavins
8 months ago

Hi @MattGeri- I've tested your suggestion, and it works for textured text, but fails when updating booleans (test_update_value_return_success_response_when_updating_boolean_values_no_change() here: https://core.trac.wordpress.org/attachment/ticket/42069/42069.2.diff)

I've added a test that includes a single quote per your suggestion. Any other feedback is welcome--it'd be great to have comprehensive tests of cases we know fail.

Without the change, all three of my test cases fail. :(

@dcavins
8 months ago

Update tests to use new REST fake server. Add test of single-quote case.

#8 @adamsilverstein
7 months ago

  • Milestone changed from Awaiting Review to 5.0

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


7 months ago

@flixos90
7 months ago

#10 @flixos90
7 months ago

  • Keywords needs-testing added

I cannot replicate this issue unfortunately. The tests you wrote all passed for me without actually using the changes in the code. @dcavins @MattGeri @adamsilverstein Can you provide some more background on your environment? I wonder if it's some PHP version-related detail or similar.

Regarding the patch, I just uploaded 42069.diff with optimized tests using a data provider, which reduces the necessary code and at the same time tests a lot more combinations. There's one test for checking updating a value with exactly the same value, and another one for checking updating a value with another value that essentially means the same (like 0/1 or false/true for booleans). My new patch only includes the tests as they passed without the code change. I'm not at all saying that change is wrong, it just did not fix a problem for me, so I excluded it for now. Of course, as we determine where exactly this problem is coming from, we can add the fix back as needed. Please use the new patch going forward and iterate from there.

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

#11 @adamsilverstein
7 months ago

@flixos90

Nice work on the unit test cleanup! I pushed the changes from 42069.diff to a branch/fork to run in travis and see some meta tests failing, surprising these don't fail for you locally. are you sure the new tests are running, possibly you need to run build before testing?

https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/396971510

#12 @flixos90
7 months ago

@adamsilverstein I did make sure all of this is there. Must be something in my setup, maybe I can dive in in a couple days. Did you check whether bringing back the actual code change makes all those tests pass? My general feeling is, if so, we can commit this. Those tests should catch enough of these cases.

#13 @dcavins
7 months ago

Thanks very much for the unit test expansion, @flixos90! I haven't seen or used the dataProvider functionality and look forward to simplifying many of my unit tests in the future.

I also tested the new tests, and I'm not seeing any failures on my local machine, either. However, when I create a build, I'm still seeing a 500 response code when updating textured text via REST, so something must have changed in the test environment that is masking the failures.

Thanks again for taking a look at this issue.

@MattGeri
6 months ago

#14 @MattGeri
6 months ago

@flixos90 This is still very much an issue for us in a production environment. I've run your tests locally and some of the tests fail for me (as expected). By applying the patch I'm attaching 42069.1.2.diff (combination of your tests + a modified fix from above), it fixes the issue and the tests pass. Not too sure why they're passing on your local without the changes applied, but I've got multiple use cases where this bug breaks saving meta because it's not unslashed.

I do however seem to get 2 unrelated test fails when I run them. The dataProvider is somehow passing the test_bad_auth meta field in to tests that expect a 200 response, and the update fails. I'll try look in to this when I get some free time.

@dcavins In the patch I'm attaching 42069.1.2.diff, I've applied what I suggested above (only sanitize and unslash the $meta_value coming in and not the $old_value) and the tests pass. I'm not sure why you were only able to get the tests (and boolean use cases) to pass by sanitizing the old and new values. We shouldn't need to sanitize the old value as the original method in meta.php doesn't.

#15 @dcavins
6 months ago

Hi @MattGeri-

Thanks for your interest in this problem. I took a minute to test your patch, and meta of the type boolean or integer is still failing. Example meta definitions:

// Boolean
register_meta( 'post', 'featured_item', array(
        'sanitize_callback' => 'absint',
        'type' => 'boolean',
        'description' => 'test',
        'single' => true,
        'show_in_rest' => true,
) );

// Integer
register_meta( 'post', 'post_order', array(
        'sanitize_callback' => 'absint',
        'type' => 'integer',
        'description' => 'test',
        'single' => true,
        'show_in_rest' => true,
) );

The way-down-deep issue for this case is that if you pass in an integer, like 1, and the strictly equal check doesn't catch the equivalency, it will also pass the check in update_metadata() (so at least it won't return false at that point) and be passed to $wpdb->update(). $wpdb->update() relies on $wpdb->query() for the actual insert, and the returned value is the number of rows affected, which is 0. This causes update_metadata() to return false at this point: https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/meta.php#L251

My patch catches this case because both the old value and new value are cast to be the same type by the sanitization functions. Now that I know where the integer/boolean case is failing, I see that there are more ways to achieve this result, like relaxing the strictness of the check in WP_REST_Meta_Fields::update_meta_value():

if ( sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) == $old_value[0] ) {
        return true;
}

Alternatively, we could cast the incoming value as a string, because we know all meta values retrieved by get_metadata() are strings:

if ( (string) sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) === $old_value[0] ) {
        return true;
}

Thanks again for your thoughts on this problem.

#16 @MattGeri
6 months ago

@dcavins Thanks for the insight. The unit tests provided by @flixos90 do not seem to cover that use case (at least on my local) as they all passed with my patch. If they also pass your side, could you add a test case which would cover the issue?

Relaxing the strictness would not be an option as it's against the code standards. Typecasting as string seems to be viable but we should provide a failing unit test first.

#17 @dcavins
5 months ago

Ah, the test setup was broken--the meta registration was moved in one of the patch updates. I'll upload a new patch that corrects the test setup and also moves to the (string) sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) === $old_value[0] comparison check.

With the corrected tests (and the old comparison code in place), 9 out of 15 assertions fail:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #0 ('test_boolean_update', 0)
Failed asserting that 500 matches expected 200.

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #2 ('test_boolean_update', false)
Failed asserting that 500 matches expected 200.

3) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', '')
Failed asserting that 400 matches expected 200.

4) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #5 ('test_boolean_update', '0')
Failed asserting that 500 matches expected 200.

5) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #7 ('test_textured_text_update', 'She said, "What about the > 1...chen?"')
Failed asserting that 500 matches expected 200.

6) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #8 ('test_textured_text_update', 'He's about to do something rash...')
Failed asserting that 500 matches expected 200.

7) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, '')
Failed asserting that 400 matches expected 200.

8) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #2 ('test_boolean_update', false, 0)
Failed asserting that 500 matches expected 200.

9) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #4 ('test_boolean_update', '', false)
Failed asserting that 500 matches expected 200.

7 of these failures are the expected 500 when update_metadata() fails. 2, though, are "Bad Request" 400 errors, caused by passing an empty string as the update value to the test, and these are unaffected by the changes, so I'm assuming that they happen upstream of the actual update routine.

With the new comparison in place, there are only the two "Bad Request" failures:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', '')
Failed asserting that 400 matches expected 200.

/Users/dcavins/Sites/develop.git.wordpress.org/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1239

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, '')
Failed asserting that 400 matches expected 200.

Thanks again for the help with getting this change accepted.

@dcavins
5 months ago

Correct problem in test setup; use string typecast to improve equivalency check.

#18 @boonebgorges
4 months ago

Quoting from https://core.trac.wordpress.org/ticket/42810#comment:10:

I'd like to flag this as a Gutenberg blocker, because of the following situation:

  1. You have a post with more than one block that uses post meta attributes
  2. One or more of those blocks save their data as a JSON-encoded string, as recommended in the documentation https://wordpress.org/gutenberg/handbook/block-api/attributes/#considerations
  3. You save the post without modifying the data in one or more of the blocks

Because the REST API uses *slashed* data to compare with the existing values, but get_metadata() returns *unslashed* data, most JSON-encoded strings will look "new" to the REST API https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php?marks=299,301#L287 But update_metadata() correctly compares unslashed-to-unslashed values, so returns false. Not only does this cause an "Updating failed" notice in Gutenberg, but because of the way that the update_value() method returns early out of the foreach() loop, blocks further down the page are not processed. https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php?marks=131,163-165#L129

#19 @TimothyBlynJacobs
4 months ago

#42810 was marked as a duplicate.

#20 follow-up: @boonebgorges
4 months ago

Responding to @dcavins https://core.trac.wordpress.org/ticket/42810#comment:12:

Hm, the only way I could get any of these tests to fail at all was to register_meta(). Otherwise the REST API endpoint was ignoring them. See 42069.4.diff. With that change, the tests are now failing/passing as expected.

#21 in reply to: ↑ 20 @dcavins
4 months ago

Replying to boonebgorges:

Responding to @dcavins https://core.trac.wordpress.org/ticket/42810#comment:12:

Hm, the only way I could get any of these tests to fail at all was to register_meta(). Otherwise the REST API endpoint was ignoring them. See 42069.4.diff. With that change, the tests are now failing/passing as expected.

Yes, the meta was registered in the setup() method in the earlier patches, like https://core.trac.wordpress.org/attachment/ticket/42069/42069.3.diff

The only advantage over the way you did it was that then the metas are created with a sanitization function appropriate to their type. I'll attach a patch that adds your JSON string test that _does_ fail for me, even though my example didn't (a mystery? ha ha). Thanks for being interesting in this issue. It has caused me some problems with moving old admin-ajax-reliant code to using the REST API.

@dcavins
4 months ago

Add JSON string test.

@dcavins
4 months ago

Use better meta name from Boone's patch.

#22 @danielbachhuber
3 months ago

  • Keywords has-unit-tests added

@boonebgorges @adamsilverstein Are you happy with 42069.5.2.diff?

#23 @adamsilverstein
3 months ago

@danielbachhuber Looks good to me!

#24 @boonebgorges
3 months ago

I don't think this is ready to commit yet. The test_boolean_update tests are failing for me:

` There were 2 failures:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', ) Failed asserting that 400 matches expected 200.

/home/bgorges/sites/wpdev/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1209

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, ) Failed asserting that 400 matches expected 200.

/home/bgorges/sites/wpdev/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1247

`

The problem seems to be that the test_boolean_update meta key is registered with type=boolean (and sanitize_callback=absint, which doesn't seem like a proper match). As such, passing values like 1 or "1" is causing this check to fail https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L149 (@dcavins alludes to this above https://core.trac.wordpress.org/ticket/42069#comment:17)

If I understand the design correctly, the strict checks that are coupled with proper use of register_meta() mean that we should not be testing for loosely-equal values in this way. In other words, meta values are not exposed via the REST endpoints unless they're registered with a strict type, and thus we don't support non-strict updates. If that's correct, then I believe the proper patch is 42069.6.diff.

This could use a review from @dcavins (as I may be misunderstanding the idea behind this part of his patches) as well as a member of the REST API team.

#25 @dcavins
3 months ago

Thanks @boonebgorges for taking another look at this.

Yes, the extra boolean similar-value tests were included because it appears that meta registered with the boolean type is more permissive than, say, meta registered with the integer type. (The integer type is pretty permissive, though, because it will allow numeric strings.)

If a meta is set to boolean, it seems to be currently possible to pass any boolean-ish value to it successfully, like true, false, 0, 1, "0", or "1" (or any other string, actually) without triggering the mismatched-type 400 error.

The above comments are an aside from Boone's patch, which is the necessary fix. The other tests I had included I agree should be removed because they're requiring the API to cover for poor quality code.

Thanks everyone for your interest in this ticket!

#26 @TimothyBlynJacobs
3 months ago

I made a couple of changes.

  1. Metadata has subtypes now, so we need to take this into account when sanitizing.
  2. We weren't handling the slashing of meta key consistently. It needs to be slashed when updating meta, but unslashed for all the other functions. Because of this I changed the code to keep the unslashed versions around instead of continually unslashing them and then slashed as the values went into update_metadata().

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


3 months ago

#28 @danielbachhuber
3 months ago

@boonebgorges Are you happy with 42069.7.diff?

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


3 months ago

#30 @TimothyBlynJacobs
3 months ago

There was an issue applying the patch, for some reason the new register_post_meta calls were getting inserted into the middle of another test method.

#31 @boonebgorges
3 months ago

  • Keywords commit added; needs-testing removed

Thanks for catching that in 42069.8.diff, @TimothyBlynJacobs . I've just added 42069.9.diff , which includes the following mods to 8.diff:

  • Coding standards fixes
  • Added missing @ticket annotation on new test
  • Adds a small amount of logic to the test class constructor so that errors are not thrown if the tests are run in isolation (eg using --group 42069)

I think this is ready for commit, which either I can do, or a member of the REST API team.

#32 @danielbachhuber
3 months ago

@boonebgorges Please do ship it :)

#33 @kadamwhite
3 months ago

  • Owner set to kadamwhite
  • Resolution set to fixed
  • Status changed from new to closed

In 43740:

REST API: Slash existing meta values when comparing with incoming meta upates.

When comparing the old and new values for a meta key being set, ensure both values are sanitized using the same logic so that equal values match.

props boonebgorges, dcavins, MattGeri, pilou69, TimothyBlynJacobs.
Fixes #42069.

#34 @danielbachhuber
3 months ago

  • Keywords fixed-5.0 added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merge against trunk.

#35 @desrosj
6 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44113:

REST API: Slash existing meta values when comparing with incoming meta upates.

When comparing the old and new values for a meta key being set, ensure both values are sanitized using the same logic so that equal values match.

props boonebgorges, dcavins, MattGeri, pilou69, TimothyBlynJacobs, kadamwhite.

Merges [43740] to trunk.

Fixes #42069.

Note: See TracTickets for help on using tickets.