Merge branch 'security-460289-confidential-issue-17-1' into '17-1-stable-ee'

Improve GraphQL log security

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



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Greg Myers <gmyers@gitlab.com>
Approved-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/logger_tracer.rb

@@ -5,6 +5,8 @@ module Graphql
     module Tracers
       # This tracer writes logs for certain trace events.
       module LoggerTracer
+        MUTATION_REGEXP = /^mutation/
+
         def execute_query(query:)
           start_time = ::Gitlab::Metrics::System.monotonic_time
@@ -32,7 +34,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)
@@ -55,11 +57,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::LoggerTracer::MUTATION_REGEXP
+        end
       end
     end
   end

spec/requests/api/graphql/logger_tracer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Gitlab::Graphql::Tracers::Logger integration test', :aggregate_failures, feature_category: :integrations do
+  include GraphqlHelpers
+
+  let_it_be(:user) { create(:user, username: 'instrumentation-tester') }
+
+  describe "logging" do
+    let_it_be(:common_log_info) do
+      {
+        "correlation_id" => be_a(String),
+        :trace_type => "execute_query",
+        :query_fingerprint => be_a(String),
+        :duration_s => be_a(Float),
+        :operation_fingerprint => be_a(String),
+        "meta.remote_ip" => "127.0.0.1",
+        "meta.feature_category" => "not_owned",
+        "meta.user" => "instrumentation-tester",
+        "meta.user_id" => user.id,
+        "meta.client_id" => "user/#{user.id}",
+        "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\"}",
+        query_string: "{ echo(text: \"$test\") }"
+      }))
+
+      expect(Gitlab::GraphqlLogger).to receive(:info).with(a_hash_including({
+        **common_log_info,
+        variables: "{}",
+        query_string: "{ currentUser{\n  username\n}\n }"
+      }))
+
+      queries = [
+        { query: graphql_query_for('echo', { 'text' => '$test' }, []),
+          variables: { test: "hello world" } },
+        { query: graphql_query_for('currentUser', {}, ["username"]) }
+      ]
+
+      post_multiplex(queries, current_user: user)
+
+      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
+end