Skip to content

Comments: Do not use HTTP 500 for non-server errors.#12428

Merged
jeherve merged 2 commits intomasterfrom
update/comments-should-not-die-with-500
May 21, 2019
Merged

Comments: Do not use HTTP 500 for non-server errors.#12428
jeherve merged 2 commits intomasterfrom
update/comments-should-not-die-with-500

Conversation

@mdawaffe
Copy link
Copy Markdown
Member

@mdawaffe mdawaffe commented May 20, 2019

Changes proposed in this Pull Request:

Jetpack should not be responding with HTTP 5xx errors when a client makes a bad or unauthorized requests. HTTP 4xx should be used for such requests.

Testing instructions:

It's annoying. Do the following for both master and this branch (update/comments-should-not-die-with-500).

  1. Edit modules/comments/comments.php so that the signature check always fails. Something like the following will work:
diff --git a/modules/comments/comments.php b/modules/comments/comments.php
index e352ab7f8..cfbc9a92b 100644
--- a/modules/comments/comments.php
+++ b/modules/comments/comments.php
@@ -471,4 +471,6 @@ class Jetpack_Comments extends Highlander_Comments_Base {
 		}
 
+$check = '';
+
 		// Bail if token is expired or not valid
 		if ( $check !== $post_array['sig'] ) {
  1. Ensure Jetpack Comments is enabled.
  2. Compose a comment on a post. Do not yet submit it.
  3. In your browser console, look at the network requests log.
  4. Submit the comment.

In master, you'll see the wp-comments-post.php request has a 500 response. In this branch, you'll see a 400.

Remember to undo the hack from step 1 :)

Proposed changelog entry for your changes:

  • Use HTTP 4xx status codes for comment errors.

mdawaffe added 2 commits May 20, 2019 15:38
@mdawaffe
Comments: Do not use HTTP 500 for non-server errors.
03b9fd2 
Jetpack should not be responding with HTTP 5xx errors when a client makes
a bad or unauthorized requests. HTTP 4xx should be used for such requests.
@mdawaffe
Comments: Move HTTP status definition up the stack.
657a57e
@mdawaffe mdawaffe requested a review from a team May 20, 2019 23:04
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented May 20, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello mdawaffe! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D28464-code before merging this PR. Thank you!

@mdawaffe mdawaffe self-assigned this May 20, 2019
@mdawaffe mdawaffe added [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended [Feature] Comments labels May 20, 2019
@mdawaffe mdawaffe added this to the 7.4 milestone May 20, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented May 20, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 657a57e

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented May 21, 2019

Are there any consumers of this error that is expecting a 500 within Jetpack that you're aware of? I can't think of any, but that would be the only reason not to do it (yet).

@mdawaffe
Copy link
Copy Markdown
Member Author

mdawaffe commented May 21, 2019

I know of none. I do know the Automattic Systems team would like Jetpack (and WordPress in general) to not respond with 500s unless something is actually broken with the server (which is rarely the case by the time PHP is checking this sort of thing).

I'm not sure about these 500s specifically, but Jetpack's in general cause false positives for them.

@mdawaffe
Copy link
Copy Markdown
Member Author

mdawaffe commented May 21, 2019

All of (most of) core's equivalent comment errors return 4xx: https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/comment.php#L3114

(Any WP_Error there without a message/status just results in an exit: https://core.trac.wordpress.org/browser/tags/5.2/src/wp-comments-post.php#L25)

@kraftbj 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 21, 2019
@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented May 21, 2019

The status arg on the error data probably should be more documented in Core :), but it all works and tracks.

jeherve
jeherve approved these changes May 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Merging.

@jeherve jeherve merged commit d72f0d7 into master May 21, 2019
@jeherve jeherve deleted the update/comments-should-not-die-with-500 branch May 21, 2019 17:59
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 21, 2019
@jeherve jeherve mentioned this pull request May 22, 2019
Merged
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
Changelog: add #12428
9fcf1ad
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
@westonruter
Copy link
Copy Markdown
Contributor

westonruter commented Jun 4, 2019

Related Trac ticket for wp-comment-post.php in core to send back 400 instead of 200 for invalid comment submissions: https://core.trac.wordpress.org/ticket/47393

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Apr 29, 2020

r206694-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kraftbj kraftbj kraftbj approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@mdawaffe mdawaffe

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Comments Touches WP.com Files

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

6 participants

@mdawaffe @matticbot @jetpackbot @kraftbj @westonruter @jeherve