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

encoding/json: Compact escapes U+2028 and U+2029 #30357

Closed
PhilipBorgesen opened this issue Feb 22, 2019 · 11 comments
Closed

encoding/json: Compact escapes U+2028 and U+2029 #30357

PhilipBorgesen opened this issue Feb 22, 2019 · 11 comments

Comments

@PhilipBorgesen
Copy link
Contributor

@PhilipBorgesen PhilipBorgesen commented Feb 22, 2019

What version of Go are you using (go version)?

$ go version
go version go1.11 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://play.golang.org/p/XvnF676cmhY

What did you expect to see?

I expected json.Compact to only elide insignificant whitespace as documented, in this case outputting the input untouched.

What did you see instead?

json.Compact escaped U+2028 and U+2029 inside the string, causing the output to be less compact than the input.

The cause appears to be on line 31 of indent.go (https://golang.org/src/encoding/json/indent.go?s=736:1005#L11) where the escape parameter is missing as a branch condition.

@dmitshur dmitshur added this to the Go1.13 milestone Feb 22, 2019
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Feb 22, 2019

This may be related to #5836, a security issue, which was fixed by escaping these 2 characters in commit d754647.

@PhilipBorgesen

This comment has been minimized.

Copy link
Contributor Author

@PhilipBorgesen PhilipBorgesen commented Feb 23, 2019

It definitively is related; I will argue that escaping Compact input was unintended though.
If Compact were meant to transform the input in this surprising way I would have expected the Indent function to showcase the same behavior, which is not the case:

https://play.golang.org/p/JAydUaEnVwB

I would also have expected that the documentation for Compact would have been updated as it was for HTMLEscape but that neither happened.

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Feb 23, 2019

Would you like to reopen this issue, so that it can be investigated?

@PhilipBorgesen

This comment has been minimized.

Copy link
Contributor Author

@PhilipBorgesen PhilipBorgesen commented Feb 23, 2019

Hi dmitshur

I think I accidentially tapped “Close and comment” instead of “Comment”. I did not mean to close the issue.

Sorry for the inconvenience, I want the issue to be investigated.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Feb 25, 2019

CC @rsc @dsnet @bradfitz @mvdan for encoding/json.

(Note also https://github.com/tc39/proposal-json-superset, which may mitigate some of the concerns relating to #5836.)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 25, 2019

Perhaps we just add a package comment to encoding/json that says, effectively, that "whenever we say JSON we actually mean JSON that's also valid JavaScript". Then Compact's docs would be correct.

Does that work?

Because I think this is really a documentation omission if anything. I don't think we want to change behavior here.

@PhilipBorgesen

This comment has been minimized.

Copy link
Contributor Author

@PhilipBorgesen PhilipBorgesen commented Feb 25, 2019

@bradfitz: If we go with such wording as a package comment I would expect Compact to return an error, not to alter the contents silently. To be clear I do not want errors to generated for RFC 7159 compliant JSON, which is what I expect encoding/json to deal with based on the package comment.

Why do we want Compact to perform this additional escaping? Is it not the purpose and responsibility of the HTMLEscape function?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 23, 2019

Change https://golang.org/cl/173417 mentions this issue: encoding/json: document HTML escaping in Compact

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 2, 2019

Change https://golang.org/cl/188717 mentions this issue: encoding/json: revert Compact HTML escaping documentation

gopherbot pushed a commit that referenced this issue Sep 1, 2019
This partly reverts CL 173417 as it incorrectly documented that Compact
performed HTML escaping and the output was safe to embed inside HTML
<script> tags. This has never been true.

Although Compact does escape U+2028 and U+2029, it doesn't escape <, >
or &. Compact is thus only performing a subset of HTML escaping and it's
output is not safe to embed inside HTML <script> tags.

A more complete fix would be for Compact to either never perform any
HTML escaping, as it was prior to CL 10883045, or to actually perform
the same HTML escaping as HTMLEscape. Neither change is likely safe
enough for go1.13.

Updates #30357

Change-Id: I912f0fe9611097d988048b28228c4a5b985080ba
GitHub-Last-Rev: aebabab
GitHub-Pull-Request: #33427
Reviewed-on: https://go-review.googlesource.com/c/go/+/188717
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 2, 2019

Change https://golang.org/cl/192747 mentions this issue: [release-branch.go1.13] encoding/json: revert Compact HTML escaping documentation

gopherbot pushed a commit that referenced this issue Sep 2, 2019
…ocumentation

This partly reverts CL 173417 as it incorrectly documented that Compact
performed HTML escaping and the output was safe to embed inside HTML
<script> tags. This has never been true.

Although Compact does escape U+2028 and U+2029, it doesn't escape <, >
or &. Compact is thus only performing a subset of HTML escaping and it's
output is not safe to embed inside HTML <script> tags.

A more complete fix would be for Compact to either never perform any
HTML escaping, as it was prior to CL 10883045, or to actually perform
the same HTML escaping as HTMLEscape. Neither change is likely safe
enough for go1.13.

Fixes #34006
Updates #30357

Change-Id: I912f0fe9611097d988048b28228c4a5b985080ba
GitHub-Last-Rev: aebabab
GitHub-Pull-Request: #33427
Reviewed-on: https://go-review.googlesource.com/c/go/+/188717
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 79669dc)
Reviewed-on: https://go-review.googlesource.com/c/go/+/192747
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
This partly reverts CL 173417 as it incorrectly documented that Compact
performed HTML escaping and the output was safe to embed inside HTML
<script> tags. This has never been true.

Although Compact does escape U+2028 and U+2029, it doesn't escape <, >
or &. Compact is thus only performing a subset of HTML escaping and it's
output is not safe to embed inside HTML <script> tags.

A more complete fix would be for Compact to either never perform any
HTML escaping, as it was prior to CL 10883045, or to actually perform
the same HTML escaping as HTMLEscape. Neither change is likely safe
enough for go1.13.

Updates golang#30357

Change-Id: I912f0fe9611097d988048b28228c4a5b985080ba
GitHub-Last-Rev: aebabab
GitHub-Pull-Request: golang#33427
Reviewed-on: https://go-review.googlesource.com/c/go/+/188717
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 10, 2019

Change https://golang.org/cl/200217 mentions this issue: encoding/json: stop escaping U+2028 and U+2029 in Compact

gopherbot pushed a commit that referenced this issue Oct 10, 2019
Compact has been inconsistently escaping only some problematic characters
(U+2028 and U+2029), but not others (<, > and &). This change addresses
this inconsistency by removing the escaping of U+2028 and U+2029.

Callers who need to escape the output of Compact should use HTMLEscape
which escapes <, >, &, U+2028 and U+2029.

Fixes #34070
Fixes #30357
Updates #5836

Change-Id: Icfce7691d2b8b1d9b05ba7b64d2d1e4f3b67871b
GitHub-Last-Rev: 38859fe
GitHub-Pull-Request: #34804
Reviewed-on: https://go-review.googlesource.com/c/go/+/200217
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.