incorrect version accept header does not return a 406 response

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,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