[PLAT-319] Configurable Redacted Keywords (#27)

* Add configurable secret keywords

* add proper testing tools, add test for adding different types of sensitive keywords

* wip

* Add `sensitive_keywords` info to documentation

* Remove extra args

* Fix tests

* Remove .github dir from gitignore

* Reintroduce replace_keys args

It is used in another context with a custom value to redact headers

* Testing nested keys

Co-authored-by: tiago-macedo <tiago.macedo@pier.digital>
Co-authored-by: Leonardo Bighetti <leonardo.bighetti@gmail.com>

.gitignore

@@ -6,4 +6,5 @@
 /pkg/
 /spec/reports/
 /tmp/
-*.gem
+*.gem
+.byebug_history

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    pier_logging (0.3.1)
+    pier_logging (0.3.3)
       awesome_print
       facets
       ougai
@@ -90,11 +90,12 @@ GEM
     mini_mime (1.0.3)
     mini_portile2 (2.5.1)
     minitest (5.14.4)
+    mocha (1.12.0)
     nio4r (2.5.7)
     nokogiri (1.11.5)
       mini_portile2 (~> 2.5.0)
       racc (~> 1.4)
-    oj (3.11.3)
+    oj (3.11.5)
     ougai (2.0.0)
       oj (~> 3.10)
     racc (1.5.2)
@@ -128,6 +129,12 @@ GEM
       rake (>= 0.8.7)
       thor (~> 1.0)
     rake (13.0.3)
+    shoulda (4.0.0.rc2)
+      shoulda-context (= 2.0.0.rc4)
+      shoulda-matchers (~> 4.0)
+    shoulda-context (2.0.0.rc4)
+    shoulda-matchers (4.5.1)
+      activesupport (>= 4.2.0)
     sprockets (4.0.2)
       concurrent-ruby (~> 1.0)
       rack (> 1, < 3)
@@ -150,8 +157,10 @@ DEPENDENCIES
   bundler (>= 2.1.4)
   byebug (>= 11.1.3)
   minitest (>= 5.8.4)
+  mocha
   pier_logging!
   rake (>= 12.3.3)
+  shoulda (= 4.0.0.rc2)
 
 BUNDLED WITH
    2.1.4

README.md

@@ -42,6 +42,7 @@ Use `PierLogging.configure_request_logger` block to configure request logs. Acce
 | ---------------- | --------:| ---------------:| ----------:|
 | enabled          | false    | boolean         | false      |
 | user_info_getter | true     | block (headers) | nil        |
+| sensitive_keywords | false | array of symbols, strings or regexps | `REDACT_REPLACE_KEYS` in request_logger.rb
 
 The block passed to `user_info_getter` receives the headers of the request so you can use your headers to define the username or role. 
 

lib/pier_logging.rb

@@ -41,7 +41,7 @@ def app_name=(app_name)
       raise ArgumentError, "Config 'app_name' must be a String" unless app_name.is_a?(String)
       @app_name = app_name
     end
-    
+
     def env=(env)
       raise ArgumentError, "Config 'env' must be a String" unless env.is_a?(String)
       @env = env
@@ -54,8 +54,8 @@ def formatter=(formatter)
   end
 
   class RequestLoggerConfiguration
-    attr_reader :enabled, :user_info_getter, :hide_request_body_for_paths, :hide_response_body_for_paths, 
-                :log_request_body, :log_response, :hide_request_headers, :correlation_id_getter
+    attr_reader :enabled, :user_info_getter, :hide_request_body_for_paths, :hide_response_body_for_paths,
+                :log_request_body, :log_response, :hide_request_headers, :correlation_id_getter, :sensitive_keywords
 
     def initialize
       @user_info_getter = ->(_ = nil) { nil }
@@ -66,6 +66,7 @@ def initialize
       @log_response = true
       @hide_request_headers = nil
       @correlation_id_getter = ->(_ = nil, _ = nil) { nil }
+      @sensitive_keywords = []
     end
 
     def user_info_getter=(proc)
@@ -85,27 +86,27 @@ def log_response=(log_response)
 
     def hide_request_body_for_paths=(hide_request_body_for_paths)
       unless (hide_request_body_for_paths.is_a? Array) && (hide_request_body_for_paths.all?{|item| item.is_a? Regexp})
-        raise ArgumentError, "Config 'hide_request_body_for_paths' must be an 'Array of Regexps'" 
+        raise ArgumentError, "Config 'hide_request_body_for_paths' must be an 'Array of Regexps'"
       end
-      
+
       @hide_request_body_for_paths = hide_request_body_for_paths
     end
 
     def hide_response_body_for_paths=(hide_response_body_for_paths)
       unless (hide_response_body_for_paths.is_a? Array) && (hide_response_body_for_paths.all?{|item| item.is_a? Regexp})
-        raise ArgumentError, "Config 'hide_response_body_for_paths' must be an 'Array of Regexps'" 
+        raise ArgumentError, "Config 'hide_response_body_for_paths' must be an 'Array of Regexps'"
       end
-      
+
       @hide_response_body_for_paths = hide_response_body_for_paths
     end
 
     def hide_request_headers=(hide_request_headers)
       unless (hide_request_headers.is_a? Array) && (hide_request_headers.all?{|item| item.is_a? Regexp})
-        raise ArgumentError, "Config 'hide_request_headers' must be an 'Array of Regexps'" 
+        raise ArgumentError, "Config 'hide_request_headers' must be an 'Array of Regexps'"
       end
       @hide_request_headers = hide_request_headers
     end
-    
+
     def enabled=(enabled = false)
       raise ArgumentError, "Config 'enabled' must be a 'boolean'" unless !!enabled == enabled
       @enabled = enabled
@@ -115,5 +116,16 @@ def correlation_id_getter=(proc)
       raise ArgumentError, "Config 'correlation_id_getter' must be a 'Proc'" unless proc.is_a? Proc
       @correlation_id_getter = proc
     end
+
+    def sensitive_keywords=(keywords)
+      keywords.map! do |kw|
+        if kw.is_a? Regexp
+          kw
+        else
+          Regexp.new(kw.to_s)
+        end
+      end
+      @sensitive_keywords += keywords
+    end
   end
 end

lib/pier_logging/request_logger.rb

@@ -13,7 +13,7 @@ class RequestLogger
       /^connect\.sid$/
     ].freeze
     REDACT_REPLACE_BY = '*'.freeze
