From 398b106f0cb50f544bb99130e717f746fbfcebaf Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 28 Jul 2025 19:55:20 +0200 Subject: [PATCH] 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();