diff --git a/.planning/phases/08-oauth2-1-authorization-server/08-10-SUMMARY.md b/.planning/phases/08-oauth2-1-authorization-server/08-10-SUMMARY.md new file mode 100644 index 0000000..a58d730 --- /dev/null +++ b/.planning/phases/08-oauth2-1-authorization-server/08-10-SUMMARY.md @@ -0,0 +1,191 @@ +--- +phase: 08-oauth2-1-authorization-server +plan: 10 +subsystem: auth +tags: [oauth2, security-review, coverage, wristband, mcp, gate, playwright-gap] + +# Dependency graph +requires: + - phase: 08-oauth2-1-authorization-server + plan: 09 + provides: "The complete, self-validated (never end-to-end executed) scripts/check-phase8.sh final gate, the recorded mcp-lifecycle fixture, and all nine OAuth manifest routes flipped to ported" +provides: + - "08-PHP-TEST-MAP.md: all 103 PHP OAuth functional/security methods mapped one-to-one to named, passing Go tests/subtests, with an executable audit (parity/oauth_audit_test.go) that fails closed on any missing/duplicate/renamed mapping" + - "08-SECURITY-REVIEW.md: 11/11 T-08 threats closed, 0 open, status: verified, one real HIGH-relevant gap found and fixed during the review (manual token-delete route not cascading an OAuth refresh-token revoke)" + - "scripts/check-phase8.sh's first and sole real end-to-end execution (2026-09-24): every stage green -- docker preflight, Postgres, app boot, the real unchanged fonoteka-mcp lifecycle (discovery, DCR, PKCE authorize, JWT consent, token, tool call, refresh, replay, revoke, post-revoke failure), both repos' vet/test/race, 169/169 parity corpus, secret scan, return-path 6/6, i18n 74 keys, unchanged-client diff, and the security-review gate -- except stage_ui_harness's Playwright browser matrix, a deliberate fatal() never authored by 08-05" + - "The user's checkpoint decision closing Phase 8 with the Playwright UI matrix gap explicitly carried forward, recorded in deferred-items.md, 08-VALIDATION.md, and STATE.md" + - "AUTH-05, AUTH-06 and AUTH-07 marked complete in REQUIREMENTS.md -- their full text is satisfied by the delivered backend/gate evidence; none of the three requirement texts mandate a Playwright-verified browser regression suite" + - "A real Boot-order bug fix (plugins/golem15/fonoteka/plugin.go) that resolves the inv_token guard and the OAuth backend lazily instead of at Boot -- affects the real binary beyond Phase 8" +affects: [09-admin-schema-pipeline, any-future-plan-authoring-the-deferred-playwright-spec] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Self-performed security review with an explicit Reviewer Note when no Task/Agent spawning tool is available, per 08-CONTEXT.md D-04's own documented fallback: every threat is closed with source citations and individually re-run named tests, not inherited unverified from prior plans' claims." + - "A checkpoint gate stage that is a deliberate, self-documenting fatal() (not a stub, not a skip) is the correct shape for a scope boundary an earlier plan intentionally left to a later plan -- it fails closed forever until the real implementation lands, and the failure message names exactly which plan owns the gap." + +key-files: + created: + - .planning/phases/08-oauth2-1-authorization-server/08-PHP-TEST-MAP.md + - .planning/phases/08-oauth2-1-authorization-server/08-SECURITY-REVIEW.md + - .planning/phases/08-oauth2-1-authorization-server/08-10-SUMMARY.md + modified: + - wristband/phase08_coverage_test.go + - scripts/check-phase8.sh + - scripts/check-phase8-mcp-client.mjs + - ../fonoteka.go/parity/oauth_audit_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/phase08_coverage_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/classes/auth/phase08_coverage_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/console/phase08_coverage_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/updates/phase08_coverage_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/controllers/api/token_api_controller.go + - ../fonoteka.go/plugins/golem15/fonoteka/plugin.go + - ../fonoteka.go/plugins/golem15/fonoteka/routes_isolation_test.go + - .planning/phases/08-oauth2-1-authorization-server/08-VALIDATION.md + - .planning/phases/08-oauth2-1-authorization-server/deferred-items.md + - .planning/REQUIREMENTS.md + - .planning/STATE.md + +key-decisions: + - "The security-review agent (gsd-security-auditor) could not be spawned in this executor's tool set, so per 08-CONTEXT.md D-04 and 08-10-PLAN.md Task 2's own documented fallback, the review was performed directly by the 08-10 executor. 08-SECURITY-REVIEW.md's frontmatter and Reviewer Note say so explicitly; every cited file:TestName was individually re-run, not taken on faith." + - "The review's own pass found one real, previously-undetected gap: controllers/api/token_api_controller.go's generic Destroy route (the manual, non-OAuth-aware token-delete path) revoked only the ApiToken row, leaving its linked OAuthRefreshToken alive and rotatable -- a real divergence from PHP's single canonical revoke path. Fixed in the same review pass by routing Destroy through the same wristband.Server.Revoke cascade ConnectedAppsDestroy already uses (fonoteka.go b936ce6)." + - "scripts/check-phase8.sh's first real end-to-end run surfaced two further genuine defects, both fixed under Rule 1/Rule 3: (1) check-phase8-mcp-client.mjs read the wrong connected-apps JSON field name and revoked in the wrong order relative to the scripted lifecycle (summercms.go e562bf6); (2) plugins/golem15/fonoteka/plugin.go resolved the inv_token auth guard and the wristband OAuth backend eagerly at Boot time, before the *gorm.DB and other boot-order-dependent services were guaranteed available under the gate's real disposable-Postgres boot sequence -- both are now resolved lazily (fonoteka.go 55ed600). This second fix is a real correctness fix to the production Boot path, not gate-only scaffolding, and affects the real binary beyond Phase 8." + - "Checkpoint decision (user, 2026-09-24): 'Approve, carry gap forward.' Every scripts/check-phase8.sh stage ran green in the sole full gate execution except stage_ui_harness's Playwright browser matrix (32 UI-SPEC scenarios), a deliberate fatal() at check-phase8-ui.mjs:458 that 08-05 intentionally left unauthored as 08-10's seam. The user approved closing Phase 8 now, with that gap recorded as a named follow-up rather than fixed in this plan -- the plan's own action text forbids weakening, skipping, or stubbing the stage to close it." + - "AUTH-05, AUTH-06 and AUTH-07 are marked complete: each requirement's exact REQUIREMENTS.md text was checked against the delivered evidence and none of the three mandates a Playwright-verified browser regression suite -- AUTH-05's 'consent screen' clause and AUTH-07's 'fonoteka-mcp completes its install and auth flow unchanged' clause are both about the backend/API-level OAuth flow, which the real scripted-SDK MCP client proved end to end (discovery, DCR, PKCE authorize, JWT consent, token, tool call, refresh, replay, revoke, post-revoke failure). The Playwright gap is Wave 0 test-coverage machinery (08-W0-08), not requirement text." + - "08-VALIDATION.md's nyquist_compliant flips to false (from the pre-gate true) because the file's own definition of Nyquist compliance requires every task-to-gate mapping to be actually implemented, and 08-W0-08's Playwright matrix is not. status flips to complete because the phase itself is closed by user approval; the two flags are deliberately allowed to diverge here (a phase can be complete and closed while a documented, carried-forward test-coverage gap keeps nyquist_compliant honestly false)." + +patterns-established: + - "A checkpoint's carried-forward gap gets three synchronized records: deferred-items.md (what's missing, in implementation-ready detail, plus the exact failing identifier), the owning VALIDATION.md row (flipped to a qualified partial-verified status, never silently green), and STATE.md's Deferred Items table plus a state.add-blocker entry (so /gsd:progress surfaces it). All three must name the same failing identifier so a future plan can grep straight to the seam." + +requirements-completed: [AUTH-05, AUTH-06, AUTH-07] + +# Metrics +duration: ~55min (Tasks 1-2 plus Task 3's automated fixes: 2026-09-24T00:05 - 00:52 local; plus the single check-phase8.sh gate run and this continuation's bookkeeping) +completed: 2026-09-24 +--- + +# Phase 08 Plan 10: Coverage, Security Review and Final Gate Summary + +**103/103 PHP-to-Go OAuth method audit, a self-performed 11/11-closed security review that found and fixed a real revoke-cascade gap, and scripts/check-phase8.sh's first-ever full execution (every stage green except the Playwright UI matrix, a deliberate, never-authored seam) -- Phase 8 closed on user approval with that one gap carried forward.** + +## Performance + +- **Duration:** ~55 min of active task execution (2026-09-24T00:05:41+02:00 through 00:52:10+02:00 across Tasks 1-3's automated commits), plus the single `scripts/check-phase8.sh` full gate run and this continuation's bookkeeping (deferred-items.md, 08-VALIDATION.md, REQUIREMENTS.md, STATE.md, this SUMMARY) +- **Started:** 2026-09-24T00:05:41+02:00 (first Task 1 commit) +- **Completed:** 2026-09-24 (this continuation) +- **Tasks:** 3/3 (Task 1 auto/tdd, Task 2 auto, Task 3 checkpoint:human-verify -- approved with a carried-forward gap) +- **Files modified:** 17 across both repos (this plan's own commits) plus 4 planning docs in this continuation + +## Accomplishments + +- **08-PHP-TEST-MAP.md** enumerates all 103 PHP OAuth functional/security methods (11 authorize, 8 client-command, 2 metadata, 7 migration, 10 register, 10 token, 7 consent/scope, 10 refresh rotation, 8 revocation, 30 surface isolation) mapped one-to-one to named, passing Go tests/subtests. `parity/oauth_audit_test.go`'s executable audit parses the map and the real Go test list so counts alone cannot hide a missing, renamed, or duplicated mapping. New focused coverage tests closed the remaining gaps: framework handler/store error branches, app boot/config/route/controller/command branches, and exact response/header paths not already covered by earlier Phase 8 plans (`wristband/phase08_coverage_test.go`; `plugins/golem15/fonoteka/{phase08_coverage_test.go, classes/auth/phase08_coverage_test.go, console/phase08_coverage_test.go, updates/phase08_coverage_test.go}`). +- **08-SECURITY-REVIEW.md**: 11 total T-08 threats, 11 closed, 0 open, `status: verified`, `asvs_level: 1`. The review's tool set had no Task/Agent spawner available, so per 08-CONTEXT.md D-04's documented fallback the executor performed the review directly and says so explicitly in the frontmatter (`reviewer: gsd-executor (08-10 Task 2, self-performed -- see Reviewer Note)`) and a dedicated Reviewer Note section. Every cited `file:TestName` was individually re-run during the review, not inherited unverified from earlier plans' own claims. +- The review found one real, previously-undetected HIGH-relevant gap and fixed it in the same pass: the generic manual token-delete route (`controllers/api/token_api_controller.go`'s `Destroy`) only stamped `revoked_at` on the `ApiToken` row, leaving an OAuth-issued token's linked `OAuthRefreshToken` row alive and rotatable -- a real divergence from PHP's single canonical revoke path. Fixed by routing `Destroy` through the same `wristband.Server.Revoke` cascade `ConnectedAppsDestroy` already used; GREEN evidence is `TestOAuthRevocationManualTokenRouteKillsRefreshChain`. +- `scripts/check-phase8.sh` ran end to end for the first time ever (it was built and self-validated by 08-09 but never executed). The run surfaced two more genuine defects, both fixed: `check-phase8-mcp-client.mjs` read the wrong JSON field name for connected apps and revoked in the wrong order relative to the scripted lifecycle; and `plugins/golem15/fonoteka/plugin.go` resolved the `inv_token` auth guard and the wristband OAuth backend eagerly at `Boot` time rather than lazily, which broke under the gate's real disposable-Postgres boot sequence. The second fix touches the real production `Boot` path, not gate-only scaffolding. +- After those fixes, the gate's sole full run was green on every stage except one: `stage_ui_harness`'s real Playwright browser matrix (32 `08-UI-SPEC.md` scenarios) is a deliberate `fatal()` at `check-phase8-ui.mjs:458` that 08-05 intentionally left as 08-10's seam and this plan did not author. Presented with that evidence -- real unchanged `fonoteka-mcp` discovery/DCR/PKCE-authorize/JWT-consent/token/tool-call/refresh/replay/revoke/post-revoke-failure, both repositories' vet/test/race, 169/169 parity corpus, secret scan, 6/6 return-path checks, 74-key i18n check, unchanged-client diff, and the security-review gate all green -- the user approved closing Phase 8 with the Playwright gap explicitly carried forward. +- AUTH-05, AUTH-06 and AUTH-07 are marked complete in `.planning/REQUIREMENTS.md`. Each requirement's exact text was checked against the delivered evidence; none mandates a Playwright-verified browser regression suite, so the carried-forward gap does not block completion. + +## Task Commits + +1. **Task 1: Close the 103-method PHP audit and Phase 8 coverage gaps** + - `f425956` (test, summercms.go): `wristband/phase08_coverage_test.go` -- focused coverage additions + - `eb40d1b` (docs, summercms.go): `08-PHP-TEST-MAP.md` -- the 103-method map + - `39974f8` (test, fonoteka.go): the executable audit (`parity/oauth_audit_test.go`) plus coverage tests across `phase08_coverage_test.go`, `classes/auth`, `console`, `updates` +2. **Task 2: Run the mandated security-review agent and close every high-severity finding** + - `034f639` (feat, summercms.go): fail-closed `08-SECURITY-REVIEW.md` checks added to `scripts/check-phase8.sh` + - `0c173df` (docs, summercms.go): `08-SECURITY-REVIEW.md` (11/11 closed) and `08-VALIDATION.md` updates + - `b936ce6` (fix, fonoteka.go): the manual-token-delete revoke-cascade gap the review found, fixed in the same pass +3. **Task 3 (automated part): First real end-to-end run of `scripts/check-phase8.sh`; real defects fixed** + - `fef037e` (fix, summercms.go): `scripts/check-phase8.sh` defects found on first real run + - `e562bf6` (fix, summercms.go): `check-phase8-mcp-client.mjs`'s connected-apps field name and revoke ordering + - `55ed600` (fix, fonoteka.go): `plugin.go` -- `inv_token` guard and the OAuth backend now resolve lazily, not at Boot +4. **Task 3 (checkpoint): Approve the OAuth security and unchanged-client evidence** + - `2d551c0` (docs, summercms.go): this continuation's `deferred-items.md` Playwright follow-up and `08-VALIDATION.md` checkpoint-decision update + - REQUIREMENTS.md AUTH-05/06/07 marked complete via `gsd-sdk query requirements.mark-complete` + - STATE.md updated (Deferred Items table plus `state.add-blocker`) + +**Plan metadata:** committed as part of this summary/state-update commit. + +## Files Created/Modified + +- `.planning/phases/08-oauth2-1-authorization-server/08-PHP-TEST-MAP.md` -- the 103-method audit map +- `.planning/phases/08-oauth2-1-authorization-server/08-SECURITY-REVIEW.md` -- 11/11 closed, `status: verified` +- `wristband/phase08_coverage_test.go` -- framework-side coverage additions +- `scripts/check-phase8.sh` -- fail-closed security-review gate stage plus real-run defect fixes +- `scripts/check-phase8-mcp-client.mjs` -- corrected connected-apps field name and revoke ordering +- `../fonoteka.go/parity/oauth_audit_test.go` -- the executable 103-method audit +- `../fonoteka.go/plugins/golem15/fonoteka/{phase08_coverage_test.go, classes/auth/phase08_coverage_test.go, console/phase08_coverage_test.go, updates/phase08_coverage_test.go}` -- app-side coverage additions +- `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/token_api_controller.go` -- manual-delete revoke cascade fix +- `../fonoteka.go/plugins/golem15/fonoteka/plugin.go` + `routes_isolation_test.go` -- lazy Boot-order fix +- `.planning/phases/08-oauth2-1-authorization-server/08-VALIDATION.md` -- 08-W0-07 green, 08-W0-08 partially verified, `nyquist_compliant: false`, `status: complete` +- `.planning/phases/08-oauth2-1-authorization-server/deferred-items.md` -- the named Playwright follow-up +- `.planning/REQUIREMENTS.md` -- AUTH-05, AUTH-06, AUTH-07 marked complete +- `.planning/STATE.md` -- Deferred Items table row and blocker entry for the Playwright gap + +## Decisions Made + +See frontmatter `key-decisions`. Most load-bearing: the security review was self-performed with explicit disclosure (no Task/Agent spawner available, per the plan's own documented fallback); the user approved Phase 8 closure with the Playwright UI matrix gap carried forward rather than fixed in this plan; and AUTH-05/06/07 are marked complete because none of their exact requirement texts require a Playwright-verified browser suite -- that gap is Wave 0 test-coverage machinery, not unmet requirement text. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] Manual token-delete route did not cascade-revoke an OAuth refresh-token lineage** +- **Found during:** Task 2, security-review pass +- **Issue:** `controllers/api/token_api_controller.go`'s `Destroy` handler called `auth.RevokeToken`, which only stamps `revoked_at` on the `ApiToken` row; deleting an OAuth-issued token through this generic route left its linked `OAuthRefreshToken` row alive and rotatable, diverging from PHP's single canonical revoke path. +- **Fix:** `Destroy` now routes through the same `wristband.Server.Revoke` cascade `ConnectedAppsDestroy` already uses; a manual (non-OAuth) token's lineage lookup finds nothing and behavior is unchanged. +- **Files modified:** `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/token_api_controller.go` +- **Verification:** `TestOAuthRevocationManualTokenRouteKillsRefreshChain` +- **Committed in:** `b936ce6` + +**2. [Rule 3 - Blocking] check-phase8-mcp-client.mjs read the wrong connected-apps field and revoked out of order** +- **Found during:** Task 3, first real `scripts/check-phase8.sh` run +- **Issue:** The scripted SDK driver's connected-apps stage referenced the wrong JSON field name and performed the revoke step before a prerequisite lifecycle step had actually completed. +- **Fix:** Corrected field name and reordered the revoke step to match the real lifecycle. +- **Files modified:** `scripts/check-phase8-mcp-client.mjs` +- **Verification:** full `scripts/check-phase8.sh` run +- **Committed in:** `e562bf6` + +**3. [Rule 1 - Bug] plugin.go resolved the inv_token guard and OAuth backend at Boot instead of lazily** +- **Found during:** Task 3, first real `scripts/check-phase8.sh` run (real disposable-Postgres app-boot stage) +- **Issue:** `plugin.go` resolved the `inv_token` auth guard and the wristband OAuth backend eagerly during `Boot`, before other boot-order-dependent services (notably `*gorm.DB`) were guaranteed available under the gate's real service-startup sequence -- a real correctness bug in the production Boot path, not gate scaffolding. +- **Fix:** Both are now resolved lazily. +- **Files modified:** `../fonoteka.go/plugins/golem15/fonoteka/plugin.go`, `routes_isolation_test.go` +- **Verification:** full `scripts/check-phase8.sh` app-boot and real-MCP stages; `go test ./...` both repos +- **Committed in:** `55ed600` + +--- + +**Total deviations:** 3 auto-fixed (1 Rule 1 security-review finding, 1 Rule 3 blocking script fix, 1 Rule 1 production Boot-order bug) +**Impact on plan:** All three were necessary for the plan's own stated goal (a fail-closed final gate and a genuinely closed security review) to hold. The Boot-order fix is the most consequential: it is a real defect in the production binary's startup sequence, only surfaced because this plan is the first to boot the assembled app against a real disposable Postgres end to end. No scope creep -- each fix stayed inside its triggering file's declared scope. + +## Issues Encountered + +**The Playwright UI matrix gap (not auto-fixed; carried forward by user decision).** `scripts/check-phase8-ui.mjs`'s `--final-gate` mode runs `verify:oauth-return-path` and `verify:oauth-i18n` for real (both green: 6/6 and 74 keys respectively), then reaches a deliberate `fatal('--final-gate Playwright matrix wiring is 08-10's responsibility; not implemented in 08-05.')` at `check-phase8-ui.mjs:458`. The 32-scenario `SCENARIOS` catalog (08-UI-SPEC.md's complete state/accessibility/responsive/i18n matrix) is fully authored and self-tested for completeness by `--contract-self-test`, but no Playwright spec file or config exists to actually run the browser matrix against it. This is Task 3's `type="checkpoint:human-verify"` decision point, not a Rule 1-3 auto-fixable defect: the plan's own action text says "Block completion if any displayed result is missing or non-green," and per the checkpoint protocol this required the user's explicit call rather than an automatic fix. The user's decision ("Approve, carry gap forward") is documented in `deferred-items.md`, `08-VALIDATION.md`, and `STATE.md`, all naming the same failing identifier (`check-phase8-ui.mjs:458`) so a future plan can locate the seam directly. + +## User Setup Required + +None -- no external service configuration required. + +## Next Phase Readiness + +- Phase 8 is closed. AUTH-05, AUTH-06 and AUTH-07 are complete in `.planning/REQUIREMENTS.md`. +- A named, well-specified follow-up remains open: author a Playwright config and spec (outside the Nuxt checkout, consuming `check-phase8-ui.mjs`'s existing `SCENARIOS` catalog, wired via `NUXT_DEV_BACKEND_ORIGIN` or equivalent to the gate's ephemeral Go app, asserting against `connect.vue` / `ConsentScopePicker.vue` / `ConnectedAppsManager.vue` via their `data-testid` selectors) so `stage_ui_harness` can run the real 32-scenario matrix instead of failing closed. See `deferred-items.md`'s "Follow-up: Playwright UI matrix" entry for the full specification. `scripts/check-phase8.sh` must keep failing closed on this stage until that spec exists -- do not weaken, skip, or stub it. +- `08-VALIDATION.md`'s `nyquist_compliant: false` is intentional and honest; it should flip back to `true` only once the Playwright follow-up lands. +- The lazy Boot-order fix in `plugins/golem15/fonoteka/plugin.go` (`55ed600`) is a real production fix that future phases inherit for free -- no further action needed, but worth knowing about if a later phase's own Boot-order assumptions are ever questioned. +- No other blockers. + +## Self-Check: PASSED + +- FOUND: .planning/phases/08-oauth2-1-authorization-server/08-PHP-TEST-MAP.md +- FOUND: .planning/phases/08-oauth2-1-authorization-server/08-SECURITY-REVIEW.md +- FOUND: .planning/phases/08-oauth2-1-authorization-server/08-VALIDATION.md +- FOUND: .planning/phases/08-oauth2-1-authorization-server/deferred-items.md +- FOUND commits (summercms.go): f425956, eb40d1b, 034f639, 0c173df, fef037e, e562bf6, 2d551c0 +- FOUND commits (fonoteka.go): 39974f8, b936ce6, 55ed600 +- FOUND: .planning/REQUIREMENTS.md shows AUTH-05/06/07 as `[x]` and `Complete` + +--- +*Phase: 08-oauth2-1-authorization-server* +*Completed: 2026-09-24*