From 47eaf85f02e280db8c24cfc4f9bc5a34e99da49e Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 4 Jul 2022 11:08:30 +0200 Subject: [PATCH 1/8] Fix crash when a remote Flag activity mentions a private post (#18760) * Add tests * Fix crash when a remote Flag activity mentions a private post --- app/services/report_service.rb | 11 ++- spec/lib/activitypub/activity/flag_spec.rb | 88 ++++++++++++++++++++-- spec/services/report_service_spec.rb | 25 ++++++ 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/app/services/report_service.rb b/app/services/report_service.rb index d251bb33f..59fd78604 100644 --- a/app/services/report_service.rb +++ b/app/services/report_service.rb @@ -57,7 +57,16 @@ class ReportService < BaseService end def reported_status_ids - AccountStatusesFilter.new(@target_account, @source_account).results.with_discarded.find(Array(@status_ids)).pluck(:id) + return AccountStatusesFilter.new(@target_account, @source_account).results.with_discarded.find(Array(@status_ids)).pluck(:id) if @source_account.local? + + # If the account making reports is remote, it is likely anonymized so we have to relax the requirements for attaching statuses. + domain = @source_account.domain.to_s.downcase + has_followers = @target_account.followers.where(Account.arel_table[:domain].lower.eq(domain)).exists? + visibility = has_followers ? %i(public unlisted private) : %i(public unlisted) + scope = @target_account.statuses.with_discarded + scope.merge!(scope.where(visibility: visibility).or(scope.where('EXISTS (SELECT 1 FROM mentions m JOIN accounts a ON m.account_id = a.id WHERE lower(a.domain) = ?)', domain))) + # Allow missing posts to not drop reports that include e.g. a deleted post + scope.where(id: Array(@status_ids)).pluck(:id) end def payload diff --git a/spec/lib/activitypub/activity/flag_spec.rb b/spec/lib/activitypub/activity/flag_spec.rb index ec7359f2f..2f2d13876 100644 --- a/spec/lib/activitypub/activity/flag_spec.rb +++ b/spec/lib/activitypub/activity/flag_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Flag do - let(:sender) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/account') } + let(:sender) { Fabricate(:account, username: 'example.com', domain: 'example.com', uri: 'http://example.com/actor') } let(:flagged) { Fabricate(:account) } let(:status) { Fabricate(:status, account: flagged, uri: 'foobar') } let(:flag_id) { nil } @@ -23,16 +23,88 @@ RSpec.describe ActivityPub::Activity::Flag do describe '#perform' do subject { described_class.new(json, sender) } - before do - subject.perform + context 'when the reported status is public' do + before do + subject.perform + end + + it 'creates a report' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end end - it 'creates a report' do - report = Report.find_by(account: sender, target_account: flagged) + context 'when the reported status is private and should not be visible to the remote server' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } - expect(report).to_not be_nil - expect(report.comment).to eq 'Boo!!' - expect(report.status_ids).to eq [status.id] + before do + subject.perform + end + + it 'creates a report with no attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [] + end + end + + context 'when the reported status is private and the author has a follower on the remote instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:follower) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/users/account') } + + before do + follower.follow!(flagged) + subject.perform + end + + it 'creates a report with the attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end + end + + context 'when the reported status is private and the author mentions someone else on the remote instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:mentioned) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/users/account') } + + before do + status.mentions.create(account: mentioned) + subject.perform + end + + it 'creates a report with the attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end + end + + context 'when the reported status is private and the author mentions someone else on the local instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:mentioned) { Fabricate(:account) } + + before do + status.mentions.create(account: mentioned) + subject.perform + end + + it 'creates a report with no attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [] + end end end diff --git a/spec/services/report_service_spec.rb b/spec/services/report_service_spec.rb index 7e6a113e0..ea68b3344 100644 --- a/spec/services/report_service_spec.rb +++ b/spec/services/report_service_spec.rb @@ -28,6 +28,31 @@ RSpec.describe ReportService, type: :service do end end + context 'when the reported status is a DM' do + let(:target_account) { Fabricate(:account) } + let(:status) { Fabricate(:status, account: target_account, visibility: :direct) } + + subject do + -> { described_class.new.call(source_account, target_account, status_ids: [status.id]) } + end + + context 'when it is addressed to the reporter' do + before do + status.mentions.create(account: source_account) + end + + it 'creates a report' do + is_expected.to change { target_account.targeted_reports.count }.from(0).to(1) + end + end + + context 'when it is not addressed to the reporter' do + it 'errors out' do + is_expected.to raise_error + end + end + end + context 'when other reports already exist for the same target' do let!(:target_account) { Fabricate(:account) } let!(:other_report) { Fabricate(:report, target_account: target_account) } From 1659788de4aa12f78108defb7294a1a23fa363bf Mon Sep 17 00:00:00 2001 From: Pierre Bourdon Date: Fri, 11 Nov 2022 07:45:16 +0100 Subject: [PATCH 2/8] blurhash_transcoder: prevent out-of-bound reads with <8bpp images (#20388) The Blurhash library used by Mastodon requires an input encoded as 24 bits raw RGB data. The conversion to raw RGB using Imagemagick did not previously specify the desired bit depth. In some situations, this leads Imagemagick to output in a pixel format using less bpp than expected. This then manifested as segfaults of the Sidekiq process due to out-of-bounds read, or potentially a (highly noisy) memory infoleak. Fixes #19235. --- lib/paperclip/blurhash_transcoder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/paperclip/blurhash_transcoder.rb b/lib/paperclip/blurhash_transcoder.rb index 1c3a6df02..c22c20c57 100644 --- a/lib/paperclip/blurhash_transcoder.rb +++ b/lib/paperclip/blurhash_transcoder.rb @@ -5,7 +5,7 @@ module Paperclip def make return @file unless options[:style] == :small || options[:blurhash] - pixels = convert(':source RGB:-', source: File.expand_path(@file.path)).unpack('C*') + pixels = convert(':source -depth 8 RGB:-', source: File.expand_path(@file.path)).unpack('C*') geometry = options.fetch(:file_geometry_parser).from_file(@file) attachment.instance.blurhash = Blurhash.encode(geometry.width, geometry.height, pixels, **(options[:blurhash] || {})) From 063579373e401fdac72b856971ecc01b06065365 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 26 Oct 2022 14:58:52 +0200 Subject: [PATCH 3/8] Fix rate limiting for paths with formats --- config/initializers/rack_attack.rb | 49 ++++++++++++++++++------------ config/routes.rb | 6 ++-- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 745eb5d3b..72ef7ba80 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -17,6 +17,18 @@ class Rack::Attack @remote_ip ||= (@env["action_dispatch.remote_ip"] || ip).to_s end + def throttleable_remote_ip + @throttleable_remote_ip ||= begin + ip = IPAddr.new(remote_ip) + + if ip.ipv6? + ip.mask(64) + else + ip + end + end.to_s + end + def authenticated_user_id authenticated_token&.resource_owner_id end @@ -29,6 +41,10 @@ class Rack::Attack path.start_with?('/api') end + def path_matches?(other_path) + /\A#{Regexp.escape(other_path)}(\..*)?\z/ =~ path + end + def web_request? !api_request? end @@ -51,19 +67,19 @@ class Rack::Attack end throttle('throttle_unauthenticated_api', limit: 300, period: 5.minutes) do |req| - req.remote_ip if req.api_request? && req.unauthenticated? + req.throttleable_remote_ip if req.api_request? && req.unauthenticated? end throttle('throttle_api_media', limit: 30, period: 30.minutes) do |req| - req.authenticated_user_id if req.post? && req.path.match?('^/api/v\d+/media') + req.authenticated_user_id if req.post? && req.path.match?(/\A\/api\/v\d+\/media\z/i) end throttle('throttle_media_proxy', limit: 30, period: 10.minutes) do |req| - req.remote_ip if req.path.start_with?('/media_proxy') + req.throttleable_remote_ip if req.path.start_with?('/media_proxy') end throttle('throttle_api_sign_up', limit: 5, period: 30.minutes) do |req| - req.remote_ip if req.post? && req.path == '/api/v1/accounts' + req.throttleable_remote_ip if req.post? && req.path == '/api/v1/accounts' end throttle('throttle_authenticated_paging', limit: 300, period: 15.minutes) do |req| @@ -71,39 +87,34 @@ class Rack::Attack end throttle('throttle_unauthenticated_paging', limit: 300, period: 15.minutes) do |req| - req.remote_ip if req.paging_request? && req.unauthenticated? + req.throttleable_remote_ip if req.paging_request? && req.unauthenticated? end - API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog/.freeze - API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+/.freeze + API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog\z/.freeze + API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+\z/.freeze throttle('throttle_api_delete', limit: 30, period: 30.minutes) do |req| req.authenticated_user_id if (req.post? && req.path.match?(API_DELETE_REBLOG_REGEX)) || (req.delete? && req.path.match?(API_DELETE_STATUS_REGEX)) end throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| - if req.post? && req.path == '/auth' - addr = req.remote_ip - addr = IPAddr.new(addr) if addr.is_a?(String) - addr = addr.mask(64) if addr.ipv6? - addr.to_s - end + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth') end throttle('throttle_password_resets/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path == '/auth/password' + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/password') end throttle('throttle_password_resets/email', limit: 5, period: 30.minutes) do |req| - req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/password') end throttle('throttle_email_confirmations/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && %w(/auth/confirmation /api/v1/emails/confirmations).include?(req.path) + req.throttleable_remote_ip if req.post? && (req.path_matches?('/auth/confirmation') || req.path == '/api/v1/emails/confirmations') end throttle('throttle_email_confirmations/email', limit: 5, period: 30.minutes) do |req| - if req.post? && req.path == '/auth/password' + if req.post? && req.path_matches?('/auth/password') req.params.dig('user', 'email').presence elsif req.post? && req.path == '/api/v1/emails/confirmations' req.authenticated_user_id @@ -111,11 +122,11 @@ class Rack::Attack end throttle('throttle_login_attempts/ip', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path == '/auth/sign_in' + req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/sign_in') end throttle('throttle_login_attempts/email', limit: 25, period: 1.hour) do |req| - req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in' + req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/sign_in') end self.throttled_responder = lambda do |request| diff --git a/config/routes.rb b/config/routes.rb index b05d31e4e..ce467f447 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -47,7 +47,7 @@ Rails.application.routes.draw do end end - devise_for :users, path: 'auth', controllers: { + devise_for :users, path: 'auth', format: false, controllers: { omniauth_callbacks: 'auth/omniauth_callbacks', sessions: 'auth/sessions', registrations: 'auth/registrations', @@ -182,7 +182,7 @@ Rails.application.routes.draw do resource :statuses_cleanup, controller: :statuses_cleanup, only: [:show, :update] get '/public', to: 'public_timelines#show', as: :public_timeline - get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy + get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy, format: false resource :authorize_interaction, only: [:show, :create] resource :share, only: [:show, :create] @@ -353,7 +353,7 @@ Rails.application.routes.draw do get '/admin', to: redirect('/admin/dashboard', status: 302) - namespace :api do + namespace :api, format: false do # OEmbed get '/oembed', to: 'oembed#show', as: :oembed From 2db06e1d089404844b632b3a2164c4bd3af24424 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 9 Nov 2022 14:16:02 +0100 Subject: [PATCH 4/8] Fix emoji substitution not applying only to text nodes in Web UI Signed-off-by: Claire --- .../mastodon/features/emoji/emoji.js | 80 +++++++++++-------- 1 file changed, 46 insertions(+), 34 deletions(-) diff --git a/app/javascript/mastodon/features/emoji/emoji.js b/app/javascript/mastodon/features/emoji/emoji.js index fb1a3804c..0ab32767a 100644 --- a/app/javascript/mastodon/features/emoji/emoji.js +++ b/app/javascript/mastodon/features/emoji/emoji.js @@ -19,15 +19,23 @@ const emojiFilename = (filename) => { return borderedEmoji.includes(filename) ? (filename + '_border') : filename; }; -const emojify = (str, customEmojis = {}) => { - const tagCharsWithoutEmojis = '<&'; - const tagCharsWithEmojis = Object.keys(customEmojis).length ? '<&:' : '<&'; - let rtn = '', tagChars = tagCharsWithEmojis, invisible = 0; +const emojifyTextNode = (node, customEmojis) => { + const parentElement = node.parentElement; + let str = node.textContent; + for (;;) { - let match, i = 0, tag; - while (i < str.length && (tag = tagChars.indexOf(str[i])) === -1 && (invisible || !(match = trie.search(str.slice(i))))) { - i += str.codePointAt(i) < 65536 ? 1 : 2; + let match, i = 0; + + if (customEmojis === null) { + while (i < str.length && !(match = trie.search(str.slice(i)))) { + i += str.codePointAt(i) < 65536 ? 1 : 2; + } + } else { + while (i < str.length && str[i] !== ':' && !(match = trie.search(str.slice(i)))) { + i += str.codePointAt(i) < 65536 ? 1 : 2; + } } + let rend, replacement = ''; if (i === str.length) { break; @@ -35,8 +43,6 @@ const emojify = (str, customEmojis = {}) => { if (!(() => { rend = str.indexOf(':', i + 1) + 1; if (!rend) return false; // no pair of ':' - const lt = str.indexOf('<', i + 1); - if (!(lt === -1 || lt >= rend)) return false; // tag appeared before closing ':' const shortname = str.slice(i, rend); // now got a replacee as ':shortname:' // if you want additional emoji handler, add statements below which set replacement and return true. @@ -47,29 +53,6 @@ const emojify = (str, customEmojis = {}) => { } return false; })()) rend = ++i; - } else if (tag >= 0) { // <, & - rend = str.indexOf('>;'[tag], i + 1) + 1; - if (!rend) { - break; - } - if (tag === 0) { - if (invisible) { - if (str[i + 1] === '/') { // closing tag - if (!--invisible) { - tagChars = tagCharsWithEmojis; - } - } else if (str[rend - 2] !== '/') { // opening tag - invisible++; - } - } else { - if (str.startsWith('