From 8863b7591661d3aa76f66afda0b3a67e964965b7 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:08:42 -0700 Subject: [PATCH] Fix push SSRF, cross-user unsub, and personalization edge cases Code review of the personalization/feeds/PWA/push work surfaced ten confirmed issues, now fixed: - Web Push delivery bypassed the SSRF guard (unguarded default client); now routes through safehttp.NewClient with a hard timeout, and the subscribe handler validates the endpoint URL. - Push unsubscribe deleted by endpoint with no owner check; added RemovePushSubscriptionForUser scoped to the signed-in user. - Byte-slice body/content truncation could split a UTF-8 rune and break the RSS content:encoded XML; added a rune-safe truncateUTF8 helper. - Digest sender could permanently starve a user who hid a high-volume source; step the watermark past a full hidden-source scan window. - Service worker cached personalized HTML navigations into a shared cache (identity leak across PWA users); navigations are now network-only, CACHE_VERSION bumped to v2 to purge stale pages. - Public /api/article leaked discarded/unclassified bodies; filter to classified, non-sentinel stories. - runLocal never started the push sender; digests now fire in -local. - Push client had no timeout, so one hung endpoint stalled all sends. - Reader migration resurrected cross-device-cleared reads; gate it behind a one-time flag so the server stays authoritative. - Bookmarks count didn't match the classified list filter. --- internal/ingestion/article.go | 5 +---- internal/ingestion/parser.go | 21 ++++++++++++++++++-- internal/storage/push.go | 18 +++++++++++++++-- internal/storage/queries.go | 9 +++++++-- internal/storage/storage_test.go | 20 ++++++++++++++++++- internal/storage/userstate.go | 8 ++++++-- internal/web/push_sender.go | 34 ++++++++++++++++++++++++++++++++ internal/web/pwa.go | 17 ++++++++++++---- internal/web/server.go | 1 + internal/web/static/js/reader.js | 18 +++++++++++++---- internal/web/static/sw.js | 27 +++++++++++-------------- main.go | 4 ++++ 12 files changed, 145 insertions(+), 37 deletions(-) diff --git a/internal/ingestion/article.go b/internal/ingestion/article.go index 0b53c43..2f05c8a 100644 --- a/internal/ingestion/article.go +++ b/internal/ingestion/article.go @@ -414,10 +414,7 @@ func extractBodyText(doc *goquery.Document) string { out = alt } } - if len(out) > MaxBodyChars { - out = out[:MaxBodyChars] - } - return out + return truncateUTF8(out, MaxBodyChars) } // bodyContainerSelectors is the list of selectors we try when
/
diff --git a/internal/ingestion/parser.go b/internal/ingestion/parser.go index 1e6d824..227dc2e 100644 --- a/internal/ingestion/parser.go +++ b/internal/ingestion/parser.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "github.com/mmcdole/gofeed" ext "github.com/mmcdole/gofeed/extensions" @@ -174,8 +175,24 @@ func extractContentText(raw string) string { } s = manyNewlineRe.ReplaceAllString(strings.Join(lines, "\n"), "\n\n") s = strings.TrimSpace(s) - if len(s) > maxContentChars { - s = s[:maxContentChars] + return truncateUTF8(s, maxContentChars) +} + +// truncateUTF8 caps s at max bytes without splitting a multi-byte rune. A plain +// byte slice can leave a partial trailing rune, which is invalid UTF-8; that +// corrupt byte then breaks the RSS content:encoded XML (Go's xml encoder writes +// it raw) and shows as a replacement char in JSON. Trim back to a rune boundary. +func truncateUTF8(s string, max int) string { + if len(s) <= max { + return s + } + s = s[:max] + for len(s) > 0 { + if r, size := utf8.DecodeLastRuneInString(s); r == utf8.RuneError && size <= 1 { + s = s[:len(s)-1] // strip one byte of an incomplete trailing rune + continue + } + break } return s } diff --git a/internal/storage/push.go b/internal/storage/push.go index 76fca70..49b0efa 100644 --- a/internal/storage/push.go +++ b/internal/storage/push.go @@ -34,8 +34,10 @@ func AddPushSubscription(sub, endpoint, p256dh, auth string) error { return nil } -// RemovePushSubscription drops one endpoint. Used both for an explicit opt-out -// and when a push service reports the endpoint is gone (404/410). +// RemovePushSubscription drops one endpoint regardless of owner. Reserved for +// the digest sender's prune path, where a push service has reported the endpoint +// gone (404/410) and there's no caller identity to scope by. User-initiated +// opt-outs must use RemovePushSubscriptionForUser. func RemovePushSubscription(endpoint string) error { _, err := Get().Exec(`DELETE FROM push_subscriptions WHERE endpoint = ?`, endpoint) if err != nil { @@ -44,6 +46,18 @@ func RemovePushSubscription(endpoint string) error { return nil } +// RemovePushSubscriptionForUser drops an endpoint only if it belongs to sub, so +// a signed-in user can't unsubscribe another account's device by presenting its +// endpoint string. A no-op (no matching row) is not an error. +func RemovePushSubscriptionForUser(sub, endpoint string) error { + _, err := Get().Exec( + `DELETE FROM push_subscriptions WHERE endpoint = ? AND user_sub = ?`, endpoint, sub) + if err != nil { + return fmt.Errorf("remove push subscription: %w", err) + } + return nil +} + // ListPushSubscriptions returns every stored subscription, for the digest sender. func ListPushSubscriptions() ([]PushSubscription, error) { rows, err := Get().Query( diff --git a/internal/storage/queries.go b/internal/storage/queries.go index 00dff4d..8b7b16e 100644 --- a/internal/storage/queries.go +++ b/internal/storage/queries.go @@ -46,11 +46,16 @@ func InsertStory(s *Story) error { } // GetStoryReaderText returns the stored full article text and lede for a single -// story, for reader mode. found is false when no story has that id. +// story, for reader mode. found is false when no story has that id. The query +// mirrors the visible-story filter (classified, non-sentinel channel) so the +// public /api/article endpoint can't be enumerated to pull the captured bodies +// of discarded or not-yet-classified stories that never surface in the UI. func GetStoryReaderText(id int64) (content, lede string, found bool, err error) { var c sql.NullString var l sql.NullString - row := Get().QueryRow(`SELECT content, lede FROM stories WHERE id = ?`, id) + row := Get().QueryRow( + `SELECT content, lede FROM stories + WHERE id = ? AND classified = 1 AND channel NOT IN ('_discarded', '_duplicate')`, id) switch err = row.Scan(&c, &l); { case errors.Is(err, sql.ErrNoRows): return "", "", false, nil diff --git a/internal/storage/storage_test.go b/internal/storage/storage_test.go index 9a21852..176d8a2 100644 --- a/internal/storage/storage_test.go +++ b/internal/storage/storage_test.go @@ -64,6 +64,9 @@ func TestInsertStoryAndGUIDSeen(t *testing.T) { func TestGetStoryReaderText(t *testing.T) { setupTestDB(t) + // Reader text is only served for classified, non-sentinel stories — the same + // filter the public /api/article endpoint applies so discarded/unclassified + // bodies can't be pulled by id enumeration. s := &Story{ GUID: "reader-guid", Headline: "Reader Headline", @@ -71,6 +74,8 @@ func TestGetStoryReaderText(t *testing.T) { Content: "First paragraph.\n\nSecond paragraph.", ArticleURL: "https://example.com/reader", Source: "Src", + Channel: "tech", + Classified: true, SeenAt: 1700000000, } if err := InsertStory(s); err != nil { @@ -97,7 +102,7 @@ func TestGetStoryReaderText(t *testing.T) { // A story ingested before content capture (no content) is still found, with // an empty content string so the reader falls back to the lede. - bare := &Story{GUID: "bare-guid", Headline: "H", Lede: "Only a lede.", ArticleURL: "https://a.com", Source: "S", SeenAt: 1} + bare := &Story{GUID: "bare-guid", Headline: "H", Lede: "Only a lede.", ArticleURL: "https://a.com", Source: "S", Channel: "tech", Classified: true, SeenAt: 1} if err := InsertStory(bare); err != nil { t.Fatal(err) } @@ -120,6 +125,19 @@ func TestGetStoryReaderText(t *testing.T) { if _, _, found, err = GetStoryReaderText(999999); err != nil || found { t.Errorf("unknown id: found=%v err=%v, want found=false err=nil", found, err) } + + // A discarded story must not be readable via /api/article, even by direct id. + discarded := &Story{GUID: "disc-guid", Headline: "H", Lede: "hidden", Content: "secret body", ArticleURL: "https://d.com", Source: "S", Channel: "_discarded", Classified: true, SeenAt: 1} + if err := InsertStory(discarded); err != nil { + t.Fatal(err) + } + var discID int64 + if err := Get().QueryRow(`SELECT id FROM stories WHERE guid = ?`, discarded.GUID).Scan(&discID); err != nil { + t.Fatal(err) + } + if _, _, found, err = GetStoryReaderText(discID); err != nil || found { + t.Errorf("discarded story: found=%v err=%v, want found=false", found, err) + } } func TestInsertStoryDuplicateGUID(t *testing.T) { diff --git a/internal/storage/userstate.go b/internal/storage/userstate.go index 875a05d..72e0379 100644 --- a/internal/storage/userstate.go +++ b/internal/storage/userstate.go @@ -121,11 +121,15 @@ func ListBookmarks(sub string, limit, offset int) ([]Story, error) { return out, rows.Err() } -// CountBookmarks returns how many stories the user has bookmarked. +// CountBookmarks returns how many bookmarked stories the user would see on the +// bookmarks page. It applies the same classified filter as ListBookmarks so the +// "N saved" header can't exceed the number of rendered cards. func CountBookmarks(sub string) (int, error) { var n int err := Get().QueryRow( - `SELECT COUNT(*) FROM user_story_state WHERE user_sub = ? AND bookmarked_at IS NOT NULL`, + `SELECT COUNT(*) FROM user_story_state u + JOIN stories s ON s.id = u.story_id + WHERE u.user_sub = ? AND u.bookmarked_at IS NOT NULL AND s.classified = 1`, sub).Scan(&n) return n, err } diff --git a/internal/web/push_sender.go b/internal/web/push_sender.go index 35d6657..c587913 100644 --- a/internal/web/push_sender.go +++ b/internal/web/push_sender.go @@ -5,8 +5,10 @@ import ( "encoding/json" "fmt" "log/slog" + "net/http" "time" + "pete/internal/safehttp" "pete/internal/storage" webpush "github.com/SherClockHolmes/webpush-go" @@ -16,6 +18,11 @@ import ( // pass. Well past MinStories; the digest only needs a count and one headline. const digestScan = 60 +// pushSendTimeout bounds one push delivery. The endpoint is user-supplied, so a +// hostile or dead push service must not be able to wedge the (serial) digest +// loop and starve every other subscriber. +const pushSendTimeout = 15 * time.Second + // runPushSender periodically builds and delivers a "N new stories" digest to // each subscriber, respecting their disabled-sources preference. It's started // only when push is configured. Best-effort throughout: a failed send never @@ -80,6 +87,18 @@ func (s *Server) sendDigests() { count++ } if count < s.cfg.Push.MinStories { + // If a full scan window filled entirely with stories the user has + // hidden, the non-hidden count can stay below the threshold forever + // while the watermark never advances — the same hidden window is + // re-scanned every pass and the subscriber is permanently starved of + // digests. When the window was capped (a genuine glut, not a quiet + // spell), step the watermark past it so the next pass sees fresh + // stories. A non-full window is a real lull; leave it to accumulate. + if len(stories) == digestScan { + if derr := storage.TouchPushSubscription(sub.Endpoint, stories[0].SeenAt); derr != nil { + slog.Error("push: advance watermark past hidden glut failed", "err", derr) + } + } continue } @@ -133,6 +152,11 @@ func (s *Server) sendPush(sub storage.PushSubscription, payload []byte) (gone bo Endpoint: sub.Endpoint, Keys: webpush.Keys{P256dh: sub.P256dh, Auth: sub.Auth}, }, &webpush.Options{ + // The endpoint URL comes from the browser and is attacker-influenceable, + // so deliver through the SSRF-guarded client (blocks loopback/RFC1918/ + // link-local/metadata targets) with a hard timeout — never the library's + // unguarded default http.Client. + HTTPClient: s.pushClient(), Subscriber: s.cfg.Push.Subject, VAPIDPublicKey: s.cfg.Push.VAPIDPublicKey, VAPIDPrivateKey: s.cfg.Push.VAPIDPrivateKey, @@ -189,6 +213,16 @@ func disabledSourcesFor(sub string) map[string]bool { return out } +// pushClient returns the shared SSRF-guarded, timeout-bounded HTTP client used +// for Web Push delivery, building it once on first use. The digest loop is a +// single goroutine, so the lazy init needs no lock. +func (s *Server) pushClient() *http.Client { + if s.pushHTTP == nil { + s.pushHTTP = safehttp.NewClient(pushSendTimeout) + } + return s.pushHTTP +} + // StartPushSender launches the digest loop if push is enabled. Safe to call // unconditionally; it's a no-op when push is off. func (s *Server) StartPushSender(ctx context.Context) { diff --git a/internal/web/pwa.go b/internal/web/pwa.go index 68d9006..cce8b31 100644 --- a/internal/web/pwa.go +++ b/internal/web/pwa.go @@ -5,6 +5,7 @@ import ( "log/slog" "net/http" + "pete/internal/safehttp" "pete/internal/storage" ) @@ -38,6 +39,14 @@ func (s *Server) handlePushSubscribe(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"incomplete subscription"}`, http.StatusBadRequest) return } + // The endpoint is delivered to server-side; reject non-http(s) schemes here so + // a client can't stash a file:// or gopher:// target. The digest sender's + // SSRF-guarded client blocks non-public hosts at dial time, but keeping bad + // endpoints out of the table avoids storing garbage in the first place. + if err := safehttp.ValidateURL(req.Endpoint); err != nil { + http.Error(w, `{"error":"invalid endpoint"}`, http.StatusBadRequest) + return + } if err := storage.AddPushSubscription(u.Sub, req.Endpoint, req.Keys.P256dh, req.Keys.Auth); err != nil { slog.Error("push: subscribe failed", "sub", u.Sub, "err", err) http.Error(w, `{"error":"internal error"}`, http.StatusInternalServerError) @@ -46,9 +55,9 @@ func (s *Server) handlePushSubscribe(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -// handlePushUnsubscribe drops a stored subscription by endpoint. It doesn't -// require the endpoint to belong to the caller beyond being signed in; the -// endpoint is an unguessable capability URL, and dropping a stale one is benign. +// handlePushUnsubscribe drops the caller's own stored subscription by endpoint. +// The delete is scoped to the signed-in user so one account can't remove +// another's subscription by presenting its endpoint string. func (s *Server) handlePushUnsubscribe(w http.ResponseWriter, r *http.Request) { u := s.requireUser(w, r) if u == nil { @@ -64,7 +73,7 @@ func (s *Server) handlePushUnsubscribe(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"missing endpoint"}`, http.StatusBadRequest) return } - if err := storage.RemovePushSubscription(req.Endpoint); err != nil { + if err := storage.RemovePushSubscriptionForUser(u.Sub, req.Endpoint); err != nil { slog.Error("push: unsubscribe failed", "sub", u.Sub, "err", err) http.Error(w, `{"error":"internal error"}`, http.StatusInternalServerError) return diff --git a/internal/web/server.go b/internal/web/server.go index a1db26b..5029d35 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -59,6 +59,7 @@ type Server struct { tpls map[string]*template.Template // keyed by page name (e.g. "index", "channel") auth *Authenticator // nil when sign-in is disabled or unavailable adminSubs map[string]bool // OIDC subjects allowed to view /status + pushHTTP *http.Client // SSRF-guarded client for Web Push delivery; built lazily by pushClient() // Daily-rotated salt for the privacy-preserving unique-visitor estimate. // Guarded by metricsMu; never persisted (see metrics.go). diff --git a/internal/web/static/js/reader.js b/internal/web/static/js/reader.js index 645192f..dfb538a 100644 --- a/internal/web/static/js/reader.js +++ b/internal/web/static/js/reader.js @@ -143,10 +143,20 @@ serverRead[id] = 1; readSet[id] = 1; paintCard(id, true); }); (data.bookmarked || []).forEach(function (id) { setBookmarkQuiet(id, true); }); - // Push up reads made on this device before the account knew them. - ids.forEach(function (id) { - if (readSet[id] && !serverRead[id]) postState("/api/read", { id: Number(id), read: true }); - }); + // Migrate device-local reads the account doesn't have yet — but only + // once per device. After the first sync the server is authoritative, so + // a story the user later marks unread on another device stays unread + // instead of being perpetually resurrected from this device's stale + // local set on every page load. + var MIGRATED_KEY = "pete.readMigrated.v1"; + var migrated = false; + try { migrated = localStorage.getItem(MIGRATED_KEY) === "1"; } catch (e) {} + if (!migrated) { + ids.forEach(function (id) { + if (readSet[id] && !serverRead[id]) postState("/api/read", { id: Number(id), read: true }); + }); + try { localStorage.setItem(MIGRATED_KEY, "1"); } catch (e) {} + } saveRead(readSet); }) .catch(function () {}); diff --git a/internal/web/static/sw.js b/internal/web/static/sw.js index 47ba3a5..74964f3 100644 --- a/internal/web/static/sw.js +++ b/internal/web/static/sw.js @@ -4,7 +4,7 @@ // // Bump CACHE_VERSION whenever the precached shell assets change; activate() // drops every cache that doesn't match the current version. -var CACHE_VERSION = "v1"; +var CACHE_VERSION = "v2"; var SHELL_CACHE = "pete-shell-" + CACHE_VERSION; var RUNTIME_CACHE = "pete-runtime-" + CACHE_VERSION; @@ -132,23 +132,18 @@ self.addEventListener("fetch", function (event) { return; } - // Page navigations: network-first, fall back to a cached copy of the same page, - // then to the offline card. Successful HTML is cached so revisits work offline. + // Page navigations: network-only, falling back to the offline card when the + // network is unreachable. We deliberately do NOT cache HTML responses: pages + // are personalized (they embed the signed-in user's name/email and a "For you" + // rail), and the runtime cache is shared across everyone who uses this + // installed PWA. Caching a navigation would let a signed-out visitor — or a + // second person on the same device — be served the previous user's identity + // and personalized stories offline. Offline reading still works: the reader + // fetches cached /api/article JSON on top of the cached static shell. if (req.mode === "navigate") { event.respondWith( - fetch(req).then(function (res) { - if (res && res.ok) { - var copy = res.clone(); - caches.open(RUNTIME_CACHE).then(function (cache) { - cache.put(req, copy); - trimCache(RUNTIME_CACHE, RUNTIME_MAX); - }); - } - return res; - }).catch(function () { - return caches.match(req).then(function (hit) { - return hit || offlineFallback(); - }); + fetch(req).catch(function () { + return offlineFallback(); }) ); return; diff --git a/main.go b/main.go index 09f07a4..22bd476 100644 --- a/main.go +++ b/main.go @@ -377,6 +377,10 @@ func runLocal(cfg *config.Config) { os.Exit(1) } go ws.Start(ctx) + // Push only needs the web auth layer, not Matrix, so a web-only deployment + // still runs the digest sender — otherwise subscriptions accumulate but no + // digest ever fires. + ws.StartPushSender(ctx) slog.Info("local: web UI listening", "addr", cfg.Web.ListenAddr) storage.RunMaintenance()