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.
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user