From 8b5af5806667e4cb3ca31af0a491e477d9c55225 Mon Sep 17 00:00:00 2001 From: elliotlarson Date: Tue, 11 Aug 2015 11:56:43 -0700 Subject: [PATCH] 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 | 1 + lib/grape/middleware/versioner/header.rb | 124 +++++++++++------- .../grape/middleware/versioner/header_spec.rb | 40 +----- 3 files changed, 88 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd1ed4cda5..e76cafac92 100644 --- a/CHANGELOG.md +++ b/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) ================== diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 1da5675ea9..fea3b2541d 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/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,10 +107,16 @@ 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 @@ -94,19 +124,25 @@ def 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 diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index c3a23c6f40..b356da1433 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/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