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

Fix 3007 #3028

Merged
merged 7 commits into from
Jul 27, 2019
Merged

Fix 3007 #3028

merged 7 commits into from
Jul 27, 2019

Conversation

amedora
Copy link
Contributor

@amedora amedora commented May 24, 2019

Description

Not only for mermaid, other code block like 'sequence' as below has potentially XSS problem.

```sequence
<iframe src="javascript:alert('xss');">
\```

Issue fixed

#3007

Type of changes

  • 🔘 Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • ⚪ All existing tests have been passed
  • ⚪ I have attached a screenshot/video to visualize my change if possible

@Rokt33r Rokt33r requested review from ZeroX-DG and Rokt33r May 24, 2019 07:29
@Rokt33r Rokt33r added the awaiting review ❇️ Pull request is awaiting a review. label May 24, 2019
ZeroX-DG
ZeroX-DG requested changes May 25, 2019
View reviewed changes
browser/components/MarkdownPreview.js Outdated Show resolved Hide resolved
@amedora
Revert "fix mermaid xss"
1e317c4 
This reverts commit 1ff179a.
@amedora
Copy link
Contributor Author

amedora commented May 28, 2019

I think that most straightforward way to fix this is passing "flowchart: { htmlLabels: false }" option to mermaidAPI.initialize().
https://github.com/BoostIO/Boostnote/blob/76335f78aca94b39841b03af4c212bec65862e5b/browser/components/render/MermaidRender.js#L29-L33
However it would be drawback for users who use HTML in mermaid charts.

@AWolf81
Copy link
Contributor

AWolf81 commented May 28, 2019

@amedora maybe we can add this as option - disabled html labels by default. With a warning at the option that there is a risk of xss. So Mermaid html label users can decide and accept the risk.

That's not perfect but it will fix xss. Maybe we can open an issue at Mermaid so we can track if there will be a fix.

@amedora
Copy link
Contributor Author

amedora commented May 29, 2019

It's now configurable in the option panel. works perfect on my end.
option

@amedora
Merge branch 'master' into fix-3007
a0d2b19 
# Conflicts:
#	browser/main/lib/ConfigManager.js
@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jul 22, 2019
@amedora
Merge branch 'master' into fix-3007
9b231aa 
# Conflicts:
#	locales/da.json
#	locales/de.json
#	locales/en.json
#	locales/es-ES.json
#	locales/fa.json
#	locales/fr.json
#	locales/hu.json
#	locales/it.json
#	locales/ja.json
#	locales/ko.json
#	locales/no.json
#	locales/pl.json
#	locales/pt-BR.json
#	locales/pt-PT.json
#	locales/ru.json
#	locales/sq.json
#	locales/th.json
#	locales/tr.json
#	locales/zh-CN.json
#	locales/zh-TW.json
@ZeroX-DG ZeroX-DG added needs extra review 🔎 Pull request requires review from an additional reviewer. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jul 26, 2019
@ZeroX-DG
Copy link
Member

LGTM 🎉 But I'll need review from @Rokt33r before I can approve this.

Rokt33r
Rokt33r approved these changes Jul 27, 2019
View reviewed changes
Copy link
Member

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Rokt33r Rokt33r added approved 👍 Pull request has been approved by sufficient reviewers. and removed needs extra review 🔎 Pull request requires review from an additional reviewer. labels Jul 27, 2019
@Rokt33r Rokt33r added this to the v0.13.0 milestone Jul 27, 2019
@Rokt33r Rokt33r merged commit 606be43 into BoostIO:master Jul 27, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Jul 27, 2019
@amedora amedora mentioned this pull request Aug 19, 2019
Closed
@amedora amedora deleted the fix-3007 branch September 3, 2019 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AWolf81 AWolf81 AWolf81 left review comments

@ZeroX-DG ZeroX-DG ZeroX-DG requested changes

@Rokt33r Rokt33r Rokt33r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@amedora @AWolf81 @ZeroX-DG @Rokt33r