woocommerce / woocommerce-gateway-stripe

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

Feature Request: Filter hook for subscription admin form validation #1348

Open lonkelle opened 3 years ago

lonkelle commented 3 years ago

Specifically a filter to add new Stripe prefixes to the input admin page validation check here: https://github.com/woocommerce/woocommerce-gateway-stripe/blob/27381ccf22b35afe3a81eb9c111a71b5fcbe3a6d/includes/compat/class-wc-stripe-subs-compat.php#L501

It's hardcoded to src_ and card_ right now, and the function is run in a way that I haven't been able to figure out how to override it by remove_action so I was wondering if you would mind adding a filter to that conditional to bypass it? You've provided hooks to give my plugin the ability to support other Stripe prefixes (I'm using ba_ for my project) in literally every other way except for the Subscription Admin Page validation check being hardcoded on that line there.

lonkelle commented 3 years ago

I've bypassed this restriction by using the card_ prefix (lucky for me you guys deprecated that so it's free for me to use since none of my old subscriptions need it) for all subscriptions whose payment sources actually start with ba_ and search and replace every call (using your "bodyrequest" hook) to replace `cardwithba_`. It works both for manual subscription payments and subscription "off-session" payments in testing.

lonkelle commented 3 years ago

@kalessil This is not a question. I've already bypassed the restriction, but only on my install because none of my customer's have a card_ payment source (they all have src_ and ba_). You've hardcoded the Subscription Admin Screen's Stripe Source ID input field and validated it server-side via:

https://github.com/woocommerce/woocommerce-gateway-stripe/blob/27381ccf22b35afe3a81eb9c111a71b5fcbe3a6d/includes/compat/class-wc-stripe-subs-compat.php#L503 https://github.com/woocommerce/woocommerce-gateway-stripe/blob/27381ccf22b35afe3a81eb9c111a71b5fcbe3a6d/includes/compat/class-wc-stripe-subs-compat.php#L504 https://github.com/woocommerce/woocommerce-gateway-stripe/blob/27381ccf22b35afe3a81eb9c111a71b5fcbe3a6d/includes/compat/class-wc-stripe-subs-compat.php#L505

These are the only 3 allowed, but I have customers who have ba_ Stripe source prefixes. I've worked around this using card_ and re-writing it to ba_ on your Stripe API call, but to have a clean solution, you should add a apply_filters to give user's the ability add more source prefixes since there are more than the three that you've hardcoded.

PS. Here's a screenshot of this exact input field in the admin:

Screen Shot 2020-10-30 at 7 16 59 PM
lonkelle commented 3 years ago

Does my filter request make sense now?

lonkelle commented 3 years ago

Wait...looking more thoroughly at your code, I could bypass it simply by adding one of your "allowed" source prefixes anywhere (it doesn't check if it's the actual prefix 😂) in the Stripe Source ID string (which would hilariously allow it to bypass your admin validation check, making your validation conditional...well, incomplete) and then just cut it out of the source when your plugin sends it to Stripe's API. But that's still very hacky and an apply_filters on this conditional sounds like the most pragmatic solution.

lonkelle commented 3 years ago

@luizreis This was originally a Feature Request for an apply_filters to be added into the conditional. But you should know that this is actually a bug. If you read this comment: https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1348#issuecomment-719843603

You'll see that these three lines are inherently exploitable conditionals: https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1348#issuecomment-719841500

Those three lines are supposed to check and validate the prefixes of sources in Stripe for the validation of the form (those 3 prefixes being the only ones you currently support). They're actually checking the entire string which is a validation risk for your users, so this is a [Type] Enhancement, that's correct, but during my comments I found it's also a Type [Bug] as well due to those lines not checking the prefixes and checking the whole string so XXcfdsdAAffwwdcard_Acds would pass your validation check even though card_ is not the actual prefix.

lonkelle commented 3 years ago

Of note, I got around this restriction by changing some $POST values at the right time (I just add `cardanywhere in the string since the validation function doesn't check for the **actual** prefix - thus making this a bug, as I outlined), tricking the validation function, and then switching it back. Still a hack, but the least hack-y way I could do this without a filter to add theba_` source.