-    
+
     attr_reader :logger
 
     def initialize(app, logger = PierLogging::Logger.new(STDOUT))
@@ -69,15 +69,15 @@ def log(args)
       # We should never fall in this part as the only errors that could result in this are errors
       # in our logger (inside this same method)
       @logger.error(error.message)
-    end 
+    end
 
     private
     def get_request_headers_from_env(env)
       hide_request_headers = PierLogging.request_logger_configuration.hide_request_headers
-      
+
       headers = env.select { |k,v| k[0..4] == 'HTTP_'}.
         transform_keys { |k| k[5..-1].split('_').join('-').upcase }
-      
+
       return redact_object(headers, hide_request_headers, nil) if hide_request_headers.present?
 
       headers
@@ -91,19 +91,19 @@ def request_body(request_path, body)
 
       parse_body(body)
     end
-    
+
     def response_body(request_path, body)
       return nil unless PierLogging.request_logger_configuration.log_response
-      
+
       hide_response_body_for_paths = PierLogging.request_logger_configuration.hide_response_body_for_paths
       return nil if hide_response_body_for_paths&.any?{ |path|request_path =~ path }
-      
+
       parse_body(body)
     end
 
     def build_message_from_request(request)
       [
-        request.request_method.upcase, 
+        request.request_method.upcase,
         [request.base_url,request.path].join(''),
       ].join(' ')
     end
@@ -129,7 +129,12 @@ def get_body_object(body)
       body
     end
 
-    def redact_object(obj, replace_keys = REDACT_REPLACE_KEYS, replace_by = REDACT_REPLACE_BY)
+    def sensitive_keywords
+      REDACT_REPLACE_KEYS + PierLogging.request_logger_configuration.sensitive_keywords
+    end
+
+    def redact_object(obj, replace_keys = nil, replace_by = REDACT_REPLACE_BY)
+      replace_keys ||= sensitive_keywords
       if obj === Array
         redact_array(obj, replace_keys, replace_by)
       elsif obj === Hash
