From 7efc33b909da0ec8a75521efab4357280a2f40ff Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 7 Feb 2024 14:35:37 +0100 Subject: [PATCH] Move HTTP Signature parsing code to its own class (#28932) --- .../activitypub/inboxes_controller.rb | 5 +- .../concerns/signature_verification.rb | 41 +------------- app/lib/signature_parser.rb | 53 +++++++++++++++++++ spec/lib/signature_parser_spec.rb | 34 ++++++++++++ 4 files changed, 91 insertions(+), 42 deletions(-) create mode 100644 app/lib/signature_parser.rb create mode 100644 spec/lib/signature_parser_spec.rb diff --git a/app/controllers/activitypub/inboxes_controller.rb b/app/controllers/activitypub/inboxes_controller.rb index ba85e0a722..e8b0f47cde 100644 --- a/app/controllers/activitypub/inboxes_controller.rb +++ b/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 = SignatureParamsTransformer.new.apply(tree) + params = SignatureParser.parse(raw_params) ActivityPub::PrepareFollowersSynchronizationService.new.call(signed_request_account, params) - rescue Parslet::ParseFailed + rescue SignatureParser::ParsingError Rails.logger.warn 'Error parsing Collection-Synchronization header' end diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 92f1eb5a16..3155866271 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/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 - raw_signature = request.headers['Signature'] - tree = SignatureParamsParser.new.parse(raw_signature) - SignatureParamsTransformer.new.apply(tree) - end - rescue Parslet::ParseFailed + @signature_params ||= SignatureParser.parse(request.headers['Signature']) + rescue SignatureParser::ParsingError raise SignatureVerificationError, 'Error parsing signature parameters' end diff --git a/app/lib/signature_parser.rb b/app/lib/signature_parser.rb new file mode 100644 index 0000000000..c09ab0c841 --- /dev/null +++ b/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 diff --git a/spec/lib/signature_parser_spec.rb b/spec/lib/signature_parser_spec.rb new file mode 100644 index 0000000000..183b486d56 --- /dev/null +++ b/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