-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,20 @@ | |
expect(component).to be_a(described_class) | ||
end | ||
|
||
context 'when using old ffi version with Ruby 3.3.x' do | ||
before do | ||
stub_const('RUBY_VERSION', '3.3.0') | ||
allow(Gem).to receive(:loaded_specs).and_return('ffi' => double(version: Gem::Version.new('1.15.4'))) | ||
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. If you use 1.9.0 for the FFI version the test will fail right? 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. no, it is still green: Gem::Version.new('1.9.0') < '1.16.0'
=> true I think this is because we are comparing 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. We can dismantle version on major, minor and test piece-by-piece (just in case) 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. Yep, https://github.com/rubygems/rubygems/blob/master/lib/rubygems/version.rb#L358, added in rubygems/rubygems@7e0dbb7 2 years ago, which sounds like this functionality is probably not going to exist in Ruby 2.5? 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. rails/rails#47480 conveniently says that the threshold for this feature is Ruby 3.1. |
||
end | ||
|
||
it 'returns a Datadog::AppSec::Component instance with a nil processor' do | ||
expect(Datadog.logger).to receive(:warn) | ||
|
||
component = described_class.build_appsec_component(settings, telemetry: telemetry) | ||
expect(component).to be_nil | ||
end | ||
end | ||
|
||
context 'when processor is ready' do | ||
it 'returns a Datadog::AppSec::Component with a processor instance' do | ||
expect_any_instance_of(Datadog::AppSec::Processor).to receive(:ready?).and_return(true) | ||
|
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.
Not sure how it could be relevant and don't want to over complicate it, but if we check that
Gem.loaded_specs['ffi']
notnil
, what if it'snil
?Then entire
ffi_version
isnil
, right? And it could fail next checkThere 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.
I will still add a guard clause for this, just in case
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.
although I'm not even sure what to do in this case - we don't want to create a processor, and we probably don't want to use the same warning message. Do we want to silently return true in such case?
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.
I think it would be fine to just not warn in this situation and carry on to subsequent code.
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.
I think we can consider it to be "not there" ...
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.
Sorry, I merged to early. Here is a follow-up PR: #3978