From 92eba07da7b91f5eafe3a76c3ccaa9ecfc29cf48 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Mon, 10 Aug 2026 04:07:53 +0700 Subject: [PATCH] A newly bookmarked Series acquires its Cover at creation (#59) (#68) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #59. Part of spec #55, and the ticket that fixes the reported bug #47. Architecture: `docs/adr/0007-backend-hosts-cover-bytes.md`. Does not close #47 or #55. ## What changed A Reader bookmarks a Series nobody holds yet — the exact case in #47 — and within seconds the list shows its artwork instead of a broken image. The first Bookmark to create a Series fires `Store.OnSeriesCreated` after commit, and the new `latest.Acquirer` turns that into **one** series-page fetch that yields both the Latest Chapter and the cover URL. The bytes go through the gated cover fetcher from #57 and are stored content-addressed through #56, so the wire carries an absolute URL on this deployment's own origin — never a third-party address, and never one that 404s. ### Store - Migration `0009_series_cover_address.sql` adds `series.cover_address`. The two facts are now split: `series.cover` is the third-party source address the bytes came from (the acquisition path's dedupe key), `series.cover_address` is the SHA-256 they are stored under. An empty `cover_address` is precisely what "no Cover yet" means, which is the distinction both the API and the UI depend on. - `SetSeriesCover` writes the address only after the bytes are on disk, so the wire can never name an object that is not there. - `CoverWireURL` builds `PUBLIC_BASE_URL + /covers/` for every scanned row, and returns `""` for a blank address. - The cover columns are gone from `Upsert`'s `INSERT` and its `DO UPDATE`. A client-supplied cover cannot reach the shared Series row on any path, not just the creation path. - `Open` now rejects a base URL that is not an absolute `http(s)` origin: `PUBLIC_BASE_URL=bookmarks.example.com` would otherwise start cleanly and emit addresses no browser can load. ### Acquisition - `internal/latest/acquire.go`: one fetch, gated by the poller's own `fetchableSeriesURL` (a `series_url` arrives in a client-supplied PUT body, so without the gate a token-holder chooses what the server fetches from its own network position). - Asynchronous and log-and-drop. The Bookmark, its progress and its Latest Chapter are already committed; a Site that is down or a cover that cannot be produced disturbs none of them. - Bounded by a two-slot semaphore. A bulk sync creating N Series would otherwise fire N simultaneous requests from one IP — the traffic shape the poller's stagger exists to avoid. - Cancelled at shutdown (shares the poller's context) and stamps `latest_checked_at`, so the poller does not refetch the same page a tick later. - Browser-backed Sites (kagane, novelfull) are deliberately skipped: their pages only yield a Cloudflare challenge to the TLS client, so the request would be spent for nothing. They arrive in #62. ### Wire and route - `GET /covers/{address}` serves the bytes publicly and uncredentialed with `Cache-Control: public, max-age=604800, immutable`. The address is gated by a `^[0-9a-f]{64}$` pattern and cross-checked against a pure function of itself before any filesystem read, so no request shaped like a traversal reaches disk. - `PUT /bookmarks/{key}` still accepts a `cover` field and discards it, permanently. Rejecting it would break every installed userscript the moment this deploys, and ADR-0004's compatibility argument depends on those scripts continuing to work. The decode site says so in place of a TODO nobody intends to keep. - `store.CoverContentType` canonicalises comix's non-standard `image/jpg` to `image/jpeg`, so one image cannot land under two spellings. This one was found by the live smoke test, not by reading. ### Config `PUBLIC_BASE_URL` is new and required (cover URLs must go out absolute — the userscript renders them on third-party origins, where a relative path resolves against the Site). Documented in `.env.example`, `docker-compose.yml` (`:?` so compose fails too), `DEPLOY.md` and `backend/AGENTS.md`. ## Acceptance criteria All twelve of #59's criteria are met; the checklist on the issue is ticked with the evidence. ## Verification - `go test ./...` green (Docker-backed Postgres suite). - Live smoke against a real backend + Postgres: bookmarking `comix:n8we-dungeons-and-crayons` produced `"cover": "http://127.0.0.1:8099/covers/8ce74d80…"` and `"latest_chapter": "Chapter 81"` within seconds of the PUT; `curl` on that address returned `200`, `Content-Type: image/jpeg`, `Cache-Control: public, max-age=604800, immutable`, and a 280x420 JPEG. That run is what surfaced the `image/jpg` content type. - Mutation-checked the asynchrony test: removing the `go` from `Acquire` turns `TestAcquireDoesNotBlockTheWrite` red. ## Reviewed Both axes of `/code-review` were run against this diff before commit. Their findings that were actionable here are folded in: the concurrency bound, the shutdown tie, the `PUBLIC_BASE_URL` validation, the missing `latest_checked_at` stamp, and a test that could not fail. ## Known sequencing A kagane/novelfull Series created between this deploy and #62 has no cover source at all: the acquisition skips those Sites and `Upsert` no longer persists the userscript-scraped address. This is #59's stated boundary rather than a defect, but it is a user-visible gap on two Sites and should order #62 accordingly. Reviewed-on: https://gitea.violetcrown.my.id/sulthan/mangaBookmark/pulls/68 Co-authored-by: Sulthan Zaki Co-committed-by: Sulthan Zaki --- .env.example | 5 + DEPLOY.md | 5 + backend/AGENTS.md | 15 ++ backend/api_test.go | 10 +- backend/cover_test.go | 84 +++++- backend/internal/api/handlers.go | 32 +++ backend/internal/latest/acquire.go | 136 ++++++++++ backend/internal/latest/acquire_test.go | 239 ++++++++++++++++++ backend/internal/latest/cover.go | 8 +- backend/internal/latest/cover_fetch_test.go | 20 ++ backend/internal/latest/poller.go | 23 +- backend/internal/latest/poller_test.go | 67 +++-- .../migrations/0009_series_cover_address.sql | 8 + backend/internal/store/store.go | 192 +++++++++++--- backend/internal/store/store_test.go | 204 ++++++++++++--- backend/internal/web/cover.go | 4 +- backend/main.go | 30 ++- docker-compose.yml | 4 + 18 files changed, 971 insertions(+), 115 deletions(-) create mode 100644 backend/internal/latest/acquire.go create mode 100644 backend/internal/latest/acquire_test.go create mode 100644 backend/internal/store/migrations/0009_series_cover_address.sql diff --git a/.env.example b/.env.example index 1fe4cb6..0d8e37e 100644 --- a/.env.example +++ b/.env.example @@ -29,6 +29,11 @@ POSTGRES_PASSWORD=changeme-generate-a-long-random-password # Compose builds the image and mounts its named volume at this path. COVER_DIR=/covers +# Public origin this deployment answers on, no trailing slash. Required: Cover +# URLs go out absolute, because the userscript renders them on a Site's own +# origin where a relative path would resolve against the Site (ADR-0007). +PUBLIC_BASE_URL=https://bookmark-api.example.com + # --- Prod override (Traefik) only --- # Subdomain Traefik routes to this service (required by the prod override). # BOOKMARK_API_HOST=bookmark-api.example.com diff --git a/DEPLOY.md b/DEPLOY.md index f83a5e3..2d57b29 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -57,6 +57,11 @@ POSTGRES_PASSWORD= # named cover-data volume at this path. COVER_DIR=/covers +# Required — the origin this deployment answers on, no trailing slash. Cover +# URLs on the wire are absolute, because the userscript renders them on a +# Site's own origin (ADR-0007). Same host as BOOKMARK_API_HOST below. +PUBLIC_BASE_URL=https://bookmark-api.violetcrown.my.id + # Required for the Traefik override. Both have no fallback — compose refuses # to start without them. BOOKMARK_WEB_HOST is required even if the web UI # were unused; see 1b. diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 220ce68..8f8a48c 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -98,6 +98,18 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN (`Store.SetLatestChapter`), not a read-modify-write of the whole bookmark: it cannot revert read progress or move `updated_at`, so the old stale-re-read race is gone with the Get+Upsert flow. +- **Covers are acquired at creation, then served from our own origin + (ADR-0007):** the first Bookmark of a Series fires `Store.OnSeriesCreated`, + which `latest.Acquirer` turns into one series-page fetch yielding both the + Latest Chapter and the cover URL; the bytes then go through + `latest.CoverBytesFetcher` into `Store.SetSeriesCover`. It runs in a + goroutine — the Reader's PUT must neither block on a Site nor fail with one + — and every failure is logged and dropped, leaving the Bookmark intact. The + wire's `cover` is the absolute `PUBLIC_BASE_URL + /covers/{sha256}` once + bytes exist and `""` before, never an address that 404s. `GET /covers/{addr}` + is public and uncredentialed: the userscript renders it on a Site's origin, + where no cookie or token of ours travels. A client-sent `cover` is decoded + and discarded, permanently (ADR-0004 compatibility). - **`updated_at` drives list order, so moves only on real reading progress:** server apply its timestamp when row new or `last_chapter_num` changes, else keep stored value — favouriting series or recording newly published chapter must not reorder list. `PUT` therefore returns row **as stored**, clients must adopt that response rather than own payload. See `plans/2026-07-25-bookmark-list-favorites-design.md` §4. - **Lifecycle buckets:** `status` on each bookmark is `reading` | `archived` | `finished`, orthogonal to `favorite`. Archived and finished appear only in @@ -115,6 +127,9 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN `ALLOWED_ORIGINS` (comma list), `DATABASE_URL` (Postgres connection URL, required — no default), `COVER_DIR` (required filesystem volume for content-addressed Cover bytes), + `PUBLIC_BASE_URL` (required origin this deployment answers on, trailing + slash trimmed; every Cover URL on the wire is built from it, absolute + because the userscript renders on a Site's origin — ADR-0007), `PORT` (default `8080`), `DISCORD_CLIENT_ID`/`_CLIENT_SECRET`/`_GUILD_ID`/ `_REDIRECT_URI` (required; Discord OAuth for the browser UI), `DISCORD_REQUIRED_ROLE` (optional role gate, empty by default), diff --git a/backend/api_test.go b/backend/api_test.go index 7f007c8..1d87b25 100644 --- a/backend/api_test.go +++ b/backend/api_test.go @@ -26,6 +26,10 @@ const testTokenKey = "test-token-key" // credential is a function of it. const testDiscordID = "test-owner" +// testCoverBaseURL is the public origin cover URLs are built from, standing in +// for PUBLIC_BASE_URL. +const testCoverBaseURL = "https://bookmarks.test" + func testConfig() Config { return Config{ TokenKey: testTokenKey, @@ -60,7 +64,7 @@ func newTestStoreURL(t *testing.T) (*store.Store, string) { url := pgtest.URL(t) s, err := store.Open(url, store.Owner{ DiscordID: testDiscordID, TokenHash: token.Hash(ownerCredential()), - }, t.TempDir()) + }, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("store.Open: %v", err) } @@ -329,12 +333,14 @@ func TestFlatWireFieldSet(t *testing.T) { latestNum := floatPtr(8) want := store.Bookmark{ Key: key, Site: "comix", SeriesID: "some-title", - Title: in.Title, SeriesURL: in.SeriesURL, Cover: in.Cover, + Title: in.Title, SeriesURL: in.SeriesURL, LastChapter: in.LastChapter, LastChapterNum: in.LastChapterNum, LastChapterURL: in.LastChapterURL, Favorite: true, LatestChapter: in.LatestChapter, LatestChapterNum: latestNum, Status: store.StatusArchived, Kind: store.KindManga, } + // Cover is deliberately absent above: the client's cover is discarded, and + // this wiring acquires none, so the field is present and empty (ADR-0007). if stored.Title != want.Title || stored.SeriesURL != want.SeriesURL || stored.Cover != want.Cover || stored.LastChapter != want.LastChapter || stored.LastChapterNum != want.LastChapterNum || stored.LastChapterURL != want.LastChapterURL || stored.Favorite != want.Favorite || diff --git a/backend/cover_test.go b/backend/cover_test.go index d22297e..660b845 100644 --- a/backend/cover_test.go +++ b/backend/cover_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "sync/atomic" "testing" @@ -106,7 +107,7 @@ func TestKaganeCoverServesPersistedBytesAfterRestart(t *testing.T) { DiscordID: "cover-owner", TokenHash: sha256.Sum256([]byte("cover-owner-token")), } - first, err := store.Open(url, owner, coverDir) + first, err := store.Open(url, owner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -118,7 +119,7 @@ func TestKaganeCoverServesPersistedBytesAfterRestart(t *testing.T) { t.Fatalf("close first store: %v", err) } - second, err := store.Open(url, owner, coverDir) + second, err := store.Open(url, owner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("reopen: %v", err) } @@ -224,3 +225,82 @@ func TestKaganeCoverWithoutFetcher(t *testing.T) { t.Fatalf("status = %d, want 404", rr.Code) } } + +// The acquired Cover is served from this deployment's own origin, to any +// browser rendering a third-party page — no session, no credential (ADR-0007). +func TestPublicCoverServesStoredBytesUnauthenticated(t *testing.T) { + const sourceURL = "https://cdn.asurascans.com/covers/solo.webp" + srv, st := newWebTestServer(t, testConfig()) + if err := st.PutCover(sourceURL, []byte("\x00webp-bytes"), "image/webp"); err != nil { + t.Fatalf("PutCover: %v", err) + } + + // The wire URL is what a client actually requests, so the path under test + // is taken from it rather than rebuilt by hand. + wire := st.CoverWireURL(store.CoverAddress(sourceURL)) + path, ok := strings.CutPrefix(wire, testCoverBaseURL) + if !ok { + t.Fatalf("wire URL %q is not on the public origin %q", wire, testCoverBaseURL) + } + rr := getCover(t, srv, path, nil) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 without any credential", rr.Code) + } + if got := rr.Body.String(); got != "\x00webp-bytes" { + t.Fatalf("body = %q, want the stored bytes", got) + } + if got := rr.Header().Get("Content-Type"); got != "image/webp" { + t.Fatalf("Content-Type = %q, want the stored one", got) + } + // Content-addressed bytes never change, so a client that has them must + // never need to ask again. + if got := rr.Header().Get("Cache-Control"); !strings.Contains(got, "immutable") { + t.Fatalf("Cache-Control = %q, want an immutable cache directive", got) + } +} + +func TestPublicCoverRejectsUnknownAddress(t *testing.T) { + srv, _ := newWebTestServer(t, testConfig()) + cases := map[string]string{ + "unknown": "/covers/" + store.CoverAddress("https://cdn.example/never-stored.jpg"), + "malformed": "/covers/not-an-address", + "traversal": "/covers/../../etc/passwd", + "empty": "/covers/", + } + for name, path := range cases { + t.Run(name, func(t *testing.T) { + if rr := getCover(t, srv, path, nil); rr.Code == http.StatusOK { + t.Fatalf("%s: status = 200, want anything but a served body", path) + } + }) + } +} + +// The whole point of acquiring bytes is that the UI shows them: the card's +// must carry the public address, not a third-party URL and not a +// placeholder. +func TestListRendersAcquiredCover(t *testing.T) { + const sourceURL = "https://static.comix.to/039d/i/1/34/6a6742bf15736@280.jpg" + srv, st := newWebTestServer(t, testConfig()) + if _, err := st.Upsert(st.OwnerID(), store.Bookmark{ + Key: "comix:n8we", Site: "comix", SeriesID: "n8we", Title: "Dungeons and Crayons", + SeriesURL: "https://comix.to/title/n8we", UpdatedAt: 1000, + }); err != nil { + t.Fatalf("seed: %v", err) + } + if err := st.SetSeriesCover("comix", "n8we", sourceURL, []byte("\xff\xd8jpeg"), "image/jpeg"); err != nil { + t.Fatalf("SetSeriesCover: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/ui/list", nil) + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rr.Code) + } + want := `src="` + testCoverBaseURL + "/covers/" + store.CoverAddress(sourceURL) + `"` + if !strings.Contains(rr.Body.String(), want) { + t.Fatalf("rendered list does not contain %s", want) + } +} diff --git a/backend/internal/api/handlers.go b/backend/internal/api/handlers.go index ae918ea..c48b489 100644 --- a/backend/internal/api/handlers.go +++ b/backend/internal/api/handlers.go @@ -50,6 +50,13 @@ func (h *Handler) Put(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid JSON body", http.StatusBadRequest) return } + // A body may carry a cover, and it is discarded here rather than + // rejected: every installed userscript still sends one, and ADR-0004's + // compatibility argument depends on those scripts continuing to work. The + // Cover is acquired server-side (ADR-0007), so the field is permanently + // inert - not pending removal, and not a value any later code should + // start reading. + b.Cover = "" // Path key is authoritative; derive site/series_id from it when the body // omits them so the stored row is always self-consistent. @@ -124,3 +131,28 @@ func Healthz(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("ok")) } + +// Cover serves stored cover bytes. GET /covers/{address} +// +// Public on purpose: the userscript renders these on Sites the deployment +// does not control, where no credential of ours may be sent, and the address +// is the SHA-256 of a URL the Site already publishes (ADR-0007). An unknown +// address is a 404 rather than an error - "no Cover yet" is a normal state, +// and the clients fall back to their placeholder. +func (h *Handler) Cover(w http.ResponseWriter, r *http.Request) { + body, contentType, ok, err := h.Store.CoverByAddress(r.PathValue("address")) + if err != nil { + log.Printf("cover: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if !ok { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", contentType) + // Content-addressed, so the bytes at this URL can never change. Public + // rather than private: no credential gates the route. + w.Header().Set("Cache-Control", "public, max-age=604800, immutable") + _, _ = w.Write(body) +} diff --git a/backend/internal/latest/acquire.go b/backend/internal/latest/acquire.go new file mode 100644 index 0000000..b4c71d1 --- /dev/null +++ b/backend/internal/latest/acquire.go @@ -0,0 +1,136 @@ +package latest + +import ( + "context" + "log" + "slices" + "sync" + "time" + + "bookmarkmanager/backend/internal/store" +) + +// acquireTimeout bounds one creation-time acquisition end to end: the series +// page plus the cover bytes. Nothing is waiting on it — the Reader's write has +// already returned — so this only stops a stalled Site from holding a +// goroutine and a connection open forever. +const acquireTimeout = 45 * time.Second + +// Acquirer gives a Series its Latest Chapter and its Cover the moment the +// first Bookmark creates it, instead of leaving the Reader to wait out the +// poll queue — which is ordered by Reader count, so a Series with one Reader +// sits behind every popular one (ADR-0007). +// +// Both facts come from a single series-page fetch, which is also why no +// client-supplied cover hint is worth accepting: the page has to be fetched +// for the chapter signal regardless, so a hint would save no request while +// adding a client-controlled input to a server-side fetch. +// +// Every failure path is "log and move on". The Bookmark, its progress and its +// Latest Chapter are already committed; a Site that is down or a Cover that +// cannot be produced must not disturb any of them, and the Series is simply +// left blank until the poll's own cover pass (#61) fills it. +type Acquirer struct { + Store *store.Store + // Fetch retrieves the series page. Nil disables acquisition entirely. + Fetch Fetcher + // Covers retrieves the cover bytes. Nil leaves the Cover blank and the + // chapter half working. + Covers CoverBytesFetcher + // Ctx cancels in-flight acquisitions at shutdown. A hook signature has + // nowhere to pass one, so it lives here; nil means context.Background. + Ctx context.Context + + inflight sync.WaitGroup +} + +// acquireSlots caps how many creation-time fetches run at once. A Reader whose +// userscript bulk-syncs creates many Series at once, and a burst of +// simultaneous requests from one server IP is the traffic shape most likely to +// move that IP's bot score — the same reason the poller staggers its batch. +var acquireSlots = make(chan struct{}, 2) + +// Acquire starts one acquisition and returns immediately: a Reader's bookmark +// action may not block on a third-party Site's latency, nor fail with it. It +// is the store's OnSeriesCreated hook, so it only ever runs for a Series no +// Reader had bookmarked before. +func (a *Acquirer) Acquire(sr store.Series) { + a.inflight.Add(1) + go func() { + defer a.inflight.Done() + defer func() { + if r := recover(); r != nil { + log.Printf("acquire %q: recovered from panic: %v", sr.Key(), r) + } + }() + parent := a.Ctx + if parent == nil { + parent = context.Background() + } + select { + case acquireSlots <- struct{}{}: + defer func() { <-acquireSlots }() + case <-parent.Done(): + return + } + ctx, cancel := context.WithTimeout(parent, acquireTimeout) + defer cancel() + a.acquire(ctx, sr) + }() +} + +// Wait blocks until every started acquisition has finished. It exists for +// tests: an asynchronous side effect is otherwise unobservable without +// polling for it. +func (a *Acquirer) Wait() { a.inflight.Wait() } + +func (a *Acquirer) acquire(ctx context.Context, sr store.Series) { + // Browser-backed Sites are deliberately not acquired here: their pages + // only yield a Cloudflare challenge to the TLS client, so the request + // would be spent for nothing. + if a.Fetch == nil || slices.Contains(browserBackedSites, sr.Site) { + return + } + // series_url arrives in a client-supplied PUT body, so the same gate the + // poller uses applies here — without it a token-holder chooses what the + // server fetches from its own network position. + if !fetchableSeriesURL(sr.Site, sr.SeriesURL) { + log.Printf("acquire %q: not fetchable: site=%q url=%q", sr.Key(), sr.Site, sr.SeriesURL) + return + } + + body, status, err := a.Fetch.Get(ctx, sr.SeriesURL) + if err != nil { + log.Printf("acquire %q: fetch %s: %v", sr.Key(), sr.SeriesURL, err) + return + } + if status != 200 { + log.Printf("acquire %q: fetch %s: status %d", sr.Key(), sr.SeriesURL, status) + return + } + + // This page just served the same purpose a poll tick would have; without + // the stamp the row stays due and the poller refetches it immediately. + if err := a.Store.MarkLatestChecked(sr.Site, sr.SeriesID, time.Now().UnixMilli()); err != nil { + log.Printf("acquire %q: mark checked: %v", sr.Key(), err) + } + + if latest, ok := latestChapterFrom(sr.Site, sr.SeriesURL, body); ok { + if err := a.Store.SetLatestChapter(sr.Site, sr.SeriesID, latest.Label, latest.Num); err != nil { + log.Printf("acquire %q: set latest chapter: %v", sr.Key(), err) + } + } + + cover, ok := coverFrom(sr.Site, sr.SeriesURL, body) + if !ok || a.Covers == nil { + return + } + bytes, contentType, err := a.Covers.Fetch(ctx, cover) + if err != nil { + log.Printf("acquire %q: fetch cover %s: %v", sr.Key(), cover, err) + return + } + if err := a.Store.SetSeriesCover(sr.Site, sr.SeriesID, cover, bytes, contentType); err != nil { + log.Printf("acquire %q: persist cover: %v", sr.Key(), err) + } +} diff --git a/backend/internal/latest/acquire_test.go b/backend/internal/latest/acquire_test.go new file mode 100644 index 0000000..af142d3 --- /dev/null +++ b/backend/internal/latest/acquire_test.go @@ -0,0 +1,239 @@ +package latest + +import ( + "context" + "errors" + "testing" + "time" + + "bookmarkmanager/backend/internal/store" +) + +// The series page carries both facts, which is the whole argument for taking +// them from one fetch. +const asuraSeriesAndCoverFixture = asuraSeriesFixture + asuraCoverFixture + +const ( + acquireKey = "asura:chronicles-of-the-demon-faction-f886a8af" + acquireSeriesID = "chronicles-of-the-demon-faction-f886a8af" + acquireSeriesURL = "https://asurascans.com/comics/chronicles-of-the-demon-faction-f886a8af" + acquireCoverURL = "https://cdn.asurascans.com/asura-images/covers/chronicles-of-the-demon-faction.d4dcb8.webp" +) + +// newAcquirer wires an acquirer onto the store's creation hook, which is how +// main wires it: the write path is what starts an acquisition. +func newAcquirer(s *store.Store, page *fakeFetcher, covers *fakeBytesCoverFetcher) *Acquirer { + a := &Acquirer{Store: s, Fetch: page, Covers: covers} + s.OnSeriesCreated = a.Acquire + return a +} + +func bookmarkNewSeries(t *testing.T, s *store.Store, seriesURL string) store.Bookmark { + t.Helper() + stored, err := s.Upsert(s.OwnerID(), store.Bookmark{ + Key: acquireKey, Site: "asura", SeriesID: acquireSeriesID, + Title: "Chronicles of the Demon Faction", SeriesURL: seriesURL, + Cover: "https://evil.example/client-supplied.jpg", UpdatedAt: 1000, + }) + if err != nil { + t.Fatalf("Upsert: %v", err) + } + return stored +} + +func readBookmark(t *testing.T, s *store.Store, key string) store.Bookmark { + t.Helper() + b, ok, err := s.Get(s.OwnerID(), key) + if err != nil || !ok { + t.Fatalf("Get %q = %v, %v", key, ok, err) + } + return b +} + +// The reported bug: a Reader bookmarks a Series nobody holds and expects the +// Cover, not a broken image. Both facts come from the one series-page fetch. +func TestAcquireFillsChapterAndCoverFromOneFetch(t *testing.T) { + s, _ := newTestStore(t) + page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200} + covers := &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"} + acq := newAcquirer(s, page, covers) + + // The write itself must not carry the acquisition: it returns before the + // Cover exists, and the field is empty until the bytes land. + stored := bookmarkNewSeries(t, s, acquireSeriesURL) + if stored.Cover != "" { + t.Fatalf("Cover on the creating write = %q, want empty", stored.Cover) + } + acq.Wait() + + if got := page.callCount(); got != 1 { + t.Fatalf("series page fetches = %d, want exactly 1", got) + } + if got := covers.callCount(); got != 1 { + t.Fatalf("cover fetches = %d, want 1", got) + } + got := readBookmark(t, s, acquireKey) + if got.LatestChapterNum == nil || *got.LatestChapterNum != 181 { + t.Fatalf("LatestChapterNum = %v, want 181", got.LatestChapterNum) + } + if want := testCoverBaseURL + "/covers/" + store.CoverAddress(acquireCoverURL); got.Cover != want { + t.Fatalf("Cover = %q, want the absolute address %q", got.Cover, want) + } + body, contentType, ok, err := s.CoverByAddress(store.CoverAddress(acquireCoverURL)) + if err != nil || !ok { + t.Fatalf("CoverByAddress = %v, %v", ok, err) + } + if string(body) != "cover-bytes" || contentType != "image/jpeg" { + t.Fatalf("stored cover = (%q, %q), want the fetched bytes", body, contentType) + } +} + +// A Series that already exists is not re-acquired: no fetch, and the Cover it +// already has is left alone. +func TestAcquireSkipsAnExistingSeries(t *testing.T) { + s, _ := newTestStore(t) + page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200} + covers := &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"} + acq := newAcquirer(s, page, covers) + + bookmarkNewSeries(t, s, acquireSeriesURL) + acq.Wait() + bookmarkNewSeries(t, s, acquireSeriesURL) + acq.Wait() + + if got := page.callCount(); got != 1 { + t.Fatalf("series page fetches = %d, want 1 — an existing series is not re-acquired", got) + } + if got := covers.callCount(); got != 1 { + t.Fatalf("cover fetches = %d, want 1", got) + } + got := readBookmark(t, s, acquireKey) + if want := testCoverBaseURL + "/covers/" + store.CoverAddress(acquireCoverURL); got.Cover != want { + t.Fatalf("Cover = %q, want the acquired one %q", got.Cover, want) + } +} + +// A Site that is down costs the Cover and nothing else. +func TestAcquireFailureLeavesTheBookmarkIntact(t *testing.T) { + cases := []struct { + name string + page *fakeFetcher + covers *fakeBytesCoverFetcher + // wantLatest is the chapter that still lands; 0 means none did. + wantLatest float64 + }{ + { + "the series page is unreachable", + &fakeFetcher{err: errors.New("connection reset")}, + &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"}, + 0, + }, + { + "the series page answers with a challenge", + &fakeFetcher{body: challengeFixture, status: 200}, + &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"}, + 0, + }, + { + "only the cover bytes fail", + &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200}, + &fakeBytesCoverFetcher{err: errors.New("403")}, + 181, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s, _ := newTestStore(t) + acq := newAcquirer(s, tc.page, tc.covers) + + stored := bookmarkNewSeries(t, s, acquireSeriesURL) + acq.Wait() + + got := readBookmark(t, s, acquireKey) + if got.Cover != "" { + t.Fatalf("Cover = %q, want empty rather than an address that 404s", got.Cover) + } + if got.Title != stored.Title || got.UpdatedAt != stored.UpdatedAt { + t.Fatalf("bookmark = %+v, want it untouched by the failed acquisition", got) + } + if tc.wantLatest == 0 { + if got.LatestChapterNum != nil { + t.Fatalf("LatestChapterNum = %v, want none captured", *got.LatestChapterNum) + } + return + } + if got.LatestChapterNum == nil || *got.LatestChapterNum != tc.wantLatest { + t.Fatalf("LatestChapterNum = %v, want %v", got.LatestChapterNum, tc.wantLatest) + } + }) + } +} + +// series_url arrives in a client-supplied body, so the acquisition reuses the +// poller's gate rather than deriving a second one: a non-https scheme, a +// site the parsers do not know, or a host pinned to another site is refused +// before the server spends a request from its own network position. +func TestAcquireRefusesAnUnfetchableSeriesURL(t *testing.T) { + for _, seriesURL := range []string{ + "http://asurascans.com/comics/x", + "file:///etc/passwd", + "", + } { + t.Run(seriesURL, func(t *testing.T) { + s, _ := newTestStore(t) + page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200} + acq := newAcquirer(s, page, &fakeBytesCoverFetcher{}) + + bookmarkNewSeries(t, s, seriesURL) + acq.Wait() + + if got := page.callCount(); got != 0 { + t.Fatalf("fetches for %q = %d, want 0", seriesURL, got) + } + }) + } +} + +// blockingFetcher stands in for a Site that never answers, so a synchronous +// acquisition would be visible as a stalled write rather than a slow one. +type blockingFetcher struct { + release <-chan struct{} + body string +} + +func (f *blockingFetcher) Get(ctx context.Context, _ string) (string, int, error) { + select { + case <-f.release: + return f.body, 200, nil + case <-ctx.Done(): + return "", 0, ctx.Err() + } +} + +// The Reader's write may not wait on a third-party Site: with the acquisition +// wedged on an unanswering page, the PUT still returns. +func TestAcquireDoesNotBlockTheWrite(t *testing.T) { + s, _ := newTestStore(t) + release := make(chan struct{}) + acq := &Acquirer{Store: s, Fetch: &blockingFetcher{release: release, body: asuraSeriesAndCoverFixture}} + s.OnSeriesCreated = acq.Acquire + + upserted := make(chan error, 1) + go func() { + _, err := s.Upsert(s.OwnerID(), store.Bookmark{ + Key: acquireKey, Site: "asura", SeriesID: acquireSeriesID, + Title: "Chronicles of the Demon Faction", SeriesURL: acquireSeriesURL, UpdatedAt: 1000, + }) + upserted <- err + }() + select { + case err := <-upserted: + if err != nil { + t.Fatalf("Upsert: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("the creating write blocked on the acquisition") + } + close(release) + acq.Wait() +} diff --git a/backend/internal/latest/cover.go b/backend/internal/latest/cover.go index 2487dc2..b14d473 100644 --- a/backend/internal/latest/cover.go +++ b/backend/internal/latest/cover.go @@ -123,10 +123,14 @@ func (f *TLSCoverFetcher) Fetch(ctx context.Context, sourceURL string) ([]byte, if resp.StatusCode != http.StatusOK { return nil, "", fmt.Errorf("fetch cover: status %d", resp.StatusCode) } - contentType, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) - if err != nil || !store.IsCoverContentType(contentType) { + raw, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if err != nil { return nil, "", fmt.Errorf("fetch cover: unsupported content type %q", resp.Header.Get("Content-Type")) } + contentType, ok := store.CoverContentType(raw) + if !ok { + return nil, "", fmt.Errorf("fetch cover: unsupported content type %q", raw) + } if resp.ContentLength > maxBodyBytes { return nil, "", fmt.Errorf("fetch cover: response exceeds %d bytes", maxBodyBytes) } diff --git a/backend/internal/latest/cover_fetch_test.go b/backend/internal/latest/cover_fetch_test.go index fd3c188..5b847ab 100644 --- a/backend/internal/latest/cover_fetch_test.go +++ b/backend/internal/latest/cover_fetch_test.go @@ -204,3 +204,23 @@ func TestCoverFetcherRejectsNonImage(t *testing.T) { t.Fatalf("network calls = %d, want 1", calls) } } + +// comix labels its covers "image/jpg", which is not a registered type but is +// what the Site actually answers with; the bytes are stored under the real +// name so one image cannot land under two spellings. +func TestCoverFetcherCanonicalisesJpgAlias(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return coverResponse(http.StatusOK, "image/jpg", "", []byte("cover-bytes")), nil + })} + fetcher := newCoverFetcher(client, func(context.Context, string) ([]netip.Addr, error) { + return []netip.Addr{netip.MustParseAddr("198.51.100.10")}, nil + }) + + body, contentType, err := fetcher.Fetch(context.Background(), "https://static.comix.to/cover.jpg") + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if string(body) != "cover-bytes" || contentType != "image/jpeg" { + t.Fatalf("Fetch = (%q, %q), want (cover-bytes, image/jpeg)", body, contentType) + } +} diff --git a/backend/internal/latest/poller.go b/backend/internal/latest/poller.go index e867ade..0a344d9 100644 --- a/backend/internal/latest/poller.go +++ b/backend/internal/latest/poller.go @@ -87,23 +87,26 @@ func (p *Poller) prefetchCover(ctx context.Context, sr store.Series) { } return } - if p.CoverBytesFetch == nil { + if p.CoverBytesFetch == nil || sr.CoverAddress != "" { return } - _, _, found, err := p.Store.GetCover(sr.Cover) + // Bytes may already be stored from an earlier poll that ran before the + // Series carried an address; storing them again is free (they are + // content-addressed and immutable), and the point of the second call is + // the address, which is what makes the Cover visible on the wire. + body, contentType, found, err := p.Store.GetCover(sr.Cover) if err != nil { log.Printf("latest poll %q: read cover: %v", sr.Key(), err) return } - if found { - return + if !found { + body, contentType, err = p.CoverBytesFetch.Fetch(ctx, sr.Cover) + if err != nil { + log.Printf("latest poll %q: fetch cover: %v", sr.Key(), err) + return + } } - body, contentType, err := p.CoverBytesFetch.Fetch(ctx, sr.Cover) - if err != nil { - log.Printf("latest poll %q: fetch cover: %v", sr.Key(), err) - return - } - if err := p.Store.PutCover(sr.Cover, body, contentType); err != nil { + if err := p.Store.SetSeriesCover(sr.Site, sr.SeriesID, sr.Cover, body, contentType); err != nil { log.Printf("latest poll %q: persist cover: %v", sr.Key(), err) } } diff --git a/backend/internal/latest/poller_test.go b/backend/internal/latest/poller_test.go index 84eefe4..2bcc8b7 100644 --- a/backend/internal/latest/poller_test.go +++ b/backend/internal/latest/poller_test.go @@ -3,6 +3,7 @@ package latest import ( "context" "crypto/sha256" + "database/sql" "errors" "log" "os" @@ -21,13 +22,16 @@ func TestMain(m *testing.M) { os.Exit(pgtest.Main(m)) } // test needs one, is created by opening the same database as a second owner. var testOwner = store.Owner{DiscordID: "test-owner", TokenHash: sha256.Sum256([]byte("owner-token-hash"))} +// testCoverBaseURL is the public origin every stored cover URL is built from. +const testCoverBaseURL = "https://bookmarks.test" + // newTestStore opens a store on a Postgres database of this test's own and // returns the URL, for helpers that need a second connection to the same // database (see TestRunOnceFetchesSharedSeriesOnce). func newTestStore(t *testing.T) (*store.Store, string) { t.Helper() url := pgtest.URL(t) - s, err := store.Open(url, testOwner, t.TempDir()) + s, err := store.Open(url, testOwner, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -168,8 +172,27 @@ func newTestPoller(t *testing.T, s *store.Store, f Fetcher, at time.Time) *Polle } } +// seedCoverSource writes a Series' cover source address without any stored +// bytes. Nothing in production produces that state any more — a client cover +// is discarded and an acquired one arrives with its bytes — but rows created +// before covers moved server-side still carry one, and the prefetch is what +// heals them. +func seedCoverSource(t *testing.T, dbURL, site, seriesID, coverURL string) { + t.Helper() + db, err := sql.Open("pgx", dbURL) + if err != nil { + t.Fatalf("open %s: %v", dbURL, err) + } + defer db.Close() + if _, err := db.Exec( + `UPDATE series SET cover = $3 WHERE site = $1 AND series_id = $2`, + site, seriesID, coverURL); err != nil { + t.Fatalf("seed cover source %s:%s: %v", site, seriesID, err) + } +} + func TestRunOncePrefetchesPublicCover(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "asura:chronicles-of-the-demon-faction-f886a8af" seriesURL = "https://asurascans.com/comics/chronicles-of-the-demon-faction-f886a8af" @@ -177,10 +200,11 @@ func TestRunOncePrefetchesPublicCover(t *testing.T) { ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "asura", SeriesID: "chronicles-of-the-demon-faction-f886a8af", - SeriesURL: seriesURL, Cover: coverURL, UpdatedAt: 1000, + SeriesURL: seriesURL, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "asura", "chronicles-of-the-demon-faction-f886a8af", coverURL) covers := &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"} p := &Poller{ Store: s, Fetch: &fakeFetcher{body: asuraSeriesFixture, status: 200}, CoverBytesFetch: covers, @@ -201,18 +225,19 @@ func TestRunOncePrefetchesPublicCover(t *testing.T) { } func TestRunOnceDoesNotStoreNonImagePublicCover(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "asura:non-image-cover" seriesURL = "https://asurascans.com/comics/non-image-cover" coverURL = "https://cdn.example/covers/challenge" ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ - Key: key, Site: "asura", SeriesID: "non-image-cover", SeriesURL: seriesURL, Cover: coverURL, + Key: key, Site: "asura", SeriesID: "non-image-cover", SeriesURL: seriesURL, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "asura", "non-image-cover", coverURL) p := &Poller{ Store: s, Fetch: &fakeFetcher{body: asuraSeriesFixture, status: 200}, CoverBytesFetch: &fakeBytesCoverFetcher{body: []byte("challenge"), contentType: "text/html"}, @@ -355,7 +380,7 @@ func TestRunOnceFetchesSharedSeriesOnce(t *testing.T) { // A second reader tracks the same series. The seed is the only // reader-creation path, so a second Open as a different owner is how a // test gets a second reader on the same database. - other, err := store.Open(url, store.Owner{DiscordID: "second-reader", TokenHash: sha256.Sum256([]byte("second-token-hash"))}, t.TempDir()) + other, err := store.Open(url, store.Owner{DiscordID: "second-reader", TokenHash: sha256.Sum256([]byte("second-token-hash"))}, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("Open second reader: %v", err) } @@ -686,7 +711,7 @@ func TestKaganeUsesBrowserFetcher(t *testing.T) { } func TestRunOncePrefetchesKaganeCover(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "kagane:019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" coverURL = "https://kagane.to/api/v2/image/019f84bc-9ba0-7ed9-86f5-8b905ec7c28b/compressed" @@ -694,10 +719,11 @@ func TestRunOncePrefetchesKaganeCover(t *testing.T) { if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "kagane", SeriesID: "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b", SeriesURL: "https://kagane.to/series/019f84bc-9ba0-7ed9-86f5-8b905ec7c28b", - Cover: coverURL, UpdatedAt: 1000, + UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "kagane", "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b", coverURL) covers := &fakeCoverFetcher{body: []byte("cover-bytes"), contentType: "image/webp"} p := &Poller{ Store: s, Fetch: &fakeFetcher{body: kaganeAPIFixture, status: 200}, @@ -720,7 +746,7 @@ func TestRunOncePrefetchesKaganeCover(t *testing.T) { } func TestRunOnceDoesNotRefetchKaganeCover(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "kagane:019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" seriesID = "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" @@ -728,10 +754,11 @@ func TestRunOnceDoesNotRefetchKaganeCover(t *testing.T) { ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "kagane", SeriesID: seriesID, - SeriesURL: "https://kagane.to/series/" + seriesID, Cover: coverURL, UpdatedAt: 1000, + SeriesURL: "https://kagane.to/series/" + seriesID, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "kagane", seriesID, coverURL) at := time.UnixMilli(5_000_000) covers := &fakeCoverFetcher{body: []byte("cover-bytes"), contentType: "image/webp"} p := &Poller{ @@ -748,7 +775,7 @@ func TestRunOnceDoesNotRefetchKaganeCover(t *testing.T) { } func TestRunOnceCoverFailureDoesNotBlockChapter(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "kagane:019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" seriesID = "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" @@ -756,10 +783,11 @@ func TestRunOnceCoverFailureDoesNotBlockChapter(t *testing.T) { ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "kagane", SeriesID: seriesID, - SeriesURL: "https://kagane.to/series/" + seriesID, Cover: coverURL, UpdatedAt: 1000, + SeriesURL: "https://kagane.to/series/" + seriesID, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "kagane", seriesID, coverURL) now := time.UnixMilli(5_000_000) p := &Poller{ Store: s, BrowserFetch: &fakeFetcher{body: kaganeAPIFixture, status: 200}, @@ -781,7 +809,7 @@ func TestRunOnceCoverFailureDoesNotBlockChapter(t *testing.T) { } func TestRunOnceRejectsInvalidKaganeCover(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "kagane:019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" seriesID = "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" @@ -789,10 +817,11 @@ func TestRunOnceRejectsInvalidKaganeCover(t *testing.T) { ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "kagane", SeriesID: seriesID, - SeriesURL: "https://kagane.to/series/" + seriesID, Cover: coverURL, UpdatedAt: 1000, + SeriesURL: "https://kagane.to/series/" + seriesID, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "kagane", seriesID, coverURL) p := &Poller{ Store: s, BrowserFetch: &fakeFetcher{body: kaganeAPIFixture, status: 200}, CoverFetch: &fakeCoverFetcher{body: []byte("not an image"), contentType: "text/html"}, @@ -806,7 +835,7 @@ func TestRunOnceRejectsInvalidKaganeCover(t *testing.T) { } func TestRunOnceWithoutCoverFetcherStillPollsKagane(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const ( key = "kagane:019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" seriesID = "019f84bc-9ba0-7ed9-86f5-8b905ec7c28b" @@ -814,10 +843,11 @@ func TestRunOnceWithoutCoverFetcherStillPollsKagane(t *testing.T) { ) if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "kagane", SeriesID: seriesID, - SeriesURL: "https://kagane.to/series/" + seriesID, Cover: coverURL, UpdatedAt: 1000, + SeriesURL: "https://kagane.to/series/" + seriesID, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "kagane", seriesID, coverURL) p := &Poller{ Store: s, BrowserFetch: &fakeFetcher{body: kaganeAPIFixture, status: 200}, Now: func() time.Time { return time.UnixMilli(5_000_000) }, Cooldown: time.Hour, BrowserCooldown: time.Hour, Batch: 10, @@ -830,15 +860,16 @@ func TestRunOnceWithoutCoverFetcherStillPollsKagane(t *testing.T) { } func TestRunOnceRoutesNonKaganeCoverToPublicFetcher(t *testing.T) { - s, _ := newTestStore(t) + s, dbURL := newTestStore(t) const key = "asura:solo" const coverURL = "https://asurascans.com/covers/solo.jpg" if _, err := s.Upsert(s.OwnerID(), store.Bookmark{ Key: key, Site: "asura", SeriesID: "solo", SeriesURL: "https://asurascans.com/comics/solo", - Cover: coverURL, UpdatedAt: 1000, + UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + seedCoverSource(t, dbURL, "asura", "solo", coverURL) browserCovers := &fakeCoverFetcher{body: []byte("must not be fetched"), contentType: "image/webp"} publicCovers := &fakeBytesCoverFetcher{body: []byte("public cover"), contentType: "image/webp"} p := &Poller{ diff --git a/backend/internal/store/migrations/0009_series_cover_address.sql b/backend/internal/store/migrations/0009_series_cover_address.sql new file mode 100644 index 0000000..92a8b13 --- /dev/null +++ b/backend/internal/store/migrations/0009_series_cover_address.sql @@ -0,0 +1,8 @@ +-- The Cover splits into two facts. `cover` keeps the third-party address the +-- bytes come from, which is what the acquisition path refetches and dedupes +-- on; `cover_address` is the content address of the bytes once they are +-- actually stored, and is what the wire's absolute URL is built from. +-- +-- Empty `cover_address` therefore means "no Cover yet" rather than "a Cover +-- that 404s", which is the distinction the API and the UI both depend on. +ALTER TABLE series ADD COLUMN cover_address text NOT NULL DEFAULT ''; diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 50a1366..1237fe1 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -30,12 +30,20 @@ import ( // between readers: progress, favourite, lifecycle bucket, updated_at. The wire // format stays flat regardless — see ADR-0004. type Bookmark struct { - Key string `json:"key"` - Site string `json:"site"` - SeriesID string `json:"series_id"` - Title string `json:"title"` - SeriesURL string `json:"series_url"` - Cover string `json:"cover"` + Key string `json:"key"` + Site string `json:"site"` + SeriesID string `json:"series_id"` + Title string `json:"title"` + SeriesURL string `json:"series_url"` + // Cover is the wire value: an absolute URL on this deployment's own + // origin once the bytes exist, and "" until they do — never a third-party + // address and never an address that 404s (ADR-0007). A client may still + // send this field and it is discarded on the way in; see Upsert. + Cover string `json:"cover"` + // CoverSource is the third-party address the bytes were fetched from. It + // stays off the wire: it is the acquisition path's dedupe key, and no + // client is ever asked to render one. + CoverSource string `json:"-"` LastChapter string `json:"last_chapter"` LastChapterNum float64 `json:"last_chapter_num"` LastChapterURL string `json:"last_chapter_url"` @@ -62,11 +70,16 @@ type Bookmark struct { // bookmark's own fields. Never serialized: the wire format is the flat // Bookmark (ADR-0004). type Series struct { - Site string - SeriesID string - Title string - SeriesURL string + Site string + SeriesID string + Title string + SeriesURL string + // Cover is the third-party source address the bytes come from, and + // CoverAddress the content address they are stored under. A blank + // CoverAddress is what "no Cover yet" means: the poll fills it and never + // replaces a filled one (ADR-0007). Cover string + CoverAddress string Kind string LatestChapter string LatestChapterNum *float64 // nil until first captured @@ -156,23 +169,27 @@ func KaganeImageID(cover string) (string, bool) { return m[1], true } -// IsCoverContentType reports whether a fetched response is safe to store and serve. -func IsCoverContentType(contentType string) bool { +// CoverContentType canonicalises a fetched response's media type and reports +// whether the bytes are safe to store and serve. comix answers "image/jpg", +// which no standard lists but browsers accept; it is stored as the real name +// rather than passed through, so one image never lands under two spellings. +func CoverContentType(contentType string) (string, bool) { switch contentType { + case "image/jpg": + return "image/jpeg", true case "image/webp", "image/jpeg", "image/png", "image/avif", "image/gif": - return true + return contentType, true default: - return false + return "", false } } -// CoverURL is the src the web UI puts in an . For every site but kagane -// that is Cover as stored. kagane serves its images behind a Cloudflare -// challenge *and* with `cross-origin-resource-policy: same-origin`, so no page -// on another origin can load one however it asks (verified 2026-08-08); those -// go through the backend's own proxy instead. +// CoverURL is the src the web UI puts in an . Cover already is an address +// on this origin, so for every site but kagane it is used as-is. kagane's +// bytes still arrive through the browser-backed proxy, which is keyed by image +// id rather than by content address until #62 moves it onto the same path. func (b Bookmark) CoverURL() string { - if imageID, ok := KaganeImageID(b.Cover); ok { + if imageID, ok := KaganeImageID(b.CoverSource); ok { return "/img/kagane/" + imageID } return b.Cover @@ -200,14 +217,14 @@ var migrations embed.FS // compile-time constant; every request value is bound as a parameter. The // series-owned fields are joined in from the series table, in scanBookmark // order, so the flat Bookmark reads back whole despite the split (ADR-0004). -const bookmarkColumns = `b.site, b.series_id, s.title, s.series_url, s.cover, +const bookmarkColumns = `b.site, b.series_id, s.title, s.series_url, s.cover, s.cover_address, b.last_chapter, b.last_chapter_num, b.last_chapter_url, b.favorite, s.latest_chapter, s.latest_chapter_num, b.updated_at, b.status, s.kind` // seriesColumns is the series row in scanSeries order, used by the poller's // due query. latest_checked_at lives only on series — see MarkLatestChecked // for why it stays off every client-visible write. -const seriesColumns = `s.site, s.series_id, s.title, s.series_url, s.cover, +const seriesColumns = `s.site, s.series_id, s.title, s.series_url, s.cover, s.cover_address, s.kind, s.latest_chapter, s.latest_chapter_num, s.latest_checked_at` // Owner is the person running the service: the first Reader, seeded at startup @@ -230,6 +247,16 @@ type Store struct { // method takes a reader id explicitly, so ownership is never implicit. ownerID int64 coverDir string + // coverBaseURL is this deployment's public origin. Cover addresses are + // absolute because the userscript renders them on third-party origins, + // where a relative path would resolve against the Site (ADR-0007). + coverBaseURL string + // OnSeriesCreated fires once, after commit, for a Series no Reader had + // bookmarked before. It is how creation-time Cover and Latest Chapter + // acquisition is triggered without the write waiting on a third-party + // Site; nil disables it, which is what every test that does not care + // about acquisition leaves it as. + OnSeriesCreated func(Series) } // OwnerID returns the seeded owner Reader's id: the administrator, and the @@ -365,10 +392,19 @@ const allMigrations = 0 // Open connects to Postgres at url — a libpq connection URL such as // "postgres://user:pass@host:5432/bookmarks?sslmode=disable" — brings its // schema up to date, seeds the owner Reader, and prepares cover storage. -func Open(url string, owner Owner, coverDir string) (*Store, error) { +func Open(url string, owner Owner, coverDir, coverBaseURL string) (*Store, error) { if strings.TrimSpace(coverDir) == "" { return nil, errors.New("cover directory is required") } + // Every wire Cover is this string with a path glued on, rendered by a + // userscript on a Site's own origin: anything but an absolute origin + // produces addresses no client can load, silently (ADR-0007). + base := strings.TrimRight(coverBaseURL, "/") + if host, ok := strings.CutPrefix(base, "https://"); !ok || host == "" { + if host, ok := strings.CutPrefix(base, "http://"); !ok || host == "" { + return nil, fmt.Errorf("cover base URL %q is not an absolute http(s) origin", coverBaseURL) + } + } if err := os.MkdirAll(coverDir, 0o755); err != nil { return nil, fmt.Errorf("create cover directory: %w", err) } @@ -414,7 +450,9 @@ func Open(url string, owner Owner, coverDir string) (*Store, error) { db.Close() return nil, fmt.Errorf("resolve owner: %w", err) } - return &Store{db: db, ownerID: ownerID, coverDir: coverDir}, nil + return &Store{ + db: db, ownerID: ownerID, coverDir: coverDir, coverBaseURL: base, + }, nil } // seedOwner makes sure the configured owner exists as exactly one readers row. @@ -517,18 +555,20 @@ func applyMigration(db *sql.DB, version int64, body string) error { // scanBookmark reads one row in bookmarkColumns order. Every column is NOT // NULL except latest_chapter_num, where NULL means "never captured" — a // distinct state from chapter zero, and the reason for the pointer. -func scanBookmark(scan func(...any) error) (Bookmark, error) { +func (s *Store) scanBookmark(scan func(...any) error) (Bookmark, error) { var ( b Bookmark + coverAddress string latestChapterNum sql.NullFloat64 ) if err := scan( - &b.Site, &b.SeriesID, &b.Title, &b.SeriesURL, &b.Cover, + &b.Site, &b.SeriesID, &b.Title, &b.SeriesURL, &b.CoverSource, &coverAddress, &b.LastChapter, &b.LastChapterNum, &b.LastChapterURL, &b.Favorite, &b.LatestChapter, &latestChapterNum, &b.UpdatedAt, &b.Status, &b.Kind, ); err != nil { return Bookmark{}, err } + b.Cover = s.CoverWireURL(coverAddress) if latestChapterNum.Valid { b.LatestChapterNum = &latestChapterNum.Float64 } @@ -553,7 +593,7 @@ func scanSeries(scan func(...any) error) (Series, error) { latestChapterNum sql.NullFloat64 ) if err := scan( - &sr.Site, &sr.SeriesID, &sr.Title, &sr.SeriesURL, &sr.Cover, + &sr.Site, &sr.SeriesID, &sr.Title, &sr.SeriesURL, &sr.Cover, &sr.CoverAddress, &sr.Kind, &sr.LatestChapter, &latestChapterNum, &sr.LatestCheckedAt, &sr.readerCount, ); err != nil { @@ -582,7 +622,10 @@ func kaganeCoverSourceURL(imageID string) string { } func (s *Store) getCover(sourceURL string) ([]byte, string, bool, error) { - address := coverSourceAddress(sourceURL) + return s.getCoverByAddress(coverSourceAddress(sourceURL)) +} + +func (s *Store) getCoverByAddress(address string) ([]byte, string, bool, error) { var relativePath, contentType string err := s.db.QueryRow( `SELECT path, content_type FROM covers WHERE address = $1`, address, @@ -608,9 +651,11 @@ func (s *Store) getCover(sourceURL string) ([]byte, string, bool, error) { } func (s *Store) putCover(sourceURL string, body []byte, contentType string) error { - if !IsCoverContentType(contentType) { + stored, ok := CoverContentType(contentType) + if !ok { return fmt.Errorf("put cover %q: unsupported content type %q", sourceURL, contentType) } + contentType = stored address := coverSourceAddress(sourceURL) relativePath := coverRelativePath(address) coverPath := filepath.Join(s.coverDir, filepath.FromSlash(relativePath)) @@ -670,6 +715,55 @@ func (s *Store) PutKaganeCover(imageID string, body []byte, contentType string) return s.putCover(kaganeCoverSourceURL(imageID), body, contentType) } +// CoverAddress is the content address bytes fetched from sourceURL are stored +// under. It is a pure function of the URL, so the acquisition path can name a +// Cover before it has the bytes. +func CoverAddress(sourceURL string) string { return coverSourceAddress(sourceURL) } + +// coverAddressRe is the shape of a stored address: the hex SHA-256 of a source +// URL. Request paths reach CoverByAddress, so the shape is checked before the +// value is ever turned into a filesystem path. +var coverAddressRe = regexp.MustCompile(`^[0-9a-f]{64}$`) + +// CoverByAddress returns the immutable object at one content address. An +// address that is not a stored one - malformed, unknown, or recorded but with +// its file gone - is reported with ok=false rather than as an error. +func (s *Store) CoverByAddress(address string) ([]byte, string, bool, error) { + if !coverAddressRe.MatchString(address) { + return nil, "", false, nil + } + return s.getCoverByAddress(address) +} + +// CoverWireURL is the absolute URL a client renders for a stored Cover, and "" +// for a Series that has none yet. A blank is a real state, not a placeholder +// address: it is what tells both clients to draw their own fallback instead of +// requesting bytes that do not exist (ADR-0007). +func (s *Store) CoverWireURL(address string) string { + if address == "" { + return "" + } + return s.coverBaseURL + "/covers/" + address +} + +// SetSeriesCover stores the bytes and points the Series at them, but only +// while the Series has no Cover: acquisition at creation and the poll both +// call this, and whichever arrives second must not overwrite the first. The +// bytes themselves are content-addressed and immutable, so storing them twice +// is free. +func (s *Store) SetSeriesCover(site, seriesID, sourceURL string, body []byte, contentType string) error { + if err := s.putCover(sourceURL, body, contentType); err != nil { + return err + } + if _, err := s.db.Exec(` + UPDATE series SET cover = $3, cover_address = $4 + WHERE site = $1 AND series_id = $2 AND cover_address = ''`, + site, seriesID, sourceURL, coverSourceAddress(sourceURL)); err != nil { + return fmt.Errorf("set cover for %q: %w", site+":"+seriesID, err) + } + return nil +} + // List returns every bookmark of one reader, newest activity first. // Series-owned fields are joined in, so each Bookmark reads back whole and // flat (ADR-0004). @@ -686,7 +780,7 @@ func (s *Store) List(readerID int64) ([]Bookmark, error) { out := []Bookmark{} for rows.Next() { - b, err := scanBookmark(rows.Scan) + b, err := s.scanBookmark(rows.Scan) if err != nil { return nil, fmt.Errorf("scan bookmark: %w", err) } @@ -703,7 +797,7 @@ func (s *Store) Get(readerID int64, key string) (Bookmark, bool, error) { if !ok { return Bookmark{}, false, nil } - b, err := scanBookmark(s.db.QueryRow( + b, err := s.scanBookmark(s.db.QueryRow( `SELECT `+bookmarkColumns+` FROM bookmarks b JOIN series s ON s.site = b.site AND s.series_id = b.series_id WHERE b.reader_id = $1 AND b.site = $2 AND b.series_id = $3`, @@ -760,18 +854,28 @@ func (s *Store) Upsert(readerID int64, b Bookmark) (Bookmark, error) { // The ::text casts are load-bearing: inside COALESCE/NULLIF there is no // target column to infer the parameter type from, and Postgres rejects the // statement rather than guessing. - if _, err := tx.Exec(` - INSERT INTO series (site, series_id, title, series_url, cover, kind, + // + // The cover columns are absent on purpose: the Cover is acquired + // server-side (ADR-0007), so a client-supplied one is not written even + // when the row is brand new. + // + // xmax is zero only on a row this statement inserted, which is how a + // Series nobody had bookmarked before is told apart from one that already + // existed — DO UPDATE returns a row either way. + var created bool + if err := tx.QueryRow(` + INSERT INTO series (site, series_id, title, series_url, kind, latest_chapter, latest_chapter_num) - VALUES ($1, $2, $3, $4, $5, - COALESCE(NULLIF($6::text, ''), (SELECT kind FROM series WHERE site = $1 AND series_id = $2), 'manga'), - $7, $8) + VALUES ($1, $2, $3, $4, + COALESCE(NULLIF($5::text, ''), (SELECT kind FROM series WHERE site = $1 AND series_id = $2), 'manga'), + $6, $7) ON CONFLICT (site, series_id) DO UPDATE SET kind=excluded.kind, latest_chapter=excluded.latest_chapter, - latest_chapter_num=excluded.latest_chapter_num`, - b.Site, b.SeriesID, b.Title, b.SeriesURL, b.Cover, b.Kind, - b.LatestChapter, latestNum); err != nil { + latest_chapter_num=excluded.latest_chapter_num + RETURNING xmax = 0`, + b.Site, b.SeriesID, b.Title, b.SeriesURL, b.Kind, + b.LatestChapter, latestNum).Scan(&created); err != nil { return Bookmark{}, fmt.Errorf("upsert series for %q: %w", b.Key, err) } @@ -801,7 +905,7 @@ func (s *Store) Upsert(readerID int64, b Bookmark) (Bookmark, error) { return Bookmark{}, fmt.Errorf("upsert %q: %w", b.Key, err) } - stored, err := scanBookmark(tx.QueryRow( + stored, err := s.scanBookmark(tx.QueryRow( `SELECT `+bookmarkColumns+` FROM bookmarks b JOIN series s ON s.site = b.site AND s.series_id = b.series_id WHERE b.reader_id = $1 AND b.site = $2 AND b.series_id = $3`, @@ -812,6 +916,14 @@ func (s *Store) Upsert(readerID int64, b Bookmark) (Bookmark, error) { if err := tx.Commit(); err != nil { return Bookmark{}, fmt.Errorf("commit %q: %w", b.Key, err) } + // After commit, never inside the transaction: the hook reaches a + // third-party Site, and the Reader's write must not wait on it. + if created && s.OnSeriesCreated != nil { + s.OnSeriesCreated(Series{ + Site: b.Site, SeriesID: b.SeriesID, Title: stored.Title, + SeriesURL: stored.SeriesURL, Kind: stored.Kind, + }) + } return stored, nil } diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index 01b7645..1dde1a4 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -21,9 +21,12 @@ func TestMain(m *testing.M) { os.Exit(pgtest.Main(m)) } // reader register one (see secondReader). var testOwner = Owner{DiscordID: "test-owner", TokenHash: sha256.Sum256([]byte("owner-token-hash"))} +// testCoverBaseURL is the public origin every stored cover URL is built from. +const testCoverBaseURL = "https://bookmarks.test" + func newTestStore(t *testing.T) *Store { t.Helper() - store, err := Open(pgtest.URL(t), testOwner, t.TempDir()) + store, err := Open(pgtest.URL(t), testOwner, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -49,7 +52,7 @@ func secondReader(t *testing.T, s *Store) int64 { func TestOpenIsIdempotent(t *testing.T) { url := pgtest.URL(t) coverDir := t.TempDir() - first, err := Open(url, testOwner, coverDir) + first, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -60,7 +63,7 @@ func TestOpenIsIdempotent(t *testing.T) { } first.Close() - second, err := Open(url, testOwner, coverDir) + second, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("reopen: %v", err) } @@ -113,7 +116,7 @@ func TestReaderTokenInfo(t *testing.T) { func TestRotateTokenInvalidatesOldAndSurvivesRestart(t *testing.T) { url := pgtest.URL(t) coverDir := t.TempDir() - store, err := Open(url, testOwner, coverDir) + store, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -145,7 +148,7 @@ func TestRotateTokenInvalidatesOldAndSurvivesRestart(t *testing.T) { } store.Close() - reopened, err := Open(url, testOwner, coverDir) + reopened, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("reopen: %v", err) } @@ -547,32 +550,39 @@ func TestDisplayChapter(t *testing.T) { } } +// CoverURL reads the source address for the kagane branch and the wire value +// otherwise, so both are set the way scanBookmark sets them. func TestCoverURL(t *testing.T) { cases := []struct { - name string - cover string - want string + name string + coverSource string + cover string + want string }{ { "kagane routes through the proxy", "https://kagane.to/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed", + "https://bookmarks.test/covers/" + CoverAddress("kagane"), "/img/kagane/019fe11a-84c3-7fc3-a84b-88787374b617", }, { - "another site is served as stored", - "https://gg.asuracomic.net/storage/media/1/conversions/cover.webp", + "another site is served from our own origin", "https://gg.asuracomic.net/storage/media/1/conversions/cover.webp", + "https://bookmarks.test/covers/" + CoverAddress("asura"), + "https://bookmarks.test/covers/" + CoverAddress("asura"), }, { "a lookalike host is not rewritten", "https://evil.example/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed", - "https://evil.example/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed", + "https://bookmarks.test/covers/" + CoverAddress("evil"), + "https://bookmarks.test/covers/" + CoverAddress("evil"), }, - {"no cover stays empty", "", ""}, + {"no cover stays empty", "", "", ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := (Bookmark{Cover: tc.cover}).CoverURL(); got != tc.want { + b := Bookmark{CoverSource: tc.coverSource, Cover: tc.cover} + if got := b.CoverURL(); got != tc.want { t.Errorf("CoverURL() = %q, want %q", got, tc.want) } }) @@ -672,7 +682,7 @@ func TestMigration0002BackfillsExistingBookmarks(t *testing.T) { // Bring it current through the production path: Open runs the schema to // 0003, seeds the owner, then applies 0004 which attaches this row. 0002 // must have backfilled the series row, not lost data. - st, err := Open(url, testOwner, t.TempDir()) + st, err := Open(url, testOwner, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("Open after migrate: %v", err) } @@ -756,39 +766,143 @@ func readSeries(t *testing.T, s *Store, site, seriesID string) Series { return sr } -// The first PUT for a series creates its row from the client's title, cover -// and URL — there is no other source for them (ADR-0003). +// The first PUT for a series creates its row from the client's title and URL — +// there is no other source for them (ADR-0003). The Cover is not among them: +// it is acquired server-side, so a client-supplied one is dropped even on a +// brand-new row (ADR-0007). func TestUpsertCreatesSeriesFromClient(t *testing.T) { store := newTestStore(t) - if _, err := store.Upsert(store.OwnerID(), Bookmark{ + stored, err := store.Upsert(store.OwnerID(), Bookmark{ Key: "asura:solo", Site: "asura", SeriesID: "solo", Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo", Cover: "https://asurascans.com/covers/solo.jpg", Kind: KindManga, UpdatedAt: 1000, - }); err != nil { + }) + if err != nil { t.Fatalf("Upsert: %v", err) } + if stored.Cover != "" { + t.Fatalf("Cover = %q, want empty — a client cover is never stored", stored.Cover) + } sr := readSeries(t, store, "asura", "solo") - if sr.Title != "Solo Leveling" || sr.SeriesURL != "https://asurascans.com/comics/solo" || - sr.Cover != "https://asurascans.com/covers/solo.jpg" { - t.Fatalf("series = %+v, want client title/url/cover stored", sr) + if sr.Title != "Solo Leveling" || sr.SeriesURL != "https://asurascans.com/comics/solo" { + t.Fatalf("series = %+v, want client title/url stored", sr) + } + if sr.Cover != "" { + t.Fatalf("series cover = %q, want empty", sr.Cover) } } -// A PUT naming an existing series must not overwrite its title, cover or URL: -// the row is shared, and those values are scraped page content (ADR-0003). +// The hook is what starts creation-time acquisition, so it must fire exactly +// once per Series — on the PUT that created it, and on no later one, whichever +// Reader sends it. +func TestOnSeriesCreatedFiresOnceForANewSeries(t *testing.T) { + store := newTestStore(t) + var created []Series + store.OnSeriesCreated = func(sr Series) { created = append(created, sr) } + + b := Bookmark{ + Key: "comix:solo", Site: "comix", SeriesID: "solo", Title: "Solo Leveling", + SeriesURL: "https://comix.to/series/solo", Kind: KindManga, UpdatedAt: 1000, + } + if _, err := store.Upsert(store.OwnerID(), b); err != nil { + t.Fatalf("Upsert: %v", err) + } + b.LastChapterNum = 12 + b.UpdatedAt = 2000 + if _, err := store.Upsert(store.OwnerID(), b); err != nil { + t.Fatalf("second Upsert: %v", err) + } + if _, err := store.Upsert(secondReader(t, store), b); err != nil { + t.Fatalf("second reader Upsert: %v", err) + } + + if len(created) != 1 { + t.Fatalf("hook fired %d times, want 1: %+v", len(created), created) + } + if created[0].Site != "comix" || created[0].SeriesID != "solo" || + created[0].SeriesURL != "https://comix.to/series/solo" { + t.Fatalf("hook got %+v, want the created series' identity and URL", created[0]) + } +} + +// Acquisition at creation and the poll both write covers, and whichever +// arrives second must leave the first one alone: a Cover is replaced by +// nothing short of the series row being rebuilt. +func TestSetSeriesCoverDoesNotOverwrite(t *testing.T) { + store := newTestStore(t) + if _, err := store.Upsert(store.OwnerID(), Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", UpdatedAt: 1000, + }); err != nil { + t.Fatalf("seed: %v", err) + } + first := "https://asurascans.com/covers/first.jpg" + if err := store.SetSeriesCover("asura", "solo", first, []byte("first"), "image/jpeg"); err != nil { + t.Fatalf("SetSeriesCover: %v", err) + } + if err := store.SetSeriesCover("asura", "solo", "https://asurascans.com/covers/second.jpg", + []byte("second"), "image/jpeg"); err != nil { + t.Fatalf("second SetSeriesCover: %v", err) + } + + got, ok, err := store.Get(store.OwnerID(), "asura:solo") + if err != nil || !ok { + t.Fatalf("Get = %v, %v", ok, err) + } + if want := "https://bookmarks.test/covers/" + CoverAddress(first); got.Cover != want { + t.Fatalf("Cover = %q, want the first one %q", got.Cover, want) + } +} + +// The address comes straight off a public request path, so anything that is +// not a stored address must be a miss rather than a filesystem lookup. +func TestCoverByAddress(t *testing.T) { + store := newTestStore(t) + if _, err := store.Upsert(store.OwnerID(), Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", UpdatedAt: 1000, + }); err != nil { + t.Fatalf("seed: %v", err) + } + source := "https://asurascans.com/covers/solo.jpg" + if err := store.SetSeriesCover("asura", "solo", source, []byte("bytes"), "image/jpeg"); err != nil { + t.Fatalf("SetSeriesCover: %v", err) + } + + body, contentType, ok, err := store.CoverByAddress(CoverAddress(source)) + if err != nil || !ok { + t.Fatalf("CoverByAddress = %v, %v", ok, err) + } + if string(body) != "bytes" || contentType != "image/jpeg" { + t.Fatalf("CoverByAddress = %q, %q, want the stored bytes", body, contentType) + } + + for _, address := range []string{"", "../../etc/passwd", "ZZ" + CoverAddress(source)[2:], + CoverAddress("never stored")} { + _, _, ok, err := store.CoverByAddress(address) + if err != nil || ok { + t.Fatalf("CoverByAddress(%q) = %v, %v, want a clean miss", address, ok, err) + } + } +} + +// A PUT naming an existing series must not overwrite its title or URL: the row +// is shared, and those values are scraped page content (ADR-0003). An acquired +// Cover is likewise untouched by any client. func TestUpsertExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) { store := newTestStore(t) base := Bookmark{ Key: "asura:solo", Site: "asura", SeriesID: "solo", Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo", - Cover: "https://asurascans.com/covers/solo.jpg", LastChapterNum: 10, - UpdatedAt: 1000, + LastChapterNum: 10, UpdatedAt: 1000, } if _, err := store.Upsert(store.OwnerID(), base); err != nil { t.Fatalf("seed: %v", err) } + acquired := "https://asurascans.com/covers/solo.jpg" + if err := store.SetSeriesCover("asura", "solo", acquired, []byte("bytes"), "image/jpeg"); err != nil { + t.Fatalf("SetSeriesCover: %v", err) + } // Same series, hostile/compromised values, real progress advance. base.Title = "Scraped Rename" @@ -799,8 +913,9 @@ func TestUpsertExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) { if err != nil { t.Fatalf("Upsert: %v", err) } + wantCover := "https://bookmarks.test/covers/" + CoverAddress(acquired) if got.Title != "Solo Leveling" || got.SeriesURL != "https://asurascans.com/comics/solo" || - got.Cover != "https://asurascans.com/covers/solo.jpg" { + got.Cover != wantCover { t.Fatalf("stored = %+v, want original title/url/cover kept", got) } if got.LastChapterNum != 11 { @@ -836,16 +951,19 @@ func TestUpsertExistingSeriesAcceptsKindAndLatest(t *testing.T) { } // Deleting the last bookmark must leave the series row behind, so a later -// re-bookmark shows title and cover immediately instead of waiting for a poll. +// re-bookmark shows title and cover immediately instead of re-acquiring them. func TestDeleteKeepsSeriesRow(t *testing.T) { store := newTestStore(t) if _, err := store.Upsert(store.OwnerID(), Bookmark{ Key: "asura:solo", Site: "asura", SeriesID: "solo", - Title: "Solo Leveling", Cover: "https://asurascans.com/covers/solo.jpg", - UpdatedAt: 1000, + Title: "Solo Leveling", UpdatedAt: 1000, }); err != nil { t.Fatalf("seed: %v", err) } + acquired := "https://asurascans.com/covers/solo.jpg" + if err := store.SetSeriesCover("asura", "solo", acquired, []byte("bytes"), "image/jpeg"); err != nil { + t.Fatalf("SetSeriesCover: %v", err) + } if err := store.Delete(store.OwnerID(), "asura:solo"); err != nil { t.Fatalf("Delete: %v", err) } @@ -863,7 +981,8 @@ func TestDeleteKeepsSeriesRow(t *testing.T) { if err != nil { t.Fatalf("re-upsert: %v", err) } - if stored.Title != "Solo Leveling" || stored.Cover != "https://asurascans.com/covers/solo.jpg" { + wantCover := "https://bookmarks.test/covers/" + CoverAddress(acquired) + if stored.Title != "Solo Leveling" || stored.Cover != wantCover { t.Fatalf("re-bookmark = %+v, want title/cover from the surviving series row", stored) } } @@ -940,14 +1059,14 @@ func TestDueForLatestCheckExcludesOrphanSeries(t *testing.T) { func TestSeedOwnerIdempotentAndRefreshesTokenHash(t *testing.T) { url := pgtest.URL(t) coverDir := t.TempDir() - first, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v1"))}, coverDir) + first, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v1"))}, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } ownerID := first.OwnerID() first.Close() - second, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v2"))}, coverDir) + second, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v2"))}, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("reopen: %v", err) } @@ -1004,7 +1123,7 @@ func TestMigration0004AttachesBookmarksToOwner(t *testing.T) { t.Fatalf("migrate to 0002: %v", err) } - st, err := Open(url, testOwner, t.TempDir()) + st, err := Open(url, testOwner, t.TempDir(), testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -1281,7 +1400,7 @@ func TestTwoReadersShareOneSeriesWithIndependentProgress(t *testing.T) { func TestKaganeCoverPersistsAcrossReopen(t *testing.T) { url := pgtest.URL(t) coverDir := t.TempDir() - first, err := Open(url, testOwner, coverDir) + first, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } @@ -1293,7 +1412,7 @@ func TestKaganeCoverPersistsAcrossReopen(t *testing.T) { t.Fatalf("close first store: %v", err) } - second, err := Open(url, testOwner, coverDir) + second, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("reopen: %v", err) } @@ -1308,15 +1427,26 @@ func TestKaganeCoverPersistsAcrossReopen(t *testing.T) { } func TestOpenRequiresCoverDirectory(t *testing.T) { - if _, err := Open(pgtest.URL(t), testOwner, ""); err == nil || !strings.Contains(err.Error(), "cover directory is required") { + if _, err := Open(pgtest.URL(t), testOwner, "", testCoverBaseURL); err == nil || !strings.Contains(err.Error(), "cover directory is required") { t.Fatalf("Open without cover directory = %v, want required-directory error", err) } } +// A base URL without a scheme reads like a hostname and starts cleanly, but +// every Cover it puts on the wire is an address no browser can resolve. +func TestOpenRequiresAbsoluteCoverBaseURL(t *testing.T) { + for _, base := range []string{"", "bookmarks.test", "https://", "ftp://bookmarks.test"} { + if _, err := Open(pgtest.URL(t), testOwner, t.TempDir(), base); err == nil || + !strings.Contains(err.Error(), "absolute http(s) origin") { + t.Fatalf("Open with base %q = %v, want absolute-origin error", base, err) + } + } +} + func TestKaganeCoverIsContentAddressedOnFilesystem(t *testing.T) { url := pgtest.URL(t) coverDir := t.TempDir() - first, err := Open(url, testOwner, coverDir) + first, err := Open(url, testOwner, coverDir, testCoverBaseURL) if err != nil { t.Fatalf("Open: %v", err) } diff --git a/backend/internal/web/cover.go b/backend/internal/web/cover.go index cbd1261..03e0c04 100644 --- a/backend/internal/web/cover.go +++ b/backend/internal/web/cover.go @@ -65,11 +65,13 @@ func (h *Handler) kaganeCover(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - if !store.IsCoverContentType(contentType) { + canonical, ok := store.CoverContentType(contentType) + if !ok { log.Printf("kagane cover %s: unexpected content type %q", id, contentType) http.NotFound(w, r) return } + contentType = canonical if err := h.store.PutKaganeCover(id, body, contentType); err != nil { log.Printf("persist kagane cover %s: %v", id, err) http.Error(w, "internal error", http.StatusInternalServerError) diff --git a/backend/main.go b/backend/main.go index ff61752..74a2df9 100644 --- a/backend/main.go +++ b/backend/main.go @@ -34,7 +34,13 @@ type Config struct { // serving a stored address without durable bytes would be worse than a // startup failure. CoverDir string - Port string + // PublicBaseURL is the origin this deployment answers on, e.g. + // "https://bookmarks.example.com". Required: cover URLs go out absolute + // because the userscript renders them on third-party origins, where a + // relative path would resolve against the Site (ADR-0007), and there is + // no way to guess it from a request the poller never sees. + PublicBaseURL string + Port string // OwnerDiscordID identifies the seeded owner Reader (issue #22). Required: // bookmarks are scoped to a Reader, and a fresh deployment needs one // before anybody logs in. The owner is also the only Reader who can revoke @@ -172,6 +178,7 @@ func loadConfig() Config { TokenKey: os.Getenv("TOKEN_KEY"), DatabaseURL: os.Getenv("DATABASE_URL"), CoverDir: os.Getenv("COVER_DIR"), + PublicBaseURL: os.Getenv("PUBLIC_BASE_URL"), Port: envOr("PORT", "8080"), OwnerDiscordID: os.Getenv("OWNER_DISCORD_ID"), UserscriptPath: envOr("USERSCRIPT_PATH", "/userscript/manga-bookmark.user.js"), @@ -199,7 +206,12 @@ func loadConfig() Config { // /healthz is public. func newRouter(s *store.Store, cfg Config) http.Handler { mux := http.NewServeMux() + h := &api.Handler{Store: s} mux.HandleFunc("GET /healthz", api.Healthz) + // Public: cover bytes are rendered by the userscript on origins that may + // not send our credentials, and the address is the hash of a URL the Site + // already publishes (ADR-0007). + mux.HandleFunc("GET /covers/{address}", h.Cover) // Outside httpmw.Auth (the updater sends no Authorization header) and // outside the web UI's Discord auth (the script must be installable @@ -211,7 +223,6 @@ func newRouter(s *store.Store, cfg Config) http.Handler { mux.HandleFunc("GET /u/{token}/novel-bookmark.user.js", userscript.Handler(s, cfg.NovelUserscriptPath)) - h := &api.Handler{Store: s} protected := http.NewServeMux() protected.HandleFunc("GET /bookmarks", h.List) protected.HandleFunc("PUT /bookmarks/{key}", h.Put) @@ -262,6 +273,9 @@ func main() { if cfg.CoverDir == "" { log.Fatal("COVER_DIR is required") } + if cfg.PublicBaseURL == "" { + log.Fatal("PUBLIC_BASE_URL is required") + } // The web UI signs in through Discord, so a deployment without the OAuth // application is misconfigured rather than passwordless. for key, v := range map[string]string{ @@ -282,7 +296,7 @@ func main() { TokenHash: token.Hash(token.Token([]byte(cfg.TokenKey), cfg.OwnerDiscordID, 0)), } - s, err := store.Open(cfg.DatabaseURL, owner, cfg.CoverDir) + s, err := store.Open(cfg.DatabaseURL, owner, cfg.CoverDir, cfg.PublicBaseURL) if err != nil { log.Fatalf("open store: %v", err) } @@ -310,6 +324,16 @@ func main() { log.Printf("browser fetcher at %s", ws) } } + // A Series nobody had bookmarked before gets its Latest Chapter and its + // Cover from one fetch, at creation, instead of waiting out a poll queue + // ordered by Reader count. Off the write path: the hook returns as soon + // as the goroutine is started. + if f, err := latest.NewTLSFetcher(); err != nil { + log.Printf("creation-time acquisition disabled, cannot build client: %v", err) + } else { + acq := &latest.Acquirer{Store: s, Fetch: f, Covers: latest.NewCoverFetcher(), Ctx: pollCtx} + s.OnSeriesCreated = acq.Acquire + } startLatestPoller(pollCtx, s, cfg.LatestPoll, browser) srv := &http.Server{ diff --git a/docker-compose.yml b/docker-compose.yml index 36cebc4..a6f8ea2 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -32,6 +32,10 @@ services: # Required path inside the API. The build seeds ownership at this path # and the named volume below mounts there. COVER_DIR: ${COVER_DIR:?set COVER_DIR in .env} + # Public origin of this deployment, no trailing slash. Required: the + # Cover URLs on the wire are absolute, since the userscript renders them + # on a Site's origin rather than ours (ADR-0007). + PUBLIC_BASE_URL: ${PUBLIC_BASE_URL:?set PUBLIC_BASE_URL in .env} PORT: "8080" # Log timestamps only. Go's `log` stamps lines in local time, and this # service has no other use for a zone: bookmark timestamps are unix ms