Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Sep 30, 2016

@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
Copy link
Member

@LukasReschke LukasReschke left a comment

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
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
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
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
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
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
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
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
@LukasReschke LukasReschke force-pushed the markdown-support-for-app-descriptions branch from dfc1b39 to b25a3b9 Compare January 13, 2017 17:36
@LukasReschke
Copy link
Member

Added DOMPurify at b25a3b9 – ok for me now.

'li',
'em',
's',
'blockquote'
Copy link
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 <[email protected]>
Signed-off-by: Lukas Reschke <[email protected]>
@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 <[email protected]>
@nickvergessen
Copy link
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
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.

@MorrisJobke
Copy link
Member

I fixed it and it now looks like this:

bildschirmfoto 2017-01-16 um 22 18 31

@MorrisJobke
Copy link
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
Member Author

Thanks, so ready to merge!

Copy link
Member

@ChristophWurst ChristophWurst left a comment

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
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
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
Copy link
Member

karlitschek commented Jan 17, 2017

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.

8 participants