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

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)