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

MD Editor: More buttons and such #168

Open
Zren opened this issue Jun 14, 2014 · 14 comments
Open

MD Editor: More buttons and such #168

Zren opened this issue Jun 14, 2014 · 14 comments
Labels
enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. mightfix Ehh maybe... convince us all... question A question has been encountered by anyone and has remained unanswered until cleared.

Comments

@Martii
Copy link
Member

Martii commented Jun 14, 2014

Make sure we triple check the html markup sanitizing and md (gfm)... there's currently an issue with content in code fences being stripped nicely... haven't reported it yet because everyone can work around it with <pre><code> tags and I am not entirely sure when it appeared.

One exampled dev/production homepage for my display of the sample json.

See also:

@sizzlemctwizzle
Copy link
Member

there's currently an issue with content in code fences being stripped nicely...
One exampled homepage for my display of the sample json

Maybe I need to get my eyes checked, but what exactly is wrong with that json? What do you mean by "stripped nicely"?

@sizzlemctwizzle
Copy link
Member

sizzlemctwizzle commented Jun 14, 2014

@Zren

Have you seen @jerone's script where he adds a bunch of other buttons for GitHub. I'm guessing that it probably can't be easily adapted to Bootstrap Markdown, but it's a good example of what buttons it would be nice to have.

@Martii
Copy link
Member

Martii commented Jun 14, 2014

but what exactly is wrong with that json?

Should look like this:

{
 "Monkey Barrel": [
  "/scripts/show/114843",
  {
   "recent posts": "/posts?kind=all",
   "recent topics": "/topics",
   "recent comments": "/comments",
   "recent images": "/images",
   "": "",
   "recent spam": "/spam",
   "recent potential spam": "/posts?kind=all&spam=1",
   "recent potential spam by score": "/posts?kind=all&spam=score",
   "spam and malware \u00bb": "/topics/9#posts-last",
   "cookie stealing scripts \u00bb": "/topics/704#posts-last"
  }
 ],
 "Scripts": [
  "/scripts",
  {
   "recent posts": "/posts?kind=script",
   "recent reviews": "/reviews",
   "recent potential spam": "/posts?kind=script&spam=1",
   "recent potential spam by score": "/posts?kind=script&spam=score",
   "": "",
   "popular scripts": "/",
   "all scripts by name": "/scripts?sort=name",
   "most discussed": "/scripts?sort=posts",
   "highest rated": "/scripts?sort=rating",
   "most favorited": "/scripts?sort=fans",
   "most installed": "/scripts?sort=installs",
   " ": "",
   "new script by upload": "/scripts/new",
   "new script by pasting": "/scripts/new?form=true"
  }
 ],
 "Tags": [
  "/tags",
  {
   "most frequently used tags": "/tags?sort=count",
   "all tags by name": "/tags?page=1"
  }
 ],
 "Forums": [
  "/forums",
  {
   "recent posts": "/posts?kind=forum",
   "recent potential spam": "/posts?spam=1",
   "recent potential spam by score": "/posts?kind=forum&spam=score",
   "": "",
   "script development": "/forums/1",
   "ideas and script requests": "/forums/2",
   "userscripts.org discussion": "/forums/3",
   "the banana bar": "/forums/4",
   "greasefire": "/forums/5"
  }
 ],
 "People": [
  "/users",
  {
   "all members by name": "/users?page=1",
   "all members by # of scripts": "/users?sort=scripts",
   "all members by # of comments": "/users?sort=comments",
   "all members by # of posts": "/users?sort=posts"
  }
 ],
 "Blog": [
  "/articles",
  {
   "rss feed": "/feeds/recent_articles"
  }
 ],
 "Guides": [
  "/guides",
  {
   "highest rated": "/guides?sort=votes",
   "most discussed": "/guides?sort=comments",
   "sorted by author": "/guides?sort=author",
   "sorted by date": "/guides?sort=updated"
  }
 ]
}

but looks like this:

{
 "": ,
 "": ,
 "": ,
 "": ,
 "": ,
 "": ,
 "": }

@Zren
Copy link
Contributor Author

Zren commented Jun 14, 2014

Hmmm. It stores fine, it must be somewhere in the rendering.

https://openuserjs.org/admin/json?model=Script&id=537812c3fb78e9000055d860

