Merge branch 'block-project-serialization' into 'master'

Add BlocksUnsafeSerialization to Project

See merge request gitlab-org/gitlab!81900

app/models/concerns/blocks_json_serialization.rb

-# frozen_string_literal: true
-
-# Overrides `as_json` and `to_json` to raise an exception when called in order
-# to prevent accidentally exposing attributes
-#
-# Not that would ever happen... but just in case.
-module BlocksJsonSerialization
-  extend ActiveSupport::Concern
-
-  JsonSerializationError = Class.new(StandardError)
-
-  def to_json(*)
-    raise JsonSerializationError,
-      "JSON serialization has been disabled on #{self.class.name}"
-  end
-
-  alias_method :as_json, :to_json
-end

app/models/concerns/blocks_unsafe_serialization.rb

+# frozen_string_literal: true
+
+# Overrides `#serializable_hash` to raise an exception when called without the `only` option
+# in order to prevent accidentally exposing attributes.
+#
+# An `unsafe: true` option can also be passed in to bypass this check.
+#
+# `#serializable_hash` is used by ActiveModel serializers like `ActiveModel::Serializers::JSON`
+# which overrides `#as_json` and `#to_json`.
+#
+module BlocksUnsafeSerialization
+  extend ActiveSupport::Concern
+  extend ::Gitlab::Utils::Override
+
+  UnsafeSerializationError = Class.new(StandardError)
+
+  override :serializable_hash
+  def serializable_hash(options = nil)
+    return super if allow_serialization?(options)
+
+    raise UnsafeSerializationError,
+      "Serialization has been disabled on #{self.class.name}"
+  end
+
+  private
+
+  def allow_serialization?(options = nil)
+    return false unless options
+
+    !!(options[:only] || options[:unsafe])
+  end
+end

app/models/project.rb

@@ -38,6 +38,7 @@ class Project < ApplicationRecord
   include GitlabRoutingHelper
   include BulkMemberAccessLoad
   include RunnerTokenExpirationInterval
+  include BlocksUnsafeSerialization
 
   extend Gitlab::Cache::RequestCache
   extend Gitlab::Utils::Override
@@ -3047,6 +3048,10 @@ class Project < ApplicationRecord
       Projects::SyncEvent.enqueue_worker
     end
   end
+
+  def allow_serialization?(options = nil)
+    Feature.disabled?(:block_project_serialization, self, default_enabled: :yaml) || super
+  end
 end
 
 Project.prepend_mod_with('Project')

app/models/project_import_data.rb

@@ -14,7 +14,12 @@ class ProjectImportData < ApplicationRecord
                  insecure_mode: true,
                  algorithm: 'aes-256-cbc'
 
-  serialize :data, JSON # rubocop:disable Cop/ActiveRecordSerialize
+  # NOTE
+  # We are serializing a project as `data` in an "unsafe" way here
+  # because the credentials are necessary for a successful import.
+  # This is safe because the serialization is only going between rails
+  # and the database, never to any end users.
+  serialize :data, Serializers::UnsafeJson # rubocop:disable Cop/ActiveRecordSerialize
 
   validates :project, presence: true
 

app/models/user.rb

@@ -16,7 +16,7 @@ class User < ApplicationRecord
   include FeatureGate
   include CreatedAtFilterable
   include BulkMemberAccessLoad
-  include BlocksJsonSerialization
+  include BlocksUnsafeSerialization
   include WithUploads
   include OptionallySearch
   include FromUnion

config/feature_flags/development/block_project_serialization.yml

+---
+name: block_project_serialization
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81900
+rollout_issue_url:
+milestone: '14.9'
+type: development
+group: group::workspace
+default_enabled: false

lib/serializers/unsafe_json.rb

+# frozen_string_literal: true
+
+module Serializers
+  class UnsafeJson
+    class << self
+      def dump(obj)
+        obj.to_json(unsafe: true)
+      end
+
+      delegate :load, to: :JSON
+    end
+  end
+end

spec/controllers/boards/lists_controller_spec.rb

