woocommerce / woocommerce-gateway-stripe

The official Stripe Payment Gateway for WooCommerce
https://wordpress.org/plugins/woocommerce-gateway-stripe/
234 stars 204 forks source link

Destructive requests passed when environment type not set to production #3358

Closed nicdwilson closed 1 month ago

nicdwilson commented 2 months ago

Describe the bug We have had a report from a developer in 8377567-zd-a8c of Stripe removing payment methods from a live account when a site was set to staging.

We have repeated this on a site set to local and used a cloned site to delete payment methods from a related site.

We checked the cloned site and it is returning local from wp_get_environment_type meaning that the code in this PR is not doing what it sets out to do.

To Reproduce Steps to reproduce the behavior:

  1. Enable the new checkout experience in Stripe plugin settings.
  2. Set the site environment to local or staging
    • Set the WP_ENVIRONMENT_TYPE global variable to local.
  3. Delete a user with a stored payment method
  4. See Stripe account receive the related POST request

Expected behavior Any environment other than production should not work.

Screenshots zVfJVm.png

Environment (please complete the following information):

System Status Report ``` ### WordPress Environment ### WordPress address (URL): [Redacted] Site address (URL): [Redacted] WC Version: 9.1.4 Legacy REST API Package Version: The Legacy REST API plugin is not installed on this site. Action Scheduler Version: ✔ 3.7.4 Log Directory Writable: ✔ WP Version: 6.6.1 WP Multisite: – WP Memory Limit: 256 MB WP Debug Mode: ✔ WP Cron: ✔ Language: en_US External object cache: – ### Server Environment ### Server Info: nginx/1.16.0 PHP Version: 7.4.30 PHP Post Max Size: 1,000 MB PHP Time Limit: 1200 PHP Max Input Vars: 4000 cURL Version: 8.6.0 (SecureTransport) LibreSSL/3.3.6 SUHOSIN Installed: – MySQL Version: 8.0.16 Max Upload Size: 300 MB Default Timezone is UTC: ✔ fsockopen/cURL: ✔ SoapClient: ✔ DOMDocument: ✔ GZip: ✔ Multibyte String: ✔ Remote Post: ✔ Remote Get: ✔ ### Database ### [REDACTED] ### Post Type Counts ### attachment: 23 aw_workflow: 2 page: 16 post: 2 product: 19 product_variation: 7 revision: 6 shop_coupon: 1 shop_order: 9302 shop_order_refund: 3 shop_subscription: 508 wc_membership_plan: 1 wc_user_membership: 755 wcpf_item: 4 wcpf_project: 1 wp_font_face: 33 wp_font_family: 12 wp_navigation: 1 wp_template: 2 ### Security ### Secure connection (HTTPS): ✔ Hide errors from visitors: ❌Error messages should not be shown to visitors. ### Active Plugins (5) ### Meks Quick Plugin Disabler: by Meks – 1.0 Subscription Renewal Tool: by nicw – Beta WooCommerce Stripe Gateway: by WooCommerce – 8.6.1 WooCommerce Subscriptions: by WooCommerce – 6.5.0 WooCommerce: by Automattic – 9.1.4 ### Inactive Plugins (14) ### Action Scheduler Logger: by nicw – 1 Advanced Ajax Layered Navigation for WooCommerce: by Kestrel – 2.0.2 AutomateWoo: by WooCommerce – 6.0.29 (update to version 6.0.31 is available) AutomateWoo Custom Additions: by nicw – Beta Code Snippets: by Code Snippets Pro – 3.6.5.1 Product Filters for WooCommerce: by WooCommerce – 1.4.31 (update to version 1.4.32 is available) WooCommerce Memberships: by SkyVerge – 1.26.7 (update to version 1.26.8 is available) WooCommerce Order Status Logger: by @nicw – 0.1 WooCommerce Subscriptions Gifting: by WooCommerce – 2.7.0 WooPayments: by WooCommerce – 7.9.2 (update to version 8.0.2 is available) WordPress Importer: by wordpressdotorg – 0.8.2 WP Crontrol: by John Blackbourn – 1.17.0 WP Migrate DB Anonymization: by Delicious Brains – 0.3.4 WP Migrate Lite: by WP Engine – 2.6.11 ### Dropin Plugins () ### db.php: Query Monitor Database Class (Drop-in) ### Settings ### API Enabled: – Force SSL: – Currency: EUR (€) Currency Position: left Thousand Separator: Decimal Separator: , Number of Decimals: 2 Taxonomies: Product Types: external (external) grouped (grouped) simple (simple) subscription (subscription) variable (variable) variable subscription (variable-subscription) Taxonomies: Product Visibility: exclude-from-catalog (exclude-from-catalog) exclude-from-search (exclude-from-search) featured (featured) outofstock (outofstock) rated-1 (rated-1) rated-2 (rated-2) rated-3 (rated-3) rated-4 (rated-4) rated-5 (rated-5) Connected to WooCommerce.com: ✔ Enforce Approved Product Download Directories: ✔ HPOS feature enabled: ✔ Order datastore: Automattic\WooCommerce\Internal\DataStores\Orders\OrdersTableDataStore HPOS data sync enabled: ✔ ### Logging ### Enabled: ✔ Handler: Automattic\WooCommerce\Internal\Admin\Logging\LogHandlerFileV2 Retention period: 30 days Level threshold: – Log directory size: 172 KB ### WC Pages ### Shop base: #6 - /shop/ Cart: #7 - /cart/ - Contains the [woocommerce_cart] shortcode Checkout: #8 - /checkout/ - Contains the [woocommerce_checkout] shortcode My account: #9 - /my-account/ Terms and conditions: ❌ Page not set ### Theme ### Name: Storefront Version: 4.6.0 Author URL: https://woocommerce.com/ Child Theme: ❌ – If you are modifying WooCommerce on a parent theme that you did not build personally we recommend using a child theme. See: How to create a child theme WooCommerce Support: ✔ ### Templates ### Overrides: – ### Subscriptions ### WCS_DEBUG: ✔ No Subscriptions Mode: ❌ Staging Subscriptions Live URL: https://order-generator.local Subscriptions-core Library Version: 7.3.0 Subscription Statuses: wc-active: 28 wc-cancelled: 5 wc-expired: 1 wc-on-hold: 417 wc-pending: 55 wc-pending-cancel: 2 WooCommerce Account Connected: ✔ Yes Active Product Key: ❌ No Custom Retry Rules: ✔ No Custom Retry Rule Class: ✔ No Custom Raw Retry Rule: ✔ No Custom Retry Rule: ✔ No Retries Migration Status: ✔ Completed Report Cache Enabled: ✔ Yes Cache Update Failures: ✔ 0 failure ### Store Setup ### Country / State: Netherlands ### Subscriptions by Payment Gateway ### Stripe: wc-active: 28 wc-cancelled: 4 wc-expired: 1 wc-on-hold: 417 wc-pending: 53 wc-pending-cancel: 2 ### Payment Gateway Support ### Direct bank transfer: products Stripe: products refunds tokenization add_payment_method subscriptions subscription_cancellation subscription_suspension subscription_reactivation subscription_amount_changes subscription_date_changes subscription_payment_method_change subscription_payment_method_change_customer subscription_payment_method_change_admin multiple_subscriptions :: products refunds tokenization subscriptions subscription_cancellation subscription_suspension subscription_reactivation subscription_amount_changes subscription_date_changes subscription_payment_method_change subscription_payment_method_change_customer subscription_payment_method_change_admin multiple_subscriptions ### Admin ### Enabled Features: activity-panels analytics product-block-editor coupons core-profiler customize-store customer-effort-score-tracks import-products-task experimental-fashion-sample-products shipping-smart-defaults shipping-setting-tour homescreen marketing mobile-app-banner navigation onboarding onboarding-tasks product-custom-fields remote-inbox-notifications remote-free-extensions payment-gateway-suggestions shipping-label-banner subscriptions store-alerts transient-notices woo-mobile-welcome wc-pay-promotion wc-pay-welcome-page launch-your-store Disabled Features: experimental-blocks minified-js pattern-toolkit-full-composability product-pre-publish-modal printful settings async-product-editor-category-field product-editor-template-system Daily Cron: ✔ Next scheduled: 2024-08-15 01:29:17 +00:00 Options: ✔ Notes: 134 Onboarding: completed ### Action Scheduler ### Canceled: 1 Oldest: 2024-07-25 03:07:18 +0000 Newest: 2024-07-25 03:07:18 +0000 Complete: 953 Oldest: 2024-07-15 22:26:19 +0000 Newest: 2024-08-08 00:33:55 +0000 Failed: 3,741 Oldest: 2023-12-07 07:20:42 +0000 Newest: 2024-07-18 02:32:23 +0000 Pending: 36 Oldest: 2024-08-14 05:23:01 +0000 Newest: 2025-06-17 21:41:35 +0000 ### Status report information ### Generated at: 2024-08-14 05:19:09 +00:00 ```
ryanr14 commented 2 months ago

