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

Content-Type forced for all responses #1238

Closed
MOZGIII opened this issue Jan 23, 2017 · 9 comments
Closed

Content-Type forced for all responses #1238

MOZGIII opened this issue Jan 23, 2017 · 9 comments

Comments

@MOZGIII
Copy link

@MOZGIII MOZGIII commented Jan 23, 2017

From Sinatra::Base @ https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L915:

    def call!(env) # :nodoc:
      @env      = env
      @request  = Request.new(env)
      @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
          content_type body[0].content_type
        else
          content_type :html
        end
      end

      @response.finish
    end

Code

      unless @response['Content-Type']
        if Array === body and body[0].respond_to? :content_type
          content_type body[0].content_type
        else
          content_type :html
        end
      end

is problematic, because it forces response to have a Content-Type header. In some cases we don't want that. Can we make a setting to allow disabling this behavior?

My usecase is not limited to 204 response. I have some error codes that should have no content as well.

@MOZGIII MOZGIII changed the title Content-Type for 204 response Content-Type forced for all responses Jan 23, 2017
@MOZGIII
Copy link
Author

@MOZGIII MOZGIII commented Jan 23, 2017

Simply extracting this "default" Content-Type handler into a separate method would also do the trick for me. I'd just redefine that method to do nothing.

Currently I ended up reimplementing the whole call! in my app.

@kgrz
Copy link
Member

@kgrz kgrz commented Jan 23, 2017

Can you instead use a middleware which removes the content type or any other information that needs to be removed? For instance:

class MyMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, response = @app.call(env)
    headers.delete('Content-Type')
    [status, headers, response]
  end
end

And in config.ru:

require './middleware.rb'

use MyMiddleware
run MyApp

This way you can customise all the routes according to your use case, and avoid overriding any internal method of the library itself. This will save you from unexpected issues if and when this def call! method changes in future versions.

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Jan 23, 2017

If you halt 204 it will not set a Content-Type. Can you share some examples with us that demonstrate the behavior you're trying to work around?

@MOZGIII
Copy link
Author

@MOZGIII MOZGIII commented Jan 23, 2017

For example error 404. I expect Content-Type to be nil. But it's forced to be text/html.

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Jan 23, 2017

Unfortunately, it looks like Rack::Protection requires that a Content-Type be set most of the time, so it fails with weird errors if we try to work around Sinatra by setting it to nil or anything like that.

Quite frankly, I don't understand why it's so important to you to remove the Content-Type header in these cases. My understanding is that browsers and other user-agents expect this header to be present on most responses, even on 404. The Content-Length and Transfer-Encoding headers are intended to be used to determine whether or not there is content. Just set it to some appropriate default for your intended audience(s) and forget about it.

If you're still determined to do this, I think @kgrz's idea is best.

@MOZGIII
Copy link
Author

@MOZGIII MOZGIII commented Jan 23, 2017

Best is to reimplement call!, which I already did.
There is no reason why the Content-Type header is needed to be forced for all users. A middleware like @kgrz would require a non-trivial logic to be added to remove a header since I don't need it everywhere but only for a certain codepaths. To add some metadata to env just to remove headers later - it just seems to like a nonsense.
Furthermore, I don't use Rack::Protection at all. Since it's on by default, to maintain backward-compatibility this should be made an option with enabled state by default (force_default_content_type).

The current implementation is based on an assumption that Content-Type is a header that only 204 requests must not have, however it's now always the case (does not work for everyone).

My actual goal is to implement the API spec that I have as strict as possible.

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Jan 23, 2017

I opened #1239 to address this. We'll see what the higher-ups have to say!

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

@MOZGIII MOZGIII commented May 26, 2019

It's been a long time now... I wonder if it's still relevant. If the issue is still there - the PR solves it, so why don't we merge it?

@jkowens
Copy link
Member

@jkowens jkowens commented Mar 19, 2020

Fixed by #1239.

@jkowens jkowens closed this Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.