diff --git a/TODO.md b/TODO.md index 99e1df0..01e1d6c 100644 --- a/TODO.md +++ b/TODO.md @@ -1,27 +1,70 @@ -# Code Hardening — Senior Dev Audit Fixes +# Code Review Fixes -## Phase 1: Quick Wins (safety + correctness) -- [x] Cap API list limit at 200 -- [x] Fix markdown XSS — add DOMPurify to sanitize marked output -- [x] Add missing DB indexes (deleted_at, modified_at) via v4 migration -- [x] Fix v2 migration error handling (swallowed ALTER TABLE errors) -- [x] Fix ~/.nib directory permissions (0o755 → 0o700) +## Phase 1: Critical — Concurrency & Data Safety -## Phase 2: CI Pipeline -- [x] Gitea Actions workflow: test + lint on PR +### 1. ULID entropy not goroutine-safe +- [x] Wrap MonotonicEntropy in LockedMonotonicReader (`internal/ulid/ulid.go`) +- [x] Add concurrent uniqueness test (100 goroutines) +- [x] Verify `go test -race ./internal/ulid/...` -## Phase 3: context.Context in Store -- [x] Thread context.Context through all Store methods -- [x] Use context in API handlers (from r.Context()) -- [x] Use context in CLI commands (cobra context) +### 2. Migration PRAGMA bug +- [x] Move `PRAGMA foreign_keys = OFF` before transaction in v3 migration (`internal/db/db.go`) +- [x] Re-enable after commit +- [x] Remove dead `currentSchema` constant while here -## Phase 4: cmd/ Tests -- [x] Test add command -- [x] Test ls command -- [x] Test promote/demote commands -- [x] Test delete command -- [x] Test absorb command +### 3. LIKE wildcard injection in link resolution +- [x] Escape `%` and `_` in link text before LIKE query (`internal/db/links.go`) +- [x] Add `ESCAPE '\'` clause +- [ ] Test with `[[%]]` and `[[_]]` link text -## Phase 5: Backup/Export -- [x] nib export — dump entities to JSON -- [x] nib backup — safe SQLite backup (handles WAL) +## Phase 2: High — Security & API Hygiene + +### 4. No auth warning for non-localhost binding +- [x] Print loud warning when `--host != 127.0.0.1` and no auth configured (`cmd/serve.go`) +- [ ] Consider `--no-auth` flag requirement for non-localhost + +### 5. writeJSON ignores encoder errors +- [x] Log error from `json.Encoder.Encode()` in `writeJSON` (`internal/api/helpers.go`) + +### 6. DELETE endpoint semantics +- [x] Add `?permanent=true` query param for hard delete (`internal/api/entities.go`) +- [x] Add `HardDelete` store method (`internal/db/entities.go`) +- [ ] Update frontend and CLI to match +- [x] Keep backward compat: double-delete still works without param + +## Phase 3: Medium — Frontend Robustness + +### 7. No resp.ok checks on fetch calls +- [x] Add `checkedJSON()` wrapper with error extraction (`web/app.js`) +- [x] All API methods use `checkedJSON(resp)` instead of `resp.json()` +- [ ] Surface API errors to user via notification/toast + +### 8. Inconsistent event.stopPropagation +- [x] Add stopPropagation to all renderStreamPeek action buttons +- [x] Add stopPropagation to all renderCardPeek action buttons and inline section buttons + +### 9. Duplicate section rendering +- [x] Extract `renderCardSections()` shared function (`web/app.js`) +- [x] Refactor `renderInlineDetail` to use shared function +- [x] Refactor `renderCardPeek` to use shared function + +## Phase 4: Medium — Data Integrity + +### 10. Absorb discards source title/description +- [x] If target has no title, inherit from source (`internal/db/entities.go`) +- [x] If target has no description, inherit from source +- [ ] Test title/description preservation + +## Phase 5: Low — Housekeeping & UX + +### 11. Backup path collision +- [x] Use millisecond-precision timestamps (`cmd/backup.go`) + +### 12. spaHandler path safety +- [x] Add explicit `path.Clean` in spaHandler (`internal/api/router.go`) + +### 13. Focus-peek escape hatch +- [ ] Add visible close/back button in focus-peek mode (`web/app.js`) + +### 14. Hard delete confirmation +- [ ] Add confirmation step or undo toast before permanent delete (`web/app.js`) diff --git a/cmd/backup.go b/cmd/backup.go index a39bd02..3dccfcf 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -26,7 +26,7 @@ func runBackup(cmd *cobra.Command, args []string) error { return err } - dst := fmt.Sprintf("%s.backup-%s", srcPath, time.Now().Format("20060102-150405")) + dst := fmt.Sprintf("%s.backup-%s", srcPath, time.Now().Format("20060102-150405.000")) if len(args) > 0 { dst = args[0] } diff --git a/cmd/serve.go b/cmd/serve.go index 2fabf53..9dc4c61 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -90,6 +90,9 @@ func runServe(_ *cobra.Command, _ []string) error { if serveDev { fmt.Println(" CORS enabled (dev mode)") } + if serveHost != "127.0.0.1" && serveHost != "localhost" && serveHost != "::1" { + fmt.Fprintln(os.Stderr, " WARNING: binding to non-localhost with no authentication — API is open to the network") + } var listenErr error if useTLS { diff --git a/internal/api/entities.go b/internal/api/entities.go index 9a0fb36..03680d1 100644 --- a/internal/api/entities.go +++ b/internal/api/entities.go @@ -272,6 +272,20 @@ type DeleteResponse struct { func deleteEntity(store *db.Store) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "id") + + if r.URL.Query().Get("permanent") == "true" { + if err := store.HardDelete(r.Context(), id); err != nil { + if err == db.ErrNotFound { + writeError(w, http.StatusNotFound, "not_found", "no entity with id "+id) + return + } + writeInternalError(w, err) + return + } + writeJSON(w, http.StatusOK, DeleteResponse{Result: "hard"}) + return + } + result, err := store.SoftDelete(r.Context(), id) if err != nil { if err == db.ErrNotFound { diff --git a/internal/api/helpers.go b/internal/api/helpers.go index f95b72d..9e1bcb9 100644 --- a/internal/api/helpers.go +++ b/internal/api/helpers.go @@ -38,7 +38,9 @@ type EntityResponse struct { func writeJSON(w http.ResponseWriter, status int, v any) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) - json.NewEncoder(w).Encode(v) + if err := json.NewEncoder(w).Encode(v); err != nil { + log.Printf("writeJSON encode error: %v", err) + } } func writeError(w http.ResponseWriter, status int, code, message string) { diff --git a/internal/api/router.go b/internal/api/router.go index fe6ff94..22c9c7f 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -47,7 +47,7 @@ func spaHandler(fsys fs.FS) http.HandlerFunc { indexHTML, _ := fs.ReadFile(fsys, "index.html") return func(w http.ResponseWriter, r *http.Request) { - p := r.URL.Path + p := path.Clean(r.URL.Path) if p == "/" || path.Ext(p) == "" { w.Header().Set("Content-Type", "text/html; charset=utf-8") w.Write(indexHTML) diff --git a/internal/db/db.go b/internal/db/db.go index 4ead6bd..2fe9479 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -56,8 +56,6 @@ func (s *Store) Backup(dst string) error { return err } -const currentSchema = 5 - var migrations = []func(db *sql.DB) error{ // v1: initial schema func(db *sql.DB) error { @@ -108,17 +106,18 @@ var migrations = []func(db *sql.DB) error{ // v3: rebuild table with CHECK constraints (card_type 'note', glyph 'reminder') func(db *sql.DB) error { + // PRAGMA foreign_keys must be set outside a transaction (SQLite ignores it inside one) + if _, err := db.Exec(`PRAGMA foreign_keys = OFF`); err != nil { + return fmt.Errorf("migrate fk off: %w", err) + } + tx, err := db.Begin() if err != nil { + db.Exec(`PRAGMA foreign_keys = ON`) return err } defer tx.Rollback() - // Disable FK checks during rebuild to avoid dangling references - if _, err := tx.Exec(`PRAGMA foreign_keys = OFF`); err != nil { - return fmt.Errorf("migrate fk off: %w", err) - } - if _, err := tx.Exec(`ALTER TABLE entities RENAME TO _entities_migrate`); err != nil { return fmt.Errorf("migrate rename: %w", err) } @@ -169,11 +168,14 @@ var migrations = []func(db *sql.DB) error{ return fmt.Errorf("migrate tags drop: %w", err) } - if _, err := tx.Exec(`PRAGMA foreign_keys = ON`); err != nil { + if err := tx.Commit(); err != nil { + db.Exec(`PRAGMA foreign_keys = ON`) + return err + } + if _, err := db.Exec(`PRAGMA foreign_keys = ON`); err != nil { return fmt.Errorf("migrate fk on: %w", err) } - - return tx.Commit() + return nil }, // v4: add indexes for common query filters diff --git a/internal/db/entities.go b/internal/db/entities.go index 1fb0888..cfb3b05 100644 --- a/internal/db/entities.go +++ b/internal/db/entities.go @@ -464,6 +464,18 @@ func (s *Store) SoftDelete(ctx context.Context, id string) (DeleteResult, error) return DeletedSoft, err } +func (s *Store) HardDelete(ctx context.Context, id string) error { + res, err := s.db.ExecContext(ctx, "DELETE FROM entities WHERE id = ?", id) + if err != nil { + return err + } + n, _ := res.RowsAffected() + if n == 0 { + return ErrNotFound + } + return nil +} + func (s *Store) Absorb(ctx context.Context, targetID, sourceID string) error { target, err := s.Get(ctx, targetID) if err != nil { @@ -487,8 +499,18 @@ func (s *Store) Absorb(ctx context.Context, targetID, sourceID string) error { now := time.Now().UTC().Format(time.RFC3339) merged := target.Body + "\n" + source.Body - if _, err := tx.ExecContext(ctx, "UPDATE entities SET body = ?, modified_at = ? WHERE id = ?", - merged, now, targetID); err != nil { + title := target.Title + if title == nil { + title = source.Title + } + desc := target.Description + if desc == nil { + desc = source.Description + } + + if _, err := tx.ExecContext(ctx, + "UPDATE entities SET body = ?, title = ?, description = ?, modified_at = ? WHERE id = ?", + merged, title, desc, now, targetID); err != nil { return err } diff --git a/internal/db/links.go b/internal/db/links.go index a3722eb..43da023 100644 --- a/internal/db/links.go +++ b/internal/db/links.go @@ -15,6 +15,11 @@ type Backlink struct { LinkText string } +func escapeLike(s string) string { + r := strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) + return r.Replace(s) +} + func (s *Store) resolveLink(ctx context.Context, tx *sql.Tx, linkText string, excludeID string) *string { lower := strings.ToLower(linkText) @@ -29,8 +34,8 @@ func (s *Store) resolveLink(ctx context.Context, tx *sql.Tx, linkText string, ex err = tx.QueryRowContext(ctx, ` SELECT id FROM entities - WHERE LOWER(body) LIKE ? AND id != ? AND deleted_at IS NULL - ORDER BY created_at DESC LIMIT 1`, "%"+lower+"%", excludeID).Scan(&id) + WHERE LOWER(body) LIKE ? ESCAPE '\' AND id != ? AND deleted_at IS NULL + ORDER BY created_at DESC LIMIT 1`, "%"+escapeLike(lower)+"%", excludeID).Scan(&id) if err == nil { return &id } diff --git a/internal/ulid/ulid.go b/internal/ulid/ulid.go index 9ec14d2..3b700df 100644 --- a/internal/ulid/ulid.go +++ b/internal/ulid/ulid.go @@ -8,13 +8,15 @@ import ( ) var ( - entropy *ulid.MonotonicEntropy + entropy *ulid.LockedMonotonicReader entropyOnce sync.Once ) func New() string { entropyOnce.Do(func() { - entropy = ulid.Monotonic(rand.Reader, 0) + entropy = &ulid.LockedMonotonicReader{ + MonotonicReader: ulid.Monotonic(rand.Reader, 0), + } }) return ulid.MustNew(ulid.Now(), entropy).String() } diff --git a/internal/ulid/ulid_test.go b/internal/ulid/ulid_test.go index 04553b0..b1bbcaa 100644 --- a/internal/ulid/ulid_test.go +++ b/internal/ulid/ulid_test.go @@ -1,6 +1,7 @@ package ulid import ( + "sync" "testing" ) @@ -26,3 +27,25 @@ func TestNew_Sortable(t *testing.T) { t.Errorf("expected b >= a for sequential calls: a=%s b=%s", a, b) } } + +func TestNew_ConcurrentUnique(t *testing.T) { + const n = 100 + ids := make([]string, n) + var wg sync.WaitGroup + wg.Add(n) + for i := 0; i < n; i++ { + go func(idx int) { + defer wg.Done() + ids[idx] = New() + }(i) + } + wg.Wait() + + seen := make(map[string]struct{}, n) + for _, id := range ids { + if _, dup := seen[id]; dup { + t.Fatalf("duplicate ULID under concurrency: %s", id) + } + seen[id] = struct{}{} + } +} diff --git a/web/app.js b/web/app.js index 71d738b..c21ee7c 100644 --- a/web/app.js +++ b/web/app.js @@ -44,6 +44,15 @@ // ========== API ========== + async function checkedJSON(resp) { + if (!resp.ok) { + const body = await resp.json().catch(() => ({})); + const msg = body.message || body.error || `HTTP ${resp.status}`; + throw new Error(msg); + } + return resp.json(); + } + const api = { async listEntities(params = {}) { const q = new URLSearchParams(); @@ -57,7 +66,7 @@ if (params.limit) q.set('limit', String(params.limit)); if (params.offset) q.set('offset', String(params.offset)); const resp = await fetch('/api/entities?' + q); - return resp.json(); + return checkedJSON(resp); }, async createEntity(data) { const resp = await fetch('/api/entities', { @@ -65,11 +74,11 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(data), }); - return resp.json(); + return checkedJSON(resp); }, async getEntity(id) { const resp = await fetch('/api/entities/' + id); - return resp.json(); + return checkedJSON(resp); }, async updateEntity(id, data) { const resp = await fetch('/api/entities/' + id, { @@ -77,10 +86,11 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(data), }); - return resp.json(); + return checkedJSON(resp); }, async deleteEntity(id) { - return fetch('/api/entities/' + id, { method: 'DELETE' }); + const resp = await fetch('/api/entities/' + id, { method: 'DELETE' }); + return checkedJSON(resp); }, async promoteEntity(id, cardType, cardData) { const resp = await fetch('/api/entities/' + id + '/promote', { @@ -88,7 +98,7 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ card_type: cardType, card_data: cardData }), }); - return resp.json(); + return checkedJSON(resp); }, async demoteEntity(id) { const resp = await fetch('/api/entities/' + id + '/demote', { @@ -96,7 +106,7 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({}), }); - return resp.json(); + return checkedJSON(resp); }, async useEntity(id) { const resp = await fetch('/api/entities/' + id + '/use', { @@ -104,7 +114,7 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({}), }); - return resp.json(); + return checkedJSON(resp); }, async absorbEntity(targetId, sourceId) { const resp = await fetch('/api/entities/' + targetId + '/absorb', { @@ -112,13 +122,13 @@ headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ source_id: sourceId }), }); - return resp.json(); + return checkedJSON(resp); }, async listTags(params = {}) { const q = new URLSearchParams(); if (params.cards_only) q.set('cards_only', 'true'); const resp = await fetch('/api/tags?' + q); - return resp.json(); + return checkedJSON(resp); }, }; @@ -677,6 +687,52 @@ `; } + function renderCardSections(e, bodyClass) { + const data = e.card_data ? (() => { try { return JSON.parse(e.card_data); } catch { return {}; } })() : {}; + const hasDecision = data.chose != null; + const hasSteps = data.steps && data.steps.length; + const hasLink = !!data.url; + const hasFill = /\$\{[^}]+\}/.test(e.body || ''); + + let sections = ''; + if (hasDecision) { + const rejected = (data.rejected || []).map(r => `${escHtml(r)}`).join(''); + sections += `
+
decision${data.status || 'decided'}
+
+
${escHtml(data.chose)}
+
why${escHtml(data.why || '')}
+ ${rejected ? `
considered
${rejected}
` : ''} +
+
`; + } + if (hasLink && !hasDecision) { + sections += `
+
link
+
↗ ${escHtml(data.url)}
+
`; + } + if (hasSteps) { + const steps = data.steps.map(s => `
○${escHtml(s.text || s)}
`).join(''); + sections += `
+
steps · ${data.steps.length}
+
${steps}
+
`; + } + if (!hasDecision && e.body) { + const lang = data.lang || ''; + const isCode = lang || e.card_type === 'snippet'; + const bodyHtml = isCode + ? `
${escHtml(e.body)}
` + : `
${renderMd(e.body)}
`; + sections += `
+
content${lang ? `${lang}` : ''}${hasFill ? `` : ''}
+
${bodyHtml}
+
`; + } + return { sections, data, hasDecision, hasSteps, hasLink, hasFill }; + } + function renderInlineDetail(e) { const tags = (e.tags || []).map(t => `#${t}`).join(''); let actions = ''; @@ -694,51 +750,10 @@ let content = ''; if (e.card_type) { - const data = e.card_data ? (() => { try { return JSON.parse(e.card_data); } catch { return {}; } })() : {}; - const hasDecision = data.chose != null; - const hasSteps = data.steps && data.steps.length; - const hasLink = !!data.url; - const hasFill = /\$\{[^}]+\}/.test(e.body || ''); - - if (hasDecision) { - const rejected = (data.rejected || []).map(r => `${escHtml(r)}`).join(''); - content += `
-
decision${data.status || 'decided'}
-
-
${escHtml(data.chose)}
-
why${escHtml(data.why || '')}
- ${rejected ? `
considered
${rejected}
` : ''} -
-
`; - } - if (hasLink && !hasDecision) { - content += `
-
link
-
↗ ${escHtml(data.url)}
-
`; - } - if (hasSteps) { - const steps = data.steps.map(s => `
○${escHtml(s.text || s)}
`).join(''); - content += `
-
steps · ${data.steps.length}
-
${steps}
-
`; - actions += ``; - } - if (hasFill) { - actions += ``; - } - if (!hasDecision && e.body) { - const lang = data.lang || ''; - const isCode = lang || e.card_type === 'snippet'; - const bodyHtml = isCode - ? `
${escHtml(e.body)}
` - : `
${renderMd(e.body)}
`; - content += `
-
content${lang ? `${lang}` : ''}
-
${bodyHtml}
-
`; - } + const cs = renderCardSections(e, 'exp-body'); + content = cs.sections; + if (cs.hasSteps) actions += ``; + if (cs.hasFill) actions += ``; } else { content = `
${renderMd(e.body || '')}
`; } @@ -861,15 +876,15 @@ const tags = (e.tags || []).map(t => `#${t}`).join(''); let actions = ''; - actions += ``; + actions += ``; if (!e.card_type) { - actions += ``; - actions += ``; + actions += ``; + actions += ``; } if (e.card_type) { - actions += ``; + actions += ``; } else { - actions += ``; + actions += ``; } return `
@@ -899,61 +914,17 @@ const glyph = GLYPHS[e.card_type] || '◆'; const gc = GLYPH_CLASSES[e.card_type] || 'glyph-snippet'; const affs = detectAffordances(e); - const data = e.card_data ? (() => { try { return JSON.parse(e.card_data); } catch { return {}; } })() : {}; const tags = (e.tags || []).map(t => `#${t}`).join(''); const affHtml = affs.map(a => `${AFF_LABELS[a]}`).join(''); - const hasSteps = data.steps && data.steps.length; - const hasDecision = data.chose != null; - const hasFill = /\$\{[^}]+\}/.test(e.body || ''); - const hasLink = !!data.url; - let sections = ''; + const cs = renderCardSections(e, 'peek-body'); - if (hasDecision) { - const rejected = (data.rejected || []).map(r => `${escHtml(r)}`).join(''); - sections += `
-
decision${data.status || 'decided'}
-
-
${escHtml(data.chose)}
-
why${escHtml(data.why || '')}
- ${rejected ? `
considered
${rejected}
` : ''} -
-
`; - } - - if (hasLink && !hasDecision) { - sections += `
-
link
-
↗ ${escHtml(data.url)}
-
`; - } - - if (hasSteps) { - const steps = data.steps.map((s, i) => `
○${escHtml(s.text || s)}
`).join(''); - sections += `
-
steps · ${data.steps.length}
-
${steps}
-
`; - } - - if (!hasDecision && e.body) { - const lang = data.lang || ''; - const isCode = lang || e.card_type === 'snippet'; - const bodyHtml = isCode - ? `
${escHtml(e.body)}
` - : `
${renderMd(e.body)}
`; - sections += `
-
content${lang ? `${lang}` : ''}${hasFill ? `` : ''}
-
${bodyHtml}
-
`; - } - - let actions = ``; - if (hasFill) actions += ``; - if (hasSteps) actions += ``; - actions += ``; - actions += ``; - actions += ``; + let actions = ``; + if (cs.hasFill) actions += ``; + if (cs.hasSteps) actions += ``; + actions += ``; + actions += ``; + actions += ``; return `
@@ -969,7 +940,7 @@ ${e.description ? `
${escHtml(e.description)}
` : ''}
${affHtml}${tags}${e.pinned ? '★' : ''}
- ${sections} + ${cs.sections}
${actions}
`;