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 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 43 deletions(-) create mode 100644 app/models/account/field.rb (limited to 'app/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 -- cgit From 0cd0786aef140ea41aa229cd52ac67867259a3f5 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 10 Nov 2022 05:34:42 +0100 Subject: Revert filtering public timelines by locale by default (#20294) --- app/controllers/api/v1/timelines/public_controller.rb | 1 - app/models/public_feed.rb | 11 ++--------- app/models/tag_feed.rb | 1 - 3 files changed, 2 insertions(+), 11 deletions(-) (limited to 'app/models') diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb index 15b91d63e..d253b744f 100644 --- a/app/controllers/api/v1/timelines/public_controller.rb +++ b/app/controllers/api/v1/timelines/public_controller.rb @@ -35,7 +35,6 @@ class Api::V1::Timelines::PublicController < Api::BaseController def public_feed PublicFeed.new( current_account, - locale: content_locale, local: truthy_param?(:local), remote: truthy_param?(:remote), only_media: truthy_param?(:only_media) diff --git a/app/models/public_feed.rb b/app/models/public_feed.rb index 2cf9206d2..1cfd9a500 100644 --- a/app/models/public_feed.rb +++ b/app/models/public_feed.rb @@ -8,7 +8,6 @@ class PublicFeed # @option [Boolean] :local # @option [Boolean] :remote # @option [Boolean] :only_media - # @option [String] :locale def initialize(account, options = {}) @account = account @options = options @@ -28,7 +27,7 @@ class PublicFeed scope.merge!(remote_only_scope) if remote_only? scope.merge!(account_filters_scope) if account? scope.merge!(media_only_scope) if media_only? - scope.merge!(language_scope) + scope.merge!(language_scope) if account&.chosen_languages.present? scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) end @@ -86,13 +85,7 @@ class PublicFeed end def language_scope - if account&.chosen_languages.present? - Status.where(language: account.chosen_languages) - elsif @options[:locale].present? - Status.where(language: @options[:locale]) - else - Status.all - end + Status.where(language: account.chosen_languages) end def account_filters_scope diff --git a/app/models/tag_feed.rb b/app/models/tag_feed.rb index 8502b79af..b8cd63557 100644 --- a/app/models/tag_feed.rb +++ b/app/models/tag_feed.rb @@ -12,7 +12,6 @@ class TagFeed < PublicFeed # @option [Boolean] :local # @option [Boolean] :remote # @option [Boolean] :only_media - # @option [String] :locale def initialize(tag, account, options = {}) @tag = tag super(account, options) -- cgit From 78a6b871fe3dae308380ea88132ddadc86a1431e Mon Sep 17 00:00:00 2001 From: James Tucker Date: Wed, 9 Nov 2022 20:49:30 -0800 Subject: Improve performance by avoiding regex construction (#20215) ```ruby 10.times { p /#{FOO}/.object_id } 10.times { p FOO_RE.object_id } ``` --- app/controllers/api/v1/tags_controller.rb | 2 +- app/lib/hashtag_normalizer.rb | 2 +- app/models/account.rb | 3 ++- app/models/custom_emoji.rb | 3 ++- app/models/featured_tag.rb | 2 +- app/models/tag.rb | 13 ++++++++----- app/services/account_search_service.rb | 4 +++- app/services/resolve_url_service.rb | 4 +++- 8 files changed, 21 insertions(+), 12 deletions(-) (limited to 'app/models') diff --git a/app/controllers/api/v1/tags_controller.rb b/app/controllers/api/v1/tags_controller.rb index 9e5c53330..32f71bdce 100644 --- a/app/controllers/api/v1/tags_controller.rb +++ b/app/controllers/api/v1/tags_controller.rb @@ -24,7 +24,7 @@ class Api::V1::TagsController < Api::BaseController private def set_or_create_tag - return not_found unless /\A(#{Tag::HASHTAG_NAME_RE})\z/.match?(params[:id]) + return not_found unless Tag::HASHTAG_NAME_RE.match?(params[:id]) @tag = Tag.find_normalized(params[:id]) || Tag.new(name: Tag.normalize(params[:id]), display_name: params[:id]) end end diff --git a/app/lib/hashtag_normalizer.rb b/app/lib/hashtag_normalizer.rb index c1f99e163..49fa6101d 100644 --- a/app/lib/hashtag_normalizer.rb +++ b/app/lib/hashtag_normalizer.rb @@ -8,7 +8,7 @@ class HashtagNormalizer private def remove_invalid_characters(str) - str.gsub(/[^[:alnum:]#{Tag::HASHTAG_SEPARATORS}]/, '') + str.gsub(Tag::HASHTAG_INVALID_CHARS_RE, '') end def ascii_folding(str) diff --git a/app/models/account.rb b/app/models/account.rb index be1968fa6..cc3a8f3df 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -64,6 +64,7 @@ class Account < ApplicationRecord USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[[:word:]\.\-]+[[:word:]]+)?)/i URL_PREFIX_RE = /\Ahttp(s?):\/\/[^\/]+/ + USERNAME_ONLY_RE = /\A#{USERNAME_RE}\z/i include Attachmentable include AccountAssociations @@ -84,7 +85,7 @@ class Account < ApplicationRecord validates_with UniqueUsernameValidator, if: -> { will_save_change_to_username? } # Remote user validations - validates :username, format: { with: /\A#{USERNAME_RE}\z/i }, if: -> { !local? && will_save_change_to_username? } + validates :username, format: { with: USERNAME_ONLY_RE }, if: -> { !local? && will_save_change_to_username? } # Local user validations validates :username, format: { with: /\A[a-z0-9_]+\z/i }, length: { maximum: 30 }, if: -> { local? && will_save_change_to_username? && actor_type != 'Application' } diff --git a/app/models/custom_emoji.rb b/app/models/custom_emoji.rb index 077ce559a..7b19cd2ac 100644 --- a/app/models/custom_emoji.rb +++ b/app/models/custom_emoji.rb @@ -30,6 +30,7 @@ class CustomEmoji < ApplicationRecord SCAN_RE = /(?<=[^[:alnum:]:]|\n|^) :(#{SHORTCODE_RE_FRAGMENT}): (?=[^[:alnum:]:]|$)/x + SHORTCODE_ONLY_RE = /\A#{SHORTCODE_RE_FRAGMENT}\z/ IMAGE_MIME_TYPES = %w(image/png image/gif image/webp).freeze @@ -41,7 +42,7 @@ class CustomEmoji < ApplicationRecord before_validation :downcase_domain validates_attachment :image, content_type: { content_type: IMAGE_MIME_TYPES }, presence: true, size: { less_than: LIMIT } - validates :shortcode, uniqueness: { scope: :domain }, format: { with: /\A#{SHORTCODE_RE_FRAGMENT}\z/ }, length: { minimum: 2 } + validates :shortcode, uniqueness: { scope: :domain }, format: { with: SHORTCODE_ONLY_RE }, length: { minimum: 2 } scope :local, -> { where(domain: nil) } scope :remote, -> { where.not(domain: nil) } diff --git a/app/models/featured_tag.rb b/app/models/featured_tag.rb index debae2212..70f949b6a 100644 --- a/app/models/featured_tag.rb +++ b/app/models/featured_tag.rb @@ -17,7 +17,7 @@ class FeaturedTag < ApplicationRecord belongs_to :account, inverse_of: :featured_tags belongs_to :tag, inverse_of: :featured_tags, optional: true # Set after validation - validates :name, presence: true, format: { with: /\A(#{Tag::HASHTAG_NAME_RE})\z/i }, on: :create + validates :name, presence: true, format: { with: Tag::HASHTAG_NAME_RE }, on: :create validate :validate_tag_uniqueness, on: :create validate :validate_featured_tags_limit, on: :create diff --git a/app/models/tag.rb b/app/models/tag.rb index 8929baf66..b66f85423 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -27,11 +27,14 @@ class Tag < ApplicationRecord has_many :followers, through: :passive_relationships, source: :account HASHTAG_SEPARATORS = "_\u00B7\u200c" - HASHTAG_NAME_RE = "([[:word:]_][[:word:]#{HASHTAG_SEPARATORS}]*[[:alpha:]#{HASHTAG_SEPARATORS}][[:word:]#{HASHTAG_SEPARATORS}]*[[:word:]_])|([[:word:]_]*[[:alpha:]][[:word:]_]*)" - HASHTAG_RE = /(?:^|[^\/\)\w])#(#{HASHTAG_NAME_RE})/i + HASHTAG_NAME_PAT = "([[:word:]_][[:word:]#{HASHTAG_SEPARATORS}]*[[:alpha:]#{HASHTAG_SEPARATORS}][[:word:]#{HASHTAG_SEPARATORS}]*[[:word:]_])|([[:word:]_]*[[:alpha:]][[:word:]_]*)" - validates :name, presence: true, format: { with: /\A(#{HASHTAG_NAME_RE})\z/i } - validates :display_name, format: { with: /\A(#{HASHTAG_NAME_RE})\z/i } + HASHTAG_RE = /(?:^|[^\/\)\w])#(#{HASHTAG_NAME_PAT})/i + HASHTAG_NAME_RE = /\A(#{HASHTAG_NAME_PAT})\z/i + HASHTAG_INVALID_CHARS_RE = /[^[:alnum:]#{HASHTAG_SEPARATORS}]/ + + validates :name, presence: true, format: { with: HASHTAG_NAME_RE } + validates :display_name, format: { with: HASHTAG_NAME_RE } validate :validate_name_change, if: -> { !new_record? && name_changed? } validate :validate_display_name_change, if: -> { !new_record? && display_name_changed? } @@ -102,7 +105,7 @@ class Tag < ApplicationRecord names = Array(name_or_names).map { |str| [normalize(str), str] }.uniq(&:first) names.map do |(normalized_name, display_name)| - tag = matching_name(normalized_name).first || create(name: normalized_name, display_name: display_name.gsub(/[^[:alnum:]#{HASHTAG_SEPARATORS}]/, '')) + tag = matching_name(normalized_name).first || create(name: normalized_name, display_name: display_name.gsub(HASHTAG_INVALID_CHARS_RE, '')) yield tag if block_given? diff --git a/app/services/account_search_service.rb b/app/services/account_search_service.rb index 9f2330a94..85538870b 100644 --- a/app/services/account_search_service.rb +++ b/app/services/account_search_service.rb @@ -3,6 +3,8 @@ class AccountSearchService < BaseService attr_reader :query, :limit, :offset, :options, :account + MENTION_ONLY_RE = /\A#{Account::MENTION_RE}\z/i + # Min. number of characters to look for non-exact matches MIN_QUERY_LENGTH = 5 @@ -180,7 +182,7 @@ class AccountSearchService < BaseService end def username_complete? - query.include?('@') && "@#{query}".match?(/\A#{Account::MENTION_RE}\Z/) + query.include?('@') && "@#{query}".match?(MENTION_ONLY_RE) end def likely_acct? diff --git a/app/services/resolve_url_service.rb b/app/services/resolve_url_service.rb index 37c856cf8..52f35daf3 100644 --- a/app/services/resolve_url_service.rb +++ b/app/services/resolve_url_service.rb @@ -4,6 +4,8 @@ class ResolveURLService < BaseService include JsonLdHelper include Authorization + USERNAME_STATUS_RE = %r{/@(?#{Account::USERNAME_RE})/(?[0-9]+)\Z} + def call(url, on_behalf_of: nil) @url = url @on_behalf_of = on_behalf_of @@ -43,7 +45,7 @@ class ResolveURLService < BaseService # We don't have an index on `url`, so try guessing the `uri` from `url` parsed_url = Addressable::URI.parse(@url) - parsed_url.path.match(%r{/@(?#{Account::USERNAME_RE})/(?[0-9]+)\Z}) do |matched| + parsed_url.path.match(USERNAME_STATUS_RE) do |matched| parsed_url.path = "/users/#{matched[:username]}/statuses/#{matched[:status_id]}" scope = scope.or(Status.where(uri: parsed_url.to_s, url: @url)) 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 'app/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