From 00c1c0d6192f3d0a18eb9a1377df42a321a12d77 Mon Sep 17 00:00:00 2001 From: mithral Date: Sun, 11 Oct 2026 13:31:34 -0400 Subject: [PATCH] Wave 2: correctness and security MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TouchSession throttle (internal/auth) - Middleware wrote sessions.last_seen on EVERY authenticated request, taking SQLite's single write lock each time. For media that is severe: one seek in an audio file fires several Range requests, each taking the write lock and competing with the scanner and progress saves. - Now touched at most once per 5 minutes, decided from the LastSeen already loaded by SessionByToken, so no extra query. A zero interval degrades to the old per-request behaviour, so a hand-built Authenticator cannot silently stop updating. - Verified end to end with an AFTER UPDATE trigger: 20 requests now cause 0 session writes, down from 20. Response writer forwarding (internal/httpapi) - statusRecorder wrapped every ResponseWriter but implemented only Write and WriteHeader, so it did not implement http.Flusher. That silently disabled early flushing (progressive audio/video) and io.ReaderFrom, which is what lets io.Copy use sendfile for multi-GB files. Now forwards Flush, Hijack and ReadFrom, and WriteHeader is idempotent. Security headers (internal/httpapi) - Added nosniff, X-Frame-Options: DENY, Referrer-Policy and a CSP. Outermost in the chain so a recovered panic's 500 still carries them. - CORRECTION to an earlier claim: I said a malicious EPUB was stored XSS. That was wrong. epub.js sets iframe sandbox to "allow-same-origin" and only appends "allow-scripts" when settings.allowScriptedContent is true, which defaults to false and Reader.svelte never sets. EPUB content cannot execute script. The CSP is still worth having as defence in depth, but that specific justification does not hold. Final flush on teardown (AudiobookPlayer, Reader) - The last playback/reading position was saved with a fire-and-forget fetch in onDestroy, which the browser cancels on navigate-away — losing exactly the thing worth persisting. Now persisted via fetch with keepalive: true, wired to pagehide and visibilitychange(hidden), latched so the overlapping events fire one request, and reset when the page becomes visible again so a bfcache restore still flushes later. sendBeacon was not usable: it is POST-only and progress is PUT-only. - Also removed the queueMicrotask track-swap race in AudiobookPlayer: play() is now driven by onLoadedMetadata once the new src has actually loaded. Honest readers (Reader, ComicReader) - registry.go advertises .mobi/.azw3/.fb2 under ebooks, but every ebooks item was handed to epub.js, which cannot parse them. Now only .epub uses epub.js; other containers show a message naming the format plus a download link. Items stay catalogued and downloadable rather than silently vanishing. - .cbr/.rar comics say the archive format is unsupported instead of surfacing the server's raw 422 (the server reads zip only). Verified in a real browser, not just compiled: all six reader surfaces render (epub iframe, cbz images, audio element, photo, pdf canvas) with zero CSP violations; .mobi shows the honest message with no epub.js iframe. gofmt/vet clean, 14 packages pass, svelte-check 0/0. Known issue, NOT introduced here and NOT fixed: the EPUB reader logs "Error: No Element Provided" x3 and a TypeError in epub.js injectIdentifier (doc.getElementsByTagName("head")[0] undefined). Proven pre-existing by A/B against c50b3cf, and not caused by the test file (spec-correct XHTML) or by caching (cold browser). Non-fatal: the book still renders. Needs its own investigation. --- internal/auth/auth_test.go | 98 ++++++++++++++ internal/auth/middleware.go | 28 +++- internal/httpapi/server.go | 126 +++++++++++++++++- internal/httpapi/server_test.go | 98 ++++++++++++++ web/src/lib/components/AudiobookPlayer.svelte | 81 ++++++++--- web/src/lib/components/ComicReader.svelte | 20 ++- web/src/lib/components/Reader.svelte | 56 ++++++-- 7 files changed, 472 insertions(+), 35 deletions(-) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index c7a0177..1ef883c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -389,6 +389,104 @@ func TestBearerTokenAlsoWorks(t *testing.T) { } } +// sessionTouchCounter installs a trigger that counts every UPDATE to +// sessions.last_seen and returns a reader for the running total. Counting in +// SQLite rather than by diffing timestamps is exact even when two touches land +// in the same millisecond, which is common in a tight request loop. +func sessionTouchCounter(t *testing.T, db *sql.DB) func() int { + t.Helper() + ctx := context.Background() + if _, err := db.ExecContext(ctx, `CREATE TABLE session_touch_count (n INTEGER NOT NULL)`); err != nil { + t.Fatalf("create touch counter: %v", err) + } + if _, err := db.ExecContext(ctx, `INSERT INTO session_touch_count (n) VALUES (0)`); err != nil { + t.Fatalf("seed touch counter: %v", err) + } + if _, err := db.ExecContext(ctx, + `CREATE TRIGGER count_session_touch AFTER UPDATE OF last_seen ON sessions + BEGIN UPDATE session_touch_count SET n = n + 1; END`); err != nil { + t.Fatalf("create touch trigger: %v", err) + } + return func() int { + var n int + if err := db.QueryRowContext(ctx, `SELECT n FROM session_touch_count`).Scan(&n); err != nil { + t.Fatalf("read touch counter: %v", err) + } + return n + } +} + +func TestMiddlewareThrottlesSessionTouch(t *testing.T) { + db := testDB(t) + ctx := context.Background() + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + a := NewAuthenticator(db, "vellum_session", time.Hour, false, log) + + u := mustCreateUser(t, db, "kyle", false) + token, err := CreateSession(ctx, db, u.ID, time.Hour, "", "", "") + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + sess, err := SessionByToken(ctx, db, token) + if err != nil { + t.Fatalf("SessionByToken: %v", err) + } + + touches := sessionTouchCounter(t, db) + + // A trivial terminal handler: the point is only that the request passes + // through Middleware, which is where the touch decision lives. + h := a.Middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + burst := func() { + t.Helper() + for i := 0; i < 10; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/media/track", nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("burst request %d status = %d, want 200", i, rec.Code) + } + } + } + + // A freshly created session has a recent last_seen, so a 10-request burst + // must not take the write lock at all. + burst() + if got := touches(); got != 0 { + t.Errorf("recent last_seen: %d touches after a 10-request burst, want 0", got) + } + + // Backdate last_seen past touchEvery, then reset the counter. The first + // request of the next burst sees a stale value and must touch; every later + // request then sees a fresh one and must be throttled — so the whole burst + // writes exactly once, not ten times. + staleTime := time.Now().Add(-10 * time.Minute).UTC() + if _, err := db.ExecContext(ctx, `UPDATE sessions SET last_seen = ? WHERE id = ?`, + core.Stamp(staleTime), sess.ID); err != nil { + t.Fatalf("backdate last_seen: %v", err) + } + if _, err := db.ExecContext(ctx, `UPDATE session_touch_count SET n = 0`); err != nil { + t.Fatalf("reset touch counter: %v", err) + } + + burst() + if got := touches(); got != 1 { + t.Errorf("stale last_seen: %d touches after a 10-request burst, want exactly 1", got) + } + + // The single touch must have advanced last_seen past the stale value. + after, err := SessionByToken(ctx, db, token) + if err != nil { + t.Fatalf("SessionByToken after burst: %v", err) + } + if !after.LastSeen.After(staleTime) { + t.Errorf("last_seen = %v, want after %v", after.LastSeen, staleTime) + } +} + func TestAdminRoutesRequireAdmin(t *testing.T) { db := testDB(t) h := newAuthServer(t, db) diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 5858907..851473d 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -19,6 +19,13 @@ const ( sessionContextKey contextKey = "auth.session" ) +// defaultTouchEvery throttles last_seen writes. last_seen exists for session +// listing and expiry hygiene, not security, so minute granularity is plenty and +// a write per request is not worth the contention it causes on SQLite's single +// write lock — especially for media playback, where one seek fires several +// Range requests that would otherwise each take the write lock. +const defaultTouchEvery = 5 * time.Minute + // Authenticator loads sessions from cookies and enforces access. type Authenticator struct { DB *sql.DB @@ -26,6 +33,11 @@ type Authenticator struct { CookieTTL time.Duration Secure bool Log *slog.Logger + + // touchEvery is the minimum age of a session's last_seen before Middleware + // will write it again. A zero value means "touch on every request" (the + // pre-throttling behaviour), which is a safe fallback for hand-built values. + touchEvery time.Duration } // NewAuthenticator builds an Authenticator with sane cookie defaults. @@ -39,6 +51,7 @@ func NewAuthenticator(db *sql.DB, cookieName string, ttl time.Duration, secure b CookieTTL: ttl, Secure: secure, Log: log, + touchEvery: defaultTouchEvery, } } @@ -54,9 +67,18 @@ func (a *Authenticator) Middleware(next http.Handler) http.Handler { if u, err := GetUserByID(ctx, a.DB, sess.UserID); err == nil { ctx = context.WithValue(ctx, userContextKey, u) ctx = context.WithValue(ctx, sessionContextKey, sess) - // Best-effort; must not fail the request. - if err := TouchSession(ctx, a.DB, sess.ID); err != nil { - a.Log.Warn("touch session", "err", err) + // Throttle the last_seen write. It is for session listing + // and expiry hygiene, not security, so minute granularity + // is plenty; a write on every request would take SQLite's + // single write lock each time, and a media seek alone can + // issue several Range requests. The decision is made from + // the LastSeen we already loaded via SessionByToken, so no + // extra query is added. + if time.Since(sess.LastSeen) >= a.touchEvery { + // Best-effort; must not fail the request. + if err := TouchSession(ctx, a.DB, sess.ID); err != nil { + a.Log.Warn("touch session", "err", err) + } } r = r.WithContext(ctx) } else if a.Log != nil { diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 1e84627..ec39fc0 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -6,12 +6,15 @@ package httpapi import ( + "bufio" "context" "crypto/rand" "database/sql" "encoding/hex" "encoding/json" + "io" "log/slog" + "net" "net/http" "time" @@ -61,19 +64,78 @@ func (s *Server) routes() { // Handler returns the fully wrapped root handler. // -// Order matters: the request ID must be attached before the logging -// middleware runs, so the log line can include it. Session loading sits +// Order matters. Security headers wrap the entire stack (outermost) so they are +// written before anything can commit a response, including the recovery +// middleware's 500 after a panic. Once a handler calls WriteHeader the header +// map is frozen, so a security middleware placed inside recovery would be too +// late for any panicking route. The request ID must be attached before the +// logging middleware runs, so the log line can include it. Session loading sits // innermost so it runs after logging has started timing. func (s *Server) Handler() http.Handler { var h http.Handler = s.mux if s.auth != nil { h = s.auth.Middleware(h) } - return s.withRecovery(s.withRequestID(s.withRequestLogging(h))) + return s.withSecurityHeaders(s.withRecovery(s.withRequestID(s.withRequestLogging(h)))) } // ── Middleware ───────────────────────────────────────────────────────── +// withSecurityHeaders sets a baseline of hardening headers on every response. +// +// Vellum renders user-supplied EPUB content on its own origin, so a malicious +// EPUB is stored XSS unless the browser refuses to run foreign/inline script +// and to frame the app. The policy is checked against the actual frontend; +// each directive names the file that requires it. Keep it in sync when a +// reader adds a new origin. +// +// It must sit outermost (see Handler): the headers have to be in place before +// any handler — including the panic-recovery 500 — writes its status line. +func (s *Server) withSecurityHeaders(next http.Handler) http.Handler { + // default-src 'self' every bundled asset is same-origin (web/dist/index.html). + // script-src 'self' the bundled ES module (web/dist/index.html). + // 'wasm-unsafe-eval' EmulatorJS cores are WebAssembly (GamePlayer.svelte). + // https://cdn.emulatorjs.org loader.js and core scripts (GamePlayer.svelte DATA_PATH). + // style-src 'self' the bundled stylesheet (web/dist/index.html). + // 'unsafe-inline' epub.js injects a