search

stripe / stripe-react-native

React Native library for Stripe.
https://stripe.dev/stripe-react-native
MIT License
1.27k stars 265 forks source link

Hide fields from `CardDetails` #63

Closed bg-stripe closed 3 years ago

bg-stripe commented 3 years ago

Developers should not be able to easily inspect properties on the CardDetails object returned from CardField. We should hide all the properties:

export type CardDetails = {
};

Currently, developers can look at cardDetails.number easily. Instead they should just be using the CardDetails object directly.

Ideally, developers should not be able to inspect these properties from CardDetails at all. If that is too difficult, we should do our best to hide them. (cc @thorsten-stripe )

thorsten-stripe commented 3 years ago

@arekkubaczkowski maybe we can do this? We just want to make sure folks don't accidentally end up sending full card details to their servers.

export type CardDetails = {
  last4: string;
  expiryMonth: number;
  expiryYear: number;
  postalCode?: string;
};
arekkubaczkowski commented 3 years ago

@thorsten-stripe @bg-stripe I agree with you that it should be hidden from a security point of view. Just to clarify, do I understand correctly that you want to restrict an access by changing types of CardDetails but leaving a real data there? If it's ok for you, we can go with this but we have to keep in mind that someone just can console.log datails and check what is inside. There is some solution to completely get rid of access to card details, we could store it inside of SDK state and use it if it's needed but there appears another case, these data are needed for further actions like making the payments, so without that developer is not able do it so it can be tricky a bit because of necessity to associate card details with particular payment, we can tackle this by e.g. assigning ID of native card input to them to be able recognize proper association. This is of course just an idea and still can be improved but at the moment we can stay with type limitation and tackle this case deeper in next part of development.

WDYT?

bg-stripe commented 3 years ago

Let's stay with a type limitation for now. I don't want to introduce too much complexity here just yet. @thorsten-stripe 's suggestion of just removing the number prop from the type seems reasonable.

arekkubaczkowski commented 3 years ago

@thorsten-stripe @bg-stripe I want to ask about defaultValue prop of CardField component. Because of changing CardDetails interface, do you wanna leave this prop? If we assume that we don't want to give developer's the control of card field values I think it's not needed anymore. Also there is used the same interface as in onCardChange callback so we would have to either get rid of this prop or create separate interface for it.

thorsten-stripe commented 3 years ago

I think we can remove the defaultValue prop for the CardField component, I don't see a use case for it.

Regarding onCardChange I think it would be great if we could return the modified CardDetails type here: https://github.com/stripe/stripe-react-native/issues/63#issuecomment-768764494

Also, in case it's easy to add, it would be nice to additionally add the following params to it:

arekkubaczkowski commented 3 years ago

@thorsten-stripe Unfortunately I can't find the way to get brand name on android because all of the related fields are private, there is also the problem to detect if card field is complete or not because there is only a callback which is called when card field is completed (is valid) but without any value so if we remove some data from fields we can't distinguish if details are not valid again

to clarify

override fun onCardComplete() {
        // we know here that card field is completed
      }

but we don't have possibility to check opposite situation

thorsten-stripe commented 3 years ago

We could set complete to false on onCardChange and to true on onCardComplete maybe? But don't worry about it if it's too much of a hassle.

thorsten-stripe commented 3 years ago

Let's just make sure we remove access to the full card details as best we can

arekkubaczkowski commented 3 years ago

We could set complete to false on onCardChange and to true on onCardComplete maybe? But don't worry about it if it's too much of a hassle.

I have notice that it works a bit differently than on iOS because on android onCardComplete callback is called after card details are fulfilled instead of all of the fields (like on iOS) so we would have such scenario on android:

  1. fulfill card details
  2. complete is set to true
  3. fulfill the rest of fields
  4. complete is set to false
thorsten-stripe commented 3 years ago

Okay, let's leave it for now and figure this out if users ask for it.

arekkubaczkowski commented 3 years ago

@thorsten-stripe could you take a look and let me know if that's what you meant? https://github.com/stripe/stripe-react-native/pull/68 in short: there are returned both last4 and cardNumber but number is omitted in interface. On android I could't find last4, theoretically I could implement it on my own but I need complete information for it which is also unavailable at the moment

bg-stripe commented 3 years ago

Late to the conversations, but these decisions sound good to me! We may get user feedback about this, but I think it's fine to just wait until we do.