diff --git a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php index bcd845fd..74fd9542 100644 --- a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php +++ b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php @@ -17,8 +17,10 @@ use App\Modules\Platform\Capabilities\Capability; use App\Modules\Platform\Capabilities\CapabilityRegistry; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; +use Illuminate\Http\JsonResponse; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Queue; use Illuminate\Validation\Rule; use Inertia\Inertia; use Inertia\Response; @@ -56,7 +58,9 @@ class VirusScanningSettingsController extends Controller // scanner is set up, the other is revisited — and a single // column of fields with two Save buttons reads as one form // that saves half of itself. - 'tab' => $request->query('tab') === 'options' ? 'options' : 'scanner', + 'tab' => in_array($request->query('tab'), ['options', 'activity'], true) + ? (string) $request->query('tab') + : 'scanner', // Read from the session here rather than shared as a flash // prop: HandleInertiaRequests shares `success` and `error` and // nothing else, which is why the Test button appeared to do @@ -189,6 +193,61 @@ class VirusScanningSettingsController extends Controller return back()->with('success', __('Scanning existing files has started. It runs in the background.')); } + /** + * What the scanner is doing right now, and what it last decided. + * + * Polled by the Activity tab rather than rendered with the page: a + * backfill takes minutes to hours, and a screen that only tells you + * where things stood when you opened it is the screen somebody + * reloads repeatedly instead of watching. + * + * JSON rather than an Inertia partial, the way the notification bell + * and the zip builder already poll — see use-notification-poll.ts. + */ + public function activity(): JsonResponse + { + $recent = File::query() + ->whereNotNull('scanned_at') + ->orderByDesc('scanned_at') + ->limit(20) + ->get(['id', 'name', 'scan_status', 'scan_note', 'scanned_at', 'scan_engine']); + + $waiting = File::query()->where('scan_status', ScanStatus::Pending)->count(); + + // Counted as well as the files above, and this is the half that + // makes a backfill visible: re-scanning a file that already went + // 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'); + + return response()->json([ + // "Something is happening" is the one thing a person watching + // this screen wants to know, and it is worth being explicit + // about rather than left to be inferred from a count. + 'running' => $waiting > 0 || $queued > 0, + 'waiting' => $waiting, + 'queued' => $queued, + 'checked_last_hour' => File::query()->where('scanned_at', '>=', now()->subHour())->count(), + 'last_scanned_at' => $recent->first()?->scanned_at?->toIso8601String(), + 'never_scanned' => File::query()->neverScanned()->count(), + 'quarantined' => File::query()->whereIn('scan_status', [ + ScanStatus::Infected->value, + ScanStatus::UnscannableBlocked->value, + ])->count(), + 'recent' => $recent->map(fn (File $file): array => [ + 'id' => $file->id, + 'name' => $file->name, + 'status' => $file->scan_status->value, + // A reason is a key and is translated; a threat name is + // the scanner's own words and is passed through. + 'note' => $this->noteFor($file), + 'scanned_at' => $file->scanned_at?->toIso8601String(), + 'engine' => $file->scan_engine, + ])->all(), + ]); + } + /** * Whether this installation connects its own scanner. * @@ -200,6 +259,21 @@ class VirusScanningSettingsController extends Controller return $this->capabilities->has(Capability::VirusScanningConnect); } + private function noteFor(File $file): ?string + { + $note = $file->scan_note; + + if ($note === null) { + return $file->scan_status === ScanStatus::NotScanned + ? (string) __(NotScannedReason::BeforeScanning->label()) + : null; + } + + $reason = NotScannedReason::tryFrom($note); + + return $reason === null ? $note : (string) __($reason->label()); + } + /** * @return array */ diff --git a/app/Modules/Files/Jobs/ScanFileJob.php b/app/Modules/Files/Jobs/ScanFileJob.php index d6e30f3a..c512e34f 100644 --- a/app/Modules/Files/Jobs/ScanFileJob.php +++ b/app/Modules/Files/Jobs/ScanFileJob.php @@ -160,11 +160,12 @@ class ScanFileJob implements ShouldQueue } if ($stream === null) { - // Not the scanner's fault and not a verdict about the file: - // treated as "could not be checked", so the installation's - // own policy decides, rather than calling a file nobody read - // clean. - return ScanVerdict::unavailable(__('The file could not be read from storage.')); + // Not the scanner's fault, and not something waiting will fix + // — an orphaned row, or storage that moved. It goes through + // the same policy as a file the scanner could not open, and + // deliberately not through the scanner-unavailable path, + // which is retried hourly and would retry this forever. + return ScanVerdict::unreadable(__('The file could not be read from storage.')); } try { diff --git a/app/Modules/Files/Scanning/NotScannedReason.php b/app/Modules/Files/Scanning/NotScannedReason.php index cc694466..bb8c4c9a 100644 --- a/app/Modules/Files/Scanning/NotScannedReason.php +++ b/app/Modules/Files/Scanning/NotScannedReason.php @@ -25,6 +25,15 @@ enum NotScannedReason: string /** Uploaded before scanning was switched on, or while it is off. */ case BeforeScanning = 'before_scanning'; + /** + * The bytes were not there to read — an orphaned row, or storage that + * has moved. Its own reason rather than "the scanner could not be + * reached", which is what it used to say: that reading is both wrong + * on screen and wrong in behaviour, because the hourly sweep retries + * an unreachable scanner and would have retried these forever. + */ + case Unreadable = 'unreadable'; + public function label(): string { return match ($this) { @@ -32,6 +41,7 @@ enum NotScannedReason: string self::Encrypted => 'Encrypted, so it could not be scanned', self::ScannerUnavailable => 'The scanner could not be reached', self::BeforeScanning => 'Uploaded before virus scanning was switched on', + self::Unreadable => 'The file itself could not be read from storage', }; } } diff --git a/app/Modules/Files/Scanning/ScanOutcome.php b/app/Modules/Files/Scanning/ScanOutcome.php index 68973fef..d36b1990 100644 --- a/app/Modules/Files/Scanning/ScanOutcome.php +++ b/app/Modules/Files/Scanning/ScanOutcome.php @@ -10,5 +10,8 @@ enum ScanOutcome case Infected; case TooLarge; case Encrypted; + /** The file's own bytes could not be read. Nothing to do with the scanner. */ + case Unreadable; + case Unavailable; } diff --git a/app/Modules/Files/Scanning/ScanPolicy.php b/app/Modules/Files/Scanning/ScanPolicy.php index 45f8187e..6b0c4f99 100644 --- a/app/Modules/Files/Scanning/ScanPolicy.php +++ b/app/Modules/Files/Scanning/ScanPolicy.php @@ -47,6 +47,7 @@ class ScanPolicy ScanOutcome::Infected => $this->quarantine($file, $verdict->detail ?? 'unknown', $verdict->engine), ScanOutcome::TooLarge => $this->unscannable($file, NotScannedReason::TooLarge, $verdict->engine), ScanOutcome::Encrypted => $this->unscannable($file, NotScannedReason::Encrypted, $verdict->engine), + ScanOutcome::Unreadable => $this->unscannable($file, NotScannedReason::Unreadable, $verdict->engine), ScanOutcome::Unavailable => $this->unavailable($file, $verdict->detail), }; } diff --git a/app/Modules/Files/Scanning/ScanVerdict.php b/app/Modules/Files/Scanning/ScanVerdict.php index ed6e2e2b..2dd7fba3 100644 --- a/app/Modules/Files/Scanning/ScanVerdict.php +++ b/app/Modules/Files/Scanning/ScanVerdict.php @@ -43,6 +43,12 @@ final class ScanVerdict return new self(ScanOutcome::Encrypted, null, $engine); } + /** The file could not be read, so nothing was scanned. */ + public static function unreadable(string $reason): self + { + return new self(ScanOutcome::Unreadable, $reason); + } + /** The scanner could not be reached, or did not answer in time. */ public static function unavailable(string $reason): self { diff --git a/resources/js/components/virus-scan-activity.tsx b/resources/js/components/virus-scan-activity.tsx new file mode 100644 index 00000000..4d094d50 --- /dev/null +++ b/resources/js/components/virus-scan-activity.tsx @@ -0,0 +1,189 @@ +import { router } from '@inertiajs/react'; +import { CheckCircle2, Loader2, ShieldAlert, ShieldQuestion } from 'lucide-react'; +import { useEffect, useState } from 'react'; + +import HeadingSmall from '@/components/heading-small'; +import { TableShell } from '@/components/table-shell'; +import { Badge } from '@/components/ui/badge'; +import { useFormatDate } from '@/hooks/use-format-date'; +import { useTranslation } from '@/hooks/use-translation'; + +/** Often enough to feel live, rarely enough to be a read every few seconds. */ +const POLL_INTERVAL_MS = 4000; + +interface ScannedFile { + id: number; + name: string; + status: string; + note: string | null; + scanned_at: string | null; + engine: string | null; +} + +interface Activity { + running: boolean; + /** New uploads withheld until they are checked. */ + waiting: number; + /** Jobs on the scans queue — a backfill lives here, not in `waiting`. */ + queued: number; + checked_last_hour: number; + last_scanned_at: string | null; + never_scanned: number; + quarantined: number; + recent: ScannedFile[]; +} + +/** + * What the scanner is doing, refreshed while somebody is watching. + * + * A backfill runs for minutes or hours in a queue worker, where nothing + * about it is visible: this is the only place it can be watched. When + * nothing is running the same list is the record of what was decided + * last, which is what a person opening this tab after the fact is + * looking for. + */ +export function VirusScanActivity() { + const { t } = useTranslation(); + const { dateTime } = useFormatDate(); + const [activity, setActivity] = useState(null); + const [failed, setFailed] = useState(false); + + useEffect(() => { + let stopped = false; + + const poll = () => { + fetch(route('system-settings.virus-scanning.activity'), { + credentials: 'same-origin', + headers: { Accept: 'application/json' }, + }) + .then((r) => r.json()) + .then((body: Activity) => { + if (!stopped) { + setActivity(body); + setFailed(false); + } + }) + .catch(() => { + // A missed poll tries again on the next tick. Only say + // so once nothing has ever arrived, or a blip would + // replace a screen full of real numbers with an error. + if (!stopped) setFailed(true); + }); + }; + + poll(); + const intervalId = window.setInterval(poll, POLL_INTERVAL_MS); + // A scan started from the Options tab lands here as a redirect + // back; refresh then rather than waiting out the interval. + const stopOnSuccess = router.on('success', poll); + + return () => { + stopped = true; + window.clearInterval(intervalId); + stopOnSuccess(); + }; + }, []); + + if (activity === null) { + return ( +

