Merge tag 'v3.5.4' into hometown-dev

3 years ago · f5ffda7cf3
11 changed files with 239 additions and 106 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -3,6 +3,18 @@ Changelog

 All notable changes to this project will be documented in this file.

+## [3.5.4] - 2022-11-14
+### Fixed
+
+- Fix error when a remote report includes a private post the server has no access to ([ClearlyClaire](https://github.com/mastodon/mastodon/pull/18760))
+
+### Security
+
+- Fix emoji substitution not applying only to text nodes in backend code ([ClearlyClaire](https://github.com/mastodon/mastodon/pull/20641))
+- Fix emoji substitution not applying only to text nodes in Web UI ([ClearlyClaire](https://github.com/mastodon/mastodon/pull/20640))
+- Fix rate limiting for paths with formats ([Gargron](https://github.com/mastodon/mastodon/pull/20675))
+- Fix out-of-bound reads in blurhash transcoder ([delroth](https://github.com/mastodon/mastodon/pull/20388))
+
 ## [3.5.3] - 2022-05-26
 ### Added

--- 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('<span class="invisible">', i)) {
-            // avoid emojifying on invisible text
-            invisible = 1;
-            tagChars = tagCharsWithoutEmojis;
-          }
-        }
-      }
-      i = rend;
    } else { // matched to unicode emoji
      const { filename, shortCode } = unicodeMapping[match];
      const title = shortCode ? `:${shortCode}:` : '';
@ -80,10 +63,39 @@ const emojify = (str, customEmojis = {}) => {
        rend += 1;
      }
    }
-    rtn += str.slice(0, i) + replacement;
+
+    node.textContent = str.slice(0, i);
+    parentElement.insertAdjacentHTML('beforeend', replacement);
    str = str.slice(rend);
+    node = document.createTextNode(str);
+    parentElement.append(node);
+  }
+};
+
+const emojifyNode = (node, customEmojis) => {
+  for (const child of node.childNodes) {
+    switch(child.nodeType) {
+    case Node.TEXT_NODE:
+      emojifyTextNode(child, customEmojis);
+      break;
+    case Node.ELEMENT_NODE:
+      if (!child.classList.contains('invisible'))
+        emojifyNode(child, customEmojis);
+      break;
+    }
  }
-  return rtn + str;
+};
+
+const emojify = (str, customEmojis = {}) => {
+  const wrapper = document.createElement('div');
+  wrapper.innerHTML = str;
+
+  if (!Object.keys(customEmojis).length)
+    customEmojis = null;
+
+  emojifyNode(wrapper, customEmojis);
+
+  return wrapper.innerHTML;
 };

 export default emojify;
--- a/app/lib/emoji_formatter.rb
+++ b/app/lib/emoji_formatter.rb
@ -23,48 +23,40 @@ class EmojiFormatter
  def to_s
    return html if custom_emojis.empty? || html.blank?

-    i                     = -1
-    tag_open_index        = nil
-    inside_shortname      = false
-    shortname_start_index = -1
-    invisible_depth       = 0
-    last_index            = 0
-    result                = ''.dup
-
-    while i + 1 < html.size
-      i += 1
-
-      if invisible_depth.zero? && inside_shortname && html[i] == ':'
-        inside_shortname = false
-        shortcode = html[shortname_start_index + 1..i - 1]
-        char_after = html[i + 1]
-
-        next unless (char_after.nil? || !DISALLOWED_BOUNDING_REGEX.match?(char_after)) && (emoji = emoji_map[shortcode])
-
-        result << html[last_index..shortname_start_index - 1] if shortname_start_index.positive?
-        result << image_for_emoji(shortcode, emoji)
-        last_index = i + 1
-      elsif tag_open_index && html[i] == '>'
-        tag = html[tag_open_index..i]
-        tag_open_index = nil
-
-        if invisible_depth.positive?
-          invisible_depth += count_tag_nesting(tag)
-        elsif tag == '<span class="invisible">'
-          invisible_depth = 1
+    tree = Nokogiri::HTML.fragment(html)
+    tree.xpath('./text()|.//text()[not(ancestor[@class="invisible"])]').to_a.each do |node|
+      i                     = -1
+      inside_shortname      = false
+      shortname_start_index = -1
+      last_index            = 0
+      text                  = node.content
+      result                = Nokogiri::XML::NodeSet.new(tree.document)
+
+      while i + 1 < text.size
+        i += 1
+
+        if inside_shortname && text[i] == ':'
+          inside_shortname = false
+          shortcode = text[shortname_start_index + 1..i - 1]
+          char_after = text[i + 1]
+
+          next unless (char_after.nil? || !DISALLOWED_BOUNDING_REGEX.match?(char_after)) && (emoji = emoji_map[shortcode])
+
+          result << Nokogiri::XML::Text.new(text[last_index..shortname_start_index - 1], tree.document) if shortname_start_index.positive?
+          result << Nokogiri::HTML.fragment(image_for_emoji(shortcode, emoji))
+
+          last_index = i + 1
+        elsif text[i] == ':' && (i.zero? || !DISALLOWED_BOUNDING_REGEX.match?(text[i - 1]))
+          inside_shortname = true
+          shortname_start_index = i
        end
-      elsif html[i] == '<'
-        tag_open_index = i
-        inside_shortname = false
-      elsif !tag_open_index && html[i] == ':' && (i.zero? || !DISALLOWED_BOUNDING_REGEX.match?(html[i - 1]))
-        inside_shortname = true
-        shortname_start_index = i
      end
-    end

-    result << html[last_index..-1]
+      result << Nokogiri::XML::Text.new(text[last_index..-1], tree.document)
+      node.replace(result)
+    end

-    result.html_safe # rubocop:disable Rails/OutputSafety
+    tree.to_html.html_safe # rubocop:disable Rails/OutputSafety
  end

  private
--- 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
--- a/chart/values.yaml
+++ b/chart/values.yaml
@ -8,7 +8,7 @@ image:
  # built from the most recent commit
  #
  # tag: latest
-  tag: v3.5.2
+  tag: v3.5.4
  # use `Always` when using `latest` tag
  pullPolicy: IfNotPresent

--- 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|
--- 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

--- a/lib/mastodon/version.rb
+++ b/lib/mastodon/version.rb
@ -21,7 +21,7 @@ module Mastodon
    end

    def suffix
-      '+3.5.3'
+      '+3.5.4'
    end

    def to_a
--- 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] || {}))
--- 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

--- 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) }