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

Html escaping #7445

Merged
merged 3 commits into from
Jun 27, 2018
Merged

Html escaping #7445

merged 3 commits into from
Jun 27, 2018

Conversation

urso
Copy link

@urso urso commented Jun 27, 2018

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.

  • Add new setting escape_html to codec.json
  • Add new setting output.elasticsearch.escape_html
  • Add new setting output.logstash.escape_html
  • Add settings to reference config files and output documentation

@urso urso force-pushed the html-escaping branch 2 times, most recently from 04cc7c1 to 5793d03 Compare June 27, 2018 16:45
@urso urso added the review label Jun 27, 2018
@ph ph added the libbeat label Jun 27, 2018
e := &Encoder{version: version, config: config{
Pretty: pretty,
EscapeHTML: escapeHTML,
}}
e.reset()
return e
}
Copy link
Contributor

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.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM WFG

urso added 3 commits June 27, 2018 19:58
Add support to codecs and outputs to enable/disable escaping of html
symbols in JSON strings.

By default html escaping is enabled.
@ph
Copy link
Contributor

ph commented Jun 27, 2018

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.

@ph ph merged commit 1f01fec into elastic:master Jun 27, 2018
@ruflin
Copy link
Member

ruflin commented Jun 27, 2018

WDYT about having it off by default for 7.0? I would expect by default that no escaping is happening.

@andrewkroh
Copy link
Member

+1 for changing this in 7.0 to disabled.

@ph
Copy link
Contributor

ph commented Jun 27, 2018

Also agree with the above.

@ruflin
Copy link
Member

ruflin commented Jun 28, 2018

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?

@urso
Copy link
Author

urso commented Jun 28, 2018

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.

@ruflin
Copy link
Member

ruflin commented Jun 28, 2018

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.

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

Successfully merging this pull request may close these issues.

4 participants