Add FormatURLPath helper to allow safer URL path building
#573
Conversation
Adds a helper to the top-level package called `FormatURLPath`. It's
basically a pass-through to `Sprintf`, except that it only takes string
parameters. This is designed to make invocations slightly more safe --
we've had a little trouble in the past where a string pointer was
accidentally passed instead of a string, and because `Sprintf` doesn't
automatically dereference or anything, the result is an invalid URL.
It also gives us the opportunity to call `url.QueryEscape` on every
parameter for a little additional safety. Previously, this was being
invoked manually only in certain packages like `plan`.
I changed all uses of `%v` in invocations to `%s` for consistency and
because we won't need any types beyond strings. In the same spirit, I
also changed all the places that we were concatenating strings (like
`"/skus/"+id`) to invoke `FormatURLPath`.
This patch is fairly noisy, but you can mostly just examine the added
function in `stripe.go` and test in `stripe_test.go`:
``` go
func FormatURLPath(format string, params ...string) string {
// Parameters must be converted to interface{} before we send them to
// Sprintf.
untypedParams := make([]interface{}, len(params))
for i, param := range params {
untypedParams[i] = interface{}(url.QueryEscape(param))
}
return fmt.Sprintf(format, untypedParams...)
}
```
brandur-stripe
force-pushed
the
brandur-safer-path-building
branch
from
09d2c22 to
265e94c
Compare
| // Tests that each parameter is escaped for use in URLs | ||
| assert.Equal(t, "/resources/%25", | ||
| stripe.FormatURLPath("/resources/%s", "%")) | ||
| } |
There was a problem hiding this comment.
should we test/handle cases where we're missing a param like
stripe.FormatURLPath("/resources/%s/subresource/%v", "%")
There was a problem hiding this comment.
Okay, I added some tests (and an implementation) for this. See 78d772a.
Just a heads up though: this does add a little bit of runtime overhead because we have to check the response from Sprintf for bad values (see 78d772a for details there). There's also a minute chance it could cause a problem if a user has coupon or plan ID that contains (MISSING) or %!(EXTRA . That's hopefully quite unlikely, but still unfortunately possible.
There was a problem hiding this comment.
Gotcha, I did not think about the overhead. Maybe it's better to skip this then since it would mostly be our own mistake and it is unlikely to happen and not be caught by another crash or stripe-mock right?
Sorry for asking you to do the work and then undo it.
There was a problem hiding this comment.
No worries! It was an interesting experiment.
Alright, I'm going to revert the commit, but leave it in the history so that we can recover it if we want to. I don't think this has been a huge problem so far so it's probably slightly better to leave it out, but we can bring it back if mismatches in number of parameters is ever a problem in the future.
| @@ -61,7 +61,7 @@ func (c Client) GetByID(id string, params *stripe.AccountParams) (*stripe.Accoun | |||
| } | |||
|
|
|||
| account := &stripe.Account{} | |||
| err := c.B.Call("GET", "/accounts/"+id, c.Key, body, commonParams, account) | |||
| err := c.B.Call("GET", stripe.FormatURLPath("/accounts/%s", id), c.Key, body, commonParams, account) | |||
There was a problem hiding this comment.
a bit minor but would it be cleaner to use a temporary variable? That's my personal dislike of methods called as params
There was a problem hiding this comment.
Yeah, I could see this being set as path := stripe.FormatURLPath(...). Given this is fairly minor and quite a few lines to change though, would you mind if we did that as a follow up?
There was a problem hiding this comment.
totally. Especially if we end up changing most of that code to be a helper function globally too.
Reverts 78d772a which added some protections against mismatches in number of parameters. This was an interesting experiment, but it adds some runtime overhead, and so far this hasn't been a huge problem. I'm taking it back out but leaving it in Git history in case we want to resurrect it at some point.
|
Released as 32.2.0. |
Adds a helper to the top-level package called
FormatURLPath. It'sbasically a pass-through to
Sprintf, except that it only takes stringparameters. This is designed to make invocations slightly more safe —
we've had a little trouble in the past where a string pointer was
accidentally passed instead of a string, and because
Sprintfdoesn'tautomatically dereference or anything, the result is an invalid URL.
It also gives us the opportunity to call
url.QueryEscapeon everyparameter for a little additional safety. Previously, this was being
invoked manually only in certain packages like
plan.I changed all uses of
%vin invocations to%sfor consistency andbecause we won't need any types beyond strings. In the same spirit, I
also changed all the places that we were concatenating strings (like
"/skus/"+id) to invokeFormatURLPath.This patch is fairly noisy, but you can mostly just examine the added
function in
stripe.goand test instripe_test.go:r? @remi-stripe If you wouldn't mind.
cc @stripe/api-libraries