From 36b0ff57b7699db3d6acaa969a6c1491d1b9f9e3 Mon Sep 17 00:00:00 2001 From: Roni Laukkarinen Date: Tue, 8 Nov 2022 17:35:42 +0200 Subject: Fix grammar (#20106) --- spec/models/account_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/models') diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 467d41836..75e0235b8 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -755,7 +755,7 @@ RSpec.describe Account, type: :model do expect(account).to model_have_error_on_field(:username) end - it 'is invalid if the username is longer then 30 characters' do + it 'is invalid if the username is longer than 30 characters' do account = Fabricate.build(:account, username: Faker::Lorem.characters(number: 31)) account.valid? expect(account).to model_have_error_on_field(:username) @@ -801,7 +801,7 @@ RSpec.describe Account, type: :model do expect(account).to model_have_error_on_field(:username) end - it 'is valid even if the username is longer then 30 characters' do + it 'is valid even if the username is longer than 30 characters' do account = Fabricate.build(:account, domain: 'domain', username: Faker::Lorem.characters(number: 31)) account.valid? expect(account).not_to model_have_error_on_field(:username) -- cgit From 6ba52306f9d2611a06326445230b54c156c37e53 Mon Sep 17 00:00:00 2001 From: luzpaz Date: Tue, 8 Nov 2022 11:32:03 -0500 Subject: Fix typos (#19849) Found via `codespell -q 3 -S ./yarn.lock,./CHANGELOG.md,./AUTHORS.md,./config/locales,./app/javascript/mastodon/locales -L ba,followings,keypair,medias,pattens,pixelx,rememberable,ro,te` --- spec/lib/webfinger_resource_spec.rb | 2 +- spec/models/account_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'spec/models') diff --git a/spec/lib/webfinger_resource_spec.rb b/spec/lib/webfinger_resource_spec.rb index 236e9f3e2..5c7f475d6 100644 --- a/spec/lib/webfinger_resource_spec.rb +++ b/spec/lib/webfinger_resource_spec.rb @@ -50,7 +50,7 @@ describe WebfingerResource do end it 'finds the username in a mixed case http route' do - resource = 'HTTp://exAMPLEe.com/users/alice' + resource = 'HTTp://exAMPLe.com/users/alice' result = WebfingerResource.new(resource).username expect(result).to eq 'alice' diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 75e0235b8..edae05f9d 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -255,7 +255,7 @@ RSpec.describe Account, type: :model do Fabricate(:status, reblog: original_status, account: author) end - it 'is is true when this account has favourited it' do + it 'is true when this account has favourited it' do Fabricate(:favourite, status: original_reblog, account: subject) expect(subject.favourited?(original_status)).to eq true @@ -267,7 +267,7 @@ RSpec.describe Account, type: :model do end context 'when the status is an original status' do - it 'is is true when this account has favourited it' do + it 'is true when this account has favourited it' do Fabricate(:favourite, status: original_status, account: subject) expect(subject.favourited?(original_status)).to eq true -- cgit From e98833748e80275a88560155a0b912667dd2d70b Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 9 Nov 2022 08:24:21 +0100 Subject: Fix being able to spoof link verification (#20217) - Change verification to happen in `default` queue - Change verification worker to only be queued if there's something to do - Add `link` tags from metadata fields to page header of profiles --- app/models/account.rb | 44 +------ app/models/account/field.rb | 73 ++++++++++++ .../activitypub/process_account_service.rb | 2 +- app/services/update_account_service.rb | 2 +- app/views/accounts/show.html.haml | 3 + app/workers/verify_account_links_worker.rb | 5 +- spec/models/account/field_spec.rb | 130 +++++++++++++++++++++ 7 files changed, 211 insertions(+), 48 deletions(-) create mode 100644 app/models/account/field.rb create mode 100644 spec/models/account/field_spec.rb (limited to 'spec/models') diff --git a/app/models/account.rb b/app/models/account.rb index 3647b8225..be1968fa6 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -295,7 +295,7 @@ class Account < ApplicationRecord def fields (self[:fields] || []).map do |f| - Field.new(self, f) + Account::Field.new(self, f) rescue nil end.compact @@ -401,48 +401,6 @@ class Account < ApplicationRecord requires_review? && !requested_review? end - class Field < ActiveModelSerializers::Model - attributes :name, :value, :verified_at, :account - - def initialize(account, attributes) - @original_field = attributes - string_limit = account.local? ? 255 : 2047 - super( - account: account, - name: attributes['name'].strip[0, string_limit], - value: attributes['value'].strip[0, string_limit], - verified_at: attributes['verified_at']&.to_datetime, - ) - end - - def verified? - verified_at.present? - end - - def value_for_verification - @value_for_verification ||= begin - if account.local? - value - else - ActionController::Base.helpers.strip_tags(value) - end - end - end - - def verifiable? - value_for_verification.present? && value_for_verification.start_with?('http://', 'https://') - end - - def mark_verified! - self.verified_at = Time.now.utc - @original_field['verified_at'] = verified_at - end - - def to_h - { name: name, value: value, verified_at: verified_at } - end - end - class << self DISALLOWED_TSQUERY_CHARACTERS = /['?\\:‘’]/.freeze TEXTSEARCH = "(setweight(to_tsvector('simple', accounts.display_name), 'A') || setweight(to_tsvector('simple', accounts.username), 'B') || setweight(to_tsvector('simple', coalesce(accounts.domain, '')), 'C'))" diff --git a/app/models/account/field.rb b/app/models/account/field.rb new file mode 100644 index 000000000..4e0fd9230 --- /dev/null +++ b/app/models/account/field.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +class Account::Field < ActiveModelSerializers::Model + MAX_CHARACTERS_LOCAL = 255 + MAX_CHARACTERS_COMPAT = 2_047 + + attributes :name, :value, :verified_at, :account + + def initialize(account, attributes) + # Keeping this as reference allows us to update the field on the account + # from methods in this class, so that changes can be saved. + @original_field = attributes + @account = account + + super( + name: sanitize(attributes['name']), + value: sanitize(attributes['value']), + verified_at: attributes['verified_at']&.to_datetime, + ) + end + + def verified? + verified_at.present? + end + + def value_for_verification + @value_for_verification ||= begin + if account.local? + value + else + extract_url_from_html + end + end + end + + def verifiable? + value_for_verification.present? && /\A#{FetchLinkCardService::URL_PATTERN}\z/.match?(value_for_verification) + end + + def requires_verification? + !verified? && verifiable? + end + + def mark_verified! + @original_field['verified_at'] = self.verified_at = Time.now.utc + end + + def to_h + { name: name, value: value, verified_at: verified_at } + end + + private + + def sanitize(str) + str.strip[0, character_limit] + end + + def character_limit + account.local? ? MAX_CHARACTERS_LOCAL : MAX_CHARACTERS_COMPAT + end + + def extract_url_from_html + doc = Nokogiri::HTML(value).at_xpath('//body') + + return if doc.children.size > 1 + + element = doc.children.first + + return if element.name != 'a' || element['href'] != element.text + + element['href'] + end +end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 3834d79cc..99bcb3835 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -40,7 +40,7 @@ class ActivityPub::ProcessAccountService < BaseService unless @options[:only_key] || @account.suspended? check_featured_collection! if @account.featured_collection_url.present? check_featured_tags_collection! if @json['featuredTags'].present? - check_links! unless @account.fields.empty? + check_links! if @account.fields.any?(&:requires_verification?) end @account diff --git a/app/services/update_account_service.rb b/app/services/update_account_service.rb index 77f794e17..71976ab00 100644 --- a/app/services/update_account_service.rb +++ b/app/services/update_account_service.rb @@ -28,7 +28,7 @@ class UpdateAccountService < BaseService end def check_links(account) - VerifyAccountLinksWorker.perform_async(account.id) + VerifyAccountLinksWorker.perform_async(account.id) if account.fields.any?(&:requires_verification?) end def process_hashtags(account) diff --git a/app/views/accounts/show.html.haml b/app/views/accounts/show.html.haml index a51dcd7be..e8fd27e10 100644 --- a/app/views/accounts/show.html.haml +++ b/app/views/accounts/show.html.haml @@ -8,6 +8,9 @@ %link{ rel: 'alternate', type: 'application/rss+xml', href: @rss_url }/ %link{ rel: 'alternate', type: 'application/activity+json', href: ActivityPub::TagManager.instance.uri_for(@account) }/ + - @account.fields.select(&:verifiable?).each do |field| + %link{ rel: 'me', type: 'text/html', href: field.value }/ + = opengraph 'og:type', 'profile' = render 'og', account: @account, url: short_account_url(@account, only_path: false) diff --git a/app/workers/verify_account_links_worker.rb b/app/workers/verify_account_links_worker.rb index 8114d59be..f606e6c26 100644 --- a/app/workers/verify_account_links_worker.rb +++ b/app/workers/verify_account_links_worker.rb @@ -3,14 +3,13 @@ class VerifyAccountLinksWorker include Sidekiq::Worker - sidekiq_options queue: 'pull', retry: false, lock: :until_executed + sidekiq_options queue: 'default', retry: false, lock: :until_executed def perform(account_id) account = Account.find(account_id) account.fields.each do |field| - next unless !field.verified? && field.verifiable? - VerifyLinkService.new.call(field) + VerifyLinkService.new.call(field) if field.requires_verification? end account.save! if account.changed? diff --git a/spec/models/account/field_spec.rb b/spec/models/account/field_spec.rb new file mode 100644 index 000000000..7d61a2c62 --- /dev/null +++ b/spec/models/account/field_spec.rb @@ -0,0 +1,130 @@ +require 'rails_helper' + +RSpec.describe Account::Field, type: :model do + describe '#verified?' do + let(:account) { double('Account', local?: true) } + + subject { described_class.new(account, 'name' => 'Foo', 'value' => 'Bar', 'verified_at' => verified_at) } + + context 'when verified_at is set' do + let(:verified_at) { Time.now.utc.iso8601 } + + it 'returns true' do + expect(subject.verified?).to be true + end + end + + context 'when verified_at is not set' do + let(:verified_at) { nil } + + it 'returns false' do + expect(subject.verified?).to be false + end + end + end + + describe '#mark_verified!' do + let(:account) { double('Account', local?: true) } + let(:original_hash) { { 'name' => 'Foo', 'value' => 'Bar' } } + + subject { described_class.new(account, original_hash) } + + before do + subject.mark_verified! + end + + it 'updates verified_at' do + expect(subject.verified_at).to_not be_nil + end + + it 'updates original hash' do + expect(original_hash['verified_at']).to_not be_nil + end + end + + describe '#verifiable?' do + let(:account) { double('Account', local?: local) } + + subject { described_class.new(account, 'name' => 'Foo', 'value' => value) } + + context 'for local accounts' do + let(:local) { true } + + context 'for a URL with misleading authentication' do + let(:value) { 'https://spacex.com @h.43z.one' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + + context 'for a URL' do + let(:value) { 'https://example.com' } + + it 'returns true' do + expect(subject.verifiable?).to be true + end + end + + context 'for text that is not a URL' do + let(:value) { 'Hello world' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + + context 'for text that contains a URL' do + let(:value) { 'Hello https://example.com world' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + end + + context 'for remote accounts' do + let(:local) { false } + + context 'for a link' do + let(:value) { 'patreon.com/mastodon' } + + it 'returns true' do + expect(subject.verifiable?).to be true + end + end + + context 'for a link with misleading authentication' do + let(:value) { 'google.com' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + + context 'for HTML that has more than just a link' do + let(:value) { 'google.com @h.43z.one' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + + context 'for a link with different visible text' do + let(:value) { 'https://example.com/foo' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + + context 'for text that is a URL but is not linked' do + let(:value) { 'https://example.com/foo' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + end + end +end -- cgit From 9965a23b043b0ab511e083c44acda891ea441859 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 10 Nov 2022 06:27:45 +0100 Subject: Change link verification to ignore IDN domains (#20295) Fix #3833 --- app/models/account/field.rb | 16 +++++++++++++++- spec/models/account/field_spec.rb | 8 ++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) (limited to 'spec/models') diff --git a/app/models/account/field.rb b/app/models/account/field.rb index 4e0fd9230..d74f90b2b 100644 --- a/app/models/account/field.rb +++ b/app/models/account/field.rb @@ -3,6 +3,7 @@ class Account::Field < ActiveModelSerializers::Model MAX_CHARACTERS_LOCAL = 255 MAX_CHARACTERS_COMPAT = 2_047 + ACCEPTED_SCHEMES = %w(http https).freeze attributes :name, :value, :verified_at, :account @@ -34,7 +35,20 @@ class Account::Field < ActiveModelSerializers::Model end def verifiable? - value_for_verification.present? && /\A#{FetchLinkCardService::URL_PATTERN}\z/.match?(value_for_verification) + return false if value_for_verification.blank? + + # This is slower than checking through a regular expression, but we + # need to confirm that it's not an IDN domain. + + parsed_url = Addressable::URI.parse(value_for_verification) + + ACCEPTED_SCHEMES.include?(parsed_url.scheme) && + parsed_url.user.nil? && + parsed_url.password.nil? && + parsed_url.host.present? && + parsed_url.normalized_host == parsed_url.host + rescue Addressable::URI::InvalidURIError, IDN::Idna::IdnaError + false end def requires_verification? diff --git a/spec/models/account/field_spec.rb b/spec/models/account/field_spec.rb index 7d61a2c62..fcb2a884a 100644 --- a/spec/models/account/field_spec.rb +++ b/spec/models/account/field_spec.rb @@ -66,6 +66,14 @@ RSpec.describe Account::Field, type: :model do end end + context 'for an IDN URL' do + let(:value) { 'http://twitter.com∕dougallj∕status∕1590357240443437057.ê.cc/twitter.html' } + + it 'returns false' do + expect(subject.verifiable?).to be false + end + end + context 'for text that is not a URL' do let(:value) { 'Hello world' } -- cgit