From ebb2e029bbd72749155922e3348bded00c41dede Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:34 +0000 Subject: [PATCH 1/8] Fetch over the cURL transport so address pinning works Real hostname fetches failed: asking for a streamed response made Guzzle 8 route the request to its PHP-stream handler, which rejects the CURLOPT_RESOLVE pin (Guzzle 7 would have ignored it silently). The tests replaced the transport with Http::fake() and could not see it. SafeHttpClient now always uses Guzzle's cURL handler and enforces the size cap with a progress callback that aborts the transfer. A new RealTransportTest goes through the real transport: a made-up hostname is pointed at a local PHP server, which is only reachable through the pin. Oversized responses throw ResponseTooLargeException. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Exceptions/ResponseTooLargeException.php | 15 ++ app/Services/Fetching/IpGuard.php | 10 ++ app/Services/Fetching/SafeHttpClient.php | 68 ++++++---- tests/Feature/Items/RealTransportTest.php | 136 +++++++++++++++++++ tests/Fixtures/server/router.php | 36 +++++ 5 files changed, 240 insertions(+), 25 deletions(-) create mode 100644 app/Exceptions/ResponseTooLargeException.php create mode 100644 tests/Feature/Items/RealTransportTest.php create mode 100644 tests/Fixtures/server/router.php diff --git a/app/Exceptions/ResponseTooLargeException.php b/app/Exceptions/ResponseTooLargeException.php new file mode 100644 index 0000000..3e89eb4 --- /dev/null +++ b/app/Exceptions/ResponseTooLargeException.php @@ -0,0 +1,15 @@ +vet($url); - $response = $this->send($url, $host, $port, $address, $accept); + $response = $this->send($url, $host, $port, $address, $accept, $maxBytes, $timeout ?? (int) settings('fetch.timeout')); $status = $response->status(); $location = $response->header('Location'); @@ -73,7 +87,7 @@ public function get(string $url, int $maxBytes, string $accept = '*/*'): Fetched $length = $response->header('Content-Length'); if (is_numeric($length) && (int) $length > $maxBytes) { - throw FetchFailedException::permanent($this->tooLargeMessage($maxBytes)); + throw new ResponseTooLargeException($this->tooLargeMessage($maxBytes), $maxBytes); } return new FetchedResponse( @@ -81,7 +95,7 @@ public function get(string $url, int $maxBytes, string $accept = '*/*'): Fetched status: $status, contentType: $contentType, charset: $charset, - body: $this->readBody($response, $maxBytes), + body: $this->body($response, $maxBytes), ); } } @@ -112,7 +126,7 @@ public function vet(string $url): array } foreach ($addresses as $address) { - if (! IpGuard::isPublic($address)) { + if (! $this->guard->allows($address)) { throw new UnsafeUrlException("Refused to fetch {$host}: it points at a private or local network address."); } } @@ -123,7 +137,7 @@ public function vet(string $url): array /** * @throws FetchFailedException */ - protected function send(string $url, string $host, int $port, string $address, string $accept): Response + protected function send(string $url, string $host, int $port, string $address, string $accept, int $maxBytes, int $timeout): Response { $pinned = str_contains($address, ':') ? "[{$address}]" : $address; @@ -132,46 +146,50 @@ protected function send(string $url, string $host, int $port, string $address, s ? [CURLOPT_RESOLVE => ["{$host}:{$port}:{$pinned}"]] : []; + $tooLarge = false; + try { return Http::withHeaders([ 'User-Agent' => (string) config('docket.fetch.user_agent'), 'Accept' => $accept, 'Accept-Language' => 'en-GB,en;q=0.9,*;q=0.5', ]) - ->timeout((int) settings('fetch.timeout')) + ->setHandler(new CurlHandler()) + ->timeout(max(1, $timeout)) ->connectTimeout((int) config('docket.fetch.connect_timeout', 10)) ->withoutRedirecting() ->withOptions([ - 'stream' => true, 'protocols' => ['http', 'https'], 'curl' => $curl, + // Returning true aborts the transfer. The expected size + // is known as soon as the headers arrive, so an + // oversized Content-Length stops before the body. + 'progress' => function (int $expected, int $downloaded) use ($maxBytes, &$tooLarge): bool { + return $tooLarge = $expected > $maxBytes || $downloaded > $maxBytes; + }, ]) ->get($url); - } catch (ConnectionException $e) { + } catch (ConnectionException|TransferException $e) { + if ($tooLarge) { + throw new ResponseTooLargeException($this->tooLargeMessage($maxBytes), $maxBytes); + } + throw new FetchFailedException('Could not connect: '.$this->shortError($e->getMessage())); } } /** - * Read the body in chunks, giving up as soon as it passes the cap. + * The body, with the cap checked again for transports that don't report + * progress (Http::fake() in tests). * * @throws FetchFailedException */ - protected function readBody(Response $response, int $maxBytes): string + protected function body(Response $response, int $maxBytes): string { - $stream = $response->toPsrResponse()->getBody(); - $body = ''; + $body = $response->body(); - try { - while (! $stream->eof()) { - $body .= $stream->read(65536); - - if (strlen($body) > $maxBytes) { - throw FetchFailedException::permanent($this->tooLargeMessage($maxBytes)); - } - } - } finally { - $stream->close(); + if (strlen($body) > $maxBytes) { + throw new ResponseTooLargeException($this->tooLargeMessage($maxBytes), $maxBytes); } return $body; diff --git a/tests/Feature/Items/RealTransportTest.php b/tests/Feature/Items/RealTransportTest.php new file mode 100644 index 0000000..dba8273 --- /dev/null +++ b/tests/Feature/Items/RealTransportTest.php @@ -0,0 +1,136 @@ +markTestSkipped('The curl extension is required.'); + } + + $this->port = $this->freePort(); + $this->server = new Process([PHP_BINARY, '-S', "127.0.0.1:{$this->port}", base_path('tests/Fixtures/server/router.php')]); + $this->server->start(); + $this->waitForServer(); + + Http::allowStrayRequests(); + $this->dns->set(self::HOST, '127.0.0.1'); + $this->app->instance(IpGuard::class, new class() extends IpGuard + { + public function allows(string $ip): bool + { + return $ip === '127.0.0.1'; + } + }); + } + + protected function tearDown(): void + { + $this->server->stop(0); + + parent::tearDown(); + } + + private function url(string $path): string + { + return 'http://'.self::HOST.':'.$this->port.$path; + } + + public function test_a_hostname_is_fetched_through_the_pinned_address(): void + { + $response = app(SafeHttpClient::class)->get($this->url('/article'), 10_000); + + $this->assertSame(200, $response->status); + $this->assertSame('text/html', $response->contentType); + $this->assertSame('utf-8', $response->charset); + // The Host header is still the hostname: only the connection is pinned. + $this->assertStringContainsString('Served to '.self::HOST.':'.$this->port, $response->body); + } + + public function test_redirects_are_followed_over_the_real_transport(): void + { + $response = app(SafeHttpClient::class)->get($this->url('/redirect'), 10_000); + + $this->assertSame($this->url('/article'), $response->url); + } + + public function test_the_size_cap_aborts_a_declared_oversized_body(): void + { + $this->expectException(FetchFailedException::class); + $this->expectExceptionMessage('larger than'); + + app(SafeHttpClient::class)->get($this->url('/declared-large'), 10_000); + } + + public function test_the_size_cap_aborts_while_an_undeclared_body_downloads(): void + { + $this->expectException(FetchFailedException::class); + $this->expectExceptionMessage('larger than'); + + app(SafeHttpClient::class)->get($this->url('/streamed-large'), 10_000); + } + + public function test_a_hostname_on_a_blocked_address_never_connects(): void + { + $this->app->instance(IpGuard::class, new IpGuard()); + + $this->expectException(FetchFailedException::class); + $this->expectExceptionMessage('private or local network address'); + + app(SafeHttpClient::class)->get($this->url('/article'), 10_000); + } + + private function freePort(): int + { + $socket = stream_socket_server('tcp://127.0.0.1:0', $errno, $error); + $this->assertNotFalse($socket, $error); + $name = (string) stream_socket_get_name($socket, false); + fclose($socket); + + return (int) substr($name, strrpos($name, ':') + 1); + } + + private function waitForServer(): void + { + for ($i = 0; $i < 50; $i++) { + $connection = @fsockopen('127.0.0.1', $this->port, $errno, $error, 0.1); + + if ($connection !== false) { + fclose($connection); + + return; + } + + usleep(100_000); + } + + $this->fail('The fixture server did not start: '.$this->server->getErrorOutput()); + } +} diff --git a/tests/Fixtures/server/router.php b/tests/Fixtures/server/router.php new file mode 100644 index 0000000..df496c6 --- /dev/null +++ b/tests/Fixtures/server/router.php @@ -0,0 +1,36 @@ +Fixture

Served to ' + .htmlspecialchars($_SERVER['HTTP_HOST'] ?? '').'

'; + break; + + case '/redirect': + header('Location: /article', true, 302); + break; + + case '/declared-large': + header('Content-Type: text/html'); + header('Content-Length: 50000'); + echo str_repeat('a', 50000); + break; + + case '/streamed-large': + // No Content-Length: the cap has to trip while the body arrives. + header('Content-Type: text/html'); + for ($i = 0; $i < 50; $i++) { + echo str_repeat('b', 1000); + flush(); + } + break; + + default: + http_response_code(404); +} From f9b82d2fe4bfcd9f747d9ab27c48990fb11f58c2 Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:34 +0000 Subject: [PATCH 2/8] Make installation and seeding unable to create a second account - The web installer asks for a one-time setup code that can only be read on the server, so a stranger who finds a fresh deployment can't claim it. - Installations take a non-blocking lock and re-check ownership inside it; createOwner() refuses once any account exists, whatever its email. - isInstalled() fails closed: an installed marker keeps /install shut, and a database error other than a missing SQLite file answers 503 instead of reopening the installer. .env is only written once the account exists. - The seeder never adds a user when one exists and no longer falls back to a published password. Installer state lives in config('docket.install.state_path'); tests get their own directory. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Console/Commands/DocketInstallCommand.php | 82 ++++++--- .../InstallStateUnknownException.php | 12 ++ app/Http/Controllers/InstallController.php | 79 ++++++--- app/Http/Middleware/EnsureNotInstalled.php | 15 +- app/Services/Installer.php | 163 +++++++++++++++++- config/docket.php | 36 +++- database/seeders/DatabaseSeeder.php | 32 +++- resources/views/install/setup.blade.php | 15 +- resources/views/install/unavailable.blade.php | 11 ++ tests/Feature/DatabaseSeederTest.php | 57 ++++++ tests/Feature/InstallCommandTest.php | 28 +++ tests/Feature/WebInstallerTest.php | 88 ++++++++++ tests/TestCase.php | 18 ++ 13 files changed, 555 insertions(+), 81 deletions(-) create mode 100644 app/Exceptions/InstallStateUnknownException.php create mode 100644 resources/views/install/unavailable.blade.php create mode 100644 tests/Feature/DatabaseSeederTest.php diff --git a/app/Console/Commands/DocketInstallCommand.php b/app/Console/Commands/DocketInstallCommand.php index 63bfb18..26983e7 100644 --- a/app/Console/Commands/DocketInstallCommand.php +++ b/app/Console/Commands/DocketInstallCommand.php @@ -2,6 +2,7 @@ namespace App\Console\Commands; +use App\Exceptions\InstallStateUnknownException; use App\Services\Installer; use Illuminate\Console\Attributes\Description; use Illuminate\Console\Attributes\Signature; @@ -41,7 +42,15 @@ public function handle(Installer $installer): int return SymfonyCommand::FAILURE; } - if ($installer->isInstalled() && ! $this->option('force')) { + try { + $installed = $installer->isInstalled(); + } catch (InstallStateUnknownException $e) { + $this->components->error($e->getMessage()); + + return SymfonyCommand::FAILURE; + } + + if ($installed && ! $this->option('force')) { $this->components->warn( 'Docket already has a user account. Re-run with --force to run the installer anyway (existing data is never deleted).' ); @@ -95,32 +104,51 @@ public function handle(Installer $installer): int return SymfonyCommand::FAILURE; } - // Persist the identity to .env so a later `db:seed` recreates the - // same account rather than the config/docket.php fallback owner. - // The password is deliberately NOT written to .env: it is hashed - // straight into the database below. - $installer->writeEnvValues([ - 'APP_URL' => $url, - 'DOCKET_USER_NAME' => $name, - 'DOCKET_USER_EMAIL' => $email, - ]); - $this->components->task('.env updated (APP_URL, owner name/email)', fn (): bool => true); - - $installer->ensureSqliteDatabase(); - - $this->components->task('running database migrations', fn (): bool => $installer->migrate()); - - if ($installer->ownerExists($email)) { - $this->components->warn("An account for {$email} already exists — its password was left unchanged."); - } else { - $this->components->task( - "creating owner account for {$email}", - function () use ($installer, $name, $email, $plainPassword): bool { - $installer->createOwner($name, $email, $plainPassword); - - return true; - } - ); + // Serialised with the web installer, so the two can't race each + // other into creating two accounts. + $lock = $installer->acquireLock(); + + if ($lock === null) { + $this->components->error('Another installation is running right now. Try again in a moment.'); + + return SymfonyCommand::FAILURE; + } + + try { + $installer->ensureSqliteDatabase(); + + $this->components->task('running database migrations', fn (): bool => $installer->migrate()); + + if ($installer->ownerExists($email)) { + $this->components->warn("An account for {$email} already exists — its password was left unchanged."); + } elseif ($installer->hasOwner()) { + $this->components->warn('Docket already has its owner account, under a different email. It has one account, so no new one was created; change the email on the Profile page instead.'); + + return SymfonyCommand::FAILURE; + } else { + $this->components->task( + "creating owner account for {$email}", + function () use ($installer, $name, $email, $plainPassword): bool { + $installer->createOwner($name, $email, $plainPassword); + + return true; + } + ); + } + + // Written once the account is in place, so a refused install + // leaves .env alone. The password is deliberately NOT written + // to .env: it is hashed straight into the database. + $installer->writeEnvValues([ + 'APP_URL' => $url, + 'DOCKET_USER_NAME' => $name, + 'DOCKET_USER_EMAIL' => $email, + ]); + $this->components->task('.env updated (APP_URL, owner name/email)', fn (): bool => true); + + $installer->markInstalled(); + } finally { + $installer->releaseLock($lock); } $this->printNextSteps($url, $email); diff --git a/app/Exceptions/InstallStateUnknownException.php b/app/Exceptions/InstallStateUnknownException.php new file mode 100644 index 0000000..57af7c3 --- /dev/null +++ b/app/Exceptions/InstallStateUnknownException.php @@ -0,0 +1,12 @@ +installer->blockingProblems() !== []) { return redirect()->route('install.requirements'); } - return view('install.setup', [ - 'input' => ['url' => $request->getSchemeAndHttpHost()], - 'errors' => new MessageBag(), - 'installError' => null, - ]); + // Creates the code on first visit, so it is there to read. + $this->installer->setupCode(); + + return $this->setupView(['url' => $request->getSchemeAndHttpHost()], new MessageBag(), null); } /** @@ -59,36 +58,43 @@ public function store(Request $request): View|RedirectResponse|Response } $validator = Validator::make($request->all(), [ + 'setup_code' => ['required', 'string', 'max:64'], 'url' => ['required', 'string', 'url', 'max:255'], 'name' => ['required', 'string', 'max:255'], 'email' => ['required', 'string', 'email', 'max:255'], 'password' => ['required', 'string', 'min:8', 'confirmed'], ]); + $validator->after(function ($validator) use ($request): void { + if (is_string($request->input('setup_code')) && ! $this->installer->checkSetupCode($request->input('setup_code'))) { + $validator->errors()->add('setup_code', 'That setup code is not right. Copy it from the file named below.'); + } + }); + $safeInput = $request->except(['password', 'password_confirmation']); if ($validator->fails()) { - return response()->view('install.setup', [ - 'input' => $safeInput, - 'errors' => $validator->errors(), - 'installError' => null, - ], 422); + return $this->setupView($safeInput, $validator->errors(), null, 422); } $data = $validator->validated(); + // One installation at a time, and the "already installed?" question + // asked again inside the lock: two requests that both got past the + // middleware can't both create an owner. + $lock = $this->installer->acquireLock(); + + if ($lock === null) { + return $this->setupView($safeInput, new MessageBag(), 'Another installation is running right now. Wait a moment, then reload.', 409); + } + try { + if ($this->installer->isInstalled()) { + return redirect()->route('login'); + } + $this->installer->ensureEnvFile(); $this->installer->ensureAppKey(); - - // The password is deliberately NOT written to .env: it is - // hashed straight into the database below. - $this->installer->writeEnvValues([ - 'APP_URL' => $data['url'], - 'DOCKET_USER_NAME' => $data['name'], - 'DOCKET_USER_EMAIL' => $data['email'], - ]); - $this->installer->ensureSqliteDatabase(); if (! $this->installer->migrate()) { @@ -96,14 +102,24 @@ public function store(Request $request): View|RedirectResponse|Response } $this->installer->createOwner($data['name'], $data['email'], $data['password']); + + // Configuration changes only once the account exists, so a + // failed or refused install leaves .env as it was. The password + // is deliberately NOT written to .env: it is hashed into the + // database above. + $this->installer->writeEnvValues([ + 'APP_URL' => $data['url'], + 'DOCKET_USER_NAME' => $data['name'], + 'DOCKET_USER_EMAIL' => $data['email'], + ]); + + $this->installer->markInstalled(); } catch (Throwable $e) { report($e); - return response()->view('install.setup', [ - 'input' => $safeInput, - 'errors' => new MessageBag(), - 'installError' => $e->getMessage(), - ], 500); + return $this->setupView($safeInput, new MessageBag(), $e->getMessage(), 500); + } finally { + $this->installer->releaseLock($lock); } return view('install.complete', [ @@ -112,4 +128,17 @@ public function store(Request $request): View|RedirectResponse|Response 'assetsBuilt' => file_exists(public_path('build/manifest.json')), ]); } + + /** + * @param array $input + */ + protected function setupView(array $input, MessageBag $errors, ?string $installError, int $status = 200): Response + { + return response()->view('install.setup', [ + 'input' => $input, + 'errors' => $errors, + 'installError' => $installError, + 'codeLocation' => $this->installer->setupCodeLocation(), + ], $status); + } } diff --git a/app/Http/Middleware/EnsureNotInstalled.php b/app/Http/Middleware/EnsureNotInstalled.php index b6620ad..78cbda5 100644 --- a/app/Http/Middleware/EnsureNotInstalled.php +++ b/app/Http/Middleware/EnsureNotInstalled.php @@ -2,6 +2,7 @@ namespace App\Http\Middleware; +use App\Exceptions\InstallStateUnknownException; use App\Services\Installer; use Closure; use Illuminate\Http\Request; @@ -10,7 +11,9 @@ /** * Gates the web installer: once Docket has its owner account the /install * routes stop existing in any useful sense and just bounce to the login - * screen, so a live site never exposes a working installer. + * screen, so a live site never exposes a working installer. If the + * database can't be read, the installer stays closed too (503) rather + * than mistaking a broken site for a fresh one. */ class EnsureNotInstalled { @@ -23,7 +26,15 @@ public function __construct(protected Installer $installer) {} */ public function handle(Request $request, Closure $next): Response { - if ($this->installer->isInstalled()) { + try { + $installed = $this->installer->isInstalled(); + } catch (InstallStateUnknownException $e) { + report($e); + + return response()->view('install.unavailable', ['message' => $e->getMessage()], 503); + } + + if ($installed) { return redirect()->route('login'); } diff --git a/app/Services/Installer.php b/app/Services/Installer.php index 966ba64..36d0aa6 100644 --- a/app/Services/Installer.php +++ b/app/Services/Installer.php @@ -2,11 +2,14 @@ namespace App\Services; +use App\Exceptions\InstallStateUnknownException; use App\Models\User; use Illuminate\Contracts\Foundation\Application; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\Schema; +use Illuminate\Support\Str; use PDO; +use RuntimeException; use Throwable; /** @@ -14,24 +17,128 @@ * (php artisan docket:install) and the web installer (/install). * Each step is a small idempotent method so either front-end can run * them one at a time and narrate progress in its own way. + * + * Docket has exactly one account, and the web installer is reachable by + * anyone who finds a fresh deployment, so installation is guarded three + * ways: the web form needs a setup code that can only be read on the + * server; installations are serialised by a lock and re-check ownership + * inside it; and createOwner() refuses outright once any account exists. + * Once installed, a marker file keeps the installer closed even when the + * database can't be read. */ class Installer { public function __construct(protected Application $app) {} /** - * Docket counts as installed once the owner account exists — the - * final step of either installer. Anything earlier (missing .env, - * empty database) is a fresh or half-finished install. + * Docket counts as installed once the installed marker exists or an + * account does. A missing SQLite file is a fresh install; any other + * database error is not proof of anything, so it fails closed rather + * than reopening the installer on a live site. + * + * @throws InstallStateUnknownException */ public function isInstalled(): bool { + if (is_file($this->statePath('installed'))) { + return true; + } + try { - return Schema::hasTable('users') && User::query()->exists(); - } catch (Throwable) { - // No database yet — definitely a fresh install. - return false; + $installed = Schema::hasTable('users') && User::query()->exists(); + } catch (Throwable $e) { + if ($this->databaseIsMissing()) { + return false; + } + + throw new InstallStateUnknownException( + 'Docket could not read its database to check whether it is installed, so the installer stays closed. Check storage/logs/laravel.log.', + previous: $e, + ); + } + + if ($installed) { + // Installs from before the marker existed get one now. + $this->markInstalled(); + } + + return $installed; + } + + /** + * Record a finished installation and retire the setup code. + */ + public function markInstalled(): void + { + $this->ensureStateDirectory(); + @file_put_contents($this->statePath('installed'), now()->toIso8601String()."\n"); + @unlink($this->statePath('setup-code')); + } + + /** + * The one-time code the web installer asks for, created on first use. + * Reading it needs shell access to the server, which is what proves + * the person at /install is the person deploying Docket. + */ + public function setupCode(): string + { + $path = $this->statePath('setup-code'); + + if (! is_file($path)) { + $this->ensureStateDirectory(); + file_put_contents($path, implode('-', str_split(Str::lower(Str::random(16)), 4))."\n"); + @chmod($path, 0600); + } + + return trim((string) file_get_contents($path)); + } + + public function checkSetupCode(string $given): bool + { + return hash_equals($this->setupCode(), strtolower(trim($given))); + } + + /** + * Where the setup code lives, relative to the app root when it is + * inside it, for showing in the installer. + */ + public function setupCodeLocation(): string + { + return Str::after($this->statePath('setup-code'), $this->app->basePath().DIRECTORY_SEPARATOR); + } + + /** + * Take the installation lock without waiting: a second installation + * running at the same moment is told to try again rather than queued + * behind the first. + * + * @return resource|null the lock handle, or null when it is held + */ + public function acquireLock() + { + $this->ensureStateDirectory(); + $handle = fopen($this->statePath('install.lock'), 'c'); + + if ($handle === false) { + throw new RuntimeException('Could not open the installation lock file.'); + } + + if (! flock($handle, LOCK_EX | LOCK_NB)) { + fclose($handle); + + return null; } + + return $handle; + } + + /** + * @param resource $handle + */ + public function releaseLock($handle): void + { + flock($handle, LOCK_UN); + fclose($handle); } /** @@ -56,7 +163,7 @@ public function requirements(): array ], ]; - foreach (['pdo_sqlite', 'mbstring', 'fileinfo', 'dom', 'xml'] as $extension) { + foreach (['pdo_sqlite', 'mbstring', 'fileinfo', 'dom', 'xml', 'curl'] as $extension) { $checks[] = [ 'label' => "PHP extension: {$extension}", 'ok' => extension_loaded($extension), @@ -230,8 +337,23 @@ public function ownerExists(string $email): bool return User::query()->where('email', $email)->exists(); } + /** + * Whether any account exists. Docket has one, whatever its email. + */ + public function hasOwner(): bool + { + return User::query()->exists(); + } + + /** + * @throws RuntimeException when an account already exists + */ public function createOwner(string $name, string $email, string $plainPassword): User { + if ($this->hasOwner()) { + throw new RuntimeException('Docket already has its owner account; it does not create a second one.'); + } + // The User model's 'hashed' cast hashes the plain password. return User::query()->create([ 'name' => $name, @@ -240,6 +362,31 @@ public function createOwner(string $name, string $email, string $plainPassword): ]); } + protected function statePath(string $file): string + { + return rtrim((string) config('docket.install.state_path'), DIRECTORY_SEPARATOR).DIRECTORY_SEPARATOR.$file; + } + + protected function ensureStateDirectory(): void + { + $directory = dirname($this->statePath('x')); + + if (! is_dir($directory)) { + mkdir($directory, 0700, true); + } + } + + /** + * True only for a fresh install: SQLite whose database file doesn't + * exist yet (or is still empty). + */ + protected function databaseIsMissing(): bool + { + $path = $this->sqlitePath(); + + return $path !== null && (! is_file($path) || filesize($path) === 0); + } + /** * Whether this PHP's SQLite library was compiled with FTS5, which the * items_fts search index needs. diff --git a/config/docket.php b/config/docket.php index b67ffae..a2cb84a 100644 --- a/config/docket.php +++ b/config/docket.php @@ -17,17 +17,27 @@ /* | The single owner account. There is no registration flow: the installer - | (php artisan docket:install or /install) creates the account directly, - | and these are the fallbacks DatabaseSeeder uses if the database is ever - | re-seeded from scratch. Once the user exists the database is the source - | of truth, so changing these afterwards has no effect on the live login. + | (php artisan docket:install or /install) creates the account directly. + | DatabaseSeeder uses these only on an empty users table, and refuses to + | run without DOCKET_USER_PASSWORD: there is deliberately no default + | password. Once the user exists the database is the source of truth. */ 'user' => [ // ?: not the env() default param: a present-but-empty line in .env // (e.g. DOCKET_USER_NAME=) would otherwise seed an empty string. 'name' => env('DOCKET_USER_NAME') ?: 'Docket Owner', 'email' => env('DOCKET_USER_EMAIL') ?: 'owner@example.com', - 'password' => env('DOCKET_USER_PASSWORD') ?: 'change-me-on-first-login', + 'password' => env('DOCKET_USER_PASSWORD') ?: null, + ], + + /* + | Installer state: the "installed" marker that keeps /install closed even + | if the database is briefly unreadable, the one-time setup code the web + | installer asks for, and the lock that serialises installations. Outside + | public/, and readable only from the server. + */ + 'install' => [ + 'state_path' => env('DOCKET_INSTALL_STATE_PATH') ?: storage_path('app/install'), ], /* @@ -53,16 +63,24 @@ ], /* - | Inline images are downloaded to private storage so saved articles - | survive link rot. SVG is never stored locally: it can carry scripts and - | the copies are served from this origin. + | Inline and lead images are downloaded to private storage so saved + | articles survive link rot. SVG is never stored locally: it can carry + | scripts and the copies are served from this origin. Every limit below + | is per article and counts attempts, not just successes, so a page full + | of broken images can't keep a worker busy; images past a limit keep + | their remote src. */ 'images' => [ 'enabled' => true, // Per image, in kilobytes (5 MB). 'max_kb' => 5120, - // Images per article; the rest keep their remote src. + // Download attempts per article, lead image included. 'max_per_item' => 40, + // Total downloaded per article, in kilobytes (25 MB). + 'max_total_kb' => 25600, + // Seconds an article may spend downloading images, kept well inside + // the fetch job's 300-second timeout. + 'time_budget' => 120, ], /* diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php index cb7fe79..a0087b3 100644 --- a/database/seeders/DatabaseSeeder.php +++ b/database/seeders/DatabaseSeeder.php @@ -5,19 +5,33 @@ use App\Models\User; use Illuminate\Database\Seeder; use Illuminate\Support\Facades\Hash; +use RuntimeException; class DatabaseSeeder extends Seeder { public function run(): void { - // Single-user app: one seeded account, no registration flow. - // Set DOCKET_USER_PASSWORD in .env before seeding in production. - User::firstOrCreate( - ['email' => config('docket.user.email')], - [ - 'name' => config('docket.user.name'), - 'password' => Hash::make(config('docket.user.password')), - ] - ); + // Single-user app. The installer creates the owner; this exists for + // rebuilding an empty database and never adds a second account, + // whatever email the existing one now has. + if (User::query()->exists()) { + $this->command->info('An owner account already exists, so no user was seeded.'); + + return; + } + + $password = config('docket.user.password'); + + // No published fallback: a seeded account with a known password + // would open the whole library to anyone who read the source. + if (! is_string($password) || $password === '') { + throw new RuntimeException('Set DOCKET_USER_PASSWORD in .env before seeding, or run php artisan docket:install.'); + } + + User::query()->create([ + 'name' => config('docket.user.name'), + 'email' => config('docket.user.email'), + 'password' => Hash::make($password), + ]); } } diff --git a/resources/views/install/setup.blade.php b/resources/views/install/setup.blade.php index feb3d5c..7bb04d5 100644 --- a/resources/views/install/setup.blade.php +++ b/resources/views/install/setup.blade.php @@ -14,11 +14,24 @@ @endif {{-- No @csrf: these routes run before the session store exists and are - disabled entirely once the owner account has been created. + disabled entirely once the owner account has been created. The + setup code does the job CSRF would: a request that can't read the + server's files can't supply it. Relative action, not route(): before .env exists the app defaults to APP_ENV=production and forces https URLs, which breaks installs served over plain http. --}}
+ + + + Proves you control the server. Read it there: + cat {{ $codeLocation }} + + @if ($errors->has('setup_code')) +

{{ $errors->first('setup_code') }}

+ @endif + diff --git a/resources/views/install/unavailable.blade.php b/resources/views/install/unavailable.blade.php new file mode 100644 index 0000000..5cace7b --- /dev/null +++ b/resources/views/install/unavailable.blade.php @@ -0,0 +1,11 @@ +@extends('install.layout') + +@section('content') +

The installer is unavailable

+ +

+ Docket couldn't confirm whether it is already installed, so it won't + offer to install itself. Fix the database connection (or its file + permissions), then reload this page. +

+@endsection diff --git a/tests/Feature/DatabaseSeederTest.php b/tests/Feature/DatabaseSeederTest.php new file mode 100644 index 0000000..ba49f1d --- /dev/null +++ b/tests/Feature/DatabaseSeederTest.php @@ -0,0 +1,57 @@ + 'old-owner@example.com', 'docket.user.password' => 'some-password-1']); + User::factory()->create(['email' => 'new-owner@example.com']); + + Artisan::call('db:seed', ['--class' => DatabaseSeeder::class, '--force' => true]); + + $this->assertSame(['new-owner@example.com'], User::query()->pluck('email')->all()); + } + + public function test_seeding_an_empty_database_needs_an_explicit_password(): void + { + config(['docket.user.password' => null]); + + try { + Artisan::call('db:seed', ['--class' => DatabaseSeeder::class, '--force' => true]); + $this->fail('Seeding without DOCKET_USER_PASSWORD should refuse.'); + } catch (RuntimeException $e) { + $this->assertStringContainsString('DOCKET_USER_PASSWORD', $e->getMessage()); + } + + $this->assertSame(0, User::query()->count()); + } + + public function test_seeding_an_empty_database_with_a_password(): void + { + config([ + 'docket.user.name' => 'Owner', + 'docket.user.email' => 'owner@example.com', + 'docket.user.password' => 'chosen-password-1', + ]); + + Artisan::call('db:seed', ['--class' => DatabaseSeeder::class, '--force' => true]); + + $user = User::query()->sole(); + $this->assertSame('owner@example.com', $user->email); + $this->assertTrue(Hash::check('chosen-password-1', $user->password)); + } +} diff --git a/tests/Feature/InstallCommandTest.php b/tests/Feature/InstallCommandTest.php index c8c4ce6..c1fff5a 100644 --- a/tests/Feature/InstallCommandTest.php +++ b/tests/Feature/InstallCommandTest.php @@ -142,4 +142,32 @@ public function test_rejects_short_password(): void $this->assertSame(0, User::query()->count()); } + + public function test_force_never_adds_a_second_account_under_another_email(): void + { + User::factory()->create(['email' => 'owner@example.com']); + + $this->artisan('docket:install', [ + '--url' => 'http://localhost', + '--name' => 'Someone Else', + '--email' => 'someone@example.com', + '--password' => 'another-password-1', + '--force' => true, + ])->assertFailed(); + + $this->assertSame(['owner@example.com'], User::query()->pluck('email')->all()); + $this->assertStringNotContainsString('someone@example.com', $this->envContents()); + } + + public function test_installing_leaves_the_installed_marker(): void + { + $this->artisan('docket:install', [ + '--url' => 'http://localhost', + '--email' => 'jane@example.com', + '--password' => 'super-secret-99', + '--no-interaction' => true, + ])->assertSuccessful(); + + $this->assertFileExists($this->installState.'/installed'); + } } diff --git a/tests/Feature/WebInstallerTest.php b/tests/Feature/WebInstallerTest.php index 374594f..5a9d89d 100644 --- a/tests/Feature/WebInstallerTest.php +++ b/tests/Feature/WebInstallerTest.php @@ -3,10 +3,14 @@ namespace Tests\Feature; use App\Models\User; +use App\Services\Installer; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\Facades\URL; use Illuminate\Support\Str; +use Mockery; +use RuntimeException; use Tests\TestCase; class WebInstallerTest extends TestCase @@ -43,6 +47,7 @@ protected function tearDown(): void private function validPayload(): array { return [ + 'setup_code' => app(Installer::class)->setupCode(), 'url' => 'https://docket.example.com', 'name' => 'Jane Doe', 'email' => 'jane@example.com', @@ -145,4 +150,87 @@ public function test_install_routes_do_not_require_csrf_or_session(): void // outside the 'web' group because no session store exists yet. $this->post('/install/setup', [])->assertStatus(422); } + + public function test_the_setup_form_says_where_to_find_the_setup_code(): void + { + $this->get('/install/setup') + ->assertOk() + ->assertSee('Setup code') + ->assertSee('cat '.app(Installer::class)->setupCodeLocation()); + + $this->assertFileExists($this->installState.'/setup-code'); + } + + public function test_a_wrong_or_missing_setup_code_is_refused(): void + { + foreach (['', 'aaaa-bbbb-cccc-dddd'] as $code) { + $this->post('/install/setup', ['setup_code' => $code] + $this->validPayload()) + ->assertStatus(422) + ->assertSee('Setup code'); + } + + $this->assertSame(0, User::query()->count()); + $this->assertFileDoesNotExist($this->envDir.'/.env'); + } + + public function test_installing_retires_the_setup_code_and_leaves_a_marker(): void + { + $this->post('/install/setup', $this->validPayload())->assertOk(); + + $this->assertFileDoesNotExist($this->installState.'/setup-code'); + $this->assertFileExists($this->installState.'/installed'); + } + + public function test_the_marker_keeps_the_installer_closed_even_with_no_accounts(): void + { + app(Installer::class)->markInstalled(); + + $this->get('/install')->assertRedirect(route('login')); + $this->post('/install/setup', $this->validPayload())->assertRedirect(route('login')); + $this->assertSame(0, User::query()->count()); + } + + public function test_concurrent_installs_cannot_create_two_owners(): void + { + // Pause the first installation just before it creates its account + // and run a complete second one in that window, as two overlapping + // requests would. The second is refused by the lock. + $real = new Installer($this->app); + $installer = Mockery::mock(Installer::class, [$this->app])->makePartial(); + $second = null; + + $installer->shouldReceive('createOwner')->andReturnUsing(function (string $name, string $email, string $password) use (&$second, $real) { + $second ??= $this->post('/install/setup', array_replace($this->validPayload(), ['email' => 'intruder@example.com'])); + + return $real->createOwner($name, $email, $password); + }); + $this->app->instance(Installer::class, $installer); + + $this->post('/install/setup', $this->validPayload())->assertOk()->assertSee('Docket is installed'); + + $this->assertNotNull($second); + $second->assertStatus(409)->assertSee('Another installation is running'); + $this->assertSame(['jane@example.com'], User::query()->pluck('email')->all()); + } + + public function test_a_second_owner_is_never_created_even_past_the_checks(): void + { + User::factory()->create(); + + $this->expectException(RuntimeException::class); + + app(Installer::class)->createOwner('Someone', 'someone@example.com', 'password-123'); + } + + public function test_an_unreadable_database_keeps_the_installer_closed(): void + { + // A configured database that errors is not a fresh install: fail + // closed, and touch nothing. + Schema::shouldReceive('hasTable')->andThrow(new RuntimeException('database is locked')); + + $this->get('/install')->assertStatus(503)->assertSee('installer stays closed'); + $this->post('/install/setup', $this->validPayload())->assertStatus(503); + + $this->assertFileDoesNotExist($this->envDir.'/.env'); + } } diff --git a/tests/TestCase.php b/tests/TestCase.php index be42ace..4ae4145 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,13 +4,21 @@ use App\Services\Fetching\HostResolver; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; +use Illuminate\Support\Facades\File; use Illuminate\Support\Facades\Http; +use Illuminate\Support\Str; use Tests\Support\FakeHostResolver; abstract class TestCase extends BaseTestCase { protected FakeHostResolver $dns; + /** + * Per-test installer state (marker, setup code, lock), so no test sees + * another's installation and none touches storage/app/install. + */ + protected string $installState; + protected function setUp(): void { parent::setUp(); @@ -22,5 +30,15 @@ protected function setUp(): void $this->app->instance(HostResolver::class, $this->dns); Http::preventStrayRequests(); + + $this->installState = sys_get_temp_dir().'/docket-install-state-'.Str::random(10); + config(['docket.install.state_path' => $this->installState]); + } + + protected function tearDown(): void + { + File::deleteDirectory($this->installState); + + parent::tearDown(); } } From f7e8e514a18dd2a006109a7acac96d5ff16d48a6 Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:47 +0000 Subject: [PATCH 3/8] Bound image work, keep saved copies, stop overlapping fetches - Queue retry_after defaults to 360s, above the fetch job's 300s timeout, and the job takes a per-item lock so a retry can't run alongside a fetch still in progress. - Image downloads share one per-article budget for inline and lead images: attempts (not just successes), total bytes and time. The lead image obeys the same on/off switch and reuses an inline copy. - A failed image write keeps the remote src instead of pointing the article at a missing file. - A failed refresh no longer hides the copy already saved: the reader shows it with a note, and a refresh that finds no article only fills in missing details rather than overwriting them. - Lists, search, bulk actions, the CSV export and refetch select every column but the article bodies, bulk actions run in batches, and exports and refetch page by ID, so a shrinking result set can't make refetch skip items. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Jobs/FetchItemContent.php | 57 +++++++++--- app/Models/Item.php | 47 +++++++++- app/Services/ImageDownloads.php | 91 ++++++++++++++++++++ app/Services/ImageLocaliser.php | 76 +++++++++++----- app/Services/Settings.php | 2 +- config/queue.php | 6 +- resources/views/items/partials/row.blade.php | 7 +- resources/views/items/show.blade.php | 8 ++ 8 files changed, 253 insertions(+), 41 deletions(-) create mode 100644 app/Services/ImageDownloads.php diff --git a/app/Jobs/FetchItemContent.php b/app/Jobs/FetchItemContent.php index 0005c9c..1be177a 100644 --- a/app/Jobs/FetchItemContent.php +++ b/app/Jobs/FetchItemContent.php @@ -13,6 +13,7 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Queue\Jobs\SyncJob; +use Illuminate\Queue\Middleware\WithoutOverlapping; use Illuminate\Support\Str; use Throwable; @@ -32,6 +33,8 @@ class FetchItemContent implements ShouldQueue /** * Image downloads can add up; this is the ceiling for the whole job. + * The queue's retry_after (config/queue.php) must stay above it, or a + * second worker picks the job up while the first is still running. */ public int $timeout = 300; @@ -42,6 +45,22 @@ class FetchItemContent implements ShouldQueue public function __construct(public Item $item) {} + /** + * One fetch per item at a time: a retry or refetch queued while an + * earlier fetch of the same item is still running is dropped, not run + * alongside it (the running one is about to store the article anyway). + * + * @return list + */ + public function middleware(): array + { + return [ + (new WithoutOverlapping('item-'.$this->item->getKey())) + ->dontRelease() + ->expireAfter($this->timeout + 60), + ]; + } + public function tries(): int { return max(1, (int) settings('fetch.tries')); @@ -92,27 +111,45 @@ public function handle( $html = $article->html !== null ? $sanitiser->sanitise($article->html) : ''; - $item->forceFill([ + $metadata = [ 'resolved_url' => $response->url !== $item->url ? $response->url : null, 'author' => $article->author, 'site_name' => $article->siteName, 'lead_image_url' => $article->leadImageUrl, 'language' => $article->language, 'published_at' => $article->publishedAt, - ]); - - if ($this->shouldReplaceTitle($item) && $article->title !== null) { - $item->title = $article->title; - } + 'excerpt' => $article->excerpt, + ]; if (trim(strip_tags($html, '')) === '') { - $item->forceFill(['excerpt' => $article->excerpt])->save(); + // Nothing readable this time. Fill in only what was missing, so + // a refresh of a page that has since changed or broken doesn't + // wipe the details of the copy already saved. + foreach ($metadata as $field => $value) { + if ($value !== null && blank($item->getAttribute($field))) { + $item->setAttribute($field, $value); + } + } + + if ($this->shouldReplaceTitle($item) && $article->title !== null) { + $item->title = $article->title; + } + + $item->save(); $this->recordFailure('Could not find any article text on this page. The original link still works.'); return; } - $html = $images->localise($item, $html); + $item->forceFill($metadata); + + if ($this->shouldReplaceTitle($item) && $article->title !== null) { + $item->title = $article->title; + } + + // One budget for the inline images and the lead image. + $downloads = $images->forItem($item); + $html = $downloads->localise($html); $text = HtmlSanitiser::toText($html); $words = HtmlSanitiser::wordCount($text); $wordsPerMinute = max(1, (int) settings('words_per_minute')); @@ -123,9 +160,7 @@ public function handle( 'excerpt' => $article->excerpt ?? Str::limit(str_replace("\n", ' ', $text), 280), 'word_count' => $words, 'reading_time' => $words > 0 ? (int) max(1, ceil($words / $wordsPerMinute)) : 0, - 'lead_image_path' => $article->leadImageUrl !== null - ? $images->store($item, $article->leadImageUrl) - : null, + 'lead_image_path' => $downloads->lead($article->leadImageUrl), 'fetch_status' => FetchStatus::Fetched, 'fetch_error' => null, 'fetched_at' => now(), diff --git a/app/Models/Item.php b/app/Models/Item.php index e64d53a..06fd0e8 100644 --- a/app/Models/Item.php +++ b/app/Models/Item.php @@ -70,6 +70,20 @@ class Item extends Model 'longest' => 'Longest', ]; + /** + * Every column except the two article bodies, which can run to + * hundreds of kilobytes each: what lists, bulk actions and the CSV + * export select. + */ + public const LIST_COLUMNS = [ + 'id', 'url', 'normalized_url', 'resolved_url', 'title', 'author', + 'site_name', 'excerpt', 'lead_image_url', 'lead_image_path', + 'word_count', 'reading_time', 'language', 'published_at', 'status', + 'archived_at', 'is_favourite', 'favourited_at', 'read_progress', + 'last_read_at', 'source', 'fetch_status', 'fetch_error', + 'fetch_attempts', 'fetched_at', 'added_at', 'created_at', 'updated_at', + ]; + protected $fillable = [ 'url', 'normalized_url', @@ -194,9 +208,21 @@ public function isArchived(): bool return $this->status === ItemStatus::Archived; } + /** + * Whether a readable copy is stored. Independent of the latest fetch: + * a failed refresh keeps the last good copy readable. + */ public function hasArticle(): bool { - return $this->fetch_status === FetchStatus::Fetched && trim((string) $this->content_html) !== ''; + return trim((string) $this->content_html) !== ''; + } + + /** + * A copy is stored but the latest attempt to refresh it failed. + */ + public function refreshFailed(): bool + { + return $this->fetch_status === FetchStatus::Failed && $this->hasArticle(); } public function archive(): void @@ -248,6 +274,16 @@ public function syncTagNames(iterable $names): void $this->tags()->sync(Tag::idsForNames($names)); } + /** + * Select everything but the article bodies. + * + * @param Builder $query + */ + public function scopeWithoutBodies(Builder $query): void + { + $query->select(array_map(fn (string $column): string => 'items.'.$column, self::LIST_COLUMNS)); + } + /** * @param Builder $query */ @@ -300,8 +336,13 @@ public function scopeSearch(Builder $query, string $term, bool $orderByRelevance ->selectRaw('rowid AS fts_rowid, bm25(items_fts, 10.0, 5.0, 1.0) AS fts_rank') ->whereRaw('items_fts MATCH ?', [$match]); - $query->joinSub($matches, 'fts', 'fts.fts_rowid', '=', 'items.id') - ->select('items.*'); + // Keep any column list the caller chose (withoutBodies()); otherwise + // select the item's own columns, not the join's. + if ($query->getQuery()->columns === null) { + $query->select('items.*'); + } + + $query->joinSub($matches, 'fts', 'fts.fts_rowid', '=', 'items.id'); if ($orderByRelevance) { $query->orderBy('fts.fts_rank'); diff --git a/app/Services/ImageDownloads.php b/app/Services/ImageDownloads.php new file mode 100644 index 0000000..628db0e --- /dev/null +++ b/app/Services/ImageDownloads.php @@ -0,0 +1,91 @@ + stored filename, or null when it wasn't stored. + * + * @var array + */ + protected array $results = []; + + public function __construct( + protected ImageLocaliser $localiser, + protected Item $item, + ) { + $this->attemptsLeft = config('docket.images.enabled') ? max(0, (int) settings('images.max_per_item')) : 0; + $this->bytesLeft = max(0, (int) config('docket.images.max_total_kb')) * 1024; + $this->deadline = microtime(true) + max(0, (int) config('docket.images.time_budget')); + } + + public function localise(string $html): string + { + return $this->localiser->rewrite($this->item, $html, fn (string $url): ?string => $this->download($url)); + } + + /** + * The stored filename for the lead image, if it could be stored. + */ + public function lead(?string $url): ?string + { + return $url === null ? null : $this->download($url); + } + + /** + * How many download attempts are left (for tests and logging). + */ + public function attemptsLeft(): int + { + return $this->attemptsLeft; + } + + protected function download(string $url): ?string + { + if (array_key_exists($url, $this->results)) { + return $this->results[$url]; + } + + $secondsLeft = (int) floor($this->deadline - microtime(true)); + + if ($this->attemptsLeft <= 0 || $this->bytesLeft <= 0 || $secondsLeft < 1) { + return $this->results[$url] = null; + } + + $this->attemptsLeft--; + + [$filename, $bytes] = $this->localiser->store( + $this->item, + $url, + min((int) config('docket.images.max_kb') * 1024, $this->bytesLeft), + min($secondsLeft, (int) settings('fetch.timeout')), + ); + + $this->bytesLeft -= $bytes; + + return $this->results[$url] = $filename; + } +} diff --git a/app/Services/ImageLocaliser.php b/app/Services/ImageLocaliser.php index 39cce59..026c237 100644 --- a/app/Services/ImageLocaliser.php +++ b/app/Services/ImageLocaliser.php @@ -3,6 +3,7 @@ namespace App\Services; use App\Exceptions\FetchFailedException; +use App\Exceptions\ResponseTooLargeException; use App\Models\Item; use App\Services\Fetching\SafeHttpClient; use DOMElement; @@ -10,6 +11,7 @@ use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Masterminds\HTML5; +use Throwable; /** * Downloads an article's images to private storage and points their src @@ -18,8 +20,9 @@ * * Images go through SafeHttpClient like any other fetch. Only raster * formats are kept (SVG can carry scripts and would be served from this - * origin); anything that fails, is too big, or is past the per-article cap - * keeps its remote src. + * origin); anything that fails, is too big, or is past the article's + * budget keeps its remote src. The budget (ImageDownloads) is shared by the + * inline images and the lead image of one article. */ class ImageLocaliser { @@ -43,13 +46,32 @@ class ImageLocaliser public function __construct(protected SafeHttpClient $http) {} /** - * Rewrite every in already-sanitised article HTML. + * A download budget for one article: use it for the inline images and + * then the lead image, so both share one set of limits and the lead + * image reuses an inline copy. + */ + public function forItem(Item $item): ImageDownloads + { + return new ImageDownloads($this, $item); + } + + /** + * Rewrite every in already-sanitised article HTML, with a fresh + * budget. Use forItem() when the lead image should share it. */ public function localise(Item $item, string $html): string { - $limit = (int) settings('images.max_per_item'); + return $this->forItem($item)->localise($html); + } - if (! config('docket.images.enabled') || $limit < 1 || ! str_contains($html, ' src that $download stores to the local copy. + * + * @param callable(string): ?string $download URL in, stored filename (or null) out + */ + public function rewrite(Item $item, string $html, callable $download): string + { + if (! str_contains($html, 'getElementsByTagName('img')) as $img) { $src = $img->getAttribute('src'); @@ -73,14 +93,10 @@ public function localise(Item $item, string $html): string continue; } - if (! array_key_exists($src, $stored)) { - $stored[$src] = count(array_filter($stored)) < $limit - ? $this->store($item, $src) - : null; - } + $filename = $download($src); - if ($stored[$src] !== null) { - $img->setAttribute('src', $this->url($item, $stored[$src])); + if ($filename !== null) { + $img->setAttribute('src', $this->url($item, $filename)); } } @@ -90,32 +106,50 @@ public function localise(Item $item, string $html): string /** * Download one image into the item's directory. * - * @return string|null the stored filename, or null if it was not stored + * @return array{0: string|null, 1: int} the stored filename (null if + * not stored) and bytes downloaded */ - public function store(Item $item, string $url): ?string + public function store(Item $item, string $url, int $maxBytes, int $timeout): array { try { - $response = $this->http->get($url, (int) config('docket.images.max_kb') * 1024, 'image/avif,image/webp,image/png,image/jpeg,image/gif;q=0.9,*/*;q=0.1'); + $response = $this->http->get($url, $maxBytes, 'image/avif,image/webp,image/png,image/jpeg,image/gif;q=0.9,*/*;q=0.1', $timeout); } catch (FetchFailedException $e) { Log::debug('Image not stored', ['item' => $item->id, 'url' => $url, 'reason' => $e->getMessage()]); - return null; + // An abandoned oversized download spent (up to) its cap. + return [null, $e instanceof ResponseTooLargeException ? $e->limit : 0]; } + $bytes = strlen($response->body); + // Trust the bytes, not the header: a server can label anything // image/png. $sniffed = (new \finfo(FILEINFO_MIME_TYPE))->buffer($response->body); $extension = self::TYPES[$sniffed] ?? null; if ($extension === null) { - return null; + return [null, $bytes]; } $filename = sha1($url).'.'.$extension; + $path = $item->storageDirectory().'/'.$filename; - Storage::disk(Item::DISK)->put($item->storageDirectory().'/'.$filename, $response->body); + // A failed write (full disk, permissions) must leave the working + // remote src in place, not point the article at a missing file. + try { + $written = Storage::disk(Item::DISK)->put($path, $response->body); + } catch (Throwable $e) { + report($e); + $written = false; + } + + if ($written === false) { + Log::warning('Could not store an article image; keeping its remote address.', ['item' => $item->id, 'path' => $path]); + + return [null, $bytes]; + } - return $filename; + return [$filename, $bytes]; } /** diff --git a/app/Services/Settings.php b/app/Services/Settings.php index 2dbc208..40bc8f9 100644 --- a/app/Services/Settings.php +++ b/app/Services/Settings.php @@ -76,7 +76,7 @@ public static function fields(): array ], 'images.max_per_item' => [ 'label' => 'Images saved per article', - 'help' => 'Inline images downloaded so articles survive link rot. Set to 0 to keep every image remote.', + 'help' => 'Images downloaded (or tried) per article, lead image included, so articles survive link rot. Set to 0 to keep every image remote.', 'type' => 'int', 'rules' => ['required', 'integer', 'min:0', 'max:500'], ], diff --git a/config/queue.php b/config/queue.php index 79c2c0a..20d5345 100644 --- a/config/queue.php +++ b/config/queue.php @@ -40,7 +40,7 @@ 'connection' => env('DB_QUEUE_CONNECTION'), 'table' => env('DB_QUEUE_TABLE', 'jobs'), 'queue' => env('DB_QUEUE', 'default'), - 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 90), + 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 360), 'after_commit' => false, ], @@ -48,7 +48,7 @@ 'driver' => 'beanstalkd', 'host' => env('BEANSTALKD_QUEUE_HOST', 'localhost'), 'queue' => env('BEANSTALKD_QUEUE', 'default'), - 'retry_after' => (int) env('BEANSTALKD_QUEUE_RETRY_AFTER', 90), + 'retry_after' => (int) env('BEANSTALKD_QUEUE_RETRY_AFTER', 360), 'block_for' => 0, 'after_commit' => false, ], @@ -68,7 +68,7 @@ 'driver' => 'redis', 'connection' => env('REDIS_QUEUE_CONNECTION', 'default'), 'queue' => env('REDIS_QUEUE', 'default'), - 'retry_after' => (int) env('REDIS_QUEUE_RETRY_AFTER', 90), + 'retry_after' => (int) env('REDIS_QUEUE_RETRY_AFTER', 360), 'block_for' => null, 'after_commit' => false, ], diff --git a/resources/views/items/partials/row.blade.php b/resources/views/items/partials/row.blade.php index b7fc64e..cdde0c1 100644 --- a/resources/views/items/partials/row.blade.php +++ b/resources/views/items/partials/row.blade.php @@ -1,6 +1,9 @@ @php /** @var \App\Models\Item $item */ $failed = $item->fetch_status === \App\Enums\FetchStatus::Failed; + // fetched_at is only set by a successful fetch, so a failed item that + // has one still has its saved copy (lists don't load article bodies). + $refreshFailed = $failed && $item->fetched_at !== null; $pending = $item->fetch_status === \App\Enums\FetchStatus::Pending; @endphp
  • @@ -33,7 +36,7 @@ class="rounded border-slate-300 text-teal-700 focus:ring-teal-500"> {{ __('Fetching…') }} @elseif ($failed) - {{ __('Could not fetch') }} + {{ $refreshFailed ? __('Refresh failed') : __('Could not fetch') }} @endif @if ($item->isArchived()) @@ -45,7 +48,7 @@ class="rounded border-slate-300 text-teal-700 focus:ring-teal-500">

    {{ $item->excerpt }}

    @endif - @if ($failed && $item->fetch_error) + @if ($failed && ! $refreshFailed && $item->fetch_error)

    {{ $item->fetch_error }}

    @endif diff --git a/resources/views/items/show.blade.php b/resources/views/items/show.blade.php index 1aa1730..abaa7cb 100644 --- a/resources/views/items/show.blade.php +++ b/resources/views/items/show.blade.php @@ -149,6 +149,14 @@ class="reader-panel absolute right-0 mt-2 w-64 space-y-4 rounded-lg border p-4 s @if ($item->hasArticle()) + @if ($item->refreshFailed()) +

    + {{ __('Could not refresh this article: :error You are reading the copy saved on :date.', [ + 'error' => rtrim((string) $item->fetch_error, '.').'.', + 'date' => $item->fetched_at?->format('j F Y') ?? __('an earlier visit'), + ]) }} +

    + @endif
    {!! $item->content_html !!}
    From 2d8c8431746ebd6a890ef0544b56224a84378687 Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:47 +0000 Subject: [PATCH 4/8] Batch list and export queries; add a spreadsheet-safe CSV Part of the same review: the list, bulk and refetch code that uses the new withoutBodies() scope and ID paging, and an export option that prefixes any cell a spreadsheet would run as a formula with an apostrophe. The plain CSV stays lossless for re-importing. HardeningTest covers this and the previous commit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Console/Commands/DocketRefetchCommand.php | 5 +- app/Http/Controllers/BulkItemController.php | 63 ++-- app/Http/Controllers/ExportController.php | 14 +- app/Http/Controllers/ItemController.php | 2 +- app/Services/Exporter.php | 42 ++- resources/views/import/index.blade.php | 4 + tests/Feature/HardeningTest.php | 274 ++++++++++++++++++ 7 files changed, 357 insertions(+), 47 deletions(-) create mode 100644 tests/Feature/HardeningTest.php diff --git a/app/Console/Commands/DocketRefetchCommand.php b/app/Console/Commands/DocketRefetchCommand.php index 8573dcf..ab6a44e 100644 --- a/app/Console/Commands/DocketRefetchCommand.php +++ b/app/Console/Commands/DocketRefetchCommand.php @@ -42,7 +42,10 @@ public function handle(): int $spacing = max(0, (int) settings('import.spacing_seconds')); $count = 0; - foreach ($query->orderBy('id')->lazy() as $item) { + // By ID rather than by offset: items leave the failed/pending + // selection as they are fetched (at once, on a sync queue), and + // offset paging over a shrinking result set skips rows. + foreach ($query->withoutBodies()->lazyById(200, 'items.id', 'id') as $item) { $this->queue($item, $count * $spacing); $count++; } diff --git a/app/Http/Controllers/BulkItemController.php b/app/Http/Controllers/BulkItemController.php index e2fe718..0d46712 100644 --- a/app/Http/Controllers/BulkItemController.php +++ b/app/Http/Controllers/BulkItemController.php @@ -37,37 +37,44 @@ public function __invoke(Request $request): RedirectResponse 'tags.required_if' => 'Enter the tags to add.', ]); - $items = Item::query()->whereIn('id', $data['ids'])->get(); $tags = Tag::parseList($data['tags'] ?? null); + $count = 0; - foreach ($items as $item) { - switch ($data['action']) { - case 'archive': - $item->archive(); - break; - case 'unarchive': - $item->unarchive(); - break; - case 'favourite': - $item->setFavourite(true); - break; - case 'unfavourite': - $item->setFavourite(false); - break; - case 'tag': - $item->attachTagNames($tags); - break; - case 'retry': - $this->retry($item); - break; - case 'delete': - // One by one, so each item's stored images go too. - $item->delete(); - break; - } - } + // In batches, without the article bodies: a thousand ticked items + // must not mean a thousand full articles in memory at once. + Item::query() + ->withoutBodies() + ->whereIn('items.id', $data['ids']) + ->chunkById(100, function ($items) use ($data, $tags, &$count): void { + foreach ($items as $item) { + $count++; - $count = $items->count(); + switch ($data['action']) { + case 'archive': + $item->archive(); + break; + case 'unarchive': + $item->unarchive(); + break; + case 'favourite': + $item->setFavourite(true); + break; + case 'unfavourite': + $item->setFavourite(false); + break; + case 'tag': + $item->attachTagNames($tags); + break; + case 'retry': + $this->retry($item); + break; + case 'delete': + // One by one, so each item's stored images go too. + $item->delete(); + break; + } + } + }, 'items.id', 'id'); return back()->with('status', sprintf( '%s: %d %s.', diff --git a/app/Http/Controllers/ExportController.php b/app/Http/Controllers/ExportController.php index e961014..8dfc703 100644 --- a/app/Http/Controllers/ExportController.php +++ b/app/Http/Controllers/ExportController.php @@ -3,25 +3,29 @@ namespace App\Http\Controllers; use App\Services\Exporter; +use Illuminate\Http\Request; use Symfony\Component\HttpFoundation\StreamedResponse; class ExportController extends Controller { /** - * Pocket-shaped CSV: title, url, time_added, tags, status. + * Pocket-shaped CSV: title, url, time_added, tags, status. ?for=spreadsheet + * neutralises cells a spreadsheet would run as formulas. */ - public function csv(Exporter $exporter): StreamedResponse + public function csv(Request $request, Exporter $exporter): StreamedResponse { + $forSpreadsheets = $request->query('for') === 'spreadsheet'; + return response()->streamDownload( - function () use ($exporter): void { + function () use ($exporter, $forSpreadsheets): void { $handle = fopen('php://output', 'w'); if ($handle !== false) { - $exporter->writeCsv($handle); + $exporter->writeCsv($handle, $forSpreadsheets); fclose($handle); } }, - 'docket-export-'.now()->format('Y-m-d').'.csv', + 'docket-export-'.now()->format('Y-m-d').($forSpreadsheets ? '-spreadsheet' : '').'.csv', ['Content-Type' => 'text/csv; charset=UTF-8'] ); } diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index 5af9f95..8277b00 100644 --- a/app/Http/Controllers/ItemController.php +++ b/app/Http/Controllers/ItemController.php @@ -51,7 +51,7 @@ protected function list(Request $request, string $view, ?Tag $tag = null): View $sort = $request->query('sort'); $sort = is_string($sort) && array_key_exists($sort, Item::SORTS) ? $sort : null; - $query = Item::query()->with('tags'); + $query = Item::query()->withoutBodies()->with('tags'); if ($view === 'archive') { $query->archived(); diff --git a/app/Services/Exporter.php b/app/Services/Exporter.php index c4e383a..0ff733e 100644 --- a/app/Services/Exporter.php +++ b/app/Services/Exporter.php @@ -21,26 +21,43 @@ class Exporter public const CSV_COLUMNS = ['title', 'url', 'time_added', 'tags', 'status']; /** + * Pocket's CSV. Lossless by default, so it imports back exactly. With + * $forSpreadsheets, any cell a spreadsheet would run as a formula gets + * a leading apostrophe: titles and tags come from other people's web + * pages, and "=HYPERLINK(...)" in a title is someone else's formula. + * * @param resource $handle */ - public function writeCsv($handle): void + public function writeCsv($handle, bool $forSpreadsheets = false): void { fputcsv($handle, self::CSV_COLUMNS, ',', '"', ''); - foreach ($this->items() as $item) { - fputcsv($handle, [ + foreach ($this->items(withBodies: false) as $item) { + $row = [ $item->title ?? '', $item->url, (string) ($item->added_at?->getTimestamp() ?? ''), $item->tags->pluck('name')->implode('|'), $item->status->pocketName(), - ], ',', '"', ''); + ]; + + fputcsv($handle, $forSpreadsheets ? array_map(self::spreadsheetSafe(...), $row) : $row, ',', '"', ''); } } /** - * Streamed one item at a time so a large library never has to fit in - * memory as a single array. + * Neutralise a cell a spreadsheet would treat as a formula: one starting + * with = + - or @, including after leading whitespace or control + * characters, or starting with a tab or carriage return. + */ + public static function spreadsheetSafe(string $value): string + { + return preg_match('/^[\s\x00-\x1f]*[=+\-@]|^[\t\r]/u', $value) === 1 ? "'".$value : $value; + } + + /** + * Written one item at a time, from small batches, so a large library + * never has to fit in memory. * * @param resource $handle */ @@ -52,7 +69,7 @@ public function writeJson($handle): void $first = true; - foreach ($this->items() as $item) { + foreach ($this->items(withBodies: true) as $item) { fwrite($handle, ($first ? "\n" : ",\n").json_encode($this->itemToArray($item), $flags)); $first = false; } @@ -99,16 +116,17 @@ public function itemToArray(Item $item): array } /** - * Oldest first, so an export re-imported in order keeps its sequence. + * Every item in ID order, fetched in small keyset batches (stable even + * if items change mid-export). The CSV needs no article bodies; the + * JSON does, so its batches are smaller. * * @return LazyCollection */ - protected function items(): LazyCollection + protected function items(bool $withBodies): LazyCollection { return Item::query() + ->when(! $withBodies, fn ($query) => $query->withoutBodies()) ->with('tags') - ->orderBy('added_at') - ->orderBy('id') - ->lazy(200); + ->lazyById($withBodies ? 25 : 500, 'items.id', 'id'); } } diff --git a/resources/views/import/index.blade.php b/resources/views/import/index.blade.php index c7fef07..5540994 100644 --- a/resources/views/import/index.blade.php +++ b/resources/views/import/index.blade.php @@ -37,6 +37,10 @@ class="mt-1 block w-full text-sm text-slate-700 file:mr-4 file:rounded-md file:b {{ __('Download CSV') }} — {{ __('the same columns as Pocket\'s export, so it imports back into Docket or anything that reads Pocket files.') }}
  • +
  • + {{ __('Download CSV for spreadsheets') }} + — {{ __('the same columns, safe to open in Excel, Numbers or Sheets: any title or tag that starts like a formula is kept as plain text (with a leading apostrophe), so a hostile page title can\'t run as a formula. Use the plain CSV for importing.') }} +
  • {{ __('Download JSON') }} — {{ __('everything, including the saved article text and HTML.') }} diff --git a/tests/Feature/HardeningTest.php b/tests/Feature/HardeningTest.php new file mode 100644 index 0000000..61b1129 --- /dev/null +++ b/tests/Feature/HardeningTest.php @@ -0,0 +1,274 @@ +create($attributes + [ + 'url' => $url, + 'normalized_url' => app(UrlNormaliser::class)->normalise($url), + 'title' => null, + ]); + } + + private function articleHtml(string $body = 'A readable paragraph about reading and libraries.'): string + { + return 'Article' + .'
    ' + .str_repeat('

    '.$body.' '.str_repeat('More words to make a proper paragraph. ', 8).'

    ', 6) + .'
    '; + } + + // Queue timing -------------------------------------------------------- + + public function test_queue_reservations_outlast_the_fetch_job(): void + { + $timeout = (new FetchItemContent($this->item()))->timeout; + + foreach (['database', 'redis', 'beanstalkd'] as $connection) { + $this->assertGreaterThan($timeout, config("queue.connections.{$connection}.retry_after"), $connection); + } + } + + public function test_the_same_item_is_never_fetched_twice_at_once(): void + { + Http::fake(); + $item = $this->item(); + $job = new FetchItemContent($item); + + // Another worker is mid-fetch on this item. + $lock = Cache::lock($job->middleware()[0]->getLockKey($job), 600); + $this->assertTrue($lock->get()); + + FetchItemContent::dispatchSync($item); + + Http::assertNothingSent(); + $this->assertSame(0, $item->refresh()->fetch_attempts); + + $lock->release(); + } + + // Memory --------------------------------------------------------------- + + public function test_lists_bulk_actions_and_csv_export_never_select_article_bodies(): void + { + Queue::fake(); + $user = User::factory()->create(); + $items = Item::factory()->count(3)->fetched()->create(); + + DB::enableQueryLog(); + + $this->actingAs($user)->get('/')->assertOk(); + $this->actingAs($user)->get('/search?q=the')->assertOk(); + $this->actingAs($user)->post('/items/bulk', ['action' => 'archive', 'ids' => $items->pluck('id')->all()]); + $this->actingAs($user)->get('/export/csv')->streamedContent(); + + $bodies = collect(DB::getQueryLog()) + ->pluck('query') + ->filter(fn (string $sql): bool => str_starts_with(strtolower($sql), 'select') + && str_contains($sql, '"items"') + && (str_contains($sql, '"items".*') || str_contains($sql, 'select *') || str_contains($sql, 'content_html'))); + + $this->assertSame([], $bodies->values()->all()); + $this->assertSame(3, Item::query()->archived()->count()); + } + + // Images --------------------------------------------------------------- + + public function test_failed_image_downloads_use_up_the_per_article_limit(): void + { + config(['docket.images.max_per_item' => 2]); + Http::fake(['*' => Http::response('', 404)]); + + $html = ''; + for ($i = 0; $i < 7; $i++) { + $html .= ''; + } + + app(ImageLocaliser::class)->localise($this->item(), $html); + + Http::assertSentCount(2); + } + + public function test_image_downloads_stop_at_the_per_article_byte_budget(): void + { + Storage::fake(Item::DISK); + config(['docket.images.max_total_kb' => 1]); + Http::fake(['*' => Http::response(str_repeat('x', 900))]); + + app(ImageLocaliser::class)->localise($this->item(), ''); + + // 900 bytes, then 124 left (capped), then nothing left to spend. + Http::assertSentCount(2); + } + + public function test_disabled_downloads_skip_the_lead_image_too(): void + { + Storage::fake(Item::DISK); + config(['docket.images.enabled' => false]); + Http::fake([ + 'https://example.com/article' => Http::response($this->articleHtml(), 200, ['Content-Type' => 'text/html']), + '*' => Http::response((string) base64_decode(self::PNG)), + ]); + $item = $this->item(); + + FetchItemContent::dispatchSync($item); + + $item->refresh(); + $this->assertSame(FetchStatus::Fetched, $item->fetch_status); + $this->assertNull($item->lead_image_path); + $this->assertSame('https://cdn.example.com/lead.png', $item->lead_image_url); + Http::assertSentCount(1); + } + + public function test_a_lead_image_shown_inline_is_downloaded_once(): void + { + Storage::fake(Item::DISK); + $html = str_replace('
    ', '

    ', $this->articleHtml()); + Http::fake([ + 'https://example.com/article' => Http::response($html, 200, ['Content-Type' => 'text/html']), + 'https://cdn.example.com/lead.png' => Http::response((string) base64_decode(self::PNG)), + ]); + $item = $this->item(); + + FetchItemContent::dispatchSync($item); + + $this->assertSame(sha1('https://cdn.example.com/lead.png').'.png', $item->refresh()->lead_image_path); + Http::assertSentCount(2); + } + + public function test_a_failed_image_write_keeps_the_remote_address(): void + { + Http::fake(['*' => Http::response((string) base64_decode(self::PNG))]); + $disk = Mockery::mock(Filesystem::class); + $disk->shouldReceive('put')->once()->andReturn(false); + Storage::shouldReceive('disk')->with(Item::DISK)->andReturn($disk); + + $item = $this->item(); + $html = app(ImageLocaliser::class)->localise($item, ''); + + $this->assertStringContainsString('src="https://example.com/image.png"', $html); + $this->assertStringNotContainsString('/items/'.$item->id.'/images/', $html); + } + + // Refresh failures ----------------------------------------------------- + + public function test_a_failed_refresh_keeps_the_saved_article_readable(): void + { + Http::fake(['*' => Http::response('', 404)]); + $item = Item::factory()->fetched('The preserved article body.')->create([ + 'author' => 'Original Author', + 'site_name' => 'Original Site', + ]); + + FetchItemContent::dispatchSync($item); + $item->refresh(); + + $this->assertSame(FetchStatus::Failed, $item->fetch_status); + $this->assertTrue($item->hasArticle()); + $this->assertTrue($item->refreshFailed()); + $this->assertSame('Original Author', $item->author); + + $this->actingAs(User::factory()->create()) + ->get("/items/{$item->id}") + ->assertOk() + ->assertSee('The preserved article body.') + ->assertSee('Could not refresh this article: The site responded with HTTP 404.'); + + $this->actingAs(User::factory()->create())->get('/')->assertSee('Refresh failed'); + } + + public function test_a_refresh_that_finds_no_article_keeps_the_saved_details(): void + { + Http::fake(['*' => Http::response('Gone', 200, ['Content-Type' => 'text/html'])]); + $item = Item::factory()->fetched('Still here.')->create([ + 'title' => 'Original title', + 'excerpt' => 'Original excerpt', + 'site_name' => 'Original Site', + ]); + + FetchItemContent::dispatchSync($item); + $item->refresh(); + + $this->assertSame('Original title', $item->title); + $this->assertSame('Original excerpt', $item->excerpt); + $this->assertSame('Original Site', $item->site_name); + $this->assertStringContainsString('Still here.', (string) $item->content_html); + } + + // Refetch paging ------------------------------------------------------- + + public function test_refetch_reaches_every_item_as_the_selection_shrinks(): void + { + // On a sync queue each item is fetched (and leaves the failed set) + // during the loop; offset paging would skip the rows behind it. + config(['queue.default' => 'sync', 'docket.import.spacing_seconds' => 0, 'docket.images.enabled' => false]); + Http::fake(fn () => Http::response($this->articleHtml(), 200, ['Content-Type' => 'text/html'])); + + Item::factory()->count(450)->failed()->create(['fetch_attempts' => 0, 'title' => 'Kept']); + + $this->artisan('docket:refetch')->assertSuccessful(); + + $this->assertSame(0, Item::query()->where('fetch_attempts', 0)->count()); + $this->assertSame(450, Item::query()->where('fetch_status', FetchStatus::Fetched->value)->count()); + } + + // Export --------------------------------------------------------------- + + public function test_the_spreadsheet_csv_neutralises_formulas_and_the_plain_one_is_lossless(): void + { + Item::factory()->create(['title' => '=HYPERLINK("https://evil.example","x")', 'url' => 'https://example.com/a']); + Item::factory()->create(['title' => " \t@SUM(A1)", 'url' => 'https://example.com/b']); + Item::factory()->create(['title' => 'An ordinary title', 'url' => 'https://example.com/c']); + + $user = User::factory()->create(); + $parse = fn (string $csv): array => array_map( + fn (string $line): array => str_getcsv($line, ',', '"', ''), + array_values(array_filter(explode("\n", $csv))) + ); + + $plain = $parse($this->actingAs($user)->get('/export/csv')->streamedContent()); + $safe = $this->actingAs($user)->get('/export/csv?for=spreadsheet'); + $this->assertStringContainsString('-spreadsheet.csv', (string) $safe->headers->get('Content-Disposition')); + $safe = $parse($safe->streamedContent()); + + $this->assertSame('=HYPERLINK("https://evil.example","x")', $plain[1][0]); + $this->assertSame('\'=HYPERLINK("https://evil.example","x")', $safe[1][0]); + $this->assertSame("' \t@SUM(A1)", $safe[2][0]); + $this->assertSame('An ordinary title', $safe[3][0]); + $this->assertSame('https://example.com/a', $safe[1][1]); + + foreach (['-1', '+1', '@x', "\r=1", "\x01=1"] as $dangerous) { + $this->assertStringStartsWith("'", Exporter::spreadsheetSafe($dangerous), json_encode($dangerous) ?: ''); + } + } +} From 767305c21a9d8b8a82faaf18e0d6d22c81b401ba Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:47 +0000 Subject: [PATCH 5/8] Route tags by ID A tag name such as "work/personal" produced a URL that did not match its route. Tags are now addressed by ID; the slug remains the case-insensitive identity used to match names. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Models/Tag.php | 9 +++------ routes/web.php | 6 +++--- tests/Feature/ContentSecurityPolicyTest.php | 2 +- tests/Feature/Items/OrganisingTest.php | 22 +++++++++++++++++---- tests/Feature/PageSmokeTest.php | 2 +- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/app/Models/Tag.php b/app/Models/Tag.php index f879b19..fc18a15 100644 --- a/app/Models/Tag.php +++ b/app/Models/Tag.php @@ -24,11 +24,6 @@ class Tag extends Model protected $fillable = ['name', 'slug']; - public function getRouteKeyName(): string - { - return 'slug'; - } - /** * @return BelongsToMany */ @@ -50,7 +45,9 @@ public static function cleanName(string $name): string } /** - * The case-insensitive identity of a tag name. Not Str::slug(): that + * The case-insensitive identity of a tag name, used to match names, not + * in URLs: a name may contain "/" or other characters a route can't + * carry, so tags are routed by ID. Not Str::slug(): that * transliterates to ASCII and would turn a non-Latin tag into nothing. */ public static function slugFor(string $name): string diff --git a/routes/web.php b/routes/web.php index 20a7b5c..eafce07 100644 --- a/routes/web.php +++ b/routes/web.php @@ -23,7 +23,7 @@ function () { Route::get('archive', [ItemController::class, 'index'])->defaults('view', 'archive')->name('items.archive'); Route::get('favourites', [ItemController::class, 'index'])->defaults('view', 'favourites')->name('items.favourites'); Route::get('search', [ItemController::class, 'index'])->defaults('view', 'search')->name('items.search'); - Route::get('tags/{tag}', [ItemController::class, 'tagged'])->name('items.tagged'); + Route::get('tags/{tag}', [ItemController::class, 'tagged'])->whereNumber('tag')->name('items.tagged'); Route::post('items', [ItemController::class, 'store'])->name('items.store'); Route::post('items/bulk', BulkItemController::class)->name('items.bulk'); @@ -48,8 +48,8 @@ function () { Route::get('share', [SaveController::class, 'share'])->name('save.share'); Route::get('tags', [TagController::class, 'index'])->name('tags.index'); - Route::patch('tags/{tag}', [TagController::class, 'update'])->name('tags.update'); - Route::delete('tags/{tag}', [TagController::class, 'destroy'])->name('tags.destroy'); + Route::patch('tags/{tag}', [TagController::class, 'update'])->whereNumber('tag')->name('tags.update'); + Route::delete('tags/{tag}', [TagController::class, 'destroy'])->whereNumber('tag')->name('tags.destroy'); Route::get('import', [ImportController::class, 'index'])->name('import.index'); Route::post('import', [ImportController::class, 'store'])->name('import.store'); diff --git a/tests/Feature/ContentSecurityPolicyTest.php b/tests/Feature/ContentSecurityPolicyTest.php index faddbb6..d63d7cc 100644 --- a/tests/Feature/ContentSecurityPolicyTest.php +++ b/tests/Feature/ContentSecurityPolicyTest.php @@ -94,7 +94,7 @@ public function test_no_page_relies_on_inline_handlers_or_style_attributes(): vo $user->createToken('Shortcut'); $pages = [ - '/', '/archive', '/favourites', '/search?q=the', '/tags', '/tags/reading', + '/', '/archive', '/favourites', '/search?q=the', '/tags', "/tags/{$tag->id}", "/items/{$item->id}", '/import', '/settings', '/tools', '/profile', '/save?url='.urlencode('https://example.com/new'), ]; diff --git a/tests/Feature/Items/OrganisingTest.php b/tests/Feature/Items/OrganisingTest.php index 009a551..89447a5 100644 --- a/tests/Feature/Items/OrganisingTest.php +++ b/tests/Feature/Items/OrganisingTest.php @@ -109,7 +109,8 @@ public function test_tags_are_added_in_the_reader_and_listed(): void $this->assertSame('Renamed', $item->title); $this->assertSame(['Laravel', 'php'], $item->tags->pluck('name')->all()); - $this->actingAs($this->user)->get('/tags/laravel')->assertOk()->assertSee('Renamed'); + $laravel = Tag::query()->where('slug', 'laravel')->sole(); + $this->actingAs($this->user)->get("/tags/{$laravel->id}")->assertOk()->assertSee('Renamed'); $this->actingAs($this->user)->get('/tags')->assertOk()->assertSee('Laravel')->assertSee('1 item'); // Clearing the field removes every tag (the tags themselves stay). @@ -126,17 +127,17 @@ public function test_tags_can_be_renamed_merged_and_deleted(): void $b->attachTagNames(['JavaScript']); $js = Tag::query()->where('slug', 'js')->sole(); - $this->actingAs($this->user)->patch("/tags/{$js->slug}", ['name' => 'javascript'])->assertRedirect('/tags'); + $this->actingAs($this->user)->patch("/tags/{$js->id}", ['name' => 'javascript'])->assertRedirect('/tags'); $this->assertModelMissing($js); $merged = Tag::query()->where('slug', 'javascript')->sole(); $this->assertSame(2, $merged->items()->count()); - $this->actingAs($this->user)->patch("/tags/{$merged->slug}", ['name' => 'Web dev']); + $this->actingAs($this->user)->patch("/tags/{$merged->id}", ['name' => 'Web dev']); $this->assertSame('Web dev', $merged->refresh()->name); $this->assertSame('web-dev', $merged->slug); - $this->actingAs($this->user)->delete('/tags/web-dev')->assertRedirect('/tags'); + $this->actingAs($this->user)->delete("/tags/{$merged->id}")->assertRedirect('/tags'); $this->assertSame(0, Tag::query()->count()); $this->assertSame(2, Item::query()->count()); } @@ -213,4 +214,17 @@ public function test_deleting_a_single_item(): void $other = Item::factory()->create(); $this->actingAs($this->user)->from("/items/{$other->id}")->delete("/items/{$other->id}")->assertRedirect('/'); } + + public function test_tags_with_url_characters_in_their_names_still_open(): void + { + $item = Item::factory()->create(['title' => 'Slashed']); + + foreach (['work/personal', 'C# & .NET', '50% off?', 'naïve café'] as $name) { + $item->syncTagNames([$name]); + $tag = Tag::query()->where('slug', Tag::slugFor($name))->sole(); + + $this->actingAs($this->user)->get(route('items.tagged', $tag))->assertOk()->assertSee('Slashed'); + $this->actingAs($this->user)->patch(route('tags.update', $tag), ['name' => $name.' 2'])->assertRedirect('/tags'); + } + } } diff --git a/tests/Feature/PageSmokeTest.php b/tests/Feature/PageSmokeTest.php index 745a767..da56482 100644 --- a/tests/Feature/PageSmokeTest.php +++ b/tests/Feature/PageSmokeTest.php @@ -35,7 +35,7 @@ public function test_all_pages_render(): void '/search?q=the&sort=newest', '/search', '/tags', - '/tags/reading', + "/tags/{$tag->id}", "/items/{$fetched->id}", "/items/{$pending->id}", "/items/{$failed->id}", From f6c08cd67265d6a652d3f4886c21f46a625747e1 Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:52:47 +0000 Subject: [PATCH 6/8] Fix the restore instructions and document the setup code The backup's database is an SQL dump to import with sqlite3, not a database file to move into place. The steps now import, check and swap it in with the site and worker stopped, keeping the old files. Also documents the installer's setup code and the queue timeout relationship. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- README.md | 52 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index f17ced6..43277ab 100644 --- a/README.md +++ b/README.md @@ -142,9 +142,23 @@ npm ci && npm run build Point your web server at `public/`, then open **`https://your-domain/install`**. The wizard checks the server -requirements, then asks for the site URL, your name, email and password, -and installs. It only exists while Docket has no user account: once your -login is created, `/install` permanently redirects to the login screen. +requirements, then asks for a setup code, the site URL, your name, email +and password, and installs. + +The setup code proves you control the server: anyone can find a fresh +deployment's `/install`, so it won't create the account without it. Read +it on the server once the setup page has loaded: + +```sh +cat storage/app/install/setup-code +``` + +The installer only exists while Docket has no account. Once your login +is created it leaves a marker (`storage/app/install/installed`) and +`/install` permanently redirects to the login screen. If the database +can't be read, it answers 503 rather than treating the site as fresh. +Docket has exactly one account: neither installer, nor `db:seed`, will +create a second one. ### Console installer @@ -191,6 +205,10 @@ Development server (app + queue + logs + Vite): `composer dev`. User=www-data ``` + Keep `--timeout` below the queue's `retry_after` (360 seconds by + default, `DB_QUEUE_RETRY_AFTER`), or a slow fetch is handed to a second + worker while the first is still running it. + 5. One cron entry drives the scheduler (backups, cleanup, monitor): ``` @@ -218,18 +236,38 @@ BACKUP_ENDPOINT=https://.r2.cloudflarestorage.com BACKUP_REGION=auto ``` -Test a restore early: +The database in the archive is an SQL dump (`db-dumps/*.sql`, written by +`sqlite3 .dump`), not a database file: it has to be imported into a new +database. Test a restore early, on a copy of the server if you can: ```sh php artisan backup:run # download the newest zip from the bucket, then: unzip Docket/….zip -d /tmp/restore -mv /tmp/restore/db-dumps/database.sqlite database/database.sqlite +ls /tmp/restore/db-dumps/ # one .sql file + +# Build a new database from the dump and check it. +sqlite3 /tmp/restore/restored.sqlite < /tmp/restore/db-dumps/*.sql +sqlite3 /tmp/restore/restored.sqlite "PRAGMA integrity_check;" # expect: ok +sqlite3 /tmp/restore/restored.sqlite "SELECT count(*) FROM items;" +sqlite3 /tmp/restore/restored.sqlite "INSERT INTO items_fts(items_fts) VALUES('rebuild');" + +# Swap it in with nothing writing: stop the worker and the site first. +php artisan down +sudo systemctl stop docket-worker # whatever runs queue:work +mkdir -p /tmp/before-restore && cp database/database.sqlite* /tmp/before-restore/ +mv /tmp/restore/restored.sqlite database/database.sqlite +rm -f database/database.sqlite-wal database/database.sqlite-shm rsync -a /tmp/restore/…/storage/app/private/ storage/app/private/ -php artisan migrate:status +chown -R www-data: database storage/app/private +php artisan migrate --force # if the backup predates a release +php artisan up +sudo systemctl start docket-worker ``` -Open a saved article and check its images load. +Then open a saved article, check its images load, and search for a word +you know is in it. The originals stay in `/tmp/before-restore` until +you're satisfied. ## Security notes From cbc4b7bd4b54280f87445776ba998b369aa6cf4d Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:54:11 +0000 Subject: [PATCH 7/8] Apply Pint formatting Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- app/Services/ImageLocaliser.php | 2 +- app/Services/Installer.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Services/ImageLocaliser.php b/app/Services/ImageLocaliser.php index 026c237..5ffb491 100644 --- a/app/Services/ImageLocaliser.php +++ b/app/Services/ImageLocaliser.php @@ -107,7 +107,7 @@ public function rewrite(Item $item, string $html, callable $download): string * Download one image into the item's directory. * * @return array{0: string|null, 1: int} the stored filename (null if - * not stored) and bytes downloaded + * not stored) and bytes downloaded */ public function store(Item $item, string $url, int $maxBytes, int $timeout): array { diff --git a/app/Services/Installer.php b/app/Services/Installer.php index 36d0aa6..828491d 100644 --- a/app/Services/Installer.php +++ b/app/Services/Installer.php @@ -251,7 +251,7 @@ public function ensureEnvFile(): bool $example = $this->app->environmentPath().DIRECTORY_SEPARATOR.'.env.example'; if (! copy($example, $envPath)) { - throw new \RuntimeException("Could not create {$envPath} from .env.example."); + throw new RuntimeException("Could not create {$envPath} from .env.example."); } return true; From 75a46fa61fe876c4793f8e29a6af1a0a2ad60fa2 Mon Sep 17 00:00:00 2001 From: Ajay D'Souza Date: Tue, 20 Oct 2026 03:55:08 +0000 Subject: [PATCH 8/8] Give RealTransportTest a database SafeHttpClient reads the fetch timeout through the Settings service, which queries the settings table. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe --- tests/Feature/Items/RealTransportTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Feature/Items/RealTransportTest.php b/tests/Feature/Items/RealTransportTest.php index dba8273..a981c6a 100644 --- a/tests/Feature/Items/RealTransportTest.php +++ b/tests/Feature/Items/RealTransportTest.php @@ -5,6 +5,7 @@ use App\Exceptions\FetchFailedException; use App\Services\Fetching\IpGuard; use App\Services\Fetching\SafeHttpClient; +use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Http; use Symfony\Component\Process\Process; use Tests\TestCase; @@ -21,6 +22,8 @@ */ class RealTransportTest extends TestCase { + use RefreshDatabase; + private const HOST = 'docket-fixture.test'; private Process $server;