Add Vary: Accept header when using Accept header for response

[st0012]

Problem description (quoted from @rafaelfranca's excellent explanation in rails/jquery-ujs#318 (comment)):

Let say that we requested /tasks/1 using Ajax, and the previous page has the same url. When we click the back button the browser tries to get the response from its cache and it gets the javascript response. With vary we "fix" this behavior because we are telling the browser that the url is the same but it is not from the same type what will skip the cache.

And there's a Rails issue discussing about this problem as well #25842

Also, according to RFC 7231 7.1.4

An origin server SHOULD send a Vary header field when its algorithm
for selecting a representation varies based on aspects of the request
message other than the method and request target

we should add Vary: Accept header when determining content based on the Accept header.

I think from the discussions in rails/jquery-ujs#318 and #25842, we can know that this problem is not well acknowledged by most users before they step on it. So it's not easy for users to foresee and prevent it. And adding Vary: Accept can prevent such issues from happen again.

I also know that adding such header by default could cause unnecessary cache invalidation. But this PR only adds the header if:

• The format param is not provided
• The request is a xhr request
• The request has accept headers and the headers are valid

So if the user

• sends request with explicit format, like /users/1.json
• or sends a normal request (non xhr)
• or doesn't specify accept headers

then the header won't be added.

[st0012]

@sikachu would you mind take a look at this?

[st0012]

@DanielHeath I've discussed this issue with some Rails maintainers few days ago and updated the description to explain the issue more clearly. But since I don't have this issue personally, can you explain how serious the problem is and how important the header would be?
Also, these issues are created years ago, do people still have same problem? And why can't they change their app's config to add this header theirselves if they have acknowledged the cause of the issue?

[DanielHeath]

So, the developer experience goes like this (can still reproduce it with latest rails, but it's been an issue since respond_to was added. I've had this issue, and helped multiple people debug it since.):

You create a new rails app, do some development, put it into production.

You add a blog at /articles/:id. Of course, the article pages have cache headers set to keep things fast.

You add a javascript handler to the 'next blog post' button. It updates the DOM to render the next post by fetching the article as JSON.

You feel very smart and cool, because get /articles/4 will return either the HTML or the JSON according to whether you fetched it by loading the page or by using jQuery.getJSON. You built it exactly the way the guide tells you to, and it certainly doesn't mention that you have to also set the Vary header.

At some point, you add a CDN (ex cloudflare/akamai) or a caching layer (eg varnish) to your production stack.

Some time later, you visit an article on your blog, but all you get is a page full of JSON. You are confused, and wonder how long this has been going on and how often it happens.

You try a bunch of different things, but can't reproduce it locally, or on any article other than that one. You eventually try requesting it directly from production and it works, so you flush your CDN cache and the issue goes away for a week. Then it returns; a different article this time.

Most of the time, you change your app so the urls all use .json, flush the cache again, and the issue is gone for good (until you accidentally forget to put the .json on the end of a URL and it starts happening again).

If you're really lucky and you keep at it, you eventually figure out that your app is using the Accept header to decide what to send, but is violating the HTTP spec by not setting Vary: Accept.

You think about updating the application code to always set the header, but that seems fragile; any route that forgets to do it will have the problem.

Instead, you hard-code your nginx config to always set the header, regardless of what the app does. Your CDN cache-hit-rate halves unnecessarily due to the header getting set on routes it doesn't need to, but at least your site works now.

You've had a buggy site for 6 months at this point and it's cost you >20 hours of debugging, but the issue is fixed for good.

You file a ticket upstream.

[fschwahn]

Just to stress that this is still relevant: I'm watching this PR for backporting, as we have the "Back-Button renders JS-code" problem described above. Our app runs on rails 5.2.3.

[st0012]

@DanielHeath @fschwahn can adding

config.action_dispatch.default_headers = { "Vary" => "Accept" }

solve ths issue?

[DanielHeath]

That's a very simple solution that will work.

It's a little heavy-handed (will drive slightly higher than required memory use at the CDN, leading to earlier cache eviction), but those who need the extra performance can easily override it themselves.

I think those who care that much about CDN performance are probably already doing so.

[st0012]

So I think the options are quite clear now:

1. Have this PR (or any similar solution) merged, which adds the header for users when necessary. By necessary I mean
   • The format param is not provided
   • And the response has accept headers and the they are valid
2. Mention this problem in a guide or something, with the most simple solution (default_headers config) and let users be aware of it and deal with it themselves

[fschwahn]

Here is a simple outline to reproduce this issue. I think this is a somewhat common pattern in Rails apps, so in my opinion it should be supported out of the box.

routes.rb

Rails.application.routes.draw do
  root to: "test#show"

  get "other_page" => "test#other_page"
end

app/controllers/test_controller.rb

class TestController < ApplicationController
  def show; end

  def other_page; end
end

app/views/test/show.html.erb

<h1 id="test-header">Test</h1>

<%= link_to "Refresh", root_path, remote: true %>
<%= link_to "Other page", other_page_path, "data-turbolinks" => "false" %>

app/views/test/show.js.erb

document.getElementById("test-header").innerHTML = "Replaced";

app/views/test/other_page.html.erb

<h1>Other page</h1>

And follow these steps.

• Visit root path
• Click Refresh (which triggers a JS response)
• Click Other page
• Click back button in browser
• You see the JS-code returned by clicking Refresh.

Important: While playing with this I noticed that this is only happening consistently in Chrome. I saw it happening in Firefox as well, but I can't come up with reliant reproduction steps. It's not reproducible at all in Safari.

[matthewd]

I think we want to solve it along these lines, but I think it's important that #36213 (comment) is covered too -- if there are multiple views and our choice between them is influenced by the accept header, then we need to report the vary. My reading of the PR code is that it doesn't currently do so.. is that right?

[st0012]

@fschwahn I can't reproduce that on my mac, what's your setup for producing it? Here's my env:

• Ruby 2.5.3
• Rails 5.2.3
• Chrome 75.0.3770.100

[fschwahn]

@st0012 I posted the app here: https://github.com/fschwahn/vary-header-issue

I also added a system test which reproduces the issue. I also added a commented out after_action, which makes the system test pass, see fschwahn/vary-header-issue@eae41b3

Note: I'm using rails 6.0.0.rc1, ruby 2.5.5, and Chrome 75.0.3770.142 (on macOS), but I think it shouldn't make a difference.

[st0012]

@fschwahn the test is awesome! I found that it didn't work because I had debugging console opened.

[st0012]

@matthewd The code now solves the problem described in #36213 (comment). I also moved the tests into integration_test.rb and I think this provides enough coverage for the problems. Let me know if you think adding system tests for this is necessary.

[sikachu]

The code looks good to me. Thank you for the patch.

However, would you mind adding a CHANGELOG entry explaining the change as well?

[st0012]

@sikachu updated, thanks!