REST API: 403 error occur when resolving or reopening note

[wildworks]

Originally reported on ​https://github.com/WordPress/gutenberg/issues/72690

When resolving or reopening a note, the comment meta _wp_note_status is updated. This itself works correctly, but a 403 error occurs with a message Sorry, you are not allowed to edit the _wp_note_status custom field..

My understanding is that because the meta key has an underscore prefix, it is treated as a protected meta key. Therefore, I believe that the auth_callback that was deleted ​here is still necessary.

[wildworks]

Strangely, this issue is reproducible in the release version of Beta 1, but it does not occur when building the wordpress-develop repository locally. The problem might have already been fixed in the trunk. I'd like to test it again once Beta2 is released.

[desrosj]

Replying to wildworks:

Strangely, this issue is reproducible in the release version of Beta 1, but it does not occur when building the wordpress-develop repository locally. The problem might have already been fixed in the trunk. I'd like to test it again once Beta2 is released.

I came to report this same problem and had the same findings as @wildworks.

[desrosj]

#64162 was marked as a duplicate.

[wildworks]

but it does not occur when building the wordpress-develop repository locally

Sorry, this was because my local environment is a multisite setup, and I was logged in as a super admin user. The super admin user has all caps: ​https://github.com/WordPress/wordpress-develop/blob/781fb28d4773147edb17b0fd7eefa52671b5daa6/src/wp-includes/class-wp-user.php#L786-L792

My understanding is that because the meta key has an underscore prefix, it is treated as a protected meta key. Therefore, I believe that the auth_callback that was deleted here is still necessary.

In the end, I think this prediction was correct. ​The PR 10430 should solve the problem.

Do you have any ideas why unit tests didn't catch this failure? Why isn't test_create_empty_note_with_resolution_meta test failing on trunk?

It would be nice to have unit test coverage for _wp_note_status updated and permissions.

I was just doing more tests and realized that running only test_create_empty_note_with_resolution_meta properly fails on trunk, but it passes when running the whole test suite.

Unfortunately, I'm not sure what is causing this different behavior. @desrosj, @TimothyBJacobs, any ideas?

npm run test:php -- --filter test_create_empty_note_with_resolution_meta

I also noticed this and was in the process of investigating the cause. As a result of this, adding new tests and testing them becomes somewhat complicated 😅

If the problem cannot be resolved, I plan to split the test into two parts, without using a data provider.

If the problem cannot be resolved, I plan to split the test into two parts, without using a data provider.

