Add support for ruby-jwt v2.6.0 and above, bump development deps, new Ruby vers

[oskarpearson]

Description

• Remove reference to 'secure_compare' method in ruby-jwt that this code shouldn't be using, so as to support ruby-jwt v2.6.0 and above
• Add support for ruby-jwt up to and including version 3.* (currently in beta) so that we don't have to bump support again later
• Add recent Ruby versions to the test matrix (I left version 3.0 in place, although it's no longer officially supported, so as to avoid inconveniencing existing users that are stuck on version 3.0)

Prior versions of this code depended on the method JWT::SecurityUtils.secure_compare

Release v2.6.0 of ruby-jwt moved the JWT::SecurityUtils.secure_compare method to a different namespace as part of https://github.com/jwt/ruby-jwt/pull/521

While we could simply change this code to instead refer to that new namespace, I believe that the intention of ruby-jwt was that secure_compare should not have been used outside of the internals of ruby-jwt.

This is supported by the The ruby-jwt README examples where the ruby-jwt team recommend using OpenSSL.fixed_length_secure_compare instead of JWT::SecurityUtils.secure_compare

The code in this change is based on the logic in https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/activesupport/lib/active_support/security_utils.rb#L33 so as to avoid adding a dependency on ActiveSupport for this single method.

Unlike the code in the activesupport url above we don't fall back to a custom implementation of fixed_length_secure_compare, since OpenSSL.fixed_length_secure_compare is present in OpenSSL 2.2 and this gem already depends on Ruby 3.0 and above, which already includes that version of OpenSSL

This code also doesn't need to handle nil/empty cases, unlike the original implementation of JWT::SecurityUtils.secure_compare because these are already handled in the call to validate_url in one case, and validate_payload itself in the other.

Changelog for Automated Release

Note: this will be used by release.yml to automatically adjust CHANGELOG and publish the gem.

CHANGELOG

• [CHANGED] Run tests against newer supported versions of ruby
• [CHANGED] Replace dependency on JWT::SecurityUtils.secure_compare with OpenSSL.fixed_length_secure_compare
• [CHANGED] Support recent versions of ruby-jwt

[oskarpearson]

A note here on ruby-jwt versions: I've tested this with both the beta version of ruby-jwt and recent versions of ruby-jwt (2.10.1, 2.6, 2.5) and they also work.

I've thus relaxed the constraint on ruby-jwt to allow ruby-jwt 3.* when it's been released, so that we don't need to keep re-pinning the supported version through PRs

[oskarpearson]

NOTE to the messagebird reviewers:

PLEASE SET A RELEASE LABEL on this PR before merging. You should be able to find this on the right hand side of the page.

I suggest using release-major to build a new version 5.0 of the gem

Please add the label here:

Why?

If merged correctly, this PR should be able to automatically create Version 5 and publish the new gem

See https://github.com/messagebird/ruby-rest-api/issues/92 for a detailed breakdown of the code and process.

The code in release.yml appears to:

• Get a label from the PR (you'll need to set this)
• Update CHANGELOG based on the text in my original PR description above
• Bump the version number and adjust version.rb based on the label chosen
• Commit CHANGELOG and version.rb changes to the repo

Then, gh-release.yml and release.yml will publish new github and gem releases with these version numbers.

You can see this automation in action with these previous PRs:

• https://github.com/messagebird/ruby-rest-api/pull/74
• https://github.com/messagebird/ruby-rest-api/pull/73

[marcelcorso]

thank you @oskarpearson for the PR!

I have only 1 small comment

[marcelcorso]

@oskarpearson another small request: please add a unit test 🙏 ?

[oskarpearson]

@oskarpearson another small request: please add a unit test 🙏 ?

Thanks for the review, @marcelcorso 🎉

I had a look at this, and encountered a few issues. Wanted to get your opinion on how to proceed.

Unfortunately the code isn't really written in a way that makes it easy to test without lots of mocking or similar tradeoffs.

Option 1 - Add a test for the private secure_compare method

In general I try and avoid testing private methods like secure_compare, and instead focus on the externally-visible behaviour.

Adding a test to ensure secure_compare works as expected would be quite simple and easy, though.

Option 2 - Lots of mocking

If I want to avoid testing the private method, then I need to exercise all parts of the code to ensure that bytesize and OpenSSL.fixed_length_secure_compare are called/not called for the URL and payloads.

This turns out to be "challenging" with the current code. The spec ends up being a mess of interception of methods on JWT and Digest::SHA256 to return mocked objects where I can check that bytesize is run on them.

I've given an example of a single "happy path test" after Option 4 below. Consider how messy this gets if I have to test that bytesize/fixed_length_secure_compare are called for all code paths for all variations of the URL and payload parameters.

Option 3 - Significant refactoring of request_validator.rb to be more testable

Honestly I'd like to avoid this given time constraints. Making it more testable would also likely involve changing it's method signature, which then has knock-on effects on other code, or adding another object under the existing code layer that I can then mock/stub specific methods on.

Option 4 - Don't test all code paths through secure_compare

This is probably my preference.

If I test the 'happy path' where the url and payload are valid and check that the underlying calls to bytesize/OpenSSL.fixed_length_secure_compare are done, it would mean one test that does a significant amount of mocking.

It would still not be very readable, and would likely need to change the mocking if the code in request_validator.rb changes.

Example of 'Happy Path' test

Here's an example of a test of request_validator that ensures bytesize is called for the url and payload values, and that fixed_length_secure_compare is also called for both parameters.

Note that to be able to ensure that .bytesize is called on specific strings (payload_hash and url_hash) I need to mock the request to JWT and ensure it returns something that I've previously been able to set an expectation on. Otherwise JWT constructs its own strings (with their own object IDs) and the rspec expect doesn't trigger.

Similarly, to be able to ensure that .bytesize is called on the output of Digest::SHA256.hexdigest I need to mock those too.

You can see how this starts to get very long if I'm trying to exercise all the codepaths of both MessageBird::RequestValidator#validate_url and MessageBird::RequestValidator#validate_payload as per option 2 above

  describe 'secure string comparison' do
    subject { MessageBird::RequestValidator.new('test_signature_key') }

    let(:url) { 'https://example.com/?example=42' }
    let(:payload) { 'test_payload' }

    let!(:url_hash) { Digest::SHA256.hexdigest(url) }
    let!(:payload_hash) { Digest::SHA256.hexdigest(payload) }

    let(:mocked_jwt_decoded_response) do
      {
        'payload_hash' => payload_hash.dup,
        'url_hash' => url_hash.dup
      }
    end

    it 'compares hashes using secure_compare' do
      expect(JWT).to receive(:decode).with(any_args).and_return(mocked_jwt_decoded_response)

      expect(Digest::SHA256).to receive(:hexdigest).with(url).and_return(url_hash)
      expect(url_hash).to receive(:bytesize).and_call_original
      expect(mocked_jwt_decoded_response['url_hash']).to receive(:bytesize).and_call_original
      expect(OpenSSL).to receive(:fixed_length_secure_compare).with(
        url_hash,
        mocked_jwt_decoded_response['url_hash']
      ).and_call_original

      expect(Digest::SHA256).to receive(:hexdigest).with(payload).and_return(payload_hash)
      expect(payload_hash).to receive(:bytesize).and_call_original
      expect(mocked_jwt_decoded_response['payload_hash']).to receive(:bytesize).and_call_original

      expect(OpenSSL).to receive(:fixed_length_secure_compare).with(
        payload_hash,
        mocked_jwt_decoded_response['payload_hash']
      ).and_call_original

      subject.validate_signature('irrelevant_as_mocked', url, payload)
    end
  end

[oskarpearson]

@marcelcorso Wanted to check in here - we're running this in production without any incidents. Could you let me know what approach you'd like me to take with the tests?

[marcelcorso]

hey @oskarpearson ! It all looks good. We'll merge and release ~tomorrow.

Thank you so much 🙏