From afb4c2c6d47aac0de9751f0b8adbd048bb19be11 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Wed, 16 Sep 2026 22:57:58 -0300 Subject: [PATCH] Test the address on screen, and count the quarantine in the sidebar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Test button asked the scanner on file, which makes it useless at the moment it is most needed: the first attempt, before anything has been saved. It now tries what is typed, falling back to the stored address when the field is empty so the button still answers on a screen nobody has touched. Nothing is written either way — testing is not saving. The address travels as a request field and is applied to the request's own ScanningConfig, which is scoped so the screen and the scanner it resolves share one. A preview address beats even a managed one, and is set in exactly one place. Quarantine now carries a count in the sidebar, like Comments — in amber rather than the usual colour, because the others count work waiting and this one counts something that went wrong. Shown only to whoever holds the permission to act on it. Both checked in a browser: an address typed and not saved came back "No answer from tcp://escrito-a-mano.invalid:3310", the stored one untouched, and the badge renders amber with the real count. --- app/Http/Middleware/HandleInertiaRequests.php | 14 +++++++ app/Modules/Files/FilesServiceProvider.php | 7 ++++ .../VirusScanningSettingsController.php | 12 +++++- app/Modules/Files/Scanning/ScanningConfig.php | 23 ++++++++++ resources/js/components/app-sidebar.tsx | 10 ++++- resources/js/components/nav-main.tsx | 8 +++- .../pages/system/settings/virus-scanning.tsx | 11 ++++- resources/js/types/index.ts | 8 ++++ .../Files/VirusScanningSettingsTest.php | 42 +++++++++++++++++++ 9 files changed, 131 insertions(+), 4 deletions(-) diff --git a/app/Http/Middleware/HandleInertiaRequests.php b/app/Http/Middleware/HandleInertiaRequests.php index c03f95ce..9ad2654e 100644 --- a/app/Http/Middleware/HandleInertiaRequests.php +++ b/app/Http/Middleware/HandleInertiaRequests.php @@ -15,7 +15,9 @@ use App\Modules\Platform\Attribution\Attribution; use App\Modules\Platform\Capabilities\CapabilityRegistry; use App\Modules\Platform\Captcha\Captcha; use App\Modules\Platform\Installation\Installation; +use App\Modules\Files\Models\File; use App\Modules\Files\Queue\StalledZipBuilds; +use App\Modules\Files\Scanning\ScanStatus; use App\Modules\Platform\Localization\LocaleRegistry; use App\Modules\Platform\Localization\TimezoneRegistry; use App\Modules\Platform\OfficialLinks; @@ -183,6 +185,18 @@ class HandleInertiaRequests extends Middleware $counts['comments'] = app(VisibleCommentScope::class)->pendingTotal($user); } + if ($checker->allows($user, Permission::ReleaseQuarantinedFiles)) { + // Deliberately not library-scoped, unlike the comments count + // above: a quarantined file is not a file anybody is working + // with, it is one somebody has to decide about, and the + // permission is already narrow enough that whoever holds it + // is meant to see all of them. + $counts['quarantine'] = File::query()->whereIn('scan_status', [ + ScanStatus::Infected->value, + ScanStatus::UnscannableBlocked->value, + ])->count(); + } + // Unlike the counts above, every authenticated user (staff or // client) has their own personal notifications — no permission // gate here. diff --git a/app/Modules/Files/FilesServiceProvider.php b/app/Modules/Files/FilesServiceProvider.php index 23278fbc..4e1e0333 100644 --- a/app/Modules/Files/FilesServiceProvider.php +++ b/app/Modules/Files/FilesServiceProvider.php @@ -17,6 +17,7 @@ use App\Modules\Files\Notifications\FileSharedNotification; use App\Modules\Files\Notifications\NewVersionAvailableNotification; use App\Modules\Files\Notifications\NewVersionDigestNotification; use App\Modules\Files\Scanning\ClamAvScanner; +use App\Modules\Files\Scanning\ScanningConfig; use App\Modules\Files\Scanning\ScanStatus; use App\Modules\Files\Scanning\VirusScanner; use App\Modules\Files\Thumbnails\Events\ImageRenderingChanged; @@ -43,6 +44,12 @@ class FilesServiceProvider extends ServiceProvider // per viewer and the file listings ask it once per row. $this->app->scoped(ClientIdentityScope::class); + // Scoped, so the settings screen and the scanner it resolves share + // one instance: that is what lets the Test button try the address + // being typed rather than the one on file. Scoped rather than a + // singleton so a queue worker starts each job with a clean one. + $this->app->scoped(ScanningConfig::class); + // One implementation ships, and the interface exists so the test // suite can state a verdict instead of producing a file that // provokes one — and so a commercial engine can be added later diff --git a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php index e0183ba5..804d1ba7 100644 --- a/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php +++ b/app/Modules/Files/Http/Controllers/VirusScanningSettingsController.php @@ -140,12 +140,22 @@ class VirusScanningSettingsController extends Controller * agreement — so the answer is "it detected something" rather than * "it did not complain". */ - public function test(VirusScanner $scanner): RedirectResponse + public function test(Request $request, VirusScanner $scanner): RedirectResponse { // Nothing to test where the connection is not this installation's // to make. abort_unless($this->canConnect(), 403); + $typed = trim((string) $request->input('address', '')); + + // What the button is for: the address on screen, which on a first + // attempt has never been saved. Falls back to the stored one when + // the field is empty, so the button still answers on a screen + // somebody has not touched. + if ($typed !== '') { + $this->config->preview($typed); + } + $status = $scanner->status(); if (! $status->reachable) { diff --git a/app/Modules/Files/Scanning/ScanningConfig.php b/app/Modules/Files/Scanning/ScanningConfig.php index 16f6a845..e2cddea8 100644 --- a/app/Modules/Files/Scanning/ScanningConfig.php +++ b/app/Modules/Files/Scanning/ScanningConfig.php @@ -40,6 +40,22 @@ class ScanningConfig return $this->isManaged() || $this->settings->get(Setting::VirusScanningEnabled) === true; } + /** + * An address to use instead of the stored one, for this request only. + * + * The Test button exists to answer "is *this* address right?", and + * the address in question is the one being typed — testing what is + * saved would make the button useless exactly when it is needed, on + * the first attempt, before anything is saved. Set by + * VirusScanningSettingsController::test() and never persisted. + */ + private ?string $preview = null; + + public function preview(string $address): void + { + $this->preview = trim($address); + } + public function isManaged(): bool { return $this->managedAddress() !== ''; @@ -47,6 +63,13 @@ class ScanningConfig public function address(): string { + // Ahead of the managed address too: an operator on a managed + // installation has no field to type in, so nothing sets this + // there — and where something does, it was asked for. + if ($this->preview !== null && $this->preview !== '') { + return $this->preview; + } + if ($this->isManaged()) { return $this->managedAddress(); } diff --git a/resources/js/components/app-sidebar.tsx b/resources/js/components/app-sidebar.tsx index 0555d120..0da0f2fa 100644 --- a/resources/js/components/app-sidebar.tsx +++ b/resources/js/components/app-sidebar.tsx @@ -65,7 +65,15 @@ export function AppSidebar() { 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 }); + // Amber rather than the usual badge colour: the others count work + // waiting, this one counts something that went wrong. + fileItems.push({ + title: t('Quarantine'), + url: '/files/quarantine', + icon: ShieldAlert, + badge: pending.quarantine, + badgeTone: 'warning', + }); } if (can('moderate_comments')) { // Just "Comments" — the old "Comments awaiting approval" wrapped diff --git a/resources/js/components/nav-main.tsx b/resources/js/components/nav-main.tsx index 24d4a67f..31ab5095 100644 --- a/resources/js/components/nav-main.tsx +++ b/resources/js/components/nav-main.tsx @@ -85,7 +85,13 @@ export function NavMain({ groups = [] }: { groups: NavGroup[] }) { )} {item.badge !== undefined && item.badge > 0 && ( - + {item.badge} )} diff --git a/resources/js/pages/system/settings/virus-scanning.tsx b/resources/js/pages/system/settings/virus-scanning.tsx index 73cc0e59..8efbde37 100644 --- a/resources/js/pages/system/settings/virus-scanning.tsx +++ b/resources/js/pages/system/settings/virus-scanning.tsx @@ -217,7 +217,16 @@ export default function VirusScanningSettings({ type="button" variant="outline" className="w-fit" - onClick={() => router.post(route('system-settings.virus-scanning.test'), {}, { preserveScroll: true })} + // The address on screen, not the one on + // file: the question is whether what is + // being typed works. + onClick={() => + router.post( + route('system-settings.virus-scanning.test'), + { address: data.address }, + { preserveScroll: true }, + ) + } > {t('Test scanner')} diff --git a/resources/js/types/index.ts b/resources/js/types/index.ts index 33fd95d8..e497163e 100644 --- a/resources/js/types/index.ts +++ b/resources/js/types/index.ts @@ -24,6 +24,12 @@ export interface NavItem { isActive?: boolean; items?: NavItem[]; badge?: number; + /** + * What the badge is saying. The default reads as "there is work + * here"; "warning" reads as "something is wrong here", which is the + * difference between a queue and a quarantine. + */ + badgeTone?: 'default' | 'warning'; /** * Leaves this installation. Rendered as a plain anchor opening in a * new tab rather than an Inertia , which would try to fetch a @@ -130,6 +136,8 @@ export interface SharedData { membership_requests?: number; /** Comments held for approval anywhere in this viewer's library. */ comments?: number; + /** Files the scanner refused, waiting for somebody to decide. */ + quarantine?: number; notifications_unread?: number; }; update_notice: { diff --git a/tests/Feature/Files/VirusScanningSettingsTest.php b/tests/Feature/Files/VirusScanningSettingsTest.php index 5ee1035f..25e14e2c 100644 --- a/tests/Feature/Files/VirusScanningSettingsTest.php +++ b/tests/Feature/Files/VirusScanningSettingsTest.php @@ -423,3 +423,45 @@ test('an installation that connects its own scanner is', function () { fn (AssertableInertia $page) => $page->where('can_test', true), ); }); + +test('the test button tries the address on screen, not the one on file', function () { + // The real client, deliberately: a fake would answer whatever it was + // told and prove nothing about which address was used. Neither + // address has a scanner behind it, so the answer names the one it + // actually tried. + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://saved.invalid:3310'); + + $this->actingAs($this->admin) + ->post('/system/settings/virus-scanning/test', ['address' => 'tcp://typed.invalid:3310']) + ->assertSessionHas('scanner_test_result', fn (array $result): bool => $result['ok'] === false + && str_contains($result['message'], 'typed.invalid')); + + // And the stored address is untouched: testing is not saving. + expect(app(Settings::class)->get(Setting::VirusScannerAddress))->toBe('tcp://saved.invalid:3310'); +}); + +test('an empty field falls back to the address on file', function () { + app(Settings::class)->set(Setting::VirusScannerAddress, 'tcp://saved.invalid:3310'); + + $this->actingAs($this->admin) + ->post('/system/settings/virus-scanning/test', ['address' => '']) + ->assertSessionHas('scanner_test_result', fn (array $result): bool => str_contains($result['message'], 'saved.invalid')); +}); + +test('the sidebar carries a count of what is in quarantine', function () { + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Clean]); + + $this->actingAs($this->admin)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page->where('pending.quarantine', 1), + ); +}); + +test('somebody who cannot release is not shown the count', function () { + $staff = User::factory()->role(App\Modules\Identity\Permissions\SystemRole::Uploader)->create(); + App\Modules\Files\Models\File::factory()->create(['scan_status' => ScanStatus::Infected, 'scan_note' => 'X']); + + $this->actingAs($staff)->get('/dashboard')->assertInertia( + fn (AssertableInertia $page) => $page->missing('pending.quarantine'), + ); +});