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

text/template: JSEscape generates invalid JSON #37634

Open
sding3 opened this issue Mar 3, 2020 · 12 comments
Open

text/template: JSEscape generates invalid JSON #37634

sding3 opened this issue Mar 3, 2020 · 12 comments
Labels
NeedsInvestigation
Milestone

Comments

@sding3
Copy link
Contributor

@sding3 sding3 commented Mar 3, 2020

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

$ go version
go version go1.14 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/shang/.cache/go-build"
GOENV="/home/shang/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY="*.epic.com"
GONOSUMDB="*.epic.com"
GOOS="linux"
GOPATH="/home/shang/go"
GOPRIVATE="*.epic.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build973203171=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran this code:

package main

import (
        "log"
        "os"
        "text/template"
)

func main() {
        const plate = `"{{ .Value | js}}"`

        type Vals struct {
                Value string
        }

        vals := &Vals{`=`}

        t := template.Must(template.New("plate").Parse(plate))

        err := t.Execute(os.Stdout, vals)
        if err != nil {
                log.Println("executing template:", err)
        }

}

What did you expect to see?

Expected to see "=", which is the behavior in go1.13.8

What did you see instead?

Insead see "\x3D".

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 3, 2020

This is intentional as security hardening. The semantic meaning of the two values are the same, so it should not break anything but tests. Escaping those extra characters makes the output robust against misuse, which was reported in the wild.

See #35665 and 94e9a5e.

@mheggeseth
Copy link

@mheggeseth mheggeseth commented Mar 3, 2020

However "\x3D" is not valid JSON so the js function can no longer be used to escape strings when templating a JSON document. Is there an alternative?

"\u003D" would have been valid in both code and JSON.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 3, 2020

JSEscape was already generating \x3C and \x3E so it sounds like it was never valid JSON? It would be nicer if it worked for both, though. @empijei, any thoughts?

@FiloSottile FiloSottile reopened this Mar 3, 2020
@FiloSottile FiloSottile changed the title text/template: go1.14 built-in function js behaves differently than go1.13.8 text/template: JSEscape generates invalid JSON Mar 3, 2020
@FiloSottile FiloSottile added the NeedsInvestigation label Mar 3, 2020
@FiloSottile FiloSottile added this to the Backlog milestone Mar 3, 2020
@mheggeseth
Copy link

@mheggeseth mheggeseth commented Mar 3, 2020

JSEscape was already generating \x3C and \x3E so it sounds like it was never valid JSON?

We were just lucky not to hit those characters in our use case which is writing a JSON document with values that are PEM-formatted certificates (< and > aren't in the Base64 character set).

@empijei
Copy link
Contributor

@empijei empijei commented Mar 10, 2020

Interesting issue.

I confirm this is intended behavior as JSEscape was only designed to be used to escape JS and not JSON. The fact that you were lucky not to hit escaping with b64 was just a coincidence and was not intended.

That said I undestand your pain and I would like to ask for a little bit more information before I try to address this:

  • Why are you using text/template to format JSON? If this has to be served as a webpage it would be better to use html/template with autoescaping: https://play.golang.org/p/0YXfGQooTq0 this will correctly print this as a "="
  • When you say "templating a JSON document" what do you mean? Is there any reasons why you are not using the encoding/json package?

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 27, 2020

Change https://golang.org/cl/226097 mentions this issue: html/template: switch to unicode escape instea of hex

@lpar
Copy link

@lpar lpar commented Mar 18, 2021

Note that JSEscape still generates invalid JSON:

	value := `"I said 'No!'"`
	var buf bytes.Buffer
	template.JSEscape(&buf, []byte(value))
	fmt.Println(buf.String()) // => \"I said \'No!\'\"

I'm guessing maybe @sding3 is wanting to template JSON for the same reason I do, for generating Slack messages?

@sding3
Copy link
Contributor Author

@sding3 sding3 commented Mar 19, 2021

@lpar , we were using it for escaping PEM encoded certificates. We ended up writing up our own custom template function to do JSON escapae. Sounds like the standard library could benefit from a JSONEscape function, eh?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 22, 2021

Huh, CL 226097 looks like it's replacing ' and " with Unicode escapes, too, but the code indeed changed the escape of = and not ' and ". @empijei?

@FiloSottile FiloSottile reopened this Mar 22, 2021
@empijei
Copy link
Contributor

@empijei empijei commented Apr 6, 2021

I have a couple of questions: if you are templating JSON, why not use text/template to run the templating step, and then encoding/json to serialize your message?

And if we find a legit case for supporting this: do we want to work on making sure that JSEscape will always yield valid JSON strings or do we prefer to add a new API to perform JSONEscape?

@lpar
Copy link

@lpar lpar commented Apr 6, 2021

In our case, the idea is to let users build messages using Slack block kit and then insert placeholders for the data in the JSON generated by Slack. Building objects from text/template generated data and then using encoding/json to serialize would basically involve duplicating the block kit API.

@empijei
Copy link
Contributor

@empijei empijei commented Apr 6, 2021

So I guess parsing the JSON generated by Slack into a map and then replacing the placeholders would not work for you?

How do you make sure users don't corrupt the JSON data structure when changing your template?

(While we discuss this I'll take a look into extending the change in https://golang.org/cl/226097 to also cover all the JS escaping functions so that we only use the JSON subset when emitting escaped data. This might take a while)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

6 participants