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

Allow url protocol block/allow lists #1361

Closed
wants to merge 9 commits into from

Conversation

aprotim
Copy link
Contributor

@aprotim aprotim commented Oct 19, 2018

Markdown flavor: all

Description

Add the ability to set lists of allowable or blocked url protocols. Replaces the current "sanitize" code for URLs with explicit controls. (Existing behavior for sanitize is retained, for compatibility, but if/when sanitize is removed generally, urls can still be restricted to only desired protocols.

The included tests have examples.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@aprotim
Copy link
Contributor Author

aprotim commented Oct 19, 2018

9 commits later, I got the tests to finally pass.

@styfle
Copy link
Member

styfle commented Oct 20, 2018

Hi @aprotim thanks again for the PR

This looks like it's adding new options which is not the desire for marked at this time. Especially since we are going to remove sanitization in the future (#1232).

A block-list or allow-list can be implemented outside of marked using something like dompurify or sanitize-html.

Example sanitize

Here's an example of using an block-list to remove bad protocols.

var blockList = ["http", "https", "ftp", "mailto"];
var dirty = marked(md);
var clean = sanitizeHtml(dirty, {
  transformTags: {
    'a': function(tagName, attribs) {
        if (attribs.href && blockList.some(p => attribs.href.startsWith(p))) {
            delete attribs.href;
        }
        return { tagName: tagName, attribs: attribs };
    }
  }
});

@aprotim
Copy link
Contributor Author

aprotim commented Oct 21, 2018

I understand the desire to limit feature creep, to which end I've filed #1362.

I would like to point out that there's a lot of value of sanitizing the (post-parse, pre-render) input and not (just) the output: keeping up with the ins and outs of the complex ways HTML can be abused is a harder problem than restricting the already smaller subset of functionality of markdown. (As far as I can tell, the vast majority, if not all, markdown-related XSS vulnerabilities could be avoided by disallowing embedded HTML and only allowing URLs from a small set of URL schemas.)

@aprotim aprotim closed this Oct 21, 2018
@aprotim aprotim deleted the allowable_url_protocols branch October 21, 2018 09:37
@aprotim aprotim restored the allowable_url_protocols branch October 21, 2018 09:37
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