From 90863d2cf1fdbacecdb1d61b44f1c53b5f9c0a25 Mon Sep 17 00:00:00 2001
From: Alessandro Berardi <berardialessandro@gmail.com>
Date: Mon, 19 Dec 2016 10:48:51 +1030
Subject: [PATCH] Handle gracefully only JSON parsing errors

---
 CHANGELOG.md                                  |  2 +-
 README.md                                     |  4 ++-
 .../middleware/parameter_parser.rb            | 22 +++++++++++-----
 .../middleware/parameter_parser_spec.rb       | 26 +++++++------------
 4 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ce5c2e1..6534d71 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,7 +8,7 @@ This changelog adheres to [Keep a CHANGELOG](http://keepachangelog.com/).
 - Updates README for usage with Rails
 
 ### Fixed
-- Handles parsing exceptions gracefully
+- Handles JSON parsing exceptions gracefully
 
 ## [0.2.0] - 2016-12-13
 ### Added
diff --git a/README.md b/README.md
index 7d0da2e..0a2b5c0 100644
--- a/README.md
+++ b/README.md
@@ -99,7 +99,9 @@ The arguments for `config.register_parser` are:
 * a parser for the parameters
 * an unparser to convert parameters back to the encoded format
  
-The parser and unparser must be objects that respond to `call` and accept the parameters as an argument (e.g. procs or lambdas).
+The parser and unparser must be objects that respond to `call` and accept the parameters as an argument (e.g. procs or lambdas).  
+The parser should handle parsing exceptions gracefully by returning the arguments. 
+This ensures that sensitive data scanning and masking is applied on the raw parameters.
 
 ## Development
 
diff --git a/lib/sensitive_data_filter/middleware/parameter_parser.rb b/lib/sensitive_data_filter/middleware/parameter_parser.rb
index 9b1c571..1e4aa1c 100644
--- a/lib/sensitive_data_filter/middleware/parameter_parser.rb
+++ b/lib/sensitive_data_filter/middleware/parameter_parser.rb
@@ -27,14 +27,10 @@ def can_parse?(content_type)
 
       def parse(params)
         @parse.call params
-      rescue
-        params
       end
 
       def unparse(params)
         @unparse.call params
-      rescue
-        params
       end
 
       NULL_PARSER = new('', ->(params) { params }, ->(params) { params })
@@ -44,9 +40,23 @@ def unparse(params)
             ->(params) { Rack::Utils.parse_query(params) },
             ->(params) { Rack::Utils.build_query(params) }),
         new('json', # e.g.: 'application/json'
-            ->(params) { JSON.parse(params) },
-            ->(params) { JSON.unparse(params) })
+            ->(params) { JsonParser.new.parse(params) },
+            ->(params) { JsonParser.new.unparse(params) })
       ].freeze
+
+      class JsonParser
+        def parse(params)
+          JSON.parse(params)
+        rescue JSON::ParserError
+          params
+        end
+
+        def unparse(params)
+          JSON.unparse(params)
+        rescue JSON::GeneratorError
+          params
+        end
+      end
     end
   end
 end
diff --git a/spec/sensitive_data/middleware/parameter_parser_spec.rb b/spec/sensitive_data/middleware/parameter_parser_spec.rb
index 8530797..c4c2d50 100644
--- a/spec/sensitive_data/middleware/parameter_parser_spec.rb
+++ b/spec/sensitive_data/middleware/parameter_parser_spec.rb
@@ -30,6 +30,15 @@
     specify { expect(parser).not_to be null_parser }
     specify { expect(parser.parse(parameters)).to eq 'test' => true }
     specify { expect(parser.unparse('test' => true)).to eq parameters }
+
+    context 'when parsing raises exceptions' do
+      specify { expect{parser.parse('not json')}.not_to raise_error }
+      specify { expect(parser.parse('not json')).to eq 'not json' }
+
+      let(:nan) { 0.0/0 }
+      specify { expect{parser.unparse(nan)}.not_to raise_error }
+      specify { expect(parser.unparse(nan)).to be nan }
+    end
   end
 
   describe 'a custom parser' do
@@ -68,23 +77,6 @@
     end
   end
 
-  context 'when parsing raises exceptions' do
-    let(:content_type) { 'application/test' }
-
-    before do
-      parser_class.register_parser(
-        'test',
-        ->(_params) { fail 'Parsing Error' },
-        ->(_params) { fail 'Parsing Error' }
-      )
-    end
-
-    specify { expect{parser.parse('test')}.not_to raise_error }
-    specify { expect(parser.parse('test')).to eq 'test' }
-    specify { expect{parser.unparse('test')}.not_to raise_error }
-    specify { expect(parser.unparse('test')).to eq 'test' }
-  end
-
   context 'when the content type is nil' do
     let(:content_type) { nil }
     specify { expect(parser).to be null_parser }