DEV: Add validation for allowed iframes setting (#28178)

- Adds a validator for the allowed iframes site setting
- Adds a migration to update any values that don't pass the validator

Follow up to: 188cb58daa

config/locales/server.en.yml

@@ -2709,6 +2709,7 @@ en:
       invalid_reply_by_email_address: "Value must contain '%{reply_key}' and be different from the notification email."
       invalid_alternative_reply_by_email_addresses: "All values must contain '%{reply_key}' and be different from the notification email."
       invalid_domain_hostname: "Must not include * or ? characters."
+      invalid_allowed_iframes_url: "Iframe urls must start with http:// or https:// and have at least one more additional '/'"
       invalid_csp_script_src: "Value must be either 'unsafe-eval' or 'wasm-unsafe-eval', or in the form '<hash algorithm>-<base64 value>' where supported hash algorithms are sha256, sha384 or sha512. Ensure that your input is wrapped in single quotation marks."
       pop3_polling_host_is_empty: "You must set a 'pop3 polling host' before enabling POP3 polling."
       pop3_polling_username_is_empty: "You must set a 'pop3 polling username' before enabling POP3 polling."

config/site_settings.yml

@@ -2020,6 +2020,7 @@ security:
     type: list
     list_type: simple
     client: true
+    validator: "AllowedIframesValidator"
   allowed_crawler_user_agents:
     type: list
     default: ""

db/migrate/20240731190511_update_invalid_allowed_iframe_values.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+class UpdateInvalidAllowedIframeValues < ActiveRecord::Migration[7.1]
+  def up
+    prev_value =
+      DB.query_single("SELECT value FROM site_settings WHERE name = 'allowed_iframes'").first
+
+    return if prev_value.blank?
+
+    # Url starts with http:// or https:// and has at least one more additional '/'
+    regex = %r{\Ahttps?://([^/]*/)+[^/]*\z}x
+
+    new_value =
+      prev_value
+        .split("|")
+        .map do |substring|
+          if substring.match?(regex)
+            substring
+          else
+            "#{substring}/"
+          end
+        end
+        .uniq
+        .join("|")
+
+    return if new_value == prev_value
+
+    DB.exec(<<~SQL, new_value:)
+      UPDATE site_settings
+      SET value = :new_value
+      WHERE name = 'allowed_iframes'
+    SQL
+
+    DB.exec(<<~SQL, prev_value:, new_value:)
+      INSERT INTO user_histories (action, subject, previous_value, new_value, admin_only, updated_at, created_at, acting_user_id)
+      VALUES (3, 'allowed_iframes', :prev_value, :new_value, true, NOW(), NOW(), -1)
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/validators/allowed_iframes_validator.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class AllowedIframesValidator
+  # Url starts with http:// or https:// and has at least one more additional '/'
+  VALID_ALLOWED_IFRAME_URL_REGEX = %r{\Ahttps?://([^/]*/)+[^/]*\z}x
+
+  def initialize(opts = {})
+    @opts = opts
+  end
+
+  def valid_value?(values)
+    values.split("|").all? { _1.match? VALID_ALLOWED_IFRAME_URL_REGEX }
+  end
+
+  def error_message
+    I18n.t("site_settings.errors.invalid_allowed_iframes_url")
+  end
+end

spec/lib/oneboxer_spec.rb

@@ -713,7 +713,7 @@ RSpec.describe Oneboxer do
       body: allowlisted_oembed.to_json,
     )
 
-    SiteSetting.allowed_iframes = "discourse.org|https://ifram.es"
+    SiteSetting.allowed_iframes = "https://discourse.org/|https://ifram.es/"
 
     expect(Oneboxer.onebox("https://blocklist.ed/iframes", invalidate_oneboxes: true)).to be_empty
     expect(Oneboxer.onebox("https://allowlist.ed/iframes", invalidate_oneboxes: true)).to match(