Commit e4ac6bbf authored by Douwe Maan's avatar Douwe Maan

Merge branch 'atom-xhtml-squashed' into 'master'

Fix invalid Atom feeds when using emoji, horizontal rules, or images

This is a fix for issues #880, #723, #1113.

Markdown must be rendered to XHTML, not HTML, when generating summary content for Atom feeds. Otherwise, content-less tags like *img* and *hr* are not terminated and make the Atom XML invalid. Such tags are generated when issue descriptions, merge request descriptions, comments, or commit messages use emoji, horizontal rules, or images.

To pass this option through from the relevant Haml templates to the proper place in the `gfm()` method, a new method `gfm_with_options()` is introduced. It reuses the options dictionary passed to `markdown()` and interprets options `xhtml` and `parse_tasks` from it (the latter was a convenient replacement for `gfm_with_tasks()`). `xhtml` is already interpreted by Redcarpet::Render::HTML, but that alone was not sufficient, because the post-processing in `gfm()` would convert its XHTML tags back to HTML.

I found no way of passing additional optional options to the existing `gfm()` method without requiring updates to existing callers and without getting in the way of the existing optional arguments, but maybe someone who knows more about Ruby than I can think of one.

Thorough review appreciated since this is the first time I have used Ruby.

See merge request !344
parents 409097bd 90aa870c
......@@ -101,6 +101,9 @@ v 7.8.1
- Fix urls for the issues when relative url was enabled
v 7.8.0
- Fix invalid Atom feeds when using emoji, horizontal rules, or images (Christian Walther)
v 7.8.0 (unreleased)
- Fix access control and protection against XSS for note attachments and other uploads.
- Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
- Make project search case insensitive (Hannes Rosenögger)
......
%div{xmlns: "http://www.w3.org/1999/xhtml"}
- if issue.description.present?
= markdown issue.description
= markdown(issue.description, xhtml: true)
%div{xmlns: "http://www.w3.org/1999/xhtml"}
- if merge_request.description.present?
= markdown merge_request.description
= markdown(merge_request.description, xhtml: true)
%div{xmlns: "http://www.w3.org/1999/xhtml"}
= markdown note.note
= markdown(note.note, xhtml: true)
......@@ -6,7 +6,7 @@
%i
at
= commit[:timestamp].to_time.to_s(:short)
%blockquote= markdown(escape_once(commit[:message]))
%blockquote= markdown(escape_once(commit[:message]), xhtml: true)
- if event.commits_count > 15
%p
%i
......
......@@ -34,17 +34,23 @@ module Gitlab
attr_reader :html_options
def gfm_with_tasks(text, project = @project, html_options = {})
text = gfm(text, project, html_options)
parse_tasks(text)
# Public: Parse the provided text with GitLab-Flavored Markdown
#
# text - the source text
# project - extra options for the reference links as given to link_to
# html_options - extra options for the reference links as given to link_to
def gfm(text, project = @project, html_options = {})
gfm_with_options(text, {}, project, html_options)
end
# Public: Parse the provided text with GitLab-Flavored Markdown
#
# text - the source text
# options - parse_tasks: true - render tasks
# - xhtml: true - output XHTML instead of HTML
# project - extra options for the reference links as given to link_to
# html_options - extra options for the reference links as given to link_to
def gfm(text, project = @project, html_options = {})
def gfm_with_options(text, options = {}, project = @project, html_options = {})
return text if text.nil?
# Duplicate the string so we don't alter the original, then call to_str
......@@ -87,14 +93,22 @@ module Gitlab
markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline
result = markdown_pipeline.call(text, markdown_context)
text = result[:output].to_html(save_with: 0)
saveoptions = 0
if options[:xhtml]
saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML
end
text = result[:output].to_html(save_with: saveoptions)
allowed_attributes = ActionView::Base.sanitized_allowed_attributes
allowed_tags = ActionView::Base.sanitized_allowed_tags
sanitize text.html_safe,
text = sanitize text.html_safe,
attributes: allowed_attributes + %w(id class style),
tags: allowed_tags + %w(table tr td th)
if options[:parse_tasks]
text = parse_tasks(text)
end
text
end
private
......
......@@ -67,10 +67,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML
unless @template.instance_variable_get("@project_wiki") || @project.nil?
full_document = h.create_relative_links(full_document)
end
if @options[:parse_tasks]
h.gfm_with_tasks(full_document)
else
h.gfm(full_document)
end
h.gfm_with_options(full_document, @options)
end
end
......@@ -15,17 +15,24 @@ describe "User Feed", feature: true do
let(:project) { create(:project) }
let(:issue) do
create(:issue, project: project,
author: user, description: '')
author: user, description: "Houston, we have a bug!\n\n***\n\nI guess.")
end
let(:note) do
create(:note, noteable: issue, author: user,
note: 'Bug confirmed', project: project)
note: 'Bug confirmed :+1:', project: project)
end
let(:merge_request) do
create(:merge_request,
title: 'Fix bug', author: user,
source_project: project, target_project: project,
description: "Here is the fix: ![an image](image.png)")
end
before do
project.team << [user, :master]
issue_event(issue, user)
note_event(note, user)
merge_request_event(merge_request, user)
visit user_path(user, :atom, private_token: user.private_token)
end
......@@ -37,6 +44,18 @@ describe "User Feed", feature: true do
expect(body).
to have_content("#{safe_name} commented on issue ##{issue.iid}")
end
it 'should have XHTML summaries in issue descriptions' do
expect(body).to match /we have a bug!<\/p>\n\n<hr ?\/>\n\n<p>I guess/
end
it 'should have XHTML summaries in notes' do
expect(body).to match /Bug confirmed <img[^>]*\/>/
end
it 'should have XHTML summaries in merge request descriptions' do
expect(body).to match /Here is the fix: <img[^>]*\/>/
end
end
end
......@@ -48,6 +67,10 @@ describe "User Feed", feature: true do
EventCreateService.new.leave_note(note, user)
end
def merge_request_event(request, user)
EventCreateService.new.open_mr(request, user)
end
def safe_name
html_escape(user.name)
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