-
Notifications
You must be signed in to change notification settings - Fork 73
THREESCALE-15318: Update Faraday dependency for enhanced query parameter handling #4330
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.
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
Changes from 1 commit
b76cdec
ef88e9b
5783363
71d10ba
05be635
c79e8f3
4b520b7
4d8c766
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file seems to be totally useless here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, removed it. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| diff --git a/pisoni.gemspec b/pisoni.gemspec | ||
| index 4f62346..fc89442 100644 | ||
| --- a/pisoni.gemspec | ||
| +++ b/pisoni.gemspec | ||
| @@ -19,7 +19,8 @@ Gem::Specification.new do |s| | ||
| s.license = 'Apache-2.0' | ||
|
|
||
| # faraday v2.9.0 removes support for Ruby 2.7, see https://github.com/lostisland/faraday/releases/tag/v2.9.0 | ||
| - s.add_runtime_dependency 'faraday', '~> 2.0', '<= 2.9' | ||
| + # Updated to allow Faraday 2.14.3+ which fixes nested query string vulnerability | ||
| + s.add_runtime_dependency 'faraday', '~> 2.0', '>= 2.14.3' | ||
| s.add_runtime_dependency 'json', '~> 2.7', '>= 2.7.1' | ||
| s.add_runtime_dependency 'injectedlogger', '0.0.13' | ||
| s.add_runtime_dependency 'faraday-net_http_persistent', '~> 2.1' |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good test to make sure the updated faraday works as advertised. But maybe no need to add tests for gems in our project
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! I can remove the test if you prefer. The test serves two purposes:
However, you're right that testing gem behavior isn't really our responsibility - we trust the gem's own tests. Would you prefer I remove it and just rely on the Gemfile constraint?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, remove the test file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, removed. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'test_helper' | ||
|
|
||
| class FaradayNestedParamsTest < ActiveSupport::TestCase | ||
| test 'Faraday handles deeply nested query parameters without stack overflow' do | ||
| # THREESCALE-15318: Test protection against CVE-2026-54297 | ||
| # Faraday versions before 1.10.6 and 2.14.3 are vulnerable to DoS | ||
| # via crafted nested query strings that cause SystemStackError | ||
|
|
||
| # Create a deeply nested query string that would trigger the vulnerability | ||
| # in vulnerable Faraday versions | ||
| nested_depth = 100 | ||
| nested_params = 'a' * nested_depth + '=1' | ||
| nested_depth.times { |i| nested_params = "a[#{nested_params}]" } | ||
|
|
||
| connection = Faraday.new(url: 'http://example.com') do |faraday| | ||
| faraday.adapter :test do |stub| | ||
| stub.get('/test') { |env| [200, {}, 'OK'] } | ||
| end | ||
| end | ||
|
|
||
| # This should not raise SystemStackError in patched versions | ||
| assert_nothing_raised do | ||
| # Try to make a request with deeply nested parameters | ||
| # The fix should handle this gracefully | ||
| connection.get('/test', {deep: nested_params}) | ||
| end | ||
| rescue Faraday::ParamPart::PartLimitError | ||
| # This is expected in newer versions that enforce limits | ||
| # This means the fix is working | ||
| assert true | ||
| end | ||
|
|
||
| test 'oauth2 gem can handle complex query parameters' do | ||
| # THREESCALE-15318: Ensure oauth2 gem works correctly with the updated Faraday | ||
| # oauth2 gem uses Faraday internally, so we need to ensure compatibility | ||
|
|
||
| authentication_provider = FactoryBot.build_stubbed(:authentication_provider) | ||
| authentication = ThreeScale::OAuth2::Client.build_authentication(authentication_provider) | ||
|
|
||
| # This should work without errors | ||
| assert_nothing_raised do | ||
| client = ::OAuth2::Client.new( | ||
| authentication.client_id, | ||
| authentication.client_secret, | ||
| site: 'https://example.com' | ||
| ) | ||
| assert client | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we limit range to 2.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
~> 2.0constraint already limits to 2.x (it's equivalent to ">= 2.0, < 3.0"). Sofaraday '~> 2.0', '>= 2.14.3'means any 2.x version starting from 2.14.3.