From c8f57d3262a91ec568e1b366e56413f27e6eca28 Mon Sep 17 00:00:00 2001 From: Mike Pastore Date: Mon, 23 Jan 2017 16:36:00 -0600 Subject: [PATCH] Add default_content_type setting Historically, Sinatra::Response defaults to a text/html Content-Type. However, in practice, Sinatra immediately clears this attribute after instantiating a new Sinatra::Response object, so this default is some- what suspect. Let's break this behavior and have Sinatra::Response be Content-Type-less by default, and update the tests to reflect this. Next, let's introduce a new default_content_type setting that will be applied (instead of text/html) if the Content-Type is not set before the response is finalized. This will be great for e.g. JSON API developers. Let's also make it nil-able to indicate that a default Content-Type should never be applied. Wherever Sinatra is emitting HTML, e.g. in error pages, force the Content-Type to text/html. Finally, clean up the error-handling logic to behave as follows: * Set the X-Cascade header as early as possible. * If an error block matches and returns a value, stop processing and return that value. * If we have a not found or bad request error, inspect the exception (if any) for an error message and present it as the response body if it exists, or present a default error message. The remaining logic is the same otherwise. This should make error handlers simpler to write and behave more consistently by not polluting the body with a default message when none may be necessary. --- README.md | 9 +++++++++ lib/sinatra/base.rb | 34 +++++++++++++++++++--------------- test/response_test.rb | 2 +- test/settings_test.rb | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 1555fe4c66..25c05cf341 100644 --- a/README.md +++ b/README.md @@ -2264,6 +2264,15 @@ set :protection, :session => true used for built-in server. +
default_content_type
+
+ Content-Type to assume if unknown (defaults to "text/html"). Set + to nil to not set a default Content-Type on every response; when + configured so, you must set the Content-Type manually when emitting content + or the user-agent will have to sniff it (or, if nosniff is enabled + in Rack::Protection::XSSHeader, assume application/octet-stream). +
+
default_encoding
Encoding to assume if unknown (defaults to "utf-8").
diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb index b0a52b9072..4368abcb68 100644 --- a/lib/sinatra/base.rb +++ b/lib/sinatra/base.rb @@ -159,10 +159,6 @@ def matches_params?(params) # http://rubydoc.info/github/rack/rack/master/Rack/Response/Helpers class Response < Rack::Response DROP_BODY_RESPONSES = [204, 304] - def initialize(*) - super - headers['Content-Type'] ||= 'text/html' - end def body=(value) value = value.body while Rack::Response === value @@ -189,7 +185,7 @@ def finish if calculate_content_length? # if some other code has already set Content-Length, don't muck with it # currently, this would be the static file-handler - headers["Content-Length"] = body.inject(0) { |l, p| l + p.bytesize }.to_s + headers["Content-Length"] = body.map(&:bytesize).reduce(0, :+).to_s end [status.to_i, headers, result] @@ -940,15 +936,14 @@ def call!(env) # :nodoc: @response = Response.new template_cache.clear if settings.reload_templates - @response['Content-Type'] = nil invoke { dispatch! } invoke { error_block!(response.status) } unless @env['sinatra.error'] unless @response['Content-Type'] - if Array === body and body[0].respond_to? :content_type + if Array === body && body[0].respond_to?(:content_type) && body[0].content_type content_type body[0].content_type - else - content_type :html + elsif default = settings.default_content_type + content_type default end end @@ -1160,19 +1155,27 @@ def handle_exception!(boom) status(500) unless status.between? 400, 599 - boom_message = boom.message if boom.message && boom.message != boom.class.name if server_error? dump_errors! boom if settings.dump_errors? raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler elsif not_found? headers['X-Cascade'] = 'pass' if settings.x_cascade? - body boom_message || '

Not Found

' - elsif bad_request? - body boom_message || '

Bad Request

' end - res = error_block!(boom.class, boom) || error_block!(status, boom) - return res if res or not server_error? + if res = error_block!(boom.class, boom) || error_block!(status, boom) + return res + end + + if not_found? || bad_request? + if boom.message && boom.message != boom.class.name + body boom.message + else + content_type 'text/html' + body '

' + (not_found? ? 'Not Found' : 'Bad Request') + '

' + end + end + + return unless server_error? raise boom if settings.raise_errors? or settings.show_exceptions? error_block! Exception, boom end @@ -1813,6 +1816,7 @@ def force_encoding(*args) settings.force_encoding(*args) end set :add_charset, %w[javascript xml xhtml+xml].map { |t| "application/#{t}" } settings.add_charset << /^text\// set :mustermann_opts, {} + set :default_content_type, 'text/html' # explicitly generating a session secret eagerly to play nice with preforking begin diff --git a/test/response_test.rb b/test/response_test.rb index a5d360b435..c5bb32fa64 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -3,7 +3,7 @@ require File.expand_path('../helper', __FILE__) class ResponseTest < Minitest::Test - setup { @response = Sinatra::Response.new } + setup { @response = Sinatra::Response.new([], 200, { 'Content-Type' => 'text/html' }) } def assert_same_body(a, b) assert_equal a.to_enum(:each).to_a, b.to_enum(:each).to_a diff --git a/test/settings_test.rb b/test/settings_test.rb index dcf7b2aa0b..6ba8d2bfaa 100644 --- a/test/settings_test.rb +++ b/test/settings_test.rb @@ -163,6 +163,41 @@ def foo=(value) assert_equal :foo, @base.settings.environment end + describe 'default_content_type' do + it 'defaults to html' do + assert_equal 'text/html', @base.default_content_type + end + + it 'can be changed' do + @base.set :default_content_type, 'application/json' + @base.get('/') { '{"a":1}' } + @app = @base + get '/' + assert_equal 200, status + assert_equal 'application/json', response.content_type + end + + it 'can be disabled' do + @base.set :default_content_type, nil + @base.error(404) { "" } + @app = @base + get '/' + assert_equal 404, status + assert_nil response.content_type + assert_empty body + end + + it 'may emit content without a content-type (to be sniffed)' do + @base.set :default_content_type, nil + @base.get('/') { raise Sinatra::BadRequest, "This is a drill" } + @app = @base + get '/' + assert_equal 400, status + assert_nil response.content_type + assert_body "This is a drill" + end + end + describe 'methodoverride' do it 'is disabled on Base' do assert ! @base.method_override?