Quellcode durchsuchen

Docs: mark R01-N10 fixed, refresh SPEC §9 / §13

Records the c1dbfc1 placeholder-binding fix in REVIEW_01.md (Status
flipped to fixed-in-c1dbfc1, Suggested-ordering tick), SPEC §9
Shipped list (new entry above the new-sprint-form item), and SPEC §13
git history.

R01-N15 was already fixed in d16bff4 (verified `rel="noopener noreferrer"`
present in views/sprints/_task_list.twig:213); no change needed there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa vor 2 Tagen
Ursprung
Commit
d6b163d5fe
2 geänderte Dateien mit 29 neuen und 2 gelöschten Zeilen
  1. 15 0
      SPEC.md
  2. 14 2
      doc/REVIEW_01.md

+ 15 - 0
SPEC.md

@@ -1309,6 +1309,19 @@ is gone — see `src/Auth/BootstrapAdmin.php`.
       contract. Tests: 211 / 562 (was 202 / 533). Ninth fix from
       `doc/REVIEW_01.md`.
 
+- [x] **R01-N10 — Bind sprint_id with placeholder in MAX(sort_order)
+      lookups** (`c1dbfc1`). Three repo-level read paths previously
+      interpolated an integer route parameter directly into SQL
+      (`'... WHERE sprint_id = ' . $sprintId`). The route layer
+      int-casts the value, so this was not exploitable today, but the
+      contract was implicit — one careless future caller passing an
+      unvalidated string would have made the repo accept it. Switched
+      `TaskRepository::create`, `TaskRepository::moveToSprint`, and
+      `SprintWorkerRepository::add` to prepared statements with `?`
+      placeholders. Mechanical refactor, behaviour identical, no new
+      tests (existing `tests/Cascade` + `tests/Controllers` already
+      exercise these paths). Tenth fix from `doc/REVIEW_01.md`.
+
 - [x] **New sprint form: drop weeks input + task list row hover**
       (`3728106`). The `/sprints/new` form no longer collects an
       `n_weeks` value — the week count is derived from `start_date` /
@@ -1425,6 +1438,8 @@ before acting — nothing here is load-bearing once it grows stale.
 ## 13. Git history (as of this writing)
 
 ```
+c1dbfc1 Fix R01-N10: bind sprint_id with placeholder in MAX(sort_order) lookups
+a8ed6af Docs: mark R01-N08 fixed, refresh SPEC §9 / §11 / §13
 bc745cd Fix R01-N08: idle session timeout + CSRF rotation on login
 a2e77ea Fix R01-N05 + R01-N07: trusted-proxy aware HTTPS + client IP
 f565c86 Fix R01-N03: explicit env-bootstrap for the first OIDC admin

+ 14 - 2
doc/REVIEW_01.md

@@ -341,7 +341,19 @@ note. Do not delete entries — they're history.
 
 ### R01-N10 — SQL string concatenation of integer parameters in repos
 - **Severity**: MEDIUM (non-exploitable today, brittle by design).
-- **Status**: open.
+- **Status**: fixed-in-`c1dbfc1` — all three `MAX(sort_order)` lookups
+  now use prepared statements with `?` placeholders instead of inlining
+  the route-cast `int` into the SQL. `TaskRepository::create` (was
+  line 60) and `TaskRepository::moveToSprint` (was line 100) prepare
+  `'SELECT COALESCE(MAX(sort_order), 0) FROM tasks WHERE sprint_id = ?'`
+  and bind `$sprintId` / `$destSprintId`. `SprintWorkerRepository::add`
+  (was line 56) does the same against `sprint_workers`. The contract is
+  now explicit at the SQL boundary — a future caller passing an
+  unvalidated string is still type-safe because the value travels as a
+  bound parameter, never as a string fragment of the query. No new
+  tests: the fix is a mechanical refactor, the existing repo tests
+  exercise these paths under `tests/Cascade` and `tests/Controllers`.
+  No SPEC behaviour change.
 - **Where**:
   - `src/Repositories/TaskRepository.php` line 60 (`MAX(sort_order) FROM
     tasks WHERE sprint_id = ' . $sprintId`) and line 100 (`moveToSprint`).
@@ -775,7 +787,7 @@ A reasonable cadence (do not treat as binding):
    fixed in `a2e77ea`.
 9. ~~**R01-N08** (idle session timeout + CSRF rotation)~~ — fixed in
    `bc745cd`.
-10. **R01-N10** (bind-not-concat sweep) — mechanical.
+10. ~~**R01-N10** (bind-not-concat sweep)~~ — fixed in `c1dbfc1`.
 11. **R01-N12** (date filter validation) — UX bug.
 12. The rest as time permits.