From f1810d659e5faf1f499a1a49f6c64951a2c7837d Mon Sep 17 00:00:00 2001
From: Brett Walker <bwalker@gitlab.com>
Date: Thu, 8 Feb 2024 18:23:34 +0000
Subject: [PATCH] Remove math rendering exception for wikis

Wikis and repositories no longer allow unlimited
math rendering. It is now controlled by the
`math_rendering_limits_enabled` instance and
group setting.

Changelog: fixed
---
 .../behaviors/markdown/render_math.js         |  8 +--
 .../groups/application_controller.rb          |  4 ++
 .../projects/application_controller.rb        |  4 ++
 app/models/namespaces/traversal/linear.rb     | 10 +--
 doc/administration/instance_limits.md         | 10 ++-
 lib/banzai/filter/math_filter.rb              | 22 +++----
 lib/gitlab/gon_helper.rb                      |  6 ++
 spec/features/markdown/math_spec.rb           | 19 ++++--
 spec/lib/banzai/filter/math_filter_spec.rb    | 61 ++++++++++++++++---
 spec/lib/gitlab/gon_helper_spec.rb            | 35 ++++++++++-
 10 files changed, 135 insertions(+), 44 deletions(-)

diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js
index 4cba3eccb45a6..20450b4d6cb89 100644
--- a/app/assets/javascripts/behaviors/markdown/render_math.js
+++ b/app/assets/javascripts/behaviors/markdown/render_math.js
@@ -2,7 +2,6 @@ import { escape } from 'lodash';
 import { spriteIcon } from '~/lib/utils/common_utils';
 import { differenceInMilliseconds } from '~/lib/utils/datetime_utility';
 import { s__, sprintf } from '~/locale';
-import { unrestrictedPages } from './constants';
 
 // Renders math using KaTeX in an element.
 //
@@ -85,15 +84,12 @@ class SafeMathRenderer {
     }
 
     const el = chosenEl || this.queue.shift();
-    const forceRender =
-      Boolean(chosenEl) ||
-      unrestrictedPages.includes(this.pageName) ||
-      !gon.math_rendering_limits_enabled;
+    const forceRender = Boolean(chosenEl) || !gon.math_rendering_limits_enabled;
     const text = el.textContent;
 
     el.removeAttribute('style');
     if (!forceRender && (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS)) {
-      // Show unrendered math code
+      // Show un-rendered math code
       const codeElement = document.createElement('pre');
 
       codeElement.className = 'code';
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index 59343ec8b0823..a581d2778a705 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -13,6 +13,10 @@ class Groups::ApplicationController < ApplicationController
   before_action :set_sorting
   requires_cross_project_access
 
+  before_action do
+    push_namespace_setting(:math_rendering_limits_enabled, @group)
+  end
+
   private
 
   def group
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 4bfee0c9c826c..97d1f097d35ee 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -10,6 +10,10 @@ class Projects::ApplicationController < ApplicationController
   before_action :repository
   layout 'project'
 
+  before_action do
+    push_namespace_setting(:math_rendering_limits_enabled, @project&.parent)
+  end
+
   helper_method :repository, :can_collaborate_with_project?, :user_access
 
   rescue_from Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError do |exception|
diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb
index 5779b777fd7e2..0b408220911a7 100644
--- a/app/models/namespaces/traversal/linear.rb
+++ b/app/models/namespaces/traversal/linear.rb
@@ -5,7 +5,7 @@
 #
 # Namespace is a nested hierarchy of one parent to many children. A search
 # using only the parent-child relationships is a slow operation. This process
-# was previously optimized using Postgresql recursive common table expressions
+# was previously optimized using PostgreSQL recursive common table expressions
 # (CTE) with acceptable performance. However, it lead to slower than possible
 # performance, and resulted in complicated queries that were difficult to make
 # performant.
@@ -31,7 +31,7 @@
 # Note that this search method works so long as the IDs are unique and the
 # traversal path is ordered from root to leaf nodes.
 #
-# We implement this in the database using Postgresql arrays, indexed by a
+# We implement this in the database using PostgreSQL arrays, indexed by a
 # generalized inverted index (gin).
 module Namespaces
   module Traversal
@@ -55,8 +55,8 @@ module Linear
       end
 
       class_methods do
-        # This method looks into a list of namespaces trying to optimise a returned traversal_ids
-        # into a list of shortest prefixes, due to fact that the shortest prefixes include all childrens.
+        # This method looks into a list of namespaces trying to optimize a returned traversal_ids
+        # into a list of shortest prefixes, due to fact that the shortest prefixes include all children.
         # Example:
         # INPUT: [[4909902], [4909902,51065789], [4909902,51065793], [7135830], [15599674, 1], [15599674, 1, 3], [15599674, 2]]
         # RESULT: [[4909902], [7135830], [15599674, 1], [15599674, 2]]
@@ -148,7 +148,7 @@ def ancestor_ids(hierarchy_order: nil)
         hierarchy_order == :desc ? traversal_ids[0..-2] : traversal_ids[0..-2].reverse
       end
 
-      # Returns all ancestors upto but excluding the top.
+      # Returns all ancestors up to but excluding the top.
       # When no top is given, all ancestors are returned.
       # When top is not found, returns all ancestors.
       #
diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md
index 04cc371f2229d..158c1328e8c31 100644
--- a/doc/administration/instance_limits.md
+++ b/doc/administration/instance_limits.md
@@ -1001,6 +1001,7 @@ Set the limit to `0` to disable it.
 
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132939) in GitLab 16.5.
 > - [Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/368009) the 50-node limit from Wiki and repository files.
