From 6ecf37c482603cba27510e41723fdd1909d0a135 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Thu, 21 Dec 2017 12:27:17 +0800
Subject: [PATCH] Improve URL validation to check for a valid host.

Parsing a URL with `URI` is not sufficient as the following cases
are considered valid:

URI.parse("http://https://google.com")
=> #<URI::HTTP http://https//google.com>
---
 app/models/user_profile.rb                    |  7 +---
 lib/validators/url_validator.rb               | 10 ++++--
 .../validators/url_validator_spec.rb          | 33 +++++++++++++++++++
 spec/models/user_profile_spec.rb              | 17 ++++++----
 4 files changed, 52 insertions(+), 15 deletions(-)
 create mode 100644 spec/components/validators/url_validator_spec.rb

diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb
index 1d07636a507..d10b63f034d 100644
--- a/app/models/user_profile.rb
+++ b/app/models/user_profile.rb
@@ -1,13 +1,8 @@
 class UserProfile < ActiveRecord::Base
   belongs_to :user, inverse_of: :user_profile
 
-  # This is not very picky about most DNS labels (the bits between the
-  # periods), but isn't taking much guff from the TLD. No leading
-  # digit, and no hyphens unless IDN.
-  WEBSITE_REGEXP = /(^$)|(^(https?:\/\/)?([a-z0-9][a-z0-9-]*\.)+([a-z][a-z0-9]+|xn--[a-z0-9-]+)(\/.*)?$)/i
-
   validates :bio_raw, length: { maximum: 3000 }
-  validates :website, format: { with: WEBSITE_REGEXP }, allow_blank: true, if: Proc.new { |c| c.new_record? || c.website_changed? }
+  validates :website, url: true, allow_blank: true, if: Proc.new { |c| c.new_record? || c.website_changed? }
   validates :user, presence: true
   before_save :cook
   after_save :trigger_badges
diff --git a/lib/validators/url_validator.rb b/lib/validators/url_validator.rb
index c3295e63684..7c91bf206ba 100644
--- a/lib/validators/url_validator.rb
+++ b/lib/validators/url_validator.rb
@@ -1,9 +1,15 @@
 class UrlValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     if value.present?
-      uri = URI.parse(value) rescue nil
+      valid =
+        begin
+          uri = URI.parse(value)
+          uri.is_a?(URI::HTTP) && !uri.host.nil? && uri.host.include?(".")
+        rescue
+          nil
+        end
 
-      unless uri
+      unless valid
         record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid'))
       end
     end
diff --git a/spec/components/validators/url_validator_spec.rb b/spec/components/validators/url_validator_spec.rb
new file mode 100644
index 00000000000..5554700b774
--- /dev/null
+++ b/spec/components/validators/url_validator_spec.rb
@@ -0,0 +1,33 @@
+require 'rails_helper'
+require 'validators/topic_title_length_validator'
+
+RSpec.describe UrlValidator do
+  let(:record) { Fabricate.build(:user_profile, user: Fabricate.build(:user)) }
+  let(:validator) { described_class.new(attributes: :website) }
+  subject(:validate) { validator.validate_each(record, :website, record.website) }
+
+  [
+    "http://https://google.com",
+    "http://google/",
+    "ftp://ftp.google.com",
+    "http:///what.is.this",
+    'http://meta.discourse.org TEST'
+  ].each do |invalid_url|
+    it "#{invalid_url} should not be valid" do
+      record.website = invalid_url
+      validate
+      expect(record.errors[:website]).to be_present
+    end
+  end
+
+  [
+    "http://discourse.productions",
+    "https://google.com",
+  ].each do |valid_url|
+    it "#{valid_url} should be valid" do
+      record.website = valid_url
+      validate
+      expect(record.errors[:website]).to_not be_present
+    end
+  end
+end
diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb
index 7ae25e14cdc..0fffc455aaa 100644
--- a/spec/models/user_profile_spec.rb
+++ b/spec/models/user_profile_spec.rb
@@ -55,18 +55,21 @@ describe UserProfile do
     end
 
     context "website validation" do
-      let(:user) { Fabricate(:user) }
+      let(:user_profile) { Fabricate.build(:user_profile, user: Fabricate(:user)) }
 
-      it "ensures website is valid" do
-        expect(Fabricate.build(:user_profile, user: user, website: "http://https://google.com")).not_to be_valid
-        expect(Fabricate.build(:user_profile, user: user, website: "http://discourse.productions")).to be_valid
-        expect(Fabricate.build(:user_profile, user: user, website: "https://google.com")).to be_valid
+      it "should not allow invalid URLs" do
+        user_profile.website = "http://https://google.com"
+        expect(user_profile).to_not be_valid
       end
 
       it "validates website domain if user_website_domains_whitelist setting is present" do
         SiteSetting.user_website_domains_whitelist = "discourse.org"
-        expect(Fabricate.build(:user_profile, user: user, website: "https://google.com")).not_to be_valid
-        expect(Fabricate.build(:user_profile, user: user, website: "http://discourse.org")).to be_valid
+
+        user_profile.website = "https://google.com"
+        expect(user_profile).not_to be_valid
+
+        user_profile.website = "http://discourse.org"
+        expect(user_profile).to be_valid
       end
     end