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

Allow Remote images #359

Closed
shgysk8zer0 opened this issue Oct 13, 2019 · 9 comments
Closed

Allow Remote images #359

shgysk8zer0 opened this issue Oct 13, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@shgysk8zer0
Copy link

Is your feature request related to a problem? Please describe.
There's an open issue (#54 ) for images shared by public link, but this relates to issues from sites like Imgur, where permissions are not a factor.

Describe the solution you'd like

  1. Show remote images when file is shared instead of "Show image" with placeholder
  2. Add Image by URL to toolbar and/or allow regular markdown image syntax

Describe alternatives you've considered
I've resorted to converting to PDF using pandoc when I need to share a file and have images shown.

Additional context
This is just my use case, but I use Nextcloud pretty extensively when deal with clients in web development. Sometimes it's screenshots for a project, and other times I want to provide tutorials showing how to use certain features.

@shgysk8zer0 shgysk8zer0 added the enhancement New feature or request label Oct 13, 2019
@juliushaertl
Copy link
Member

The main issue here is that embedding external images might be a security risk. I don't see any solution for this besides having an image proxy on a different domain. cc @rullzer

@rullzer
Copy link
Member

rullzer commented Oct 14, 2019

Yes. We should properly proxy them.

@shgysk8zer0
Copy link
Author

What would you think of an admin set whitelist? Or giving user option to show remote images (as often seen in email)?

I think that an image proxy on a different domain is prohibitive and has its own issues. I don't want to setup another subdomain just for this purpose or pay for a third-party service. And, should I rely on a third-party solution, what makes using that any more secure than just allowing images from an admin set list of domains?

@juliushaertl
Copy link
Member

@ChristophWurst Anything you do in that regard in the mail app?

@ChristophWurst
Copy link
Member

We proxy the image request through a special route to make sure the request token from the iframe URL is not leaked. Other than that, we don't do much: https://github.com/nextcloud/mail/blob/72cc7c527ca2f3025186cf10e88d9323d5024737/lib/Controller/ProxyController.php#L110-L130

@rullzer
Copy link
Member

rullzer commented Oct 21, 2019

Well you do. You set the mimetype etc.

In order to proxy images yiou should pipe them trough GD and get the image form there to make sure there is no malicious image injected and just shown to the user.

@gwojcieszczuk
Copy link

guys, supporting standard markdown image syntax is crucial. Not everybody is going to use your app to edit markdown. For example I'm using qownnotes, which has integration with nextcloud to share note. ANy other MD editor does this properly. Don't try to implement some custom ways for something that is already part of MD syntax.

@samtuke
Copy link

samtuke commented Dec 11, 2021

This is still an issue, and undermines Nextcloud for valuable use cases. Why not just give the admin the ability to disable this security feature? On installations where every user who has write access is trusted, automatically blocking all remote images has no benefit AFAICS. Blocking them by default prevents markdown documents from being readable, on the other hand (definite value reduction in exchange for uncertain value add (security)).

@juliushaertl
Copy link
Member

This is now implemented by keeping a copy of the remote image with #1900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants