Revert "Strip query params" (#223)

* Revert "Strip query params (#222)"

This reverts commit 434b0e9.

* an empty commit

Author: mgomes

CHANGELOG.md

@@ -5,7 +5,6 @@
 - **Drop support for Ruby < 3.2** - Now requires Ruby 3.2, 3.3, or 3.4+
 - **Drop support for Rails 6.x** - Now requires Rails 7.2+ or 8.0+
 - **Remove deprecated Ruby 2.x compatibility code**
-- **Canonical string no longer includes query parameters** – signatures now use only the request path. During a staged rollout you can temporarily accept legacy query-aware signatures by setting `ApiAuth.legacy_query_params_compatibility = true`.
 
 ## New Features
 
@@ -31,8 +30,8 @@
   - RSpec ~> 3.13
   - Rake ~> 13.0
   - Rest-Client ~> 2.1
-  - Typhoeus ~> 1.4
 - Remove implicit ActiveSupport requirement from runtime
+  - Typhoeus ~> 1.4
 
 # 2.6.0 (2025-01-18)
 

README.md

@@ -30,8 +30,6 @@ automatically added to the request. The canonical string is computed as follows:
 canonical_string = "#{http method},#{content-type},#{X-Authorization-Content-SHA256},#{request URI},#{timestamp}"
 ```
 
-> **Note:** As of v3.0 the "request URI" component above is just the path (query parameters are excluded) so signatures remain stable even when intermediaries rewrite a query string.
-
 e.g.,
 
 ```ruby
@@ -60,16 +58,6 @@ access id that was attached in the header. The access id can be any integer or
 string that uniquely identifies the client. The signed request expires after 15
 minutes in order to avoid replay attacks.
 
-### Legacy query parameter compatibility
-
-Versions prior to 3.0 included the query string inside the canonical request URI. If you have to roll out the 3.0 change gradually across multiple services, you can temporarily enable support for legacy signatures on the server side:
-
-```ruby
-ApiAuth.legacy_query_params_compatibility = true
-```
-
-With the flag disabled (the default) only the path segment is considered part of the canonical string.
-
 ## References
 
 * [Hash functions](https://en.wikipedia.org/wiki/Cryptographic_hash_function)

lib/api_auth/base.rb

@@ -10,12 +10,6 @@ module ApiAuth
   class << self
     include Helpers
 
-    attr_writer :legacy_query_params_compatibility
-
-    def legacy_query_params_compatibility?
-      !!@legacy_query_params_compatibility
-    end
-
     # Signs an HTTP request using the client's access id and secret key.
     # Returns the HTTP request object with the modified headers.
     #
@@ -96,16 +90,9 @@ def signatures_match?(headers, secret_key, options)
       options = { digest: digest }.merge(options)
 
       header_sig = match_data[3]
-      calculated_sig = hmac_signature(headers, secret_key, options, include_query: false)
-
-      return true if secure_equals?(header_sig, calculated_sig, secret_key)
+      calculated_sig = hmac_signature(headers, secret_key, options)
 
-      if legacy_query_params_compatibility?
-        legacy_sig = hmac_signature(headers, secret_key, options, include_query: true)
-        return true if secure_equals?(header_sig, legacy_sig, secret_key)
-      end
-
-      false
+      secure_equals?(header_sig, calculated_sig, secret_key)
     end
 
     def secure_equals?(m1, m2, key)
@@ -117,15 +104,15 @@ def sha1_hmac(key, message)
       OpenSSL::HMAC.digest(digest, key, message)
     end
 
-    def hmac_signature(headers, secret_key, options, include_query: false)
-      canonical_string = headers.canonical_string(options[:override_http_method], options[:headers_to_sign], include_query: include_query)
+    def hmac_signature(headers, secret_key, options)
+      canonical_string = headers.canonical_string(options[:override_http_method], options[:headers_to_sign])
       digest = OpenSSL::Digest.new(options[:digest])
       b64_encode(OpenSSL::HMAC.digest(digest, secret_key, canonical_string))
     end
 
     def auth_header(headers, access_id, secret_key, options)
       hmac_string = "-HMAC-#{options[:digest].upcase}" unless options[:digest] == 'sha1'
-      "APIAuth#{hmac_string} #{access_id}:#{hmac_signature(headers, secret_key, options, include_query: false)}"
+      "APIAuth#{hmac_string} #{access_id}:#{hmac_signature(headers, secret_key, options)}"
     end
 
     def parse_auth_header(auth_header)

lib/api_auth/headers.rb

@@ -70,7 +70,7 @@ def timestamp
       @request.timestamp
     end
 
-    def canonical_string(override_method = nil, headers_to_sign = [], include_query: false)
+    def canonical_string(override_method = nil, headers_to_sign = [])
       request_method = override_method || @request.http_method
 
       raise ArgumentError, 'unable to determine the http method from the request, please supply an override' if request_method.nil?
@@ -80,7 +80,7 @@ def canonical_string(override_method = nil, headers_to_sign = [], include_query:
       canonical_array = [request_method.upcase,
                          @request.content_type,
                          @request.content_hash,
-                         parse_uri(@request.original_uri || @request.request_uri, include_query: include_query),
+                         parse_uri(@request.original_uri || @request.request_uri),
                          @request.timestamp]
 
       if headers_to_sign.is_a?(Array) && headers_to_sign.any?
@@ -125,8 +125,8 @@ def sign_header(header)
 
     private
 
-    def parse_uri(uri, include_query: false)
-      canonical_request_uri(uri, nil, include_query: include_query)
+    def parse_uri(uri)
+      canonical_request_uri(uri)
     end
   end
 end

lib/api_auth/helpers.rb

@@ -36,17 +36,13 @@ def value_present?(value)
       !value_blank?(value)
     end
 
-    def canonical_request_uri(base_url, additional_query = nil, include_query: false)
+    def canonical_request_uri(base_url, additional_query = nil)
       base = base_url.to_s
       return '/' if base.empty?
 
       uri = URI.parse(base)
-      if include_query
-        merged_query = merge_query_strings(uri.query, normalize_query_component(additional_query))
-        uri.query = merged_query if value_present?(merged_query)
-      else
-        uri.query = nil
-      end
+      merged_query = merge_query_strings(uri.query, normalize_query_component(additional_query))
+      uri.query = merged_query if value_present?(merged_query)
 
       result = uri.respond_to?(:request_uri) ? uri.request_uri : uri.to_s
       value_present?(result) ? result : '/'

lib/api_auth/request_drivers/typhoeus_request.rb

@@ -55,7 +55,7 @@ def original_uri
       end
 
       def request_uri
-        canonical_request_uri(@request.base_url, params_query, include_query: true)
+        canonical_request_uri(@request.base_url, params_query)
       end
 
       def set_date

spec/api_auth_spec.rb

@@ -176,44 +176,6 @@ def hmac(secret_key, request, canonical_string = nil, digest = 'sha1')
         expect(ApiAuth.authentic?(signed_request, '123', headers_to_sign: %w[HTTP_X_FORWARDED_FOR])).to eq true
       end
     end
-
-    context 'legacy query parameter compatibility' do
-      let(:legacy_request) do
-        Net::HTTP::Get.new('/resource.xml?foo=bar&bar=foo',
-                           'content-type' => 'text/plain',
-                           'date' => Time.now.utc.httpdate)
-      end
-      let(:access_id) { 'legacy' }
-      let(:secret_key) { 'secret' }
-      let(:headers) { ApiAuth::Headers.new(legacy_request) }
-
-      def legacy_signature(headers, secret)
-        canonical = headers.canonical_string(nil, [], include_query: true)
-        digest = OpenSSL::Digest.new('sha1')
-        ApiAuth.b64_encode(OpenSSL::HMAC.digest(digest, secret, canonical))
-      end
-
-      before do
-        sig = legacy_signature(headers, secret_key)
-        legacy_request['Authorization'] = "APIAuth #{access_id}:#{sig}"
-      end
-
-      around do |example|
-        original = ApiAuth.legacy_query_params_compatibility?
-        example.run
-        ApiAuth.legacy_query_params_compatibility = original
-      end
-
-      it 'rejects legacy signatures by default' do
-        ApiAuth.legacy_query_params_compatibility = false
-        expect(ApiAuth.authentic?(legacy_request, secret_key)).to eq false
-      end
-
-      it 'accepts legacy signatures when compatibility is enabled' do
-        ApiAuth.legacy_query_params_compatibility = true
-        expect(ApiAuth.authentic?(legacy_request, secret_key)).to eq true
-      end
-    end
   end
 
   describe '.access_id' do

spec/headers_spec.rb

@@ -34,8 +34,8 @@
       context 'uri has a string matching https:// in it' do
         let(:uri) { 'https://google.com/?redirect_to=https://www.example.com'.freeze }
 
-        it 'return / as canonical string path' do
-          expect(subject.canonical_string).to eq('GET,,,/,')
+        it 'return /?redirect_to=https://www.example.com as canonical string path' do
+          expect(subject.canonical_string).to eq('GET,,,/?redirect_to=https://www.example.com,')
         end
 
         it 'does not change request url (by removing host)' do
@@ -121,7 +121,7 @@
 
         context 'the driver uses the original_uri' do
           it 'constructs the canonical_string with the original_uri' do
-            expect(headers.canonical_string).to eq 'GET,text/html,12345,/api/resource.xml,Mon, 23 Jan 1984 03:29:56 GMT'
+            expect(headers.canonical_string).to eq 'GET,text/html,12345,/api/resource.xml?foo=bar&bar=foo,Mon, 23 Jan 1984 03:29:56 GMT'
           end
         end
       end
@@ -147,7 +147,7 @@
         context 'the driver uses the original_uri' do
           it 'constructs the canonical_string with the original_uri' do
             expect(headers.canonical_string(nil, %w[X-FORWARDED-FOR]))
-              .to eq 'GET,text/html,12345,/resource.xml,Mon, 23 Jan 1984 03:29:56 GMT,192.168.1.1'
+              .to eq 'GET,text/html,12345,/resource.xml?bar=foo&foo=bar,Mon, 23 Jan 1984 03:29:56 GMT,192.168.1.1'
           end
         end
       end