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

net/http: document that Etags in ServeContent must be RFC 7232 compliant #18054

Closed
dsnet opened this issue Nov 26, 2016 · 8 comments

Comments

Projects
None yet
5 participants
@dsnet
Copy link
Member

commented Nov 26, 2016

Change f386274 (CL/32014) caused a regression in the behavior of http.ServeContent such that explicitly setting the Etag no longer caused the server to return status 304 when the Etags match.

func Test(t *testing.T) {
	fs := http.FileServer(http.Dir("/tmp"))
	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		// Delete if-modified-since header so that ETags can be used instead of the standard cache policy.
		r.Header.Del("If-Modified-Since")

		// Set ETag to to uniquely identify the unchanged static asset.
		w.Header().Set("Etag", "6")

		fs.ServeHTTP(w, r)
	})
	ts := httptest.NewServer(h)

	// Ensure /tmp/test.txt exists.
	if err := ioutil.WriteFile("/tmp/test.txt", nil, 0664); err != nil {
		t.Fatal(err)
	}

	var c http.Client
	req, err := http.NewRequest("GET", ts.URL+"/test.txt", nil)
	if err != nil {
		t.Fatal(err)
	}
	req.Header.Add("If-None-Match", "6") // Same Etag as above
	resp, err := c.Do(req)
	if err != nil {
		t.Fatal(err)
	}
	if resp.StatusCode != 304 {
		t.Error()
	}
}

I believe this is a regression because the documentation for ServeContent explicitly says:

If the caller has set w's ETag header, ServeContent uses it to handle requests using If-Range and If-None-Match.

\cc @bradfitz

@dsnet dsnet added the NeedsFix label Nov 26, 2016

@dsnet dsnet added this to the Go1.8 milestone Nov 26, 2016

@dsnet dsnet self-assigned this Nov 26, 2016

@odeke-em

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

@dsnet that CL added a helper function scanETag which rejects ETags such as 6 because they are unquoted f386274#diff-f7034090d2073d288d00540ff2b1ff73R281

Also I took a quick look at https://www.rfc-editor.org/rfc/rfc7232.txt and it says that a valid ETag is quoted so by extension "has to be quoted"
screen shot 2016-11-26 at 3 52 31 am

If we update your test to properly use a quoted ETag, it passes

--- 18054_test.go	2016-11-26 03:55:16.000000000 -0800
+++ fixed.go	2016-11-26 03:55:01.000000000 -0800
@@ -14,7 +14,7 @@
 		r.Header.Del("If-Modified-Since")
 
 		// Set ETag to to uniquely identify the unchanged static asset.
-		w.Header().Set("Etag", "6")
+		w.Header().Set("Etag", "\"6\"")
 
 		fs.ServeHTTP(w, r)
 	})
@@ -30,7 +30,7 @@
 	if err != nil {
 		t.Fatal(err)
 	}
-	req.Header.Add("If-None-Match", "6") // Same Etag as above
+	req.Header.Add("If-None-Match", "\"6\"") // Same Etag as above
 	resp, err := c.Do(req)
 	if err != nil {
 		t.Fatal(err)

Perhaps it is that, the new code does the right thing and rejects invalid ETags
that were previously accepted.

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2016

Thanks @odeke-em for looking at this!

I'm looking at a number of use cases of Etag and more than half of them are wrong. They don't look terribly hard to fix either. I think we should either:

  • be loose in our behavior and accept bad Etags OR
  • go with the new behavior, but document it better

\cc @bradfitz, thoughts?

@dsnet dsnet added NeedsDecision and removed NeedsFix labels Nov 26, 2016

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2016

Looking at several hundred uses of Etag, more than half of them are wrong.
(But most of them don't touch http.ServeContent and implement their own server-side caching logic).

@gopherbot

This comment has been minimized.

Copy link

commented Nov 26, 2016

CL https://golang.org/cl/33630 mentions this issue.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2016

@dsnet if that many are wrong...vet check? Seems good on correctness, maybe on frequency, and very good on precision. (/me ducks)

@bcmills

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

@josharian How many of the bad etags are passed as literals in Go clients (vs. sent from non-Go clients or constructed programmatically)?

Not that I object to a vet check... it just seems like it only addresses a small part of the problem.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2016

Right. Brain was not engaged.

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2016

We'll go with the current logic of http.ServeContent as it currently is for the following reasons:

  • Most improper uses of Etag did not even involve http.ServeContent, so "fixing" the regression would not help those cases anyways.
  • For the few cases that did use ServeContent with incorrect Etags, it was a trivial (<10 line) change to fix them.
  • It's pretty gross "fixing" the regression by having it intentionally violate the RFC.

I'm not sure a vet check works well for the reason @bcmills gives. Detecting bad Etag would probably require a more comprehensive HTTP compliance test, which is beyond the scope of this bug.

@dsnet dsnet changed the title net/http: regression in http.ServeContent net/http: document that Etags in ServeContent must be RFC 7232 compliant Nov 28, 2016

@dsnet dsnet added Documentation and removed NeedsDecision labels Nov 28, 2016

@gopherbot gopherbot closed this in 993214a Nov 28, 2016

@golang golang locked and limited conversation to collaborators Nov 28, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.