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

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
Projects
Milestone

Comments

@empijei
Copy link
Contributor

@empijei 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
@gopherbot gopherbot added the Proposal label Sep 1, 2020
@mvdan
Copy link
Member

@mvdan 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 ainar-g commented Sep 1, 2020

Related: #10630.

@empijei
Copy link
Contributor Author

@empijei 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 thoeni commented Sep 2, 2020

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 Sep 2, 2020
@rsc
Copy link
Contributor

@rsc 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 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 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 Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.