Properly override API version if it's set in the request

[remi-stripe]

While testing the strict-version-check flag, I discovered that we were incorrectly overriding the version forced in the request creating ephemeral keys.

This fixes the issue by calling Set instead of Add. I've confirmed it works, but I'm not sure this is the right approach here.

Marking as a breaking change since Ephemeral Keys would suddenly be created with the correct API version, which is what people would likely want, but still a dangerous change. There's also another major version going out anyway.

r? @brandur-stripe
cc @stripe/api-libraries

[brandur-stripe]

@remi-stripe Doh, I think ephemeral keys overriding the version is actually by design right?

func (c Client) New(params *stripe.EphemeralKeyParams) (*stripe.EphemeralKey, error) {
	if params.StripeVersion == nil || len(stripe.StringValue(params.StripeVersion)) == 0 {
		return nil, fmt.Errorf("params.StripeVersion must be specified")
	}

	if params.Headers == nil {
		params.Headers = make(http.Header)
	}
	params.Headers.Add("Stripe-Version", stripe.StringValue(params.StripeVersion))

	ephemeralKey := &stripe.EphemeralKey{}
	err := c.B.Call(http.MethodPost, "/v1/ephemeral_keys", c.Key, params, ephemeralKey)
	return ephemeralKey, err
}

IIRC, the idea here was that ephemeral keys require an explicit version in order to get a more explicit compatibility guarantee for a client out in the field like an iOS app that may have a long app store review cycle to get it even updated.

I was going to suggest the same fix that it looks like you came up with in #811 — not super optimal because it means we're not really testing anything interesting, but at least gets everything passing.

[remi-stripe]

@brandur-stripe The issue is that the code today does not work at all while my fix does. This is due to the code using Header.Add which does not override the previous header, it's just additive. Because of this, stripe-mock still receives the library one instead of the one passed in the ephemeral key options.

[brandur-stripe]

@remi-stripe Doesn't it still produce the wrong result though? The Stripe-Version from ephemeral keys gets added first as in the code snippet above. Later, we'd call Set as we range through params.Headers, but I would think that will still blow away the ephemeral key version regardless of whether we use Add or Set.

[remi-stripe]

@brandur-stripe It does not get added first. I got confused by this too! The code you shared does not add the header to the request, it adds the header to the params:

params.Headers.Add("Stripe-Version", stripe.StringValue(params.StripeVersion))

Then in stripe.go, we add the library's version first in NewRequest:

req.Header.Add("Stripe-Version", apiversion)

Then, at the end of NewRequest we used to do:

for k, v := range params.Headers {
  for _, line := range v {
    req.Header.Add(k, line)
  }
}

This would add the version from the params but it would not replace the one we set earlier in the code in the same function.

[brandur-stripe]

Ah, shoot, I see! I misread that — you're totally right.

In that case, I think using Set like you have here is fine in terms of a solution. LGTM.

I'd even optimistically release this one without a major version bump just because the previous code was broken and ephemeral keys probably have relatively low use, but I'll leave that up to you.

[remi-stripe]

I'd even optimistically release this one without a major version bump just because the previous code was broken and ephemeral keys probably have relatively low use, but I'll leave that up to you.

Yeah I was hesitating, but since the other PR is a breaking change, it's just easier to do both at once and fix the behaviour.

Since I'm not really familiar with Go myself, are you sure this is really safe to do and the right fix?

[brandur-stripe]

Since I'm not really familiar with Go myself, are you sure this is really safe to do and the right fix?

I think it's strictly better than what we had before. Set will behave the same as Add except in the case of a duplicate, and in that case Add is particularly bad because it'll end up encoding the same value twice (in this case, I think both Stripe-Versions get added to the request headers), and that means it's up to the server in how to interpret it. Some languages like Go will probably do a slightly better job of interpreting a doubled up header (at least they give you the opportunity to try and handle it), but others like Ruby/Rack probably just arbitrarily allow one to win.

(For Ruby/Rack I think it's the last one received which means that this might've actually worked when sent to Stripe, but I'd have to run some tests to be sure of that.)

Changing to Set means that we remove the ambiguity, and we know that the ephemeral key Stripe-Version gets used every time.

The only problem I can potentially think of is that someone could exploit this to override the default API version that stripe-go is tied to by just arbitrarily making sure to set Headers on all the parameters they send. This problem isn't new or likely though, so I think it's okay.

[remi-stripe]

thanks a lot for the detailed explanation, this makes perfect sense.

[brandur-stripe]

Released as 58.0.0.