Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New UPE - Initial minimum viable product #5770

Closed
bborman22 opened this issue Mar 14, 2023 · 6 comments · Fixed by #5869
Closed

New UPE - Initial minimum viable product #5770

bborman22 opened this issue Mar 14, 2023 · 6 comments · Fixed by #5869
Assignees
Labels
category: projects For any issues which are part of any project, including bugs, enhancements, etc. component: upe priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. size: large The issue is sized large. type: enhancement The issue is a request for an enhancement.

Comments

@bborman22
Copy link
Contributor

Description

Please refer to pc2DNy-2e2-p2 for more details.

[Stripe has informed us that they have] added two important payment flow features with our desires in mind: the payment intent is no longer required prior to creating the payment method and the payment element can be used singularly to create the payment method, allowing us to create and confirm the payment intent ourselves on the Payments Server. This feature would solve our security concerns over exposed payment intent information on the client and would allow the UPE to be compatible with WooPay, since we can use our legacy payment intent confirmation flow with the UPE.

Unfortunately, this feature is not yet ready. Stripe plans to open up a private beta for this feature by the end of October and plans to fully release the feature by the end of Q1 next year.

Please refer to this file for the current description of this new payment flow.

If you scroll down to the section titled "Server-side confirmation", you will see that this almost exactly matches our current payment flow for our legacy card element, where the payment intent is created and confirmed simultaneously and subsequent to creating the payment method. This will allow us to resolve almost all of our current security and integration concerns with our existing UPE implementation.

This issue exists to create the initial PoC implementation of this new payment flow, once the tool has been released to beta and we are given access to it by Stripe. This issue is simply to implement the basic shortcode checkout payment flow and there will be other issues created to support other checkout and payment flows. It's also assumed that this tool will be in a fluid state while in limited beta release, so this PoC will not need to be considered in a final state once completed.

Acceptance criteria

  • Implements Enhanced UPE shortcode checkout flow.

Additional context

Stripe UPE – Options to solve WooPay compatibility and WC Pay loss of control over the payments UI/UX
New integration path for Payment Element

@bborman22 bborman22 added category: projects For any issues which are part of any project, including bugs, enhancements, etc. component: upe type: enhancement The issue is a request for an enhancement. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. size: large The issue is sized large. labels Mar 14, 2023
@timur27 timur27 self-assigned this Mar 20, 2023
@FangedParakeet FangedParakeet changed the title New UPE - Initial proof of concept New UPE - Initial minimum viable product Apr 14, 2023
@FangedParakeet
Copy link
Contributor

Just a heads up, but I've renamed this issue from a POC to an MVP, because the scope of our efforts is much greater than a puny prototype and we are consequently expending as much energy to possible to create an end-product that may be limited in its initial ability, but that is still robust enough to be released and reused.

@timur27
Copy link
Contributor

timur27 commented Apr 18, 2023

@FangedParakeet There's one thing regarding the 3DS authentication for credit cards we'd need to find the best approach to handle.

For the legacy card (non-UPE), the flow looks the following:

Screen.Recording.2023-04-18.at.17.43.41.mov

A pop-up authentication window allows users to authenticate or fail the transaction. On the 7th second, there is a very quick URL update which then disappears, but it essentially looks following:
image

As you can see, payment intent and client secret are attached to the URL. The payment intent and secret are parsed and used by this method. Surprisingly, the intent is then confirmed by the front-end implementation here.

The UPE with deferred intent creation does not inherit this piece of implementation. I have concerns about just copying this approach to the deferred intent creation, as we want to avoid exposing payment intent on the client side as part of the project.

Instead, for the deferred intent creation, we currently provide the return_url for the card gateway the same way we do for all the non-card payment methods (acf9e3b and 1cafab7), which allows us to avoid processing intents/client secrets on the client side, but the flow looks a bit different since Stripe acts differently when the return_url is part of the request.

Screen.Recording.2023-04-18.at.18.02.48.mov

