feature: add rel=nofollow to links if the `nofollow` option is set - Githubissues

npm / marky-markdown

npm's markdown parser

https://www.npmjs.com/package/@npmcorp/marky-markdown

405 stars 72 forks source link

feature: add rel=nofollow to links if the `nofollow` option is set #426

Closed ceejbot closed 6 years ago

ceejbot commented 6 years ago

Added a plugin that adds rel=nofollow to links and does no more.
Added an option to control if the plugin is loaded, defaulted to true, and documented the option.
Wrote tests to verify that the rel attr is added, and that the option is respected.
Added rel to the allowed list of attributes in the sanitizer.

isaacs commented 6 years ago

This works for [markdown](links) but plain old <a> tags don't get munged.

A patch that adds support for <a> html tags as well, not sure if it's the most elegant approach, but seems to work. https://gist.github.com/isaacs/16c0595f48bbaa8d44898112ff112630

ceejbot commented 6 years ago

Aha, I missed that case. Nice catch. Will snag patch.

revin commented 6 years ago

Weird; what happened to the CI build? Manually canceled?

isaacs commented 6 years ago

@revin That's correct. I think in that case, you've explicitly said that you want to add rel=nofollow to markdown links, but don't want to do any sanitization of inline html. So, any inline html should be untouched (because we're not sanitizing), and thus, you're getting exactly what you asked for. Note that you'd also be allowing <a onclick=alert(/xss/)>.

I think that cases where you're using nofollow on trusted code would be kind of unusual anyway, no?

revin commented 6 years ago

@isaacs yeah I'll buy that. I don't think we've ever had a particularly well defined policy on whether or not we treat markdown content and HTML content as having different semantics around their processing. I'd be very interested in just leaving raw HTML alone and being only a markdown processor, although I'm like 95% sure that would break real READMEs in the wild.

revin commented 6 years ago

Guess now we gotta figure out how to fix the CI build.

revin commented 6 years ago

(labeling this as semver-major since the new option is defaulting to true)

ceejbot commented 6 years ago

Tests passed when I ran them by hand, fwiw.

revin commented 6 years ago

Yeah they only seem to fail on node 6, which type of problem historically in this project indicates an upstream dependency dropping support (read: using newer JS features unsupported in the older node) somewhere along the line