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

Custom classes for extra styling #120

Open
michaelmyc opened this issue Apr 19, 2020 · 18 comments
Open

Custom classes for extra styling #120

michaelmyc opened this issue Apr 19, 2020 · 18 comments

Comments

@michaelmyc
Copy link

michaelmyc commented Apr 19, 2020

I was frustrated that there was no extra styling options available. The project was not actively maintained, and the dependencies are out of date and has security vulnerabilities. So I decided to fork the project and overhaul everything.

My package supports adding custom classes, ids, and attributes, so you would be able to more fancy styling with CSS. For example, you may want some images to be rendered small and some large, some centered and some aligned to the right. My new package would allow you to do so with ease.

Here's a demo: https://github.michaelmao.me/vue-markdown-plus. You can find it on NPM here: https://www.npmjs.com/package/vue-markdown-plus.

@pcmacdon
Copy link

For others considering using -plus, I gave it a try noted a number of issues.

  • Site has no list of feature enhancements/differences.
  • The styling feature could be implemented using postrender.
  • The dist contains only minified source, which is a security issue in itself.

@michaelmyc
Copy link
Author

For others considering using -plus, I gave it a try noted a number of issues.

  • Site has no list of feature enhancements/differences.
  • The styling feature could be implemented using postrender.
  • The dist contains only minified source, which is a security issue in itself.

Good suggestions.

  • It's a quick feature add I mostly intend to use myself. I forgot to thoroughly update the README. I'll do that soon.
  • Can you elaborate more about your comment on postrender? I think even if you try to change styling through javascript, you would still need to have to specify class/id for a particular segment of the content, which isn't offered here. That is the feature added in the -plus repo.
  • This repo does not offer prettified code either, and since I basically forked it, I didn't make changes to the build pipeline. You can check the commit history to see the differences (fairly minor). Or you can also prettify it to see what's in there. It make sense that a non-prettified one will make debugging a lot easier, but I'm not sure I understand why it is a security issue. Could you also elaborate on why this is a security issue?

@pcmacdon
Copy link

Looking only at your first example, a pattern such as [work]{.class} would be fairly simple to use with "replace" in postrender, although admittedly more difficult with code blocks.

I just looked at the vue-markdown repo and dist/* is not minified, unlike -plus.

wc vue-markdown-master/dist/vue-markdown-plus.js 
  22447  104344 1017818 vue-markdown-master-plus/dist/vue-markdown.js
wc vue-markdown-plus-master/dist/vue-markdown-plus.js 
    11   2918 561903 vue-markdown-plus-master/dist/vue-markdown-plus.js

The security issue arises due to the inability to run "diff" to see what lines you had changed from the original.

I ran into a real life example yesterday going to http://particlephysics.com/ which Ublock origin broke due to including:

http://img.sedoparking.com/js/jquery-1.11.3.custom.min.js

Turns out sedoparking is a known malware site, and worse nefarious scammers often claim to be providing "security improved versions".

Finally, the prettify version is not really human readable.

eg. it was fairly easily able to add the missing "tocAnchorLinkBefore" option to vue-markdown.

53,256d252
<           tocAnchorLinkBefore: {
<             type: Boolean,
<             default: true
<           },
331d326
<               anchorLinkBefore: this.tocAnchorLinkBefore,

Try that with minified js.

Yes, I could have got node and rebuilt the package, but that doesn't help when I was trying to debug the above problem.

@pcmacdon
Copy link

BTW, on the issue of security: your demo page https://github.michaelmao.me/vue-markdown-plus/

I get GET https://static.cloudflareinsights.com/beacon.min.js net::ERR_BLOCKED_BY_CLIENT

The first result from a google search "beacon.min.js" is

Beacon.min.js as Malware - General - Cloudflare Community
https://community.cloudflare.com/t/beacon-min-js-as-malware/117552

Ok, maybe it's not actually malware, but minifying makes it difficult to determine tampering one way or another.

@michaelmyc
Copy link
Author

BTW, on the issue of security: your demo page https://github.michaelmao.me/vue-markdown-plus/

I get GET https://static.cloudflareinsights.com/beacon.min.js net::ERR_BLOCKED_BY_CLIENT

The first result from a google search "beacon.min.js" is

Beacon.min.js as Malware - General - Cloudflare Community
https://community.cloudflare.com/t/beacon-min-js-as-malware/117552

Ok, maybe it's not actually malware, but minifying makes it difficult to determine tampering one way or another.

That is most likely because I'm using cloudflare. I'm not getting that on my chrome browser. Which browser are you using, and which version is your browser? I'll turn off the feature anyways since it has no real use to me.

@pcmacdon
Copy link

I use chrome as well, but it is Ublock Origin blocking it, not the browser.

@michaelmyc
Copy link
Author

Looking only at your first example, a pattern such as [work]{.class} would be fairly simple to use with "replace" in postrender, although admittedly more difficult with code blocks.

I just looked at the vue-markdown repo and dist/* is not minified, unlike -plus.

wc vue-markdown-master/dist/vue-markdown-plus.js 
  22447  104344 1017818 vue-markdown-master-plus/dist/vue-markdown.js
wc vue-markdown-plus-master/dist/vue-markdown-plus.js 
    11   2918 561903 vue-markdown-plus-master/dist/vue-markdown-plus.js

The security issue arises due to the inability to run "diff" to see what lines you had changed from the original.

I ran into a real life example yesterday going to http://particlephysics.com/ which Ublock origin broke due to including:

http://img.sedoparking.com/js/jquery-1.11.3.custom.min.js

Turns out sedoparking is a known malware site, and worse nefarious scammers often claim to be providing "security improved versions".

Finally, the prettify version is not really human readable.

eg. it was fairly easily able to add the missing "tocAnchorLinkBefore" option to vue-markdown.

53,256d252
<           tocAnchorLinkBefore: {
<             type: Boolean,
<             default: true
<           },
331d326
<               anchorLinkBefore: this.tocAnchorLinkBefore,

Try that with minified js.

Yes, I could have got node and rebuilt the package, but that doesn't help when I was trying to debug the above problem.

I'll take a look. I'll generate both minified and non-minified versions.

@michaelmyc
Copy link
Author

I use chrome as well, but it is Ublock Origin blocking it, not the browser.

Funny. I also have uBlock. It's not happening to me. I turned off that feature and it shouldn't be an issue anymore.

@michaelmyc
Copy link
Author

Actually I found out that markdown-it-katex has XSS vulnerabilities and it's not been updated for 4 years. Will look into some drop-in replacements.

@michaelmyc
Copy link
Author

I realized there are some more serious issues with vue-markdown-it, as it doesn't protect against xss. It doesn't look trivial to solve it and I don't have the time to fix this at the moment.

@pcmacdon
Copy link

Do you mean there are xss problems outside of "html" and "katex"?
I know only the basics of xss, but it seems to me the enabling the "html" feature is problematic.
And I would think you should also be able to disable "katex", especially if it has potential xss issues.

@pcmacdon
Copy link

Re: Katex. Looks to me like the xss exploit is only if you enable "raw html" in katex (disabled by default)
ie. this is ignored when used in vue-markdown:

$\href{javascript:alert('hello');}{test}$

@michaelmyc
Copy link
Author

michaelmyc commented Aug 13, 2020

$<img src="" onerror="alert('hi')" />%$ will run. It's a known issue for markdown-it-katex. I tried sanitizing the output html before mounting, but that didn't help prevent script execution. I tried to use markdown-it-katexx, which claims to fix xss, and their demo page actually did prevent scripts from executing. That didn't help even with katex rendering. Needs more time to triage the issue, but I'm pretty busy atm. I'd be happy to review your code if you open a PR.

@pcmacdon
Copy link

There are two different issues here.

The first and most important one is that vue-markdown use of Katex should (like emoji) be optional: see patch below.

The other (katex not doing the right thing) is more open ended. And not really relevant to the 99.99% of markdown users who do not need math rendering.

And sure, blog sites could still allow conditional math use via a moderator.

--- vue-markdown.js.orig	2020-07-14 15:28:15.698495629 -0700
+++ vue-markdown.js	2020-08-13 15:28:36.352299159 -0700
@@ -250,6 +250,10 @@
 	      type: Boolean,
 	      default: true
 	    },
+	    tocAnchorLinkBefore: {
+	      type: Boolean,
+	      default: true
+	    },
 	    tocAnchorLinkClass: {
 	      type: String,
 	      default: 'toc-anchor-link'
@@ -271,6 +275,10 @@
 	      default: function _default(htmlData) {
 	        return htmlData;
 	      }
+	    },
+	    katex: {
+	      type: Boolean,
+	      default: false
 	    }
 	  },
 
@@ -283,8 +291,10 @@
 	  render: function render(createElement) {
 	    var _this = this;
 
-	    this.md = new _markdownIt2.default().use(_markdownItSub2.default).use(_markdownItSup2.default).use(_markdownItFootnote2.default).use(_markdownItDeflist2.default).use(_markdownItAbbr2.default).use(_markdownItIns2.default).use(_markdownItMark2.default).use(_markdownItKatex2.default, { "throwOnError": false, "errorColor": " #cc0000" }).use(_markdownItTaskLists2.default, { enabled: this.taskLists });
-
+	    this.md = new _markdownIt2.default().use(_markdownItSub2.default).use(_markdownItSup2.default).use(_markdownItFootnote2.default).use(_markdownItDeflist2.default).use(_markdownItAbbr2.default).use(_markdownItIns2.default).use(_markdownItMark2.default).use(_markdownItTaskLists2.default, { enabled: this.taskLists });
+        if (this.katex) {
+            this.md.use(_markdownItKatex2.default, { "throwOnError": false, "errorColor": " #cc0000" });
+        }
 	    if (this.emoji) {
 	      this.md.use(_markdownItEmoji2.default);
 	    }
@@ -324,6 +334,7 @@
 	        tocLastLevel: this.tocLastLevelComputed,
 	        anchorLink: this.tocAnchorLink,
 	        anchorLinkSymbol: this.tocAnchorLinkSymbol,
+	        anchorLinkBefore: this.tocAnchorLinkBefore,
 	        anchorLinkSpace: this.tocAnchorLinkSpace,
 	        anchorClassName: this.tocAnchorClass,
 	        anchorLinkSymbolClassName: this.tocAnchorLinkClass,
@@ -10649,6 +10660,7 @@
 	    anchorLinkBefore: true,
 	    anchorClassName: "markdownIt-Anchor",
 	    resetIds: true,
+	    katex:false,
 	    anchorLinkSpace: true,
 	    anchorLinkSymbolClassName: null
 	  }, options);

@pcmacdon
Copy link

pcmacdon commented Aug 14, 2020

To answer your previous question, here is how we might use "postrender" to provide block spans with attributes. eg.

   here is some
   ::: red
   important stuff
   :::
   for you!

postrender looks something like:

        postrender(val) {
            return val.replace(/\n:::(.*?)\n:::/gms, function (mat, pat) {
                let attrs, opts = pat.match(/^\s*([^\n]+)\n(.*)$/);
                if (opts && opts.length>=3) {
                    attrs = opts[1].trim();
                    pat = opts[2];
                    let sattrs = '';
                    if (attrs[0] == '{' && attrs[attrs.length-1]==='}') { // TODO: parse {attrs} options...
                    } else {
                        sattrs = '  class="'+attrs+'"'; // Insert as class.
                    }
                    pat = '<span'+sattrs+'>'+pat+'</span>';
                }
                return '\n'+pat+'\n';
            });
        },

FYI, I grabbed webpack, rebuilt vue-markdown adding spans/attributes: the result was buggy.

@michaelmyc
Copy link
Author

I investigated a little more. The XSS issue seems to only apply to the simple html example but not the vue and webpack examples (both uses webpack). I'm not sure why that happens, but it seems like webpack is doing something to combat XSS. I'm waiting for my PR on markdown-it-katex to get accepted and I'll add a warning about this.

@michaelmyc
Copy link
Author

Postrender looks complicated. I feel the whole point of markdown is to make things simple, and I feel having custom clas/id/attributes and slight modifications in CSS makes things very simple. Postrender can be used for fancier stuff.

What do you mean by adding spans/attributes is buggy? I'm using markdown-it-bracketed-spans, and it's not giving me any trouble.

@pcmacdon
Copy link

My bad. After updating nodejs and webpack to all newest versions the problems with attrs went way. And yes, postrender is non-trivial.

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

No branches or pull requests

2 participants