Add Stripe Checkout (iFrame) support

wow, no kidding. looks like i've got a new project for the evening. thanks for the report.

on a closer look, i think this is a false alarm.

the stripe support post linked by OP describes changes to stripe.js that were made in response to the new version of PCI DSS. the relevant part is this:

We’re in the process of ramping up the new version to all users, but you can also opt in today by including https://js.stripe.com/v2/stripe-dss3.js instead of Stripe.js on your payments page

the new file (stripe-dss3.js) was rolled out to allow people to opt in early to PCI DSS 3.0. At this point, they have rolled out the changes to everyone by merging the changes in to the main stripe.js file. i have verified this by downloading both https://js.stripe.com/v2/stripe-dss3.js and https://js.stripe.com/v2/stripe.js and comparing them in a diff tool. they are identical. in addition, commerce stripe has always implemented the inclusion of this file by simply including https://js.stripe.com/v2/ (without specifying a file name), which allows Stripe to control exactly which file is served by default. i have verified that the content served at the path https://js.stripe.com/v2/ is also identical to what's served from https://js.stripe.com/v2/stripe.js and https://js.stripe.com/v2/stripe-dss3.js.

so as far as i can tell, the only reason you would have a version of stripe.js that is out of PCI complaince is if you were somehow caching the file locally, as opposed to allowing it to be downloaded from js.stripe.com as this module is designed to do.

I don't think we should close this out. Currently the module is still using the direct post api regardless of the javascript it's including. It would be great to add support for Stripe's iframe solution which would enable store owners to use PCI SAQ A. Currently they have to use PCI SAQ A-EP because of updates to PCI in Jan 2015.

I was told that 7.x-1.x is closed for new features, so setting this as a feature request for 7.x-2,x

See #2449881: Add support for Drop-in UI for PCI SAQ A Compliance for information on how I hope to handle this for commerce_braintree.

@rickmanelius has created http://drupalpcicompliance.org/ which can help answer questions around PCI

My understanding is that andyg5000 is correct - the direct post method will now place a user under SAQ-EP, not SAQ-A. I have been using Authorize.net's DPM (direct post method) for a couple of years with no problems. However, my bank has advised me that if I continue with DPM, I will fall under the scope for SAQ-EP and must have quarterly penetration scans, 139 check points, a higher level of hosting server, etc. The bank is implementing a program that brings the cost down to $5000 per year, but that is still too much for me.

The PCI DSS committee specifically lists iframe implementations as qualifying for SAQ-A. My hope is that Drupal will have an iframes solution for both Stripe and Braintree....

i agree this is something the module really needs. i've updated the module description with some notes about this issue.

one thing i am trying to figure out is whether it would be possible for Card On File to be compliant with SAQ A. anyone have any insight on that? the way i'm reading this seems to suggest that someone using SAQ A would be precluded from using Card On File, since that relies heavily on the token implementation of Stripe. It seems like the only way it could work under SAQ A would be if everything including the selection of stored cards was handled within the iframe.

I believe that card on file would leave you in the same compliance as iFrame solution. Credit card data never touches the server, you're delivering the iframe via javascript and html that's hosted on the payment providers network and you're generating charges by a token that can only be used on your merchant account. If someone hacked the token, they could only make charges to your merchant account.

ok now i'm more confused. if doing everything via tokens "leave[s] you in the same compliance as iFrame solution", then what is the problem in the first place with how we are handling payments? im not asking about the general idea of saving cards, im asking specifically about how this module implements integration with the Card On File module (using tokens and not iframes). Would that implementation be ok with SAQ-A? if so, then i dont see any reason why the rest of the (tokenized, non-iframe) payment process would not also be ok with SAQ-A.

The way I understand it is the current commerce_stripe implementation is PCI SAQ-EP. No credit card data hits Drupal, but Drupal is still serving up the form and all javascript to submit the form. An iFrame is SAP-A because all of the javascript and code is loaded directly from the gateway servers. See https://github.com/rickmanelius/drupalpcicompliance/blob/master/DrupalPC... for more info.

