Move HTTP Signature parsing code to its own class (#28932)

app/controllers/activitypub/inboxes_controller.rb

@@ -62,11 +62,10 @@ class ActivityPub::InboxesController < ActivityPub::BaseController
     return if raw_params.blank? || ENV['DISABLE_FOLLOWERS_SYNCHRONIZATION'] == 'true' || signed_request_account.nil?
 
     # Re-using the syntax for signature parameters
-    tree   = SignatureParamsParser.new.parse(raw_params)
+    params = SignatureParser.parse(raw_params)
-    params = SignatureParamsTransformer.new.apply(tree)
 
     ActivityPub::PrepareFollowersSynchronizationService.new.call(signed_request_account, params)
-  rescue Parslet::ParseFailed
+  rescue SignatureParser::ParsingError
     Rails.logger.warn 'Error parsing Collection-Synchronization header'
   end
 

app/controllers/concerns/signature_verification.rb

@@ -12,39 +12,6 @@ module SignatureVerification
 
   class SignatureVerificationError < StandardError; end
 
-  class SignatureParamsParser < Parslet::Parser
-    rule(:token)         { match("[0-9a-zA-Z!#$%&'*+.^_`|~-]").repeat(1).as(:token) }
-    rule(:quoted_string) { str('"') >> (qdtext | quoted_pair).repeat.as(:quoted_string) >> str('"') }
-    # qdtext and quoted_pair are not exactly according to spec but meh
-    rule(:qdtext)        { match('[^\\\\"]') }
-    rule(:quoted_pair)   { str('\\') >> any }
-    rule(:bws)           { match('\s').repeat }
-    rule(:param)         { (token.as(:key) >> bws >> str('=') >> bws >> (token | quoted_string).as(:value)).as(:param) }
-    rule(:comma)         { bws >> str(',') >> bws }
-    # Old versions of node-http-signature add an incorrect "Signature " prefix to the header
-    rule(:buggy_prefix)  { str('Signature ') }
-    rule(:params)        { buggy_prefix.maybe >> (param >> (comma >> param).repeat).as(:params) }
-    root(:params)
-  end
-
-  class SignatureParamsTransformer < Parslet::Transform
-    rule(params: subtree(:param)) do
-      (param.is_a?(Array) ? param : [param]).each_with_object({}) { |(key, value), hash| hash[key] = value }
-    end
-
-    rule(param: { key: simple(:key), value: simple(:val) }) do
-      [key, val]
-    end
-
-    rule(quoted_string: simple(:string)) do
-      string.to_s
-    end
-
-    rule(token: simple(:string)) do
-      string.to_s
-    end
-  end
-
   def require_account_signature!
     render json: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_account
   end
@@ -135,12 +102,8 @@ module SignatureVerification
   end
 
   def signature_params
-    @signature_params ||= begin
+    @signature_params ||= SignatureParser.parse(request.headers['Signature'])
-      raw_signature = request.headers['Signature']
+  rescue SignatureParser::ParsingError
-      tree          = SignatureParamsParser.new.parse(raw_signature)
-      SignatureParamsTransformer.new.apply(tree)
-    end
-  rescue Parslet::ParseFailed
     raise SignatureVerificationError, 'Error parsing signature parameters'
   end
 

app/lib/signature_parser.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+class SignatureParser
+  class ParsingError < StandardError; end
+
+  # The syntax of this header is defined in:
+  # https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-12#section-4
+  # See https://datatracker.ietf.org/doc/html/rfc7235#appendix-C
+  # and https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
+
+  # In addition, ignore a `Signature ` string prefix that was added by old versions
+  # of `node-http-signatures`
+
+  class SignatureParamsParser < Parslet::Parser
+    rule(:token)         { match("[0-9a-zA-Z!#$%&'*+.^_`|~-]").repeat(1).as(:token) }
+    rule(:quoted_string) { str('"') >> (qdtext | quoted_pair).repeat.as(:quoted_string) >> str('"') }
+    # qdtext and quoted_pair are not exactly according to spec but meh
+    rule(:qdtext)        { match('[^\\\\"]') }
+    rule(:quoted_pair)   { str('\\') >> any }
+    rule(:bws)           { match('\s').repeat }
+    rule(:param)         { (token.as(:key) >> bws >> str('=') >> bws >> (token | quoted_string).as(:value)).as(:param) }
+    rule(:comma)         { bws >> str(',') >> bws }
+    # Old versions of node-http-signature add an incorrect "Signature " prefix to the header
+    rule(:buggy_prefix)  { str('Signature ') }
+    rule(:params)        { buggy_prefix.maybe >> (param >> (comma >> param).repeat).as(:params) }
+    root(:params)
+  end
+
+  class SignatureParamsTransformer < Parslet::Transform
+    rule(params: subtree(:param)) do
+      (param.is_a?(Array) ? param : [param]).each_with_object({}) { |(key, value), hash| hash[key] = value }
+    end
+
+    rule(param: { key: simple(:key), value: simple(:val) }) do
+      [key, val]
+    end
+
+    rule(quoted_string: simple(:string)) do
+      string.to_s
+    end
+
+    rule(token: simple(:string)) do
+      string.to_s
+    end
+  end
+
+  def self.parse(raw_signature)
+    tree = SignatureParamsParser.new.parse(raw_signature)
+    SignatureParamsTransformer.new.apply(tree)
+  rescue Parslet::ParseFailed
+    raise ParsingError
+  end
+end

spec/lib/signature_parser_spec.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe SignatureParser do
+  describe '.parse' do
+    subject { described_class.parse(header) }
+
+    context 'with Signature headers conforming to draft-cavage-http-signatures-12' do
+      let(:header) do
+        # This example signature string deliberately mixes uneven spacing
+        # and quoting styles to ensure everything is covered
+        'keyId = "https://remote.domain/users/bob#main-key",algorithm=  rsa-sha256 ,  headers="host date digest (request-target)",signature="gmhMjgMROGElJU3fpehV2acD5kMHeELi8EFP2UPHOdQ54H0r55AxIpji+J3lPe+N2qSb/4H1KXIh6f0lRu8TGSsu12OQmg5hiO8VA9flcA/mh9Lpk+qwlQZIPRqKP9xUEfqD+Z7ti5wPzDKrWAUK/7FIqWgcT/mlqB1R1MGkpMFc/q4CIs2OSNiWgA4K+Kp21oQxzC2kUuYob04gAZ7cyE/FTia5t08uv6lVYFdRsn4XNPn1MsHgFBwBMRG79ng3SyhoG4PrqBEi5q2IdLq3zfre/M6He3wlCpyO2VJNdGVoTIzeZ0Zz8jUscPV3XtWUchpGclLGSaKaq/JyNZeiYQ=="' # rubocop:disable Layout/LineLength
+      end
+
+      it 'correctly parses the header' do
+        expect(subject).to eq({
+          'keyId' => 'https://remote.domain/users/bob#main-key',
+          'algorithm' => 'rsa-sha256',
+          'headers' => 'host date digest (request-target)',
+          'signature' => 'gmhMjgMROGElJU3fpehV2acD5kMHeELi8EFP2UPHOdQ54H0r55AxIpji+J3lPe+N2qSb/4H1KXIh6f0lRu8TGSsu12OQmg5hiO8VA9flcA/mh9Lpk+qwlQZIPRqKP9xUEfqD+Z7ti5wPzDKrWAUK/7FIqWgcT/mlqB1R1MGkpMFc/q4CIs2OSNiWgA4K+Kp21oQxzC2kUuYob04gAZ7cyE/FTia5t08uv6lVYFdRsn4XNPn1MsHgFBwBMRG79ng3SyhoG4PrqBEi5q2IdLq3zfre/M6He3wlCpyO2VJNdGVoTIzeZ0Zz8jUscPV3XtWUchpGclLGSaKaq/JyNZeiYQ==', # rubocop:disable Layout/LineLength
+        })
+      end
+    end
+
+    context 'with a malformed Signature header' do
+      let(:header) { 'hello this is malformed!' }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(SignatureParser::ParsingError)
+      end
+    end
+  end
+end