Docs: mark R01-N16 / R01-N17 / R01-N18 closed, refresh SPEC §3 / §9 / §11 / §13

REVIEW_01.md: status blocks for the three findings.
  - R01-N18 fixed-in-`a0b717e` — drop preferred_username fallback,
    honour email_verified, fall back to entra:<oid>; rationale on
    why we don't require strict `=== true` (Entra v2.0 omits the
    flag for tenant-managed accounts).
  - R01-N16 fixed-in-`ef9b9b8` — bin/audit.sh + admin-manual cadence
    block; baseline composer audit returns no advisories today.
  - R01-N17 accepted-by-design — confirmed jumbojett uses fixed
    session keys (lines 1789-1866); the second-tab failure is the
    correct OIDC state-mismatch rejection. admin-manual §5.6
    documents the rule. The "Suggested ordering" section grows
    entries 15-17.

SPEC.md:
  - §3 — bin/ added to the directory tree; Auth/ now lists
    OidcClaims (R01-N18).
  - §9 — combined "R01-N18 + R01-N16 + R01-N17" entry inserted at
    the head of Shipped per the maintenance contract.
  - §11 — expected count bumped to 252 / 683.
  - §13 — git history block extended (also picks up the previously
    missed `868fe6c Docs: mark R01-N12 fixed` line).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

SPEC.md

@@ -86,8 +86,13 @@ per-cell audit trail.
 │               ├── alpine-csp.min.js   # @alpinejs/csp — Alpine without `unsafe-eval`
 │               ├── htmx.min.js         # htmx.org
 │               └── sortable.min.js     # SortableJS
+├── bin/
+│   └── audit.sh                # R01-N16 — wraps `composer audit --locked`
+│                               # inside the runtime image; honours
+│                               # SPRINT_PLANER_IMAGE for non-default tags
 ├── src/
-│   ├── Auth/            BootstrapAdmin, LocalAdmin, OidcClient, SessionGuard
+│   ├── Auth/            BootstrapAdmin, LocalAdmin, OidcClaims (R01-N18),
+│   │                    OidcClient, SessionGuard
 │   ├── Controllers/     AuthController, WorkerController, SprintController,
 │   │                    TaskController, AuditController, UserController,
 │   │                    SettingsController, ImportController (Phase 20)
@@ -1323,6 +1328,62 @@ is gone — see `src/Auth/BootstrapAdmin.php`.
       tests (existing `tests/Cascade` + `tests/Controllers` already
       exercise these paths). Tenth fix from `doc/REVIEW_01.md`.
 
+- [x] **R01-N18 + R01-N16 + R01-N17 — OIDC email trust path,
+      composer-audit helper, concurrent-tab note** (`a0b717e`,
+      `ef9b9b8`).
+      *N18 (`a0b717e`):* the OIDC callback's email-resolution rule was
+      `$claims->email ?? $claims->preferred_username ?? ''`. In some
+      Entra tenant configurations `preferred_username` is *not* a
+      verified email — it can be controlled by the end user — so an
+      attacker could spoof their forensic identity in the audit log.
+      The user identity itself is keyed by `oid`/`sub` (immutable) and
+      was never at risk; only the displayed actor was. New
+      `App\Auth\OidcClaims::resolveEmail($claims, $oid)` is the
+      single decision point: drops the `preferred_username` fallback,
+      honours `claims.email` only when `email_verified !== false`
+      (Entra v2.0 work/school tokens don't emit the flag at all and
+      ship a directory-controlled email — strict `=== true` would
+      have flipped every existing audit row to a fallback label),
+      and falls back to the documented immutable label `entra:<oid>`
+      when no trusted email is available. The `oid`-or-`sub` check
+      moved up so a missing identifier rejects the callback BEFORE
+      the resolver runs (the resolver assumes a non-empty oid for
+      its fallback). New `tests/Auth/OidcClaimsTest.php` (10 cases)
+      pins the matrix: verified-true / verified-absent / verified-
+      false / preferred_username-rejected / blank-email / whitespace-
+      trim / no-claims-at-all / non-bool-truthy email_verified /
+      non-scalar email defence.
+      *N16 (`ef9b9b8`):* PhpSpreadsheet has a long history of XML-
+      related advisories; the `^3.4` caret range lets minor upgrades
+      land on each `docker compose build --no-cache`, but operators
+      may not rebuild promptly. New `bin/audit.sh` wraps `composer
+      audit --locked --no-interaction` inside the runtime image so
+      the audit reflects the exact dependency tree the live container
+      ships (host PHP often lacks the right ext-* set). Honours
+      `SPRINT_PLANER_IMAGE` for non-default tags; refuses cleanly
+      when docker is missing or the image hasn't been built. Today's
+      baseline: "No security vulnerability advisories found." admin-
+      manual §5.5 grew a "Composer dependency cadence" block —
+      recommended rhythm (rebuild after every git pull, monthly
+      minimum), the auditor invocation, and a copy-pasteable weekly-
+      cron snippet that pipes a non-zero exit to `mail`. No CI hook
+      (this repo has no CI today and adding GitHub Actions would
+      have introduced a new external dependency).
+      *N17 (doc-only, in `ef9b9b8`):* accepted-by-design after
+      reading `vendor/jumbojett/openid-connect-php/src/
+      OpenIDConnectClient.php` lines 1789-1866. The library uses
+      fixed session keys (`openid_connect_state`,
+      `openid_connect_nonce`, `openid_connect_code_verifier`); two
+      parallel sign-in tabs clobber each other's state and the older
+      tab gets bounced to `/?auth_error=1` on the state-mismatch
+      guard at line 322 — the *correct* OIDC behaviour. The "fix"
+      (subclass + transient flow-id cookie, ~80 LoC + tests) trades
+      real complexity for a one-line UX nit. Not worth it for a
+      single-admin tool. admin-manual §5.6 documents the rule:
+      complete one sign-in at a time. No code change for N17.
+      Tests: 252 / 683 (was 242 / 673). Fourteenth, fifteenth, and
+      doc-only fix from `doc/REVIEW_01.md`.
+
 - [x] **R01-N13 + R01-N14 — Fatal-error safety net + XLSX session cap**
       (`d7dbfb5`). Two findings, one commit (both touch
       `public/index.php`).