Examples\r\n\r\n``` json\r\n{\r\n \"Monkey Barrel\": [\r\n  \"/scripts/show/114843\",\r\n  {\r\n   \"recent posts\": \"/posts?kind=all\",\r\n   \"recent topics\": \"/topics\",\r\n   \"recent comments\": \"/comments\",\r\n   \"recent images\": \"/images\",\r\n   \"\": \"\",\r\n   \"recent spam\": \"/spam\",\r\n   \"recent potential spam\": \"/posts?kind=all&spam=1\",\r\n   \"recent potential spam by score\": \"/posts?kind=all&spam=score\",\r\n   \"spam and malware \\u00bb\": \"/topics/9#posts-last\",\r\n   \"cookie stealing scripts \\u00bb\": \"/topics/704#posts-last\"\r\n  }\r\n ],\r\n \"Scripts\": [\r\n  \"/scripts\",\r\n  {\r\n   \"recent posts\": \"/posts?kind=script\",\r\n   \"recent reviews\": \"/reviews\",\r\n   \"recent potential spam\": \"/posts?kind=script&spam=1\",\r\n   \"recent potential spam by score\": \"/posts?kind=script&spam=score\",\r\n   \"\": \"\",\r\n   \"popular scripts\": \"/\",\r\n   \"all scripts by name\": \"/scripts?sort=name\",\r\n   \"most discussed\": \"/scripts?sort=posts\",\r\n   \"highest rated\": \"/scripts?sort=rating\",\r\n   \"most favorited\": \"/scripts?sort=fans\",\r\n   \"most installed\": \"/scripts?sort=installs\",\r\n   \" \": \"\",\r\n   \"new script by upload\": \"/scripts/new\",\r\n   \"new script by pasting\": \"/scripts/new?form=true\"\r\n  }\r\n ],\r\n \"Tags\": [\r\n  \"/tags\",\r\n  {\r\n   \"most frequently used tags\": \"/tags?sort=count\",\r\n   \"all tags by name\": \"/tags?page=1\"\r\n  }\r\n ],\r\n \"Forums\": [\r\n  \"/forums\",\r\n  {\r\n   \"recent posts\": \"/posts?kind=forum\",\r\n   \"recent potential spam\": \"/posts?spam=1\",\r\n   \"recent potential spam by score\": \"/posts?kind=forum&spam=score\",\r\n   \"\": \"\",\r\n   \"script development\": \"/forums/1\",\r\n   \"ideas and script requests\": \"/forums/2\",\r\n   \"userscripts.org discussion\": \"/forums/3\",\r\n   \"the banana bar\": \"/forums/4\",\r\n   \"greasefire\": \"/forums/5\"\r\n  }\r\n ],\r\n \"People\": [\r\n  \"/users\",\r\n  {\r\n   \"all members by name\": \"/users?page=1\",\r\n   \"all members by # of scripts\": \"/users?sort=scripts\",\r\n   \"all members by # of comments\": \"/users?sort=comments\",\r\n   \"all members by # of posts\": \"/users?sort=posts\"\r\n  }\r\n ],\r\n \"Blog\": [\r\n  \"/articles\",\r\n  {\r\n   \"rss feed\": \"/feeds/recent_articles\"\r\n  }\r\n ],\r\n \"Guides\": [\r\n  \"/guides\",\r\n  {\r\n   \"highest rated\": \"/guides?sort=votes\",\r\n   \"most discussed\": \"/guides?sort=comments\",\r\n   \"sorted by author\": \"/guides?sort=author\",\r\n   \"sorted by date\": \"/guides?sort=updated\"\r\n  }\r\n ]\r\n}\r\n```

Edit: Er, of course it stores fine. We only sanitize/render when it gets parsed. Not sure why I was thinking the MD Editor would strip it out.

@sizzlemctwizzle
Copy link
Member

Stripping is done by xss. Removing that call shows the json properly.

@sizzlemctwizzle
Copy link
Member

Okay, the problem is that highlight.js is generating class attributes to style the code and that is not a default whitelisted attribute, so the tags (and their contents) get removed.

@sizzlemctwizzle
Copy link
Member

Would not stripping the class attribute be horrible?

@Martii
Copy link
Member

Martii commented Jun 15, 2014

Would not stripping the class attribute be horrible?

You mean whitelisting the class attribute? It all depends on if javascript:void(0) and everything else script related in the value would be able to run for XSS attacks. I don't think I've ever tried that... most, if not all, of CSS rules are for styling only... so any risk would probably be in changing the style for that particular element in the user-content type areas... I'm not 100% sure because it's a use case I've never thought of. Might be a handy thing for allowing some common or user account defined classes down the line.

USO doesn't allow the class attribute yet their md sort of works... they also have code fence issues... which is a reason why I usually never use md there ... I would like to encourage use of github-flavored-markdown (gfm) though over markup... but as we've chatted about in the past some things md just can't do.


