From aa1e03b8cded58fd6e660f169277cf66ca4b7eb8 Mon Sep 17 00:00:00 2001 From: Chris Gannon Date: Thu, 7 Nov 2024 10:11:56 +0000 Subject: [PATCH 1/2] ensure filtered params aren't revealed in sql --- .../active_record/log_subscriber.rb | 9 ++++-- test/active_record_test.rb | 28 ++++++++++++++++++ test/dummy/db/test.sqlite3 | Bin 28672 -> 28672 bytes test/test_helper.rb | 7 +++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/lib/rails_semantic_logger/active_record/log_subscriber.rb b/lib/rails_semantic_logger/active_record/log_subscriber.rb index 357303b..46632d8 100644 --- a/lib/rails_semantic_logger/active_record/log_subscriber.rb +++ b/lib/rails_semantic_logger/active_record/log_subscriber.rb @@ -54,8 +54,13 @@ def sql(event) # When multiple values are received for a single bound field, it is converted into an array def add_bind_value(binds, key, value) - key = key.downcase.to_sym unless key.nil? - value = (Array(binds[key]) << value) if binds.key?(key) + key = key.downcase.to_sym unless key.nil? + if Rails.configuration.filter_parameters.include? key + value = "[FILTERED]" + elsif binds.key?(key) + value = (Array(binds[key]) << value) + end + binds[key] = value end diff --git a/test/active_record_test.rb b/test/active_record_test.rb index 0fc3866..7714a18 100644 --- a/test/active_record_test.rb +++ b/test/active_record_test.rb @@ -87,6 +87,34 @@ class ActiveRecordTest < Minitest::Test assert_instance_of Integer, messages[0].payload[:allocations] if Rails.version.to_i >= 6 end + it "filtered bind value" do + filter_params_setting true, %i[name] do + expected_sql = + if Rails.version.to_f >= 5.2 + "SELECT #{extra_space}\"samples\".* FROM \"samples\" WHERE \"samples\".\"name\" = ? ORDER BY \"samples\".\"id\" ASC LIMIT ?" + else + "SELECT \"samples\".* FROM \"samples\" WHERE \"samples\".\"name\" = ? ORDER BY \"samples\".\"id\" ASC LIMIT ?" + end + + messages = semantic_logger_events do + Sample.where(name: "Jack").first + end + assert_equal 1, messages.count, messages + + assert_semantic_logger_event( + messages[0], + level: :debug, + name: "ActiveRecord", + message: "Sample Load", + payload_includes: { + sql: expected_sql, + binds: {name: "[FILTERED]", limit: 1} + } + ) + assert_instance_of Integer, messages[0].payload[:allocations] if Rails.version.to_i >= 6 + end + end + it "multiple bind values" do skip "Not applicable to older rails" if Rails.version.to_f <= 5.1 diff --git a/test/dummy/db/test.sqlite3 b/test/dummy/db/test.sqlite3 index a720ed84c4595109d6ce2ec5c691af3adfe28eea..cebf7e6c7c8ca20e98108c7843cdedb0f3d57bad 100644 GIT binary patch delta 48 zcmZp8z}WDBQ6@OhC$l6~AuYcsH?c&)m_dMniHX5ML4kpRfqkNkGb=lTUR}h-lsWkT DSLO`J delta 48 zcmZp8z}WDBQ6@OhC$l6~AuYcsH?c&)m_dMnk&(ecL4kpRfo-CUGb Date: Thu, 7 Nov 2024 12:09:34 +0000 Subject: [PATCH 2/2] ensure filter params can still be checked when cast to regex --- .../active_record/log_subscriber.rb | 11 +++++++- test/active_record_test.rb | 28 +++++++++++++++++++ test/test_helper.rb | 19 ++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/rails_semantic_logger/active_record/log_subscriber.rb b/lib/rails_semantic_logger/active_record/log_subscriber.rb index 46632d8..5fe56e5 100644 --- a/lib/rails_semantic_logger/active_record/log_subscriber.rb +++ b/lib/rails_semantic_logger/active_record/log_subscriber.rb @@ -55,7 +55,8 @@ def sql(event) # When multiple values are received for a single bound field, it is converted into an array def add_bind_value(binds, key, value) key = key.downcase.to_sym unless key.nil? - if Rails.configuration.filter_parameters.include? key + + if rails_filter_params_include?(key) value = "[FILTERED]" elsif binds.key?(key) value = (Array(binds[key]) << value) @@ -64,6 +65,14 @@ def add_bind_value(binds, key, value) binds[key] = value end + def rails_filter_params_include?(key) + filter_parameters = Rails.configuration.filter_parameters + + return filter_parameters.first.match? key if filter_parameters.first.is_a? Regexp + + filter_parameters.include? key + end + def logger self.class.logger end diff --git a/test/active_record_test.rb b/test/active_record_test.rb index 7714a18..e1b3a0f 100644 --- a/test/active_record_test.rb +++ b/test/active_record_test.rb @@ -115,6 +115,34 @@ class ActiveRecordTest < Minitest::Test end end + it "filtered bind value when filter_parameters set as regex" do + filter_params_regex_setting true, %i[name] do + expected_sql = + if Rails.version.to_f >= 5.2 + "SELECT #{extra_space}\"samples\".* FROM \"samples\" WHERE \"samples\".\"name\" = ? ORDER BY \"samples\".\"id\" ASC LIMIT ?" + else + "SELECT \"samples\".* FROM \"samples\" WHERE \"samples\".\"name\" = ? ORDER BY \"samples\".\"id\" ASC LIMIT ?" + end + + messages = semantic_logger_events do + Sample.where(name: "Jack").first + end + assert_equal 1, messages.count, messages + + assert_semantic_logger_event( + messages[0], + level: :debug, + name: "ActiveRecord", + message: "Sample Load", + payload_includes: { + sql: expected_sql, + binds: {name: "[FILTERED]", limit: 1} + } + ) + assert_instance_of Integer, messages[0].payload[:allocations] if Rails.version.to_i >= 6 + end + end + it "multiple bind values" do skip "Not applicable to older rails" if Rails.version.to_f <= 5.1 diff --git a/test/test_helper.rb b/test/test_helper.rb index 7474ec1..39cb10c 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -25,8 +25,25 @@ ActionMailer::Base.delivery_method = :test def filter_params_setting(value, user_defined_params, &block) + original_value = Rails.configuration.filter_parameters + Rails.configuration.filter_parameters = user_defined_params + block.call +ensure + Rails.configuration.filter_parameters = original_value +end + +def filter_params_regex_setting(value, user_defined_params, &block) + original_value = Rails.configuration.filter_parameters + Rails.configuration.filter_parameters += user_defined_params + + filter_params_regex = Rails.configuration.filter_parameters.map do |key| + "(?i:#{key})" + end.join("|") + + Rails.configuration.filter_parameters = [/(?-mix:#{filter_params_regex})/] + block.call ensure - Rails.configuration.filter_parameters -= user_defined_params + Rails.configuration.filter_parameters = original_value end