use of custom icons causes a security warning

[j-ed]

I had opened nextcloud/apps#20 because no separate repository existed that time.

Affected apps

The external sites app

Expected behaviour

The external sites app shouldn't cause a security warning when custom icons have been copied to the ./apps/external/img directory and have been assigned to external site links. Additional icons in that directory should be excluded from the security check or the app should be extended to load icons from a different location which is not checked by the security function.

Actual behaviour

The external sites app causes a security warning when custom icons are copied to the ./apps/external/img directory and assigned to an external site link.

Steps to reproduce

1. Copy a new icon file to the directory ./apps/external/img
2. Open Administration -> Additional settings -> External sites
3. Assign a new icon to an external site link.
4. Install a new app etc. so that a complete security check is forced. (unfortunately I don't know if it's possible to force a check from the command line.
5. A security warning is shown because an unknown file was found in the directory:

Server configuration

Operating system: Linux 3.2.82
Web server: Apache2 2.4.23
Database: MariaDB 5.5.53
PHP version: 5.6.23
Nextcloud version: 10.0.2

Client configuration

Browser: Firefox 50.0.2
Operating system: Windows 7

Logs

No errors have been logged

[nickvergessen]

cc @LukasReschke any idea?

[LukasReschke]

The current behaviour is quite a hack and I'm uncertain why we've even accepted it into the code. I'd say we should allow upload of favicons via the GUI and store those in appdata and serve them then with aggressive caching headers.

[nickvergessen]

Exactly my thoughts. So let's keep it like this for now, until someone implements that.

[cubefoo]

how about in the meantime we add some common icons to the apps img folder before upload?
Just the most common categories like shop, support, question mark etc. should be fine for majority of users

[nickvergessen]

Yes, I think the updater remove all file of apps while updating them. So you indeed need to reupload them afterwards.

Thanks for the hint, I will make this higher priority then.

[nickvergessen]

We will use the new appdata directory which was designed for such use cases and add a upload-option to the admin ui.