Handle 402 errors so it won't be hijacked by the hosting provider error page #8102
+14
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.26 MB ℹ️ View Unchanged
|
frosso
approved these changes
Feb 2, 2024
includes/class-wc-payments-utils.php
Outdated
Comment on lines
594
to
603
| $status_code = 400; | ||
| if ( $e instanceof API_Exception ) { | ||
| return $e->get_http_code() ?? 400; | ||
| $status_code = $e->get_http_code() ?? 400; | ||
| // Sometimes hosting companies hijack this status code and return their own predefined error page. | ||
| // In this case, we want to return a 400 instead, since it will break the flow of the checkout page. | ||
| if ( 402 === $status_code ) { | ||
| $status_code = 400; | ||
| } | ||
| } | ||
| return 400; | ||
| return $status_code; |
There was a problem hiding this comment.
What are your thoughts on the following alternative, to decrease the indentation due to the cyclomatic complexity?
$status_code = null;
if ( $e instanceof API_Exception ) {
$status_code = $e->get_http_code();
}
// Sometimes hosting companies hijack the 402 status code to return their own predefined error page.
// In this case, we want to return a 400 instead, since it will break the flow of the checkout page.
if ( 402 === $status_code ) {
$status_code = 400;
}
return $status_code ?? 400;
There was a problem hiding this comment.
@frosso's suggestion is easier to read 👍
There was a problem hiding this comment.
Addressed here: ffb36ec . Thank you for the feedback.
includes/class-wc-payments-utils.php
Outdated
Comment on lines
596
to
601
| $status_code = $e->get_http_code() ?? 400; | ||
| // Sometimes hosting companies hijack this status code and return their own predefined error page. | ||
| // In this case, we want to return a 400 instead, since it will break the flow of the checkout page. | ||
| if ( 402 === $status_code ) { | ||
| $status_code = 400; | ||
| } |
There was a problem hiding this comment.
@zmaglica do you plan to add a test for this change? As I checked the test file does exist.
I am not planning to write tests in this PR for this file, since the tests for this particular file don't exist, as you mentioned too. I will keep this PR as clean as possible and open a new issue to address the issue with the tests.
includes/class-wc-payments-utils.php
Outdated
Comment on lines
594
to
603
| $status_code = 400; | ||
| if ( $e instanceof API_Exception ) { | ||
| return $e->get_http_code() ?? 400; | ||
| $status_code = $e->get_http_code() ?? 400; | ||
| // Sometimes hosting companies hijack this status code and return their own predefined error page. | ||
| // In this case, we want to return a 400 instead, since it will break the flow of the checkout page. | ||
| if ( 402 === $status_code ) { | ||
| $status_code = 400; | ||
| } | ||
| } | ||
| return 400; | ||
| return $status_code; |
There was a problem hiding this comment.
@frosso's suggestion is easier to read 👍
|
The discussion is underway if we should go this route or not. @jrodger would it okay that we wait for your +1 before merging this? |
We can go ahead and get this merged. I like that the code is well commented with the reasoning for the change, so if we did decide to remove it in the future it would be easy to understand what was being taken out. One question, I think I saw that @zmaglica investigated this elsewhere already, but we're definitely not relying on returning the 402 code anywhere in the JavaScript code right? |
Merged
6 tasks
402 can occur on adding the payment method and on the checkout page. I think we still need to address the 402 error on the checkout page. Let me take a look a bit more, and I will comment here results. |
3 tasks
dmvrtx
approved these changes
Aug 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8080, #8079, #6546
Changes proposed in this Pull Request
Stripe typically returns a 402 status code when a payment method is invalid. However, this standard response can be intercepted by some hosting companies, leading to a significant issue. These companies replace the expected 402 error with their custom error pages, formatted differently from what the client anticipates.
This PR resolves this problem by returning 400 status code instead of 402, on the AJAX calls where the status code is known that could be hijacked.
Testing instructions
Test 1:
Test 2:
You can skip steps from 1-4 if you have already configured JN site with WooPayments.
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge