Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add check for nil params in webhook_valid? #35

Closed
wants to merge 1 commit into from

3 participants

@dsgh

Hi, this helps preventing errors in case someone POST's to the callback url with no parameters.

@timcraft

When/where would params be nil? In Sinatra it's a Hash, in Rails it's an ActiveSupport::HashWithIndifferentAccess.

@dsgh

From https://gocardless.com/docs/ruby/merchant_tutorial_webhook#receiving-webhooks

def webhook
  if GoCardless.webhook_valid?(params[:payload])
    render :text => "true", :status => 200
  else
    render :text => "false", :status => 403
  end
end

So params is not nil, but params[:payload] might be.

@timcraft

True, my mistake. Would be good to rename that variable for clarity.

So params[:payload] could be nil, but it could also be an array, or a string, or an integer, all of which will throw different exceptions. So just checking for nil isn't sufficient.

@dsgh

I guess so, I didn't check the implementation of signature_valid. You could check for params.is_a?(Hash) instead (It works for ActiveSupport::HashWithIndifferentAccess as well).

@timcraft

Sure, that works at the top level, but still not 100% robust. For example, looking through the implementation of the signature validation it looks like GoCardless::Utils.flatten_params doesn't handle an empty array. So something like {"payload": {"bills": []}} might also throw an exception. Instead of fixing it with defensive checks it might just be easier for the caller to rescue/log/403 all exceptions?

@dsgh

While it doesn't seem optimal to have to rescue from unknown exceptions, I'll change my code that way then.

Thanks

@dsgh dsgh closed this
@timcraft

I agree, definitely not the most elegant.

@hmarr What are your thoughts on tweaking the webhooks and the signature validation to make it easier to verify securely e.g. signing the raw request body and providing the signature in a header?

@hmarr
Owner

I've fixed that issue with empty arrays in Utils#flatten_params.

As it's only GoCardless that access the webhook endpoint, you should be able to rely on us sending valid requests. If we send you an invalid request, that is and error, so I'd advise against putting a catch-all rescue in!

The Rails community generally seems not to worry about handling this kind of thing gracefully. For example, you can cause a 500 in pretty much any Rails app that uses query strings by adding [] to parameter names (e.g. change /search?q=x to /search?q[]=x). Maybe nil is a case worth handling, but I'd say that's up to the application, not this library.

@timcraft

@hmarr In theory it's only GoCardless that should be using the webhook endpoint, but in practice anybody can access it, hence the need for verification. To quote the Pusher docs:

Since anyone could in principle send WebHooks to your application,
it's important to verify that these WebHooks originated from Pusher

Hoping the endpoint will remain secret is weak security. Any attacker can reasonably assume a website using GoCardless and Rails might have a webhook at /gocardless/webhook and easily test that. Combined with some of the recently disclosed JSON/Rails vulnerabilities and there are various attacks they could use.

Catching all exceptions is not optimal I agree. The alternatives are to make sure the verification code is robust against bad input (which can either be done in the app, or this library), or to change the API. Signing the request body is more secure because the requests can be verified before the JSON body is parsed. Pusher, Shopify, and Chargify all appear to take that approach.

Not worrying about handling this kind of thing is why we are in the situation we are in right now :)

@hmarr
Owner

It's certainly extremely important to ensure that the request is coming from GoCardless; that's the purpose of the signature. However, I'm really not keen on putting a catch-all rescue in signature_valid? method as it could hide errors in the code, which I'd much rather actually fix. An exception being raised should result in the webhook being considered invalid, to assume success in the case of an exception would be really bizarre behaviour.

To be clear, I'm not saying I'm satisfied with the popular attitude to this kind of thing! If every Rails developer properly validated input, we wouldn't be having this discussion.

I'll investigate moving to header-based signatures, but that'll be a longer-term solution, as it will require updating our API, docs, and all client libraries.

@timcraft

@hmarr Sorry, I meant catch the exceptions in the app not the library. Definitely not a good idea in the library. Not ideal in the application, but a reasonable solution if you want to catch errors. Something like this:

def webhook
  # handle verified webhook
end

before_filter :verify_webhook, :only => :webhook

def verify_webhook
  unless valid_webhook?
    render :text => 'Invalid webhook', :status => 403
  end
end

def valid_webhook?
  GoCardless.webhook_valid?(params[:payload])
rescue => exception
  # maybe log the exception

  false
end

An exception in that context is likely to be either a bug in the library, a bug in the data sent from GoCardless, or some kind of malicious input. All three cases can be handled in the same way: catch the exception, log it (optional), and return a 4xx/5xx response.

I appreciate changing the API to header-based signatures is a lot of work, but thought it worth mentioning because that would improve things from a security perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 7, 2013
  1. @dsgh
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 1 deletion.
  1. +1 −1  lib/gocardless/client.rb
  2. +5 −0 spec/client_spec.rb
View
2  lib/gocardless/client.rb
@@ -261,7 +261,7 @@ def response_params_valid?(params)
# @param [Hash] params the contents of payload of the webhook
# @return [Boolean] true when valid, false otherwise
def webhook_valid?(params)
- signature_valid?(params)
+ !params.nil? && signature_valid?(params)
end
# Set the base URL for this client instance. Overrides all other settings
View
5 spec/client_spec.rb
@@ -458,6 +458,11 @@ def get_params(url)
end
describe "#webhook_valid?" do
+ it "returns false when params are nil" do
+ @client.webhook_valid?(nil).
+ should be_false
+ end
+
it "returns false when the webhook signature is invalid" do
@client.webhook_valid?({:some => 'stuff', :signature => 'invalid'}).
should be_false
Something went wrong with that request. Please try again.