@@ -140,22 +145,22 @@ def redact_object(obj, replace_keys = REDACT_REPLACE_KEYS, replace_by = REDACT_R
         obj
       end
     end
-    
-    def redact_array(arr, replace_keys = REDACT_REPLACE_KEYS, replace_by = REDACT_REPLACE_BY)
+
+    def redact_array(arr, replace_keys, replace_by = REDACT_REPLACE_BY)
       raise StandardError, 'Could not redact_array for non-array objects' unless arr.is_a? Array
       arr.map { |el| redact_object(el, replace_keys, replace_by) }
     end
-    
-    def redact_hash(hash, replace_keys = REDACT_REPLACE_KEYS, replace_by = REDACT_REPLACE_BY)
+
+    def redact_hash(hash, replace_keys, replace_by = REDACT_REPLACE_BY)
       raise StandardError, 'Could not redact_hash for non-hash objects' unless hash.is_a? Hash
       hash.traverse do |k,v|
-        should_redact = replace_keys.any?{ |regex| k =~regex }
+        should_redact = replace_keys.any?{ |regex| k =~ regex }
         if (should_redact)
           [k, replace_by]
         else
           case v
           when Array then [k, redact_array(v, replace_keys, replace_by)]
-          else 
+          else
             [k, v]
           end
         end

pier_logging.gemspec

@@ -28,6 +28,8 @@ Gem::Specification.new do |spec|
   spec.add_dependency "rails"
   spec.add_dependency "facets"
 
+  spec.add_development_dependency "shoulda", "4.0.0.rc2"
+  spec.add_development_dependency "mocha"
   spec.add_development_dependency "bundler", ">= 2.1.4"
   spec.add_development_dependency "rake", ">= 12.3.3"
   spec.add_development_dependency "minitest", ">= 5.8.4"

test/pier_logging/request_logger_configuration_test.rb

@@ -0,0 +1,25 @@
+require "test_helper"
+
+class PierLogging::RequestLoggerConfigurationTest < Minitest::Test
+  subject { PierLogging::RequestLoggerConfiguration.new }
+
+  context "#sensitive_keywords" do
+    should 'transform strings to regexps when adding them' do
+      keyword = "blah"
+      subject.sensitive_keywords = [keyword]
+      assert_equal Regexp.new(keyword), subject.sensitive_keywords.first
+    end
+
+    should 'transform symbols to regexps when adding them' do
+      keyword = :blah
+      subject.sensitive_keywords = [keyword]
+      assert_equal Regexp.new(keyword.to_s), subject.sensitive_keywords.first
+    end
+
+    should 'be able to add regexps' do
+      keyword = /blah/
+      subject.sensitive_keywords = [keyword]
+      assert_equal keyword, subject.sensitive_keywords.first
+    end
+  end
+end

test/pier_logging/request_logger_test.rb

@@ -0,0 +1,48 @@
+require "test_helper"
+
+class PierLogging::RequestLoggerTest < Minitest::Test
+  subject { PierLogging::RequestLogger.new(mock, @logger) }
+
+  context "#log" do
+    setup do
+      PierLogging.request_logger_configuration.sensitive_keywords = [:blah]
+      PierLogging.request_logger_configuration.enabled = true
+      @logger = PierLogging::Logger.new($stdout)
+      @env_mock = Rack::MockRequest.env_for
+      body = {blah: "foo", bluh: "plaft"}
+      @args = [@env_mock, "status", {"Content-type": "12"}, body, Time.now, Time.now]
+    end
+
+    teardown do
+      PierLogging.request_logger_configuration.enabled = false
+    end
+
+    context 'when sensitive keyword is at root' do
+      should "redact sensitive keywords" do
+        @logger.expects(:info).with do |logged_args|
+          redacted_body = logged_args[:response][:body]
+          assert_equal "*", redacted_body[:blah]
+          assert_equal "plaft", redacted_body[:bluh]
+        end
+
+        subject.log(@args)
+      end
+    end
+
+    context 'when sensitive keyword is nested in array' do
+      setup do
+        body = {blirgh: [{blah: "foo"}, {bluh: "plaft"}]}
+        @args = [@env_mock, "status", {"Content-type": "12"}, body, Time.now, Time.now]
+      end
+      should "redact sensitve keywords" do
+        @logger.expects(:info).with do |logged_args|
+          redacted_body = logged_args[:response][:body]
+          assert_equal "*", redacted_body[:blirgh][0][:blah]
+          assert_equal "plaft", redacted_body[:blirgh][1][:bluh]
+        end
+
+        subject.log(@args)
+      end
+    end
+  end
+end

test/pier_logging_test.rb

@@ -5,7 +5,4 @@ def test_that_it_has_a_version_number
     refute_nil ::PierLogging::VERSION
   end
 
-  def test_it_does_something_useful
-    assert false
-  end
 end

test/test_helper.rb

@@ -2,3 +2,12 @@
 require "pier_logging"
 
 require "minitest/autorun"
+require 'mocha/minitest'
+require 'shoulda'
+require 'byebug'
+
+Shoulda::Matchers.configure do |config|
+  config.integrate do |with|
+    with.test_framework :minitest
+  end
+end