+ {failed ? t('Could not read what the scanner is doing.') : t('Loading…')} +

+ ); + } + + const badge = (file: ScannedFile) => { + if (file.status === 'clean') { + return ( + + {t('Clean')} + + ); + } + + if (file.status === 'infected' || file.status === 'unscannable_blocked') { + return ( + + {file.note ?? t('Quarantined')} + + ); + } + + if (file.status === 'released') { + return ( + + {t('Released')} + + ); + } + + return ( + + {file.note ?? t('Not scanned')} + + ); + }; + + return ( +
+
+
+ {activity.running && } + +
+ +
+
+ {/* Two different facts, and the difference matters: + an upload nobody can download yet, and work the + scanner has not reached. A backfill shows up in + the second and never in the first. */} +
{t('Uploads held')}
+
{activity.waiting}
+
+
+
{t('In the queue')}
+
{activity.queued}
+
+
+
{t('Checked in the last hour')}
+
{activity.checked_last_hour}
+
+
+
{t('In quarantine')}
+
{activity.quarantined}
+
+
+
{t('Never scanned')}
+
{activity.never_scanned}
+
+
+
+ + {t('No file has been checked yet.')}} + > + {activity.recent.map((file) => ( + + {file.name} + {badge(file)} + {dateTime(file.scanned_at)} + + ))} + + + {failed &&

{t('The last refresh did not go through. Still trying.')}

} +
+ ); +} diff --git a/resources/js/pages/files/quarantine.tsx b/resources/js/pages/files/quarantine.tsx index 4502cd71..a05537e9 100644 --- a/resources/js/pages/files/quarantine.tsx +++ b/resources/js/pages/files/quarantine.tsx @@ -1,5 +1,5 @@ -import { type BreadcrumbItem } from '@/types'; -import { Head, useForm } from '@inertiajs/react'; +import { type BreadcrumbItem, type SharedData } from '@/types'; +import { Head, Link, useForm, usePage } from '@inertiajs/react'; import { ShieldAlert } from 'lucide-react'; import { useState } from 'react'; @@ -105,6 +105,7 @@ function ReleaseDialog({ file }: { file: QuarantinedFile }) { export default function Quarantine({ files, pagination }: QuarantineProps) { const { t } = useTranslation(); const { dateTime } = useFormatDate(); + const { auth } = usePage().props; const breadcrumbs: BreadcrumbItem[] = [ { title: t('All files'), href: '/files' }, @@ -116,7 +117,17 @@ export default function Quarantine({ files, pagination }: QuarantineProps) {
- +
+ + + {/* The way back to the screen that decides what gets + refused, for whoever may change it. */} + {auth.permissions.includes('edit_settings') && ( + + )} +
{files.some((file) => file.was_available) && ( diff --git a/resources/js/pages/system/settings/virus-scanning.tsx b/resources/js/pages/system/settings/virus-scanning.tsx index af9edf1b..81f8d4d0 100644 --- a/resources/js/pages/system/settings/virus-scanning.tsx +++ b/resources/js/pages/system/settings/virus-scanning.tsx @@ -1,5 +1,5 @@ -import { type BreadcrumbItem } from '@/types'; -import { Head, Link, router, useForm } from '@inertiajs/react'; +import { type BreadcrumbItem, type SharedData } from '@/types'; +import { Head, Link, router, useForm, usePage } from '@inertiajs/react'; import { CheckCircle2, ShieldAlert, TriangleAlert } from 'lucide-react'; import { FormEventHandler } from 'react'; @@ -13,10 +13,11 @@ import { Checkbox } from '@/components/ui/checkbox'; import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select'; +import { VirusScanActivity } from '@/components/virus-scan-activity'; import { useTranslation } from '@/hooks/use-translation'; import AppLayout from '@/layouts/app-layout'; -type Tab = 'scanner' | 'options'; +type Tab = 'scanner' | 'options' | 'activity'; interface VirusScanningProps { tab: Tab; @@ -53,6 +54,7 @@ export default function VirusScanningSettings({ counts, }: VirusScanningProps) { const { t } = useTranslation(); + const { auth } = usePage().props; const breadcrumbs: BreadcrumbItem[] = [ { title: t('Settings'), href: '/system/settings' }, @@ -80,6 +82,7 @@ export default function VirusScanningSettings({ const tabs: { key: Tab; label: string }[] = [ { key: 'scanner', label: t('Scanner') }, { key: 'options', label: t('Options') }, + { key: 'activity', label: t('Activity') }, ]; return ( @@ -87,7 +90,24 @@ export default function VirusScanningSettings({
- +
+ + + {/* The screen this one leads to: whatever the scanner + actually refused. Only for somebody who may act on + it — the quarantine screen answers 403 otherwise, + and a button that leads to a refusal is worse than + no button. */} + {auth.permissions.includes('release_quarantined_files') && ( + + )} +
{counts.let_through > 0 && ( @@ -105,7 +125,7 @@ export default function VirusScanningSettings({ {tabs.map(({ key, label }) => (
)} + {/* Mounted only while the tab is open, which is also what + starts and stops its polling. */} + {tab === 'activity' && }
); diff --git a/routes/settings.php b/routes/settings.php index 3eb078f7..2c4ff92d 100644 --- a/routes/settings.php +++ b/routes/settings.php @@ -193,6 +193,11 @@ Route::middleware('auth')->group(function () { // be a button somebody can sit on. Route::post('system/settings/virus-scanning/test', [VirusScanningSettingsController::class, 'test']) ->middleware('throttle:12,1,scanner-test')->name('system-settings.virus-scanning.test'); + // Polled every few seconds while the Activity tab is open, so it + // is a plain JSON read with its own generous bucket rather than + // an Inertia render. + Route::get('system/settings/virus-scanning/activity', [VirusScanningSettingsController::class, 'activity']) + ->middleware('throttle:120,1,scanner-activity')->name('system-settings.virus-scanning.activity'); Route::post('system/settings/virus-scanning/scan-existing', [VirusScanningSettingsController::class, 'scanExisting']) ->middleware('throttle:6,1,scanner-backfill')->name('system-settings.virus-scanning.scan-existing'); Route::get('system/settings/comments', [CommentSettingsController::class, 'edit'])->name('system-settings.comments.edit'); diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index 279ccfa9..7b3cd7c1 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -318,3 +318,74 @@ test('a hosted installation cannot connect a scanner of its own, but keeps its p expect(app(Settings::class)->get(Setting::VirusScannerAddress))->toBe('') ->and(app(Settings::class)->get(Setting::VirusUnscannablePolicy))->toBe('block'); }); + +/* +|-------------------------------------------------------------------------- +| Watching a scan happen +|-------------------------------------------------------------------------- +*/ + +test('the activity endpoint says what is running and what was decided', function () { + $waiting = File::factory()->create(['scan_status' => ScanStatus::Pending]); + $done = File::factory()->create([ + 'name' => 'Contrato', + 'scan_status' => ScanStatus::Infected, + 'scan_note' => 'Eicar-Test-Signature', + 'scanned_at' => now()->subMinute(), + ]); + + $body = $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity')->assertOk()->json(); + + expect($body['running'])->toBeTrue() + ->and($body['waiting'])->toBe(1) + ->and($body['checked_last_hour'])->toBe(1) + ->and($body['quarantined'])->toBe(1) + ->and($body['recent'][0]['name'])->toBe('Contrato') + ->and($body['recent'][0]['note'])->toBe('Eicar-Test-Signature'); + + expect($waiting->fresh()->scan_status)->toBe(ScanStatus::Pending) + ->and($done->fresh()->scan_status)->toBe(ScanStatus::Infected); +}); + +test('with nothing waiting it reports the last run rather than nothing at all', function () { + File::factory()->create([ + 'scan_status' => ScanStatus::Clean, + 'scanned_at' => now()->subDays(2), + ]); + + $body = $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity')->assertOk()->json(); + + expect($body['running'])->toBeFalse() + ->and($body['last_scanned_at'])->not->toBeNull() + ->and($body['recent'])->toHaveCount(1); +}); + +test('a file that was never scanned reads as such rather than as a bare "not scanned"', function () { + File::factory()->create(['scan_status' => ScanStatus::NotScanned, 'scan_note' => null, 'scanned_at' => now()]); + + $body = $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity')->assertOk()->json(); + + expect($body['recent'][0]['note'])->toContain('before virus scanning'); +}); + +test('watching a scan needs the same permission as changing its settings', function () { + $staff = User::factory()->role(App\Modules\Identity\Permissions\SystemRole::Uploader)->create(); + + $this->actingAs($staff)->getJson('/system/settings/virus-scanning/activity')->assertForbidden(); +}); + +test('a backfill counts as running even though it holds nothing back', function () { + // The case the first version of this screen got wrong: re-scanning + // files that already went out deliberately leaves them available, so + // nothing is "pending" and a screen watching only that said nothing + // was happening while the queue worked through a whole library. + Illuminate\Support\Facades\Queue::fake(); + + App\Modules\Files\Jobs\ScanFileJob::dispatch(1, true); + + $body = $this->actingAs($this->admin)->getJson('/system/settings/virus-scanning/activity')->assertOk()->json(); + + expect($body['waiting'])->toBe(0) + ->and($body['queued'])->toBe(1) + ->and($body['running'])->toBeTrue(); +}); diff --git a/tests/Feature/Files/VirusScanningTest.php b/tests/Feature/Files/VirusScanningTest.php index 8c4da1b6..74149c76 100644 --- a/tests/Feature/Files/VirusScanningTest.php +++ b/tests/Feature/Files/VirusScanningTest.php @@ -476,3 +476,36 @@ test('the backfill queues those files', function () { Illuminate\Support\Facades\Queue::assertPushed(ScanFileJob::class, fn (ScanFileJob $job): bool => $job->fileId === $old->id); }); + +test('a file whose bytes are gone says so, and is not retried forever', function () { + // An orphaned row, or storage that moved. It used to be recorded as + // "the scanner could not be reached" — wrong on screen, and wrong in + // behaviour: that is the one reason the hourly sweep re-queues, so + // every missing file would have been rescanned every hour for good. + $scanner = fakeScanner(ScanVerdict::clean()); + $file = scannableFile(); + Storage::disk('files')->delete($file->path); + + runScan($file); + + $file->refresh(); + expect($file->scan_status)->toBe(ScanStatus::NotScanned) + ->and($file->scan_note)->toBe(NotScannedReason::Unreadable->value) + // Never offered to the scanner: there was nothing to offer. + ->and($scanner->scans)->toBe(0); + + Illuminate\Support\Facades\Queue::fake(); + $this->artisan('projectsend:scan-files')->assertSuccessful(); + Illuminate\Support\Facades\Queue::assertNothingPushed(); +}); + +test('an unreadable file is blocked where this installation blocks what it cannot scan', function () { + app(App\Modules\Platform\Settings\Settings::class)->set(Setting::VirusUnscannablePolicy, 'block'); + fakeScanner(ScanVerdict::clean()); + $file = scannableFile(); + Storage::disk('files')->delete($file->path); + + runScan($file); + + expect($file->refresh()->scan_status)->toBe(ScanStatus::UnscannableBlocked); +});