Cache-Control header behavior has changed in 1.4.x

[bobbytables]

Hey there, we've recently upgraded our version of Grape from 1.2.2 -> 1.4.0. We saw a sudden spike in cached requests in our CDN (Fastly) for our API. Fastly relies on the Cache-Control header to determine if it should or should not cache the response.

We also use the Rack::ETag middleware (default in Rails afaik), which by default sets the Cache-Control header, see here: https://github.com/rack/rack/blob/master/lib/rack/etag.rb#L18

However, this middleware will not set the cache control header if one is already present. See: https://github.com/rack/rack/blob/master/lib/rack/etag.rb#L39

This is where I believe Grape introduced a difference, I have a request spec that added an assertion to verify these findings:

expect(response.headers['Cache-Control']).to include('private') # tells fastly to skip caching, set by Rack::Etag

In Grape 1.2.5, this passes, in Grape 1.4.0 however...

  1) PublicAPI::V1::Ping /v1/ping Header Authentication for bots returns a successful response with a bot token provided in the header
     Failure/Error: expect(response.headers['Cache-Control']).to include('private')
       expected "no-cache" to include "private"
     # ./spec/requests/public_api/ping_spec.rb:36:in `block (5 levels) in <top (required)>'
     # ./spec/rails_helper.rb:29:in `with_flag'
     # ./spec/rails_helper.rb:108:in `block (2 levels) in <top (required)>'

I believe something has changed in the way that Grape assigns response headers, including the Cache-Control header. And my gut is that is has something to do with this change in the compare I looked through:

https://github.com/ruby-grape/grape/compare/v1.2.2..v1.4.0#diff-417f63bb9cd022ea7ea57549e03fcb5bR33-R54

This is a potential security risk for people

This change can cause API responses to be cached in CDNs unbeknownst to people upgrading Grape and served to other users for an API endpoint they do not have access to simply because they hit some endpoint with a unique identifier (/api/tasks for example)

[bobbytables]

I believe I see why:

https://github.com/ruby-grape/grape/compare/v1.2.2..v1.4.0#diff-8e15cddb80f42ca7d43872846d80ecbeR277

response_object = stream || [body]

This is calling the #stream method, which explicitly assigns the Cache-Control header here: https://github.com/ruby-grape/grape/compare/v1.2.2..v1.4.0#diff-e9e8424a5238d48301e313d8fe285697R333

[bobbytables]

This looks like it's fixed in master, but I really recommend pushing a patch version for this at 1.4.1, this can really be bad for folks behind a CDN.

[bobbytables]

Closing this since I see there was a discussion, apologies. I definitely think this needs a patch level push.

Ref:
#2088
#2087

[dblock]

If you really want to do a 1.4.1 patch go for it @bobbytables, otherwise i'll release 1.5.0 soon (unless someone beats me to it).