Section "Version 3.0 Disclaimer"

Ok, so what this suggests to me (someone please correct me if i'm wrong) is that our implementation of Card On File cannot qualify for SAQ-A, because it includes the ability to add credit cards using a direct post method. In order to qualify for SAQ-A, we would have to ensure all Card On File related features are either disabled or ported to using iframes instead of direct post.

Yea. Anywhere there is a form generated by drupal_get_form will have to be replaced with the iFrame version. Existing tokens and tokens returned from the new form are fine.

I really apologize for not providing a review at this time (between having a sick baby and some work deadlines, I've been slammed). That said, if this goes through and does offer card on file support, it'll be added to the PCI white paper as a recommend SAQ A gateway. Tracking here => https://github.com/rickmanelius/drupalpcicompliance/issues/39

I've been a bit buried in work the last 2 months, but here's my review.

First, to get this running you need to address a few known issues:

• Need to move to a library prior to 2.x #2428475: Stripe PHP library v2 unsupported. Use v1.18.0 here https://github.com/stripe/stripe-php/releases/tag/v1.18.0.
• Need to use the dev branch (7.x-1.0-rc7+22-dev).
• Need to update Stripe API to most recent version (2015-04-07).
• Card on File support is currently broken #2476029: Charging cards fails if using Stripe API > 2015-02-18.

Now onto the bad news.

The issue summary is correct in that the JS libraries by Stripe are already in parity. Namely https://js.stripe.com/v2/ is identical to https://js.stripe.com/v2/stripe-dss3.js as specified https://support.stripe.com/questions/what-about-pci-dss-3-0. When the checkout page loads with the credit card form, you can see an iframe has been injected into the DOM right before the the body tag closes.

Unfortunately, this is not a proper implementation of an iframe solution. One of the protections given by an iframe solution is to prevent keyloggers. However you can easily attach a jquery event to watch for text input to the credit card field and do whatever to it.

In fact, if you compare to the Commerce Hosted PCI module, you'll see that everything within id=payment-details in the normal commerce form has been replaced with an iframe.

Unless I'm testing this incorrectly, the iframe is simply not replacing/wrapping the credit card portion of the checkout form. And unfortunately I don't have enough experience using Stripe to know if it's a simple matter of connecting the external JS library in a different way.

The good news is that, as implemented, this appears to be using the direct post correctly and therefore it still falls within the scope of SAQ A-EP. However, that is still very cost prohibitive for most small merchants to achieve, so I would highly recommend getting the iframe implemented correctly. It would be such a godsend to the Drupal Commerce community because there are only a few other iframe solutions.

@rickmanelius -- thanks for the review. your findings sound correct to me. we have not even attempted at this point to implement the iframe solution. if there are any hints in the code of something that is supposed to support an iframe, they are either incidental or left over from prior developers in an incomplete state.

at present, i am the only active maintainer, and my time commitment at this point is limited to fixing minor or extremely urgent bugs, and reviewing submitted patches. i am very much in support of getting the iframe solution working, but its unlikely i will be able to contribute much to this effort. If there is any way you help us drum up some additional developer resources, it would be greatly appreciated.

Hi @aviindub. And thank you for the honest response. I'm also in a position where I don't have enough time to contribute to this at this time, but at least there is a trail of where to pick this up should someone else have the capacity/experience to take this on.

One suggestion. On the project page there is mention of implementing the iframe as part of the 2.x branch (see https://www.drupal.org/project/commerce_stripe). It would probably be more helpful to state a request for help/assistance there so that others don't assume that it's already in progress. I had made that assumption as well, which is why I reached out.

So others don't get their hopes up, I'm going to mark this as postponed until we can get someone to step up for this. Again, not pressuring you because this could potentially be a lot of work.

Just as a hail mary, tried reaching out to @stripe on twitter to see if they'd be willing to fund/sponsor work on this. Unlikely to get a response, but I figured why not try?

Hi Ryan. That would be amazing! Especially if your colleagues already have experience and know the ins and outs of stripe.

I've received a few other inquiries regarding how to help move this along. If you ultimately get blocked, let me know. In the meantime I'm going to keep reaching out as well.

Any progress on this? I could support you on this if I can.

Speaking of the iframe solution do you mean Stripe Checkout (https://stripe.com/docs/checkout)?

@Lukas von Blarer -- any help you can offer would be greatly appreciated.

yes, Stripe Checkout is their iframe solution. see https://support.stripe.com/questions/what-about-pci-dss-3-0

If you’re using Checkout, we’ve got you covered with Stripe.js, so Checkout users will be largely unaffected by the upgrade. Since it already rests inside an iframe controlled by Stripe, you can continue to fill out SAQ A. Similarly, the CSP situation has not changed.

from the last few comments, it looks like @rickmanelius and @Ryan Weal have put out calls asking for help developing this, but it does not look like anyone has yet agreed to help.

Hi @aviindub. Still looking at funding options. I know a few developers that would be interested, but given the amount of effort that could be involved (unless the patch by Ryan Weal proves to be simple/straightforward) it looks like would need something to help compensate.

thanks rick

I'd love to help as well. I have other priorities but this seems pretty urgent.

@Ryan Weal: Would love to know if you had any luck generating or getting a patch from your colleagues. I'd be happy to test it out if so.

Hey guys, attached is our attempt to add iframe support to commerce_stripe. I believe this is what Ryan mentioned earlier.
The patch will enable stripe iframe Checkout as an option, besides the existing stripe.js. Once "checkout" option is chosen, payment pane will be rendered without credit card fields. Users will be able to provide their credit card details in checkout iframe, which will be opened upon clicking on "continue" button.

To test the patch:

1. Apply it!
2. Go to "Stripe" reaction rule admin/commerce/config/payment-methods/manage/commerce_payment_commerce_stripe
3. Edit action "Enable payment method: Stripe"
4. In "Payment settings" there should be a new select element called Integration type. Select "checkout" and save.
5. Add a test product/product display. Add it to the cart then proceed to checkout.
6. Confirm that payment pane does NOT show any credit card fields, and that checkout iframe opens once you click on "continue" button
7. Enjoy!

Needless to say feedback is more than welcome!

Forgot to mention that patch applies on 7.x-1.x branch.

NICE! I can confirm the patch in #30 does indeed work and it resolves the issues I mentioned in #18.

It's worth noting that this review was more on functionality versus security, but that type of analysis is ongoing and requires a deeper dive to review the module as a whole.

Before setting to RBTC, it would be helpful to get another person to review this from a functionality perspective.

This is very exciting and I hope we can get this to the finish line soon :)

So far so good, I haven't yet tested all scenarios. We are using Card on File, and we hope to implement recurring billing through Commerce Recurring, so it would be great if we could ensure that CoF functionality remains intact (even if that means we're supplying a customer token/id to the Checkout form). I'm not sure how the StripeCheckout form is being built just yet but I'll be digging in a bit deeper, because we also want to make sure we have Bitcoin support enabled. Will dig in a bit today and provide feedback as soon as I can. Thanks!

EDIT: Saw the change to the module's commerce_stripe.js file, which includes the config functions. I'd be happy to update this patch to include some Bitcoin functionality (which would be a setting you configure).

EDIT 2: Testing Bitcoin integration, getting "Invalid integer" and I discovered that there is no "amount" property being set to handler.open(). This seems to be a "highly recommended" configuration options, and seems like one that is required when interacting with the Checkout form.

Yeah, probably better left to its own issue. Same with any Card on File support, since we'd probably need to introduce some Webhooks and another integration method, given that the Stripe Checkout form bypasses the onsite payment form's stored card data.

I'll try to get to testing the iFrame integration as it is now. So far it's looking really good though. Thanks!

Just tested a bit more fully, and can confirm that Card on File functionality actually does work. It's just a little bit odd, since the "Store this card" checkbox appears without any actual credit card forms. Just less than intuitive, but overall it does seem to work well. (This might also be helped by some descriptive text beneath the Credit Card radio button).

Nice work, thanks for getting this up here! I'm excited to have our users test it out.

please move bitcoin discussion to this thread https://www.drupal.org/node/2382147

patch committed to dev, and i will cut a new stable release to include this once we are comfortable with it.

Awesome thanks! I noticed on the project page though, that 2.x was mentioned as the ground for the iFrame integration, so perhaps the patch should get applied there as well?

2.x is basically defunct for now. i had a separation in my mind of which features would go into 2.0, but at this point things are just getting worked on when people get to them, and we need to get the iframe thing rolled out without waiting for other things, so at this point i am thinking about dropping the idea of 2.0 and just doing point releases on 1.x for now.

Cool, thanks for the clarification. I'm okay with that course of action for now, especially if the features can be rolled in without breaking any backwards compatibility.

Stripe Checkout is a great feature thank you. I tested the latest dev version and came across a few issues. When checking out anonymously, the user's email is not passed to the checkout form and the following error appears:

Notice: Undefined property: stdClass::$mail in commerce_stripe_submit_form() (line 227 of C:\xampp\htdocs\new\sites\all\modules\commerce_stripe\commerce_stripe.module).

This error occurs because $user->mail is being used as shown bellow:

    drupal_add_js(array('stripe' => array(
      'currency' => $payment_method['settings']['stripe_currency'],
      'name' => variable_get('site_name', ''),
      'verify_zipcode' => ($verify_zipcode) ? TRUE : FALSE,
      'client_email' => $user->mail,
    )), 'setting');

Should we use $order->mail instead?

Also in the following code should !empty be isset?

   $verify_zipcode = !empty($payment_method['settings']['checkout_settings']['verify_zipcode']) ? $payment_method['settings']['checkout_settings']['verify_zipcode'] : 0;

Actually do we even need this check? Doesn't a checkbox always have a value?

Thank you,
Paul

@pnigro:

Should we use $order->mail instead?

Actually I think we'll want to check for both, e.g.:

  'client_email' => (!empty($user->mail)) ? $user->mail : $order->mail,

That should work for anonymous users but I'd have to verify.

We could specify $order->mail instead, but I'm not 100% sure if there is much delineation between a user's email address and the email address used in the order. I'd have to dig in a bit more to see how $order->mail is populated.

should !empty be isset?

Using empty() instead of isset() is the preferred method in this case, because the value we are checking can be FALSE, which an isset() would return as TRUE, and thus a false positive. We want a value of FALSE to return as such.

Doesn't a checkbox always have a value?

Right, and that value can be FALSE, which is why isset() would not work here. If a user submits the settings form with that checkbox unchecked it can either be not set or 0 (FALSE), so an empty() seems to be the most succinct way to check. My opinion, anyway!

I realized a slight problem with the current patch's Checkout implementation. This line:

form$.get(0).submit(form$);

If you move the Payment form to the first Checkout page, this line of Javascript seems to be causing other forms in the Checkout page to be submitted, in our case, the Commerce Coupon redemption form. I could be wrong because I was under the impression that the JS stuff was attaching to the "closest" form. But what happens is that both forms get submitted and if you don't have a coupon code in the redemption form it errors out with "Please enter a code."

It might also just be related to posting to the main /checkout/ORDER-ID page, because the checkout router system seems to want to submit any available pane forms depending on the page it's on. So this might be closer to a Commerce Coupon problem. I'll dig in a bit more and try to verify.

@torgosPizza

  'client_email' => (!empty($user->mail)) ? $user->mail : $order->mail,

The above check does work for both registered and anonymous users.

Thank you for the explanation regarding isset() and !empty. I updated my patch for the Remember Me option #2493633-3: Make Remember Me feature optionally on Stripe Checkout.

Glad to help, and I did test and it does seem that $order->mail would also work. I suppose the question more becomes, rather if it's possible to attach a different email address than the one the user actually uses. The only method I know that does something like this is verification after returning from PayPal, and there are a bunch of issues related to that. Happy to help with the isset() thing, that's bitten me before on more than one occasion!

Regarding my own comment in #46 this appears to be a manifestation of $form_state triggering_element seems to get last button rendered not last button clicked? and #1342066: Document that buttons with the same #value need a unique #name for the form API to distinguish them, or change the form API to assign unique #names automatically but adding #name to the Continue button didn't seem to help. I can open that as its own ticket for Stripe or Commerce.

I'll set this to Fixed since it was committed in #40. I'll spin off a new issue from the feedback in #45 and #49.

Created child issues, and updated title to be more relevant.

I tested this solution and all looks to be functioning properly. However, if I choose to close the iframe popup window and return to the Drupal checkout form (using the x in the upper left corner of the iframe), the 'Go back' button in my checkout form is set to "disabled".

Looks like we disable these buttons when we show the iframe, but do not release them back to active if the user cancels the iframe. I tried to look and find where this is being introduced but I can't figure it out.

It's probably the onClose() handler in commerce_stripe.js. It's supposed to remove the "disabled" attribute from Continue but perhaps we need to specify the Go Back button as well.

var handler = StripeCheckout.configure({
                key: settings.stripe.publicKey,
                token: function(token) {
                  $('#stripe_token').val(token.id);

                  // And submit.
                  form$.get(0).submit(form$);
                },
                closed: function() {
                  submitButtons$.removeClass('auth-processing');
                  $('.checkout-processing').hide();
                  submitButtons$.removeAttr("disabled");
                }
              });

That being said I would suggest starting a new Issue for this. Since this one is closed and the feature request is "in" the -dev version, we shouldn't keep this open for that particular bug.

@torgosPizza -- got it. I have opened a new issue.

We're looking to clarify the PCI situation regarding this module, as we're reading different things in this thread to what we're being told by Stripe. Happy to open a new issue for it, however it's been discussed extensively in this issue so far.

Having read the PCI DSS v.3 spec, it states:

If any element of a payment page delivered to consumers’ browsers originates from the merchant’s website, SAQ A does not apply; however, SAQ A-EP may be applicable. Examples of e-commerce implementations addressed by SAQ A-EP include:

* Merchant website creates the payment form, and the payment data is delivered directly from the consumer browser to the payment processor (often referred to as “Direct Post”).
* Merchant website loads or delivers script that runs in consumers’ browsers (for example, JavaScript) and provides functionality that supports creation of the payment page and/or how the data is transmitted to the payment processor.

And this supports andyg5000 comments in #6:

I don't think we should close this out. Currently the module is still using the direct post api regardless of the javascript it's including. It would be great to add support for Stripe's iframe solution which would enable store owners to use PCI SAQ A. Currently they have to use PCI SAQ A-EP because of updates to PCI in Jan 2015

(which is how we've interpreted the revised specification)

We've since been discussing this with Stripe's support, as they state on the following documentation page that Stipe.js handles this: https://support.stripe.com/questions/what-about-pci-dss-3-0

And this is from a support email from Stripe today:

Our new version of Stripe.js meets the criteria [of SAQ A] by performing all transmission of sensitive cardlholder data within an iframe served off of a Stripe.com domain controlled by Stripe. This allows our users to continue to be eligible for SAQ-A in most cases. This even goes for if you’re using a customer built form, and not Stripe Checkout. Our Auditors have confirmed that as long as you’re using Checkout or Stripe.js to tokenize cards, you continue to covered, and eligible for the SAQ A.

Could someone please clarify this as, unless we're misunderstanding how this module works or if it isn't using Stripe.js in the correct way, it seems that both methods that this module now uses (Stripe.js and Checkout) are SAQ-A compliant (at least according to Stripe)?

@Adam Wood -- well, thats interesting. I mean, the language from PCI v3 seems pretty damn clear and straightforward on that point. But if that's what Stripe is saying, then i'd like to hear some more details from them on how that works, and specifically what "in most cases" means. im going to contact them and hopefully just get someone from there to weigh in on this thread.

sent them a message just now, hopefully will hear back within 24 hours.

Hi Everyone. It's true the nuances of PCI compliance can be confusing and stressful, which is one of the reasons that my colleagues and I worked on the Drupal PCI Complance white paper (http://drupalpcicompliance.org/) to give everyone a plain english overview and hopefully save time/frustration.

That said, there was considerable confusion on where HPP and iframe solutions landed when the 3.0 spec was originally published. In fact, it wasn't until there was significant pushback from the community that the PCI council released clarification documents and FAQs that back peddled a little bit. My interpretation (and I've consulted with two different QSA's on the subject matter) is that HPP and iframe solutions are considered compatible with SAQ A within the 3.0 spec.

For those wanting specific references, please see this message https://github.com/rickmanelius/drupalpcicompliance/issues/30#issuecomme....

Finally, a caveat—While PCI compliance and security are correlated, they are not interchangeable. If you are using the iframe solution from Stripe but you're running Drupal 7.31 or earlier (i.e. vulnerable to the infamous https://www.drupal.org/SA-CORE-2014-005), then someone can still hack your site, change your payment gateway, and/or find other ways to cause a data breach. However, such an attack wouldn't go easily undetected, which is why it's considered safer than direct post and merchant managed solutions.

If people really want to go into details, I recommend viewing the github thread and/or posting PCI related questions there after reading the white paper and determining if there is a way to improve the language to make it even more clear.

Also see https://github.com/rickmanelius/drupalpcicompliance/blob/master/DrupalPC....

PS. I'm in a process of writing up an article on the subject matter of PCI v3.0 and the newly release commerce stripe/braintree functionality. I'll post to this thread for those that are reading along and want to dive a little deeper.

Hi Rick,

Thanks for your insight on this.

Just to clarify, are you saying that your opinion about this module's PCI compliance has changed from you comments in #18 below?

Unfortunately, this is not a proper implementation of an iframe solution. ... as implemented, this appears to be using the direct post correctly and therefore it still falls within the scope of SAQ A-EP.

... or that Stripe need to clarify the matter?

I should add that I'm not very experienced with Stripe, however their example implementation of it using Stripe.js doesn't use a traditional iframe to load the form, rather it appends an iframe to the body. Based on my quotes from them in #56 and their documentation, they state that this is SAQ A compliant. Do you agree with this?

Hi Adam. To briefly respond to your question in #61 regarding whether it changed my opinion from my previous post (#18). Yes, I clarified my position change in #34 because the issues in #18 had been addressed.

I would like to re-iterate what has been said a couple times in this thread... this issue is currently fixed from the perspective that the iframe implementation is resolved. If people wish to do a deeper dive on the PCI compliance implications and whether or not it technically adheres to an iframe solution and whether it technically falls within scope of SAQ A, I'm happy to participate. However a dedicated thread to that would be better so we can focus on just that and manage the issue status appropriately.

We can then simply link to the new issue and let this issue officially close for good :)

agreed. lets move the PCI discussion here https://www.drupal.org/node/2500027

re #62, i think what @rickmanelius means is that his concerns were addressed by adding support for Stripe Checkout. i do not think he regards our old, direct-post implementation of Stripe.js as qualifying for SAQ A, contrary to what @Adam Wood's conversation with Stripe seems to indicate.

in any case, please move any further discussion to https://www.drupal.org/node/2500027

Yes. Direct post (old stripe implementation) is SAQ A-EP.

Should the billing address show up in strip with this method? I'm getting all null and US as country (Should be CA)

@drewmacphee: Yes, the address should be sent to Stripe in the token creation request, but it does seem to require that you have a Billing Address enabled on Checkout. Is that not the case for you?

I'd also suggest opening a new issue since this one is Fixed.

Yes I do, I even checked that it ends up in the $billing_address and $address variables, it does.
Doesn't make it to stripe though, all NULL.

Logged in and annon users.

I would open a new ticket, because I can't reproduce your issue.

EDIT: Ticket is here: #2508359: Using Checkout, Stripe gets null address info

ATTENTION: PLEASE DONT MAKE ANY NEW POSTS TO THIS THREAD!! START A NEW ISSUE!!