diff --git a/.travis.yml b/.travis.yml index cc08bc6..50c4279 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,9 @@ rvm: - 2.2 - 2.3.0 +env: + - RUBYOPT="-W0" + gemfile: - gemfiles/Gemfile.rails32 - gemfiles/Gemfile.rails41 diff --git a/app/assets/javascripts/ndr_error/client_errors.js b/app/assets/javascripts/ndr_error/client_errors.js new file mode 100644 index 0000000..aba0e12 --- /dev/null +++ b/app/assets/javascripts/ndr_error/client_errors.js @@ -0,0 +1,36 @@ +var NdrError = { + url: function() { + var origin = window.location.origin; + + // IE doesn't support `location.origin`: + origin = origin || ( + window.location.protocol + '//' + + window.location.hostname + + (window.location.port ? ':' + window.location.port: '') + ); + + // TODO: '/fingerprinting' is configureable, use + // client_errors_url helper? + return origin + '/fingerprinting/client_errors'; + }, + + notify: function(message, source, lineno, colno, error) { + jQuery.post(NdrError.url(), { + 'client_error': { + 'message': message, + 'source': source, + 'lineno': lineno, + 'colno': colno, + 'stack': error && error.stack, + 'window.width': window.innerWidth, + 'window.height': window.innerHeight, + 'screen.width': window.screen.width, + 'screen.height': window.screen.height + } + }) + } +}; + +window.onerror = function(message, source, lineno, colno, error) { + NdrError.notify(message, source, lineno, colno, error); +} diff --git a/app/assets/javascripts/ndr_error/ndr_error.js b/app/assets/javascripts/ndr_error/ndr_error.js index 23a314b..41c0b16 100644 --- a/app/assets/javascripts/ndr_error/ndr_error.js +++ b/app/assets/javascripts/ndr_error/ndr_error.js @@ -13,6 +13,7 @@ //= require jquery //= require jquery_ujs //= require ndr_error/bootstrap/bootstrap +//= require ndr_error/client_errors jQuery(function() { // Backtrace toggling: @@ -46,6 +47,7 @@ jQuery(function() { jQuery('.badge').tooltip(); $searchField.keydown(function(event) { + wenewnew // will submit the search form. if (event.keyCode == 13) { this.form.submit(); @@ -58,3 +60,15 @@ jQuery(function() { }); })(); }); + +window.onerror = function(message, source, lineno, colno, error) { + jQuery.post('client_errors/', { + 'client_error': { + 'message': message, + 'source': source, + 'lineno': lineno, + 'colno': colno, + 'stack': error && error.stack + } + }) +} diff --git a/app/controllers/ndr_error/client_errors_controller.rb b/app/controllers/ndr_error/client_errors_controller.rb new file mode 100644 index 0000000..315fe58 --- /dev/null +++ b/app/controllers/ndr_error/client_errors_controller.rb @@ -0,0 +1,15 @@ +module NdrError + # Controller for receiving client errors + class ClientErrorsController < ApplicationController + def create + exception = JavascriptError.new(params[:client_error]) + ancillary = {} # TODO: populate this as the middleware does + fingerprint, log = NdrError.log(exception, ancillary, request) + + render json: { + fingerprint: fingerprint.id, + uuid: log.try(:id) + } + end + end +end diff --git a/app/models/ndr_error/log.rb b/app/models/ndr_error/log.rb index 5b1cc8c..0162980 100644 --- a/app/models/ndr_error/log.rb +++ b/app/models/ndr_error/log.rb @@ -116,13 +116,15 @@ def register_exception(exception) self.error_class = exception.class.to_s self.backtrace = exception.backtrace self.description = description_from(exception.message) + + self.parameters_yml = exception.metadata if client_error? end # Stores parameters from the given _request_ object # as YAML. Will attempt to store as many as possible # of the parameters in the available 4000 chars. def register_request(request) - extract_request_params(request) + extract_request_params(request) unless client_error? extract_request_attributes(request) end @@ -151,7 +153,7 @@ def parameters_yml=(params) # Returns the params hash associated # with the request. def parameters - YAML.load(parameters_yml) + YAML.safe_load(parameters_yml, [Symbol]) end # Formats error to be like the ruby error. @@ -176,6 +178,10 @@ def md5_digest=(digest) @_digest = digest end + def client_error? + error_class == 'NdrError::JavascriptError' + end + private # For the given `request' object, return the @@ -199,9 +205,11 @@ def extract_request_params(request) def extract_request_attributes(request) return unless request + uri = request.env[client_error? ? 'HTTP_REFERER' : 'REQUEST_URI'] + self.port = request.env['SERVER_PORT'] self.ip = "#{request.env['REMOTE_ADDR']}/#{request.remote_ip}" - self.url = "#{request.env['REQUEST_URI']} (on #{request.host})" + self.url = "#{uri} (on #{request.host})" self.user_agent = request.env['HTTP_USER_AGENT'] end diff --git a/config/routes.rb b/config/routes.rb index 072e7c8..3416f95 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,4 +3,8 @@ only: [:index, :show, :edit, :update, :destroy], controller: 'errors', as: 'error_fingerprints' + + resources :client_errors, + only: [:create], + controller: 'client_errors' end diff --git a/lib/ndr_error.rb b/lib/ndr_error.rb index 0cba0a0..1d0e741 100644 --- a/lib/ndr_error.rb +++ b/lib/ndr_error.rb @@ -3,6 +3,7 @@ require 'ndr_error/backtrace_compression' require 'ndr_error/finder' require 'ndr_error/fuzzing' +require 'ndr_error/javascript_error' require 'ndr_error/logging' require 'ndr_error/uuid_builder' diff --git a/lib/ndr_error/fuzzing.rb b/lib/ndr_error/fuzzing.rb index befd457..a840e3e 100644 --- a/lib/ndr_error/fuzzing.rb +++ b/lib/ndr_error/fuzzing.rb @@ -17,6 +17,7 @@ def fuzz_description(description) # * independent of deployment paths # * independent of line numbers def fuzz_backtrace(backtrace) + return '' if client_error? backtrace.map { |line| fuzz_line(line) }.join("\n") end diff --git a/lib/ndr_error/javascript_error.rb b/lib/ndr_error/javascript_error.rb new file mode 100644 index 0000000..ece69f2 --- /dev/null +++ b/lib/ndr_error/javascript_error.rb @@ -0,0 +1,25 @@ +module NdrError + # Class to wrap / normalise Javascript exception + # data, and allow it to be logged by NdrError. + class JavascriptError < Exception + attr_reader :source + + def initialize(parameters) + @source = parameters.with_indifferent_access + + super(@source['message']) + + set_backtrace_from_stack + end + + def metadata + source.except('message', 'stack') + end + + private + + def set_backtrace_from_stack + set_backtrace @source.fetch('stack', '').split("\n") + end + end +end diff --git a/test/dummy/app/assets/javascripts/application.js b/test/dummy/app/assets/javascripts/application.js index 9097d83..e64b770 100644 --- a/test/dummy/app/assets/javascripts/application.js +++ b/test/dummy/app/assets/javascripts/application.js @@ -12,4 +12,5 @@ // //= require jquery //= require jquery_ujs +//= require ndr_error/client_errors //= require_tree . diff --git a/test/dummy/app/controllers/disaster_controller.rb b/test/dummy/app/controllers/disaster_controller.rb index c45661f..8d3ebcd 100644 --- a/test/dummy/app/controllers/disaster_controller.rb +++ b/test/dummy/app/controllers/disaster_controller.rb @@ -1,9 +1,15 @@ # Some application logic that we should be able to log failures from: class DisasterController < ApplicationController def no_panic + # Triggers no exceptions end def cause + # Triggers a server-side exception fail params[:message] end + + def cause_client + # Triggers a client-side exception + end end diff --git a/test/dummy/app/views/disaster/cause_client.html.erb b/test/dummy/app/views/disaster/cause_client.html.erb new file mode 100644 index 0000000..521e086 --- /dev/null +++ b/test/dummy/app/views/disaster/cause_client.html.erb @@ -0,0 +1,4 @@ +

This page should raise a Javascript error

+ diff --git a/test/dummy/config/environments/development.rb b/test/dummy/config/environments/development.rb index 19920c4..9c51e8e 100644 --- a/test/dummy/config/environments/development.rb +++ b/test/dummy/config/environments/development.rb @@ -10,7 +10,7 @@ config.whiny_nils = true # Show full error reports and disable caching - config.consider_all_requests_local = true + config.consider_all_requests_local = false config.action_controller.perform_caching = false # Don't care if the mailer can't send diff --git a/test/integration/client_error_logging_test.rb b/test/integration/client_error_logging_test.rb new file mode 100644 index 0000000..bc25d41 --- /dev/null +++ b/test/integration/client_error_logging_test.rb @@ -0,0 +1,22 @@ +require 'test_helper' + +# Ensure host app is able to log errors sent to the API endpoint +class ClientErrorLoggingTest < ActionDispatch::IntegrationTest + def setup + NdrError::Fingerprint.delete_all + NdrError::Log.delete_all + end + + test 'should log client exceptions' do + assert_difference(-> { NdrError::Log.count }, 2) do + 2.times { visit '/disaster/cause_client' } + end + + error_logs = NdrError::Log.all + assert error_logs.map(&:error_fingerprint).uniq.one? + error_log = error_logs.first + + assert_equal 'NdrError::JavascriptError', error_log.error_class + assert_equal "ReferenceError: Can't find variable: fooBarBaz", error_log.description + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 66a8235..a6713df 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -33,8 +33,9 @@ Capybara.register_driver :poltergeist do |app| options = { # debug: true, # Uncomment for more verbose - # inspector: true, # DEBUGGING suppport. + inspector: true, # DEBUGGING suppport. phantomjs_options: ['--proxy-type=none'], + js_errors: false, timeout: 60 } diff --git a/test/unit/ndr_error/fuzzing_test.rb b/test/unit/ndr_error/fuzzing_test.rb index 8b4b0df..0e33924 100644 --- a/test/unit/ndr_error/fuzzing_test.rb +++ b/test/unit/ndr_error/fuzzing_test.rb @@ -3,40 +3,51 @@ module NdrError # Test our fuzzing / digest creation: class FuzzingTest < ActiveSupport::TestCase - include NdrError::Fuzzing + # Dummy class for testing Fuzzing mixin + class Fuzzable + include NdrError::Fuzzing + + def client_error? + false + end + end + + def setup + @fuzzable = Fuzzable.new + end test 'should fuzz descriptions correctly' do description = "TemplateError: undefined method `clean' for [\"XYZ\", \"ABC\"]:Array" - refute send(:fuzz_description, description).include?('XYZ') + refute fuzz_description(description).include?('XYZ') # Should obfuscate objectids: assert_equal fuzz('#', []), fuzz('#', []) end test 'should fuzz Rails root directory from backtraces' do - trace = send(:fuzz_backtrace, [Rails.root.join('app').to_s]) + trace = fuzz_backtrace([Rails.root.join('app').to_s]) assert_equal 'Rails.root/app', trace end test 'should fuzz gem differences from backtraces' do # Should fuzz gem paths: - trace = send(:fuzz_backtrace, [Gem.path.first + '/app']) + trace = fuzz_backtrace([Gem.path.first + '/app']) assert_equal 'Gem.path/app', trace # Should remove gem version number when fuzzing gem paths line = Gem.path.first + "/gems/evil-1.4.3/lib/evil/file.rb:623:in `method'" - assert_equal "Gem.path/gems/evil-/lib/evil/file.rbin `method'", send(:fuzz_backtrace, [line]) + assert_equal "Gem.path/gems/evil-/lib/evil/file.rbin `method'", fuzz_backtrace([line]) end test 'should fuzz LOAD_PATH from backtraces' do # Should fuzz load path entries: - trace = send(:fuzz_backtrace, [$LOAD_PATH.first + '/app']) + trace = fuzz_backtrace([$LOAD_PATH.first + '/app']) refute trace.include?($LOAD_PATH.first) end test 'should fuzz line numbers from backtraces' do # Should fuzz line numbers: - trace = send(:fuzz_backtrace, ['app/myfile.rb:12: in function']) + trace = fuzz_backtrace(['app/myfile.rb:12: in function']) assert_equal 'app/myfile.rb in function', trace end @@ -44,17 +55,48 @@ class FuzzingTest < ActiveSupport::TestCase template = ActionView::Template.new('test template', 'template.html.erb', nil, {}) compiled = template.send(:method_name) # The method name of the template once compiled - assert_equal '_template_html_erb__COMPILED_ID', send(:fuzz_backtrace, [compiled]) + assert_equal '_template_html_erb__COMPILED_ID', fuzz_backtrace([compiled]) partial = ActionView::Template.new('test partial', '_partial.html.erb', nil, {}) compiled = partial.send(:method_name) # The method name of the partial once compiled - assert_equal '__partial_html_erb__COMPILED_ID', send(:fuzz_backtrace, [compiled]) + assert_equal '__partial_html_erb__COMPILED_ID', fuzz_backtrace([compiled]) end test 'should fuzz compiled callbacks from backtraces' do - trace = send(:fuzz_backtrace, ['_run__2058915813__process_action__1931044129__callbacks']) + trace = fuzz_backtrace(['_run__2058915813__process_action__1931044129__callbacks']) assert_equal '_run__COMPILED_ID__process_action__COMPILED_ID__callbacks', trace end + + test '#fuzz_backtrace should be consistent with client errors' do + @fuzzable.stubs(client_error?: true) + assert_equal fuzz_backtrace(['abc']), @fuzzable.send(:fuzz_backtrace, ['bcd']) + end + + test 'fuzzing should be sensitive to client error descriptions' do + @fuzzable.stubs(client_error?: true) + assert_equal fuzz('test', []), fuzz('test', []) + refute_equal fuzz('test', []), fuzz('zest', []) + end + + test 'fuzzing should be not sensitive to client error backtraces' do + @fuzzable.stubs(client_error?: true) + assert_equal fuzz('test', %w(t e s t)), fuzz('test', %w(t e s t)) + assert_equal fuzz('test', %w(t e s t)), fuzz('test', %w(e s t e)) + end + + private + + def fuzz_description(string) + @fuzzable.send(:fuzz_description, string) + end + + def fuzz_backtrace(trace) + @fuzzable.send(:fuzz_backtrace, trace) + end + + def fuzz(description, backtrace) + @fuzzable.fuzz(description, backtrace) + end end end diff --git a/test/unit/ndr_error/javascript_error_test.rb b/test/unit/ndr_error/javascript_error_test.rb new file mode 100644 index 0000000..3d8f7a5 --- /dev/null +++ b/test/unit/ndr_error/javascript_error_test.rb @@ -0,0 +1,20 @@ +require 'test_helper' + +module NdrError + # Unit Test Log. + class JavaScriptErrorTest < ActiveSupport::TestCase + test 'should set backtrace' do + params = { + 'message' => 'ReferenceError: adfgrdfgkljh is not defined', + 'source' => 'http://localhost:3000', + 'lineno' => '49', + 'colno' => '1', + 'stack' => "stack_values\nline 1\nline 2\nline 3" + } + error = JavascriptError.new(params) + + assert_equal 'ReferenceError: adfgrdfgkljh is not defined', error.message + assert_equal 4, error.backtrace.length + end + end +end diff --git a/test/unit/ndr_error/log_test.rb b/test/unit/ndr_error/log_test.rb index 4d67ff9..1e11eed 100644 --- a/test/unit/ndr_error/log_test.rb +++ b/test/unit/ndr_error/log_test.rb @@ -96,6 +96,12 @@ def setup assert_equal({}, error.parameters) end + test 'should safe-load paramaters' do + error = Log.new { |log| log.parameters_yml = { not: /allowed/ } } + exception = assert_raises(Psych::DisallowedClass) { error.parameters } + assert_match(/Regexp/, exception.message) + end + test 'should store the params hash correctly when they fit in the column' do params1 = { a: 1 } params2 = { b: 2 } @@ -394,5 +400,13 @@ def setup assert_raises(SecurityError) { trigger.call } end end + + test 'should return metadata if client error' do + error = Log.new + exception = JavascriptError.new(message: 'oops', stack: "a\nb\nc", test: 'foobar') + + error.register_exception(exception) + assert_equal({ 'test' => 'foobar' }, error.parameters) + end end end