From e9cb82c72e0b58aa90adecf4c51cd1a98206bfdf Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Tue, 13 Jun 2017 15:42:34 -0700 Subject: [PATCH 1/2] Rescue exceptions raised when calling to_s while sanitizing When calling to_s will raise, rescue and insert a placeholder. --- lib/bugsnag/cleaner.rb | 3 ++- spec/cleaner_spec.rb | 9 +++++++++ spec/helper_spec.rb | 13 +++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index b720291e9..6c851542e 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -6,6 +6,7 @@ class Cleaner FILTERED = '[FILTERED]'.freeze RECURSION = '[RECURSION]'.freeze OBJECT = '[OBJECT]'.freeze + RAISED = '[RAISED]'.freeze def initialize(filters) @filters = Array(filters) @@ -43,7 +44,7 @@ def traverse_object(obj, seen, scope) when String clean_string(obj) else - str = obj.to_s + str = obj.to_s rescue RAISED # avoid leaking potentially sensitive data from objects' #inspect output if str =~ /#<.*>/ OBJECT diff --git a/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index f60ac8fc4..3318599ac 100644 --- a/spec/cleaner_spec.rb +++ b/spec/cleaner_spec.rb @@ -82,6 +82,15 @@ params = {:request => {:params => {:foo => {:bar => "baz"}}}} expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:request => {:params => {:foo => {:bar => '[FILTERED]'}}}}) end + + it "filters objects which can't be stringified" do + class StringRaiser + def to_s + raise 'Oh no you do not!' + end + end + expect(subject.clean_object({ :foo => StringRaiser.new })).to eq({ :foo => '[RAISED]' }) + end end describe "#clean_url" do diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index a9c486190..a02fe9415 100644 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -39,6 +39,19 @@ expect(value[4]).to be_a(String) end + context "an object will throw if `to_s` is called" do + class StringRaiser + def to_s + raise 'Oh no you do not!' + end + end + + it "uses the string '[RAISED]' instead" do + value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRaiser.new]) + expect(value[2]).to eq "[RAISED]" + end + end + context "payload length is less than allowed" do it "does not change strings" do From 8e4389dde877c9342b6c321144e59f295718e9a5 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Tue, 13 Jun 2017 17:41:19 -0700 Subject: [PATCH 2/2] Replace client IP address with SPOOF when it cannot be serialized If a ActionDispatch::RemoteIp instance cannot be memoized, it throws an IpSpoofAttackError from to_s. In this case, replace the IP with a placeholder. Ref: https://github.com/rails/rails/blob/34fe2a4fc778d18b7fe6bdf3629c1481bee789b9/actionpack/lib/action_dispatch/middleware/remote_ip.rb#L134 --- lib/bugsnag/middleware/rack_request.rb | 6 ++- lib/bugsnag/middleware/rails3_request.rb | 7 +++- spec/rails3_request_spec.rb | 52 ++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 spec/rails3_request_spec.rb diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 41cb045aa..080c2adcb 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -1,5 +1,7 @@ module Bugsnag::Middleware class RackRequest + SPOOF = "[SPOOF]".freeze + def initialize(bugsnag) @bugsnag = bugsnag end @@ -11,7 +13,7 @@ def call(notification) request = ::Rack::Request.new(env) params = request.params rescue {} - + client_ip = request.ip.to_s rescue SPOOF session = env["rack.session"] # Set the context @@ -51,7 +53,7 @@ def call(notification) :httpMethod => request.request_method, :params => params.to_hash, :referer => request.referer, - :clientIp => request.ip, + :clientIp => client_ip, :headers => headers }) diff --git a/lib/bugsnag/middleware/rails3_request.rb b/lib/bugsnag/middleware/rails3_request.rb index c0b18d487..61e7858fd 100644 --- a/lib/bugsnag/middleware/rails3_request.rb +++ b/lib/bugsnag/middleware/rails3_request.rb @@ -1,5 +1,7 @@ module Bugsnag::Middleware class Rails3Request + SPOOF = "[SPOOF]".freeze + def initialize(bugsnag) @bugsnag = bugsnag end @@ -8,6 +10,7 @@ def call(notification) if notification.request_data[:rack_env] env = notification.request_data[:rack_env] params = env["action_dispatch.request.parameters"] + client_ip = env["action_dispatch.remote_ip"].to_s rescue SPOOF if params # Set the context @@ -22,11 +25,11 @@ def call(notification) # Use action_dispatch.remote_ip for IP address fields and send request id notification.add_tab(:request, { - :clientIp => env["action_dispatch.remote_ip"], + :clientIp => client_ip, :requestId => env["action_dispatch.request_id"] }) - notification.user_id = env["action_dispatch.remote_ip"] + notification.user_id = client_ip # Add the rails version if notification.configuration.send_environment diff --git a/spec/rails3_request_spec.rb b/spec/rails3_request_spec.rb new file mode 100644 index 000000000..722dcf669 --- /dev/null +++ b/spec/rails3_request_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Bugsnag::Middleware::Rails3Request do + before(:each) do + Bugsnag.configuration.middleware.use(described_class) + end + + describe "#call" do + it "sets request metadata" do + Bugsnag.configuration.set_request_data(:rack_env, { + "action_dispatch.remote_ip" => "10.2.2.224", + "action_dispatch.request_id" => "5", + }) + Bugsnag.notify(BugsnagTestException.new('Grimbles')) + + expect(Bugsnag).to have_sent_notification { |payload| + event = get_event_from_payload(payload) + puts event["metaData"].inspect + expect(event["metaData"]["request"]).to eq({ + "clientIp" => "10.2.2.224", + "requestId" => "5" + }) + } + end + + context "the Remote IP will throw when serialized" do + + it "sets the client IP metdata to [SPOOF]" do + class SpecialIP + def to_s + raise BugsnagTestException.new('oh no') + end + end + Bugsnag.configuration.set_request_data(:rack_env, { + "action_dispatch.remote_ip" => SpecialIP.new, + "action_dispatch.request_id" => "5", + }) + + Bugsnag.notify(BugsnagTestException.new('Grimbles')) + + expect(Bugsnag).to have_sent_notification { |payload| + event = get_event_from_payload(payload) + puts event["metaData"].inspect + expect(event["metaData"]["request"]).to eq({ + "clientIp" => "[SPOOF]", + "requestId" => "5" + }) + } + end + end + end +end