diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 24263a0f619e45bbba0fa44bff7b88ffe1fbb137..c309f890d96fc5efe47e224c773e7c6508b74910 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -72,146 +72,6 @@ module GitlabMarkdownHelper end end - # TODO (rspeicher): This should be its own filter - def create_relative_links(text) - paths = extract_paths(text) - - paths.uniq.each do |file_path| - # If project does not have repository - # its nothing to rebuild - # - # TODO: pass project variable to markdown helper instead of using - # instance variable. Right now it generates invalid path for pages out - # of project scope. Example: search results where can be rendered markdown - # from different projects - if @repository && @repository.exists? && !@repository.empty? - new_path = rebuild_path(file_path) - # Finds quoted path so we don't replace other mentions of the string - # eg. "doc/api" will be replaced and "/home/doc/api/text" won't - text.gsub!("\"#{file_path}\"", "\"/#{new_path}\"") - end - end - - text - end - - def extract_paths(text) - links = substitute_links(text) - image_links = substitute_image_links(text) - links + image_links - end - - def substitute_links(text) - links = text.scan(//) - relative_links = links.flatten.reject{ |link| link_to_ignore? link } - relative_links - end - - def substitute_image_links(text) - links = text.scan(/ - true - else - ignored_protocols.map{ |protocol| link.include?(protocol) }.any? - end - end - - def ignored_protocols - ["http://","https://", "ftp://", "mailto:", "smb://"] - end - - def rebuild_path(file_path) - file_path = file_path.dup - file_path.gsub!(/(#.*)/, "") - id = $1 || "" - file_path = relative_file_path(file_path) - file_path = sanitize_slashes(file_path) - - [ - Gitlab.config.gitlab.relative_url_root, - @project.path_with_namespace, - path_with_ref(file_path), - file_path - ].compact.join("/").gsub(/\A\/*|\/*\z/, '') + id - end - - def sanitize_slashes(path) - path[0] = "" if path.start_with?("/") - path.chop if path.end_with?("/") - path - end - - def relative_file_path(path) - requested_path = @path - nested_path = build_nested_path(path, requested_path) - return nested_path if file_exists?(nested_path) - path - end - - # Covering a special case, when the link is referencing file in the same directory eg: - # If we are at doc/api/README.md and the README.md contains relative links like [Users](users.md) - # this takes the request path(doc/api/README.md), and replaces the README.md with users.md so the path looks like doc/api/users.md - # If we are at doc/api and the README.md shown in below the tree view - # this takes the request path(doc/api) and adds users.md so the path looks like doc/api/users.md - def build_nested_path(path, request_path) - return request_path if path == "" - return path unless request_path - if local_path(request_path) == "tree" - base = request_path.split("/").push(path) - base.join("/") - else - base = request_path.split("/") - base.pop - base.push(path).join("/") - end - end - - # Checks if the path exists in the repo - # eg. checks if doc/README.md exists, if not then link to blob - def path_with_ref(path) - if file_exists?(path) - "#{local_path(path)}/#{correct_ref}" - else - "blob/#{correct_ref}" - end - end - - def file_exists?(path) - return false if path.nil? - @repository.blob_at(current_sha, path).present? || @repository.tree(current_sha, path).entries.any? - end - - # Check if the path is pointing to a directory(tree) or a file(blob) - # eg. doc/api is directory and doc/README.md is file - def local_path(path) - return "tree" if @repository.tree(current_sha, path).entries.any? - return "raw" if @repository.blob_at(current_sha, path).image? - "blob" - end - - def current_sha - if @commit - @commit.id - elsif @repository && !@repository.empty? - if @ref - @repository.commit(@ref).try(:sha) - else - @repository.head_commit.sha - end - end - end - - # We will assume that if no ref exists we can point to master - def correct_ref - @ref ? @ref : "master" - end - private # Return +text+, truncated to +max_chars+ characters, excluding any HTML diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 63294aa54c069fc3e2406702a9e3bf11674c625e..cc68416d5fcea13cf4aa7793a020ef4b25ea01f1 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -15,6 +15,7 @@ module Gitlab autoload :IssueReferenceFilter, 'gitlab/markdown/issue_reference_filter' autoload :LabelReferenceFilter, 'gitlab/markdown/label_reference_filter' autoload :MergeRequestReferenceFilter, 'gitlab/markdown/merge_request_reference_filter' + autoload :RelativeLinkFilter, 'gitlab/markdown/relative_link_filter' autoload :SanitizationFilter, 'gitlab/markdown/sanitization_filter' autoload :SnippetReferenceFilter, 'gitlab/markdown/snippet_reference_filter' autoload :TableOfContentsFilter, 'gitlab/markdown/table_of_contents_filter' @@ -64,7 +65,12 @@ module Gitlab current_user: current_user, only_path: options[:reference_only_path], project: project, - reference_class: html_options[:class] + reference_class: html_options[:class], + + # RelativeLinkFilter + ref: @ref, + requested_path: @path, + project_wiki: @project_wiki } result = pipeline.call(text, context) @@ -91,6 +97,7 @@ module Gitlab [ Gitlab::Markdown::SanitizationFilter, + Gitlab::Markdown::RelativeLinkFilter, Gitlab::Markdown::EmojiFilter, Gitlab::Markdown::TableOfContentsFilter, Gitlab::Markdown::AutolinkFilter, diff --git a/lib/gitlab/markdown/relative_link_filter.rb b/lib/gitlab/markdown/relative_link_filter.rb new file mode 100644 index 0000000000000000000000000000000000000000..deb302c88e13425dac1edfe1832a01c1fdac790f --- /dev/null +++ b/lib/gitlab/markdown/relative_link_filter.rb @@ -0,0 +1,114 @@ +require 'html/pipeline/filter' +require 'uri' + +module Gitlab + module Markdown + # HTML filter that "fixes" relative links to files in a repository. + # + # Context options: + # :commit + # :project + # :project_wiki + # :requested_path + # :ref + class RelativeLinkFilter < HTML::Pipeline::Filter + + def call + if linkable_files? + doc.search('a').each do |el| + process_link_attr el.attribute('href') + end + + doc.search('img').each do |el| + process_link_attr el.attribute('src') + end + end + + doc + end + + protected + + def linkable_files? + context[:project_wiki].nil? && repository.try(:exists?) && !repository.empty? + end + + def process_link_attr(html_attr) + return if html_attr.blank? + + uri = URI(html_attr.value) + if uri.relative? && uri.path.present? + html_attr.value = rebuild_relative_uri(uri).to_s + end + end + + def rebuild_relative_uri(uri) + file_path = relative_file_path(uri.path) + + uri.path = [ + relative_url_root, + context[:project].path_with_namespace, + path_type(file_path), + ref || 'master', # assume that if no ref exists we can point to master + file_path + ].compact.join('/').squeeze('/').chomp('/') + + uri + end + + def relative_file_path(path) + nested_path = build_nested_path(path, context[:requested_path]) + file_exists?(nested_path) ? nested_path : path + end + + # Covering a special case, when the link is referencing file in the same + # directory. + # If we are at doc/api/README.md and the README.md contains relative + # links like [Users](users.md), this takes the request + # path(doc/api/README.md) and replaces the README.md with users.md so the + # path looks like doc/api/users.md. + # If we are at doc/api and the README.md shown in below the tree view + # this takes the request path(doc/api) and adds users.md so the path + # looks like doc/api/users.md + def build_nested_path(path, request_path) + return request_path if path.empty? + return path unless request_path + + parts = request_path.split('/') + parts.pop if path_type(request_path) != 'tree' + parts.push(path).join('/') + end + + def file_exists?(path) + return false if path.nil? + repository.blob_at(current_sha, path).present? || + repository.tree(current_sha, path).entries.any? + end + + # Check if the path is pointing to a directory(tree) or a file(blob) + # eg. doc/api is directory and doc/README.md is file. + def path_type(path) + return 'tree' if repository.tree(current_sha, path).entries.any? + return 'raw' if repository.blob_at(current_sha, path).try(:image?) + 'blob' + end + + def current_sha + context[:commit].try(:id) || + ref ? repository.commit(ref).try(:sha) : repository.head_commit.sha + end + + def relative_url_root + Gitlab.config.gitlab.relative_url_root.presence || '/' + end + + def ref + context[:ref] + end + + def repository + context[:project].try(:repository) + end + end + end +end diff --git a/lib/redcarpet/render/gitlab_html.rb b/lib/redcarpet/render/gitlab_html.rb index bea66e6cdc12ee10c1eeefdc702a708e7368a5aa..15eb6effe083da3b50fd43b0c1b485f0d52970b6 100644 --- a/lib/redcarpet/render/gitlab_html.rb +++ b/lib/redcarpet/render/gitlab_html.rb @@ -36,10 +36,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML end def postprocess(full_document) - unless @template.instance_variable_get("@project_wiki") || @project.nil? - full_document = h.create_relative_links(full_document) - end - h.gfm_with_options(full_document, @options) end end diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 2f67879efdcb3094700e0f7a2e33fb8adea51195..7d0335c2320365f9408ab3bc5c708672f83f2b8b 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -96,79 +96,6 @@ describe GitlabMarkdownHelper do end end - describe "#markdown" do - # TODO (rspeicher): These belong in a relative link filter spec - context 'relative links' do - context 'with a valid repository' do - before do - @repository = project.repository - @ref = 'markdown' - end - - it "should handle relative urls for a file in master" do - actual = "[GitLab API doc](doc/api/README.md)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls for a file in master with an anchor" do - actual = "[GitLab API doc](doc/api/README.md#section)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - - it "should not handle relative urls for the current file with an anchor" do - actual = "[GitLab API doc](#section)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls for a directory in master" do - actual = "[GitLab API doc](doc/api)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle absolute urls" do - actual = "[GitLab](https://www.gitlab.com)\n" - expected = "

