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.

Sign up for GitHub

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

Jump to bottom

proposal: encoding/json: make encoders set Content-Type #41162

Closed
empijei opened this issue Sep 1, 2020 · 6 comments
Closed

proposal: encoding/json: make encoders set Content-Type #41162

empijei opened this issue Sep 1, 2020 · 6 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@empijei
Copy link
Contributor

empijei commented Sep 1, 2020

What we do

Currently encoding a JSON to a http.ResponseWriter sets a plaintext content-type:

func handler(w http.ResponseWriter, r *http.Request) {
	e := json.NewEncoder(w)
	err := e.Encode(map[string]string{"foo": "bar"})
	if err != nil {
		panic(err)
	}
}

func main() {
	r, w := httptest.NewRequest("GET", "/", nil), httptest.NewRecorder()
	handler(w, r)
	fmt.Println(w.Header().Get("Content-Type")) // text/plain; charset=utf-8
}

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 uses http.DetectContentType on the first slice passed to Write, 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 a Header() http.Header method, and if so set the appropriate content-type.

Cons:

  1. encoding/json will depend on net/http, which seems less than desirable
  2. It will make encoding slightly slower as it adds a type inference call to Encode calls

Pros:

  1. Setting the appropriate content-type is more correct (while technically JSON is also plain text, "application/json" would be more appropriate)
  2. Browsers implement some security features to protect sensitive data from being leaked cross-site, and those features heavily rely on content-type (e.g. CORB)

/cc @mvdan @rsc @dsnet @bradfitz as per owners.
@gopherbot gopherbot added this to the Proposal milestone Sep 1, 2020
@mvdan
Copy link
Member

mvdan commented Sep 1, 2020

there is no good way to sniff JSON.

Could you expand a bit on this, for the sake of clarity?

encoding/json will depend on net/http, which seems less than desirable

I share this concern; teaching encoding/json about net/http sounds wrong :)

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.

@ainar-g
Copy link
Contributor

ainar-g commented Sep 1, 2020

Related: #10630.

@empijei
Copy link
Contributor Author

empijei commented Sep 2, 2020

Could you expand a bit on this, for the sake of clarity?

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.

I share this concern; teaching encoding/json about net/http sounds wrong :)

Yup, this is my biggest concern TBH.

Also, have you thought about fixing this via static analysis?

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.

@thoeni
Copy link
Contributor

thoeni commented Sep 2, 2020 •
edited

What we could do

I propose to make (*json.Encoder).Encode() calls check if the passed argument has a Header() http.Header method, and if so set the appropriate content-type.

Instead of using the Encoder we could have the json.NewEncoder check:

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 Encode() call, but number 1. would still hold true and json would know about http, which I agree on your point 1.

Cons:

  1. encoding/json will depend on net/http, which seems less than desirable
  2. It will make encoding slightly slower as it adds a type inference call to Encode calls

It would feel somewhat more natural to have a http.NewJSONEncoder that wraps json.NewEncoder and knows what to do with http headers when dealing with JSON, but I guess there you have no guarantee that people would use it: is this why this option might now be a good one?

@rsc rsc added this to Active in Proposals (old) Sep 2, 2020
@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

JSON comes from many places; tying this to the encoder seems like a mistake.
Also lots of non-JSON is valid JSON, such as the one-word English text true.

Can you say more about the Pros? I am not too worried about "technically more correct".
What are the security features that are defeated by sending JSON as text/plain?

@empijei
Copy link
Contributor Author

empijei commented Sep 7, 2020

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.

@empijei empijei closed this as completed Sep 7, 2020
@rsc rsc changed the title proposal: encoding/json: Make encoders set Content-Type. proposal: encoding/json: Make encoders set Content-Type Sep 16, 2020
@rsc rsc changed the title proposal: encoding/json: Make encoders set Content-Type proposal: encoding/json: make encoders set Content-Type Sep 16, 2020
@rsc rsc moved this from Active to Declined in Proposals (old) Sep 16, 2020
@rsc rsc mentioned this issue Sep 16, 2020
Open
@golang golang locked and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
6 participants
@rsc @empijei @thoeni @mvdan @ainar-g @gopherbot