Tweak wiki page title handling

- Add specs for handling of special characters.
- Fix `title_changed?` for all cases.
- Remove redundant `persisted` argument.
- Don't unescape title returned by gollum, it's not escaped anymore now.

GITALY_SERVER_VERSION

-1.87.0
+12.9.0-rc1

app/models/project_wiki.rb

@@ -107,7 +107,7 @@ def list_pages(limit: 0, sort: nil, direction: DIRECTION_ASC, load_content: fals
       direction_desc: direction == DIRECTION_DESC,
       load_content: load_content
     ).map do |page|
-      WikiPage.new(self, page, true)
+      WikiPage.new(self, page)
     end
   end
 
@@ -122,7 +122,7 @@ def find_page(title, version = nil)
     page_title, page_dir = page_title_and_dir(title)
 
     if page = wiki.page(title: page_title, version: version, dir: page_dir)
-      WikiPage.new(self, page, true)
+      WikiPage.new(self, page)
     end
   end
 

app/models/wiki_page.rb

@@ -70,10 +70,9 @@ def hook_attrs
     Gitlab::HookData::WikiPageBuilder.new(self).build
   end
 
-  def initialize(wiki, page = nil, persisted = false)
+  def initialize(wiki, page = nil)
     @wiki       = wiki
     @page       = page
-    @persisted  = persisted
     @attributes = {}.with_indifferent_access
 
     set_attributes if persisted?
@@ -94,11 +93,7 @@ def human_title
 
   # The formatted title of this page.
   def title
-    if @attributes[:title]
-      CGI.unescape_html(self.class.unhyphenize(@attributes[:title]))
-    else
-      ""
-    end
+    @attributes[:title] || ''
   end
 
   # Sets the title of this page.
@@ -176,7 +171,7 @@ def latest?
   # Returns boolean True or False if this instance
   # has been fully created on disk or not.
   def persisted?
-    @persisted == true
+    @page.present?
   end
 
   # Creates a new Wiki Page.
@@ -196,7 +191,7 @@ def persisted?
   def create(attrs = {})
     update_attributes(attrs)
 
-    save(page_details: title) do
+    save do
       wiki.create_page(title, content, format, attrs[:message])
     end
   end
@@ -222,18 +217,12 @@ def update(attrs = {})
 
     update_attributes(attrs)
 
-    if title_changed?
-      page_details = title
-
-      if wiki.find_page(page_details).present?
-        @attributes[:title] = @page.url_path
-        raise PageRenameError
-      end
-    else
-      page_details = @page.url_path
+    if title.present? && title_changed? && wiki.find_page(title).present?
+      @attributes[:title] = @page.title
+      raise PageRenameError
     end
 
-    save(page_details: page_details) do
+    save do
       wiki.update_page(
         @page,
         content: content,
@@ -266,7 +255,14 @@ def id
   end
 
   def title_changed?
-    title.present? && (@page.nil? || self.class.unhyphenize(@page.url_path) != title)
+    if persisted?
+      old_title, old_dir = wiki.page_title_and_dir(self.class.unhyphenize(@page.url_path))
+      new_title, new_dir = wiki.page_title_and_dir(title)
+
+      new_title != old_title || (title.include?('/') && new_dir != old_dir)
+    else
+      title.present?
+    end
   end
 
   # Updates the current @attributes hash by merging a hash of params
@@ -313,26 +309,24 @@ def set_attributes
     attributes[:format] = @page.format
   end
 
-  def save(page_details:)
-    return unless valid?
+  def save
+    return false unless valid?
 
     unless yield
       errors.add(:base, wiki.error_message)
       return false
     end
 
-    page_title, page_dir = wiki.page_title_and_dir(page_details)
-    gitlab_git_wiki = wiki.wiki
-    @page = gitlab_git_wiki.page(title: page_title, dir: page_dir)
-
+    @page = wiki.find_page(title).page
     set_attributes
-    @persisted = errors.blank?
+
+    true
   end
 
   def validate_path_limits
     *dirnames, title = @attributes[:title].split('/')
 
-    if title.bytesize > MAX_TITLE_BYTES
+    if title && title.bytesize > MAX_TITLE_BYTES
       errors.add(:title, _("exceeds the limit of %{bytes} bytes") % { bytes: MAX_TITLE_BYTES })
     end
 

app/views/projects/wikis/_form.html.haml

@@ -12,7 +12,7 @@
   .form-group.row
     .col-sm-12= f.label :title, class: 'control-label-full-width'
     .col-sm-12
-      = f.text_field :title, class: 'form-control qa-wiki-title-textbox', value: @page.title, required: true, autofocus: !@page.persisted?, placeholder: _('Wiki|Page title')
+      = f.text_field :title, class: 'form-control qa-wiki-title-textbox', value: @page.title, required: true, autofocus: !@page.persisted?, placeholder: s_('Wiki|Page title')
       %span.d-inline-block.mw-100.prepend-top-5
         = icon('lightbulb-o')
         - if @page.persisted?

changelogs/unreleased/16131-wiki-titles-with-special-characters.yml

+---
+title: Tweak wiki page title handling
+merge_request: 25647
+author:
+type: changed

doc/user/project/wiki/index.md

@@ -60,6 +60,14 @@ if you clone the wiki repository locally. All uploaded files prior to GitLab
 11.3 are stored in GitLab itself. If you want them to be part of the wiki's Git
 repository, you will have to upload them again.
 
+### Special characters in page titles
+
+Wiki pages are stored as files in a Git repository, so certain characters have a special meaning:
+
+- Spaces are converted into hyphens when storing a page.
+- Hyphens (`-`) are converted back into spaces when displaying a page.
+- Slashes (`/`) can't be used, because they're used as path separator.
+
 ### Length restrictions for file and directory names
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24364) in GitLab 12.8.