The test is still passing for me, even after removing the data provider (at least that's the case in the Gutenberg repo). I can only fail it when running the command above.

test_create_empty_note_with_resolution_meta properly fails on trunk, but it passes when running the whole test suite.

Odd, that sounds like some other test is causing a side effect. 👀

When I run the test in isolation, the response error is: Sorry, you are not allowed to edit the _wp_note_status custom field

When I run the test in isolation, the response error is: Sorry, you are not allowed to edit the _wp_note_status custom field

Yes, for some reason, the comment metadata itself is either not registered or the authentication has failed.

The underlying cause here was the meta was no longer registered after the first test ran - the test controller probably cleans up all meta registration between test runs. The solution was to add the meta registration to the set_up method, after this change the test fails in isolation and when run with other tests. I added the change to this PR - ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/27bd29d5ff011ae3abe15fb8b84a16fa65d3ec4d assuming you are trying to fix this issue, but feel free to remove it if it belongs elsewhere.

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

Looking into this further, the meta registrations are stored in the $wp_meta_keys global, so the registration must run before the test. Whats unclear to me so far is why the init hook isn't firing for every run, it could have to do with where we are calling add_action( 'init'. I'm looking further.

the test controller probably cleans up all meta registration between test runs.

That's unexpected.

It happens as part of the top level tear down, see ​https://github.com/WordPress/wordpress-develop/blob/828a3fe5e225dd0c3206f5b7ec2f5b7b1d9afb4e/tests/phpunit/includes/abstract-testcase.php#L457

it could have to do with where we are calling add_action( 'init'. I'm looking further.

This wasn't the issue, although I we should probably move that hook to src/wp-includes/default-filters.php like every other core default hook.

it could have to do with where we are calling add_action( 'init'. I'm looking further.

This wasn't the issue, although I we should probably move that hook to src/wp-includes/default-filters.php like every other core default hook.

Actually this was entirely the issue. The test suite fires "rest_api_init" - ​https://github.com/WordPress/wordpress-develop/blob/828a3fe5e225dd0c3206f5b7ec2f5b7b1d9afb4e/tests/phpunit/includes/testcase-rest-controller.php#L13 so moving the hook there fixed the issue. After ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/55011513596eebd858824299737eb1926fc97405 tests now pass locally and I see the meta values being set.

Some tests were failing,I tried regenerating fixtures to see if that is related.

Some tests were failing,I tried regenerating fixtures to see if that is related.

The failures here were actually because REST API fixture generation was failing (not sure when this started, possibly related to commit 8c2ec298ad). The code in embed.php uses file_get_contents() to read the wp-embed.js file, which doesn't exist in the src directory where tests are run (it's a built asset). It became apparent here because this PR results in a fixture change.

Other oEmbed tests already handle this touch()ing the expected file in their set_up() method to create an empty placeholder file. The REST schema fixture generation test was missing this setup, causing it to fail.

Fixed in 05e386cbbff046adf03f8ddc2028738fde457e39, I reverted my other changes.

cc: @westonruter fun times

Working in ​https://github.com/WordPress/wordpress-develop/pull/10434 to avoid too much noise here, I see one other failing test thaat is meta related, once I have those tests green, I'll merge the changes back here

The failures here were actually because REST API fixture generation was failing (not sure when this started, possibly related to commit ​8c2ec29). The code in embed.php uses file_get_contents() to read the wp-embed.js file, which doesn't exist in the src directory where tests are run (it's a built asset). It became apparent here because this PR results in a fixture change.

Other oEmbed tests already handle this touch()ing the expected file in their set_up() method to create an empty placeholder file. The REST schema fixture generation test was missing this setup, causing it to fail.

Fixed in ​05e386c, I reverted my other changes.

Indeed!

​https://github.com/WordPress/wordpress-develop/blob/87cbbb1dfcf19fcfc128fc66603462a649d01502/tests/phpunit/tests/oembed/controller.php#L38-L39

​https://github.com/WordPress/wordpress-develop/blob/87cbbb1dfcf19fcfc128fc66603462a649d01502/tests/phpunit/tests/oembed/getResponseData.php#L11-L12

​https://github.com/WordPress/wordpress-develop/blob/87cbbb1dfcf19fcfc128fc66603462a649d01502/tests/phpunit/tests/oembed/wpOembed.php#L38-L39

• ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/1cba83918cbff0a8a60bfcb758b0ea50719a16c5 ensures the meta is registered for tests (see ​https://github.com/WordPress/wordpress-develop/blob/828a3fe5e225dd0c3206f5b7ec2f5b7b1d9afb4e/tests/phpunit/includes/testcase-rest-controller.php#L13) - otherwise the test_create_empty_note_with_resolution_meta test only runs once and the meta registration is later deleted (in ​https://github.com/WordPress/wordpress-develop/blob/828a3fe5e225dd0c3206f5b7ec2f5b7b1d9afb4e/tests/phpunit/includes/abstract-testcase.php#L457)
• ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/a9ffe5c84aaafc0d10b7c24303ab119cf980872e expands test_create_empty_note_with_resolution_meta to check that the meta value is actually set. previously no meta value was set, but the API call was still resulting in a status of 201 so the tests were passing.
• ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/2188f3e556431ed7dd1932581a54828944be5a0e updated rest-api QUnit test fixtures which is required since we are adding a new meta field. Long story.
• ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/31d9ef95e1aff4ced0f9a14a9f3cf5f8cee3e6b0 updates another test to include _wp_note_status in the expected results for comment meta.

@westonruter can you give this another review, see my last comment for an explanation of the additional changes.

@t-hamano Apologies for taking over your ticket, I was trying to help with the failing tests and went down a bit of a rabbit hole. I am done making changes here!

@adamsilverstein Thank you so much for the detailed investigation!

• ​https://github.com/WordPress/wordpress-develop/pull/10430/commits/1cba83918cbff0a8a60bfcb758b0ea50719a16c5 switches _wp_note_status meta registration from init to rest_api_init which ensures the it is registered for tests

My only concern is that with this hook, the timing for registering the comment meta might be a little late. There might be cases where someone wants to access this comment meta before the REST API is initialized.

What are your thoughts on directly executing the callback function within the set_up method? Also, note that +add_action is being moved to default-filters.php.

src/wp-includes/comment.php

@@ -4135 +4135 @@
                )
        );
 }
