diff --git a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php index 968489eb..df71a775 100644 --- a/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php +++ b/app/Modules/Files/Http/Controllers/ChunkedUploadsController.php @@ -235,7 +235,10 @@ class ChunkedUploadsController extends Controller // chooses its own chunking and only the last part is short. $limit = $maxPartBytes * 2; - if ($request->header('Content-Length') !== null && (int) $request->header('Content-Length') > $limit) { + $contentLength = $request->header('Content-Length'); + $reservationLimit = $contentLength !== null ? (int) $contentLength : $limit; + + if ($reservationLimit < 1 || $reservationLimit > $limit) { abort(413); } @@ -254,7 +257,12 @@ class ChunkedUploadsController extends Controller // what a part gets is whatever the session has left, and the write // is then capped at exactly that — an over-long body is cut off // mid-stream as it always was, just against a smaller number. - $reserve = $this->reservePartRoom($session, $part, $limit); + // Reserve the declared request length when available. Reserving the + // full per-part ceiling (40 MiB for a normal 20 MiB chunk) makes + // concurrent final parts exhaust the session allowance prematurely. + // Unknown-length requests retain the conservative ceiling, and the + // streamed byte count is still enforced against the reservation. + $reserve = $this->reservePartRoom($session, $part, $reservationLimit); if ($reserve < 1) { // 413 rather than 422: this is about the size of what is being @@ -264,9 +272,10 @@ class ChunkedUploadsController extends Controller abort(413); } - $stream = $request->getContent(true); + $stream = null; try { + $stream = $request->getContent(true); $etag = $this->parts->storePart($session, $part, $stream, $reserve); } catch (PartTooLargeException) { abort(413); diff --git a/tests/Feature/Files/ChunkedUploadsTest.php b/tests/Feature/Files/ChunkedUploadsTest.php index 2e13e66f..dbd005fa 100644 --- a/tests/Feature/Files/ChunkedUploadsTest.php +++ b/tests/Feature/Files/ChunkedUploadsTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); use App\Models\User; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLog; +use App\Modules\Files\Http\Controllers\ChunkedUploadsController; use App\Modules\Files\Models\File; use App\Modules\Files\Uploads\UploadSession; use App\Modules\Identity\Models\RolePermission; @@ -12,6 +13,7 @@ use App\Modules\Identity\Permissions\Permission; use App\Modules\Identity\Permissions\SystemRole; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; +use Illuminate\Http\Request; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Storage; use Illuminate\Testing\TestResponse; @@ -534,6 +536,92 @@ test('staged bytes are released when a part is replaced, refused or falls short' expect($session()->staged_bytes)->toBe(10); }); +test('concurrent final parts reserve their declared lengths without exhausting the session', function () { + $this->actingAs($this->admin); + config(['projectsend.upload_part_size_mb' => 1]); + + $contents = [ + str_repeat('a', 1024 * 1024), + str_repeat('b', 1024 * 1024), + str_repeat('c', 1024 * 1024), + str_repeat('d', 1024 * 1024), + 'last-part', + ]; + $size = array_sum(array_map(strlen(...), $contents)); + $sessionId = createSession($size, 'concurrent.txt'); + $fibers = []; + + foreach ($contents as $index => $content) { + $request = new class extends Request + { + public function getContent($asResource = false) + { + // All requests reserve room before any request finishes + // reading. Fibers make this interleaving deterministic + // without separate processes or a shared test database. + Fiber::suspend(); + + return parent::getContent($asResource); + } + }; + $request->initialize([], [], [], [], [], ['CONTENT_LENGTH' => (string) strlen($content)], $content); + $request->setUserResolver(fn () => $this->admin); + $session = UploadSession::query()->findOrFail($sessionId); + $fiber = new Fiber(fn () => app(ChunkedUploadsController::class)->putPart($request, $session, $index + 1)); + $fiber->start(); + expect($fiber->isSuspended())->toBeTrue(); + $fibers[] = $fiber; + } + + expect(UploadSession::query()->findOrFail($sessionId)->staged_bytes)->toBe($size); + + foreach ($fibers as $fiber) { + $fiber->resume(); + expect($fiber->getReturn()->getStatusCode())->toBe(200); + } + + expect(UploadSession::query()->findOrFail($sessionId)->staged_bytes)->toBe($size); + + $response = $this->postJson("/uploads/{$sessionId}/complete")->assertOk(); + $file = File::query()->findOrFail($response->json('file_id')); + expect($file->size)->toBe($size) + ->and($file->checksum)->toBe(hash('sha256', implode('', $contents))) + ->and(Storage::disk('files')->get($file->path))->toBe(implode('', $contents)); +}); + +test('a declared part length is enforced while streaming and refunded after rejection', function () { + $this->actingAs($this->admin); + $sessionId = createSession(10); + $url = $this->getJson("/uploads/{$sessionId}/parts/1/sign")->assertOk()->json('url'); + + $this->call('PUT', $url, [], [], [], ['CONTENT_LENGTH' => '2'], 'four')->assertStatus(413); + + expect(UploadSession::query()->findOrFail($sessionId)->staged_bytes)->toBe(0) + ->and(is_file(partsRoot().'/'.$sessionId.'/1.part'))->toBeFalse(); + + $this->call('PUT', $url, [], [], [], ['CONTENT_LENGTH' => '4'], 'four')->assertOk(); + expect(UploadSession::query()->findOrFail($sessionId)->staged_bytes)->toBe(4); +}); + +test('a failure opening the request stream releases its reservation', function () { + $this->actingAs($this->admin); + $sessionId = createSession(10); + $session = UploadSession::query()->findOrFail($sessionId); + $request = new class extends Request + { + public function getContent($asResource = false) + { + throw new RuntimeException('Could not open request stream.'); + } + }; + $request->headers->set('Content-Length', '4'); + $request->setUserResolver(fn () => $this->admin); + + expect(fn () => app(ChunkedUploadsController::class)->putPart($request, $session, 1)) + ->toThrow(RuntimeException::class, 'Could not open request stream.'); + expect($session->fresh()->staged_bytes)->toBe(0); +}); + test('open sessions count against a client quota, so it cannot be spent twice', function () { $client = User::factory()->client()->create(['storage_quota_mb' => 1]); grantChunkedUploadPermission($client);