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 markdown XSS #11

Open
wants to merge 3 commits into
base: sandstorm
Choose a base branch
from
Open

Fix markdown XSS #11

wants to merge 3 commits into from

Conversation

Framartin
Copy link

This MR fixes the stored XSS described here.

There are 2 commits :

  • one that adds the sanitize option of marked, cf doc
  • one that updates marked to avoid this vulnerability

Marked wasn't updated because of markedjs/marked#815

@ocdtrekkie
Copy link
Owner

It looks like the modified marked lib tosses the linksInNewTab support I added to Marked in 6acde85

@Framartin
Copy link
Author

Right, I didn't saw that you have added this commit. Sorry.

Is there not a cleaner way to add target="_blank" to all links, like using a callback function?

@ocdtrekkie
Copy link
Owner

In such that I don't actually know any JavaScript and someone had already made that handy change in a different unmerged PR to marked.js... it is the cleanest way I know how, literally. ;)

I guess the question is if the two adjustments to marked.js are mutually exclusive or if it's possible to merge them. It is really hard to tell on minified JS. :P

@Framartin
Copy link
Author

Framartin commented Jul 6, 2017

Yes, it's hard to work on minified JS :)

What version of marked, scrumblr is currently using? Is it a version that includes this PR markedjs/marked#592 ? Maybe the current file is ok, because I didn't checked it, I assume that it wasn't updated because it was different.

BTW, be careful to not use the marked.min.js file inside the marked GitHub repo. It's not updated and it doesn't include the PR.

@Framartin
Copy link
Author

I don't know any JS neither, but I know that directly modifying lib files is not a good idea, because it's very hard to keep track when you have to update them (especially on minified version).

@Framartin
Copy link
Author

Sorry, I didn't saw your comment on 6acde85. So the version of marked inside the marked.min.js file inside this repo is not the last one and is vulnerable. Because the marked.min.js inside the Luc's PR on the upstream scrumblr repo includes the vulnerable is a copy of the vulnerable marked.min.js that is at the top-level of the marked repo.

@Framartin
Copy link
Author

By the way, you should also add rel="noopener noreferrer" when you use target='_blank' to avoid this vulnerability.

@Framartin
Copy link
Author

Framartin commented Jul 6, 2017

@ocdtrekkie I have added 7d7ca5c to my PR. Can you check that it's working? I'm not sure, because I have some Uncaught ReferenceError: markedCallback is not defined on my JS console when I try to edit notes. But it may be because I'm modifying script.js on the dev panel of my web browser, so I add this modification when the script is already loaded.

@Framartin
Copy link
Author

@ocdtrekkie Up :) Do you need help or more info?

@ocdtrekkie
Copy link
Owner

Sorry, I just haven't tackled going in and testing this yet.

@Framartin
Copy link
Author

No problem. Keep me updated when you will do.

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.

2 participants