Have you tried doing your original commit but whitelist class ? to see if #125 (comment) goes away? Just a random thought and will probably not work... but worth a quick try.


This might be a bug in simple-xss not recognizing that content between properly terminated<pre> and <code> tags should be ignored too or even md aware. IDK... or reversing the logic of ignoring ` and ```. < --- see even GH has issues going one way ... supposed to be visualized as being described as the single backward apostrophe encased in code tags to denote code tag and three backward apostrophes for code fencing.

See also:

@Martii
Copy link
Member

Martii commented Jun 15, 2014

NOTE: More links added in previous post to examine


Going to do some tests here... so this particular reply may change some:

Define a table with GH gfm

Left Center Right
abcdefghijklmnopqrstuvwxyz a abcdefghijklmnopqrstuvwxyz

Test to determine if GH gfm has a class attribute to center the middle column..... NO it uses the align attribute... this is different than our gfm implementation I think... this result kills further testing for trying to apply the class value into another markup tag to see if GH allows class hijacking. :\

Btw if you didn't notice my Compatibility Matrix is also sanitized on OUJS now... most of my icons groupings are supposed to be centered with the class that is added via :----: and is stripped. (I used <center> on USO)


Swipe the GH generated output table source and repost as markup

Left Center Right
abcdefghijklmnopqrstuvwxyz a abcdefghijklmnopqrstuvwxyz

... result: ALLOWS align attribute

NOTE: align attribute is not HTML5 compliant and recommendations for using CSS are in quite a few places.


This p tag has classy as a class

.... result: SANITIZED


Applying the `align` attribute for `p` tag.. should be far `right`

... result: SUCCESS with a consequence of GH gfm FAILING

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jun 20, 2014
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
Copy link
Member

Martii commented Jan 22, 2015

So bringing this back to topic basically we're missing "pin" which I don't recommend keeping our comment reply box open constantly. It has full screen editing now which is pretty good.

The other thing I see @jerone 's script is horizontal rule e.g. ---... is this really a necessity? There's a vague reference to "additionalButtons" over on that project under the API header (sorry no direct linkage because no id or name)


What buttons does anyone want to see?


I would love to see a "Reply and Close" button on Issue discussions if possible... any thoughts?

@Martii Martii added the question A question has been encountered by anyone and has remained unanswered until cleared. label Aug 7, 2015
@Martii
Copy link
Member

Martii commented Feb 22, 2016

What buttons does anyone want to see?

Reiterating again... will close soon if there isn't any interest.

@Martii
Copy link
Member

Martii commented Oct 27, 2017

Investigated this. bootstrap-markdown only supports jQuery $('selector').markdown({ additionalButtons...

Due to migration away from jQuery in our core, esp. in the client-side scripts, this is now not programmatically an option. e.g. Does not currently work with node.querySelector or the like. Ref: toopay/bootstrap-markdown/blob/878f349/js/bootstrap-markdown.js#L61

We could however fork the project and hard code the needed buttons. We would need to maintain this with all upstream changes.

Currently strike is missing and GFM tables... both of which are specific to GFM. CommonMark may have something different available but it's still newer to the scene. It is possible to append those using jQuery and their API however it's quite messy to get the buttons in the right order.

But I'm going to close this very soon due to complete lack of interest shown. I really think there are plenty of buttons already.

@Martii Martii added mightfix Ehh maybe... convince us all... and removed later A long time ahead, in a galaxy near, near... labels Oct 27, 2017
@Martii
Copy link
Member

Martii commented Oct 31, 2017

So looked into Jerones script source... he mixes markup with markdown. e.g. <ins></ins> for underline. He also supports GH task lists, multiple header values, tab insertion, UserAgent insertion, Horizontal Rule, selection delete, emojis, notice for CONTRIBUTING.md., GFM tables, etc. Bad news is that script doesn't seem functional anymore (deprecation at jerone/UserScripts#106 (comment)) and screenshots outdated which is why the source needed to be examined.

The more I think of this the better it would be to do this .user.js client-side if more are wanted. Dabbling on a local copy of bootstrap-markdown this can be achieved directly, and in jQuery currently, but again order is the thing when just adding buttons the "normal way" (via that maintainers API).

I'm at a -1 for modifying bootstrap-markdown... way too messy.


Btw last chance to convince otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. mightfix Ehh maybe... convince us all... question A question has been encountered by anyone and has remained unanswered until cleared.
Development

No branches or pull requests

3 participants