We were previously using a bit of a hack to get a free deep copy
implementation through Ruby's marshaling framework. Lint call this out
as a security problem though, and rightfully so: when combined with
unsanitized user input, unmarshaling can result in very serious security
breaches involving arbitrary code execution.
This patch removes all uses of marshal/unmarshal in favor of
implementing a deep copy method for `StripeObject`. I also reworked some
of the constants around what keys are available for `opts`. I'm still
not completely happy with the results, but I think it's going to need a
slightly larger refactor in order to get somewhere truly good.
There is what could be a breaking change for people doing non-standard
stuff with the library: the opts that we copy with an object are now
whitelisted, so if they were being used to pass around extraneous data,
that might not work as expected anymore. But because this is a contract
that we never committed to, I don't think I'd bump the major version for
change.
Removes Rubocop TODO around guard clauses and fixes the outstanding
offenses.
This is starting to get into territory that feels of more dubious value
to me, but at least it did get me writing a couple more tests, so let's
see how it goes by keeping this on.
I wanted to see what fixing Rubocop TODOs was like, so I tried to
eliminate all the easy ones. Most of these were pretty easy, and the
changes required are relatively minimal.
Some of the stuff left is harder. Pretty much everything under
`Metrics/*` is going to be a pretty big yak shave. A few of the others
are just going to need a little more work (e.g. `Style/ClassVars` and
`Style/GuardClause`). Going to stop here for now.
A few changes:
* Add a new `Util.log_error` method which will forward to the equivalent
of `#error` on a logger.
* Move errors produced by `StripeClient` to use `Util.log_error`.
* Change standard stdout logging behavior to log to stderr in the case
of `Util.log_error.
* Change `Stripe.log_level` values to be an enum in a similar fashion as
the standard library's built in `Logger`.
Adds support for setting `Stripe.logger` to a logger that's compatible
with `Logger` from Ruby's standard library. In set, the library will no
longer log to stdout, and instead emit straight to the logger and defer
decision on what log level to print to it.
Addresses a request in #566.
Hopefully the last tweak in a while, but a discussion on [1] tipped me
off that this was missing. Here we add a `Stripe-Account` for a request
and response to logging. Follows #566 and #567.
[1] https://github.com/stripe/stripe-node/issues/364
This one is minor, but I realized after shipping #566 that it would be
nice if the number of retries was also logged for every request. This
patch follows up #566 by adding that in.
I also renamed `retry_count` to `num_retries` because I subjectively
think this name is a little better.
Adds logging support for stripe-ruby in a similar way that we did it for
stripe-python [1], with the idea that users you can optionally get some
additional low-cost-to-configure logging for operational visibility or
debugging.
I made a few tweaks from the Python implementation (which I'll try to
contribute back to there):
* Added an elapsed parameter to responses so you can tell how long they
lasted.
* Mixed in idempotency_key to all lines that users have a way to
aggregate logs related to a request from start to finish.
* Standardized naming between different log lines as much as possible.
* Detect a TTY and produce output that's colorized and formatted.
[1] https://github.com/stripe/stripe-python/pull/269
Currently, with a normal API resource, you can unset fields by
specifying a `nil` to that field's setter:
``` ruby
c = Charge.retrieve('ch_123')
c.customer = nil
c.save
```
This actually gets serialized as the form `customer=` (i.e. an empty
string), but we had to use the empty string to handle unsets because
form encoding has no concept of a `nil`/`null`.
To try and prevent usage errors, we actually prevent you from setting
fields with an empty string:
``` ruby
c = Charge.retrieve('ch_123')
c.customer = '' # error! use nil instead
```
When specifying parameters though, this doesn't work anywhere nearly as
well because usage patterns like this are very common in Ruby:
``` ruby
charge_opts = {
params[:amount],
params[:currency],
params[:customer],
}
charge = Charge.create(charge_opts)
```
Each one of `params` above may or may not be `nil`, so we've
traditionally filtered those fields out during the invocation of
`Charge.create`.
Recently, I suggested to Slava that we may be able to change this
behavior, and we ended up putting in a patch as part of #557. Users
brought to my attention that this would be far too disruptive of a
change in #560 though, and having thought about it more carefully, I
agree. There's also an argument that filtered `nil` values are just a
better API, especially in Ruby where patterns like the one above are
frequently in effect.
So the best thing I can think of currently is to leave things as they
were before #557, and just require that users use an explicit empty
string when passes in parameter hashes:
``` ruby
Charge.update(customer: '') # will try to unset customer
```
Empty strings will continue to error for `StripeObject` fields like they
always have.
I don't think this is a perfect solution by any means (the different
between values on `StripeObject` versus using parameters is weird), but
it's the least disruptive thing that I can think of right now that gets
us the functionality that we need for endpoints like
`/v1/invoices/upcoming`.
Fixes#560.
It was brought up in #562 that in case we receive an OAuth error that we
don't know about, `specific_oauth_error` will fall through with a `nil`,
then picked up by `specific_api_error` which will always try to handle
the error as if it were a `Hash` (even if we know it's not!) and thus
lead to typing problems at runtime.
This patch throws a generic `OAuthError` in cases where a code comes
back that we don't recognize. I'm still crazy about the fact that we
don't have a better way of recognizing an OAuth error in particular, but
it should do the trick.
Tweaks the serialization behavior so that when a resource is explicitly
set to a resource's field and that resource is subsequently saved, then
if it looks like the set resource was persisted we extract its ID and
send it up to the API.
By slight extension we also throw an `ArgumentError` if it looks like
that set resource was _not_ persisted because if the user set it
explicitly then it was probably not their intention to have it silently
ignored by the library in the event of a problem.