@@ -1475,7 +1536,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 (242 tests, 673 assertions)
+# → OK (252 tests, 683 assertions)
 ```
 
 The Phase 20 parser tests need `ext-dom`, `ext-zip`, `ext-xmlreader`,
@@ -1522,7 +1583,11 @@ before acting — nothing here is load-bearing once it grows stale.
 ## 13. Git history (as of this writing)
 
 ```
+ef9b9b8 Fix R01-N16, doc R01-N17: composer-audit helper + admin-manual cadence note
+a0b717e Fix R01-N18: trust OIDC email only when issuer hasn't marked it unverified
+50f9bd2 Docs: mark R01-N09 / R01-N13 / R01-N14 fixed, refresh SPEC §3 / §9 / §11 / §13
 d7dbfb5 Fix R01-N13 + R01-N14: fatal-error safety net + XLSX session cap
+868fe6c Docs: mark R01-N12 fixed, refresh SPEC §9 / §13
 1b28469 Fix R01-N12: validate audit date filters server-side
 d6b163d Docs: mark R01-N10 fixed, refresh SPEC §9 / §13
 c1dbfc1 Fix R01-N10: bind sprint_id with placeholder in MAX(sort_order) lookups

doc/REVIEW_01.md

@@ -562,24 +562,45 @@ note. Do not delete entries — they're history.
 
 ### R01-N16 — PhpSpreadsheet 3.4 — pin and watch CVE feed
 - **Severity**: MEDIUM (advisory; depends on actual installed version).
