fix: address code review findings across backend and frontend
CI / test (pull_request) Successful in 2m13s
CI / test (pull_request) Successful in 2m13s
Fix goroutine-unsafe ULID entropy by wrapping in LockedMonotonicReader. Move PRAGMA foreign_keys outside transaction in v3 migration where SQLite was silently ignoring it. Escape LIKE wildcards in link resolution to prevent false matches. Add non-localhost binding warning, log writeJSON encoder errors, add ?permanent=true for explicit hard delete, preserve title/description during absorb, use millisecond backup timestamps, add path.Clean to spaHandler. Frontend gains checkedJSON() for resp.ok validation, consistent stopPropagation, and shared renderCardSections() to eliminate duplicate rendering.
This commit is contained in:
@@ -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`)
|
||||
|
||||
Reference in New Issue
Block a user