From 746ba0d8fda60d4b1f2d8d155b3fae401802e331 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 12 Dec 2019 13:15:40 +1100 Subject: [PATCH 1/5] SECURITY: upgrade rack-mini-profiler to avoid possible XSS (#8537) --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index e10257d4396..6b19bd13f3f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -273,7 +273,7 @@ GEM nio4r (~> 2.0) r2 (0.2.7) rack (2.0.7) - rack-mini-profiler (1.1.3) + rack-mini-profiler (1.1.4) rack (>= 1.2.0) rack-openid (1.3.1) rack (>= 1.1.0) From 1e59371a4f5443984353b312da5c9a03e5648287 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 12 Dec 2019 02:27:23 +0000 Subject: [PATCH 2/5] DEV: Remove unused omit_stats variable from user serializer (#8513) * DEV: Remove unused omit_stats variable from user serializer This was hard-coded to true in a8b5192efd43bcbf64ffe7ae44e5c5b7f3be0b51, and is no longer used anywhere * Remove attribute declarations --- app/controllers/users_controller.rb | 2 -- app/serializers/user_serializer.rb | 21 +-------------------- app/serializers/web_hook_user_serializer.rb | 4 ---- 3 files changed, 1 insertion(+), 26 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index da3a23dfe80..c08952c59af 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -60,8 +60,6 @@ class UsersController < ApplicationController user_serializer = nil if guardian.can_see_profile?(@user) user_serializer = UserSerializer.new(@user, scope: guardian, root: 'user') - # TODO remove this options from serializer - user_serializer.omit_stats = true topic_id = params[:include_post_count_for].to_i if topic_id != 0 diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index cb7bc9971e4..43bbc569768 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -2,8 +2,7 @@ class UserSerializer < BasicUserSerializer - attr_accessor :omit_stats, - :topic_post_count + attr_accessor :topic_post_count def self.staff_attributes(*attrs) attributes(*attrs) @@ -48,7 +47,6 @@ class UserSerializer < BasicUserSerializer :can_edit_username, :can_edit_email, :can_edit_name, - :stats, :ignored, :muted, :can_ignore_user, @@ -109,7 +107,6 @@ class UserSerializer < BasicUserSerializer :tracked_category_ids, :watched_category_ids, :watched_first_post_category_ids, - :private_messages_stats, :system_avatar_upload_id, :system_avatar_template, :gravatar_avatar_upload_id, @@ -257,14 +254,6 @@ class UserSerializer < BasicUserSerializer scope.can_edit_name?(object) end - def include_stats? - !omit_stats == true - end - - def stats - UserAction.stats(object.id, scope) - end - def ignored IgnoredUser.where(user_id: scope.user&.id, ignored_user_id: object.id).exists? end @@ -378,14 +367,6 @@ class UserSerializer < BasicUserSerializer IgnoredUser.where(user_id: object.id).joins(:ignored_user).pluck(:username) end - def include_private_messages_stats? - can_edit && !(omit_stats == true) - end - - def private_messages_stats - UserAction.private_messages_stats(object.id, scope) - end - def system_avatar_upload_id # should be left blank end diff --git a/app/serializers/web_hook_user_serializer.rb b/app/serializers/web_hook_user_serializer.rb index b14f75a2647..4fd63a8abe6 100644 --- a/app/serializers/web_hook_user_serializer.rb +++ b/app/serializers/web_hook_user_serializer.rb @@ -3,10 +3,6 @@ class WebHookUserSerializer < UserSerializer attributes :external_id - def omit_stats - true - end - # remove staff attributes def staff_attributes(*attrs) end From 61ac0d47eea5f2621972180e5f1abe85fcf0d957 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Thu, 12 Dec 2019 13:29:00 +1100 Subject: [PATCH 3/5] DEV: Bump stackprof from 0.2.13 to 0.2.14 (#8531) Bumps [stackprof](https://github.com/tmm1/stackprof) from 0.2.13 to 0.2.14. - [Release notes](https://github.com/tmm1/stackprof/releases) - [Changelog](https://github.com/tmm1/stackprof/blob/master/CHANGELOG.md) - [Commits](https://github.com/tmm1/stackprof/compare/v0.2.13...v0.2.14) Minor upgrade to stackprof which is only used for diagnostics and not default required. Changes all look safe. --- Gemfile.lock | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6b19bd13f3f..da88bab73ff 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -45,7 +45,6 @@ GEM rake (>= 10.4, < 14.0) ast (2.4.0) aws-eventstream (1.0.3) - aws-partitions (1.252.0) aws-sdk-core (3.85.0) aws-eventstream (~> 1.0, >= 1.0.2) @@ -399,7 +398,7 @@ GEM activesupport (>= 4.0) sprockets (>= 3.0.0) sshkey (2.0.0) - stackprof (0.2.13) + stackprof (0.2.14) test-prof (0.10.1) thor (0.20.3) thread_safe (0.3.6) From b6acfb78476a7603e2a7ffb93e6f2aa157041d3e Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Thu, 12 Dec 2019 13:35:43 +1100 Subject: [PATCH 4/5] DEV: upgrade redis-namespace gem New release has a few extra commands namespaced, nothing we use. Also added a comment about why this is explicitly required. --- Gemfile | 7 +++++++ Gemfile.lock | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 67c472649e3..cbde5ec5b4a 100644 --- a/Gemfile +++ b/Gemfile @@ -43,6 +43,13 @@ gem 'mini_mime' gem 'mini_suffix' gem 'redis' + +# This is explicitly used by Sidekiq and is an optional dependency. +# We tell Sidekiq to use the namespace "sidekiq" which triggers this +# gem to be used. There is no explicit dependency in sidekiq cause +# redis namespace support is optional +# We already namespace stuff in DiscourseRedis, so we should consider +# just using a single implementation in core vs having 2 namespace implementations gem 'redis-namespace' # NOTE: AM serializer gets a lot slower with recent updates diff --git a/Gemfile.lock b/Gemfile.lock index da88bab73ff..4367c2db141 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -309,7 +309,7 @@ GEM optimist (>= 3.0.0) rchardet (1.8.0) redis (4.1.3) - redis-namespace (1.6.0) + redis-namespace (1.7.0) redis (>= 3.0.4) request_store (1.4.1) rack (>= 1.4) From edbc3565933819a9ebe4e80d567c35f5dca061a2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Dec 2019 12:49:21 +1000 Subject: [PATCH 5/5] 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"])