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

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

Closed
4 of 8 tasks
Jerome-Herbinet opened this issue Feb 16, 2022 · 15 comments
Closed
4 of 8 tasks

Comments

@Jerome-Herbinet
Copy link
Member

Jerome-Herbinet commented Feb 16, 2022

⚠️ This issue respects the following points: ⚠️

  • This is a bug, not a question or a configuration/webserver/proxy issue.
  • This issue is not already reported on Github (I've searched it).
  • Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
  • I agree to follow Nextcloud's Code of Conduct.

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?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

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 Jerome-Herbinet added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Feb 16, 2022
@Jerome-Herbinet
Copy link
Member Author

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

@Jerome-Herbinet
Copy link
Member Author

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

Could be the code at

$originalDiv.find('.icon').css({ 'background-image': "url('" + previewpath + "')" })
getCroppedPreview(replacement).then(
function(path) {
$replacementDiv.find('.icon').css('background-image', 'url(' + path + ')')
}, function() {
path = OC.MimeType.getIconUrl(replacement.type)
$replacementDiv.find('.icon').css('background-image', 'url(' + path + ')')
}
)

@Jerome-Herbinet
Copy link
Member Author

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

@ChristophWurst
Copy link
Member

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

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

szaimen commented Jan 23, 2023

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

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

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

After a little more looking, the template file above is indeed referenced, but for the icon itself, it is ignored in favor of this:

server/core/src/OC/dialogs.js

Lines 1265 to 1280 in 7c477d4

if (entry.type === 'file') {
var urlSpec = {
file: dir + '/' + entry.name,
x: 100,
y: 100
}
var img = new Image()
var previewUrl = OC.generateUrl('/core/preview.png?') + $.param(urlSpec)
img.onload = function() {
if (img.width > 5) {
$row.find('td.filename').attr('style', 'background-image:url(' + previewUrl + ')')
}
}
img.src = previewUrl
}
self.$filelist.append($row)

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.

niloc132 added a commit to niloc132/nextcloud-server that referenced this issue Mar 2, 2023
This ensures that the browser will expect that single-quotes in the url
string are not the end of the string, but part of the url.

Fixes nextcloud#31224
Signed-off-by: Colin Alworth <colin@colinalworth.com>
@Jerome-Herbinet
Copy link
Member Author

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

skjnldsv commented Sep 2, 2023

Fixed on 28

@skjnldsv skjnldsv closed this as completed Sep 2, 2023
@Jerome-Herbinet
Copy link
Member Author

Fixed on 28

Awesome !
Could this fix be backported to NC 27 ?

@skjnldsv
Copy link
Member

skjnldsv commented Sep 2, 2023

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

@skjnldsv skjnldsv removed the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Sep 2, 2023
@Jerome-Herbinet
Copy link
Member Author

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

Great !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants