diff --git a/Gemfile b/Gemfile index 9c49d83fd1c935339e5e9f41a52dbba1d576663b..3528fecf6b2f44baffa05378fc04f37a49f63279 100644 --- a/Gemfile +++ b/Gemfile @@ -274,7 +274,7 @@ gem 'asciidoctor-kroki', '~> 0.10.0', require: false, feature_category: :markdow gem 'rouge', '~> 4.6.1', feature_category: :shared gem 'truncato', '~> 0.7.13', feature_category: :team_planning gem 'nokogiri', '~> 1.18', feature_category: :shared -gem 'gitlab-glfm-markdown', '~> 0.0.38', feature_category: :markdown +gem 'gitlab-glfm-markdown', '~> 0.0.39', feature_category: :markdown gem 'tanuki_emoji', '~> 0.13', feature_category: :markdown gem 'unicode-emoji', '~> 4.0', feature_category: :markdown diff --git a/Gemfile.checksum b/Gemfile.checksum index 572f7930ce4d6d8c298b6c56d17cd346e53c85d0..ccc86e4f7f060ed4aa19408ce277eb375aa27247 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -227,13 +227,13 @@ {"name":"gitlab-dangerfiles","version":"4.10.0","platform":"ruby","checksum":"0adb9cfec58ffce42f68b1aef528503bdc89aed3994ba461c67e1d9246513e1c"}, {"name":"gitlab-experiment","version":"1.0.0","platform":"ruby","checksum":"9ff76fe55d30012f0531be97143f3af5f74dd1ef3f33d630f320de0ceec9eab9"}, {"name":"gitlab-fog-azure-rm","version":"2.4.0","platform":"ruby","checksum":"678b86e542a37eda10e63ca02d04c9ff998b771df4aabc1f87e5c20148cb360b"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"aarch64-linux-gnu","checksum":"cc07d942d61c2efab764aa5065e21dc336128d8562db7e0815d0a060689f50fb"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"aarch64-linux-musl","checksum":"4efb65768d641a920583c885d26f255f180e4fe4b3016c86e58e2dfa099fe480"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"arm64-darwin","checksum":"4599b09016698b38bf407fe99139893175bc5d83ccc9040bd77faeceaabdc96f"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"ruby","checksum":"8e57dd9283d655a40f6120dac0728f1d088900228977c139e3c969c756f40bc6"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-darwin","checksum":"0be7832fd9066e296abfd6c465c98e920d0f5722844c75ba0e47867b5b767161"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-linux-gnu","checksum":"6725a555a9d6b35e6a89570acb76b249e2248187d5a665c7581449a26fe2958f"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-linux-musl","checksum":"34a741398ba503873d18dac70994284fefae3b3da3ce0fe6c4e97dd4ac002fc0"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"aarch64-linux-gnu","checksum":"3c589bf98fa04b3ad7760564af0892d53b7cfc7058153a0aab331cf15ffbf06f"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"aarch64-linux-musl","checksum":"c3f1ccd9f958b8be0dfeb4f3cb58a185afdc602318748509f009da58fdd49a51"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"arm64-darwin","checksum":"efddd0a07a1b26a1b9af61dc4d9246b9599877a1221a9e0ceb2a5e69ab664cc2"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"ruby","checksum":"a7d7d237203ebe290c2519b742c2c536ee94401a038ad63f043db9f4166fa0c7"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-darwin","checksum":"9f326f6e6771e4fad240bf9bfb2acd85eb86eb44ef058c58d2ba81bc2e0075d5"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-linux-gnu","checksum":"90a12179eb5a9d311a840dccbab7f6d61c52fef5e526586bcfc70154016af683"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-linux-musl","checksum":"f353114bf503e3389159e22c8af1bd12fea7016081bf60ee05b269b80793c101"}, {"name":"gitlab-kas-grpc","version":"18.5.0.pre.rc4","platform":"ruby","checksum":"8efe8bc957572bad2ee90c22836b285eb65574591db5c8a7bc8080f69ddebcc6"}, {"name":"gitlab-labkit","version":"0.42.2","platform":"ruby","checksum":"c3ac152352d3d508e5b44eddd6299113e168031a9042b25b27df269d3815495d"}, {"name":"gitlab-license","version":"2.6.0","platform":"ruby","checksum":"2c1f8ae73835640ec77bf758c1d0c9730635043c01cf77902f7976e826d7d016"}, diff --git a/Gemfile.lock b/Gemfile.lock index 31a8c9dabf40a2e44e797f934a546aa83b9ddbb6..ee8edd2dac057327d441fa8a79a3b3d60620da38 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -771,7 +771,7 @@ GEM mime-types net-http-persistent (~> 4.0) nokogiri (~> 1, >= 1.10.8) - gitlab-glfm-markdown (0.0.38) + gitlab-glfm-markdown (0.0.39) rb_sys (~> 0.9.109) gitlab-kas-grpc (18.5.0.pre.rc4) grpc (~> 1.0) @@ -2192,7 +2192,7 @@ DEPENDENCIES gitlab-duo-workflow-service-client (~> 0.6)! gitlab-experiment (~> 1.0.0) gitlab-fog-azure-rm (~> 2.4.0) - gitlab-glfm-markdown (~> 0.0.38) + gitlab-glfm-markdown (~> 0.0.39) gitlab-grape-openapi! gitlab-housekeeper! gitlab-http! diff --git a/Gemfile.next.checksum b/Gemfile.next.checksum index 1ee9f50fdd7dfd01cd9aa92f5a5a22385f6df3b1..f153f642e171e5ba0acc366da06c5b2684ae9e01 100644 --- a/Gemfile.next.checksum +++ b/Gemfile.next.checksum @@ -227,13 +227,13 @@ {"name":"gitlab-dangerfiles","version":"4.10.0","platform":"ruby","checksum":"0adb9cfec58ffce42f68b1aef528503bdc89aed3994ba461c67e1d9246513e1c"}, {"name":"gitlab-experiment","version":"1.0.0","platform":"ruby","checksum":"9ff76fe55d30012f0531be97143f3af5f74dd1ef3f33d630f320de0ceec9eab9"}, {"name":"gitlab-fog-azure-rm","version":"2.4.0","platform":"ruby","checksum":"678b86e542a37eda10e63ca02d04c9ff998b771df4aabc1f87e5c20148cb360b"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"aarch64-linux-gnu","checksum":"cc07d942d61c2efab764aa5065e21dc336128d8562db7e0815d0a060689f50fb"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"aarch64-linux-musl","checksum":"4efb65768d641a920583c885d26f255f180e4fe4b3016c86e58e2dfa099fe480"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"arm64-darwin","checksum":"4599b09016698b38bf407fe99139893175bc5d83ccc9040bd77faeceaabdc96f"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"ruby","checksum":"8e57dd9283d655a40f6120dac0728f1d088900228977c139e3c969c756f40bc6"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-darwin","checksum":"0be7832fd9066e296abfd6c465c98e920d0f5722844c75ba0e47867b5b767161"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-linux-gnu","checksum":"6725a555a9d6b35e6a89570acb76b249e2248187d5a665c7581449a26fe2958f"}, -{"name":"gitlab-glfm-markdown","version":"0.0.38","platform":"x86_64-linux-musl","checksum":"34a741398ba503873d18dac70994284fefae3b3da3ce0fe6c4e97dd4ac002fc0"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"aarch64-linux-gnu","checksum":"3c589bf98fa04b3ad7760564af0892d53b7cfc7058153a0aab331cf15ffbf06f"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"aarch64-linux-musl","checksum":"c3f1ccd9f958b8be0dfeb4f3cb58a185afdc602318748509f009da58fdd49a51"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"arm64-darwin","checksum":"efddd0a07a1b26a1b9af61dc4d9246b9599877a1221a9e0ceb2a5e69ab664cc2"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"ruby","checksum":"a7d7d237203ebe290c2519b742c2c536ee94401a038ad63f043db9f4166fa0c7"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-darwin","checksum":"9f326f6e6771e4fad240bf9bfb2acd85eb86eb44ef058c58d2ba81bc2e0075d5"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-linux-gnu","checksum":"90a12179eb5a9d311a840dccbab7f6d61c52fef5e526586bcfc70154016af683"}, +{"name":"gitlab-glfm-markdown","version":"0.0.39","platform":"x86_64-linux-musl","checksum":"f353114bf503e3389159e22c8af1bd12fea7016081bf60ee05b269b80793c101"}, {"name":"gitlab-kas-grpc","version":"18.5.0.pre.rc4","platform":"ruby","checksum":"8efe8bc957572bad2ee90c22836b285eb65574591db5c8a7bc8080f69ddebcc6"}, {"name":"gitlab-labkit","version":"0.42.2","platform":"ruby","checksum":"c3ac152352d3d508e5b44eddd6299113e168031a9042b25b27df269d3815495d"}, {"name":"gitlab-license","version":"2.6.0","platform":"ruby","checksum":"2c1f8ae73835640ec77bf758c1d0c9730635043c01cf77902f7976e826d7d016"}, diff --git a/Gemfile.next.lock b/Gemfile.next.lock index d2a47efe52ca44302b6ddb9c22305e966994d8b2..64e8a91c97429993338838f45e10bdb117feb927 100644 --- a/Gemfile.next.lock +++ b/Gemfile.next.lock @@ -765,7 +765,7 @@ GEM mime-types net-http-persistent (~> 4.0) nokogiri (~> 1, >= 1.10.8) - gitlab-glfm-markdown (0.0.38) + gitlab-glfm-markdown (0.0.39) rb_sys (~> 0.9.109) gitlab-kas-grpc (18.5.0.pre.rc4) grpc (~> 1.0) @@ -2187,7 +2187,7 @@ DEPENDENCIES gitlab-duo-workflow-service-client (~> 0.6)! gitlab-experiment (~> 1.0.0) gitlab-fog-azure-rm (~> 2.4.0) - gitlab-glfm-markdown (~> 0.0.38) + gitlab-glfm-markdown (~> 0.0.39) gitlab-grape-openapi! gitlab-housekeeper! gitlab-http! 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 6ccd218411de7b17834df250d9a557a8352a3feb..7fe196e0a343e6af9bcb4d56c23cd65703898e87 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -611,25 +611,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/include_filter.rb b/lib/banzai/filter/include_filter.rb index 8fdf55962d2cd427fddf56ca3923ad9bc87b6181..f6400a5b5c114900a53011705571c6de2e11f721 100644 --- a/lib/banzai/filter/include_filter.rb +++ b/lib/banzai/filter/include_filter.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'glfm_markdown' +require 'gitlab-glfm-markdown' module Banzai module Filter diff --git a/lib/banzai/filter/markdown_engines/glfm_markdown.rb b/lib/banzai/filter/markdown_engines/glfm_markdown.rb index 01bdbab94a46976b2bb56f283de488d6d915034a..9bc9b764a97638deca4bd39b10f6b691c9cbeecc 100644 --- a/lib/banzai/filter/markdown_engines/glfm_markdown.rb +++ b/lib/banzai/filter/markdown_engines/glfm_markdown.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true -require 'glfm_markdown' +require 'gitlab-glfm-markdown' -# Use the glfm_markdown gem (https://gitlab.com/gitlab-org/ruby/gems/gitlab-glfm-markdown) +# Use the gitlab-glfm-markdown gem +# (https://gitlab.com/gitlab-org/ruby/gems/gitlab-glfm-markdown) # to interface with the Rust based `comrak` parser # https://github.com/kivikakk/comrak module Banzai @@ -24,18 +25,22 @@ class GlfmMarkdown < Base github_pre_lang: true, hardbreaks: false, 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, + 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 b64ce6520d09fbec7a9ecb515ad7fcbd7ef216bc..ba32d46633e06e7ab2d61467a089d677a59ce803 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -19,6 +19,7 @@ def customize_allowlist(allowlist) allow_id_attributes(allowlist) allow_class_attributes(allowlist) allow_section_footnotes(allowlist) + allow_tasklists(allowlist) allowlist end @@ -61,7 +62,11 @@ def allow_class_attributes(allowlist) allowlist[:attributes]['p'] = %w[class] allowlist[:attributes]['span'].push('class') allowlist[:attributes]['code'].push('class') - allowlist[:transformers].push(self.class.remove_unsafe_classes) + 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 def allow_section_footnotes(allowlist) @@ -71,6 +76,12 @@ def allow_section_footnotes(allowlist) allowlist[:attributes]['a'].push('data-footnote-ref', 'data-footnote-backref', 'data-footnote-backref-idx') 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) do @@ -87,24 +98,28 @@ def remove_unsafe_table_style end end - def remove_unsafe_classes - ->(env) do - node = env[:node] - - return unless node.has_attribute?('class') - - case node.name - when 'a' - node.remove_attribute('class') if remove_link_class?(node) - when 'div' - node.remove_attribute('class') if remove_div_class?(node) - when 'p' - node.remove_attribute('class') if remove_p_class?(node) - when 'span' - node.remove_attribute('class') if remove_span_class?(node) - when 'code' - node.remove_attribute('class') if remove_code_class?(node) - end + def remove_unsafe_classes(env) # rubocop:disable Metrics/CyclomaticComplexity -- dispatch method. + node = env[:node] + + return unless node.has_attribute?('class') + + case node.name + when 'a' + node.remove_attribute('class') if remove_link_class?(node) + when 'div' + node.remove_attribute('class') if remove_div_class?(node) + when 'p' + node.remove_attribute('class') if remove_p_class?(node) + when 'span' + 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 @@ -153,6 +168,29 @@ def remove_id_attributes node.remove_attribute('id') end 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_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/lib/gitlab/string_placeholder_replacer.rb b/lib/gitlab/string_placeholder_replacer.rb index 3f2b66e96d683218073cd7d110df30ed27544a37..c5181acef0b8d1aec81fc4be320c4bd7501fccef 100644 --- a/lib/gitlab/string_placeholder_replacer.rb +++ b/lib/gitlab/string_placeholder_replacer.rb @@ -17,7 +17,7 @@ def self.replace_string_placeholders(string, placeholder_regex = nil, limit: 0, end def self.placeholder_full_regex(placeholder_regex) - /%(\{|%7B)(#{placeholder_regex})(\}|%7D)/ + /%(\{|25%7B)(#{placeholder_regex})(\}|%7D)/ end class << self diff --git a/spec/lib/banzai/filter/placeholders_post_filter_spec.rb b/spec/lib/banzai/filter/placeholders_post_filter_spec.rb index cbde1df2cdb91e6330487e1333fcbf8bbcb25017..233ef0970d71cdf090b064adb279f04efb444d9c 100644 --- a/spec/lib/banzai/filter/placeholders_post_filter_spec.rb +++ b/spec/lib/banzai/filter/placeholders_post_filter_spec.rb @@ -284,28 +284,26 @@ def expect_replaces_placeholder(markdown, expected) it 'does not allow project_title in a link href' do markdown = '[test](%{gitlab_server}/%{project_title}/foo.png)' expected = '

        test

        ' + 'data-placeholder="%25%7Bgitlab_server%7D/%25%7Bproject_title%7D/foo.png" ' \ + 'data-canonical-src="%25%7Bgitlab_server%7D/%25%7Bproject_title%7D/foo.png">test

        ' expect(run_pipeline(markdown)).to eq expected end it 'does not recognize unknown placeholder in a link href' do markdown = '[test](%{gitlab_server}/%{foo}/foo.png)' - expected = '

        test

        ' + expected = '

        test

        ' expect(run_pipeline(markdown)).to eq expected end - it 'does not allow placeholders in link text (parser limitation)' do + it 'allows placeholders in link text' do markdown = '[%{gitlab_server}](%{gitlab_server})' - expected = '

        %{gitlab_server}

        ' + expected = '

        ' \ + 'localhost

        ' expect(run_pipeline(markdown)).to eq expected end @@ -336,9 +334,9 @@ def expect_replaces_placeholder(markdown, expected) it 'generates the correct attributes' do markdown = '![](https://%{gitlab_server}/%{project_name}/foo.png)' expected = "

        ' + 'data-canonical-src="https://%25%7Bgitlab_server%7D/%25%7Bproject_name%7D/foo.png">

        ' # can't use the pipeline because the image_link_filter modifies `src` expect(run_filter(markdown)).to eq expected @@ -347,9 +345,9 @@ def expect_replaces_placeholder(markdown, expected) it 'does not allow project_title in a src' do markdown = '![](https://%{gitlab_server}/%{project_title}/foo.png)' expected = "

        ' + 'data-canonical-src="https://%25%7Bgitlab_server%7D/%25%7Bproject_title%7D/foo.png">

        ' expect(run_filter(markdown)).to eq expected end @@ -359,9 +357,9 @@ def expect_replaces_placeholder(markdown, expected) markdown = '![](https://%{gitlab_server}/%{project_name}/foo.png)' expected = "

        ' + 'data-canonical-src="https://%25%7Bgitlab_server%7D/%25%7Bproject_name%7D/foo.png">

        ' expect(run_filter(markdown)).to eq expected end @@ -379,7 +377,7 @@ def expect_replaces_placeholder(markdown, expected) expect(result).to include "href=\"https://#{Gitlab.config.gitlab.host}/#{project.path}/foo.png\"" expect(result).to include "data-src=\"https://#{Gitlab.config.gitlab.host}/#{project.path}/foo.png\"" - expect(result).to include 'data-canonical-src="https://%%7Bgitlab_server%7D/%%7Bproject_name%7D/foo.png"' + expect(result).to include 'data-canonical-src="https://%25%7Bgitlab_server%7D/%25%7Bproject_name%7D/foo.png"' expect(result) .to include ') | %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 @@ -275,10 +288,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/lib/gitlab/string_placeholder_replacer_spec.rb b/spec/lib/gitlab/string_placeholder_replacer_spec.rb index 3b4c04cbd9c627a2c8b0e4dedd89b80448025cdc..4a3604bfef7bc6cba99533fdb1aed0383f4ce58a 100644 --- a/spec/lib/gitlab/string_placeholder_replacer_spec.rb +++ b/spec/lib/gitlab/string_placeholder_replacer_spec.rb @@ -45,7 +45,7 @@ end it 'replaces placeholders with encoded brackets' do - string = '%%7Bpath%7D/%{id}/%{branch}' + string = '%25%7Bpath%7D/%{id}/%{branch}' result = described_class.replace_string_placeholders(string, regex) do |arg| 'WHATEVER' @@ -60,7 +60,7 @@ it 'returns correct regex' do regex = described_class.placeholder_full_regex('(one|two)') - expect(regex).to eq(/%(\{|%7B)((one|two))(\}|%7D)/) + expect(regex).to eq(/%(\{|25%7B)((one|two))(\}|%7D)/) end 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 "