gsadmin/projectsend
1
0
Fork 0
mirror of https://github.com/projectsend/projectsend.git synced 2026-09-24 04:16:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1692 from denkfabrik-li/fix/zip-download-limit-at-delivery

Browse Source 
Enforce the download limit when a zip is delivered
This commit is contained in:
Ignacio Nelson
2026-08-26 22:31:57 -03:00
committed by GitHub
parent 9b3f7023d0 acda732ff4
commit e4cd56f5d6
6 changed files with 305 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/Modules/Files/Http/Controllers/ZipDownloadsController.php
+76 -11
View File
@@ -18,6 +18,7 @@ use App\Modules\Files\Uploads\StoreUploadedFile;
use App\Modules\Platform\Settings\Setting;
use App\Modules\Platform\Settings\Settings;
use App\Support\ContentDisposition;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Http\Response;
@@ -180,8 +181,7 @@ class ZipDownloadsController extends Controller
// Only the first time. Re-fetching one prepared archive is the
// same delivery, not a fresh download of everything inside it.
if ($zipDownload->delivered_at === null) {
$this->logContainedDownloads($zipDownload, $user);
$zipDownload->forceFill(['delivered_at' => now()])->save();
$this->deliverOnce($zipDownload, $user);
}
$size = Storage::disk('files')->size($path);
@@ -195,14 +195,81 @@ class ZipDownloadsController extends Controller
}
/**
* Every file actually bundled gets a FileDownloaded entry — otherwise
* a file's download history/count would silently miss zip downloads.
* Hand the archive over, once: refuse it if anything inside is out of
* allowance, otherwise count everything it holds as downloaded.
*
* This is the only point that spends a download limit, which is why
* it also has to be the point that enforces it. Building an archive
* takes nothing, so ordering the same limited file into any number of
* archives passes every check on the way — store() and the job both
* look at an allowance nothing has drawn on yet — and collecting them
* all afterwards would hand over more copies than the limit allows.
*
* One refused file refuses the whole delivery, because nothing can be
* taken out of a finished archive without building it again. Ordering
* the same selection afresh is the way through: the build leaves the
* spent file out and names it in skipped_files.
*
* An archive from before the job recorded its contents is handed over
* the way it always was, without this check. Its contents can only be
* guessed at by resolving the selection again, and guessing is exactly
* what must not decide a refusal: the same reconstruction both refuses
* over files the archive does not hold and misses files it does. Those
* rows stop existing within a day or two of an upgrade, and until then
* they behave as they did before this change rather than worse.
*/
private function logContainedDownloads(ZipDownload $zipDownload, User $requester): void
private function deliverOnce(ZipDownload $zipDownload, User $requester): void
{
$recorded = $zipDownload->contained_file_ids;
// What the job wrote down, read back as it stands — deliberately
// not filtered by what the requester may see today. The bytes are
// in the archive already, so a file that has since expired or left
// their scope is still being given to them, and a count that
// quietly dropped it would understate what was taken.
$contained = $recorded === null
? $this->resolveSelection($zipDownload, $requester)
: File::query()->whereIn('id', $recorded)->get();
abort_if(
$recorded !== null
&& $contained->contains(fn (File $file): bool => ! $this->allowance->allows($file, $requester)),
403,
__('Those files have reached their download limit.'),
);
// Atomic, so two fetches arriving together are still one delivery:
// only the request that actually moves delivered_at logs anything.
// Same reasoning as the conditional increment guarding a share
// link's max_downloads in PublicShareController. The other request
// still receives the archive — that is the re-fetch rule above.
$claimed = ZipDownload::query()
->whereKey($zipDownload->id)
->whereNull('delivered_at')
->update(['delivered_at' => now()]);
if ($claimed === 0) {
return;
}
// Every file actually bundled gets a FileDownloaded entry —
// otherwise a file's download history/count would silently miss
// zip downloads.
foreach ($contained as $file) {
$this->activity->log(Action::FileDownloaded, subject: $file);
}
}
/**
* What an archive built before the job recorded its contents is taken
* to hold: the selection, resolved again, which is how this worked
* throughout. Only reachable for rows written by an older release,
* and PurgeZipDownloadsCommand removes those within a day.
*
* @return Collection<int, File>
*/
private function resolveSelection(ZipDownload $zipDownload, User $requester): Collection
{
// Same per-file filter the job used to decide what actually went
// into the archive, so the log records what was really downloaded
// rather than everything that happened to sit in the folder.
$visible = $this->viewable->for($requester);
$fileIds = collect($zipDownload->file_ids);
@@ -220,9 +287,7 @@ class ZipDownloadsController extends Controller
// further past it.
$skipped = collect($zipDownload->skipped_files ?? [])->pluck('id')->all();
foreach ((clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get() as $file) {
$this->activity->log(Action::FileDownloaded, subject: $file);
}
return (clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get();
}
private function filenameFor(ZipDownload $zipDownload): string
app/Modules/Files/Jobs/BuildZipDownloadJob.php
+14 -9
View File
@@ -122,9 +122,12 @@ class BuildZipDownloadJob implements ShouldQueue
$tempFiles = [];
$skipped = [];
// Counted rather than derived from $usedNames, which also
// holds the folder entry names.
$added = 0;
// Collected rather than derived from $usedNames, which also
// holds the folder entry names. Recording the ids, not just a
// count, is what lets the download action log exactly what it
// hands over instead of resolving the selection a second time
// against a scope that may have moved since.
$addedIds = [];
foreach ((clone $visible)->whereIn('id', $zipDownload->file_ids)->get() as $file) {
// Re-checked here for the same reason visibility is: the
@@ -139,11 +142,11 @@ class BuildZipDownloadJob implements ShouldQueue
$entryName = $this->dedupeName($usedNames, $this->entrySegment($file->original_name));
$zip->addFile($this->localPathFor($file, $tempFiles), $entryName);
$totalSize += $file->size;
$added++;
$addedIds[] = $file->id;
}
foreach (Folder::query()->whereIn('id', $zipDownload->folder_ids)->get() as $folder) {
$totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $added);
$totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $addedIds);
}
// Re-checked here, not only in ZipDownloadsController: the
@@ -186,7 +189,7 @@ class BuildZipDownloadJob implements ShouldQueue
@unlink($tempFile);
}
if ($written !== true || $added === 0) {
if ($written !== true || $addedIds === []) {
if ($written !== true) {
// What the requester sees stays generic: a libzip
// string means nothing to them and can name a server
@@ -218,7 +221,8 @@ class BuildZipDownloadJob implements ShouldQueue
'status' => ZipDownload::STATUS_READY,
'path' => $relativePath,
'total_size' => $totalSize,
'file_count' => $added,
'file_count' => count($addedIds),
'contained_file_ids' => $addedIds,
'skipped_files' => $skipped === [] ? null : $skipped,
]);
} catch (Throwable $e) {
@@ -317,8 +321,9 @@ class BuildZipDownloadJob implements ShouldQueue
* @param array<int, string> $tempFiles
* @param Builder<File> $visible every file the requester may read
* @param list<array{id: int, name: string}> $skipped
* @param list<int> $addedIds every file really written into the archive
*/
private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, int &$added): int
private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, array &$addedIds): int
{
$allowance = app(DownloadAllowance::class);
@@ -344,7 +349,7 @@ class BuildZipDownloadJob implements ShouldQueue
$entryPath = $this->dedupeName($usedNames, $entryPath);
$zip->addFile($this->localPathFor($file, $tempFiles), $entryPath);
$totalSize += $file->size;
$added++;
$addedIds[] = $file->id;
}
return $totalSize;
app/Modules/Files/Models/ZipDownload.php
+2
View File
@@ -22,6 +22,7 @@ use Illuminate\Support\Carbon;
* @property string|null $error
* @property list<int> $file_ids
* @property list<int> $folder_ids
* @property list<int>|null $contained_file_ids
* @property list<array{id: int, name: string}>|null $skipped_files
* @property Carbon|null $delivered_at
*/
@@ -40,6 +41,7 @@ class ZipDownload extends Model
return [
'file_ids' => 'array',
'folder_ids' => 'array',
'contained_file_ids' => 'array',
'skipped_files' => 'array',
'delivered_at' => 'datetime',
];
database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php
+32
View File
@@ -0,0 +1,32 @@
<?php
declare(strict_types=1);
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
return new class extends Migration
{
public function up(): void
{
Schema::table('zip_downloads', function (Blueprint $table) {
// What the job actually put in the archive, recorded as it
// adds each file. `file_ids` and `folder_ids` are the request,
// not the result: a folder's contents were resolved again at
// download time, against a scope that may have moved since the
// archive was written. That made the download log describe a
// selection rather than a delivery — a file added to the folder
// afterwards was counted without ever being in the zip, and one
// moved out of it was handed over without being counted.
$table->json('contained_file_ids')->nullable()->after('folder_ids');
});
}
public function down(): void
{
Schema::table('zip_downloads', function (Blueprint $table) {
$table->dropColumn('contained_file_ids');
});
}
};
tests/Feature/Files/DownloadLimitEnforcementTest.php
+143
View File
@@ -236,3 +236,146 @@ test('re-fetching one prepared zip does not count as downloading everything agai
expect($file->downloads()->count())->toBe(1)
->and($zip->fresh()->delivered_at)->not->toBeNull();
});
test('a prepared zip cannot be collected once its files have been spent elsewhere', function () {
$file = limitedFile();
shareFileWith($file, $this->client);
// Two archives of the same file, both ordered and both built before
// either is collected. Nothing is spent yet, so every check made
// while ordering and building passes for both of them.
$first = $this->actingAs($this->client)
->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk();
$second = $this->actingAs($this->client)
->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk();
$firstZip = ZipDownload::query()->findOrFail($first->json('id'));
$secondZip = ZipDownload::query()->findOrFail($second->json('id'));
expect($firstZip->file_count)->toBe(1)
->and($secondZip->file_count)->toBe(1);
$this->actingAs($this->client)->get("/zip-downloads/{$firstZip->id}/download")->assertOk();
// The single allowed download is now spent. The second archive was
// built while it still was not, and holding it must not be a way to
// take a copy that is no longer allowed.
$this->actingAs($this->client)->get("/zip-downloads/{$secondZip->id}/download")->assertForbidden();
expect($file->downloads()->count())->toBe(1)
->and($secondZip->fresh()->delivered_at)->toBeNull();
});
test('one spent file refuses the whole archive rather than part of it', function () {
$folder = makeFolder('Shared');
$spendable = limitedFile(['name' => 'Spendable', 'original_name' => 'spendable.pdf']);
$uncapped = limitedFile(['name' => 'Uncapped', 'original_name' => 'uncapped.pdf', 'download_limit' => null]);
$spendable->update(['folder_id' => $folder->id]);
$uncapped->update(['folder_id' => $folder->id]);
$staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']);
$response = $this->actingAs($staff)
->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk();
$zip = ZipDownload::query()->findOrFail($response->json('id'));
expect($zip->file_count)->toBe(2);
// Spent after the archive was built, so the copy inside it is one the
// limit no longer covers.
spend($spendable, $staff);
$this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertForbidden();
// Refused as a whole: the file that was still free is not counted as
// downloaded either, since nothing was handed over.
expect($uncapped->downloads()->count())->toBe(0);
});
test('a spent file that is not in the archive does not refuse it', function () {
$folder = makeFolder('Shared');
$bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]);
$bundled->update(['folder_id' => $folder->id]);
$staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']);
$response = $this->actingAs($staff)
->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk();
$zip = ZipDownload::query()->findOrFail($response->json('id'));
// Lands in the folder after the archive was written, and is already
// spent. The selection would resolve to it now; the archive does not
// hold it, so it has no say over handing that archive over.
$late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']);
$late->update(['folder_id' => $folder->id]);
spend($late, $staff);
$this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk();
// Still only the download that spent it — the delivery neither
// refused over it nor counted it.
expect($bundled->downloads()->count())->toBe(1)
->and($late->downloads()->count())->toBe(1);
});
test('an archive from before its contents were recorded is handed over as it always was', function () {
$folder = makeFolder('Shared');
$bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]);
$bundled->update(['folder_id' => $folder->id]);
$staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']);
$response = $this->actingAs($staff)
->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk();
$zip = ZipDownload::query()->findOrFail($response->json('id'));
// Stands in for a row written by an older release, which has no
// record of what went into the archive.
ZipDownload::query()->whereKey($zip->id)->update(['contained_file_ids' => null]);
// A spent file joins the folder afterwards. Resolving the selection
// again — all such a row can do — would sweep it up and refuse over a
// file the archive does not hold, so this path refuses nothing.
$late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']);
$late->update(['folder_id' => $folder->id]);
spend($late, $staff);
$this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk();
expect($bundled->downloads()->count())->toBe(1)
->and($zip->fresh()->delivered_at)->not->toBeNull();
});
test('two fetches of one archive arriving together still count as one delivery', function () {
$file = limitedFile(['download_limit' => null]);
shareFileWith($file, $this->client);
$response = $this->actingAs($this->client)
->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk();
$zip = ZipDownload::query()->findOrFail($response->json('id'));
// Stands in for a second fetch of the same archive that arrives at
// the same moment and claims the delivery first. The request below
// has already read the row by then, so its own copy still says the
// archive has never been handed over — the check-then-set this
// replaced would believe it and count everything a second time.
ZipDownload::retrieved(function (ZipDownload $retrieved): void {
ZipDownload::query()
->whereKey($retrieved->id)
->whereNull('delivered_at')
->update(['delivered_at' => now()]);
});
$this->actingAs($this->client)->get("/zip-downloads/{$zip->id}/download")->assertOk();
// Losing the claim means not logging: the fetch that won it is the
// one that counts, and here that is the stand-in, which logs nothing.
expect($file->downloads()->count())->toBe(0);
});
tests/Feature/Files/ZipDownloadsTest.php
+38
View File
@@ -177,6 +177,44 @@ test('the download endpoint logs a FileDownloaded entry for every bundled file',
->assertHeader('Content-Disposition', 'attachment; filename="Reports.zip"');
expect(ActivityLog::query()->where('action', Action::FileDownloaded)->where('subject_id', $file->id)->exists())->toBeTrue();
expect($zipDownload->fresh()->contained_file_ids)->toBe([$file->id]);
});
test('a file added to the folder after the build is not counted as downloaded', function () {
$folder = Folder::query()->create(['name' => 'Reports']);
$bundled = zipUploadFile($this->admin, 'a.pdf', $folder->id);
$response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk();
$zipDownload = ZipDownload::query()->findOrFail($response->json('id'));
// The archive is written by now. Anything that lands in the folder
// from here on is not in it.
$late = zipUploadFile($this->admin, 'b.pdf');
$late->update(['folder_id' => $folder->id]);
$this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk();
expect(zipEntryNames($zipDownload))->toBe(['Reports/a.pdf'])
->and($bundled->downloads()->count())->toBe(1)
->and($late->downloads()->count())->toBe(0);
});
test('a file moved out of the folder after the build is still counted', function () {
$folder = Folder::query()->create(['name' => 'Reports']);
$elsewhere = Folder::query()->create(['name' => 'Elsewhere']);
$file = zipUploadFile($this->admin, 'a.pdf', $folder->id);
$response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk();
$zipDownload = ZipDownload::query()->findOrFail($response->json('id'));
// Moving it does not take its bytes back out of the archive, so the
// download still hands it over and still has to say so.
$file->update(['folder_id' => $elsewhere->id]);
$this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk();
expect($file->downloads()->count())->toBe(1);
});
test('a pending zip download 404s until ready', function () {