parse /groups/:groupName markdown; #345
Conversation
Copies auspice approach: https://github.com/nextstrain/auspice/blob/master/src/util/parseMarkdown.js but uses "sanitize-html" library: https://github.com/apostrophecms/sanitize-html A couple of config options from the auspice approach didn't seem to have analogous options in the library I use here. Also I don't add a hook to set the <a> tags to open links in a new tab as is done in auspice. There's probably a way to set this with the "sanitize-html" library but it wasn't obvious to me and it wasn't working anyway on nextstrain.org groups pages (which was using the auspice code to parse markdown until recently for groups pages).
|
I'm curious what's the motivation for switching to the "sanitize-html" library? |
|
@joverlee521 sorry, I meant to say. When adding DOMPurify to the package.json, I couldn't get the static-site to build. I think the reason has something to do with cure53/DOMPurify#29 (comment). There seemed to be a solution for that problem in general and someone even packaged it up here https://github.com/kkomelin/isomorphic-dompurify but that didn't work for me either; I ran into this issue kkomelin/isomorphic-dompurify#54 which doesn't seem to have a clear solution (yet). |
|
|
There was a problem hiding this comment.
I left a couple of small notes, but overall looks good.
sanitize-html seemed like the best alternative I could find.
Thanks for the clarification here, I forget this is now in Gatsby.
Also I don't add a hook to set the tags to open links in a new tab as is done in auspice.
The transformTags option might be a good for this? (I think it's definitely just a nice to have)
| console.error(`Error parsing footer description: ${error}`); | ||
| cleanDescription = '<p>There was an error parsing the footer description.</p>'; |
There was a problem hiding this comment.
The error/HTML message here may be misleading since this markdown display is not just used for the footer anymore.
| // These two config settings come from https://github.com/cure53/DOMPurify | ||
| // and it's not clear how to set them in https://github.com/apostrophecms/sanitize-html | ||
| // which we are using here. | ||
| // KEEP_CONTENT: false, |
There was a problem hiding this comment.
I think the closest equivalent for this in sanitize-html is the nonTextTags option. Their default is to discard text within style, script, textarea, option tags, which I think is enough?
It might be useful to explicitly state the default option here for future reference.
| // and it's not clear how to set them in https://github.com/apostrophecms/sanitize-html | ||
| // which we are using here. | ||
| // KEEP_CONTENT: false, | ||
| // ALLOW_DATA_ATTR: false |
There was a problem hiding this comment.
If i remember correctly, I included this as an extra precaution since it was an available option in DOMPurify. I don't think it's necessary since we explicitly state the allowed attributes (which does not include data-* attributes).
see #345 (review) for details
|
Thanks @joverlee521! I've tried to address all your feedback in e7b59e3. |
| const url = new URL(attribs.href); // URL is not supported on Internet Explorer | ||
| if (url.hostname !== location.hostname) { | ||
| attribs.target = '_blank'; | ||
| attribs.rel = 'noreferrer nofollow'; |
There was a problem hiding this comment.
@joverlee521 @jameshadfield I just got the following es-lint message in another context:
Using target="_blank" without rel="noopener noreferrer" is a security risk: see https://mathiasbynens.github.io/rel-noopenereslintreact/jsx-no-target-blank
Should we also have noopener here in addition to noreferrer nofollow?
Copies auspice approach:
https://github.com/nextstrain/auspice/blob/master/src/util/parseMarkdown.js
but uses "sanitize-html" library:
https://github.com/apostrophecms/sanitize-html
A couple of config options from the auspice approach
didn't seem to have analogous options in the library I
use here. Also I don't add a hook to set the tags
to open links in a new tab as is done in auspice. There's
probably a way to set this with the "sanitize-html" library
but it wasn't obvious to me and it wasn't working anyway
on nextstrain.org groups pages (which was using the auspice
code to parse markdown until recently for groups pages).