-
Notifications
You must be signed in to change notification settings - Fork 18k
proposal: encoding/json: make encoders set Content-Type #41162
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
Comments
Could you expand a bit on this, for the sake of clarity?
I share this concern; teaching Also, have you thought about fixing this via static analysis? It would probably not meet the inclusion bar for vet, since I doubt this problem is common and important enough, but it could perhaps be added to staticcheck. With its fairly advanced code analysis, it should be possible to detect http handler funcs which write JSON but never set a content-type, even if there are func calls in between. CC @dominikh in case he's interested in new security checks. |
Related: #10630. |
Sniffing is cropped at 512 bytes for performance reasons, and you cannot know if a JSON is a JSON until you have tokenized the entire thing.
Yup, this is my biggest concern TBH.
It seems a mess to check if one of the handlers in a chain of handlers and decorators set the CT at one point. The whole decoration chain could be set up at runtime so I think it might even be impossible. |
Instead of using the Encoder we could have the if _, ok := w.(http.ResponseWriter); ok {
// Set something on the encoder so it knows what to do.
} This would avoid possibly the cons number 2. as it won't try to figure out at each
It would feel somewhat more natural to have a |
JSON comes from many places; tying this to the encoder seems like a mistake. Can you say more about the Pros? I am not too worried about "technically more correct". |
The more I think about this the more cons I see. The security feature I was referring to is Cross-Origin-Read-Blocking, but even if the specification doesn't say so, browsers still seem to protect text/plain if it sniffs as JSON (I had to do some experimentation). Since I am also not really interested in the correctness of the CT and the security benefit seems to be none, I am closing this one. Thanks everyone for the inputs. |
What we do
Currently encoding a JSON to a http.ResponseWriter sets a plaintext content-type:
playground
This happens on a response recorder the same way it happens on a standard http serving stack.
The issue is that when content-type is not explicitly set,
http
useshttp.DetectContentType
on the first slice passed toWrite
, which will sniff as plaintext since there is no good way to sniff JSON.What we could do
I propose to make
(*json.Encoder).Encode()
calls check if the passed argument has aHeader() http.Header
method, and if so set the appropriate content-type.Cons:
encoding/json
will depend onnet/http
, which seems less than desirableEncode
callsPros:
/cc @mvdan @rsc @dsnet @bradfitz as per owners.
The text was updated successfully, but these errors were encountered: