diff --git a/app/assets/stylesheets/framework.scss b/app/assets/stylesheets/framework.scss
index 9b1d9d51f9c330b6b66c09100f09ccc519a366e2..82b4ec750ff28d0eb1669e69c29c8f945e49ce87 100644
--- a/app/assets/stylesheets/framework.scss
+++ b/app/assets/stylesheets/framework.scss
@@ -9,7 +9,6 @@
@import 'framework/animations';
@import 'framework/vue_transitions';
-@import 'framework/asciidoctor';
@import 'framework/banner';
@import 'framework/blocks';
@import 'framework/buttons';
diff --git a/app/assets/stylesheets/framework/asciidoctor.scss b/app/assets/stylesheets/framework/asciidoctor.scss
deleted file mode 100644
index 1586265d40ef00073b106b08bb652c2fd4925789..0000000000000000000000000000000000000000
--- a/app/assets/stylesheets/framework/asciidoctor.scss
+++ /dev/null
@@ -1,27 +0,0 @@
-.admonitionblock td.icon {
- width: 1%;
-
- [class^='fa icon-'] {
- @extend .fa-2x;
- }
-
- .icon-note {
- @extend .fa-thumb-tack;
- }
-
- .icon-tip {
- @extend .fa-lightbulb-o;
- }
-
- .icon-warning {
- @extend .fa-exclamation-triangle;
- }
-
- .icon-caution {
- @extend .fa-fire;
- }
-
- .icon-important {
- @extend .fa-exclamation-circle;
- }
-}
diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss
index 7baab478034aec6a5e4a481a9082c29eab4c263d..c201605e83d8a274237c63231c55cf0c9fcb622a 100644
--- a/app/assets/stylesheets/framework/typography.scss
+++ b/app/assets/stylesheets/framework/typography.scss
@@ -1,5 +1,5 @@
/**
- * Apply Markdown typography
+ * Apply Markup (Markdown/AsciiDoc) typography
*
*/
.md:not(.use-csslab) {
@@ -245,6 +245,21 @@
}
}
+ ul.checklist,
+ ul.none,
+ ol.none,
+ ul.no-bullet,
+ ol.no-bullet,
+ ol.unnumbered,
+ ul.unstyled,
+ ol.unstyled {
+ list-style-type: none;
+
+ li {
+ margin-left: 0;
+ }
+ }
+
li {
line-height: 1.6em;
margin-left: 25px;
@@ -321,6 +336,54 @@
visibility: visible;
}
}
+
+ .big {
+ font-size: larger;
+ }
+
+ .small {
+ font-size: smaller;
+ }
+
+ .underline {
+ text-decoration: underline;
+ }
+
+ .overline {
+ text-decoration: overline;
+ }
+
+ .line-through {
+ text-decoration: line-through;
+ }
+
+ .admonitionblock td.icon {
+ width: 1%;
+
+ [class^='fa icon-'] {
+ @extend .fa-2x;
+ }
+
+ .icon-note {
+ @extend .fa-thumb-tack;
+ }
+
+ .icon-tip {
+ @extend .fa-lightbulb-o;
+ }
+
+ .icon-warning {
+ @extend .fa-exclamation-triangle;
+ }
+
+ .icon-caution {
+ @extend .fa-fire;
+ }
+
+ .icon-important {
+ @extend .fa-exclamation-circle;
+ }
+ }
}
diff --git a/changelogs/unreleased/63298-prevent-excessive-sanitization-asciidoc.yml b/changelogs/unreleased/63298-prevent-excessive-sanitization-asciidoc.yml
new file mode 100644
index 0000000000000000000000000000000000000000..cd8206cdb99f1f593f1cb43393a73d201c9bfeb4
--- /dev/null
+++ b/changelogs/unreleased/63298-prevent-excessive-sanitization-asciidoc.yml
@@ -0,0 +1,5 @@
+---
+title: "Prevent excessive sanitization of AsciiDoc ouptut"
+merge_request: 30290
+author: Guillaume Grossetie
+type: added
\ No newline at end of file
diff --git a/lib/banzai/filter/ascii_doc_sanitization_filter.rb b/lib/banzai/filter/ascii_doc_sanitization_filter.rb
new file mode 100644
index 0000000000000000000000000000000000000000..a78bb60103c2e981c624b506154a566d8f0adad0
--- /dev/null
+++ b/lib/banzai/filter/ascii_doc_sanitization_filter.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+module Banzai
+ module Filter
+ # Sanitize HTML produced by AsciiDoc/Asciidoctor.
+ #
+ # Extends Banzai::Filter::BaseSanitizationFilter with specific rules.
+ class AsciiDocSanitizationFilter < Banzai::Filter::BaseSanitizationFilter
+ # Classes used by Asciidoctor to style components
+ ADMONITION_CLASSES = %w(fa icon-note icon-tip icon-warning icon-caution icon-important).freeze
+ CALLOUT_CLASSES = ['conum'].freeze
+ CHECKLIST_CLASSES = %w(fa fa-check-square-o fa-square-o).freeze
+
+ LIST_CLASSES = %w(checklist none no-bullet unnumbered unstyled).freeze
+
+ ELEMENT_CLASSES_WHITELIST = {
+ span: %w(big small underline overline line-through).freeze,
+ div: ['admonitionblock'].freeze,
+ td: ['icon'].freeze,
+ i: ADMONITION_CLASSES + CALLOUT_CLASSES + CHECKLIST_CLASSES,
+ ul: LIST_CLASSES,
+ ol: LIST_CLASSES
+ }.freeze
+
+ def customize_whitelist(whitelist)
+ # Allow marks
+ whitelist[:elements].push('mark')
+
+ # Allow any classes in `span`, `i`, `div`, `td`, `ul` and `ol` elements
+ # but then remove any unknown classes
+ whitelist[:attributes]['span'] = %w(class)
+ whitelist[:attributes]['div'].push('class')
+ whitelist[:attributes]['td'] = %w(class)
+ whitelist[:attributes]['i'] = %w(class)
+ whitelist[:attributes]['ul'] = %w(class)
+ whitelist[:attributes]['ol'] = %w(class)
+ whitelist[:transformers].push(self.class.remove_element_classes)
+
+ whitelist
+ end
+
+ class << self
+ def remove_element_classes
+ lambda do |env|
+ node = env[:node]
+
+ return unless (classes_whitelist = ELEMENT_CLASSES_WHITELIST[node.name.to_sym])
+ return unless node.has_attribute?('class')
+
+ classes = node['class'].strip.split(' ')
+ allowed_classes = (classes & classes_whitelist)
+ if allowed_classes.empty?
+ node.remove_attribute('class')
+ else
+ node['class'] = allowed_classes.join(' ')
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/lib/banzai/filter/base_sanitization_filter.rb b/lib/banzai/filter/base_sanitization_filter.rb
new file mode 100644
index 0000000000000000000000000000000000000000..420e92cb1e836f7d1a86b3a0403bd5c443fa0ccf
--- /dev/null
+++ b/lib/banzai/filter/base_sanitization_filter.rb
@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+module Banzai
+ module Filter
+ # Sanitize HTML produced by markup languages (Markdown, AsciiDoc...).
+ # Specific rules are implemented in dedicated filters:
+ #
+ # - Banzai::Filter::SanitizationFilter (Markdown)
+ # - Banzai::Filter::AsciiDocSanitizationFilter (AsciiDoc/Asciidoctor)
+ #
+ # Extends HTML::Pipeline::SanitizationFilter with common rules.
+ class BaseSanitizationFilter < HTML::Pipeline::SanitizationFilter
+ include Gitlab::Utils::StrongMemoize
+
+ UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
+
+ def whitelist
+ strong_memoize(:whitelist) do
+ whitelist = super.deep_dup
+
+ # Allow span elements
+ whitelist[:elements].push('span')
+
+ # Allow data-math-style attribute in order to support LaTeX formatting
+ whitelist[:attributes]['code'] = %w(data-math-style)
+ whitelist[:attributes]['pre'] = %w(data-math-style)
+
+ # Allow html5 details/summary elements
+ whitelist[:elements].push('details')
+ whitelist[:elements].push('summary')
+
+ # Allow abbr elements with title attribute
+ whitelist[:elements].push('abbr')
+ whitelist[:attributes]['abbr'] = %w(title)
+
+ # Disallow `name` attribute globally, allow on `a`
+ whitelist[:attributes][:all].delete('name')
+ whitelist[:attributes]['a'].push('name')
+
+ # Allow any protocol in `a` elements
+ # and then remove links with unsafe protocols
+ whitelist[:protocols].delete('a')
+ whitelist[:transformers].push(self.class.remove_unsafe_links)
+
+ # Remove `rel` attribute from `a` elements
+ whitelist[:transformers].push(self.class.remove_rel)
+
+ customize_whitelist(whitelist)
+ end
+ end
+
+ def customize_whitelist(whitelist)
+ raise NotImplementedError
+ end
+
+ class << self
+ def remove_unsafe_links
+ lambda do |env|
+ node = env[:node]
+
+ return unless node.name == 'a'
+ return unless node.has_attribute?('href')
+
+ begin
+ node['href'] = node['href'].strip
+ uri = Addressable::URI.parse(node['href'])
+
+ return unless uri.scheme
+
+ # Remove all invalid scheme characters before checking against the
+ # list of unsafe protocols.
+ #
+ # See https://tools.ietf.org/html/rfc3986#section-3.1
+ scheme = uri.scheme
+ .strip
+ .downcase
+ .gsub(/[^A-Za-z0-9\+\.\-]+/, '')
+
+ node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(scheme)
+ rescue Addressable::URI::InvalidURIError
+ node.remove_attribute('href')
+ end
+ end
+ end
+
+ def remove_rel
+ lambda do |env|
+ if env[:node_name] == 'a'
+ env[:node].remove_attribute('rel')
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb
index a4a06eae7b7f3e0c37167d043d4f6e9e7edaf8ca..f57e57890f8ed8ce1637b9f7656a8ea255e79e7b 100644
--- a/lib/banzai/filter/sanitization_filter.rb
+++ b/lib/banzai/filter/sanitization_filter.rb
@@ -2,23 +2,13 @@
module Banzai
module Filter
- # Sanitize HTML
+ # Sanitize HTML produced by Markdown.
#
- # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist.
- class SanitizationFilter < HTML::Pipeline::SanitizationFilter
- include Gitlab::Utils::StrongMemoize
-
- UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
+ # Extends Banzai::Filter::BaseSanitizationFilter with specific rules.
+ class SanitizationFilter < Banzai::Filter::BaseSanitizationFilter
+ # Styles used by Markdown for table alignment
TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/.freeze
- def whitelist
- strong_memoize(:whitelist) do
- customize_whitelist(super.deep_dup)
- end
- end
-
- private
-
def customize_whitelist(whitelist)
# Allow table alignment; we whitelist specific text-align values in a
# transformer below
@@ -26,36 +16,9 @@ def customize_whitelist(whitelist)
whitelist[:attributes]['td'] = %w(style)
whitelist[:css] = { properties: ['text-align'] }
- # Allow span elements
- whitelist[:elements].push('span')
-
- # Allow data-math-style attribute in order to support LaTeX formatting
- whitelist[:attributes]['code'] = %w(data-math-style)
- whitelist[:attributes]['pre'] = %w(data-math-style)
-
- # Allow html5 details/summary elements
- whitelist[:elements].push('details')
- whitelist[:elements].push('summary')
-
- # Allow abbr elements with title attribute
- whitelist[:elements].push('abbr')
- whitelist[:attributes]['abbr'] = %w(title)
-
# Allow the 'data-sourcepos' from CommonMark on all elements
whitelist[:attributes][:all].push('data-sourcepos')
- # Disallow `name` attribute globally, allow on `a`
- whitelist[:attributes][:all].delete('name')
- whitelist[:attributes]['a'].push('name')
-
- # Allow any protocol in `a` elements
- # and then remove links with unsafe protocols
- whitelist[:protocols].delete('a')
- whitelist[:transformers].push(self.class.remove_unsafe_links)
-
- # Remove `rel` attribute from `a` elements
- whitelist[:transformers].push(self.class.remove_rel)
-
# Remove any `style` properties not required for table alignment
whitelist[:transformers].push(self.class.remove_unsafe_table_style)
@@ -69,43 +32,6 @@ def customize_whitelist(whitelist)
end
class << self
- def remove_unsafe_links
- lambda do |env|
- node = env[:node]
-
- return unless node.name == 'a'
- return unless node.has_attribute?('href')
-
- begin
- node['href'] = node['href'].strip
- uri = Addressable::URI.parse(node['href'])
-
- return unless uri.scheme
-
- # Remove all invalid scheme characters before checking against the
- # list of unsafe protocols.
- #
- # See https://tools.ietf.org/html/rfc3986#section-3.1
- scheme = uri.scheme
- .strip
- .downcase
- .gsub(/[^A-Za-z0-9\+\.\-]+/, '')
-
- node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(scheme)
- rescue Addressable::URI::InvalidURIError
- node.remove_attribute('href')
- end
- end
- end
-
- def remove_rel
- lambda do |env|
- if env[:node_name] == 'a'
- env[:node].remove_attribute('rel')
- end
- end
- end
-
def remove_unsafe_table_style
lambda do |env|
node = env[:node]
diff --git a/lib/banzai/pipeline/ascii_doc_pipeline.rb b/lib/banzai/pipeline/ascii_doc_pipeline.rb
index 6be489c6572defe7ce83bb79443a3f51bda7aaa6..d25b74b23b229ae785bf618ef03989d2de4bdfa5 100644
--- a/lib/banzai/pipeline/ascii_doc_pipeline.rb
+++ b/lib/banzai/pipeline/ascii_doc_pipeline.rb
@@ -5,7 +5,7 @@ module Pipeline
class AsciiDocPipeline < BasePipeline
def self.filters
FilterArray[
- Filter::SanitizationFilter,
+ Filter::AsciiDocSanitizationFilter,
Filter::SyntaxHighlightFilter,
Filter::ExternalLinkFilter,
Filter::PlantumlFilter,
diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb
index 8f2434acd26414989736ccedd483e08843e2ddef..ff002acbd354c44ebd331843df13d81b2ab79775 100644
--- a/spec/lib/gitlab/asciidoc_spec.rb
+++ b/spec/lib/gitlab/asciidoc_spec.rb
@@ -45,28 +45,117 @@ module Gitlab
end
context "XSS" do
- links = {
- 'links' => {
+ items = {
+ 'link with extra attribute' => {
input: 'link:mylink"onmouseover="alert(1)[Click Here]',
output: "<div>\n<p><a href=\"mylink\">Click Here</a></p>\n</div>"
},
- 'images' => {
+ 'link with unsafe scheme' => {
+ input: 'link:data://danger[Click Here]',
+ output: "<div>\n<p><a>Click Here</a></p>\n</div>"
+ },
+ 'image with onerror' => {
input: 'image:https://localhost.com/image.png[Alt text" onerror="alert(7)]',
output: "<div>\n<p><span><img src=\"https://localhost.com/image.png\" alt='Alt text\" onerror=\"alert(7)'></span></p>\n</div>"
},
- 'pre' => {
+ 'fenced code with inline script' => {
input: '```mypre"><script>alert(3)</script>',
output: "<div>\n<div>\n<pre class=\"code highlight js-syntax-highlight plaintext\" lang=\"plaintext\" v-pre=\"true\"><code><span id=\"LC1\" class=\"line\" lang=\"plaintext\">\"></span></code></pre>\n</div>\n</div>"
}
}
- links.each do |name, data|
+ items.each do |name, data|
it "does not convert dangerous #{name} into HTML" do
expect(render(data[:input], context)).to include(data[:output])
end
end
end
+ context 'with admonition' do
+ it 'preserves classes' do
+ input = <<~ADOC
+ NOTE: An admonition paragraph, like this note, grabs the reader’s attention.
+ ADOC
+
+ output = <<~HTML
+ <div class="admonitionblock">
+ <table>
+ <tr>
+ <td class="icon">
+ <i class="fa icon-note" title="Note"></i>
+ </td>
+ <td>
+ An admonition paragraph, like this note, grabs the reader’s attention.
+ </td>
+ </tr>
+ </table>
+ </div>
+ HTML
+
+ expect(render(input, context)).to include(output.strip)
+ end
+ end
+
+ context 'with checklist' do
+ it 'preserves classes' do
+ input = <<~ADOC
+ * [x] checked
+ * [ ] not checked
+ ADOC
+
+ output = <<~HTML
+ <div>
+ <ul class="checklist">
+ <li>
+ <p><i class="fa fa-check-square-o"></i> checked</p>
+ </li>
+ <li>
+ <p><i class="fa fa-square-o"></i> not checked</p>
+ </li>
+ </ul>
+ </div>
+ HTML
+
+ expect(render(input, context)).to include(output.strip)
+ end
+ end
+
+ context 'with marks' do
+ it 'preserves classes' do
+ input = <<~ADOC
+ Werewolves are allergic to #cassia cinnamon#.
+
+ Did the werewolves read the [.small]#small print#?
+
+ Where did all the [.underline.small]#cores# run off to?
+
+ We need [.line-through]#ten# make that twenty VMs.
+
+ [.big]##O##nce upon an infinite loop.
+ ADOC
+
+ output = <<~HTML
+ <div>
+ <p>Werewolves are allergic to <mark>cassia cinnamon</mark>.</p>
+ </div>
+ <div>
+ <p>Did the werewolves read the <span class="small">small print</span>?</p>
+ </div>
+ <div>
+ <p>Where did all the <span class="underline small">cores</span> run off to?</p>
+ </div>
+ <div>
+ <p>We need <span class="line-through">ten</span> make that twenty VMs.</p>
+ </div>
+ <div>
+ <p><span class="big">O</span>nce upon an infinite loop.</p>
+ </div>
+ HTML
+
+ expect(render(input, context)).to include(output.strip)
+ end
+ end
+
context 'with fenced block' do
it 'highlights syntax' do
input = <<~ADOC