Exclude sensitive attributes in serializable_hash by default

Fields declared as encrypted via below is excluded by serializable_hash:

- add_authentication_token_field
- attr_encrypted

Skip SensitiveSerializableHash within JsonCache

We skip SensitiveSerializableHash in JsonCache as the current behavior
expects to serialize the entirety of the object, specifically Geo.

Also fix other broken specs from using SensitiveSerializableHash

app/models/application_record.rb

@@ -5,6 +5,7 @@ class ApplicationRecord < ActiveRecord::Base
   include Transactions
   include LegacyBulkInsert
   include CrossDatabaseModification
+  include SensitiveSerializableHash
 
   self.abstract_class = true
 

app/models/concerns/sensitive_serializable_hash.rb

+# frozen_string_literal: true
+
+module SensitiveSerializableHash
+  extend ActiveSupport::Concern
+
+  # Override serializable_hash to exclude sensitive attributes by default
+  #
+  # In general, prefer NOT to use serializable_hash / to_json / as_json in favor
+  # of serializers / entities instead which has an allowlist of attributes
+  def serializable_hash(options = nil)
+    return super(options) if options && options[:unsafe_serialization_hash]
+
+    options = options.try(:dup) || {}
+    options[:except] = Array(options[:except]).dup
+
+    if self.class.respond_to?(:token_authenticatable_fields)
+      options[:except].concat self.class.token_authenticatable_fields
+
+      # See https://gitlab.com/gitlab-org/security/gitlab/-/tree/master/app/models/concerns/token_authenticatable_strategies
+      # TODO expose this fields from the TokenAuthenticatable module instead
+      options[:except].concat self.class.token_authenticatable_fields.map { |token_field| "#{token_field}_expires_at" }
+      options[:except].concat self.class.token_authenticatable_fields.map { |token_field| "#{token_field}_digest" }
+      options[:except].concat self.class.token_authenticatable_fields.map { |token_field| "#{token_field}_encrypted" }
+    end
+
+    if self.class.respond_to?(:encrypted_attributes)
+      options[:except].concat self.class.encrypted_attributes.keys
+
+      # Per https://github.com/attr-encrypted/attr_encrypted/blob/a96693e9a2a25f4f910bf915e29b0f364f277032/lib/attr_encrypted.rb#L413
+      options[:except].concat self.class.encrypted_attributes.values.map { |v| v[:attribute] }
+      options[:except].concat self.class.encrypted_attributes.values.map { |v| "#{v[:attribute]}_iv" }
+    end
+
+    super(options)
+  end
+end

app/models/concerns/token_authenticatable.rb

@@ -8,6 +8,10 @@ module TokenAuthenticatable
       @encrypted_token_authenticatable_fields ||= []
     end
 
+    def token_authenticatable_fields
+      @token_authenticatable_fields ||= []
+    end
+
     private
 
     def add_authentication_token_field(token_field, options = {})
@@ -82,9 +86,5 @@ module TokenAuthenticatable
       @token_authenticatable_module ||=
         const_set(:TokenAuthenticatable, Module.new).tap(&method(:include))
     end
-
-    def token_authenticatable_fields
-      @token_authenticatable_fields ||= []
-    end
   end
 end

lib/gitlab/json_cache.rb

@@ -43,7 +43,9 @@ module Gitlab
     end
 
     def write(key, value, options = nil)
-      backend.write(cache_key(key), value.to_json, options)
+      # As we use json as the serialization format, return everything from
+      # ActiveModel objects included encrypted values.
+      backend.write(cache_key(key), value.to_json(unsafe_serialization_hash: true), options)
     end
 
     def fetch(key, options = {}, &block)

spec/features/projects/jobs/user_triggers_manual_job_with_variables_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe 'User triggers manual job with variables', :js do
 
     wait_for_requests
 
-    expect(build.job_variables.as_json).to contain_exactly(
+    expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
       hash_including('key' => 'key_name', 'value' => 'key_value'))
   end
 end

