From f4f6c9c9e96c1c05214d673741e170dd83c97788 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 8 Aug 2026 20:14:21 +0700 Subject: [PATCH] fix: address code review on #27 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The empty state is about an empty library, not a brand-new Reader: listView.Fresh becomes EmptyLibrary and moves behind the tab-specific branches, so "No favourites yet" is no longer shadowed for a Reader whose library happens to be empty. The action key stays put — hiding it was never asked for. - The owner is not a revocable Reader: their row offers no button and POST /readers/{owner}/revoke is a 404, so the one row where the control would sign out the tapping browser cannot be reached by a hand-rolled POST either. - Modify isolation is asserted in both directions, and the owner's own sign-in through the OAuth callback is pinned to the seeded row. - CUTOVER.md and REDEPLOY.md still grepped API_TOKEN out of .env for their smoke tests, which the last commit deleted; both now take the acting Reader's derived credential. - Roster type follows the machine-fact spec (500 10-11px mono, tracked), and PRODUCT.md names the Readers panel instead of claiming there is no owner surface at all. --- CUTOVER.md | 5 ++- PRODUCT.md | 2 +- REDEPLOY.md | 7 ++-- backend/internal/web/static/style.css | 11 ++++-- backend/internal/web/templates/chrome.html | 6 ++-- backend/internal/web/templates/list.html | 23 ++++++------- backend/internal/web/templates/readers.html | 5 ++- backend/internal/web/web.go | 35 ++++++++++++------- backend/reader_credential_test.go | 23 +++++++++++-- backend/web_test.go | 38 ++++++++++++++++++++- 10 files changed, 115 insertions(+), 40 deletions(-) diff --git a/CUTOVER.md b/CUTOVER.md index a44aa92..e1e04e9 100644 --- a/CUTOVER.md +++ b/CUTOVER.md @@ -271,7 +271,10 @@ would catch a correct import behind a broken join: ```bash API=https://bookmark-api.violetcrown.my.id -TOKEN=$(grep -E '^API_TOKEN=' .env | cut -d= -f2) # grace-window credential +# Your own Reader credential: sign in to the web UI and take it from the +# Userscripts panel's install link, or read the API_TOKEN constant out of an +# already-installed script. There is no credential in .env to grep. +TOKEN= curl -s -H "Authorization: Bearer $TOKEN" $API/bookmarks | python3 -c 'import json,sys; print(len(json.load(sys.stdin)))' # -> 29 ``` diff --git a/PRODUCT.md b/PRODUCT.md index a6ec893..1c9e02b 100644 --- a/PRODUCT.md +++ b/PRODUCT.md @@ -54,7 +54,7 @@ Not a public reading tracker or social app — a private, self-hosted sync layer - Mobile is the primary target; desktop is an enhancement, not the design center. - Progress data integrity over visual flourish: `updated_at`/list-ordering behavior is a correctness constraint the UI must respect, not decorate over. - A leak between Readers fails silently and looks like working software — isolation is asserted from both directions, never inferred from counting one Reader's rows. -- No roles, no admin console, no org chrome: one owner capability and otherwise every Reader's view is the same. +- No roles, no org chrome: the owner's Readers panel is one list with one button (revoke someone's sessions), not an admin console, and otherwise every Reader's view is the same. - Prefer native platform affordances (system dark/light, native touch targets) over custom widgetry — this is a lean self-hosted tool, not a product to demo. ## Accessibility & Inclusion diff --git a/REDEPLOY.md b/REDEPLOY.md index 6ef8ba4..86e7ebf 100644 --- a/REDEPLOY.md +++ b/REDEPLOY.md @@ -231,9 +231,10 @@ Same four API checks as `DEPLOY.md` §3, plus the web UI. Set the host names onc ```bash API=https://bookmark-api.violetcrown.my.id WEB=https://bookmark.violetcrown.my.id -# During the grace window the retired global credential still resolves to the -# owner; afterwards it is 401 like any other wrong credential. -TOKEN=$(grep -E '^API_TOKEN=' .env | cut -d= -f2) +# Your own Reader credential - derived, never stored in .env. Take it from the +# Userscripts panel's install link after signing in, or from an installed +# script's API_TOKEN constant. +TOKEN= curl -s $API/healthz # -> ok curl -s -o /dev/null -w '%{http_code}\n' $API/bookmarks # -> 401 diff --git a/backend/internal/web/static/style.css b/backend/internal/web/static/style.css index d8bd889..fbf07ec 100644 --- a/backend/internal/web/static/style.css +++ b/backend/internal/web/static/style.css @@ -301,10 +301,15 @@ button { cursor: pointer; } border-top: 1px solid var(--rule); } .readerlist form { margin: 0 0 0 auto; } -.reader-id { font: 400 13px/1.4 var(--font-mono); color: var(--paper); } -.reader-sessions { - font: 400 11px/1.4 var(--font-mono); +.reader-id { + font: 500 13px/1.4 var(--font-mono); letter-spacing: .04em; + color: var(--paper); +} +.reader-sessions { + font: 500 10px/1 var(--font-mono); + letter-spacing: .14em; + text-transform: uppercase; color: var(--mute); } /* Revocation cuts someone off, so it wears --danger. Ember stays reserved for diff --git a/backend/internal/web/templates/chrome.html b/backend/internal/web/templates/chrome.html index 2d7c469..7537c9a 100644 --- a/backend/internal/web/templates/chrome.html +++ b/backend/internal/web/templates/chrome.html @@ -30,11 +30,9 @@ {{/* The action key. The icon strip on a card is unlabelled, so one permanent line under the tabs names every glyph. It follows the tab rather than the row: the archived and finished buckets swap Archive for Restore, and a - finished series has no Done to offer. A Reader with no cards at all has - nothing for it to name, so it hides rather than disappearing — see the - note above about out-of-band swaps needing their target to exist. */}} + finished series has no Done to offer. */}} {{define "keyrow"}} -
+
Read Fav Chapter diff --git a/backend/internal/web/templates/list.html b/backend/internal/web/templates/list.html index 8102309..33dc485 100644 --- a/backend/internal/web/templates/list.html +++ b/backend/internal/web/templates/list.html @@ -8,18 +8,6 @@ No titles match “”.
-{{else if .Fresh}} - {{/* Nothing anywhere, not an empty bucket: this Reader has just registered, - so the empty state is the setup instruction rather than a filter - report. Both scripts, because the two libraries are separate installs. */}} -
- Your library is empty. -

