diff --git a/app/assets/javascripts/content_editor/extensions/task_item.js b/app/assets/javascripts/content_editor/extensions/task_item.js index d05944bc0028a2d38561b282842e2e8003da2cf7..84b75a4267541c199547aca7bf40fc332dfb5925 100644 --- a/app/assets/javascripts/content_editor/extensions/task_item.js +++ b/app/assets/javascripts/content_editor/extensions/task_item.js @@ -46,7 +46,7 @@ export default TaskItem.extend({ priority: PARSE_HTML_PRIORITY_HIGHEST, }, { - tag: 'li.inapplicable > s, li.inapplicable > p:first-of-type > s', + tag: 'li.inapplicable s.inapplicable', skip: true, priority: PARSE_HTML_PRIORITY_HIGHEST, }, diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 4ea41798e7b4aea1e8f34c57015d1f28024c8753..66382c517adffde1d9e5a1fe31d01dca0fba032f 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -603,25 +603,13 @@ } li.inapplicable { - // for a single line list item, no paragraph (tight list) - > s { + // For a single line list item, there's no

(tight list); text nodes are wrapped in . + s.inapplicable { @apply gl-text-disabled; } - // additional blocks, other than paragraphs - > div { - text-decoration: line-through; - @apply gl-text-disabled; - } - - // because of the embedded checkbox, putting line-through on the entire - // paragraph causes the space between the checkbox and the text to have the - // line-through. Targeting just the `s` fixes this - > p:first-of-type > s { - @apply gl-text-disabled; - } - - > p:not(:first-of-type) { + // Strikethrough block-level items in non-tight lists as a whole; we don't do sublists. + > p, div { text-decoration: line-through; @apply gl-text-disabled; } diff --git a/lib/banzai/filter/markdown_engines/glfm_markdown.rb b/lib/banzai/filter/markdown_engines/glfm_markdown.rb index fc891d9919bfa23686e6d4c0eba2b7cfbe1bb953..77a6cfdd01ec67c8d85a931e43c75e8aa9697606 100644 --- a/lib/banzai/filter/markdown_engines/glfm_markdown.rb +++ b/lib/banzai/filter/markdown_engines/glfm_markdown.rb @@ -24,19 +24,22 @@ class GlfmMarkdown < Base hardbreaks: false, header_accessibility: true, header_ids: Banzai::Renderer::USER_CONTENT_ID_PREFIX, + inapplicable_tasks: true, math_code: true, math_dollars: true, multiline_block_quotes: true, only_escape_chars: REFERENCE_CHARS, placeholder_detection: true, relaxed_autolinks: true, - sourcepos: true, + relaxed_tasklist_character: true, smart: false, + sourcepos: true, strikethrough: true, table: true, tagfilter: false, - tasklist: false, # still handled by a banzai filter/gem, - taskfilter_in_table: false, + tasklist: true, + tasklist_classes: true, + tasklist_in_table: false, wikilinks_title_before_pipe: true, unsafe: true }.freeze diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index 5e768163716a55f854cf5decd7b451a9d8cc3f89..4d758019a1a91fde73c2810289d5f8518924bb41 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -21,6 +21,7 @@ def customize_allowlist(allowlist) allow_class_attributes(allowlist) allow_section_footnotes(allowlist) allow_anchor_data_heading_content(allowlist) + allow_tasklists(allowlist) allowlist end @@ -65,6 +66,10 @@ def allow_class_attributes(allowlist) allowlist[:attributes]['p'] = %w[class] allowlist[:attributes]['span'].push('class') allowlist[:attributes]['code'].push('class') + allowlist[:attributes]['ul'] = %w[class] + allowlist[:attributes]['ol'] = %w[class] + allowlist[:attributes]['li'].push('class') + allowlist[:attributes]['input'] = %w[class] allowlist[:transformers].push(self.class.method(:remove_unsafe_classes)) end @@ -79,6 +84,12 @@ def allow_anchor_data_heading_content(allowlist) allowlist[:attributes]['a'].push('data-heading-content') end + def allow_tasklists(allowlist) + allowlist[:elements].push('input') + allowlist[:attributes]['input'].push('data-inapplicable') + allowlist[:transformers].push(self.class.method(:remove_non_tasklist_inputs)) + end + class << self def remove_unsafe_table_style(env) node = env[:node] @@ -93,7 +104,7 @@ def remove_unsafe_table_style(env) end end - def remove_unsafe_classes(env) + def remove_unsafe_classes(env) # rubocop:disable Metrics/CyclomaticComplexity -- dispatch method. node = env[:node] return unless node.has_attribute?('class') @@ -109,6 +120,12 @@ def remove_unsafe_classes(env) node.remove_attribute('class') if remove_span_class?(node) when 'code' node.remove_attribute('class') if remove_code_class?(node) + when 'ul', 'ol' + node.remove_attribute('class') if remove_ul_ol_class?(node) + when 'li' + node.remove_attribute('class') if remove_li_class?(node) + when 'input' + node.remove_attribute('class') if remove_input_class?(node) end end @@ -138,6 +155,19 @@ def remove_code_class?(node) node['class'] != 'idiff' end + def remove_ul_ol_class?(node) + node['class'] != 'task-list' + end + + def remove_li_class?(node) + node['class'] != 'task-list-item' && + node['class'] != 'inapplicable task-list-item' + end + + def remove_input_class?(node) + node['class'] != 'task-list-item-checkbox' + end + def remove_id_attributes(env) node = env[:node] return unless node.has_attribute?('id') @@ -160,6 +190,16 @@ def remove_id_attributes(env) node.remove_attribute('id') end + + def remove_non_tasklist_inputs(env) + node = env[:node] + + return unless node.name == 'input' + + return if node['type'] == 'checkbox' && node['class'] == 'task-list-item-checkbox' && node.parent.name == 'li' + + node.remove + end end end end diff --git a/lib/banzai/filter/task_list_filter.rb b/lib/banzai/filter/task_list_filter.rb index f2fa74fa9fe99d5a811d23dc625a37443d241d20..0ec613889ba63833f8fed133146b0dd35cf346f8 100644 --- a/lib/banzai/filter/task_list_filter.rb +++ b/lib/banzai/filter/task_list_filter.rb @@ -1,108 +1,88 @@ # frozen_string_literal: true -require 'task_list/filter' - -# Generated HTML is transformed back to GFM by: -# - app/assets/javascripts/behaviors/markdown/nodes/ordered_task_list.js -# - app/assets/javascripts/behaviors/markdown/nodes/task_list.js -# - app/assets/javascripts/behaviors/markdown/nodes/task_list_item.js module Banzai module Filter - # TaskList filter replaces task list item markers (`[ ]`, `[x]`, and `[~]`) - # with checkboxes, marked up with metadata and behavior. - # - # This should be run on the HTML generated by the Markdown filter, after the - # SanitizationFilter. - # - # Syntax - # ------ - # - # Task list items must be in a list format: + # TaskListFilter annotates task list items with aria-labels, creates preceding + # elements, and adds strikethroughs to the text body of inapplicable items (created with `[~]`). # - # ``` - # - [ ] incomplete - # - [x] complete - # - [~] inapplicable - # ``` - # - # This class overrides TaskList::Filter in the `deckar01-task_list` gem - # to add support for inapplicable task items - class TaskListFilter < TaskList::Filter + # This should be run on the HTML generated by the Markdown filter, which handles the actual + # parsing, after the SanitizationFilter. + class TaskListFilter < HTML::Pipeline::Filter prepend Concerns::PipelineTimingCheck - extend ::Gitlab::Utils::Override - XPATH = 'descendant-or-self::li[input[@data-inapplicable]] | descendant-or-self::li[p[input[@data-inapplicable]]]' - INAPPLICABLE = '[~]' - INAPPLICABLEPATTERN = /\[~\]/ + CSS = 'input.task-list-item-checkbox' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze - # Pattern used to identify all task list items. - # Useful when you need iterate over all items. - NEWITEMPATTERN = / - \A - (?:\s*[-+*]|(?:\d+\.))? # optional list prefix - \s* # optional whitespace prefix - ( # checkbox - #{CompletePattern}| - #{IncompletePattern}| - #{INAPPLICABLEPATTERN} - ) - (?=\s) # followed by whitespace - /x + def call + doc.xpath(XPATH).each do |node| + text_content = +'' + yield_next_siblings_until(node, %w[ol ul]) do |el| + text_content << el.text + end + truncated_text_content = text_content.strip.truncate(100, separator: ' ', omission: '…') + node['aria-label'] = format(_('Check option: %{option}'), option: truncated_text_content) - # Force the gem's constant to use our new one - superclass.send(:remove_const, :ItemPattern) # rubocop: disable GitlabSecurity/PublicSend - superclass.const_set(:ItemPattern, NEWITEMPATTERN) + node.add_previous_sibling(node.document.create_element('task-button')) - def inapplicable?(item) - !!(item.checkbox_text =~ INAPPLICABLEPATTERN) - end + next unless node.has_attribute?('data-inapplicable') - override :render_item_checkbox - def render_item_checkbox(item) - stripped_source = item.source.sub(ItemPattern, '').strip - text = stripped_source.partition(/\<(ol|ul)/) - source = ActionView::Base.full_sanitizer.sanitize(text[0]) - truncated_source = source.truncate(100, separator: ' ', omission: '…') - aria_label = format(_('Check option: %{option}'), option: truncated_source) + # We manually apply a to strikethrough text in inapplicable task items, + # specifically in tight lists where text within the list items isn't contained in a paragraph. + # (Those are handled entirely by styles.) + # + # To handle tight lists, we wrap every text node after the checkbox in , not descending + # into

or

(as they're indicative of non-tight lists) or
    or
      (as we + # explicitly want to avoid strikethrough styles on sublists, which may have applicable + # task items!). - %() - end + # This is awkward, but we need to include a text node with a space after the input. + # Otherwise, the strikethrough will start *immediately* next to the , because + # the first next sibling of the input is always a text node that starts with a space! + space = node.add_next_sibling(node.document.create_text_node(' ')) - override :render_task_list_item - def render_task_list_item(item) - source = item.source + inapplicable_s = node.document.create_element('s') + inapplicable_s['class'] = 'inapplicable' - if inapplicable?(item) - # Add a `` tag around the list item text. However because of the - # way tasks are built, the source can include an embedded sublist, like - # `[~] foobar\n
        ` should only be added to the main text. - source = source.partition("#{INAPPLICABLE} ") - text = source.last.partition(/\<(ol|ul)/) - text[0] = "#{text[0]}" - source[-1] = text.join - source = source.join + yield_text_nodes_without_descending_into(space.next_sibling, %w[p div ul ol]) do |el| + el.wrap(inapplicable_s) + end end - Nokogiri::HTML.fragment \ - source.sub(ItemPattern, render_item_checkbox(item)), 'utf-8' + doc end - override :call - def call - super - - # add class to li for any inapplicable checkboxes - doc.xpath(XPATH).each do |li| - li.add_class('inapplicable') + # Yields the #next_sibling of start, and then the #next_sibling of that, until either + # there are no more next siblings or a matching element is encountered. + # + # The following #next_sibling is evaluated *before* each element is yielded, so they + # can safely be reparented or removed without affecting iteration. + def yield_next_siblings_until(start, els) + it = start.next_sibling + while it && els.exclude?(it.name) + following = it.next_sibling + yield it + it = following end + end - doc + # Starting from start, iteratively yield text nodes contained within its children, + # and its (repeated) #next_siblings and their children, not descending into any of + # the elements given by els. + # + # The following #next_sibling is evaluated before yielding, as above. + def yield_text_nodes_without_descending_into(start, els) + stack = [start] + while stack.any? + it = stack.pop + + stack << it.next_sibling if it.next_sibling + + if it.text? + yield it unless it.content.blank? + elsif els.exclude?(it.name) + stack.concat(it.children.reverse) + end + end end end end diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index ad1c084bca8678e792125d0b8fcf2d08c17bc2a2..e2d6320b445a447e18c7839a91421c6d56e1c8d5 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -261,6 +261,19 @@ %q() | %q() %q() | %q() + + %q(
          ) | %q(
            ) + %q(
              ) | %q(
                ) + + %q(
                  ) | %q(
                    ) + %q(
                      ) | %q(
                        ) + + %q(
                        ) | %q(
                        ) + %q(
                        ) | %q(
                        ) + %q(
                        ) | %q(
                        ) + + %q(
                        ) | %q(
                        ) + %q(
                        ) | %q(
                        ) end # rubocop:enable Layout/LineLength @@ -268,10 +281,40 @@ it 'removes classes' do act = filter(html) - expect(act.to_html).to eq sanitized + expect(act.to_html).to eq_html sanitized end end end + + describe 'tasklist inputs' do + it 'leaves tasklist inputs' do + exp = %q(
                        ) + act = filter(exp) + + expect(act.to_html).to eq exp + end + + it 'removes tasklist inputs outside of lists' do + exp = %q(

                        Hello

                        ) + act = filter(%q(

                        Hello

                        )) + + expect(act.to_html).to eq exp + end + + it 'removes non-tasklist class inputs' do + exp = %q(
                        ) + act = filter(%q(
                        )) + + expect(act.to_html).to eq_html exp + end + + it 'removes non-tasklist type inputs' do + exp = %q(
                        ) + act = filter(%q(
                        )) + + expect(act.to_html).to eq_html exp + end + end end it_behaves_like 'does not use pipeline timing check' diff --git a/spec/lib/banzai/filter/task_list_filter_spec.rb b/spec/lib/banzai/filter/task_list_filter_spec.rb index 3e16c92046b11b84e6c88eb3f6f75b69f081c2ab..2850324dbcc75e8a6ecd9c96ea0ed46f119199e1 100644 --- a/spec/lib/banzai/filter/task_list_filter_spec.rb +++ b/spec/lib/banzai/filter/task_list_filter_spec.rb @@ -6,13 +6,30 @@ include FilterSpecHelper it 'adds `` to every list item' do - doc = filter("
                          \n
                        • [ ] testing item 1
                          • \n
                        • [x] testing item 2
                          • \n
                        ") + doc = reference_filter(<<~MARKDOWN) + * [ ] testing item 1 + * [x] testing item 2 + MARKDOWN expect(doc.xpath('.//li//task-button').count).to eq(2) end it 'adds `aria-label` to every checkbox in the list' do - doc = filter("
                          \n
                        • [ ] testing item 1
                          • \n
                        • [x] testing item 2
                          • \n
                        • [~] testing item 3
                          • \n
                        • [~] testing item 4 this is a very long label that should be truncated at some point but where does it truncate?
                          • \n
                        • [ ]
                          suspicious item
                          • \n
                        • [ ]
                          suspicious item 2
                          • \n
                        • [~]
                          • \n
                        • [~]
                          • \n
                        • [~]
                          • \n
                        • [~] " hijacking quotes \" a \' b ' c
                        ") + # Some of these test cases are not possible to encounter in practice: + # they imply these tags or attributes made it past SanitizationFilter. + # Even if they were inserted into aria-label, there is no XSS possible as aria-label is text. + doc = reference_filter(<<~MARKDOWN) + * [ ] testing item 1 + * [x] testing item 2 + * [~] testing item 3 + * [~] testing item 4 this is a very long label that should be truncated at some point but where does it truncate? + * [ ]
                        suspicious item
                        + * [ ]
                        suspicious item 2
                        + * [~] + * [~] + * [~] + * [~] " hijacking quotes " a ' b ' c + MARKDOWN aria_labels = doc.xpath('.//li//input/@aria-label') @@ -23,60 +40,81 @@ expect(aria_labels[3].value).to eq('Check option: testing item 4 this is a very long label that should be truncated at some point but where does it…') expect(aria_labels[4].value).to eq('Check option: suspicious item') expect(aria_labels[5].value).to eq('Check option: suspicious item 2') - expect(aria_labels[6].value).to eq('Check option: ') - expect(aria_labels[7].value).to eq('Check option: ') - expect(aria_labels[8].value).to eq('Check option: ') + expect(aria_labels[6].value).to eq('Check option: alert(1)') + expect(aria_labels[7].value).to eq('Check option: alert(1)') + expect(aria_labels[8].value).to eq('Check option: x="",alert(1)//";') expect(aria_labels[9].value).to eq("Check option: \" hijacking quotes \" a ' b ' c") end it 'ignores checkbox on following line' do - doc = filter( - <<~HTML -
                          -
                        • one -
                            -
                          • foo - [ ] bar
                            • -
                          -
                          • -
                        - HTML - ) + doc = reference_filter(<<~MARKDOWN) + * one + * foo + [ ] bar + MARKDOWN expect(doc.xpath('.//li//input').count).to eq(0) end + it 'supports all kinds of spaces for unchecked items' do + doc = reference_filter(<<~MARKDOWN) + - [\u00a0] NO-BREAK SPACE (U+00A0) + - [\u2007] FIGURE SPACE (U+2007) + - [\u202f] NARROW NO-BREAK SPACE (U+202F) + - [\u2009] THIN SPACE (U+2009) + - [\u0020] SPACE (U+0020) + - [x] Checked! + MARKDOWN + + expect(doc.css('input[checked]').count).to eq(1) + expect(doc.css('input:not([checked])').count).to eq(5) + end + describe 'inapplicable list items' do - shared_examples 'a valid inapplicable task list item' do |html| - it "behaves correctly for `#{html}`" do - doc = filter("
                        • #{html}
                        ") + shared_examples 'a valid inapplicable task list item' do |markdown, s_nodes_expected| + it "behaves correctly for `#{markdown}`" do + doc = reference_filter("* #{markdown}") expect(doc.css('li.inapplicable input[data-inapplicable]').count).to eq(1) - expect(doc.css('li.inapplicable > s').count).to eq(1) + expect(doc.css('li.inapplicable s.inapplicable').count).to eq(s_nodes_expected) end end - shared_examples 'an invalid inapplicable task list item' do |html| - it "does nothing for `#{html}`" do - doc = filter("
                        • #{html}
                        ") + shared_examples 'an invalid inapplicable task list item' do |markdown| + it "does nothing for `#{markdown}`" do + doc = reference_filter("* #{markdown}") expect(doc.css('li.inapplicable input[data-inapplicable]').count).to eq(0) end end - it_behaves_like 'a valid inapplicable task list item', '[~] foobar' - it_behaves_like 'a valid inapplicable task list item', '[~] foo bar' + it_behaves_like 'a valid inapplicable task list item', '[~] foobar', 1 + it_behaves_like 'a valid inapplicable task list item', '[~] foo bar', 2 it_behaves_like 'an invalid inapplicable task list item', '[ ] foobar' it_behaves_like 'an invalid inapplicable task list item', '[x] foobar' it_behaves_like 'an invalid inapplicable task list item', 'foo [~] bar' it 'does not wrap a sublist with ' do - html = '[~] foo bar\n
                        1. sublist
                        ' - doc = filter("
                        • #{html}
                        ") + doc = reference_filter(<<~MARKDOWN) + * [~] foo _bar_
                        1. cursed
                        **quux** xyz + MARKDOWN - expect(doc.to_html).to include('foo bar\n') + # Non-blank text nodes should be wrapped in , apart from those within a sublist. + expect(doc.to_html).to include_html(' foo bar ') + expect(doc.to_html).to include_html(' quux xyz') expect(doc.css('li.inapplicable input[data-inapplicable]').count).to eq(1) - expect(doc.css('li.inapplicable > s').count).to eq(1) + expect(doc.css('li.inapplicable s.inapplicable').count).to eq(4) + end + + it 'does cooperate with a following paragraph' do + doc = reference_filter(<<~MARKDOWN) + * [~] foo _bar_ + + yay! + MARKDOWN + + # All content is within paragraph tag; no required. + expect(doc.css('li.inapplicable s.inapplicable').count).to eq(0) end end diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index d188e17712b497723a2704a3138f5ad291327cfc..afed8ad5815008180851d28e8ab5e05856526171 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -196,7 +196,10 @@ expect(toggler.execute).to be_truthy expect(toggler.updated_markdown.lines[0]).to eq "> > * [x] Task 1\n" - expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + task_1_checkbox = toggler_updated_fragment(toggler).css( + 'li[data-sourcepos="1:5-1:16"] > input.task-list-item-checkbox').first + expect(task_1_checkbox['checked']).not_to be_nil + expect(task_1_checkbox['disabled']).not_to be_nil end it 'properly handles a GitLab blockquote' do @@ -221,7 +224,10 @@ expect(toggler.execute).to be_truthy expect(toggler.updated_markdown.lines[4]).to eq "* [x] Task 1\n" - expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + task_1_checkbox = toggler_updated_fragment(toggler).css( + 'li[data-sourcepos="5:1-5:12"] > input.task-list-item-checkbox').first + expect(task_1_checkbox['checked']).not_to be_nil + expect(task_1_checkbox['disabled']).not_to be_nil end context 'when clicking an embedded subtask' do @@ -243,7 +249,10 @@ expect(toggler.execute).to be_truthy expect(toggler.updated_markdown.lines[0]).to eq "- - [x] Task 1\n" - expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + task_1_checkbox = toggler_updated_fragment(toggler).css( + 'li[data-sourcepos="1:3-1:14"] > input.task-list-item-checkbox').first + expect(task_1_checkbox['checked']).not_to be_nil + expect(task_1_checkbox['disabled']).not_to be_nil end it 'properly handles it inside an ordered list' do @@ -264,11 +273,18 @@ expect(toggler.execute).to be_truthy expect(toggler.updated_markdown.lines[0]).to eq "1. - [x] Task 1\n" - expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + task_1_checkbox = toggler_updated_fragment(toggler).css( + 'li[data-sourcepos="1:4-1:15"] > input.task-list-item-checkbox').first + expect(task_1_checkbox['checked']).not_to be_nil + expect(task_1_checkbox['disabled']).not_to be_nil end end def parse_markdown(markdown) Banzai::Pipeline::FullPipeline.call(markdown, project: nil)[:output].to_html end + + def toggler_updated_fragment(toggler) + Nokogiri::HTML.fragment(toggler.updated_markdown_html) + end end diff --git a/spec/support/matchers/eq_html.rb b/spec/support/matchers/eq_html.rb index ab1c9dcf846cc2c106a52d5e334a8154c440e8c1..a494fa25a325d73fc0682036200a530baaa12cba 100644 --- a/spec/support/matchers/eq_html.rb +++ b/spec/support/matchers/eq_html.rb @@ -52,25 +52,32 @@ # partial tag will normalise to text ("" # includes the HTML "