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

Can't share with user that only has numbers as name #37324

Closed
weberdigital opened this issue Apr 30, 2020 · 11 comments · Fixed by #37336
Closed

Can't share with user that only has numbers as name #37324

weberdigital opened this issue Apr 30, 2020 · 11 comments · Fixed by #37336
Assignees
Labels

Comments

@weberdigital
Copy link

Steps to reproduce

  1. Create a user with a name like "12345" (just numbers)
  2. Try to share a file with the user by searching their name and clicking on it

Expected behaviour

The file should get shared with the user.

Actual behaviour

The "popup" comes back again and the file won't get shared. A 500 error comes back from /ocs/v2.php/apps/files_sharing/api/v1/shares?format=json. However, there are no informations in the owncloud.log about this.

Server configuration

Operating system: CentOS 6.10

Web server: Apache

Database: MYSQL

PHP version: 7.2

ownCloud version: 10.4.1

Updated from an older ownCloud or fresh install: updated from older ownCloud

Where did you install ownCloud from: https://download.owncloud.org/community/owncloud-10.4.1.tar.bz2

Signing status (ownCloud 9.0 and above):

No errors have been found.

The content of config/config.php:
https://hastebin.com/givitefefo.json

List of activated apps:

Enabled:
  - activity: 2.5.3
  - comments: 0.3.0
  - configreport: 0.2.0
  - dav: 0.5.0
  - external: 1.4.0
  - federatedfilesharing: 0.5.0
  - federation: 0.1.0
  - files: 1.5.2
  - files_antivirus: 0.15.1
  - files_external: 0.7.1
  - files_mediaviewer: 1.0.2
  - files_pdfviewer: 0.11.1
  - files_sharing: 0.12.0
  - files_texteditor: 2.3.0
  - files_trashbin: 0.9.1
  - files_versions: 1.3.0
  - firstrunwizard: 1.2.0
  - gallery: 16.1.1
  - impersonate: 0.5.0
  - market: 0.5.0
  - notifications: 0.5.0
  - ##redacted##: 1.0.0
  - provisioning_api: 0.5.0
  - systemtags: 0.3.0
  - templateeditor: 0.4.0
  - updatenotification: 0.2.1
Disabled:
  - encryption
  - user_external

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

@phil-davis
Copy link
Contributor

Confirmed - using current core master and PHP dev server and PHP 7.4 I tried to share from user jimto user 456 There is nothing in owncloud.log. The server has:

[Thu Apr 30 21:24:07 2020] 10.49.210.9:40186 Accepted
[Thu Apr 30 21:24:07 2020] 10.49.210.9:40186 [200]: GET /ocs/v1.php/apps/files_sharing/api/v1/sharees?format=json&search=456&perPage=200&itemType=folder
[Thu Apr 30 21:24:07 2020] 10.49.210.9:40186 Closing
[Thu Apr 30 21:24:07 2020] 10.49.210.9:40188 Accepted
[Thu Apr 30 21:24:07 2020] 10.49.210.9:40188 [200]: GET /index.php/avatar/456/32
[Thu Apr 30 21:24:07 2020] 10.49.210.9:40188 Closing
[Thu Apr 30 21:24:09 2020] 10.49.210.9:40190 Accepted
[Thu Apr 30 21:24:10 2020] 10.49.210.9:40190 [500]: POST /ocs/v2.php/apps/files_sharing/api/v1/shares?format=json
[Thu Apr 30 21:24:10 2020] 10.49.210.9:40190 Closing

@phil-davis
Copy link
Contributor

phil-davis commented Apr 30, 2020

Here is a curl command that gives the error:

$ curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": 123, "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
{"ocs":{"meta":{"status":"failure","statuscode":500,"message":"","totalitems":"","itemsperpage":""},"data":[]}}

And it works fine if the shareWith user 123 is put in quotes in the JSON as "123"

$ curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": "123", "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
{"ocs":{"meta":{"status":"ok","statuscode":200,"message":null,"totalitems":"","itemsperpage":""},"data":{"id":"14","share_type":0,"uid_owner":"jim","displayname_owner":"jim","permissions":31,"stime":1588264259,"parent":null,"expiration":null,"token":null,"uid_file_owner":"jim","displayname_file_owner":"jim","additional_info_owner":null,"additional_info_file_owner":null,"path":"\/faaa","item_type":"folder","mimetype":"httpd\/unix-directory","storage_id":"home::jim","storage":7,"item_source":2147483717,"file_source":2147483717,"file_parent":9,"file_target":"\/faaa","share_with":"123","share_with_displayname":"123","share_with_additional_info":null,"mail_send":0,"attributes":null}}}

