From 424d2c6600d976e2deed85e8bf5d07a1a3331354 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 22 Aug 2026 09:21:00 +0700 Subject: [PATCH] Series URL repair: owner-typed, gated by the poller's own fetch gate (#151) --- AGENTS.md | 2 +- backend/internal/latest/browser.go | 4 +- backend/internal/latest/cover.go | 2 +- backend/internal/latest/poller.go | 8 +- backend/internal/latest/poller_test.go | 8 +- backend/internal/latest/read.go | 2 +- backend/internal/latest/sites.go | 2 +- backend/internal/latest/smoke_lnw_test.go | 2 +- backend/internal/store/store.go | 24 +++- backend/internal/store/store_test.go | 35 +++++ backend/internal/web/admin.go | 1 + backend/internal/web/admin_series.go | 54 ++++++++ backend/internal/web/admin_series_detail.go | 6 +- .../internal/web/templates/series-detail.html | 16 ++- backend/web_test.go | 121 ++++++++++++++++++ 15 files changed, 265 insertions(+), 22 deletions(-) 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/internal/latest/browser.go b/backend/internal/latest/browser.go index 88a7254..05050eb 100644 --- a/backend/internal/latest/browser.go +++ b/backend/internal/latest/browser.go @@ -124,7 +124,7 @@ func (f *BrowserFetcher) Get(ctx context.Context, seriesURL string) (string, int // request must be made from inside the page so it carries the clearance // cookie, and the API is the only place the list exists. Refusing any other // address is the per-Site half of the SSRF gate, kept deliberately behind -// fetchableSeriesURL (see browserRead.Read). +// FetchableSeriesURL (see browserRead.Read). func kaganeRead(seriesURL string, out *string) (chromedp.Action, bool) { apiURL, ok := kaganeAPIURL(seriesURL) if !ok { @@ -326,7 +326,7 @@ func (f *BrowserFetcher) run(ctx context.Context, target string, read chromedp.A // kaganeAPIURL maps a stored series_url to the JSON endpoint carrying its // chapter list. Returning false for anything else is a second line of defence -// behind fetchableSeriesURL: a headless browser is a strong SSRF primitive and +// behind FetchableSeriesURL: a headless browser is a strong SSRF primitive and // series_url is client-supplied, so the host is pinned here too. func kaganeAPIURL(seriesURL string) (string, bool) { u, err := url.Parse(seriesURL) diff --git a/backend/internal/latest/cover.go b/backend/internal/latest/cover.go index 84186c7..6e765b0 100644 --- a/backend/internal/latest/cover.go +++ b/backend/internal/latest/cover.go @@ -174,7 +174,7 @@ func (f *TLSCoverFetcher) Fetch(ctx context.Context, sourceURL string) ([]byte, return body, contentType, nil } -// This gate deliberately differs from fetchableSeriesURL: cover hosts are +// This gate deliberately differs from FetchableSeriesURL: cover hosts are // site-independent CDNs, so a Site host allowlist would reject valid covers. func (f *TLSCoverFetcher) validateURL(ctx context.Context, u *url.URL) error { if u == nil || u.Scheme != "https" || u.Host == "" || u.User != nil { diff --git a/backend/internal/latest/poller.go b/backend/internal/latest/poller.go index 035ff84..9e520c4 100644 --- a/backend/internal/latest/poller.go +++ b/backend/internal/latest/poller.go @@ -710,7 +710,7 @@ func (p *Poller) waitCovers() { p.coverWG.Wait() } -// fetchableSeriesURL reports whether site is a Site the registry knows and +// FetchableSeriesURL reports whether site is a Site the registry knows and // seriesURL is safe to hand to a fetcher: an https URL whose host matches the // Site's pinned hostname exactly. series_url comes from client-supplied PUT // bodies, so this is a defence against the poller being used to probe @@ -719,7 +719,11 @@ func (p *Poller) waitCovers() { // browser Site guards a control that executes JavaScript and carries cookies, // a parser Site guards a wasted request — but the rule is one rule, from the // registry. -func fetchableSeriesURL(site, seriesURL string) bool { +// +// The owner's series URL repair (issue #151) is a second caller: the web +// layer validates with this same gate before storing a repair, so there is +// never a second copy of it. +func FetchableSeriesURL(site, seriesURL string) bool { s, known := sites[site] if !known { return false diff --git a/backend/internal/latest/poller_test.go b/backend/internal/latest/poller_test.go index 3bfc797..3d3a9f0 100644 --- a/backend/internal/latest/poller_test.go +++ b/backend/internal/latest/poller_test.go @@ -588,8 +588,8 @@ func TestFetchableSeriesURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := fetchableSeriesURL(tt.site, tt.seriesURL); got != tt.want { - t.Errorf("fetchableSeriesURL(%q, %q) = %v, want %v", + if got := FetchableSeriesURL(tt.site, tt.seriesURL); got != tt.want { + t.Errorf("FetchableSeriesURL(%q, %q) = %v, want %v", tt.site, tt.seriesURL, got, tt.want) } }) @@ -1019,8 +1019,8 @@ func TestFetchableSeriesURLPinsNovelHosts(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := fetchableSeriesURL(tc.site, tc.url); got != tc.want { - t.Fatalf("fetchableSeriesURL(%q, %q) = %v, want %v", tc.site, tc.url, got, tc.want) + if got := FetchableSeriesURL(tc.site, tc.url); got != tc.want { + t.Fatalf("FetchableSeriesURL(%q, %q) = %v, want %v", tc.site, tc.url, got, tc.want) } }) } diff --git a/backend/internal/latest/read.go b/backend/internal/latest/read.go index dcf8d4b..134223d 100644 --- a/backend/internal/latest/read.go +++ b/backend/internal/latest/read.go @@ -41,7 +41,7 @@ var ( // its own network position to whatever URL a token-holder writes, including // link-local/internal addresses or non-https schemes. func readSeriesPage(ctx context.Context, site, seriesURL string, browser, tls Fetcher) (seriesRead, error) { - if !fetchableSeriesURL(site, seriesURL) { + if !FetchableSeriesURL(site, seriesURL) { return seriesRead{}, fmt.Errorf("%w: site=%q url=%q", errNotFetchable, site, seriesURL) } f := fetcherFor(site, browser, tls) diff --git a/backend/internal/latest/sites.go b/backend/internal/latest/sites.go index 3be1f34..52f3824 100644 --- a/backend/internal/latest/sites.go +++ b/backend/internal/latest/sites.go @@ -46,7 +46,7 @@ type site struct { type browserRead struct { // Read builds the tab read for seriesURL, refusing (false) an address // this Site will not open in a browser — the per-Site half of the SSRF - // gate, kept deliberately behind fetchableSeriesURL: a headless browser + // gate, kept deliberately behind FetchableSeriesURL: a headless browser // executes JavaScript and carries cookies, and series_url is // client-supplied. Read func(seriesURL string, out *string) (chromedp.Action, bool) diff --git a/backend/internal/latest/smoke_lnw_test.go b/backend/internal/latest/smoke_lnw_test.go index cd3e589..60896a5 100644 --- a/backend/internal/latest/smoke_lnw_test.go +++ b/backend/internal/latest/smoke_lnw_test.go @@ -40,7 +40,7 @@ func TestSmokeLnwCommentBoundary(t *testing.T) { if seriesURL == "" { t.Skip("SMOKE_LNW_SERIES_URL unset") } - if !fetchableSeriesURL("lightnovelworld", seriesURL) { + if !FetchableSeriesURL("lightnovelworld", seriesURL) { t.Fatalf("%q is not a fetchable lightnovelworld series URL", seriesURL) } diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index f4fd351..4321818 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -60,10 +60,11 @@ type Bookmark struct { // exists once no matter how many bookmarks point at it (ADR-0003). // // Title, SeriesURL and Cover are written once, at creation: a PUT naming an -// existing Series has them ignored, and only the backend's own Poll may change -// them. Kind and the latest-chapter fields are last-write-wins like the -// bookmark's own fields. Never serialized: the wire format is the flat -// Bookmark (ADR-0004). +// existing Series has them ignored, and only the backend's own Poll may +// change them. The one exception is SeriesURL, which the owner's +// SetSeriesURL may repair (issue #151). Kind and the latest-chapter fields +// are last-write-wins like the bookmark's own fields. Never serialized: the +// wire format is the flat Bookmark (ADR-0004). type Series struct { Site string SeriesID string @@ -1394,6 +1395,21 @@ func (s *Store) SetLatestChapter(site, seriesID, label string, num float64) erro return nil } +// SetSeriesURL stores the owner's repair for a Series' source address +// (issue #151): the one write that lifts the write-once rule documented on +// Series.SeriesURL. It is a store, not a verification — the caller has +// already passed the poller's fetch gate. The handler 404s on an unknown row +// before calling; the write itself is a plain single-column UPDATE like +// MarkLatestChecked. +func (s *Store) SetSeriesURL(site, seriesID, seriesURL string) error { + if _, err := s.db.Exec( + `UPDATE series SET series_url = $3 WHERE site = $1 AND series_id = $2`, + site, seriesID, seriesURL); err != nil { + return fmt.Errorf("set series url %s:%s: %w", site, seriesID, err) + } + return nil +} + // CorrectLatestChapter makes the Latest Chapter the owner's: one UPDATE // carrying the number, the derived label and the correction stamp. The label // shape is the poller's and the userscript's ("Chapter " + the number as diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index 2297003..a14be84 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -2144,3 +2144,38 @@ func TestGetCoverResolvesLegacyURLDerivedAddress(t *testing.T) { t.Fatalf("legacy cover = (%q, %q), want the planted bytes", body, contentType) } } + +// SetSeriesURL is the one write that lifts the write-once rule of +// Series.SeriesURL (issue #151): a client PUT naming an existing Series still +// has its new URL dropped, yet the owner's repair lands where the Upsert +// would have ignored it. +func TestSetSeriesURLWritesWhereUpsertIgnores(t *testing.T) { + store := newTestStore(t) + base := Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo", + UpdatedAt: 1000, + } + if _, err := store.Upsert(store.OwnerID(), base); err != nil { + t.Fatalf("seed: %v", err) + } + + // A client PUT naming the existing Series is refused: the row is shared, + // so the stored URL stands. + base.SeriesURL = "https://evil.example/solo" + if got, err := store.Upsert(store.OwnerID(), base); err != nil { + t.Fatalf("Upsert: %v", err) + } else if got.SeriesURL != "https://asurascans.com/comics/solo" { + t.Fatalf("Upsert stored %q, want the original URL untouched", got.SeriesURL) + } + + // The owner's repair writes where the Upsert would have ignored it. + repair := "https://asurascans.com/comics/solo-renumbered" + if err := store.SetSeriesURL("asura", "solo", repair); err != nil { + t.Fatalf("SetSeriesURL: %v", err) + } + sr := readSeries(t, store, "asura", "solo") + if sr.SeriesURL != repair { + t.Fatalf("stored URL = %q, want %q", sr.SeriesURL, repair) + } +} diff --git a/backend/internal/web/admin.go b/backend/internal/web/admin.go index bb22fe7..cad99c5 100644 --- a/backend/internal/web/admin.go +++ b/backend/internal/web/admin.go @@ -48,6 +48,7 @@ func (h *Handler) adminRoutes() []adminRoute { {"GET /admin/series/{key}", h.adminSeriesDetail}, {"POST /admin/series/{key}/poll", h.adminSeriesPoll}, {"POST /admin/series/{key}/latest", h.adminSeriesCorrectLatest}, + {"POST /admin/series/{key}/series-url", h.adminSeriesSetURL}, {"POST /admin/lanes/{site}/pause", h.adminLanePause}, {"POST /admin/lanes/{site}/resume", h.adminLaneResume}, {"GET /ui/admin/lanes", h.uiLanes}, diff --git a/backend/internal/web/admin_series.go b/backend/internal/web/admin_series.go index ef536aa..d94c255 100644 --- a/backend/internal/web/admin_series.go +++ b/backend/internal/web/admin_series.go @@ -225,6 +225,60 @@ func (h *Handler) adminSeriesCorrectLatest(w http.ResponseWriter, r *http.Reques h.render(w, http.StatusOK, "series-detail-meta", h.seriesDetailView(a)) } +// adminSeriesSetURL is the series URL repair: the owner types one address +// and the Series' Poll fetches it from then on, verified by the same gate +// the poller uses before it fetches anything — a URL failing +// latest.FetchableSeriesURL answers 400 and never reaches the store. The +// repair is a store, not a verification: it performs no outbound fetch, and +// the owner presses Check now afterwards. This lifts the write-once rule of +// Series.SeriesURL for the owner only — a Reader's PUT is still ignored. The +// owner gate is the route's, not this handler's; the body is capped like the +// API path caps its bodies; the key is validated here — a malformed key is a +// 400 and an unknown one a 404. +func (h *Handler) adminSeriesSetURL(w http.ResponseWriter, r *http.Request) { + site, seriesID, ok := strings.Cut(r.PathValue("key"), ":") + if !ok || site == "" || seriesID == "" { + http.Error(w, "bad series key", http.StatusBadRequest) + return + } + r.Body = http.MaxBytesReader(w, r.Body, 1<<16) + if err := r.ParseForm(); err != nil { + http.Error(w, "invalid form", http.StatusBadRequest) + return + } + seriesURL := r.PostFormValue("series_url") + if !latest.FetchableSeriesURL(site, seriesURL) { + http.Error(w, "series URL must be an https address on this site's host", http.StatusBadRequest) + return + } + if _, found, err := h.adminSeriesByKey(site, seriesID); err != nil { + log.Printf("series url %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } else if !found { + http.NotFound(w, r) + return + } + if err := h.store.SetSeriesURL(site, seriesID, seriesURL); err != nil { + log.Printf("series url %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + // Re-read after the write: the answer must describe the state after the + // press, like the correction's answer does. + a, found, err := h.adminSeriesByKey(site, seriesID) + if err != nil { + log.Printf("series url %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if !found { + http.NotFound(w, r) + return + } + h.render(w, http.StatusOK, "series-detail-meta", h.seriesDetailView(a)) +} + // seriesListView assembles one Series list view from the request's query // string. An unknown filter value is the absent All case, never an error: the // select's options are not the only way this URL can be reached. diff --git a/backend/internal/web/admin_series_detail.go b/backend/internal/web/admin_series_detail.go index d905906..8178aba 100644 --- a/backend/internal/web/admin_series_detail.go +++ b/backend/internal/web/admin_series_detail.go @@ -24,11 +24,14 @@ type seriesDetailView struct { Cover string Chapter string // Latest Chapter number, or "—" before the first capture Checked string // how long ago the poller last checked, or "never" + // URL is the stored source address, prefilled into the repair input — + // the one stored string this page renders back into a form (issue #151). + URL string + Readers int // Corrected is the correction marker's text, "" while no Correction // stands: "corrected ago" — the copy that says the value is the // owner's, and it dies with the stamp (a machine write of the number). Corrected string - Readers int // Marks, one per hygiene fact, rendered only while it holds. Unpollable bool // no SeriesURL to fetch @@ -106,6 +109,7 @@ func (h *Handler) seriesDetailView(a store.AdminSeries) seriesDetailView { Kind: a.Kind, Title: a.Title, Cover: h.store.CoverWireURL(a.CoverAddress), + URL: a.SeriesURL, Readers: a.ReaderCount, Unpollable: a.SeriesURL == "", NoCover: a.CoverAddress == "", diff --git a/backend/internal/web/templates/series-detail.html b/backend/internal/web/templates/series-detail.html index 50cc2fb..c5a800c 100644 --- a/backend/internal/web/templates/series-detail.html +++ b/backend/internal/web/templates/series-detail.html @@ -1,9 +1,9 @@ {{/* Per-Series page: one address per Series, keyed ":" so the list row is one hop from it. Everything here is a Series-level fact plus - the anonymous Reader count. The Check now control lands in its own .dform - below the .detail-grid; the correction form is the grid's first column, - and the grid's second waits for the series-URL repair (#151). The pending - and corrected markers ride the meta line with the other marks. */}} + the anonymous Reader count. Check now lands in its own .dform below the + .detail-grid; the correction form is the grid's first column and the URL + repair the second (issue #151). The pending and corrected markers ride the + meta line with the other marks. */}} {{define "series-detail"}} ← Series

