Przeglądaj źródła

Docs: mark R01-N06 fixed, refresh SPEC §3 / §4 / §7 / §9 / §11 / §13

Mark R01-N06 fixed in `doc/REVIEW_01.md` with the implementation
summary (table, repository, policy thresholds, audit reason string,
test count delta) and tick its line in the suggested-ordering list.
Refresh SPEC.md:
  - §3: list `AuthThrottleRepository` and `005_auth_throttle.sql`
  - §4: rename header to migrations/001..005, add the new table to
        the table list, document the policy summary
  - §7: note the new `LOGIN_FAILED` reason `local_admin_throttled_
        until_<iso>` alongside the existing `*_credential_mismatch`
  - §9: add the R01-N06 shipped entry (sixth review fix)
  - §11: bump expected test count to 176 / 484
  - §13: prepend `e295432` to the git-history block (along with the
        two doc-only commits 851f8cf and 270c0c1 that the previous
        review-fix loops landed without back-filling here)
chiappa 2 dni temu
rodzic
commit
2b8f1673f1
2 zmienionych plików z 73 dodań i 7 usunięć
  1. 55 5
      SPEC.md
  2. 18 2
      doc/REVIEW_01.md

+ 55 - 5
SPEC.md

@@ -101,7 +101,7 @@ per-cell audit trail.
 │   │                    SprintWeekRepository, SprintWorkerRepository,
 │   │                    SprintWorkerDayRepository, TaskRepository,
 │   │                    TaskAssignmentRepository, AuditRepository,
-│   │                    AppSettingsRepository
+│   │                    AppSettingsRepository, AuthThrottleRepository
 │   └── Services/        AuditLogger, CapacityCalculator
 │       └── Import/      (Phase 20) XlsxColorClassifier, XlsxSprintImporter,
 │                        SprintImporter
@@ -109,6 +109,7 @@ per-cell audit trail.
 │                        002_sprint_week_active_days.sql (Phase 12 — mask column)
 │                        003_task_status_and_app_settings.sql (Phase 18 — task-cell status + KV)
 │                        004_task_metadata_and_links.sql (Phase 22 — task description/url + linked_task_id)
+│                        005_auth_throttle.sql (R01-N06 — local-admin login throttle)
 ├── views/               (Twig 3) layout.twig, layout-bare.twig, home.twig,
 │                        auth/local.twig, workers/index.twig,
 │                        users/index.twig, audit/index.twig,
@@ -123,18 +124,26 @@ per-cell audit trail.
                          (volume-mounted, gitignored)
 ```
 
-## 4. Schema (migrations/001..004)
+## 4. Schema (migrations/001..005)
 
 Tables (already applied): `users`, `workers`, `sprints`, `sprint_weeks`,
 `sprint_workers`, `sprint_worker_days`, `tasks`, `task_assignments`,
 `audit_log`, `app_settings` (Phase 18 — KV store for global flags),
-plus the `schema_version` tracking table.
+`auth_throttle` (R01-N06 — local-admin login throttle), plus the
+`schema_version` tracking table.
 
 Phase 22 (migration 004) adds three columns to `tasks`:
 `description TEXT NOT NULL DEFAULT ''`, `url TEXT NOT NULL DEFAULT ''`,
 and `linked_task_id INTEGER REFERENCES tasks(id) ON DELETE SET NULL`
 — set on a copy and pointed at the source. Plus index `idx_tasks_linked`.
 
+R01-N06 (migration 005) adds `auth_throttle(ip_address, email,
+attempts, first_failure_at, last_failure_at, locked_until)` with PK
+`(ip_address, email)` plus index `idx_auth_throttle_locked`.
+`AuthThrottleRepository` owns the policy: 5 failures in a 15-minute
+window → 5-min lock, 10 → 30-min, 20+ → 1-hour. A successful sign-in
+deletes the row.
+
 `sprint_weeks.active_days_mask INTEGER NOT NULL DEFAULT 31` (Phase 12) is
 a 5-bit mask — bit0=Mo, bit1=Di, bit2=Mi, bit3=Do, bit4=Fr — and is the
 source of truth for "is this a workday this week." `max_working_days`
@@ -254,7 +263,11 @@ is called inside the same transaction as the DB change. Controllers prefer
     - `SprintController::removeWorker()` — sprint_worker → sprint_worker_days + task_assignments
     - `SprintController::replaceWeeks()` — sprint_week → sprint_worker_days (on shrink)
 - Non-mutation events (LOGIN, LOGOUT, LOGIN_FAILED, BOOTSTRAP_ADMIN) → always
-  one row.
+  one row. R01-N06 adds a second `LOGIN_FAILED` reason on the local-admin
+  path: `local_admin_throttled_until_<iso>` is written when the
+  `(ip, email)` bucket is currently locked, separate from the existing
+  `local_admin_credential_mismatch` row written when the password
+  itself was wrong.
 
 ## 8. Env (.env.example)
 
@@ -1136,6 +1149,40 @@ with a `BOOTSTRAP_ADMIN` audit row.
       attempt). Tests: 163 / 417 (was 159 / 413). Fifth fix from
       `doc/REVIEW_01.md`.
 
+- [x] **R01-N06 — Throttle local-admin login by (ip, email)**
+      (`e295432`). The `/auth/local` path had no rate limiting —
+      attackers could brute-force the password as fast as the server
+      could respond. With R01-N01 already enforcing a hash-only
+      credential, throttling is the remaining defence on the local-admin
+      path (the OIDC path has Entra's own). New migration
+      `005_auth_throttle.sql` adds `auth_throttle(ip_address, email,
+      attempts, first_failure_at, last_failure_at, locked_until)` PK
+      `(ip_address, email)` + `idx_auth_throttle_locked`. New
+      `AuthThrottleRepository` owns three operations (`lockoutFor`,
+      `recordFailure`, `clear`) plus a `purgeExpired` housekeeping
+      helper (not yet wired). Policy lives in the pure-static
+      `computeLockout(attempts, now)`: 1-4 → no lock, 5-9 → +5 min,
+      10-19 → +30 min, 20+ → +1 hour. Counter naturally rolls over
+      after a 15-minute idle window so an honest user who later
+      mistypes isn't penalised forever; a successful sign-in deletes
+      the row outright. Email is canonicalised (lowercased + trimmed)
+      to defeat case-variation bypass; IPs key verbatim so two
+      attacker IPs don't share a lock. `AuthController::loginLocal`
+      checks the lock BEFORE `LocalAdmin::verify()` (so a slow
+      `password_verify()` can't be turned into an oracle by a still-
+      locked attacker). Throttle hits emit a `LOGIN_FAILED` audit
+      row with reason `local_admin_throttled_until_<iso>`, separate
+      from the `local_admin_credential_mismatch` row written when
+      the password itself was wrong. Form template gains an amber
+      "Too many failed attempts" notice when the controller redirects
+      with `?throttled=1`. New
+      `tests/Repositories/AuthThrottleRepositoryTest.php` (13 cases)
+      pins the threshold matrix, the 4:59 / 5:00 lockout boundary,
+      idle-window reset, IP / email bucketing, the full
+      `computeLockout` matrix, and `purgeExpired`'s selectivity —
+      time is injected so no test sleeps. Tests: 176 / 484 (was 163 /
+      417). Sixth 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` /
@@ -1205,7 +1252,7 @@ for f in $(git ls-files '*.php'); do php -l "$f" | tail -1 | sed "s|^|$f: |"; do
 Run the test suite:
 ```bash
 vendor/bin/phpunit
-# → OK (159 tests, 443 assertions)
+# → OK (176 tests, 484 assertions)
 ```
 
 The Phase 20 parser tests need `ext-dom`, `ext-zip`, `ext-xmlreader`,
