Skip to content

fix(dav): only derive the write size from a PUT Content-Length - #63510

Open
mosi-kha wants to merge 1 commit into
nextcloud:masterfrom
mosi-kha:fix/dav-assembly-zero-content-length
Open

fix(dav): only derive the write size from a PUT Content-Length#63510
mosi-kha wants to merge 1 commit into
nextcloud:masterfrom
mosi-kha:fix/dav-assembly-zero-content-length

Conversation

@mosi-kha

Copy link
Copy Markdown
Contributor

Summary

File::put() takes the expected write size from the request's Content-Length, but it is not reached only by PUT. The chunked upload assembly step is a MOVE (or COPY) of <upload>/.file: Directory::moveInto() and Directory::copyInto() only short-circuit File and Directory sources at storage level, so a FutureFile falls through to Tree::copyNode()Directory::createFile()File::put(). There the data is the AssemblyStream of the uploaded chunks and the request itself carries no body at all, so its Content-Length describes nothing relevant.

$lengthHeader = $this->request->getHeader('content-length');
$expected = $lengthHeader !== '' ? (int)$lengthHeader : null;
...
$count = $partStorage->writeStream($internalPartPath, $wrappedData, $expected);

Clients that send Content-Length: 0 on that request — Safari does, Chrome omits the header entirely — make $expected 0 instead of null. ObjectStoreStorage::writeStream() only measures the stream when it is given no size:

public function writeStream(string $path, $stream, ?int $size = null): int {
	if ($size === null) {
		$stats = fstat($stream);
		...

so the fstat() fallback is skipped, $stat['size'] becomes 0, and the file is recorded as empty while the request still answers 201. The upload looks successful and the file is silently empty.

The size comparison further down in the same method is already restricted to PUT for exactly this reason:

if ($expected !== null
	&& $expected !== $count
	&& $this->request->getMethod() === 'PUT'
) {

This applies the same restriction when deriving the expected size in the first place. Because the guard is on the method rather than on the value, it covers the COPY variant as well as MOVE — a fix that special-cased a zero length, or only MOVE, would not.

Genuine empty PUT uploads are unaffected: the method is still PUT, so a Content-Length of 0 is passed through exactly as before.

Scope

Object-storage primary storage is affected. OC\Files\Storage\Local::writeStream() ignores the $size argument entirely and just calls file_put_contents(), so local-storage installs should not be.

Methods that reach File::put():

method reaches put() data written Content-Length meaningful
PUT yes, CorePlugin::httpPut the request body yes
MOVE of <upload>/.file yes AssemblyStream no
COPY of <upload>/.file yes AssemblyStream no
MOVE/COPY of a normal file no, handled at storage level

Testing

FileTest::testPutExpectedSizeOnlyComesFromPutContentLength covers the size handed to writeStream() for PUT (with a length, with 0, and with the header absent) and for MOVE/COPY (with 0 and with a non-zero length).

Against master without the change, 4 of the 7 cases fail:

...FFFF                                                             7 / 7 (100%)

1) ... with data set "MOVE ignores a zero length" ('MOVE', ['0'], null)
Failed asserting that 0 is identical to null.
2) ... with data set "MOVE ignores a non-zero length" ('MOVE', ['9'], null)
Failed asserting that 9 is identical to null.
3) ... with data set "COPY ignores a zero length" ('COPY', ['0'], null)
Failed asserting that 0 is identical to null.
4) ... with data set "COPY ignores a non-zero length" ('COPY', ['9'], null)
Failed asserting that 9 is identical to null.

FAILURES!
Tests: 7, Assertions: 13, Failures: 4

With the change, Tests: 7, Assertions: 21, all passing.

Verified end to end as well, on 33.0.7 with nginx + php-fpm and S3 (MinIO) primary storage, using a 102 395 904 byte file in five chunks. Adding Content-Length: 0 to the assembly request is the only variable that breaks it, for both MOVE and COPY, sequentially or in parallel.

Optional hardening, not included here

@vpecinka independently diagnosed this in #7995 (comment) and patched the storage layer instead, forcing $size = null in ObjectStoreStorage::writeStream() when a zero is announced for a stream that fstat() shows is larger. That defends against any other caller passing a bogus zero, which this change does not.

It is deliberately left out to keep this PR to the root cause; writeStream() putting the announced size straight into the filecache is arguably worth guarding on its own. Happy to add it here or as a separate PR if you would prefer it.

Checklist

  • Code is properly formatted (php-cs-fixer reports no changes for both files)
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Screenshots before/after for front-end changes
  • Documentation (manuals or wiki) has been updated or is not required
  • Backports requested where applicable (ex: critical bugfixes) — happy to request these, say which branches
  • Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
  • Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

put() is not only reached by PUT. The chunked upload assembly step is a MOVE or
COPY of <upload>/.file: Directory::moveInto() and ::copyInto() only
short-circuit File and Directory sources at storage level, so a FutureFile
falls through to Tree::copyNode() -> createFile() -> put(), where the data is
the AssemblyStream of the uploaded chunks and the request itself carries no
body at all.

For those requests the Content-Length says nothing about the stream being
written. Clients that send "Content-Length: 0" there, Safari among them, made
File::put() hand a 0 to IWriteStreamStorage::writeStream(). Storages that only
measure the stream when they are given no size, ObjectStoreStorage among them,
then wrote an empty file while the request still answered 201, so the upload
looked successful and the file was silently empty.

The size comparison further down already restricts itself to PUT for exactly
this reason. Apply the same restriction when deriving the expected size, which
covers both the MOVE and the COPY variant.

Genuine empty PUT uploads are unaffected: the method is still PUT, so a
Content-Length of 0 is passed through as before.

Signed-off-by: mostafa <mostafakhaki00@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mosi-kha
mosi-kha requested a review from a team as a code owner August 23, 2026 15:38
@mosi-kha
mosi-kha requested review from Altahrim, icewind1991, provokateurin and salmart-dev and removed request for a team August 23, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chunked uploading - MOVE with Content-Length: 0 results in 0bytes files in Object Storage

1 participant