Skip to content

Commit

Permalink
Add default_content_type setting
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mwpastore authored and jkowens committed Mar 19, 2020
1 parent 2850541 commit 0e8acd0
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 18 deletions.
9 changes: 9 additions & 0 deletions README.md
Expand Up @@ -2264,6 +2264,15 @@ set :protection, :session => true
used for built-in server.
</dd>

<dt>default_content_type</dt>
<dd>
Content-Type to assume if unknown (defaults to <tt>"text/html"</tt>). Set
to <tt>nil</tt> 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 <tt>nosniff</tt> is enabled
in Rack::Protection::XSSHeader, assume <tt>application/octet-stream</tt>).
</dd>

<dt>default_encoding</dt>
<dd>Encoding to assume if unknown (defaults to <tt>"utf-8"</tt>).</dd>

Expand Down
40 changes: 23 additions & 17 deletions lib/sinatra/base.rb
Expand Up @@ -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
Expand All @@ -176,6 +172,8 @@ def each
def finish
result = body

headers.delete "Content-Type" if headers["Content-Type"].nil?

if drop_content_info?
headers.delete "Content-Length"
headers.delete "Content-Type"
Expand All @@ -189,7 +187,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]
Expand Down Expand Up @@ -940,15 +938,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

Expand Down Expand Up @@ -1089,10 +1086,10 @@ def route_missing
# Attempt to serve static files from public directory. Throws :halt when
# a matching file is found, returns nil otherwise.
def static!(options = {})
return if (public_dir = settings.public_folder).nil?
return if (public_dir = settings.public_folder).nil?
path = "#{public_dir}#{URI_INSTANCE.unescape(request.path_info)}"
return unless valid_path?(path)

path = File.expand_path(path)
return unless File.file?(path)

Expand Down Expand Up @@ -1160,19 +1157,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 || '<h1>Not Found</h1>'
elsif bad_request?
body boom_message || '<h1>Bad Request</h1>'
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 '<h1>' + (not_found? ? 'Not Found' : 'Bad Request') + '</h1>'
end
end

return unless server_error?
raise boom if settings.raise_errors? or settings.show_exceptions?
error_block! Exception, boom
end
Expand Down Expand Up @@ -1813,6 +1818,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
Expand Down
2 changes: 1 addition & 1 deletion test/response_test.rb
Expand Up @@ -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
Expand Down
35 changes: 35 additions & 0 deletions test/settings_test.rb
Expand Up @@ -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?
Expand Down

0 comments on commit 0e8acd0

Please sign in to comment.