@@ -1252,7 +1299,10 @@ before acting — nothing here is load-bearing once it grows stale.
 ## 13. Git history (as of this writing)
 
 ```
+e295432 Fix R01-N06: throttle local-admin login by (ip, email)
+851f8cf Docs: mark R01-N11 fixed, refresh SPEC §9 / §13
 4ae1817 Fix R01-N11: whitelist column in AuditRepository::distinctColumn
+270c0c1 Docs: mark R01-N15 fixed, refresh SPEC §9 / §13
 d16bff4 Fix R01-N15: add noreferrer to external task URL link
 48a351c Docs: mark R01-N01 fixed, refresh SPEC §9 / §11 / §13
 857df15 Fix R01-N01: hash-only LOCAL_ADMIN_PASSWORD_HASH (no plaintext fallback)

+ 18 - 2
doc/REVIEW_01.md

@@ -172,7 +172,23 @@ note. Do not delete entries — they're history.
 
 ### R01-N06 — No rate-limiting on local-admin login
 - **Severity**: HIGH (when the local-admin path is enabled).
-- **Status**: open.
+- **Status**: fixed-in-`e295432` — new migration `005_auth_throttle.sql`
+  + `AuthThrottleRepository` keyed on `(ip_address, email)`. `loginLocal`
+  now checks `lockoutFor()` BEFORE `LocalAdmin::verify()` (so a slow
+  `password_verify()` can't be turned into an oracle while locked); on
+  failure it calls `recordFailure()`, on success it `clear()`s the bucket.
+  Policy in the pure-static `computeLockout(attempts, now)`: 1-4 → no
+  lock, 5-9 → +5 min, 10-19 → +30 min, 20+ → +1 hour. Counter rolls
+  over after 15 minutes of no failures, so honest mistypes don't get
+  punished forever. Email key is lowercased + trimmed so case variation
+  doesn't dodge the bucket; IPs separate, so two attacker IPs don't
+  share a lock. Throttle hits emit a `LOGIN_FAILED` audit row with
+  `reason=local_admin_throttled_until_<iso>` and the form renders an
+  amber "Too many failed attempts" notice when redirected with
+  `?throttled=1`. New `tests/Repositories/AuthThrottleRepositoryTest.php`
+  (13 cases) pins the threshold matrix, the 4:59 / 5:00 lockout
+  boundary, the idle-window reset, IP / email bucketing, and
+  `purgeExpired()`'s selectivity. Tests: 176 / 484 (was 163 / 417).
 - **Where**: `src/Controllers/AuthController.php::loginLocal` lines 194-276.
 - **What**: There is no per-IP / per-account throttle on `/auth/local`.
   Attackers can brute-force the password as fast as the server can respond.
@@ -666,7 +682,7 @@ A reasonable cadence (do not treat as binding):
 4. ~~**R01-N11** (audit column whitelist)~~ — fixed in `4ae1817`.
 5. ~~**R01-N01** (local-admin password hashing)~~ — fixed in `857df15`
    (hash-only, no plaintext fallback per operator decision).
-6. **R01-N06** (login throttling) — tiny new repo + integration.
+6. ~~**R01-N06** (login throttling)~~ — fixed in `e295432`.
 7. **R01-N03** (first-user bootstrap hardening).
 8. **R01-N07** + **R01-N05** (proxy / HTTPS handling) — paired.
 9. **R01-N08** (idle session timeout + CSRF rotation).