Ensure that X-OC-MTime header is an integer with chunked uploads

[danxuliu]

This pull request is a follow up to #3793. That pull request ensured that the X-OC-MTime header processed by the server was an integer, but not when chunked uploads were used. This pull request ensures that too for chunked uploads: if the header value can be converted to an integer it will be (so it will not fail if a float is given, it will simply convert it to an integer), and if not an exception will be thrown.

Note, however, that there is a difference in the exceptions thrown for regular uploads and for chunked uploads: if the validation fails on a regular upload an InvalidArgumentException is thrown, but if a validation fails on a chunked upload a Sabre\DAV\Exception is thrown instead. The reason is that I have not touched the exception handling code, so the behaviour is the same as it was for regular and chunked uploads before this pull request (that is, exceptions in chunked uploads are converted to Sabre\DAV\Exception while exceptions in regular uploads are converted only in some specific parts of its code, which looks like something that may need a fix).

Besides that, this pull request also acts as an import of owncloud/core#28066 and owncloud/core#28676.

owncloud/core#28066 is the ownCloud equivalent of #3793; in the case of ownCloud no exception is thrown if the value is invalid, and it is converted to an integer with a different technique. However, the name of the function in which the code that ensures the correct type of X-OC-MTime was refactored, sanitizeMTime, is based on the one introduced in owncloud/core#28066.

owncloud/core#28066 also provides unit tests to ensure the proper handling of X-OC-MTime; those tests (and the changes to the code being tested) were adapted and included in this pull request. Note that in this pull request there is no special handling for negative mtimes in the tests as, if I am not mistaken, currently all the storages used in unit tests have support for negative mtimes.

owncloud/core#28676 is a follow up to owncloud/core#28066 that removes the need of running the unit tests in a separate process. This was needed due to the direct call to header in File, which caused the Cannot modify header information - headers already sent by error if tests were not run in a separate process. I am not familiar with this code, so in this pull request it was fixed like in the pull request from ownCloud, that is, by wrapping header with a protected function and mocking that function in the tests; I am sure that there are better alternatives, so please tell me :-)

owncloud/core#28676 also includes a totally unrelated change: remove forcing mtime values to be positive in the default filemtime function of Storages (apparently it was done just to remove the need of having a special handling for negative mtimes in their tests). I have not included that change in this pull request, but the question is: should that be added in a different pull request or is the current behaviour in Nextcloud right?

Finally, this pull request should not conflict with the Web UI, nor the desktop, Android or iOS clients:

• Some browsers (at least, Chromium) send a float value for X-OC-MTime with the current file-upload.js file, but it is not a problem due to the conversion to an integer performed in the server (although the Web UI should not be sending X-OC-MTime at all; it was introduced when chunked uploads were added to the Web UI and it will be fixed in a different pull request)
• The desktop client, as far as I know, always sends an integer X-OC-MTime for chunked uploads; even if the server is 32 bit and the client 64 bit and it sends a value too large for a 32 bit system the value will simply be truncated by the server to the largest positive or negative value available (depending on the case)
• If I am not mistaken, the Android client does not use chunked uploads yet sends a signed decimal representation of a long; like the desktop client it should work, but please test to be sure :-) @nextcloud/android
• I have no idea about the iOS client, so please test to be sure :-) @nextcloud/ios

[danxuliu]

Oops, I tested it on PHP 5.6, but it seems that there is a test case (string castable hex int) that fails on PHP 7 (because is_numeric does not support hexadecimal notation). I will fix it later, sorry to the reviewers for the noise :-)

[AndyScherzinger]

@danxuliu afaik the Android client does do chunked uploads already and will also do chunked downloads in the next release. Pinging @mario @tobiasKaminsky just to be sure.

[danxuliu]

@AndyScherzinger You are totally right; I recalled seeing an open issue about chunked uploads for the Android client, but it was for resuming them; chunked uploads are indeed being used already.

Sorry for my confusion :-)

[codecov]

Codecov Report

Merging #7313 into master will decrease coverage by 21.26%.
The diff coverage is 17.39%.

@@              Coverage Diff              @@
##             master    #7313       +/-   ##
=============================================
- Coverage     50.86%   29.59%   -21.27%     
- Complexity    24538    24542        +4     
=============================================
  Files          1584     1584               
  Lines         93794    93802        +8     
  Branches       1358     1358               
=============================================
- Hits          47706    27760    -19946     
- Misses        46088    66042    +19954

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Connector/Sabre/File.php	1.49% <17.39%> (-70.05%)	103 <5> (+4)
lib/private/DB/OCSqlitePlatform.php	0% <0%> (-100%)	5% <0%> (ø)
apps/files_versions/lib/AppInfo/Application.php	0% <0%> (-100%)	2% <0%> (ø)
lib/private/DB/QueryBuilder/QueryFunction.php	0% <0%> (-100%)	2% <0%> (ø)
...rivate/Authentication/Token/DefaultTokenMapper.php	0% <0%> (-100%)	11% <0%> (ø)
apps/user_ldap/lib/BackendUtility.php	0% <0%> (-100%)	1% <0%> (ø)
lib/private/TagManager.php	0% <0%> (-100%)	4% <0%> (ø)
...s/dav/lib/Connector/Sabre/Exception/FileLocked.php	0% <0%> (-100%)	3% <0%> (ø)
apps/admin_audit/composer/autoload.php	0% <0%> (-100%)	0% <0%> (ø)
apps/user_ldap/lib/LDAPProviderFactory.php	0% <0%> (-100%)	2% <0%> (ø)
... and 379 more

[danxuliu]

Done. I changed the code and adjusted the test accordingly to always reject X-OC-MTime if given as a string with hexadecimal notation.

Drone failure is not related (it failed to connect to PostgreSQL); this pull request is ready again to be reviewed :-)

[tobiasKaminsky]

chunked downloads in android

@AndyScherzinger I am not aware of this? Maybe you are mistaken this with "transfer-encoding: chunked" which is something different.

[tobiasKaminsky]

A quick test with a 19mb big files showed no problems:

• simply uploaded it and checked the md5sum
• upload it, disable data inbetween to force stop upload, resume, check md5sum

So from android side it should be 👍

[MorrisJobke]

should that be added in a different pull request or is the current behaviour in Nextcloud right?

Something for @icewind1991 - I would remove it in a separate PR.

[MorrisJobke]

I have no idea about the iOS client, so please test to be sure :-) @nextcloud/ios

I tested it and it worked. 👍