Fix: Incorrect media-type Accept header now correctly returns 406 with strict: true.

If you've configured Grape to use accept header versioning with strict
equal to true, Grape should return a 406 response when making a request
with an incorrect version header.  However, the system was not
responding with a 406 when using the header 'Accept:application/xml'.

Adding relevant line to changelog.

Refactoring the middleware version header class:
- Breaking up the before method into more manageable pieces, removing some
of the conditional (cyclomatic) complexity
- fixing some line lengths
- removing some local variables from methods

Author: elliotlarson

CHANGELOG.md

@@ -8,7 +8,7 @@
 #### Fixes
 
 * [#1109](https://github.com/ruby-grape/grape/pull/1109): Memoize Virtus attribute and fix memory leak - [@marshall-lee](https://github.com/marshall-lee).
-* Your contribution here.
+* [#1101](https://github.com/intridea/grape/pull/1101): Fix: Incorrect media-type `Accept` header now correctly returns 406 with `strict: true` - [@elliotlarson](https://github.com/elliotlarson).
 
 0.13.0 (8/10/2015)
 ==================

lib/grape/middleware/versioner/header.rb

@@ -22,54 +22,78 @@ module Versioner
       # X-Cascade header to alert Rack::Mount to attempt the next matched
       # route.
       class Header < Base
+        VENDOR_VERSION_HEADER_REGEX =
+          /\Avnd\.([a-z0-9*.]+)(?:-([a-z0-9*\-.]+))?(?:\+([a-z0-9*\-.+]+))?\z/
+
         def before
-          header = rack_accept_header
+          strict_header_checks if strict?
 
-          if strict?
-            # If no Accept header:
-            if header.qvalues.empty?
-              fail Grape::Exceptions::InvalidAcceptHeader.new('Accept header must be set.', error_headers)
-            end
-            # Remove any acceptable content types with ranges.
-            header.qvalues.reject! do |media_type, _|
-              Rack::Accept::Header.parse_media_type(media_type).find { |s| s == '*' }
-            end
-            # If all Accept headers included a range:
-            if header.qvalues.empty?
-              fail Grape::Exceptions::InvalidAcceptHeader.new('Accept header must not contain ranges ("*").',
-                                                              error_headers)
-            end
+          if media_type
+            media_type_header_handler
+          elsif headers_contain_wrong_vendor_or_version?
+            fail_with_invalid_accept_header!('API vendor or version not found.')
           end
+        end
 
-          media_type = header.best_of available_media_types
+        private
 
-          if media_type
-            type, subtype = Rack::Accept::Header.parse_media_type media_type
-            env['api.type']    = type
-            env['api.subtype'] = subtype
-
-            if /\Avnd\.([a-z0-9*.]+)(?:-([a-z0-9*\-.]+))?(?:\+([a-z0-9*\-.+]+))?\z/ =~ subtype
-              env['api.vendor']  = Regexp.last_match[1]
-              env['api.version'] = Regexp.last_match[2]
-              env['api.format']  = Regexp.last_match[3]  # weird that Grape::Middleware::Formatter also does this
-            end
-          # If none of the available content types are acceptable:
-          elsif strict?
-            fail Grape::Exceptions::InvalidAcceptHeader.new('406 Not Acceptable', error_headers)
-          # If all acceptable content types specify a vendor or version that doesn't exist:
-          elsif header.values.all? { |header_value| has_vendor?(header_value) || version?(header_value) }
-            fail Grape::Exceptions::InvalidAcceptHeader.new('API vendor or version not found.', error_headers)
+        def strict_header_checks
+          strict_accept_header_presence_check
+          strict_verion_vendor_accept_header_presence_check
+        end
+
+        def strict_accept_header_presence_check
+          return unless header.qvalues.empty?
+          fail_with_invalid_accept_header!('Accept header must be set.')
+        end
+
+        def strict_verion_vendor_accept_header_presence_check
+          return unless versions.present?
+          return if an_accept_header_with_version_and_vendor_is_present?
+          fail_with_invalid_accept_header!('API vendor or version not found.')
+        end
+
+        def an_accept_header_with_version_and_vendor_is_present?
+          header.qvalues.keys.any? do |h|
+            VENDOR_VERSION_HEADER_REGEX =~ h.sub('application/', '')
           end
         end
 
-        private
+        def header
+          @header ||= rack_accept_header
+        end
+
+        def media_type
+          @media_type ||= header.best_of(available_media_types)
+        end
+
+        def media_type_header_handler
+          type, subtype = Rack::Accept::Header.parse_media_type(media_type)
+          env['api.type'] = type
+          env['api.subtype'] = subtype
+
+          if VENDOR_VERSION_HEADER_REGEX =~ subtype
+            env['api.vendor'] = Regexp.last_match[1]
+            env['api.version'] = Regexp.last_match[2]
+            # weird that Grape::Middleware::Formatter also does this
+            env['api.format'] = Regexp.last_match[3]
+          end
+        end
+
+        def fail_with_invalid_accept_header!(message)
+          fail Grape::Exceptions::InvalidAcceptHeader
+            .new(message, error_headers)
+        end
 
         def available_media_types
           available_media_types = []
 
           content_types.each do |extension, _media_type|
             versions.reverse_each do |version|
-              available_media_types += ["application/vnd.#{vendor}-#{version}+#{extension}", "application/vnd.#{vendor}-#{version}"]
+              available_media_types += [
+                "application/vnd.#{vendor}-#{version}+#{extension}",
+                "application/vnd.#{vendor}-#{version}"
+              ]
             end
             available_media_types << "application/vnd.#{vendor}+#{extension}"
           end
@@ -83,30 +107,42 @@ def available_media_types
           available_media_types.flatten
         end
 
+        def headers_contain_wrong_vendor_or_version?
+          header.values.all? do |header_value|
+            has_vendor?(header_value) || version?(header_value)
+          end
+        end
+
         def rack_accept_header
           Rack::Accept::MediaType.new env[Grape::Http::Headers::HTTP_ACCEPT]
         rescue RuntimeError => e
-          raise Grape::Exceptions::InvalidAcceptHeader.new(e.message, error_headers)
+          fail_with_invalid_accept_header!(e.message)
         end
 
         def versions
           options[:versions] || []
         end
 
         def vendor
-          options[:version_options] && options[:version_options][:vendor]
+          version_options && version_options[:vendor]
         end
 
         def strict?
-          options[:version_options] && options[:version_options][:strict]
+          version_options && version_options[:strict]
+        end
+
+        def version_options
+          options[:version_options]
         end
 
-        # By default those errors contain an `X-Cascade` header set to `pass`, which allows nesting and stacking
-        # of routes (see [Rack::Mount](https://github.com/josh/rack-mount) for more information). To prevent
-        # this behavior, and not add the `X-Cascade` header, one can set the `:cascade` option to `false`.
+        # By default those errors contain an `X-Cascade` header set to `pass`,
+        # which allows nesting and stacking of routes
+        # (see [Rack::Mount](https://github.com/josh/rack-mount) for more
+        # information). To prevent # this behavior, and not add the `X-Cascade`
+        # header, one can set the `:cascade` option to `false`.
         def cascade?
-          if options[:version_options] && options[:version_options].key?(:cascade)
-            !!options[:version_options][:cascade]
+          if version_options && version_options.key?(:cascade)
+            !!version_options[:cascade]
           else
             true
           end
@@ -119,14 +155,14 @@ def error_headers
         # @param [String] media_type a content type
         # @return [Boolean] whether the content type sets a vendor
         def has_vendor?(media_type)
-          _, subtype = Rack::Accept::Header.parse_media_type media_type
+          _, subtype = Rack::Accept::Header.parse_media_type(media_type)
           subtype[/\Avnd\.[a-z0-9*.]+/]
         end
 
         # @param [String] media_type a content type
         # @return [Boolean] whether the content type sets an API version
         def version?(media_type)
-          _, subtype = Rack::Accept::Header.parse_media_type media_type
+          _, subtype = Rack::Accept::Header.parse_media_type(media_type)
           subtype[/\Avnd\.[a-z0-9*.]+-[a-z0-9*\-.]+/]
         end
       end

spec/grape/middleware/versioner/header_spec.rb

@@ -184,24 +184,6 @@
       end
     end
 
-    it 'fails with 406 Not Acceptable if type is a range' do
-      expect { subject.call('HTTP_ACCEPT' => '*/*').last }.to raise_exception do |exception|
-        expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader)
-        expect(exception.headers).to eql('X-Cascade' => 'pass')
-        expect(exception.status).to eql 406
-        expect(exception.message).to include('Accept header must not contain ranges ("*").')
-      end
-    end
-
-    it 'fails with 406 Not Acceptable if subtype is a range' do
-      expect { subject.call('HTTP_ACCEPT' => 'application/*').last }.to raise_exception do |exception|
-        expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader)
-        expect(exception.headers).to eql('X-Cascade' => 'pass')
-        expect(exception.status).to eql 406
-        expect(exception.message).to include('Accept header must not contain ranges ("*").')
-      end
-    end
-
     it 'succeeds if proper header is set' do
       expect(subject.call('HTTP_ACCEPT' => 'application/vnd.vendor-v1+json').first).to eq(200)
     end
