Commit 5dc2516

@@ -49,13 +49,24 @@ module Saml
 
           return document.signature_verified! if provider.verify(
             algorithm_for(params[:SigAlg]),
-            decode(signature),
+            decode(signature_from(signature)),
             canonicalize(params)
           )
 
           raise ArgumentError, 'Invalid Signature'
         end
 
+        # The signature arrives percent encoded from any conformant peer, but
+        # versions of this library before 1.4.1 emitted it raw. A percent sign
+        # is not in the base64 alphabet, so its presence distinguishes the two
+        # without a guess. Unescaping unconditionally would corrupt a raw
+        # signature, because CGI.unescape turns + into a space.
+        #
+        # @param signature [String] the Signature query parameter.
+        def signature_from(signature)
+          signature.include?('%') ? unescape(signature) : signature
+        end
+
         def canonicalize(params)
           %i[SAMLRequest SAMLResponse RelayState SigAlg].map do |key|
             value = params[key]

@@ -21,7 +21,7 @@ module Saml
           destination = document.destination
           if configuration.sign?
             payload = canonicalize(document, relay_state)
-            "#{destination}?#{payload}&Signature=#{signature_for(payload)}"
+            "#{destination}?#{payload}&Signature=#{escape(signature_for(payload))}"
           else
             "#{destination}?" + to_query_string(
               document.query_string_parameter => serialize(document.to_xml),

@@ -154,6 +154,41 @@ RSpec.describe Saml::Kit::Bindings::HttpRedirect do
       expect(result).to be_valid
     end
 
+    it 'url encodes the signature it generates' do
+      configuration = Saml::Kit::Configuration.new do |config|
+        config.entity_id = entity_id
+        config.generate_key_pair_for(use: :signing)
+      end
+      url, = subject.serialize(
+        Saml::Kit::AuthenticationRequest.builder(configuration: configuration)
+      )
+      signature = query_params_from(url)['Signature']
+
+      expect(signature).not_to match(%r{[+/=]})
+    end
+
+    it 'verifies a signature that arrives url encoded' do
+      configuration = Saml::Kit::Configuration.new do |config|
+        config.entity_id = entity_id
+        config.generate_key_pair_for(use: :signing)
+      end
+      provider = Saml::Kit::IdentityProviderMetadata.build(configuration: configuration)
+      url, = subject.serialize(
+        Saml::Kit::AuthenticationRequest.builder(configuration: configuration)
+      )
+      allow(configuration.registry).to receive(:metadata_for).with(entity_id).and_return(provider)
+
+      # Every other SAML implementation percent encodes each query parameter,
+      # including the signature. Base64 alphabet characters +, / and = all
+      # require escaping, so a conformant peer never sends them raw.
+      query_params = query_params_from(url)
+      query_params['Signature'] = CGI.escape(CGI.unescape(query_params['Signature']))
+
+      result = subject.deserialize(query_params, configuration: configuration)
+      expect(result).to be_signed
+      expect(result).to be_trusted
+    end
+
     it 'returns an unverfied document when the provider is unknown' do
       configuration = Saml::Kit::Configuration.new do |config|
         config.generate_key_pair_for(use: :signing)

@@ -64,7 +64,7 @@ RSpec.describe Saml::Kit::Bindings::UrlBuilder do
                     "&SigAlg=#{query_params['SigAlg']}"
           private_key = configuration.private_keys(use: :signing).last
           expected_signature = Base64.strict_encode64(private_key.sign(OpenSSL::Digest::SHA256.new, payload))
-          expect(query_params['Signature']).to eql(expected_signature)
+          expect(query_params['Signature']).to eql(CGI.escape(expected_signature))
         end
 
         it 'generates the signature correctly when the relay state is absent' do
@@ -76,7 +76,7 @@ RSpec.describe Saml::Kit::Bindings::UrlBuilder do
                     "&SigAlg=#{query_params['SigAlg']}"
           private_key = configuration.private_keys(use: :signing).last
           expected_signature = Base64.strict_encode64(private_key.sign(OpenSSL::Digest::SHA256.new, payload))
-          expect(query_params['Signature']).to eql(expected_signature)
+          expect(query_params['Signature']).to eql(CGI.escape(expected_signature))
         end
       end
     end

@@ -70,7 +70,9 @@ GEM
     lint_roller (1.1.0)
     logger (1.6.6)
     mini_portile2 (2.8.8)
-    minitest (5.25.5)
+    minitest (6.0.6)
+      drb (~> 2.0)
+      prism (~> 1.5)
     net-hippie (1.2.0)
       logger (~> 1.0)
     nokogiri (1.18.6)
@@ -83,7 +85,7 @@ GEM
     pp (0.6.2)
       prettyprint
     prettyprint (0.2.0)
-    prism (1.4.0)
+    prism (1.9.0)
     psych (5.2.3)
       date
       stringio
@@ -148,7 +150,7 @@ GEM
       concurrent-ruby (~> 1.0)
     unicode-display_width (3.1.4)
       unicode-emoji (~> 4.0, >= 4.0.4)
-    unicode-emoji (4.0.4)
+    unicode-emoji (4.2.0)
     uri (1.0.3)
     webmock (3.25.1)
       addressable (>= 2.8.0)