Merge branch 'security-1164-confidential-issue-17-3' into '17-3-stable-ee'

Improve GraphQL log security

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4400



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Reviewed-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Co-authored-by: Radamanthus Batnag <rbatnag@gitlab.com>

lib/gitlab/graphql/query_analyzers/ast/logger_analyzer.rb

@@ -9,6 +9,7 @@ class LoggerAnalyzer < GraphQL::Analysis::AST::Analyzer
           DEPTH_ANALYZER = GraphQL::Analysis::AST::QueryDepth
           FIELD_USAGE_ANALYZER = GraphQL::Analysis::AST::FieldUsage
           ALL_ANALYZERS = [COMPLEXITY_ANALYZER, DEPTH_ANALYZER, FIELD_USAGE_ANALYZER].freeze
+          FILTER_PARAMETERS = (::Rails.application.config.filter_parameters + [/password/i]).freeze
           def initialize(query)
             super
@@ -91,7 +92,7 @@ def process_variables(variables)
           def filter_sensitive_variables(variables)
             ActiveSupport::ParameterFilter
-              .new(::Rails.application.config.filter_parameters)
+              .new(FILTER_PARAMETERS)
               .filter(variables)
           end

lib/gitlab/graphql/tracers/instrumentation_tracer.rb

@@ -5,6 +5,8 @@ module Graphql
     module Tracers
       # This tracer writes logs for certain trace events.
       module InstrumentationTracer
+        MUTATION_REGEXP = /^mutation/
+
         # All queries pass through a multiplex, even if only one query is executed
         # https://github.com/rmosolgo/graphql-ruby/blob/43e377b5b743a9102381d6ad3adaaed13ff5b6dd/lib/graphql/schema.rb#L1303
         #
@@ -71,7 +73,7 @@ def log_execute_query(query: nil, duration_s: 0, exception: nil)
             operation_fingerprint: query.operation_fingerprint,
             is_mutation: query.mutation?,
             variables: clean_variables(query.provided_variables),
-            query_string: query.query_string
+            query_string: clean_query_string(query)
           }
           token_info = auth_token_info(query)
@@ -96,11 +98,21 @@ def auth_token_info(query)
         def clean_variables(variables)
           filtered = ActiveSupport::ParameterFilter
-            .new(::Rails.application.config.filter_parameters)
+            .new(::Gitlab::Graphql::QueryAnalyzers::AST::LoggerAnalyzer::FILTER_PARAMETERS)
             .filter(variables)
           filtered&.to_s
         end
+
+        def clean_query_string(query)
+          return query.query_string unless mutation?(query)
+
+          query.sanitized_query_string
+        end
+
+        def mutation?(query)
+          query.query_string =~ ::Gitlab::Graphql::Tracers::InstrumentationTracer::MUTATION_REGEXP
+        end
       end
     end
   end

spec/requests/api/graphql/instrumentation_tracer_spec.rb

@@ -8,8 +8,8 @@
   let_it_be(:user) { create(:user, username: 'instrumentation-tester') }
   describe "logging" do
-    it "logs a message for each query in a request" do
-      common_log_info = {
+    let_it_be(:common_log_info) do
+      {
         "correlation_id" => be_a(String),
         :trace_type => "execute_query",
         :query_fingerprint => be_a(String),
@@ -23,7 +23,9 @@
         "query_analysis.duration_s" => be_a(Float),
         "meta.caller_id" => "graphql:unknown"
       }
+    end
+    it "logs a message for each query in a request" do
       expect(Gitlab::GraphqlLogger).to receive(:info).with(a_hash_including({
         **common_log_info,
         variables: "{\"test\"=>\"hello world\"}",
@@ -68,6 +70,36 @@
       expect(json_response.size).to eq(2)
     end
+
+    context "with a mutation query" do
+      let_it_be_with_reload(:package) { create(:package) }
+      let(:project) { package.project }
+
+      let(:query) do
+        <<~GQL
+          errors
+        GQL
+      end
+
+      let(:id) { package.to_global_id.to_s }
+      let(:params) { { id: id } }
+      let(:mutation) { graphql_mutation(:destroy_package, params, query) }
+
+      let(:expected_variables) { "{\"destroyPackageInput\"=>{\"id\"=>\"#{id}\"}}" }
+      let(:sanitized_mutation_query_string) do
+        "mutation {\n  destroyPackage(input: {id: \"<REDACTED>\"}) {\n    errors\n  }\n}"
+      end
+
+      it "sanitizes the query string" do
+        expect(Gitlab::GraphqlLogger).to receive(:info).with(a_hash_including({
+          **common_log_info,
+          variables: expected_variables,
+          query_string: sanitized_mutation_query_string
+        }))
+
+        post_graphql_mutation(mutation, current_user: user)
+      end
+    end
   end
   describe "metrics" do