Merge branch '16131-wiki-titles-with-special-characters' into 'master'

Tweak wiki page title handling

See merge request gitlab-org/gitlab!25647

GITALY_SERVER_VERSION

-1.87.0
+12.9.0-rc1

app/models/project_wiki.rb

@@ -107,7 +107,7 @@ class ProjectWiki
       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 @@ class ProjectWiki
     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 @@ class WikiPage
     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 @@ class WikiPage
 
   # 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 @@ class WikiPage
   # 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 @@ class WikiPage
   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 @@ class WikiPage
 
     update_attributes(attrs)
 
-    if title_changed?
-      page_details = title
-
-      if wiki.find_page(page_details).present?
-        @attributes[:title] = @page.url_path
+    if title.present? && title_changed? && wiki.find_page(title).present?
+      @attributes[:title] = @page.title
       raise PageRenameError
     end
-    else
-      page_details = @page.url_path
-    end
 
-    save(page_details: page_details) do
+    save do
       wiki.update_page(
         @page,
         content: content,
@@ -266,7 +255,14 @@ class WikiPage
   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 @@ class WikiPage
     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 @@ FactoryBot.define do
 
     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 @@ describe 'User views a wiki page' do
         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 @@ describe 'User views a wiki page' do
       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 @@ describe 'User views a wiki page' do
     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 @@ describe WikiPage do
     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('')