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

Fix/xss sanity check #2285

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Fix/xss sanity check #2285

merged 2 commits into from
Oct 1, 2021

Conversation

tx0c
Copy link
Contributor

@tx0c tx0c commented Sep 30, 2021

  • make unittest for server side for sure not to touch the pre code-block … 0341e8f
    part of reason for thematters/matters-web#2114 is server not saving exactly same pre code-block, as

    <pre class="ql-syntax" spellcheck="false">Pre1\n</pre><p>next paragraph...

    server side is always dropping the spellcheck="false", then next time when editor is loading
    the incomplete pre code-block, it triggers clipboard.convert to parse again, and in MattersArticleEditor
    the mentionContainer reference is causing ReactQuill to re-initialize twice every time, caused
    converting between HTML <=> Delta happening 6 or even more times, and eventually caused wrong parsing

  • sanitize xss filter whitelist pre: ['spellcheck'] 3c9fccc

part of reason for thematters/matters-web#2114 is server not saving exactly same pre code-block, as

    <pre class="ql-syntax" spellcheck="false">Pre1\n</pre><p>next paragraph...

server side is always dropping the `spellcheck="false"`, then next time when editor is loading
the pre code-block incomplete, it triggers `clipboard.convert` to parse again, and in MattersArticleEditor
the `mentionContainer` reference is causing ReactQuill to re-initialize twice every time, caused
converting between `HTML <=> Delta` happening 6 or even more times, and eventually caused wrong parsing
@tx0c tx0c requested a review from a team as a code owner September 30, 2021 18:42
@tx0c tx0c self-assigned this Sep 30, 2021
@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #2285 (3628a9d) into develop (3be1db8) will not change coverage.
The diff coverage is n/a.

❗ Current head 3628a9d differs from pull request most recent head 3c9fccc. Consider uploading reports for the commit 3c9fccc to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2285   +/-   ##
========================================
  Coverage    53.94%   53.94%           
========================================
  Files          412      412           
  Lines        10398    10398           
  Branches      2026     2026           
========================================
  Hits          5609     5609           
  Misses        4783     4783           
  Partials         6        6           
Impacted Files Coverage Δ
src/common/utils/xss.ts 28.57% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3be1db8...3c9fccc. Read the comment docs.

Copy link
Contributor

@guoliu guoliu left a comment

Choose a reason for hiding this comment

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

Nice catch! 🌿

@tx0c tx0c merged commit c053890 into develop Oct 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix/xss-sanity-check branch October 1, 2021 03:58
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