Install both userscripts, then open a series and read a chapter — bookmarks arrive on their own.

-

- Install Manga script - Install Novels script -

-
{{else if eq .Tab "fav"}}
No favourites yet.

Star a series to pin it here.

{{else if eq .Tab "new"}} @@ -28,6 +16,17 @@
Nothing archived.

Shelve a series to park it here — it keeps getting checked for new chapters.

{{else if eq .Tab "finished"}}
Nothing finished yet.

Mark a series finished and it moves out of your reading list.

+{{else if .EmptyLibrary}} + {{/* Nothing in either library, so the links are the only thing this page can + usefully say. Both scripts: the two libraries are separate installs. */}} +
+ Nothing here yet. +

Install the userscripts, then open a series and read a chapter — bookmarks arrive on their own.

+

+ Install Manga script + Install Novels script +

+
{{else}}
Nothing here yet.

Bookmarks appear once the userscript records a chapter.

{{end}} diff --git a/backend/internal/web/templates/readers.html b/backend/internal/web/templates/readers.html index 4f192e0..85b844c 100644 --- a/backend/internal/web/templates/readers.html +++ b/backend/internal/web/templates/readers.html @@ -13,7 +13,10 @@
  • {{.DiscordID}} {{.Sessions}} session{{if ne .Sessions 1}}s{{end}} - {{if .Sessions}} + {{/* The owner's own row never offers Revoke: it is the one row where the + button would sign the tapping browser out, and the endpoint refuses + it anyway. Logout is the deliberate way to do that. */}} + {{if and .Sessions (ne .ID $.OwnerID)}}
    diff --git a/backend/internal/web/web.go b/backend/internal/web/web.go index 5405ec5..4c3c753 100644 --- a/backend/internal/web/web.go +++ b/backend/internal/web/web.go @@ -72,16 +72,19 @@ type listView struct { // Rotated marks the setup panel as having just rotated the credential: // it swaps the reinstall warning in over the button row. Rotated bool - // Fresh means this Reader has no bookmarks at all, in either library — a - // brand-new registration rather than an empty bucket. The empty state then - // explains how a library gets filled instead of describing a filter. - Fresh bool + // EmptyLibrary means this Reader holds no bookmarks in either library, so + // the empty state can offer the installs instead of reporting on a filter. + // It is not "newly registered": a Reader who deletes their last bookmark is + // in the same position and needs the same links. + EmptyLibrary bool // Owner marks the acting Reader as the deployment's owner, which unlocks // the Readers panel. Nothing else in the UI differs. Owner bool // Readers is the owner's roster, populated only for the owner's own page - // render and the revocation fragment. + // render and the revocation fragment. OwnerID travels with it so the roster + // can tell the owner's own row apart from the Readers they may revoke. Readers []store.ReaderSummary + OwnerID int64 } // PageURL and ListURL are the two link shapes every tab needs. Building them @@ -238,7 +241,7 @@ func (h *Handler) index(w http.ResponseWriter, r *http.Request) { return } if readerID == h.store.OwnerID() { - view.Owner = true + view.Owner, view.OwnerID = true, readerID if view.Readers, err = h.store.Readers(); err != nil { log.Printf("index readers: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -292,10 +295,10 @@ func (h *Handler) buildListView(readerID int64, lib, tab string) (listView, erro if err != nil { return listView{}, err } - // Fresh is about the Reader, not the library, so it is taken before the - // filter narrows the slice: it decides whether an empty list reads as - // "install the scripts" or "this bucket is empty". - fresh := len(all) == 0 + // Taken before the filter narrows the slice: a Reader with novels but no + // manga has a working install already, and does not need to be told to go + // and get one. + emptyLibrary := len(all) == 0 // Narrow to one library first: reading, withNew and recent all derive from // this slice, so doing it later would let the other library's rows into the // strip and the Updated badge. @@ -340,7 +343,7 @@ func (h *Handler) buildListView(readerID int64, lib, tab string) (listView, erro } } return listView{Lib: lib, Tab: tab, Recent: recent, Items: items, - NewCount: len(withNew), Fresh: fresh}, nil + NewCount: len(withNew), EmptyLibrary: emptyLibrary}, nil } func (h *Handler) uiList(w http.ResponseWriter, r *http.Request) { @@ -631,6 +634,14 @@ func (h *Handler) revokeReaderSessions(w http.ResponseWriter, r *http.Request) { http.Error(w, "bad reader id", http.StatusBadRequest) return } + // The owner is not one of the Readers this endpoint reaches: revoking + // themselves would sign out the browser making the request, which is what + // logout is for. The roster hides the button; this refuses the hand-rolled + // POST behind it. + if target == h.store.OwnerID() { + http.NotFound(w, r) + return + } if err := h.store.DeleteReaderSessions(target); err != nil { log.Printf("revoke sessions: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -642,5 +653,5 @@ func (h *Handler) revokeReaderSessions(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal error", http.StatusInternalServerError) return } - h.render(w, http.StatusOK, "readers", listView{Owner: true, Readers: readers}) + h.render(w, http.StatusOK, "readers", listView{Owner: true, Readers: readers, OwnerID: h.store.OwnerID()}) } diff --git a/backend/reader_credential_test.go b/backend/reader_credential_test.go index f2fa40f..e66970c 100644 --- a/backend/reader_credential_test.go +++ b/backend/reader_credential_test.go @@ -119,8 +119,27 @@ func TestPerReaderIsolation(t *testing.T) { if err := json.Unmarshal(rr.Body.Bytes(), &owners); err != nil { t.Fatalf("decode: %v", err) } - if len(owners) != 1 || owners[0].Title != "Solo Leveling" { - t.Fatalf("owner list = %+v, want their own row", owners) + if len(owners) != 1 || owners[0].Title != "Solo Leveling" || owners[0].LastChapterNum != 0 { + t.Fatalf("owner list = %+v, want their own row at their own progress", owners) + } + + // The mirror: the owner's write does not move the other Reader's progress + // either. Without it, isolation is only asserted in one direction. + req = credRequest(http.MethodPut, "/bookmarks/"+ownerKey, ownerCredential()) + req.Header.Set("Content-Type", "application/json") + rr = httptest.NewRecorder() + srv.ServeHTTP(rr, withBody(req, `{"key":"asura:solo","site":"asura","series_id":"solo","title":"Solo Leveling","last_chapter_num":9}`)) + if rr.Code != http.StatusOK { + t.Fatalf("owner put: status = %d, want 200", rr.Code) + } + rr = httptest.NewRecorder() + srv.ServeHTTP(rr, credRequest(http.MethodGet, "/bookmarks", readerCredential("other-reader"))) + var theirs3 []store.Bookmark + if err := json.Unmarshal(rr.Body.Bytes(), &theirs3); err != nil { + t.Fatalf("decode: %v", err) + } + if len(theirs3) != 1 || theirs3[0].LastChapterNum != 3 { + t.Fatalf("other reader list = %+v, want progress 3 after the owner's write", theirs3) } // DELETE is scoped to its caller too, asserted in both directions: each diff --git a/backend/web_test.go b/backend/web_test.go index daf60ad..eb3c704 100644 --- a/backend/web_test.go +++ b/backend/web_test.go @@ -485,6 +485,23 @@ func TestGuildMemberRegistersOnFirstLoginAndReusesIt(t *testing.T) { } } +// The seeded owner signs in through the same path: their row is found, not +// created a second time. +func TestOwnerLoginReusesTheSeededReader(t *testing.T) { + stub, stubSrv := newDiscordStub(t) + stub.ownerID = testDiscordID + cfg := testConfig() + cfg.Discord = discordConfig(stubSrv.URL) + router, st := newWebTestServer(t, cfg) + + if got := signedInReader(t, router, st); got != st.OwnerID() { + t.Fatalf("owner's sign-in landed on Reader %d, want the seeded %d", got, st.OwnerID()) + } + if n := len(storeReaders(t, st)); n != 1 { + t.Fatalf("readers after the owner's login = %d, want 1 (the seed was duplicated)", n) + } +} + // A brand-new Reader's page explains how a library gets filled and offers both // install links, and the script it serves carries their credential — not the // owner's. @@ -514,7 +531,7 @@ func TestNewReaderSeesEmptyLibraryAndTheirOwnScript(t *testing.T) { } body := rr.Body.String() for _, want := range []string{ - "Your library is empty", + "Nothing here yet", `href="/install/manga-bookmark.user.js"`, `href="/install/novel-bookmark.user.js"`, } { @@ -570,6 +587,20 @@ func TestOwnerRevokesAnotherReadersSessions(t *testing.T) { t.Fatal("a non-owner's revoke attempt still killed the owner's session") } + // The owner is not a revocable Reader: the button would sign out the browser + // making the request, so both the roster and the endpoint refuse it. + req = httptest.NewRequest(http.MethodPost, + "/readers/"+strconv.FormatInt(st.OwnerID(), 10)+"/revoke", nil) + req.AddCookie(ownerCookie) + rr = httptest.NewRecorder() + router.ServeHTTP(rr, req) + if rr.Code != http.StatusNotFound { + t.Fatalf("owner revoking themselves: status = %d, want 404", rr.Code) + } + if _, ok, _ := st.GetSession(ownerCookie.Value, time.Now()); !ok { + t.Fatal("the owner signed themselves out through the revoke endpoint") + } + req = httptest.NewRequest(http.MethodPost, "/readers/"+strconv.FormatInt(theirSession.ReaderID, 10)+"/revoke", nil) req.AddCookie(ownerCookie) @@ -614,6 +645,11 @@ func TestOwnerSeesReadersPanel(t *testing.T) { if !strings.Contains(body, "Revoke sessions") { t.Fatal("the roster offers no revocation control for a signed-in Reader") } + // Exactly one revocable row: the other Reader's. The owner's own row carries + // the same session count and no button. + if n := strings.Count(body, "/revoke"); n != 1 { + t.Fatalf("roster has %d revoke controls, want 1 (the owner's own row must have none):\n%s", n, body) + } } func TestDiscordLoginTokenEndpointDown(t *testing.T) {