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
Comments
@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" 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 |
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:
\cc @bradfitz, thoughts? |
Looking at several hundred uses of Etag, more than half of them are wrong. |
CL https://golang.org/cl/33630 mentions this issue. |
@dsnet if that many are wrong...vet check? Seems good on correctness, maybe on frequency, and very good on precision. (/me ducks) |
@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. |
Right. Brain was not engaged. |
We'll go with the current logic of
I'm not sure a vet check works well for the reason @bcmills gives. Detecting bad |
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.
I believe this is a regression because the documentation for
ServeContent
explicitly says:\cc @bradfitz
The text was updated successfully, but these errors were encountered: