Skip to content
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

Jump to bottom

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51a803f
Add a force disable of appsec when using Ruby >= 3.3 with old ffi
y9v Oct 2, 2024
b4f6641
Add guard clause to AppSec::Component#incompatible_ffi_version?
y9v Oct 2, 2024
03f3bc9
Use Gem::Version for comparing Ruby and ffi versions
y9v Oct 7, 2024
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
16 changes: 15 additions & 1 deletion lib/datadog/appsec/component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ module AppSec
class Component
class << self
def build_appsec_component(settings, telemetry:)
return unless settings.respond_to?(:appsec) && settings.appsec.enabled
return if !settings.respond_to?(:appsec) || !settings.appsec.enabled
return if incompatible_ffi_version?

processor = create_processor(settings, telemetry)

Expand All @@ -28,6 +29,19 @@ def build_appsec_component(settings, telemetry:)

private

def incompatible_ffi_version?
ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version
return false unless RUBY_VERSION >= '3.3.0' && ffi_version < '1.16.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you would need to parse the FFI version correctly, string comparison works for Ruby versions only because their components are kept to single digits.

I would also check against 3.3 not 3.3.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added Gem::Version for all version comparisons here: 03f3bc9


Datadog.logger.warn(
'AppSec is not supported in Ruby versions above 3.3.0 when using `ffi` versions older than 1.16.0, ' \
'and will be forcibly disabled due to a memory leak in `ffi`. ' \
'Please upgrade your `ffi` version to 1.16.0 or higher.'
)

true
end

def create_processor(settings, telemetry)
rules = AppSec::Processor::RuleLoader.load_rules(
telemetry: telemetry,
Expand Down
14 changes: 14 additions & 0 deletions spec/datadog/appsec/component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')))
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Gem::Version with a String (not sure this will work correctly in older ruby versions though)

Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down
Loading