From 6ee6b1f1d111ff8082c8d354b5963fc7376c460d Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Thu, 1 Aug 2024 06:51:02 -0600
Subject: [PATCH] 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: 188cb58daa833839c54c266ce22db150a3f3a210
---
 config/locales/server.en.yml                  |  1 +
 config/site_settings.yml                      |  1 +
 ...11_update_invalid_allowed_iframe_values.rb | 42 +++++++++++++++++++
 lib/validators/allowed_iframes_validator.rb   | 18 ++++++++
 spec/lib/oneboxer_spec.rb                     |  2 +-
 5 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20240731190511_update_invalid_allowed_iframe_values.rb
 create mode 100644 lib/validators/allowed_iframes_validator.rb

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 24d1489856c..aff2d7474ed 100644
--- a/config/locales/server.en.yml
+++ b/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."
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 87f0b132936..216ed3c0796 100644
--- a/config/site_settings.yml
+++ b/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: ""
diff --git a/db/migrate/20240731190511_update_invalid_allowed_iframe_values.rb b/db/migrate/20240731190511_update_invalid_allowed_iframe_values.rb
new file mode 100644
index 00000000000..8947b45e391
--- /dev/null
+++ b/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
diff --git a/lib/validators/allowed_iframes_validator.rb b/lib/validators/allowed_iframes_validator.rb
new file mode 100644
index 00000000000..63e155364fc
--- /dev/null
+++ b/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
diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb
index cbf4ba55e4e..a2cde501cda 100644
--- a/spec/lib/oneboxer_spec.rb
+++ b/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(