GitLab

\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls in reference links for a file in master" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls in reference links for a directory in master" do - actual = "[GitLab API doc directory][GitLab readmes]\n [GitLab readmes]: doc/api/\n" - expected = "

GitLab API doc directory

\n" - expect(markdown(actual)).to match(expected) - end - - it "should not handle malformed relative urls in reference links for a file in master" do - actual = "[GitLab readme]: doc/api/README.md\n" - expected = "" - expect(markdown(actual)).to match(expected) - end - end - - context 'with an empty repository' do - before do - @project = create(:empty_project) - @repository = @project.repository - end - - it "should not touch relative urls" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - end - end - end - describe '#render_wiki_content' do before do @wiki = double('WikiPage') diff --git a/spec/lib/gitlab/markdown/relative_link_filter_spec.rb b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..38cf567d73f1a09e55caaac4655850f329bee191 --- /dev/null +++ b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb @@ -0,0 +1,120 @@ +require 'spec_helper' + +module Gitlab::Markdown + describe RelativeLinkFilter do + include ActionView::Helpers::TagHelper + + let!(:project) { create(:project) } + + let(:commit) { project.commit } + let(:project_path) { project.path_with_namespace } + let(:repository) { project.repository } + let(:ref) { 'markdown' } + + let(:project_wiki) { nil } + let(:requested_path) { '/' } + let(:blob) { RepoHelpers.sample_blob } + + let(:context) do + { + commit: commit, + project: project, + project_wiki: project_wiki, + requested_path: requested_path, + ref: ref + } + end + + + shared_examples :preserve_unchanged do + + it "should not modify any relative url in anchor" do + doc = tag(:a, href: 'README.md') + expect( filter(doc) ).to match '"README.md"' + end + + it "should not modify any relative url in image" do + doc = tag(:img, src: 'files/images/logo-black.png') + expect( filter(doc) ).to match '"files/images/logo-black.png"' + end + end + + shared_examples :relative_to_requested do + + it "should rebuild url relative to the requested path" do + expect( filter(tag(:a, href: 'users.md')) ).to \ + match %("/#{project_path}/blob/#{ref}/doc/api/users.md") + end + end + + + context "with a project_wiki" do + let(:project_wiki) { double('ProjectWiki') } + + include_examples :preserve_unchanged + end + + context "without a repository" do + let!(:project) { create(:empty_project) } + + include_examples :preserve_unchanged + end + + context "with an empty repository" do + let!(:project) { create(:project_empty_repo) } + + include_examples :preserve_unchanged + end + + + context "with a valid repository" do + + it "should rebuild relative url for a file in the repo" do + expect( filter(tag(:a, href: 'doc/api/README.md')) ).to \ + match %("/#{project_path}/blob/#{ref}/doc/api/README.md") + end + + it "should rebuild relative url for a file in the repo with an anchor" do + expect( filter(tag(:a, href: 'README.md#section')) ).to \ + match %("/#{project_path}/blob/#{ref}/README.md#section") + end + + it "should rebuild relative url for a directory in the repo" do + expect( filter(tag(:a, href: 'doc/api/')) ).to \ + match %("/#{project_path}/tree/#{ref}/doc/api") + end + + it "should rebuild relative url for an image in the repo" do + expect( filter(tag(:img, src: 'files/images/logo-black.png')) ).to \ + match %("/#{project_path}/raw/#{ref}/files/images/logo-black.png") + end + + it "should not modify relative url with an anchor only" do + doc = tag(:a, href: '#section-1') + expect( filter(doc) ).to match %("#section-1") + end + + it "should not modify absolute url" do + expect( filter(tag(:a, href: 'http://example.org')) ).to \ + match %("http://example.org") + end + + context "when requested path is a file in the repo" do + let(:requested_path) { 'doc/api/README.md' } + + include_examples :relative_to_requested + end + + context "when requested path is a directory in the repo" do + let(:requested_path) { 'doc/api' } + + include_examples :relative_to_requested + end + end + + + def filter(doc) + described_class.call(doc, context).to_s + end + end +end