Enforce max chars and max render time in markdown math

KaTeX math will now render progressivly and asynchronously. There
are upper limits on the character count of each formula, and on
cumulative render time.

4 files changed, 143 insertions, 23 deletions

diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js
index a68936d79e2..53867b3096b 100644
--- a/app/assets/javascripts/behaviors/markdown/render_math.js
+++ b/app/assets/javascripts/behaviors/markdown/render_math.js
@@ -1,6 +1,5 @@
-import $ from 'jquery';
-import { __ } from '~/locale';
 import flash from '~/flash';
+import { s__, sprintf } from '~/locale';
 
 // Renders math using KaTeX in any element with the
 // `js-render-math` class
@@ -10,21 +9,131 @@ import flash from '~/flash';
 //   <code class="js-render-math"></div>
 //
 
-// Loop over all math elements and render math
-function renderWithKaTeX(elements, katex) {
-  elements.each(function katexElementsLoop() {
-    const mathNode = $('<span></span>');
-    const $this = $(this);
-
-    const display = $this.attr('data-math-style') === 'display';
-    try {
-      katex.render($this.text(), mathNode.get(0), { displayMode: display, throwOnError: false });
-      mathNode.insertAfter($this);
-      $this.remove();
-    } catch (err) {
-      throw err;
+const MAX_MATH_CHARS = 1000;
+const MAX_RENDER_TIME_MS = 2000;
+
+// These messages might be used with inline errors in the future. Keep them around. For now, we will
+// display a single error message using flash().
+
+// const CHAR_LIMIT_EXCEEDED_MSG = sprintf(
+//   s__(
+//     'math|The following math is too long. For performance reasons, math blocks are limited to %{maxChars} characters. Try splitting up this block, or include an image instead.',
+//   ),
+//   { maxChars: MAX_MATH_CHARS },
+// );
+// const RENDER_TIME_EXCEEDED_MSG = s__(
+//   "math|The math in this entry is taking too long to render. Any math below this point won't be shown. Consider splitting it among multiple entries.",
+// );
+
+const RENDER_FLASH_MSG = sprintf(
+  s__(
+    'math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead.',
+  ),
+  { maxChars: MAX_MATH_CHARS },
+);
+
+// Wait for the browser to reflow the layout. Reflowing SVG takes time.
+// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object".
+const waitForReflow = fn => {
+  window.requestAnimationFrame(fn);
+};
+
+/**
+ * Renders math blocks sequentially while protecting against DoS attacks. Math blocks have a maximum character limit of MAX_MATH_CHARS. If rendering math takes longer than MAX_RENDER_TIME_MS, all subsequent math blocks are skipped and an error message is shown.
+ */
+class SafeMathRenderer {
+  /*
+  How this works:
+
+  The performance bottleneck in rendering math is in the browser trying to reflow the generated SVG.
+  During this time, the JS is blocked and the page becomes unresponsive.
+  We want to render math blocks one by one until a certain time is exceeded, after which we stop
+  rendering subsequent math blocks, to protect against DoS. However, browsers do reflowing in an
+  asynchronous task, so we can't time it synchronously.
+
+  SafeMathRenderer essentially does the following:
+  1. Replaces all math blocks with placeholders so that they're not mistakenly rendered twice.
+  2. Places each placeholder element in a queue.
+  3. Renders the element at the head of the queue and waits for reflow.
+  4. After reflow, gets the elapsed time since step 3 and repeats step 3 until the queue is empty.
+   */
+  queue = [];
+  totalMS = 0;
+
+  constructor(elements, katex) {
+    this.elements = elements;
+    this.katex = katex;
+
+    this.renderElement = this.renderElement.bind(this);
+    this.render = this.render.bind(this);
+  }
+
+  renderElement() {
+    if (!this.queue.length) {
+      return;
     }
-  });
+
+    const el = this.queue.shift();
+    const text = el.textContent;
+
+    el.removeAttribute('style');
+
+    if (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS) {
+      if (!this.flashShown) {
+        flash(RENDER_FLASH_MSG);
+        this.flashShown = true;
+      }
+
+      // Show unrendered math code
+      const codeElement = document.createElement('pre');
+      codeElement.className = 'code';
+      codeElement.textContent = el.textContent;
+      el.parentNode.replaceChild(codeElement, el);
+
+      // Render the next math
+      this.renderElement();
+    } else {
+      this.startTime = Date.now();
+
+      try {
+        el.innerHTML = this.katex.renderToString(text, {
+          displayMode: el.getAttribute('data-math-style') === 'display',
+          throwOnError: true,
+          maxSize: 20,
+          maxExpand: 20,
+        });
+      } catch {
+        // Don't show a flash for now because it would override an existing flash message
+        el.textContent = s__('math|There was an error rendering this math block');
+        // el.style.color = '#d00';
+        el.className = 'katex-error';
+      }
+
+      // Give the browser time to reflow the svg
+      waitForReflow(() => {
+        const deltaTime = Date.now() - this.startTime;
+        this.totalMS += deltaTime;
+
+        this.renderElement();
+      });
+    }
+  }
+
+  render() {
+    // Replace math blocks with a placeholder so they aren't rendered twice
+    this.elements.forEach(el => {
+      const placeholder = document.createElement('span');
+      placeholder.style.display = 'none';
+      placeholder.setAttribute('data-math-style', el.getAttribute('data-math-style'));
+      placeholder.textContent = el.textContent;
+      el.parentNode.replaceChild(placeholder, el);
+      this.queue.push(placeholder);
+    });
+
+    // If we wait for the browser thread to settle down a bit, math rendering becomes 5-10x faster
+    // and less prone to timeouts.
+    setTimeout(this.renderElement, 400);
+  }
 }
 
 export default function renderMath($els) {
@@ -34,7 +143,8 @@ export default function renderMath($els) {
     import(/* webpackChunkName: 'katex' */ 'katex/dist/katex.min.css'),
   ])
     .then(([katex]) => {
-      renderWithKaTeX($els, katex);
+      const renderer = new SafeMathRenderer($els.get(), katex);
+      renderer.render();
     })
-    .catch(() => flash(__('An error occurred while rendering KaTeX')));
+    .catch(() => {});
 }
diff --git a/changelogs/unreleased/security-katex-dos-12-1.yml b/changelogs/unreleased/security-katex-dos-12-1.yml
new file mode 100644
index 00000000000..df803a5eafd
--- /dev/null
+++ b/changelogs/unreleased/security-katex-dos-12-1.yml
@@ -0,0 +1,5 @@
+---
+title: Enforce max chars and max render time in markdown math
+merge_request:
+author:
+type: security
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index a5e55585480..d4d2045c49f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -1055,9 +1055,6 @@ msgstr ""
 msgid "An error occurred while parsing recent searches"
 msgstr ""
 
-msgid "An error occurred while rendering KaTeX"
-msgstr ""
-
 msgid "An error occurred while rendering preview broadcast message"
 msgstr ""
 
@@ -12890,6 +12887,12 @@ msgstr ""
 msgid "manual"
 msgstr ""
 
+msgid "math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead."
+msgstr ""
+
+msgid "math|There was an error rendering this math block"
+msgstr ""
+
 msgid "merge request"
 msgid_plural "merge requests"
 msgstr[0] ""
diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb
index 16ad0d456be..776da128a47 100644
--- a/spec/features/markdown/math_spec.rb
+++ b/spec/features/markdown/math_spec.rb
@@ -32,7 +32,9 @@ describe 'Math rendering', :js do
 
     visit project_issue_path(project, issue)
 
-    expect(page).to have_selector('.katex-error', text: "\href{javascript:alert('xss');}{xss}")
-    expect(page).to have_selector('.katex-html a', text: 'Gitlab')
+    page.within '.description > .md' do
+      expect(page).to have_selector('.katex-error')
+      expect(page).to have_selector('.katex-html a', text: 'Gitlab')
+    end
   end
 end