Merge branch 'security-comment-dos-14-9' into '14-9-stable-ee'

Fix kroki exploit

See merge request gitlab-org/security/gitlab!2306

app/assets/javascripts/behaviors/components/diagram_performance_warning.vue

+<script>
+import { GlAlert } from '@gitlab/ui';
+import { __ } from '~/locale';
+
+export default {
+  i18n: {
+    bodyText: __('Warning: Displaying this diagram might cause performance issues on this page.'),
+    buttonText: __('Display'),
+  },
+  components: {
+    GlAlert,
+  },
+};
+</script>
+
+<template>
+  <gl-alert
+    :primary-button-text="$options.i18n.buttonText"
+    variant="warning"
+    @dismiss="$emit('closeAlert')"
+    @primaryAction="$emit('showImage')"
+  >
+    {{ $options.i18n.bodyText }}
+  </gl-alert>
+</template>

app/assets/javascripts/behaviors/markdown/constants.js

 // https://prosemirror.net/docs/ref/#model.ParseRule.priority
 export const DEFAULT_PARSE_RULE_PRIORITY = 50;
 export const HIGHER_PARSE_RULE_PRIORITY = 1 + DEFAULT_PARSE_RULE_PRIORITY;
+
+export const unrestrictedPages = [
+  // Group wiki
+  'groups:wikis:show',
+  'groups:wikis:edit',
+  'groups:wikis:create',
+
+  // Project wiki
+  'projects:wikis:show',
+  'projects:wikis:edit',
+  'projects:wikis:create',
+
+  // Project files
+  'projects:show',
+  'projects:blob:show',
+];

app/assets/javascripts/behaviors/markdown/render_gfm.js

@@ -2,6 +2,7 @@ import $ from 'jquery';
 import syntaxHighlight from '~/syntax_highlight';
 import initUserPopovers from '../../user_popovers';
 import highlightCurrentUser from './highlight_current_user';
+import { renderKroki } from './render_kroki';
 import renderMath from './render_math';
 import renderMermaid from './render_mermaid';
 import renderSandboxedMermaid from './render_sandboxed_mermaid';