@@ -208,7 +208,7 @@ RSpec.describe Boards::ListsController do
       sign_in(user)
 
       params = { namespace_id: project.namespace.to_param,
-                 project_id: project,
+                 project_id: project.id,
                  board_id: board.to_param,
                  id: list.to_param,
                  list: { position: position },
@@ -221,7 +221,7 @@ RSpec.describe Boards::ListsController do
       sign_in(user)
 
       params = { namespace_id: project.namespace.to_param,
-                 project_id: project,
+                 project_id: project.id,
                  board_id: board.to_param,
                  id: list.to_param,
                  list: setting,

spec/lib/banzai/filter/issuable_reference_expansion_filter_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe Banzai::Filter::IssuableReferenceExpansionFilter do
   end
 
   it 'ignores non-issuable links' do
-    link = create_link('text', project: project, reference_type: 'issue')
+    link = create_link('text', project: project.id, reference_type: 'issue')
     doc = filter(link, context)
 
     expect(doc.css('a').last.text).to eq('text')

spec/lib/serializers/unsafe_json_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'oj'
+
+RSpec.describe Serializers::UnsafeJson do
+  let(:result) { double(:result) }
+
+  describe '.dump' do
+    let(:obj) { { key: "value" } }
+
+    it 'calls object#to_json with unsafe: true and returns the result' do
+      expect(obj).to receive(:to_json).with(unsafe: true).and_return(result)
+      expect(described_class.dump(obj)).to eq(result)
+    end
+  end
+
+  describe '.load' do
+    let(:data_string) { '{"key":"value","variables":[{"key":"VAR1","value":"VALUE1"}]}' }
+    let(:data_hash) { Gitlab::Json.parse(data_string) }
+
+    it 'calls JSON.load and returns the result' do
+      expect(JSON).to receive(:load).with(data_hash).and_return(result)
+      expect(described_class.load(data_hash)).to eq(result)
+    end
+  end
+end

spec/models/concerns/blocks_json_serialization_spec.rb → spec/models/concerns/blocks_unsafe_serialization_spec.rb

@@ -2,21 +2,16 @@
 
 require 'spec_helper'
 
-RSpec.describe BlocksJsonSerialization do
+RSpec.describe BlocksUnsafeSerialization do
   before do
     stub_const('DummyModel', Class.new)
     DummyModel.class_eval do
-      include BlocksJsonSerialization
+      include ActiveModel::Serializers::JSON
+      include BlocksUnsafeSerialization
     end
   end
 
-  it 'blocks as_json' do
-    expect { DummyModel.new.as_json }
-      .to raise_error(described_class::JsonSerializationError, /DummyModel/)
-  end
-
-  it 'blocks to_json' do
-    expect { DummyModel.new.to_json }
-      .to raise_error(described_class::JsonSerializationError, /DummyModel/)
+  it_behaves_like 'blocks unsafe serialization' do
+    let(:object) { DummyModel.new }
   end
 end

spec/models/project_spec.rb

@@ -8022,6 +8022,20 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe 'serialization' do
+    let(:object) { build(:project) }
+
+    it_behaves_like 'blocks unsafe serialization'
+
+    context 'when feature flag block_project_serialization is disabled' do
+      before do
+        stub_feature_flags(block_project_serialization: false)
+      end
+
+      it_behaves_like 'allows unsafe serialization'
+    end
+  end
+
   describe '#runners_token' do
     let_it_be(:project) { create(:project) }
 

spec/models/user_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe User do
     it { is_expected.to include_module(Referable) }
     it { is_expected.to include_module(Sortable) }
     it { is_expected.to include_module(TokenAuthenticatable) }
-    it { is_expected.to include_module(BlocksJsonSerialization) }
+    it { is_expected.to include_module(BlocksUnsafeSerialization) }
     it { is_expected.to include_module(AsyncDeviseEmail) }
   end
 

spec/support/shared_examples/blocks_unsafe_serialization_shared_examples.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Requires a context with:
+# - object
+#
+RSpec.shared_examples 'blocks unsafe serialization' do
+  it 'blocks as_json' do
+    expect { object.as_json }.to raise_error(described_class::UnsafeSerializationError, /#{object.class.name}/)
+  end
+
+  it 'blocks to_json' do
+    expect { object.to_json }.to raise_error(described_class::UnsafeSerializationError, /#{object.class.name}/)
+  end
+end
+
+RSpec.shared_examples 'allows unsafe serialization' do
+  it 'allows as_json' do
+    expect { object.as_json }.not_to raise_error
+  end
+
+  it 'allows to_json' do
+    expect { object.to_json }.not_to raise_error
+  end
+end

spec/support/shared_examples/serializers/note_entity_shared_examples.rb

@@ -68,6 +68,29 @@ RSpec.shared_examples 'note entity' do
     end
   end
 
+  describe ':outdated_line_change_path' do
+    before do
+      allow(note).to receive(:show_outdated_changes?).and_return(show_outdated_changes)
+    end
+
+    context 'when note shows outdated changes' do
+      let(:show_outdated_changes) { true }
+
+      it 'returns correct outdated_line_change_namespace_project_note_path' do
+        path = "/#{note.project.namespace.path}/#{note.project.path}/notes/#{note.id}/outdated_line_change"
+        expect(subject[:outdated_line_change_path]).to eq(path)
+      end
+    end
+
+    context 'when note does not show outdated changes' do
+      let(:show_outdated_changes) { false }
+
+      it 'does not expose outdated_line_change_path' do
+        expect(subject).not_to include(:outdated_line_change_path)
+      end
+    end
+  end
+
   context 'when note was edited' do
     before do
       note.update!(updated_at: 1.minute.from_now, updated_by: user)