-add_action( 'rest_api_init', 'wp_create_initial_comment_meta' );

src/wp-includes/default-filters.php

@@ -151 +151 @@
 add_action( 'added_comment_meta', 'wp_cache_set_comments_last_changed' );
 add_action( 'updated_comment_meta', 'wp_cache_set_comments_last_changed' );
 add_action( 'deleted_comment_meta', 'wp_cache_set_comments_last_changed' );
+add_action( 'init', 'wp_create_initial_comment_meta' );
 
 // Places to balance tags on input.
 foreach ( array( 'content_save_pre', 'excerpt_save_pre', 'comment_save_pre', 'pre_comment_content' ) as $filter ) {

tests/phpunit/tests/rest-api/rest-comments-controller.php

@@ -170 +170 @@
        public function set_up() {
                parent::set_up();
                $this->endpoint = new WP_REST_Comments_Controller();
+               wp_create_initial_comment_meta();
+
                if ( is_multisite() ) {
                        update_site_option( 'site_admins', array( 'superadmin' ) );
                }

Yes that works and aligns with how meta is usually registered so maybe better.

I pushed the following changes.

• 713c73cf5c05a176266745c2c19bf0125467b69f: In the setup method, I explicitly executed wp_create_initial_comment_meta. At the same time, I moved the hook to default-filters.php.
• 1c75fb910d14ce9c2daea3f072bd2d5b7008f109: I reverted some of the changes because they didn't seem to affect the test results. Are these changes necessary in this pull request?

Thanks for debugging this, folks. I think this is good to commit.

Sidenote: The unregister_all_meta_keys makes testing meta values for REST controllers somewhat unpredictable. Do you know if this behavior is documented somewhere?

May I try to commit this pull request myself? I believe I have the SVN environment set up locally: ​https://wordpress.slack.com/archives/C18723MQ8/p1761227412356249

I created the following commit message, and I would appreciate it if you could review it.

Editor: Add `auth_callback` to `_wp_note_status` comment meta.

Adds an `auth_callback` to the `_wp_note_status` comment meta so that only users with the `edit_comment` capability can update this meta field via the REST API. 

This is necessary to ensure that users can properly resolve or reopen Notes.

Props wildworks, adamsilverstein, westonruter, mamaduka, desrosj.
Fixes #64153.

The commit message looks good to me 👍

Thank you! I'm going to start preparing for the commit now...

[wildworks]

In 61089:

Editor: Add auth_callback to _wp_note_status comment meta.

Adds an auth_callback to the _wp_note_status comment meta so that only users with the edit_comment capability can update this meta field via the REST API.

This is necessary to ensure that users can properly resolve or reopen Notes.

Props wildworks, adamsilverstein, westonruter, mamaduka, desrosj.
Fixes #64153.

The commit appears to have completed successfully 👍

Congrats @t-hamano!

Nice work on your first commit to core @t-hamano! 🎉 🎉 🎉

1c75fb9: I reverted some of the changes because they didn't seem to affect the test results. Are these changes necessary in this pull request?

Thats fine; these fix some CI errors I noticed, but they aren't blockers - I can add in a future commit.

[jorbin]

In 61138:

Coding Standards: Fix alignment

In [61089], auth_callback was added which is causing phpcs to complain about the alignment with the other arguments in the array.

Follow-up to [61089] and [61036].

See #64153, #63168.