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.
This commit is contained in:
Alan Guo Xiang Tan 2024-04-30 16:45:07 +08:00 committed by GitHub
parent bfc0f3f4cd
commit a6624af66e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 99 additions and 2 deletions

View File

@ -15,6 +15,12 @@ class ThemeSettingsMigrationsRunner
def get_category_id_by_name(category_name) def get_category_id_by_name(category_name)
Category.where(name_lower: category_name).pick(:id) Category.where(name_lower: category_name).pick(:id)
end 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 end
Migration = Struct.new(:version, :name, :original_name, :code, :theme_field_id) Migration = Struct.new(:version, :name, :original_name, :code, :theme_field_id)

View File

@ -188,7 +188,7 @@ class ThemeSettingsObjectValidator
return false return false
end 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) add_error(property_name, :string_value_not_valid_url)
return false return false
end end

View File

@ -24,6 +24,29 @@ class UrlHelper
rescue URI::Error rescue URI::Error
end 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) def self.encode_and_parse(url)
URI.parse(Addressable::URI.encode(url)) URI.parse(Addressable::URI.encode(url))
end end

View File

@ -424,7 +424,7 @@ RSpec.describe ThemeSettingsObjectValidator do
expect(errors["/string_property"].full_messages).to contain_exactly("must be a string") expect(errors["/string_property"].full_messages).to contain_exactly("must be a string")
end 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 = { schema = {
name: "section", name: "section",
properties: { properties: {

View File

@ -271,4 +271,53 @@ RSpec.describe UrlHelper do
expect(described_class.rails_route_from_url(url)).to eq(nil) expect(described_class.rails_route_from_url(url)).to eq(nil)
end end
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 end

View File

@ -336,6 +336,25 @@ describe ThemeSettingsMigrationsRunner do
expect(results[0][:settings_after]).to eq({}) expect(results[0][:settings_after]).to eq({})
end 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 it "attaches the getCategoryIdByName() function to the context of the migrations" do
category = Fabricate(:category, name: "some-category") category = Fabricate(:category, name: "some-category")