From 4aaf1d4f91280379185c75b1a3b7aba5c1822cec Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 22 Aug 2026 12:10:40 +0700 Subject: [PATCH] =?UTF-8?q?Spec=20#135:=20owner=20data-correction=20action?= =?UTF-8?q?s=20=E2=80=94=20Latest=20Chapter,=20series=5Furl,=20Cover,=20or?= =?UTF-8?q?phan=20removal=20(#156)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements spec #135 (spec 2 of 4, derived from wayfinder map #114; decisions settled in #120/#121/#125/#131). Blocked-by #134 is merged, so this lands on `main`. Four owner actions the dashboard can now perform, one ticket each: - **#149** — Latest Chapter correction: one numeric input, overwritten by the next machine write. - **#151** — Series URL repair: owner-typed, gated by the poller's own fetch gate. - **#150 / #153 / #154** — Cover replacement: addresses derived from bytes (`#150`), a Forced Poll replaces the Cover while an ordinary pass still only fills a blank one (`#153`), and byte reclamation is one guarded helper, file first / covers row last (`#154`). - **#155** — Orphan removal: one Series at a time, with the foreign key as the guard. Plus **#152** — Latest Chapter provenance: one derived line naming the actor class, so an owner can tell a hand-edited number from a machine read. - Migration `0015_latest_correction.sql` adds the correction/provenance columns; `0009` now derives cover addresses from bytes. - ADR `0014-cover-addresses-from-bytes.md` records the address scheme. Backend tests cover the store, poller, admin handlers, and web routes (`go test ./...`, needs Docker). Reviewed-on: https://gitea.violetcrown.my.id/sulthan/mangaBookmark/pulls/156 Co-authored-by: Sulthan Zaki Co-committed-by: Sulthan Zaki --- AGENTS.md | 2 +- backend/cover_test.go | 8 +- backend/internal/latest/acquire_test.go | 14 +- backend/internal/latest/browser.go | 4 +- backend/internal/latest/cover.go | 2 +- backend/internal/latest/poller.go | 65 +- backend/internal/latest/poller_test.go | 394 +++++++++++- backend/internal/latest/read.go | 2 +- backend/internal/latest/sites.go | 2 +- backend/internal/latest/smoke_image_test.go | 20 +- backend/internal/latest/smoke_lnw_test.go | 2 +- backend/internal/store/admin.go | 19 +- .../migrations/0009_series_cover_address.sql | 13 +- .../migrations/0015_latest_correction.sql | 4 + backend/internal/store/store.go | 241 ++++++-- backend/internal/store/store_test.go | 574 +++++++++++++++++- backend/internal/web/admin.go | 3 + backend/internal/web/admin_series.go | 276 ++++++++- backend/internal/web/admin_series_detail.go | 49 +- .../internal/web/admin_series_detail_test.go | 34 ++ backend/internal/web/static/admin.css | 28 + .../internal/web/templates/series-detail.html | 43 +- .../internal/web/templates/series-list.html | 16 +- backend/web_test.go | 556 ++++++++++++++++- docs/adr/0014-cover-addresses-from-bytes.md | 86 +++ 25 files changed, 2327 insertions(+), 130 deletions(-) create mode 100644 backend/internal/store/migrations/0015_latest_correction.sql create mode 100644 backend/internal/web/admin_series_detail_test.go create mode 100644 docs/adr/0014-cover-addresses-from-bytes.md diff --git a/AGENTS.md b/AGENTS.md index 11b7750..7ac1467 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -99,7 +99,7 @@ Go backend: - SQL always parameterized (`$N`). Only compile-time constants (`bookmarkColumns`) may be concatenated into query text — never a request value, not even a validated one. - `html/template` only for anything a browser parses, never `text/template`. Never wrap stored or fetched strings in `template.HTML`/`JS`/`URL`; that switches off the escaping every template depends on. -- Any outbound fetch of a client-supplied URL passes `fetchableSeriesURL` (site + `https` + host check) first. `series_url` arrives in a PUT body, so without the gate the poller will probe arbitrary hosts from the server's own network position. New fetch path reuses the gate rather than re-deriving one. +- Any outbound fetch of a client-supplied URL passes `FetchableSeriesURL` (site + `https` + host check) first. `series_url` arrives in a PUT body, so without the gate the poller will probe arbitrary hosts from the server's own network position. New fetch path reuses the gate rather than re-deriving one. - Cap every remote body with `io.LimitReader` (`maxBodyBytes`). An unbounded read is an OOM handed to whatever is on the other end. - Compare secrets with `hmac.Equal` / `subtle.ConstantTimeCompare`, never `==`. A credential is matched by the SHA-256 the `readers` table holds, which is already a fixed-width equality — a new secret comparison must not regress to `==`. - Errors: generic text to the client (`http.Error(w, "internal error", 500)`), detail to `log.Printf`. Never log `TOKEN_KEY`, a Reader's credential, `DISCORD_CLIENT_SECRET`, a session id, or a whole `Authorization` header. diff --git a/backend/cover_test.go b/backend/cover_test.go index 9c3c93f..87db88a 100644 --- a/backend/cover_test.go +++ b/backend/cover_test.go @@ -32,7 +32,7 @@ func TestPublicCoverServesStoredBytesUnauthenticated(t *testing.T) { // 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)) + wire := st.CoverWireURL(store.CoverAddressForBytes([]byte("\x00webp-bytes"))) path, ok := strings.CutPrefix(wire, testCoverBaseURL) if !ok { t.Fatalf("wire URL %q is not on the public origin %q", wire, testCoverBaseURL) @@ -57,7 +57,7 @@ func TestPublicCoverServesStoredBytesUnauthenticated(t *testing.T) { func TestPublicCoverRejectsUnknownAddress(t *testing.T) { srv, _ := newWebTestServer(t, testConfig()) cases := map[string]string{ - "unknown": "/covers/" + store.CoverAddress("https://cdn.example/never-stored.jpg"), + "unknown": "/covers/" + store.CoverAddressForBytes([]byte("never-stored")), "malformed": "/covers/not-an-address", "traversal": "/covers/../../etc/passwd", "empty": "/covers/", @@ -86,7 +86,7 @@ func TestPublicCoverNeverEchoesNonImage(t *testing.T) { } // A legitimate row, then the content type flipped behind the store's back: // the bytes exist at the address, so only the type is hostile. - address := store.CoverAddress(sourceURL) + address := store.CoverAddressForBytes([]byte("`, bookmarks: 1, + }) + router := newRouter(st, testConfig()) + + body := seriesDetailPage(t, router, st, "asura:solo") + for _, want := range []string{ + `name="series_url"`, + `hx-post="/admin/series/asura:solo/series-url"`, + `value="https://asurascans.com/stories/solo"`, + "A Site-wide host change", "SQL migration", + } { + if !strings.Contains(body, want) { + t.Errorf("detail page lacks %q:\n%s", want, body) + } + } + + // The stored value that is markup stays markup in the input's value + // attribute, never executable HTML. + body = seriesDetailPage(t, router, st, "asura:evil") + if strings.Contains(body, ``) { + t.Errorf("repair input renders stored URL unescaped:\n%s", body) + } + if !strings.Contains(body, `value="https://asurascans.com/x"><script>alert(1)</script>"`) { + t.Errorf("repair input does not carry the escaped stored URL:\n%s", body) + } +} + +// The Remove control is offered only to the owner, and only on a Series no +// Reader holds: on the orphan's list row and on the orphan's detail page, +// nowhere else (#155). +func TestSeriesRemoveRendersOnlyOnOrphans(t *testing.T) { + st, dsn := newTestStoreURL(t) + db, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open %s: %v", dsn, err) + } + defer db.Close() + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:ok", url: "u", checkedAt: 9000, bookmarks: 1}) + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:orphan", url: "u", checkedAt: 9000, bookmarks: 0}) + router := newRouter(st, testConfig()) + + body := adminSeriesPage(t, router, st, "") + if got := strings.Count(body, ">Remove<"); got != 1 { + t.Errorf("list offers Remove %d times, want 1 (only the orphan):\n%s", got, body) + } + for _, tc := range []struct { + key string + want bool + }{ + {"asura:ok", false}, + {"asura:orphan", true}, + } { + body := seriesDetailPage(t, router, st, tc.key) + if got := strings.Contains(body, ">Remove<"); got != tc.want { + t.Errorf("%s detail offers Remove = %v, want %v", tc.key, got, tc.want) + } + } +} + +// A removal from the list answers with the removed row's fragment and the +// heading re-rendered out of band with the fresh count: the row and the +// count are one fact. The row's press carries the list's filter state, so +// the count describes the list the owner is looking at, and the HX-Reswap +// header deletes the row through the same button that swaps the refusal in. +func TestRemoveFromListAnswersRowAndFreshHeading(t *testing.T) { + st, dsn := newTestStoreURL(t) + db, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open %s: %v", dsn, err) + } + defer db.Close() + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:a", url: "u", checkedAt: 9000, bookmarks: 0}) + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:b", url: "u", checkedAt: 9000, bookmarks: 0}) + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:held", url: "u", checkedAt: 9000, bookmarks: 1}) + router := newRouter(st, testConfig()) + + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:a/remove", + strings.NewReader("filter=no_readers&band=0")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("removal status = %d, want 200", rr.Code) + } + if got := rr.Header().Get("HX-Reswap"); got != "delete" { + t.Errorf("response does not ask htmx to delete the row (HX-Reswap = %q)", got) + } + body := rr.Body.String() + if !strings.Contains(body, "Title of asura:a") { + t.Errorf("answer does not carry the removed row's fragment:\n%s", body) + } + if !strings.Contains(body, `hx-swap-oob="true"`) || + !strings.Contains(body, "1 series") || !strings.Contains(body, "No Readers") { + t.Errorf("answer does not re-render the heading out of band with the fresh count:\n%s", body) + } + + // Gone from the store, gone from the list, and the heading lies no longer. + var one int + if err := db.QueryRow(`SELECT 1 FROM series WHERE site = 'asura' AND series_id = 'a'`).Scan(&one); err != sql.ErrNoRows { + t.Fatalf("series row after removal = %v, want sql.ErrNoRows", err) + } + body = adminSeriesPage(t, router, st, "?filter=no_readers") + if strings.Contains(body, "Title of asura:a") || !strings.Contains(body, "1 series") { + t.Errorf("list after removal is not the fresh view:\n%s", body) + } +} + +// A removal from the detail page navigates to the No-Readers list: htmx gets +// a full navigation (HX-Redirect — a 303 would be followed by the request +// and the list page swapped into the press's target), plain clients the 303 +// the ticket names, to the wire filter the orphan list actually is. +func TestRemoveFromDetailRedirectsToNoReadersList(t *testing.T) { + st, dsn := newTestStoreURL(t) + db, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open %s: %v", dsn, err) + } + defer db.Close() + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:gone", url: "u", checkedAt: 9000, bookmarks: 0}) + // A second orphan for the htmx dialect's request, whose row must still + // exist after the first request removed its own. + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:gone2", url: "u", checkedAt: 9000, bookmarks: 0}) + router := newRouter(st, testConfig()) + target := "/admin/series?filter=" + store.SeriesFilterNoReaders + + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:gone/remove", nil) + req.Header.Set("HX-Target", "detail-meta") + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusSeeOther { + t.Fatalf("detail removal status = %d, want 303", rr.Code) + } + if got := rr.Header().Get("Location"); got != target { + t.Errorf("Location = %q, want %q", got, target) + } + var one int + if err := db.QueryRow(`SELECT 1 FROM series WHERE site = 'asura' AND series_id = 'gone'`).Scan(&one); err != sql.ErrNoRows { + t.Fatalf("series row after detail removal = %v, want sql.ErrNoRows", err) + } + + req = httptest.NewRequest(http.MethodPost, "/admin/series/asura:gone2/remove", nil) + req.Header.Set("HX-Request", "true") + req.Header.Set("HX-Target", "detail-meta") + req.AddCookie(sessionCookie(t, st)) + rr = httptest.NewRecorder() + router.ServeHTTP(rr, req) + if got := rr.Header().Get("HX-Redirect"); got != target { + t.Errorf("HX-Redirect = %q, want %q", got, target) + } +} + +// A removal that races a fresh Bookmark is a refusal, not an error: the row +// is rendered again at its new count with the fact spelled out, never a 500, +// and it must not vanish from the list — the delete never happened. The +// detail-surface refusal navigates back to the detail page, where the same +// fresh count is visible. +func TestRemoveRacedBookmarkIsRefusedNotError(t *testing.T) { + st, dsn := newTestStoreURL(t) + db, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open %s: %v", dsn, err) + } + defer db.Close() + seedSeriesRow(t, st, db, seriesRowSeed{key: "asura:raced", url: "u", checkedAt: 9000, bookmarks: 1}) + router := newRouter(st, testConfig()) + + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:raced/remove", + strings.NewReader("filter=no_readers&band=0")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("refusal status = %d, want 200 (never a 500)", rr.Code) + } + if got := rr.Header().Get("HX-Reswap"); got != "" { + t.Errorf("refusal carries HX-Reswap = %q, want none (the row must stay)", got) + } + body := rr.Body.String() + if !strings.Contains(body, "a Reader has bookmarked this Series again") { + t.Errorf("refusal does not say what happened:\n%s", body) + } + if !strings.Contains(body, `class="c-rd">1`) { + t.Errorf("refusal does not render the fresh count:\n%s", body) + } + var one int + if err := db.QueryRow(`SELECT 1 FROM series WHERE site = 'asura' AND series_id = 'raced'`).Scan(&one); err != nil { + t.Fatalf("series row after refusal = %v, want present", err) + } + list := adminSeriesPage(t, router, st, "") + if !strings.Contains(list, "Title of asura:raced") { + t.Fatal("row vanished from the list after a refused removal") + } + if strings.Contains(list, ">Remove<") { + t.Errorf("a held series still offers Remove:\n%s", list) + } + + // The detail-surface refusal navigates back to the detail page. + req = httptest.NewRequest(http.MethodPost, "/admin/series/asura:raced/remove", nil) + req.Header.Set("HX-Target", "detail-meta") + req.AddCookie(sessionCookie(t, st)) + rr = httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusSeeOther { + t.Fatalf("detail refusal status = %d, want 303", rr.Code) + } + if got := rr.Header().Get("Location"); got != "/admin/series/asura:raced" { + t.Errorf("refusal Location = %q, want the detail page", got) + } +} + +// The remove route trusts the same way the poll route does: a malformed key +// is a 400 and an unknown Series a 404, and an oversized body is a 400 that +// removes nothing. +func TestRemoveRejectsBadKeysAndCapsBody(t *testing.T) { + router, st := newWebTestServer(t, testConfig()) + seed(t, st, store.Bookmark{ + Key: "asura:x", Site: "asura", SeriesID: "x", + Title: "Title of asura:x", SeriesURL: "u", + }) + for _, tc := range []struct { + path string + want int + }{ + {"/admin/series/nocolon/remove", http.StatusBadRequest}, + {"/admin/series/:x/remove", http.StatusBadRequest}, + {"/admin/series/asura:/remove", http.StatusBadRequest}, + {"/admin/series/asura:ghost/remove", http.StatusNotFound}, + } { + req := httptest.NewRequest(http.MethodPost, tc.path, nil) + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != tc.want { + t.Errorf("POST %s status = %d, want %d", tc.path, rr.Code, tc.want) + } + } + + big := strings.Repeat("a", 1<<17) // 128 KiB, over the 64 KiB cap + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:x/remove", strings.NewReader(big)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusBadRequest { + t.Fatalf("oversized body status = %d, want 400", rr.Code) + } + list := adminSeriesPage(t, router, st, "") + if !strings.Contains(list, "Title of asura:x") { + t.Errorf("an oversized body still removed the row:\n%s", list) + } +} diff --git a/docs/adr/0014-cover-addresses-from-bytes.md b/docs/adr/0014-cover-addresses-from-bytes.md new file mode 100644 index 0000000..eef6dde --- /dev/null +++ b/docs/adr/0014-cover-addresses-from-bytes.md @@ -0,0 +1,86 @@ +# ADR-0014: Cover addresses derived from the bytes, not the source URL + +Date: 2026-08-22 +Status: accepted + +## Decision + +A Cover's content address is the hex SHA-256 of its **bytes**, not of the +source URL it was fetched from. `CoverAddressForBytes(body)` names the address +`putCover` stores under, `SetSeriesCover` and `ReplaceSeriesCover` point the +Series row at it, and the wire URL is built from it exactly as before — same +route, same 64-hex-digit shape, same immutability, only the input to the hash +changes. Rows written before this ADR keep their URL-derived addresses +forever: they are never rehashed on read, and they heal into byte addressing +only when a Forced Poll replaces them. + +`ReplaceSeriesCover(site, seriesID, sourceURL, body, contentType)` +`(previous, current, error)` is the one write that may move a Cover once one +exists. It stores the bytes, then in one transaction locks the Series row, +reads the old `cover_address`, writes the new one and the source URL, and +reports both addresses: `previous == ""` means there was no Cover, +`previous == current` means the Site served identical artwork, and any other +pair names the stranded address. + +## Why a future reader will find this surprising + +The address is what makes a re-art visible at all. URL addressing collapses +every image behind a stable URL into one address, so a Series whose Cover +changes (a big-budget CPI blitz on a light novel is the standing example) +keeps serving its original cover bytes: the poll refetches the same URL, +hashes it, and the store records the same address, everyone happy except the +Reader. Nothing in the system can detect the change, because the address is a +pure function of the fetch target, and identical bytes written 1,000 times +are one blob on disk. Storing bytes we already know how to store is only a +few lines of work. **Rejecting that work is the surprising part, and the +answer is the Forced Poll wave**: for a corrupt/blank cover the poll's +fill-if-blank installer already worked, but for a *wrong but non-blank* cover +there was no write that would move it at all — only a manual truth in +`series.cover_address`, which is exactly the thing that must never be set by +hand. Byte addressing gives the replacement write a **new address to write**, +and with it a legitimate, transaction-safe mover. + +## Considered options + +**Keep URL addressing and add a generic "clear the cover" write.** +Rejected: clearing is a two-phase action (blank it, wait for the poll to +re-fill, hope the bytes changed in between) that cannot report what the +write did, and it makes the Series render cover-less in between. The +replacement write is atomic, reports its displacement, and has one effect: +the Series now points at bytes that actually came from its source URL. + +**Address by URL, but salt it so a re-art is a new address.** +Rejected: the salt would have to live somewhere addressable (a stored per- +Series nonce), turning the address from a content fact into a mutable fact — +two rows could then hold identical bytes under different addresses and the +invariant "same bytes object" is gone. + +## Consequences + +- `store.CoverAddress` (URL-hash) is deleted; `CoverAddressForBytes` is + public so tests and the forced-poll wave can predict addresses from the + bytes fakes serve. +- Legacy URL-addressed rows are read-only facts: `GetCover(sourceURL)` keeps + resolving them (the poll heal path), and they are re-addressed only by a + forced replacement. Until one happens, they are invisible to byte-derived + lookups — the reverse direction was always true, so this side has no + migration and no lookup fan-out. +- A replaced Cover's old bytes stay on disk under their address (the `covers` + row is untouched — only the Series row moves). Nothing reclaims them + today; a later sweep is a small query over `covers` addresses not + referenced by any `series` row. +- `SetSeriesCover` keeps its `cover_address = ''` guard untouched: the + acquisition-at-creation and poll fill paths still may not overwrite a + non-blank Cover. The two installers are now deliberately different + functions instead of one function with a conditional. +- The address is still a filesystem path (≤64 hex chars, no separators), so + `coverAddressRe` and the sharding stay exactly as they are. + +## Cost of reversing + +The URL-hash side of the current rows is uncomputable from the rows alone: a +rollback would need every stored blob's source URL, a join to a table that +does not store it, or a refetch of every Series. Keeping both derivations +resolvable is cheaper than either, so the two derivations are documented in +the 0009 migration comment: no component may assume which derivation a +stored address came from, because the 64-hex shape hides it. \ No newline at end of file