On the 8th second, you might notice that the URL has some client secret. I'm not sure if we can do anything to avoid this. A big advantage of this approach is we don't handle intents/secrets in the client scripts, and it also works almost out-of-the-box. I'd like us to refine the advantages and drawbacks of both solutions and choose the path forward. I'm leaning towards the second option but unsure if we shouldn't confirm with a broader audience.

@FangedParakeet
Copy link
Contributor

There's one thing regarding the 3DS authentication for credit cards we'd need to find the best approach to handle.

This is a great callout. This is part of the 3DS flow, where if the status on the intent is requires_action after we attempt to confirm it, we attach the intent ID and client secret to the current page URL, which is consequently captured on the front-end, and eventually we parse these details and use them to confirm the intent on the client.

AFAIA, this is now the only remaining flow where the intent is still confirmed on the client. Client-side confirmation here is required, because we need to rely on Stripe JS to present the authentication modal before we can complete the transaction (I believe that's what this call to confirmCardPayment is achieving): this is similar to the original UPE flow where we require Stripe to take action and lose a little control in the process.

I'm not sure that there is an obvious workaround for this scenario, without creating a rather large refactor of this payment flow. It's worth noting that when public key encryption is enabled, this attack is restricted since the client secret is concealed. I would suggest that this could eventually be enforced, though our own settings state that "it's not necessarily a good idea to have this enabled at all times," although, I am unaware why exactly that might not be such a good thing to do.

Personally, I don't believe this resolution should be a part of this MVP issue, since it already affects the legacy card element. It's a reasonably slim edge case, since a MITM attacker would only be able to take advantage of this on transactions with cards that already require additional authentication. Moreover there is a present defence with our encryption implementation. It's probably worth discovering why this is not a permanent solution and, if truly not, what other recourse we may have at our disposal, but I think this has a broader scope than our shortcode checkout...but let me know if you feel otherwise!

@timur27
Copy link
Contributor

timur27 commented Apr 18, 2023

@FangedParakeet Indeed, the 3DS authentication and ways to handle it are the broader scope. However, I'm curious if we see the potential in the second solution (which is now implemented in #5869 and can be verified by testing the PR). The second recording here demonstrates how it works. This solution allows us to confirm the intent as we do for the UPE methods, automatically eliminating the need to process the client's payment intent/secret data. I like its simplicity, but as mentioned before, I'm still unsure how hard we want to stick to the pop-up authentication window instead of the separate page for the 3DS card authentication.

I'd appreciate hearing your opinion on this. I'm happy to take care of this if we need to consult anyone else. I think that implementing it as the second recording demonstrates here can be included to the current scope of the MVC, whereas building handlers for the payment intent confirmation on the client could be considered as a candidate for a separate ticket.

@FangedParakeet
Copy link
Contributor

However, I'm curious if we see the potential in the second solution (which is now implemented in #5869 and can be verified by testing the PR). The second recording #5770 (comment) demonstrates how it works.

That's quite interesting: I didn't realise the UPE offered us another solution there. Honestly, I don't really know enough about the development of our existing 3DS implementation or any pitfalls of alternate approaches. I think it would be beneficial to seek some feedback to this approach in the #wcpay slack channel or perhaps take it to P2, if you feel your solution requires a more in-depth explanation.

On the 8th second, you might notice that the URL has some client secret. I'm not sure if we can do anything to avoid this.

I'm not certain that we need to do anything here, since at this point the payment is complete and the intent is essentially used and disposable...but again, I'd defer to the wisdom of others before stating this with much confidence. 😅

@timur27
Copy link
Contributor

timur27 commented Apr 19, 2023

I confirmed that the pop-up modal is a preferred option and the redirect should be avoided (more details in p1681887162644549-slack-CGGCLBN58). I will push 3DS authentication handling to the PR by tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: projects For any issues which are part of any project, including bugs, enhancements, etc. component: upe priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. size: large The issue is sized large. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants