diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index be76449..ed34e7b 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -333,7 +333,7 @@ Plans: **Wave 3** *(blocked on 08-02)* -- [ ] 08-03-PLAN.md — Create durable PKCE-bound authorize requests on the assembled raw route surface +- [x] 08-03-PLAN.md — Create durable PKCE-bound authorize requests on the assembled raw route surface **Wave 4** *(blocked on 08-03)* @@ -496,7 +496,7 @@ Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 → | 5. Data layer full fidelity | 6/6 | Complete | 2026-09-18 | | 6. HTTP routing, auth groups and rate limiting | 14/14 | Complete | 2026-09-21 | | 7. User plugin and authentication | 8/8 | Complete | 2026-09-23 | -| 8. OAuth2.1 authorization server | 2/10 | In Progress| | +| 8. OAuth2.1 authorization server | 3/10 | In Progress| | | 9. Backend admin authentication and schema pipeline | 0/TBD | Not started | - | | 10. Admin Vue SPA | 0/TBD | Not started | - | | 11. Jobs, realtime and search infrastructure | 0/TBD | Not started | - | diff --git a/.planning/STATE.md b/.planning/STATE.md index 5ad4bc9..620e8a7 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -3,14 +3,14 @@ gsd_state_version: 1.0 milestone: v1.0 milestone_name: milestone status: executing -stopped_at: Completed 08-02-PLAN.md -last_updated: "2026-09-23T17:54:25.008Z" +stopped_at: Completed 08-03-PLAN.md +last_updated: "2026-09-23T18:11:44.182Z" last_activity: 2026-09-23 progress: total_phases: 15 completed_phases: 7 total_plans: 55 - completed_plans: 47 + completed_plans: 48 percent: 47 --- @@ -26,11 +26,11 @@ See: .planning/PROJECT.md (updated 2026-09-16) ## Current Position Phase: 08 (oauth2-1-authorization-server) — EXECUTING -Plan: 3 of 10 +Plan: 4 of 10 Status: Ready to execute Last activity: 2026-09-23 -Progress: [█████████░] 85% +Progress: [█████████░] 87% ## Performance Metrics @@ -93,6 +93,7 @@ Progress: [█████████░] 85% | Phase 07-user-plugin-and-authentication P08 | 25min | 3 tasks | 6 files | | Phase 08 P01 | 25min | 2 tasks | 6 files | | Phase 08 P02 | 30min | 3 tasks | 14 files | +| Phase 08 P03 | 20min | 2 tasks | 6 files | ## Accumulated Context @@ -218,6 +219,10 @@ Recent decisions affecting current work: - [Phase ?]: [Phase 08 P02]: Register strips C0/DEL control characters from client_name at DCR capture time (PHP only strips at ConnectedApp/Consent display time) per 08-CONTEXT.md discretion - [Phase ?]: [Phase 08 P02]: OAuth config keys live under golem15.fonoteka.oauth.* (bare plugin ID, matching compass.MergePlugin), not plugins.golem15.fonoteka.oauth.* - [Phase ?]: [Phase 08 P02]: AUTH-05/AUTH-06/AUTH-07 remain Pending in REQUIREMENTS.md: this plan ships DCR only, not the full authorize/token/consent surface +- [Phase 08]: [Phase 08 P03]: Options gained Resource and PendingRequestTTL (not in the plan's file list for server.go) following the 08-02 precedent of extending Options for deployment-configurable values +- [Phase 08]: [Phase 08 P03]: authorize's allowed-scope set is a package-level authorizeAllowedScopes constant, not Options.ScopesSupported (RFC 8414 metadata field) -- conceptually distinct config surfaces matching PHP's own separation +- [Phase 08]: [Phase 08 P03]: Client lookup and pending-row creation each open their own WithinTx call, matching PHP's lack of a wrapping transaction around authorize; only DCR and later code-exchange/refresh-rotation need single-transaction atomicity +- [Phase 08]: [Phase 08 P03]: AUTH-05/AUTH-06/AUTH-07 remain Pending in REQUIREMENTS.md, continuing 08-01/08-02's decision: this plan ships authorize only, not the full RFC surface ### Pending Todos @@ -239,6 +244,6 @@ Items acknowledged and carried forward from previous milestone close: ## Session Continuity -Last session: 2026-09-23T17:54:24.991Z -Stopped at: Completed 08-02-PLAN.md +Last session: 2026-09-23T18:11:30.777Z +Stopped at: Completed 08-03-PLAN.md Resume file: None diff --git a/.planning/phases/08-oauth2-1-authorization-server/08-03-SUMMARY.md b/.planning/phases/08-oauth2-1-authorization-server/08-03-SUMMARY.md new file mode 100644 index 0000000..7b1e4e9 --- /dev/null +++ b/.planning/phases/08-oauth2-1-authorization-server/08-03-SUMMARY.md @@ -0,0 +1,136 @@ +--- +phase: 08-oauth2-1-authorization-server +plan: 03 +subsystem: auth +tags: [oauth2, rfc6749, pkce, wristband, raw-routes, rfc3986, standard-library] + +# Dependency graph +requires: + - phase: 08-oauth2-1-authorization-server + plan: 02 + provides: "wristband.Backend/Tx transaction-scoped store bundle (ClientStore/AuthCodeStore/RefreshTokenStore/AccessTokenIssuer), the fonoteka OAuthStore GORM adapter, and the corrected OAuth schema" +provides: + - "wristband.Server.Authorize: exact port of OAuthAuthorizeController::authorize's validation order (usable client, exact redirect, response_type, S256 syntax, scope/ceiling, resource) and its local-400-vs-trusted-redirect open-redirect defense" + - "An RFC 3986 ordered-pair query encoder (buildOrderedQuery/appendOrderedQuery/rfc3986Escape) used for every authorize error redirect, distinct from net/url.Values.Encode's key-sorting and '+'-for-space behavior" + - "GET /oauth/mcp/authorize mounted raw with zero middleware on the assembled fonoteka.go app; golem15.fonoteka.oauth.resource and .pending_request_ttl_seconds wired into wristband.Options" +affects: [08-04-token-exchange, 08-05-consent-and-connected-apps, 08-06-lifecycle-and-sweeps, 08-09-parity-and-real-mcp-gate, 08-10-unit-tests-and-security-review] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Two-phase client/redirect validation: an unknown or unusable client, or a redirect_uri that is not an exact member of the client's registered list, is a local text/plain 400 with no Location; only after both pass does any later failure construct a redirect to the now-trusted URI (T-08-OPEN-REDIRECT)" + - "Ordered RFC 3986 query encoding for OAuth error redirects: percent-encode only non-unreserved bytes (space becomes %20, not '+'), never sort keys — implemented as three small package-level functions (rfc3986Escape/buildOrderedQuery/appendOrderedQuery) rather than reusing net/url.Values.Encode" + - "Scope-ceiling truncation happens before the offline_access peel: a client's ScopeCeiling keeps offline_access unconditionally (never counted as a 'data' scope for the empty-intersection check) and truncates everything else; the offline_access boolean is only split out of the scope list at pending-row-creation time, matching PHP's createPendingRequest" + +key-files: + created: [] + modified: + - wristband/authorize.go + - wristband/authorize_test.go + - wristband/server.go + - ../fonoteka.go/plugins/golem15/fonoteka/plugin.go + - ../fonoteka.go/plugins/golem15/fonoteka/routes.go + - ../fonoteka.go/plugins/golem15/fonoteka/oauth_authorize_test.go + +key-decisions: + - "Options gained Resource and PendingRequestTTL fields (not listed in the plan's files_modified for server.go) to carry the RFC 8707 expected-resource value and the 600s pending-request TTL as deployment-configurable values, following the same Options-extension pattern 08-02 established for DCRClientCap/DCRUnconsentedSweepAge/RegisterMaxBodyBytes" + - "authorize's allowed-scope set is authorizeAllowedScopes, a package-level constant matching PHP's private ALLOWED_SCOPES, not wristband's Options.ScopesSupported (which is the RFC 8414 metadata field) — the two happen to share the same PHP-default values but are conceptually distinct config surfaces, matching the PHP source's own separation" + - "Client lookup and pending-row creation each open their own wristband.Backend.WithinTx call (not one transaction spanning the whole request), matching PHP's own lack of a wrapping DB transaction around authorize; only DCR's sweep+cap+create (08-02) and later code-exchange/refresh-rotation (08-04) need single-transaction atomicity" + +patterns-established: + - "RFC 3986 ordered-pair redirect encoding is now the shared pattern 08-04's token-endpoint success/error redirects and 08-05's consent/deny redirects will reuse via the same buildOrderedQuery/appendOrderedQuery helpers in wristband/authorize.go" + +requirements-completed: [] + +# Metrics +duration: ~20min +completed: 2026-09-23 +--- + +# Phase 08 Plan 03: Authorize Summary + +**`wristband.Server.Authorize` ports the exact PHP authorize-request validation order and open-redirect defense, persists an opaque PKCE-bound pending row through the transaction-scoped store bundle, and is now connector-visible raw on the assembled Go app.** + +## Performance + +- **Duration:** ~20 min +- **Started:** 2026-09-23T19:55:00Z (approx.) +- **Completed:** 2026-09-23T20:08:45+02:00 +- **Tasks:** 2 completed (4 commits: RED/GREEN pairs across both repos) +- **Files modified:** 6 (3 in summercms.go's wristband package, 3 in fonoteka.go) + +## Accomplishments + +- `wristband.Server.Authorize` is a byte-for-byte port of `OAuthAuthorizeController::authorize`'s validation order: usable client, exact redirect-URI membership, `response_type=code`, `code_challenge_method=S256`, challenge length (43-128), scope parsing with a `["read"]` default, client scope-ceiling truncation (preserving `offline_access` and never rejecting except on an empty data-scope intersection), and the RFC 8707 resource check — only after all of these does it persist a pending row and redirect +- An unknown/unusable client or an unregistered `redirect_uri` returns a local `text/plain; charset=UTF-8` 400 with no `Location` header (T-08-OPEN-REDIRECT); every later validation failure redirects to the now-trusted `redirect_uri` with an ordered `error`, `error_description`, `iss`, optional `state` query built by a dedicated RFC 3986 encoder (`rfc3986Escape`/`buildOrderedQuery`/`appendOrderedQuery`) that never uses `net/url.Values.Encode` (which sorts keys and encodes spaces as `+`) +- A valid S256 request persists exactly one durable, hash-only-free pending `AuthCodeRecord` (nil `CodeHash`, nil `UserID`, 600s expiry from `Options.PendingRequestTTL`) and redirects to `/connect?request=` with `Cache-Control: no-store` and no `code`/`state`/`client_secret` leaked onto the app's own redirect +- `GET /oauth/mcp/authorize` is now mounted on the assembled fonoteka.go raw route group with zero middleware (D-09); `golem15.fonoteka.oauth.resource` and `.pending_request_ttl_seconds` are wired from config into `wristband.Options` in `Plugin.Boot` +- 08-01 metadata and 08-02 DCR exact-byte tests, plus the full `plugins/golem15/fonoteka` module test suite (including `-race`), remain green + +## Task Commits + +Each task was committed atomically (TDD RED then GREEN, split per repo since both repos changed): + +1. **Task 1: authorize RED anchor** — `787e612` (test, summercms.go): `Server.Authorize` 501 stub, `TestPhase8RedAuthorize` fails the exact S256 success contract against it; `57049f8` (test, fonoteka.go): `TestPhase8RedAuthorizeApp` fails 404 against the unmounted route. Both verified fail-closed via `scripts/check-phase8-red.sh`. +2. **Task 2: implement and mount authorize** — `90752be` (feat, summercms.go): the real `Authorize` handler, the RFC 3986 ordered-query encoder, `Options.Resource`/`Options.PendingRequestTTL`, and the full framework-level test matrix (`TestAuthorize*`, `TestPKCE*`, `TestOrderedRedirect*`); `804da83` (feat, fonoteka.go): mounts the raw route, wires config into `Options`, and adds `TestOAuthAuthorizeAssembled`, `TestOAuthAuthorizeInvalidRequestsCreateNoPendingRows`, `TestOAuthAuthorizeRawRouteSurface`. + +**Plan metadata:** committed as part of this summary/state-update commit. + +_Note: both tasks carry `tdd="true"`; RED/GREEN pairs land as separate commits, and Task 2 splits its GREEN across the two repositories it touches._ + +## Files Created/Modified + +- `wristband/authorize.go` — `Server.Authorize`, `writeAuthorizeLocalError`, `authorizeErrorRedirect`, `parseAuthorizeScopes`, `stringSliceContains`, `rfc3986Escape`/`isRFC3986Unreserved`, `buildOrderedQuery`, `appendOrderedQuery` +- `wristband/authorize_test.go` — `TestPhase8RedAuthorize` (RED anchor) plus the full unit matrix: unknown/revoked client, unregistered/trailing-slash redirect, response-type/PKCE-method/PKCE-length failures, valid success + pending-row assertions, `iss`-on-every-error, scope default/invalid/ceiling truncation/ceiling-rejection (4 cases mirroring the PHP test file), resource mismatch/omission, RFC3986 encoding fixtures, backend-unavailable 500 +- `wristband/server.go` — `Options` gains `Resource` and `PendingRequestTTL` with PHP-parity defaults +- `../fonoteka.go/plugins/golem15/fonoteka/plugin.go` — wires `golem15.fonoteka.oauth.resource`/`.pending_request_ttl_seconds` into `wristband.Options` +- `../fonoteka.go/plugins/golem15/fonoteka/routes.go` — mounts `GET /oauth/mcp/authorize` on the raw group with no middleware +- `../fonoteka.go/plugins/golem15/fonoteka/oauth_authorize_test.go` — `TestPhase8RedAuthorizeApp` (RED anchor), `TestOAuthAuthorizeAssembled`, `TestOAuthAuthorizeInvalidRequestsCreateNoPendingRows`, `TestOAuthAuthorizeRawRouteSurface` + +## Decisions Made + +See frontmatter `key-decisions`. Most notable: `Options` was extended with `Resource`/`PendingRequestTTL` even though `wristband/server.go` was not in the plan's `` list for either task — this follows the exact precedent 08-02 set when it extended the same struct for DCR's cap/sweep/body-limit options, and is necessary to make the resource check and 600s TTL configurable rather than hardcoded (Rule 2 — auto-add missing critical functionality implied by the task's own action text: "600-second expiry using the configured server/backend from 08-02"). + +## Deviations from Plan + +**1. [Rule 2 - Missing functionality] Extended `wristband.Options` with `Resource` and `PendingRequestTTL`** +- **Found during:** Task 2 (implementing the resource check and pending-request expiry) +- **Issue:** The plan's action text requires the resource check and 600-second expiry to be "configured" through the server, but neither value existed on `Options` yet (only DCR-related fields were added in 08-02) +- **Fix:** Added `Options.Resource` (default `"https://mcp.plytarium.com/mcp"`) and `Options.PendingRequestTTL` (default `600 * time.Second"`) to `DefaultOptions()`, and wired both from `golem15.fonoteka.oauth.resource`/`.pending_request_ttl_seconds` config in `plugin.go` +- **Files modified:** `wristband/server.go`, `../fonoteka.go/plugins/golem15/fonoteka/plugin.go` +- **Verification:** `TestAuthorizeResourceMismatchRedirectsInvalidTarget`, `TestAuthorizeResourceOmittedIsAccepted`, and the pending-row `ExpiresAt` assertion in `TestAuthorizeValidRequestRedirectsToConnectWithOpaqueHandleOnly` all pass +- **Committed in:** `90752be` (Task 2 GREEN commit, summercms.go) + +--- + +**Total deviations:** 1 auto-fixed (1 missing functionality) +**Impact on plan:** No scope change; a struct-field addition following an established precedent, required by the task's own stated action. + +## Issues Encountered + +Full-suite verification (`go vet`/`go test ./...` in both repos, per CLAUDE.md) surfaced two pre-existing, out-of-scope failures unrelated to this plan's changed files — logged to `deferred-items.md` rather than fixed here (scope-boundary rule): + +- `fonoteka.go`'s root-module `parity` package: `TestRemainingMigrationsUpDown` and `TestRollbackIsolatesFonotekaFullSchema` fail because 08-02's `202609230019_oauth_schema_correction` migration is now the last registered migration, which these Phase-5-era tests' hardcoded "last migration" assertions don't know about. Neither test file nor any migration/model file is in this plan's `files_modified`. +- `summercms.go`'s `fetchguard` package: `TestFetchTooLargeIsStreaming` is intermittently flaky under the full `go test ./...` run but passes reliably in isolation; `fetchguard` is untouched by this plan. + +The module this plan actually modifies (`fonoteka.go/plugins/golem15/fonoteka`) is fully green, including `go vet` and `go test -race ./...`. + +## User Setup Required + +None — no external service configuration required. + +## Next Phase Readiness + +- The RFC 3986 ordered-query encoder (`buildOrderedQuery`/`appendOrderedQuery`) is ready for 08-04's token-endpoint redirects and 08-05's consent/deny redirects to reuse directly. +- The pending `AuthCodeRecord` created by `Authorize` (hash-only-free, `RequestID` set, `CodeHash`/`UserID` nil) is exactly the shape 08-05's consent flow needs to read via `AuthCodeStore.ByRequestID` and turn into an issued code via `MarkIssued`. +- `Options.Resource`/`Options.PendingRequestTTL` establish the pattern for 08-04 to add `Options.CodeTTL`/`AccessTokenTTL`/`RefreshTokenTTL` the same way. +- AUTH-05/AUTH-06/AUTH-07 remain Pending in REQUIREMENTS.md, continuing 08-01/08-02's decision: this plan ships authorize only; consent, token exchange, and refresh remain for 08-04/08-05. +- No blockers. + +## Self-Check: PASSED + +- FOUND: wristband/authorize.go, wristband/authorize_test.go, wristband/server.go, .planning/phases/08-oauth2-1-authorization-server/08-03-SUMMARY.md +- FOUND: ../fonoteka.go/plugins/golem15/fonoteka/plugin.go, routes.go, oauth_authorize_test.go +- FOUND commits (summercms.go): 787e612, 90752be +- FOUND commits (fonoteka.go): 57049f8, 804da83 diff --git a/.planning/phases/08-oauth2-1-authorization-server/deferred-items.md b/.planning/phases/08-oauth2-1-authorization-server/deferred-items.md new file mode 100644 index 0000000..72e1c37 --- /dev/null +++ b/.planning/phases/08-oauth2-1-authorization-server/deferred-items.md @@ -0,0 +1,43 @@ +# Phase 08 Deferred Items + +Out-of-scope discoveries logged during plan execution, per the executor's +scope-boundary rule (fix only what the current task's changes directly +caused). + +## 08-03: pre-existing full-schema rollback test failures (not caused by this plan) + +**Found during:** 08-03 Task 2 full-suite verification (`go test ./...` in `fonoteka.go`). + +**Failing tests:** `TestRemainingMigrationsUpDown` (`parity/remaining_models_test.go`), +`TestRollbackIsolatesFonotekaFullSchema` (`parity/rollback_isolation_full_test.go`). + +**Symptom:** both tests assert the *last* fonoteka migration is +`create_fonoteka_settings` (a Phase 5 migration) and/or that a full +up/down/up cycle leaves no tables behind. Since 08-02 added +`202609230019_oauth_schema_correction.go` (the corrective OAuth +nullability/index migration, 08-02-SUMMARY.md), that migration is now the +last one in the registered slice, so the hardcoded "last migration" name +assertion is stale, and rollback of the corrected schema leaves +`golem15_fonoteka_settings` behind. + +**Scope:** neither test file, nor `plugins/golem15/fonoteka/updates/`, nor +any model file is in 08-03's `files_modified` list; this plan (authorize) +touches `wristband/authorize.go`, `wristband/server.go` (Options +extension), `plugin.go`, and `routes.go` only. Confirmed pre-existing via +`git log` on the failing test files: both were last touched by Phase 5 +(`6c9695f`), and 08-02's migration-correction commit (`4536b3e`) is what +shifted the "last migration" identity without updating these two tests. + +**Disposition:** deferred to whichever later Phase 8 plan owns migration/ +schema test maintenance (or the phase-closing unit-test plan). Not fixed +here per the executor's scope-boundary rule. + +## 08-03: pre-existing flaky test in an unrelated package (summercms.go) + +**Found during:** 08-03 Task 2 full-suite verification (`go test ./...` in `summercms.go`). + +**Failing test:** `TestFetchTooLargeIsStreaming` (`fetchguard/fetch_test.go`), +intermittently fails with "server wrote 71680 bytes, client appears to have +buffered unbounded body" under `go test ./...` but passes reliably when run +in isolation (`go test ./fetchguard -run TestFetchTooLargeIsStreaming +-count=3`). `fetchguard` is untouched by this plan. Not fixed here.