diff --git a/app/assets/javascripts/behaviors/markdown/render_gfm.js b/app/assets/javascripts/behaviors/markdown/render_gfm.js index c02950534ff2bf606bc7b9bcdf28e66b7caf7ac8..b95c1b98226e5965a9f97f9c16621cb386c34450 100644 --- a/app/assets/javascripts/behaviors/markdown/render_gfm.js +++ b/app/assets/javascripts/behaviors/markdown/render_gfm.js @@ -3,6 +3,7 @@ import highlightCurrentUser from './highlight_current_user'; import { renderKroki } from './render_kroki'; import renderMath from './render_math'; import renderSandboxedMermaid from './render_sandboxed_mermaid'; +import renderIframe from './render_iframe'; import { renderGlql } from './render_glql'; import { renderJSONTable, renderJSONTableHTML } from './render_json_table'; import { addAriaLabels } from './accessibility'; @@ -17,43 +18,37 @@ function initPopovers(elements) { .catch(() => {}); } -// Render GitLab flavored Markdown +// Render GitLab Flavored Markdown export function renderGFM(element) { if (!element) { return; } - const [ - highlightEls, - krokiEls, - mathEls, - mermaidEls, - tableEls, - tableHTMLEls, - glqlEls, - userEls, - popoverEls, - taskListCheckboxEls, - imageEls, - ] = [ - '.js-syntax-highlight', - '.js-render-kroki[hidden]', - '.js-render-math', - '.js-render-mermaid', - '[data-canonical-lang="json"][data-lang-params~="table"]', - 'table[data-table-fields]', - '[data-canonical-lang="glql"], .language-glql', - '.gfm-project_member', + function arrayFromAll(selector) { + return Array.from(element.querySelectorAll(selector)); + } + + const highlightEls = arrayFromAll('.js-syntax-highlight'); + const krokiEls = arrayFromAll('.js-render-kroki[hidden]'); + const mathEls = arrayFromAll('.js-render-math'); + const mermaidEls = arrayFromAll('.js-render-mermaid'); + const iframeEls = arrayFromAll('.js-render-iframe'); + const tableEls = arrayFromAll('[data-canonical-lang="json"][data-lang-params~="table"]'); + const tableHTMLEls = arrayFromAll('table[data-table-fields]'); + const glqlEls = arrayFromAll('[data-canonical-lang="glql"], .language-glql'); + const userEls = arrayFromAll('.gfm-project_member'); + const popoverEls = arrayFromAll( '.gfm-issue, .gfm-work_item, .gfm-merge_request, .gfm-epic, .gfm-milestone', - '.task-list-item-checkbox', - // eslint-disable-next-line @gitlab/require-i18n-strings - 'a>img', - ].map((selector) => Array.from(element.querySelectorAll(selector))); + ); + const taskListCheckboxEls = arrayFromAll('.task-list-item-checkbox'); + // eslint-disable-next-line @gitlab/require-i18n-strings + const imageEls = arrayFromAll('a>img'); syntaxHighlight(highlightEls); renderKroki(krokiEls); renderMath(mathEls); renderSandboxedMermaid(mermaidEls); + renderIframe(iframeEls); renderJSONTable(tableEls.map((e) => e.parentNode)); renderJSONTableHTML(tableHTMLEls); highlightCurrentUser(userEls); diff --git a/app/assets/javascripts/behaviors/markdown/render_iframe.js b/app/assets/javascripts/behaviors/markdown/render_iframe.js new file mode 100644 index 0000000000000000000000000000000000000000..ef20e5e97ebccd3e5ea75ebba68dfaf153943d05 --- /dev/null +++ b/app/assets/javascripts/behaviors/markdown/render_iframe.js @@ -0,0 +1,65 @@ +import { setAttributes } from '~/lib/utils/dom_utils'; + +// https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/iframe#sandbox +const IFRAME_SANDBOX_RESTRICTIONS = 'allow-scripts allow-popups allow-same-origin'; + +const elsProcessingMap = new WeakMap(); + +function renderIframeEl(el) { + // Obtain src from data-src first, in case image lazy loading hasn't + // resolved this yet. See Banzai::Filter::ImageLazyLoadFilter. + const src = el.dataset.src || el.src; + if (!src) return; + + const srcUrl = new URL(src); + + const allowlist = window.gon?.iframe_rendering_allowlist ?? []; + const allowlistUrls = allowlist.map((domain) => new URL(`https://${domain}`)); + const allowed = allowlistUrls.some((allowlistUrl) => allowlistUrl.origin === srcUrl.origin); + if (!allowed) return; + + const iframeEl = document.createElement('iframe'); + setAttributes(iframeEl, { + src, + sandbox: IFRAME_SANDBOX_RESTRICTIONS, + frameBorder: 0, + class: 'gl-inset-0 gl-h-full gl-w-full', + allowfullscreen: 'true', + referrerpolicy: 'strict-origin-when-cross-origin', + }); + + // We propagate these attributes, but currently the gl-h-full/gl-w-full above override them, + // as they can easily overrun the container and break the layout. + // For potential later use with some frontend design help. + if (el.getAttribute('width')) { + iframeEl.setAttribute('width', el.getAttribute('width')); + } + if (el.getAttribute('height')) { + iframeEl.setAttribute('height', el.getAttribute('height')); + } + + const wrapper = document.createElement('div'); + wrapper.appendChild(iframeEl); + + const container = el.closest('.media-container'); + container.replaceChildren(wrapper); +} + +export default function renderIframes(els) { + if (!window.gon?.iframe_rendering_enabled) return; + if (!window.gon?.features.allowIframesInMarkdown) return; + + if (!els.length) return; + + els.forEach((el) => { + if (elsProcessingMap.has(el)) { + return; + } + + const requestId = window.requestIdleCallback(() => { + renderIframeEl(el); + }); + + elsProcessingMap.set(el, requestId); + }); +} diff --git a/app/assets/javascripts/pages/admin/application_settings/general/index.js b/app/assets/javascripts/pages/admin/application_settings/general/index.js index c33a7237805da365ce4ef1b76b7f56a480aac631..2299ba1d3ac41967e33fa04be32d04d08c31fa11 100644 --- a/app/assets/javascripts/pages/admin/application_settings/general/index.js +++ b/app/assets/javascripts/pages/admin/application_settings/general/index.js @@ -5,6 +5,7 @@ import { initAdminDeletionProtectionSettings } from '~/admin/application_setting import initAccountAndLimitsSection from '../account_and_limits'; import initGitpod from '../gitpod'; import initSignupRestrictions from '../signup_restrictions'; +import initIframeSettings from '../iframe'; (() => { initAccountAndLimitsSection(); @@ -12,6 +13,7 @@ import initSignupRestrictions from '../signup_restrictions'; initSignupRestrictions(); initSilentModeSettings(); initAdminDeletionProtectionSettings(); + initIframeSettings(); initSimpleApp('#js-extension-marketplace-settings-app', VscodeExtensionMarketplaceSettings); })(); diff --git a/app/assets/javascripts/pages/admin/application_settings/iframe.js b/app/assets/javascripts/pages/admin/application_settings/iframe.js new file mode 100644 index 0000000000000000000000000000000000000000..ad09ded5a372b2fa57d74669c166a567c3015953 --- /dev/null +++ b/app/assets/javascripts/pages/admin/application_settings/iframe.js @@ -0,0 +1,22 @@ +function toggleIframeAllowlist() { + const checkbox = document.getElementById('application_setting_iframe_rendering_enabled'); + const allowlistGroup = document.getElementById('iframe-allowlist-group'); + + if (!checkbox || !allowlistGroup) return; + + function updateVisibility() { + if (checkbox.checked) { + allowlistGroup.style.display = ''; + } else { + allowlistGroup.style.display = 'none'; + } + } + + updateVisibility(); + + checkbox.addEventListener('change', updateVisibility); +} + +export default function initIframeSettings() { + toggleIframeAllowlist(); +} diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a20f6ce8124376191c3b8de394e35d4de1c4087a..cf4b235c6ecac5168aa89cd42bc16001281dc64d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -31,6 +31,16 @@ class ApplicationController < BaseActionController include Gitlab::HttpRouter::RuleMetrics include CookiesHelper + content_security_policy do |p| + next if p.directives.blank? + next unless Gitlab::CurrentSettings.try(:iframe_rendering_enabled?) + + add_frame_srcs = Gitlab::CurrentSettings.iframe_rendering_allowlist.map { |domain| "https://#{domain}" } + + append_to_content_security_policy(p, 'frame-src', add_frame_srcs) + append_to_content_security_policy(p, 'child-src', add_frame_srcs) + end + around_action :set_current_ip_address before_action :authenticate_user!, except: [:route_not_found] diff --git a/app/controllers/base_action_controller.rb b/app/controllers/base_action_controller.rb index 7f46a153f4252617681207d20c08945d522da205..f3ef206c49dcc17a14b0cb89813c06626f28721a 100644 --- a/app/controllers/base_action_controller.rb +++ b/app/controllers/base_action_controller.rb @@ -27,9 +27,13 @@ class BaseActionController < ActionController::Base next if p.directives.blank? next unless Gitlab::CurrentSettings.snowplow_enabled? && !Gitlab::CurrentSettings.snowplow_collector_hostname.blank? - default_connect_src = p.directives['connect-src'] || p.directives['default-src'] - connect_src_values = Array.wrap(default_connect_src) | [Gitlab::CurrentSettings.snowplow_collector_hostname] - p.connect_src(*connect_src_values) + append_to_content_security_policy(p, 'connect-src', [Gitlab::CurrentSettings.snowplow_collector_hostname]) + end + + def append_to_content_security_policy(policy, directive, values) + existing_value = policy.directives[directive] || policy.directives['default-src'] + new_value = Array.wrap(existing_value) | values + policy.directives[directive] = new_value end end # rubocop:enable Gitlab/NamespacedClass diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 03fa486055bf4087329cb6ffc56d3f7d57690ffb..489d2db9a02b3a00ad5dee8d43ec65bb22e00d12 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -365,6 +365,9 @@ def visible_attributes :housekeeping_incremental_repack_period, :housekeeping_optimize_repository_period, :html_emails_enabled, + :iframe_rendering_enabled, + :iframe_rendering_allowlist, + :iframe_rendering_allowlist_raw, :import_sources, :inactive_resource_access_tokens_delete_after_days, :inactive_projects_delete_after_months, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 0d0b1889808ff1cdd2d6c9b52d26d48994a19a51..6db6b93ca8c519ead639de1cca8c726910fe0afd 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -88,6 +88,7 @@ def self.kroki_formats_attributes serialize :disabled_oauth_sign_in_sources, type: Array # rubocop:disable Cop/ActiveRecordSerialize serialize :domain_allowlist, type: Array # rubocop:disable Cop/ActiveRecordSerialize serialize :domain_denylist, type: Array # rubocop:disable Cop/ActiveRecordSerialize + serialize :iframe_rendering_allowlist, type: Array # rubocop:disable Cop/ActiveRecordSerialize # See https://gitlab.com/gitlab-org/gitlab/-/issues/300916 serialize :asset_proxy_allowlist, type: Array # rubocop:disable Cop/ActiveRecordSerialize @@ -1025,6 +1026,21 @@ def self.kroki_formats_attributes validates :math_rendering_limits_enabled, inclusion: { in: [true, false], message: N_('must be a boolean value') } + validates :iframe_rendering_enabled, + inclusion: { in: [true, false], message: N_('must be a boolean value') } + + validates_each :iframe_rendering_allowlist, on: :update do |record, attr, value| + # Leading "http://" or "https://" and trailing "/" are removed in + # ApplicationSettingImplementation#coerce_iframe_rendering_allowlist. + # Normalising the values in here is crucial, since we rely on it to + # correctly (securely) check iframe src attributes and construct our frame-src CSP. + value.each do |entry| + unless %r{\A[a-zA-Z0-9.-]+(?::\d+)?\z}.match?(entry) + record.errors.add(attr, format(_("'%{entry}' is not a valid domain name"), entry:)) + end + end + end + validates :require_admin_two_factor_authentication, inclusion: { in: [true, false], message: N_('must be a boolean value') } @@ -1058,6 +1074,7 @@ def self.kroki_formats_attributes before_validation :ensure_uuid! before_validation :coerce_repository_storages_weighted, if: :repository_storages_weighted_changed? before_validation :normalize_default_branch_name + before_validation :coerce_iframe_rendering_allowlist, if: :iframe_rendering_allowlist_changed? before_save :ensure_runners_registration_token before_save :ensure_health_check_access_token diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index ccd5d153af689dc90bc2d947a5459c003028d96f..d52c7650aac72d46456bafa5ecb0fc880d7fb97e 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -506,6 +506,24 @@ def search_rate_limit_allowlist_raw=(values) self.search_rate_limit_allowlist = strings_to_array(values).map(&:downcase) end + def coerce_iframe_rendering_allowlist + self.iframe_rendering_allowlist = iframe_rendering_allowlist.map do |entry| + # We mandate https://, and always add a trailing slash; we expect the configured + # list has neither present, so we remove them if present. + entry + .sub(%r{\Ahttps?://}i, '') + .sub(%r{/+\z}, '') + end.sort + end + + def iframe_rendering_allowlist_raw + array_to_string(iframe_rendering_allowlist) + end + + def iframe_rendering_allowlist_raw=(values) + self.iframe_rendering_allowlist = strings_to_array(values) + end + def asset_proxy_whitelist=(values) values = strings_to_array(values) if values.is_a?(String) diff --git a/app/views/admin/application_settings/_iframe.html.haml b/app/views/admin/application_settings/_iframe.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..7892ea213471923c3bc7991e6da1a8e7c8d28b36 --- /dev/null +++ b/app/views/admin/application_settings/_iframe.html.haml @@ -0,0 +1,23 @@ +- expanded = integration_expanded?('iframe_') + += render ::Layouts::SettingsBlockComponent.new(_('Embedded content'), + id: 'js-iframe-settings', + testid: 'admin-iframe-settings', + expanded: expanded) do |c| + - c.with_description do + = _('Allow embedding of external videos and designs in Markdown.') + - c.with_body do + = gitlab_ui_form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-iframe-settings'), html: { class: 'fieldset-form', id: 'iframe-settings' } do |f| + = form_errors(@application_setting) if expanded + + %fieldset + .form-group + = f.gitlab_ui_checkbox_component :iframe_rendering_enabled, _('Enable embedded content'), help_text: _('When enabled, users can embed YouTube videos, Figma designs, and other external content.') + + .form-group{ id: 'iframe-allowlist-group', style: @application_setting.iframe_rendering_enabled? ? '' : 'display: none;' } + = f.label :iframe_rendering_allowlist_raw, _('Allowed domains'), class: 'label-bold' + = f.text_area :iframe_rendering_allowlist_raw, class: 'form-control gl-form-input', rows: 8 + .form-text.gl-text-subtle + = _('Enter trusted domains that users can embed content from. One domain per line.') + + = f.submit _('Save changes'), pajamas_button: true diff --git a/app/views/admin/application_settings/_kroki.html.haml b/app/views/admin/application_settings/_kroki.html.haml index 7fbd2ede999f6bd43af596d5d91db41010854db7..376b900e1052101e01f3f31be39385fe03dfb100 100644 --- a/app/views/admin/application_settings/_kroki.html.haml +++ b/app/views/admin/application_settings/_kroki.html.haml @@ -15,12 +15,10 @@ = f.gitlab_ui_checkbox_component :kroki_enabled, _('Enable Kroki') .form-group - = f.label :kroki_url, 'Kroki URL', class: 'label-bold' - = f.text_field :kroki_url, class: 'form-control gl-form-input', placeholder: 'http://your-kroki-instance:8000' + = f.label :kroki_url, _('Kroki URL'), class: 'label-bold' + = f.text_field :kroki_url, class: 'form-control gl-form-input' .form-text.gl-text-subtle - - install_link_url = 'https://docs.kroki.io/kroki/setup/install/' - - install_link_start = ''.html_safe % { url: install_link_url } - = html_escape(_('Use the public cloud instance URL (%{kroki_public_url}) or %{install_link_start}install Kroki%{install_link_end} on your own infrastructure and use your own instance URL.')) % { kroki_public_url: 'https://kroki.io'.html_safe, install_link_start: install_link_start, install_link_end: ''.html_safe } + = _('The Kroki server that will be used to generate diagrams.') .form-group = f.label :kroki_formats, _('Additional diagram formats'), class: 'label-bold' .form-text.gl-text-subtle diff --git a/app/views/admin/application_settings/general.html.haml b/app/views/admin/application_settings/general.html.haml index 1daa21c126df4bfc9654939a5f679927ee0777df..647ad3391590835f100fe27415909fbc0cc0bf0c 100644 --- a/app/views/admin/application_settings/general.html.haml +++ b/app/views/admin/application_settings/general.html.haml @@ -88,10 +88,11 @@ = render_if_exists 'admin/application_settings/maintenance_mode_settings_form' = render 'admin/application_settings/silent_mode_settings_form' = render 'admin/application_settings/gitpod' -= render 'admin/application_settings/kroki' = render 'admin/application_settings/mailgun' += render 'admin/application_settings/kroki' = render 'admin/application_settings/plantuml' = render 'admin/application_settings/diagramsnet' += render 'admin/application_settings/iframe' = render 'admin/application_settings/sourcegraph' -# this partial is from JiHu, see details in https://jihulab.com/gitlab-cn/gitlab/-/merge_requests/417 = render_if_exists 'admin/application_settings/dingtalk_integration' diff --git a/config/application_setting_columns/iframe_rendering_allowlist.yml b/config/application_setting_columns/iframe_rendering_allowlist.yml new file mode 100644 index 0000000000000000000000000000000000000000..c2b1e2a1aa4631643dee91c5942075413e05abe5 --- /dev/null +++ b/config/application_setting_columns/iframe_rendering_allowlist.yml @@ -0,0 +1,13 @@ +--- +api_type: array of strings +attr: iframe_rendering_allowlist +clusterwide: true +column: iframe_rendering_allowlist +db_type: text +default: +description: List of allowed iframe `src` host[:port] entries used for Content Security + Policy and sanitization. +encrypted: false +gitlab_com_different_than_default: false +jihu: false +not_null: false diff --git a/config/application_setting_columns/iframe_rendering_enabled.yml b/config/application_setting_columns/iframe_rendering_enabled.yml new file mode 100644 index 0000000000000000000000000000000000000000..cca9058240bc593b5616a0ee3e0d273d7aeef0a4 --- /dev/null +++ b/config/application_setting_columns/iframe_rendering_enabled.yml @@ -0,0 +1,12 @@ +--- +api_type: boolean +attr: iframe_rendering_enabled +clusterwide: true +column: iframe_rendering_enabled +db_type: boolean +default: 'false' +description: Allow rendering of iframes in Markdown. Disabled by default. +encrypted: false +gitlab_com_different_than_default: false +jihu: false +not_null: true diff --git a/db/migrate/20251125011943_add_iframe_allowlist_to_application_settings.rb b/db/migrate/20251125011943_add_iframe_allowlist_to_application_settings.rb new file mode 100644 index 0000000000000000000000000000000000000000..893e45e153b463beb4430a35170508082827402a --- /dev/null +++ b/db/migrate/20251125011943_add_iframe_allowlist_to_application_settings.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddIframeAllowlistToApplicationSettings < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + milestone '18.7' + + def up + with_lock_retries do + add_column :application_settings, :iframe_rendering_enabled, :boolean, default: false, null: false, + if_not_exists: true + add_column :application_settings, :iframe_rendering_allowlist, :text, if_not_exists: true + end + + # Limit size of the allowlist text column for safety + add_text_limit :application_settings, :iframe_rendering_allowlist, 5000 + end + + def down + with_lock_retries do + remove_column :application_settings, :iframe_rendering_allowlist, if_exists: true + remove_column :application_settings, :iframe_rendering_enabled, if_exists: true + end + end +end diff --git a/db/schema_migrations/20251125011943 b/db/schema_migrations/20251125011943 new file mode 100644 index 0000000000000000000000000000000000000000..29bbb56b819dfc6a3b9c92c510a97f61df866db5 --- /dev/null +++ b/db/schema_migrations/20251125011943 @@ -0,0 +1 @@ +985069c3755868f3daf5d97b07a5013997f721efc8df935f7ee9d5fb743457a3 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 853309fdc7eaf2f47a225ffa4c9ba493b29d32d0..15599dae0c33a7536f559bfcc165d85eacba757a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12146,6 +12146,8 @@ CREATE TABLE application_settings ( lock_duo_foundational_flows_enabled boolean DEFAULT false NOT NULL, duo_sast_fp_detection_enabled boolean DEFAULT true NOT NULL, lock_duo_sast_fp_detection_enabled boolean DEFAULT false NOT NULL, + iframe_rendering_enabled boolean DEFAULT false NOT NULL, + iframe_rendering_allowlist text, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), @@ -12188,6 +12190,7 @@ CREATE TABLE application_settings ( CONSTRAINT check_85a39b68ff CHECK ((char_length(encrypted_ci_jwt_signing_key_iv) <= 255)), CONSTRAINT check_8dca35398a CHECK ((char_length(public_runner_releases_url) <= 255)), CONSTRAINT check_8e7df605a1 CHECK ((char_length(cube_api_base_url) <= 512)), + CONSTRAINT check_987903e806 CHECK ((char_length(iframe_rendering_allowlist) <= 5000)), CONSTRAINT check_9a719834eb CHECK ((char_length(secret_detection_token_revocation_url) <= 255)), CONSTRAINT check_9c6c447a13 CHECK ((char_length(maintenance_mode_message) <= 255)), CONSTRAINT check_a5704163cc CHECK ((char_length(secret_detection_revocation_token_types_url) <= 255)), diff --git a/doc/api/settings.md b/doc/api/settings.md index cb87ac9bbc97293b827b4f886a64c6d86280a786..f0200692e4eaf4dece74996a45935f8146b84177 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -440,6 +440,7 @@ This heading is referenced by a script: `scripts/cells/application-settings-anal - `require_personal_access_token_expiry` [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/470192) in GitLab 17.3. - `receptive_cluster_agents_enabled` [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/463427) in GitLab 17.4. - `allow_all_integrations` and `allowed_integrations` [added](https://gitlab.com/gitlab-org/gitlab/-/issues/500610) in GitLab 17.6. +- `iframe_rendering_enabled`, `iframe_rendering_allowlist`, and `iframe_rendering_allowlist_raw` introduced in GitLab 18.6. {{< /history >}} @@ -835,6 +836,9 @@ to configure other related settings. These requirements are | `secret_push_protection_available` | boolean | no | Allow projects to enable secret push protection. This does not enable secret push protection. Ultimate only. | | `disable_invite_members` | boolean | no | Disable invite members functionality for group. | | `enforce_pipl_compliance` | boolean | no | Sets whether pipl compliance is enforced for the saas application or not | +| `iframe_rendering_enabled` | boolean | no | Allow rendering of iframes in Markdown. Disabled by default. | +| `iframe_rendering_allowlist` | array of strings | no | List of allowed iframe `src` host[:port] entries used for Content Security Policy and sanitization. | +| `iframe_rendering_allowlist_raw` | string | no | Raw newline- or comma-separated list of allowed iframe `src` host[:port] entries. | ### Dormant project settings diff --git a/doc/development/cells/application_settings_analysis.md b/doc/development/cells/application_settings_analysis.md index 5414b9cfeb9cca574bdd00b7c77b47dcff54e442..e2c7643c11847dde5c749008f6dc4f3023ed0f7b 100644 --- a/doc/development/cells/application_settings_analysis.md +++ b/doc/development/cells/application_settings_analysis.md @@ -14,12 +14,12 @@ title: Application Settings analysis ## Statistics -- Number of attributes: 506 +- Number of attributes: 508 - Number of encrypted attributes: 42 (8.0%) -- Number of attributes documented: 295 (57.99999999999999%) +- Number of attributes documented: 297 (57.99999999999999%) - Number of attributes on GitLab.com different from the defaults: 224 (44.0%) -- Number of attributes with `clusterwide` set: 506 (100.0%) -- Number of attributes with `clusterwide: true` set: 133 (26.0%) +- Number of attributes with `clusterwide` set: 508 (100.0%) +- Number of attributes with `clusterwide: true` set: 135 (27.0%) ## Individual columns @@ -237,6 +237,8 @@ title: Application Settings analysis | `html_emails_enabled` | `false` | `boolean` | `boolean` | `false` | `true` | `false` | `false`| `true` | | `id` | `false` | `bigint` | `` | `true` | `???` | `false` | `false`| `false` | | `identity_verification_settings` | `false` | `jsonb` | `` | `true` | `'{}'::jsonb` | `true` | `true`| `false` | +| `iframe_rendering_allowlist` | `false` | `text` | `array of strings` | `false` | `null` | `false` | `true`| `true` | +| `iframe_rendering_enabled` | `false` | `boolean` | `boolean` | `true` | `false` | `false` | `true`| `true` | | `import_sources` | `false` | `text` | `array of strings` | `false` | `null` | `true` | `true`| `true` | | `importers` | `false` | `jsonb` | `` | `true` | `'{}'::jsonb` | `true` | `true`| `false` | | `inactive_projects_delete_after_months` | `false` | `integer` | `` | `true` | `2` | `false` | `false`| `false` | diff --git a/doc/user/markdown.md b/doc/user/markdown.md index 5eb0a117c7d9cc874ba7b0b903d5c305aa764d78..7ae85f8127138096290608a38a9391daaeb1b701 100644 --- a/doc/user/markdown.md +++ b/doc/user/markdown.md @@ -1038,6 +1038,10 @@ Watch the following video walkthrough of this feature: +{{< alert type="note" >}} +Administrators can enable rendering of iframes in Markdown and configure the allowed iframe `src` hosts at the instance level. You can manage these settings via the [Application settings API](../api/settings.md#available-settings): `iframe_rendering_enabled`, `iframe_rendering_allowlist`, and `iframe_rendering_allowlist_raw`. +{{< /alert >}} + The `items` attribute is a list of objects representing the data points. ````markdown diff --git a/lib/api/entities/application_setting.rb b/lib/api/entities/application_setting.rb index 68052f5aeb92a05d0b4c5d71d20ce8341397b268..96981b4cc4c353df772a500f4264b661a08ef811 100644 --- a/lib/api/entities/application_setting.rb +++ b/lib/api/entities/application_setting.rb @@ -42,6 +42,9 @@ def self.exposed_attributes expose :password_authentication_enabled_for_web, as: :signin_enabled expose :allow_local_requests_from_web_hooks_and_services, as: :allow_local_requests_from_hooks_and_services expose :asset_proxy_allowlist, as: :asset_proxy_whitelist + expose :iframe_rendering_enabled + expose :iframe_rendering_allowlist + expose :iframe_rendering_allowlist_raw # This field is deprecated and always returns true expose(:housekeeping_bitmaps_enabled) { |_settings, _options| true } diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 4a337e81e68f8901b9ee9f2f4d75e66b98fabe44..d62c41b6611f44ace5b48e8e4c71503699f8d5bc 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -71,6 +71,9 @@ def filter_attributes_using_license(attrs) optional :domain_denylist_enabled, type: Boolean, desc: 'Enable domain denylist for sign ups' optional :domain_denylist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Enter multiple entries on separate lines. Ex: domain.com, *.domain.com' optional :domain_allowlist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'ONLY users with e-mail addresses that match these domain(s) will be able to sign-up. Wildcards allowed. Enter multiple entries on separate lines. Ex: domain.com, *.domain.com' + optional :iframe_rendering_enabled, type: Boolean, desc: 'Allow rendering of iframes in Markdown.' + optional :iframe_rendering_allowlist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Allowed iframe src host[:port] entries. Enter multiple entries separated by commas or on separate lines.' + optional :iframe_rendering_allowlist_raw, type: String, desc: 'Raw newline- or comma-separated list of allowed iframe src host[:port] entries.' optional :eks_integration_enabled, type: Boolean, desc: 'Enable integration with Amazon EKS' given eks_integration_enabled: ->(val) { val } do requires :eks_account_id, type: String, desc: 'Amazon account ID for EKS integration' diff --git a/lib/banzai/filter/iframe_link_filter.rb b/lib/banzai/filter/iframe_link_filter.rb index 5c355c5b502d6de0a5d060f1eb39d79f46ec46c3..1646114046c9405a2aa37cc6c4cba2c6042f6e2c 100644 --- a/lib/banzai/filter/iframe_link_filter.rb +++ b/lib/banzai/filter/iframe_link_filter.rb @@ -12,6 +12,16 @@ module Banzai module Filter class IframeLinkFilter < PlayableLinkFilter extend ::Gitlab::Utils::Override + include ::Gitlab::Utils::StrongMemoize + + def call + return doc unless Gitlab::CurrentSettings.iframe_rendering_enabled? + + return doc unless context[:project]&.allow_iframes_in_markdown_feature_flag_enabled? || + context[:group]&.allow_iframes_in_markdown_feature_flag_enabled? + + super + end private @@ -20,21 +30,19 @@ def media_type end def safe_media_ext - # TODO: will change to use the administrator defined allow list - # Gitlab::CurrentSettings.iframe_src_allowlist - ['www.youtube.com/embed'] + Gitlab::CurrentSettings.iframe_rendering_allowlist.map do |domain| + Addressable::URI.parse("https://#{domain}") + end end + strong_memoize_attr :safe_media_ext override :has_allowed_media? def has_allowed_media?(element) - return unless context[:project]&.allow_iframes_in_markdown_feature_flag_enabled? || - context[:group]&.allow_iframes_in_markdown_feature_flag_enabled? - src = element.attr('data-canonical-src').presence || element.attr('src') - return unless src.present? - src.start_with?('https://') && safe_media_ext.any? { |domain| src.start_with?("https://#{domain}") } + uri = Addressable::URI.parse(src) + safe_media_ext.any? { |allowed_uri| allowed_uri.origin == uri.origin } end def extra_element_attrs(element) diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb index a49c63c6b6ac96b625d1d4bd53dd947182528205..b17d0b06a06afed1852ccb2e97004d7d63ceb3a5 100644 --- a/lib/gitlab/gon_helper.rb +++ b/lib/gitlab/gon_helper.rb @@ -22,6 +22,8 @@ def add_gon_variables gon.math_rendering_limits_enabled = Gitlab::CurrentSettings.math_rendering_limits_enabled gon.allow_immediate_namespaces_deletion = Gitlab::CurrentSettings.allow_immediate_namespaces_deletion_for_user?(current_user) + gon.iframe_rendering_enabled = Gitlab::CurrentSettings.iframe_rendering_enabled? + gon.iframe_rendering_allowlist = Gitlab::CurrentSettings.iframe_rendering_allowlist # Sentry configurations for the browser client are done # via `Gitlab::CurrentSettings` from the Admin panel: @@ -106,6 +108,7 @@ def add_gon_feature_flags push_frontend_feature_flag(:paneled_view, current_user) push_frontend_feature_flag(:archive_group) push_frontend_feature_flag(:accessible_loading_button, current_user) + push_frontend_feature_flag(:allow_iframes_in_markdown, current_user) # Expose the Project Studio user preference as if it were a feature flag push_force_frontend_feature_flag(:project_studio_enabled, Users::ProjectStudio.new(current_user).enabled?) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5909e0cd20e57fe22e6d5fc4ae9302c5b771c687..c19cbe6947f213735e7a5c711784c7fa66ffdb3b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1686,6 +1686,9 @@ msgstr "" msgid "%{wildcards_link_start}Wildcards%{wildcards_link_end} such as %{code_tag_start}v*%{code_tag_end} or %{code_tag_start}*-release%{code_tag_end} are supported." msgstr "" +msgid "'%{entry}' is not a valid domain name" +msgstr "" + msgid "'%{group_name}' has been scheduled for deletion and will be deleted on %{date}." msgstr "" @@ -7594,6 +7597,9 @@ msgstr "" msgid "Allow commits from members who can merge to the target branch. %{link_start}About this feature.%{link_end}" msgstr "" +msgid "Allow embedding of external videos and designs in Markdown." +msgstr "" + msgid "Allow group and project access tokens" msgstr "" @@ -7669,6 +7675,9 @@ msgstr "" msgid "Allowed" msgstr "" +msgid "Allowed domains" +msgstr "" + msgid "Allowed email domain restriction only permitted for top-level groups" msgstr "" @@ -26370,6 +26379,9 @@ msgstr "" msgid "Embed" msgstr "" +msgid "Embedded content" +msgstr "" + msgid "Embedded list view" msgstr "" @@ -26499,6 +26511,9 @@ msgstr "" msgid "Enable email notification" msgstr "" +msgid "Enable embedded content" +msgstr "" + msgid "Enable feature to choose access level" msgstr "" @@ -26775,6 +26790,9 @@ msgstr "" msgid "Enter the username for password-protected Elasticsearch or OpenSearch servers." msgstr "" +msgid "Enter trusted domains that users can embed content from. One domain per line." +msgstr "" + msgid "Enter values to populate the .gitlab-ci.yml configuration file." msgstr "" @@ -38416,6 +38434,9 @@ msgstr "" msgid "Kroki" msgstr "" +msgid "Kroki URL" +msgstr "" + msgid "Kubernetes" msgstr "" @@ -67094,6 +67115,9 @@ msgstr "" msgid "The Issue Tracker is the place to add things that need to be improved or solved in a project. You can register or sign in to create issues for this project." msgstr "" +msgid "The Kroki server that will be used to generate diagrams." +msgstr "" + msgid "The Matrix access token (for example, `syt-zyx57W2v1u123ew11`)." msgstr "" @@ -72023,9 +72047,6 @@ msgstr "" msgid "Use the link below to confirm your email address." msgstr "" -msgid "Use the public cloud instance URL (%{kroki_public_url}) or %{install_link_start}install Kroki%{install_link_end} on your own infrastructure and use your own instance URL." -msgstr "" - msgid "Use the search bar on the top of this page" msgstr "" @@ -75334,6 +75355,9 @@ msgstr "" msgid "When enabled, job logs are collected by Datadog and displayed along with pipeline execution traces." msgstr "" +msgid "When enabled, users can embed YouTube videos, Figma designs, and other external content." +msgstr "" + msgid "When left blank, default value of %{max_lifetime_in_days} is applied. When set, value must be %{max_lifetime_in_days} or less. When changed, existing access tokens with an expiration date beyond the maximum allowable lifetime are revoked." msgstr "" diff --git a/spec/features/markdown/iframe_spec.rb b/spec/features/markdown/iframe_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7d3635afb4b4cd694a83e7d25799dc167a89b239 --- /dev/null +++ b/spec/features/markdown/iframe_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'iframe rendering', :js, feature_category: :markdown do + let_it_be(:project) { create(:project, :public, :repository) } + + # No registration of .example domains is possible: + # https://en.wikipedia.org/wiki/.example + let(:markdown) do + <<~MARKDOWN + ![](https://iframe.example/some-video) + MARKDOWN + end + + let(:expected_selector) do + 'iframe[src="https://iframe.example/some-video"][sandbox]' + end + + let(:untouched_selector) do + 'img[data-src="https://iframe.example/some-video"], + img[src="https://iframe.example/some-video"]' + end + + context 'when feature is enabled and configured' do + before do + stub_feature_flags(allow_iframes_in_markdown: true) + stub_application_setting(iframe_rendering_enabled: true, iframe_rendering_allowlist: ['iframe.example']) + end + + context 'in an issue' do + let(:issue) { create(:issue, project: project, description: markdown) } + + it 'includes iframe embed correctly' do + visit project_issue_path(project, issue) + wait_for_requests + + expect(page).to have_css(expected_selector) + end + end + + context 'in a merge request' do + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, description: markdown) } + + it 'renders diffs and includes iframe correctly' do + visit(diffs_project_merge_request_path(project, merge_request)) + + wait_for_requests + + page.within('.tab-content') do + expect(page).to have_selector('.diffs') + end + + visit(project_merge_request_path(project, merge_request)) + + wait_for_requests + + page.within('.merge-request') do + expect(page).to have_css(expected_selector) + end + end + end + + context 'in a project milestone' do + let(:milestone) { create(:project_milestone, project: project, description: markdown) } + + it 'includes iframe correctly' do + visit(project_milestone_path(project, milestone)) + + wait_for_requests + + expect(page).to have_css(expected_selector) + end + end + + context 'in a project home page' do + let!(:wiki) { create(:project_wiki, project: project) } + let!(:wiki_page) { create(:wiki_page, wiki: wiki, title: 'home', content: markdown) } + + before do + project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) + end + + it 'includes iframe correctly' do + visit(project_path(project)) + + wait_for_all_requests + + page.within '.js-wiki-content' do + expect(page).to have_css(expected_selector) + end + end + end + + context 'in a group milestone' do + let(:group_milestone) { create(:group_milestone, description: markdown) } + + it 'includes iframe correctly' do + visit(group_milestone_path(group_milestone.group, group_milestone)) + + wait_for_requests + + expect(page).to have_css(expected_selector) + end + end + end + + context 'when feature is enabled but not configured' do + before do + stub_feature_flags(allow_iframes_in_markdown: true) + stub_application_setting(iframe_rendering_enabled: false) + end + + context 'in an issue' do + let(:issue) { create(:issue, project: project, description: markdown) } + + it 'no iframe is added, the image tag is left untouched' do + visit project_issue_path(project, issue) + wait_for_requests + + expect(page).not_to have_css(expected_selector) + # The image may be considered invisible because of the invalid target; + # the main thing is it's there, and it's not an iframe. + expect(page).to have_css(untouched_selector, visible: :all) + end + end + end +end diff --git a/spec/lib/banzai/filter/iframe_link_filter_spec.rb b/spec/lib/banzai/filter/iframe_link_filter_spec.rb index cb3a1f198e527d24e8a4af582e37eb3fc094be20..cad27ed2293306ececb67a2ccbcf972944a580e0 100644 --- a/spec/lib/banzai/filter/iframe_link_filter_spec.rb +++ b/spec/lib/banzai/filter/iframe_link_filter_spec.rb @@ -10,17 +10,23 @@ def filter(doc, contexts = {}) end def link_to_image(path, height = nil, width = nil) - return '' if path.nil? + img = Nokogiri::HTML.fragment("").css('img').first + return img.to_html if path.nil? - attrs = %(src="#{path}") - attrs += %( width="#{width}") if width - attrs += %( height="#{height}") if height - - %() + img["src"] = path + img["width"] = width if width + img["height"] = height if height + img.to_html end let_it_be(:project) { create(:project, :repository) } + before do + allow(Gitlab::CurrentSettings).to receive_messages( + iframe_rendering_enabled?: true, + iframe_rendering_allowlist: ["www.youtube.com"]) + end + shared_examples 'an iframe element' do let(:image) { link_to_image(src, height, width) } @@ -138,5 +144,7 @@ def link_to_image(path, height = nil, width = nil) it_behaves_like 'an unchanged element' end - it_behaves_like 'pipeline timing check' + it_behaves_like 'pipeline timing check' do + let(:context) { { project: } } + end end diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 57971d29256be5f7d9a1526b152d9b601a99c2d0..cd936f04d951647e22fdbdeff627215b133eaa55 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -105,12 +105,55 @@ expect(json_response['require_personal_access_token_expiry']).to eq(true) expect(json_response['organization_cluster_agent_authorization_enabled']).to eq(false) expect(json_response['terraform_state_encryption_enabled']).to eq(true) + expect(json_response['iframe_rendering_enabled']).to be(false) + expect(json_response['iframe_rendering_allowlist']).to eq([]) end end describe "PUT /application/settings" do let(:group) { create(:group) } + context 'iframe in markdown settings' do + it 'updates iframe_rendering_enabled and iframe_rendering_allowlist via array' do + put api('/application/settings', admin), + params: { + iframe_rendering_enabled: true, + iframe_rendering_allowlist: ['example.com', 'videos.example.com:443', 'https://example.net/'] + } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['iframe_rendering_enabled']).to be(true) + expect(json_response['iframe_rendering_allowlist']).to match_array(['example.com', 'videos.example.com:443', 'example.net']) + expect(ApplicationSetting.current.iframe_rendering_enabled?).to be(true) + expect(ApplicationSetting.current.iframe_rendering_allowlist).to match_array(['example.com', 'videos.example.com:443', 'example.net']) + end + + it 'denies bad allowlist entries' do + put api('/application/settings', admin), + params: { + iframe_rendering_enabled: true, + iframe_rendering_allowlist: ['gopher://gopherz.tv'] + } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['iframe_rendering_allowlist']).to include("'gopher://gopherz.tv' is not a valid domain name") + expect(ApplicationSetting.current.iframe_rendering_allowlist.join(',')).not_to include('gopherz') + end + + it 'allows a raw string for iframe_rendering_allowlist_raw' do + raw = "example.com\nvideos.example.com:443" + put api('/application/settings', admin), + params: { + iframe_rendering_allowlist_raw: raw + } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['iframe_rendering_allowlist']).to match_array(['example.com', 'videos.example.com:443']) + expect(json_response['iframe_rendering_allowlist_raw']).to eq(raw) + expect(ApplicationSetting.current.iframe_rendering_allowlist).to match_array(['example.com', 'videos.example.com:443']) + end + end + context "custom repository storage type set in the config" do before do # Add a possible storage to the config diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index bacb57e926bdf2d303d02a62275f29eec7cf6331..d157d8b7c8318113b3d3e40fd6a914f8d2dadab8 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -315,4 +315,43 @@ end end end + + describe 'content security policy' do + before do + sign_in(user) + end + + context 'when configuring iframes in Markdown' do + let(:iframe_rendering_allowlist) { ['www.youtube.com', 'embed.figma.com', 'www.figma.com'] } + + before do + allow(Gitlab::CurrentSettings).to receive_messages( + iframe_rendering_enabled?: iframe_rendering_enabled?, + iframe_rendering_allowlist: iframe_rendering_allowlist) + end + + context 'when disabled' do + let(:iframe_rendering_enabled?) { false } + + it 'does not modify frame-src' do + get root_path + + expect(response.headers['Content-Security-Policy']).not_to include('www.youtube.com') + expect(response.headers['Content-Security-Policy']).not_to include('figma.com') + end + end + + context 'when enabled' do + let(:iframe_rendering_enabled?) { true } + + it 'adds the domains to frame-src' do + get root_path + + expect(response.headers['Content-Security-Policy']).to include('https://www.youtube.com') + expect(response.headers['Content-Security-Policy']).to include('https://embed.figma.com') + expect(response.headers['Content-Security-Policy']).to include('https://www.figma.com') + end + end + end + end end diff --git a/spec/support/shared_examples/lib/banzai/filters/filter_timeout_shared_examples.rb b/spec/support/shared_examples/lib/banzai/filters/filter_timeout_shared_examples.rb index 4074745cfbc10aec83d975c957b03e47d32d933f..b168acfbecc03a924cbf6e4cf39d4b06af691e8b 100644 --- a/spec/support/shared_examples/lib/banzai/filters/filter_timeout_shared_examples.rb +++ b/spec/support/shared_examples/lib/banzai/filters/filter_timeout_shared_examples.rb @@ -64,13 +64,13 @@ # Usage: # # it_behaves_like 'pipeline timing check' -RSpec.shared_examples 'pipeline timing check' do |context: {}| +RSpec.shared_examples 'pipeline timing check' do |**opts| it 'checks the pipeline timing' do expect_next_instance_of(described_class) do |instance| expect(instance).to receive(:exceeded_pipeline_max?).and_return(true) end - filter = described_class.new('text', context) + filter = described_class.new('text', defined?(context) ? context : opts.fetch(:context, {})) filter.call end end