From 22bf68f12f7adb89a6a9d4a3e1b59ecbffeddcc1 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sun, 26 Jul 2026 03:18:00 +0700 Subject: [PATCH] fix(backend): bind session key to both secrets and guard no-op chapter saves Final-review fix wave over the web UI branch. - sessionKey now derives from API_TOKEN and WEB_PASSWORD with a \x00 separator, so rotating the password logs every browser out too. - uiChapter only clears last_chapter_url when the number actually changes. The form is pre-filled, so a bare tap of Save resubmits the same value; that used to destroy the chapter URL silently while updated_at stayed put, degrading Continue to the series index page. - MANGA_WEB_HOST is now required by the prod override rather than falling back to manga.example.com, matching MANGA_API_HOST. - Comment fixes: static cache rationale, pruneLocked aliasing invariant, and the stale "3 routes" line in CLAUDE.md. Co-Authored-By: Claude Opus 5 --- .env.example | 7 ++++--- CLAUDE.md | 2 +- DEPLOY.md | 9 +++++++-- backend/session.go | 16 +++++++++++----- backend/session_test.go | 20 ++++++++++++++------ backend/web.go | 22 ++++++++++++++-------- backend/web_test.go | 36 +++++++++++++++++++++++++++++++++++- docker-compose.prod.yml | 4 ++-- 8 files changed, 88 insertions(+), 28 deletions(-) diff --git a/.env.example b/.env.example index 41c9511..c4057b1 100644 --- a/.env.example +++ b/.env.example @@ -23,6 +23,7 @@ ALLOWED_ORIGINS=https://asuracomic.net,https://asurascans.com,https://demonicsca # Generate one: openssl rand -base64 18 WEB_PASSWORD= -# Subdomain Traefik routes to the browser UI (prod override only). The same -# container also answers on MANGA_API_HOST for the userscript's API. -# MANGA_WEB_HOST=manga.example.com +# Subdomain Traefik routes to the browser UI (required by the prod override +# whenever the web UI is enabled). The same container also answers on +# MANGA_API_HOST for the userscript's API. +MANGA_WEB_HOST=manga.example.com diff --git a/CLAUDE.md b/CLAUDE.md index 6f3a8e6..9405fd0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,7 +26,7 @@ Bromite userscript (isolated world, per-site adapters, localStorage cache) -- fetch() HTTPS --> reverse proxy (TLS + CORS) --> Go net/http --> SQLite (volume) ``` -- **Backend** (`backend/`): stdlib `net/http` (3 routes, no framework) + `modernc.org/sqlite` (pure Go, `CGO_ENABLED=0` -> static binary -> distroless/scratch image). The reverse proxy terminates TLS; the Go service listens plain `:8080`. +- **Backend** (`backend/`): stdlib `net/http` (a handful of routes, no framework) + `modernc.org/sqlite` (pure Go, `CGO_ENABLED=0` -> static binary -> distroless/scratch image). The reverse proxy terminates TLS; the Go service listens plain `:8080`. - **Single-user store.** One `bookmarks` table keyed `:` (`asura`|`demonic`). Sync is **last-write-wins**. Schema and endpoint list are in the plan. - **Endpoints:** `GET /bookmarks`, `PUT /bookmarks/{key}` (upsert; see `updated_at` rule below), `DELETE /bookmarks/{key}`, `GET /healthz` (no auth). - **Web UI:** the same binary serves a password-gated browser UI on a second diff --git a/DEPLOY.md b/DEPLOY.md index 850ca91..5f87122 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -93,8 +93,13 @@ The browser UI is served by the same container on a second hostname. Leaving `WEB_PASSWORD` unset is safe: the web routes are not registered and `/` returns 404. The userscript's API on `MANGA_API_HOST` is unaffected either way. -Sessions are signed with a key derived from `API_TOKEN`, so rotating the token -logs every browser out. The session cookie lasts 60 days. +`MANGA_WEB_HOST` itself is required by the prod override regardless — like +`MANGA_API_HOST`, its Traefik label has no fallback, so `docker compose up` +refuses to start without it even if `WEB_PASSWORD` is unset and the web UI is +otherwise dormant. + +Sessions are signed with a key derived from `API_TOKEN` and `WEB_PASSWORD`, so +rotating either one logs every browser out. The session cookie lasts 60 days. --- diff --git a/backend/session.go b/backend/session.go index 70caf91..9224320 100644 --- a/backend/session.go +++ b/backend/session.go @@ -22,11 +22,13 @@ const ( sessionKeyPurpose = "mangabm-web-session-v1" ) -// sessionKey derives the cookie-signing key from the API token. Sessions are -// stateless — there is no session table — so rotating API_TOKEN invalidates -// every outstanding cookie at once. -func sessionKey(apiToken string) []byte { - sum := sha256.Sum256([]byte(apiToken + sessionKeyPurpose)) +// sessionKey derives the cookie-signing key from both secrets. Sessions are +// stateless — there is no session table — so rotating either API_TOKEN or +// WEB_PASSWORD invalidates every outstanding cookie at once. The \x00 +// separator prevents the concatenation ambiguity a bare apiToken+webPassword +// would have (e.g. "ab"+"c" colliding with "a"+"bc"). +func sessionKey(apiToken, webPassword string) []byte { + sum := sha256.Sum256([]byte(apiToken + "\x00" + webPassword + sessionKeyPurpose)) return sum[:] } @@ -166,6 +168,10 @@ func (l *loginLimiter) reset(ip string) { // The caller must hold l.mu. func (l *loginLimiter) pruneLocked(ip string, now time.Time) []time.Time { cutoff := now.Add(-loginWindow) + // In-place filter: kept reuses the backing array of the slice being + // ranged over. The range expression captures the slice header once at + // the start, so the append cursor (kept) can never outrun the read + // cursor (the range index) — safe to alias. kept := l.failures[ip][:0] for _, at := range l.failures[ip] { if at.After(cutoff) { diff --git a/backend/session_test.go b/backend/session_test.go index dd1a9ae..4c261b4 100644 --- a/backend/session_test.go +++ b/backend/session_test.go @@ -10,7 +10,7 @@ import ( ) func TestSessionRoundTrip(t *testing.T) { - key := sessionKey("token-abc") + key := sessionKey("token-abc", "pw-abc") now := time.Now().UnixMilli() value := signSession(key, now+60_000) if !verifySession(key, value, now) { @@ -19,7 +19,7 @@ func TestSessionRoundTrip(t *testing.T) { } func TestSessionRejects(t *testing.T) { - key := sessionKey("token-abc") + key := sessionKey("token-abc", "pw-abc") now := time.Now().UnixMilli() valid := signSession(key, now+60_000) payload, sig, _ := strings.Cut(valid, ".") @@ -34,7 +34,7 @@ func TestSessionRejects(t *testing.T) { {"expired", signSession(key, now-1)}, {"tampered signature", payload + "." + flipLastChar(sig)}, {"tampered expiry", "99999999999999." + sig}, - {"signed with another key", signSession(sessionKey("other-token"), now+60_000)}, + {"signed with another key", signSession(sessionKey("other-token", "pw-abc"), now+60_000)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -57,13 +57,21 @@ func flipLastChar(s string) string { } func TestSessionKeyDependsOnToken(t *testing.T) { - a := sessionKey("token-a") - b := sessionKey("token-b") + a := sessionKey("token-a", "pw-abc") + b := sessionKey("token-b", "pw-abc") if string(a) == string(b) { t.Fatal("sessionKey collided for different API tokens") } } +func TestSessionKeyDependsOnWebPassword(t *testing.T) { + a := sessionKey("token-abc", "pw-a") + b := sessionKey("token-abc", "pw-b") + if string(a) == string(b) { + t.Fatal("sessionKey collided for different web passwords with the same API token") + } +} + func TestSetSessionCookieAttributes(t *testing.T) { cases := []struct { name string @@ -86,7 +94,7 @@ func TestSetSessionCookieAttributes(t *testing.T) { r.Header.Set("X-Forwarded-Proto", tc.forwarded) } rr := httptest.NewRecorder() - setSessionCookie(rr, r, sessionKey("token-abc")) + setSessionCookie(rr, r, sessionKey("token-abc", "pw-abc")) cookies := rr.Result().Cookies() if len(cookies) != 1 { diff --git a/backend/web.go b/backend/web.go index e039497..1b1bd97 100644 --- a/backend/web.go +++ b/backend/web.go @@ -55,7 +55,7 @@ func newWebHandler(store *Store, cfg Config) (*webHandler, error) { return &webHandler{ store: store, tmpl: tmpl, - key: sessionKey(cfg.Token), + key: sessionKey(cfg.Token, cfg.WebPassword), password: cfg.WebPassword, limiter: newLoginLimiter(), }, nil @@ -73,9 +73,10 @@ func (h *webHandler) register(mux *http.ServeMux) { mux.HandleFunc("DELETE /ui/bookmarks/{key}", h.requireSession(h.uiDelete)) } -// staticHandler serves the embedded assets. The vendored htmx build and the -// stylesheet change only on deploy, so a long max-age is safe; a redeploy -// changes the binary and the browser revalidates on its own schedule. +// staticHandler serves the embedded assets. An hour, not longer: assets are +// not fingerprinted, and embed.FS reports a zero ModTime, so http.FileServer +// emits no Last-Modified or ETag and a client has no way to revalidate a +// cached copy after a deploy short of waiting out max-age. func staticHandler() http.Handler { sub, err := fs.Sub(staticFS, "static") if err != nil { @@ -253,9 +254,12 @@ func (h *webHandler) uiFavorite(w http.ResponseWriter, r *http.Request) { // uiChapter forces the read chapter to a value the user typed. // -// It clears last_chapter_url: that URL points at the chapter actually read, and -// once the number is forced elsewhere it would send the reader backwards. -// ContinueURL then falls back to the series page, which is always right. +// When that value actually changes the number, it also clears last_chapter_url: +// that URL points at the chapter actually read, and once the number is forced +// elsewhere it would send the reader backwards. ContinueURL then falls back to +// the series page, which is always right. Resubmitting the same number — the +// form is pre-filled, so a bare tap of Save is an easy accidental submit — +// leaves last_chapter_url untouched instead of destroying it for no reason. func (h *webHandler) uiChapter(w http.ResponseWriter, r *http.Request) { b, ok := h.loadForMutation(w, r) if !ok { @@ -272,9 +276,11 @@ func (h *webHandler) uiChapter(w http.ResponseWriter, r *http.Request) { return } + if num != b.LastChapterNum { + b.LastChapterURL = "" + } b.LastChapter = raw b.LastChapterNum = num - b.LastChapterURL = "" b.UpdatedAt = time.Now().UnixMilli() h.saveAndRenderCard(w, b) } diff --git a/backend/web_test.go b/backend/web_test.go index 8f793a1..e811ab6 100644 --- a/backend/web_test.go +++ b/backend/web_test.go @@ -36,7 +36,7 @@ func sessionCookie(t *testing.T, cfg Config) *http.Cookie { t.Helper() return &http.Cookie{ Name: sessionCookieName, - Value: signSession(sessionKey(cfg.Token), time.Now().Add(time.Hour).UnixMilli()), + Value: signSession(sessionKey(cfg.Token, cfg.WebPassword), time.Now().Add(time.Hour).UnixMilli()), } } @@ -352,6 +352,40 @@ func TestChapterOverrideMovesUpdatedAt(t *testing.T) { } } +func TestChapterOverrideNoOpPreservesURLAndUpdatedAt(t *testing.T) { + cfg := webConfig() + srv, store := newWebTestServer(t, cfg) + before := seed(t, store, Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + Title: "Solo Leveling", LastChapter: "45", LastChapterNum: 45, + LastChapterURL: "https://example.test/ch/45", SeriesURL: "https://example.test/solo", + UpdatedAt: 1_000_000, + }) + + // The chapter form is pre-filled with the current value, so tapping Save + // without editing resubmits the unchanged number. That must be a no-op: + // it must not silently clear last_chapter_url or move updated_at. + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, uiRequest(t, cfg, http.MethodPost, + "/ui/bookmarks/asura:solo/chapter", url.Values{"chapter": {"45"}})) + if rr.Code != http.StatusOK { + t.Fatalf("chapter no-op status = %d, want 200", rr.Code) + } + + after, ok, err := store.Get("asura:solo") + if err != nil || !ok { + t.Fatalf("Get after no-op override: %v ok=%v", err, ok) + } + if after.LastChapterURL != before.LastChapterURL { + t.Fatalf("LastChapterURL = %q, want preserved %q on a no-op save", + after.LastChapterURL, before.LastChapterURL) + } + if after.UpdatedAt != before.UpdatedAt { + t.Fatalf("UpdatedAt = %d, want unchanged %d on a no-op save", + after.UpdatedAt, before.UpdatedAt) + } +} + func TestChapterOverrideRejectsBadInput(t *testing.T) { cfg := webConfig() srv, store := newWebTestServer(t, cfg) diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index f37e64e..78bc45b 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -5,7 +5,7 @@ # # Set in .env: # MANGA_API_HOST=manga-api.example.com # your subdomain (required) -# MANGA_WEB_HOST=manga.example.com # browser UI subdomain, same container +# MANGA_WEB_HOST=manga.example.com # browser UI subdomain, same container (required) # PROXY_NETWORK=proxy # Traefik's network name, if not "proxy" # TRAEFIK_ENTRYPOINT=websecure # your HTTPS entrypoint name # TRAEFIK_CERTRESOLVER=le # your ACME/cert resolver name @@ -30,7 +30,7 @@ services: # Second hostname for the browser UI, same container. Traefik needs the # service named explicitly once more than one router targets it. - "traefik.http.routers.mangabm.service=mangabm" - - "traefik.http.routers.mangaweb.rule=Host(`${MANGA_WEB_HOST:-manga.example.com}`)" + - "traefik.http.routers.mangaweb.rule=Host(`${MANGA_WEB_HOST:?set MANGA_WEB_HOST in .env}`)" - "traefik.http.routers.mangaweb.entrypoints=${TRAEFIK_ENTRYPOINT:-websecure}" - "traefik.http.routers.mangaweb.tls=true" - "traefik.http.routers.mangaweb.tls.certresolver=${TRAEFIK_CERTRESOLVER:-le}"