fix: code principles audit — correctness, security, testability

- Add rows.Err() checks after all scan loops (entities, tags, resolve) - Surface time.Parse errors instead of silently discarding - Extract entityRow scan helper to eliminate Get/List duplication - Cap request body at 1MB via MaxBytesReader - Stop leaking internal errors to API clients (log server-side only) - Block javascript: URIs in link card open button (XSS) - Fix all go vet failures in api_test.go (unchecked http errors) - Add tests for display package, generateCardData, absorb-source-card - Run go mod tidy to fix direct/indirect dep markers
2026-05-14 17:41:30 -04:00
parent e708ea5c13
commit 6278cb1022
12 changed files with 500 additions and 104 deletions
@@ -141,18 +141,12 @@ func (s *Store) Create(e *Entity) error {

 func (s *Store) Get(id string) (*Entity, error) {
 	e := &Entity{}
-	var createdAt, modifiedAt string
-	var completedAt, deletedAt, lastUsedAt sql.NullString
-	var timeAnchor, cardType, cardData sql.NullString
-	var pinned int
+	row := newEntityRow()

 	err := s.db.QueryRow(`
 		SELECT id, created_at, modified_at, body, glyph, time_anchor,
 			completed_at, pinned, deleted_at, card_type, card_data, use_count, last_used_at
-		FROM entities WHERE id = ?`, id).Scan(
-		&e.ID, &createdAt, &modifiedAt, &e.Body, &e.Glyph, &timeAnchor,
-		&completedAt, &pinned, &deletedAt, &cardType, &cardData, &e.UseCount, &lastUsedAt,
-	)
+		FROM entities WHERE id = ?`, id).Scan(row.ptrs(e)...)
 	if err == sql.ErrNoRows {
 		return nil, ErrNotFound
 	}
@@ -160,15 +154,9 @@ func (s *Store) Get(id string) (*Entity, error) {
 		return nil, err
 	}

-	e.CreatedAt, _ = time.Parse(time.RFC3339, createdAt)
-	e.ModifiedAt, _ = time.Parse(time.RFC3339, modifiedAt)
-	e.TimeAnchor = nullToPtr(timeAnchor)
-	e.CompletedAt = parseTimePtr(completedAt)
-	e.Pinned = pinned != 0
-	e.DeletedAt = parseTimePtr(deletedAt)
-	e.CardType = nullToCardType(cardType)
-	e.CardData = nullToPtr(cardData)
-	e.LastUsedAt = parseTimePtr(lastUsedAt)
+	if err := row.apply(e); err != nil {
+		return nil, fmt.Errorf("scan entity %s: %w", id, err)
+	}

 	tags, err := s.loadTags(id)
 	if err != nil {
@@ -253,31 +241,18 @@ func (s *Store) List(params ListParams) ([]*Entity, error) {
 	var entities []*Entity
 	for rows.Next() {
 		e := &Entity{}
-		var createdAt, modifiedAt string
-		var completedAt, deletedAt, lastUsedAt sql.NullString
-		var timeAnchor, cardType, cardData sql.NullString
-		var pinned int
-
-		if err := rows.Scan(
-			&e.ID, &createdAt, &modifiedAt, &e.Body, &e.Glyph, &timeAnchor,
-			&completedAt, &pinned, &deletedAt, &cardType, &cardData,
-			&e.UseCount, &lastUsedAt,
-		); err != nil {
+		row := newEntityRow()
+		if err := rows.Scan(row.ptrs(e)...); err != nil {
+			return nil, err
+		}
+		if err := row.apply(e); err != nil {
 			return nil, err
 		}
-
-		e.CreatedAt, _ = time.Parse(time.RFC3339, createdAt)
-		e.ModifiedAt, _ = time.Parse(time.RFC3339, modifiedAt)
-		e.TimeAnchor = nullToPtr(timeAnchor)
-		e.CompletedAt = parseTimePtr(completedAt)
-		e.Pinned = pinned != 0
-		e.DeletedAt = parseTimePtr(deletedAt)
-		e.CardType = nullToCardType(cardType)
-		e.CardData = nullToPtr(cardData)
-		e.LastUsedAt = parseTimePtr(lastUsedAt)
-
 		entities = append(entities, e)
 	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}

 	if err := s.batchLoadTags(entities); err != nil {
 		return nil, err
@@ -502,6 +477,9 @@ func (s *Store) Resolve(prefix string) (string, error) {
 		}
 		ids = append(ids, id)
 	}
+	if err := rows.Err(); err != nil {
+		return "", err
+	}

 	switch len(ids) {
 	case 0:
@@ -513,6 +491,43 @@ func (s *Store) Resolve(prefix string) (string, error) {
 	}
 }

+// entityRow holds intermediate scan values for a single entity row.
+// Both Get and List use this to avoid duplicating the 14-var scan + mapping logic.
+type entityRow struct {
+	createdAt, modifiedAt              string
+	completedAt, deletedAt, lastUsedAt sql.NullString
+	timeAnchor, cardType, cardData     sql.NullString
+	pinned                             int
+}
+
+func newEntityRow() *entityRow { return &entityRow{} }
+
+func (r *entityRow) ptrs(e *Entity) []any {
+	return []any{
+		&e.ID, &r.createdAt, &r.modifiedAt, &e.Body, &e.Glyph, &r.timeAnchor,
+		&r.completedAt, &r.pinned, &r.deletedAt, &r.cardType, &r.cardData,
+		&e.UseCount, &r.lastUsedAt,
+	}
+}
+
+func (r *entityRow) apply(e *Entity) error {
+	var err error
+	if e.CreatedAt, err = time.Parse(time.RFC3339, r.createdAt); err != nil {
+		return fmt.Errorf("created_at: %w", err)
+	}
+	if e.ModifiedAt, err = time.Parse(time.RFC3339, r.modifiedAt); err != nil {
+		return fmt.Errorf("modified_at: %w", err)
+	}
+	e.TimeAnchor = nullToPtr(r.timeAnchor)
+	e.CompletedAt = parseTimePtr(r.completedAt)
+	e.Pinned = r.pinned != 0
+	e.DeletedAt = parseTimePtr(r.deletedAt)
+	e.CardType = nullToCardType(r.cardType)
+	e.CardData = nullToPtr(r.cardData)
+	e.LastUsedAt = parseTimePtr(r.lastUsedAt)
+	return nil
+}
+
 // helpers

 func (s *Store) batchLoadTags(entities []*Entity) error {
@@ -568,6 +583,9 @@ func (s *Store) loadTags(entityID string) ([]string, error) {
 		}
 		tags = append(tags, tag)
 	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
 	if tags == nil {
 		tags = []string{}
 	}
@@ -631,11 +649,13 @@ func boolToInt(b bool) int {
 	return 0
 }

-func (e *Entity) CardDataJSON() map[string]interface{} {
+func (e *Entity) CardDataJSON() (map[string]interface{}, error) {
 	if e.CardData == nil {
-		return nil
+		return nil, nil
 	}
 	var m map[string]interface{}
-	json.Unmarshal([]byte(*e.CardData), &m)
-	return m
+	if err := json.Unmarshal([]byte(*e.CardData), &m); err != nil {
+		return nil, fmt.Errorf("card_data: %w", err)
+	}
+	return m, nil
 }
@@ -441,3 +441,34 @@ func TestResolve_NotFound(t *testing.T) {
 		t.Errorf("expected ErrNotFound, got %v", err)
 	}
 }
+
+func TestAbsorb_SourceIsCard(t *testing.T) {
+	s := testStore(t)
+	target := &Entity{Body: "target", Glyph: GlyphNote, Tags: []string{"a"}}
+	s.Create(target)
+
+	source := &Entity{Body: "source", Glyph: GlyphNote}
+	s.Create(source)
+	s.Promote(source.ID, CardSnippet, nil)
+	s.IncrementUse(source.ID)
+
+	if err := s.Absorb(target.ID, source.ID); err != nil {
+		t.Fatal(err)
+	}
+
+	got, _ := s.Get(target.ID)
+	if got.Body != "target\nsource" {
+		t.Errorf("merged body: %q", got.Body)
+	}
+
+	src, _ := s.Get(source.ID)
+	if src.CardType != nil {
+		t.Error("source card_type should be cleared after absorb")
+	}
+	if src.UseCount != 0 {
+		t.Errorf("source use_count should be reset, got %d", src.UseCount)
+	}
+	if src.DeletedAt == nil {
+		t.Error("source should be soft-deleted")
+	}
+}
@@ -26,6 +26,9 @@ func (s *Store) ListTags() ([]TagCount, error) {
 		}
 		tags = append(tags, tc)
 	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
 	if tags == nil {
 		tags = []TagCount{}
 	}