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

Stripe plugin should not require secret key #281

Open
Faeriol opened this issue Sep 3, 2016 · 12 comments
Open

Stripe plugin should not require secret key #281

Faeriol opened this issue Sep 3, 2016 · 12 comments

Comments

@Faeriol
Copy link

Faeriol commented Sep 3, 2016

The stripe provider requires the secret key. From stripe docs

We support cross-origin resource sharing, allowing you to interact securely with our API from a client-side web application (though you should never expose your secret API key in any public website's client-side code) (emphasis mine)

I'll have an alternate version up shortly and will link here. Simply put, making the request that requires the sk should be done in the backend...

@nraboy
Copy link
Owner

nraboy commented Sep 3, 2016

I agree with you, but users of this library are already depending on it in its current format. https://github.com/nraboy/ng2-cordova-oauth for Ionic 2 uses the approach that you mentioned since it was a fresh start.

@nraboy nraboy closed this as completed Sep 3, 2016
@remi-stripe
Copy link

@nraboy : While users are relying on this, it's a huge security risk for them. Any attacker can look into their application's binary and retrieve their Secret key. Once they have that they can charge customers, create transfers, refund all charges, etc.

I would second what @Faeriol said here and strongly recommend that you clearly mention that this plugin/solution is not secure when using Stripe so that no one else implements this in their application today if they come upon your library. One solution could be to display an error or a log when using Stripe directly for example so that developers know about this new version.

@nraboy
Copy link
Owner

nraboy commented Sep 8, 2016

Yes I hear what you're saying, but ultimately it is not my job to play cyber nanny on what good or bad things they do in their code. I would never use an explicit grant type in my app and I can tell you wouldn't either. Since this library is specifically labeled as an Oauth library, the developer should do their research before trying to use it.

Explicit grant providers (Stripe, etc.) were continuously requested. If I remove it, people will complain. If I leave it people will complain. More than half the providers in this library were contributed by someone else.

This was in no way an attack against you. I'd like to invite you to add a section to the README that educates people on explicit grant types and which providers are currently of that type.

Best,

@nraboy
Copy link
Owner

nraboy commented Sep 8, 2016

I take back some of what I said.

People will complain either way, but it is probably best they complain and be safe.

Please do a PR on the README educating people on explicit grants. I'd like to invite you to help me change all the explicit grant providers to only return a request token that the user can then use server side for the access token. Same how it is done in the Ionic 2 library.

@remi-stripe
Copy link

@nraboy: First off, I totally understand where you're coming from here really. People asking for this and ultimately they should be aware that this is an issue and not use it or at least make their own informed decision. Unfortunately it can be easy to end up here by googling "Cordova OAuth Stripe" (you're the first result on Google) and not realize the security implications behind this as the repo has multiple providers documented.

I'm not familiar with most of those providers though and I think I've only used Facebook out of those (beyond Stripe obviously). I'd recommend separating the list in the ones that are safe and the ones that aren't with a comment indicating this explicitly

Here's a basic patch to both the README and the comment associated with the Stripe function at least:
0001-Warn-explicitly-that-you-should-not-use-cordovaOauth.txt

It's not great but I feel like people might search for Stripe in the docs and having the comment "in place" would help with the warning being really seen at least.

Hope this helps and thanks for being open about this!

@nraboy
Copy link
Owner

nraboy commented Sep 8, 2016

Want to do a PR so you get the credit?

@remi-stripe
Copy link

@nraboy If the patch works for you that works for me too. Also there is no emergency in shipping this so if you want to look at other providers and see the ones you also considered deprecated because they have the same security issue and make this in one PR that might make more sense.

@nraboy
Copy link
Owner

nraboy commented Sep 8, 2016

It's all done on the development branch so it won't hit the public until it's finished

@nraboy nraboy reopened this Sep 8, 2016
@remi-stripe
Copy link

Sure, just wanted to say that while problematic, it's not an "emergency fix" or anything and didn't want to pressure you into doing this. I just wanted to voice that this is unfortunately a common misunderstanding from users that I deal with often enough at Stripe!

@Faeriol
Copy link
Author

Faeriol commented Sep 8, 2016

Thanks for this. I understand the situation you are in, and know users are not always easy. I'll open a PR for an updated function which returns the code stripe gives. May I suggest we either:

  1. Replace the insecure call with the new one
  2. Push the new call alongside the old one and specify that the older one is a "legacy, insecure feature" (open on suggestions on how this could be done)

Edit: you dont push a new call along side a new one, but alongside an old one.

@Faeriol
Copy link
Author

Faeriol commented Sep 18, 2016

IM SOOOO Sorry... got busy with other stuff...

Here's a copy of a module that only spits out the code we get from stripe.

(function() {
  'use strict';

  angular.module('oauth.stripe', ['oauth.utils'])
    .factory('$ngCordovaStripe', stripe);

  function stripe($q, $http, $cordovaOauthUtility) {
    return { signin: oauthStripe };

    /*
     * Sign into the Stripe service
     *
     * @param    string clientId
     * @param    string clientSecret
     * @param    string appScope
     * @param    object options
     * @return   promise
     */
    function oauthStripe(clientId, clientSecret, appScope, options) {
      var deferred = $q.defer();
      if(window.cordova) {
        if($cordovaOauthUtility.isInAppBrowserInstalled()) {
          var redirect_uri = "http://localhost/callback";
          if(options !== undefined) {
            if(options.hasOwnProperty("redirect_uri")) {
              redirect_uri = options.redirect_uri;
            }
          }
          var browserRef = window.cordova.InAppBrowser.open('https://connect.stripe.com/oauth/authorize?client_id=' + clientId + '&redirect_uri=' + redirect_uri + '&scope=' + appScope + '&response_type=code', '_blank', 'location=no,clearsessioncache=yes,clearcache=yes');
          browserRef.addEventListener('loadstart', function(event) {
            if((event.url).indexOf("http://localhost/callback") === 0) {
              var requestToken = (event.url).split("code=")[1];
              deferred.resolve(requestToken);
              setTimeout(browserRef.close, 5);
            }
          });
          browserRef.addEventListener('exit', function(event) {
            deferred.reject("The sign in flow was canceled");
          });
        } else {
          deferred.reject("Could not find InAppBrowser plugin");
        }
      } else {
        deferred.reject("Cannot authenticate via a web browser");
      }

      return deferred.promise;
    }
  }

  stripe.$inject = ['$q', '$http', '$cordovaOauthUtility'];
})();

Users can use this to obtain a code they can then forward to a backend that will securely manage the API keys

@Faeriol
Copy link
Author

Faeriol commented Sep 18, 2016

In theory a PR could be opened with this if we agree on the format (which shouldnt be hard to do)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants