From 6f0114d49ecd035718f50b3c236bf6d42439a337 Mon Sep 17 00:00:00 2001 From: John Mortlock Date: Tue, 16 Jan 2018 14:20:25 +1030 Subject: [PATCH 1/5] Ensure we mutate the env variable rather than copy it --- lib/sensitive_data_filter/middleware.rb | 3 +- .../middleware/detect.rb | 28 +++++++++ .../middleware/env_filter.rb | 39 ------------ .../middleware/env_parser.rb | 11 ++-- .../middleware/filter.rb | 16 +++-- .../middleware/occurrence.rb | 18 +++--- .../middleware/env_filter_spec.rb | 63 ------------------- .../middleware/env_parser_spec.rb | 38 +++-------- .../middleware/filter_spec.rb | 45 +++++++++---- .../middleware/occurrence_spec.rb | 25 ++++---- 10 files changed, 110 insertions(+), 176 deletions(-) create mode 100644 lib/sensitive_data_filter/middleware/detect.rb delete mode 100644 lib/sensitive_data_filter/middleware/env_filter.rb delete mode 100644 spec/sensitive_data_filter/middleware/env_filter_spec.rb diff --git a/lib/sensitive_data_filter/middleware.rb b/lib/sensitive_data_filter/middleware.rb index dc749d0..d0f654e 100644 --- a/lib/sensitive_data_filter/middleware.rb +++ b/lib/sensitive_data_filter/middleware.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true module SensitiveDataFilter module Middleware + FILTERABLE = %i(query_params body_params).freeze end end require 'sensitive_data_filter/middleware/parameter_parser' require 'sensitive_data_filter/middleware/env_parser' require 'sensitive_data_filter/middleware/occurrence' -require 'sensitive_data_filter/middleware/env_filter' +require 'sensitive_data_filter/middleware/detect' require 'sensitive_data_filter/middleware/filter' diff --git a/lib/sensitive_data_filter/middleware/detect.rb b/lib/sensitive_data_filter/middleware/detect.rb new file mode 100644 index 0000000..c166516 --- /dev/null +++ b/lib/sensitive_data_filter/middleware/detect.rb @@ -0,0 +1,28 @@ +module SensitiveDataFilter + module Middleware + class Detect + def initialize(filter) + @filter = filter + end + + def call + changeset = nil + scan = run_scan + if scan.matches? + changeset = OpenStruct.new(SensitiveDataFilter::Middleware::FILTERABLE.each_with_object({}) { |filterable, hash| + hash[filterable.to_s] = SensitiveDataFilter::Mask.mask(@filter.send(filterable)) + }) + end + [changeset, scan] + end + + private + + def run_scan + SensitiveDataFilter::Scan.new( + SensitiveDataFilter::Middleware::FILTERABLE.map { |filterable| @filter.send(filterable) } + ) + end + end + end +end diff --git a/lib/sensitive_data_filter/middleware/env_filter.rb b/lib/sensitive_data_filter/middleware/env_filter.rb deleted file mode 100644 index 7a05b31..0000000 --- a/lib/sensitive_data_filter/middleware/env_filter.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true -require 'facets/kernel/present' - -module SensitiveDataFilter - module Middleware - class EnvFilter - attr_reader :occurrence - - def initialize(env) - @original_env_parser = EnvParser.new(env) - @filtered_env_parser = @original_env_parser.copy - @scan = build_scan - @filtered_env_parser.mask! if @scan.matches? - @occurrence = build_occurrence - end - - def filtered_env - @filtered_env_parser.env - end - - def occurrence? - @occurrence.present? - end - - private - - def build_occurrence - return nil unless @scan.matches? - Occurrence.new(@original_env_parser, @filtered_env_parser, @scan.matches) - end - - def build_scan - SensitiveDataFilter::Scan.new( - [@original_env_parser.query_params, @original_env_parser.body_params] - ) - end - end - end -end diff --git a/lib/sensitive_data_filter/middleware/env_parser.rb b/lib/sensitive_data_filter/middleware/env_parser.rb index 0923b6e..9ab1994 100644 --- a/lib/sensitive_data_filter/middleware/env_parser.rb +++ b/lib/sensitive_data_filter/middleware/env_parser.rb @@ -36,13 +36,10 @@ def body_params=(new_params) @env[RACK_INPUT] = StringIO.new @parameter_parser.unparse(new_params) end - def copy - self.class.new(@env.clone) - end - - def mask! - self.query_params = SensitiveDataFilter::Mask.mask(query_params) - self.body_params = SensitiveDataFilter::Mask.mask(body_params) + def mutate(mutation) + SensitiveDataFilter::Middleware::FILTERABLE.each do |filterable| + self.send("#{filterable}=", mutation.send(filterable)) + end end def_delegators :@request, :ip, :request_method, :url, :content_type, :session diff --git a/lib/sensitive_data_filter/middleware/filter.rb b/lib/sensitive_data_filter/middleware/filter.rb index 4cf520d..abf4c8f 100644 --- a/lib/sensitive_data_filter/middleware/filter.rb +++ b/lib/sensitive_data_filter/middleware/filter.rb @@ -7,16 +7,20 @@ def initialize(app) end def call(env) - env_filter = EnvFilter.new env - handle_occurrence env_filter - @app.call env_filter.filtered_env + original_env = EnvParser.new(env) + changeset, scan = Detect.new(original_env).call + unless changeset.nil? + handle_occurrence(original_env, changeset, scan) + original_env.mutate(changeset) + end + @app.call(env) end private - def handle_occurrence(env_filter) - return unless env_filter.occurrence? - SensitiveDataFilter.handle_occurrence env_filter.occurrence + def handle_occurrence(filter, changeset, scan) + occurence = Occurrence.new(filter, changeset, scan.matches) + SensitiveDataFilter.handle_occurrence(occurence) end end end diff --git a/lib/sensitive_data_filter/middleware/occurrence.rb b/lib/sensitive_data_filter/middleware/occurrence.rb index d119086..e7a0d28 100644 --- a/lib/sensitive_data_filter/middleware/occurrence.rb +++ b/lib/sensitive_data_filter/middleware/occurrence.rb @@ -9,9 +9,9 @@ class Occurrence attr_reader :matches - def initialize(original_env_parser, filtered_env_parser, matches) + def initialize(original_env_parser, changeset, matches) @original_env_parser = original_env_parser - @filtered_env_parser = filtered_env_parser + @changeset = changeset @matches = matches end @@ -28,22 +28,26 @@ def original_body_params end def filtered_query_params - @filtered_env_parser.query_params + @changeset.query_params end def filtered_body_params - @filtered_env_parser.body_params + @changeset.body_params + end + + def changeset + @changeset end def original_env @original_env_parser.env end - def filtered_env - @filtered_env_parser.env + def url + SensitiveDataFilter::Mask.mask(@original_env_parser.url) end - def_delegators :@filtered_env_parser, :request_method, :url, :content_type, :session + def_delegators :@original_env_parser, :request_method, :content_type, :session def matches_count @matches.map { |type, matches| [type, matches.count] }.to_h diff --git a/spec/sensitive_data_filter/middleware/env_filter_spec.rb b/spec/sensitive_data_filter/middleware/env_filter_spec.rb deleted file mode 100644 index b7e199d..0000000 --- a/spec/sensitive_data_filter/middleware/env_filter_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true -# rubocop:disable Style/BlockDelimiters -require 'spec_helper' - -require 'sensitive_data_filter/middleware/env_filter' - -describe SensitiveDataFilter::Middleware::EnvFilter do - let(:env_parser_class) { double } - let(:env_parser) { - double 'original_env_parser', query_params: query_params, body_params: body_params - } - let(:query_params) { double } - let(:body_params) { double } - let(:env_parser_copy) { double 'filtered_env_parser', env: filtered_env } - let(:filtered_env) { double 'filtered_env' } - - let(:scan_class) { double } - let(:scan) { double matches?: matches?, matches: scan_matches } - let(:scan_matches) { double } - - let(:parameter_masker_class) { double } - let(:parameter_masker) { double } - - let(:occurrence_class) { double } - let(:occurrence) { double } - - let(:env) { double } - let(:env_filter) { SensitiveDataFilter::Middleware::EnvFilter.new(env) } - - before do - stub_const 'SensitiveDataFilter::Middleware::EnvParser', env_parser_class - allow(env_parser_class).to receive(:new).with(env).and_return env_parser - allow(env_parser).to receive(:copy).and_return env_parser_copy - - allow(env_parser_copy).to receive(:mask!) - - stub_const 'SensitiveDataFilter::Scan', scan_class - allow(scan_class) - .to receive(:new).with([env_parser.query_params, env_parser.body_params]).and_return scan - - stub_const 'SensitiveDataFilter::Middleware::Occurrence', occurrence_class - allow(occurrence_class) - .to receive(:new).with(env_parser, env_parser_copy, scan_matches).and_return occurrence - - env_filter - end - - context 'when sensitive data is detected' do - let(:matches?) { true } - specify { expect(env_parser_copy).to have_received :mask! } - specify { expect(env_filter.occurrence?).to be true } - specify { expect(env_filter.occurrence).to eq occurrence } - specify { expect(env_filter.filtered_env).to eq filtered_env } - end - - context 'when sensitive data is not detected' do - let(:matches?) { false } - specify { expect(env_parser_copy).not_to have_received :mask! } - specify { expect(env_filter.occurrence?).to be false } - specify { expect(env_filter.occurrence).to be_nil } - specify { expect(env_filter.filtered_env).to eq filtered_env } - end -end diff --git a/spec/sensitive_data_filter/middleware/env_parser_spec.rb b/spec/sensitive_data_filter/middleware/env_parser_spec.rb index 1821c61..f428e29 100644 --- a/spec/sensitive_data_filter/middleware/env_parser_spec.rb +++ b/spec/sensitive_data_filter/middleware/env_parser_spec.rb @@ -114,25 +114,7 @@ specify { expect(env_parser.session).to eq 'session_id' => '01ab02cd' } end - describe '#copy' do - let(:masked_env_parser) { env_parser.copy } - - before do - masked_env_parser.query_params = { id: 2 } - masked_env_parser.body_params = { test: 2 } - - env_parser.query_params = { id: 1 } - env_parser.body_params = { test: 1 } - end - - specify { expect(env_parser.query_params).to eq 'id' => '1' } - specify { expect(env_parser.body_params).to eq 'test' => 1 } - - specify { expect(masked_env_parser.query_params).to eq 'id' => '2' } - specify { expect(masked_env_parser.body_params).to eq 'test' => 2 } - end - - describe '#mask!' do + describe '#mutate!' do let(:query_params) { { 'sensitive_query' => 'sensitive_data' } } let(:body_params) { { 'sensitive_body' => 'sensitive_data' } } @@ -141,25 +123,25 @@ env_parser.body_params = { sensitive_body: 'sensitive_data' } end - context 'before masking' do + context 'before mutation' do specify { expect(env_parser.query_params).to eq 'sensitive_query' => 'sensitive_data' } specify { expect(env_parser.body_params).to eq 'sensitive_body' => 'sensitive_data' } end - context 'after masking' do - let(:mask) { double } + context 'after mutation' do let(:filtered_query_params) { { 'sensitive_query' => '[FILTERED]' } } let(:filtered_body_params) { { 'sensitive_body' => '[FILTERED]' } } + let(:changeset) { + double( + query_params: filtered_query_params, + body_params: filtered_body_params + ) + } before do - stub_const 'SensitiveDataFilter::Mask', mask - allow(mask).to receive(:mask).with(query_params).and_return filtered_query_params - allow(mask).to receive(:mask).with(body_params).and_return filtered_body_params - env_parser.mask! + env_parser.mutate(changeset) end - specify { expect(mask).to have_received(:mask).with query_params } - specify { expect(mask).to have_received(:mask).with body_params } specify { expect(env_parser.query_params).to eq filtered_query_params } specify { expect(env_parser.body_params).to eq filtered_body_params } end diff --git a/spec/sensitive_data_filter/middleware/filter_spec.rb b/spec/sensitive_data_filter/middleware/filter_spec.rb index aba4ed2..13f574d 100644 --- a/spec/sensitive_data_filter/middleware/filter_spec.rb +++ b/spec/sensitive_data_filter/middleware/filter_spec.rb @@ -5,35 +5,54 @@ require 'sensitive_data_filter/middleware/filter' describe SensitiveDataFilter::Middleware::Filter do - let(:env_filter_class) { double } - let(:env_filter) { - double occurrence?: occurrence?, occurrence: occurrence, filtered_env: filtered_env + let(:env_parser_class) { double } + let(:env_parser) { + double 'EnvParser' } - let(:occurrence) { double } + let(:occurrence) { double 'Occurrence' } + let(:occurrence_class) { double } let(:filtered_env) { double 'filtered_env' } - let(:app) { double } + let(:app) { double 'App' } let(:middleware) { SensitiveDataFilter::Middleware::Filter } let(:stack) { middleware.new(app) } - let(:env) { double } + let(:env) { double 'Env' } + + let(:detect) { double } + let(:detect_class) { double } + + let(:scan) { double('Scam', matches: []) } + let(:changeset) { double('Changeset') } + before do - stub_const 'SensitiveDataFilter::Middleware::EnvFilter', env_filter_class - allow(env_filter_class).to receive(:new).with(env).and_return env_filter + stub_const 'SensitiveDataFilter::Middleware::Detect', detect_class + stub_const 'SensitiveDataFilter::Middleware::EnvParser', env_parser_class + allow(SensitiveDataFilter).to receive(:handle_occurrence).with occurrence - allow(app).to receive(:call).with filtered_env + allow(env_parser_class).to receive(:new).and_return(env_parser) + allow(detect_class).to receive(:new).with(env_parser).and_return detect + allow(detect).to receive(:call).and_return [changeset, scan] + + allow(env_parser).to receive(:mutate).with(changeset) + + stub_const 'SensitiveDataFilter::Middleware::Occurrence', occurrence_class + allow(occurrence_class) + .to receive(:new).with(env_parser, changeset, scan.matches).and_return occurrence + + allow(app).to receive(:call).with env stack.call(env) end context 'when an occurrence is detected' do - let(:occurrence?) { true } + let(:changeset) { double } specify { expect(SensitiveDataFilter).to have_received(:handle_occurrence).with occurrence } - specify { expect(app).to have_received(:call).with filtered_env } + specify { expect(app).to have_received(:call).with env } end context 'when sensitive data is detected' do - let(:occurrence?) { false } + let(:changeset) { nil } specify { expect(SensitiveDataFilter).not_to have_received(:handle_occurrence) } - specify { expect(app).to have_received(:call).with filtered_env } + specify { expect(app).to have_received(:call).with env } end end diff --git a/spec/sensitive_data_filter/middleware/occurrence_spec.rb b/spec/sensitive_data_filter/middleware/occurrence_spec.rb index 2d61dea..e82205a 100644 --- a/spec/sensitive_data_filter/middleware/occurrence_spec.rb +++ b/spec/sensitive_data_filter/middleware/occurrence_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # rubocop:disable Style/BlockDelimiters require 'spec_helper' - +require 'rack/mock' require 'sensitive_data_filter/middleware/occurrence' describe SensitiveDataFilter::Middleware::Occurrence do @@ -16,7 +16,7 @@ let(:filtered_body_params) { { credit_cards: '[FILTERED] and [FILTERED]' } } let(:session) { { 'session_id' => '01ab02cd' } } let(:original_env) { double } - let(:filtered_env) { double } + let(:changeset) { double } let(:original_env_parser) { double( ip: ip, @@ -29,16 +29,10 @@ env: original_env ) } - let(:filtered_env_parser) { + let(:changeset) { double( - ip: ip, - request_method: request_method, - url: filtered_url, - content_type: content_type, query_params: filtered_query_params, - body_params: filtered_body_params, - session: session, - env: filtered_env + body_params: filtered_body_params ) } let(:matches) { @@ -47,10 +41,17 @@ } } let(:matches_count) { { 'CreditCard' => 2 } } + + let(:mask_class) { double } + before do + stub_const('SensitiveDataFilter::Mask', mask_class) + allow(mask_class).to receive(:mask).and_return(filtered_url) + end + subject(:occurrence) { SensitiveDataFilter::Middleware::Occurrence.new( original_env_parser, - filtered_env_parser, + changeset, matches ) } @@ -66,7 +67,7 @@ specify { expect(occurrence.session).to eq session } specify { expect(occurrence.matches_count).to eq matches_count } specify { expect(occurrence.original_env).to eq original_env } - specify { expect(occurrence.filtered_env).to eq filtered_env } + specify { expect(occurrence.changeset).to eq changeset } let(:expected_to_h) { { From 0ee654e473e9cabc405646e62e8737a46c2e0f71 Mon Sep 17 00:00:00 2001 From: John Mortlock Date: Wed, 17 Jan 2018 09:00:49 +1030 Subject: [PATCH 2/5] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f26a0..aa088a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). This changelog adheres to [Keep a CHANGELOG](http://keepachangelog.com/). +## Unreleased +### Changed +- No longer clone the "env" middleware variable + ## [0.3.0] - 2016-12-28 ### Changed - Allows whitelisting hash values based on the key From bdf019014ab981c14086299b30a6b36b7bf82aa4 Mon Sep 17 00:00:00 2001 From: John Mortlock Date: Wed, 17 Jan 2018 09:15:27 +1030 Subject: [PATCH 3/5] Add the jira ticket number into changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa088a9..aef52a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ This changelog adheres to [Keep a CHANGELOG](http://keepachangelog.com/). ## Unreleased ### Changed -- No longer clone the "env" middleware variable +- [TT-3520] No longer clone the "env" middleware variable ## [0.3.0] - 2016-12-28 ### Changed From 10d92e8f682872de91a0a97601a312d2264eb2c5 Mon Sep 17 00:00:00 2001 From: John Mortlock Date: Wed, 17 Jan 2018 08:45:29 +1030 Subject: [PATCH 4/5] Filter action_dispatch.request.request_parameters --- lib/sensitive_data_filter/middleware.rb | 2 +- lib/sensitive_data_filter/middleware/env_parser.rb | 9 +++++++++ spec/sensitive_data_filter/middleware/env_parser_spec.rb | 9 ++++++++- spec/sensitive_data_filter/middleware/occurrence_spec.rb | 1 + 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/sensitive_data_filter/middleware.rb b/lib/sensitive_data_filter/middleware.rb index d0f654e..3ba0ab6 100644 --- a/lib/sensitive_data_filter/middleware.rb +++ b/lib/sensitive_data_filter/middleware.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module SensitiveDataFilter module Middleware - FILTERABLE = %i(query_params body_params).freeze + FILTERABLE = %i(query_params body_params request_params).freeze end end diff --git a/lib/sensitive_data_filter/middleware/env_parser.rb b/lib/sensitive_data_filter/middleware/env_parser.rb index 9ab1994..20c0b8a 100644 --- a/lib/sensitive_data_filter/middleware/env_parser.rb +++ b/lib/sensitive_data_filter/middleware/env_parser.rb @@ -6,6 +6,7 @@ module Middleware class EnvParser QUERY_STRING = 'QUERY_STRING'.freeze RACK_INPUT = 'rack.input'.freeze + REQUEST_PARAMS = 'action_dispatch.request.request_parameters'.freeze extend Forwardable @@ -28,6 +29,10 @@ def body_params @parameter_parser.parse(body) end + def request_params + @env[REQUEST_PARAMS] + end + def query_params=(new_params) @env[QUERY_STRING] = Rack::Utils.build_query(new_params) end @@ -36,6 +41,10 @@ def body_params=(new_params) @env[RACK_INPUT] = StringIO.new @parameter_parser.unparse(new_params) end + def request_params=(new_params) + @env[REQUEST_PARAMS] = new_params + end + def mutate(mutation) SensitiveDataFilter::Middleware::FILTERABLE.each do |filterable| self.send("#{filterable}=", mutation.send(filterable)) diff --git a/spec/sensitive_data_filter/middleware/env_parser_spec.rb b/spec/sensitive_data_filter/middleware/env_parser_spec.rb index f428e29..0397a08 100644 --- a/spec/sensitive_data_filter/middleware/env_parser_spec.rb +++ b/spec/sensitive_data_filter/middleware/env_parser_spec.rb @@ -117,24 +117,30 @@ describe '#mutate!' do let(:query_params) { { 'sensitive_query' => 'sensitive_data' } } let(:body_params) { { 'sensitive_body' => 'sensitive_data' } } + let(:request_params) { { 'sensitive_request' => 'sensitive_request' } } before do env_parser.query_params = { sensitive_query: 'sensitive_data' } env_parser.body_params = { sensitive_body: 'sensitive_data' } + env_parser.request_params = { sensitive_request: 'sensitive_request' } end context 'before mutation' do specify { expect(env_parser.query_params).to eq 'sensitive_query' => 'sensitive_data' } specify { expect(env_parser.body_params).to eq 'sensitive_body' => 'sensitive_data' } + specify { expect(env_parser.request_params).to eq({ sensitive_request: 'sensitive_request' }) } end context 'after mutation' do let(:filtered_query_params) { { 'sensitive_query' => '[FILTERED]' } } let(:filtered_body_params) { { 'sensitive_body' => '[FILTERED]' } } + let(:filtered_request_params) { { 'sensitive_request' => '[FILTERED]' } } + let(:changeset) { double( query_params: filtered_query_params, - body_params: filtered_body_params + body_params: filtered_body_params, + request_params: filtered_request_params ) } @@ -144,6 +150,7 @@ specify { expect(env_parser.query_params).to eq filtered_query_params } specify { expect(env_parser.body_params).to eq filtered_body_params } + specify { expect(env_parser.request_params).to eq filtered_request_params } end end end diff --git a/spec/sensitive_data_filter/middleware/occurrence_spec.rb b/spec/sensitive_data_filter/middleware/occurrence_spec.rb index e82205a..5484ea9 100644 --- a/spec/sensitive_data_filter/middleware/occurrence_spec.rb +++ b/spec/sensitive_data_filter/middleware/occurrence_spec.rb @@ -14,6 +14,7 @@ let(:filtered_query_params) { { 'credit_card' => '[FILTERED]' } } let(:original_body_params) { { credit_cards: '4111 1111 1111 1111 and 5123 4567 8901 2346' } } let(:filtered_body_params) { { credit_cards: '[FILTERED] and [FILTERED]' } } + let(:filtered_request_params) { { 'credit_card' => '[FILTERED]' } } let(:session) { { 'session_id' => '01ab02cd' } } let(:original_env) { double } let(:changeset) { double } From 55942a9f3120264fbc80d9cf3c9568f5014e254f Mon Sep 17 00:00:00 2001 From: John Mortlock Date: Wed, 17 Jan 2018 09:19:06 +1030 Subject: [PATCH 5/5] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aef52a9..7cbf6a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ This changelog adheres to [Keep a CHANGELOG](http://keepachangelog.com/). ## Unreleased ### Changed - [TT-3520] No longer clone the "env" middleware variable +- [TT-3521] filter action dispatch parameter fields ## [0.3.0] - 2016-12-28 ### Changed