stripe-java icon indicating copy to clipboard operation
stripe-java copied to clipboard

Unpredictable failure mode due to Stripe.apiKey not being set and lazy-loaded collections

Open Dretch opened this issue 4 years ago • 4 comments

Java Version: 11 Stripe-Java Library version: 19.33.0

This may not be a bug exactly, but it looks a lot like one from here.

When Stripe.apiKey is not set, the Stripe Java API works fine most of the time, so long as a RequestOptions containing the API key is supplied with every API call.

However, when a webhook endpoint triggers a lazy-load of a collection, then the Stripe call fails with com.stripe.exception.AuthenticationException: No API key provided... because Stripe doesn't have an API key to use.

Whether a collection is lazy-loaded is not easily predictable.

In my case I found that my webhook only failed when the Payment Intent payload it received had more than one charge (i.e. one failed charge and then one successful charge). With only a single charge then lazy-loading was not triggered because the single charge was included in the webhook payload, but with two charges then only one of the two charges was included and so when the webhook code iterated over all the charges then a lazy-load was triggered.

Luckily this behaviour was found during testing, but it could easily have been missed. The API docs don't make it clear that you really to need to set Stripe.apiKey even if you also set the API key in a RequestOptions (https://stripe.com/docs/api/authentication)

I'm not sure how to fix this. Perhaps Stripe should fail to do anything if Stripe.apiKey is not set, so that this coding error is detecting as soon as possible?

Dretch avatar Jan 22 '21 16:01 Dretch

@Dretch Thanks for the detailed report! Would you by any chance have an easy reproduction for this? I'm having a little bit of trouble grasping the example you mentioned about the webhook endpoint. Just to clarify, do you mean that this happens when you receive an event and you deserialize the raw JSON with constructEvent() and that event has multiple charges in the underlying PaymentIntent? I tried this flow but it works locally for me.

remi-stripe avatar Jan 23 '21 23:01 remi-stripe

Thanks for responding @remi-stripe :)

Just to clarify, do you mean that this happens when you receive an event and you deserialize the raw JSON with constructEvent() and that event has multiple charges in the underlying PaymentIntent? I tried this flow but it works locally for me.

I mean this case, yes. The steps that occur are something like this:

  1. The webhook endpoint is called with a serialised event and a signature.
  2. The webhook code calls constructWebhookEvent to deserialise the event into a PaymentIntent object.
  3. The webhook code calls paymentIntent.getCharges().autoPagingIterable() and does some processing on each charge.
  4. The iterator from the call in step 4 fails with a stack trace like:
com.stripe.exception.AuthenticationException: No API key provided. Set your API key using `Stripe.apiKey = "<API-KEY>"`. You can generate API keys from the Stripe Dashboard. See https://stripe.com/docs/api/authentication for details or contact support at https://support.stripe.com/email if you have any questions.
 	at com.stripe.net.StripeRequest.buildHeaders(StripeRequest.java:152)
 	at com.stripe.net.StripeRequest.<init>(StripeRequest.java:75)
 	at com.stripe.net.LiveStripeResponseGetter.request(LiveStripeResponseGetter.java:53)
 	at com.stripe.net.ApiResource.request(ApiResource.java:179)
 	at com.stripe.net.ApiResource.requestCollection(ApiResource.java:199)
 	at com.stripe.model.PagingIterator.list(PagingIterator.java:78)
 	at com.stripe.model.PagingIterator.next(PagingIterator.java:53)
 	at com.stripe.model.PagingIterator.next(PagingIterator.java:11)
        ...

At no point was an API key supplied. Stripe.apiKey was never assigned (because we don't use this and instead pass RequestOptions to every Stripe call - but no method called here takes a RequestOptions) -- so the behaviour makes sense in hindsight. I clearly failed to predict that this would happen, though!

Dretch avatar Jan 25 '21 09:01 Dretch

This definitely feels like a trap to me. I agree it's worth considering throwing an exception immediately from .autoPagingIterator if it is ever called without the global APIKey set or passing in RequestOptions to .autoPagingIterator -- although this would be a breaking change.

Marking this as future and breaking-api-change, hope to put together a PR soon.

richardm-stripe avatar Jan 25 '21 15:01 richardm-stripe

Thanks @richardm-stripe

Dretch avatar Jan 25 '21 17:01 Dretch

The fix has shipped with v21.

kamil-stripe avatar Sep 28 '22 21:09 kamil-stripe