Improve `from` search prefix error handling (#37)

* Make `from` search prefix more robust (addresses mastodon/mastodon#17941)

* Improve robustness for account string validation

Using unsupported prefixes now reports a 422; searching for posts from an
account the instance is not aware of reports a 404. TODO: The UI for this
on the front end is abysmal.

Searching `from:username@domain` now succeeds when `domain` is the local
domain; searching `from:@username(@domain)?` now works as expected.

* Satisfy upstream rubcocp

* Unbreak upstream tests

* Make account string validation consistent with mention processing

We previously matched on one-character domains and domains ending with
`[\.-]`, allowing `from:@a@a` and `from:@a@a-` searches to cause an
account lookup. This commit will raise a syntax error in both cases, as
MENTION_RE would never match them.

* Refactor `from` prefix error handling.

Incorporates changes suggested in #37. In doing so, adopts an error
handling style more consistent with the existing codebase (for which I
must thank @ClearlyClaire).

Removes new code no longer in use.

diff --git a/app/controllers/api/v2/search_controller.rb b/app/controllers/api/v2/search_controller.rb
index ddcf92200966ffef3913006a807a1c11a087e78b..116f53618049d3fab795e419ef3f5c3ce52ced40 100644 (file)
--- a/app/controllers/api/v2/search_controller.rb
+++ b/app/controllers/api/v2/search_controller.rb
@@ -11,6 +11,16 @@ class Api::V2::SearchController < Api::BaseController
   def index
     @search = Search.new(search_results)
     render json: @search, serializer: REST::SearchSerializer
+
+  # TODO: in the front end, these will show a toast that is only barely helpful
+  # TODO: semantics?
+
+  # user searched with a prefix that does exist
+  rescue Mastodon::SyntaxError
+    unprocessable_entity
+  # user searched for posts from an account the instance is not aware of
+  rescue ActiveRecord::NotFound
+    not_found
   end
 
   private

diff --git a/app/lib/search_query_transformer.rb b/app/lib/search_query_transformer.rb
index c685d7b6fd7b0dc0cf86c83d675fa01ed5906067..c65da48aecde7418d548b86950f078ec63f90842 100644 (file)
--- a/app/lib/search_query_transformer.rb
+++ b/app/lib/search_query_transformer.rb
@@ -88,14 +88,12 @@ class SearchQueryTransformer < Parslet::Transform
       case prefix
       when 'from'
         @filter = :account_id
-        username, domain = term.split('@')
-        account = Account.find_remote(username, domain)
-
-        raise "Account not found: #{term}" unless account
-
+        username, domain = Account.validate_account_string!(term)
+        account = Account.find_local_or_remote!(username, domain)
         @term = account.id
+      # TODO: consider, instead of erroring on non-prefixes, treating them as words to search for. motivating case: searching for `https://.*`
       else
-        raise "Unknown prefix: #{prefix}"
+        raise Mastodon::SyntaxError, "Unknown prefix: #{prefix}"
       end
     end
   end

diff --git a/app/models/concerns/account_finder_concern.rb b/app/models/concerns/account_finder_concern.rb
index 0dadddad12abf3940e9c91c6ff655b1ecd4b0986..7e165d3ab92cfe4ef1f4be6fd71c2f1a1a956071 100644 (file)
--- a/app/models/concerns/account_finder_concern.rb
+++ b/app/models/concerns/account_finder_concern.rb
@@ -12,6 +12,10 @@ module AccountFinderConcern
       find_remote(username, domain) || raise(ActiveRecord::RecordNotFound)
     end
 
+    def find_local_or_remote!(username, domain)
+      find_local_or_remote(username, domain) || raise(ActiveRecord::RecordNotFound)
+    end
+
     def representative
       Account.find(-99)
     rescue ActiveRecord::RecordNotFound
@@ -25,8 +29,29 @@ module AccountFinderConcern
     def find_remote(username, domain)
       AccountFinder.new(username, domain).account
     end
+
+    def find_local_or_remote(username, domain)
+      TagManager.instance.local_domain?(domain) ? find_local(username) : find_remote(username, domain)
+    end
+
+    def validate_account_string!(account_string)
+      match = ACCOUNT_STRING_RE.match(account_string)
+      raise Mastodon::SyntaxError if match.nil? || match[:username].nil?
+
+      [match[:username], match[:domain]]
+    end
   end
 
+  # TODO: where should this go?
+  #
+  # this is adapted from MENTION_RE to
+  #   + capture only a mention,
+  #   + not require the mention to begin with an @,
+  #   + not match if there is anything surrounding the mention, and
+  #   + add named subgroup matches
+  # it would be ideal to explicitly refer to MENTION_RE, or a more fundamental regexp that we refactor MENTION_RE to incorporate
+  ACCOUNT_STRING_RE = /^@?(?<username>#{Account::USERNAME_RE})(?:@(?<domain>[[:word:]\.\-]+[[:word:]]+))?$/i
+
   class AccountFinder
     attr_reader :username, :domain
 

diff --git a/lib/exceptions.rb b/lib/exceptions.rb
index eb472abaabfb67d9a853d8c38c3b77325ffdad29..0c677b6605d24689d75dee2ed47715d133d2364a 100644 (file)
--- a/lib/exceptions.rb
+++ b/lib/exceptions.rb
@@ -10,6 +10,7 @@ module Mastodon
   class StreamValidationError < ValidationError; end
   class RaceConditionError < Error; end
   class RateLimitExceededError < Error; end
+  class SyntaxError < Error; end
 
   class UnexpectedResponseError < Error
     attr_reader :response