Remove Stripe iFrame when StripeProvider unmounts

rogersnm commented 7 years ago

Every time the StripeProvider component is constructed it creates a new iFrame in the DOM that is not removed when the component is unmounted (the iFrame appears to be asynchronously created after the Provider constructor executes due to window.Stripe() being called). After transitioning between various different pages that may or may not contain stripe components I'm left with numerous iframes cluttering up the DOM.

I'm aware that I can avoid the problem by placing the StripeProvider higher up the component tree, however I'm unwilling to force the user to download the Stripe.js lib unnecessarily if they're never going to see any Stripe components and therefore would rather keep it inside the pages concerned.

I couldn't see any nice API on the object that comes back from window.Stripe() to remove the iFrame/destroy the instance, however simply running: this._stripe._controller._controllerFrame._iframe.remove(); inside componentWillUnmount on the Provider achieves the desired effect (dirty though that is).

To me, it seems like the ideal solution would be to update Stripe.js to contain a more elegant unmount/destroy/unregister function and then trigger that in componentWillUnmount on the Provider.

jenan-stripe commented 7 years ago

Hi @rogersnm!

however I'm unwilling to force the user to download the Stripe.js lib unnecessarily if they're never going to see any Stripe components and therefore would rather keep it inside the pages concerned.

Can you help me understand your use case a bit more?

Our fraud detection works best when Stripe is loaded and initialized at the beginning of the user session, as early as possible. Additionally, we do a bit of bootstrapping when the client is initialized, so you'll probably want it earlier rather than later for performance reasons.

Just loading and initializing Stripe.js shouldn't cause any drag on the rest of your page (if it does, please let us know!).

rogersnm commented 7 years ago

Hi @jenan-stripe, thanks for taking the time to evaluate this issue 😄

The blocking nature of including Stripe.js via the script tag (<script src="https://js.stripe.com/v3/"></script>) increased our "time to first meaningful paint" by 150% (from 700ms to 1800ms) in our Google Lighthouse report.

In addition to this, we're leveraging Stripe's ability to store payment details to ensure our returning users don't have to re-enter their card details, and so for the majority of visits to our website, users will never see any Stripe components (only seeing them on the rare occasions they wish to add/update their card details). I'm unsure of how Stripe's Radar functionality can be beneficial in this circumstance, given that there is no direct link between a user's session on our website and our backend triggering a payment on their card.

For these reasons, we came to the decision that the Stripe.js library should only be loaded in asynchronously when required, so as to improve the user experience in the majority of use cases, however this causes the issues mentioned above.

michelle commented 7 years ago

Wow! Those times are really concerning and not at all what we'd expect. If you don't mind providing us with additional details on the experiments you ran, that would be really helpful for us in debugging.

We'll also look more into the specific issue, though.

michelle commented 7 years ago

Hey @rogersnm -- we just landed https://github.com/stripe/react-stripe-elements/releases/tag/v1.1.1. Does this release fix your issue?

rogersnm commented 7 years ago

Hi @michelle,

Sorry for the delay in replying previously. The new release does indeed fix the problem 😄, I'm more than happy for you to close this issue (although the release is not up on NPM yet).

Re: the experiments - I can't provide you with too much specific information at this point in development but in general:

