From edbc3565933819a9ebe4e80d567c35f5dca061a2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Dec 2019 12:49:21 +1000 Subject: [PATCH] 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 | 1 + Gemfile.lock | 1 + app/controllers/list_controller.rb | 4 ++-- app/helpers/application_helper.rb | 3 +-- app/jobs/regular/download_backup_email.rb | 2 +- app/jobs/regular/update_username.rb | 2 +- app/mailers/user_notifications.rb | 4 ++-- app/models/concerns/has_url.rb | 2 +- app/models/embeddable_host.rb | 2 +- app/models/post.rb | 2 +- app/models/topic.rb | 2 +- app/models/user.rb | 4 ++-- lib/final_destination.rb | 5 +--- lib/url_helper.rb | 28 +++++++++++++++++------ lib/validators/url_validator.rb | 2 +- script/import_scripts/ipboard.rb | 2 +- spec/requests/list_controller_spec.rb | 2 +- 17 files changed, 40 insertions(+), 28 deletions(-) diff --git a/Gemfile b/Gemfile index cbde5ec5b4a..52da8538fe1 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 4367c2db141..795b54e18f5 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 40c42fab07d..7ee85c7579b 100644 --- a/app/controllers/list_controller.rb +++ b/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) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4a5bf248e82..e664fa0cdaa 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -383,8 +383,7 @@ module ApplicationHelper def topic_featured_link_domain(link) begin - uri = URI.encode(link) - uri = URI.parse(uri) + uri = UrlHelper.encode_and_parse(link) uri = URI.parse("http://#{uri}") if uri.scheme.nil? host = uri.host.downcase host.start_with?('www.') ? host[4..-1] : host diff --git a/app/jobs/regular/download_backup_email.rb b/app/jobs/regular/download_backup_email.rb index 0977098e974..44e13495f29 100644 --- a/app/jobs/regular/download_backup_email.rb +++ b/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 diff --git a/app/jobs/regular/update_username.rb b/app/jobs/regular/update_username.rb index 3295fd08004..26657124e34 100644 --- a/app/jobs/regular/update_username.rb +++ b/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 diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 0be2f2b3e12..d91ee570ec1 100644 --- a/app/mailers/user_notifications.rb +++ b/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 } diff --git a/app/models/concerns/has_url.rb b/app/models/concerns/has_url.rb index 4e38d755f0e..300dafc11ca 100644 --- a/app/models/concerns/has_url.rb +++ b/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 diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index 8944a4fcd72..32781522c3e 100644 --- a/app/models/embeddable_host.rb +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index 73acc182d5d..67e30df4c1a 100644 --- a/app/models/post.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index 113d2fe5c31..7c3c99e0421 100644 --- a/app/models/topic.rb +++ b/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) diff --git a/app/models/user.rb b/app/models/user.rb index 44dbfa7b1fe..b5247b85fdb 100644 --- a/app/models/user.rb +++ b/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! "{first_letter}", CGI.escape(normalized_username.grapheme_clusters.first) + url.gsub! "{username}", UrlHelper.encode_component(username) + url.gsub! "{first_letter}", UrlHelper.encode_component(normalized_username.grapheme_clusters.first) url.gsub! "{hostname}", Discourse.current_hostname url else diff --git a/lib/final_destination.rb b/lib/final_destination.rb index 4177800ae64..02930a71c8f 100644 --- a/lib/final_destination.rb +++ b/lib/final_destination.rb @@ -315,10 +315,7 @@ class FinalDestination end def escape_url - UrlHelper.escape_uri( - CGI.unescapeHTML(@url), - Regexp.new("[^#{URI::PATTERN::UNRESERVED}#{URI::PATTERN::RESERVED}#]") - ) + UrlHelper.escape_uri(@url) end def private_ranges diff --git a/lib/url_helper.rb b/lib/url_helper.rb index 420420d3f0b..51e448e8566 100644 --- a/lib/url_helper.rb +++ b/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) - encoded = URI.encode(uri, pattern) - encoded.gsub!(DOUBLE_ESCAPED_REGEXP, '%\1') - encoded + def self.escape_uri(uri) + UrlHelper.encode_component(CGI.unescapeHTML(UrlHelper.unencode(uri))) end def self.cook_url(url, secure: false) diff --git a/lib/validators/url_validator.rb b/lib/validators/url_validator.rb index 4e56f81f264..be60a57354e 100644 --- a/lib/validators/url_validator.rb +++ b/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 diff --git a/script/import_scripts/ipboard.rb b/script/import_scripts/ipboard.rb index 9f88d169862..b849f75a47c 100644 --- a/script/import_scripts/ipboard.rb +++ b/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 diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 788ce3b738a..3897f03abcb 100644 --- a/spec/requests/list_controller_spec.rb +++ b/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"])