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

Response etags to always be weak: Prefixed 'W/' to value returned by Act... #17573

Merged
merged 1 commit into from Jan 20, 2016

Conversation

@zerothabhishek
Copy link
Contributor

zerothabhishek commented Nov 10, 2014

...ionDispatch::Http::Cache::Response#etag such that etags set in fresh_when and stale? are weak. Fixes #17556.

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 10, 2014

/cc @jeremy

@chancancode
Copy link
Member

chancancode commented Nov 10, 2014

Nice! If we make this more conservative, we can probably get away with sneaking this into 4.2:

  1. Add a new configuration option called generate_weak_etags or something that defaults to false (similar to this one)
  2. In #etag=, check the value of that option. If it's truthy, processed with generating a weak etag (similar to here)
  3. Otherwise, generate a strong etag like we used to, but print a deprecation warning message saying that the defualt will flip in Rails 5 (similar to this one)

This seems like an overall improvement to the situation and a good halfway point. Ideally though, we should also figure out the final API we would like to achieve with this (though I'm not sure if something like that is still appropriate for 4.2 at this time)...

  1. Weak etag is a good default for the 90% cases (it seems quite difficult, at least at the controller level, to determine if the response would meet the byte-level equivalence requirement... given all the things that goes on in a typical Rails stack)
  2. That said, there are probably cases that you would like to explicitly make the etag a strong one, such as the send_file use case @jeremy brought up.

With the current API, I'm not sure what's the best way to achieve point 2, and I'd love to hear ideas 😄

@egilburg
Copy link
Contributor

egilburg commented Nov 10, 2014

"Sorry, Diff contents are not available for this pull request."

First time I'm seeing this, why?

@rafaelfranca
Copy link
Member

rafaelfranca commented Nov 10, 2014

Good question. I'm seeing this too.

@chancancode
Copy link
Member

chancancode commented Nov 10, 2014

Yeah, I have no idea, I had to pull the branch locally to check the diff

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 10, 2014

Same here, I checked the changes in https://github.com/rails/rails/pull/17573.diff

@zerothabhishek zerothabhishek force-pushed the zerothabhishek:master branch to 3fbb75e Nov 11, 2014
@zerothabhishek
Copy link
Contributor Author

zerothabhishek commented Nov 11, 2014

[Sorry about the late response.]
There was an error in the earlier code, hence the new commit.


I'm a bit of a noob here, but from what I've seen in the code, our current behavior resembles weak etags more. Even though the etags emitted look strong. I could not find any code that actually creates an etag based off the entire response body. Here's an example to illustrate -

In the classic blog post application, we can write the PostsController#show as follows to take advantage of etags -

def show
  fresh_when(@post)
end

And then here's what we see from the browser -

Request Response Comment
GET /posts/1 200 first request, no cache
GET /posts/1 304 subsequent request, no change to @post, uses cache
Edit app/views/posts/show.html.erb
GET /posts/1 200 @post view changed, cache invalid, regenerate
GET /posts/1 304 cache as usual
Edit app/view/layouts/application.html.erb
GET /posts/1 304 view changed, still uses cache

The same happens when using the stale? method and the controller wide etag. So it looks like etag generation and verification does not take the full response body into account. And If I am correct so far, we will need to introduce support for strong etags.

I too feel that strong etags are important for cases like send_file. The send_file doesn't use any etags as of now, and I'm tempted to have them added, but am not sure if its a good idea.

@fabn
Copy link

fabn commented Aug 25, 2015

I'm having a lot of cache misses because of this behavior. I'm using nginx as reverse proxy and when a page is requested with Accept-Encoding: deflate, gzip (i.e. always) it automatically changes the generated strong etag by adding the prefix W/. Then following requests sent by user with the same browser won't match rails default etag causing 200 instead of 304 even if the response is the same.

Rack::ETag already do this and I think it's very important to have rails following this behavior for the reasons explained here

As a quick alternative the etag_matches? method can be changed to strip any W/ prefix before matching, but I don't know what side effects this can cause.

I'm going to try rails_weak_etags middleware to see if it solves my issues.

@felixbuenemann
Copy link
Contributor

felixbuenemann commented Sep 18, 2015

LGTM. What's missing to get this merged?

I don't think there needs to be a configuration options, because the way etags are calculated is already weak validation and Rack::ETag even weakens it's strong etags (a body digest is considered a strong validator based on RFC 7232 Section 2.1, while determining the etag from arbitrary data is considered a weak validator).

The current problem without this patch is that NGINX weakens strong ETags when it changes the content encoding (gzip). So right now fresh_when and stale? are useless on NGINX with gzip enabled.

Another approach would be to consider a weakened etag equivalent to the strong etag sent by action dispatch (but I think sending weak etags as proposed here is the correct approach):

module ActionDispatch
  module Http
    module Cache
      module Request

        NORMALIZE_ETAG = /^(?:W\/)?\"|\"$/.freeze

        def if_none_match_etags
          (if_none_match ? if_none_match.split(/\s*,\s*/) : []).collect do |etag|
            etag.gsub(NORMALIZE_ETAG, "")
          end
        end

        def etag_matches?(etag)
          if etag
            etag = etag.gsub(NORMALIZE_ETAG, "")
            if_none_match_etags.include?(etag)
          end
        end
      end
    end
  end
end

I'm currently using the above code as a monkey patch with rails 4.2.4 to fix If-None-Match on NGINX 1.8.

@fabn
Copy link

fabn commented Sep 19, 2015

I installed rails_weak_etags and everything work like a charm, no monkey patch needed, just a middleware.

In any case it would be very nice to have this merged (and backported to rails 4.x)

@bboe
Copy link
Contributor

bboe commented Nov 20, 2015

Is there any status update on this issue? The rails_weak_etags gem appears to solve the problem, but it seems like the current strong ETAG behavior is a bug in rails and should be fixed.

@arthurnn
Copy link
Member

arthurnn commented Dec 6, 2015

FWIW the last big rails projects I worked on, they all had a middleware to do something like this.

@maclover7
Copy link
Member

maclover7 commented Jan 10, 2016

Please rebase.

@zerothabhishek
Copy link
Contributor Author

zerothabhishek commented Jan 11, 2016

Sorry I was a little out of touch off this. Allow me a bit to rebase.

…tionDispatch::Http::Cache::Response#etag= such that etags set in fresh_when and stale? are weak. For #17556.
@zerothabhishek zerothabhishek force-pushed the zerothabhishek:master branch from 3fbb75e to e362c31 Jan 20, 2016
@zerothabhishek
Copy link
Contributor Author

zerothabhishek commented Jan 20, 2016

Rebased.

@jeremy
Copy link
Member

jeremy commented Jan 20, 2016

Looks good to merge @chancancode. We'll need a guide update and some mention of how to use strong ETags now (set the header manually, I presume).

chancancode added a commit that referenced this pull request Jan 20, 2016
Response etags to always be weak: Prefixed 'W/' to value returned by Act...
@chancancode chancancode merged commit 24e39ff into rails:master Jan 20, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chancancode chancancode mentioned this pull request Jan 20, 2016
4 of 4 tasks complete
@chancancode
Copy link
Member

chancancode commented Jan 20, 2016

Thanks everyone for working on this, sorry it took so long! I opened a new issue to track the remaining work: #23148

@zerothabhishek
Copy link
Contributor Author

zerothabhishek commented Jan 21, 2016

Thanks @chancancode and others. Should this be back-ported to 4.2 as well?

@chancancode
Copy link
Member

chancancode commented Jan 21, 2016

This is a new feature/breaking change, so we cannot. We only backport bug fixes.

maclover7 added a commit to maclover7/rails that referenced this pull request Feb 1, 2016
Fixes some parts of rails#23148.

[ci skip]
rafaelfranca added a commit that referenced this pull request Feb 1, 2016
Add documentation for #17573
@sfcgeorge
Copy link

sfcgeorge commented Apr 4, 2016

Thanks. Commenting for future Rails 4.2 users on CloudFlare because CF (silently) automatically convert strong ETags to weak ETags by prepending W/ in the header. Googling W/ is pretty ineffective and CloudFlare doesn't document this behaviour so hopefully this comment helps visibility. TLDR use Rails 5 or the rails_weak_etags gem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.