search

nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.3k stars 4.06k forks source link

[Bug]: The preview of the personal wallpapers of the dashboard is not displayed #31224

Closed Jerome-Herbinet closed 1 year ago

Jerome-Herbinet commented 2 years ago

⚠️ This issue respects the following points: ⚠️

Bug description

When I want to choose a background other than those proposed by default in my dashboard, I click on the button allowing me to choose among the images available in my files. When the popup that allows me to browse and select the file is displayed, I notice that no thumbnails are displayed (despite the fact that the files and folders are correctly listed and accessible).

Important : The problem occurs only when the name of the folder containing the wallpapers includes an apostrophe. Check additional information that have been added below recently (with screenshots ...).

Steps to reproduce

  1. Click on the "Customize" button
  2. Pick on the "Pick from files" button
  3. The popup appears ; folders and files appear but no image thumbnails appear at all

Expected behavior

Each image should have a thumbnail.

Installation method

Web installer

Operating system

Ubuntu 20.04

PHP engine version

8.0.15

Web server

Apache

Database engine version

MariaDB

Is this bug present after an update or on a fresh install?

After minor update

Are you using the Nextcloud Server Encryption module?

No

What user-backends are you using?

Configuration report

{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "nextcloud.mydomain.fr"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "mysql",
        "version": "22.2.5.1",
        "overwrite.cli.url": "https:\/\/nextcloud.mydomain.fr",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "default_language": "fr",
        "default_locale": "fr_FR",
        "default_phone_region": "ISO3166-2",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "smtp",
        "mail_sendmailmode": "smtp",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpauthtype": "LOGIN",
        "mail_smtpauth": 1,
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpport": "25",
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "memcache.local": "\\OC\\Memcache\\Redis",
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "port": 6379
        },
        "maintenance": false,
        "theme": "",
        "loglevel": 2,
        "updater.release.channel": "stable",
        "updater.secret": "***REMOVED SENSITIVE VALUE***"
    }
}

List of activated Apps

Enabled:
  - accessibility: 1.8.0
  - activity: 2.15.0
  - announcementcenter: 6.1.1
  - bruteforcesettings: 2.3.0
  - calendar: 3.0.6
  - camerarawpreviews: 0.7.15
  - circles: 22.2.0
  - cloud_federation_api: 1.5.0
  - comments: 1.12.0
  - contacts: 4.0.7
  - contactsinteraction: 1.3.0
  - dashboard: 7.2.0
  - dav: 1.20.0
  - extract: 1.3.3
  - federatedfilesharing: 1.12.0
  - federation: 1.12.0
  - files: 1.17.0
  - files_automatedtagging: 1.12.0
  - files_external: 1.13.1
  - files_pdfviewer: 2.3.1
  - files_rightclick: 1.1.0
  - files_sharing: 1.14.0
  - files_trashbin: 1.12.0
  - files_versions: 1.15.0
  - files_videoplayer: 1.11.0
  - files_zip: 1.0.1
  - firstrunwizard: 2.11.0
  - groupfolders: 10.0.2
  - guests: 2.1.0
  - impersonate: 1.9.0
  - logreader: 2.7.0
  - lookup_server_connector: 1.10.0
  - metadata: 0.15.0
  - nextcloud_announcements: 1.11.0
  - notes: 4.3.0
  - notifications: 2.10.1
  - oauth2: 1.10.0
  - password_policy: 1.12.0
  - passwords: 2021.12.20
  - phonetrack: 0.7.0
  - photos: 1.4.0
  - privacy: 1.6.0
  - provisioning_api: 1.12.0
  - quota_warning: 1.13.0
  - recommendations: 1.1.0
  - richdocuments: 4.2.4
  - richdocumentscode: 21.11.103
  - serverinfo: 1.12.0
  - settings: 1.4.0
  - sharebymail: 1.12.0
  - spreed: 12.2.3
  - support: 1.5.0
  - survey_client: 1.10.0
  - systemtags: 1.12.0
  - text: 3.3.0
  - theming: 1.13.0
  - twofactor_backupcodes: 1.11.0
  - twofactor_totp: 6.2.0
  - updatenotification: 1.12.0
  - user_status: 1.2.0
  - viewer: 1.6.0
  - weather_status: 1.2.0
  - workflowengine: 2.4.0
Disabled:
  - admin_audit
  - duplicatefinder: 0.0.13
  - encryption
  - user_ldap

Nextcloud Signing status

No errors have been found.

Nextcloud Logs

Too long - ask me if necessary

Additional info

I think I have always seen this bug as long as the dashboard has existed (since NextCloud 20).

Jerome-Herbinet commented 2 years ago

2022-02-16_22-33 2022-02-16_22-32

Jerome-Herbinet commented 2 years ago

I have found the (simple) technical reason why it doesn't work, and I think it can be fixed easily and quickly. The image preview thumbnails are called up using a "background-image" css property. The problem is as follows. Between the brackets, double quotes " are missing.