For what it's worth, for this developer in 8377567-zd-a8c they have one site hosted with SiteGround that doesn't have this issue.

They created a site to test this on, added a customer and saved a payment method. They then clone this site and SiteGround sets the WordPress environment to staging. If they then delete the user from the cloned site, where Stripe is still in live mode, it correctly does NOT detach the payment method.

So far, we're not sure why Stripe correctly handles that staging site. While in all other testing between myself, Nic, and this merchant with other sites, the payment method is detached.

james-allan commented 2 months ago

Thanks all for the details on this issue.

Looking at the code flow, I think this issue impacts the new checkout experience. The PR referenced (https://github.com/woocommerce/woocommerce-gateway-stripe/pull/2672), updated the flow used when a user is deleted on sites with the legacy checkout, the new checkout experience uses a different flow and skipped the checks added in that PR.

@ryanr14 that might explain why you're seeing different behaviour on different stores.

I'll update the issue description to make sure that's clear.

rossodigital commented 2 months ago

@james-allan - I'm the person that originally pointed this glitch out. I'm not a developer so I wasn't sure if this is my place to chime in but felt I might be able to shed some more light on the troubleshooting we've already done.

This issue affected one of my client's websites so I replicated this on my own website (not usually a Woocommerce store but installed WC Core and Stripe gateway for testing). Both sites used the new checkout experience and the issue occurred. I then created a brand-new website on a temporary domain, installed WC Core and Stripe gateway and added a test user with a credit card added from the front-end to the saved payments. I then created a staging clone from that newly created website. Same steps to reproduce I took on my own website for testing purposes. On that staging site cloned from a new website, when deleting a user from the admin backend with Stripe set into Live mode but the WP environment set to staging the payment method was not detached in Stripe. That website where the issue did not occur was also using the new checkout experience. I also tested before and after following the new 're-authenticate' flow and it didn't make a difference.

I've been liaising with @ryanr14 for quite a bit and I cannot figure out why a new site is behaving differently to two 'older' sites when the steps to reproduce are the same and the checkout experience is the same on all sites.

james-allan commented 2 months ago

No problem chiming in @rossodigital.

On that staging site cloned from a new website, when deleting a user from the admin backend with Stripe set into Live mode but the WP environment set to staging the payment method was not detached in Stripe. That website where the issue did not occur was also using the new checkout experience.

Hmm interesting. I'm not entirely sure what's causing the difference between the two sites in that case. When I looked into this earlier I was just following the code.

When a WP user is deleted, WooCommerce calls the wc_delete_user_data() function which deletes all the user's payment tokens (code ref).

When a payment token is deleted the WC Stripe plugin hooks in and in the woocommerce_payment_token_deleted() function (code ref), it detaches the payment methods in Stripe -- this is where the flow slits.

If WC_Stripe_Feature_Flags::is_upe_checkout_enabled() passes (new checkout exp) we call $stripe_customer->detach_payment_method(), on stores with the legacy checkout we call $stripe_customer->delete_source() instead.

delete_source() (legacy) eventually calls WC_Stripe_API::detach_payment_method_from_customer() which calls should_detach_payment_method_from_customer(). That's the function that checks the wp environment before detaching the source in Stripe.

detach_payment_method() (new) does not lead to should_detach_payment_method_from_customer(), it just detaches the source directly.

So perhaps there's another reason why it's not being detached on your fresh site, but from following the code, I can see how on the new checkout experience the payment methods are being detached.

rossodigital commented 2 months ago

Could be that there are multiple issues at play? I didn't want to make the troubleshooting even more complex by throwing in that on a fresh site with staging copy the issue doesn't present. I just felt that if we can figure out what the difference is between older sites and the fresh site (despite using the same checkout method) it could help us get to the bottom of this.

james-allan commented 2 months ago

Yeah no problem, as you mentioned there might be another thing at play. There are a couple of avenues that could be causing it to not detach on your other site. The tokens might not be returned from WC_Payment_Tokens::get_customer_tokens( $user_id ) for some reason or the token's payment method ID isn't one of the ones in UPE_REUSABLE_GATEWAYS_BY_PAYMENT_METHOD. I'm not sure.

rossodigital commented 2 months ago

Could the issue be related to how sites are being connected to Stripe? I might remember this incorrectly but didn't we use to have to copy the publishable and secret keys manually out of Stripe and enter them in the WC Stripe Gateway settings?

I just had a look and when using the new oAuth way of connecting to Stripe, the API keys in the DB don't actually match any API keys we have set up in the Stripe account so I'm assuming those API keys are set up during the connection process but don't show in the Stripe Dashboard? The connection still works fine as we've tested this before.

When I check the original client site that had the issue, the wp_options entry doesn't show "connection_type" "connect" and on that site the API keys I can see in the DB actually match the ones I can see in Stripe.

csgaraglino commented 1 month ago

When I sent this problem to Strip/Woo, I was asked to give a process that they could try to duplicate. This was, in part, my response.

Hopefully, something in here might help.


Ok, so we're going to start prior to Woo 9.0. We have a Stripe account that is strictly a test account. It has all fake information in it, and we use that account for testing client code, and the client tests the new custom code to make sure everything is working as expected. It's never been out of test mode and we've been doing it for over a decade.

Up until now, all we did was clone the template website, which is just a skeleton website using Divi and Woo, and bring it up. We'd add a few products there and make sure that everything was working properly, and then we would get onto doing our custom design work.

Again, prior to the 9.0 upgrade, once we created a clone, we opened it up, and it was already connected to the Stripe test site, and we went right to work. 

Here's what's happening to us:

  1. We clone a prebuilt design template that includes all our settings, including Woo. This template was a fully functioning site with Woo connected to Stripe and in Test Mode.
  2. I ran the initial test order, to make sure that everything was working. And that included updates to plug-ins PHP DiVi, WP, Woo, and so on. However, the order failed! The error said that there was something wrong with the payment type, which was the test codes from Stripe. So that let me know that it was actually something within stripe itself. 
  3. However, I opened up the payment settings, all looked normal.
  4. Then in the Stripe plug-in settings, I got a message stating that I was disconnected from Stripe. When I opened up the configuration for stripe, this is what I saw.  So, at some point, your system disconnected the design template from Stripe. Normally, when this happens, I'll have to go into the configuration and click the reconnect button, and it reconnects, and everything's good. Not so much anymore.
  5. OK, mind you, at this point, the connection and stripe were in test mode. That's important for the next step to remember.
  6. I went ahead and clicked configure, and then this screen showed up. Now it says clearly here, Create or connect an account, or create and connect a test account instead. Obviously, since it was already a test account set up, ready to go with all the information working for the last 5 to 10 years, I clicked create or connect a test account instead.
  7. So that brings us to the screen that wants me to verify my personal data. This is a test stripe account. There is no personal data. There's nothing for it to connect to for verification. On top of that, if I fill out my personal information, my customer would have access to all of MY information. There's absolutely no way in hell I'm going give them access to my Social Security number and my banking account information and all that stuff. This is a test account. I don't need live data in here. It's been working for the last 5 to 10 years, and there's absolutely no reason why it can't work now, well, because you guys say it can't! 
  8. Now, as you can see, we have a very serious problem! Because we are now stuck and can't get past the screen. This means we cannot use test accounts with Stripe anymore!9.

Side Note: I am not exactly sure how, but at some point yesterday, digging around inside here I was able to get past this screen and even got the test account information set up screen. But as soon as I got to the connected bank account information screen, I got a little more confused, I can't load a real bank into a Public Test account. After a little research online, I was able to find some test bank account numbers that developers use in order to do testing on things like this. I actually got them entered and accepted as valid numbers, but when I submitted them, I got a red message underneath the routing number saying that I can't use test bank account information on a live site! At that point, everything got shut down!

NOW, BEFORE YOU SAY THAT THIS IS A UNIQUE SITUATION WHEN IT KIND OF IS, LET'S LOOK AT THIS CHAIN OF EVENTS IN A REAL WORLD LIVE SITUATION!

As I mentioned, we manage many different clients. If one of those live clients needs to make a Stripe change in the way that it's connected, maybe to a different account, or update the API keys or the webhook, for whatever reason, someone would have to go through this whole process which is going to disconnect to the existing live stripe account, and force that someone to follow all of these steps again. If that someone's a designer/developer/accountant/webmaster, web manager, [fill in the blanks], ANYONE other than the owner of the account, they're not going have any of this private/personal banking/owner information at all in order to fill out all of the verification that you're asking for. And a client will not and should not ever give out their Social Security number and all of their banking information to a strange developer in some strange area!

I cloned a live working site that is processing credit cards every day and proved my theory. I went to the Stripe configuration to set the site into Test Mode and got this message!!!

I tried Both Methods and BOTH, yes, BOTH disconnected the site and led me to this screen.

This site is now offline and can no longer process credit cards - a serious fail!

This could take a live business offline for hours, days, or even weeks!