Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default_content_type setting. Fixes #1238 #1239

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Add default_content_type setting. Fixes #1238 #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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)
content_type body[0].content_type
jkowens marked this conversation as resolved.
Show resolved Hide resolved
else
content_type :html
elsif default = settings.default_content_type
jkowens marked this conversation as resolved.
Show resolved Hide resolved
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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If default content type is already html, why do we need to set it here?

This seems like a code smell to me that we're changing things we shouldn't,
and weary that subtle bugs will creep into the release if we start trying to do too much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default default content type is html, but the default content type may not be html. We're emitting html in this error message, so we force the content type to html here.

body '<h1>' + (not_found? ? 'Not Found' : 'Bad Request') + '</h1>'
end
end

return unless server_error?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this change, makes more sense now.

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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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