Merge branch 'fix-wiki-page-content-validation' into 'master'

Fix content validation for existing wiki pages

See merge request gitlab-org/gitlab!37310

app/models/wiki_page.rb

@@ -285,7 +285,14 @@ class WikiPage
   end
 
   def content_changed?
-    attributes[:content] != page&.text_data
+    if persisted?
+      # gollum-lib always converts CRLFs to LFs in Gollum::Wiki#normalize,
+      # so we need to do the same here.
+      # Also see https://gitlab.com/gitlab-org/gitlab/-/issues/21431
+      raw_content.delete("\r") != page&.text_data
+    else
+      raw_content.present?
+    end
   end
 
   # Updates the current @attributes hash by merging a hash of params

changelogs/unreleased/fix-wiki-page-content-validation.yml

+---
+title: Fix content validation for existing wiki pages
+merge_request: 37310
+author:
+type: fixed

spec/features/projects/wiki/user_updates_wiki_page_spec.rb

@@ -234,4 +234,30 @@ RSpec.describe 'User updates wiki page' do
 
     it_behaves_like 'wiki file attachments'
   end
+
+  context 'when an existing page exceeds the content size limit' do
+    let_it_be(:project) { create(:project, :wiki_repo) }
+    let!(:wiki_page) { create(:wiki_page, wiki: project.wiki, content: "one\ntwo\nthree") }
+
+    before do
+      stub_application_setting(wiki_page_max_content_bytes: 10)
+
+      visit wiki_page_path(wiki_page.wiki, wiki_page, action: :edit)
+    end
+
+    it 'allows changing the title if the content does not change' do
+      fill_in 'Title', with: 'new title'
+      click_on 'Save changes'
+
+      expect(page).to have_content('Wiki was successfully updated.')
+    end
+
+    it 'shows a validation error when trying to change the content' do
+      fill_in 'Content', with: 'new content'
+      click_on 'Save changes'
+
+      expect(page).to have_content('The form contains the following error:')
+      expect(page).to have_content('Content is too long (11 Bytes). The maximum size is 10 Bytes.')
+    end
+  end
 end

spec/models/wiki_page_spec.rb

@@ -7,7 +7,11 @@ RSpec.describe WikiPage do
   let(:container) { create(:project, :wiki_repo) }
   let(:wiki) { Wiki.for_container(container, user) }
   let(:new_page) { build(:wiki_page, wiki: wiki, title: 'test page', content: 'test content') }
-  let(:existing_page) { create(:wiki_page, wiki: wiki, title: 'test page', content: 'test content', message: 'test commit') }
+
+  let(:existing_page) do
+    create(:wiki_page, wiki: wiki, title: 'test page', content: 'test content', message: 'test commit')
+    wiki.find_page('test page')
+  end
 
   subject { new_page }
 
@@ -257,59 +261,66 @@ RSpec.describe WikiPage do
       subject.attributes.delete(:title)
 
       expect(subject).not_to be_valid
-      expect(subject.errors.keys).to contain_exactly(:title)
+      expect(subject.errors.messages).to eq(title: ["can't be blank"])
     end
 
     it "validates presence of content" do
       subject.attributes.delete(:content)
 
       expect(subject).not_to be_valid
-      expect(subject.errors.keys).to contain_exactly(:content)
+      expect(subject.errors.messages).to eq(content: ["can't be blank"])
     end
 
-    describe 'content size validation' do
-      let(:limit) { 10 }
-
-      before do
-        stub_application_setting(wiki_page_max_content_bytes: limit)
-      end
+    describe '#validate_content_size_limit' do
+      context 'with a new page' do
+        before do
+          stub_application_setting(wiki_page_max_content_bytes: 10)
+        end
 
-      it 'accepts content below the limit' do
-        subject.attributes[:content] = 'a' * 10
+        it 'accepts content below the limit' do
+          subject.attributes[:content] = 'a' * 10
 
-        expect(subject).to be_valid
-      end
+          expect(subject).to be_valid
+        end
 
-      it 'rejects content exceeding the limit' do
-        subject.attributes[:content] = 'a' * 11
+        it 'rejects content exceeding the limit' do
+          subject.attributes[:content] = 'a' * 11
 
-        expect(subject).not_to be_valid
-        expect(subject.errors.messages).to eq(
-          content: ['is too long (11 Bytes). The maximum size is 10 Bytes.']
-        )
-      end
+          expect(subject).not_to be_valid
+          expect(subject.errors.messages).to eq(
+            content: ['is too long (11 Bytes). The maximum size is 10 Bytes.']
+          )
+        end
 
-      it 'counts content size in bytes rather than characters' do
-        subject.attributes[:content] = '💩💩💩'
+        it 'counts content size in bytes rather than characters' do
+          subject.attributes[:content] = '💩💩💩'
 
-        expect(subject).not_to be_valid
-        expect(subject.errors.messages).to eq(
-          content: ['is too long (12 Bytes). The maximum size is 10 Bytes.']
-        )
+          expect(subject).not_to be_valid
+          expect(subject.errors.messages).to eq(
+            content: ['is too long (12 Bytes). The maximum size is 10 Bytes.']
+          )
+        end
       end
 
       context 'with an existing page exceeding the limit' do
-        let(:limit) { existing_page.content.bytesize - 1 }
+        subject { existing_page }
+
+        before do
+          subject
+          stub_application_setting(wiki_page_max_content_bytes: 11)
+        end
 
         it 'accepts content when it has not changed' do
-          expect(existing_page).to be_valid
+          expect(subject).to be_valid
         end
 
         it 'rejects content when it has changed' do
-          existing_page.attributes[:content] = 'a' * (limit + 1)
+          subject.attributes[:content] = 'a' * 12
 
-          expect(existing_page).not_to be_valid
-          expect(existing_page.errors.keys).to contain_exactly(:content)
+          expect(subject).not_to be_valid
+          expect(subject.errors.messages).to eq(
+            content: ['is too long (12 Bytes). The maximum size is 11 Bytes.']
+          )
         end
       end
     end
@@ -753,22 +764,16 @@ RSpec.describe WikiPage do
     context 'with a new page' do
       subject { new_page }
 
-      it 'returns false if content is nil' do
-        subject.attributes[:content] = nil
-
-        expect(subject.content_changed?).to be(false)
-      end
-
       it 'returns true if content is set' do
         subject.attributes[:content] = 'new'
 
         expect(subject.content_changed?).to be(true)
       end
 
-      it 'returns true if content is blank' do
-        subject.attributes[:content] = ''
+      it 'returns false if content is blank' do
+        subject.attributes[:content] = ' '
 
-        expect(subject.content_changed?).to be(true)
+        expect(subject.content_changed?).to be(false)
       end
     end
 
@@ -790,6 +795,20 @@ RSpec.describe WikiPage do
 
         expect(subject.content_changed?).to be(true)
       end
+
+      it 'returns true if content is changed to a blank string' do
+        subject.attributes[:content] = ' '
+
+        expect(subject.content_changed?).to be(true)
+      end
+
+      it 'returns false if only the newline format has changed' do
+        expect(subject.page).to receive(:text_data).and_return("foo\nbar")
+
+        subject.attributes[:content] = "foo\r\nbar"
+
+        expect(subject.content_changed?).to be(false)
+      end
     end
   end