Commit f85e5897 authored by Markus Koller's avatar Markus Koller

Fix content validation for existing wiki pages

This new validation didn't work correctly yet because of two reasons:

- When loading the wiki page in the `#update` controller action, the
  content in `attributes[:content]` won't be initialized which causes
  `content_changed?` to return true. Our factory on the other hand
  sets `attributes[:content]` as part of its initialization.

  We can fix this by using `raw_content` instead in `content_changed?`,
  and verify this in the specs by reloading the existing page from the
  wiki.

- Gollum converts CRLF line endings ("\r\n") to LFs ("\n"), so we also
  need to do this conversion in `WikiPage#content_changed?`.
parent 2a91735c
......@@ -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
......
---
title: Fix content validation for existing wiki pages
merge_request: 37310
author:
type: fixed
......@@ -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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment