encoding/json: Compact escapes U+2028 and U+2029 #30357
Comments
dmitshur
added
the
NeedsInvestigation
|
It definitively is related; I will argue that escaping Compact input was unintended though. 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. |
|
Would you like to reopen this issue, so that it can be investigated? |
|
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. |
|
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. |
|
@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? |
|
Change https://golang.org/cl/173417 mentions this issue: |
|
Change https://golang.org/cl/188717 mentions this issue: |
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>
dmitshur
added
Documentation
NeedsFix
|
Change https://golang.org/cl/192747 mentions this issue: |
…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>
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>
|
Change https://golang.org/cl/200217 mentions this issue: |
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>
What version of Go are you using (
go version)?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.
The text was updated successfully, but these errors were encountered: