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

diff --git a/app/lib/language_detector.rb b/app/lib/language_detector.rb
index 2cc8ac6158dcab83ac8e644ab3e08befe155858b..40452eddc96df180acc6b964b85e99cb83b3da6c 100644 (file)
--- 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 d4e4931e6703110dd515f169322fa6c3eabec44e..fa1636e41116582c92a199eb82b99e2b744bebd2 100644 (file)
--- 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 7ea2bb3e53a524932020c98d6569956a94d97620..554ad49ce296222b44c916cbf05c5a43d3575424 100644 (file)
--- 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 b56c5a3212e18ce9ec98a5be3fa90e0e47513b67..d036f19256b2de6ac1b7f014ba9059f2e884e2be 100644 (file)
--- 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 8b296cc704876382736ec632f088136862edf193..736a6078dd637a76bd0bb1ae0261c50c7dd4c4ee 100644 (file)
--- 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 (file)
index 0000000..6e9b4e1
--- /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