@@ -13,6 +14,7 @@ import renderMetrics from './render_metrics';
 //
 $.fn.renderGFM = function renderGFM() {
   syntaxHighlight(this.find('.js-syntax-highlight').get());
+  renderKroki(this.find('.js-render-kroki[hidden]').get());
   renderMath(this.find('.js-render-math'));
   if (gon.features?.sandboxedMermaid) {
     renderSandboxedMermaid(this.find('.js-render-mermaid'));

app/assets/javascripts/behaviors/markdown/render_kroki.js

+import Vue from 'vue';
+import DiagramPerformanceWarning from '../components/diagram_performance_warning.vue';
+import { unrestrictedPages } from './constants';
+
+/**
+ * Create alert element.
+ *
+ * @param {Element} krokiImage Kroki `img` element
+ * @return {Element} Alert element
+ */
+function createAlert(krokiImage) {
+  const app = new Vue({
+    el: document.createElement('div'),
+    name: 'DiagramPerformanceWarningRoot',
+    render(createElement) {
+      return createElement(DiagramPerformanceWarning, {
+        on: {
+          closeAlert() {
+            app.$destroy();
+            app.$el.remove();
+          },
+          showImage() {
+            krokiImage.removeAttribute('hidden');
+            app.$destroy();
+            app.$el.remove();
+          },
+        },
+      });
+    },
+  });
+
+  return app.$el;
+}
+
+/**
+ * Add warning alert to hidden Kroki images,
+ * or show Kroki image if on an unrestricted page.
+ *
+ * Kroki images are given a hidden attribute by the
+ * backend when the original markdown source is large.
+ *
+ * @param {Array<Element>} krokiImages Array of hidden Kroki `img` elements
+ */
+export function renderKroki(krokiImages) {
+  const pageName = document.querySelector('body').dataset.page;
+  const isUnrestrictedPage = unrestrictedPages.includes(pageName);
+
+  krokiImages.forEach((krokiImage) => {
+    if (isUnrestrictedPage) {
+      krokiImage.removeAttribute('hidden');
+      return;
+    }
+
+    const parent = krokiImage.closest('.js-markdown-code');
+
+    // A single Kroki image is processed multiple times for some reason,
+    // so this condition ensures we only create one alert per Kroki image
+    if (!parent.hasAttribute('data-kroki-processed')) {
+      parent.setAttribute('data-kroki-processed', 'true');
+      parent.after(createAlert(krokiImage));
+    }
+  });
+}

app/assets/javascripts/behaviors/markdown/render_mermaid.js

@@ -3,6 +3,7 @@ import { once, countBy } from 'lodash';
 import createFlash from '~/flash';
 import { darkModeEnabled } from '~/lib/utils/color_utils';
 import { __, sprintf } from '~/locale';
+import { unrestrictedPages } from './constants';
 // Renders diagrams and flowcharts from text using Mermaid in any element with the
 // `js-render-mermaid` class.
@@ -30,24 +31,6 @@ let renderedMermaidBlocks = 0;
 let mermaidModule = {};
-// Whitelist pages where we won't impose any restrictions
-// on mermaid rendering
-const WHITELISTED_PAGES = [
-  // Group wiki
-  'groups:wikis:show',
-  'groups:wikis:edit',
-  'groups:wikis:create',
-
-  // Project wiki
-  'projects:wikis:show',
-  'projects:wikis:edit',
-  'projects:wikis:create',
-
-  // Project files
-  'projects:show',
-  'projects:blob:show',
-];
-
 export function initMermaid(mermaid) {
   let theme = 'neutral';
@@ -163,7 +146,7 @@ function renderMermaids($els) {
          * up the entire thread and causing a DoS.
          */
         if (
-          !WHITELISTED_PAGES.includes(pageName) &&
+          !unrestrictedPages.includes(pageName) &&
           ((source && source.length > MAX_CHAR_LIMIT) ||
             renderedChars > MAX_CHAR_LIMIT ||
             renderedMermaidBlocks >= MAX_MERMAID_BLOCK_LIMIT ||

app/assets/javascripts/behaviors/markdown/render_sandboxed_mermaid.js

@@ -9,6 +9,7 @@ import {
 } from '~/lib/utils/url_utility';
 import { darkModeEnabled } from '~/lib/utils/color_utils';
 import { setAttributes } from '~/lib/utils/dom_utils';
+import { unrestrictedPages } from './constants';
 // Renders diagrams and flowcharts from text using Mermaid in any element with the
 // `js-render-mermaid` class.
@@ -36,23 +37,6 @@ const BUFFER_IFRAME_HEIGHT = 10;
 const elsProcessingMap = new WeakMap();
 let renderedMermaidBlocks = 0;
-// Pages without any restrictions on mermaid rendering
-const PAGES_WITHOUT_RESTRICTIONS = [
-  // Group wiki
-  'groups:wikis:show',
-  'groups:wikis:edit',
-  'groups:wikis:create',
-
-  // Project wiki
-  'projects:wikis:show',
-  'projects:wikis:edit',
-  'projects:wikis:create',
-
-  // Project files
-  'projects:show',
-  'projects:blob:show',
-];
-
 function shouldLazyLoadMermaidBlock(source) {
   /**
    * If source contains `&`, which means that it might
@@ -149,7 +133,7 @@ function renderMermaids($els) {
      * up the entire thread and causing a DoS.
      */
     if (
-      !PAGES_WITHOUT_RESTRICTIONS.includes(pageName) &&
+      !unrestrictedPages.includes(pageName) &&
       ((source && source.length > MAX_CHAR_LIMIT) ||
         renderedChars > MAX_CHAR_LIMIT ||
         renderedMermaidBlocks >= MAX_MERMAID_BLOCK_LIMIT ||

lib/banzai/filter/kroki_filter.rb

@@ -6,8 +6,10 @@ require "asciidoctor/extensions/asciidoctor_kroki/extension"
 module Banzai
   module Filter
     # HTML that replaces all diagrams supported by Kroki with the corresponding img tags.
-    #
+    # If the source content is large then the hidden attribute is added to the img tag.
     class KrokiFilter < HTML::Pipeline::Filter
+      MAX_CHARACTER_LIMIT = 2000
+
       def call
         return doc unless settings.kroki_enabled
@@ -21,7 +23,12 @@ module Banzai
         diagram_format = "svg"
         doc.xpath(xpath).each do |node|
           diagram_type = node.parent['lang']
-          img_tag = Nokogiri::HTML::DocumentFragment.parse(%(<img src="#{create_image_src(diagram_type, diagram_format, node.content)}"/>))
+          diagram_src = node.content
+          image_src = create_image_src(diagram_type, diagram_format, diagram_src)
+          lazy_load = diagram_src.length > MAX_CHARACTER_LIMIT
+          other_attrs = lazy_load ? "hidden" : ""
+
+          img_tag = Nokogiri::HTML::DocumentFragment.parse(%(<img class="js-render-kroki" src="#{image_src}" #{other_attrs} />))
           node.parent.replace(img_tag)
         end

locale/gitlab.pot

@@ -13120,6 +13120,9 @@ msgstr ""
 msgid "Dismissed on pipeline %{pipelineLink} at %{projectLink}"
 msgstr ""
+msgid "Display"
+msgstr ""
+
 msgid "Display alerts from all configured monitoring tools."
 msgstr ""

spec/features/markdown/kroki_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'kroki rendering', :js do
+  let_it_be(:project) { create(:project, :public) }
+
+  before do
+    stub_application_setting(kroki_enabled: true, kroki_url: 'http://localhost:8000')
+  end
+
+  it 'shows kroki image' do
+    plain_text = 'This text length is ignored. ' * 300
+
+    description = <<~KROKI
+      #{plain_text}
+      ```plantuml
+      A -> A: T
+      ```
+    KROKI
+
+    issue = create(:issue, project: project, description: description)
+
+    visit project_issue_path(project, issue)
+
+    within('.description') do
+      expect(page).to have_css('img')
+      expect(page).not_to have_text 'Warning: Displaying this diagram might cause performance issues on this page.'
+    end
+  end
+
+  it 'hides kroki image and shows warning alert when kroki source size is large' do
+    plantuml_text = 'A -> A: T ' * 300
+
+    description = <<~KROKI
+      ```plantuml
+      #{plantuml_text}
+      ```
+    KROKI
+
+    issue = create(:issue, project: project, description: description)
+
+    visit project_issue_path(project, issue)
+
+    within('.description') do
+      expect(page).not_to have_css('img')
+      expect(page).to have_text 'Warning: Displaying this diagram might cause performance issues on this page.'
+
+      click_button 'Display'
+
+      expect(page).to have_css('img')
+      expect(page).not_to have_text 'Warning: Displaying this diagram might cause performance issues on this page.'
+    end
+  end
+end

spec/frontend/behaviors/components/diagram_performance_warning_spec.js

+import { GlAlert } from '@gitlab/ui';
+import { shallowMount } from '@vue/test-utils';
+import DiagramPerformanceWarning from '~/behaviors/components/diagram_performance_warning.vue';
+
+describe('DiagramPerformanceWarning component', () => {
+  let wrapper;
+
+  const findAlert = () => wrapper.findComponent(GlAlert);
+
+  beforeEach(() => {
+    wrapper = shallowMount(DiagramPerformanceWarning);
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it('renders warning alert with button', () => {
+    expect(findAlert().props()).toMatchObject({
+      primaryButtonText: DiagramPerformanceWarning.i18n.buttonText,
+      variant: 'warning',
+    });
+  });
+
+  it('renders warning message', () => {
+    expect(findAlert().text()).toBe(DiagramPerformanceWarning.i18n.bodyText);
+  });
+
+  it('emits event when closing alert', () => {
+    findAlert().vm.$emit('dismiss');
+
+    expect(wrapper.emitted('closeAlert')).toEqual([[]]);
+  });
+
+  it('emits event when accepting alert', () => {
+    findAlert().vm.$emit('primaryAction');
+
+    expect(wrapper.emitted('showImage')).toEqual([[]]);
+  });
+});

spec/lib/banzai/filter/kroki_filter_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Banzai::Filter::KrokiFilter do
     stub_application_setting(kroki_enabled: true, kroki_url: "http://localhost:8000")
     doc = filter("<pre lang='nomnoml'><code>[Pirate|eyeCount: Int|raid();pillage()|\n  [beard]--[parrot]\n  [beard]-:>[foul mouth]\n]</code></pre>")
-    expect(doc.to_s).to eq '<img src="http://localhost:8000/nomnoml/svg/eNqLDsgsSixJrUmtTHXOL80rsVLwzCupKUrMTNHQtC7IzMlJTE_V0KzhUlCITkpNLEqJ1dWNLkgsKsoviUUSs7KLTssvzVHIzS8tyYjligUAMhEd0g==">'
+    expect(doc.to_s).to eq '<img class="js-render-kroki" src="http://localhost:8000/nomnoml/svg/eNqLDsgsSixJrUmtTHXOL80rsVLwzCupKUrMTNHQtC7IzMlJTE_V0KzhUlCITkpNLEqJ1dWNLkgsKsoviUUSs7KLTssvzVHIzS8tyYjligUAMhEd0g==">'
   end
   it 'replaces nomnoml pre tag with img tag if both kroki and plantuml are enabled' do
@@ -19,7 +19,7 @@ RSpec.describe Banzai::Filter::KrokiFilter do
                              plantuml_url: "http://localhost:8080")
     doc = filter("<pre lang='nomnoml'><code>[Pirate|eyeCount: Int|raid();pillage()|\n  [beard]--[parrot]\n  [beard]-:>[foul mouth]\n]</code></pre>")
-    expect(doc.to_s).to eq '<img src="http://localhost:8000/nomnoml/svg/eNqLDsgsSixJrUmtTHXOL80rsVLwzCupKUrMTNHQtC7IzMlJTE_V0KzhUlCITkpNLEqJ1dWNLkgsKsoviUUSs7KLTssvzVHIzS8tyYjligUAMhEd0g==">'
+    expect(doc.to_s).to eq '<img class="js-render-kroki" src="http://localhost:8000/nomnoml/svg/eNqLDsgsSixJrUmtTHXOL80rsVLwzCupKUrMTNHQtC7IzMlJTE_V0KzhUlCITkpNLEqJ1dWNLkgsKsoviUUSs7KLTssvzVHIzS8tyYjligUAMhEd0g==">'
   end
   it 'does not replace nomnoml pre tag with img tag if kroki is disabled' do
@@ -38,4 +38,12 @@ RSpec.describe Banzai::Filter::KrokiFilter do
     expect(doc.to_s).to eq '<pre lang="plantuml"><code>Bob-&gt;Alice : hello</code></pre>'
   end
+
+  it 'adds hidden attribute when content size is large' do
+    stub_application_setting(kroki_enabled: true, kroki_url: "http://localhost:8000")
+    text = '[Pirate|eyeCount: Int|raid();pillage()|\n  [beard]--[parrot]\n  [beard]-:>[foul mouth]\n]' * 25
+    doc = filter("<pre lang='nomnoml'><code>#{text}</code></pre>")
+
+    expect(doc.to_s).to eq '<img class="js-render-kroki" src="http://localhost:8000/nomnoml/svg/eNqLDsgsSixJrUmtTHXOL80rsVLwzCupKUrMTNHQtC7IzMlJTE_V0KyJyVNQiE5KTSxKidXVjS5ILCrKL4lFFrSyi07LL81RyM0vLckAysRGjxo8avCowaMGjxo8avCowaMGU8lgAE7mIdc=" hidden>'
+  end
 end