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

Sanitize only potential XSS HTML code, rather than all HTML on submit of edit page #380

Open
Syndamia opened this issue Sep 15, 2022 · 4 comments
Assignees
Labels
bug Issue with Existing Functionality priority Important
Milestone

Comments

@Syndamia
Copy link

Syndamia commented Sep 15, 2022

Problem

On the Edit page of any document, on submit, content sanitization is done like this:

var sanitized_content = validator.escape(complete_content);

This seems to have been done to fix cross-site scripting issues.

However, I feel it is a bit too aggressive. There are genuine use cases for inline HTML. Definition lists and table colspan (for example) are useful features of HTML that Markdown doesn't support.

Also, backticks are escaped, which prevents code blocks from rendering (blockquotes too are escaped, but they seem to have no styling).

Finally, given that meta tags are stored in the markdown files themselves, meta data also has issues. For example, quotes in description or title are escaped, while they shouldn't!

Proposition

Use a proper cross-site scripting sanitization library, like DOMPurify. It should be able to replace the validator library in page.edit.route.js (and sanitize.js?).

@ryanlelek
Copy link
Owner

You're right and that's a great suggestion.

The patch #370 was put out quickly
#381 is also related

Marking as a bug.
I have limited free time at the moment, but will make some time on the weekends.
PRs also welcomed if you have the time and interest.

#379 also mentions Blockquotes for reference

@ryanlelek ryanlelek added the bug Issue with Existing Functionality label Oct 26, 2022
@ryanlelek ryanlelek added this to the v0.18.x milestone Feb 21, 2024
@ryanlelek ryanlelek self-assigned this Feb 21, 2024
@ryanlelek
Copy link
Owner

@Syndamia please check the latest main branch or newest NPM package v0.17.7 (should be published in 24 hours)
I loosened the aggressive sanitization because while security is good it's massively affecting the usability like you mentioned.

Related commit: 863aaf5

Opinion: Security good, but many users are not (hopefully) letting any public visitor edit. Only trusted members / employees / etc

@ryanlelek ryanlelek added the pending Pending Close. Respond label Feb 21, 2024
@Syndamia
Copy link
Author

Hello, I'm sorry for the very late response.

Inline HTML

All inline HTML still seems to get escaped. Definition lists seem to be completely unsupported (before I think they were properly parsed and rendered while editing, but then didn't show up on a saved page).

Backticks & quoteblocks

Backticks and quoteblocks seem fixed.

Quotes in metadata

Quotes in metadata still cause issues. Quotes in Title will be saved as-is, while description will be surrounded with single quotes:

---
Title: Test "Number 4"
Description: '"Something important"'
---

This seems to cause issues for the UI, it looks as if any (single or double) quoted text in a metadata block is disregarded. The metadata fields of the aforementioned Markdown looks like this when editing:

image

P.S. On version 0.17.8 all pages disappear from the left "menu", even when I add a new page. No, they're not just hidden by the styling, the HTML for them doesn't even exist.
For this reason, all testing was done on 0.17.7

image
image

@ryanlelek
Copy link
Owner

Thank you for the detailed update!
I'll look further into this. Seems like the foundation needs a rework, it has been well over 10 years and can use some attention.

@ryanlelek ryanlelek added priority Important and removed pending Pending Close. Respond labels Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue with Existing Functionality priority Important
Projects
None yet
Development

No branches or pull requests

2 participants