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

Security fix for Cross-Site Scripting Vulnerability in the legend fields #604

Merged
merged 3 commits into from Mar 12, 2021

Conversation

arjunshibu
Copy link
Contributor

Fix for Cross-site Scripting vulnerability

Resolves the bug mentioned in #601 completely without affecting any functionality

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

Fully described here 418sec#2

Please do not merge this commit directly as I said here #601 (comment)
Once you are happy with the fix, please comment here 418sec#2 πŸš€. Thanks in advance πŸ˜„


Thank you for your contribution to TOAST UI product. πŸŽ‰ 😘 ✨

@jung-han
Copy link
Member

jung-han commented Mar 3, 2021

@arjunshibu
Sorry for late. I left a review on that repo! I would be grateful if you check it out.

@arjunshibu
Copy link
Contributor Author

@jung-han sure. I'll update it ASAP

@arjunshibu
Copy link
Contributor Author

arjunshibu commented Mar 5, 2021

@jung-han I've modified the types and added trailing commas. But as you can see one type check is still failing. When I give type as string | DocumentFragment in finalizeHtml(), this error goes but another one come from line 150 and 151 https://github.com/arjunshibu/tui.chart/blob/a7a212124dae136a84034648fc90fe9bfadfbd03/apps/chart/src/component/tooltip.ts#L147-L156.

A little help about the type from your side will be very useful.

@jung-han
Copy link
Member

jung-han commented Mar 8, 2021

@arjunshibu
This is the new sanitizer with the type added. I brought a link in case it could be a reference. If you see this part and it is still difficult to confirm, please let me know. 🌝

@arjunshibu
Copy link
Contributor Author

@jung-han thank you so much. I'll work on this tomorrow πŸ˜„

@arjunshibu
Copy link
Contributor Author

@jung-han sorry for the delay. All errors are now resolved πŸ˜„ Thanks again for the type suggestion. If you are okay with this commit, please comment @huntr-helper - LGTM on 418sec#2

@jung-han
Copy link
Member

@arjunshibu
I'll let you know after testing tomorrow! Thanks for the fix!

@arjunshibu
Copy link
Contributor Author

arjunshibu commented Mar 11, 2021

@jung-han Similar issue exists on https://github.com/nhn/tui.calendar in the event fields. Can I fix this?

@jung-han
Copy link
Member

@arjunshibu Well, how about posting it on the calendar discussion? It would be nice to talk to the maintainer.

@jung-han jung-han merged commit 1a3f455 into nhn:main Mar 12, 2021
8 checks passed
@arjunshibu
Copy link
Contributor Author

@jung-han can you please comment the message bot wants to hear on the 418sec repo, so that I can earn a reward for fixing this vulnerability 😊

@jung-han
Copy link
Member

jung-han commented Mar 12, 2021

@arjunshibu

@huntr-helper - LGTM right?

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

Successfully merging this pull request may close these issues.

None yet

2 participants