proposal: net/http: delete inappropriate headers in func Error about go HOT 15 OPEN

ETag and Last-Modified are Validator Header Fields, for which there is some specific language in the RFC relating to this question:

Note that, depending on the status code semantics, the selected representation for a given response is not necessarily the same as the representation enclosed as response payload.

The part "depending on the status code semantics" suggests that whether it is correct to delete these depends on which status code is being returned.

However, I wasn't able to determine which part of this spec (or any other spec) explicitly describes "the status code semantics" for the purpose of this rule. Some of the status codes are described as "cacheable", which seems at least plausibly related to whether the response payload is "the selected representation", because otherwise it would not make sense to use If-None-Match/If-Modified-Since/etc to check if that cached representation is still current, but I can't find an explicit connection between those two ideas in the specification text.

With all of that said, it does seem like unsetting them is a good conservative default. It seems to me that doing anything "better" than that would require figuring out which status codes the above rule applies to and making Error behave differently depending on the status code, which seems unnecessarily complicated and perhaps a little too clever/surprising.

from go.

CC @neild @bradfitz

from go.

I am not convinced that deleting Etag and Last-Modified is correct. Those describe the resource at the URL, and it seems sensible to me to say that a response can be an error that says “something about your request was invalid” but still also send back information about the underlying resource. It's not obviously wrong in the way that using the underlying resource's Content-Encoding or Content-Length is obviously wrong.

It is not always wrong, but it can be wrong. In my own use case, I set immutable caching header with etag for the contents I am planing to serve (which I have in metadata in db) and then I go to serve the file. If that file serving fails for some reason (Read returns an error, or if range request is invalid, etc.) an error is returned. That error might be permanent or transient. For transient errors those caching headers and etag and last-modified should be removed. But it is hard to know if it is transient, so removing those headers is reasonable in my view.

Another perspective on this is that etag and last-modified is about content to be served or returned, but Error serves some completely different content. So they are probably misleading.

I propose we do (4), because:

I am OK with that.

GODEBUG=httpcleanerrorheaders=0 disables the Etag, Last-Modified, and Cache-Control deletions

One approach done in one iteration of past CLs was to extend Error with varargs list of headers to keep. So by default a set of headers is deleted, but one can pass Etag string as the varag argument to make sure it is kept. httpcleanerrorheaders could still control the default list of headers to keep.

from go.

Perhaps we should have a GODEBUG that controls the last three deletions?
Let's say httpcleanerrorheaders=0 preserves Etag, Last-Modified, and Cache-Control.

That would mean that if, say, a library wants to call http.Error with Cache-Control showing up in the response, they now have to require all consumers to set httpcleanerrorheaders=0 instead of it "just working".

One approach done in one iteration of past CLs was to extend Error with varargs list of headers to keep.

This is a breaking change, since today I can do:

var f func(http.ResponseWriter, string, int) = http.Error

from go.

This is a breaking change, since today I can do:

True. That was an internal error function used just by ServeContent.

Maybe one solution could be that http.Error remove all headers listed above except for Cache-Control and that ServeContent additionally removes Cache-Control on errors (by using a small internal wrapper around http.Error).

External callers who want to remove Cache-Control as well on errors, could use a similar wrapper they make, or we could make some additional http.ErrorBis or something which does that as well and users can slowly migrate to it if they want. But the issue is that this should be then also called from most other places in stdlib and elsewhere.

My personal take is that I like current proposal that http.Error does remove all those headers because in almost all cases this is what improves the response and httpcleanerrorheaders seems like a good alternative to switch this off.

from go.

(Apologies for not being around to deal with rollbacks; four years of dodging COVID-19 have come to an end, and I was out sick last week.)

Error can't be all things to all users: It's reasonable to send an error with a Cache-Control header. It's also reasonable to want to send an error with no Cache-Control. We can satisfy some users by clearing Cache-Control and others by not, but either way someone is going to get behavior they don't want.

Error is also a very simple function; it isn't difficult for users to write their own version that does exactly what they want.

I think, therefore, that we should err on the side of preserving the current behavior, except in cases where the current behavior is objectively wrong in all cases. I'd really like to avoid introducing a GODEBUG and the concomitant cognitive complexity. "Error does this simple thing, which might not be perfect in all circumstances" is better than "Error does this or that depending on a global variable".

• Content-Type: Since its introduction, Error has always set Content-Type: text/plain; charset=utf-8.
• Content-Length: This obviously shouldn't be set, and so Error should delete it. (As it does now.)
• Content-Encoding: I can't come up with a scenario where this can reasonably have a value. You theoretically could provide gzipped content to Error and set Content-Type: gzip, but that seems unlikely in practice. Error could probably delete it.
• Etag, Last-Modified, Cache-Control, X-Content-Type-Options: There are scenarios where a user might want to delete these. There are also scenarios where a user might be setting one of them deliberately. Error should leave them alone.

from go.

(Apologies for not being around to deal with rollbacks; four years of dodging COVID-19 have come to an end, and I was out sick last week.)

I hope it was not a rough case.

Error is also a very simple function; it isn't difficult for users to write their own version that does exactly what they want.

Yes, unless Error is called from inside the standard library directly. In that case it should do the right thing and the result should be correct. The motivation behind all this was that http.ServeContent was returning invalid headers and that it has a contract that you have to set some headers (like Etag) before calling it, but it was not clearing them.

My first CL to fix http.ServeContent had internal "error" function doing the correct thing (wrapping http.Error). Then we moved its logic to Error and now there is push back on that.

Maybe we should reintroduce this new "error" function which does more header cleanup and call it from standard library (at least from http.ServeContent) while keeping changes to http.Error limited to only those which work in all (reasonable) cases.

The question might then be: should this new "error" function be made public or should it be just internal? In a way, it is easy also for others to wrap http.Error in a similar way? But maybe it is useful to make it public?

from go.

I agree that we should add an internal error function for ServeContent. I think it's fine for it to stay internal; it would be easy for us to provide something that is useful for some users some of the time, but the bar for adding a new API is higher than that. (If we can do better than Error for most users most of the time, that'd probably meet the bar, but I'm not sure that any of the suggested options would satisfy that.)

from go.

I also noticed in our own code, that we do set Content-Length before calling http.Error. We in fact have such a wrapper:

func Error(w http.ResponseWriter, _ *http.Request, code int) {
	body := http.StatusText(code)
	w.Header().Set("Cache-Control", "no-cache")
	// http.Error appends a newline so we have to add 1.
	w.Header().Set("Content-Length", strconv.Itoa(len(body)+1))
	http.Error(w, body, code)
}

So not sure even about removing Content-Length. Maybe Content-Length should be removed if the value does not match the length of the content to be written (+1)?

from go.

Removing Content-Length is at best important and at worst harmless, since the protocol will figure out the content length either way.

The GODEBUG was only meant as a transitional mechanism so that people control when the change happens, not a long-term knob. If we did the extra deletions, code that wanted to send those headers would inline the pieces they wanted of http.Error, not set the GODEBUG.

That said I am also happy to keep deleting Content-Length, delete Content-Encoding, and stop there.

from go.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

from go.

Following @neild's suggestion, I revise this proposal to:

• Delete Content-Length (already done, obviously correct)
• Delete Content-Encoding (obviously correct)

and nothing else.

from go.

But we can still delete the other headers in the response from ServeContent, yes? So the proposal is just to not delete them in general from Error?

from go.

@mitar what specific headers would ServeContent delete before calling Error? From https://pkg.go.dev/net/http#ServeContent it looks like just Etag?

from go.

@rsc: Etag, Last-Modified, Cache-Control (or override it to no-cache).

from go.