+> - [Added](https://gitlab.com/gitlab-org/gitlab/-/issues/368009) a group-level setting to allow disabling math rendering limits, and re-enabled by default the math limits for wiki and repository files in GitLab 16.9.
 
 GitLab imposes default limits when rendering math in Markdown fields. These limits provide better security and performance.
 
@@ -1008,9 +1009,6 @@ The limits for issues, merge requests, epics, wikis, and repository files:
 
 - Maximum number of macro expansions: `1000`.
 - Maximum user-specified size in [em](https://en.wikipedia.org/wiki/Em_(typography)): `20`.
-
-The limits for issues, merge requests, and epics:
-
 - Maximum number of nodes rendered: `50`.
 - Maximum number of characters in a math block: `1000`.
 - Maximum rendering time: `2000 ms`.
@@ -1023,6 +1021,12 @@ Use the [GitLab Rails console](operations/rails_console.md#starting-a-rails-cons
 ApplicationSetting.update(math_rendering_limits_enabled: false)
 ```
 
+These limits can also be disabled per-group using the GraphQL or REST API.
+
+If the limits are disabled, math is rendered with mostly no limits in issues, merge requests, epics, wikis, and repository files.
+This means a malicious actor _could_ add math that would cause a DoS when viewing in the browser. You must ensure
+that only people you trust can add content.
+
 ## Wiki limits
 
 - [Wiki page content size limit](wikis/index.md#wiki-page-content-size-limit).
diff --git a/lib/banzai/filter/math_filter.rb b/lib/banzai/filter/math_filter.rb
index 511da4b6ba520..4d03cd13fadf3 100644
--- a/lib/banzai/filter/math_filter.rb
+++ b/lib/banzai/filter/math_filter.rb
@@ -11,6 +11,7 @@ module Filter
     class MathFilter < HTML::Pipeline::Filter
       # Handle the $`...`$ and ```math syntax in this filter.
       # Also add necessary classes any existing math blocks.
+      include ::Gitlab::Utils::StrongMemoize
 
       CSS_MATH   = 'pre[data-canonical-lang="math"] > code'
       XPATH_MATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_MATH).freeze
@@ -88,24 +89,19 @@ def process_math_codeblock
         end
       end
 
-      def settings
-        Gitlab::CurrentSettings.current_application_settings
-      end
-
       def render_nodes_limit_reached?(count)
-        return false if wiki?
-        return false if blob?
-        return false unless settings.math_rendering_limits_enabled?
-
-        count >= RENDER_NODES_LIMIT
+        count >= RENDER_NODES_LIMIT && math_rendering_limits_enabled?
       end
 
-      def wiki?
-        context[:wiki].present?
+      def math_rendering_limits_enabled?
+        return true unless group && group.namespace_settings
+
+        group.namespace_settings.math_rendering_limits_enabled?
       end
+      strong_memoize_attr :math_rendering_limits_enabled?
 
-      def blob?
-        context[:text_source] == :blob
+      def group
+        context[:project]&.parent || context[:group]
       end
     end
   end
diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb
index 3e75ada1f7fe1..cb52ad535d48f 100644
--- a/lib/gitlab/gon_helper.rb
+++ b/lib/gitlab/gon_helper.rb
@@ -111,6 +111,12 @@ def push_force_frontend_feature_flag(name, enabled)
       push_to_gon_attributes(:features, name, !!enabled)
     end
 
+    def push_namespace_setting(key, object)
+      return unless object&.namespace_settings.respond_to?(key)
+
+      gon.push({ key => object.namespace_settings.public_send(key) }) # rubocop:disable GitlabSecurity/PublicSend
+    end
+
     def push_to_gon_attributes(key, name, enabled)
       var_name = name.to_s.camelize(:lower)
       # Here the `true` argument signals gon that the value should be merged
diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb
index 0d12aade80786..1835661b6a3c8 100644
--- a/spec/features/markdown/math_spec.rb
+++ b/spec/features/markdown/math_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe 'Math rendering', :js, feature_category: :team_planning do
-  let!(:project) { create(:project, :public) }
+  let_it_be(:project) { create(:project, :public) }
 
   it 'renders inline and display math correctly' do
     description = <<~MATH
@@ -98,7 +98,7 @@
         end
       end
 
-      it 'renders without any limits on wiki page', :js do
+      it 'renders with limits on wiki page', :js do
         wiki_page = build(:wiki_page, { container: project, content: lazy_load_description })
         wiki_page.create message: 'math test commit' # rubocop:disable Rails/SaveBang
         wiki_page = project.wiki.find_page(wiki_page.slug)
@@ -108,20 +108,26 @@
         wait_for_requests
 
         page.within '.js-wiki-page-content' do
-          expect(page).not_to have_selector('.js-lazy-render-math')
+          # the find is needed to ensure the lazy container is loaded, otherwise
+          # it can be a flaky test, similar to
+          # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25408
+          find('.js-lazy-render-math-container')
+
+          expect(page).to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/)
         end
       end
     end
 
     context 'when limits are disabled' do
-      before do
-        stub_application_setting(math_rendering_limits_enabled: false)
-      end
+      let_it_be(:namespace_settings) { create(:namespace_settings, math_rendering_limits_enabled: false) }
+      let_it_be(:group) { create(:group, namespace_settings: namespace_settings) }
+      let_it_be(:project) { create(:project, :public, group: group) }
 
       it 'does not render lazy load button' do
         create_and_visit_issue_with_description(lazy_load_description)
 
         page.within '.description > .md' do
+          expect(page).not_to have_selector('button', text: 'Display anyway')
           expect(page)
             .not_to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/)
         end
@@ -131,6 +137,7 @@
         create_and_visit_issue_with_description(excessive_expansion_description)
 
         page.within '.description > .md' do
+          expect(page).not_to have_selector('button', text: 'Display anyway')
           expect(page).not_to have_selector('.katex-error', text: /Too many expansions/)
         end
       end
diff --git a/spec/lib/banzai/filter/math_filter_spec.rb b/spec/lib/banzai/filter/math_filter_spec.rb
index 9e9e4110b44d4..57de4bc255d23 100644
--- a/spec/lib/banzai/filter/math_filter_spec.rb
+++ b/spec/lib/banzai/filter/math_filter_spec.rb
@@ -208,7 +208,9 @@
   end
 
   context 'when limiting how many elements can be marked as math' do
-    subject { pipeline_filter('$`2+2`$ + $3+3$ + $$4+4$$') }
+    let_it_be(:context) { {} }
+
+    subject { pipeline_filter('$`2+2`$ + $3+3$ + $$4+4$$', context) }
 
     before do
       stub_const('Banzai::Filter::MathFilter::RENDER_NODES_LIMIT', 2)
@@ -218,22 +220,61 @@
       expect(subject.search('.js-render-math').count).to eq(2)
     end
 
-    it 'does not limit when math_rendering_limits_enabled is false' do
-      stub_application_setting(math_rendering_limits_enabled: false)
+    context 'when project with user namespace (no group)' do
+      let_it_be(:project) { create(:project, :public) }
+      let_it_be(:context) { { project: project } }
+
+      it 'limits' do
+        expect(subject.search('.js-render-math').count).to eq(2)
+      end
+    end
+
+    context 'when project with group, no namespace settings' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:project) { create(:project, :public, group: group) }
+      let_it_be(:context) { { project: project } }
+
+      it 'limits' do
+        expect(subject.search('.js-render-math').count).to eq(2)
+      end
+    end
+
+    context 'when project with group, default namespace settings' do
+      let_it_be(:namespace_settings) { create(:namespace_settings) }
+      let_it_be(:group) { create(:group, namespace_settings: namespace_settings) }
+      let_it_be(:project) { create(:project, :public, group: group) }
+      let_it_be(:context) { { project: project } }
+
+      it 'limits' do
+        expect(subject.search('.js-render-math').count).to eq(2)
+      end
+    end
+
+    context 'when limits math_rendering_limits_enabled is false' do
+      let_it_be(:namespace_settings) { create(:namespace_settings, math_rendering_limits_enabled: false) }
+      let_it_be(:group) { create(:group, namespace_settings: namespace_settings) }
+      let_it_be(:project) { create(:project, :public, group: group) }
+      let_it_be(:context) { { project: project } }
 
