Skip to content

Fix the issues from the 20 October code review - #3

Merged
ajaydsouza merged 8 commits into
masterfrom
wz/elegant-dijkstra-lvdz1o
Oct 20, 2026
Merged

ajaydsouza merged 8 commits into
masterfrom
wz/elegant-dijkstra-lvdz1o

Conversation

@ajaydsouza

Copy link
Copy Markdown
Contributor

Fixes all 14 findings in the external review (reviews/2026-10-20-code-review.md, on the reviews/code-review-2026-10-20 branch). I checked each against the code and none turned out to be a false positive.

Fixes

# Finding Fix
1 (P1) Real hostname fetches failed: with stream => true, Guzzle 8 picked its PHP-stream handler, which rejects the CURLOPT_RESOLVE pin. Guzzle 7 would have ignored the pin silently. SafeHttpClient always uses Guzzle's cURL handler. The size cap is enforced by a progress callback that aborts the transfer. New RealTransportTest goes through the real transport: a made-up hostname is pointed at a local PHP server, which can only be reached through the pin. It covers a plain fetch, a redirect, oversized bodies (declared and undeclared) and a blocked address.
2 (P1) Installer race, and anyone could claim a fresh deployment One-time setup code read on the server (storage/app/install/setup-code). A non-blocking install lock with an ownership re-check inside it. createOwner() refuses once any account exists. The console installer shares the lock and the one-owner rule.
3 A database error reopened the installer An installed marker keeps /install shut. Unexpected database errors return a 503. .env is written only after the account exists.
4 db:seed could add an account with a published password The seeder refuses when any user exists and requires DOCKET_USER_PASSWORD; there's no fallback password any more.
5 Queue retry_after (90s) was shorter than the job timeout (300s) retry_after is now 360s, and a WithoutOverlapping per-item lock on the job stops two fetches of the same item running at once.
6 Article bodies loaded into lists, bulk actions and exports New withoutBodies() scope (search keeps it too). Bulk actions run in batches of 100. Exports and refetch page through items by ID.
7 + 14 Failed images didn't count towards the limit, and the lead image ignored the settings ImageDownloads gives each article one budget shared by inline and lead images. It counts attempts, total bytes and time. The lead image follows the same on/off switch and reuses an inline copy if there is one.
8 A failed storage write left a broken local image link The write result is checked; on failure the remote src is kept and a warning logged.
9 A failed refresh hid the saved article hasArticle() depends only on stored content. The reader shows the saved copy with a "Could not refresh" note. A refresh that finds no article only fills in details that were missing.
10 Refetch skipped items It now pages with lazyById().
11 Tags such as work/personal led to 404s Tags are routed by ID.
12 CSV export kept spreadsheet formulas New CSV for spreadsheets option (/export/csv?for=spreadsheet). The plain CSV stays lossless for re-importing.
13 Wrong restore instructions The README now imports the SQL dump with sqlite3, checks it, and swaps it in with the site and worker stopped. I ran the steps with the real sqlite3 CLI: the FTS5 table and its triggers survive.

Tests

  • New tests:
    • RealTransportTest
    • HardeningTest: one or more tests each for #5–#10, #12 and #14
    • DatabaseSeederTest
    • in WebInstallerTest: the setup code, the marker, a concurrent install refused with 409, a database error answered with 503
    • in InstallCommandTest: --force won't add a second owner
    • in OrganisingTest: tag names containing URL characters
  • Updated: the installer tests send the setup code, and the tag tests use IDs.
  • Each test gets its own installer-state directory, so tests can't leak install state to each other.

Generated by Claude Code

ajaydsouza and others added 8 commits October 20, 2026 03:52
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
SafeHttpClient reads the fetch timeout through the Settings service,
which queries the settings table.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014DPvSBmPyHN2QVmXsQQVKe
@ajaydsouza
ajaydsouza merged commit 30e4cf9 into master Oct 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant