Raise error with missing signature

[jeffmcfadden]

instead of passing nil to base64 decode.

[coveralls]

Coverage decreased (-1.4%) to 98.618% when pulling bac6d25bbb5d2061a27dc737c5b7b025c2771510 on jeffmcfadden:master into bfb49d091cfbef661f9b1ff7a858cb5d9d09f473 on sidoh:master.

[sidoh]

Hi @jeffmcfadden, thanks! Makes sense to handle this more gracefully. Going to leave a small suggestion.

Looks like the build is busted. Guessing that the Timecop.freeze stuff is not playing nicely with system-provided trust stores. Won't be able to dig into it right away. tagging @mattrayner in case you're available.

[mattrayner]

I’ll try and get eyes on this tomorrow 👍

On Fri, 19 Apr 2019 at 17:46, Chris Mullins [email protected] wrote:

@sidoh commented on this pull request.

In lib/alexa_verifier/verifier.rb https://github.com/sidoh/alexa_verifier/pull/7#discussion_r277033722:

@@ -117,7 +117,7 @@ def check_that_request_was_signed(certificate_public_key, request, raw_body) OpenSSL::Digest::SHA1.new, Base64.decode64(request.env['HTTP_SIGNATURE']), raw_body

•  )
  

•  ) if !request.env['HTTP_SIGNATURE'].nil? && !request.env['HTTP_SIGNATURE'].empty?
  

I might suggest verifying that these fields are present before this line, and throwing a more specific error in the case that they're not. This is, for example, how we check if the time field is present above:

https://github.com/sidoh/alexa_verifier/blob/f3834d27817818272e1b884e29293570a11911f5/lib/alexa_verifier/verifier.rb#L83

Should still raise an InvalidRequestError, but can make the message something like

HTTP_SIGNATURE header is required, but was not present.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/sidoh/alexa_verifier/pull/7#pullrequestreview-228752793, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2XGNWW5PQYBNECBAKNAC3PRHZPTANCNFSM4HGWJGBA .

-- Matt Rayner http://www.mattrayner.co.uk/

[jeffmcfadden]

Thanks @sidoh I'll make the changes and update the PR. I was having trouble getting tests to run locally (certificate chain problems, perhaps trust store as you suggest). Sorry for the trouble.