From 591577e6b83400045253145befc587033e57a988 Mon Sep 17 00:00:00 2001 From: Daniel Uber Date: Thu, 22 Jul 2021 14:16:43 -0500 Subject: [PATCH] [hotfix] website_url update script should handle parse errors (#14306) * Add additional test cases to the DUS And then handle them. * rubocop changes Keep let blocks together prefer blank? to ! + present? * Test copied script as well Since the file was fixed, then copied - we want to test the copy that will actually run (these _should_ be idempotent and running both back to back will cause no harm). --- ...210722135549_profile_website_url_format.rb | 16 ++++- ...160452_profile_website_url_format_fixup.rb | 41 +++++++++++ .../profile_website_url_format_fixup_spec.rb | 70 +++++++++++++++++++ .../profile_website_url_format_spec.rb | 38 +++++++--- 4 files changed, 155 insertions(+), 10 deletions(-) create mode 100644 lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb create mode 100644 spec/lib/data_update_scripts/profile_website_url_format_fixup_spec.rb diff --git a/lib/data_update_scripts/20210722135549_profile_website_url_format.rb b/lib/data_update_scripts/20210722135549_profile_website_url_format.rb index a6d9e5c9f..85d33dccd 100644 --- a/lib/data_update_scripts/20210722135549_profile_website_url_format.rb +++ b/lib/data_update_scripts/20210722135549_profile_website_url_format.rb @@ -10,9 +10,15 @@ module DataUpdateScripts def fix_or_clear_website_url(profile) profile.website_url = begin - new_url = "https://#{profile.website_url}" + new_url = "https://#{profile.website_url.strip}" uri = URI.parse(new_url) - uri.scheme.present? && uri.host.present? && new_url || "" + if acceptable_uri?(uri) + new_url + else + "" + end + rescue URI::InvalidURIError + "" end end @@ -25,5 +31,11 @@ module DataUpdateScripts .where.not("website_url like 'http__/%'") .where.not(website_url: [nil, ""]) end + + def acceptable_uri?(uri) + uri.scheme.present? && + uri.host.present? && + uri.user.blank? + end end end diff --git a/lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb b/lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb new file mode 100644 index 000000000..69b39b3f2 --- /dev/null +++ b/lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb @@ -0,0 +1,41 @@ +module DataUpdateScripts + class ProfileWebsiteUrlFormatFixup + def run + profiles_to_fix.each do |profile| + fix_or_clear_website_url(profile) + profile.save || log_failure(profile) + end + end + + def fix_or_clear_website_url(profile) + profile.website_url = + begin + new_url = "https://#{profile.website_url.strip}" + uri = URI.parse(new_url) + if acceptable_uri?(uri) + new_url + else + "" + end + rescue URI::InvalidURIError + "" + end + end + + def log_failure(profile) + Rails.logger.warn("Attempted to update website_url for profile #{profile.id} but failed with #{profile.errors}") + end + + def profiles_to_fix + Profile + .where.not("website_url like 'http__/%'") + .where.not(website_url: [nil, ""]) + end + + def acceptable_uri?(uri) + uri.scheme.present? && + uri.host.present? && + uri.user.blank? + end + end +end diff --git a/spec/lib/data_update_scripts/profile_website_url_format_fixup_spec.rb b/spec/lib/data_update_scripts/profile_website_url_format_fixup_spec.rb new file mode 100644 index 000000000..07ab8a360 --- /dev/null +++ b/spec/lib/data_update_scripts/profile_website_url_format_fixup_spec.rb @@ -0,0 +1,70 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb", +) + +describe DataUpdateScripts::ProfileWebsiteUrlFormatFixup do + let(:nil_profile) { create(:profile, website_url: nil) } + let(:invalid_profile) { make_profile_for("www.example.com") } + let(:unfixable_profile) { make_profile_for("/local.html") } + let(:email_profile) { make_profile_for("dan@forem.com") } + let(:spacey_profile) { make_profile_for(" example.com ") } + let(:unparseable_profile) { make_profile_for("this will not parse") } + let(:empty_profile) { create(:profile, website_url: "") } + let(:valid_profile) { create(:profile, website_url: "https://www.example.com") } + + def make_profile_for(website_url) + build(:profile, website_url: website_url).tap do |profile| + profile.save(validate: false) + end + end + + it "does not modify profiles where website url is null" do + expect { described_class.new.run }.not_to change(nil_profile, :website_url) + end + + it "does not modify profiles where website url is empty" do + expect { described_class.new.run }.not_to change(empty_profile, :website_url) + end + + it "does not modify profiles where website url is valid" do + expect { described_class.new.run }.not_to change(valid_profile, :website_url) + end + + it "prepends https:// to invalid urls to make a valid url from hostnames" do + expect { described_class.new.run } + .to change { invalid_profile.reload.website_url } + .from("www.example.com") + .to("https://www.example.com") + end + + it "clears websites that don't form valid urls by prepending" do + expect { described_class.new.run } + .to change { unfixable_profile.reload.website_url } + .from("/local.html") + .to("") + end + + it "rejects users in the link" do + # what was meant to be an email address is actually a valid user@host url + # we just don't want to do this + expect { described_class.new.run } + .to change { email_profile.reload.website_url } + .from("dan@forem.com") + .to("") + end + + it "trims the input to help parsing" do + expect { described_class.new.run } + .to change { spacey_profile.reload.website_url } + .from(" example.com ") + .to("https://example.com") + end + + it "handles parse errors by clearing the website url" do + expect { described_class.new.run } + .to change { unparseable_profile.reload.website_url } + .from("this will not parse") + .to("") + end +end diff --git a/spec/lib/data_update_scripts/profile_website_url_format_spec.rb b/spec/lib/data_update_scripts/profile_website_url_format_spec.rb index 1238de366..7f7ff7979 100644 --- a/spec/lib/data_update_scripts/profile_website_url_format_spec.rb +++ b/spec/lib/data_update_scripts/profile_website_url_format_spec.rb @@ -5,17 +5,16 @@ require Rails.root.join( describe DataUpdateScripts::ProfileWebsiteUrlFormat do let(:nil_profile) { create(:profile, website_url: nil) } + let(:invalid_profile) { make_profile_for("www.example.com") } + let(:unfixable_profile) { make_profile_for("/local.html") } + let(:email_profile) { make_profile_for("dan@forem.com") } + let(:spacey_profile) { make_profile_for(" example.com ") } + let(:unparseable_profile) { make_profile_for("this will not parse") } let(:empty_profile) { create(:profile, website_url: "") } let(:valid_profile) { create(:profile, website_url: "https://www.example.com") } - let(:invalid_profile) do - build(:profile, website_url: "www.example.com").tap do |profile| - profile.save(validate: false) - end - end - - let(:unfixable_profile) do - build(:profile, website_url: "/local.html").tap do |profile| + def make_profile_for(website_url) + build(:profile, website_url: website_url).tap do |profile| profile.save(validate: false) end end @@ -45,4 +44,27 @@ describe DataUpdateScripts::ProfileWebsiteUrlFormat do .from("/local.html") .to("") end + + it "rejects users in the link" do + # what was meant to be an email address is actually a valid user@host url + # we just don't want to do this + expect { described_class.new.run } + .to change { email_profile.reload.website_url } + .from("dan@forem.com") + .to("") + end + + it "trims the input to help parsing" do + expect { described_class.new.run } + .to change { spacey_profile.reload.website_url } + .from(" example.com ") + .to("https://example.com") + end + + it "handles parse errors by clearing the website url" do + expect { described_class.new.run } + .to change { unparseable_profile.reload.website_url } + .from("this will not parse") + .to("") + end end