diff options
author | Douwe Maan <douwe@gitlab.com> | 2015-06-02 18:19:01 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2015-06-02 18:19:01 +0000 |
commit | 34d176ad577ea421c99c87a54196deda92f796e8 (patch) | |
tree | 3ea0626b8d56d3c14309ccf07e6c3fcb4fd6f465 | |
parent | d85a7437a5651a93fc20d9bf7f183293151adb77 (diff) | |
parent | 9e7a9c63a59f4e673271b3600b735e3fa6702432 (diff) | |
download | gitlab-ce-34d176ad577ea421c99c87a54196deda92f796e8.tar.gz |
Merge branch 'rs-more-nofollow' into 'master'
Render Group and Project descriptions with our Markdown pipeline
Continuation of !727, this ensures external links in these fields also get `rel="nofollow"` added.
Bonus: Emoji now works in them! :sparkles:
See merge request !735
26 files changed, 216 insertions, 110 deletions
@@ -10,9 +10,6 @@ end gem "rails", "~> 4.1.0" -# Make links from text -gem 'rails_autolink', '~> 1.1' - # Default values for AR models gem "default_value_for", "~> 3.0.0" diff --git a/Gemfile.lock b/Gemfile.lock index bbc5639c84f..80ae41dc8fc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -448,8 +448,6 @@ GEM sprockets-rails (~> 2.0) rails-observers (0.1.2) activemodel (~> 4.0) - rails_autolink (1.1.6) - rails (> 3.1) railties (4.1.9) actionpack (= 4.1.9) activesupport (= 4.1.9) @@ -779,7 +777,6 @@ DEPENDENCIES rack-mini-profiler rack-oauth2 (~> 1.0.5) rails (~> 4.1.0) - rails_autolink (~> 1.1) raphael-rails (~> 2.1.2) rb-fsevent rb-inotify diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 12489ccc2d7..b93ea0f020e 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -48,14 +48,16 @@ } .project-home-desc { + color: $gray; + float: left; font-size: 16px; line-height: 1.3; margin-right: 250px; - } - .project-home-desc { - float: left; - color: $gray; + // Render Markdown-generated HTML inline for this block + p { + display: inline; + } } } diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 89dcdf57798..a539ec49f7a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -279,10 +279,6 @@ module ApplicationHelper html_options end - def escaped_autolink(text) - auto_link ERB::Util.html_escape(text), link: :urls - end - def promo_host 'about.gitlab.com' end diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 1678311141e..0687840af39 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -11,7 +11,7 @@ @#{@group.path} - if @group.description.present? .description - = escaped_autolink(@group.description) + = markdown(@group.description, pipeline: :description) %hr = render 'shared/show_aside' diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index f9cdda4a3ba..076afb11a9d 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -5,7 +5,7 @@ .project-home-row.project-home-row-top .project-home-desc - if @project.description.present? - = escaped_autolink(@project.description) + = markdown(@project.description, pipeline: :description) - if can?(current_user, :admin_project, @project) – = link_to 'Edit', edit_namespace_project_path diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 5db1566f55d..fa9c0975bb8 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -57,6 +57,9 @@ module Gitlab pipeline = HTML::Pipeline.new(filters) context = { + # SanitizationFilter + pipeline: options[:pipeline], + # EmojiFilter asset_root: Gitlab.config.gitlab.url, asset_host: Gitlab::Application.config.asset_host, diff --git a/lib/gitlab/markdown/sanitization_filter.rb b/lib/gitlab/markdown/sanitization_filter.rb index 88781fea0c8..74b3a8d274f 100644 --- a/lib/gitlab/markdown/sanitization_filter.rb +++ b/lib/gitlab/markdown/sanitization_filter.rb @@ -8,33 +8,54 @@ module Gitlab # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist. class SanitizationFilter < HTML::Pipeline::SanitizationFilter def whitelist - whitelist = super + # Descriptions are more heavily sanitized, allowing only a few elements. + # See http://git.io/vkuAN + if pipeline == :description + whitelist = LIMITED + whitelist[:elements] -= %w(pre code img ol ul li) + else + whitelist = super + end + + customize_whitelist(whitelist) + + whitelist + end + private + + def pipeline + context[:pipeline] || :default + end + + def customized?(transformers) + transformers.last.source_location[0] == __FILE__ + end + + def customize_whitelist(whitelist) # Only push these customizations once - unless customized?(whitelist[:transformers]) - # Allow code highlighting - whitelist[:attributes]['pre'] = %w(class) - whitelist[:attributes]['span'] = %w(class) + return if customized?(whitelist[:transformers]) - # Allow table alignment - whitelist[:attributes]['th'] = %w(style) - whitelist[:attributes]['td'] = %w(style) + # Allow code highlighting + whitelist[:attributes]['pre'] = %w(class) + whitelist[:attributes]['span'] = %w(class) - # Allow span elements - whitelist[:elements].push('span') + # Allow table alignment + whitelist[:attributes]['th'] = %w(style) + whitelist[:attributes]['td'] = %w(style) - # Remove `rel` attribute from `a` elements - whitelist[:transformers].push(remove_rel) + # Allow span elements + whitelist[:elements].push('span') - # Remove `class` attribute from non-highlight spans - whitelist[:transformers].push(clean_spans) - end + # Remove `rel` attribute from `a` elements + whitelist[:transformers].push(remove_rel) + + # Remove `class` attribute from non-highlight spans + whitelist[:transformers].push(clean_spans) whitelist end - private - def remove_rel lambda do |env| if env[:node_name] == 'a' @@ -53,10 +74,6 @@ module Gitlab end end end - - def customized?(transformers) - transformers.last.source_location[0] == __FILE__ - end end end end diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb new file mode 100644 index 00000000000..edc1c63a0aa --- /dev/null +++ b/spec/features/groups_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +feature 'Group' do + describe 'description' do + let(:group) { create(:group) } + let(:path) { group_path(group) } + + before do + login_as(:admin) + end + + it 'parses Markdown' do + group.update_attribute(:description, 'This is **my** group') + visit path + expect(page).to have_css('.description > p > strong') + end + + it 'passes through html-pipeline' do + group.update_attribute(:description, 'This group is the :poop:') + visit path + expect(page).to have_css('.description > p > img') + end + + it 'sanitizes unwanted tags' do + group.update_attribute(:description, '# Group Description') + visit path + expect(page).not_to have_css('.description h1') + end + + it 'permits `rel` attribute on links' do + group.update_attribute(:description, 'https://google.com/') + visit path + expect(page).to have_css('.description a[rel]') + end + end +end diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index ee1b3bf749d..902968cebcb 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -18,11 +18,13 @@ require 'erb' # -> `gfm_with_options` helper # -> HTML::Pipeline # -> Sanitize +# -> RelativeLink # -> Emoji # -> Table of Contents # -> Autolinks # -> Rinku (http, https, ftp) # -> Other schemes +# -> ExternalLink # -> References # -> TaskList # -> `html_safe` diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index cae11be7cdd..56523f6e1a8 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -1,24 +1,56 @@ require 'spec_helper' -describe "Projects", feature: true, js: true do - before { login_as :user } +feature 'Project' do + describe 'description' do + let(:project) { create(:project) } + let(:path) { namespace_project_path(project.namespace, project) } - describe "DELETE /projects/:id" do before do - @project = create(:project, namespace: @user.namespace) - @project.team << [@user, :master] - visit edit_namespace_project_path(@project.namespace, @project) + login_as(:admin) end - it "should remove project" do + it 'parses Markdown' do + project.update_attribute(:description, 'This is **my** project') + visit path + expect(page).to have_css('.project-home-desc > p > strong') + end + + it 'passes through html-pipeline' do + project.update_attribute(:description, 'This project is the :poop:') + visit path + expect(page).to have_css('.project-home-desc > p > img') + end + + it 'sanitizes unwanted tags' do + project.update_attribute(:description, '# Project Description') + visit path + expect(page).not_to have_css('.project-home-desc h1') + end + + it 'permits `rel` attribute on links' do + project.update_attribute(:description, 'https://google.com/') + visit path + expect(page).to have_css('.project-home-desc a[rel]') + end + end + + describe 'removal', js: true do + let(:user) { create(:user) } + let(:project) { create(:project, namespace: user.namespace) } + + before do + login_with(user) + project.team << [user, :master] + visit edit_namespace_project_path(project.namespace, project) + end + + it 'should remove project' do expect { remove_project }.to change {Project.count}.by(-1) end it 'should delete the project from disk' do - expect(GitlabShellWorker).to( - receive(:perform_async).with(:remove_repository, - /#{@project.path_with_namespace}/) - ).twice + expect(GitlabShellWorker).to receive(:perform_async). + with(:remove_repository, /#{project.path_with_namespace}/).twice remove_project end @@ -26,7 +58,7 @@ describe "Projects", feature: true, js: true do def remove_project click_link "Remove project" - fill_in 'confirm_name_input', with: @project.path + fill_in 'confirm_name_input', with: project.path click_button 'Confirm' end end diff --git a/spec/lib/gitlab/markdown/autolink_filter_spec.rb b/spec/lib/gitlab/markdown/autolink_filter_spec.rb index 0bbdc11a979..a14cb2da089 100644 --- a/spec/lib/gitlab/markdown/autolink_filter_spec.rb +++ b/spec/lib/gitlab/markdown/autolink_filter_spec.rb @@ -2,11 +2,9 @@ require 'spec_helper' module Gitlab::Markdown describe AutolinkFilter do - let(:link) { 'http://about.gitlab.com/' } + include FilterSpecHelper - def filter(html, options = {}) - described_class.call(html, options) - end + let(:link) { 'http://about.gitlab.com/' } it 'does nothing when :autolink is false' do exp = act = link diff --git a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb index d3695ee46d0..e8391cc7aca 100644 --- a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe CommitRangeReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:project) } let(:commit1) { project.commit } diff --git a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb index a0d2cd7e22b..a10d43c9a02 100644 --- a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe CommitReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:project) } let(:commit) { project.commit } diff --git a/spec/lib/gitlab/markdown/emoji_filter_spec.rb b/spec/lib/gitlab/markdown/emoji_filter_spec.rb index 18d55c4818f..11efd9bb4cd 100644 --- a/spec/lib/gitlab/markdown/emoji_filter_spec.rb +++ b/spec/lib/gitlab/markdown/emoji_filter_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe EmojiFilter do - def filter(html, contexts = {}) - described_class.call(html, contexts) - end + include FilterSpecHelper before do ActionController::Base.asset_host = 'https://foo.com' diff --git a/spec/lib/gitlab/markdown/external_issue_reference_filter_spec.rb b/spec/lib/gitlab/markdown/external_issue_reference_filter_spec.rb index bf9409589fa..f16095bc2b2 100644 --- a/spec/lib/gitlab/markdown/external_issue_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/external_issue_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe ExternalIssueReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper def helper IssuesHelper diff --git a/spec/lib/gitlab/markdown/external_link_filter_spec.rb b/spec/lib/gitlab/markdown/external_link_filter_spec.rb index c2ff4f80a42..a040b34577b 100644 --- a/spec/lib/gitlab/markdown/external_link_filter_spec.rb +++ b/spec/lib/gitlab/markdown/external_link_filter_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe ExternalLinkFilter do - def filter(html, options = {}) - described_class.call(html, options) - end + include FilterSpecHelper it 'ignores elements without an href attribute' do exp = act = %q(<a id="ignored">Ignore Me</a>) diff --git a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb index a838d7570c8..fa43d33794d 100644 --- a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe IssueReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper def helper IssuesHelper diff --git a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb index 41987f57bca..cf3337b1ba1 100644 --- a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb @@ -3,7 +3,7 @@ require 'html/pipeline' module Gitlab::Markdown describe LabelReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:empty_project) } let(:label) { create(:label, project: project) } diff --git a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb index 6aeb1093602..5945302a2da 100644 --- a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe MergeRequestReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:project) } let(:merge) { create(:merge_request, source_project: project) } diff --git a/spec/lib/gitlab/markdown/sanitization_filter_spec.rb b/spec/lib/gitlab/markdown/sanitization_filter_spec.rb index 4a1aa766149..e50c82d0b3c 100644 --- a/spec/lib/gitlab/markdown/sanitization_filter_spec.rb +++ b/spec/lib/gitlab/markdown/sanitization_filter_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe SanitizationFilter do - def filter(html, options = {}) - described_class.call(html, options) - end + include FilterSpecHelper describe 'default whitelist' do it 'sanitizes tags that are not whitelisted' do @@ -42,6 +40,13 @@ module Gitlab::Markdown end describe 'custom whitelist' do + it 'customizes the whitelist only once' do + instance = described_class.new('Foo') + 3.times { instance.whitelist } + + expect(instance.whitelist[:transformers].size).to eq 4 + end + it 'allows syntax highlighting' do exp = act = %q{<pre class="code highlight white c"><code><span class="k">def</span></code></pre>} expect(filter(act).to_html).to eq exp @@ -87,5 +92,27 @@ module Gitlab::Markdown expect(doc.at_css('a')['href']).to be_nil end end + + context 'when pipeline is :description' do + it 'uses a stricter whitelist' do + doc = filter('<h1>Description</h1>', pipeline: :description) + expect(doc.to_html.strip).to eq 'Description' + end + + %w(pre code img ol ul li).each do |elem| + it "removes '#{elem}' elements" do + act = "<#{elem}>Description</#{elem}>" + expect(filter(act, pipeline: :description).to_html.strip). + to eq 'Description' + end + end + + %w(b i strong em a ins del sup sub p).each do |elem| + it "still allows '#{elem}' elements" do + exp = act = "<#{elem}>Description</#{elem}>" + expect(filter(act, pipeline: :description).to_html).to eq exp + end + end + end end end diff --git a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb index 07ece66e903..38619a3c07f 100644 --- a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe SnippetReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:empty_project) } let(:snippet) { create(:project_snippet, project: project) } diff --git a/spec/lib/gitlab/markdown/table_of_contents_filter_spec.rb b/spec/lib/gitlab/markdown/table_of_contents_filter_spec.rb index f383a5850d5..ddf583a72c1 100644 --- a/spec/lib/gitlab/markdown/table_of_contents_filter_spec.rb +++ b/spec/lib/gitlab/markdown/table_of_contents_filter_spec.rb @@ -4,9 +4,7 @@ require 'spec_helper' module Gitlab::Markdown describe TableOfContentsFilter do - def filter(html, options = {}) - described_class.call(html, options) - end + include FilterSpecHelper def header(level, text) "<h#{level}>#{text}</h#{level}>\n" diff --git a/spec/lib/gitlab/markdown/task_list_filter_spec.rb b/spec/lib/gitlab/markdown/task_list_filter_spec.rb index 2a1e1cc5127..94f39cc966e 100644 --- a/spec/lib/gitlab/markdown/task_list_filter_spec.rb +++ b/spec/lib/gitlab/markdown/task_list_filter_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe TaskListFilter do - def filter(html, options = {}) - described_class.call(html, options) - end + include FilterSpecHelper it 'does not apply `task-list` class to non-task lists' do exp = act = %(<ul><li>Item</li></ul>) diff --git a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb index 0ecbdee9b9e..08e6941028f 100644 --- a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' module Gitlab::Markdown describe UserReferenceFilter do - include ReferenceFilterSpecHelper + include FilterSpecHelper let(:project) { create(:empty_project) } let(:user) { create(:user) } diff --git a/spec/support/reference_filter_spec_helper.rb b/spec/support/filter_spec_helper.rb index afbea55ab99..755964e9a3d 100644 --- a/spec/support/reference_filter_spec_helper.rb +++ b/spec/support/filter_spec_helper.rb @@ -1,46 +1,23 @@ -# Common methods and setup for Gitlab::Markdown reference filter specs +# Helper methods for Gitlab::Markdown filter specs # # Must be included into specs manually -module ReferenceFilterSpecHelper +module FilterSpecHelper extend ActiveSupport::Concern - # Shortcut to Rails' auto-generated routes helpers, to avoid including the - # module - def urls - Rails.application.routes.url_helpers - end - - # Modify a String reference to make it invalid - # - # Commit SHAs get reversed, IDs get incremented by 1, all other Strings get - # their word characters reversed. - # - # reference - String reference to modify - # - # Returns a String - def invalidate_reference(reference) - if reference =~ /\A(.+)?.\d+\z/ - # Integer-based reference with optional project prefix - reference.gsub(/\d+\z/) { |i| i.to_i + 1 } - elsif reference =~ /\A(.+@)?(\h{6,40}\z)/ - # SHA-based reference with optional prefix - reference.gsub(/\h{6,40}\z/) { |v| v.reverse } - else - reference.gsub(/\w+\z/) { |v| v.reverse } - end - end - # Perform `call` on the described class # - # Automatically passes the current `project` value to the context if none is - # provided. + # Automatically passes the current `project` value, if defined, to the context + # if none is provided. # - # html - String text to pass to the filter's `call` method. + # html - HTML String to pass to the filter's `call` method. # contexts - Hash context for the filter. (default: {project: project}) # - # Returns the String text returned by the filter's `call` method. + # Returns a Nokogiri::XML::DocumentFragment def filter(html, contexts = {}) - contexts.reverse_merge!(project: project) + if defined?(project) + contexts.reverse_merge!(project: project) + end + described_class.call(html, contexts) end @@ -50,7 +27,7 @@ module ReferenceFilterSpecHelper # body - String text to run through the pipeline # contexts - Hash context for the filter. (default: {project: project}) # - # Returns the Hash of the pipeline result + # Returns the Hash def pipeline_result(body, contexts = {}) contexts.reverse_merge!(project: project) @@ -58,13 +35,43 @@ module ReferenceFilterSpecHelper pipeline.call(body) end + # Modify a String reference to make it invalid + # + # Commit SHAs get reversed, IDs get incremented by 1, all other Strings get + # their word characters reversed. + # + # reference - String reference to modify + # + # Returns a String + def invalidate_reference(reference) + if reference =~ /\A(.+)?.\d+\z/ + # Integer-based reference with optional project prefix + reference.gsub(/\d+\z/) { |i| i.to_i + 1 } + elsif reference =~ /\A(.+@)?(\h{6,40}\z)/ + # SHA-based reference with optional prefix + reference.gsub(/\h{6,40}\z/) { |v| v.reverse } + else + reference.gsub(/\w+\z/) { |v| v.reverse } + end + end + + # Stub CrossProjectReference#user_can_reference_project? to return true for + # the current test def allow_cross_reference! allow_any_instance_of(described_class). to receive(:user_can_reference_project?).and_return(true) end + # Stub CrossProjectReference#user_can_reference_project? to return false for + # the current test def disallow_cross_reference! allow_any_instance_of(described_class). to receive(:user_can_reference_project?).and_return(false) end + + # Shortcut to Rails' auto-generated routes helpers, to avoid including the + # module + def urls + Rails.application.routes.url_helpers + end end |