From 398b106f0cb50f544bb99130e717f746fbfcebaf Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 28 Jul 2025 19:55:20 +0200 Subject: [PATCH 1/3] fix: validate written size for s3 multipart uploads Signed-off-by: Robin Appelman --- apps/dav/lib/Connector/Sabre/File.php | 12 +++---- .../Files/ObjectStore/ObjectStoreStorage.php | 3 ++ .../Files/ObjectStore/S3ObjectTrait.php | 33 +++++++++++++++++-- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 218d38e1c4b..d2a71eb3e7b 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -204,6 +204,9 @@ class File extends Node implements IFile { } } + $lengthHeader = $this->request->getHeader('content-length'); + $expected = $lengthHeader !== '' ? (int)$lengthHeader : null; + if ($partStorage->instanceOfStorage(IWriteStreamStorage::class)) { $isEOF = false; $wrappedData = CallbackWrapper::wrap($data, null, null, null, null, function ($stream) use (&$isEOF): void { @@ -215,7 +218,7 @@ class File extends Node implements IFile { $count = -1; try { /** @var IWriteStreamStorage $partStorage */ - $count = $partStorage->writeStream($internalPartPath, $wrappedData); + $count = $partStorage->writeStream($internalPartPath, $wrappedData, $expected); } catch (GenericFileException $e) { $logger = Server::get(LoggerInterface::class); $logger->error('Error while writing stream to storage: ' . $e->getMessage(), ['exception' => $e, 'app' => 'webdav']); @@ -235,10 +238,7 @@ class File extends Node implements IFile { [$count, $result] = Files::streamCopy($data, $target, true); fclose($target); } - - $lengthHeader = $this->request->getHeader('content-length'); - $expected = $lengthHeader !== '' ? (int)$lengthHeader : -1; - if ($result === false && $expected >= 0) { + if ($result === false && $expected !== null) { throw new Exception( $this->l10n->t( 'Error while copying file to target location (copied: %1$s, expected filesize: %2$s)', @@ -253,7 +253,7 @@ class File extends Node implements IFile { // if content length is sent by client: // double check if the file was fully received // compare expected and actual size - if ($expected >= 0 + if ($expected !== null && $expected !== $count && $this->request->getMethod() === 'PUT' ) { diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 10ee6aec167..bb42fa3e450 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -475,6 +475,9 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil 'original-storage' => $this->getId(), 'original-path' => $path, ]; + if ($size) { + $metadata['size'] = $size; + } $stat['mimetype'] = $mimetype; $stat['etag'] = $this->getETag($path); diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php index 5e6dcf88a42..89405de2e8e 100644 --- a/lib/private/Files/ObjectStore/S3ObjectTrait.php +++ b/lib/private/Files/ObjectStore/S3ObjectTrait.php @@ -6,6 +6,8 @@ */ namespace OC\Files\ObjectStore; +use Aws\Command; +use Aws\Exception\MultipartUploadException; use Aws\S3\Exception\S3MultipartUploadException; use Aws\S3\MultipartCopy; use Aws\S3\MultipartUploader; @@ -96,7 +98,9 @@ trait S3ObjectTrait { protected function writeSingle(string $urn, StreamInterface $stream, array $metaData): void { $mimetype = $metaData['mimetype'] ?? null; unset($metaData['mimetype']); - $this->getConnection()->putObject([ + unset($metaData['size']); + + $args = [ 'Bucket' => $this->bucket, 'Key' => $urn, 'Body' => $stream, @@ -104,7 +108,13 @@ trait S3ObjectTrait { 'ContentType' => $mimetype, 'Metadata' => $this->buildS3Metadata($metaData), 'StorageClass' => $this->storageClass, - ] + $this->getSSECParameters()); + ] + $this->getSSECParameters(); + + if ($size = $stream->getSize()) { + $args['ContentLength'] = $size; + } + + $this->getConnection()->putObject($args); } @@ -119,12 +129,15 @@ trait S3ObjectTrait { protected function writeMultiPart(string $urn, StreamInterface $stream, array $metaData): void { $mimetype = $metaData['mimetype'] ?? null; unset($metaData['mimetype']); + unset($metaData['size']); $attempts = 0; $uploaded = false; $concurrency = $this->concurrency; $exception = null; $state = null; + $size = $stream->getSize(); + $totalWritten = 0; // retry multipart upload once with concurrency at half on failure while (!$uploaded && $attempts <= 1) { @@ -139,6 +152,15 @@ trait S3ObjectTrait { 'Metadata' => $this->buildS3Metadata($metaData), 'StorageClass' => $this->storageClass, ] + $this->getSSECParameters(), + 'before_upload' => function (Command $command) use (&$totalWritten) { + $totalWritten += $command['ContentLength']; + }, + 'before_complete' => function ($_command) use (&$totalWritten, $size, &$uploader, &$attempts) { + if ($size !== null && $totalWritten != $size) { + $e = new \Exception('Incomplete multi part upload, expected ' . $size . ' bytes, wrote ' . $totalWritten); + throw new MultipartUploadException($uploader->getState(), $e); + } + }, ]); try { @@ -155,6 +177,9 @@ trait S3ObjectTrait { if ($stream->isSeekable()) { $stream->rewind(); } + } catch (MultipartUploadException $e) { + $exception = $e; + break; } } @@ -180,7 +205,9 @@ trait S3ObjectTrait { public function writeObjectWithMetaData(string $urn, $stream, array $metaData): void { $canSeek = fseek($stream, 0, SEEK_CUR) === 0; - $psrStream = Utils::streamFor($stream); + $psrStream = Utils::streamFor($stream, [ + 'size' => $metaData['size'] ?? null, + ]); $size = $psrStream->getSize(); From 83b8a390cd3cbbd56bc4cda3c260845ae1f76d85 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 29 Jul 2025 17:47:03 +0200 Subject: [PATCH 2/3] fix: always do stream counting for object store upload Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index bb42fa3e450..c21eea90a3f 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -499,32 +499,27 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil $urn = $this->getURN($fileId); try { //upload to object storage - if ($size === null) { - $countStream = CountWrapper::wrap($stream, function ($writtenSize) use ($fileId, &$size) { + + $totalWritten = 0; + $countStream = CountWrapper::wrap($stream, function ($writtenSize) use ($fileId, $size, $exists, &$totalWritten) { + if (is_null($size) && !$exists) { $this->getCache()->update($fileId, [ 'size' => $writtenSize, ]); - $size = $writtenSize; - }); - if ($this->objectStore instanceof IObjectStoreMetaData) { - $this->objectStore->writeObjectWithMetaData($urn, $countStream, $metadata); - } else { - $this->objectStore->writeObject($urn, $countStream, $metadata['mimetype']); } - if (is_resource($countStream)) { - fclose($countStream); - } - $stat['size'] = $size; + $totalWritten = $writtenSize; + }); + + if ($this->objectStore instanceof IObjectStoreMetaData) { + $this->objectStore->writeObjectWithMetaData($urn, $countStream, $metadata); } else { - if ($this->objectStore instanceof IObjectStoreMetaData) { - $this->objectStore->writeObjectWithMetaData($urn, $stream, $metadata); - } else { - $this->objectStore->writeObject($urn, $stream, $metadata['mimetype']); - } - if (is_resource($stream)) { - fclose($stream); - } + $this->objectStore->writeObject($urn, $countStream, $metadata['mimetype']); } + if (is_resource($countStream)) { + fclose($countStream); + } + + $stat['size'] = $totalWritten; } catch (\Exception $ex) { if (!$exists) { /* @@ -564,7 +559,7 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil } } - return $size; + return $totalWritten; } public function getObjectStore(): IObjectStore { From 97efc95efc751d54b27048bec3acd6edcb5389cd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 30 Jul 2025 14:56:41 +0200 Subject: [PATCH 3/3] fix: better object store write error propagation Signed-off-by: Robin Appelman --- lib/private/Files/ObjectStore/ObjectStoreStorage.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index c21eea90a3f..9ab11f8a3df 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -543,7 +543,7 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil ] ); } - throw $ex; // make this bubble up + throw new GenericFileException('Error while writing stream to object store', 0, $ex); } if ($exists) {