Disable HTMLEscape for zapcore.JSONEncoder

[henriquemenezes]

Hi,

I've been used zap for structured logging using zap.JSONEncoder, but currently it encodes HTML characters like (<, >, &) to Unicode codes (\u003c, \u003e, \u0026). So, the log that would be:

{ "level": "info", "time": "2019-04-17T19:54:53.739900902Z", "caller": "go-log/main.go:90", "message": "Hello & World" }

It is logged as:

{ "level": "info", "time": "2019-04-17T19:54:53.739900902Z", "caller": "go-log/main.go:90", "message": "Hello \u0026 World" }

My sugestion is to disable the HTML Escape in the zap.JSONEncoder after line zapcore/json_encoder.go#L139 as follow:

enc.reflectEnc.SetEscapeHTML(false)

[prashantv]

Seems reasonable, from the json documentation:

String values encode as JSON strings coerced to valid UTF-8, replacing invalid bytes with the Unicode replacement rune. The angle brackets "<" and ">" are escaped to "\u003c" and "\u003e" to keep some browsers from misinterpreting JSON output as HTML. Ampersand "&" is also escaped to "\u0026" for the same reason. This escaping can be disabled using an Encoder that had SetEscapeHTML(false) called on it.

Since zap logs to console or to files, I don't think the security concerns affect zap. Thoughts @abhinav?

[abhinav]

This seems like it would be a significant behavioral change. I disagree that
the security concern doesn't apply here. It's not inconceivable that someone
implements a system which displays JSON logs emitted by Zap in a browser.

I still think this is a reasonable ask but it should be opt-in. Perhaps a new
constructor for JSON encoder which takes options in addition to EncoderConfig?

[prashantv]

Actually looking more at the code, we already don't try to protect against this. Zap uses a custom JSON encoder for known primitives, and these types are not protected:

zap/zapcore/json_encoder.go

Lines 429 to 431 in badef73

	// safeAddString JSON-escapes a string and appends it to the internal buffer.
	// Unlike the standard library's encoder, it doesn't attempt to protect the
	// user from browser vulnerabilities or JSONP-related problems.

And the tests also verify that we don't escape <,

zap/zapcore/json_encoder_impl_test.go

Lines 75 to 76 in badef73

	"<": "<",
	">": ">",

I verified by changing one of the example tests to include <&> in the message, and saw that they did not get escaped:

{"level":"info","logger":"main","msg":"main logger <&>"}

The only case where we don't escape is within a nested object that's encoded by the JSON reflection library. I think there's no reason not to enable this by default since we're already not protecting against this case.

[abhinav]

Fair enough.