So we have : background-image: url(/index.php/core/preview.png?file=%2FFonds%20d'%C3%A9cran%2FDashBoard%2F1560151.jpg&x=100&y=100);

Instead of : background-image: url("/index.php/core/preview.png?file=%2FFonds%20d'%C3%A9cran%2FDashBoard%2F1560151.jpg&x=100&y=100");

This displays an error when inspecting the property with the browser's development tools (invalid property).

2022-05-24_13-43

Cc : @ChristophWurst

ChristophWurst commented 2 years ago

Could be the code at https://github.com/nextcloud/server/blob/45a75c631e638ccab8934584aedad834229d10a7/core/src/OC/dialogs.js#L832-L840

Jerome-Herbinet commented 2 years ago

I can't find the /src folder inside /core folder. Can you give me a little more information to find it ?

ChristophWurst commented 2 years ago

Is that a production system? The front-end sources are not shipped for components that go through compilation.

I'm afraid you will need a dev env to change the code, rebuild the front-end and give this a local test.

Jerome-Herbinet commented 2 years ago

My last tests lead me to the following conclusion: The problem occurs only when the name of the folder containing the wallpapers includes an apostrophe; that's why you have to add double quotes in the css property (as it is done elsewhere in Nextcloud Files).

However, I don't have the skills to build a development environment.

Knowing that there is (after my research) a good track for the resolution of the problem, could you ping a contributor-developer so that it can take charge of the resolution of the problem (pull request) ... and add the adequate labels to this issue?

Thank you very much for your time on this issue.

Best regards.

szaimen commented 1 year ago

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

Jerome-Herbinet commented 1 year ago

@szaimen I've tested it on a 25.0.3 test instance a few minutes ago and the problem remains exactly the same : the wallpapers thumbnails in the file explorer popup are not appearing a parent folder includes an apostrophe in its name.

niloc132 commented 1 year ago

Also tested 25.0.3 and 25.0.4, same symptoms, though I'm seeing it on the "Insert an attachment" screen of nextcloud/collectives. The "style" attribute of the td.filename is at fault here, with this value:

style="background-image:url(/core/preview.png?file=%2FPhotos%2FColin's%20Phone%2F19-10-18%2016-54-05%200013.jpg&x=100&y=100)"

The fact that there is a ' in the string breaks css parsing, since url() can contain a quoted string, or an unquoted string, but not a string with quotes in it. Possible legal values (which don't show an "Invalid property value" warning in firefox's dev tools) include replacing the ' character with its url-encoded value %27 (as the other slashes and spaces seem to be escaped:

style="background-image:url(/core/preview.png?file=%2FPhotos%2FColin%27s%20Phone%2F19-10-18%2016-54-05%200013.jpg&x=100&y=100)"

Wrapping the contents of url() in double quotes ", html-encoded as ":

style="background-image:url("/core/preview.png?file=%2FPhotos%2FColin's%20Phone%2F19-10-18%2016-54-05%200013.jpg&x=100&y=100")"

Or both:

style="background-image:url("/core/preview.png?file=%2FPhotos%2FColin%27s%20Phone%2F19-10-18%2016-54-05%200013.jpg&x=100&y=100")"

"Both" is probably the best answer, but in the spirit of cleaning up any code you touch, I should point out further that the & chars should also be correctly html encoded to be in an html attribute. That is, replaced by &.

Thus, my suggested steps here are

  1. First, URL-encode the "file" query string param. This is likely already happening, but perhaps could be more aggressive and encode the ' too. Note that it would be incorrect to suggest that the ' must be encoded, RFC 2396 section 2.3 calls out this character as "unreserved" and not needing encoding. However, if we don't encode, then care must be taken in step 3.
  2. Build the full URL, /core/preview.png with its three args, file, x, and y
  3. Wrap the URL in quotes, as an argument to the url() function. Use double quotes (") here to ensure that step one is optional - while single quotes are legal, that would require that single quotes within the URL are escaped.
  4. Build the rest of the style attribute, by prepending background-image: to the url function and its contents
  5. To assign this string to the style attribute of an HTML element, HTML-encode the entire attribute. This means replacing various characters with their &-encoded variants - first replace & with &amp;, then other characters such as <, >, ", ' to avoid escaping issues with the HTML attribute itself.

From a glance at https://github.com/nextcloud/server/blob/master/core/templates/filepicker.html#L48, it appears to be the source that this is generated from, probably a client-side template tool. It might be that adding quotes around the URL is enough to let this work, and that I'm being overcautious above.

niloc132 commented 1 year ago

After a little more looking, the template file above is indeed referenced, but for the icon itself, it is ignored in favor of this: https://github.com/nextcloud/server/blob/7c477d4028c7c80e9ff48156fa95bb76f84aec71/core/src/OC/dialogs.js#L1265-L1280

The attribute is set in such a way that it should indeed be safe, suggesting that we just need a quote wrapper around the url() function's argument. I'll see if I can work out how to build and test the repository, and if it works, I'll make a pull request changing this.

Jerome-Herbinet commented 1 year ago

For your information, I've tested it again this morning and this issue still occurring in NC27. This is a particular problem for the French-speaking world because, when English-speaking users may create a "Wallpaper" folder, the French users may often create "Fonds d'écran", which includes an apostrophe and which makes thumbnails invisible (due to apostrophe).

skjnldsv commented 1 year ago

Fixed on 28

Jerome-Herbinet commented 1 year ago

Fixed on 28

Awesome ! Could this fix be backported to NC 27 ?

skjnldsv commented 1 year ago

Done with the new picker for the upcoming 27.1 too yes :)

Jerome-Herbinet commented 1 year ago

Done with the new picker for the upcoming 27.1 too yes :)

Great !!