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
This commit is contained in:
Eugen Rochko 2022-11-09 08:24:21 +01:00 committed by GitHub
parent 53817294fc
commit e98833748e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 211 additions and 48 deletions

View file

@ -295,7 +295,7 @@ class Account < ApplicationRecord
def fields def fields
(self[:fields] || []).map do |f| (self[:fields] || []).map do |f|
Field.new(self, f) Account::Field.new(self, f)
rescue rescue
nil nil
end.compact end.compact
@ -401,48 +401,6 @@ class Account < ApplicationRecord
requires_review? && !requested_review? requires_review? && !requested_review?
end 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 class << self
DISALLOWED_TSQUERY_CHARACTERS = /['?\\:]/.freeze 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'))" TEXTSEARCH = "(setweight(to_tsvector('simple', accounts.display_name), 'A') || setweight(to_tsvector('simple', accounts.username), 'B') || setweight(to_tsvector('simple', coalesce(accounts.domain, '')), 'C'))"

View file

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

View file

@ -40,7 +40,7 @@ class ActivityPub::ProcessAccountService < BaseService
unless @options[:only_key] || @account.suspended? unless @options[:only_key] || @account.suspended?
check_featured_collection! if @account.featured_collection_url.present? check_featured_collection! if @account.featured_collection_url.present?
check_featured_tags_collection! if @json['featuredTags'].present? check_featured_tags_collection! if @json['featuredTags'].present?
check_links! unless @account.fields.empty? check_links! if @account.fields.any?(&:requires_verification?)
end end
@account @account

View file

@ -28,7 +28,7 @@ class UpdateAccountService < BaseService
end end
def check_links(account) def check_links(account)
VerifyAccountLinksWorker.perform_async(account.id) VerifyAccountLinksWorker.perform_async(account.id) if account.fields.any?(&:requires_verification?)
end end
def process_hashtags(account) def process_hashtags(account)

View file

@ -8,6 +8,9 @@
%link{ rel: 'alternate', type: 'application/rss+xml', href: @rss_url }/ %link{ rel: 'alternate', type: 'application/rss+xml', href: @rss_url }/
%link{ rel: 'alternate', type: 'application/activity+json', href: ActivityPub::TagManager.instance.uri_for(@account) }/ %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' = opengraph 'og:type', 'profile'
= render 'og', account: @account, url: short_account_url(@account, only_path: false) = render 'og', account: @account, url: short_account_url(@account, only_path: false)

View file

@ -3,14 +3,13 @@
class VerifyAccountLinksWorker class VerifyAccountLinksWorker
include Sidekiq::Worker 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) def perform(account_id)
account = Account.find(account_id) account = Account.find(account_id)
account.fields.each do |field| account.fields.each do |field|
next unless !field.verified? && field.verifiable? VerifyLinkService.new.call(field) if field.requires_verification?
VerifyLinkService.new.call(field)
end end
account.save! if account.changed? account.save! if account.changed?

View file

@ -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) { '<a href="https://www.patreon.com/mastodon" target="_blank" rel="nofollow noopener noreferrer me"><span class="invisible">https://www.</span><span class="">patreon.com/mastodon</span><span class="invisible"></span></a>' }
it 'returns true' do
expect(subject.verifiable?).to be true
end
end
context 'for a link with misleading authentication' do
let(:value) { '<a href="https://google.com @h.43z.one" target="_blank" rel="nofollow noopener noreferrer me"><span class="invisible">https://</span><span class="">google.com</span><span class="invisible"> @h.43z.one</span></a>' }
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) { '<a href="https://google.com" target="_blank" rel="nofollow noopener noreferrer me"><span class="invisible">https://</span><span class="">google.com</span><span class="invisible"></span></a> @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) { '<a href="https://google.com/bar">https://example.com/foo</a>' }
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