-- **Status**: open.
-- **Where**: `composer.json` line 12 (`^3.4`), `composer.lock` (not read).
-- **What**: PhpSpreadsheet has a long history of XML-related vulns
-  (XXE etc.) when reading XLSX. Version 3.4 disables XXE by default but the
-  caret range allows minor upgrades, which is good — but operators may not
-  rebuild promptly.
-- **Why it matters**: Importer is admin-only, but admins can still feed a
-  hostile workbook (insider risk, compromised email, etc.).
-- **Suggested fix**:
-  - Document upgrade cadence (Dockerfile rebuild) in admin manual.
-  - Keep `setReadDataOnly(false)` (already correct — needed for fill colour
-    parsing) but explicitly call `LIBXML_NOENT | LIBXML_DTDLOAD` to off if
-    we ever reach to lower-level XML APIs.
-  - Run `composer audit` in CI.
+- **Status**: fixed-in-`ef9b9b8` — went with the doc + tooling path, no
+  CI hook (this repo has no CI today; adding GitHub Actions would
+  introduce a new external dependency the operator hasn't asked for).
+  New `bin/audit.sh` wraps `composer audit --locked --no-interaction`
+  inside `sprint_planer_web-app:latest`, so the audit reflects the
+  exact dependency tree the live container ships and not whatever the
+  host PHP happens to have. Honours `SPRINT_PLANER_IMAGE` for
+  non-default tags. Refuses cleanly when docker is missing or the
+  image hasn't been built. Today's baseline: "No security
+  vulnerability advisories found." `doc/admin-manual.md` §5.5 grew a
+  "Composer dependency cadence" block — recommended rebuild rhythm
+  (after every git pull, monthly minimum), the auditor invocation,
+  and a copy-pasteable weekly-cron snippet that pipes a non-zero exit
+  to `mail`. PhpSpreadsheet itself stays at the `^3.4` caret range —
+  minor upgrades will land on each `docker compose build --no-cache`
+  with no manifest change required. No new tests (script is operator
+  tooling, not application code).
 
 ### R01-N17 — Login `state`/PKCE storage in session — verify lifetime
-- **Severity**: MEDIUM (compliance; needs runtime check).
-- **Status**: open.
+- **Severity**: MEDIUM (usability hazard; not a vulnerability).
+- **Status**: accepted-by-design — confirmed by reading
+  `vendor/jumbojett/openid-connect-php/src/OpenIDConnectClient.php`. The
+  library uses fixed session keys `openid_connect_state`,
+  `openid_connect_nonce`, `openid_connect_code_verifier` (lines
+  1789-1866); the methods `setSessionKey()` / `getSessionKey()` /
+  `unsetSessionKey()` are `protected` so a subclass *could* namespace
+  them, but the namespace would then have to round-trip via a separate
+  short-lived cookie (the OIDC `state` parameter is generated and read
+  by the library itself, not by us). The realistic failure mode is
+  small: the second tab clobbers the first tab's `state`, the first
+  tab's callback hits the `if ($_REQUEST['state'] !== $this->getState())
+  { throw 'Unable to determine state' }` guard at line 322, the user
+  is bounced to `/?auth_error=1`, and they retry. That guard is the
+  *correct* OIDC state-mismatch behaviour — it's what stops a CSRF on
+  the OIDC callback. The "fix" (subclass + transient flow-id cookie,
+  ~80 LoC + tests) trades real complexity for a one-line UX nit. Not
+  worth it for a single-admin tool. `doc/admin-manual.md` §5.6
+  "Tabbed sign-in note" documents the rule for operators: complete
+  one sign-in at a time. No code change.
 - **Where**: `src/Auth/OidcClient.php` + `src/Controllers/AuthController.php`
   callback.
 - **What**: `jumbojett/openid-connect-php` stores its `state` and PKCE
@@ -589,13 +610,32 @@ note. Do not delete entries — they're history.
   bounce back. If two browser tabs initiate auth flows simultaneously, the
   second overwrites the first's state. Not a direct vuln but a usability
   hazard.
-- **Suggested fix**: Confirm the library's per-flow state isolation; if it
-  stores under fixed keys, file an upstream issue or wrap with a per-flow
-  random suffix.
+- **Suggested fix**: NONE. Decision recorded — see Status.
 
 ### R01-N18 — Email-claim trust path during OIDC upsert
 - **Severity**: MEDIUM.
-- **Status**: open.
+- **Status**: fixed-in-`a0b717e` — new `App\Auth\OidcClaims::resolveEmail
+  ($claims, $oid)` is the single decision point for the audit-trail
+  email. The `preferred_username` fallback is removed entirely (the
+  actual hazard the finding flagged: user-controlled in some Entra
+  tenants). `claims.email` is honoured when present, EXCEPT when the
+  issuer explicitly marks it `email_verified === false` — that case
+  falls back to the documented immutable label `entra:<oid>` so the
+  forensic trail still has a stable, non-spoofable actor. A *missing*
+  `email_verified` flag is treated as "no negative signal" because
+  Entra v2.0 work/school tokens do not emit it (the email there comes
+  from the directory and is server-controlled); requiring strict
+  `=== true` would flip every existing audit row to `entra:<oid>`
+  which is an unnecessary regression for the threat model. The
+  `oid`-or-`sub` check moved up so a missing identifier rejects the
+  callback BEFORE the email resolver runs (the resolver assumes a
+  non-empty oid for its fallback). Identity is still keyed by `oid`
+  / `sub` and is unaffected. New `tests/Auth/OidcClaimsTest.php` (10
+  cases) pins the matrix: verified-true / verified-absent /
+  verified-false / preferred-username-fallback-rejected / blank-email
+  / whitespace-trim / no-claims-at-all / non-bool-truthy
+  email_verified / non-scalar email defence. Suite: 252 / 683 (was
+  242 / 673).
 - **Where**: `src/Controllers/AuthController.php` lines 75-77.
 - **What**: `email = $claims->email ?? $claims->preferred_username`. In Entra,
   `preferred_username` is *not* guaranteed to be a verified email; in some
@@ -888,7 +928,14 @@ A reasonable cadence (do not treat as binding):
 13. ~~**R01-N13** (fatal-error safety net)~~ — fixed in `d7dbfb5`.
 14. ~~**R01-N14** (XLSX session cap + abandoned-token audit)~~ —
     fixed in `d7dbfb5`.
-15. The rest as time permits.
+15. ~~**R01-N18** (drop preferred_username fallback, honour
+    email_verified)~~ — fixed in `a0b717e`.
+16. ~~**R01-N16** (composer-audit helper + admin-manual cadence)~~ —
+    fixed in `ef9b9b8`.
+17. ~~**R01-N17** (concurrent-tab OIDC state)~~ — accepted-by-design;
+    library uses fixed session keys, the realistic failure mode is a
+    one-line UX nit, see Status block.
+18. The rest as time permits.
 
 Each fix should ship as its own commit per SPEC.md §14, with a follow-up
 SPEC update if behaviour or config surface changes.