Merge pull request #5527 from rmosolgo/ar-backend-fixes

DetailedTrace: improve generator, filter hashes before logging

Author: rmosolgo

lib/generators/graphql/detailed_trace_generator.rb

@@ -67,6 +67,9 @@ def install_detailed_traces
 
             RUBY
           end
+
+          gem("google-protobuf")
+
         end
       end
     end

lib/generators/graphql/templates/create_graphql_detailed_traces.erb

@@ -1,9 +1,9 @@
 class CreateGraphqlDetailedTraces < ActiveRecord::Migration[<%= ActiveRecord::Migration.current_version %>]
   def change
     create_table :graphql_detailed_traces, force: true do |t|
-      t.integer :begin_ms, null: false
+      t.bigint :begin_ms, null: false
       t.float :duration_ms, null: false
-      t.text :trace_data, null: false
+      t.binary :trace_data, null: false
       t.string :operation_name, null: false
     end
   end

lib/graphql/dashboard.rb

@@ -120,7 +120,7 @@ def show
     end
 
     class StaticsController < ApplicationController
-      skip_after_action :verify_same_origin_request
+      skip_forgery_protection
       # Use an explicit list of files to avoid any chance of reading other files from disk
       STATICS = {}
 

lib/graphql/tracing/detailed_trace/active_record_backend.rb

@@ -46,7 +46,7 @@ def save_trace(operation_name, duration_ms, begin_ms, trace_data)
             begin_ms: begin_ms,
             operation_name: operation_name,
             duration_ms: duration_ms,
-            trace_data: Base64.encode64(trace_data),
+            trace_data: trace_data,
           )
           if @limit
             @model_class
@@ -64,7 +64,7 @@ def record_to_stored_trace(gdt)
             begin_ms: gdt.begin_ms,
             operation_name: gdt.operation_name,
             duration_ms: gdt.duration_ms,
-            trace_data: Base64.decode64(gdt.trace_data)
+            trace_data: gdt.trace_data
           )
 
         end

lib/graphql/tracing/perfetto_trace.rb

