From bfae84c5c37120cec3044e9b6a53dab160a8135b Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sun, 9 Aug 2026 06:52:02 +0700 Subject: [PATCH] Persist kagane covers in Postgres (#49) Closes #43 Persist kagane cover bytes in a dedicated Postgres covers table keyed by image ID. The web handler reads storage before the browser, writes validated fetches through, and no longer keeps an in-process cover cache. Added migration, store persistence tests including reopen, handler coverage for stored/miss/rejected paths, and corrected repository guidance. Verification: - go test ./... - CGO_ENABLED=0 go build ./... Reviewed-on: https://gitea.violetcrown.my.id/sulthan/mangaBookmark/pulls/49 Co-authored-by: Sulthan Zaki Co-committed-by: Sulthan Zaki --- backend/AGENTS.md | 12 +-- backend/cover_test.go | 98 ++++++++++++++++--- .../internal/store/migrations/0007_covers.sql | 8 ++ backend/internal/store/store.go | 31 ++++++ backend/internal/store/store_test.go | 27 +++++ backend/internal/web/cover.go | 78 +++++---------- backend/internal/web/web.go | 3 +- 7 files changed, 180 insertions(+), 77 deletions(-) create mode 100644 backend/internal/store/migrations/0007_covers.sql diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 53115d3..06ecf7f 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -136,12 +136,12 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN behind the same challenge as its pages and with `cross-origin-resource-policy: same-origin`, so no `` on the web UI's origin can load one — not even from a browser holding the clearance cookie - (verified 2026-08-08). `Bookmark.CoverURL` rewrites a stored kagane - `og:image` to `/img/kagane/{id}`, served by `internal/web/cover.go` through - `latest.BrowserFetcher.Image` and memoised in-process. The templates render - `.CoverURL`, never `.Cover`. The id is matched against a UUID regex before it - reaches the browser: the stored value is client-supplied, so an unchecked one - is an SSRF primitive pointed at the deployment's own network. + `og:image` to `/img/kagane/{id}`. `internal/web/cover.go` reads the persistent + `covers` table first, then fetches a miss through `latest.BrowserFetcher.Image`. + The templates render `.CoverURL`, never `.Cover`. The id is matched against a + UUID regex before it reaches the browser: the stored value is client-supplied, + so an unchecked one is an SSRF primitive pointed at the deployment's own + network. - **Web UI also owns:** session-gated `GET /install/{manga,novel}-bookmark.user.js` (renders the bindmounted script with the acting Reader's derived credential substituted in — the credential never appears in page markup, the address diff --git a/backend/cover_test.go b/backend/cover_test.go index bf9b44f..86a658f 100644 --- a/backend/cover_test.go +++ b/backend/cover_test.go @@ -2,15 +2,19 @@ package main import ( "context" + "crypto/sha256" "errors" "net/http" "net/http/httptest" "sync/atomic" "testing" + + "bookmarkmanager/backend/internal/pgtest" + "bookmarkmanager/backend/internal/store" ) // fakeCovers stands in for the headless browser. It counts calls so the test -// can prove the cache spares the browser a second navigation. +// can prove the store spares the browser after the first navigation. type fakeCovers struct { body []byte contentType string @@ -44,33 +48,92 @@ func getCover(t *testing.T, srv http.Handler, path string, cookie *http.Cookie) // kagane serves its covers behind a Cloudflare challenge and with // cross-origin-resource-policy: same-origin, so the UI can only show one by // re-serving the bytes from its own origin. -func TestKaganeCoverProxiesAndCaches(t *testing.T) { +func TestKaganeCoverPersistsAndReusesStoredBytes(t *testing.T) { cf := &fakeCovers{body: []byte("\x00webp-bytes"), contentType: "image/webp"} cfg := testConfig() cfg.Covers = cf srv, st := newWebTestServer(t, cfg) cookie := sessionCookie(t, st) - for i := range 2 { - rr := getCover(t, srv, "/img/kagane/"+testCoverID, cookie) - if rr.Code != http.StatusOK { - t.Fatalf("request %d: status = %d, want 200", i, rr.Code) - } - if got := rr.Body.String(); got != string(cf.body) { - t.Fatalf("request %d: body = %q, want %q", i, got, cf.body) - } - if got := rr.Header().Get("Content-Type"); got != "image/webp" { - t.Fatalf("request %d: Content-Type = %q, want image/webp", i, got) - } + rr := getCover(t, srv, "/img/kagane/"+testCoverID, cookie) + if rr.Code != http.StatusOK { + t.Fatalf("first request: status = %d, want 200", rr.Code) + } + if got := rr.Body.String(); got != string(cf.body) { + t.Fatalf("first request: body = %q, want %q", got, cf.body) + } + + // A new Handler has no process-local state from the first request. The same + // store must still answer without navigating the browser again. + srv = newRouter(st, cfg) + rr = getCover(t, srv, "/img/kagane/"+testCoverID, cookie) + if rr.Code != http.StatusOK { + t.Fatalf("stored request: status = %d, want 200", rr.Code) + } + if got := rr.Body.String(); got != string(cf.body) { + t.Fatalf("stored request: body = %q, want %q", got, cf.body) } if got := cf.calls.Load(); got != 1 { - t.Fatalf("fetcher called %d times, want 1 — the second read must come from the cache", got) + t.Fatalf("fetcher called %d times, want 1 — stored bytes must survive a new handler", got) } if got := cf.lastID.Load(); got != testCoverID { t.Fatalf("fetched image id = %v, want %s", got, testCoverID) } } +func TestKaganeCoverServesStoredBytesWithoutBrowser(t *testing.T) { + cf := &fakeCovers{err: errors.New("browser must not be called")} + cfg := testConfig() + cfg.Covers = cf + srv, st := newWebTestServer(t, cfg) + if err := st.PutKaganeCover(testCoverID, []byte("already-stored"), "image/png"); err != nil { + t.Fatalf("PutKaganeCover: %v", err) + } + + rr := getCover(t, srv, "/img/kagane/"+testCoverID, sessionCookie(t, st)) + if rr.Code != http.StatusOK || rr.Body.String() != "already-stored" { + t.Fatalf("stored request = (%d, %q), want (200, already-stored)", rr.Code, rr.Body.String()) + } + if got := cf.calls.Load(); got != 0 { + t.Fatalf("fetcher called %d times for a stored cover, want 0", got) + } +} + +func TestKaganeCoverServesPersistedBytesAfterRestart(t *testing.T) { + url := pgtest.URL(t) + owner := store.Owner{ + DiscordID: "cover-owner", + TokenHash: sha256.Sum256([]byte("cover-owner-token")), + } + first, err := store.Open(url, owner) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := first.PutKaganeCover(testCoverID, []byte("survives-restart"), "image/jpeg"); err != nil { + first.Close() + t.Fatalf("PutKaganeCover: %v", err) + } + if err := first.Close(); err != nil { + t.Fatalf("close first store: %v", err) + } + + second, err := store.Open(url, owner) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer second.Close() + cf := &fakeCovers{err: errors.New("browser must not be called after restart")} + cfg := testConfig() + cfg.Covers = cf + rr := getCover(t, newRouter(second, cfg), "/img/kagane/"+testCoverID, sessionCookie(t, second)) + if rr.Code != http.StatusOK || rr.Body.String() != "survives-restart" { + t.Fatalf("restarted request = (%d, %q), want (200, survives-restart)", rr.Code, rr.Body.String()) + } + if got := cf.calls.Load(); got != 0 { + t.Fatalf("fetcher called %d times after restart, want 0", got) + } +} + // The proxy reaches a headless browser, so it is not open to the internet. func TestKaganeCoverRequiresSession(t *testing.T) { cf := &fakeCovers{body: []byte("x"), contentType: "image/webp"} @@ -123,6 +186,13 @@ func TestKaganeCoverRejectsBadInput(t *testing.T) { if rr.Code != http.StatusNotFound { t.Fatalf("status = %d, want 404", rr.Code) } + _, _, ok, err := st.GetKaganeCover(tc.id) + if err != nil { + t.Fatalf("GetKaganeCover after rejection: %v", err) + } + if ok { + t.Fatal("rejected cover was persisted") + } }) } } diff --git a/backend/internal/store/migrations/0007_covers.sql b/backend/internal/store/migrations/0007_covers.sql new file mode 100644 index 0000000..92a8063 --- /dev/null +++ b/backend/internal/store/migrations/0007_covers.sql @@ -0,0 +1,8 @@ +-- Kagane cover bytes belong in their own table so image blobs never enter the +-- series queries that drive the latest-chapter poller. +CREATE TABLE covers ( + image_id text PRIMARY KEY, + body bytea NOT NULL, + content_type text NOT NULL, + fetched_at timestamptz NOT NULL DEFAULT now() +); diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index d60e2ec..9c9e918 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -529,6 +529,37 @@ func scanSeries(scan func(...any) error) (Series, error) { // Close releases the underlying database handle. func (s *Store) Close() error { return s.db.Close() } +// GetKaganeCover returns one persisted cover. Missing covers are reported with +// ok=false rather than as an error so the web handler can fetch them once. +func (s *Store) GetKaganeCover(imageID string) ([]byte, string, bool, error) { + var ( + body []byte + contentType string + ) + err := s.db.QueryRow( + `SELECT body, content_type FROM covers WHERE image_id = $1`, imageID, + ).Scan(&body, &contentType) + if errors.Is(err, sql.ErrNoRows) { + return nil, "", false, nil + } + if err != nil { + return nil, "", false, fmt.Errorf("get kagane cover %q: %w", imageID, err) + } + return body, contentType, true, nil +} + +// PutKaganeCover persists one fetched cover. Image ids are immutable, so a +// later fetch cannot replace the bytes already made durable. +func (s *Store) PutKaganeCover(imageID string, body []byte, contentType string) error { + if _, err := s.db.Exec(` + INSERT INTO covers (image_id, body, content_type) + VALUES ($1, $2, $3) + ON CONFLICT (image_id) DO NOTHING`, imageID, body, contentType); err != nil { + return fmt.Errorf("put kagane cover %q: %w", imageID, 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). diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index 4aec053..e22a37e 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -1231,3 +1231,30 @@ func TestTwoReadersShareOneSeriesWithIndependentProgress(t *testing.T) { t.Fatalf("due after one Reader left = %+v, want the series still polled", due) } } +func TestKaganeCoverPersistsAcrossReopen(t *testing.T) { + url := pgtest.URL(t) + first, err := Open(url, testOwner) + if err != nil { + t.Fatalf("Open: %v", err) + } + body := []byte("stored-cover") + if err := first.PutKaganeCover("019fe11a-84c3-7fc3-a84b-88787374b617", body, "image/webp"); err != nil { + t.Fatalf("PutKaganeCover: %v", err) + } + if err := first.Close(); err != nil { + t.Fatalf("close first store: %v", err) + } + + second, err := Open(url, testOwner) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer second.Close() + got, contentType, ok, err := second.GetKaganeCover("019fe11a-84c3-7fc3-a84b-88787374b617") + if err != nil { + t.Fatalf("GetKaganeCover: %v", err) + } + if !ok || !bytes.Equal(got, body) || contentType != "image/webp" { + t.Fatalf("stored cover = (%q, %q, %v), want (%q, image/webp, true)", got, contentType, ok, body) + } +} diff --git a/backend/internal/web/cover.go b/backend/internal/web/cover.go index 18007cc..079ec3b 100644 --- a/backend/internal/web/cover.go +++ b/backend/internal/web/cover.go @@ -5,14 +5,13 @@ import ( "log" "net/http" "regexp" - "sync" "time" ) // CoverFetcher retrieves one kagane cover by image id. Satisfied by -// latest.BrowserFetcher, and nil when BROWSER_WS_URL is unset — which leaves -// kagane covers exactly as unavailable as they were before this endpoint -// existed, rather than hanging a request on a fetcher that cannot run. +// latest.BrowserFetcher. It is nil when BROWSER_WS_URL is unset; uncached +// covers are then unavailable, while covers already stored by the backend +// remain available without a browser. type CoverFetcher interface { Image(ctx context.Context, imageID string) (body []byte, contentType string, err error) } @@ -39,40 +38,6 @@ var coverTypes = map[string]bool{ // holding a connection open. const coverTimeout = 20 * time.Second -// coverCacheMax caps the in-memory cover cache. Covers are immutable per image -// id and a library holds tens of series, so this is a ceiling that is never -// reached in practice; reaching it clears the map rather than evicting by age. -// -// ponytail: flush-on-full, not LRU. Swap it for an LRU if a library ever grows -// past this and the flush starts costing refetches. -const coverCacheMax = 500 - -type cachedCover struct { - body []byte - contentType string -} - -type coverCache struct { - mu sync.Mutex - m map[string]cachedCover -} - -func (c *coverCache) get(id string) (cachedCover, bool) { - c.mu.Lock() - defer c.mu.Unlock() - v, ok := c.m[id] - return v, ok -} - -func (c *coverCache) put(id string, v cachedCover) { - c.mu.Lock() - defer c.mu.Unlock() - if c.m == nil || len(c.m) >= coverCacheMax { - c.m = make(map[string]cachedCover, coverCacheMax) - } - c.m[id] = v -} - // kaganeCover serves a kagane cover from the backend's own origin. // // kagane answers image requests with a Cloudflare challenge and @@ -81,29 +46,30 @@ func (c *coverCache) put(id string, v cachedCover) { // (verified 2026-08-08). Fetching it through the headless browser that already // clears the challenge, and re-serving it here, is what puts the bytes on an // origin the page may load from. -// -// ponytail: covers are fetched on first view, one browser navigation at a time -// behind the fetcher's mutex, so a first load of a large kagane library -// trickles in over a few seconds. The cache makes it a one-off. Prefetching -// during the poll cycle is the upgrade if that ever grates. func (h *Handler) kaganeCover(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if !coverIDRe.MatchString(id) { http.NotFound(w, r) return } - if h.covers == nil { - http.NotFound(w, r) + body, contentType, ok, err := h.store.GetKaganeCover(id) + if err != nil { + log.Printf("read kagane cover %s: %v", id, err) + http.Error(w, "internal error", http.StatusInternalServerError) return } - if v, ok := h.coverCache.get(id); ok { - writeCover(w, v) + if ok { + writeCover(w, body, contentType) + return + } + if h.covers == nil { + http.NotFound(w, r) return } ctx, cancel := context.WithTimeout(r.Context(), coverTimeout) defer cancel() - body, contentType, err := h.covers.Image(ctx, id) + body, contentType, err = h.covers.Image(ctx, id) if err != nil { log.Printf("kagane cover %s: %v", id, err) http.NotFound(w, r) @@ -114,16 +80,18 @@ func (h *Handler) kaganeCover(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - - v := cachedCover{body: body, contentType: contentType} - h.coverCache.put(id, v) - writeCover(w, v) + 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) + return + } + writeCover(w, body, contentType) } // writeCover sends the bytes with a long cache life: an image id names one // immutable rendering, so a client that has it never needs to ask again. -func writeCover(w http.ResponseWriter, v cachedCover) { - w.Header().Set("Content-Type", v.contentType) +func writeCover(w http.ResponseWriter, body []byte, contentType string) { + w.Header().Set("Content-Type", contentType) w.Header().Set("Cache-Control", "private, max-age=604800, immutable") - w.Write(v.body) + w.Write(body) } diff --git a/backend/internal/web/web.go b/backend/internal/web/web.go index 872cf6a..4dd0470 100644 --- a/backend/internal/web/web.go +++ b/backend/internal/web/web.go @@ -52,8 +52,7 @@ type Handler struct { httpClient *http.Client // covers proxies kagane cover images, which no browser can load directly. // Nil disables the endpoint — see CoverFetcher. - covers CoverFetcher - coverCache coverCache + covers CoverFetcher } // listView is what every list-rendering template receives.