postrank-labs/goliath

JSONP middleware may invalidate Content-Length

ajvondrak opened this issue · 3 comments

Basic problem: when using the JSONP middleware and specifying a callback parameter, there are cases where the Content-Length will reflect the length of the body before applying the callback; thus, the returned value is actually truncated.

I'm not entirely sure who to open this issue for, because it came up for me when using Grape under Goliath, as demonstrated by @dblock here, and I cannot reproduce this without Grape.

I looked around, and I'm not the only one who has had this problem:

Minimal working example:

require 'goliath'
require 'grape'

class API < Grape::API
  format :json
  get do
    { content_length: 21 }
  end
end

class App < Goliath::API
  use Goliath::Rack::Params
  use Goliath::Rack::JSONP

  def response(env)
    API.call(env)
  end
end

Without the callback param, the content-length is exactly 21, and the Content-Length header is correct:

$ curl -i 'localhost:9000' && echo
HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 21
Server: Goliath
Date: Thu, 11 Dec 2014 23:38:27 GMT

{"content_length":21}

With the callback param, the content-length is actually 21 + 2 (for the parens) + the length of the callback argument. However, the Content-Length header is still set to 21:

$ curl -i 'localhost:9000?callback=callback' && echo
HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 21
CONTENT_TYPE: application/javascript
Server: Goliath
Date: Thu, 11 Dec 2014 23:38:32 GMT

callback({"content_le

Setting up breakpoints, I see that by the time the Goliath::Rack::JSONP middleware gets invoked, we have already set a Content-Length header. I'm having troubles tracing exactly why the header's present specifically when using Grape (and not with plain-old-Goliath itself), but whatever way it's happening, it's happening.

If we study Rack's JSONP middleware, we see that they adjust the Content-Length if mutated: https://github.com/rack/rack-contrib/blob/master/lib/rack/contrib/jsonp.rb#L51-L55 So, I think it stands to reason that Goliath should do something similar.

+1 If you modify the body you have to adjust the content-length.

dj2 commented

Goliath uses AsyncRack::ContentLength (which, under the hood) uses Rack::ContentLength to set the length header. Rack::ContentLength will only set the length if it hasn't been set. So, there is the assumption that if you're setting the length, you're setting it correctly, it doesn't get changed.

The ContentLength middleware is injected as the second one in the chain, which means it will get executed after the JSONP middleware and do the right thing, unless a content length has been set in the header.

So, probably the right solution is to do the same as JSON Rack in this case and force modify the header if it's been set, otherwise, leave it for the content length middleware to handle.

Fixed by #303.