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

fix(apps-store): Fix exception on generating preview url for installed app screenshot #48912

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

claucambra
Copy link
Contributor

@claucambra claucambra commented Oct 27, 2024

Summary

Some installed apps meant for older server versions might unexpectedly offer up screenshot values in a non-string format (e.g. health). Avoid an exception by checking first if the first app screenshot is indeed a string and only then trying to create the $proxyPreviewUrl

Example error:

{"reqId":"HMJg7Rkp0hOomzjCuXPs","level":3,"time":"2024-10-27T11:40:53+00:00","remoteAddr":"192.168.21.3","user":"admin","app":"index","method":"GET","url":"/index.php/settings/apps/list","message":"OCA\\Settings\\Controller\\AppSettingsController::createProxyPreviewUrl(): Argument #1 ($url) must be of type string, array given, called in /var/www/html/apps/settings/lib/Controller/AppSettingsController.php on line 250 in file '/var/www/html/apps/settings/lib/Controller/AppSettingsController.php' line 236","userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.0 Safari/605.1.15","version":"31.0.0.3","exception":{"Exception":"Exception","Message":"OCA\\Settings\\Controller\\AppSettingsController::createProxyPreviewUrl(): Argument #1 ($url) must be of type string, array given, called in /var/www/html/apps/settings/lib/Controller/AppSettingsController.php on line 250 in file '/var/www/html/apps/settings/lib/Controller/AppSettingsController.php' line 236","Code":0,"Trace":[{"file":"/var/www/html/lib/private/AppFramework/App.php","line":161,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Settings\\Controller\\AppSettingsController"},"listApps"]},{"file":"/var/www/html/lib/private/Route/Router.php","line":306,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Settings\\Controller\\AppSettingsController","listApps",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"_route":"settings.appsettings.listapps"}]},{"file":"/var/www/html/lib/base.php","line":1010,"function":"match","class":"OC\\Route\\Router","type":"->","args":["/settings/apps/list"]},{"file":"/var/www/html/index.php","line":24,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"/var/www/html/lib/private/AppFramework/Http/Dispatcher.php","Line":146,"Previous":{"Exception":"TypeError","Message":"OCA\\Settings\\Controller\\AppSettingsController::createProxyPreviewUrl(): Argument #1 ($url) must be of type string, array given, called in /var/www/html/apps/settings/lib/Controller/AppSettingsController.php on line 250","Code":0,"Trace":[{"file":"/var/www/html/apps/settings/lib/Controller/AppSettingsController.php","line":250,"function":"createProxyPreviewUrl","class":"OCA\\Settings\\Controller\\AppSettingsController","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/var/www/html/apps/settings/lib/Controller/AppSettingsController.php","line":295,"function":"fetchApps","class":"OCA\\Settings\\Controller\\AppSettingsController","type":"->","args":[]},{"file":"/var/www/html/lib/private/AppFramework/Http/Dispatcher.php","line":208,"function":"listApps","class":"OCA\\Settings\\Controller\\AppSettingsController","type":"->","args":[]},{"file":"/var/www/html/lib/private/AppFramework/Http/Dispatcher.php","line":114,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Settings\\Controller\\AppSettingsController"},"listApps"]},{"file":"/var/www/html/lib/private/AppFramework/App.php","line":161,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Settings\\Controller\\AppSettingsController"},"listApps"]},{"file":"/var/www/html/lib/private/Route/Router.php","line":306,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Settings\\Controller\\AppSettingsController","listApps",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"_route":"settings.appsettings.listapps"}]},{"file":"/var/www/html/lib/base.php","line":1010,"function":"match","class":"OC\\Route\\Router","type":"->","args":["/settings/apps/list"]},{"file":"/var/www/html/index.php","line":24,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"/var/www/html/apps/settings/lib/Controller/AppSettingsController.php","Line":236},"message":"OCA\\Settings\\Controller\\AppSettingsController::createProxyPreviewUrl(): Argument #1 ($url) must be of type string, array given, called in /var/www/html/apps/settings/lib/Controller/AppSettingsController.php on line 250 in file '/var/www/html/apps/settings/lib/Controller/AppSettingsController.php' line 236","exception":{},"CustomMessage":"OCA\\Settings\\Controller\\AppSettingsController::createProxyPreviewUrl(): Argument #1 ($url) must be of type string, array given, called in /var/www/html/apps/settings/lib/Controller/AppSettingsController.php on line 250 in file '/var/www/html/apps/settings/lib/Controller/AppSettingsController.php' line 236"}}

TODO

This is my first server PR so apologies if I messed something up

Checklist

@claucambra claucambra self-assigned this Oct 27, 2024
@provokateurin provokateurin requested a review from susnux October 27, 2024 16:32
@nickvergessen nickvergessen added this to the Nextcloud 31 milestone Oct 28, 2024
@claucambra claucambra force-pushed the bugfix/exception-appscreenshot-notstring branch from 0c41b0f to 732d2ec Compare October 28, 2024 06:50
@nickvergessen
Copy link
Member

I'll amend the actual fix...

…enshots

Some installed apps meant for older server versions might unexpectedly
offer up screenshot values in a non-string format (e.g. health). Avoid
an exception by checking first if the first app screenshot is indeed a
string and otherwise we take the value of the parameter

Signed-off-by: Claudio Cambra <[email protected]>
@nickvergessen nickvergessen force-pushed the bugfix/exception-appscreenshot-notstring branch from 732d2ec to 7e6b23b Compare October 28, 2024 07:30
@nickvergessen nickvergessen force-pushed the bugfix/exception-appscreenshot-notstring branch from 7e6b23b to 220bd34 Compare October 28, 2024 07:46
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: Do not trust the appinfo XSD.

@susnux susnux merged commit 8600636 into master Oct 28, 2024
177 checks passed
@susnux susnux deleted the bugfix/exception-appscreenshot-notstring branch October 28, 2024 10:32
Copy link

welcome bot commented Oct 28, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@nickvergessen
Copy link
Member

TIL: Do not trust the appinfo XSD.

Well the problem is a bit that PHP's XML engine makes things a string until they appear multiple times 🙈

@joshtrichards
Copy link
Member

/backport to stable30

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

Successfully merging this pull request may close these issues.

4 participants