Skip to content
This repository has been archived by the owner on Aug 29, 2018. It is now read-only.

Replaced marked with markdown-it #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liamcurry
Copy link

marked hasn't been updated in nearly a year, is looking unmaintained, and was just found to be vulnerable to XSS attacks.

This PR replaces marked with markdown-it, a newer, actively maintained Markdown parser based on remarkable. markdown-it conforms to the CommonMark spec, but also supports Github flavored Markdown stuff like tables and code fencing (disabled by default). It's also faster than marked according to their benchmarks.

The changes in this PR are mainly replacing the marked library with markdown-it, and tweaking the Options for this new parser. Here are the options markdown-it supports natively for reference.

One non-trivial change (from a users perspective) is enabling input sanitization by default (which is now behind the html = False flag). It's very easy to forget about enabling sanitization with custom options, and I think having it on by default is worth avoiding the risk of accidentally releasing XSS exploitable code into production. I'd like to hear what you all think about this.

Feedback welcome. I'm happy to make nitpick/stylistic changes as well.

Should fix #6, #9, #16, and #17.

@@ -64,36 +77,47 @@ toHtml attrs string =
[dash]: http://en.wikipedia.org/wiki/Dash
-}
type alias Options =
{ githubFlavored : Maybe { tables : Bool, breaks : Bool }
{ full : Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

full is a terrible name for an option. I have no idea what this does

@eeue56
Copy link
Contributor

eeue56 commented May 19, 2016

I've left some comments. Generally, it's best practice to not just copy whatever JS libaries do. They use bad names and bad data structures. Think about the perspective of the Elm user. Names should be clear, and sensible. You can convert a option record to a record that matches the markdown library after the Elm programmer has touched it. Make life easier for the Elm programmer, not for the JS lib.

@liamcurry
Copy link
Author

"full" enables all of what markdown-it has to offer. I didn't stay with "githubFlavored" because markdown-it offers more than that I think. But "githubFlavored" would still probably be a better/more accurate name than "full".

For the quotes, markdown-it allows you to change the "smart" quotes that it adds if you've enabled that option. To change them, you can use a string 4 characters wide, e.g. “”‘’, or an array of 4 characters, ['“', '”', '‘', '’']. To be more precise and avoid runtime errors I thought it'd be better to use a small record, where "dl" = double quote left, "dr" = double quote right, "sl" = single quote left, and "sr" = single quote right. I suppose I could make them a little longer, to something like "doubleLeft", "doubleRight", "singleLeft", and "singleRight". Would that work?

@eeue56
Copy link
Contributor

eeue56 commented May 19, 2016

naming is hard! githubFlavoured would keep the API consistent, so I would probably stick with that. Or withExtras or something.

👍 much better names for the quote options

@liamcurry
Copy link
Author

Just updated the PR with some better names. Any other issues?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use CommonMark instead of some loosely defined Markdown flavour
2 participants