The primary experiment we ran was the Google Lighthouse report (https://developers.google.com/web/tools/lighthouse/#devtools), in addition to simply observing the Chrome network tab
We're using SSR with embedded style sheets (with all other dependencies loaded asynchronously) so that the entirety of the page can be rendered before the client side React instance loads and initializes
Including the Stripe.js <script> in the <head> blocks rendering until the script is loaded, parsed and executed (which allegedly takes ~950ms according to Lighthouse); including Stripe.js in the <body> causes a race condition with the <StripeProvider> constructor (as window.Stripe won't necessarily be defined in time)
We were testing in London against a London based AWS EC2 instance, although from what I can see Stripe.js is also being served by a London-based Fastly cache

Thanks! Nick

skogsmaskin commented 6 years ago

I'm missing a destroy / cleanup function as well. I think this is an important feature especially for single page apps, because there are many reasons why you don't want external scripts and iframes stuck in your page when the user has navigated away from the relevant functionality (security issues for one).

This is what I'm currently doing to destroy:

controllerFrame._iframe.remove() for removing the Iframe
Removing the script tag from the document head.
Deleting window.Stripe

This works kind of as expected, however it seems like deleting window.Stripe causes the controller iframe to be loaded twice the next time Stripe is loaded. So for now I'm keeping window.Stripe and it works as expected. But I would really like an official way to do this, also removing window.Stripe without side-effects.

jSchez commented 6 years ago

For anyone interested, I came for the same issue.

This is how I have managed to clean the iframe on an Angular4 app

` ngOnDestroy() {

this.card.unmount();
if ( this.stripe['_controller'] ) {
  const $iframe = this.stripe['_controller']['_controllerFrame']['_controllerId'] || false;
  if ( $iframe ) {
    const $element = $('iframe[name="' + $iframe + '"]');
    if ( $element.length ) {
      $element.remove();
    }
  }
}

}`

MovingGifts commented 6 years ago

I still see the iframes build up, is there an official react stripe fix for this? cc @michelle

Gidgidonihah commented 6 years ago

I can verify that I too am still seeing this behavior with v1.6.0

ItsJonQ commented 6 years ago

👋 Hai all!

I checked this out, and got a rough example working:

stripe-provider-unmount-iframe

I did this by adding the following functions to the <Provider> component's componentWillUnmount() method:

const getStripeControllerFrameNode = stripe => {
  const frameId = stripe && stripe._controller && stripe._controller._id
  if (!frameId) return
  // Note: Using `document` isn't full-proof as it may not work if <Provider>
  // is rendered within an iFrame. This can be improved.
  return document.querySelector(`[name='${frameId}']`)
}

const removeStripeControllerFrame = stripe => {
  const frameNode = getStripeControllerFrameNode(stripe)
  // Note: Does not work, unless Stripe is [ready]. Will need to hook into
  // ready callback (somehow)
  if (frameNode && frameNode.parentNode) {
    frameNode.parentNode.removeChild(frameNode)
  }
}

And then calling it like:

export default class Provider extends React.Component<Props> {
  ...
  componentWillUnmount () {
    removeStripeControllerFrame(this._meta.stripe)
  }
  ...
}

This approach makes some vague assumptions on DOM things, but maybe it's something that can be explored if it'll solve the multi-iFrame issue.

🙌

asolove-stripe commented 6 years ago

I think I'm going to close this issue as the original problem is resolved.

If you want to load Stripe.js asynchronously, we now support doing that via StripeProvider, and have some pretty good docs and an demo of how to do so. The Stripe.js library does not support a way to "unload" itself and remove the controller instance. Adding it presents some API difficulties since the stripe instance itself may be in use elsewhere on the page.

peitalin commented 5 years ago

I think I'm going to close this issue as the original problem is resolved.

If you want to load Stripe.js asynchronously, we now support doing that via StripeProvider, and have some pretty good docs and an demo of how to do s

Can you comment on whether these API difficulties are something the Stripe team is planning to work on? or won't fix?

If not, might be good to include this issue in the docs so people are aware that they can't mount/unmount Stripe elements in say modals, etc. It can easily bloat the application.

@asolove-stripe

asolove-stripe commented 5 years ago

@peitalin Can you say a little more about what you're trying to do that causes this to happen?

My mental model is that a StripeProvider should only ever be mounted once, fairly high up in the DOM tree, and that you can then mount and unmount individual Elements where you want them, which will not generate this multiple controller iframe problem.

If you unmount/remount the whole StripeProvider dynamically in response to user actions, that corresponds to us needing to load the whole Stripe.js library again, reinitialize it, load several documents inside iframes, etc. This is generally too slow to be a satisfying experience if it's done dynamically in response to user input. That's why we would recommend having the StripeProvider stay in place, and just mount/unmount individual Elements, which is much faster.

peitalin commented 5 years ago

Actually, it was a separate issue with rendering Payment Request Buttons in modals, even when <StripeProvider/> is high up in the component tree.

A new Payment Request Button was created when the modal opens as it needs the subtotal:

    const paymentRequest = props.stripe.paymentRequest({
      country: 'US', currency: 'usd',
      total: {
        label: 'total',
        amount: props.subtotal,
      }
    });
    const prButton = props.stripe.elements().create('paymentRequestButton', { paymentRequest });
    prButton.mount('#payment-request-button');

Which was leaving behind a bunch of <iframes name="__privateStripeFrame125"> which needed to be cleaned up on dismount. I've resorted to this for now:

  componentWillUnmount() {
    let ivan = document.getElementsByTagName("iframe");
    let i = 0;
    while (i < ivan.length) {
      if (ivan[i].name.startsWith("__privateStripeFrame")) {
        ivan[i].remove()
      }
      i += 1;
    }
    // this.state.prButton.destroy('#payment-request-button');
    let dave = document.getElementById('payment-request-button');
    dave.removeChild(dave.firstChild)
  }

The normal Stripe Card Element removes the iframe on unmount when the <StripeProvider/> is placed at top level.

asolove-stripe commented 5 years ago

Ah, yikes! Thanks for the minimal case, that's definitely a bug. We will fix at the Stripe.js level.

brndnmtthws commented 5 years ago

Here's what worked for me:

https://gist.github.com/brndnmtthws/c5b28592c321533b8aa6971212295045

The way stripe is handling this is really not okay. If I don't want your code running anymore, I should be able to unload it.

SV thought leaders need to realize that people don't want to be constantly surveilled. We're not your rats in a giant experiment.

patrickmichalina commented 5 years ago

@brndnmtthws absolutely. This makes me think twice about stripe's intentions and I no longer believe them at all when they claim this is for "fraud monitoring". Anyway, clearing timeouts was a no-go for me since it also cleared animations and other app related tasks. Instead I used mutation observers. It only takes care of the pesky spyware injected every 5 seconds or so. I handle the other controllers in a component destruction handler (no need to show here).

// typescript
const config = { attributes: false, childList: true, subtree: false }

const filterNodes = (nl: NodeList) => Array.from(nl)
      .filter(b => b.nodeName === 'IFRAME')
      .filter((b: HTMLIFrameElement) => b.name.match(/privateStripeMetricsController/g)) as HTMLIFrameElement[]

const mutationCallback = (a: MutationRecord[]) => a.reduce((acc, curr) =>
      curr.type === 'childList'
        ? [...acc, ...filterNodes(curr.addedNodes)]
        : acc, [] as HTMLIFrameElement[])
      .forEach(a => a.remove())

    new MutationObserver(mutationCallback)
      .observe(document.body, config)

brndnmtthws commented 5 years ago

@patrickmichalina smart! Thanks for sharing.

asolove-stripe commented 5 years ago

@brndnmtthws @patrickmichalina I appreciate you all holding us to account for privacy. We care about this a lot and hope you won't let up until we've explained this clearly and to your satisfaction!

A few quick thoughts:

We are working on a plain-English description of exactly what we do with data gathered from Stripe.js. This is currently covered in our Terms&Conditions and Privacy Policy, but not in an easy-enough-to-understand way.
At a high level, we are definitely using this data to evaluate fraud risk of specific transactions and to observe when things aren't working. We are not using it for third-party sharing or to create advertising profiles.
I'm not aware of any tracking we do other than on first page load, when your JS calls our methods, or when a customer interacts with our specific embedded UIs. If you're seeing more than that, would love to dig in to it. In particular, I don't think leaving Stripe.js on the page after you're done with it has any privacy implications.
Nevertheless, it does seem like we should offer a way to tear down all the things Stripe.js sets up, at least to save the memory consumption and for sanity. We'll look into what that would require from an implementation perspective.

(Update 2020-04-21: thanks to Michael Lynch for pointing out that point 3 above is incorrect: I apologize for that comment. We are clarifying the privacy protections applied to the data that is tracked here.)

(Update 2020-04-28: please see our blog post for an updated explanation of our privacy policies and a new way to turn off tracking features in Stripe.js.)

brndnmtthws commented 5 years ago

@asolove-stripe Stripe is definitely phoning home continuously after navigating away from the payments page, unless I manually remove everything. Perhaps that's a bug? In any case, there should be a way to remove everything after the payment is complete.

I appreciate the update. I also understand that this problem is somewhat unique to SPAs. For a regular webpage, once you navigate away everything gets cleaned up by the browser so it's not such a big deal.

Just to be perfectly clear: I'm not suggesting Stripe is doing anything nefarious like logging keyboard events, but it's also impractical to verify that you're not doing that. I could step through the code in a debugger, but it's much easier to just make sure all the timers have stopped and event handlers have unmounted.

lantanios commented 5 years ago

Are there any updates from stripe about cleaning up left behind frames? I'm on way choosing payment system and need a clean code. Definitely don't want to clean up frames Some custom way which can stop working at any moment. One more question how the braintree is handling the same case, will it make sense to switch?

Alph44rchitect commented 4 years ago

I found a solution that fits for our React project. First, only on the payment page, we injecting Stripe.js into the dom. When the user is loading another page and the component is unmounting, we trigger a page refresh.

Might be not the perfect solution but a simple and sufficient workarround for us.

const [stripe, setStripe] = useState(null)

useEffect(() => {
  const stripeKey = 'YOUR_STRIPE_PK'
  const stripeUrl = 'https://js.stripe.com/v3/'

  if (!document.querySelector('#stripe-js')) {
    const script = document.createElement('script')
    script.async = true
    script.id = 'stripe-js'
    script.onload = () => {
      setStripe(window.Stripe(stripeKey))
    }
    document.body.appendChild(script)
    script.src = stripeUrl
  } else if (window.Stripe) {
    setStripe(window.Stripe(stripeKey))
  }

  return () => {
    window.location.reload()
  }
}, [])

grimpa commented 4 years ago

For anyone interested, I came for the same issue.

This is how I have managed to clean the iframe on an Angular4 app

` ngOnDestroy() {
this.card.unmount();
if ( this.stripe['_controller'] ) {
  const $iframe = this.stripe['_controller']['_controllerFrame']['_controllerId'] || false;
  if ( $iframe ) {
    const $element = $('iframe[name="' + $iframe + '"]');
    if ( $element.length ) {
      $element.remove();
    }
  }
}
}`

I'm using angularjs but making some changes to angular version this trick works well for me. xD

The $destroy event works like a destructor of a angularjs controller. $scope.$on("$destroy", function handler() {});

So, inside the controller I've put this code:

$scope.$on("$destroy", function handler() {

            $scope.card.unmount();

            if ( $scope.card['_controller'] ) {
                const iframe = $scope.card['_controller']['_controllerFrame']['_controllerId'] || false;
                if ( iframe ) {
                    const iFrameelement = $('iframe[name="' + iframe + '"]');
                    if ( iFrameelement.length ) {
                        iFrameelement.remove();
                    }
                }
            }

        });

theprojectsomething commented 4 years ago

For those that need it, I've put together a simple example showing how to sandbox Stripe.js code and unload it when you're done. It maintains a reasonable level of trust in Stripe (who deserve it), and doesn't involve secondary domains or reverse engineering of the Stripe.js library:

https://codepen.io/theprojectsomething/full/ExVNEoZ

Of course, would definitely prefer a simple, built-in mechanism to unload the library so we weren't relying on workarounds.

abeaudoin2013 commented 4 years ago

Hey there. A checkout page I was working on was going completely blank due to an error trying to find the stripe frame id. (This was happening on a componentWillUnmount in React). Well, it turns out that something changed and now the stripe id is dynamic. Here is my work around for those that need it:

const frameId = Object.keys(this.props.stripe._controller._frames)[0]
if (this.props.stripe._controller._frames[frameId]) {
    // Stop all listeners, remove iframes
    this.props.stripe._controller._frames[frameId]._iframe.remove()
    this.props.stripe._controller._frames[frameId]._removeAllListeners(
    this.props.stripe._controller._controllerFrame._removeAllListeners()
    this.props.stripe._controller._controllerFrame._iframe.remove()
}

jonathanstanley commented 3 years ago

I'm actually a bit astonished stripe has yet to facilitate unloading the stripe iframe. It may help to consider a major use case: healthcare.

In avoiding the iframe unmount, Stripe may actually be generating HIPAA violations for its surprised healthcare clients.
I am not a lawyer; I have worked extensively in healthcare/HIPAA technical implementation for over a decade but somewhat new to react-stripe.

I see that the @stripe/stripe-js/pure would defer the initialization (but still wouldn't unload once loaded). I see that advancedFraudSignals can be disabled. These are insufficient for health organizations. Although stripe may not be directly responsible for HIPAA compliance, should the OCR decide to pursue this in the future... it could be devastating to both healthcare organizations and stripe. The good news: this is avoidable

Here's why:

stripe suggests it does not offer a Business Associate Agreement (BAA) for its medical clients. Very little info on stripe's site.
Doctors are on the hook to safeguard "PHI" - basically any information that could relate to a person's health. Ex: an IP's url history on a doctor's website is PHI. Failures are fined millions and put on HHS' wall of humiliation - a list of sophisticated organizations that thought they were compliant... until boom, even state-run health organizations were fined $millions by HHS.
Lack of a BAA would forbid doctors from using Stripe, but a narrow exception to the law conditionally allows healthcare orgs to use stripe, so long as it exclusively and directly facilitates the transfer of funds. BUT, pernicious results can arise due to subtle nuances of the law, which others have written about stripe.
The surprise, is that using the loadStripe doesn't merely "load stripe" for payment processing on execution. By default, it injects the stripe iframe at the root simply by an import (prior to executing the function) and enables stripe to collect user behavior on all pages of the website/application. Despite possible benefits, that persistent iframe means the data is available to stripe when it is not allowed to be available to stripe. Turning off fraud detection makes no difference; it could be treated just as as any other HIPAA violation. HHS routinely fines organizations for failing to protect information even when 1 or 0 others ever received that information. HIPAA law does not forgive mistakes; even when well intended.

Similar disaster-patterns have happened:

example: In 2017, HHS discovered a small children's hospital was using FileFax for its facsimile processing. Many would have thought this just fine. HHS, even today, says using fax is permitted by HIPAA. But, HHS fined the children's hospital, pointing out that a BAA was required for the particular use case. HHS began investigating FileFax... and FileFax went bankrupt. FileFax may have done everything else right, but failed to lock one of its trucks during bankruptcy. So HHS fined FileFax even after they were bankrupt.

In Short:

✅ A doctor website that puts up a link to an independent, stripe-hosted payment page, could be considered HIPAA compliant.
❌ A doctor website using loadStripe, could be blind-sighted with a HIPAA violation and fine.

Resolution:

✅ Stripe iframe only on payment page / componenent

Certainly stripe could offer a BAA to covered entities and incur the extended liability imposed with the handling of healthcare information. Quick note. Reports are that an SSN is worth $0.10, a credit card is worth $0.25 and health records can be worth $100-$1000+. Given these risks along with OCR enforcement actions, most avoid BAA like the plague.

A preferable solution is for the libraries simply to give developers, like me, the choice to load and unload the stripe iframe at our discretion. I'm sure many would choose to keep the iframe and fraud detection benefits when given a choice. Others, like those in the healthcare industry, don't have this flexibility. I see many references in the documentation on loadstripe, but it isn't always clear what the side effects are. I spent many hours reading through stripe documentation and did not realize the react-stripe-persistence and telementry of the stripe iframe until I audited my front end code. This was quite the opposite of a pleasant surprise. Apparently I'm not the only one, and really appreciate several others' attempts (thank you @theprojectsomething @Alph44rchitect @patrickmichalina ) to work-around this problem. However, without an update to the stripe library, there is a lot of added complexity or poor-ui to our code - and we run the risk that stripe changes something and breaks us.

I'd think this issue should be re-opened and would love to see a library update soon. Frankly, giving developers the option to choose when the iframe is loaded and unloaded is the right thing to do.

CryDeTaan commented 3 years ago

This is so frustrating, visitors of my site will make a purchase once but now I must burden them with a ridiculous remnant iframe and listeners.

I have tried several things, but nothing seems to work.

It seems the "functions" to remove the listeners and iframes don't work,

Here is what I do(its a VueJS app): I add the stripe.js to head on the page I need it.

let src = 'https://js.stripe.com/v3/'
let script = document.createElement('script')
script.setAttribute('src', src)
script.setAttribute('type', 'text/javascript')
script.setAttribute('id', 'stripev3')
document.head.appendChild(script)

Once I am done with this page,

// remove stripe.js from head
let el = document.getElementById('stripev3')
if (el) { el.remove() }

// This does not remove listeners or frames. 
let frames = this.stripe._controller._frames

for(let i = 0; i < frames.length; i++){
    frames[i].removeAllListeners()
    frames[i]._removeAllListeners()
    frames[i]._iframe.remove()
}

this.stripe._controller._controllerFrame.removeAllListeners()
this.stripe._controller._controllerFrame._removeAllListeners()
this.stripe._controller._controllerFrame._iframe.remove()

// manually remove
let stripeIframes = [
    document.querySelectorAll('[name^=__privateStripeMetricsController]'),
    document.querySelectorAll('[name^=__privateStripeController]'),
];

stripeIframes.forEach(iframes => iframes.forEach(iframe => {
    iframe.parentNode.removeChild(iframe);
}));

It seems that because of the event lister the frame will return

stripe-archive / react-stripe-elements