@@ -91,6 +91,21 @@ def initialize(active_support_notifications_pattern: nil, save_profile: false, *
           ctx_debug
         end
 
+        @arguments_filter = if (ctx = query&.context) && (dtf = ctx[:detailed_trace_filter])
+          dtf
+        elsif defined?(ActiveSupport::ParameterFilter)
+          fp = if defined?(Rails) && Rails.application && (app_config = Rails.application.config.filter_parameters).present? && !app_config.empty?
+            app_config
+          elsif ActiveSupport.respond_to?(:filter_parameters)
+            ActiveSupport.filter_parameters
+          else
+            EmptyObjects::EMPTY_ARRAY
+          end
+          ActiveSupport::ParameterFilter.new(fp, mask: ArgumentsFilter::FILTERED)
+        else
+          ArgumentsFilter.new
+        end
+
         Fiber[:graphql_flow_stack] = nil
         @sequence_id = object_id
         @pid = Process.pid
@@ -590,6 +605,22 @@ def fid
         Fiber.current.object_id
       end
 
+      class ArgumentsFilter
+        # From Rails defaults
+        # https://github.com/rails/rails/blob/main/railties/lib/rails/generators/rails/app/templates/config/initializers/filter_parameter_logging.rb.tt#L6-L8
+        SENSITIVE_KEY = /passw|token|crypt|email|_key|salt|certificate|secret|ssn|cvv|cvc|otp/i
+        FILTERED = "[FILTERED]"
+
+        def filter_param(key, value)
+          if (key.is_a?(String) && SENSITIVE_KEY.match?(key)) ||
+                  (key.is_a?(Symbol) && SENSITIVE_KEY.match?(key.name))
+            FILTERED
+          else
+            value
+          end
+        end
+      end
+
       def debug_annotation(iid, value_key, value)
         if iid
           DebugAnnotation.new(name_iid: iid, value_key => value)
@@ -634,7 +665,22 @@ def payload_to_debug(k, v, iid: nil, intern_value: false)
         when Array
           debug_annotation(iid, :array_values, v.each_with_index.map { |v2, idx| payload_to_debug((k ? "#{k}.#{idx}" : String(idx)), v2, intern_value: intern_value) }.compact)
         when Hash
-          debug_annotation(iid, :dict_entries, v.map { |k2, v2| payload_to_debug(k2, v2, intern_value: intern_value) }.compact)
+          debug_v = v.map { |k2, v2|
+            debug_k = case k2
+            when String
+              k2
+            when Symbol
+              k2.name
+            else
+              String(k2)
+            end
+            filtered_v2 = @arguments_filter.filter_param(debug_k, v2)
+            payload_to_debug(debug_k, filtered_v2, intern_value: intern_value)
+          }
+          debug_v.compact!
+          debug_annotation(iid, :dict_entries, debug_v)
+        when GraphQL::Schema::InputObject
+          payload_to_debug(k, v.to_h, iid: iid, intern_value: intern_value)
         else
           class_name_iid = @interned_da_string_values[v.class.name]
           da = [

spec/dummy/test/controllers/dashboard/statics_controller_test.rb

@@ -8,6 +8,15 @@ def test_it_serves_assets
     assert_equal response.headers["Cache-Control"], "max-age=31556952, public"
   end
 
+  def test_it_doesnt_trigger_csrf_failure
+    original_forgery_protection = ActionController::Base.allow_forgery_protection
+    ActionController::Base.allow_forgery_protection = true
+    get graphql_dashboard.static_path("dashboard.js")
+    assert_equal 200, response.status
+  ensure
+    ActionController::Base.allow_forgery_protection = original_forgery_protection
+  end
+
   def test_it_responds_404_for_others
     get graphql_dashboard.static_path("other.rb")
     assert_equal 404, response.status

spec/graphql/tracing/detailed_trace/active_record_backend_spec.rb

@@ -12,7 +12,7 @@ def new_backend(**kwargs)
     class DummyModel
       def self.find_by(id:)
         OpenStruct.new(
-          trace_data: Base64.encode64("DummyModel##{id}")
+          trace_data: "DummyModel##{id}"
         )
       end
     end

spec/graphql/tracing/perfetto_trace_spec.rb

@@ -6,6 +6,11 @@
   describe GraphQL::Tracing::PerfettoTrace do
     include PerfettoSnapshot
 
+    def trace_includes?(json_str, test_str)
+      json_str.include?(Base64.encode64(test_str).strip) ||
+        json_str.include?(test_str)
+    end
+
     class PerfettoSchema < GraphQL::Schema
       class BaseObject < GraphQL::Schema::Object
       end
@@ -46,7 +51,7 @@ def self.authorized?(obj, ctx)
         end
 
         def user
-          dataload_record(::User, object.user_id)
+          dataload_record(::User, object[:user_id])
         end
       end
 
@@ -57,7 +62,7 @@ class Book < BaseObject
         field :author, "PerfettoSchema::Author"
         field :other_book, Book
         def reviews
-          object.reviews.limit(2)
+          object.reviews.limit(2).map { |r| { stars: r.stars, user_id: r.user } }
         end
 
         def average_review
@@ -105,6 +110,27 @@ def thing(id:)
         def crash
           raise "Crash the query"
         end
+
+        class SecretInput < GraphQL::Schema::InputObject
+          argument :password, String
+        end
+
+        class SecretThing < GraphQL::Schema::Object
+          field :greeting, String
+        end
+        field :secret_field, SecretThing do
+          argument :cipher, String, required: false
+          argument :password, String, required: false
+          argument :input, [[SecretInput]], required: false
+        end
+
+        def secret_field(cipher: nil, password: nil, input: nil)
+          {
+            greeting: "Hello!",
+            cipher: cipher || "FALLBACK_CIPHER",
+            password: password || (input ? input[0][0][:password] : "FALLBACK_PASSWORD"),
+          }
+        end
       end
 
       query(Query)
@@ -159,6 +185,81 @@ def self.detailed_trace?(q)
       check_snapshot(data, "example-rails-#{Rails::VERSION::MAJOR}-#{Rails::VERSION::MINOR}.json")
     end
 
+    it "filters params with Rails.application.config.filter_parameters" do
+      query_str = 'query getStuff { secretField(cipher: "abcdef") { greeting } }'
+      res = PerfettoSchema.execute(query_str)
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      assert trace_includes?(json, "abcdef")
+      refute trace_includes?(json, "FILTERED")
+
+      if Rails.application.present?
+        prev_fp = Rails.application.config.filter_parameters
+        Rails.application.config.filter_parameters = ["ciph"]
+      else
+        Rails.application = OpenStruct.new(config: OpenStruct.new(filter_parameters: ["ciph"]))
+      end
+      res = PerfettoSchema.execute(query_str)
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      refute trace_includes?(json, "abcdef")
+      assert trace_includes?(json, "FILTERED")
+    ensure
+      if prev_fp
+        Rails.application.config.filter_parameters = prev_fp
+      else
+        Rails.application = nil
+      end
+    end
+
+    it "filters params with ActiveSupport" do
+      query_str = 'query getStuff { secretField(cipher: "abcdef") { greeting } }'
+      res = PerfettoSchema.execute(query_str)
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      assert trace_includes?(json, "abcdef")
+      refute trace_includes?(json, "FILTERED")
+
+      query_str = 'query getStuff { secretField(cipher: "abcdef") { greeting } }'
+      res = PerfettoSchema.execute(query_str)
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      assert trace_includes?(json, "abcdef")
+      refute trace_includes?(json, "FILTERED")
+
+      if ActiveSupport.respond_to?(:filter_parameters=)
+        begin
+          prev_fp = ActiveSupport.filter_parameters
+          ActiveSupport.filter_parameters = ["cipher"]
+          res = PerfettoSchema.execute(query_str)
+          json = res.context.query.current_trace.write(file: nil, debug_json: true)
+          refute trace_includes?(json, "abcdef")
+          assert trace_includes?(json, "[FILTERED]")
+
+          ActiveSupport.filter_parameters = ["password"]
+          res = PerfettoSchema.execute('query getStuff { secretField(input: [[{ password: "jklmn" }]]) { greeting } }')
+          json = res.context.query.current_trace.write(file: nil, debug_json: true)
+          assert trace_includes?(json, "password"), "Name is retained"
+          refute trace_includes?(json, "jklmn"), "Value is removed"
+          assert_includes json, "[FILTERED]"
+        ensure
+          ActiveSupport.filter_parameters = prev_fp
+        end
+      end
+    end
+
+    it "filters params without ActiveSupport" do
+      query_str = 'query getStuff { secretField(password: "qrstuv") { greeting } }'
+      res = PerfettoSchema.execute(query_str, context: { detailed_trace_filter: GraphQL::Tracing::PerfettoTrace::ArgumentsFilter.new })
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      assert trace_includes?(json, "FILTERED"), "The replacement string is present"
+      assert trace_includes?(json, "FALLBACK_CIPHER"), "Unfiltered values are present"
+      refute trace_includes?(json, "qrstuv"), "The password is obscured"
+
+      query_str = 'query getStuff { secretField(input: [[{ password: "lmnop" }]]) { greeting } }'
+      res = PerfettoSchema.execute(query_str, context: { detailed_trace_filter: GraphQL::Tracing::PerfettoTrace::ArgumentsFilter.new })
+      json = res.context.query.current_trace.write(file: nil, debug_json: true)
+      assert trace_includes?(json, "password"), "Name is retained"
+      refute trace_includes?(json, "lmnop"), "The password is obscured"
+      assert trace_includes?(json, "[FILTERED]"), "The replacement string is present"
+    end
+
     it "provides an error when google-protobuf isn't available" do
       stderr_and_stdout, _status = Open3.capture2e(%|ruby -e 'require "bundler/inline"; gemfile(true) { source("https://rubygems.org"); gem("graphql", path: "./") }; class MySchema < GraphQL::Schema; trace_with(GraphQL::Tracing::PerfettoTrace); end;'|)
       assert_includes stderr_and_stdout, "GraphQL::Tracing::PerfettoTrace can't be used because the `google-protobuf` gem wasn't available. Add it to your project, then try again."