Implement 'no_store' HTTP cache directive

[tadas-s]

I tried starting conversation in rubyonrails-core but haven't really got much feedback from there.

Summary

Currently there is no way to set Cache-Control: no-store header using
built-in cache control methods (expires_now/expires_in/etc..). One of
the top StackOverflow answers currently suggests putting it directly
into header set.

Unfortunately, it cannot later be overridden in specific/individual actions by
calling say expires_in 5.minutes. Resulting header in that case is
stays the same, i.e. Cache-Control: no-store.

This:

1. Adds the no_store method to set Cache-Control: no-store header.
2. Changes cache control "merge and normalize" code so default no-store
   directive can be overridden using built in cache control methods mentioned
   above.

What's the use of it

Couple examples:

• To prevent rendering stale content if browser return button is used
  (expires_now does not help).
• To prevent browser disk cache being used. In some situations it's considered
  a privacy/security risk.

Naming it

I'm open for ideas. Couple random suggestions:

• no_store
• http_cache_no_store

Other Information

• Mozilla developer docs for Cache-Control header.
• A related open merged PR allow for only no-store in cache-control header #39461

[rafaelfranca]

Can you add a CHANGELOG entry for the new method added? Also please squash all commits in one.

[tadas-s]

@rafaelfranca done.

(ps: if anyone's wondering why @rails-bot added "activesupport" label - I updated wrong changelog on previous commit)

[tadas-s]

Any chance this could be reviewed before next minor release cycle? 🙂

[zzak]

@tadas-s Would you mind rebasing this? 🙇

[tadas-s]

@zzak rebased.

[Tonkpils]

@tadas-s I'm looking into a failing tests on the GitHub test suite. It seems this change might have introduced a change which causes the Cache-Control headers to be replaced when the controller's action uses send_data

send_data sets a public cache_control https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/data_streaming.rb#L148 and along with this patch any Cache-Control headers set on the action are cleared.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "byebug"
  gem "rails", github: "rails/rails", branch: "main"
end

require "byebug"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    headers["Cache-Control"] = "no-cache, no-store"
    send_data("foo\r\n", filename: "foobar.txt")
  end
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    get "/"
    assert last_response.ok?
    assert_equal "no-store", last_response.headers["Cache-Control"]
  end

  private
    def app
      Rails.application
    end
end

Failure:
BugTest#test_returns_success [duplication-bug.rb:49]:
Expected: "no-store"
  Actual: "private"

I'm happy to create a new issue for this if you think this is unintended behavior. A workaround here could be to explicitly set the headers with this new no_store method but I'm curious what your thoughts are here.

[tadas-s]

@Tonkpils

Oh this is a fun one.

So yes, no_store instead of headers["Cache-Control"] = "no-cache, no-store" should work. And this is why I created this PR. Although, if we're being nitpicky - according to Mozilla docs Cache-Control: no-cache, no-store is does not make sense and it should simply be Cache-Control: no-store

However.. I assumed that response.cache_control will only be manipulated via fresh_when / expires_in / expires_now .. etc. But your example shows that there's at least one piece of code which writes directly to response.cache_control, that send_data method.

Anyway, I'm still looking at this.

[tadas-s]

I'm also wondering if that IE 6.0 hack is still relevant at all 😅

https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/data_streaming.rb#L142-L148

[tadas-s]

Using github search I even found code like this: https://github.com/gitlabhq/gitlabhq/blob/master/app/controllers/concerns/sends_blob.rb#L32-L45 .. Do not think this is a documented way to handle response caching headers.

[zzak]

I'm also wondering if that IE 6.0 hack is still relevant at all 😅
https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/data_streaming.rb#L142-L148

@tadas-s Do you want to send a PR? 🙇

[tadas-s]

@zzak will do. I'm still playing around this + want to test with IE8 (lowest currently available IE vm I can get my hands on..). Just in case..

[Tonkpils]

Although, if we're being nitpicky - according to Mozilla docs Cache-Control: no-cache, no-store is does not make sense and it should simply be Cache-Control: no-store

I'm curious about this and I'm honestly not 100% sure what the right approach is here. While the Mozilla docs specify no-store should suffice, there's other places that also maintain that no-cache is necessary to prevent the browser or proxy from using cached data without revalidating it.

As a similar example https://github.com/gitlabhq/gitlabhq/blob/21e08b6197f192c983f8527f4bba1f2aaec8abf2/lib/gitlab/no_cache_headers.rb sets no-store, no-cache while also setting

ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL
=> "max-age=0, private, must-revalidate"

On another note, I also want to confirm that setting the Cache-Control header is not going to be the proper approach to controlling the values in the header? It almost seems to me like we want a more flexible method for setting these values if we're going to have specific methods like no_store

[tadas-s]

On another note, I also want to confirm that setting the Cache-Control header is not going to be the proper approach to controlling the values in the header? It almost seems to me like we want a more flexible method for setting these values if we're going to have specific methods like no_store

This is only my opinion, I'm not a Rails maintainer or anything: setting caching headers via Rails provided APIs like fresh_when / expires_in / expires_now / .. is great. If flexibility of that is not enough - setting response.headers["Cache-Control"] = ... is good as well given Rails APIs do not provide enough flexibility. Both are documented facilities (here and here) provided by Rails. However.. Directly manipulating response.cache_control from the application code (like gitlab does) should be avoided. It's not documented and it touches internal Rails structures.

[casperisfine]

Hum, this broke our test suite as well. In a controller we have a before_filter with:

      def configure_cache_control
        response.cache_control = 'no-store'
      end

And this change clears it and replace it with no-cache, which isn't what we want.

[tadas-s]

@casperisfine any chance you could try re-writing this bit of code into:

      def configure_cache_control
        no_store
      end

That should do the trick.

[casperisfine]

Yeah expires_now did the trick. I just wanted to point out how frequent this usage was.

[645383]

Is there a way to set Cache-Control: no-store, max-age=0 (example: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#preventing_caching)? Adding no-store to headers replaces other directives.

[tadas-s]

@645383

See this section: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#directives

And specifically:

no-store
The response may not be stored in any cache. Note that this will not prevent a valid pre-existing cached response being returned. Clients can set max-age=0 to also clear existing cache responses, as this forces the cache to revalidate with the server (no other directives have an effect when used with no-store).

(emphasis mine)

I understand it's client (i.e. browser side thing) and hence not applicable for Rails/server side.

[tadas-s]

@645383 if you could describe what you're trying to solve in bit more detail, myself or someone else reading this may be able to help.

[645383]

Sorry, the question is not related to this PR. There are another PRs allow for only no-store in cache-control header and Allow 'private, no-store' Cache-Control header.
I wonder what is the correct way to set Cache-Control: no-store, max-age=0? If no-store directive exists, rails does not allow max-age directive.

[645383]

Missed this part Clients can set, thank you.