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

NC20: Music app Ampache API broken because URL parameter 'action' can't be used #23544

Closed
paulijar opened this issue Oct 17, 2020 · 7 comments · Fixed by #24737
Closed

NC20: Music app Ampache API broken because URL parameter 'action' can't be used #23544

paulijar opened this issue Oct 17, 2020 · 7 comments · Fixed by #24737
Assignees
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug

Comments

@paulijar
Copy link
Contributor

paulijar commented Oct 17, 2020

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps to reproduce

  1. Call any REST API endpoint and append GET query argument action=somecontent, e.g. https://domain.org/nextcloud/index.php/apps/someapp/api/somemethod?action=somecontent
  2. Try to use the argument action in the controller class either through function argument injection public function someMethod($action) or by calling $this->request->getParam('action') or $this->request->getParams()['action'] or $this->request['action'].

Expected behaviour

The controller should be able to obtain the value somecontent of the URL argument action.

Actual behaviour

The value of the argument action is empty, no matter which method you use to obtain it. Arguments with any other name work as expected, but the name action is somehow special. Also, if you call an API endpoint without supplying the command line argument action, the array returned by $this->request->getParams() still contains the key action with an empty content.

This oddity prevents using the Ampache API of the Music app on Nextcloud 20, as reported in owncloud/music#791. This issue is new in NC20, there has never been such problem on the NC versions 12 - 19.

(Side note: The Music app version 0.17.0 with support for NC20 has just been released.)

Server configuration

Operating system: Raspbian Linux 4.9.35-v7+ armv7l

Web server: nginx/1.6.2

Database: sqlite3 3.8.7.1

PHP version: 7.4

Nextcloud version: Nextcloud 20.0.0 RC2

Updated from an older Nextcloud/ownCloud or fresh install: fresh

Where did you install Nextcloud from: https://nextcloud.com/install/

Signing status:

Signing status
No errors have been found.

List of activated apps:

App list
Enabled:
  - accessibility: 1.6.0
  - activity: 2.13.1
  - bruteforcesettings: 2.0.1
  - cloud_federation_api: 1.3.0
  - comments: 1.10.0
  - contactsinteraction: 1.1.0
  - dashboard: 7.0.0
  - dav: 1.16.0
  - federatedfilesharing: 1.10.1
  - federation: 1.10.1
  - files: 1.15.0
  - files_pdfviewer: 2.0.1
  - files_rightclick: 0.17.0
  - files_sharing: 1.12.0
  - files_trashbin: 1.10.1
  - files_versions: 1.13.0
  - files_videoplayer: 1.9.0
  - firstrunwizard: 2.9.0
  - logreader: 2.5.0
  - lookup_server_connector: 1.8.0
  - music: 0.16.1-alpha1
  - nextcloud_announcements: 1.9.0
  - notifications: 2.8.0
  - oauth2: 1.8.0
  - password_policy: 1.10.1
  - photos: 1.2.0
  - privacy: 1.4.0
  - provisioning_api: 1.10.0
  - recommendations: 0.8.0
  - serverinfo: 1.10.0
  - settings: 1.2.0
  - sharebymail: 1.10.0
  - support: 1.3.0
  - survey_client: 1.8.0
  - systemtags: 1.10.0
  - text: 3.1.0
  - theming: 1.11.0
  - twofactor_backupcodes: 1.9.0
  - updatenotification: 1.10.0
  - user_status: 1.0.0
  - viewer: 1.4.0
  - weather_status: 1.0.0
  - workflowengine: 2.2.0
Disabled:
  - admin_audit
  - encryption
  - files_external
  - user_ldap

Nextcloud configuration:

Config report
{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "***REMOVED SENSITIVE VALUE***",
            "***REMOVED SENSITIVE VALUE***",
            "***REMOVED SENSITIVE VALUE***"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "sqlite3",
        "version": "20.0.0.8",
        "overwrite.cli.url": "https:\/\/***REMOVED SENSITIVE VALUE***\/nextcloud-20",
        "installed": true,
        "loglevel": 1
    }
}

Are you using external storage, if yes which one: no

Are you using encryption: no

Are you using an external user-backend, if yes which one: no

Client configuration

Browser: N/A

Operating system: N/A

@paulijar paulijar added bug 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Oct 17, 2020
@paulijar paulijar changed the title NC20: Application can't use URL parameter 'action' NC20: Music app Ampache API broken because URL parameter 'action' can't be used Oct 26, 2020
@mariusurbelis
Copy link

If I knew how to fix this I would 🙁
Pls fix <3

@paulijar
Copy link
Contributor Author

So does anyone have any idea, why is the URL argument action special for Nextcloud 20? And what has changed on this regard between NC19 and NC20?

@paulijar
Copy link
Contributor Author

paulijar commented Nov 29, 2020

I'm almost certain that this bug has been introduced as a side effect of the commit edc1c77. Before the commit, the execution went through this branch:

if (isset($parameters['action'])) {
$action = $parameters['action'];
if (!is_callable($action)) {
throw new \Exception('not a callable action');
}
unset($parameters['action']);
call_user_func($action, $parameters);

...where the key 'action' is removed from the $parameters returned by the UrlMatcher.

After the commit, this branch is executed instead:

if (isset($parameters['caller'])) {
$caller = $parameters['caller'];
unset($parameters['caller']);
$application = $this->getApplicationClass($caller[0]);
\OC\AppFramework\App::main($caller[1], $caller[2], $application->getContainer(), $parameters);

Here, the $parameters contains the key 'action' with null value, and it is passed straight through to App::main which passes it to the Request object. Then, the array_merge here overwrites any URL arguments or POST parameters named 'action' with the null-valued key:

public function setUrlParameters(array $parameters) {
$this->items['urlParams'] = $parameters;
$this->items['parameters'] = array_merge(
$this->items['parameters'],
$this->items['urlParams']
);
}

What do you think, @rullzer?

@paulijar
Copy link
Contributor Author

To add to my previous comment, it seems that the bug can be fixed by adding the call

unset($parameters['action']);

near the line

unset($parameters['caller']);

But as I lack the knowledge of the big picture of the Nextcloud routing, I'm not certain if this is the best way to fix this, or if this could have some side effects on some case. But so far I have noticed nothing wrong when I made this experimental one line fix on my installation.

@florom
Copy link

florom commented Dec 16, 2020

Hi Devs,

I would like to upvote this issue too.
Unfortunately it breaks Power Ampache app.

Thanks.

@florom
Copy link

florom commented Jan 15, 2021

Hi Devs,

It works with Nextcloud 20.0.5.

Thanks!

@KSATDesign
Copy link

i found that the url needs to be https://nextcloudserver.tld/index.php/apps/music/ampache i also used the api password generator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants