From 6f84f4afe89307542a6ca2afe2d05d4070374afc Mon Sep 17 00:00:00 2001 From: Daniel Uber Date: Thu, 22 Jul 2021 12:02:29 -0500 Subject: [PATCH] Validate profile field "website url" is in fact a url (don't generate local path links). (#14302) * Validate website_url profile field is a url related to issue #14300 May need a data update script to (fixup? remove?) invalid profiles since not being able to save existing profiles will cause problems. * Check that URI is a valid url Require the scheme to be one of https or http, not something like mailto:// or telnet:// (nobody would do that, but don't try and link to it if they did). * Rubocop fixup for validation rule URI::regexp is obsolete and should not be used. Instead, use URI::DEFAULT_PARSER.make_regexp Prefer %w[] literals for arrays of words Prefer the new style validations `validates :column, format: value` to validates_format_of * Permit empty website_url fields The previous validation was rejecting nil, which was the default. This caused a lot of user factory calls to fail (since users didn't need website urls in the profiles during testing unless the test was about the website url link). * Use :url validation as suggested * Update validation error message The validate_url gem gives a more complete error message to the user. Update the spec to expect this. * Use url_field rather than text_field in profile form https://apidock.com/rails/v6.1.3.1/ActionView/Helpers/FormHelper/url_field * Add data update to either fixup or clear invalid website urls Checking blazer there are about 2600 profiles with "invalid" website urls, typically a hostname, sometimes a hostname + path component, which should be fixed up. Naively add https:// to the front of the url, check that a valid scheme and host are present on the resulting url, and save. If an invalid url is generated (specifically, if host or scheme are nil), just set the website url (with the invalid link) to an empty string. It's possible we'd want to notify users affected by this, that was not included in this pass. * Skip validations for intentionally invalid test cases --- app/models/profile.rb | 1 + app/views/users/_profile.html.erb | 2 +- ...210722135549_profile_website_url_format.rb | 29 +++++++++++ .../profile_website_url_format_spec.rb | 48 +++++++++++++++++++ spec/models/profile_spec.rb | 17 +++++++ 5 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 lib/data_update_scripts/20210722135549_profile_website_url_format.rb create mode 100644 spec/lib/data_update_scripts/profile_website_url_format_spec.rb diff --git a/app/models/profile.rb b/app/models/profile.rb index 7801fe838..c42a1cfb9 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -3,6 +3,7 @@ class Profile < ApplicationRecord validates :user_id, uniqueness: true validates :location, :website_url, length: { maximum: 100 } + validates :website_url, url: { allow_blank: true, no_local: true, schemes: %w[https http] } validates_with ProfileValidator # Static fields are columns on the profiles table; they have no relationship diff --git a/app/views/users/_profile.html.erb b/app/views/users/_profile.html.erb index dc1a690ac..03f538860 100644 --- a/app/views/users/_profile.html.erb +++ b/app/views/users/_profile.html.erb @@ -50,7 +50,7 @@ - <%= f.text_field "profile[website_url]", + <%= f.url_field "profile[website_url]", value: profile.website_url, placeholder: "https://yoursite.com", class: "crayons-textfield js-color-field" %> diff --git a/lib/data_update_scripts/20210722135549_profile_website_url_format.rb b/lib/data_update_scripts/20210722135549_profile_website_url_format.rb new file mode 100644 index 000000000..a6d9e5c9f --- /dev/null +++ b/lib/data_update_scripts/20210722135549_profile_website_url_format.rb @@ -0,0 +1,29 @@ +module DataUpdateScripts + class ProfileWebsiteUrlFormat + 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}" + uri = URI.parse(new_url) + uri.scheme.present? && uri.host.present? && new_url || "" + 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 + 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 new file mode 100644 index 000000000..1238de366 --- /dev/null +++ b/spec/lib/data_update_scripts/profile_website_url_format_spec.rb @@ -0,0 +1,48 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210722135549_profile_website_url_format.rb", +) + +describe DataUpdateScripts::ProfileWebsiteUrlFormat do + let(:nil_profile) { create(:profile, website_url: nil) } + 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| + 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 +end diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index d05a1a215..503509b98 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -57,6 +57,23 @@ RSpec.describe Profile, type: :model do expect(profile.errors_as_sentence).to eq "Location is too long (maximum is 100 characters)" end end + + describe "validating website_url" do + it "is valid if blank" do + profile.website_url = nil + expect(profile).to be_valid + end + it "is valid with a complete url" do + profile.website_url = "https://dev.to" + expect(profile).to be_valid + end + + it "is invalid with an incomplete url" do + profile.website_url = "dev.to" + expect(profile).not_to be_valid + expect(profile.errors_as_sentence).to eq "Website url is not a valid URL" + end + end end context "when accessing profile fields" do