filter sensitive data from error reports

fixes #9700

filter sensitive information from the data that is saved for an error report.
also add a few extra fields to the view, such as request_context_id and form
parameters.

test plan:
- make an action with the api that will cause an error report to be generated
- go view that error report
- it should have your access token filtered out
- it should have a request context id field

Change-Id: I3c4d0d8002b6f502fdeb9e4dd40f3fd5d51dc04d
Reviewed-on: https://gerrit.instructure.com/12625
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>

app/controllers/application_controller.rb

@@ -1416,15 +1416,13 @@ class ApplicationController < ActionController::Base
     data
   end
 
-  FILTERED_PARAMETERS = [:password, :access_token, :api_key, :client_secret]
-  filter_parameter_logging *FILTERED_PARAMETERS
+  filter_parameter_logging *Canvas::LoggingFilter.filtered_parameters
 
   # filter out sensitive parameters in the query string as well when logging
   # the rails "Completed in XXms" line.
   # this is fixed in Rails 3.x
   def complete_request_uri
-    @@filtered_parameters_regex ||= %r{([?&](?:#{FILTERED_PARAMETERS.join('|')}))=[^&]+}
-    uri = request.request_uri.gsub(@@filtered_parameters_regex, '\1=[FILTERED]')
+    uri = Canvas::LoggingFilter.filter_uri(request.request_uri)
     "#{request.protocol}#{request.host}#{uri}"
   end
 end

app/controllers/errors_controller.rb

@@ -26,7 +26,7 @@ class ErrorsController < ApplicationController
 
   def index
     params[:page] = params[:page].to_i > 0 ? params[:page].to_i : 1
-    @reports = ErrorReport
+    @reports = ErrorReport.scoped(:include => :user)
 
     @message = params[:message]
     if @message.present?

app/models/error_report.rb

@@ -113,6 +113,10 @@ class ErrorReport < ActiveRecord::Base
       write_attribute(:comments, val[0,self.class.maximum_text_length])
     end
   end
+
+  def url=(val)
+    write_attribute(:url, Canvas::LoggingFilter.filter_uri(val))
+  end
   
   def guess_email
     self.email = nil if self.email && self.email.empty?
@@ -131,12 +135,31 @@ class ErrorReport < ActiveRecord::Base
     self.delete_all(['created_at < ?', before_date])
   end
 
+  USEFUL_ENV = [
+    "HTTP_ACCEPT",
+    "HTTP_ACCEPT_ENCODING",
+    "HTTP_COOKIE",
+    "HTTP_HOST",
+    "HTTP_REFERER",
+    "HTTP_USER_AGENT",
+    "PATH_INFO",
+    "QUERY_STRING",
+    "REMOTE_HOST",
+    "REQUEST_METHOD",
+    "REQUEST_PATH",
+    "REQUEST_URI",
+    "SERVER_NAME",
+    "SERVER_PORT",
+    "SERVER_PROTOCOL",
+  ]
   def self.useful_http_env_stuff_from_request(request)
-    stuff = request.env.slice( *["HTTP_ACCEPT", "HTTP_ACCEPT_ENCODING", "HTTP_COOKIE", "HTTP_HOST", "HTTP_REFERER",
-                         "HTTP_USER_AGENT", "PATH_INFO", "QUERY_STRING", "REMOTE_HOST",
-                         "REQUEST_METHOD", "REQUEST_PATH", "REQUEST_URI", "SERVER_NAME", "SERVER_PORT",
-                         "SERVER_PROTOCOL", "action_controller.request.path_parameters"] )
+    stuff = request.env.slice(*USEFUL_ENV)
     stuff['REMOTE_ADDR'] = request.remote_ip # ActionController::Request#remote_ip has proxy smarts
+    stuff['QUERY_STRING'] = Canvas::LoggingFilter.filter_query_string("?" + stuff['QUERY_STRING'])
+    stuff['REQUEST_URI'] = Canvas::LoggingFilter.filter_uri(stuff['REQUEST_URI'])
+    stuff['path_parameters'] = Canvas::LoggingFilter.filter_params(request.path_parameters.dup).inspect # params rails picks up from the url
+    stuff['query_parameters'] = Canvas::LoggingFilter.filter_params(request.query_parameters.dup).inspect # params rails picks up from the query string
+    stuff['request_parameters'] = Canvas::LoggingFilter.filter_params(request.request_parameters.dup).inspect # params from forms
     stuff
   end
 

app/views/errors/index.html.erb

@@ -33,15 +33,21 @@
 
     <div style="border: 1px solid #eee; -moz-border-radius: 5px; padding: 5px 20px;">
       <%= before_label :category, "category" %> <%= report.category || t(:default_category, 'default') %><br/>
-      <%= report.created_at %><br />
-      <%= report.url %><br/>
+      <%= before_label :created_at, "created at" %> <%= report.created_at %><br />
       <% if report.user_id %>
-        <%= link_to report.user.try(:name), user_url(report.user_id) %>
+        <%= before_label :user, "user" %> <%= link_to report.user.try(:name), user_url(report.user_id) %><br />
       <% end %>
-      <% (report.data || {}).each do |k,v| %>
+      <%= before_label :url, "url" %> <%= report.url %><br/>
+      <% if report.request_context_id %>
+        <%= before_label :request_context_id, "request context id" %> <%= report.request_context_id %>
+      <% end %>
+      <% (report.data || {}).to_a.sort.each do |k,v| %>
+        <% next if k.to_s == "exception_message" %>
         <br><%= before_label k.to_s %> <%= v %>
       <% end %>
+      <br />
       <pre style="font-size: 0.8em; margin-left: 20px; <%= "max-height: 150px;" if @reports.length > 1 %> overflow: auto;">
+        <%= report.data.try(:[], "exception_message") %><br />
         <%= report.backtrace %>
       </pre>
     </div>

lib/canvas/logging_filter.rb

@@ -0,0 +1,28 @@
+module Canvas::LoggingFilter
+  FILTERED_PARAMETERS = [:password, :auth_password, :access_token, :api_key, :client_secret]
+  def self.filtered_parameters
+    FILTERED_PARAMETERS
+  end
+
+  EXTENDED_FILTERED_PARAMETERS = ["pseudonym[password]", "login[password]"]
+  def self.all_filtered_parameters
+    FILTERED_PARAMETERS.map(&:to_s) + EXTENDED_FILTERED_PARAMETERS
+  end
+
+  def self.filter_uri(uri)
+    filter_query_string(uri)
+  end
+
+  def self.filter_query_string(qs)
+    regs = all_filtered_parameters.map { |p| p.gsub("[", "\\[").gsub("]", "\\]") }.join('|')
+    @@filtered_parameters_regex ||= %r{([?&](?:#{regs}))=[^&]+}
+    qs.gsub(@@filtered_parameters_regex, '\1=[FILTERED]')
+  end
+
+  def self.filter_params(params)
+    params.keys.each do |k|
+      params[k] = "[FILTERED]" if all_filtered_parameters.include?(k.to_s.downcase)
+    end
+    params
+  end
+end

spec/lib/canvas/logging_filter_spec.rb

@@ -0,0 +1,59 @@
+require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
+
+describe "Canvas::LoggingFilter" do
+  describe "filter_uri" do
+    it "should filter sensitive information from the url query string" do
+      url = "https://www.instructure.example.com?access_token=abcdef"
+      filtered_url = Canvas::LoggingFilter.filter_uri(url)
+      filtered_url.should == "https://www.instructure.example.com?access_token=[FILTERED]"
+    end
+
+    it "should filter all query params" do
+      url = "https://www.instructure.example.com?access_token=abcdef&api_key=123"
+      filtered_url = Canvas::LoggingFilter.filter_uri(url)
+      filtered_url.should == "https://www.instructure.example.com?access_token=[FILTERED]&api_key=[FILTERED]"
+    end
+
+    it "should not filter close matches" do
+      url = "https://www.instructure.example.com?x_access_token=abcdef&api_key_x=123"
+      filtered_url = Canvas::LoggingFilter.filter_uri(url)
+      filtered_url.should == url
+    end
+  end
+
+  describe "filter_params" do
+    it "should filter sensitive keys" do
+      params = {
+        :access_token => "abcdef",
+        :api_key => 123
+      }
+      filtered_params = Canvas::LoggingFilter.filter_params(params)
+      filtered_params.should == {
+        :access_token => "[FILTERED]",
+        :api_key => "[FILTERED]"
+      }
+    end
+
+    it "should filter string or symbol keys" do
+      params = {
+        :access_token => "abcdef",
+        "api_key" => 123
+      }
+      filtered_params = Canvas::LoggingFilter.filter_params(params)
+      filtered_params.should == {
+        :access_token => "[FILTERED]",
+        "api_key" => "[FILTERED]"
+      }
+    end
+
+    it "should filter keys of any case" do
+      params = {
+        "ApI_KeY" => 123
+      }
+      filtered_params = Canvas::LoggingFilter.filter_params(params)
+      filtered_params.should == {
+        "ApI_KeY" => "[FILTERED]"
+      }
+    end
+  end
+end

spec/models/error_report_spec.rb

@@ -63,4 +63,30 @@ describe ErrorReport do
     ErrorReport.create! { |r| r.category = 'fred' }
     ErrorReport.categories.should == ['bob', 'fred', 'george']
   end
+
+  it "should filter the url when it is assigned" do
+    report = ErrorReport.new
+    report.url = "https://www.instructure.example.com?access_token=abcdef"
+    report.url.should == "https://www.instructure.example.com?access_token=[FILTERED]"
+  end
+
+  it "should filter params" do
+    req = mock(
+      :env => {
+        "QUERY_STRING" => "access_token=abcdef&pseudonym[password]=zzz",
+        "REQUEST_URI" => "https://www.instructure.example.com?access_token=abcdef&pseudonym[password]=zzz",
+      },
+      :remote_ip => "",
+      :path_parameters => { :api_key => "1" },
+      :query_parameters => { "access_token" => "abcdef", "pseudonym[password]" => "zzz" },
+      :request_parameters => { "client_secret" => "xoxo" }
+    )
+    report = ErrorReport.new
+    report.assign_data(ErrorReport.useful_http_env_stuff_from_request(req))
+    report.data["QUERY_STRING"].should == "?access_token=[FILTERED]&pseudonym[password]=[FILTERED]"
+    report.data["REQUEST_URI"].should == "https://www.instructure.example.com?access_token=[FILTERED]&pseudonym[password]=[FILTERED]"
+    report.data["path_parameters"].should == { :api_key => "[FILTERED]" }.inspect
+    report.data["query_parameters"].should == { "access_token" => "[FILTERED]", "pseudonym[password]" => "[FILTERED]" }.inspect
+    report.data["request_parameters"].should == { "client_secret" => "[FILTERED]" }.inspect
+  end
 end