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.

Sign up for GitHub

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

Jump to bottom

Set values as html to table rows #48911

Merged
merged 3 commits into from
Oct 24, 2019
Merged

Set values as html to table rows #48911

merged 3 commits into from
Oct 24, 2019

Conversation

gospodarsky
Copy link

@gospodarsky gospodarsky commented Oct 22, 2019 •
edited by timroes
Loading

Summary

Update: Has been reverted (#49306)

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@gospodarsky gospodarsky requested a review from a team as a code owner October 22, 2019 14:32
@gospodarsky gospodarsky self-assigned this Oct 22, 2019
@alexwizp
Copy link
Contributor

Fix regression of #43240

ppisljar
ppisljar approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal kertal self-requested a review October 22, 2019 20:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

kertal
kertal approved these changes Oct 23, 2019
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally in Chrome

@kertal kertal mentioned this pull request Oct 23, 2019
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@gospodarsky
Copy link
Author

Retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@gospodarsky gospodarsky merged commit 3fd9f91 into elastic:master Oct 24, 2019
@gospodarsky gospodarsky mentioned this pull request Oct 24, 2019
Closed
@kertal
Copy link
Member

kertal commented Oct 24, 2019 •
edited
Loading

dear @LeeDr could we merge this into 7.5, it's not critical but fixing 2 regressions?

@kertal
Copy link
Member

kertal commented Oct 24, 2019

We need to undo / or improve this! There are serious security concerns with this solution:

For instance, there could be an ES field containing: <img src="x" onerror="alert(1)">, this would lead to the following on IE11:

Bildschirmfoto 2019-10-24 um 11 04 44

Aside from this worst case any html stored in the ES record would be displayed

So we need to be sure / check that the html is generated by our code / formatters, then setting using dangerouslySetInnerHTML would be ok. However, it's never a good solution, so I think finishing #48728 should be preferred.

@timroes
Copy link
Contributor

timroes commented Oct 24, 2019

As @kertal mentioned: this needs to be reverted due to the introduced XSS attack. Please @Avinar-24 create a PR that reverts that change from master. So we need to wait for the actual save implementation @kertal is working on for fixing this actually.

Also that PR showed that we're missing the react/no-danger linting rule for TypeScript files. Will add that, so linting would have caught this.

timroes added a commit that referenced this pull request Oct 25, 2019
@timroes
Revert "Set values as html to table rows (#48911)"
3c420ac 
This reverts commit 3fd9f91.
@timroes timroes mentioned this pull request Oct 25, 2019
Merged
@timroes timroes added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix v7.6.0 labels Oct 25, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes pushed a commit that referenced this pull request Oct 25, 2019
Revert "Set values as html to table rows" (#49306)
2ddbc9e 
* Revert "Set values as html to table rows (#48911)"

This reverts commit 3fd9f91.

* Remove unrelated change from the revert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexwizp alexwizp alexwizp left review comments

@ppisljar ppisljar ppisljar approved these changes

@kertal kertal kertal approved these changes

Assignees

@gospodarsky gospodarsky

Labels
Feature:Discover Discover Application regression release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gospodarsky @alexwizp @elasticmachine @kertal @timroes @ppisljar