From 5614e6724e5131f33197ecbc1998058e9794aae9 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 4 Mar 2021 00:12:26 +0100 Subject: [PATCH] Fix URL scanning in note length validator and preview card fetching (#15827) * Add tests * Fix URL scanning in note length validator and preview card fetching --- app/lib/language_detector.rb | 2 +- app/services/fetch_link_card_service.rb | 15 +++++---- app/validators/note_length_validator.rb | 2 +- app/validators/status_length_validator.rb | 8 +---- spec/services/fetch_link_card_service_spec.rb | 8 +++++ spec/validators/note_length_validator_spec.rb | 33 +++++++++++++++++++ 6 files changed, 52 insertions(+), 16 deletions(-) create mode 100644 spec/validators/note_length_validator_spec.rb diff --git a/app/lib/language_detector.rb b/app/lib/language_detector.rb index 2cc8ac6158d..40452eddc96 100644 --- a/app/lib/language_detector.rb +++ b/app/lib/language_detector.rb @@ -69,7 +69,7 @@ class LanguageDetector def simplify_text(text) new_text = remove_html(text) - new_text.gsub!(FetchLinkCardService::URL_PATTERN, '') + new_text.gsub!(FetchLinkCardService::URL_PATTERN, '\1') new_text.gsub!(Account::MENTION_RE, '') new_text.gsub!(Tag::HASHTAG_RE) { |string| string.gsub(/[#_]/, '#' => '', '_' => ' ').gsub(/[a-z][A-Z]|[a-zA-Z][\d]/) { |s| s.insert(1, ' ') }.downcase } new_text.gsub!(/:#{CustomEmoji::SHORTCODE_RE_FRAGMENT}:/, '') diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index d4e4931e670..fa1636e4111 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -2,12 +2,13 @@ class FetchLinkCardService < BaseService URL_PATTERN = %r{ - ( # $1 URL - (https?:\/\/) # $2 Protocol (required) - (#{Twitter::TwitterText::Regex[:valid_domain]}) # $3 Domain(s) - (?::(#{Twitter::TwitterText::Regex[:valid_port_number]}))? # $4 Port number (optional) - (/#{Twitter::TwitterText::Regex[:valid_url_path]}*)? # $5 URL Path and anchor - (\?#{Twitter::TwitterText::Regex[:valid_url_query_chars]}*#{Twitter::TwitterText::Regex[:valid_url_query_ending_chars]})? # $6 Query String + (#{Twitter::TwitterText::Regex[:valid_url_preceding_chars]}) # $1 preceeding chars + ( # $2 URL + (https?:\/\/) # $3 Protocol (required) + (#{Twitter::TwitterText::Regex[:valid_domain]}) # $4 Domain(s) + (?::(#{Twitter::TwitterText::Regex[:valid_port_number]}))? # $5 Port number (optional) + (/#{Twitter::TwitterText::Regex[:valid_url_path]}*)? # $6 URL Path and anchor + (\?#{Twitter::TwitterText::Regex[:valid_url_query_chars]}*#{Twitter::TwitterText::Regex[:valid_url_query_ending_chars]})? # $7 Query String ) }iox @@ -63,7 +64,7 @@ class FetchLinkCardService < BaseService def parse_urls if @status.local? - urls = @status.text.scan(URL_PATTERN).map { |array| Addressable::URI.parse(array[0]).normalize } + urls = @status.text.scan(URL_PATTERN).map { |array| Addressable::URI.parse(array[1]).normalize } else html = Nokogiri::HTML(@status.text) links = html.css('a') diff --git a/app/validators/note_length_validator.rb b/app/validators/note_length_validator.rb index 7ea2bb3e53a..554ad49ce29 100644 --- a/app/validators/note_length_validator.rb +++ b/app/validators/note_length_validator.rb @@ -15,7 +15,7 @@ class NoteLengthValidator < ActiveModel::EachValidator return '' if value.nil? value.dup.tap do |new_text| - new_text.gsub!(FetchLinkCardService::URL_PATTERN, 'x' * 23) + new_text.gsub!(FetchLinkCardService::URL_PATTERN, StatusLengthValidator::URL_PLACEHOLDER) new_text.gsub!(Account::MENTION_RE, '@\2') end end diff --git a/app/validators/status_length_validator.rb b/app/validators/status_length_validator.rb index b56c5a3212e..d036f19256b 100644 --- a/app/validators/status_length_validator.rb +++ b/app/validators/status_length_validator.rb @@ -2,12 +2,6 @@ class StatusLengthValidator < ActiveModel::Validator MAX_CHARS = 500 - URL_PATTERN = %r{ - (?: - (#{Twitter::TwitterText::Regex[:valid_url_preceding_chars]}) - (#{FetchLinkCardService::URL_PATTERN}) - ) - }iox URL_PLACEHOLDER = "\1#{'x' * 23}" def validate(status) @@ -35,7 +29,7 @@ class StatusLengthValidator < ActiveModel::Validator return '' if @status.text.nil? @status.text.dup.tap do |new_text| - new_text.gsub!(URL_PATTERN, URL_PLACEHOLDER) + new_text.gsub!(FetchLinkCardService::URL_PATTERN, URL_PLACEHOLDER) new_text.gsub!(Account::MENTION_RE, '@\2') end end diff --git a/spec/services/fetch_link_card_service_spec.rb b/spec/services/fetch_link_card_service_spec.rb index 8b296cc7048..736a6078dd6 100644 --- a/spec/services/fetch_link_card_service_spec.rb +++ b/spec/services/fetch_link_card_service_spec.rb @@ -77,6 +77,14 @@ RSpec.describe FetchLinkCardService, type: :service do expect(a_request(:get, 'http://example.com/test-')).to have_been_made.at_least_once end end + + context do + let(:status) { Fabricate(:status, text: 'testhttp://example.com/sjis') } + + it 'does not fetch URLs with not isolated from their surroundings' do + expect(a_request(:get, 'http://example.com/sjis')).to_not have_been_made + end + end end context 'in a remote status' do diff --git a/spec/validators/note_length_validator_spec.rb b/spec/validators/note_length_validator_spec.rb new file mode 100644 index 00000000000..6e9b4e132f6 --- /dev/null +++ b/spec/validators/note_length_validator_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe NoteLengthValidator do + subject { NoteLengthValidator.new(attributes: { note: true }, maximum: 500) } + + describe '#validate' do + it 'adds an error when text is over 500 characters' do + text = 'a' * 520 + account = double(note: text, errors: double(add: nil)) + + subject.validate_each(account, 'note', text) + expect(account.errors).to have_received(:add) + end + + it 'counts URLs as 23 characters flat' do + text = ('a' * 476) + " http://#{'b' * 30}.com/example" + account = double(note: text, errors: double(add: nil)) + + subject.validate_each(account, 'note', text) + expect(account.errors).to_not have_received(:add) + end + + it 'does not count non-autolinkable URLs as 23 characters flat' do + text = ('a' * 476) + "http://#{'b' * 30}.com/example" + account = double(note: text, errors: double(add: nil)) + + subject.validate_each(account, 'note', text) + expect(account.errors).to have_received(:add) + end + end +end