diff --git a/CHANGELOG.md b/CHANGELOG.md index b87d4bfd7c..f007545789 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ * [#2382](https://github.com/ruby-grape/grape/pull/2382): Fix values validator for params wrapped in `with` block - [@numbata](https://github.com/numbata). * [#2387](https://github.com/ruby-grape/grape/pull/2387): Fix rubygems version within workflows - [@ericproulx](https://github.com/ericproulx). * [#2405](https://github.com/ruby-grape/grape/pull/2405): Fix edge workflow - [@ericproulx](https://github.com/ericproulx). +* [#2414](https://github.com/ruby-grape/grape/pull/2414): Fix Rack::Lint missing content-type - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 2.0.0 (2023/11/11) diff --git a/UPGRADING.md b/UPGRADING.md index f8cdb93b37..900d0a340a 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,12 @@ Upgrading Grape ### Upgrading to >= 2.1.0 +#### Changes in rescue_from + +The `rack_response` method has been deprecated and the `error_response` method has been removed. Use `error!` instead. + +See [#2414](https://github.com/ruby-grape/grape/pull/2414) for more information. + #### Grape::Router::Route.route_xxx methods have been removed - `route_method` is accessible through `request_method` diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 0286595b8c..a5ed227ef0 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -162,9 +162,9 @@ def configuration # @param status [Integer] the HTTP Status Code. Defaults to default_error_status, 500 if not set. # @param additional_headers [Hash] Addtional headers for the response. def error!(message, status = nil, additional_headers = nil) - self.status(status || namespace_inheritable(:default_error_status)) + status = self.status(status || namespace_inheritable(:default_error_status)) headers = additional_headers.present? ? header.merge(additional_headers) : header - throw :error, message: message, status: self.status, headers: headers + throw :error, message: message, status: status, headers: headers end # Creates a Rack response based on the provided message, status, and headers. @@ -180,8 +180,9 @@ def error!(message, status = nil, additional_headers = nil) # A Rack::Response object containing the specified message, status, and headers. # def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => content_type }) - message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == 'text/html' - Rack::Response.new([message], Rack::Utils.status_code(status), headers) + Grape.deprecator.warn('The rack_response method has been deprecated, use error! instead.') + message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == 'text/html' + Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), headers) end # Redirect to a new url. diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 02db9aae0d..545519a1a1 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require 'grape/middleware/base' -require 'active_support/core_ext/string/output_safety' module Grape module Middleware @@ -34,66 +33,59 @@ def initialize(app, *options) def call!(env) @env = env - begin - error_response(catch(:error) do - return @app.call(@env) - end) - rescue Exception => e # rubocop:disable Lint/RescueException - handler = - rescue_handler_for_base_only_class(e.class) || - rescue_handler_for_class_or_its_ancestor(e.class) || - rescue_handler_for_grape_exception(e.class) || - rescue_handler_for_any_class(e.class) || - raise - - run_rescue_handler(handler, e, @env[Grape::Env::API_ENDPOINT]) - end + error_response(catch(:error) { return @app.call(@env) }) + rescue Exception => e # rubocop:disable Lint/RescueException + run_rescue_handler(find_handler(e.class), e, @env[Grape::Env::API_ENDPOINT]) end - def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil) - headers = headers.reverse_merge(Rack::CONTENT_TYPE => content_type) - rack_response(format_message(message, backtrace, original_exception), status, headers) - end - - def default_rescue_handler(e) - error_response(message: e.message, backtrace: e.backtrace, original_exception: e) - end - - # TODO: This method is deprecated. Refactor out. - def error_response(error = {}) - status = error[:status] || options[:default_status] - message = error[:message] || options[:default_message] - headers = { Rack::CONTENT_TYPE => content_type } - headers.merge!(error[:headers]) if error[:headers].is_a?(Hash) - backtrace = error[:backtrace] || error[:original_exception]&.backtrace || [] - original_exception = error.is_a?(Exception) ? error : error[:original_exception] || nil - rack_response(format_message(message, backtrace, original_exception), status, headers) - end + private - def rack_response(message, status = options[:default_status], headers = { Rack::CONTENT_TYPE => content_type }) - message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML - Rack::Response.new([message], Rack::Utils.status_code(status), headers) + def rack_response(status, headers, message) + message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML + Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), headers) end def format_message(message, backtrace, original_exception = nil) format = env[Grape::Env::API_FORMAT] || options[:format] formatter = Grape::ErrorFormatter.formatter_for(format, **options) + return formatter.call(message, backtrace, options, env, original_exception) if formatter + throw :error, status: 406, message: "The requested format '#{format}' is not supported.", backtrace: backtrace, - original_exception: original_exception unless formatter - formatter.call(message, backtrace, options, env, original_exception) + original_exception: original_exception end - private + def find_handler(klass) + rescue_handler_for_base_only_class(klass) || + rescue_handler_for_class_or_its_ancestor(klass) || + rescue_handler_for_grape_exception(klass) || + rescue_handler_for_any_class(klass) || + raise + end + + def error_response(error = {}) + status = error[:status] || options[:default_status] + message = error[:message] || options[:default_message] + headers = { Rack::CONTENT_TYPE => content_type }.tap do |h| + h.merge!(error[:headers]) if error[:headers].is_a?(Hash) + end + backtrace = error[:backtrace] || error[:original_exception]&.backtrace || [] + original_exception = error.is_a?(Exception) ? error : error[:original_exception] || nil + rack_response(status, headers, format_message(message, backtrace, original_exception)) + end + + def default_rescue_handler(e) + error_response(message: e.message, backtrace: e.backtrace, original_exception: e) + end def rescue_handler_for_base_only_class(klass) error, handler = options[:base_only_rescue_handlers].find { |err, _handler| klass == err } return unless error - handler || :default_rescue_handler + handler || method(:default_rescue_handler) end def rescue_handler_for_class_or_its_ancestor(klass) @@ -101,22 +93,22 @@ def rescue_handler_for_class_or_its_ancestor(klass) return unless error - handler || :default_rescue_handler + handler || method(:default_rescue_handler) end def rescue_handler_for_grape_exception(klass) return unless klass <= Grape::Exceptions::Base - return :error_response if klass == Grape::Exceptions::InvalidVersionHeader + return method(:error_response) if klass == Grape::Exceptions::InvalidVersionHeader return unless options[:rescue_grape_exceptions] || !options[:rescue_all] - options[:grape_exceptions_rescue_handler] || :error_response + options[:grape_exceptions_rescue_handler] || method(:error_response) end def rescue_handler_for_any_class(klass) return unless klass <= StandardError return unless options[:rescue_all] || options[:rescue_grape_exceptions] - options[:all_rescue_handler] || :default_rescue_handler + options[:all_rescue_handler] || method(:default_rescue_handler) end def run_rescue_handler(handler, error, endpoint) @@ -126,9 +118,9 @@ def run_rescue_handler(handler, error, endpoint) handler = public_method(handler) end - response = (catch(:error) do + response = catch(:error) do handler.arity.zero? ? endpoint.instance_exec(&handler) : endpoint.instance_exec(error, &handler) - end) + end response = error!(response[:message], response[:status], response[:headers]) if error?(response) @@ -139,6 +131,13 @@ def run_rescue_handler(handler, error, endpoint) end end + def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil) + rack_response( + status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type), + format_message(message, backtrace, original_exception) + ) + end + def error?(response) response.is_a?(Hash) && response[:message] && response[:status] && response[:headers] end diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 97c2dbff07..95b46079c8 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -2102,7 +2102,7 @@ class CustomError < Grape::Exceptions::Base; end it 'rescues custom grape exceptions' do subject.rescue_from ApiSpec::CustomError do |e| - rack_response('New Error', e.status) + error!('New Error', e.status) end subject.get '/custom_error' do raise ApiSpec::CustomError.new(status: 400, message: 'Custom Error') @@ -2120,7 +2120,7 @@ class CustomError < Grape::Exceptions::Base; end allow(Grape::Formatter).to receive(:formatter_for) { formatter } subject.rescue_from :all do |_e| - rack_response('Formatter Error', 500) + error!('Formatter Error', 500) end subject.get('/formatter_exception') { 'Hello world' } @@ -2143,7 +2143,7 @@ class CustomError < Grape::Exceptions::Base; end describe '.rescue_from klass, block' do it 'rescues Exception' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.message}", 202) + error!("rescued from #{e.message}", 202) end subject.get '/exception' do raise 'rain!' @@ -2164,7 +2164,7 @@ class CommunicationError < StandardError; end it 'rescues an error via rescue_from :all' do subject.rescue_from :all do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2176,7 +2176,7 @@ class CommunicationError < StandardError; end it 'rescues a specific error' do subject.rescue_from ConnectionError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2188,7 +2188,7 @@ class CommunicationError < StandardError; end it 'rescues a subclass of an error by default' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/exception' do raise ConnectionError @@ -2200,10 +2200,10 @@ class CommunicationError < StandardError; end it 'rescues multiple specific errors' do subject.rescue_from ConnectionError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.rescue_from DatabaseError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/connection' do raise ConnectionError @@ -2221,7 +2221,7 @@ class CommunicationError < StandardError; end it 'does not rescue a different error' do subject.rescue_from RuntimeError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/uncaught' do raise CommunicationError @@ -2234,7 +2234,7 @@ class CommunicationError < StandardError; end describe '.rescue_from klass, lambda' do it 'rescues an error with the lambda' do subject.rescue_from ArgumentError, lambda { - rack_response('rescued with a lambda', 400) + error!('rescued with a lambda', 400) } subject.get('/rescue_lambda') { raise ArgumentError } @@ -2245,7 +2245,7 @@ class CommunicationError < StandardError; end it 'can execute the lambda with an argument' do subject.rescue_from ArgumentError, lambda { |e| - rack_response(e.message, 400) + error!(e.message, 400) } subject.get('/rescue_lambda') { raise ArgumentError, 'lambda takes an argument' } @@ -2326,7 +2326,7 @@ class ChildError < ParentError; end it 'rescues error as well as subclass errors with rescue_subclasses option set' do subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: true do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/caught_child' do raise ApiSpec::APIErrors::ChildError @@ -2347,7 +2347,7 @@ class ChildError < ParentError; end it 'sets rescue_subclasses to true by default' do subject.rescue_from ApiSpec::APIErrors::ParentError do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/caught_child' do raise ApiSpec::APIErrors::ChildError @@ -2359,7 +2359,7 @@ class ChildError < ParentError; end it 'does not rescue child errors if rescue_subclasses is false' do subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: false do |e| - rack_response("rescued from #{e.class.name}", 500) + error!("rescued from #{e.class.name}", 500) end subject.get '/uncaught' do raise ApiSpec::APIErrors::ChildError @@ -2389,7 +2389,7 @@ class ChildError < ParentError; end it 'rescues grape exceptions with a user-defined handler' do subject.rescue_from grape_exception.class do |_error| - rack_response('Redefined Error', 403) + error!('Redefined Error', 403) end exception = grape_exception @@ -2572,11 +2572,7 @@ def self.call(message, _backtrace, _option, _env, _original_exception) end get '/excel.json' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('The requested format 'txt' is not supported.') - else - expect(last_response.body).to eq('The requested format 'txt' is not supported.') - end + expect(last_response.body).to eq(Rack::Utils.escape_html("The requested format 'txt' is not supported.")) end end @@ -3375,7 +3371,7 @@ def static context 'when some rescues are defined by mounted' do it 'inherits parent rescues' do subject.rescue_from :all do |e| - rack_response("rescued from #{e.message}", 202) + error!("rescued from #{e.message}", 202) end app = Class.new(described_class) @@ -3393,14 +3389,14 @@ def static it 'prefers rescues defined by mounted if they rescue similar error class' do subject.rescue_from StandardError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from StandardError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise 'doh!' } mount app @@ -3412,14 +3408,14 @@ def static it 'prefers rescues defined by mounted even if outer is more specific' do subject.rescue_from ArgumentError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from StandardError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise ArgumentError.new } mount app @@ -3431,14 +3427,14 @@ def static it 'prefers more specific rescues defined by mounted' do subject.rescue_from StandardError do - rack_response('outer rescue') + error!('outer rescue') end app = Class.new(described_class) subject.namespace :mounted do rescue_from ArgumentError do - rack_response('inner rescue') + error!('inner rescue') end app.get('/fail') { raise ArgumentError.new } mount app @@ -4125,11 +4121,7 @@ def before end get '/something' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}') - else - expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}') - end + expect(last_response.body).to eq(Rack::Utils.escape_html({ error: "The requested format 'txt' is not supported." }.to_json)) end end @@ -4141,11 +4133,7 @@ def before end get '/something?format=' expect(last_response.status).to eq(406) - if ActiveSupport::VERSION::MAJOR == 3 - expect(last_response.body).to eq('The requested format '<script>blah</script>' is not supported.') - else - expect(last_response.body).to eq('The requested format '<script>blah</script>' is not supported.') - end + expect(last_response.body).to eq(Rack::Utils.escape_html("The requested format '' is not supported.")) end end @@ -4434,4 +4422,24 @@ def uniqe_id_route end end end + + context 'rack_response deprecated' do + let(:app) do + Class.new(described_class) do + rescue_from :all do + rack_response('deprecated', 500) + end + + get 'test' do + raise ArgumentError + end + end + end + + it 'raises a deprecation' do + expect(Grape.deprecator).to receive(:warn).with('The rack_response method has been deprecated, use error! instead.') + get 'test' + expect(last_response.body).to eq('deprecated') + end + end end diff --git a/spec/grape/exceptions/body_parse_errors_spec.rb b/spec/grape/exceptions/body_parse_errors_spec.rb index 7017400f6e..06866b7b32 100644 --- a/spec/grape/exceptions/body_parse_errors_spec.rb +++ b/spec/grape/exceptions/body_parse_errors_spec.rb @@ -6,7 +6,7 @@ before do subject.rescue_from :all do |_e| - rack_response 'message was processed', 400 + error! 'message was processed', 400 end subject.params do requires :beer @@ -58,7 +58,7 @@ def app before do subject.rescue_from :all do |_e| - rack_response 'message was processed', 400 + error! 'message was processed', 400 end subject.rescue_from :grape_exceptions @@ -96,7 +96,7 @@ def app before do subject.rescue_from :grape_exceptions do |e| - rack_response "Custom Error Contents, Original Message: #{e.message}", 400 + error! "Custom Error Contents, Original Message: #{e.message}", 400 end subject.params do diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index ca4aec5ec7..42edfb5261 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -44,7 +44,7 @@ before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.get '/beer' do 'beer received' @@ -114,7 +114,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], @@ -194,7 +194,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.get '/beer' do 'beer received' @@ -273,7 +273,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - rack_response 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e[:headers] end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb index 4780829a10..c66bf42e53 100644 --- a/spec/grape/validations/validators/values_spec.rb +++ b/spec/grape/validations/validators/values_spec.rb @@ -404,13 +404,13 @@ def even?(value) expect(last_response.body).to eq({ error: 'type does not have a valid value' }.to_json) end - it 'validates against values in an endless range', if: ActiveSupport::VERSION::MAJOR >= 6 do + it 'validates against values in an endless range' do get('/endless', type: 10) expect(last_response.status).to eq 200 expect(last_response.body).to eq({ type: 10 }.to_json) end - it 'does not allow an invalid value for a parameter using an endless range', if: ActiveSupport::VERSION::MAJOR >= 6 do + it 'does not allow an invalid value for a parameter using an endless range' do get('/endless', type: 0) expect(last_response.status).to eq 400 expect(last_response.body).to eq({ error: 'type does not have a valid value' }.to_json)