nextcloud / app_api

Nextcloud AppAPI
https://apps.nextcloud.com/apps/app_api
GNU Affero General Public License v3.0
81 stars 7 forks source link

Query param got stripped in request through proxy #384

Closed SagarGi closed 1 month ago

SagarGi commented 1 month ago

Description

For a browser request : http://localhost/stable300/index.php/apps/app_api/proxy/openproject-nextcloud-app/projects/demo-project/settings/project_storages/new?utf8=%E2%9C%93&storages_project_storage%5Bstorage_id%5D=2&button=

Through the proxy the storage_id seems to be stripped.

logs in proxy

ver': ('127.0.0.1', 9030), 'client': ('127.0.0.1', 37524), 'scheme': 'http', 'root_path': '', 'headers': '<...>', 'state': {}, 
'method': 'GET', 
'path': '/projects/demo-project/settings/project_storages/new', 
'raw_path': b'/projects/demo-project/settings/project_storages/new', 
'query_string': **b'storages_project_storage%5B%5D=2&utf8=%E2%9C%93&button='}**

Expected:

'query_string': **b'storages_project_storage%5Bstorage_id%5D=2&utf8=%E2%9C%93&button='

Actual:

'query_string': **b'storages_project_storage%5B%5D=2&utf8=%E2%9C%93&button='

Environment:

app_api version 3.2.0 nextcloud version 30.0.0 rc4

SagarGi commented 1 month ago

hi @andrey18106 not sure if this is a bug but i came across this thing.

bigcat88 commented 1 month ago

Sorry for the long delay. Is issue still valid?

If yes, we will schedule time to reproduce this, and if it confirmed as a bug, we will fix it of course.

SagarGi commented 1 month ago

Sorry for the long delay. Is issue still valid?

If yes, we will schedule time to reproduce this, and if it confirmed as a bug, we will fix it of course.

i can check again with the latest app_api and then confirm @bigcat88 if it still exists.

SagarGi commented 1 month ago

Hi @bigcat88 the issue still exists for version 5.0.0(master).

edward-ly commented 1 month ago

https://github.com/nextcloud/app_api/blob/2b37cec82885fb590829a6e86a1380c2f98fddf0/lib/Service/AppAPIService.php#L261-L272

It looks like the array keys are being stripped out of the query parameter values, although I don't recall what the purpose of this is when the http_build_query function itself is able to handle array parameters (including nested arrays) automatically. @bigcat88 or @andrey18106 do you have any ideas why we do this?

SagarGi commented 1 month ago

app_api/lib/Service/AppAPIService.php

Lines 261 to 272 in 2b37cec

private function getUriEncodedParams(array $params): string { $paramsContent = ''; foreach ($params as $key => $value) { if (is_array($value)) { foreach ($value as $oneArrayValue) { $paramsContent .= $key . '[]=' . urlencode($oneArrayValue) . '&'; } unset($params[$key]); } } return $paramsContent . http_build_query($params); } It looks like the array keys are being stripped out of the query parameter values, although I don't recall what the purpose of this is when the http_build_query function itself is able to handle array parameters (including nested arrays) automatically. @bigcat88 or @andrey18106 do you have any ideas why we do this?

yes the exact thing happened in my case. @edward-ly

andrey18106 commented 1 month ago

app_api/lib/Service/AppAPIService.php

Lines 261 to 272 in 2b37cec private function getUriEncodedParams(array $params): string { $paramsContent = ''; foreach ($params as $key => $value) { if (is_array($value)) { foreach ($value as $oneArrayValue) { $paramsContent .= $key . '[]=' . urlencode($oneArrayValue) . '&'; } unset($params[$key]); } } return $paramsContent . http_build_query($params); }

It looks like the array keys are being stripped out of the query parameter values, although I don't recall what the purpose of this is when the http_build_query function itself is able to handle array parameters (including nested arrays) automatically. @bigcat88 or @andrey18106 do you have any ideas why we do this?

I think we are using the same algorithm in several integrations that wasn't changed for years.

edward-ly commented 1 month ago

Hi @SagarGi, does applying the patch in the above PR fix the query string for you? If so, we can merge it and close this issue.