PHPCS: Likes #24368

sdixon194 · 2022-05-13T19:05:55Z

Fixes #21985

Changes proposed in this Pull Request:

PHPCS for the likes module.

Jetpack product discussion

na

Does this pull request change what data or activity we track or use?

no

Testing instructions:

Proof/test

github-actions · 2022-05-13T19:07:17Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: June 7, 2022.
Scheduled code freeze: May 31, 2022.

anomiex · 2022-05-13T19:58:48Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

-				if ( ! isset( $_POST['wpl_enable_post_sharing'] ) ) {
+			if ( isset( $_POST['post_type'] ) && in_array( $_POST['post_type'], get_post_types( array( 'public' => true ) ), true ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
+				if ( ! isset( $_POST['wpl_enable_post_sharing'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
 					update_post_meta( $post_id, 'sharing_disabled', 1 );


Err, isn't this a site change right here? And there are more below too.

anomiex · 2022-05-13T20:00:42Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

-				if ( true == $db_state && ! $this->in_jetpack ) {
-					$g_gif = file_get_contents( 'https://pixel.wp.com/g.gif?v=wpcom-no-pv&x_likes=disabled_likes' );
-				}
 				update_option( 'disabled_likes', 1 );


Again, site change right here, and more below.

anomiex · 2022-05-13T20:01:15Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

 		// WPCOM only: Comment Likes
 		if ( ! $this->in_jetpack ) {
-			$new_comments_state = ! empty( $_POST['jetpack_comment_likes_enabled'] ) ? $_POST['jetpack_comment_likes_enabled'] : false;
+			$new_comments_state = ! empty( $_POST['jetpack_comment_likes_enabled'] ) ? sanitize_option( wp_unslash( $_POST['jetpack_comment_likes_enabled'] ) ) : false; // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no changes to the site, only enable or disabling comment likes.


only enable or disabling comment likes.

That's a change to the site though, isn't it?

sdixon194 · 2022-05-17T19:11:06Z

Okay, think I fixed up the nonces. Let me know if that looks okay.

coder-karen · 2022-05-18T10:07:32Z

Spent some time testing this out to see if I could break anything but it all looks good. I see the nonce in the page source.

I went down a rabbit hole too looking into whether or not the nonce fields are checked earlier, and the 'sharing-options' nonce is only checked if sharing is not enabled.

Functionality-wise I'm not sure if I may be missing something when reviewing in terms of nonces but wanted to comment anyway with what I was looking at and what I found. Happy to approve based on that unless anyone else wants to take a look.

sdixon194 · 2022-05-23T18:25:19Z

Had to fix a merge conflict so this could use another approval 👍

matticbot · 2022-05-23T18:29:09Z

Caution: This PR has changes that must be merged to WordPress.com
Hello sdixon194! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D81308-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2022-05-24T17:18:19Z

Great news! One last step: head over to your WordPress.com diff, D81308-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

sdixon194 · 2022-05-24T17:24:23Z

r246252-wpcom

anomiex · 2022-05-24T18:01:51Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

 		if ( ! $this->in_jetpack ) {
 			if ( isset( $_POST['post_type'] ) && in_array( $_POST['post_type'], get_post_types( array( 'public' => true ) ), true ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
-				if ( ! isset( $_POST['wpl_enable_post_sharing'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
+				if ( ! isset( $_POST['wpl_enable_post_sharing'] ) && isset( $_POST['_likesharenonce'] ) && wp_verify_nonce( $_POST['_likesharenonce'], 'likes-and-shares' ) ) { // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- WordPress core doesn't unslash or verify nonces either.


This nonce check seems to have been added incorrectly. As written, if wpl_enable_post_sharing is set but the nonce check fails, it'll still enable sharing. And the check here doesn't at all protect lines 162, 166, or 176.

I'd have added the nonce check at around line 139 instead. Offhand I don't know if I'd have had it wp_die() or just return $post_id if the check fails, that depends on whether it'd make more sense to abort or just ignore the attempt.

Then see how many of the phpcs:ignores can be removed.

anomiex · 2022-05-24T18:33:19Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

+		if ( ! empty( $_POST['wpl_default'] ) && isset( $_POST['_wpnonce'] ) && wp_verify_nonce( $_POST['_wpnonce'], 'sharing-options' ) ) { // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- WordPress core doesn't unslash or verify nonces either.
+			$new_state = sanitize_text_field( wp_unslash( $_POST['wpl_default'] ) );
+		} else {
+			$new_state = 'on';


The logic here seems wrong. If the nonce check fails, we change the setting to "on"?

In practice it looks like an earlier nonce check will have already prevented this from being called so that won't actually happen. But if we're going to have a nonce check at all instead of relying on that earlier one, we should do the check separately and either return or or wp_die() depending on which behavior makes more sense.

anomiex · 2022-05-24T18:33:28Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

+			$new_state = 'on';
+		}
+
+		if ( ! empty( $_POST['jetpack_reblogs_enabled'] ) && isset( $_POST['_wpnonce'] ) && wp_verify_nonce( $_POST['_wpnonce'], 'jetpack-reblog-option' ) ) { // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- WordPress core doesn't unslash or verify nonces either.


Why are you checking the nonce again with a different action? That's never going to work.

And trying it on wpcom (I'm not sure how to get there clicking, maybe it's hidden in Calypso instead, but going directly to /wp-admin/options-general.php?page=sharing on my sandbox works) I find that I can no longer change this setting, it's always set to "on".

anomiex · 2022-05-24T18:36:32Z

projects/plugins/jetpack/modules/likes/jetpack-likes-settings.php

+			if ( ! empty( $_POST['jetpack_comment_likes_enabled'] ) && isset( $_POST['_wpnonce'] ) && wp_verify_nonce( $_POST['_wpnonce'], 'sharing-options' ) ) { // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- WordPress core doesn't unslash or verify nonces either.
+				$new_comments_state = sanitize_text_field( wp_unslash( $_POST['jetpack_comment_likes_enabled'] ) );
+			} else {
+				$new_comments_state = false;


As on line 659.

sdixon194 · 2022-05-24T18:56:53Z

@anomiex Would you recommend reverting this and the wpcom commit until this is sorted out, or is it just implemented in a poor way that it just doesn't work (as if the nonce checks weren't added before)?

anomiex · 2022-05-24T19:28:37Z

Depends how quickly someone can put together a fix.

sdixon194 · 2022-05-24T19:43:10Z

Tried to put the fixes together here: #24490

Just removed the second check altogether since that seems to be covered.

sdixon194 added 5 commits May 13, 2022 12:35

master-iframe.php

980a70e

first 250 of jetpack-likes-settings.php

76c0e84

500 lines of jetpack-likes-settings.php

6dd933e

jetpack-likes-settings.php

fd527b1

changelog

ed09380

sdixon194 added the [Status] Needs Review This PR is ready for review. label May 13, 2022

matticbot added the Touches WP.com Files label May 13, 2022

github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Likes labels May 13, 2022

sdixon194 added 2 commits May 13, 2022 15:11

remove stat stuff in jetpack-likes-settings.php

3206eea

phpcs exclude list

33f2efd

anomiex reviewed May 13, 2022

View reviewed changes

kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 16, 2022

sdixon194 added 5 commits May 17, 2022 14:00

nonce updates

0c81743

remaining nonces

b931446

undo phpcs-exclude

6a0fb30

Merge remote-tracking branch 'origin/master' into phpcs/likes

b0b6ba8

phpcs-excludefix

0922bc8

sdixon194 added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 17, 2022

Merge branch 'master' into phpcs/likes

06ce71f

kraftbj previously approved these changes May 20, 2022

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 20, 2022

jeherve added this to the jetpack/11.0 milestone May 23, 2022

fix exclude list

0a61a21

Merge remote-tracking branch 'origin/master' into phpcs/likes

9c3b3ba

sdixon194 dismissed kraftbj’s stale review via 9c3b3ba May 23, 2022 18:24

excludelist

e01fee7

samiff self-requested a review May 23, 2022 18:39

samiff previously approved these changes May 23, 2022

View reviewed changes

sdixon194 dismissed samiff’s stale review via e01fee7 May 23, 2022 19:53

Merge remote-tracking branch 'origin/master' into phpcs/likes

f192e09

samiff self-requested a review May 24, 2022 17:15

samiff approved these changes May 24, 2022

View reviewed changes

sdixon194 merged commit 1517280 into master May 24, 2022

sdixon194 deleted the phpcs/likes branch May 24, 2022 17:17

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 24, 2022

anomiex reviewed May 24, 2022

View reviewed changes

sdixon194 mentioned this pull request May 24, 2022

Likes: Fix some nonce issues #24490

Merged

kraftbj mentioned this pull request Jan 18, 2023

Likes / PHPCS: replace file_get_contents by wp_remote_get #13851

Closed

PHPCS: Likes #24368

PHPCS: Likes #24368

Uh oh!

Conversation

sdixon194 commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Uh oh!

github-actions bot commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex May 13, 2022

Choose a reason for hiding this comment

Uh oh!

anomiex May 13, 2022

Choose a reason for hiding this comment

Uh oh!

anomiex May 13, 2022

Choose a reason for hiding this comment

Uh oh!

sdixon194 commented May 17, 2022

Uh oh!

coder-karen commented May 18, 2022

Uh oh!

sdixon194 commented May 23, 2022

Uh oh!

matticbot commented May 23, 2022

Uh oh!

github-actions bot commented May 24, 2022

Uh oh!

sdixon194 commented May 24, 2022

Uh oh!

anomiex May 24, 2022

Choose a reason for hiding this comment

Uh oh!

anomiex May 24, 2022

Choose a reason for hiding this comment

Uh oh!

anomiex May 24, 2022

Choose a reason for hiding this comment

Uh oh!

anomiex May 24, 2022

Choose a reason for hiding this comment

Uh oh!

sdixon194 commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex commented May 24, 2022

Uh oh!

sdixon194 commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sdixon194 commented May 13, 2022 •

edited

Loading

github-actions bot commented May 13, 2022 •

edited

Loading

sdixon194 commented May 24, 2022 •

edited

Loading