@@ -223,30 +205,22 @@
       end
     end
 
-    it 'fails with 406 Not Acceptable if header is empty' do
-      expect { subject.call('HTTP_ACCEPT' => '').last }.to raise_exception do |exception|
-        expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader)
-        expect(exception.headers).to eql({})
-        expect(exception.status).to eql 406
-        expect(exception.message).to include('Accept header must be set.')
-      end
-    end
-
-    it 'fails with 406 Not Acceptable if type is a range' do
-      expect { subject.call('HTTP_ACCEPT' => '*/*').last }.to raise_exception do |exception|
+    it 'fails with 406 Not Acceptable if header is application/xml' do
+      expect { subject.call('HTTP_ACCEPT' => 'application/xml').last }
+        .to raise_exception do |exception|
         expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader)
         expect(exception.headers).to eql({})
         expect(exception.status).to eql 406
-        expect(exception.message).to include('Accept header must not contain ranges ("*").')
+        expect(exception.message).to include('API vendor or version not found.')
       end
     end
 
-    it 'fails with 406 Not Acceptable if subtype is a range' do
-      expect { subject.call('HTTP_ACCEPT' => 'application/*').last }.to raise_exception do |exception|
+    it 'fails with 406 Not Acceptable if header is empty' do
+      expect { subject.call('HTTP_ACCEPT' => '').last }.to raise_exception do |exception|
         expect(exception).to be_a(Grape::Exceptions::InvalidAcceptHeader)
         expect(exception.headers).to eql({})
         expect(exception.status).to eql 406
-        expect(exception.message).to include('Accept header must not contain ranges ("*").')
+        expect(exception.message).to include('Accept header must be set.')
       end
     end