@phil-davis
Copy link
Contributor

Works in 10.3.2 and the UI sends form-data:
Form-data-request-payload-v10-3-2

Broken in 10.4.0 and the UI sends JSON payload:
JSON-request-payload-v10 4 0

@phil-davis
Copy link
Contributor

Q1) what happened in 10.4.0 that the way the request is done from the UI changed from from-data to JSON?

Q2) when sending a JSON payload, like in the curl example above, there should not be a 500 response anyway - that needs to be fixed in the backend.

@karakayasemi
Copy link
Contributor

Q1) what happened in 10.4.0 that the way the request is done from the UI changed from from-data to JSON?

Looks like the below commit changes in the ajax call for OCS createShare api led to this problem:
460d544#diff-2e1f33af47835fc7dab530c048c95f9aR197
Reverting only the change in ajax call is fixing the problem, I will open a PR to see if will it break any other thing.

Q2) when sending a JSON payload, like in the curl example above, there should not be a 500 response anyway - that needs to be fixed in the backend

if (!\is_string($sharedWith)) {
throw new \InvalidArgumentException();

Share object has argument type validations on its setter methods like given in above. We can catch InvalidArgumentExceptions and return HTTP 400 (Bad request) instead of 500.

@karakayasemi karakayasemi self-assigned this Apr 30, 2020
@phil-davis
Copy link
Contributor

if (!\is_string($sharedWith)) {

Actually why do we care so much that it is string. If it is a "number" that can be cast to a string and ends up as a valid UID then all is good - e.g. 12345 1.23

Tricky cases will be UIDs like "+1" which might be sent as just the integer 1 from the client (need to check). And "4.5E6" which might get mixed up with 4500000

@phil-davis
Copy link
Contributor

phil-davis commented May 1, 2020

Fails with UIDs:
123
1234567890
-12

Works with UIDs:
1.2
1.2E3
+12
6+7
001
-00
000
12345678901234567890
true
false
array
object

Failure seems to be only with UIDs that look like a "small enough" integer (positive or negative). In that case the shareWith value is not sent in quotes and it breaks at the server.

Other things that look like numbers are OK because the UI puts quotes around them when sending the JSON payload.

Groups seem OK - I tried a group called 42 and the request payload is sent:

{shareType: 1, shareWith: "42", permissions: 31, path: "/f-12"}

42 is in quotes.

@phil-davis
Copy link
Contributor

@karakayasemi something like #37327 fixes it.

@karakayasemi
Copy link
Contributor

karakayasemi commented May 1, 2020

@phil-davis #37327 can fix int sharedWith in json payload case and the current web UI regression. However, some api requests like below will continue to fail:

$ curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": 1.2, "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
{"ocs":{"meta":{"status":"failure","statuscode":500,"message":"","totalitems":"","itemsperpage":""},"data":[]}}

In this case, we are correcting integer parameter problems, but we are not correcting any other primitive type like double problems.

IMHO, interface method signature is correct, $sharedWith parameter should be string not any other type, so I would not change IShare interface. Instead of this, we can simply cast all request parameters to correct type on the share controller.

@phil-davis
Copy link
Contributor

phil-davis commented May 2, 2020

Sending without quotes is very tricky for UIDs that look like float - e.g. I locally allowed float to be received in setSharedWith and:

  1. create UIDs 1.3 and 1.30
  2. try to share with 1.30 by:
curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": 1.30, "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
  1. UID "1.3" receives the share.

  2. create UIDs 1200 and 1.2E3

  3. try to share with 1.2E3 by:

curl --user jim:jim http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json -d '{"shareType": 0, "shareWith": 1.2E3, "permissions": 31, "path": "/faaa"}' -X POST -H "Content-Type: application/json"
  1. UID "1200" receives the share

By the time the value 1.30 or 1.2E3 gets to setSharedWith it is already internally a float, and there is no way to know how to stringify it - should it be "1.3" "1.30" "1.300" "1200" "1.2E3" "12E2" "1200.0" or...

Would have to look earlier where the JSON is received on the server and decoded to see if the 1.30 or 1.2E3 is still like that in the incoming JSON-encoded payload. At that point there might be the chance that if shareWith is not in quotes, but is a numeric-looking unquoted "string" then just put exactly those characters into a string.

@karakayasemi
Copy link
Contributor

Looks like the main cause of the problem is the server returns share autocomplete list uid's as integer if uid is in numeric format due to following described behavior of the php: https://stackoverflow.com/questions/4100488/a-numeric-string-as-array-key-in-php

I created a fix draft pr #37336 to see if newly added acceptance tests pass.

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