diff --git a/.circleci/config.yml b/.circleci/config.yml index b05273f8b..968de1eb2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -19,7 +19,7 @@ executors: DB_USER: root DISABLE_SIMPLECOV: true RAILS_ENV: test - - image: cimg/postgres:12.7 + - image: cimg/postgres:14.0 environment: POSTGRES_USER: root POSTGRES_HOST_AUTH_METHOD: trust diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index 271e428c3..535d04fbe 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -11,7 +11,6 @@ class Auth::PasswordsController < Devise::PasswordsController super do |resource| if resource.errors.empty? resource.session_activations.destroy_all - resource.forget_me! end end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index a3114ab25..3c1730f25 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Auth::RegistrationsController < Devise::RegistrationsController - include Devise::Controllers::Rememberable include RegistrationSpamConcern layout :determine_layout @@ -30,8 +29,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController super do |resource| if resource.saved_change_to_encrypted_password? resource.clear_other_sessions(current_session.session_id) - resource.forget_me! - remember_me(resource) end end end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index d48abb707..0184bfb52 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class Auth::SessionsController < Devise::SessionsController - include Devise::Controllers::Rememberable - layout 'auth' skip_before_action :require_no_authentication, only: [:create] @@ -150,7 +148,6 @@ class Auth::SessionsController < Devise::SessionsController clear_attempt_from_session user.update_sign_in!(request, new_sign_in: true) - remember_me(user) sign_in(user) flash.delete(:notice) diff --git a/app/lib/link_details_extractor.rb b/app/lib/link_details_extractor.rb new file mode 100644 index 000000000..9df8a1320 --- /dev/null +++ b/app/lib/link_details_extractor.rb @@ -0,0 +1,200 @@ +# frozen_string_literal: true + +class LinkDetailsExtractor + include ActionView::Helpers::TagHelper + + class StructuredData + def initialize(data) + @data = data + end + + def headline + json['headline'] + end + + def description + json['description'] + end + + def image + obj = first_of_value(json['image']) + + return obj['url'] if obj.is_a?(Hash) + + obj + end + + def date_published + json['datePublished'] + end + + def date_modified + json['dateModified'] + end + + def author_name + author['name'] + end + + def author_url + author['url'] + end + + def publisher_name + publisher['name'] + end + + private + + def author + first_of_value(json['author']) || {} + end + + def publisher + first_of_value(json['publisher']) || {} + end + + def first_of_value(arr) + arr.is_a?(Array) ? arr.first : arr + end + + def json + @json ||= Oj.load(@data) + end + end + + def initialize(original_url, html, html_charset) + @original_url = Addressable::URI.parse(original_url) + @html = html + @html_charset = html_charset + end + + def to_preview_card_attributes + { + title: title || '', + description: description || '', + image_remote_url: image, + type: type, + width: width || 0, + height: height || 0, + html: html || '', + provider_name: provider_name || '', + provider_url: provider_url || '', + author_name: author_name || '', + author_url: author_url || '', + embed_url: embed_url || '', + } + end + + def type + player_url.present? ? :video : :link + end + + def html + player_url.present? ? content_tag(:iframe, src: player_url, width: width, height: height, allowtransparency: 'true', scrolling: 'no', frameborder: '0') : nil + end + + def width + opengraph_tag('twitter:player:width') + end + + def height + opengraph_tag('twitter:player:height') + end + + def title + structured_data&.headline || opengraph_tag('og:title') || document.xpath('//title').map(&:content).first + end + + def description + structured_data&.description || opengraph_tag('og:description') || meta_tag('description') + end + + def image + valid_url_or_nil(opengraph_tag('og:image')) + end + + def canonical_url + valid_url_or_nil(opengraph_tag('og:url') || link_tag('canonical'), same_origin_only: true) || @original_url.to_s + end + + def provider_name + structured_data&.publisher_name || opengraph_tag('og:site_name') + end + + def provider_url + valid_url_or_nil(host_to_url(opengraph_tag('og:site'))) + end + + def author_name + structured_data&.author_name || opengraph_tag('og:author') || opengraph_tag('og:author:username') + end + + def author_url + structured_data&.author_url + end + + def embed_url + valid_url_or_nil(opengraph_tag('twitter:player:stream')) + end + + private + + def player_url + valid_url_or_nil(opengraph_tag('twitter:player')) + end + + def host_to_url(str) + return if str.blank? + + str.start_with?(/https?:\/\//) ? str : "http://#{str}" + end + + def valid_url_or_nil(str, same_origin_only: false) + return if str.blank? + + url = @original_url + Addressable::URI.parse(str) + + return if url.host.blank? || !%w(http https).include?(url.scheme) || (same_origin_only && url.host != @original_url.host) + + url.to_s + rescue Addressable::URI::InvalidURIError + nil + end + + def link_tag(name) + document.xpath("//link[@rel=\"#{name}\"]").map { |link| link['href'] }.first + end + + def opengraph_tag(name) + document.xpath("//meta[@property=\"#{name}\" or @name=\"#{name}\"]").map { |meta| meta['content'] }.first + end + + def meta_tag(name) + document.xpath("//meta[@name=\"#{name}\"]").map { |meta| meta['content'] }.first + end + + def structured_data + @structured_data ||= begin + json_ld = document.xpath('//script[@type="application/ld+json"]').map(&:content).first + json_ld.present? ? StructuredData.new(json_ld) : nil + end + end + + def document + @document ||= Nokogiri::HTML(@html, nil, encoding) + end + + def encoding + @encoding ||= begin + guess = detector.detect(@html, @html_charset) + guess&.fetch(:confidence, 0).to_i > 60 ? guess&.fetch(:encoding, nil) : nil + end + end + + def detector + @detector ||= CharlockHolmes::EncodingDetector.new.tap do |detector| + detector.strip_tags = true + end + end +end diff --git a/app/lib/request.rb b/app/lib/request.rb index 125dee3ea..4289da933 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -94,7 +94,7 @@ class Request end def http_client - HTTP.use(:auto_inflate).timeout(TIMEOUT.dup).follow(max_hops: 2) + HTTP.use(:auto_inflate).timeout(TIMEOUT.dup).follow(max_hops: 3) end end diff --git a/app/models/account_note.rb b/app/models/account_note.rb index bf61df923..b338bc92f 100644 --- a/app/models/account_note.rb +++ b/app/models/account_note.rb @@ -17,4 +17,5 @@ class AccountNote < ApplicationRecord belongs_to :target_account, class_name: 'Account' validates :account_id, uniqueness: { scope: :target_account_id } + validates :comment, length: { maximum: 2_000 } end diff --git a/app/models/status.rb b/app/models/status.rb index 3acf759f9..c7f761bc6 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -338,7 +338,7 @@ class Status < ApplicationRecord def from_text(text) return [] if text.blank? - text.scan(FetchLinkCardService::URL_PATTERN).map(&:first).uniq.filter_map do |url| + text.scan(FetchLinkCardService::URL_PATTERN).map(&:second).uniq.filter_map do |url| status = begin if TagManager.instance.local_url?(url) ActivityPub::TagManager.instance.uri_to_resource(url, Status) diff --git a/app/models/user.rb b/app/models/user.rb index 4059c96b5..c4dec4813 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -64,7 +64,7 @@ class User < ApplicationRecord devise :two_factor_backupable, otp_number_of_backup_codes: 10 - devise :registerable, :recoverable, :rememberable, :validatable, + devise :registerable, :recoverable, :validatable, :confirmable include Omniauthable diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 5732ce8ac..51956ce7e 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -13,12 +13,12 @@ class FetchLinkCardService < BaseService }iox def call(status) - @status = status - @url = parse_urls + @status = status + @original_url = parse_urls - return if @url.nil? || @status.preview_cards.any? + return if @original_url.nil? || @status.preview_cards.any? - @url = @url.to_s + @url = @original_url.to_s RedisLock.acquire(lock_options) do |lock| if lock.acquired? @@ -31,7 +31,7 @@ class FetchLinkCardService < BaseService attach_card if @card&.persisted? rescue HTTP::Error, OpenSSL::SSL::SSLError, Addressable::URI::InvalidURIError, Mastodon::HostValidationError, Mastodon::LengthValidationError => e - Rails.logger.debug "Error fetching link #{@url}: #{e}" + Rails.logger.debug "Error fetching link #{@original_url}: #{e}" nil end @@ -47,6 +47,12 @@ class FetchLinkCardService < BaseService return @html if defined?(@html) Request.new(:get, @url).add_headers('Accept' => 'text/html', 'User-Agent' => Mastodon::Version.user_agent + ' Bot').perform do |res| + # We follow redirects, and ideally we want to save the preview card for + # the destination URL and not any link shortener in-between, so here + # we set the URL to the one of the last response in the redirect chain + @url = res.request.uri.to_s.to_s + @card = PreviewCard.find_or_initialize_by(url: @url) if @card.url != @url + if res.code == 200 && res.mime_type == 'text/html' @html_charset = res.charset @html = res.body_with_limit @@ -63,12 +69,15 @@ class FetchLinkCardService < BaseService end def parse_urls - if @status.local? - urls = @status.text.scan(URL_PATTERN).map { |array| Addressable::URI.parse(array[1]).normalize } - else - html = Nokogiri::HTML(@status.text) - links = html.css('a') - urls = links.filter_map { |a| Addressable::URI.parse(a['href']) unless skip_link?(a) }.filter_map(&:normalize) + urls = begin + if @status.local? + @status.text.scan(URL_PATTERN).map { |array| Addressable::URI.parse(array[1]).normalize } + else + document = Nokogiri::HTML(@status.text) + links = document.css('a') + + links.filter_map { |a| Addressable::URI.parse(a['href']) unless skip_link?(a) }.filter_map(&:normalize) + end end urls.reject { |uri| bad_url?(uri) }.first @@ -79,18 +88,16 @@ class FetchLinkCardService < BaseService uri.host.blank? || TagManager.instance.local_url?(uri.to_s) || !%w(http https).include?(uri.scheme) end - # rubocop:disable Naming/MethodParameterName - def mention_link?(a) + def mention_link?(anchor) @status.mentions.any? do |mention| - a['href'] == ActivityPub::TagManager.instance.url_for(mention.account) + anchor['href'] == ActivityPub::TagManager.instance.url_for(mention.account) end end - def skip_link?(a) + def skip_link?(anchor) # Avoid links for hashtags and mentions (microformats) - a['rel']&.include?('tag') || a['class']&.match?(/u-url|h-card/) || mention_link?(a) + anchor['rel']&.include?('tag') || anchor['class']&.match?(/u-url|h-card/) || mention_link?(anchor) end - # rubocop:enable Naming/MethodParameterName def attempt_oembed service = FetchOEmbedService.new @@ -139,42 +146,14 @@ class FetchLinkCardService < BaseService def attempt_opengraph return if html.nil? - detector = CharlockHolmes::EncodingDetector.new - detector.strip_tags = true + link_details_extractor = LinkDetailsExtractor.new(@url, @html, @html_charset) - guess = detector.detect(@html, @html_charset) - encoding = guess&.fetch(:confidence, 0).to_i > 60 ? guess&.fetch(:encoding, nil) : nil - page = Nokogiri::HTML(@html, nil, encoding) - player_url = meta_property(page, 'twitter:player') - - if player_url && !bad_url?(Addressable::URI.parse(player_url)) - @card.type = :video - @card.width = meta_property(page, 'twitter:player:width') || 0 - @card.height = meta_property(page, 'twitter:player:height') || 0 - @card.html = content_tag(:iframe, nil, src: player_url, - width: @card.width, - height: @card.height, - allowtransparency: 'true', - scrolling: 'no', - frameborder: '0') - else - @card.type = :link - end - - @card.title = meta_property(page, 'og:title').presence || page.at_xpath('//title')&.content || '' - @card.description = meta_property(page, 'og:description').presence || meta_property(page, 'description') || '' - @card.image_remote_url = (Addressable::URI.parse(@url) + meta_property(page, 'og:image')).to_s if meta_property(page, 'og:image') - - return if @card.title.blank? && @card.html.blank? - - @card.save_with_optional_image! - end - - def meta_property(page, property) - page.at_xpath("//meta[contains(concat(' ', normalize-space(@property), ' '), ' #{property} ')]")&.attribute('content')&.value || page.at_xpath("//meta[@name=\"#{property}\"]")&.attribute('content')&.value + @card = PreviewCard.find_or_initialize_by(url: link_details_extractor.canonical_url) if link_details_extractor.canonical_url != @card.url + @card.assign_attributes(link_details_extractor.to_preview_card_attributes) + @card.save_with_optional_image! unless @card.title.blank? && @card.html.blank? end def lock_options - { redis: Redis.current, key: "fetch:#{@url}", autorelease: 15.minutes.seconds } + { redis: Redis.current, key: "fetch:#{@original_url}", autorelease: 15.minutes.seconds } end end diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb index cc2c17d32..4a900e3b8 100644 --- a/app/workers/move_worker.rb +++ b/app/workers/move_worker.rb @@ -53,10 +53,16 @@ class MoveWorker new_note = AccountNote.find_by(account: note.account, target_account: @target_account) if new_note.nil? - AccountNote.create!(account: note.account, target_account: @target_account, comment: [text, note.comment].join("\n")) + begin + AccountNote.create!(account: note.account, target_account: @target_account, comment: [text, note.comment].join("\n")) + rescue ActiveRecord::RecordInvalid + AccountNote.create!(account: note.account, target_account: @target_account, comment: note.comment) + end else new_note.update!(comment: [text, note.comment, "\n", new_note.comment].join("\n")) end + rescue ActiveRecord::RecordInvalid + nil rescue => e @deferred_error = e end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index ef612e177..5232e6cfd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,3 +1,5 @@ +require 'devise/strategies/authenticatable' + Warden::Manager.after_set_user except: :fetch do |user, warden| if user.session_active?(warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']) session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'] @@ -72,17 +74,48 @@ module Devise mattr_accessor :ldap_uid_conversion_replace @@ldap_uid_conversion_replace = nil - class Strategies::PamAuthenticatable - def valid? - super && ::Devise.pam_authentication + module Strategies + class PamAuthenticatable + def valid? + super && ::Devise.pam_authentication + end + end + + class SessionActivationRememberable < Authenticatable + def valid? + @session_cookie = nil + session_cookie.present? + end + + def authenticate! + resource = SessionActivation.find_by(session_id: session_cookie)&.user + + unless resource + cookies.delete('_session_id') + return pass + end + + if validate(resource) + success!(resource) + end + end + + private + + def session_cookie + @session_cookie ||= cookies.signed['_session_id'] + end end end end +Warden::Strategies.add(:session_activation_rememberable, Devise::Strategies::SessionActivationRememberable) + Devise.setup do |config| config.warden do |manager| manager.default_strategies(scope: :user).unshift :two_factor_ldap_authenticatable if Devise.ldap_authentication manager.default_strategies(scope: :user).unshift :two_factor_pam_authenticatable if Devise.pam_authentication + manager.default_strategies(scope: :user).unshift :session_activation_rememberable manager.default_strategies(scope: :user).unshift :two_factor_authenticatable manager.default_strategies(scope: :user).unshift :two_factor_backupable end diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index 478d5a43d..37f08ad93 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ module Mastodon end def patch - 1 + 3 end def flags diff --git a/spec/controllers/api/v1/accounts/notes_controller_spec.rb b/spec/controllers/api/v1/accounts/notes_controller_spec.rb new file mode 100644 index 000000000..0a2957fed --- /dev/null +++ b/spec/controllers/api/v1/accounts/notes_controller_spec.rb @@ -0,0 +1,48 @@ +require 'rails_helper' + +describe Api::V1::Accounts::NotesController do + render_views + + let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'write:accounts') } + let(:account) { Fabricate(:account) } + let(:comment) { 'foo' } + + before do + allow(controller).to receive(:doorkeeper_token) { token } + end + + describe 'POST #create' do + subject do + post :create, params: { account_id: account.id, comment: comment } + end + + context 'when account note has reasonable length' do + let(:comment) { 'foo' } + + it 'returns http success' do + subject + expect(response).to have_http_status(200) + end + + it 'updates account note' do + subject + expect(AccountNote.find_by(account_id: user.account.id, target_account_id: account.id).comment).to eq comment + end + end + + context 'when account note exceends allowed length' do + let(:comment) { 'a' * 2_001 } + + it 'returns 422' do + subject + expect(response).to have_http_status(422) + end + + it 'does not create account note' do + subject + expect(AccountNote.where(account_id: user.account.id, target_account_id: account.id).exists?).to be_falsey + end + end + end +end diff --git a/spec/lib/link_details_extractor_spec.rb b/spec/lib/link_details_extractor_spec.rb new file mode 100644 index 000000000..850857b2d --- /dev/null +++ b/spec/lib/link_details_extractor_spec.rb @@ -0,0 +1,29 @@ +require 'rails_helper' + +RSpec.describe LinkDetailsExtractor do + let(:original_url) { '' } + let(:html) { '' } + let(:html_charset) { nil } + + subject { described_class.new(original_url, html, html_charset) } + + describe '#canonical_url' do + let(:original_url) { 'https://foo.com/article?bar=baz123' } + + context 'when canonical URL points to another host' do + let(:html) { '' } + + it 'ignores the canonical URLs' do + expect(subject.canonical_url).to eq original_url + end + end + + context 'when canonical URL points to the same host' do + let(:html) { '' } + + it 'ignores the canonical URLs' do + expect(subject.canonical_url).to eq 'https://foo.com/article' + end + end + end +end diff --git a/spec/services/fetch_link_card_service_spec.rb b/spec/services/fetch_link_card_service_spec.rb index 736a6078d..4914c2753 100644 --- a/spec/services/fetch_link_card_service_spec.rb +++ b/spec/services/fetch_link_card_service_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe FetchLinkCardService, type: :service do - subject { FetchLinkCardService.new } + subject { described_class.new } before do stub_request(:get, 'http://example.xn--fiqs8s/').to_return(request_fixture('idn.txt')) diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb index 8ab4f182f..82449b0c7 100644 --- a/spec/workers/move_worker_spec.rb +++ b/spec/workers/move_worker_spec.rb @@ -9,7 +9,8 @@ describe MoveWorker do let(:source_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } let(:target_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } let(:local_user) { Fabricate(:user) } - let!(:account_note) { Fabricate(:account_note, account: local_user.account, target_account: source_account) } + let(:comment) { 'old note prior to move' } + let!(:account_note) { Fabricate(:account_note, account: local_user.account, target_account: source_account, comment: comment) } let(:block_service) { double } @@ -26,19 +27,37 @@ describe MoveWorker do end shared_examples 'user note handling' do - it 'copies user note' do - subject.perform(source_account.id, target_account.id) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + context 'when user notes are short enough' do + it 'copies user note with prelude' do + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + end + + it 'merges user notes when needed' do + new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + end end - it 'merges user notes when needed' do - new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + context 'when user notes are too long' do + let(:comment) { 'abc' * 333 } - subject.perform(source_account.id, target_account.id) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(source_account.acct) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) - expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + it 'copies user note without prelude' do + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(account_note.comment) + end + + it 'keeps user notes unchanged' do + new_account_note = AccountNote.create!(account: account_note.account, target_account: target_account, comment: 'new note prior to move') + + subject.perform(source_account.id, target_account.id) + expect(AccountNote.find_by(account: account_note.account, target_account: target_account).comment).to include(new_account_note.comment) + end end end diff --git a/spec/workers/publish_scheduled_announcement_worker_spec.rb b/spec/workers/publish_scheduled_announcement_worker_spec.rb new file mode 100644 index 000000000..0977bba1e --- /dev/null +++ b/spec/workers/publish_scheduled_announcement_worker_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe PublishScheduledAnnouncementWorker do + subject { described_class.new } + + let!(:remote_account) { Fabricate(:account, domain: 'domain.com', username: 'foo', uri: 'https://domain.com/users/foo') } + let!(:remote_status) { Fabricate(:status, uri: 'https://domain.com/users/foo/12345', account: remote_account) } + let!(:local_status) { Fabricate(:status) } + let(:scheduled_announcement) { Fabricate(:announcement, text: "rebooting very soon, see #{ActivityPub::TagManager.instance.uri_for(remote_status)} and #{ActivityPub::TagManager.instance.uri_for(local_status)}") } + + describe 'perform' do + before do + service = double + allow(FetchRemoteStatusService).to receive(:new).and_return(service) + allow(service).to receive(:call).with('https://domain.com/users/foo/12345') { remote_status.reload } + + subject.perform(scheduled_announcement.id) + end + + it 'updates the linked statuses' do + expect(scheduled_announcement.reload.status_ids).to eq [remote_status.id, local_status.id] + end + end +end