Ensure that X-OC-MTime header is an integer with chunked uploads #7313
Conversation
This commit extends the changes introduced in pull request #3793 also to chunked uploads. The "sanitizeMTime" method name is the same used in the equivalent pull request to this one from ownCloud (28066). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will be used in a following commit to test how the X-OC-MTime header is handled. This commit is based on the "make File::put() more testable" commit (included in 018d45cad97e0) from ownCloud by Artur Neumann. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This commit is based on the commits from pull request 28066 (included in 018d45cad97e0) from ownCloud by Artur Neumann and Phil Davis. Unit tests are currently run only on systems that support negative mtimes, so no special handling of negative values was included in the tests to keep the test code more manageable. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Directly calling "header" in the PHPUnit process causes the "Cannot modify header information - headers already sent by" error to be thrown. Instead of running the test in a separate process, which is slower, this commit wraps the call to "header" in a method that can be mocked in the tests. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
|
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 |
|
@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. |
|
@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 :-) |
In PHP 7.X hexadecimal notation support was removed from "is_numeric", so "sanitizeMtime" directly rejected those values; in PHP 5.X, on the other hand, "sanitizeMtime" returned 0 when a string with hexadecimal notation was given (as it was the behaviour of "intval"). To provide a consistent behaviour between PHP versions, and given that it does not make much sense to send X-OC-MTime in hexadecimal notation, now X-OC-MTime is always rejected if given as a string with hexadecimal notation. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Codecov Report
@@ 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
|
|
Done. I changed the code and adjusted the test accordingly to always reject Drone failure is not related (it failed to connect to PostgreSQL); this pull request is ready again to be reviewed :-) |
@AndyScherzinger I am not aware of this? Maybe you are mistaken this with "transfer-encoding: chunked" which is something different. |
|
A quick test with a 19mb big files showed no problems:
So from android side it should be 👍 |
Something for @icewind1991 - I would remove it in a separate PR. |
I tested it and it worked. 👍 |
This pull request is a follow up to #3793. That pull request ensured that the
X-OC-MTimeheader 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
InvalidArgumentExceptionis thrown, but if a validation fails on a chunked upload aSabre\DAV\Exceptionis 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 toSabre\DAV\Exceptionwhile 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-MTimewas 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
headerinFile, 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 wrappingheaderwith 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
filemtimefunction 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:
X-OC-MTimewith the currentfile-upload.jsfile, but it is not a problem due to the conversion to an integer performed in the server (although the Web UI should not be sendingX-OC-MTimeat all; it was introduced when chunked uploads were added to the Web UI and it will be fixed in a different pull request)X-OC-MTimefor 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)does not use chunked uploads yetsends a signed decimal representation of a long; like the desktop client it should work, but please test to be sure :-) @nextcloud/android