From a6624af66e512dcf9e08c0dd0ab8432a7c135298 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 30 Apr 2024 16:45:07 +0800 Subject: [PATCH] DEV: Add `isValidUrl` helper function to theme migrations (#26817) This commit adds a `isValidUrl` helper function to the context in which theme migrations are ran in. This helper function is to make it easier for theme developers to check if a string is a valid URL or path when writing theme migrations. This can be helpful in cases when migrating a string based setting to `type: objects` which contain `type: string` properties with URL validations enabled. This commit also introduces the `UrlHelper.is_valid_url?` method which actually checks that the URL string is of the valid format instead of only checking if the URL string is parseable which is what `UrlHelper.relaxed_parse` does and is not sufficient for our needs. --- .../theme_settings_migrations_runner.rb | 6 +++ lib/theme_settings_object_validator.rb | 2 +- lib/url_helper.rb | 23 +++++++++ .../theme_settings_object_validator_spec.rb | 2 +- spec/lib/url_helper_spec.rb | 49 +++++++++++++++++++ .../theme_settings_migrations_runner_spec.rb | 19 +++++++ 6 files changed, 99 insertions(+), 2 deletions(-) diff --git a/app/services/theme_settings_migrations_runner.rb b/app/services/theme_settings_migrations_runner.rb index 4f233ecb2ba..6a8c04307da 100644 --- a/app/services/theme_settings_migrations_runner.rb +++ b/app/services/theme_settings_migrations_runner.rb @@ -15,6 +15,12 @@ class ThemeSettingsMigrationsRunner def get_category_id_by_name(category_name) Category.where(name_lower: category_name).pick(:id) end + + # @param [String] URL string to check if it is a valid absolute URL, path or anchor. + # @return [Boolean] True if the URL is a valid URL or path, false otherwise. + def is_valid_url(url) + UrlHelper.is_valid_url?(url) + end end Migration = Struct.new(:version, :name, :original_name, :code, :theme_field_id) diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index 68ca804d2d5..e9cdd59e19a 100644 --- a/lib/theme_settings_object_validator.rb +++ b/lib/theme_settings_object_validator.rb @@ -188,7 +188,7 @@ class ThemeSettingsObjectValidator return false end - if validations&.dig(:url) && !UrlHelper.relaxed_parse(value) + if validations&.dig(:url) && !UrlHelper.is_valid_url?(value) add_error(property_name, :string_value_not_valid_url) return false end diff --git a/lib/url_helper.rb b/lib/url_helper.rb index 4c333839ba6..be36948bdc4 100644 --- a/lib/url_helper.rb +++ b/lib/url_helper.rb @@ -24,6 +24,29 @@ class UrlHelper rescue URI::Error end + # Heuristic checks to determine if the URL string is a valid absolute URL, path or anchor + def self.is_valid_url?(url) + uri = URI.parse(url) + + return true if uri.is_a?(URI::Generic) && url.starts_with?("/") || url.match?(/\A\#([^#]*)/) + + if uri.scheme + return true if uri.is_a?(URI::MailTo) + + if url.match?(%r{\A#{uri.scheme}://[^/]}) && + ( + uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) || uri.is_a?(URI::FTP) || + uri.is_a?(URI::LDAP) + ) + return true + end + end + + false + rescue URI::InvalidURIError + false + end + def self.encode_and_parse(url) URI.parse(Addressable::URI.encode(url)) end diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index 356eb395727..57abf3657d5 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/spec/lib/theme_settings_object_validator_spec.rb @@ -424,7 +424,7 @@ RSpec.describe ThemeSettingsObjectValidator do expect(errors["/string_property"].full_messages).to contain_exactly("must be a string") end - it "should not return an empty hash when string property satsify url validation" do + it "should not return an empty hash when string property satisfy url validation" do schema = { name: "section", properties: { diff --git a/spec/lib/url_helper_spec.rb b/spec/lib/url_helper_spec.rb index 81608561085..bf36e0c3de5 100644 --- a/spec/lib/url_helper_spec.rb +++ b/spec/lib/url_helper_spec.rb @@ -271,4 +271,53 @@ RSpec.describe UrlHelper do expect(described_class.rails_route_from_url(url)).to eq(nil) end end + + describe ".is_valid_url?" do + it "should return true for a valid HTTP URL" do + expect(described_class.is_valid_url?("http://www.example.com")).to eq(true) + end + + it "should return true for a valid HTTPS URL" do + expect(described_class.is_valid_url?("https://www.example.com")).to eq(true) + end + + it "should return true for a valid FTP URL" do + expect(described_class.is_valid_url?("ftp://example.com")).to eq(true) + end + + it "should return true for a valid mailto URL" do + expect(described_class.is_valid_url?("mailto:someone@discourse.org")).to eq(true) + end + + it "should return true for a valid LDAP URL" do + expect(described_class.is_valid_url?("ldap://ldap.example.com/dc=example;dc=com?quer")).to eq( + true, + ) + end + + it "should return true for a path" do + expect(described_class.is_valid_url?("/some/path")).to eq(true) + end + + it "should return true for a path with query params" do + expect(described_class.is_valid_url?("/some/path?query=param")).to eq(true) + end + + it "should return true for anchor links" do + expect(described_class.is_valid_url?("#anchor")).to eq(true) + expect(described_class.is_valid_url?("#")).to eq(true) + end + + it "should return false for invalid urls" do + expect(described_class.is_valid_url?("")).to eq(false) + expect(described_class.is_valid_url?("http//www.example.com")).to eq(false) + expect(described_class.is_valid_url?("http:/www.example.com")).to eq(false) + expect(described_class.is_valid_url?("https:///www.example.com")).to eq(false) + expect(described_class.is_valid_url?("mailtoooo:someone@discourse.org")).to eq(false) + expect(described_class.is_valid_url?("ftp://")).to eq(false) + expect(described_class.is_valid_url?("http://")).to eq(false) + expect(described_class.is_valid_url?("https://")).to eq(false) + expect(described_class.is_valid_url?("ldap://")).to eq(false) + end + end end diff --git a/spec/services/theme_settings_migrations_runner_spec.rb b/spec/services/theme_settings_migrations_runner_spec.rb index a986e785a28..549ec945a1f 100644 --- a/spec/services/theme_settings_migrations_runner_spec.rb +++ b/spec/services/theme_settings_migrations_runner_spec.rb @@ -336,6 +336,25 @@ describe ThemeSettingsMigrationsRunner do expect(results[0][:settings_after]).to eq({}) end + it "attaches the isValidUrl() function to the context of the migrations" do + theme.update_setting(:string_setting, "https://google.com") + theme.save! + + migration_field.update!(value: <<~JS) + export default function migrate(settings, helpers) { + if (!helpers.isValidUrl("some_invalid_string")) { + settings.set("string_setting", "is_not_valid_string"); + } + + return settings; + } + JS + + results = described_class.new(theme).run + + expect(results[0][:settings_after]).to eq({ "string_setting" => "is_not_valid_string" }) + end + it "attaches the getCategoryIdByName() function to the context of the migrations" do category = Fabricate(:category, name: "some-category")