nraboy / ng-cordova-oauth

AngularJS oauth library for use with Apache Cordova projects
https://www.thepolyglotdeveloper.com
MIT License
456 stars 199 forks source link

Stripe plugin should not require secret key #281

Open Faeriol opened 8 years ago

Faeriol commented 8 years ago

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 commented 8 years ago

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.

remi-stripe commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

Want to do a PR so you get the credit?

remi-stripe commented 8 years ago

@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 commented 8 years ago

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

remi-stripe commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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