Reject reqs missing x-hub-signature header if secret_token defined

[mindbat]

Should fix #16

Updates verify_signature to check for the absence of the x-hub-signature header when secret_token is defined, and treats such requests as invalid.

Also revises an existing test to match the new behavior, and adds three more tests to verify that we don't overcorrect by requiring (or checking) the header if secret_token is not defined.

[yaauie]

The PR looks like it works as-advertised, but I'd appreciate a little validation on my understanding of how it's supposed to work to determine whether this should be considered a breaking change or a fix to the current implementation.

From what I understand, this input sets up an HTTP web server, listening for requests, and the Github hooks API includes a signature of each raw request body as a x-hub-signature header, which we capture onto the created event. If this input is started with a secret_token directive, we use it along with the raw body to validate against the event's x-hub-signature, so the absence of an appropriate header indicates that we cannot ensure that the event stemmed from an authorised web hook.

This PR, then, includes the absence of the header along-side a secret_token as an indicator that the event is invalid, allowing further filtering like drop_invalid to work as expected when the HTTP endpoint receives payloads that are not from github.

If this is the case, then I would consider the PR to be a fix for the current implementation, in which case a patch-level bump as proposed is appropriate.

[mindbat]

This PR, then, includes the absence of the header along-side a secret_token as an indicator that the event is invalid, allowing further filtering like drop_invalid to work as expected when the HTTP endpoint receives payloads that are not from github.

@yaauie Exactly. No header means we can't validate, so we consider it invalid, and let drop_invalid take it from there.

I would consider the PR to be a fix for the current implementation

Yep, that's what's intended here. Not a breaking change, just a patch to bring the behavior in-line with expectations.

[yaauie]

While your change is functionally correct and minimal, there are a couple of anti-patterns within the existing method definition (e.g., if not, unnecessary is_valid variable) and with this change the reasoning behind this code becomes a little less clear, a little harder to follow, and a little more repetitive.

I'd prefer a functionally-equivalent refactor to this whole method, which I think is an improvement:

  def verify_signature(event,body)
    # skip validation if we have no secret token
    return true unless @secret_token

    sign_header = event.get("[headers][x-hub-signature]")
    if sign_header
      hash = 'sha1=' + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), @secret_token, body)
      event.set("hash", hash)
      return true if Rack::Utils.secure_compare(hash, sign_header)
    end

    event.tag("_Invalid_Github_Message")
    return false
  end

[mindbat]

Makes sense to me! 7d54a10

[danhermann]

LGTM pending @yaauie's approval, too.

[mindbat]

@yaauie Does everything look good now? Are there additional changes you'd like to see?

[yaauie]

I've re-triggered the travis jobs that had failed due to an unrelated issue, and will merge this and push a release as soon as the build goes green.

LGTM 👍

[ngapaillard]

Hi !
Thanks for fixing ! (A little late I know)