Skip to content

Markdown support for app descriptions#1594

Merged
nickvergessen merged 12 commits into
masterfrom
markdown-support-for-app-descriptions
Jan 17, 2017
Merged

Markdown support for app descriptions#1594
nickvergessen merged 12 commits into
masterfrom
markdown-support-for-app-descriptions

Conversation

@nickvergessen

@nickvergessen nickvergessen commented Sep 30, 2016

Copy link
Copy Markdown
Member

@janis91 this should fix the markdown showing up as plaintext...

@icewind1991 your files_markdown app uses the same lib, you might be able to drop it, in case it causes problems otherwise.
@LukasReschke @MorrisJobke

Todo

  • Decide on allowed markdown
  • Fix displaying of lists

@nickvergessen nickvergessen added this to the Nextcloud 11.0 milestone Sep 30, 2016

@LukasReschke LukasReschke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look later with regards to XSS.

@nickvergessen

Copy link
Copy Markdown
Member Author

Well the question is, which markup do we want to allow?
I guess most devs would already be happy with:

Allowed

  • string
  • italic
  • lists: *, 1.

Unsafe

  • Images are not what the page is designed for and are not loaded due to CSP by default, unless shipped (but then you still have a problem with app paths which you don't know)
  • Links are mostlikely the highest risk. Of course they got their use cases, and dev's can already provide links to docs which can have malicious content

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 30, 2016
@BernhardPosselt

Copy link
Copy Markdown
Member

Last time I've tried marked it was vulnerable to XSS. I've ended up with https://github.com/markdown-it/markdown-it#markdown-it which is "Safe by default"

@BernhardPosselt

Copy link
Copy Markdown
Member

I'd render links because most of the time you want to link to documentation or FAQ to get people started.

As for images: I'd probably relax the CSP if feasable but require them to be served over HTTPS

@nickvergessen

Copy link
Copy Markdown
Member Author

Closing, feel free to reopen and pick up, once decisions have been made

@nickvergessen nickvergessen deleted the markdown-support-for-app-descriptions branch November 2, 2016 13:12
@nickvergessen nickvergessen restored the markdown-support-for-app-descriptions branch December 15, 2016 15:36
@nickvergessen nickvergessen reopened this Dec 15, 2016
@nickvergessen nickvergessen force-pushed the markdown-support-for-app-descriptions branch from 5ef6698 to f01af3d Compare December 15, 2016 16:07
@nickvergessen

Copy link
Copy Markdown
Member Author

Rebased and pushed a commit which:

  • Disables rendering of quotes
  • Disables images
  • Disables links unless they start with http: or https:

I'd like to do this and also add it to 11, because the app store says markdown is supported and even shows it, but in the app management everything looks broken.

Please review @LukasReschke @BernhardPosselt @MorrisJobke

@BernhardPosselt please also adjust the app store to not render quotes, images and non-http links, so the feeling is the same everywhere

@BernhardPosselt

Copy link
Copy Markdown
Member

Any reason for not rendering links, quotes and images? These things are probably the most important markdown features. What MD lib are you using?

@nickvergessen

Copy link
Copy Markdown
Member Author

Any reason for not rendering links, quotes and images?

We render links when they are http or https, but javascript, ftp, whatever are just ignored.
Quotes cause layout issues on the small page and images are violating the CSP

What MD lib are you using?

https://github.com/nextcloud/server/pull/1594/files#diff-0a08a7565aba4405282251491979bb6b

nickvergessen and others added 5 commits January 13, 2017 18:33
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@LukasReschke LukasReschke force-pushed the markdown-support-for-app-descriptions branch from dfc1b39 to b25a3b9 Compare January 13, 2017 17:36
@LukasReschke

Copy link
Copy Markdown
Member

Added DOMPurify at b25a3b9 – ok for me now.

Comment thread settings/js/apps.js
'li',
'em',
's',
'blockquote'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen Whitelist here what you want to have whitelisted 😉

@LukasReschke LukasReschke force-pushed the markdown-support-for-app-descriptions branch from 4016cb9 to 110aacc Compare January 13, 2017 17:51
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke LukasReschke force-pushed the markdown-support-for-app-descriptions branch from 110aacc to ddfc7e6 Compare January 13, 2017 17:58
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen

Copy link
Copy Markdown
Member Author

@jancborchardt can you fix lists please? they have too much space and the order items seem to be stripped away. You can tests it by adding the following as an app description:

**Bold** and *Italic*

follow the [link](https://localhost)

List:
* one
* two
    + a
    + b
    + c
* thre

List2:
1. one
2. two
    1. a
    2. a
    3. a
3. thrww

@MorrisJobke MorrisJobke self-assigned this Jan 16, 2017
@MorrisJobke

Copy link
Copy Markdown
Member

@jancborchardt can you fix lists please? they have too much space and the order items seem to be stripped away. You can tests it by adding the following as an app description:

Let me try to fix this.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke

Copy link
Copy Markdown
Member

I fixed it and it now looks like this:

bildschirmfoto 2017-01-16 um 22 18 31

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke

Copy link
Copy Markdown
Member

I just added some top and bottom margin to the paragraphs:

Before:
bildschirmfoto 2017-01-16 um 22 25 07

After:
bildschirmfoto 2017-01-16 um 22 24 59

@nickvergessen

Copy link
Copy Markdown
Member Author

Thanks, so ready to merge!

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested and works

@ChristophWurst

Copy link
Copy Markdown
Member

Thanks, so ready to merge!

Go go go go! 🏎

@nickvergessen nickvergessen merged commit aea1b72 into master Jan 17, 2017
@nickvergessen nickvergessen deleted the markdown-support-for-app-descriptions branch January 17, 2017 10:11
@nickvergessen

Copy link
Copy Markdown
Member Author

I'd still like to backport this to 11, so the new fancy appstore descriptions don't appear broken in Nextcloud.

Opinions @LukasReschke @karlitschek

@karlitschek

karlitschek commented Jan 17, 2017

Copy link
Copy Markdown
Member

nice. please backport. low risk I assume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants