From 84c8b81f0aba8a93cc299bc5d4d769a9274e10c9 Mon Sep 17 00:00:00 2001 From: surtic86 Date: Thu, 22 Oct 2026 14:46:38 +0200 Subject: [PATCH] Fix the security audit's findings Fixes the nine findings of security audit run 1 on 2.4.1 (T1-T11). F1: Uploads that are not complete now count only the bytes the server received, and a chunk that goes above the storage quota gets 422. An anonymous uploader can no longer fill the quota with declared sizes. F2: The share password and the admin login have a second limit without the address (20 attempts per minute per share and per e-mail), because a forged X-Forwarded-For changes the address. F3: The system password gate and the admin check now also run on Livewire updates, so an old uploader snapshot cannot register files. F4: The session keeps a fingerprint of the system password's hash. When the admin changes the system password, earlier verifications end. F5: The page after the upload opens only in the session that created the share, and only while recipients can still reach the share. F6: The share password limit counts an attempt before the password is checked, so parallel attempts cannot pass it. F7: The system password has the same two limits (5 per address, 20 in total per minute). F8: A password change or reset signs the user's other sessions out. F9: The only admin cannot delete the account, which would open /setup to every visitor. The setup wizard also holds the admin password to the password defaults, and Livewire applies EnsureAdmin to updates. Co-Authored-By: Claude Opus 5.5 --- app/Http/Middleware/SystemPasswordGate.php | 5 +- app/Livewire/FileUploader.php | 1 + app/Livewire/SetupWizard.php | 12 ++- app/Livewire/ShareCreated.php | 6 +- app/Livewire/ShareDownload.php | 18 +++- app/Livewire/SystemPasswordPrompt.php | 23 ++++- app/Providers/AppServiceProvider.php | 6 ++ app/Providers/FortifyServiceProvider.php | 6 +- app/Services/ShareService.php | 17 +++- bootstrap/app.php | 3 + .../settings/⚡delete-user-form.blade.php | 65 ++++++++---- tests/Browser/BreakpointsTest.php | 2 + tests/Browser/SealShareTest.php | 11 ++- tests/Browser/ShareFlowTest.php | 2 + tests/Feature/Admin/AdminDashboardTest.php | 4 +- tests/Feature/Auth/AuthenticationTest.php | 22 +++++ tests/Feature/FileUploadTest.php | 81 ++++++++++++++- tests/Feature/PageTemplateTest.php | 6 +- tests/Feature/SecurityAuditTest.php | 98 +++++++++++++++++++ tests/Feature/Settings/PasswordUpdateTest.php | 94 ++++++++++++++++++ tests/Feature/Settings/ProfileUpdateTest.php | 37 +++++++ tests/Feature/SetupWizardTest.php | 16 +++ tests/Feature/ShareCreatedTest.php | 33 ++++++- tests/Feature/UploadChunkTest.php | 14 +++ tests/Screenshots/ScreenshotsTest.php | 1 + tests/Unit/ShareServiceTest.php | 28 +++++- 26 files changed, 566 insertions(+), 45 deletions(-) diff --git a/app/Http/Middleware/SystemPasswordGate.php b/app/Http/Middleware/SystemPasswordGate.php index fe9b34f..451cf05 100644 --- a/app/Http/Middleware/SystemPasswordGate.php +++ b/app/Http/Middleware/SystemPasswordGate.php @@ -17,7 +17,10 @@ class SystemPasswordGate return $next($request); } - if ($request->session()->get('system_password_verified') === true) { + // A fingerprint of the current hash: changing the system password ends every earlier verification. + $verified = $request->session()->get('system_password_verified'); + + if (is_string($verified) && hash_equals(hash('sha256', $systemPassword), $verified)) { return $next($request); } diff --git a/app/Livewire/FileUploader.php b/app/Livewire/FileUploader.php index 1612e58..ac99e17 100644 --- a/app/Livewire/FileUploader.php +++ b/app/Livewire/FileUploader.php @@ -192,6 +192,7 @@ class FileUploader extends Component ]); session()->put('pending_shares', array_values(array_diff(session('pending_shares', []), [$share->token]))); + session()->push('created_shares', $share->token); // The page the upload leads to offers the password once more, next to the link; it is // never stored in the clear, so this flash is the only way it gets there. diff --git a/app/Livewire/SetupWizard.php b/app/Livewire/SetupWizard.php index c99a960..ceeef36 100644 --- a/app/Livewire/SetupWizard.php +++ b/app/Livewire/SetupWizard.php @@ -5,6 +5,7 @@ namespace App\Livewire; use App\Models\User; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Hash; +use Illuminate\Validation\Rules\Password; use Livewire\Attributes\Layout; use Livewire\Attributes\Validate; use Livewire\Component; @@ -18,11 +19,20 @@ class SetupWizard extends Component #[Validate('required|string|email|max:255|unique:users')] public string $email = ''; - #[Validate('required|string|min:8|confirmed')] public string $password = ''; public string $password_confirmation = ''; + /** + * @return array> + */ + protected function rules(): array + { + return [ + 'password' => ['required', 'string', Password::defaults(), 'confirmed'], + ]; + } + public function mount(): void { if (User::query()->where('is_admin', true)->exists()) { diff --git a/app/Livewire/ShareCreated.php b/app/Livewire/ShareCreated.php index 472a5fe..8b304d2 100644 --- a/app/Livewire/ShareCreated.php +++ b/app/Livewire/ShareCreated.php @@ -15,13 +15,15 @@ class ShareCreated extends Component { public Share $share; - /** The share's password, offered once to the uploader who just set it; `null` on any other visit. */ + /** The share's password, offered once to the uploader who just set it; `null` on a reload. */ #[Locked] public ?string $password = null; public function mount(Share $share): void { - abort_unless($share->isCompleted(), 404); + // Only for the session that created the share, and only while recipients can still reach it. + abort_unless($share->isCompleted() && ! $share->isExpired() && ! $share->hasReachedDownloadLimit() + && in_array($share->token, session('created_shares', []), true), 404); $this->share = $share; diff --git a/app/Livewire/ShareDownload.php b/app/Livewire/ShareDownload.php index 8242797..d346294 100644 --- a/app/Livewire/ShareDownload.php +++ b/app/Livewire/ShareDownload.php @@ -39,10 +39,18 @@ class ShareDownload extends Component public function verifyPassword(ShareService $shareService): void { - $rateLimitKey = 'share-password:'.$this->share->token.'|'.request()->ip(); + $ipKey = 'share-password:'.$this->share->token.'|'.request()->ip(); + $shareKey = 'share-password:'.$this->share->token; - if (RateLimiter::tooManyAttempts($rateLimitKey, 5)) { - $seconds = RateLimiter::availableIn($rateLimitKey); + // Count before the slow hash check, so parallel requests cannot all pass a read-only check. + $ipHits = RateLimiter::hit($ipKey, 60); + $shareHits = RateLimiter::hit($shareKey, 60); + + if ($ipHits > 5 || $shareHits > 20) { + $seconds = max( + $ipHits > 5 ? RateLimiter::availableIn($ipKey) : 0, + $shareHits > 20 ? RateLimiter::availableIn($shareKey) : 0, + ); $this->addError('password', __('Too many attempts. Please try again in :seconds seconds.', ['seconds' => $seconds])); return; @@ -51,13 +59,13 @@ class ShareDownload extends Component $this->validate(); if (! $shareService->verifyPassword($this->share, $this->password)) { - RateLimiter::hit($rateLimitKey, 60); $this->addError('password', __('The password is incorrect.')); return; } - RateLimiter::clear($rateLimitKey); + RateLimiter::clear($ipKey); + RateLimiter::clear($shareKey); $encryptionKey = $shareService->getDecryptionKey($this->share, $this->password); session(['share_key_'.$this->share->token => $encryptionKey]); diff --git a/app/Livewire/SystemPasswordPrompt.php b/app/Livewire/SystemPasswordPrompt.php index 38672bc..a534312 100644 --- a/app/Livewire/SystemPasswordPrompt.php +++ b/app/Livewire/SystemPasswordPrompt.php @@ -4,6 +4,7 @@ namespace App\Livewire; use App\Models\Setting; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\RateLimiter; use Livewire\Attributes\Layout; use Livewire\Attributes\Validate; use Livewire\Component; @@ -16,6 +17,23 @@ class SystemPasswordPrompt extends Component public function verify(): void { + $ipKey = 'system-password|'.request()->ip(); + $globalKey = 'system-password'; + + // Count before the slow hash check, so parallel requests cannot all pass a read-only check. + $ipHits = RateLimiter::hit($ipKey, 60); + $globalHits = RateLimiter::hit($globalKey, 60); + + if ($ipHits > 5 || $globalHits > 20) { + $seconds = max( + $ipHits > 5 ? RateLimiter::availableIn($ipKey) : 0, + $globalHits > 20 ? RateLimiter::availableIn($globalKey) : 0, + ); + $this->addError('password', __('Too many attempts. Please try again in :seconds seconds.', ['seconds' => $seconds])); + + return; + } + $this->validate(); $systemPassword = Setting::get('system_password'); @@ -26,7 +44,10 @@ class SystemPasswordPrompt extends Component return; } - session(['system_password_verified' => true]); + RateLimiter::clear($ipKey); + RateLimiter::clear($globalKey); + + session(['system_password_verified' => hash('sha256', $systemPassword)]); $this->redirect(route('upload'), navigate: true); } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 7a84ffd..5ea1eba 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -2,12 +2,15 @@ namespace App\Providers; +use App\Http\Middleware\EnsureAdmin; +use App\Http\Middleware\SystemPasswordGate; use App\Models\Setting; use Carbon\CarbonImmutable; use Illuminate\Support\Facades\Date; use Illuminate\Support\Facades\DB; use Illuminate\Support\ServiceProvider; use Illuminate\Validation\Rules\Password; +use Livewire\Livewire; use NoNameWeb\LivewireMaterial\Support\Scheme; class AppServiceProvider extends ServiceProvider @@ -22,6 +25,9 @@ class AppServiceProvider extends ServiceProvider // The colour profile the admin chose in Admin settings; asked each time a page, mail or // error page draws its colours, so a new choice applies at once in every Octane worker. Scheme::resolveProfileUsing(fn (): ?string => Setting::get('color_profile')); + + // Livewire updates apply a page route's middleware only when it is listed as persistent. + Livewire::addPersistentMiddleware([SystemPasswordGate::class, EnsureAdmin::class]); } /** diff --git a/app/Providers/FortifyServiceProvider.php b/app/Providers/FortifyServiceProvider.php index d8bd84a..3db280e 100644 --- a/app/Providers/FortifyServiceProvider.php +++ b/app/Providers/FortifyServiceProvider.php @@ -62,7 +62,11 @@ class FortifyServiceProvider extends ServiceProvider RateLimiter::for('login', function (Request $request) { $throttleKey = Str::transliterate(Str::lower($request->input(Fortify::username())).'|'.$request->ip()); - return Limit::perMinute(5)->by($throttleKey); + // The address comes from X-Forwarded-For behind the proxy, so a second limit per e-mail does not use it. + return [ + Limit::perMinute(5)->by($throttleKey), + Limit::perMinute(20)->by(Str::transliterate(Str::lower($request->input(Fortify::username())))), + ]; }); } } diff --git a/app/Services/ShareService.php b/app/Services/ShareService.php index 8095a9e..791ded2 100644 --- a/app/Services/ShareService.php +++ b/app/Services/ShareService.php @@ -10,6 +10,7 @@ use Illuminate\Contracts\Session\Session; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Http\UploadedFile; use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; @@ -104,7 +105,7 @@ class ShareService * Verify the encrypted chunk that comes next for a file and write it into place; returns how * many of the file's chunks are stored. The plaintext only exists in memory, to be checked. * - * @throws InvalidArgumentException when the chunk has the wrong length or fails authentication + * @throws InvalidArgumentException when the chunk has the wrong length, exceeds the storage quota or fails authentication * @throws ModelNotFoundException when the file was removed meanwhile */ public function storeChunk(ShareFile $file, int $index, string $chunk): int @@ -134,6 +135,10 @@ class ShareService throw new InvalidArgumentException('Chunk '.$index.' has the wrong length'); } + if ($this->getTotalUsedSpace() + $plaintextLength > $this->getMaxStorageQuota()) { + throw new InvalidArgumentException('Chunk '.$index.' exceeds the storage quota'); + } + try { $plaintext = $this->encryptionService->decryptChunk($chunk, $share->encryption_key, $noncePrefix, $index, $isLast); } catch (RuntimeException) { @@ -448,11 +453,17 @@ class ShareService } /** - * Get total used space in bytes, files still being uploaded included. + * Get total used space in bytes; files still being uploaded count with the bytes received so far. */ public function getTotalUsedSpace(): int { - return (int) Share::query()->sum('total_size'); + $chunkSize = (int) config('uploads.chunk_size'); + + $received = ShareFile::query() + ->whereHas('share', fn ($query) => $query->whereNull('completed_at')) + ->sum(DB::raw("CASE WHEN uploaded_chunks * {$chunkSize} < file_size THEN uploaded_chunks * {$chunkSize} ELSE file_size END")); + + return (int) Share::query()->whereNotNull('completed_at')->sum('total_size') + (int) $received; } /** diff --git a/bootstrap/app.php b/bootstrap/app.php index c51f0a4..8d9a5c3 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -16,6 +16,9 @@ return Application::configure(basePath: dirname(__DIR__)) ->withMiddleware(function (Middleware $middleware): void { $middleware->trustProxies(at: '*'); + // Ends every other session of a user whose password changed. + $middleware->authenticateSessions(); + $middleware->web(append: [ EnsureSetupComplete::class, SecurityHeaders::class, diff --git a/resources/views/pages/settings/⚡delete-user-form.blade.php b/resources/views/pages/settings/⚡delete-user-form.blade.php index cdc140a..f13557b 100644 --- a/resources/views/pages/settings/⚡delete-user-form.blade.php +++ b/resources/views/pages/settings/⚡delete-user-form.blade.php @@ -2,7 +2,9 @@ use App\Concerns\PasswordValidationRules; use App\Livewire\Actions\Logout; +use App\Models\User; use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\DB; use Livewire\Component; new class extends Component { @@ -12,7 +14,15 @@ new class extends Component { public bool $showDeleteModal = false; /** - * Delete the currently authenticated user. + * Whether the user is the only admin: deleting that account would open /setup to anyone. + */ + public function isLastAdmin(): bool + { + return Auth::user()->is_admin && ! User::query()->where('is_admin', true)->whereKeyNot(Auth::id())->exists(); + } + + /** + * Delete the currently authenticated user, unless it is the only admin. */ public function deleteUser(Logout $logout): void { @@ -20,29 +30,50 @@ new class extends Component { 'password' => $this->currentPasswordRules(), ]); - tap(Auth::user(), $logout(...))->delete(); + // The admin rows stay locked until the delete, so two admins cannot delete each other's last peer at once. + $deleted = DB::transaction(function () use ($logout): bool { + User::query()->where('is_admin', true)->lockForUpdate()->get(); + + if ($this->isLastAdmin()) { + return false; + } + + tap(Auth::user(), $logout(...))->delete(); + + return true; + }); + + if (! $deleted) { + $this->addError('password', __('You are the only admin, so this account cannot be deleted.')); + + return; + } $this->redirect('/', navigate: true); } }; ?> - + @if ($this->isLastAdmin()) +

{{ __('You are the only admin, so this account cannot be deleted.') }}

+ @else + - - -

- {{ __('Once your account is deleted, all of its resources and data will be permanently deleted. Please enter your password to confirm you would like to permanently delete your account.') }} -

+ + +

+ {{ __('Once your account is deleted, all of its resources and data will be permanently deleted. Please enter your password to confirm you would like to permanently delete your account.') }} +

- - - - + + + + - - - - - + + + + + + @endif diff --git a/tests/Browser/BreakpointsTest.php b/tests/Browser/BreakpointsTest.php index 20b59e3..d3178dc 100644 --- a/tests/Browser/BreakpointsTest.php +++ b/tests/Browser/BreakpointsTest.php @@ -136,6 +136,8 @@ test('the share-created page holds at every breakpoint', function () { [], ); + $this->withSession(['created_shares' => [$share->token]]); + walkBreakpoints(ready(visit(route('share.created', $share, false)))); }); diff --git a/tests/Browser/SealShareTest.php b/tests/Browser/SealShareTest.php index c3436e4..677f2b4 100644 --- a/tests/Browser/SealShareTest.php +++ b/tests/Browser/SealShareTest.php @@ -57,6 +57,8 @@ test('a new share\'s link can be copied from the page the upload leads to', func [], ); + $this->withSession(['created_shares' => [$share->token]]); + $page = ready(visit(route('share.created', $share, false))); $page->assertSee('Share Created!') @@ -89,7 +91,10 @@ test('the new share\'s password stays masked on the page and is copied without b [['file' => UploadedFile::fake()->create('contract.pdf', 80), 'relativePath' => null]], ['password' => 'violet-orbit-canyon'], ); - $this->withSession(['share_password' => ['token' => $share->token, 'password' => Crypt::encryptString('violet-orbit-canyon')]]); + $this->withSession([ + 'created_shares' => [$share->token], + 'share_password' => ['token' => $share->token, 'password' => Crypt::encryptString('violet-orbit-canyon')], + ]); $page = ready(visit(route('share.created', $share, false))); @@ -109,6 +114,8 @@ test('the new share\'s password stays masked on the page and is copied without b test('a new share\'s QR code opens in a dialog and saves as a PNG', function () { $share = Share::factory()->withPassword()->create(); + $this->withSession(['created_shares' => [$share->token]]); + $page = ready(visit(route('share.created', $share, false))); $page->click('[data-test="show-qr-code"]') @@ -138,6 +145,8 @@ test('the share sheet gets the link, and says so only when it fails for another $share = Share::factory()->create(); $actions = "Alpine.\$data(document.querySelector('[data-test=share-actions]'))"; + $this->withSession(['created_shares' => [$share->token]]); + $page = ready(visit(route('share.created', $share, false))); // Shown only where the browser has a share sheet. diff --git a/tests/Browser/ShareFlowTest.php b/tests/Browser/ShareFlowTest.php index a57a82f..402cfdb 100644 --- a/tests/Browser/ShareFlowTest.php +++ b/tests/Browser/ShareFlowTest.php @@ -95,6 +95,8 @@ test('the drop zone hides its burst again once the drag leaves', function () { test('the QR dialog holds its code inside the box, and Escape returns focus to the button that opened it', function () { $share = Share::factory()->create(); + $this->withSession(['created_shares' => [$share->token]]); + $page = ready(visit(route('share.created', $share, false))); $page->click('[data-test="show-qr-code"]'); diff --git a/tests/Feature/Admin/AdminDashboardTest.php b/tests/Feature/Admin/AdminDashboardTest.php index 88ff62f..714857a 100644 --- a/tests/Feature/Admin/AdminDashboardTest.php +++ b/tests/Feature/Admin/AdminDashboardTest.php @@ -151,7 +151,7 @@ test('the shares list excludes the private text from its file count and shows it ->assertDontSee('3 files'); }); -test('shares whose files are still uploading are neither listed nor counted, but their bytes count as used space', function () { +test('shares whose files are still uploading are neither listed nor counted, but only their received bytes count as used space', function () { $admin = User::query()->where('is_admin', true)->first(); $completed = Share::factory()->create(['token' => 'completedshare01', 'total_size' => 1000]); ShareFile::factory()->for($completed)->create(); @@ -165,5 +165,5 @@ test('shares whose files are still uploading are neither listed nor counted, but ->assertViewHas('totalShares', 1) ->assertViewHas('activeShares', 1) ->assertViewHas('totalFiles', 1) - ->assertViewHas('usedSpace', 1500); + ->assertViewHas('usedSpace', 1000); }); diff --git a/tests/Feature/Auth/AuthenticationTest.php b/tests/Feature/Auth/AuthenticationTest.php index 0e1a511..0e5f4ab 100644 --- a/tests/Feature/Auth/AuthenticationTest.php +++ b/tests/Feature/Auth/AuthenticationTest.php @@ -37,6 +37,28 @@ test('users can not authenticate with invalid password', function () { $this->assertGuest(); }); +test('the login limit per e-mail holds when each attempt claims another address', function () { + $user = User::factory()->create(); + + for ($i = 1; $i <= 20; $i++) { + $response = $this->withHeader('X-Forwarded-For', "203.0.113.$i")->post(route('login.store'), [ + 'email' => $user->email, + 'password' => 'wrong-password', + ]); + + if ($i === 1) { + expect($response->status())->not->toBe(429); + } + } + + $this->withHeader('X-Forwarded-For', '203.0.113.21')->post(route('login.store'), [ + 'email' => $user->email, + 'password' => 'password', + ])->assertTooManyRequests(); + + $this->assertGuest(); +}); + test('users with two factor enabled are redirected to two factor challenge', function () { if (! Features::canManageTwoFactorAuthentication()) { $this->markTestSkipped('Two-factor authentication is not enabled.'); diff --git a/tests/Feature/FileUploadTest.php b/tests/Feature/FileUploadTest.php index 0e38074..a31f6de 100644 --- a/tests/Feature/FileUploadTest.php +++ b/tests/Feature/FileUploadTest.php @@ -7,6 +7,7 @@ use App\Models\Share; use App\Models\ShareFile; use App\Services\ShareService; use Illuminate\Support\Facades\Crypt; +use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Facades\Storage; use Livewire\Features\SupportTesting\Testable; use Livewire\Livewire; @@ -54,14 +55,33 @@ test('upload page requires system password when configured', function () { }); test('upload page accessible after system password verified', function () { - Setting::set('system_password', bcrypt('system-secret')); + $systemPassword = bcrypt('system-secret'); + Setting::set('system_password', $systemPassword); - $response = $this->withSession(['system_password_verified' => true]) + $response = $this->withSession(['system_password_verified' => hash('sha256', $systemPassword)]) ->get(route('upload')); $response->assertOk(); }); +test('a verification of an earlier system password no longer opens the upload page', function () { + $oldSystemPassword = bcrypt('system-secret'); + Setting::set('system_password', $oldSystemPassword); + $this->withSession(['system_password_verified' => hash('sha256', $oldSystemPassword)]); + + Setting::set('system_password', bcrypt('new-system-secret')); + + $this->get(route('upload'))->assertRedirect(route('system-password')); +}); + +test('a verification stored before 2.4.2 no longer opens the upload page', function () { + Setting::set('system_password', bcrypt('system-secret')); + + $this->withSession(['system_password_verified' => true]) + ->get(route('upload')) + ->assertRedirect(route('system-password')); +}); + test('file upload creates share', function () { Storage::fake('shares'); $component = Livewire::test(FileUploader::class); @@ -74,6 +94,7 @@ test('file upload creates share', function () { expect($share->isCompleted())->toBeTrue(); expect($share->files->pluck('original_name')->all())->toBe(['document.pdf']); expect(session('pending_shares'))->not->toContain($share->token); + expect(session('created_shares'))->toBe([$share->token]); }); test('registering files hands the browser each file\'s chunk URL, the share key and the file\'s nonce prefix', function () { @@ -377,6 +398,27 @@ test('file upload blocks when storage is full', function () { expect(ShareFile::query()->count())->toBe(0); }); +test('files declared by one session without any chunk do not block another session', function () { + Storage::fake('shares'); + Setting::set('max_file_size', 200 * 1024 * 1024); + Setting::set('max_size_per_share', 1024 * 1024 * 1024); + Setting::set('max_files_per_share', 10); + Setting::set('max_storage_quota', 300 * 1024 * 1024); + + Livewire::test(FileUploader::class)->call('registerFiles', [ + ['name' => 'a.bin', 'size' => 100 * 1024 * 1024, 'path' => null], + ['name' => 'b.bin', 'size' => 100 * 1024 * 1024, 'path' => null], + ['name' => 'c.bin', 'size' => 100 * 1024 * 1024, 'path' => null], + ])->assertHasNoErrors('files'); + + session()->flush(); + $component = Livewire::test(FileUploader::class) + ->call('registerFiles', [['name' => 'd.txt', 'size' => 1024, 'path' => null]]); + + $component->assertHasNoErrors('files'); + expect(ShareFile::query()->count())->toBe(4); +}); + test('system password prompt verifies correct password', function () { Setting::set('system_password', bcrypt('system-secret')); @@ -394,3 +436,38 @@ test('system password prompt rejects incorrect password', function () { ->call('verify') ->assertHasErrors(['password']); }); + +test('the system password prompt refuses a sixth attempt from the same address, even with the correct password', function () { + Setting::set('system_password', bcrypt('system-secret')); + + $component = Livewire::test(SystemPasswordPrompt::class); + + for ($i = 0; $i < 5; $i++) { + $component->set('password', 'wrong') + ->call('verify') + ->assertHasErrors(['password']); + } + + $component->set('password', 'system-secret') + ->call('verify') + ->assertHasErrors(['password']) + ->assertSee('Too many attempts') + ->assertNoRedirect(); +}); + +test('the global system password limit refuses the correct password from any address', function () { + Setting::set('system_password', bcrypt('system-secret')); + + for ($i = 0; $i < 20; $i++) { + RateLimiter::hit('system-password', 60); + } + + Livewire::test(SystemPasswordPrompt::class) + ->set('password', 'system-secret') + ->call('verify') + ->assertHasErrors(['password']) + ->assertSee('Too many attempts') + ->assertNoRedirect(); + + expect(session()->has('system_password_verified'))->toBeFalse(); +}); diff --git a/tests/Feature/PageTemplateTest.php b/tests/Feature/PageTemplateTest.php index ab18d70..83f54c6 100644 --- a/tests/Feature/PageTemplateTest.php +++ b/tests/Feature/PageTemplateTest.php @@ -26,7 +26,11 @@ test('every page is one page template, one h1 and the same width', function (Clo })->with([ 'upload' => [fn (TestCase $test): TestResponse => $test->get(route('upload'))], 'download' => [fn (TestCase $test): TestResponse => $test->get(route('share.download', Share::factory()->withPassword()->create()))], - 'share created' => [fn (TestCase $test): TestResponse => $test->get(route('share.created', Share::factory()->create()))], + 'share created' => [function (TestCase $test): TestResponse { + $share = Share::factory()->create(); + + return $test->withSession(['created_shares' => [$share->token]])->get(route('share.created', $share)); + }], 'login' => [fn (TestCase $test): TestResponse => $test->get(route('login'))], 'forgot password' => [fn (TestCase $test): TestResponse => $test->get(route('password.request'))], 'reset password' => [fn (TestCase $test): TestResponse => $test->get(route('password.reset', 'token'))], diff --git a/tests/Feature/SecurityAuditTest.php b/tests/Feature/SecurityAuditTest.php index 596f550..3f95963 100644 --- a/tests/Feature/SecurityAuditTest.php +++ b/tests/Feature/SecurityAuditTest.php @@ -3,11 +3,14 @@ use App\Livewire\Admin\AdminSettings; use App\Livewire\FileUploader; use App\Livewire\ShareDownload; +use App\Models\Setting; use App\Models\Share; use App\Models\ShareFile; use App\Models\User; use App\Services\ShareService; +use Illuminate\Contracts\Hashing\Hasher; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Facades\Storage; use Livewire\Features\SupportLockedProperties\CannotUpdateLockedPropertyException; @@ -106,6 +109,101 @@ test('rate limiter clears after successful password verification', function () { expect(RateLimiter::remaining($rateLimitKey, 5))->toBe(5); }); +test('a share password attempt is counted before the password is checked', function () { + Storage::fake('shares'); + + $share = app(ShareService::class)->createShare([ + ['file' => UploadedFile::fake()->create('file.txt', 100), 'relativePath' => null], + ], [ + 'password' => 'correct-password', + ]); + + $ipKey = 'share-password:'.$share->token.'|127.0.0.1'; + $attemptsSeenDuringCheck = new ArrayObject; + + Hash::swap(new class(Hash::getFacadeRoot(), $ipKey, $attemptsSeenDuringCheck) implements Hasher + { + public function __construct(private Hasher $inner, private string $ipKey, private ArrayObject $seen) {} + + public function info($hashedValue): array + { + return $this->inner->info($hashedValue); + } + + public function make(#[SensitiveParameter] $value, array $options = []): string + { + return $this->inner->make($value, $options); + } + + public function check(#[SensitiveParameter] $value, $hashedValue, array $options = []): bool + { + $this->seen[] = RateLimiter::attempts($this->ipKey); + + return $this->inner->check($value, $hashedValue, $options); + } + + public function needsRehash($hashedValue, array $options = []): bool + { + return false; + } + }); + + Livewire::test(ShareDownload::class, ['share' => $share]) + ->set('password', 'wrong-password') + ->call('verifyPassword'); + + expect($attemptsSeenDuringCheck->getArrayCopy())->not->toBeEmpty() + ->each->toBeGreaterThanOrEqual(1); +}); + +test('the share-wide limit refuses the correct password from any address', function () { + Storage::fake('shares'); + + $share = app(ShareService::class)->createShare([ + ['file' => UploadedFile::fake()->create('file.txt', 100), 'relativePath' => null], + ], [ + 'password' => 'correct-password', + ]); + + for ($i = 0; $i < 20; $i++) { + RateLimiter::hit('share-password:'.$share->token, 60); + } + + Livewire::test(ShareDownload::class, ['share' => $share]) + ->set('password', 'correct-password') + ->call('verifyPassword') + ->assertHasErrors(['password']) + ->assertSee('Too many attempts') + ->assertSet('authenticated', false); +}); + +// --- The system password gate holds for Livewire updates --- + +test('a file uploader snapshot from before the system password was set cannot register files in an unverified session', function () { + Storage::fake('shares'); + + preg_match_all('/wire:snapshot="([^"]+)"/', $this->get(route('upload'))->assertOk()->getContent(), $matches); + $snapshot = collect($matches[1]) + ->map(fn (string $raw): array => json_decode(htmlspecialchars_decode($raw, ENT_QUOTES), true)) + ->first(fn (array $snapshot): bool => str_contains($snapshot['memo']['name'] ?? '', 'file-uploader')); + expect($snapshot)->not->toBeNull(); + + Setting::set('system_password', Hash::make('system-secret')); + $this->flushSession(); + + $response = $this->withHeaders(['X-Livewire' => '1']) + ->postJson(app('livewire')->getUpdateUri(), [ + 'components' => [[ + 'snapshot' => json_encode($snapshot), + 'updates' => [], + 'calls' => [['method' => 'registerFiles', 'params' => [[['name' => 'file.bin', 'size' => 1024]]], 'path' => '']], + ]], + ]); + + $response->assertRedirect(route('system-password')); + expect(Share::query()->count())->toBe(0); +}); + // --- Share password minimum length --- test('share password must be at least 8 characters', function () { diff --git a/tests/Feature/Settings/PasswordUpdateTest.php b/tests/Feature/Settings/PasswordUpdateTest.php index 029fe20..afd95f8 100644 --- a/tests/Feature/Settings/PasswordUpdateTest.php +++ b/tests/Feature/Settings/PasswordUpdateTest.php @@ -1,9 +1,41 @@ forgetDrivers(); + app()->forgetInstance('session.store'); + test()->flushSession(); +} + +/** + * Sign the admin in on a database session, use it once as an admin would, and return its ID. + */ +function signedInAdminSessionId(User $admin): string +{ + config(['session.driver' => 'database']); + + $sessionId = test()->post(route('login.store'), ['email' => $admin->email, 'password' => 'Old-Password-1!']) + ->assertRedirect() + ->getCookie(config('session.cookie')) + ->getValue(); + + forgetRequestState(); + test()->withCookie(config('session.cookie'), $sessionId)->get(route('admin.dashboard'))->assertOk(); + forgetRequestState(); + + return $sessionId; +} + test('password can be updated', function () { $user = User::factory()->create([ 'password' => Hash::make('password'), @@ -38,3 +70,65 @@ test('correct password must be provided to update password', function () { $response->assertHasErrors(['current_password']); }); + +test('changing the password signs the user\'s other sessions out', function () { + $admin = User::factory()->admin()->create(['password' => Hash::make('Old-Password-1!')]); + $sessionId = signedInAdminSessionId($admin); + + Livewire::actingAs($admin)->test('pages::settings.password') + ->set('current_password', 'Old-Password-1!') + ->set('password', 'New-Password-2!xY') + ->set('password_confirmation', 'New-Password-2!xY') + ->call('updatePassword') + ->assertHasNoErrors(); + forgetRequestState(); + + $this->withCookie(config('session.cookie'), $sessionId) + ->get(route('admin.dashboard')) + ->assertRedirect(route('login')); +}); + +test('resetting the password signs the user\'s sessions out', function () { + $admin = User::factory()->admin()->create(['password' => Hash::make('Old-Password-1!')]); + $sessionId = signedInAdminSessionId($admin); + + // The reset comes from a browser that is not signed in, without the session's cookie. + $this->defaultCookies = []; + $this->post(route('password.update'), [ + 'token' => Password::broker()->createToken($admin), + 'email' => $admin->email, + 'password' => 'New-Password-3!xY', + 'password_confirmation' => 'New-Password-3!xY', + ])->assertSessionHasNoErrors(); + forgetRequestState(); + + $this->withCookie(config('session.cookie'), $sessionId) + ->get(route('admin.dashboard')) + ->assertRedirect(route('login')); +}); + +test('the session that changes the password stays signed in', function () { + $admin = User::factory()->admin()->create(['password' => Hash::make('Old-Password-1!')]); + $sessionId = signedInAdminSessionId($admin); + + preg_match('/wire:snapshot="([^"]+)"/', $this->withCookie(config('session.cookie'), $sessionId)->get(route('user-password.edit'))->assertOk()->getContent(), $matches); + forgetRequestState(); + + $this->withCookie(config('session.cookie'), $sessionId) + ->withCredentials() + ->withHeaders(['X-Livewire' => '1']) + ->postJson(app('livewire')->getUpdateUri(), [ + 'components' => [[ + 'snapshot' => htmlspecialchars_decode($matches[1], ENT_QUOTES), + 'updates' => ['current_password' => 'Old-Password-1!', 'password' => 'New-Password-2!xY', 'password_confirmation' => 'New-Password-2!xY'], + 'calls' => [['method' => 'updatePassword', 'params' => [], 'path' => '']], + ]], + ]) + ->assertOk(); + expect(Hash::check('New-Password-2!xY', $admin->refresh()->password))->toBeTrue(); + forgetRequestState(); + + $this->withCookie(config('session.cookie'), $sessionId) + ->get(route('admin.dashboard')) + ->assertOk(); +}); diff --git a/tests/Feature/Settings/ProfileUpdateTest.php b/tests/Feature/Settings/ProfileUpdateTest.php index 4f8d07b..3f775ff 100644 --- a/tests/Feature/Settings/ProfileUpdateTest.php +++ b/tests/Feature/Settings/ProfileUpdateTest.php @@ -74,3 +74,40 @@ test('correct password must be provided to delete account', function () { expect($user->fresh())->not->toBeNull(); }); + +test('the only admin cannot delete their account', function () { + $admin = User::query()->where('is_admin', true)->sole(); + + $this->actingAs($admin); + + Livewire::test('pages::settings.delete-user-form') + ->set('password', 'password') + ->call('deleteUser') + ->assertHasErrors(['password']) + ->assertNoRedirect(); + + expect($admin->fresh())->not->toBeNull(); + expect(auth()->check())->toBeTrue(); +}); + +test('an admin can delete their account while another admin remains', function () { + $admin = User::factory()->admin()->create(); + + $this->actingAs($admin); + + Livewire::test('pages::settings.delete-user-form') + ->set('password', 'password') + ->call('deleteUser') + ->assertHasNoErrors() + ->assertRedirect('/'); + + expect($admin->fresh())->toBeNull(); +}); + +test('the only admin\'s account card explains why it cannot be deleted instead of offering the button', function () { + $this->actingAs(User::query()->where('is_admin', true)->sole()); + + Livewire::test('pages::settings.delete-user-form') + ->assertSee('You are the only admin, so this account cannot be deleted.') + ->assertDontSee('data-test="delete-user-button"', false); +}); diff --git a/tests/Feature/SetupWizardTest.php b/tests/Feature/SetupWizardTest.php index 3c84c38..27acfcb 100644 --- a/tests/Feature/SetupWizardTest.php +++ b/tests/Feature/SetupWizardTest.php @@ -2,6 +2,7 @@ use App\Livewire\SetupWizard; use App\Models\User; +use Illuminate\Validation\Rules\Password; use Livewire\Livewire; test('setup wizard renders when no admin exists', function () { @@ -38,6 +39,21 @@ test('setup wizard creates admin user', function () { expect($admin->is_admin)->toBeTrue(); }); +test('the setup wizard holds the admin password to the password defaults', function () { + User::query()->where('is_admin', true)->delete(); + Password::defaults(fn () => Password::min(12)->mixedCase()->symbols()); + + Livewire::test(SetupWizard::class) + ->set('name', 'Admin User') + ->set('email', 'admin@example.com') + ->set('password', 'password123') + ->set('password_confirmation', 'password123') + ->call('createAdmin') + ->assertHasErrors(['password']); + + expect(User::query()->exists())->toBeFalse(); +}); + test('setup wizard validates required fields', function () { User::query()->where('is_admin', true)->delete(); diff --git a/tests/Feature/ShareCreatedTest.php b/tests/Feature/ShareCreatedTest.php index 1338751..d417a2a 100644 --- a/tests/Feature/ShareCreatedTest.php +++ b/tests/Feature/ShareCreatedTest.php @@ -12,7 +12,7 @@ test('the share created page offers the link as a QR code and through the share $url = route('share.download', $share); $svg = app(QrCodeService::class)->svg($url); - $response = $this->get(route('share.created', $share)); + $response = $this->withSession(['created_shares' => [$share->token]])->get(route('share.created', $share)); $response->assertOk() ->assertSee($url, false) @@ -30,7 +30,8 @@ test('the share created page offers the link as a QR code and through the share test('the QR code dialog reminds that a protected share also needs its password', function () { $share = Share::factory()->withPassword()->create(); - $this->get(route('share.created', $share)) + $this->withSession(['created_shares' => [$share->token]]) + ->get(route('share.created', $share)) ->assertOk() ->assertSee('Recipients also need the password.'); }); @@ -38,7 +39,10 @@ test('the QR code dialog reminds that a protected share also needs its password' test('the uploader who just set the password can copy it beside the link, without it being on screen', function () { $share = Share::factory()->withPassword()->create(); - $response = $this->withSession(['share_password' => ['token' => $share->token, 'password' => Crypt::encryptString('violet-orbit-canyon')]]) + $response = $this->withSession([ + 'created_shares' => [$share->token], + 'share_password' => ['token' => $share->token, 'password' => Crypt::encryptString('violet-orbit-canyon')], + ]) ->get(route('share.created', $share)); $response->assertSee('data-test="share-password"', false) @@ -52,7 +56,11 @@ test('the uploader who just set the password can copy it beside the link, withou test('the password is not shown without a flash for this share', function (?string $flashedFor) { $share = Share::factory()->withPassword()->create(); - $session = $flashedFor === null ? [] : ['share_password' => ['token' => $flashedFor, 'password' => Crypt::encryptString('violet-orbit-canyon')]]; + $session = ['created_shares' => [$share->token]]; + + if ($flashedFor !== null) { + $session['share_password'] = ['token' => $flashedFor, 'password' => Crypt::encryptString('violet-orbit-canyon')]; + } $response = $this->withSession($session)->get(route('share.created', $share)); @@ -75,8 +83,23 @@ test('the Private text stat shows for a share with text, and the Files stat excl 'the secret note', ); - $response = $this->get(route('share.created', $share)); + $response = $this->withSession(['created_shares' => [$share->token]])->get(route('share.created', $share)); $response->assertOk() ->assertSeeInOrder(['Files', '2', 'Private text']); }); + +test('the share created page is not found for a session that did not create the share', function () { + $share = Share::factory()->withPassword()->create(); + + $this->get(route('share.created', $share))->assertNotFound(); +}); + +test('the share created page is not found once the share is no longer reachable', function (Share $share) { + $this->withSession(['created_shares' => [$share->token]]) + ->get(route('share.created', $share)) + ->assertNotFound(); +})->with([ + 'expired' => fn () => Share::factory()->expired()->create(), + 'at its download limit' => fn () => Share::factory()->withMaxDownloads(1)->create(['download_count' => 1]), +]); diff --git a/tests/Feature/UploadChunkTest.php b/tests/Feature/UploadChunkTest.php index 1aa27c1..34fb1e5 100644 --- a/tests/Feature/UploadChunkTest.php +++ b/tests/Feature/UploadChunkTest.php @@ -1,6 +1,7 @@ refresh()->uploaded_chunks)->toBe(1); }); +test('a chunk that takes the stored bytes beyond the storage quota returns 422', function () { + Storage::fake('shares'); + config(['uploads.chunk_size' => 4]); + Setting::set('max_storage_quota', 100); + $file = app(ShareService::class)->registerFile(null, 'notes.txt', 6, null); + Share::factory()->create(['total_size' => 98]); + + $response = putChunk($this, $file, 0, encryptedChunk($file, 'abcd', 0, false), [$file->share->token]); + + $response->assertStatus(422); + expect($file->refresh()->uploaded_chunks)->toBe(0); +}); + test('a chunk for a pending share another session started returns 404', function () { Storage::fake('shares'); $file = app(ShareService::class)->registerFile(null, 'notes.txt', 4, null); diff --git a/tests/Screenshots/ScreenshotsTest.php b/tests/Screenshots/ScreenshotsTest.php index 0a8f3bc..b7b6644 100644 --- a/tests/Screenshots/ScreenshotsTest.php +++ b/tests/Screenshots/ScreenshotsTest.php @@ -172,6 +172,7 @@ test('phone', function (string $theme) use ($files) { $page->script("Alpine.\$data(document.querySelector('[data-test=share-file-list]')).supported = false"); shoot($page, 'phone', $theme, '03-download'); + $this->withSession(['created_shares' => [DemoData::DELIVERY_TOKEN]]); $created = visit(route('share.created', DemoData::DELIVERY_TOKEN, false)); $page = shotPage($created, 'phone', $theme); $page->click('[data-test="show-qr-code"]') diff --git a/tests/Unit/ShareServiceTest.php b/tests/Unit/ShareServiceTest.php index 47e32ba..16bc770 100644 --- a/tests/Unit/ShareServiceTest.php +++ b/tests/Unit/ShareServiceTest.php @@ -230,14 +230,36 @@ test('a file that takes the share beyond the admin size per share is rejected', ->toThrow(ValidationException::class, 'Total file size exceeds the maximum allowed per share.'); }); -test('a file that does not fit the storage quota beside files still uploading is rejected', function () { +test('a file that does not fit the storage quota beside the bytes already received is rejected', function () { + config(['uploads.chunk_size' => 10]); Setting::set('max_storage_quota', 100); - Share::factory()->pending()->create(['total_size' => 60]); + $pending = Share::factory()->pending()->create(['total_size' => 60]); + ShareFile::factory()->for($pending)->uploading()->create(['file_size' => 60, 'uploaded_chunks' => 1]); - expect(fn () => $this->service->registerFile(null, 'file.txt', 50, null)) + expect(fn () => $this->service->registerFile(null, 'file.txt', 95, null)) ->toThrow(ValidationException::class, 'Storage is full. Please contact the administrator.'); }); +test('a file still uploading without received chunks does not use up the storage quota', function () { + config(['uploads.chunk_size' => 10]); + Setting::set('max_storage_quota', 100); + $pending = Share::factory()->pending()->create(['total_size' => 60]); + ShareFile::factory()->for($pending)->uploading()->create(['file_size' => 60, 'uploaded_chunks' => 0]); + + expect($this->service->registerFile(null, 'file.txt', 50, null))->toBeInstanceOf(ShareFile::class); +}); + +test('a chunk that takes the stored bytes beyond the storage quota is rejected', function () { + config(['uploads.chunk_size' => 4]); + Setting::set('max_storage_quota', 100); + $file = $this->service->registerFile(null, 'notes.txt', 6, null); + Share::factory()->create(['total_size' => 98]); + + expect(fn () => $this->service->storeChunk($file, 0, encryptedChunk($file, 'abcd', 0, false))) + ->toThrow(InvalidArgumentException::class, 'Chunk 0 exceeds the storage quota'); + expect($file->refresh()->uploaded_chunks)->toBe(0); +}); + test('chunks stored in order complete the file', function () { config(['uploads.chunk_size' => 4]); $file = $this->service->registerFile(null, 'notes.txt', 6, null);