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
from

Conversation

Projects
None yet
2 participants
@Framartin

Framartin commented Jul 6, 2017

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

@Framartin Framartin referenced this pull request Jul 6, 2017

Open

Add markdown support #96

@ocdtrekkie

This comment has been minimized.

Owner

ocdtrekkie commented Jul 6, 2017

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

@Framartin

This comment has been minimized.

Framartin commented Jul 6, 2017

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

This comment has been minimized.

Owner

ocdtrekkie commented Jul 6, 2017

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

This comment has been minimized.

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

This comment has been minimized.

Framartin commented Jul 6, 2017

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

This comment has been minimized.

Framartin commented Jul 6, 2017

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

This comment has been minimized.

Framartin commented Jul 6, 2017

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

@Framartin

This comment has been minimized.

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

This comment has been minimized.

Framartin commented Jul 20, 2017

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

@ocdtrekkie

This comment has been minimized.

Owner

ocdtrekkie commented Jul 20, 2017

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

@Framartin

This comment has been minimized.

Framartin commented Jul 21, 2017

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