-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Html escaping #7445
Html escaping #7445
Conversation
04cc7c1
to
5793d03
Compare
e := &Encoder{version: version, config: config{ | ||
Pretty: pretty, | ||
EscapeHTML: escapeHTML, | ||
}} | ||
e.reset() | ||
return e | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a later refactoring would change the signature to take an object instead. No need to do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM WFG
Add support to codecs and outputs to enable/disable escaping of html symbols in JSON strings. By default html escaping is enabled.
Happy to merge this, I've looked at the current failure in the metricbeat job, this is not due to the change in this PR but a timeout on k8s. |
WDYT about having it off by default for 7.0? I would expect by default that no escaping is happening. |
+1 for changing this in 7.0 to disabled. |
Also agree with the above. |
I added this to the list here: #6106 I wonder if we should even remove to make it configurable in 7.0. What would be the reasons to enable it? |
Not sure if we should disable (or even remove) the setting by default. So far we've only heard from users not wanting to escape html. But we don't know how many users actually did rely on beats escaping the HTML symbols. Not escaping HTML puts consumers displaying raw event contents in browsers at risk. Reason escaping is still enabled by default is, I didn't want to introduce potential security issues for those users. |
Yes, we should not disable it 6.x as it would be a breaking change. Good point about the potential security implications for some users. If we disable it, we should mention it clearly that this could have an affect. In general I still think we can disable / remove it in 7.0 as I don't think escaping should be part of the ingesting data but should be done by the consumer if needed. |
Requires: #7444
Closes: #2581
Add support to codecs and outputs to enable/disable escaping of html symbols in JSON strings.
By default html escaping is enabled.
escape_html
tocodec.json
output.elasticsearch.escape_html
output.logstash.escape_html