spec/models/concerns/sensitive_serializable_hash_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SensitiveSerializableHash do
+  describe '#serializable_hash' do
+    shared_examples "attr_encrypted attribute" do |klass, attribute_name|
+      context "#{klass.name}\##{attribute_name}" do
+        let(:attributes) { [attribute_name, "encrypted_#{attribute_name}", "encrypted_#{attribute_name}_iv"] }
+
+        it 'has a encrypted_attributes field' do
+          expect(klass.encrypted_attributes).to include(attribute_name.to_sym)
+        end
+
+        it 'does not include the attribute in serializable_hash', :aggregate_failures do
+          attributes.each do |attribute|
+            expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+            expect(model.serializable_hash).not_to include(attribute)
+            expect(model.to_json).not_to include(attribute)
+            expect(model.as_json).not_to include(attribute)
+          end
+        end
+
+        context 'unsafe_serialization_hash option' do
+          it 'includes the field in serializable_hash' do
+            attributes.each do |attribute|
+              expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+              expect(model.serializable_hash(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.to_json(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.as_json(unsafe_serialization_hash: true)).to include(attribute)
+            end
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'attr_encrypted attribute', WebHook, 'token' do
+      let_it_be(:model) { create(:system_hook) }
+    end
+
+    it_behaves_like 'attr_encrypted attribute', Ci::InstanceVariable, 'value' do
+      let_it_be(:model) { create(:ci_instance_variable) }
+    end
+
+    shared_examples "add_authentication_token_field attribute" do |klass, attribute_name, encrypted_attribute: true, digest_attribute: false|
+      context "#{klass.name}\##{attribute_name}" do
+        let(:attributes) do
+          if digest_attribute
+            ["#{attribute_name}_digest"]
+          elsif encrypted_attribute
+            [attribute_name, "#{attribute_name}_encrypted"]
+          else
+            [attribute_name]
+          end
+        end
+
+        it 'has a add_authentication_token_field field' do
+          expect(klass.token_authenticatable_fields).to include(attribute_name.to_sym)
+        end
+
+        it 'does not include the attribute in serializable_hash', :aggregate_failures do
+          attributes.each do |attribute|
+            expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+            expect(model.serializable_hash).not_to include(attribute)
+            expect(model.to_json).not_to include(attribute)
+            expect(model.as_json).not_to include(attribute)
+          end
+        end
+
+        context 'unsafe_serialization_hash option' do
+          it 'includes the field in serializable_hash' do
+            attributes.each do |attribute|
+              expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+              expect(model.serializable_hash(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.to_json(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.as_json(unsafe_serialization_hash: true)).to include(attribute)
+            end
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', Ci::Runner, 'token' do
+      let_it_be(:model) { create(:ci_runner) }
+
+      it 'does not include token_expires_at in serializable_hash' do
+        attribute = 'token_expires_at'
+
+        expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+        expect(model.serializable_hash).not_to include(attribute)
+        expect(model.to_json).not_to include(attribute)
+        expect(model.as_json).not_to include(attribute)
+      end
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', ApplicationSetting, 'health_check_access_token', encrypted_attribute: false do
+      # health_check_access_token_encrypted column does not exist
+      let_it_be(:model) { create(:application_setting) }
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', PersonalAccessToken, 'token', encrypted_attribute: false, digest_attribute: true do
+      # PersonalAccessToken only has token_digest column
+      let_it_be(:model) { create(:personal_access_token) }
+    end
+  end
+end

spec/services/ci/job_artifacts/create_service_spec.rb

@@ -175,7 +175,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do
         end
 
         expect(subject[:status]).to eq(:success)
-        expect(job.job_variables.as_json).to contain_exactly(
+        expect(job.job_variables.as_json(only: [:key, :value, :source])).to contain_exactly(
           hash_including('key' => 'KEY1', 'value' => 'VAR1', 'source' => 'dotenv'),
           hash_including('key' => 'KEY2', 'value' => 'VAR2', 'source' => 'dotenv'))
       end

spec/services/ci/parse_dotenv_artifact_service_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
       it 'parses the artifact' do
         expect(subject[:status]).to eq(:success)
 
-        expect(build.job_variables.as_json).to contain_exactly(
+        expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
           hash_including('key' => 'KEY1', 'value' => 'VAR1'),
           hash_including('key' => 'KEY2', 'value' => 'VAR2'))
       end
@@ -57,7 +57,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
 
           expect(subject[:status]).to eq(:success)
 
-          expect(build.job_variables.as_json).to contain_exactly(
+          expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
             hash_including('key' => 'KEY1', 'value' => 'VAR4'),
             hash_including('key' => 'KEY2', 'value' => 'VAR3'))
         end
@@ -101,7 +101,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'trims the trailing space' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1'))
           end
         end
@@ -112,7 +112,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY', 'value' => 'VARCONTAINING=EQLS'))
           end
         end
@@ -133,7 +133,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'skateboard', 'value' => '🛹'))
           end
         end
@@ -154,7 +154,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'V A R 1'))
           end
         end
@@ -165,7 +165,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => '"VAR1"'))
           end
         end
@@ -176,7 +176,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => "'VAR1'"))
           end
         end
@@ -187,7 +187,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => '"  VAR1  "'))
           end
         end
@@ -208,7 +208,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => ''))
           end
         end
@@ -250,7 +250,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'does not support variable expansion in dotenv parser' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1'),
               hash_including('key' => 'KEY2', 'value' => '${KEY1}_Test'))
           end
@@ -284,7 +284,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'does not support comment in dotenv parser' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1         # This is variable'))
           end
         end