spec/factories/wiki_pages.rb

@@ -16,7 +16,7 @@
 
     page { OpenStruct.new(url_path: 'some-name') }
     association :wiki, factory: :project_wiki, strategy: :build
-    initialize_with { new(wiki, page, true) }
+    initialize_with { new(wiki, page) }
 
     before(:create) do |page, evaluator|
       page.attributes = evaluator.attrs

spec/features/projects/wiki/user_views_wiki_page_spec.rb

@@ -33,6 +33,8 @@
         fill_in(:wiki_content, with: 'wiki content')
         click_on('Create page')
       end
+
+      expect(page).to have_content('Wiki was successfully updated.')
     end
 
     it 'shows the history of a page that has a path' do
@@ -62,8 +64,10 @@
       expect(page).to have_content('Edit Page')
 
       fill_in('Content', with: 'Updated Wiki Content')
-
       click_on('Save changes')
+
+      expect(page).to have_content('Wiki was successfully updated.')
+
       click_on('Page history')
 
       page.within(:css, '.nav-text') do
@@ -132,6 +136,36 @@
     end
   end
 
+  context 'when a page has special characters in its title' do
+    let(:title) { '<foo> !@#$%^&*()[]{}=_+\'"\\|<>? <bar>' }
+
+    before do
+      wiki_page.update(title: title )
+    end
+
+    it 'preserves the special characters' do
+      visit(project_wiki_path(project, wiki_page))
+
+      expect(page).to have_css('.wiki-page-title', text: title)
+      expect(page).to have_css('.wiki-pages li', text: title)
+    end
+  end
+
+  context 'when a page has XSS in its title or content' do
+    let(:title) { '<script>alert("title")<script>' }
+
+    before do
+      wiki_page.update(title: title, content: 'foo <script>alert("content")</script> bar')
+    end
+
+    it 'safely displays the page' do
+      visit(project_wiki_path(project, wiki_page))
+
+      expect(page).to have_css('.wiki-page-title', text: title)
+      expect(page).to have_content('foo bar')
+    end
+  end
+
   context 'when a page has XSS in its message' do
     before do
       wiki_page.update(message: '<script>alert(true)<script>', content: 'XSS update')

spec/models/wiki_page_spec.rb

@@ -475,43 +475,59 @@
     end
   end
 
-  describe "#title" do
-    it "replaces a hyphen to a space" do
-      subject.title = "Import-existing-repositories-into-GitLab"
+  describe '#title_changed?' do
+    using RSpec::Parameterized::TableSyntax
 
-      expect(subject.title).to eq("Import existing repositories into GitLab")
+    let(:untitled_page) { described_class.new(wiki) }
+    let(:directory_page) do
+      create_page('parent/child', 'test content')
+      wiki.find_page('parent/child')
     end
 
-    it 'unescapes html' do
-      subject.title = 'foo & bar'
+    where(:page, :title, :changed) do
+      :untitled_page  | nil            | false
+      :untitled_page  | 'new title'    | true
 
-      expect(subject.title).to eq('foo & bar')
+      :new_page       | nil            | true
+      :new_page       | 'test page'    | true
+      :new_page       | 'new title'    | true
+
+      :existing_page  | nil            | false
+      :existing_page  | 'test page'    | false
+      :existing_page  | '/test page'   | false
+      :existing_page  | 'new title'    | true
+
+      :directory_page | nil            | false
+      :directory_page | 'parent/child' | false
+      :directory_page | 'child'        | false
+      :directory_page | '/child'       | true
+      :directory_page | 'parent/other' | true
+      :directory_page | 'other/child'  | true
+    end
+
+    with_them do
+      it 'returns the expected value' do
+        subject = public_send(page)
+        subject.title = title if title
+
+        expect(subject.title_changed?).to be(changed)
+      end
     end
   end
 
   describe '#path' do
-    let(:path) { 'mypath.md' }
-    let(:git_page) { instance_double('Gitlab::Git::WikiPage', path: path).as_null_object }
-
     it 'returns the path when persisted' do
-      page = described_class.new(wiki, git_page, true)
-
-      expect(page.path).to eq(path)
+      expect(existing_page.path).to eq('test-page.md')
     end
 
     it 'returns nil when not persisted' do
-      page = described_class.new(wiki, git_page, false)
-
-      expect(page.path).to be_nil
+      expect(new_page.path).to be_nil
     end
   end
 
   describe '#directory' do
     context 'when the page is at the root directory' do
-      subject do
-        create_page('file', 'content')
-        wiki.find_page('file')
-      end
+      subject { existing_page }
 
       it 'returns an empty string' do
         expect(subject.directory).to eq('')