From dc30eadad88e11c278dfcb780f8bf78727e7533f Mon Sep 17 00:00:00 2001 From: Andy Zhao <17884966+Zhao-Andy@users.noreply.github.com> Date: Tue, 27 Apr 2021 09:33:01 -0400 Subject: [PATCH] Remove unused attributes from markdown parser (#13484) * Remove definitely unused attributes * Refactor and remove unused attributes * Add parentheses * Prevent alt tag from having Liquid tags inside * Add test * Remove more unused attributes * Scrub any valid attributes * Remove unused tags and add more tests * Remove tag; is used by parser * Remove
b/c it's deprecated though still valid * is a safe tag and used by some * Eh I changed my mind about center lol * Add underline back in * Don't allow width changes * Use frozen constant for regex * Use #remove over #gsub --- app/sanitizers/rendered_markdown_scrubber.rb | 16 +++++++--- app/services/markdown_processor/parser.rb | 23 ++++++------- .../markdown_processor/parser_spec.rb | 32 ++++++++++++++++--- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/app/sanitizers/rendered_markdown_scrubber.rb b/app/sanitizers/rendered_markdown_scrubber.rb index ca7072297..1390be74b 100644 --- a/app/sanitizers/rendered_markdown_scrubber.rb +++ b/app/sanitizers/rendered_markdown_scrubber.rb @@ -1,16 +1,17 @@ class RenderedMarkdownScrubber < Rails::Html::PermitScrubber + LIQUID_TAG_SYNTAX_REGEX = /\{%|%\}/.freeze def initialize super self.tags = %w[ - a abbr add aside b blockquote br button center cite code col colgroup dd del dl dt em em figcaption - h1 h2 h3 h4 h5 h6 hr i img kbd li mark ol p pre q rp rt ruby small source span strong sub sup table + a abbr add b blockquote br button center cite code col colgroup dd del dl dt em figcaption + h1 h2 h3 h4 h5 h6 hr img kbd li mark ol p pre q rp rt ruby small source span strong sub sup table tbody td tfoot th thead time tr u ul video ] self.attributes = %w[ - alt colspan data-conversation data-lang data-no-instant data-url em height href id loop - name ref rel rowspan size span src start strong title type value width controls + alt colspan data-conversation data-lang data-no-instant data-url href id loop + name ref rel rowspan span src start title type value controls ] end @@ -30,12 +31,19 @@ class RenderedMarkdownScrubber < Rails::Html::PermitScrubber scrub_css_attribute(node) else + scrub_valid_attributes(node) super end end private + def scrub_valid_attributes(node) + node.attributes.each_value do |attribute| + attribute.value = attribute.value.remove(LIQUID_TAG_SYNTAX_REGEX) + end + end + def inside_codeblock?(node) node.attributes["class"]&.value&.include?("highlight") || (node.name == "span" && node.ancestors.first.name == "code") diff --git a/app/services/markdown_processor/parser.rb b/app/services/markdown_processor/parser.rb index 3e3c8e357..e897dfdc1 100644 --- a/app/services/markdown_processor/parser.rb +++ b/app/services/markdown_processor/parser.rb @@ -8,6 +8,7 @@ module MarkdownProcessor ].freeze WORDS_READ_PER_MINUTE = 275.0 + ALLOWED_ATTRIBUTES = %w[href src alt].freeze def initialize(content, source: nil, user: nil) @content = content @@ -52,10 +53,9 @@ module MarkdownProcessor markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG) allowed_tags = %w[strong abbr aside em p h1 h2 h3 h4 h5 h6 i u b code pre br ul ol li small sup sub img a span hr blockquote kbd] - allowed_attributes = %w[href strong em ref rel src title alt] - ActionController::Base.helpers.sanitize markdown.render(@content), + ActionController::Base.helpers.sanitize(markdown.render(@content), tags: allowed_tags, - attributes: allowed_attributes + attributes: ALLOWED_ATTRIBUTES) end def evaluate_limited_markdown @@ -64,10 +64,9 @@ module MarkdownProcessor renderer = Redcarpet::Render::HTMLRouge.new(hard_wrap: true, filter_html: false) markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG) allowed_tags = %w[strong i u b em p br code] - allowed_attributes = %w[href strong em ref rel src title alt] - ActionController::Base.helpers.sanitize markdown.render(@content), + ActionController::Base.helpers.sanitize(markdown.render(@content), tags: allowed_tags, - attributes: allowed_attributes + attributes: ALLOWED_ATTRIBUTES) end def evaluate_inline_limited_markdown @@ -76,10 +75,9 @@ module MarkdownProcessor renderer = Redcarpet::Render::HTMLRouge.new(hard_wrap: true, filter_html: false) markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG) allowed_tags = %w[strong i u b em code] - allowed_attributes = %w[href strong em ref rel src title alt] - ActionController::Base.helpers.sanitize markdown.render(@content), + ActionController::Base.helpers.sanitize(markdown.render(@content), tags: allowed_tags, - attributes: allowed_attributes + attributes: ALLOWED_ATTRIBUTES) end def evaluate_listings_markdown @@ -89,10 +87,9 @@ module MarkdownProcessor markdown = Redcarpet::Markdown.new(renderer, Constants::Redcarpet::CONFIG) allowed_tags = %w[strong abbr aside em p h4 h5 h6 i u b code pre br ul ol li small sup sub a span hr blockquote kbd] - allowed_attributes = %w[href strong em ref rel src title alt] - ActionController::Base.helpers.sanitize markdown.render(@content), + ActionController::Base.helpers.sanitize(markdown.render(@content), tags: allowed_tags, - attributes: allowed_attributes + attributes: ALLOWED_ATTRIBUTES) end def tags_used @@ -112,7 +109,7 @@ module MarkdownProcessor def catch_xss_attempts(markdown) return unless markdown.match?(Regexp.union(BAD_XSS_REGEX)) - raise ArgumentError, "Invalid markdown detected" + raise ArgumentError, "Invalid markdown detected!" end def escape_liquid_tags_in_codeblock(content) diff --git a/spec/services/markdown_processor/parser_spec.rb b/spec/services/markdown_processor/parser_spec.rb index 6a46b8770..afc11daae 100644 --- a/spec/services/markdown_processor/parser_spec.rb +++ b/spec/services/markdown_processor/parser_spec.rb @@ -125,6 +125,11 @@ RSpec.describe MarkdownProcessor::Parser, type: :service do expect(test).to be(content) end + it "permits abbr tags" do + result = generate_and_parse_markdown("OK") + expect(result).to include("OK") + end + context "when rendering links markdown" do # the following specs are testing HTMLRouge it "renders properly if protocol http is included" do @@ -361,11 +366,6 @@ RSpec.describe MarkdownProcessor::Parser, type: :service do result = generate_and_parse_markdown("- [A](#a)\n - [B](#b)\n- [C](#c)") expect(result).not_to include("
") end - - it "permits abbr and aside tags" do - result = generate_and_parse_markdown("