From c15c9c48f8e9327cbdcaf61921ff33f524fbb844 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Thu, 17 Sep 2026 02:48:03 -0300 Subject: [PATCH] Close the gaps an end-to-end and security pass found in virus scanning Run against the dev stack with real ClamAV and queue workers, and a code review looking for ways around the scanner. Quarantine now stays quarantined until somebody releases the file. A rescan only touches files people can download, and changes nothing when the scanner cannot answer or scanning is off. Before, an old infected file rescanned while clamd restarted went through the "allow" policy and became downloadable. The daily missing-files check leaves quarantined files alone, so a storage outage no longer brings one back as a fresh upload. A file longer than clamd's StreamMaxLength is "too large" again. clamd answers and hangs up; the next write raised a warning that became an exception before the answer was read, so the file was recorded as "scanner down" and retried past the unscannable policy. The production compose example gives clamd the settings it needs. On its own defaults an encrypted zip comes back clean. The Test button now sends a password-protected zip and fails when it is called clean, and says when an address answers but is not ClamAV. Saving the settings restarts the queue workers, which kept the old values in memory. New scan runs --all, as its name says, and is refused while scans are queued. A retry scheduled for later no longer counts as a scan in progress. Also: quarantine respects client scope for listing, release and notifications; a zip built before a file was quarantined is refused; public comments and version links skip unavailable files; a client no longer sees their own quarantined or missing upload; a file whose bytes return is scanned at once; clamd listens on IPv6 too, so its container health check passes. --- DOCKER.md | 6 +- .../PublicFileCommentsController.php | 4 + .../Files/Access/StaffLibraryScope.php | 15 +++ .../Console/CheckMissingFilesCommand.php | 25 +++- .../Files/Console/ScanFilesCommand.php | 12 +- .../Http/Controllers/QuarantineController.php | 39 +++++- .../VirusScanningSettingsController.php | 101 +++++++++++++-- .../Controllers/ZipDownloadsController.php | 16 +++ app/Modules/Files/Jobs/ScanFileJob.php | 46 ++++++- app/Modules/Files/Models/File.php | 15 ++- app/Modules/Files/Scanning/ClamAvScanner.php | 82 +++++++++--- .../Files/Scanning/QuarantineNotifier.php | 17 ++- .../Files/Versions/FileVersionLinks.php | 8 +- docker/clamav/clamd.conf | 5 +- docker/production/compose.example.yaml | 29 +++++ lang/ca.json | 6 +- lang/cs.json | 6 +- lang/de.json | 6 +- lang/es.json | 6 +- lang/fr.json | 6 +- lang/id.json | 6 +- lang/it.json | 6 +- lang/ja.json | 6 +- lang/nl.json | 6 +- lang/pl.json | 6 +- lang/pt_BR.json | 6 +- lang/ru.json | 6 +- lang/sw.json | 6 +- lang/tr.json | 6 +- lang/vi.json | 6 +- lang/zh_CN.json | 6 +- tests/Feature/Files/ClamAvScannerTest.php | 119 ++++++++++++++++++ .../Files/FileVersionDisclosureTest.php | 15 +++ tests/Feature/Files/MissingFilesTest.php | 33 +++++ tests/Feature/Files/QuarantineTest.php | 44 +++++++ .../Files/VirusScanningSettingsTest.php | 94 +++++++++++++- tests/Feature/Files/VirusScanningTest.php | 117 ++++++++++++++++- 37 files changed, 860 insertions(+), 78 deletions(-) create mode 100644 tests/Feature/Files/ClamAvScannerTest.php diff --git a/DOCKER.md b/DOCKER.md index 0830f725..9f248396 100644 --- a/DOCKER.md +++ b/DOCKER.md @@ -376,7 +376,11 @@ address. On a brand-new installation you can skip that step: uncomment `PROJECTSEND_SCANNER_DEFAULT_ADDRESS` in the compose file before the first start and the site comes up already pointed at the scanner. It is a starting value, not a lock — the address and the switch stay on that screen. **Test scanner** sends a harmless standard test file and tells you whether it was actually -detected. +detected. It also sends a password-protected zip, and fails if the scanner calls it clean. + +The compose file gives the scanner the settings ProjectSend needs, in the `configs` section at the +bottom. Keep them if you change that file. On its own defaults ClamAV reports an archive it cannot +open as clean, so a password-protected zip would get through unchecked. Two things to know before you turn it on. It needs about **1–1.5 GB of memory**, because the virus definitions are held in memory. And the first start downloads those definitions, which takes a few diff --git a/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php b/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php index 5faa36ab..d8298f9a 100644 --- a/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php +++ b/app/Modules/Comments/Http/Controllers/PublicFileCommentsController.php @@ -126,6 +126,10 @@ class PublicFileCommentsController extends Controller { abort_unless($this->settings->get(Setting::PublicListingSlug) === $publicSlug, 404); abort_unless($file->isEffectivelyPublic() && ! $file->isExpired(), 404); + // Same answer as the file's own public page, which 404s a file + // that is not available: otherwise a pending or quarantined file + // could be discussed, and found to exist, by anybody. + abort_unless($file->scan_status->isAvailable(), 404); abort_unless($this->rules->enabled(), 404); } } diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index c32a6b4e..ca547687 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -192,6 +192,21 @@ class StaffLibraryScope return $query->whereHas('members', fn (Builder $members) => $members->whereIn('users.id', $clientIds)); } + /** + * Whose uploads a staff member may be told about when the file is in + * nobody's library — a quarantined upload, which no client can see, + * so files() never reaches it. Their own and their assigned clients', + * or null when unrestricted. + * + * @return list|null + */ + public function uploaderIds(User $user): ?array + { + $clientIds = $this->assignableClientIds($user); + + return $clientIds === null ? null : [$user->id, ...$clientIds]; + } + public function canAssignClient(User $user, User $client): bool { $ids = $this->assignableClientIds($user); diff --git a/app/Modules/Files/Console/CheckMissingFilesCommand.php b/app/Modules/Files/Console/CheckMissingFilesCommand.php index 482ba5b7..fe769459 100644 --- a/app/Modules/Files/Console/CheckMissingFilesCommand.php +++ b/app/Modules/Files/Console/CheckMissingFilesCommand.php @@ -6,6 +6,7 @@ namespace App\Modules\Files\Console; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; +use App\Modules\Files\Jobs\ScanFileJob; use App\Modules\Files\MissingFileScanner; use App\Modules\Files\Models\File; use App\Modules\Files\Scanning\NotScannedReason; @@ -37,7 +38,20 @@ class CheckMissingFilesCommand extends Command $newlyGone = 0; foreach (array_chunk($gone, 200) as $chunk) { - foreach (File::query()->whereIn('id', $chunk)->where('scan_status', '!=', ScanStatus::Missing)->get() as $file) { + // Not a quarantined file. It is unavailable already, and + // marking it missing would wipe the threat name and then, when + // the bytes came back, send it round as a fresh upload — out + // of quarantine with nobody having released it. + $candidates = File::query() + ->whereIn('id', $chunk) + ->whereNotIn('scan_status', [ + ScanStatus::Missing->value, + ScanStatus::Infected->value, + ScanStatus::UnscannableBlocked->value, + ]) + ->get(); + + foreach ($candidates as $file) { // Stamped like any other verdict: this is the moment the // file was last looked at, and without it a missing file // never appears in the Activity list — which is exactly @@ -63,6 +77,15 @@ class CheckMissingFilesCommand extends Command File::query()->whereIn('id', $chunk)->update($scanning->enabled() ? ['scan_status' => ScanStatus::Pending->value, 'scan_note' => null] : ['scan_status' => ScanStatus::NotScanned->value, 'scan_note' => NotScannedReason::BeforeScanning->value]); + + // Straight to the scanner rather than left for the hourly + // sweep, which kept a file that had come back unavailable for + // up to an hour for no reason. + if ($scanning->enabled()) { + foreach ($chunk as $id) { + ScanFileJob::dispatch($id); + } + } } $this->info(sprintf( diff --git a/app/Modules/Files/Console/ScanFilesCommand.php b/app/Modules/Files/Console/ScanFilesCommand.php index 54c487fb..e41f1b14 100644 --- a/app/Modules/Files/Console/ScanFilesCommand.php +++ b/app/Modules/Files/Console/ScanFilesCommand.php @@ -53,14 +53,16 @@ class ScanFilesCommand extends Command $this->info("Re-queued {$waiting} waiting file(s) and {$missed} that were missed while the scanner was down."); if ($this->option('all')) { - // Everything except the two states there is no point asking - // about: a file already waiting for its first verdict, and one - // whose bytes are not there to read. Files keep their current - // state — and stay downloadable — until a new verdict arrives. + // Every file somebody can have today. Not a file waiting for + // its first verdict, not one with no bytes, and not one in or + // released from quarantine — a scan is not how a file leaves + // quarantine, and a release is not undone by one. Files keep + // their current state, and stay downloadable, until a new + // verdict arrives. $limit = $config->existingScanRatePerMinute() * 60; $checked = $this->dispatchFor( - File::query()->whereNotIn('scan_status', [ScanStatus::Pending->value, ScanStatus::Missing->value]), + File::query()->whereIn('scan_status', ScanFileJob::rescannableValues()), $limit, rescan: true, ); diff --git a/app/Modules/Files/Http/Controllers/QuarantineController.php b/app/Modules/Files/Http/Controllers/QuarantineController.php index e45a98e8..5bd6d3db 100644 --- a/app/Modules/Files/Http/Controllers/QuarantineController.php +++ b/app/Modules/Files/Http/Controllers/QuarantineController.php @@ -5,13 +5,16 @@ declare(strict_types=1); namespace App\Modules\Files\Http\Controllers; use App\Http\Controllers\Controller; +use App\Models\User; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Files\Scanning\FileAvailability; use App\Modules\Files\Scanning\NotScannedReason; use App\Modules\Files\Scanning\ScanStatus; use App\Support\Pagination; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Inertia\Inertia; @@ -36,12 +39,15 @@ class QuarantineController extends Controller public function __construct( private readonly ActivityLogger $activity, private readonly FileAvailability $availability, + private readonly StaffLibraryScope $scope, ) {} public function index(Request $request): Response { - $files = File::query() - ->whereIn('scan_status', [ScanStatus::Infected->value, ScanStatus::UnscannableBlocked->value]) + $user = $request->user(); + assert($user !== null); + + $files = $this->quarantined($user) ->with('uploader') ->orderByDesc('scanned_at') ->paginate(25) @@ -84,15 +90,15 @@ class QuarantineController extends Controller */ public function release(Request $request, File $file): RedirectResponse { - abort_unless($file->scan_status->isQuarantined(), 404); + $actor = $request->user(); + assert($actor !== null); + + abort_unless($this->quarantined($actor)->whereKey($file->id)->exists(), 404); $validated = $request->validate([ 'reason' => ['required', 'string', 'max:500'], ]); - $actor = $request->user(); - assert($actor !== null); - $file->forceFill([ 'scan_status' => ScanStatus::Released, 'released_by' => $actor->id, @@ -111,4 +117,25 @@ class QuarantineController extends Controller return back()->with('success', __('The file has been released.')); } + + /** + * The quarantined files this person may see and release. + * + * A client-scoped staff member gets their own clients' uploads and + * their own, the same boundary as the rest of the library. The + * permission alone let one read every quarantined file on the + * installation, and release a file belonging to a client they could + * not otherwise open. + * + * @return Builder + */ + private function quarantined(User $user): Builder + { + $query = File::query() + ->whereIn('scan_status', [ScanStatus::Infected->value, ScanStatus::UnscannableBlocked->value]); + + $uploaders = $this->scope->uploaderIds($user); + + return $uploaders === null ? $query : $query->whereIn('uploaded_by', $uploaders); + } } diff --git a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php index ede85375..092d94d7 100644 --- a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php +++ b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php @@ -7,6 +7,7 @@ namespace App\Modules\Files\Http\Controllers; use App\Http\Controllers\Controller; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; +use App\Modules\Files\Jobs\ScanFileJob; use App\Modules\Files\Models\File; use App\Modules\Files\Scanning\NotScannedReason; use App\Modules\Files\Scanning\ScannerAddress; @@ -45,6 +46,13 @@ use Inertia\Response; */ class VirusScanningSettingsController extends Controller { + /** + * A zip holding check.txt ("ProjectSend checks that the scanner + * reports encrypted archives."), encrypted with the password + * "projectsend". Made with `zip -P`. + */ + private const ENCRYPTED_ARCHIVE = 'UEsDBBQACQAIAACon1toMefdSgAAAEAAAAAJAAAAY2hlY2sudHh0prvUiniNGfEwEalXOcDbsYylfm2yAcyjplSfHJqk2sSxcVWFx0omz5AvASvSRdDbfeSQ+CC2qu6JEP/NYbBKy+g5t2nJr4swpv9QSwcIaDHn3UoAAABAAAAAUEsBAh4DFAAJAAgAAKifW2gx591KAAAAQAAAAAkAAAAAAAAAAQAAALSBAAAAAGNoZWNrLnR4dFBLBQYAAAAAAQABADcAAACBAAAAAAA='; + public function __construct( private readonly Settings $settings, private readonly ScanningConfig $config, @@ -132,6 +140,13 @@ class VirusScanningSettingsController extends Controller $this->settings->set(Setting::VirusScannerWaitMinutes, (int) $validated['wait_minutes']); $this->settings->set(Setting::VirusScanExistingRatePerMinute, (int) $validated['existing_rate_per_minute']); + // The scans worker is the one process that acts on every setting + // above, and it holds them in memory from the job it started on. + // Without this, switching scanning off or pointing it at another + // scanner changed the screen and nothing else until somebody + // restarted the worker. + Artisan::call('queue:restart'); + $this->activity->log(Action::SettingsUpdated, context: ['section' => 'virus_scanning']); return back(); @@ -191,6 +206,19 @@ class VirusScanningSettingsController extends Controller fclose($stream); if ($verdict->outcome === ScanOutcome::Infected) { + // Detecting is half of it. A clamd left on its own defaults + // answers "OK" for an archive it could not open, and every + // encrypted zip would be recorded as clean — while this test + // passed. So ask it about one. + if ($this->passesEncryptedArchives($scanner)) { + return back()->with('scanner_test_result', [ + 'ok' => false, + 'message' => __(':engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.', [ + 'engine' => $status->engine ?? __('The scanner'), + ]), + ]); + } + return back()->with('scanner_test_result', [ 'ok' => true, 'message' => __('Working. :engine detected the test file as ":threat".', [ @@ -212,17 +240,28 @@ class VirusScanningSettingsController extends Controller } /** - * Queue every file that has never been scanned. + * Check every file people can download again. * - * The work itself is the hourly command's, so this button does not - * hold a request open for a library of any size, and the pace is the - * setting above rather than "as fast as the queue will go". + * The work itself is the command's, so this button does not hold a + * request open for a library of any size, and the pace is the setting + * above rather than "as fast as the queue will go". */ public function scanExisting(): RedirectResponse { abort_unless($this->config->enabled(), 422); - Artisan::queue('projectsend:scan-files', ['--existing' => true]); + // The screen disables the button while a scan is working through + // the queue. Refused here too, because each press queues the whole + // library again, and the throttle alone allows six a minute. + if ($this->scansInQueue() > 0) { + return redirect() + ->route('system-settings.virus-scanning.edit', ['tab' => 'activity']) + ->with('error', __('A scan is already running. Wait for it to finish.')); + } + + // --all rather than --existing: this is "New scan", and on a + // library already scanned once --existing finds nothing to do. + Artisan::queue('projectsend:scan-files', ['--all' => true]); // Onto the tab that shows it happening rather than back where they // were: somebody who just started a scan wants to watch it, and a @@ -258,7 +297,7 @@ class VirusScanningSettingsController extends Controller // out unchecked deliberately leaves it available, so it is not // "pending" and a screen watching only that count says nothing is // happening while the queue works through a whole library. - $queued = Queue::size('scans'); + $queued = $this->scansInQueue(); return response()->json([ // "Something is happening" is the one thing a person watching @@ -287,6 +326,25 @@ class VirusScanningSettingsController extends Controller ]); } + /** + * Scan jobs waiting to run or running now. + * + * Not size(), which counts delayed jobs too. A file held while the + * scanner was down leaves a retry scheduled for up to five minutes + * after the scanner is back and the file already checked, and for + * that long the screen said "Scanning now" and refused a new scan. + */ + private function scansInQueue(): int + { + $queue = Queue::connection(); + + if (method_exists($queue, 'pendingSize') && method_exists($queue, 'reservedSize')) { + return (int) $queue->pendingSize('scans') + (int) $queue->reservedSize('scans'); + } + + return $queue->size('scans'); + } + /** * Whether this installation connects its own scanner. * @@ -333,18 +391,39 @@ class VirusScanningSettingsController extends Controller NotScannedReason::Encrypted->value, ]) ->count(), - // What a New scan would actually check: everything except a - // file already waiting for its first verdict, and one whose - // bytes are gone. + // What a New scan would actually check — see + // ScanFileJob::rescannableValues(). 'scannable' => File::query() - ->whereNotIn('scan_status', [ScanStatus::Pending->value, ScanStatus::Missing->value]) + ->whereIn('scan_status', ScanFileJob::rescannableValues()) ->count(), // So the New scan button can refuse a second scan while one is // still working through the queue. - 'queued' => Queue::size('scans'), + 'queued' => $this->scansInQueue(), ]; } + /** + * Whether the scanner calls a password-protected zip clean. + * + * The archive holds one line of text and nothing else; what matters + * is only that it cannot be opened without the password. A scanner + * set up as documented answers "encrypted". + */ + private function passesEncryptedArchives(VirusScanner $scanner): bool + { + $archive = (string) base64_decode(self::ENCRYPTED_ARCHIVE, true); + + $stream = fopen('php://temp', 'r+'); + assert($stream !== false); + fwrite($stream, $archive); + rewind($stream); + + $verdict = $scanner->scan($stream, strlen($archive)); + fclose($stream); + + return $verdict->outcome === ScanOutcome::Clean; + } + private function eicar(): string { // Assembled rather than written out, so the repository itself diff --git a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php index f6c619b3..ae10f67a 100644 --- a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php +++ b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php @@ -13,6 +13,7 @@ use App\Modules\Files\Access\DownloadAllowance; use App\Modules\Files\Access\ViewableFileScope; use App\Modules\Files\Jobs\BuildZipDownloadJob; use App\Modules\Files\Models\File; +use App\Modules\Files\Scanning\ScanStatus; use App\Modules\Files\Scanning\FileAvailability; use App\Modules\Files\Models\Folder; use App\Modules\Files\Models\ZipDownload; @@ -186,6 +187,21 @@ class ZipDownloadsController extends Controller $path = $zipDownload->path; abort_unless($zipDownload->status === ZipDownload::STATUS_READY && $path !== null, 404); + // The build left out anything not yet available, but a file can be + // quarantined after its archive was built — a rescan with newer + // definitions, say. Nothing can be taken out of a finished zip, so + // the whole archive is refused and a fresh one leaves the file out. + $contained = $zipDownload->contained_file_ids; + + abort_if( + $contained !== null && File::query() + ->whereIn('id', $contained) + ->whereNotIn('scan_status', ScanStatus::availableValues()) + ->exists(), + 423, + __('A file in this archive is no longer available. Download the selection again.'), + ); + // 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) { diff --git a/app/Modules/Files/Jobs/ScanFileJob.php b/app/Modules/Files/Jobs/ScanFileJob.php index 4b4fb9ee..e1a0bb9b 100644 --- a/app/Modules/Files/Jobs/ScanFileJob.php +++ b/app/Modules/Files/Jobs/ScanFileJob.php @@ -90,16 +90,26 @@ class ScanFileJob implements ShouldQueue return; } - // A rescan checks a file again whatever it said last — after new - // definitions, or because somebody asked. The one state it leaves - // alone is a file already waiting for its first verdict, which - // belongs to the job above. - if ($this->rescan && $file->scan_status === ScanStatus::Pending) { + // A rescan asks again about a file people can have today — after + // new definitions, or because somebody asked. Nothing else: + // + // - a file waiting for its first verdict belongs to the job above; + // - a quarantined file leaves quarantine only by being released, + // and a rescan that came back "the scanner is down" or "too + // large" would otherwise have let it out through the policy for + // those answers; + // - a released file stays released (see QuarantineController); + // - a missing file has no bytes to read. + if ($this->rescan && ! self::rescannable($file->scan_status)) { return; } if (! $config->enabled()) { - $policy->markNeverScanned($file); + // A rescan that finds scanning switched off has learned + // nothing, and a file already checked keeps its verdict. + if (! $this->rescan) { + $policy->markNeverScanned($file); + } return; } @@ -121,6 +131,13 @@ class ScanFileJob implements ShouldQueue $verdict = $this->read($file, $scanner); + // Same reasoning for a rescan the scanner could not answer: the + // file keeps the verdict it had. One let through while the scanner + // was down still says so, and the hourly sweep asks again. + if ($this->rescan && $verdict->outcome === ScanOutcome::Unavailable) { + return; + } + if ($verdict->outcome === ScanOutcome::Unavailable && $this->keepWaiting($file, $config)) { $file->forceFill(['scan_attempts' => $file->scan_attempts + 1])->save(); @@ -136,6 +153,23 @@ class ScanFileJob implements ShouldQueue $policy->record($file, $verdict); } + /** + * The states a rescan may act on: the ones a person can download. + * Shared with ScanFilesCommand and the settings screen's count, so + * what "New scan" says it will check is what it checks. + * + * @return list + */ + public static function rescannableValues(): array + { + return [ScanStatus::Clean->value, ScanStatus::NotScanned->value]; + } + + private static function rescannable(ScanStatus $status): bool + { + return in_array($status->value, self::rescannableValues(), true); + } + /** * Whether the file should wait rather than be decided now. * diff --git a/app/Modules/Files/Models/File.php b/app/Modules/Files/Models/File.php index cdf538df..29ff31ff 100644 --- a/app/Modules/Files/Models/File.php +++ b/app/Modules/Files/Models/File.php @@ -295,9 +295,14 @@ class File extends Model * Sits beside notExpired() in every scope that answers "what may this * person be shown", and for the same reason: a file nobody has * checked yet is not a file anybody may be handed. The uploader is - * the exception — their own upload stays on their screen while it is - * being checked, marked as such, because a file that vanishes for ten - * minutes after you send it reads as a failed upload. + * the exception while it is being checked — their own upload stays on + * their screen, because a file that vanishes for ten minutes after + * you send it reads as a failed upload. + * + * Only while it is being checked. A quarantined or missing upload + * stayed listed for its uploader too, with a download button that + * answered with an error page; they are told about a blocked upload + * by notification instead, and there is nothing to offer them here. * * @param Builder $query */ @@ -307,7 +312,9 @@ class File extends Model $inner->whereIn('scan_status', ScanStatus::availableValues()); if ($viewer !== null) { - $inner->orWhere('uploaded_by', $viewer->id); + $inner->orWhere(fn (Builder $own) => $own + ->where('uploaded_by', $viewer->id) + ->where('scan_status', ScanStatus::Pending)); } }); } diff --git a/app/Modules/Files/Scanning/ClamAvScanner.php b/app/Modules/Files/Scanning/ClamAvScanner.php index db7360ba..facd061f 100644 --- a/app/Modules/Files/Scanning/ClamAvScanner.php +++ b/app/Modules/Files/Scanning/ClamAvScanner.php @@ -34,6 +34,9 @@ class ClamAvScanner implements VirusScanner /** 64 KiB — clamd's own read buffer size, and small enough to stream 5 GB without holding it. */ private const CHUNK = 65536; + /** Seconds to wait for an answer to VERSION. */ + private const VERSION_TIMEOUT = 10; + /** * What the daemon said it was, the first time this instance asked. * @@ -72,9 +75,9 @@ class ClamAvScanner implements VirusScanner } try { - fwrite($socket, "zINSTREAM\0"); + $sent = $this->send($socket, "zINSTREAM\0"); - while (! feof($stream)) { + while ($sent && ! feof($stream)) { $chunk = fread($stream, self::CHUNK); if ($chunk === false) { @@ -85,16 +88,20 @@ class ClamAvScanner implements VirusScanner continue; } - // Big-endian length, then the bytes. A short write here - // means clamd hung up mid-stream — usually its own size - // limit — and the reply below says which. - if (fwrite($socket, pack('N', strlen($chunk)).$chunk) === false) { - break; - } + // Big-endian length, then the bytes. + $sent = $this->send($socket, pack('N', strlen($chunk)).$chunk); } - fwrite($socket, pack('N', 0)); + if ($sent) { + $this->send($socket, pack('N', 0)); + } + // Read whether or not every byte went: a write that fails + // means clamd hung up mid-stream, and it only does that after + // saying why — usually its own size limit. Treating the + // failed write as "the scanner is down" instead sent every + // file over that limit round the retry loop forever, and past + // the unscannable policy. $reply = $this->readReply($socket); } catch (Throwable $e) { return ScanVerdict::unavailable($e->getMessage()); @@ -122,14 +129,16 @@ class ClamAvScanner implements VirusScanner return ScannerStatus::unreachable(__(ScannerAddress::message())); } - $socket = $this->connect(); + // A short wait rather than the scan's: VERSION is answered at once + // by anything that is clamd, and the Test button waits on this. + $socket = $this->connect(self::VERSION_TIMEOUT); if ($socket === null) { return ScannerStatus::unreachable(__('No answer from :address.', ['address' => $this->config->address()])); } try { - fwrite($socket, "zVERSION\0"); + $this->send($socket, "zVERSION\0"); $reply = $this->readReply($socket); } catch (Throwable $e) { return ScannerStatus::unreachable($e->getMessage()); @@ -146,6 +155,16 @@ class ClamAvScanner implements VirusScanner // Older builds answer with the engine alone, so every part after // the first is optional rather than assumed. $parts = explode('/', $reply); + + // Anything listening on the port answers something. Without this a + // database or a web server "answered", and the first bytes of its + // greeting were shown as the engine's name. + if (! str_starts_with($parts[0], 'ClamAV ')) { + return ScannerStatus::unreachable(__('Something answered at :address, but it is not a ClamAV scanner.', [ + 'address' => $this->config->address(), + ])); + } + $definitions = isset($parts[1]) && is_numeric(trim($parts[1])) ? (int) trim($parts[1]) : null; $built = null; @@ -162,12 +181,14 @@ class ClamAvScanner implements VirusScanner private function verdictFor(string $reply, ?string $engine): ScanVerdict { - if (str_ends_with($reply, 'OK')) { + // The whole reply, not its last two letters: "clean" is the one + // answer that hands a file out, so nothing else may be read as it. + if ($reply === 'stream: OK') { return ScanVerdict::clean($engine); } // "stream: Win.Test.EICAR_HDB-1 FOUND" - if (str_ends_with($reply, 'FOUND')) { + if (str_starts_with($reply, 'stream: ') && str_ends_with($reply, ' FOUND')) { $threat = trim(str_replace(['stream:', 'FOUND'], '', $reply)); // Not threats: clamd's way of saying "I could not look @@ -223,7 +244,7 @@ class ClamAvScanner implements VirusScanner } /** @return resource|null */ - private function connect(): mixed + private function connect(?int $replyTimeout = null): mixed { $address = $this->config->address(); @@ -245,11 +266,37 @@ class ClamAvScanner implements VirusScanner // Without this a scanner that accepts the connection and then // stops answering holds the worker open indefinitely. - stream_set_timeout($socket, $this->config->replyTimeoutSeconds()); + stream_set_timeout($socket, $replyTimeout ?? $this->config->replyTimeoutSeconds()); return $socket; } + /** + * Write all of it, or say that it could not. + * + * fwrite() may take part of a buffer and return how much, and on a + * connection the other end has closed it raises a warning — which the + * framework's error handler turns into an exception. Silenced and + * checked here instead, so a hang-up reads as a hang-up and the reply + * explaining it can still be read. + * + * @param resource $socket + */ + private function send(mixed $socket, string $bytes): bool + { + while ($bytes !== '') { + $written = @fwrite($socket, $bytes); + + if ($written === false || $written === 0) { + return false; + } + + $bytes = substr($bytes, $written); + } + + return true; + } + /** * clamd's replies end with a NUL in `z` mode. Returns null when the * socket timed out rather than answered. @@ -261,7 +308,10 @@ class ClamAvScanner implements VirusScanner $reply = ''; while (! feof($socket)) { - $byte = fread($socket, 1); + // Silenced for the same reason as send(): a connection clamd + // has reset raises a warning here, and the framework would + // turn that into an exception before the loop could stop. + $byte = @fread($socket, 1); if ($byte === false || $byte === '') { break; diff --git a/app/Modules/Files/Scanning/QuarantineNotifier.php b/app/Modules/Files/Scanning/QuarantineNotifier.php index a06ccc79..9774d443 100644 --- a/app/Modules/Files/Scanning/QuarantineNotifier.php +++ b/app/Modules/Files/Scanning/QuarantineNotifier.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Modules\Files\Scanning; use App\Models\User; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Identity\Permissions\Permission; use App\Modules\Identity\Permissions\PermissionChecker; @@ -33,13 +34,15 @@ class QuarantineNotifier public function __construct( private readonly Notifier $notifier, private readonly PermissionChecker $permissions, + private readonly StaffLibraryScope $scope, ) {} public function quarantined(File $file, string $threat): void { $uploader = $file->uploader; + $staff = $this->staff($file); - $this->notifier->send('file_quarantined', $this->staff(), subject: $file, data: [ + $this->notifier->send('file_quarantined', $staff, subject: $file, data: [ 'itemName' => $file->name, 'uploaderName' => $uploader->name ?? __('a deleted account'), 'threat' => $threat, @@ -48,7 +51,7 @@ class QuarantineNotifier // The uploader hears it once. Without this check a staff member // who uploaded an infected file would get both messages, which // read as two different files. - if ($uploader !== null && ! $this->staff()->contains(fn (User $staff): bool => $staff->is($uploader))) { + if ($uploader !== null && ! $staff->contains(fn (User $member): bool => $member->is($uploader))) { $this->notifier->send('upload_blocked', [$uploader], subject: $file, data: [ 'itemName' => $file->name, 'threat' => $threat, @@ -57,15 +60,23 @@ class QuarantineNotifier } /** + * Staff who can release this file — the permission, and a client + * scope that reaches its uploader (see QuarantineController). + * * @return \Illuminate\Support\Collection */ - private function staff(): \Illuminate\Support\Collection + private function staff(File $file): \Illuminate\Support\Collection { return User::query() ->where('type', UserType::Staff) ->where('active', true) ->get() ->filter(fn (User $staff): bool => $this->permissions->allows($staff, Permission::ReleaseQuarantinedFiles)) + ->filter(function (User $staff) use ($file): bool { + $uploaders = $this->scope->uploaderIds($staff); + + return $uploaders === null || in_array($file->uploaded_by, $uploaders, true); + }) ->values(); } } diff --git a/app/Modules/Files/Versions/FileVersionLinks.php b/app/Modules/Files/Versions/FileVersionLinks.php index 98d40721..3c35f564 100644 --- a/app/Modules/Files/Versions/FileVersionLinks.php +++ b/app/Modules/Files/Versions/FileVersionLinks.php @@ -88,7 +88,7 @@ class FileVersionLinks // than failing anywhere near here. $successors = File::query() ->whereIn('previous_file_id', array_keys($rows)) - ->get(['id', 'name', 'slug', 'previous_file_id', 'public', 'expires_at', 'folder_id']); + ->get(['id', 'name', 'slug', 'previous_file_id', 'public', 'expires_at', 'folder_id', 'scan_status']); /** @var array $candidates */ $candidates = []; @@ -99,7 +99,7 @@ class FileVersionLinks if ($previousIds !== []) { $previous = File::query() ->whereIn('id', array_keys($previousIds)) - ->get(['id', 'name', 'slug', 'previous_file_id', 'public', 'expires_at', 'folder_id']); + ->get(['id', 'name', 'slug', 'previous_file_id', 'public', 'expires_at', 'folder_id', 'scan_status']); foreach ($previous as $file) { $candidates[$file->id] = $file; @@ -145,13 +145,13 @@ class FileVersionLinks } // A guest "sees both files" exactly when both are effectively - // public and unexpired — the same predicate + // public, unexpired and available — the same predicate // PublicGroupsController::showFile 404s on, so the badge can never // point at a page that would refuse to load. if ($viewer === null) { $ids = []; foreach ($candidates as $candidate) { - if ($candidate->isEffectivelyPublic() && ! $candidate->isExpired()) { + if ($candidate->isEffectivelyPublic() && ! $candidate->isExpired() && $candidate->scan_status->isAvailable()) { $ids[] = $candidate->id; } } diff --git a/docker/clamav/clamd.conf b/docker/clamav/clamd.conf index 7f4a838a..5177a484 100644 --- a/docker/clamav/clamd.conf +++ b/docker/clamav/clamd.conf @@ -13,8 +13,11 @@ Foreground yes # Listen for the application on this network only. There is no # authentication in this protocol; see the note in compose.yaml. +# +# No TCPAddr, so clamd listens on every address the container has. The +# image's health check asks "localhost", which resolves to ::1 first, and +# with TCPAddr 0.0.0.0 that check failed forever while clamd worked. TCPSocket 3310 -TCPAddr 0.0.0.0 # The largest stream clamd will accept. ProjectSend refuses anything over # its own "largest file to scan" setting before it gets here, and that diff --git a/docker/production/compose.example.yaml b/docker/production/compose.example.yaml index 1d8b9a07..dcf0d51b 100644 --- a/docker/production/compose.example.yaml +++ b/docker/production/compose.example.yaml @@ -138,6 +138,13 @@ services: # cross that connection in the clear. volumes: - clamav-data:/var/lib/clamav + # ClamAV's own defaults are not enough: left on them, clamd answers + # "OK" for an archive it cannot open, and every password-protected zip + # would be recorded as clean. The settings it needs are at the bottom + # of this file. + configs: + - source: clamd-conf + target: /etc/clamav/clamd.conf profiles: - scanner @@ -146,3 +153,25 @@ volumes: db-data: redis-data: clamav-data: + +configs: + clamd-conf: + content: | + LogTime yes + Foreground yes + TCPSocket 3310 + + # The largest stream clamd accepts. Keep it at or above "Largest file + # to scan" on the settings screen, or larger files are not scanned. + StreamMaxLength 512M + MaxFileSize 512M + MaxScanSize 1024M + MaxRecursion 16 + MaxFiles 10000 + + # Report what could not be opened instead of calling it clean. These + # four are the reason this file exists. + AlertExceedsMax yes + AlertEncrypted yes + AlertEncryptedArchive yes + AlertEncryptedDoc yes diff --git a/lang/ca.json b/lang/ca.json index 27fc4b35..4e406163 100644 --- a/lang/ca.json +++ b/lang/ca.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Per què l'alliberes?", "Working. :engine detected the test file as \":threat\".": "Funciona. :engine ha detectat el fitxer de prova com a «:threat».", "Your file \":itemName\" was blocked: :threat": "El teu fitxer «:itemName» s'ha blocat: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tots els clients ja tenien carpeta.|[1,*]:created de :total clients han rebut carpeta. :existing ja en tenien una." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tots els clients ja tenien carpeta.|[1,*]:created de :total clients han rebut carpeta. :existing ja en tenien una.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine detecta virus, però informa els arxius comprimits xifrats com a nets, així que un zip amb contrasenya passaria sense revisar. Afegeix AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc i AlertExceedsMax, cadascun amb yes, al seu clamd.conf i reinicia'l.", + "A file in this archive is no longer available. Download the selection again.": "Un fitxer d'aquest arxiu ja no està disponible. Torna a descarregar la selecció.", + "A scan is already running. Wait for it to finish.": "Ja hi ha una anàlisi en curs. Espera que acabi.", + "Something answered at :address, but it is not a ClamAV scanner.": "Alguna cosa ha respost a :address, però no és un escàner ClamAV." } diff --git a/lang/cs.json b/lang/cs.json index e966858d..90c13bde 100644 --- a/lang/cs.json +++ b/lang/cs.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Proč ho uvolňuješ?", "Working. :engine detected the test file as \":threat\".": "Funguje. :engine rozpoznal testovací soubor jako „:threat“.", "Your file \":itemName\" was blocked: :threat": "Tvůj soubor „:itemName“ byl zablokován: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Všichni klienti už měli složku.|[1,*]Vytvořeno složek: :created z :total klientů. Už mělo složku: :existing." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Všichni klienti už měli složku.|[1,*]Vytvořeno složek: :created z :total klientů. Už mělo složku: :existing.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine detekuje viry, ale šifrované archivy hlásí jako čisté, takže zip chráněný heslem by prošel bez kontroly. Přidejte AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc a AlertExceedsMax, každé nastavené na yes, do jeho clamd.conf a restartujte ho.", + "A file in this archive is no longer available. Download the selection again.": "Soubor z tohoto archivu už není k dispozici. Stáhněte výběr znovu.", + "A scan is already running. Wait for it to finish.": "Kontrola už probíhá. Počkejte, až skončí.", + "Something answered at :address, but it is not a ClamAV scanner.": "Na adrese :address něco odpovědělo, ale není to skener ClamAV." } diff --git a/lang/de.json b/lang/de.json index e85ca6b8..e0b6766e 100644 --- a/lang/de.json +++ b/lang/de.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Warum gibst du sie frei?", "Working. :engine detected the test file as \":threat\".": "Funktioniert. :engine hat die Testdatei als „:threat“ erkannt.", "Your file \":itemName\" was blocked: :threat": "Deine Datei „:itemName“ wurde blockiert: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Alle Kunden hatten bereits einen Ordner.|[1,*]:created von :total Kunden haben einen Ordner bekommen. :existing hatten bereits einen." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Alle Kunden hatten bereits einen Ordner.|[1,*]:created von :total Kunden haben einen Ordner bekommen. :existing hatten bereits einen.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine erkennt Viren, meldet verschlüsselte Archive aber als sauber, sodass eine passwortgeschützte ZIP-Datei ungeprüft durchkäme. Füge AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc und AlertExceedsMax, jeweils auf yes gesetzt, zu seiner clamd.conf hinzu und starte ihn neu.", + "A file in this archive is no longer available. Download the selection again.": "Eine Datei in diesem Archiv ist nicht mehr verfügbar. Lade die Auswahl erneut herunter.", + "A scan is already running. Wait for it to finish.": "Es läuft bereits ein Scan. Warte, bis er fertig ist.", + "Something answered at :address, but it is not a ClamAV scanner.": "Unter :address hat etwas geantwortet, aber es ist kein ClamAV-Scanner." } diff --git a/lang/es.json b/lang/es.json index cddd3922..44e17268 100644 --- a/lang/es.json +++ b/lang/es.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "¿Por qué lo liberas?", "Working. :engine detected the test file as \":threat\".": "Funciona. :engine detectó el archivo de prueba como «:threat».", "Your file \":itemName\" was blocked: :threat": "Tu archivo «:itemName» fue bloqueado: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Todos los clientes ya tenían carpeta.|[1,*]:created de :total clientes recibieron carpeta. :existing ya tenían una." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Todos los clientes ya tenían carpeta.|[1,*]:created de :total clientes recibieron carpeta. :existing ya tenían una.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine detecta virus, pero informa los archivos comprimidos cifrados como limpios, así que un zip con contraseña pasaría sin revisar. Agrega AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc y AlertExceedsMax, cada uno en yes, a su clamd.conf y reinícialo.", + "A file in this archive is no longer available. Download the selection again.": "Un archivo de este comprimido ya no está disponible. Vuelve a descargar la selección.", + "A scan is already running. Wait for it to finish.": "Ya hay un análisis en curso. Espera a que termine.", + "Something answered at :address, but it is not a ClamAV scanner.": "Algo respondió en :address, pero no es un analizador ClamAV." } diff --git a/lang/fr.json b/lang/fr.json index 862f1a40..beb2ffb9 100644 --- a/lang/fr.json +++ b/lang/fr.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Pourquoi le libères-tu ?", "Working. :engine detected the test file as \":threat\".": "Fonctionne. :engine a détecté le fichier de test comme « :threat ».", "Your file \":itemName\" was blocked: :threat": "Ton fichier « :itemName » a été bloqué : :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tous les clients avaient déjà un dossier.|[1,*]:created clients sur :total ont reçu un dossier. :existing en avaient déjà un." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tous les clients avaient déjà un dossier.|[1,*]:created clients sur :total ont reçu un dossier. :existing en avaient déjà un.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine détecte les virus, mais signale les archives chiffrées comme saines : un zip protégé par mot de passe passerait sans être vérifié. Ajoutez AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc et AlertExceedsMax, chacun à yes, à son clamd.conf puis redémarrez-le.", + "A file in this archive is no longer available. Download the selection again.": "Un fichier de cette archive n'est plus disponible. Téléchargez à nouveau la sélection.", + "A scan is already running. Wait for it to finish.": "Une analyse est déjà en cours. Attendez qu'elle se termine.", + "Something answered at :address, but it is not a ClamAV scanner.": "Quelque chose a répondu à :address, mais ce n'est pas un scanner ClamAV." } diff --git a/lang/id.json b/lang/id.json index e735314b..efd1e686 100644 --- a/lang/id.json +++ b/lang/id.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Mengapa kamu melepaskannya?", "Working. :engine detected the test file as \":threat\".": "Berfungsi. :engine mendeteksi berkas uji sebagai “:threat”.", "Your file \":itemName\" was blocked: :threat": "Berkasmu “:itemName” diblokir: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Semua klien sudah punya folder.|[1,*]:created dari :total klien mendapat folder. :existing sudah punya." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Semua klien sudah punya folder.|[1,*]:created dari :total klien mendapat folder. :existing sudah punya.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine mendeteksi virus, tetapi melaporkan arsip terenkripsi sebagai bersih, sehingga zip berkata sandi akan lolos tanpa diperiksa. Tambahkan AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc dan AlertExceedsMax, masing-masing bernilai yes, ke clamd.conf-nya lalu mulai ulang.", + "A file in this archive is no longer available. Download the selection again.": "Sebuah file dalam arsip ini sudah tidak tersedia. Unduh ulang pilihan tersebut.", + "A scan is already running. Wait for it to finish.": "Pemindaian sudah berjalan. Tunggu hingga selesai.", + "Something answered at :address, but it is not a ClamAV scanner.": "Ada yang menjawab di :address, tetapi itu bukan pemindai ClamAV." } diff --git a/lang/it.json b/lang/it.json index c38ab30b..debce6fb 100644 --- a/lang/it.json +++ b/lang/it.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Perché lo rilasci?", "Working. :engine detected the test file as \":threat\".": "Funziona. :engine ha rilevato il file di test come «:threat».", "Your file \":itemName\" was blocked: :threat": "Il tuo file «:itemName» è stato bloccato: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tutti i clienti avevano già una cartella.|[1,*]:created clienti su :total hanno ricevuto una cartella. :existing ne avevano già una." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tutti i clienti avevano già una cartella.|[1,*]:created clienti su :total hanno ricevuto una cartella. :existing ne avevano già una.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine rileva i virus, ma segnala gli archivi cifrati come puliti, quindi uno zip protetto da password passerebbe senza controlli. Aggiungi AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc e AlertExceedsMax, ciascuno impostato su yes, al suo clamd.conf e riavvialo.", + "A file in this archive is no longer available. Download the selection again.": "Un file di questo archivio non è più disponibile. Scarica di nuovo la selezione.", + "A scan is already running. Wait for it to finish.": "È già in corso una scansione. Attendi che finisca.", + "Something answered at :address, but it is not a ClamAV scanner.": "Qualcosa ha risposto su :address, ma non è uno scanner ClamAV." } diff --git a/lang/ja.json b/lang/ja.json index c9cbe723..aaf885c4 100644 --- a/lang/ja.json +++ b/lang/ja.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "解除する理由は何ですか？", "Working. :engine detected the test file as \":threat\".": "正常です。:engine がテストファイルを「:threat」として検出しました。", "Your file \":itemName\" was blocked: :threat": "ファイル「:itemName」がブロックされました: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}すべてのクライアントにすでにフォルダーがありました。|[1,*]:total 件のうち :created 件のクライアントにフォルダーを作成しました。:existing 件はすでにありました。" + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}すべてのクライアントにすでにフォルダーがありました。|[1,*]:total 件のうち :created 件のクライアントにフォルダーを作成しました。:existing 件はすでにありました。", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine はウイルスを検出しますが、暗号化されたアーカイブをクリーンと報告するため、パスワード付き zip が検査されずに通過します。clamd.conf に AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc と AlertExceedsMax をそれぞれ yes で追加し、再起動してください。", + "A file in this archive is no longer available. Download the selection again.": "このアーカイブ内のファイルは利用できなくなりました。もう一度選択をダウンロードしてください。", + "A scan is already running. Wait for it to finish.": "スキャンはすでに実行中です。終了するまでお待ちください。", + "Something answered at :address, but it is not a ClamAV scanner.": ":address で応答がありましたが、ClamAV スキャナーではありません。" } diff --git a/lang/nl.json b/lang/nl.json index 88835e66..758d5933 100644 --- a/lang/nl.json +++ b/lang/nl.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Waarom geef je het vrij?", "Working. :engine detected the test file as \":threat\".": "Werkt. :engine detecteerde het testbestand als “:threat”.", "Your file \":itemName\" was blocked: :threat": "Je bestand “:itemName” is geblokkeerd: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Alle klanten hadden al een map.|[1,*]:created van :total klanten hebben een map gekregen. :existing hadden er al een." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Alle klanten hadden al een map.|[1,*]:created van :total klanten hebben een map gekregen. :existing hadden er al een.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine detecteert virussen, maar meldt versleutelde archieven als schoon, dus een zip met wachtwoord komt ongecontroleerd door. Voeg AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc en AlertExceedsMax, elk op yes, toe aan zijn clamd.conf en herstart hem.", + "A file in this archive is no longer available. Download the selection again.": "Een bestand in dit archief is niet meer beschikbaar. Download de selectie opnieuw.", + "A scan is already running. Wait for it to finish.": "Er loopt al een scan. Wacht tot die klaar is.", + "Something answered at :address, but it is not a ClamAV scanner.": "Er antwoordde iets op :address, maar het is geen ClamAV-scanner." } diff --git a/lang/pl.json b/lang/pl.json index 1fa5c8d4..6bd42e0e 100644 --- a/lang/pl.json +++ b/lang/pl.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Dlaczego go zwalniasz?", "Working. :engine detected the test file as \":threat\".": "Działa. :engine wykrył plik testowy jako „:threat”.", "Your file \":itemName\" was blocked: :threat": "Twój plik „:itemName” został zablokowany: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Wszyscy klienci mieli już folder.|[1,*]Foldery utworzone: :created z :total klientów. Mieli już folder: :existing." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Wszyscy klienci mieli już folder.|[1,*]Foldery utworzone: :created z :total klientów. Mieli już folder: :existing.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine wykrywa wirusy, ale zgłasza zaszyfrowane archiwa jako czyste, więc zip chroniony hasłem przeszedłby bez sprawdzenia. Dodaj AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc i AlertExceedsMax, każde ustawione na yes, do jego pliku clamd.conf i uruchom go ponownie.", + "A file in this archive is no longer available. Download the selection again.": "Plik z tego archiwum nie jest już dostępny. Pobierz zaznaczenie ponownie.", + "A scan is already running. Wait for it to finish.": "Skanowanie już trwa. Poczekaj, aż się zakończy.", + "Something answered at :address, but it is not a ClamAV scanner.": "Coś odpowiedziało pod adresem :address, ale to nie jest skaner ClamAV." } diff --git a/lang/pt_BR.json b/lang/pt_BR.json index 3f1f4e27..f0941e81 100644 --- a/lang/pt_BR.json +++ b/lang/pt_BR.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Por que você está liberando?", "Working. :engine detected the test file as \":threat\".": "Funcionando. O :engine detectou o arquivo de teste como “:threat”.", "Your file \":itemName\" was blocked: :threat": "Seu arquivo “:itemName” foi bloqueado: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Todos os clientes já tinham pasta.|[1,*]:created de :total clientes receberam pasta. :existing já tinham uma." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Todos os clientes já tinham pasta.|[1,*]:created de :total clientes receberam pasta. :existing já tinham uma.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine detecta vírus, mas informa arquivos compactados criptografados como limpos, então um zip com senha passaria sem verificação. Adicione AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc e AlertExceedsMax, cada um como yes, ao clamd.conf dele e reinicie-o.", + "A file in this archive is no longer available. Download the selection again.": "Um arquivo deste pacote não está mais disponível. Baixe a seleção novamente.", + "A scan is already running. Wait for it to finish.": "Já há uma verificação em andamento. Aguarde até que termine.", + "Something answered at :address, but it is not a ClamAV scanner.": "Algo respondeu em :address, mas não é um scanner ClamAV." } diff --git a/lang/ru.json b/lang/ru.json index a2908635..76993b39 100644 --- a/lang/ru.json +++ b/lang/ru.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Почему вы его выпускаете?", "Working. :engine detected the test file as \":threat\".": "Работает. :engine определил тестовый файл как «:threat».", "Your file \":itemName\" was blocked: :threat": "Ваш файл «:itemName» заблокирован: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}У всех клиентов уже была папка.|[1,*]Создано папок: :created из :total клиентов. Уже имели папку: :existing." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}У всех клиентов уже была папка.|[1,*]Создано папок: :created из :total клиентов. Уже имели папку: :existing.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine обнаруживает вирусы, но сообщает о зашифрованных архивах как о чистых, поэтому zip с паролем пройдёт без проверки. Добавьте AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc и AlertExceedsMax, каждый со значением yes, в его clamd.conf и перезапустите его.", + "A file in this archive is no longer available. Download the selection again.": "Один из файлов этого архива больше недоступен. Скачайте выбранное заново.", + "A scan is already running. Wait for it to finish.": "Проверка уже идёт. Дождитесь её завершения.", + "Something answered at :address, but it is not a ClamAV scanner.": "По адресу :address что-то ответило, но это не сканер ClamAV." } diff --git a/lang/sw.json b/lang/sw.json index 14051fdb..16676b93 100644 --- a/lang/sw.json +++ b/lang/sw.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Kwa nini unaitoa?", "Working. :engine detected the test file as \":threat\".": "Inafanya kazi. :engine ilitambua faili ya majaribio kama “:threat”.", "Your file \":itemName\" was blocked: :threat": "Faili yako “:itemName” imezuiwa: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Kila mteja tayari alikuwa na folda.|[1,*]Wateja :created kati ya :total wamepata folda. :existing tayari walikuwa nazo." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Kila mteja tayari alikuwa na folda.|[1,*]Wateja :created kati ya :total wamepata folda. :existing tayari walikuwa nazo.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine hutambua virusi, lakini huripoti kumbukumbu zilizosimbwa kuwa safi, kwa hiyo zip yenye nenosiri ingepita bila kukaguliwa. Ongeza AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc na AlertExceedsMax, kila moja likiwa yes, kwenye clamd.conf yake kisha uiwashe upya.", + "A file in this archive is no longer available. Download the selection again.": "Faili moja katika kumbukumbu hii halipatikani tena. Pakua uteuzi upya.", + "A scan is already running. Wait for it to finish.": "Uchanganuzi tayari unaendelea. Subiri umalizike.", + "Something answered at :address, but it is not a ClamAV scanner.": "Kitu kimejibu kwenye :address, lakini si kichanganuzi cha ClamAV." } diff --git a/lang/tr.json b/lang/tr.json index d5fb01c9..04966c7a 100644 --- a/lang/tr.json +++ b/lang/tr.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Neden serbest bırakıyorsunuz?", "Working. :engine detected the test file as \":threat\".": "Çalışıyor. :engine test dosyasını “:threat” olarak algıladı.", "Your file \":itemName\" was blocked: :threat": "“:itemName” dosyanız engellendi: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tüm müşterilerin zaten bir klasörü vardı.|[1,*]:total müşteriden :created tanesi klasör aldı. :existing tanesinin zaten vardı." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Tüm müşterilerin zaten bir klasörü vardı.|[1,*]:total müşteriden :created tanesi klasör aldı. :existing tanesinin zaten vardı.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine virüsleri algılıyor ama şifreli arşivleri temiz olarak bildiriyor; parola korumalı bir zip kontrol edilmeden geçer. clamd.conf dosyasına AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc ve AlertExceedsMax seçeneklerini, her birini yes olarak ekleyin ve yeniden başlatın.", + "A file in this archive is no longer available. Download the selection again.": "Bu arşivdeki bir dosya artık kullanılamıyor. Seçimi yeniden indirin.", + "A scan is already running. Wait for it to finish.": "Zaten bir tarama çalışıyor. Bitmesini bekleyin.", + "Something answered at :address, but it is not a ClamAV scanner.": ":address adresinde bir şey yanıt verdi, ancak bu bir ClamAV tarayıcısı değil." } diff --git a/lang/vi.json b/lang/vi.json index 390b7e07..b9e5d0cb 100644 --- a/lang/vi.json +++ b/lang/vi.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "Vì sao bạn giải phóng tệp này?", "Working. :engine detected the test file as \":threat\".": "Hoạt động tốt. :engine đã nhận diện tệp kiểm thử là “:threat”.", "Your file \":itemName\" was blocked: :threat": "Tệp “:itemName” của bạn đã bị chặn: :threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Mọi khách hàng đều đã có thư mục.|[1,*]:created trong số :total khách hàng đã được tạo thư mục. :existing khách hàng vốn đã có." + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}Mọi khách hàng đều đã có thư mục.|[1,*]:created trong số :total khách hàng đã được tạo thư mục. :existing khách hàng vốn đã có.", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine phát hiện được virus nhưng báo các tệp nén được mã hóa là sạch, nên một tệp zip có mật khẩu sẽ lọt qua mà không được kiểm tra. Hãy thêm AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc và AlertExceedsMax, mỗi mục đặt là yes, vào clamd.conf của nó rồi khởi động lại.", + "A file in this archive is no longer available. Download the selection again.": "Một tệp trong gói nén này không còn khả dụng. Hãy tải lại phần đã chọn.", + "A scan is already running. Wait for it to finish.": "Đã có một lượt quét đang chạy. Hãy đợi nó hoàn tất.", + "Something answered at :address, but it is not a ClamAV scanner.": "Có dịch vụ trả lời tại :address, nhưng đó không phải là trình quét ClamAV." } diff --git a/lang/zh_CN.json b/lang/zh_CN.json index b437e24e..35f6ca30 100644 --- a/lang/zh_CN.json +++ b/lang/zh_CN.json @@ -2244,5 +2244,9 @@ "Why are you releasing it?": "解除隔离的原因是什么？", "Working. :engine detected the test file as \":threat\".": "正常。:engine 将测试文件检出为“:threat”。", "Your file \":itemName\" was blocked: :threat": "你的文件“:itemName”被拦截了：:threat", - "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}所有客户都已经有文件夹了。|[1,*]:total 个客户中有 :created 个获得了文件夹，:existing 个原本就有。" + "{0}Every client already had a folder.|[1,*]:created of :total clients got a folder. :existing already had one.": "{0}所有客户都已经有文件夹了。|[1,*]:total 个客户中有 :created 个获得了文件夹，:existing 个原本就有。", + ":engine detects viruses, but reports encrypted archives as clean, so a password-protected zip would get through unchecked. Add AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc and AlertExceedsMax, each set to yes, to its clamd.conf and restart it.": ":engine 能检测病毒，但会将加密压缩包报告为安全，因此带密码的 zip 会未经检查而通过。请在其 clamd.conf 中添加 AlertEncrypted, AlertEncryptedArchive, AlertEncryptedDoc 和 AlertExceedsMax，并各自设为 yes，然后重启。", + "A file in this archive is no longer available. Download the selection again.": "此压缩包中的某个文件已不可用。请重新下载所选内容。", + "A scan is already running. Wait for it to finish.": "已有扫描正在进行。请等待其完成。", + "Something answered at :address, but it is not a ClamAV scanner.": ":address 上有程序应答，但它不是 ClamAV 扫描器。" } diff --git a/tests/Feature/Files/ClamAvScannerTest.php b/tests/Feature/Files/ClamAvScannerTest.php new file mode 100644 index 00000000..f933b324 --- /dev/null +++ b/tests/Feature/Files/ClamAvScannerTest.php @@ -0,0 +1,119 @@ + ['pipe', 'w']], $pipes); + assert(is_resource($process)); + + $address = trim((string) fgets($pipes[1])); + + app(Settings::class)->set(Setting::VirusScanningEnabled, true); + app(Settings::class)->set(Setting::VirusScannerAddress, "tcp://{$address}"); + app(Settings::class)->set(Setting::VirusScanMaxSizeMb, 512); + + return [$process, $pipes[1]]; +} + +/** @param array{0: resource, 1: resource} $server */ +function stopFakeClamd(array $server): void +{ + fclose($server[1]); + proc_terminate($server[0]); + proc_close($server[0]); +} + +test('a stream clamd hangs up on for being too long is too large, not a scanner that is down', function () { + $server = startFakeClamd('limit'); + + // Big enough that the writes are still going when the server closes. + $stream = fopen('php://temp', 'r+'); + fwrite($stream, str_repeat('a', 8 * 1024 * 1024)); + rewind($stream); + + try { + $verdict = app(ClamAvScanner::class)->scan($stream, 8 * 1024 * 1024); + } finally { + fclose($stream); + stopFakeClamd($server); + } + + // Read as "unavailable", this file went round the retry loop and past + // the unscannable policy. + expect($verdict->outcome)->toBe(ScanOutcome::TooLarge); +}); + +test('a service that is not clamd is not reported as a scanner', function () { + $server = startFakeClamd('greeting'); + + try { + $status = app(ClamAvScanner::class)->status(); + } finally { + stopFakeClamd($server); + } + + // It used to be "reachable", with the first bytes of the greeting + // shown as the engine's name. + expect($status->reachable)->toBeFalse() + ->and($status->error)->toContain('not a ClamAV scanner'); +}); diff --git a/tests/Feature/Files/FileVersionDisclosureTest.php b/tests/Feature/Files/FileVersionDisclosureTest.php index c04c5060..bd3cf5af 100644 --- a/tests/Feature/Files/FileVersionDisclosureTest.php +++ b/tests/Feature/Files/FileVersionDisclosureTest.php @@ -169,3 +169,18 @@ test('resolving version links for a page of files does not scale with the row co // not that it is exactly three. expect($queries)->toBeLessThan(10); }); + +test('a guest is not told about a public version that is not available', function () { + $original = File::factory()->public()->create(['uploaded_by' => $this->admin->id, 'name' => 'Rev C']); + $revision = File::factory()->public()->create(['uploaded_by' => $this->admin->id, 'name' => 'Rev D']); + $this->versions->link($revision, $original, $this->admin); + + $links = app(FileVersionLinks::class); + expect($links->for($original, null)['next']['name'] ?? null)->toBe('Rev D'); + + // Still being checked, or quarantined: its public page 404s, so the + // badge would name a file and link to a page that refuses to load. + $revision->forceFill(['scan_status' => App\Modules\Files\Scanning\ScanStatus::Pending])->save(); + + expect($links->for($original->refresh(), null)['next'])->toBeNull(); +}); diff --git a/tests/Feature/Files/MissingFilesTest.php b/tests/Feature/Files/MissingFilesTest.php index d3d986b3..e2c2754d 100644 --- a/tests/Feature/Files/MissingFilesTest.php +++ b/tests/Feature/Files/MissingFilesTest.php @@ -72,11 +72,44 @@ test('a file that comes back is checked again rather than left for dead', functi app(Settings::class)->set(Setting::VirusScanningEnabled, true); app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); + Illuminate\Support\Facades\Queue::fake(); $this->artisan('projectsend:check-missing-files')->assertSuccessful(); // Back to the start: nothing here knows what the scanner had decided // about bytes that have since been away. expect($file->refresh()->scan_status)->toBe(ScanStatus::Pending); + + // And asked about now, rather than at the next hourly sweep — until + // then nobody can have it. + Illuminate\Support\Facades\Queue::assertPushed( + App\Modules\Files\Jobs\ScanFileJob::class, + fn (App\Modules\Files\Jobs\ScanFileJob $job): bool => $job->fileId === $file->id && $job->rescan === false, + ); +}); + +test('a quarantined file whose bytes vanish stays quarantined, and does not come back as a new upload', function () { + app(Settings::class)->set(Setting::VirusScanningEnabled, true); + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); + + $file = fileWithBytes(['scan_status' => ScanStatus::Infected, 'scan_note' => 'Eicar-Test-Signature']); + $path = $file->path; + + // A storage outage: the bytes are away for a day, then back. + Storage::disk('files')->delete($path); + $this->artisan('projectsend:check-missing-files')->assertSuccessful(); + + Storage::disk('files')->put($path, 'some bytes'); + Illuminate\Support\Facades\Queue::fake(); + $this->artisan('projectsend:check-missing-files')->assertSuccessful(); + + // Marked missing, it lost its threat name, and coming back made it a + // pending upload — out of quarantine, a scanner outage away from + // being let through, with nobody having released it. + $file->refresh(); + expect($file->scan_status)->toBe(ScanStatus::Infected) + ->and($file->scan_note)->toBe('Eicar-Test-Signature'); + + Illuminate\Support\Facades\Queue::assertNothingPushed(); }); test('a file that comes back on an installation with no scanner is simply available again', function () { diff --git a/tests/Feature/Files/QuarantineTest.php b/tests/Feature/Files/QuarantineTest.php index 84df10c9..adca1872 100644 --- a/tests/Feature/Files/QuarantineTest.php +++ b/tests/Feature/Files/QuarantineTest.php @@ -171,3 +171,47 @@ test('a staff member who uploaded it is told once, as staff', function () { expect(InAppNotification::query()->where('user_id', $this->admin->id)->count())->toBe(1); }); + +/* +|-------------------------------------------------------------------------- +| A client-scoped staff member +|-------------------------------------------------------------------------- +*/ + +test('a client-scoped staff member sees, releases and hears about only their own clients\' files', function () { + $role = Role::query()->create(['name' => 'Reps '.Str::random(4), 'client_scoped' => true]); + RolePermission::query()->create(['role_id' => $role->id, 'permission' => Permission::ReleaseQuarantinedFiles->value]); + $rep = User::factory()->create(['role_id' => $role->id]); + + $mine = User::factory()->client()->create(); + $stranger = User::factory()->client()->create(); + $rep->assignedClients()->sync([$mine->id]); + + $ours = quarantined(['name' => 'Ours', 'uploaded_by' => $mine->id]); + $theirs = quarantined(['name' => 'Theirs', 'uploaded_by' => $stranger->id]); + + // The permission alone showed every quarantined file on the + // installation, and released one from a client this person could not + // otherwise open. + $this->actingAs($rep)->get('/files/quarantine')->assertInertia( + fn (AssertableInertia $page) => $page->has('files', 1)->where('files.0.name', 'Ours'), + ); + + confirmPassword($rep); + $this->actingAs($rep)->post("/files/{$theirs->id}/release", ['reason' => 'not mine'])->assertNotFound(); + expect($theirs->refresh()->scan_status)->toBe(ScanStatus::Infected); + + $this->actingAs($rep)->post("/files/{$ours->id}/release", ['reason' => 'false positive'])->assertSessionHasNoErrors(); + expect($ours->refresh()->scan_status)->toBe(ScanStatus::Released); + + $policy = app(App\Modules\Files\Scanning\ScanPolicy::class); + $newTheirs = quarantined(['scan_status' => ScanStatus::Pending, 'scan_note' => null, 'uploaded_by' => $stranger->id]); + $newOurs = quarantined(['scan_status' => ScanStatus::Pending, 'scan_note' => null, 'uploaded_by' => $mine->id]); + $policy->record($newTheirs, ScanVerdict::infected('Eicar-Test-Signature')); + $policy->record($newOurs, ScanVerdict::infected('Eicar-Test-Signature')); + + expect(InAppNotification::query()->where('user_id', $rep->id)->pluck('subject_id')->all())->toBe([$newOurs->id]) + // An unscoped administrator still hears about both. + ->and(InAppNotification::query()->where('user_id', $this->admin->id)->count())->toBe(2); +}); + diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index dbae8b49..21f62aba 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -174,8 +174,76 @@ test('the test button sends the standard test file, not an empty stream', functi $this->actingAs($this->admin)->post('/system/settings/virus-scanning/test'); - expect($scanner->scans)->toBe(1) - ->and($scanner->sizes[0])->toBe(68); + // Then the encrypted archive, once the test file was detected. + expect($scanner->scans)->toBe(2) + ->and($scanner->sizes[0])->toBe(68) + ->and($scanner->sizes[1])->toBe(206); +}); + +test('the test button fails a scanner that calls an encrypted archive clean', function () { + // clamd on its own defaults: it detects the test file, and answers + // "OK" for an archive it cannot open. Every password-protected zip + // would have been recorded as clean while this button said "Working". + $scanner = (new FakeVirusScanner)->willAnswer(ScanVerdict::infected('Eicar-Test-Signature'), ScanVerdict::clean('FakeAV 1.0')); + app()->instance(VirusScanner::class, $scanner); + + $this->actingAs($this->admin)->post('/system/settings/virus-scanning/test') + ->assertSessionHas('scanner_test_result', fn (array $result): bool => $result['ok'] === false + && str_contains($result['message'], 'AlertEncryptedArchive')); +}); + +test('saving tells the queue workers to pick the new settings up', function () { + // The scans worker holds settings in memory from the job it started + // on. Pointing it at another scanner, or switching scanning off, + // changed the screen and nothing else until it was restarted. + Illuminate\Support\Facades\Cache::forget('illuminate:queue:restart'); + + $this->actingAs($this->admin)->patch('/system/settings/virus-scanning', [ + 'enabled' => true, + 'address' => 'tcp://clamav:3310', + 'max_size_mb' => 512, + 'unscannable_policy' => 'allow', + 'scanner_down_policy' => 'allow', + 'wait_minutes' => 10, + 'existing_rate_per_minute' => 60, + ])->assertSessionHasNoErrors(); + + expect(Illuminate\Support\Facades\Cache::get('illuminate:queue:restart'))->not->toBeNull(); +}); + +/** The arguments a queued artisan command was queued with. */ +function queuedCommandArguments(Illuminate\Foundation\Console\QueuedCommand $job): array +{ + return (fn (): array => $this->data)->call($job); +} + +test('new scan checks the library again, not only what was never scanned', function () { + app(Settings::class)->set(Setting::VirusScanningEnabled, true); + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); + Illuminate\Support\Facades\Queue::fake(); + + $this->actingAs($this->admin)->post('/system/settings/virus-scanning/scan-existing'); + + // --existing, which it ran before, finds nothing on a library that + // was scanned once: the button was enabled and did nothing. + Illuminate\Support\Facades\Queue::assertPushed( + Illuminate\Foundation\Console\QueuedCommand::class, + fn ($job): bool => queuedCommandArguments($job) === ['projectsend:scan-files', ['--all' => true]], + ); +}); + +test('new scan is refused while a scan is still working through the queue', function () { + app(Settings::class)->set(Setting::VirusScanningEnabled, true); + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); + Illuminate\Support\Facades\Queue::fake(); + App\Modules\Files\Jobs\ScanFileJob::dispatch(1, true); + + $this->actingAs($this->admin)->post('/system/settings/virus-scanning/scan-existing') + ->assertSessionHas('error'); + + // Each press queued the whole library again; only the screen stopped + // a second one. + Illuminate\Support\Facades\Queue::assertNotPushed(Illuminate\Foundation\Console\QueuedCommand::class); }); test('only somebody who can edit settings may test or save', function () { @@ -532,3 +600,25 @@ test('the addresses people actually type are accepted', function () { expect(app(Settings::class)->get(Setting::VirusScannerAddress))->toBe($address); } }); + +test('a retry scheduled for later is not a scan in progress', function () { + // What a scanner outage leaves behind: a held file's next attempt, + // minutes away. Counted, it showed "Scanning now" and refused a new + // scan while nothing at all was running. + config(['queue.default' => 'database']); + app(Settings::class)->set(Setting::VirusScanningEnabled, true); + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://scanner.test:3310'); + + App\Modules\Files\Jobs\ScanFileJob::dispatch(1)->delay(now()->addMinutes(5)); + + $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity') + ->assertJsonPath('queued', 0) + ->assertJsonPath('running', false); + + App\Modules\Files\Jobs\ScanFileJob::dispatch(1); + + $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity') + ->assertJsonPath('queued', 1) + ->assertJsonPath('running', true); +}); + diff --git a/tests/Feature/Files/VirusScanningTest.php b/tests/Feature/Files/VirusScanningTest.php index 25005d50..7f408fbb 100644 --- a/tests/Feature/Files/VirusScanningTest.php +++ b/tests/Feature/Files/VirusScanningTest.php @@ -528,6 +528,9 @@ test('a new scan checks files that already have a verdict', function () { ]); $waiting = scannableFile(['scan_status' => ScanStatus::Pending]); $gone = scannableFile(['scan_status' => ScanStatus::Missing]); + $infected = scannableFile(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + $blocked = scannableFile(['scan_status' => ScanStatus::UnscannableBlocked, 'scan_note' => NotScannedReason::Encrypted->value]); + $released = scannableFile(['scan_status' => ScanStatus::Released, 'scan_note' => 'X']); Illuminate\Support\Facades\Queue::fake(); @@ -537,8 +540,12 @@ test('a new scan checks files that already have a verdict', function () { Illuminate\Support\Facades\Queue::assertPushed(ScanFileJob::class, fn (ScanFileJob $job): bool => $job->fileId === $file->id); } - // Nothing to re-ask about a file with no bytes. - Illuminate\Support\Facades\Queue::assertNotPushed(ScanFileJob::class, fn (ScanFileJob $job): bool => $job->fileId === $gone->id); + // Nothing to re-ask about a file with no bytes. Nor about one in + // quarantine, which leaves it by being released and not by a scan, or + // one somebody released, which a scan does not undo. + foreach ([$gone, $infected, $blocked, $released] as $file) { + Illuminate\Support\Facades\Queue::assertNotPushed(ScanFileJob::class, fn (ScanFileJob $job): bool => $job->fileId === $file->id); + } // The one already waiting is picked up by the ordinary sweep at the // top of the command, as a first scan rather than as a rescan — the @@ -621,3 +628,109 @@ test('the editor offers no download for a file it cannot produce', function () { fn (Inertia\Testing\AssertableInertia $page) => $page->where('file.scan_available', true), ); }); + +/* +|-------------------------------------------------------------------------- +| Nothing but a release takes a file out of quarantine +|-------------------------------------------------------------------------- +*/ + +/** Run the job as a rescan, the way New scan and the hourly sweep queue it. */ +function runRescan(File $file): void +{ + (new ScanFileJob($file->id, true))->handle( + app(VirusScanner::class), + app(App\Modules\Files\Scanning\ScanPolicy::class), + app(App\Modules\Files\Scanning\ScanningConfig::class), + ); +} + +test('a rescan leaves a quarantined file in quarantine, whatever the scanner says', function () { + // The case that got out: an old infected file rescanned while clamd + // was restarting. Its wait window had long passed, so "unavailable" + // went straight to the allow policy and the file became downloadable. + foreach ([ + ScanVerdict::unavailable('down'), + ScanVerdict::tooLarge(), + ScanVerdict::encrypted(), + ScanVerdict::clean(), + ] as $verdict) { + foreach ([ScanStatus::Infected, ScanStatus::UnscannableBlocked, ScanStatus::Released] as $state) { + $scanner = fakeScanner($verdict); + $file = scannableFile(['scan_status' => $state, 'scan_note' => 'Eicar-Test-Signature', 'created_at' => now()->subYear()]); + + runRescan($file); + + expect($file->refresh()->scan_status)->toBe($state, "{$state->value} after {$verdict->outcome->name}") + ->and($scanner->scans)->toBe(0); + } + } +}); + +test('a rescan the scanner cannot answer leaves the file as it was', function () { + fakeScanner(ScanVerdict::unavailable('down')); + $file = scannableFile(['scan_status' => ScanStatus::Clean, 'created_at' => now()->subYear()]); + + runRescan($file); + + expect($file->refresh()->scan_status)->toBe(ScanStatus::Clean); +}); + +test('a rescan that finds scanning switched off changes nothing', function () { + app(Settings::class)->set(Setting::VirusScanningEnabled, false); + $scanner = fakeScanner(ScanVerdict::clean()); + $clean = scannableFile(['scan_status' => ScanStatus::Clean]); + $infected = scannableFile(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + + runRescan($clean); + runRescan($infected); + + expect($clean->refresh()->scan_status)->toBe(ScanStatus::Clean) + ->and($infected->refresh()->scan_status)->toBe(ScanStatus::Infected) + ->and($scanner->scans)->toBe(0); +}); + +test('a client does not see their own upload once it is quarantined or gone', function () { + // Listed, it offered a download that answered with an error page. The + // uploader hears about a blocked upload by notification. + $client = User::factory()->client()->create(); + $waiting = scannableFile(['uploaded_by' => $client->id]); + scannableFile(['uploaded_by' => $client->id, 'scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + scannableFile(['uploaded_by' => $client->id, 'scan_status' => ScanStatus::UnscannableBlocked]); + scannableFile(['uploaded_by' => $client->id, 'scan_status' => ScanStatus::Missing]); + + expect(File::query()->visibleToClient($client)->pluck('id')->all())->toBe([$waiting->id]); +}); + +test('an archive built before one of its files was quarantined is refused', function () { + $file = scannableFile(['uploaded_by' => $this->admin->id, 'scan_status' => ScanStatus::Clean]); + + $zipId = $this->actingAs($this->admin)->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk()->json('id'); + $this->actingAs($this->admin)->getJson("/zip-downloads/{$zipId}")->assertJsonPath('status', 'ready'); + + // A rescan with newer definitions, after the zip was ready. + $file->forceFill(['scan_status' => ScanStatus::Infected, 'scan_note' => 'Newly.Known'])->save(); + + $this->actingAs($this->admin)->get("/zip-downloads/{$zipId}/download")->assertStatus(423); +}); + +test('nobody can comment on a public file that is not available', function () { + app(Settings::class)->set(Setting::PublicListingEnabled, true); + app(Settings::class)->set(Setting::PublicListingSlug, 'public'); + app(Settings::class)->set(Setting::CommentsScope, 'all'); + app(Settings::class)->set(Setting::CommentsAuthors, 'everyone'); + app(Settings::class)->set(Setting::PublicCommentsEnabled, true); + app(Settings::class)->set(Setting::CaptchaProvider, 'none'); + + $group = App\Modules\Groups\Models\Group::query()->create(['name' => 'Showcase', 'public' => true]); + $file = File::factory()->public()->create(['uploaded_by' => $this->admin->id, 'scan_status' => ScanStatus::Clean]); + shareFileWithGroup($file, $group); + + $this->getJson("/public/files/{$file->slug}/comments")->assertOk(); + + $file->forceFill(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X'])->save(); + + // Same answer as the file's own public page, which does not exist for + // a file nobody can have. + $this->getJson("/public/files/{$file->slug}/comments")->assertNotFound(); +});