From 2ecaf3ac5096b3e9f933198f26953db4bd5cd919 Mon Sep 17 00:00:00 2001 From: tremlab Date: Thu, 3 May 2018 16:19:53 -0700 Subject: [PATCH 1/4] clean referer of metadata_filters values. --- lib/bugsnag/middleware/rack_request.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 1dff53d34..a4cc0aaa5 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -28,13 +28,21 @@ def call(report) url = "#{request.scheme}://#{request.host}" url << ":#{request.port}" unless [80, 443].include?(request.port) + cleaner = Bugsnag::Cleaner.new(report.configuration.meta_data_filters) + # If app is passed a bad URL, this code will crash attempting to clean it begin - url << Bugsnag::Cleaner.new(report.configuration.meta_data_filters).clean_url(request.fullpath) + url << cleaner.clean_url(url) rescue StandardError => stde Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.fullpath: #{stde}" end + begin + clean_referer << cleaner.clean_url(request.referer) + rescue StandardError => stde + Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.referer: #{stde}" + end + headers = {} env.each_pair do |key, value| @@ -48,13 +56,16 @@ def call(report) headers[header_key.split("_").map {|s| s.capitalize}.join("-")] = value end + + headers.referer = clean_referer + # Add a request tab report.add_tab(:request, { :url => url, :httpMethod => request.request_method, :params => params.to_hash, - :referer => request.referer, + :referer => clean_referer, :clientIp => client_ip, :headers => headers }) From 86cf1a8f3196a92969d3553f9161e50931cdfbb7 Mon Sep 17 00:00:00 2001 From: tremlab Date: Fri, 4 May 2018 17:08:16 -0700 Subject: [PATCH 2/4] redacts filtered values from 'referer' in both headers and referer. --- lib/bugsnag/middleware/rack_request.rb | 13 +++-- spec/integrations/rack_spec.rb | 75 +++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index a4cc0aaa5..c83dcc2c1 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -32,13 +32,14 @@ def call(report) # If app is passed a bad URL, this code will crash attempting to clean it begin - url << cleaner.clean_url(url) + url << cleaner.clean_url(request.fullpath) rescue StandardError => stde Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.fullpath: #{stde}" end + referer = nil begin - clean_referer << cleaner.clean_url(request.referer) + referer = cleaner.clean_url(request.referer) if request.referer rescue StandardError => stde Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.referer: #{stde}" end @@ -56,8 +57,8 @@ def call(report) headers[header_key.split("_").map {|s| s.capitalize}.join("-")] = value end - - headers.referer = clean_referer + + headers["Referer"] = referer if headers["Referer"] # Add a request tab @@ -65,7 +66,7 @@ def call(report) :url => url, :httpMethod => request.request_method, :params => params.to_hash, - :referer => clean_referer, + :referer => referer, :clientIp => client_ip, :headers => headers }) @@ -73,6 +74,8 @@ def call(report) # Add an environment tab if report.configuration.send_environment report.add_tab(:environment, env) + # below also redacts referer from environemt tab -- need to write tests + # report.meta_data[:environment]['HTTP_REFERER'] = referer if report.meta_data[:environment]['HTTP_REFERER'] end # Add a session tab diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 85717cbe5..d9916f0f5 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -69,6 +69,79 @@ class ::Request end end + +# ********************* +# ********************* +# ********************* + + it "correctly redacts from referer any value indicated by meta_data_filters" do + callback = double + rack_env = { + :env => true, + :HTTP_REFERER => "test_key", + "rack.session" => { + :session => true + } + } + + rack_request = double + rack_params = { + :param => 'test' + } + allow(rack_request).to receive_messages( + :params => rack_params, + :ip => "rack_ip", + :request_method => "TEST", + :path => "/TEST_PATH", + :scheme => "http", + :host => "test_host", + :port => 80, + :referer => "https://bugsnag.com/about?email=hello@world.com&another_param=thing", + :fullpath => "/TEST_PATH" + ) + expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) + + # modify rack_env to include redacted referer + # rack_env["HTTP_REFERER"] = "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" + report = double("Bugsnag::Report") + allow(report).to receive(:request_data).and_return({ + :rack_env => rack_env + }) + expect(report).to receive(:context=).with("TEST /TEST_PATH") + expect(report).to receive(:user).and_return({}) + + config = double + allow(config).to receive(:send_environment).and_return(true) + allow(config).to receive(:meta_data_filters).and_return(['email']) + allow(report).to receive(:configuration).and_return(config) + expect(report).to receive(:add_tab).once.with(:request, { + :url => "http://test_host/TEST_PATH", + :httpMethod => "TEST", + :params => rack_params, + :referer => "https://bugsnag.com/about?email=[FILTERED]&another_param=thing", + :clientIp => "rack_ip", + :headers => { + "Referer"=> + + "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" + } + }) + expect(report).to receive(:add_tab).once.with(:session, { + :session => true + }) + expect(report).to receive(:add_tab).once.with(:environment, rack_env) + + expect(callback).to receive(:call).with(report) + + middleware = Bugsnag::Middleware::RackRequest.new(callback) + middleware.call(report) + end + +# ********************* +# ********************* +# ********************* +# ********************* + + it "correctly extracts data from rack middleware" do callback = double rack_env = { @@ -78,7 +151,7 @@ class ::Request :session => true } } - + rack_request = double rack_params = { :param => 'test' From c93f28919f8aff8b2b958b90850b4682adc61d28 Mon Sep 17 00:00:00 2001 From: tremlab Date: Sat, 5 May 2018 14:22:49 -0700 Subject: [PATCH 3/4] add test for url redaction of fitlered values. --- lib/bugsnag/middleware/rack_request.rb | 2 -- spec/integrations/rack_spec.rb | 25 +++++++------------------ 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index c83dcc2c1..76310064e 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -74,8 +74,6 @@ def call(report) # Add an environment tab if report.configuration.send_environment report.add_tab(:environment, env) - # below also redacts referer from environemt tab -- need to write tests - # report.meta_data[:environment]['HTTP_REFERER'] = referer if report.meta_data[:environment]['HTTP_REFERER'] end # Add a session tab diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index d9916f0f5..b6312929d 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -14,7 +14,7 @@ context "when an exception is raised in rack middleware" do # Build a fake crashing rack app exception = BugsnagTestException.new("It crashed") - rack_env = {"key" => "value"} + rack_env = {"key" => "value",} app = lambda { |env| raise exception } rack_stack = Bugsnag::Rack.new(app) @@ -69,16 +69,11 @@ class ::Request end end - -# ********************* -# ********************* -# ********************* - - it "correctly redacts from referer any value indicated by meta_data_filters" do + it "correctly redacts from url and referer any value indicated by meta_data_filters" do callback = double rack_env = { :env => true, - :HTTP_REFERER => "test_key", + :HTTP_REFERER => "https://bugsnag.com/about?email=hello@world.com&another_param=thing", "rack.session" => { :session => true } @@ -97,12 +92,11 @@ class ::Request :host => "test_host", :port => 80, :referer => "https://bugsnag.com/about?email=hello@world.com&another_param=thing", - :fullpath => "/TEST_PATH" + :fullpath => "/TEST_PATH?email=hello@world.com&another_param=thing" ) expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) # modify rack_env to include redacted referer - # rack_env["HTTP_REFERER"] = "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" report = double("Bugsnag::Report") allow(report).to receive(:request_data).and_return({ :rack_env => rack_env @@ -115,7 +109,7 @@ class ::Request allow(config).to receive(:meta_data_filters).and_return(['email']) allow(report).to receive(:configuration).and_return(config) expect(report).to receive(:add_tab).once.with(:request, { - :url => "http://test_host/TEST_PATH", + :url => "http://test_host/TEST_PATH?email=[FILTERED]&another_param=thing", :httpMethod => "TEST", :params => rack_params, :referer => "https://bugsnag.com/about?email=[FILTERED]&another_param=thing", @@ -125,10 +119,11 @@ class ::Request + "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" } }) + # rack_env["HTTP_REFERER"] = "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" + expect(report).to receive(:add_tab).once.with(:environment, rack_env) expect(report).to receive(:add_tab).once.with(:session, { :session => true }) - expect(report).to receive(:add_tab).once.with(:environment, rack_env) expect(callback).to receive(:call).with(report) @@ -136,12 +131,6 @@ class ::Request middleware.call(report) end -# ********************* -# ********************* -# ********************* -# ********************* - - it "correctly extracts data from rack middleware" do callback = double rack_env = { From 42aec06f8a08e46a16c4f525ef818c3307d612d1 Mon Sep 17 00:00:00 2001 From: tremlab Date: Sat, 5 May 2018 14:25:14 -0700 Subject: [PATCH 4/4] formatting fix --- spec/integrations/rack_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index b6312929d..2b21d065c 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -14,7 +14,7 @@ context "when an exception is raised in rack middleware" do # Build a fake crashing rack app exception = BugsnagTestException.new("It crashed") - rack_env = {"key" => "value",} + rack_env = {"key" => "value"} app = lambda { |env| raise exception } rack_stack = Bugsnag::Rack.new(app)