Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ jobs:

strategy:
matrix:
ruby-version: [ "3.0.3", "3.1.1" ]
ruby-version: [ "3.0.7", "3.1.6", "3.2.6", "3.3.7", "3.4.1" ]

steps:
- uses: actions/checkout@v2

Expand Down
14 changes: 12 additions & 2 deletions lib/messagebird/request_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ def decode_signature(signature)

def validate_url(url, url_hash)
expected_url_hash = Digest::SHA256.hexdigest url
unless JWT::SecurityUtils.secure_compare(expected_url_hash, url_hash)
unless secure_compare(expected_url_hash, url_hash)
raise ValidationError, 'invalid jwt: claim url_hash is invalid'
end
end

def validate_payload(body, payload_hash)
if !body.to_s.empty? && !payload_hash.to_s.empty?
unless JWT::SecurityUtils.secure_compare(Digest::SHA256.hexdigest(body), payload_hash)
unless secure_compare(Digest::SHA256.hexdigest(body), payload_hash)
raise ValidationError, 'invalid jwt: claim payload_hash is invalid'
end
elsif !body.to_s.empty?
Expand All @@ -87,5 +87,15 @@ def validate_payload(body, payload_hash)
raise ValidationError, 'invalid jwt: claim payload_hash is set but actual payload is missing'
end
end

# Adaption of https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/activesupport/lib/active_support/security_utils.rb#L33
# Copied here so as to avoid adding a dependency on ActiveSupport to this gem
#
# Note that unlike `fixed_length_secure_compare` in the above url 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
# https://github.com/ruby/openssl/blob/master/History.md#version-220 which is included in Ruby 3.0 and above
def secure_compare(first, second)
first.bytesize == second.bytesize && OpenSSL.fixed_length_secure_compare(first, second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we missing

require "openssl"

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottom Line Up Front

Super happy to add it to do that and make the requirement explicit, but it works without it because of the below.

I'm aware this PR is already getting quite large - so let me know if you'd like to do this.

How do I know it'll work?

There's existing similar code in signed_request.rb that uses OpenSSL without the need for require 'openssl'

Why does it work?

My understanding is that this is because all recent versions of Ruby's https://ruby-doc.org/stdlib-2.6.1/libdoc/net/http/rdoc/Net/HTTP.html library depend on OpenSSL (search on that page for OpenSSL and you'll see references to OpenSSL specific errors and symbols)

So, by using ruby's net-related code, it loads OpenSSL. The existing codebase explicitly loads net/https (which then, in turn, loads OpenSSL) in a few places.

The most key of these are lib/messagebird.rb loading lib/messagebird/client.rb:

ruby-rest-api/lib/messagebird.rb

Line 8 in 64bcb3f

require 'messagebird/client'

That, in turn, then requires net/https:

ruby-rest-api/lib/messagebird/client.rb

Line 4 in 64bcb3f

require 'net/https'

Verification

As a test I ran irb without bundle exec and tried to access the OpenSSL namespace:

irb(main):001> OpenSSL
(irb):1:in `<main>': uninitialized constant OpenSSL (NameError)

OpenSSL
^^^^^^^
        from <internal:kernel>:187:in `loop'
        from .../versions/3.3.7/lib/ruby/gems/3.3.0/gems/irb-1.15.1/exe/irb:9:in `<top (required)>'
        from .../.rbenv/versions/3.3.7/bin/irb:25:in `load'
        from .../.rbenv/versions/3.3.7/bin/irb:25:in `<main>'

I then loaded net/https and it loaded OpenSSL

irb(main):002> require 'net/https'
=> true
irb(main):003> OpenSSL
=> OpenSSL
irb(main):004>

As I mentioned, OpenSSL is being implicitly loaded at the moment. I'm happy to change that, but am aware that either I need to make this code inconsistent with signed_request.rb, or I need to make changes there too, or I need to add a require for OpenSSL in lib/messagebird.rb

Let me know what you think

end
end
end
5 changes: 4 additions & 1 deletion messagebird-rest.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ Gem::Specification.new do |s|
s.files = Dir.glob('lib/**/*') + %w(LICENSE README.md)
s.require_path = 'lib'

s.add_dependency "jwt", "~> 2.3"
# This code works with at least version 3.0.0.beta1 of jwt,
# so we are supporting up to version 4 to help reduce
# the necessity for future version bumps
s.add_dependency "jwt", "< 4"

s.add_development_dependency "rspec", "~> 3.11.0"
s.add_development_dependency "rubocop", "~> 1.26.1"
Expand Down