fix(api): scope icon lookup by user in icons endpoint

The GET /v1/icons/{iconID} endpoint fetched icons by their internal
numeric identifier without any user scoping, allowing any authenticated
user to enumerate icon IDs and read favicon data associated with feeds
owned by other users on the same instance.

Rename IconByID to IconByUserAndIconID and gate the lookup on an EXISTS
check against feeds owned by the requesting user, so unauthorized icon
IDs return 404. Add integration tests covering cross-user access and
inexisting icon IDs.

internal/api/api_integration_test.go

@@ -2074,6 +2074,74 @@ func TestGetFeedIconWithInexistingFeedID(t *testing.T) {
 	}
 }
 
+func TestGetIconWithInexistingIconID(t *testing.T) {
+	t.Parallel()
+
+	testConfig := newIntegrationTestConfig()
+	if !testConfig.isConfigured() {
+		t.Skip(skipIntegrationTestsMessage)
+	}
+
+	client := miniflux.NewClient(testConfig.testBaseURL, testConfig.testAdminUsername, testConfig.testAdminPassword)
+	_, err := client.Icon(123456789)
+	if !errors.Is(err, miniflux.ErrNotFound) {
+		t.Fatalf(`Fetching an inexisting icon should return a "not found" error, got %v`, err)
+	}
+}
+
+func TestGetIconByIconIDFromAnotherUser(t *testing.T) {
+	t.Parallel()
+
+	testConfig := newIntegrationTestConfig()
+	if !testConfig.isConfigured() {
+		t.Skip(skipIntegrationTestsMessage)
+	}
+
+	adminClient := miniflux.NewClient(testConfig.testBaseURL, testConfig.testAdminUsername, testConfig.testAdminPassword)
+
+	// The owner subscribes to a feed, which fetches and stores its icon.
+	ownerUser, err := adminClient.CreateUser(testConfig.genRandomUsername(), testConfig.testRegularPassword, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer adminClient.DeleteUser(ownerUser.ID)
+
+	ownerClient := miniflux.NewClient(testConfig.testBaseURL, ownerUser.Username, testConfig.testRegularPassword)
+
+	feedID, err := ownerClient.CreateFeed(&miniflux.FeedCreationRequest{
+		FeedURL: testConfig.testFeedURL,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	ownerIcon, err := ownerClient.FeedIcon(feedID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ownerIcon == nil {
+		t.Fatalf(`Invalid icon, got nil`)
+	}
+
+	// The owner can fetch its own icon by icon ID.
+	if _, err := ownerClient.Icon(ownerIcon.ID); err != nil {
+		t.Fatalf(`The owner should be able to fetch its own icon, got %v`, err)
+	}
+
+	// Another user without access to that feed must not be able to fetch the icon.
+	otherUser, err := adminClient.CreateUser(testConfig.genRandomUsername(), testConfig.testRegularPassword, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer adminClient.DeleteUser(otherUser.ID)
+
+	otherClient := miniflux.NewClient(testConfig.testBaseURL, otherUser.Username, testConfig.testRegularPassword)
+
+	if _, err := otherClient.Icon(ownerIcon.ID); !errors.Is(err, miniflux.ErrNotFound) {
+		t.Fatalf(`Fetching an icon owned by another user should return a "not found" error, got %v`, err)
+	}
+}
+
 func TestGetFeedsEndpoint(t *testing.T) {
 	t.Parallel()
 

internal/api/icon_handlers.go

@@ -43,7 +43,7 @@ func (h *handler) getIconByIconIDHandler(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	icon, err := h.store.IconByID(iconID)
+	icon, err := h.store.IconByUserAndIconID(request.UserID(r), iconID)
 	if err != nil {
 		response.JSONServerError(w, r, err)
 		return

internal/storage/icon.go

@@ -21,24 +21,33 @@ func (s *Storage) HasFeedIcon(feedID int64) bool {
 	return result
 }
 
-// IconByID fetches a single icon by its internal identifier, returning nil when it is not found.
-func (s *Storage) IconByID(iconID int64) (*model.Icon, error) {
+// IconByUserAndIconID fetches a single icon by its internal identifier, scoped
+// to the given user. It returns nil when the icon does not exist or is not
+// associated with any feed owned by the user.
+func (s *Storage) IconByUserAndIconID(userID, iconID int64) (*model.Icon, error) {
 	var icon model.Icon
 	query := `
 		SELECT
-			id,
-			hash,
-			mime_type,
-			content,
-			external_id
-		FROM icons
-		WHERE id=$1`
-	err := s.db.QueryRow(query, iconID).Scan(&icon.ID, &icon.Hash, &icon.MimeType, &icon.Content, &icon.ExternalID)
+			i.id,
+			i.hash,
+			i.mime_type,
+			i.content,
+			i.external_id
+		FROM icons AS i
+		WHERE i.id = $2
+			AND EXISTS (
+				SELECT 1
+				FROM feeds AS f
+				INNER JOIN feed_icons AS fi ON fi.feed_id = f.id
+				WHERE f.user_id = $1
+					AND fi.icon_id = $2
+			)`
+	err := s.db.QueryRow(query, userID, iconID).Scan(&icon.ID, &icon.Hash, &icon.MimeType, &icon.Content, &icon.ExternalID)
 	switch {
 	case errors.Is(err, sql.ErrNoRows):
 		return nil, nil
 	case err != nil:
-		return nil, fmt.Errorf("store: cannot load icon id=%d: %w", iconID, err)
+		return nil, fmt.Errorf("store: cannot load icon id=%d for user_id=%d: %w", iconID, userID, err)
 	default:
 		return &icon, nil
 	}