{{.Title}}

@@ -20,6 +20,14 @@ +
+

Repair series URL

+

The Poll fetches this address — storing is not verifying it. A Site-wide host change is a SQL migration, not two hundred forms.

+
+ + +
+
{{if .CanPoll}}
diff --git a/backend/web_test.go b/backend/web_test.go index 608ab00..5c048fe 100644 --- a/backend/web_test.go +++ b/backend/web_test.go @@ -3371,3 +3371,124 @@ func TestAdminSeriesDetailCorrectionMarker(t *testing.T) { t.Errorf("detail page does not show the machine-written number:\n%s", body) } } + +// The series URL repair validates with the poller's own fetch gate and +// answers 400 before anything reaches the store; a URL that passes the gate +// is stored where an Upsert would have ignored it. The request performs no +// outbound fetch — no fetcher is ever constructed on this path (the web +// router has no fetcher seam at all, and the handler only calls the store), +// so "storing is not verifying" is enforced by construction (#151). +func TestSeriesURLRepairRoute(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:solo", url: "https://asurascans.com/comics/solo", checkedAt: 9000, bookmarks: 1, + }) + router := newRouter(st, testConfig()) + cookie := sessionCookie(t, st) + + // A URL the gate refuses — foreign host, http scheme, host of another + // Site — answers 400 and never reaches the store. + for _, body := range []string{ + "series_url=https://evil.example/solo", + "series_url=http://asurascans.com/stories/solo", + "series_url=https://kagane.to/series/solo", + "series_url=", + } { + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:solo/series-url", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(cookie) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusBadRequest { + t.Errorf("POST series-url with body %q: status = %d, want 400", body, rr.Code) + } + } + + capReq := httptest.NewRequest(http.MethodPost, "/admin/series/asura:solo/series-url", + strings.NewReader("series_url=https://asurascans.com/stories/"+strings.Repeat("a", 1<<17))) + capReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + capReq.AddCookie(cookie) + capRR := httptest.NewRecorder() + router.ServeHTTP(capRR, capReq) + if capRR.Code != http.StatusBadRequest { + t.Errorf("POST series-url with an oversized body: status = %d, want 400", capRR.Code) + } + var stored string + if err := db.QueryRow(`SELECT series_url FROM series WHERE site = 'asura' AND series_id = 'solo'`). + Scan(&stored); err != nil { + t.Fatalf("read back: %v", err) + } + if stored != "https://asurascans.com/comics/solo" { + t.Fatalf("after 400s the stored URL = %q, want the seeded one untouched", stored) + } + + // A URL that passes the gate lands, and the press answers with the meta + // fragment just like the other detail-page actions. + repair := "https://asurascans.com/stories/solo-renumbered" + req := httptest.NewRequest(http.MethodPost, "/admin/series/asura:solo/series-url", + strings.NewReader("series_url="+repair)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(cookie) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("POST series-url status = %d, want 200 (body %s)", rr.Code, rr.Body.String()) + } + if body := rr.Body.String(); !strings.Contains(body, `id="detail-meta"`) { + t.Errorf("repair answer is not the meta fragment:\n%s", body) + } + if err := db.QueryRow(`SELECT series_url FROM series WHERE site = 'asura' AND series_id = 'solo'`). + Scan(&stored); err != nil { + t.Fatalf("read back: %v", err) + } + if stored != repair { + t.Fatalf("stored URL = %q, want %q", stored, repair) + } +} + +// The detail page offers the repair input prefilled with the stored address, +// states the honest limit — a Site-wide host change is a SQL migration, not a +// per-Series form — and a stored string renders back into the input escaped +// (issue #151). +func TestAdminSeriesDetailRepairForm(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:solo", url: "https://asurascans.com/stories/solo", bookmarks: 1, + }) + seedSeriesRow(t, st, db, seriesRowSeed{ + key: "asura:evil", url: `https://asurascans.com/x">`, 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) + } +}