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

Replace lodash templates with static react renderer for field formatters #96048

Merged

Conversation

legrego
Copy link
Member

@legrego legrego commented Apr 1, 2021

Summary

Replaces the rendering logic for the source and color field formatters. Previous implementation uses lodash.template, which requires unsafe code execution (unsafe-eval in CSP parlance). New implementation uses ReactDOM to render static markup.

Resolves #94627

@legrego legrego added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Security/CSP Platform Security - Content Security Policy v7.13.0 labels Apr 1, 2021
@legrego legrego marked this pull request as ready for review April 1, 2021 16:06
@legrego legrego requested a review from a team as a code owner April 1, 2021 16:06
@legrego legrego added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 1, 2021
{defPairs.map((pair, idx) => (
<Fragment key={idx}>
<dt
dangerouslySetInnerHTML={{ __html: `${pair[0]}:` }} // eslint-disable-line react/no-danger
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at this one again, I don't think it should be dangerously rendered, unless I call escape. What is your preference / what is the expected behavior?

@legrego
Copy link
Member Author

legrego commented Apr 1, 2021

@elastic/kibana-app-services how can I test the source field formatter?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 782.4KB 783.1KB +735.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@legrego legrego enabled auto-merge (squash) April 2, 2021 17:53
Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

changes lgtm and seem to work well BUT - please verify that the url formatters are working correctly. I think its working but I did see it fail as well. I think it was a build problem so I'd like a second pair of eyes to verify.

@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #96188

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 3, 2021
…ers (#96048) (#96188)

Co-authored-by: Larry Gregory <larry.gregory@elastic.co>
@legrego
Copy link
Member Author

legrego commented Apr 3, 2021

@mattkime I'll verify url formatters on Monday. I had auto-merge turned on for this PR, otherwise I'd have waited to merge until verification. Sorry about that!

@legrego legrego deleted the chore/no-lodash-template-for-field-formatters branch April 6, 2021 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Security/CSP Platform Security - Content Security Policy release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace lodash template in field formatters
3 participants