incorrect version accept header does not return a 406 response

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

@@ -24,6 +24,7 @@
 * [#1052](https://github.com/ruby-grape/grape/pull/1052): Reset `description[:params]` when resetting validations - [@marshall-lee](https://github.com/marshall-lee).
 * [#1088](https://github.com/ruby-grape/grape/pull/1088): Support ActiveSupport 3.x by explicitly requiring `Hash#except` - [@wagenet](https://github.com/wagenet).
 * [#1096](https://github.com/ruby-grape/grape/pull/1096): Fix coercion on booleans - [@towanda](https://github.com/towanda).
+* [#1101](https://github.com/intridea/grape/pull/1101): With strict version header, 406 response when using media type accept header - [@elliotlarson](https://github.com/elliotlarson).
 
 0.12.0 (6/18/2015)
 ==================

lib/grape/middleware/versioner/header.rb

@@ -22,54 +22,82 @@ module Versioner
       # X-Cascade header to alert Rack::Mount to attempt the next matched
       # route.
       class Header < Base
-        def before
-          header = rack_accept_header
+        VENDOR_VERSION_HEADER_REGEX =
+          /\Avnd\.([a-z0-9*.]+)(?:-([a-z0-9*\-.]+))?(?:\+([a-z0-9*\-.+]+))?\z/
 
-          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
-          end
-
-          media_type = header.best_of available_media_types
+        def before
+          strict_header_checks if strict?
 
           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
+            media_type_header_handler
           # 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)
+            fail_with_invalid_accept_header!('406 Not Acceptable')
+          elsif headers_contain_wrong_vendor_or_version?
+            fail_with_invalid_accept_header!('API vendor or version not found.')
           end
         end
 
         private
 
+        def strict_header_checks
+          # If no Accept header:
+          if header.qvalues.empty?
+            fail_with_invalid_accept_header!('Accept header must be set.')
+          end
+          # Remove any acceptable content types with ranges.
+          header.qvalues.reject! do |media_type, _|
+            Rack::Accept::Header.parse_media_type(media_type).find do |s|
+              s == '*'
+            end
+          end
+          # If all Accept headers included a range:
+          if header.qvalues.empty?
+            fail_with_invalid_accept_header!(
+              'Accept header must not contain ranges ("*").'
+            )
+          end
+        end
+
+        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]
+          elsif versions.size > 0
+            fail_with_invalid_accept_header!(
+              'API vendor or version not found.'
+            )
+          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 +111,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 +159,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

@@ -223,6 +223,15 @@
       end
     end
 
+    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('API vendor or version not found.')
+      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)