Skip to content

Fix 3007#3028

Merged
Rokt33r merged 7 commits intoBoostIO:masterfrom
amedora:fix-3007
Jul 27, 2019
Merged

Fix 3007#3028
Rokt33r merged 7 commits intoBoostIO:masterfrom
amedora:fix-3007

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 Rokt33r and ZeroX-DG 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
@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

ZeroX-DG commented Jul 26, 2019

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

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

@Rokt33r Rokt33r Rokt33r approved these changes

+1 more reviewer

@AWolf81 AWolf81 AWolf81 left review comments

Reviewers whose approvals may not affect merge requirements

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