FIX: Replace deprecated URI.encode, URI.escape, URI.unescape and URI.unencode (#8528)

The following methods have long been deprecated in ruby due to flaws in their implementation per http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/29293?29179-31097:

URI.escape
URI.unescape
URI.encode
URI.unencode
escape/encode are just aliases for one another. This PR uses the Addressable gem to replace these methods with its own encode, unencode, and encode_component methods where appropriate.

I have put all references to Addressable::URI here into the UrlHelper to keep them corralled in one place to make changes to this implementation easier.

Addressable is now also an explicit gem dependency.

Gemfile

@@ -134,6 +134,7 @@ gem 'highline', '~> 1.7.0', require: false
 gem 'rack-protection' # security
 gem 'cbor', require: false
 gem 'cose', require: false
+gem 'addressable', '~> 2.7.0'
 
 # Gems used only for assets and not required in production environments by default.
 # Allow everywhere for now cause we are allowing asset debugging in production

Gemfile.lock

@@ -437,6 +437,7 @@ DEPENDENCIES
   activemodel (= 6.0.1)
   activerecord (= 6.0.1)
   activesupport (= 6.0.1)
+  addressable (~> 2.7.0)
   annotate
   aws-sdk-s3
   aws-sdk-sns

app/controllers/list_controller.rb

@@ -304,7 +304,7 @@ class ListController < ApplicationController
         (slug_path + [@category.id.to_s]).join("/")
     end
 
-    route_params[:username] = UrlHelper.escape_uri(params[:username]) if params[:username].present?
+    route_params[:username] = UrlHelper.encode_component(params[:username]) if params[:username].present?
     route_params
   end
 
@@ -374,7 +374,7 @@ class ListController < ApplicationController
 
     opts = opts.dup
     if SiteSetting.unicode_usernames && opts[:group_name]
-      opts[:group_name] = URI.encode(opts[:group_name])
+      opts[:group_name] = UrlHelper.encode_component(opts[:group_name])
     end
     opts.delete(:category) if page_params.include?(:category_slug_path_with_id)
 

app/helpers/application_helper.rb

@@ -383,8 +383,7 @@ module ApplicationHelper
 
   def topic_featured_link_domain(link)
     begin
-      uri = URI.encode(link)
+      uri = UrlHelper.encode_and_parse(link)
-      uri = URI.parse(uri)
       uri = URI.parse("http://#{uri}") if uri.scheme.nil?
       host = uri.host.downcase
       host.start_with?('www.') ? host[4..-1] : host

app/jobs/regular/download_backup_email.rb

@@ -15,7 +15,7 @@ module Jobs
       raise Discourse::InvalidParameters.new(:backup_file_path) if backup_file_path.blank?
 
       backup_file_path = URI(backup_file_path)
-      backup_file_path.query = URI.encode_www_form(token: EmailBackupToken.set(user.id))
+      backup_file_path.query = { token: EmailBackupToken.set(user.id) }.to_param
 
       message = DownloadBackupMailer.send_email(user.email, backup_file_path.to_s)
       Email::Sender.new(message, :download_backup_message).send

app/jobs/regular/update_username.rb

@@ -27,7 +27,7 @@ module Jobs
 
       cooked_username = PrettyText::Helpers.format_username(@old_username)
       @cooked_mention_username_regex = /^@#{cooked_username}$/i
-      @cooked_mention_user_path_regex = /^\/u(?:sers)?\/#{CGI.escape(cooked_username)}$/i
+      @cooked_mention_user_path_regex = /^\/u(?:sers)?\/#{UrlHelper.encode_component(cooked_username)}$/i
       @cooked_quote_username_regex = /(?<=\s)#{cooked_username}(?=:)/i
 
       update_posts

app/mailers/user_notifications.rb

@@ -624,7 +624,7 @@ class UserNotifications < ActionMailer::Base
 
     email_opts = {
       topic_title: Emoji.gsub_emoji_to_unicode(title),
-      topic_title_url_encoded: title ? URI.encode(title) : title,
+      topic_title_url_encoded: title ? UrlHelper.encode_component(title) : title,
       message: message,
       url: post.url(without_slug: SiteSetting.private_email?),
       post_id: post.id,
@@ -649,7 +649,7 @@ class UserNotifications < ActionMailer::Base
       use_topic_title_subject: use_topic_title_subject,
       site_description: SiteSetting.site_description,
       site_title: SiteSetting.title,
-      site_title_url_encoded: URI.encode(SiteSetting.title),
+      site_title_url_encoded: UrlHelper.encode_component(SiteSetting.title),
       locale: locale
     }
 

app/models/concerns/has_url.rb

@@ -22,7 +22,7 @@ module HasUrl
       return if url.blank?
 
       uri = begin
-        URI(URI.unescape(url))
+        URI(UrlHelper.unencode(url))
       rescue URI::Error
       end
 

app/models/embeddable_host.rb

@@ -34,7 +34,7 @@ class EmbeddableHost < ActiveRecord::Base
       return eh if eh.path_whitelist.blank?
 
       path_regexp = Regexp.new(eh.path_whitelist)
-      return eh if path_regexp.match(path) || path_regexp.match(URI.unescape(path))
+      return eh if path_regexp.match(path) || path_regexp.match(UrlHelper.unencode(path))
     end
 
     nil

app/models/post.rb

@@ -971,7 +971,7 @@ class Post < ActiveRecord::Base
       next unless Discourse.store.has_been_uploaded?(src) || (include_local_upload && src =~ /\A\/[^\/]/i)
 
       path = begin
-        URI(URI.unescape(GlobalSetting.cdn_url ? src.sub(GlobalSetting.cdn_url, "") : src))&.path
+        URI(UrlHelper.unencode(GlobalSetting.cdn_url ? src.sub(GlobalSetting.cdn_url, "") : src))&.path
       rescue URI::Error
       end
 

app/models/topic.rb

@@ -1354,7 +1354,7 @@ class Topic < ActiveRecord::Base
   end
 
   def featured_link_root_domain
-    MiniSuffix.domain(URI.parse(URI.encode(self.featured_link)).hostname)
+    MiniSuffix.domain(UrlHelper.encode_and_parse(self.featured_link).hostname)
   end
 
   def self.private_message_topics_count_per_day(start_date, end_date, topic_subtype)

app/models/user.rb

@@ -768,8 +768,8 @@ class User < ActiveRecord::Base
       url = SiteSetting.external_system_avatars_url.dup
       url = +"#{Discourse::base_uri}#{url}" unless url =~ /^https?:\/\//
       url.gsub! "{color}", letter_avatar_color(normalized_username)
-      url.gsub! "{username}", CGI.escape(username)
+      url.gsub! "{username}", UrlHelper.encode_component(username)
-      url.gsub! "{first_letter}", CGI.escape(normalized_username.grapheme_clusters.first)
+      url.gsub! "{first_letter}", UrlHelper.encode_component(normalized_username.grapheme_clusters.first)
       url.gsub! "{hostname}", Discourse.current_hostname
       url
     else

lib/final_destination.rb

@@ -315,10 +315,7 @@ class FinalDestination
   end
 
   def escape_url
-    UrlHelper.escape_uri(
+    UrlHelper.escape_uri(@url)
-      CGI.unescapeHTML(@url),
-      Regexp.new("[^#{URI::PATTERN::UNRESERVED}#{URI::PATTERN::RESERVED}#]")
-    )
   end
 
   def private_ranges

lib/url_helper.rb

@@ -10,13 +10,31 @@ class UrlHelper
     url, fragment = url.split("#", 2)
     uri = URI.parse(url)
     if uri
-      fragment = URI.escape(fragment) if fragment&.include?('#')
+      # Addressable::URI::CharacterClasses::UNRESERVED is used here because without it
+      # the # in the fragment is not encoded
+      fragment = Addressable::URI.encode_component(fragment, Addressable::URI::CharacterClasses::UNRESERVED) if fragment&.include?('#')
       uri.fragment = fragment
       uri
     end
   rescue URI::Error
   end
 
+  def self.encode_and_parse(url)
+    URI.parse(Addressable::URI.encode(url))
+  end
+
+  def self.encode(url)
+    Addressable::URI.encode(url)
+  end
+
+  def self.unencode(url)
+    Addressable::URI.unencode(url)
+  end
+
+  def self.encode_component(url_component)
+    Addressable::URI.encode_component(url_component)
+  end
+
   def self.is_local(url)
     url.present? && (
       Discourse.store.has_been_uploaded?(url) ||
@@ -43,14 +61,10 @@ class UrlHelper
     self.absolute(url, nil)
   end
 
-  DOUBLE_ESCAPED_REGEXP ||= /%25([0-9a-f]{2})/i
-
   # Prevents double URL encode
   # https://stackoverflow.com/a/37599235
-  def self.escape_uri(uri, pattern = URI::UNSAFE)
+  def self.escape_uri(uri)
-    encoded = URI.encode(uri, pattern)
+    UrlHelper.encode_component(CGI.unescapeHTML(UrlHelper.unencode(uri)))
-    encoded.gsub!(DOUBLE_ESCAPED_REGEXP, '%\1')
-    encoded
   end
 
   def self.cook_url(url, secure: false)

lib/validators/url_validator.rb

@@ -9,7 +9,7 @@ class UrlValidator < ActiveModel::EachValidator
           uri.is_a?(URI::HTTP) && !uri.host.nil? && uri.host.include?(".")
         rescue URI::Error => e
           if (e.message =~ /URI must be ascii only/)
-            value = URI.encode(value)
+            value = UrlHelper.encode(value)
             retry
           end
 

script/import_scripts/ipboard.rb

@@ -972,7 +972,7 @@ EOM
     User.find_each do |u|
       ucf = u.custom_fields
       if ucf && ucf["import_id"] && ucf["import_username"]
-        username = URI.escape(ucf["import_username"])
+        username = UrlHelper.encode_component(ucf["import_username"])
         Permalink.create(url: "#{USERDIR}/#{ucf['import_id']}-#{username}", external_url: "/users/#{u.username}") rescue nil
         print '.'
       end

spec/requests/list_controller_spec.rb

@@ -181,7 +181,7 @@ RSpec.describe ListController do
         unicode_group = Fabricate(:group, name: '群群组')
         unicode_group.add(user)
         topic = Fabricate(:private_message_topic, allowed_groups: [unicode_group])
-        get "/topics/private-messages-group/#{user.username}/#{URI.escape(unicode_group.name)}.json"
+        get "/topics/private-messages-group/#{user.username}/#{UrlHelper.encode_component(unicode_group.name)}.json"
         expect(response.status).to eq(200)
 
         expect(JSON.parse(response.body)["topic_list"]["topics"].first["id"])