-      expect(subject.search('.js-render-math').count).to eq(3)
+      it 'does not limit' do
+        expect(subject.search('.js-render-math').count).to eq(3)
+      end
     end
 
-    it 'does not limit for the wiki' do
-      doc = pipeline_filter('$`2+2`$ + $3+3$ + $$4+4$$', { wiki: true })
+    context 'when for wikis' do
+      let_it_be(:context) { { wiki: true } }
 
-      expect(doc.search('.js-render-math').count).to eq(3)
+      it 'does limit' do
+        expect(subject.search('.js-render-math').count).to eq(2)
+      end
     end
 
-    it 'does not limit for blobs' do
-      doc = pipeline_filter('$`2+2`$ + $3+3$ + $$4+4$$', { text_source: :blob })
+    context 'when for blobs' do
+      let_it_be(:context) { { text_source: :blob } }
 
-      expect(doc.search('.js-render-math').count).to eq(3)
+      it 'does limit for blobs' do
+        expect(subject.search('.js-render-math').count).to eq(2)
+      end
     end
   end
 
diff --git a/spec/lib/gitlab/gon_helper_spec.rb b/spec/lib/gitlab/gon_helper_spec.rb
index ba250c499a31c..068f3cd092ca3 100644
--- a/spec/lib/gitlab/gon_helper_spec.rb
+++ b/spec/lib/gitlab/gon_helper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GonHelper do
+RSpec.describe Gitlab::GonHelper, feature_category: :shared do
   let(:helper) do
     Class.new do
       include Gitlab::GonHelper
@@ -173,6 +173,39 @@
     end
   end
 
+  describe '#push_namespace_setting' do
+    it 'pushes a namespace setting to the frontend' do
+      namespace_settings = create(:namespace_settings, math_rendering_limits_enabled: false)
+      group = create(:group, namespace_settings: namespace_settings)
+
+      gon = class_double('Gon')
+      allow(helper)
+        .to receive(:gon)
+        .and_return(gon)
+
+      expect(gon)
+        .to receive(:push)
+        .with({ math_rendering_limits_enabled: false })
+
+      helper.push_namespace_setting(:math_rendering_limits_enabled, group)
+    end
+
+    it 'does not push if missing namespace setting entry' do
+      group = create(:group)
+
+      gon = class_double('Gon')
+      allow(helper)
+        .to receive(:gon)
+        .and_return(gon)
+
+      expect(gon)
+        .not_to receive(:push)
+        .with({ math_rendering_limits_enabled: false })
+
+      helper.push_namespace_setting(:math_rendering_limits_enabled, group)
+    end
+  end
+
   describe '#default_avatar_url' do
     it 'returns an absolute URL' do
       url = helper.default_avatar_url
-- 
GitLab