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.

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

Conversation

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Jan 23, 2017

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.

Copy link

@MOZGIII MOZGIII left a comment

Do not drop body if Content-Type is blank. It's ok to just have it without Content-Type and let the client do the sniffing.

@@ -179,7 +175,7 @@ def drop_content_info?
end

def drop_body?
DROP_BODY_RESPONSES.include?(status.to_i)
!headers["Content-Type"] || DROP_BODY_RESPONSES.include?(status.to_i)

This comment has been minimized.

@MOZGIII

MOZGIII Jan 24, 2017

It's legit to have a body while having no content type.

RFC 7231

   A sender that generates a message containing a payload body SHOULD
   generate a Content-Type header field in that message unless the
   intended media type of the enclosed representation is unknown to the
   sender.  If a Content-Type header field is not present, the recipient
   MAY either assume a media type of "application/octet-stream"
   ([RFC2046], Section 4.5.1) or examine the data to determine its type.

This comment has been minimized.

@MOZGIII

MOZGIII Jan 24, 2017

It's not bad in general, but there are usecases for sinatra where it would really hurt. I suggest adding an option for this as well. I.e.:

settings.drop_body_for_empty_content_type? && !headers["Content-Type"]

This comment has been minimized.

@mwpastore

mwpastore Jan 24, 2017
Author Member

I'm trying to avoid a proliferation of settings here. I read the RFC you linked (thanks, learn something new every day!) and I think it's fine to send a body without a content-type. Rack::Protection::XSSHeader will set nosniff by default, so a UA can assume octet-stream; if Rack::Protection::XSSHeader is disabled, a UA can detect the content-type. I'll amend the commit.

lib/sinatra/base.rb Show resolved Hide resolved
lib/sinatra/base.rb Show resolved Hide resolved
end
end

return unless server_error?

This comment has been minimized.

@MOZGIII

MOZGIII Jan 24, 2017

Like this change, makes more sense now.

@mwpastore mwpastore force-pushed the mwpastore:default-content-type branch from 80f460f to 29e02db Jan 24, 2017
if boom.message && boom.message != boom.class.name
body boom.message
else
content_type 'text/html'

This comment has been minimized.

@zzak

zzak Jan 30, 2017
Member

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.

This comment has been minimized.

@mwpastore

mwpastore Jan 30, 2017
Author Member

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.

@zzak
Copy link
Member

@zzak zzak commented Jan 30, 2017

My feeling is we should put this off until after 2.0, sorry for the inconvenience.

@zzak zzak added this to the Beyond milestone Jan 30, 2017
@mwpastore
Copy link
Member Author

@mwpastore mwpastore commented Jan 30, 2017

No worries. Thank you for taking a look at it.

@MOZGIII
Copy link

@MOZGIII MOZGIII commented Jan 30, 2017

And 2.0 ETA is?

@jkowens jkowens modified the milestones: Beyond, v2.1.0 Mar 14, 2020
@jkowens
Copy link
Member

@jkowens jkowens commented Mar 16, 2020

@mwpastore I hate to bother you, but would you mind taking a look at the test failure? I was going to get this merged into master, but I'm not sure why the build is failing.

@jkowens jkowens force-pushed the mwpastore:default-content-type branch 4 times, most recently from c8f57d3 to 0e8acd0 Mar 19, 2020
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.
@jkowens jkowens force-pushed the mwpastore:default-content-type branch from 0e8acd0 to 996c341 Mar 19, 2020
@jkowens jkowens merged commit 2128dcf into sinatra:master Mar 19, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.