From e9496dc3573e3c46884df730fc1b70d1d5effaf6 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Wed, 16 Sep 2026 14:29:41 -0300 Subject: [PATCH] Give quarantined files a screen, an owner, and somebody to tell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An infected file now goes somewhere rather than nowhere. Staff holding the new release_quarantined_files permission get a Quarantine screen listing what was refused, who uploaded it, and what the scanner called it. They can delete it as they always could, or release it — which needs a written reason, a password confirmation on top of the permission, and lands in the activity log under their name. Only the administrator role holds that permission by default. Deciding a threat report is wrong is a different judgement from deciding a file is no longer needed, which is why it is not delete_files. Two notifications, two audiences: staff who can act on it, and the person who uploaded it — for whom this is how they learn their own machine has something on it. The people the file was shared with are deliberately not told about a file they never received. `projectsend:scan-files` runs hourly: it re-queues files still waiting, and re-scans the ones that went out unscanned while the scanner was unreachable, since it may be back. With --existing it also works through a library uploaded before scanning was switched on, paced by a setting so it does not starve today's uploads. A file that was downloadable before it was caught says so on the screen, with its download count, because that is the case where somebody may already have a copy. --- .../Files/Console/ScanFilesCommand.php | 94 ++++++++++ app/Modules/Files/FilesServiceProvider.php | 22 +++ .../Http/Controllers/QuarantineController.php | 114 ++++++++++++ app/Modules/Files/Models/File.php | 2 + .../Files/Scanning/QuarantineNotifier.php | 71 +++++++ app/Modules/Files/Scanning/ScanPolicy.php | 7 + .../Identity/Permissions/Permission.php | 9 + .../SchedulerMonitoringController.php | 1 + ...090000_add_scan_columns_to_files_table.php | 10 +- resources/js/components/app-sidebar.tsx | 28 +-- resources/js/pages/files/quarantine.tsx | 166 +++++++++++++++++ routes/console.php | 3 + routes/web.php | 11 ++ tests/Feature/Files/QuarantineTest.php | 173 ++++++++++++++++++ .../Platform/SchedulerMonitoringTest.php | 2 +- 15 files changed, 687 insertions(+), 26 deletions(-) create mode 100644 app/Modules/Files/Console/ScanFilesCommand.php create mode 100644 app/Modules/Files/Http/Controllers/QuarantineController.php create mode 100644 app/Modules/Files/Scanning/QuarantineNotifier.php create mode 100644 resources/js/pages/files/quarantine.tsx create mode 100644 tests/Feature/Files/QuarantineTest.php diff --git a/app/Modules/Files/Console/ScanFilesCommand.php b/app/Modules/Files/Console/ScanFilesCommand.php new file mode 100644 index 00000000..f05a5bc3 --- /dev/null +++ b/app/Modules/Files/Console/ScanFilesCommand.php @@ -0,0 +1,94 @@ +enabled()) { + $this->info('Virus scanning is switched off.'); + + return self::SUCCESS; + } + + $waiting = $this->dispatchFor(File::query()->where('scan_status', ScanStatus::Pending)); + + // Allowed through while the scanner was unreachable. Now that it + // may be back, they are asked again — a file found infected at + // this point is quarantined like any other, and its quarantine + // notice says it was available in the meantime. + $missed = $this->dispatchFor( + File::query() + ->where('scan_status', ScanStatus::NotScanned) + ->where('scan_note', NotScannedReason::ScannerUnavailable->value) + ); + + $this->info("Re-queued {$waiting} waiting file(s) and {$missed} that were missed while the scanner was down."); + + if ($this->option('existing')) { + // Paced, because this can be a whole library at once and the + // scanner is also serving today's uploads. An hour's worth per + // run, since that is how often this command runs. + $limit = $config->existingScanRatePerMinute() * 60; + + $old = $this->dispatchFor( + File::query() + ->where('scan_status', ScanStatus::NotScanned) + ->where('scan_note', NotScannedReason::BeforeScanning->value), + $limit, + ); + + $this->info("Queued {$old} file(s) that had never been scanned."); + } + + return self::SUCCESS; + } + + /** + * @param Builder $query + */ + private function dispatchFor(Builder $query, ?int $limit = null): int + { + if ($limit !== null) { + $query->limit($limit); + } + + $ids = $query->orderBy('id')->pluck('id'); + + foreach ($ids as $id) { + // Back to pending first: the job only acts on a pending file, + // which is what stops two runs of this command from scanning + // the same file twice. + File::query()->whereKey($id)->update(['scan_status' => ScanStatus::Pending->value, 'scan_note' => null]); + + ScanFileJob::dispatch((int) $id); + } + + return $ids->count(); + } +} diff --git a/app/Modules/Files/FilesServiceProvider.php b/app/Modules/Files/FilesServiceProvider.php index 559c47e5..bf63f2f5 100644 --- a/app/Modules/Files/FilesServiceProvider.php +++ b/app/Modules/Files/FilesServiceProvider.php @@ -108,6 +108,27 @@ class FilesServiceProvider extends ServiceProvider url: fn (array $data): string => route('my-files.index'), )); + // Two audiences, two types, because they need different words and + // different links. Staff get a queue to act on; the person who + // uploaded gets told their file did not go through. + $registry = $this->app->make(NotificationTypeRegistry::class); + + $registry->register(new NotificationTypeDefinition( + key: 'file_quarantined', + label: 'A file was quarantined by the virus scanner', + template: 'A virus was found in ":itemName", uploaded by :uploaderName', + url: fn (array $data): string => route('files.quarantine'), + )); + + $registry->register(new NotificationTypeDefinition( + key: 'upload_blocked', + label: 'One of your uploads was blocked', + template: 'Your file ":itemName" was blocked: :threat', + // Their own files list. Deliberately not the quarantine + // screen, which they cannot open. + url: fn (array $data): string => route('my-files.index'), + )); + // Every upload path converges on FileWasStored, so this is the // one place a scan is started from. Dispatched rather than run // inline: a 5 GB file takes minutes to read, and an upload must @@ -121,6 +142,7 @@ class FilesServiceProvider extends ServiceProvider if ($this->app->runningInConsole()) { $this->commands([ + Console\ScanFilesCommand::class, Console\PurgeStaleUploadsCommand::class, Console\PurgeZipDownloadsCommand::class, Console\PurgeExpiredFilesCommand::class, diff --git a/app/Modules/Files/Http/Controllers/QuarantineController.php b/app/Modules/Files/Http/Controllers/QuarantineController.php new file mode 100644 index 00000000..e45a98e8 --- /dev/null +++ b/app/Modules/Files/Http/Controllers/QuarantineController.php @@ -0,0 +1,114 @@ +whereIn('scan_status', [ScanStatus::Infected->value, ScanStatus::UnscannableBlocked->value]) + ->with('uploader') + ->orderByDesc('scanned_at') + ->paginate(25) + ->withQueryString(); + + $files->through(fn (File $file): array => [ + 'id' => $file->id, + 'name' => $file->name, + 'original_name' => $file->original_name, + 'size' => $file->size, + 'uploader' => $file->uploader?->name, + // The threat name, or — for a file nothing could open — what + // stopped it being read. + 'threat' => $file->scan_status === ScanStatus::UnscannableBlocked + ? __(NotScannedReason::tryFrom((string) $file->scan_note)?->label() ?? 'Could not be scanned') + : $file->scan_note, + 'status' => $file->scan_status->value, + 'scanned_at' => $file->scanned_at?->toIso8601String(), + // True only for a file that went out unscanned while the + // scanner was unreachable and was caught later — which is the + // one case where somebody may already have a copy. + 'was_available' => $file->scan_was_available, + 'downloads_count' => $file->downloads()->count(), + ]); + + return Inertia::render('files/quarantine', [ + 'files' => $files->items(), + 'pagination' => Pagination::meta($files), + ]); + } + + /** + * Overrule the scanner for one file. + * + * The reason is required and is recorded against the person who gave + * it. A release is not undone by a later scan: the file stays + * released until somebody deletes it, which is the point — an + * administrator who has decided a detection is wrong should not have + * to decide it again every hour. + */ + public function release(Request $request, File $file): RedirectResponse + { + abort_unless($file->scan_status->isQuarantined(), 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, + 'released_at' => now(), + ])->save(); + + $this->activity->log(Action::FileReleased, subject: $file, context: [ + 'reason' => $validated['reason'], + 'threat' => $file->scan_note, + ]); + + // Everything that was waiting on this file — a share email, a new + // version notice — goes out now, exactly as it would have if the + // scan had passed. + $this->availability->markAvailable($file); + + return back()->with('success', __('The file has been released.')); + } +} diff --git a/app/Modules/Files/Models/File.php b/app/Modules/Files/Models/File.php index 878b0abf..9fb0caaf 100644 --- a/app/Modules/Files/Models/File.php +++ b/app/Modules/Files/Models/File.php @@ -46,6 +46,7 @@ use Illuminate\Support\Carbon; * @property Carbon|null $scanned_at * @property string|null $scan_engine * @property int $scan_attempts + * @property bool $scan_was_available * @property int|null $released_by * @property Carbon|null $released_at * @property bool $public @@ -89,6 +90,7 @@ class File extends Model 'scanned_at' => 'datetime', 'released_at' => 'datetime', 'scan_attempts' => 'integer', + 'scan_was_available' => 'boolean', 'commentable' => 'boolean', 'expires_at' => 'datetime', 'download_limit' => 'integer', diff --git a/app/Modules/Files/Scanning/QuarantineNotifier.php b/app/Modules/Files/Scanning/QuarantineNotifier.php new file mode 100644 index 00000000..a06ccc79 --- /dev/null +++ b/app/Modules/Files/Scanning/QuarantineNotifier.php @@ -0,0 +1,71 @@ +uploader; + + $this->notifier->send('file_quarantined', $this->staff(), subject: $file, data: [ + 'itemName' => $file->name, + 'uploaderName' => $uploader->name ?? __('a deleted account'), + 'threat' => $threat, + ]); + + // 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))) { + $this->notifier->send('upload_blocked', [$uploader], subject: $file, data: [ + 'itemName' => $file->name, + 'threat' => $threat, + ]); + } + } + + /** + * @return \Illuminate\Support\Collection + */ + private function staff(): \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)) + ->values(); + } +} diff --git a/app/Modules/Files/Scanning/ScanPolicy.php b/app/Modules/Files/Scanning/ScanPolicy.php index c7ecc4d1..1e542162 100644 --- a/app/Modules/Files/Scanning/ScanPolicy.php +++ b/app/Modules/Files/Scanning/ScanPolicy.php @@ -30,6 +30,7 @@ class ScanPolicy private readonly ScanningConfig $config, private readonly FileAvailability $availability, private readonly ActivityLogger $activity, + private readonly QuarantineNotifier $notifier, ) {} /** @@ -73,6 +74,8 @@ class ScanPolicy { $wasAvailable = $this->availability->isAvailable($file); + $file->forceFill(['scan_was_available' => $wasAvailable])->save(); + $this->settle($file, ScanStatus::Infected, $threat, $engine); $this->purgeRenditions($file); @@ -87,6 +90,8 @@ class ScanPolicy 'was_available' => $wasAvailable, ]); + $this->notifier->quarantined($file, $threat); + return ScanStatus::Infected; } @@ -104,6 +109,8 @@ class ScanPolicy 'was_available' => false, ]); + $this->notifier->quarantined($file, $reason->label()); + return ScanStatus::UnscannableBlocked; } diff --git a/app/Modules/Identity/Permissions/Permission.php b/app/Modules/Identity/Permissions/Permission.php index c70e6a30..60c60a44 100644 --- a/app/Modules/Identity/Permissions/Permission.php +++ b/app/Modules/Identity/Permissions/Permission.php @@ -35,6 +35,13 @@ enum Permission: string // rather than a key nobody can reach. case ModerateComments = 'moderate_comments'; + // Overrule the virus scanner: let a quarantined file out. Its own key + // rather than riding on delete_files, because deciding that a threat + // report is wrong is a different judgement from deciding a file is no + // longer needed — and only the administrator role holds it by + // default. See docs/feature-virus-scanning.md. + case ReleaseQuarantinedFiles = 'release_quarantined_files'; + // Categories case CreateCategories = 'create_categories'; case EditCategories = 'edit_categories'; @@ -95,6 +102,7 @@ enum Permission: string self::ImportOrphans => 'Import orphan files', self::LimitDownloads => 'Limit download counts', self::ModerateComments => 'Moderate comments', + self::ReleaseQuarantinedFiles => 'Release quarantined files', self::CreateCategories => 'Create categories', self::EditCategories => 'Edit categories', self::DeleteCategories => 'Delete categories', @@ -186,6 +194,7 @@ enum Permission: string self::ImportOrphans, self::LimitDownloads, self::ModerateComments => PermissionCategory::Files, + self::ReleaseQuarantinedFiles => PermissionCategory::Files, self::CreateCategories, self::EditCategories, diff --git a/app/Modules/Platform/Http/Controllers/SchedulerMonitoringController.php b/app/Modules/Platform/Http/Controllers/SchedulerMonitoringController.php index d603092a..fbc1d4d2 100644 --- a/app/Modules/Platform/Http/Controllers/SchedulerMonitoringController.php +++ b/app/Modules/Platform/Http/Controllers/SchedulerMonitoringController.php @@ -56,6 +56,7 @@ class SchedulerMonitoringController extends Controller 'projectsend:purge-zip-downloads' => (string) __('Purge zip downloads'), 'projectsend:check-for-updates' => (string) __('Check for updates'), 'projectsend:fetch-news' => (string) __('Fetch dashboard news'), + 'projectsend:scan-files' => (string) __('Scan files for viruses'), 'projectsend:purge-expired-files' => (string) __('Purge expired files'), 'projectsend:purge-orphan-files' => (string) __('Purge orphan files'), 'projectsend:purge-api-request-logs' => (string) __('Purge API request logs'), diff --git a/database/migrations/2026_09_16_090000_add_scan_columns_to_files_table.php b/database/migrations/2026_09_16_090000_add_scan_columns_to_files_table.php index d8202347..117dd2ec 100644 --- a/database/migrations/2026_09_16_090000_add_scan_columns_to_files_table.php +++ b/database/migrations/2026_09_16_090000_add_scan_columns_to_files_table.php @@ -31,6 +31,14 @@ return new class extends Migration $table->string('scan_engine')->nullable()->after('scanned_at'); $table->unsignedInteger('scan_attempts')->default(0)->after('scan_engine'); + // Whether this file could be downloaded before it was + // quarantined — true only for one that went out unscanned + // while the scanner was unreachable and was caught later. + // Recorded on the file because it changes what an + // administrator has to do, and because reconstructing it from + // the activity log afterwards means reading every entry. + $table->boolean('scan_was_available')->default(false)->after('scan_attempts'); + // Who overruled a quarantine, and when. The reason they gave // is in the activity log; this is what the file itself shows. $table->foreignId('released_by')->nullable()->after('scan_attempts')->constrained('users')->nullOnDelete(); @@ -43,7 +51,7 @@ return new class extends Migration Schema::table('files', function (Blueprint $table) { $table->dropConstrainedForeignId('released_by'); $table->dropIndex(['scan_status']); - $table->dropColumn(['scan_status', 'scan_note', 'scanned_at', 'scan_engine', 'scan_attempts', 'released_at']); + $table->dropColumn(['scan_status', 'scan_note', 'scanned_at', 'scan_engine', 'scan_attempts', 'scan_was_available', 'released_at']); }); } }; diff --git a/resources/js/components/app-sidebar.tsx b/resources/js/components/app-sidebar.tsx index a570cbe1..d769292d 100644 --- a/resources/js/components/app-sidebar.tsx +++ b/resources/js/components/app-sidebar.tsx @@ -5,30 +5,7 @@ import { Sidebar, SidebarContent, SidebarFooter, SidebarHeader, SidebarMenu, Sid import { useTranslation } from '@/hooks/use-translation'; import { type NavGroup, type SharedData } from '@/types'; import { Link, usePage } from '@inertiajs/react'; -import { - Activity, - ArrowLeftRight, - BookOpen, - Boxes, - Contact, - Download, - FileCode, - FileText, - FileWarning, - History, - KeyRound, - LayoutGrid, - ListChecks, - MailPlus, - MessageSquare, - Settings, - ShieldCheck, - Tags, - Upload, - UserCheck, - UserPlus, - Users, -} from 'lucide-react'; +import { Activity, ArrowLeftRight, BookOpen, Boxes, Contact, Download, FileCode, FileText, FileWarning, History, KeyRound, LayoutGrid, ListChecks, MailPlus, MessageSquare, Settings, ShieldAlert, ShieldCheck, Tags, Upload, UserCheck, UserPlus, Users } from 'lucide-react'; import AppLogo from './app-logo'; export function AppSidebar() { @@ -87,6 +64,9 @@ export function AppSidebar() { if (can('import_orphans')) { fileItems.push({ title: t('Import orphan files'), url: '/files/orphans', icon: FileWarning }); } + if (can('release_quarantined_files')) { + fileItems.push({ title: t('Quarantine'), url: '/files/quarantine', icon: ShieldAlert }); + } if (can('moderate_comments')) { // Just "Comments" — the old "Comments awaiting approval" wrapped // and pushed its own count badge out of the sidebar, and the screen diff --git a/resources/js/pages/files/quarantine.tsx b/resources/js/pages/files/quarantine.tsx new file mode 100644 index 00000000..4502cd71 --- /dev/null +++ b/resources/js/pages/files/quarantine.tsx @@ -0,0 +1,166 @@ +import { type BreadcrumbItem } from '@/types'; +import { Head, useForm } from '@inertiajs/react'; +import { ShieldAlert } from 'lucide-react'; +import { useState } from 'react'; + +import Heading from '@/components/heading'; +import InputError from '@/components/input-error'; +import { Pagination, PaginationMeta } from '@/components/pagination'; +import { TableShell } from '@/components/table-shell'; +import { Alert, AlertDescription } from '@/components/ui/alert'; +import { Badge } from '@/components/ui/badge'; +import { Button } from '@/components/ui/button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, + DialogTrigger, +} from '@/components/ui/dialog'; +import { Label } from '@/components/ui/label'; +import { Textarea } from '@/components/ui/textarea'; +import { useFormatDate } from '@/hooks/use-format-date'; +import { useTranslation } from '@/hooks/use-translation'; +import AppLayout from '@/layouts/app-layout'; +import { formatBytes } from '@/lib/format-bytes'; + +interface QuarantinedFile { + id: number; + name: string; + original_name: string; + size: number; + uploader: string | null; + threat: string | null; + status: string; + scanned_at: string | null; + /** It could be downloaded before it was flagged — so somebody may already have it. */ + was_available: boolean; + downloads_count: number; +} + +interface QuarantineProps { + files: QuarantinedFile[]; + pagination: PaginationMeta; +} + +function ReleaseDialog({ file }: { file: QuarantinedFile }) { + const { t } = useTranslation(); + const [open, setOpen] = useState(false); + const { data, setData, post, processing, errors, reset } = useForm({ reason: '' }); + + return ( + { + setOpen(next); + if (!next) reset(); + }} + > + + + + + + {t('Release ":name"?', { name: file.name })} + + {t( + 'The scanner reported a threat in this file. Releasing it makes it downloadable again for everyone it was shared with. Only do this if you are sure the report is wrong.', + )} + + + +
+ +