diff --git a/review/checklist.md b/review/checklist.md index 282c9944..2a758b45 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -35,16 +35,16 @@ Be terse. For each issue: one line describing the problem, one line with the fix ### Pass 1 — CRITICAL #### SQL & Data Safety -- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use `sanitize_sql_array` or Arel) -- TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `update_all` -- `update_column`/`update_columns` bypassing validations on fields that have or should have constraints -- N+1 queries: `.includes()` missing for associations used in loops/views (especially avatar, attachments) +- String interpolation in SQL — use parameterized queries (Rails: `sanitize_sql_array`/Arel; Node: prepared statements; Python: parameterized queries; Go: `db.Query` with `?` placeholders) +- TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `UPDATE` +- Bypassing model validations for direct DB writes (Rails: `update_column`/`update_columns`; Django: `QuerySet.update()`; Prisma: raw queries) +- N+1 queries: missing eager loading for associations used in loops/views (Rails: `.includes()`; SQLAlchemy: `joinedload()`; Prisma: `include`; Go: manual batch query) #### Race Conditions & Concurrency -- Read-check-write without uniqueness constraint or `rescue RecordNotUnique; retry` (e.g., `where(hash:).first` then `save!` without handling concurrent insert) -- `find_or_create_by` on columns without unique DB index — concurrent calls can create duplicates +- Read-check-write without uniqueness constraint or catch-duplicate-key-and-retry (e.g., find-then-insert without handling concurrent insert) +- Find-or-create without unique DB index — concurrent calls can create duplicates - Status transitions that don't use atomic `WHERE old_status = ? UPDATE SET new_status` — concurrent updates can skip or double-apply transitions -- `html_safe` on user-controlled data (XSS) — check any `.html_safe`, `raw()`, or string interpolation into `html_safe` output +- Unsafe HTML rendering of user-controlled data (XSS) — check for unescaped output (Rails: `.html_safe`/`raw()`; React: `dangerouslySetInnerHTML`; Vue: `v-html`; Django: `|safe`/`mark_safe`) #### LLM Output Trust Boundary - LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting. @@ -141,7 +141,7 @@ the agent auto-fixes a finding or asks the user. ``` AUTO-FIX (agent fixes without asking): ASK (needs human judgment): ├─ Dead code / unused variables ├─ Security (auth, XSS, injection) -├─ N+1 queries (missing .includes()) ├─ Race conditions +├─ N+1 queries (missing eager loading) ├─ Race conditions ├─ Stale comments contradicting code ├─ Design decisions ├─ Magic numbers → named constants ├─ Large fixes (>20 lines) ├─ Missing LLM output validation ├─ Enum completeness