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
Closed

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

dsnet opened this issue Nov 26, 2016 · 8 comments

Comments

@dsnet
Copy link
Member

dsnet 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 The path to resolution is known, but the work has not been done. 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
Copy link
Member

@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
Copy link
Member Author

dsnet 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 Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Nov 26, 2016
@dsnet
Copy link
Member Author

dsnet 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
Copy link

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

@josharian
Copy link
Contributor

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

@bcmills
Copy link
Contributor

bcmills 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
Copy link
Contributor

Right. Brain was not engaged.

@dsnet
Copy link
Member Author

dsnet 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 Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 28, 2016
@golang golang locked and limited conversation to collaborators Nov 28, 2017
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants