Swap out simple-xss for sanitize-html #190

Martii · 2014-06-20T07:37:01Z

PR is NOT COMPLETE... better than what we have and also may have future dependencies
Advantages:

Works unlike simple-xss
Lots of maintainers/contributors
Smaller than simple-xss
Correctly allows for sanitizing text first then applying markdown
Optional list is included for current defaults... this can be removed but I would feel safer matching it in case something comes down the line we don't like.
Actually sanitizes and doesn't just escape to HTML Entities
Does filter out javascript: items just like simple-xss

npm homepage
gh homepage

Additional bug report(s):

https://openuserjs.org/discuss/The_new_layout#comment-146b4da7366

MISSING:

Rest of GH supported attributes on allowed tags... TRACKING UPSTREAM
Showing raw HTML support in md code blocks
Resolves html in markup code blocks

Refs:

2019 08 25 https://github.com/jch/html-pipeline/blob/494135057cf803a8c15071a01f77223f551ad071/lib/html/pipeline/sanitization_filter.rb (GH markup support)

Advantages: * **Works** unlike simple-xss * Lots of maintainers/contributors * Smaller than simple-xss * Correctly allows sanitizing text **first** then applying markdown * Optional list is included for current defaults... this **can be removed** but I would feel safer matching it in case something comes down the line we don't like. * Does filter out `javascript:` items just like xss [npm homepage](https://www.npmjs.org/package/sanitize-html) [gh homepage](https://github.com/punkave/sanitize-html) Related: * OpenUserJS#168 * OpenUserJS#125 Tested in dev on [this page](http://localhost:8080/scripts/marti/httplocalhost.localdomain/RFC_2606%C2%A73_-_license_and_licence_Unit_Test)

Martii · 2014-06-20T07:37:55Z

Everyone please let me know if you want any default tags/attributes removed or added... I personally have never heard of the nl tag but it's in their default list.

The goal here is to encourage markdown but still allow some markup since md does have some disadvantages.

Does have one issue with highlighting but so does simple-xss... working on this.

// *********************
// This code is fenced via gfm and highlighted?
// *********************

var hello = "world";

<html>
<head></head>
<body>
</body>
</html>

* Swap to markdown first then sanitize-html to allow html code blocks * Allow img tag ... this deviates from their defaults so we need to define our own.

Martii · 2014-06-20T08:40:23Z

hmmm don't think that last commit is going to generate the DOM correctly if and when highlighting gets fixed.

sizzlemctwizzle · 2014-06-20T08:41:02Z

please let me know if you want any default tags/attributes removed or added

For compatibility, use the settings GitHub uses.

Martii · 2014-06-20T08:43:23Z

@sizzlemctwizzle
Thanks... I think we need a markdown that has a callback sanitizing instead of just a boolean... and somethings wrong with highlighting even on pro now... also if I backout simple-xss altogether it still doesn't highlight.

sizzlemctwizzle · 2014-06-20T08:52:46Z

I think we need a markdown

We don't have anything else (but I could always fork it again). What was the problem with running sanitize before the markdown renderer?

Martii · 2014-06-20T08:56:15Z

We don't have anything else. What was the problem with running sanitize before the markdown renderer?

HTML as denoted in the commit message. This is why I never use innerHTML unless I have to in code. Somehow GH is doing it but they probably have it all integrated into one object.

…ilities even though it doesn't currently work

sizzlemctwizzle · 2014-06-20T09:00:31Z

HTML as denoted in the commit message.

What happens to the HTML in the code blocks? My working tree is dirty so I can't test right now.

Martii · 2014-06-20T09:09:04Z

What happens to the HTML in the code blocks?

Totally stripped in code fences and I haven't tried inside a JavaScript block yet... I suppose we could tell them to use pre and code tags and go with intended behavior.

Do you really want to support githubs schemes? e.g. github-windows:// and github-mac://

…) stuff. * Still need to see if sanitize-html has a wildcard

Martii · 2014-06-20T09:16:55Z

Still need to see if sanitize-html has a wildcard

See also:

Specifying allowedAttributes as a glob apostrophecms/sanitize-html#4

blehh this is going to be lengthy.

…code and read in

sizzlemctwizzle · 2014-06-20T09:56:47Z

Do you really want to support githubs schemes?

No.

Totally stripped in code fences

Which it should. This might help if added to markdown.js

function escape(html, encode) {
  return html
    .replace(!encode ? /&(?!#?\w+;)/g : /&/g, '&amp;')
    .replace(/</g, '&lt;')
    .replace(/>/g, '&gt;')
    .replace(/"/g, '&quot;')
    .replace(/'/g, '&#39;');
}

renderer.code = function (code, lang) {
  code = this.options.highlight(escape(code, true), lang);

  if (!lang) {
    return '<pre><code>' + code + '\n</code></pre>';
  }

  return '<pre><code class="'
    + this.options.langPrefix
    + escape(lang, true)
    + '">'
    + code
    + '\n</code></pre>\n';
};

Edit: Nvm the code blocks need be escaped before or during HTML sanitization.

Martii · 2014-06-20T10:25:12Z

EDIT:

Which it should. This might help if added to markdown.js

That makes JavaScript code fences look like this:

var hello = &quot;world&quot;;

instead of:

var hello = "world";

I'm running out of steam for the night but I do want to follow up with this when I get back from the weekend. This should give some time for sanitize-html maintainers to answer a question too. hopefully

Nvm the code blocks need be escaped before or during HTML sanitization.

This might be what is fubarring the highlighting btw. I noticed a > with inspector and I doubt our highlighter handles that. IDKY

* Prepping for when I get back

* Gotta run now

sizzlemctwizzle · 2014-06-20T20:38:06Z

Essentially we've ran into a catch-22. If we sanitize before we render, we strip html inside markdown code blocks (plus the sanitizer is meant for html, not markdown). But if we sanitize after, we lose the highlighted code because of the class attribute.

I noticed a > with inspector and I doubt our highlighter handles that.

It does actually (check the page source to see). We're supposed to escape it before we send it to the highlighter.

As far as I can tell, the only solution: markdown (w/ escape code blocks) -> sanitize -> highlight. This might mean that we need to give the markdown library a sanitizer option that is a callback to use an external sanitization function rather than just the "all or nothing" html escape that is used when sanitize: true.

Martii · 2014-06-20T21:54:23Z

option that is a callback to use an external sanitization function

Possibly two pass as well.

sizzlemctwizzle · 2014-06-21T00:14:05Z

I fixed it. I should have spent more time trying to understand our markdown renderer.

sizzlemctwizzle · 2014-06-21T01:36:38Z

Okay, so ended up having to individually sanitize the output of the various block level markdown renders (obviously omitting the code one) and now everything seems to work correctly.

Zren · 2014-06-21T10:41:05Z

tables + images are broken.

https://openuserjs.org/discuss/The_new_layout#comment-146bdc24a14

Martii · 2014-06-21T15:51:54Z

@sizzlemctwizzle
Btw I'm still waiting on an answer from up here from those guys so this pr is partially incomplete at the moment even though it's merged and deployed. If you are feeling adventorous you can add all the attributes in per tag name into the .json file but I thought I'd wait and see first rather than a ton of typing with a ton of copy/pasting... and having another pr/commit. :) Zren also made #192 which I adjusted and explained how to fix for his comment up here currently right before this comment (which is the same as #192).

sizzlemctwizzle · 2014-06-21T21:28:36Z

Btw I'm still waiting on an answer from up here

I've looked at their code and the answer is "no, there isn't". But I've identified the code that could be modified to add the wildcard, so I'll probably submit a PR sometime. Until then, I've added some code to whitelist a list of attributes for all whitelisted tags.

If you are feeling adventurous you can add all the attributes in per tag name into the .json file

No need to do this by hand.

Martii added bug and removed expedite labels Jun 20, 2014

Martii self-assigned this Jun 20, 2014

Flip order of processing and allow img tag

fafa04c

* Swap to markdown first then sanitize-html to allow html code blocks * Allow img tag ... this deviates from their defaults so we need to define our own.

Revert last commit... otherwise DOM is stripped of highlighting capab…

7097881

…ilities even though it doesn't currently work

Match some of [GH's](https://github.com/github/markup#html-sanitization…

c31e8db

…) stuff. * Still need to see if sanitize-html has a wildcard

Move html whitelist to a json file so it doesn't clutter up the main …

2b5e93d

…code and read in

Martii added the tracking upstream label Jun 20, 2014

White space adjustment and removal of console message.

6891f97

* Prepping for when I get back

Martii mentioned this pull request Jun 20, 2014

Validate shown and linkified @keys #191

Closed

Rename json to be more specific and change affected code

41c359f

* Gotta run now

Martii added the sooner label Jun 20, 2014

sizzlemctwizzle merged commit 41c359f into OpenUserJS:master Jun 21, 2014

sizzlemctwizzle added a commit that referenced this pull request Jun 21, 2014

Use sanitizer for the html renderer. Closes #190.

4960070

Martii removed the sooner label Jun 21, 2014

Martii mentioned this pull request Jun 27, 2014

User Content CSS Adjustment for tables #221

Closed

Martii removed their assignment Jul 3, 2014

Martii deleted the fixXSSandMD branch July 7, 2014 09:24

Martii removed the tracking upstream Waiting, watching, wanting. label Feb 7, 2015

This was referenced Aug 25, 2019

Images are sanitized #192

Closed

GDPR13 #1508

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap out simple-xss for sanitize-html #190

Swap out simple-xss for sanitize-html #190

Martii commented Jun 20, 2014 •

edited

Loading

Martii commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 21, 2014

sizzlemctwizzle commented Jun 21, 2014

Zren commented Jun 21, 2014

Martii commented Jun 21, 2014

sizzlemctwizzle commented Jun 21, 2014

Swap out simple-xss for sanitize-html #190

Swap out simple-xss for sanitize-html #190

Conversation

Martii commented Jun 20, 2014 • edited Loading

Martii commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 20, 2014

Martii commented Jun 20, 2014

sizzlemctwizzle commented Jun 21, 2014

sizzlemctwizzle commented Jun 21, 2014

Zren commented Jun 21, 2014

Martii commented Jun 21, 2014

sizzlemctwizzle commented Jun 21, 2014

Martii commented Jun 20, 2014 •

edited

Loading