diff --git a/.agent-loop/LOOP_STATE.md b/.agent-loop/LOOP_STATE.md index e9c8d731..44018d78 100644 --- a/.agent-loop/LOOP_STATE.md +++ b/.agent-loop/LOOP_STATE.md @@ -4,20 +4,22 @@ - Active initiative: `WS-AUTH-001` - Workstream Authorization Service - Active planning chunk: none -- Active implementation chunk: none -- Branch: `codex/ws-auth-001-04a-post-merge-memory` -- Worktree: `/home/abiorh/flow/workstream-authorization-service` -- Status: AUTH-04A merged through PR #111 as `90c9a28` after Backend, Agent - Gates, CodeRabbit, required internal review, and explicit human approval - passed. +- Active implementation chunk: `WS-AUTH-001-04B` - PostgreSQL Rate Controls +- Branch: `codex/ws-auth-001-04b-postgres-rate-controls` +- Worktree: `/home/abiorh/flow/workstream-auth-001-04b` +- Status: AUTH-04B implementation and all required internal review tracks pass + final implementation SHA `922778b`; reviewed production SHA is `67484b5`. + Ready PR #113 is open. - Prior `WS-AUTH-001-01` reviewed implementation SHA: `be0b836` - Prior `WS-AUTH-001-01` final merged branch head: `b5217e1` -- Latest integrated `main` merge commit: `90c9a28` -- Current gate: publish and merge the AUTH-04A post-merge memory update, then - stop. +- Latest integrated `main` merge commit: `7749f54` +- Current gate: GitHub checks, CodeRabbit, and explicit human review on PR #113. +- Size checkpoint: implementation is 480 changed non-comment production lines; + the required 350-line inspection passed and the 500-line hard stop holds. - Next chunk: none; do not start `WS-AUTH-001-05` automatically. -- `WS-AUTH-001-04B` is inactive until this memory update merges and the user - gives its separate explicit start signal. +- Focused evidence: 77 isolated rate/config tests pass at 97 percent subsystem + coverage, 59 final config/object-graph tests pass, the exact migration proof + and real API E2E pass. AUTH-05 and later chunks remain inactive. - Parallel initiative: `WS-QUAL-001-01B2` is paused at the user's direction so AUTH receives the laptop's test capacity. Its last official whole-app result remains `6466/8159` statements (`79.249908%`); no replacement evidence exists. diff --git a/.agent-loop/REVIEW_LOG.md b/.agent-loop/REVIEW_LOG.md index 43372398..bc924d89 100644 --- a/.agent-loop/REVIEW_LOG.md +++ b/.agent-loop/REVIEW_LOG.md @@ -1,5 +1,181 @@ # Review Log +## 2026-07-14 - WS-AUTH-001-04B Backend CI Repair + +PR #113 Backend reached 82.12 percent repository coverage but failed because +the final AUTH-04B Alembic test left the shared isolated database at `base`. +Pytest's CI discovery order then entered `test_api_rate_controls.py` without +table `api_rate_control_counters`. The owning migration test now restores +Alembic `head` after its destructive cleanup. The exact failing +`test_alembic.py -> test_api_rate_controls.py` order is required repair proof; +no runtime, migration, coverage threshold, or workflow changes are permitted. +CodeRabbit posted one valid test-deduplication nit, now addressed through a +transaction-preserving shared insert helper. Its PR-description template +warning is addressed in the trust bundle; its docstring heuristic is superseded +by the passing repository-owned 95.8 percent docstring gate. + +## 2026-07-14 - WS-AUTH-001-04B Ready PR Published + +Ready PR #113 is open at +`https://github.com/Flow-Research/workstream/pull/113`. GitHub Backend, Agent +Gates, CodeRabbit, and explicit human review are the current gate. Publication +is not merge approval; AUTH-05 and artifact runtime remain inactive. + +## 2026-07-14 - WS-AUTH-001-04B Internal Review Passed + +All required tracks pass final SHA `922778b`; reviewed production SHA is +`67484b5`. Senior engineering, architecture, docs, reuse, QA/test, CI integrity, +test delta, security/auth, privacy/data, and product/ops found no remaining +blocker. The final test-only proof instruments the BaseSettings boundary and +would fail the prior recoverable-`SecretStr` implementation. Evidence and the +PR trust bundle are recorded under the AUTH initiative reviews directory. + +Ready PR publication and external GitHub checks are the current gate. AUTH-05, +artifact runtime, and all route attachments remain inactive. + +## 2026-07-14 - WS-AUTH-001-04B Exact-Head Review Repair + +Required implementation review at `8f54fd9` passed QA, CI integrity, and test +delta with 72 fresh isolated PostgreSQL tests. Senior engineering and security +rejected PR publication because unrelated Pydantic validation errors could +retain the raw rate-control secret through structured error APIs. Security also +found that the canonical inactive artifact plan did not yet enforce D14's +central authorization prerequisite for adapter I/O. + +The bounded repair removes the secret from Pydantic's public field graph before +all structured validation while preserving constructor, environment, and dotenv +loading. Regression tests inspect `errors()` and `json()` and cover +`model_validate`. The user-directed artifact ownership clarification amends only +the canonical ART plan and inactive chunk contract: mechanics may be developed +independently, but production dispatch requires AUTH-07 permissions, AUTH-09 +service principals, exact resource authorization evidence, and never derives +authority from a credential or receipt. No artifact runtime or AUTH-05 behavior +is activated. + +Initial repair evidence is 78 migration-inclusive isolated PostgreSQL behavior tests, +69 focused rate-control/config tests at 98 percent subsystem coverage, and the +real API contract E2E. Ruff, docstring coverage, stale authorization/artifact/ +Workstream wording, Markdown links, loop memory, diff integrity, and all 36 +agent-gate tests pass. The repaired production delta is 465 non-comment lines, +below the 500-line hard stop. Exact-SHA internal re-review is the current gate. + +Exact-head review of `0b9d0fe` passed QA, CI integrity, and test delta, but +senior engineering and security found the same retention class in +`model_validate_json` and `model_validate_strings`; malformed JSON could also +retain its complete document. Senior engineering additionally found that the +manual secret source did not preserve BaseSettings' supported layered-dotenv +precedence. The final bounded repair covers those public entry points, rejects +malformed JSON with a constant non-retaining error, and resolves layered dotenv +files in order. Exact-SHA re-review remains mandatory before publication. + +Final repair evidence adds 77 focused behavior tests at 98 percent subsystem +coverage, 56 direct configuration tests, and the exact AUTH-04B migration test +passing against a fresh isolated database. The production delta is 480 +non-comment lines and remains below the 500-line hard stop. All static gates +continue to pass. + +Exact-head review of `629d10c` found that rendered redaction was still weaker +than object-graph non-retention: Pydantic-version-dependent error input could +retain a recoverable `SecretStr`, malformed JSON remained reachable through +`JSONDecodeError.__context__`, and non-ASCII rejection chained a +`UnicodeEncodeError` containing the original key. The final privacy repair +passes only `None` into Pydantic, assigns the private validated key after +successful validation, and raises constant parse/decode failures outside catch +scopes. Tests traverse structured errors and complete exception cause/context +graphs rather than checking rendered output alone. + +The object-graph repair passes 77 focused behavior tests at 97 percent +subsystem coverage. The final production delta is 480 non-comment lines, below +the 500-line hard stop. + +QA passed runtime and CI integrity at `67484b5` but rejected the test proof: +the recursive helper did not traverse `ValidationError.errors()`, so rendered +redaction could hide a recoverable `SecretStr` on another Pydantic version. The +test-only repair now traverses the actual structured error objects and +instruments the `BaseSettings` boundary for mapping, JSON, and string-mapping +validation, asserting that only `None` crosses into Pydantic. This +deterministically fails the prior implementation that forwarded `SecretStr`. + +## 2026-07-13 - WS-AUTH-001-04B Repaired Contract Passed + +Implementation candidate `62dd18e` failed required exact-head review. Valid +findings were structured Pydantic secret retention, unpaired-surrogate identity +500s, a concurrent downgrade custody race, unsynchronized database tests, +incomplete downstream-rollback/session-open/prune/database-clock evidence, no +representative 0016 artifact row, and tests mistakenly placed in the prior +AUTH-04A file instead of the contract-owned rate-control file. One bounded +repair cycle moves the tests, closes each runtime/migration issue, adds exact +proof, and requires full-suite coverage plus exact-head re-review. + +The repository-wide coverage run for repair head `2d70581` was interrupted by +the host shutdown on 2026-07-14 and produced no valid result. Under the current +repository rule and the chunk's laptop-capacity clause, local evidence must +prove the materially changed AUTH-04B subsystem remains at least 90 percent; +GitHub CI owns the unchanged repository-wide 78 percent gate. No interrupted +result is treated as evidence. + +Required senior engineering, architecture, security/data, QA/test, CI-integrity, +test-delta, product/ops, docs, and reuse review passed exact repaired-contract +head `b5dceb1`. The second repair closed optional-secret, missing-database, +lock-order, oversized-identity, exact-setting/constraint, same-clock timestamp, +and cross-replica rotation gaps without runtime edits. + +Bounded AUTH-04B implementation is authorized under the 350-line checkpoint and +500-line hard stop. Named dependencies remain unattached; AUTH-05 and all +product authority changes remain inactive. + +`backend/tests/test_auth.py` was added as a test-only scope amendment after its +canonical-verification consumer allowlist correctly detected the new unattached +rate dependency. Only that expected inventory may change; auth runtime, routes, +compatibility behavior, and authority remain out of scope. + +The implementation candidate passed 93 owned tests with 99 percent aggregate +coverage across config, dependency, model, repository, and service modules. +Real PostgreSQL proofs passed for atomic concurrency, durable denial, expiry, +saturation, pruning, exact schema, and guarded downgrade. The real API E2E and +all stale-wording, authorization-doc, artifact-contract, link, loop-state, +ruff, and diff checks pass. Required implementation review is the current gate. + +The first production pass reached 408 changed non-comment lines. The mandatory +350-line checkpoint inspected every production path and froze scope to the +approved config, unattached dependencies, model, repository, service, model +registration, and `0017` migration. Ninety-two lines remain before the hard +stop; tests, docs, and evidence do not count. + +## 2026-07-13 - WS-AUTH-001-04B Contract Repair Required + +Required L1 preimplementation review rejected activation head `5ed410d` before +any runtime edit. The contract did not bind exact HMAC/secret bytes, overflow- +safe SQL, database-time boundaries, dedicated commit ownership, dependency +errors, real concurrency counts, transactional downgrade refusal, or expired +pseudonymous-row retention. + +The repaired contract specifies canonical Base64 secret material, exact framed +HMAC input and bounds, `BYTEA` persistence guarded to 32 bytes, one clock-bound saturating +upsert, dedicated committed sessions, canonical 429/503 behavior, bounded +pruning plus operator cleanup, exact migration custody, real-ASGI/concurrency +proof, per-file 90 percent coverage, and additive test-delta evidence. Runtime +remains gated on exact-head re-review. + +First repaired head `78e2170` resolved the original security/data ambiguities +but failed re-review on optional-secret syntax, missing-database mapping, +prune-before-upsert lock ordering, exact setting/constraint names, same-clock +`updated_at`, and local oversized-identity handling. This second repair is the +final in-place contract cycle for those classes; another failure stops and +replans before runtime code. + +## 2026-07-13 - WS-AUTH-001-04B Started + +PR #112 merged AUTH-04A post-merge memory to `main` as `7749f54` after Backend, +Agent Gates, CodeRabbit, required internal review, and explicit human approval +passed. The user then explicitly started `WS-AUTH-001-04B` in the isolated +worktree `/home/abiorh/flow/workstream-auth-001-04b` on branch +`codex/ws-auth-001-04b-postgres-rate-controls`. + +AUTH-04B is L1/P1 authorization infrastructure. Its existing bounded contract +must pass required preimplementation review before runtime edits. AUTH-05, +POL-002-04, and QA implementation remain inactive. + ## 2026-07-13 - WS-AUTH-001-04A Merged PR #111 merged `WS-AUTH-001-04A` to `main` as `90c9a28` after final branch head diff --git a/.agent-loop/WORK_QUEUE.md b/.agent-loop/WORK_QUEUE.md index a3a1c11b..f47c0d85 100644 --- a/.agent-loop/WORK_QUEUE.md +++ b/.agent-loop/WORK_QUEUE.md @@ -4,13 +4,12 @@ | Chunk | Title | Risk | Status | |---|---|---:|---| -| - | No active implementation chunk | - | AUTH-04A post-merge memory in progress; no product implementation active | +| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Internal review passed; PR #113 in external review | ## Planned Next | Chunk | Title | Risk | Status | |---|---|---:|---| -| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Inactive until 04A merge/memory and a separate explicit user start | | `WS-QUAL-001-01B2` | Baseline Evidence And CI Ratchet | L1 | Paused for AUTH priority; no valid replacement baseline yet | | `WS-QUAL-001-02` | Project Service Coverage | L1 | Inactive until 01B2 merge/memory plus explicit user start | | `WS-POL-002-04` | Locked Runtime Execution And Routing Hardening | L1 | Inactive pending relevant authorization proof and a separate explicit user start | @@ -57,9 +56,9 @@ ## Proposed Next -`WS-AUTH-001-04A` merged through PR #111 as `90c9a28` after all required checks -and reviews passed. Its post-merge memory update is active; no AUTH product -implementation is active. Do not start 04B, AUTH-05, or POL-002-04 +`WS-AUTH-001-04A` post-merge memory merged through PR #112 as `7749f54`. The +user explicitly started `WS-AUTH-001-04B`; all required internal review tracks +pass and ready PR #113 is in external review. Do not start AUTH-05 or POL-002-04 automatically. Coverage R10 merged through PR #108. Do not start 01B2, chunk 02, or another diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md index 56dd8fa8..40919613 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md @@ -261,7 +261,12 @@ approval, locking, and deterministic checker compilation. ## WS-AUTH Dependencies -- Internal artifact foundations and Flow Node work may proceed independently. +- Internal artifact schemas and adapter mechanics may proceed independently, + but production dispatch and every external adapter I/O remain disabled until + `WS-AUTH-001-07` defines the exact operation permission and + `WS-AUTH-001-09` provisions the calling service principal. Each dispatch must + carry a central authorization decision for the exact operation and resource; + an adapter credential or provider receipt never creates authority. - Guide source API cutover waits for WS-AUTH project mutation cutover (`WS-AUTH-001-12`) or its approved replacement. - Upload session and submission cutovers wait for task/submission/checker diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-02-flow-node-adapter-reconciliation.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-02-flow-node-adapter-reconciliation.md index ffb00264..52f7187e 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-02-flow-node-adapter-reconciliation.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-02-flow-node-adapter-reconciliation.md @@ -5,7 +5,9 @@ Parent: `WS-ART-001` | Repository: Workstream | Risk: L1 | SLA: P1 ## Goal Implement `FlowNodeAdapter`, metadata-only operation outbox/reconciliation, -pinned provider delivery, and local integration. +pinned provider delivery, and local integration. Production dispatch remains +disabled until the central AUTH permission and service-principal prerequisites +are merged. ## Allowed Files @@ -20,7 +22,8 @@ pinned provider delivery, and local integration. ## Not Allowed No byte payload in DB/Redis/outbox, product cutover, provider fallback, human -token forwarding, or provider-triggered lifecycle transition. +token forwarding, provider-triggered lifecycle transition, or adapter I/O that +bypasses a central authorization decision for the exact operation and resource. ## Acceptance Criteria @@ -38,6 +41,11 @@ token forwarding, or provider-triggered lifecycle transition. visibility. - Reconciliation, quarantine, retain, and release product audit events use the shared `AuditRepository`; provider receipts remain operation evidence only. +- Production upload, read, retain, release/delete, replicate, verify, and + reconcile dispatch requires the exact permission from `WS-AUTH-001-07` and an + explicitly provisioned service principal from `WS-AUTH-001-09`. Mechanical + adapter work may be tested before those chunks merge, but production dispatch + stays disabled and no receipt or adapter credential creates authority. - Concurrent dispatch/reconcile has one monotonic provider effect and one canonical receipt outcome; row counts are asserted. - Same v1 vectors pass LocalStorageAdapter and a Flow Node image pinned by full @@ -48,9 +56,10 @@ token forwarding, or provider-triggered lifecycle transition. populated-state preservation, empty downgrade, and re-upgrade are owned and proved in `test_alembic.py`; otherwise the chunk records that no migration was created. -- Compose health proves adapter-level authenticated ingest/retrieve/status from - Workstream's service principal directly through `FlowNodeAdapter`; it does not - claim a public Workstream artifact API before the product cutover. +- Compose health proves test-only adapter mechanics for ingest/retrieve/status. + Production service-principal dispatch additionally requires central AUTH + decision evidence and remains disabled before the owning AUTH cutovers; this + chunk does not claim a public Workstream artifact API. ## Verification diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md index aa89b886..7d595b5c 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md @@ -17,7 +17,7 @@ stopped. | `WS-AUTH-001-03` | Legacy Actor Classification Preflight | L1 | Merged through PR #109 as `f06532e` | | `WS-AUTH-001-04` | Request, Error, And API Control Foundation | L1 | Split before implementation into 04A and 04B | | `WS-AUTH-001-04A` | Request And Error Context | L1 | Merged through PR #111 as `90c9a28` | -| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Inactive pending 04A memory merge and separate explicit start | +| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Internal review passed; PR #113 in external review | | `WS-AUTH-001-05` | Authority Evidence And Idempotency Foundation | L1 | Proposed | | `WS-AUTH-001-06` | Canonical Actor Profile And Identity Link | L1 | Proposed | | `WS-AUTH-001-07` | Authorization Kernel And Permission Registry | L1 | Proposed | @@ -68,6 +68,11 @@ WS-AUTH-001-PLAN - Chunk 07 provides the single authorization engine before grant APIs. - Chunks 08-10 establish local grant truth before product cutover. - Chunks 11-15 migrate bounded complete product/system surfaces. +- Artifact upload, read, retention, release/delete, replication, integrity, and + reconciliation remain mechanically owned by the artifact subsystem but must + receive centralized AUTH decisions. Chunk 07 owns the permission-registry + boundary, chunk 09 owns artifact service principals, the applicable 11-15 + resource cutovers attach enforcement, and chunk 16 proves no bypass remains. - Chunk 16 proves the complete initiative; it does not backfill missing audit or idempotency evidence. - `WS-POL-002-03` merged separately through PR #90 as `a7aa474`. This initiative @@ -79,8 +84,9 @@ WS-AUTH-001-PLAN AUTH-03 post-merge memory merged through PR #110 as `1864867`. The user explicitly started parent AUTH-04. Required plan review split it before runtime -implementation. AUTH-04A merged through PR #111 as `90c9a28` after required -internal review, Backend, Agent Gates, CodeRabbit, and explicit human approval -passed. Its post-merge memory update is the current gate. -Do not start AUTH-04B, AUTH-05, or POL-002-04; each retains its separate start -signal and prerequisites. +implementation. AUTH-04A merged through PR #111 as `90c9a28`, and its +post-merge memory merged through PR #112 as `7749f54`. The user explicitly +started AUTH-04B. Its repaired contract passed at `b5dceb1`; bounded +implementation and all required internal review tracks pass final SHA +`922778b`. Ready PR #113 is in external review. Stop; do not start AUTH-05 or +POL-002-04. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md index 5a0e4141..9decb311 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md @@ -187,3 +187,28 @@ until the shared convention merges, and must then remove the old AUTH factory entry point and migrate all callers in one clean cut. It must be planned and reviewed before implementation; no compatibility factory alias, duplicate verifier, service locator, or public login/session API may be introduced. + +## D14: Artifact operations use centralized authorization + +Status: accepted by the user on 2026-07-14. + +The artifact subsystem owns upload/storage, retention, replication, integrity, +and reconciliation mechanics behind the shared object-storage adapter +foundation. It does not own or infer product authority. Every human-initiated +or internal-service artifact operation must be authorized by Workstream's one +central authorization kernel using an operation-specific permission and the +exact project/resource scope before an adapter performs external I/O. + +Human callers resolve through canonical `ActorProfile`, identity-link status, +and the applicable administrative or exact-project contributor grant. Artifact +storage, retention, and reconciliation services authenticate as explicitly +provisioned service principals with registered system permissions; they are not +Contributors and never receive fabricated human roles. Upload, read, retain, +release/delete, replicate, verify, and reconcile authority are distinct and do +not imply one another. + +The authorization decision and operation receipt must share bounded request/ +correlation evidence, resource identity, operation, and service principal. +Receipts prove that storage work occurred; they do not create authority. Exact +permission tokens and route ownership are locked in the owning AUTH cutover +chunk contracts. AUTH-04B adds no artifact permission or route attachment. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/RISKS.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/RISKS.md index 767f5d1a..7e6fe721 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/RISKS.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/RISKS.md @@ -33,6 +33,7 @@ | A17 | Canonical actor migration deletes typed-profile workflow eligibility before task/submission cutover | An intermediate merged release cannot claim, start, or submit work | Bounded non-authoritative workflow-eligibility adapter in chunk 06; remove task consumers in 13 and final consumer plus adapter in 14 | Full suite/API drill after chunks 06, 13, 14, and scanner proof in 15 | | A18 | Authority evidence is mutable or denial events arrive late | Security decisions cannot be reconstructed | Insert/read-only repository API, database append-only enforcement, per-mutation allowed/denied event proof, operational retention controls | Update/delete rejection plus atomic allowed/denied event tests in owning chunks | | A19 | Authority invalidation releases a needs-revision task as ordinary ready work | Prior findings, context, supersession, or replay obligations are bypassed | Keep the task in needs_revision with a durable unassigned obligation and controlled replacement assignment | Replacement-contributor revision-context, supersession, and high/medium replay tests | +| A20 | Rate-limit replicas split counters, leak identity keys, lose increments, or retain pseudonymous rows indefinitely | Abuse controls bypassed or privacy/availability incident | Canonical Base64 HMAC secret, exact framed digest, one committed PostgreSQL statement, coordinated quiesced rotation, bounded opportunistic pruning, and operator idle cleanup | Known-answer/privacy tests, synchronized independent-session concurrency, commit-failure proof, rotation runbook, and retention tests | ## Required reviewers diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md index 287de74d..cb75b71a 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md @@ -34,7 +34,14 @@ required tracks pass on production SHA `cdcaf77`, and final test-only head logging-state behavior-test repairs plus lifecycle evidence. Backend, Agent Gates, and CodeRabbit passed on final branch head `36c4aa5`, then explicit human approval merged PR #111 to `main` as `90c9a28` on 2026-07-13. -AUTH-04A post-merge memory is active; AUTH-04B remains inactive. +AUTH-04A post-merge memory merged through PR #112 as `7749f54`. The user then +explicitly started AUTH-04B; its required L1 preimplementation review rejected +the first activated contract before runtime edits. The second +repaired contract passed all required tracks at `b5dceb1`; bounded runtime +implementation and deterministic evidence are complete, and the candidate is +internally approved at final SHA `922778b`; reviewed production SHA is +`67484b5`. Ready PR #113 is open; GitHub checks, CodeRabbit, and explicit human +review are the current gate. ## Active planning chunk @@ -42,12 +49,12 @@ None. ## Active implementation chunk -None. +`WS-AUTH-001-04B` - PostgreSQL Rate Controls. Internal review passed; AUTH-05 +remains inactive. ## Current implementation branch -None. Post-merge memory uses `codex/ws-auth-001-04a-post-merge-memory` and makes -no product implementation change. +`codex/ws-auth-001-04b-postgres-rate-controls`. ## Chunk status @@ -59,7 +66,7 @@ no product implementation change. | `WS-AUTH-001-03` | Merged | `codex/ws-auth-001-03-legacy-actor-classification` | #109 | Merged as `f06532e`; reviewed code `8c5334c`; final branch head `43ffbfe`. | | `WS-AUTH-001-04` | Split | `codex/ws-auth-001-04-request-api-controls` | - | Parent split before runtime implementation. | | `WS-AUTH-001-04A` | Merged | `codex/ws-auth-001-04-request-api-controls` | #111 | Merged as `90c9a28`; production review `cdcaf77`; final branch head `36c4aa5`. | -| `WS-AUTH-001-04B` | Inactive | - | - | PostgreSQL rate controls; requires 04A merge/memory and separate explicit start. | +| `WS-AUTH-001-04B` | External review | `codex/ws-auth-001-04b-postgres-rate-controls` | #113 | All required tracks pass final `922778b`; production review `67484b5`. | | `WS-AUTH-001-05` | Proposed | - | - | Authority evidence and idempotency foundation. | | `WS-AUTH-001-06` | Proposed | - | - | Canonical actor profile and identity link. | | `WS-AUTH-001-07` | Proposed | - | - | Authorization kernel and permissions. | @@ -75,13 +82,16 @@ no product implementation change. ## Blockers -No active implementation blocker because no AUTH product chunk is active. -AUTH-04B may start only after this memory update merges and the user gives a -separate explicit start signal. It later owns migration `0017`, following the +No external blocker. AUTH-04B internal review passed under its repaired L1 +contract. It owns migration `0017`, following the now-owned `0016` prefix on current main. Non-test operators must later supply explicit classification evidence rather than inferred kinds before the owning canonical actor migration. +AUTH-04B review evidence and its PR trust bundle are recorded at +`reviews/WS-AUTH-001-04B-internal-review-evidence.md` and +`reviews/WS-AUTH-001-04B-pr-trust-bundle.md`. + AUTH-04A review evidence and its PR trust bundle are recorded at `reviews/WS-AUTH-001-04A-internal-review-evidence.md` and `reviews/WS-AUTH-001-04A-pr-trust-bundle.md`. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md index 8b95deb0..ae26dc96 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md @@ -2,8 +2,9 @@ ## Status -Inactive. This contract requires AUTH-04A merge/memory, a separate explicit -user start, and required preimplementation review before any runtime edit. +Active for bounded implementation. Required preimplementation review passed the +second repaired contract at `b5dceb1`; the 350-line checkpoint and 500-line hard +stop apply. ## Parent initiative @@ -23,6 +24,8 @@ L1 / P1 - Stop at 350 changed non-comment production lines for a scope checkpoint. - Stop and replan above 500 changed non-comment production lines. +- Production count includes `backend/app/**` and the Alembic migration. Tests, + docs, and `.agent-loop` evidence do not count. ## Allowed files @@ -36,10 +39,14 @@ backend/app/modules/api_controls/service.py backend/app/db/models.py backend/alembic/versions/0017_api_controls.py backend/tests/test_api_rate_controls.py +backend/tests/test_auth.py backend/tests/test_config.py backend/tests/test_alembic.py backend/scripts/api_contract_e2e.py docs/operations_authorization_service.md +docs/architecture_data_model.md +.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md +.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-02-flow-node-adapter-reconciliation.md .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/** .agent-loop/LOOP_STATE.md .agent-loop/WORK_QUEUE.md @@ -58,68 +65,160 @@ AUTH-05 or later chunk implementation ## Persistence and transaction contract -- Persist one row per `(control_scope, key_digest)` with bounded scope, fixed - HMAC-SHA256 digest length, `window_started_at`, `window_expires_at`, - `request_count`, and `updated_at` using timezone-aware database timestamps. -- Enforce nonempty bounded scope, exact digest length, positive bounded count, - ordered window timestamps, uniqueness, and the lookup/expiry indexes required - by the executed query. -- One PostgreSQL upsert uses one `statement_timestamp()` value to reset an - expired window or atomically increment an active window and returns database - now, window start/end, and count. Saturate at the database integer maximum. +- Create only `api_rate_control_counters` with `control_scope VARCHAR(32)`, + `key_digest BYTEA`, `window_started_at TIMESTAMPTZ`, + `window_expires_at TIMESTAMPTZ`, `request_count BIGINT`, and + `updated_at TIMESTAMPTZ`, all non-null. The composite primary key is + `(control_scope, key_digest)`; there is no actor or surrogate identifier. +- Name the constraints `pk_api_rate_control_counters`, + `ck_api_rate_control_counters_scope_token`, + `ck_api_rate_control_counters_digest_length`, + `ck_api_rate_control_counters_request_count`, and + `ck_api_rate_control_counters_window_order`. They enforce the composite key, + exact scope tokens, `octet_length(key_digest) = 32`, `request_count between 1 + and 9223372036854775807`, and `window_started_at < window_expires_at`. Create + `ix_api_rate_control_counters_window_expires_at`, which is consumed by the + pruning query. +- Windows are first-consumption-anchored fixed windows. One PostgreSQL upsert + binds a `clock` CTE to one `statement_timestamp()` value. It inserts count 1; + on conflict it resets start/end/count when existing `window_expires_at <=` + database now, otherwise it increments. Active-window saturation must use + `CASE WHEN request_count = 9223372036854775807 THEN request_count ELSE + request_count + 1 END`; never evaluate max plus one. New expiry is database + now plus parameterized `make_interval(secs => :window_seconds)`. +- The one statement returns the CTE database now, persisted window start/end, + and persisted count. `updated_at` is set to that same CTE database timestamp + on insert, expiry reset, ordinary increment, and saturated denial. All ORM + timestamps are timezone aware. - Counts `1..limit` allow. Count `limit+1` and later deny and remain durably consumed. `Retry-After` is the ceiling of remaining database-time seconds, - clamped to `1..window_seconds`. -- Invoke the upsert in a dedicated short-lived session/transaction and commit - before returning either allow or 429. A downstream request rollback cannot - undo rate consumption. + clamped to integer `1..window_seconds`; application time is never consulted. +- The service accepts an injectable async session factory whose production + default is the existing `get_session_factory()`. Every consume opens an + independent short-lived session, performs bounded pruning plus the upsert, + and commits before returning a decision. The dependency may construct/raise + 429 only after the committed decision returns. Downstream rollback cannot + undo consumption. +- Session-open, prune, execute, and commit `SQLAlchemyError` failures map to one + local unavailable error and then a constant retryable structured 503. The + existing `get_session_factory()` missing-database `RuntimeError` maps narrowly + by its exact stable configuration failure; every other `RuntimeError` is + re-raised. A failed transaction is rolled back without overriding the stable + error. Cancellation and other `BaseException` values propagate. Tests cover + absent database configuration separately from open/execute/commit failures. ## Privacy and configuration contract -- Derive `key_digest` with HMAC-SHA256 using domain - `workstream-api-rate/v1`, the bounded control scope, and exact issuer plus - opaque subject bytes framed by four-byte big-endian lengths and UTF-8 values. +- The exact server-owned scope tokens are ASCII `first_access` and + `admin_mutation`; callers cannot supply another scope. +- Derive `key_digest` with HMAC-SHA256. The key setting is a Pydantic + `SecretStr` containing canonical padded RFC 4648 Base64. Decode strictly with + no whitespace or alternate encoding; require 32..64 decoded bytes and the + canonical re-encoding to equal the supplied ASCII string. Pydantic repr and + validation errors must never expose the value. +- The HMAC preimage is exactly + `len(domain)||domain||len(scope)||scope||len(issuer)||issuer||len(subject)||subject`, + in that order. Every length is an unsigned four-byte big-endian byte length. + Domain is exact ASCII `workstream-api-rate/v1`; scope is its exact ASCII + token; issuer and subject are exact UTF-8. Do not trim, case-fold, or Unicode + normalize. Enforce issuer and subject independently at 1..4096 UTF-8 bytes + before framing. Commit literal known-answer vectors and collision-separation + tests that do not calculate expected values through the production helper. - Persist only scope and digest. Never persist or log raw issuer, subject, actor ID, email, role, token, claims, or network address. -- Add `api_rate_limit_key_secret` with no default and minimum 32-byte secret - material. Never render or log it. Present malformed values fail settings - construction. -- Defaults are first-access `10/60s` and admin-mutation `30/60s`, with limit - bounds `1..10000` and window bounds `1..3600`. +- Expose `api_rate_limit_key_secret` as `SecretStr | None`, backed by private + settings storage after constructor, environment, or dotenv input is removed + from Pydantic's structured validation graph. There is no non-null fallback + and no generated secret. Present + malformed, non-ASCII, noncanonical, whitespace-bearing, too-short, or + too-long values fail settings construction. Unrelated Pydantic failures and + alternate `model_validate`, `model_validate_json`, and + `model_validate_strings` entry points must not retain the raw key in + structured error APIs, recoverable `SecretStr` objects, or exception cause/ + context graphs. Malformed settings JSON fails with a constant error that does + not retain the supplied document anywhere in its exception graph. Layered + dotenv files preserve BaseSettings' later-file precedence. +- Exact numeric settings are `api_first_access_rate_limit=10`, + `api_first_access_rate_window_seconds=60`, + `api_admin_mutation_rate_limit=30`, and + `api_admin_mutation_rate_window_seconds=60`, with limit bounds `1..10000` and + window bounds `1..3600`. - Missing secret remains startup-compatible while no route is attached, but - invoking a protected dependency without it returns a safe retryable 503. - Database unavailability also fails closed as safe retryable 503. + invoking a protected dependency without it returns structured HTTP 503 with + code `service_unavailable`, message/detail `Service unavailable`, empty + details, and `retryable=true`. Database unavailability uses the same response. - Secret rotation intentionally resets effective counters. The runbook requires - quiescing protected writes for at most the largest configured window before - replacing the secret. + quiescing every protected write for at least the largest effective + pre-rotation window configured on any replica, rotating every replica while + writes remain quiesced, pruning expired rows, and only then resuming. + Mixed-secret replicas must never serve protected writes. + +## Retention contract + +- The service owns bounded opportunistic pruning in the same dedicated + transaction after its upsert: delete at most 100 other rows ordered by expiry + whose `window_expires_at <= statement_timestamp()`, using row locking with + `SKIP LOCKED`. Upsert-first lock ordering ensures a concurrently consumed key + is already active/locked and skipped rather than creating a two-key lock + cycle. A synchronized two-expired-key PostgreSQL test proves no deadlock or + 503. +- Expired rows are no longer rate authority. The runbook owns an operator + cleanup command for idle systems and secret rotation; it deletes only expired + rows by database time. Tests prove pruning is bounded, concurrent-safe, and + never persists or emits identity material. ## Route boundary and behavior proof - Export named first-access and admin-mutation dependencies for later owning chunks, but attach neither dependency here. -- Inventory path/method and dependency consumers to prove no route or consumer - changed. +- Each dependency consumes only `AuthVerificationResult.token.issuer` and + `.subject` from `get_auth_verification_result`. It must not consume + `ActorContext`, `get_registered_actor`, actor ID, roles, email, or an + issuer-specific adapter. Scope, limit, and window are server constants plus + validated settings. +- A verified issuer or subject outside the local 1..4096 UTF-8 byte boundary is + treated as the same non-echoing local unavailable condition and structured + retryable `service_unavailable` 503, never an unhandled 500. +- A denied dependency raises `StructuredHTTPException` 429 with code + `rate_limit_exceeded`, message/detail `Rate limit exceeded`, empty details, + `retryable=true`, and canonical integer `Retry-After`. Reuse 04A handlers and + database/session infrastructure; introduce no parallel error or DB layer. +- Inventory path/method and the recursive dependency graph to prove no + production route or consumer changed. Exercise dependencies only through + test-only FastAPI routes using real ASGI transport and 04A handlers. - Prove allow, exact limit, repeated exceed, expiry reset, distinct key/scope, concurrent increments, cross-session visibility, downstream rollback independence, missing/malformed secret, database unavailable, bounded - `Retry-After`, and privacy-safe persistence. + `Retry-After`, subsecond and application-clock-skew behavior, and privacy-safe + persistence/logging. +- Synchronized real-PostgreSQL concurrency uses independently created sessions + with `N > limit` and proves exactly `limit` allows, `N-limit` denials, final + durable count `N`, repeated-denial consumption, no lost increments, and + saturation at BIGINT max without overflow. ## Migration proof - `0017_api_controls` revises `0016_artifact_domain`. -- Prove `0016 -> 0017 -> 0016 -> 0017`, seeded-row preservation across the - supported direction, unique/check/index enforcement, invalid insert - rejection, and the explicit nonempty-table downgrade policy. +- Seed representative existing `0016` domain rows and prove they survive the + upgrade unchanged. Prove every named primary-key/check/index contract and + reject invalid direct inserts. +- Downgrade must query the rate table and raise a bounded `RuntimeError` when it + is nonempty. PostgreSQL/Alembic transaction rollback must leave its rows, + table, and revision at `0017`. After explicit test-only cleanup, prove empty + `0017 -> 0016 -> 0017` succeeds. Never claim a rate row survives a successful + downgrade that drops its table. ## Verification commands ```bash (cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q tests/test_api_rate_controls.py tests/test_config.py tests/test_alembic.py) -(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q --cov=app.modules.api_controls --cov=app.api.deps.api_controls --cov-report=term-missing --cov-fail-under=90 tests/test_api_rate_controls.py tests/test_config.py tests/test_alembic.py) +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q --cov=app.modules.api_controls --cov=app.api.deps.api_controls --cov=app.core.config --cov-report=term-missing tests/test_api_rate_controls.py tests/test_config.py tests/test_alembic.py) +(cd backend && for file in app/core/config.py app/api/deps/api_controls.py app/modules/api_controls/models.py app/modules/api_controls/repository.py app/modules/api_controls/service.py; do .venv/bin/coverage report --include="$file" --precision=2 --fail-under=90; done) (cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py --cov=app --cov-report=term-missing --cov-fail-under=78) (cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py) (cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python scripts/api_contract_e2e.py) (cd backend && .venv/bin/python -m ruff check app tests scripts) +(cd backend && .venv/bin/docstr-coverage --config .docstr.yaml) python3 scripts/check_loop_memory_state.py python3 scripts/check_stale_workstream_wording.py python3 scripts/check_stale_authorization_docs.py @@ -127,9 +226,16 @@ python3 scripts/check_stale_artifact_contracts.py python3 scripts/check_markdown_links.py python3 scripts/check_internal_review_evidence.py python3 scripts/test_agent_gates.py +git diff --unified=0 origin/main...HEAD -- backend/tests | (! rg '^-(.*assert|.*pytest\.raises|.*pytest\.mark\.(skip|xfail)|.*skipTest)') git diff --check ``` +`backend/tests/test_auth.py` was added as a test-only scope amendment after the +implemented canonical-verification consumer correctly tripped its static +consumer allowlist. The amendment may update only that allowlist for the new +unattached `api/deps/api_controls.py` consumer; it does not permit auth runtime, +route, compatibility, or authority changes. + ## Required reviewers - senior engineering @@ -149,3 +255,9 @@ git diff --check - Stop if a raw identity or token-derived value would be persisted or logged. - Stop if production changes exceed the size circuit breaker. - Stop after 04B; do not start AUTH-05 automatically. + +## Activation + +AUTH-04A post-merge memory merged through PR #112 as `7749f54`. The user +explicitly started AUTH-04B on 2026-07-13. Required L1 preimplementation review +passed the second repaired contract at `b5dceb1` before runtime edits. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-external-review-response.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-external-review-response.md new file mode 100644 index 00000000..e13dbe1b --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-external-review-response.md @@ -0,0 +1,51 @@ +# External Review Response: WS-AUTH-001-04B + +## Pull Request + +PR #113 - Add durable PostgreSQL authorization rate controls + +## Backend CI Failure + +The first Backend run reached 82.12 percent repository coverage but failed in +test setup. The final AUTH-04B Alembic test downgraded the shared isolated +database to `base`; CI discovery then entered the rate-control suite without +table `api_rate_control_counters`. The owning migration test now restores +Alembic `head` after destructive cleanup. The exact failing order passes 22/22. + +## CodeRabbit Findings + +One valid nitpick is addressed: repeated raw counter inserts in concurrency, +saturation, and pruning tests now use one `_insert_counter_row` helper while +preserving transaction ownership and exact timestamp/count inputs. + +The PR-description warning is addressed by adding the missing human intent, +acceptance proof, evidence, reviewer table, and CI/gate-integrity sections to +the trust bundle and published PR body. + +The CodeRabbit docstring warning is not a repository failure. The authoritative +`docstr-coverage --config .docstr.yaml` gate passes at 95.8 percent, and the +Backend docstring step passed. Adding narration-only docstrings to private test +and implementation helpers would not improve behavior or ownership clarity. + +CodeRabbit's later incremental run was rate-limited and produced no additional +actionable thread. + +## Commands Rerun + +```bash +(cd backend && isolated-runner pytest -q tests/test_alembic.py::test_api_rate_control_schema_preserves_domain_and_guards_downgrade tests/test_api_rate_controls.py) +(cd backend && ruff check tests/test_alembic.py tests/test_api_rate_controls.py) +python3 scripts/check_loop_memory_state.py +python3 scripts/check_markdown_links.py +git diff --unified=0 origin/main -- backend/tests | (! rg '^-(.*assert|.*pytest\.raises|.*pytest\.mark\.(skip|xfail)|.*skipTest)') +git diff --check +``` + +- Exact CI failure order: 22 passed. +- No assertion, raises, skip, xfail, threshold, workflow, runtime, or migration + implementation was weakened. + +## Remaining Gate + +Push the reviewed repair and require Backend, Agent Gates, CodeRabbit status, +and explicit human review on the new PR head. Publication is not merge approval. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-internal-review-evidence.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-internal-review-evidence.md new file mode 100644 index 00000000..96684cd2 --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-internal-review-evidence.md @@ -0,0 +1,101 @@ +# Internal Review Evidence: WS-AUTH-001-04B + +## Chunk + +`WS-AUTH-001-04B` - PostgreSQL Rate Controls + +open sub-agent sessions: none + +valid findings addressed: yes + +## Reviewed Revision + +Reviewed code SHA: `4fb846c76029dc0bebb11ef90982207f162b54c9` + +Reviewed production SHA: `67484b508637b7fc893e441e7afb5f5b34dd7715` + +Reviewed at: 2026-07-14T04:39:18Z + +Reviewer run IDs: engineering-architecture-docs-reuse=AUTH-04B-4FB846C-ENG-20260714; +qa-ci-test-delta=AUTH-04B-4FB846C-QA-20260714; +security-privacy-product=AUTH-04B-4FB846C-SEC-20260714 + +The final SHA differs from the reviewed production SHA only by additive tests +and review memory. Exact-head review confirmed identical production blobs and +that the boundary test fails the prior recoverable-`SecretStr` behavior. + +## Reviewed Change + +- Added PostgreSQL-owned fixed-window counters with database-clock boundaries, + atomic saturating upsert, bounded lock-safe pruning, and durable denial. +- Added privacy-keyed HMAC-SHA256 derivation and unattached first-access and + administrative-mutation dependencies with canonical 429/503 behavior. +- Added guarded migration `0017_api_controls` and exact preservation/downgrade + custody proof. +- Kept the rate key outside Pydantic's structured input/error graphs across + constructor, environment, layered dotenv, and all public model validators. +- Bound future artifact adapter I/O to central AUTH-07 permissions, AUTH-09 + service principals, and exact operation/resource authorization evidence. + +## Reviewer Results + +| Reviewer | Result | Blocking findings | Notes | +|---|---:|---|---| +| senior engineering | PASS | None | Database ownership, configuration privacy, and the 480-line bounded implementation are maintainable. | +| QA/test | PASS | None | Behavior, concurrency, migration, failure, and secret object-graph tests are genuine and mutation-sensitive. | +| security/auth | PASS | None | No raw or recoverable rate key remains in persistence, structured errors, or exception graphs. | +| product/ops | PASS | None | Dependencies remain unattached; artifact authority stays central and later chunks remain inactive. | +| architecture | PASS | None | Reuses shared sessions/errors and introduces no parallel authorization or storage authority path. | +| CI integrity | PASS | None | No workflow, threshold, exclusion, dependency, or bypass was weakened. | +| docs | PASS | None | AUTH operations and inactive ART contracts match the implemented boundaries. | +| reuse/dedup | PASS | None | One repository/service/dependency path owns rate controls. | +| test delta | PASS | None | Additive tests; no assertion, raises, skip, xfail, or skipTest weakening. | + +## Valid Findings Addressed + +- Made session-open, prune, execute, commit, rollback, cancellation, and + missing-database behavior explicit and privacy bounded. +- Proved concurrent increments, downstream rollback independence, database + time, same-clock `updated_at`, saturation, and deadlock-safe pruning. +- Locked downgrade before checking emptiness and preserved representative + `0016` artifact rows. +- Removed the secret from Pydantic's public field graph and closed constructor, + environment, layered-dotenv, mapping, JSON, string-mapping, malformed JSON, + non-ASCII, and exception-chain retention paths. +- Added deterministic BaseSettings boundary instrumentation proving that only + `None`, never a raw value or `SecretStr`, enters Pydantic validation. +- Reconciled ART planning so credentials and receipts never create authority. + +## Verification Results + +- focused isolated PostgreSQL rate/config matrix: 77 passed; +- changed subsystem statement coverage: 97 percent aggregate; dependency 96, + config 97, model 100, repository 100, and service 99 percent; +- final direct configuration/object-graph suite: 59 passed; +- exact AUTH-04B migration test: 1 passed, 8 deselected; +- Backend CI repair order (AUTH-04B migration test followed by rate suite): + 22 passed; the migration test restores Alembic head after cleanup; +- earlier migration-inclusive repaired matrix: 78 passed; +- real API contract E2E: passed; +- Ruff, docstring threshold, stale wording, authorization docs, artifact + contracts, Markdown links, loop memory, 36 Agent Gates, additive test delta, + and diff hygiene: passed. + +The repository-wide local coverage run was interrupted by the host shutdown and +is not claimed. GitHub Backend retains the unchanged 78 percent global floor; +the materially changed subsystem exceeds the required 90 percent threshold. +The first PR Backend run reached 82.12 percent but failed on test-owned schema +cleanup; the bounded test isolation repair requires a final Backend rerun. + +## Evidence Gate + +Evidence gate: PASS WITH GITHUB GLOBAL SUITE PENDING + +The reviewed implementation is 480 changed non-comment production lines, +below the 500-line hard stop. It adds no protected-route consumer, actor role, +grant, product permission, artifact runtime, or AUTH-05 behavior. + +## Stop Condition + +Publish a ready PR, then stop for GitHub Backend, Agent Gates, CodeRabbit, and +explicit human review. Do not merge or start AUTH-05 automatically. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-pr-trust-bundle.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-pr-trust-bundle.md new file mode 100644 index 00000000..2fb9d22b --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04B-pr-trust-bundle.md @@ -0,0 +1,128 @@ +# PR Trust Bundle: WS-AUTH-001-04B + +## Chunk + +`WS-AUTH-001-04B` - PostgreSQL Rate Controls + +## Goal + +Provide privacy-keyed, cross-replica PostgreSQL rate controls for future first +access and authority mutations without attaching them to production routes. + +## Human-Approved Intent + +The user explicitly started AUTH-04B after AUTH-04A and its post-merge memory +merged. The approved boundary is a cross-replica PostgreSQL rate-control +foundation only: dependencies remain unattached, product authority remains in +later AUTH chunks, and only the user may approve PR merge. + +## What Changed + +- Added atomic database-time fixed-window counters with durable denial, + saturation safety, and bounded lock-safe pruning. +- Added HMAC-keyed server dependencies for `first_access` and + `admin_mutation`; both remain unattached. +- Added migration `0017_api_controls` with guarded downgrade custody. +- Added non-retaining secret configuration across constructor, environment, + layered dotenv, and Pydantic's public model-validation methods. +- Clarified that artifact production I/O requires central AUTH permissions, + service principals, and exact resource authorization before adapter work. + +## Design + +Each consume owns a short independent committed session. One PostgreSQL upsert +uses one `statement_timestamp()` CTE to insert, reset, increment, saturate, and +return authoritative window state. HMAC framing stores only scope and a 32-byte +digest. The server key is private `SecretStr` state assigned only after +successful Pydantic validation; Pydantic receives no recoverable key object. + +## Scope Control + +The production delta is 480 non-comment lines, below the 500-line hard stop. +No route consumer, actor/grant behavior, product authority, artifact runtime, +AUTH-05 implementation, Redis counter, or in-memory cross-replica state was +added. + +## Behavior Proof + +Tests cover exact limits, repeated denial, expiry, database-clock retry values, +concurrency, cross-session durability, downstream rollback, saturation, +pruning, unavailable database phases, cancellation, privacy-safe persistence, +schema constraints, migration races, and representative artifact preservation. +Secret tests traverse structured error objects and complete exception graphs +and instrument the BaseSettings boundary directly. + +## Acceptance Criteria Proof + +- PostgreSQL owns one clock-bound atomic upsert, durable denial, saturation, + and bounded lock-safe pruning. +- Persistence contains only exact scope and a 32-byte HMAC digest. +- Constructor, environment, layered dotenv, and all public validation methods + retain no raw or recoverable secret in structured errors or exception graphs. +- Migration proof covers populated `0016` preservation, exact schema guards, + refusal on nonempty downgrade, writer races, empty downgrade, and re-upgrade. +- Route/dependency inventory proves neither new dependency is attached. +- Artifact adapter I/O remains inactive behind central AUTH-07/AUTH-09 gates. + +## Evidence Commands And Results + +- 77 focused isolated PostgreSQL tests passed at 97 percent changed-subsystem + statement coverage. +- 59 final configuration/object-graph tests passed. +- Exact AUTH-04B migration and real API E2E passed. +- The exact CI failure order now passes 22/22 after the migration test restores + Alembic head following destructive cleanup. +- Ruff, docstring threshold, stale scans, links, loop memory, 36 Agent Gates, + additive test delta, and diff hygiene passed. +- GitHub Backend must provide the unchanged repository-wide 78 percent floor; + the interrupted local global run is not evidence. + +## Reviewer Results + +| Reviewer | Result | Blocking findings | +|---|---:|---| +| Senior engineering / architecture / docs / reuse | PASS | None | +| QA / test delta / CI integrity | PASS | None | +| Security / privacy / product ops | PASS | None | + +Reviewed implementation SHA is `922778b`; reviewed production SHA is +`67484b5`. Later reviewed heads contain only tests, evidence, and lifecycle +memory unless explicitly recorded in the external-review response. + +## CI And Gate Integrity + +- [x] No workflow, threshold, exclusion, skip, or coverage bypass changed. +- [x] Changed AUTH-04B subsystem remains above 90 percent coverage. +- [x] Repository docstring gate passes at 95.8 percent. +- [x] Internal evidence, loop memory, stale scans, links, and Agent Gates pass. +- [ ] Backend must pass the repaired exact PR head at the 78 percent global floor. +- [ ] Explicit human review and merge approval remain pending. + +## External Review + +Ready PR #113 is open. Agent Gates passed on the prior published head. +CodeRabbit completed its available review with one valid test-deduplication +nit addressed; its later incremental review was rate-limited. The first Backend +run reached 82.12 percent coverage but exposed test-owned schema cleanup. All +final-head external checks and explicit human review remain pending. None +replaces the completed internal review. + +## Remaining Risks + +- The dependencies are intentionally unattached; later owning chunks must add + route-specific permissions and consumer tests. +- Secret rotation resets effective counters and requires the documented + cross-replica quiescence procedure. +- The global suite and 78 percent repository floor remain GitHub-owned because + the local run was interrupted by host shutdown. + +## Human Review Focus + +Inspect transaction custody, HMAC framing/privacy, database-time math, pruning +lock order, migration downgrade refusal, secret object-graph non-retention, and +the fact that no production route consumes either dependency. + +## Human Merge Ownership + +Only the user may approve and merge this PR. Publication is not merge approval, +and AUTH-05 does not start automatically. diff --git a/backend/alembic/versions/0017_api_controls.py b/backend/alembic/versions/0017_api_controls.py new file mode 100644 index 00000000..a8f5c41b --- /dev/null +++ b/backend/alembic/versions/0017_api_controls.py @@ -0,0 +1,71 @@ +"""add durable API rate controls + +Revision ID: 0017_api_controls +Revises: 0016_artifact_domain +Create Date: 2026-07-13 +""" + +from __future__ import annotations + +from alembic import op +import sqlalchemy as sa + +revision = "0017_api_controls" +down_revision = "0016_artifact_domain" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + """Create the privacy-keyed cross-replica counter table.""" + op.create_table( + "api_rate_control_counters", + sa.Column("control_scope", sa.String(32), nullable=False), + sa.Column("key_digest", sa.LargeBinary(), nullable=False), + sa.Column("window_started_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("window_expires_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("request_count", sa.BigInteger(), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint( + "control_scope", + "key_digest", + name="pk_api_rate_control_counters", + ), + sa.CheckConstraint( + "control_scope in ('first_access', 'admin_mutation')", + name=op.f("ck_api_rate_control_counters_scope_token"), + ), + sa.CheckConstraint( + "octet_length(key_digest) = 32", + name=op.f("ck_api_rate_control_counters_digest_length"), + ), + sa.CheckConstraint( + "request_count between 1 and 9223372036854775807", + name=op.f("ck_api_rate_control_counters_request_count"), + ), + sa.CheckConstraint( + "window_started_at < window_expires_at", + name=op.f("ck_api_rate_control_counters_window_order"), + ), + ) + op.create_index( + "ix_api_rate_control_counters_window_expires_at", + "api_rate_control_counters", + ["window_expires_at"], + ) + + +def downgrade() -> None: + """Drop only an empty rate-control table.""" + bind = op.get_bind() + bind.execute(sa.text("lock table api_rate_control_counters in access exclusive mode")) + has_rows = bind.execute( + sa.text("select exists(select 1 from api_rate_control_counters)") + ).scalar_one() + if has_rows: + raise RuntimeError("cannot downgrade non-empty API rate controls") + op.drop_index( + "ix_api_rate_control_counters_window_expires_at", + table_name="api_rate_control_counters", + ) + op.drop_table("api_rate_control_counters") diff --git a/backend/app/api/deps/api_controls.py b/backend/app/api/deps/api_controls.py new file mode 100644 index 00000000..62af6f70 --- /dev/null +++ b/backend/app/api/deps/api_controls.py @@ -0,0 +1,97 @@ +"""Unattached FastAPI dependencies for future protected mutations.""" + +from __future__ import annotations + +from typing import Annotated + +from fastapi import Depends, Request, status + +from app.api.deps.auth import get_auth_verification_result +from app.core.api_controls import StructuredHTTPException +from app.modules.api_controls.service import ( + ADMIN_MUTATION_SCOPE, + FIRST_ACCESS_SCOPE, + RateControlService, + RateControlUnavailableError, +) +from app.schemas.auth import AuthVerificationResult + + +def get_rate_control_service() -> RateControlService: + """Return a limiter whose consumption owns a dedicated database session.""" + return RateControlService() + + +def _unavailable() -> StructuredHTTPException: + return StructuredHTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Service unavailable", + error_code="service_unavailable", + error_message="Service unavailable", + retryable=True, + ) + + +async def _enforce( + *, + request: Request, + result: AuthVerificationResult, + service: RateControlService, + control_scope: str, + limit: int, + window_seconds: int, +) -> None: + try: + decision = await service.consume( + control_scope=control_scope, + issuer=result.token.issuer, + subject=result.token.subject, + limit=limit, + window_seconds=window_seconds, + secret=request.app.state.settings.api_rate_limit_key_secret, + ) + except RateControlUnavailableError as exc: + raise _unavailable() from exc + if not decision.allowed: + raise StructuredHTTPException( + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + detail="Rate limit exceeded", + error_code="rate_limit_exceeded", + error_message="Rate limit exceeded", + retryable=True, + headers={"Retry-After": str(decision.retry_after)}, + ) + + +async def enforce_first_access_rate_limit( + request: Request, + result: Annotated[AuthVerificationResult, Depends(get_auth_verification_result)], + service: Annotated[RateControlService, Depends(get_rate_control_service)], +) -> None: + """Consume the future first-access mutation allowance.""" + settings = request.app.state.settings + await _enforce( + request=request, + result=result, + service=service, + control_scope=FIRST_ACCESS_SCOPE, + limit=settings.api_first_access_rate_limit, + window_seconds=settings.api_first_access_rate_window_seconds, + ) + + +async def enforce_admin_mutation_rate_limit( + request: Request, + result: Annotated[AuthVerificationResult, Depends(get_auth_verification_result)], + service: Annotated[RateControlService, Depends(get_rate_control_service)], +) -> None: + """Consume the future authority-management mutation allowance.""" + settings = request.app.state.settings + await _enforce( + request=request, + result=result, + service=service, + control_scope=ADMIN_MUTATION_SCOPE, + limit=settings.api_admin_mutation_rate_limit, + window_seconds=settings.api_admin_mutation_rate_window_seconds, + ) diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 85de4fcd..da44220c 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -2,12 +2,23 @@ from __future__ import annotations +import base64 +import binascii +import json +import os +from collections.abc import Mapping from functools import lru_cache from pathlib import Path -from typing import Literal +from typing import Literal, Self -from pydantic import Field, model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from dotenv import dotenv_values +from pydantic import Field, PrivateAttr, SecretStr, model_validator +from pydantic_settings import ( + BaseSettings, + DotEnvSettingsSource, + PydanticBaseSettingsSource, + SettingsConfigDict, +) class Settings(BaseSettings): @@ -76,6 +87,10 @@ class Settings(BaseSettings): celery_result_backend_url: str | None = None celery_task_always_eager: bool = False actor_registry_refresh_interval_seconds: int = Field(default=300, ge=0, le=86_400) + api_first_access_rate_limit: int = Field(default=10, ge=1, le=10_000) + api_first_access_rate_window_seconds: int = Field(default=60, ge=1, le=3_600) + api_admin_mutation_rate_limit: int = Field(default=30, ge=1, le=10_000) + api_admin_mutation_rate_window_seconds: int = Field(default=60, ge=1, le=3_600) artifact_store_backend: Literal["disabled", "local", "flow_node"] = "disabled" artifact_local_root: Path | None = None artifact_retention_policy_version: str | None = None @@ -86,8 +101,79 @@ class Settings(BaseSettings): env_prefix="WORKSTREAM_", env_file=".env", extra="ignore", + hide_input_in_errors=True, ) + _api_rate_limit_key_secret: SecretStr | None = PrivateAttr(default=None) + + def __init__(self, **values: object) -> None: + """Remove rate-control secret material before structured validation.""" + secret = _extract_api_rate_limit_key_secret(values) + super().__init__(**values) + self._api_rate_limit_key_secret = secret + + @classmethod + def model_validate(cls, obj: object, **kwargs: object) -> Self: + """Sanitize secret-bearing mappings before Pydantic retains input.""" + if isinstance(obj, Mapping) and "api_rate_limit_key_secret" in obj: + sanitized = dict(obj) + secret = _extract_api_rate_limit_key_secret(sanitized) + sanitized["api_rate_limit_key_secret"] = None + settings = super().model_validate(sanitized, **kwargs) + settings._api_rate_limit_key_secret = secret + return settings + return super().model_validate(obj, **kwargs) + + @classmethod + def model_validate_json( + cls, + json_data: str | bytes | bytearray, + **kwargs: object, + ) -> Self: + """Sanitize JSON secret input before Pydantic retains the document.""" + malformed = False + try: + parsed = json.loads(json_data) + except (json.JSONDecodeError, UnicodeDecodeError): + malformed = True + parsed = None + if malformed: + raise ValueError("invalid settings JSON") + if isinstance(parsed, Mapping) and "api_rate_limit_key_secret" in parsed: + return cls.model_validate(parsed, **kwargs) + return super().model_validate_json(json_data, **kwargs) + + @classmethod + def model_validate_strings(cls, obj: object, **kwargs: object) -> Self: + """Sanitize string-mapping secret input before structured validation.""" + if isinstance(obj, Mapping) and "api_rate_limit_key_secret" in obj: + sanitized = dict(obj) + secret = _extract_api_rate_limit_key_secret(sanitized) + sanitized["api_rate_limit_key_secret"] = None + settings = super().model_validate_strings(sanitized, **kwargs) + settings._api_rate_limit_key_secret = secret + return settings + return super().model_validate_strings(obj, **kwargs) + + @classmethod + def settings_customise_sources( + cls, + settings_cls: type[BaseSettings], + init_settings: PydanticBaseSettingsSource, + env_settings: PydanticBaseSettingsSource, + dotenv_settings: PydanticBaseSettingsSource, + file_secret_settings: PydanticBaseSettingsSource, + ) -> tuple[PydanticBaseSettingsSource, ...]: + """Keep the manually resolved rate key out of dotenv validation input.""" + if isinstance(dotenv_settings, DotEnvSettingsSource): + dotenv_settings.env_vars.pop("workstream_api_rate_limit_key_secret", None) + return init_settings, env_settings, dotenv_settings, file_secret_settings + + @property + def api_rate_limit_key_secret(self) -> SecretStr | None: + """Return the validated, redacted rate-control key setting.""" + return self._api_rate_limit_key_secret + @model_validator(mode="after") def validate_artifact_storage(self) -> Settings: """Reject unsafe or incomplete artifact-storage combinations. @@ -120,3 +206,53 @@ def get_settings() -> Settings: Settings resolved from environment variables and optional ``.env``. """ return Settings() + + +def _extract_api_rate_limit_key_secret(values: dict[str, object]) -> SecretStr | None: + """Resolve and validate the key without entering Pydantic's input graph.""" + missing = object() + raw_value = values.pop("api_rate_limit_key_secret", missing) + if raw_value is missing: + raw_value = os.environ.get("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", missing) + if raw_value is missing: + env_file = values.get("_env_file", ".env") + if env_file is not None: + env_encoding = values.get("_env_file_encoding", "utf-8") + env_files = ( + (env_file,) + if isinstance(env_file, (str, os.PathLike)) + else tuple(env_file) + ) + for path in env_files: + dotenv = dotenv_values(path, encoding=env_encoding) + raw_value = dotenv.get("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", raw_value) + if raw_value is missing or raw_value is None: + return None + if isinstance(raw_value, SecretStr): + secret = raw_value + elif isinstance(raw_value, str): + secret = SecretStr(raw_value) + else: + raise ValueError("invalid API rate limit key secret") + decode_api_rate_limit_key_secret(secret) + return secret + + +def decode_api_rate_limit_key_secret(value: SecretStr) -> bytes: + """Decode one canonical padded Base64 rate-control key.""" + secret = value.get_secret_value() + if not secret.isascii(): + raise ValueError("invalid API rate limit key secret") + invalid = False + try: + encoded = secret.encode("ascii") + decoded = base64.b64decode(encoded, validate=True) + except (binascii.Error, ValueError): + invalid = True + encoded = b"" + decoded = b"" + if invalid: + raise ValueError("invalid API rate limit key secret") + if not 32 <= len(decoded) <= 64 or base64.b64encode(decoded) != encoded: + raise ValueError("invalid API rate limit key secret") + return decoded diff --git a/backend/app/db/models.py b/backend/app/db/models.py index 947d31dc..67e2d1df 100644 --- a/backend/app/db/models.py +++ b/backend/app/db/models.py @@ -1,6 +1,7 @@ """Import domain models so Alembic can discover metadata.""" from app.modules.actors.models import ActorIdentity, ActorProfile # noqa: F401 +from app.modules.api_controls.models import ApiRateControlCounter # noqa: F401 from app.modules.artifacts.models import ( # noqa: F401 ArtifactBinding, ArtifactContent, diff --git a/backend/app/modules/api_controls/__init__.py b/backend/app/modules/api_controls/__init__.py new file mode 100644 index 00000000..1a5b9bd2 --- /dev/null +++ b/backend/app/modules/api_controls/__init__.py @@ -0,0 +1 @@ +"""Durable API control primitives.""" diff --git a/backend/app/modules/api_controls/models.py b/backend/app/modules/api_controls/models.py new file mode 100644 index 00000000..a99dafb2 --- /dev/null +++ b/backend/app/modules/api_controls/models.py @@ -0,0 +1,42 @@ +"""SQLAlchemy models for durable API controls.""" + +from __future__ import annotations + +from datetime import datetime + +from sqlalchemy import CheckConstraint, DateTime, Index, LargeBinary, BigInteger, String +from sqlalchemy.orm import Mapped, mapped_column + +from app.db.base import Base + + +class ApiRateControlCounter(Base): + """Privacy-keyed fixed-window counter shared by every API replica.""" + + __tablename__ = "api_rate_control_counters" + __table_args__ = ( + CheckConstraint( + "control_scope in ('first_access', 'admin_mutation')", + name="scope_token", + ), + CheckConstraint("octet_length(key_digest) = 32", name="digest_length"), + CheckConstraint( + "request_count between 1 and 9223372036854775807", + name="request_count", + ), + CheckConstraint( + "window_started_at < window_expires_at", + name="window_order", + ), + Index( + "ix_api_rate_control_counters_window_expires_at", + "window_expires_at", + ), + ) + + control_scope: Mapped[str] = mapped_column(String(32), primary_key=True) + key_digest: Mapped[bytes] = mapped_column(LargeBinary, primary_key=True) + window_started_at: Mapped[datetime] = mapped_column(DateTime(timezone=True)) + window_expires_at: Mapped[datetime] = mapped_column(DateTime(timezone=True)) + request_count: Mapped[int] = mapped_column(BigInteger) + updated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True)) diff --git a/backend/app/modules/api_controls/repository.py b/backend/app/modules/api_controls/repository.py new file mode 100644 index 00000000..9072015e --- /dev/null +++ b/backend/app/modules/api_controls/repository.py @@ -0,0 +1,101 @@ +"""PostgreSQL statements for durable API controls.""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime + +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncSession + +UPSERT_COUNTER = text( + """ + with clock as (select statement_timestamp() as db_now) + insert into api_rate_control_counters as counters ( + control_scope, key_digest, window_started_at, window_expires_at, + request_count, updated_at + ) + select :control_scope, :key_digest, db_now, + db_now + make_interval(secs => :window_seconds), 1, db_now + from clock + on conflict (control_scope, key_digest) do update set + window_started_at = case + when counters.window_expires_at <= (select db_now from clock) + then (select db_now from clock) else counters.window_started_at end, + window_expires_at = case + when counters.window_expires_at <= (select db_now from clock) + then (select db_now from clock) + make_interval(secs => :window_seconds) + else counters.window_expires_at end, + request_count = case + when counters.window_expires_at <= (select db_now from clock) then 1 + when counters.request_count = 9223372036854775807 + then counters.request_count else counters.request_count + 1 end, + updated_at = (select db_now from clock) + returning (select db_now from clock) as db_now, window_started_at, + window_expires_at, request_count + """ +) + +PRUNE_EXPIRED = text( + """ + with expired as ( + select control_scope, key_digest + from api_rate_control_counters + where window_expires_at <= statement_timestamp() + and not (control_scope = :control_scope and key_digest = :key_digest) + order by window_expires_at + limit :batch_size + for update skip locked + ) + delete from api_rate_control_counters as counters + using expired + where counters.control_scope = expired.control_scope + and counters.key_digest = expired.key_digest + """ +) + + +@dataclass(frozen=True) +class ConsumedCounter: + """Database-time result of one atomic counter consumption.""" + + db_now: datetime + window_started_at: datetime + window_expires_at: datetime + request_count: int + + +class ApiRateControlRepository: + """Consume and prune counters inside a caller-owned transaction.""" + + def __init__(self, session: AsyncSession) -> None: + self._session = session + + async def consume( + self, control_scope: str, key_digest: bytes, window_seconds: int + ) -> ConsumedCounter: + """Atomically insert, reset, increment, or saturate one counter.""" + row = ( + await self._session.execute( + UPSERT_COUNTER, + { + "control_scope": control_scope, + "key_digest": key_digest, + "window_seconds": window_seconds, + }, + ) + ).one() + return ConsumedCounter(*row) + + async def prune_expired( + self, control_scope: str, key_digest: bytes, *, batch_size: int + ) -> None: + """Delete a bounded batch of other expired pseudonymous rows.""" + await self._session.execute( + PRUNE_EXPIRED, + { + "control_scope": control_scope, + "key_digest": key_digest, + "batch_size": batch_size, + }, + ) diff --git a/backend/app/modules/api_controls/service.py b/backend/app/modules/api_controls/service.py new file mode 100644 index 00000000..3bd35c1d --- /dev/null +++ b/backend/app/modules/api_controls/service.py @@ -0,0 +1,136 @@ +"""Privacy-keyed rate-control service with independent commit ownership.""" + +from __future__ import annotations + +from contextlib import suppress +from dataclasses import dataclass +import hashlib +import hmac +import math +from typing import TypeAlias + +from pydantic import SecretStr +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from app.core.config import decode_api_rate_limit_key_secret +from app.db.session import get_session_factory +from app.modules.api_controls.repository import ApiRateControlRepository + +RATE_KEY_DOMAIN = b"workstream-api-rate/v1" +FIRST_ACCESS_SCOPE = "first_access" +ADMIN_MUTATION_SCOPE = "admin_mutation" +RATE_SCOPES = {FIRST_ACCESS_SCOPE, ADMIN_MUTATION_SCOPE} +MAX_IDENTITY_BYTES = 4_096 +PRUNE_BATCH_SIZE = 100 +MISSING_DATABASE_ERROR = "WORKSTREAM_DATABASE_URL must be set before database access" +SessionFactory: TypeAlias = async_sessionmaker[AsyncSession] + + +class RateControlUnavailableError(RuntimeError): + """Raised when a rate decision cannot be made safely.""" + + +@dataclass(frozen=True) +class RateControlDecision: + """Committed fixed-window decision derived from database time.""" + + allowed: bool + request_count: int + retry_after: int + + +def _frame(value: bytes) -> bytes: + return len(value).to_bytes(4, "big") + value + + +def rate_key_digest( + secret: SecretStr, control_scope: str, issuer: str, subject: str +) -> bytes: + """Derive the exact privacy key for one verified external identity.""" + if control_scope not in RATE_SCOPES: + raise ValueError("unsupported rate control scope") + try: + issuer_bytes = issuer.encode("utf-8") + subject_bytes = subject.encode("utf-8") + except UnicodeEncodeError: + raise RateControlUnavailableError("rate control unavailable") from None + if not 1 <= len(issuer_bytes) <= MAX_IDENTITY_BYTES or not 1 <= len( + subject_bytes + ) <= MAX_IDENTITY_BYTES: + raise RateControlUnavailableError("rate control unavailable") + preimage = b"".join( + _frame(value) + for value in ( + RATE_KEY_DOMAIN, + control_scope.encode("ascii"), + issuer_bytes, + subject_bytes, + ) + ) + return hmac.new( + decode_api_rate_limit_key_secret(secret), preimage, hashlib.sha256 + ).digest() + + +class RateControlService: + """Consume durable counters in a transaction independent of request work.""" + + def __init__(self, session_factory: SessionFactory | None = None) -> None: + self._session_factory = session_factory + + def _factory(self) -> SessionFactory: + if self._session_factory is not None: + return self._session_factory + try: + return get_session_factory() + except RuntimeError as exc: + if str(exc) != MISSING_DATABASE_ERROR: + raise + raise RateControlUnavailableError("rate control unavailable") from exc + + async def consume( + self, + *, + control_scope: str, + issuer: str, + subject: str, + limit: int, + window_seconds: int, + secret: SecretStr | None, + ) -> RateControlDecision: + """Commit one counter use before returning its allow/deny decision.""" + if secret is None: + raise RateControlUnavailableError("rate control unavailable") + digest = rate_key_digest(secret, control_scope, issuer, subject) + session: AsyncSession | None = None + try: + async with self._factory()() as session: + repository = ApiRateControlRepository(session) + consumed = await repository.consume( + control_scope, digest, window_seconds + ) + await repository.prune_expired( + control_scope, digest, batch_size=PRUNE_BATCH_SIZE + ) + await session.commit() + except SQLAlchemyError as exc: + if session is not None: + with suppress(SQLAlchemyError): + await session.rollback() + raise RateControlUnavailableError("rate control unavailable") from exc + + retry_after = max( + 1, + min( + window_seconds, + math.ceil( + (consumed.window_expires_at - consumed.db_now).total_seconds() + ), + ), + ) + return RateControlDecision( + allowed=consumed.request_count <= limit, + request_count=consumed.request_count, + retry_after=retry_after, + ) diff --git a/backend/scripts/api_contract_e2e.py b/backend/scripts/api_contract_e2e.py index 87919274..f8f9f38b 100644 --- a/backend/scripts/api_contract_e2e.py +++ b/backend/scripts/api_contract_e2e.py @@ -20,8 +20,15 @@ import httpx from alembic import command from alembic.config import Config +from pydantic import SecretStr +from sqlalchemy import text from app.db import session as db_session +from app.modules.api_controls.service import ( + FIRST_ACCESS_SCOPE, + RateControlService, + rate_key_digest, +) from app.modules.projects.models import PostSubmitCheckerPolicy, ProjectSetupRun from app.modules.projects.post_submit_policy import ( build_project_post_submit_checker_spec, @@ -210,10 +217,62 @@ def api_environment() -> dict[str, str]: env["WORKSTREAM_CELERY_TASK_ALWAYS_EAGER"] = "true" env["WORKSTREAM_CELERY_BROKER_URL"] = "memory://" env["WORKSTREAM_CELERY_RESULT_BACKEND_URL"] = "cache+memory://" + env.setdefault( + "WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", + base64.b64encode(os.urandom(32)).decode("ascii"), + ) env["PYTHONPATH"] = str(project_root()) return env +async def exercise_rate_control_contract(env: dict[str, str]) -> None: + """Prove one durable denial without exposing the pseudonymous key.""" + subject = f"api-contract-rate-{uuid4()}" + secret = SecretStr(env["WORKSTREAM_API_RATE_LIMIT_KEY_SECRET"]) + service = RateControlService() + first = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=DEFAULT_FLOW_ISSUER, + subject=subject, + limit=1, + window_seconds=60, + secret=secret, + ) + denied = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=DEFAULT_FLOW_ISSUER, + subject=subject, + limit=1, + window_seconds=60, + secret=secret, + ) + assert first.allowed is True and first.request_count == 1 + assert denied.allowed is False and denied.request_count == 2 + assert 1 <= denied.retry_after <= 60 + + digest = rate_key_digest(secret, FIRST_ACCESS_SCOPE, DEFAULT_FLOW_ISSUER, subject) + async with db_session.get_session_factory()() as session: + row = ( + await session.execute( + text( + "select control_scope, key_digest, request_count " + "from api_rate_control_counters " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": FIRST_ACCESS_SCOPE, "digest": digest}, + ) + ).one() + assert tuple(row) == (FIRST_ACCESS_SCOPE, digest, 2) + await session.execute( + text( + "delete from api_rate_control_counters " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": FIRST_ACCESS_SCOPE, "digest": digest}, + ) + await session.commit() + + def start_api_server(port: int, env: dict[str, str]) -> tuple[subprocess.Popen, Path]: """Start a real uvicorn server process. @@ -1320,6 +1379,7 @@ async def main(env: dict[str, str]) -> None: env: Environment variables for the API server. """ await db_session.dispose_engine() + await exercise_rate_control_contract(env) port = find_free_port() base_url = f"http://127.0.0.1:{port}" diff --git a/backend/tests/test_alembic.py b/backend/tests/test_alembic.py index 8f0a8d04..646b35f7 100644 --- a/backend/tests/test_alembic.py +++ b/backend/tests/test_alembic.py @@ -1,8 +1,11 @@ from __future__ import annotations import asyncio +from concurrent.futures import ThreadPoolExecutor import json from pathlib import Path +import threading +import time from uuid import uuid4 import pytest @@ -292,6 +295,121 @@ def test_artifact_foundation_enforces_immutable_facts_and_guarded_downgrade( command.downgrade(config, "base") +def test_api_rate_control_schema_preserves_domain_and_guards_downgrade( + isolated_database_env: str, + migration_lock, +) -> None: + """Prove 0017 schema guards, preservation, and transactional downgrade refusal.""" + project_root = Path(__file__).resolve().parents[1] + config = Config(str(project_root / "alembic.ini")) + config.set_main_option("script_location", str(project_root / "alembic")) + project_id = str(uuid4()) + artifact_id = str(uuid4()) + digest = bytes(range(32)) + + with migration_lock(): + try: + command.downgrade(config, "base") + command.upgrade(config, "0015_post_submit_correction") + asyncio.run(_seed_artifact_prior_head(isolated_database_env, project_id)) + command.upgrade(config, "0016_artifact_domain") + before = asyncio.run( + _artifact_prior_head_project(isolated_database_env, project_id) + ) + artifact_before = asyncio.run( + _seed_and_fetch_0016_artifact(isolated_database_env, artifact_id) + ) + + command.upgrade(config, "0017_api_controls") + after = asyncio.run( + _artifact_prior_head_project(isolated_database_env, project_id) + ) + artifact_after = asyncio.run( + _fetch_0016_artifact(isolated_database_env, artifact_id) + ) + schema = asyncio.run(_api_rate_control_schema(isolated_database_env)) + asyncio.run(_assert_api_rate_control_guards(isolated_database_env, digest)) + + with pytest.raises(RuntimeError, match="non-empty API rate controls"): + command.downgrade(config, "0016_artifact_domain") + + asyncio.run(_clear_api_rate_controls(isolated_database_env)) + inserted = threading.Event() + release_insert = threading.Event() + downgrade_started = threading.Event() + + def guarded_downgrade() -> None: + downgrade_started.set() + command.downgrade(config, "0016_artifact_domain") + + with ThreadPoolExecutor(max_workers=2) as pool: + insert_future = pool.submit( + asyncio.run, + _insert_rate_control_until_released( + isolated_database_env, + digest, + inserted, + release_insert, + ), + ) + assert inserted.wait(timeout=5) + downgrade_future = pool.submit(guarded_downgrade) + assert downgrade_started.wait(timeout=5) + time.sleep(0.2) + assert downgrade_future.done() is False + release_insert.set() + insert_future.result(timeout=5) + with pytest.raises(RuntimeError, match="non-empty API rate controls"): + downgrade_future.result(timeout=5) + + refused_state = asyncio.run(_api_rate_control_state(isolated_database_env)) + asyncio.run(_clear_api_rate_controls(isolated_database_env)) + command.downgrade(config, "0016_artifact_domain") + downgraded_state = asyncio.run( + _api_rate_control_state(isolated_database_env) + ) + command.upgrade(config, "0017_api_controls") + finally: + asyncio.run(_clear_api_rate_controls(isolated_database_env)) + asyncio.run(_truncate_artifact_foundation(isolated_database_env)) + command.downgrade(config, "base") + command.upgrade(config, "head") + + assert after == before + assert artifact_after == artifact_before + assert schema == { + "columns": { + "control_scope:character varying:NO", + "key_digest:bytea:NO", + "window_started_at:timestamp with time zone:NO", + "window_expires_at:timestamp with time zone:NO", + "request_count:bigint:NO", + "updated_at:timestamp with time zone:NO", + }, + "constraints": { + "pk_api_rate_control_counters", + "ck_api_rate_control_counters_scope_token", + "ck_api_rate_control_counters_digest_length", + "ck_api_rate_control_counters_request_count", + "ck_api_rate_control_counters_window_order", + }, + "indexes": { + "pk_api_rate_control_counters", + "ix_api_rate_control_counters_window_expires_at", + }, + } + assert refused_state == { + "revision": "0017_api_controls", + "table_exists": True, + "row_count": 1, + } + assert downgraded_state == { + "revision": "0016_artifact_domain", + "table_exists": False, + "row_count": None, + } + + async def _fetch_columns(database_url: str) -> set[str]: """Return current public table columns as table.column names.""" engine = create_async_engine(database_url) @@ -1704,3 +1822,187 @@ async def _truncate_artifact_foundation(database_url: str) -> None: ) finally: await engine.dispose() + + +async def _api_rate_control_schema(database_url: str) -> dict[str, set[str]]: + """Return the exact public schema contract for the rate table.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + columns = ( + await connection.execute( + text( + "select column_name, data_type, is_nullable " + "from information_schema.columns " + "where table_schema = 'public' " + "and table_name = 'api_rate_control_counters'" + ) + ) + ).all() + constraints = ( + await connection.execute( + text( + "select conname from pg_constraint " + "where conrelid = 'api_rate_control_counters'::regclass" + ) + ) + ).scalars() + indexes = ( + await connection.execute( + text( + "select indexname from pg_indexes " + "where schemaname = 'public' " + "and tablename = 'api_rate_control_counters'" + ) + ) + ).scalars() + return { + "columns": { + f"{row.column_name}:{row.data_type}:{row.is_nullable}" + for row in columns + }, + "constraints": set(constraints), + "indexes": set(indexes), + } + finally: + await engine.dispose() + + +async def _seed_and_fetch_0016_artifact( + database_url: str, artifact_id: str +) -> dict[str, object]: + """Seed one representative 0016 row and return its exact persisted value.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text( + "insert into artifact_contents " + "(id, sha256, byte_count, media_type, normalized_display_name) " + "values (:id, :sha256, 17, 'text/plain', 'rate-migration.txt')" + ), + {"id": artifact_id, "sha256": "sha256:" + "7" * 64}, + ) + finally: + await engine.dispose() + return await _fetch_0016_artifact(database_url, artifact_id) + + +async def _fetch_0016_artifact( + database_url: str, artifact_id: str +) -> dict[str, object]: + """Fetch the representative 0016 artifact-domain row.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + row = ( + await connection.execute( + text( + "select id, sha256, byte_count, media_type, " + "normalized_display_name from artifact_contents where id = :id" + ), + {"id": artifact_id}, + ) + ).mappings().one() + return dict(row) + finally: + await engine.dispose() + + +async def _insert_rate_control_until_released( + database_url: str, + digest: bytes, + inserted: threading.Event, + release: threading.Event, +) -> None: + """Hold an uncommitted writer until the downgrade is waiting on its lock.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text( + "insert into api_rate_control_counters " + "(control_scope, key_digest, window_started_at, window_expires_at, " + "request_count, updated_at) values " + "('first_access', :digest, statement_timestamp(), " + "statement_timestamp() + interval '1 minute', 1, statement_timestamp())" + ), + {"digest": digest}, + ) + inserted.set() + assert await asyncio.to_thread(release.wait, 5) + finally: + await engine.dispose() + + +async def _assert_api_rate_control_guards(database_url: str, digest: bytes) -> None: + """Insert one valid counter and reject every malformed direct variant.""" + engine = create_async_engine(database_url) + insert_sql = text( + "insert into api_rate_control_counters " + "(control_scope, key_digest, window_started_at, window_expires_at, " + "request_count, updated_at) values " + "(:scope, :digest, statement_timestamp(), " + "statement_timestamp() + make_interval(secs => :seconds), " + ":count, statement_timestamp())" + ) + valid = { + "scope": "first_access", + "digest": digest, + "seconds": 60, + "count": 1, + } + try: + async with engine.begin() as connection: + await connection.execute(insert_sql, valid) + + invalid = [ + {**valid, "scope": "caller_supplied", "digest": bytes([1]) * 32}, + {**valid, "digest": bytes(31)}, + {**valid, "digest": bytes([2]) * 32, "count": 0}, + {**valid, "digest": bytes([3]) * 32, "seconds": 0}, + valid, + ] + for values in invalid: + with pytest.raises(IntegrityError): + async with engine.begin() as connection: + await connection.execute(insert_sql, values) + finally: + await engine.dispose() + + +async def _api_rate_control_state(database_url: str) -> dict[str, object]: + """Return revision, table existence, and row count after migration actions.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + revision = await connection.scalar(text("select version_num from alembic_version")) + exists = await connection.scalar( + text("select to_regclass('public.api_rate_control_counters') is not null") + ) + count = None + if exists: + count = await connection.scalar( + text("select count(*) from api_rate_control_counters") + ) + return { + "revision": revision, + "table_exists": exists, + "row_count": count, + } + finally: + await engine.dispose() + + +async def _clear_api_rate_controls(database_url: str) -> None: + """Clear rate rows only when the migration table exists.""" + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + exists = await connection.scalar( + text("select to_regclass('public.api_rate_control_counters') is not null") + ) + if exists: + await connection.execute(text("delete from api_rate_control_counters")) + finally: + await engine.dispose() diff --git a/backend/tests/test_api_rate_controls.py b/backend/tests/test_api_rate_controls.py new file mode 100644 index 00000000..d2773faa --- /dev/null +++ b/backend/tests/test_api_rate_controls.py @@ -0,0 +1,780 @@ +from __future__ import annotations + +import asyncio +from datetime import UTC, datetime, timedelta + +from fastapi import Depends +from httpx import ASGITransport, AsyncClient +from pydantic import SecretStr +import pytest +from sqlalchemy import text +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from app.api.deps.api_controls import ( + enforce_admin_mutation_rate_limit, + enforce_first_access_rate_limit, + get_rate_control_service, +) +from app.api.deps.auth import get_auth_verification_result +from app.core.config import Settings +from app.main import create_app +from app.modules.api_controls import service as rate_service_module +from app.modules.api_controls.models import ApiRateControlCounter +from app.modules.api_controls.repository import ApiRateControlRepository, ConsumedCounter +from app.modules.api_controls.service import ( + ADMIN_MUTATION_SCOPE, + FIRST_ACCESS_SCOPE, + RateControlDecision, + RateControlService, + RateControlUnavailableError, + rate_key_digest, +) +from app.schemas.auth import AuthVerificationResult, VerifiedIssuerToken + +RATE_SECRET_TEXT = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" +RATE_SECRET = SecretStr(RATE_SECRET_TEXT) +RATE_ISSUER = "https://issuer.example.test" +RATE_SUBJECT = "opaque-subject" + + +def test_rate_control_orm_model_matches_the_migration_contract() -> None: + table = ApiRateControlCounter.__table__ + + assert list(table.columns) == [ + table.c.control_scope, + table.c.key_digest, + table.c.window_started_at, + table.c.window_expires_at, + table.c.request_count, + table.c.updated_at, + ] + assert {column.name for column in table.primary_key.columns} == { + "control_scope", + "key_digest", + } + assert {constraint.name for constraint in table.constraints} == { + "pk_api_rate_control_counters", + "ck_api_rate_control_counters_scope_token", + "ck_api_rate_control_counters_digest_length", + "ck_api_rate_control_counters_request_count", + "ck_api_rate_control_counters_window_order", + } + assert {index.name for index in table.indexes} == { + "ix_api_rate_control_counters_window_expires_at" + } + + +@pytest.fixture +async def rate_control_factory(postgres_database_url: str): + """Provide independent sessions over a clean, migrated rate-control table.""" + engine = create_async_engine(postgres_database_url) + factory = async_sessionmaker(engine, expire_on_commit=False) + async with engine.begin() as connection: + await connection.execute(text("delete from api_rate_control_counters")) + try: + yield factory + finally: + async with engine.begin() as connection: + await connection.execute(text("delete from api_rate_control_counters")) + await engine.dispose() + + +def test_rate_key_digest_matches_literal_vector_and_separates_boundaries() -> None: + expected = bytes.fromhex( + "35b4fb3e6a647a52596a5240b0b3ad5c2976d91771f95e2497be247081e53b31" + ) + + assert rate_key_digest( + RATE_SECRET, FIRST_ACCESS_SCOPE, RATE_ISSUER, RATE_SUBJECT + ) == expected + assert rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "ab", "c") != ( + rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "a", "bc") + ) + assert rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "Issuer", "subject") != ( + rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "issuer", "subject") + ) + assert rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "issuer", "e\u0301") != ( + rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "issuer", "\u00e9") + ) + assert rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, "issuer", "subject") != ( + rate_key_digest(RATE_SECRET, ADMIN_MUTATION_SCOPE, "issuer", "subject") + ) + + +@pytest.mark.parametrize( + ("issuer", "subject"), + [ + ("", "subject"), + ("x" * 4_097, "subject"), + ("\u00e9" * 2_049, "subject"), + ("\ud800", "subject"), + ("issuer", "\ud800"), + ], +) +def test_rate_key_digest_rejects_unbounded_identity_without_echo( + issuer: str, subject: str +) -> None: + with pytest.raises(RateControlUnavailableError) as caught: + rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, issuer, subject) + assert str(caught.value) == "rate control unavailable" + + +async def _stored_rate_row(factory, scope: str, digest: bytes): + async with factory() as session: + return ( + await session.execute( + text( + "select window_started_at, window_expires_at, request_count " + "from api_rate_control_counters " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": scope, "digest": digest}, + ) + ).one() + + +async def _insert_counter_row( + session, + scope: str, + digest: bytes, + *, + started_offset: int, + expires_offset: int, + request_count: int, + updated_offset: int = 0, +) -> None: + await session.execute( + text( + "insert into api_rate_control_counters " + "(control_scope, key_digest, window_started_at, window_expires_at, " + "request_count, updated_at) values " + "(:scope, :digest, " + "statement_timestamp() + make_interval(secs => :started_offset), " + "statement_timestamp() + make_interval(secs => :expires_offset), " + ":request_count, " + "statement_timestamp() + make_interval(secs => :updated_offset))" + ), + { + "scope": scope, + "digest": digest, + "started_offset": started_offset, + "expires_offset": expires_offset, + "request_count": request_count, + "updated_offset": updated_offset, + }, + ) + + +async def test_rate_control_commits_fixed_window_denials_and_resets_expiry( + rate_control_factory, +) -> None: + service = RateControlService(rate_control_factory) + decisions = [ + await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=2, + window_seconds=60, + secret=RATE_SECRET, + ) + for _ in range(4) + ] + digest = rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, RATE_ISSUER, RATE_SUBJECT) + original = await _stored_rate_row(rate_control_factory, FIRST_ACCESS_SCOPE, digest) + + assert [decision.allowed for decision in decisions] == [True, True, False, False] + assert [decision.request_count for decision in decisions] == [1, 2, 3, 4] + assert all(1 <= decision.retry_after <= 60 for decision in decisions) + assert original.request_count == 4 + + async with rate_control_factory() as session: + await session.execute( + text( + "update api_rate_control_counters set " + "window_started_at = statement_timestamp() - interval '2 seconds', " + "window_expires_at = statement_timestamp() - interval '1 second' " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": FIRST_ACCESS_SCOPE, "digest": digest}, + ) + await session.commit() + + reset = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=2, + window_seconds=60, + secret=RATE_SECRET, + ) + persisted = await _stored_rate_row(rate_control_factory, FIRST_ACCESS_SCOPE, digest) + assert reset == RateControlDecision(allowed=True, request_count=1, retry_after=60) + assert persisted.request_count == 1 + assert persisted.window_started_at > original.window_started_at + + other_scope = await service.consume( + control_scope=ADMIN_MUTATION_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=2, + window_seconds=60, + secret=RATE_SECRET, + ) + other_subject = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject="distinct-subject", + limit=2, + window_seconds=60, + secret=RATE_SECRET, + ) + assert other_scope.request_count == other_subject.request_count == 1 + assert ( + await _stored_rate_row(rate_control_factory, FIRST_ACCESS_SCOPE, digest) + ).request_count == 1 + + +async def test_repository_persists_the_returned_database_timestamp( + rate_control_factory, +) -> None: + digest = bytes([17]) * 32 + async with rate_control_factory() as session: + consumed = await ApiRateControlRepository(session).consume( + FIRST_ACCESS_SCOPE, digest, 60 + ) + updated_at = await session.scalar( + text( + "select updated_at from api_rate_control_counters " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": FIRST_ACCESS_SCOPE, "digest": digest}, + ) + await session.rollback() + assert updated_at == consumed.db_now + + +async def test_rate_control_concurrency_has_no_lost_or_rolled_back_consumption( + rate_control_factory, + monkeypatch: pytest.MonkeyPatch, +) -> None: + original_consume = ApiRateControlRepository.consume + all_started = asyncio.Event() + started = 0 + + async def synchronized_consume(repository, *args, **kwargs): + nonlocal started + started += 1 + if started == 20: + all_started.set() + await asyncio.wait_for(all_started.wait(), timeout=5) + return await original_consume(repository, *args, **kwargs) + + monkeypatch.setattr(ApiRateControlRepository, "consume", synchronized_consume) + service = RateControlService(rate_control_factory) + + async def consume_once(): + return await service.consume( + control_scope=ADMIN_MUTATION_SCOPE, + issuer=RATE_ISSUER, + subject="concurrent-subject", + limit=7, + window_seconds=30, + secret=RATE_SECRET, + ) + + decisions = await asyncio.gather(*(consume_once() for _ in range(20))) + digest = rate_key_digest( + RATE_SECRET, ADMIN_MUTATION_SCOPE, RATE_ISSUER, "concurrent-subject" + ) + persisted = await _stored_rate_row(rate_control_factory, ADMIN_MUTATION_SCOPE, digest) + + assert sum(decision.allowed for decision in decisions) == 7 + assert sorted(decision.request_count for decision in decisions) == list(range(1, 21)) + assert persisted.request_count == 20 + + probe_digest = bytes([255]) * 32 + independent_subject = "rollback-independent" + independent_digest = rate_key_digest( + RATE_SECRET, FIRST_ACCESS_SCOPE, RATE_ISSUER, independent_subject + ) + async with rate_control_factory() as outer: + await _insert_counter_row( + outer, + FIRST_ACCESS_SCOPE, + probe_digest, + started_offset=0, + expires_offset=60, + request_count=1, + ) + independent = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=independent_subject, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + await outer.rollback() + async with rate_control_factory() as session: + probe_count = await session.scalar( + text( + "select count(*) from api_rate_control_counters " + "where control_scope = :scope and key_digest = :digest" + ), + {"scope": FIRST_ACCESS_SCOPE, "digest": probe_digest}, + ) + assert independent.allowed is True + assert probe_count == 0 + assert ( + await _stored_rate_row(rate_control_factory, FIRST_ACCESS_SCOPE, independent_digest) + ).request_count == 1 + + +async def test_rate_control_saturates_bigint_and_prunes_only_bounded_other_rows( + rate_control_factory, +) -> None: + service = RateControlService(rate_control_factory) + digest = rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, RATE_ISSUER, "saturated") + async with rate_control_factory() as session: + await _insert_counter_row( + session, + FIRST_ACCESS_SCOPE, + digest, + started_offset=0, + expires_offset=60, + request_count=9_223_372_036_854_775_807, + ) + for value in range(125): + await _insert_counter_row( + session, + ADMIN_MUTATION_SCOPE, + value.to_bytes(32, "big"), + started_offset=-120, + expires_offset=-60, + request_count=1, + updated_offset=-60, + ) + await session.commit() + + decision = await service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject="saturated", + limit=10_000, + window_seconds=60, + secret=RATE_SECRET, + ) + async with rate_control_factory() as session: + expired = await session.scalar( + text( + "select count(*) from api_rate_control_counters " + "where window_expires_at <= statement_timestamp()" + ) + ) + assert decision.request_count == 9_223_372_036_854_775_807 + assert decision.allowed is False + assert expired == 25 + + +async def test_concurrent_expired_keys_do_not_deadlock_during_pruning( + rate_control_factory, + monkeypatch: pytest.MonkeyPatch, +) -> None: + subjects = ("expired-a", "expired-b") + async with rate_control_factory() as session: + for subject in subjects: + await _insert_counter_row( + session, + FIRST_ACCESS_SCOPE, + rate_key_digest(RATE_SECRET, FIRST_ACCESS_SCOPE, RATE_ISSUER, subject), + started_offset=-2, + expires_offset=-1, + request_count=5, + updated_offset=-1, + ) + await session.commit() + + original_prune = ApiRateControlRepository.prune_expired + both_upserted = asyncio.Event() + upserted = 0 + + async def synchronized_prune(repository, *args, **kwargs): + nonlocal upserted + upserted += 1 + if upserted == 2: + both_upserted.set() + await asyncio.wait_for(both_upserted.wait(), timeout=5) + return await original_prune(repository, *args, **kwargs) + + monkeypatch.setattr(ApiRateControlRepository, "prune_expired", synchronized_prune) + service = RateControlService(rate_control_factory) + decisions = await asyncio.wait_for( + asyncio.gather( + *( + service.consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=subject, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + for subject in subjects + ) + ), + timeout=5, + ) + assert decisions == [ + RateControlDecision(allowed=True, request_count=1, retry_after=60), + RateControlDecision(allowed=True, request_count=1, retry_after=60), + ] + + +class _FakeSession: + """Minimal async-session context for deterministic failure tests.""" + + def __init__( + self, *, commit_error: bool = False, enter_error: bool = False + ) -> None: + self.commit_error = commit_error + self.enter_error = enter_error + self.rolled_back = False + + async def __aenter__(self): + if self.enter_error: + raise SQLAlchemyError("private session-open detail") + return self + + async def __aexit__(self, *_args) -> None: + return None + + async def commit(self) -> None: + if self.commit_error: + raise SQLAlchemyError("private commit detail") + + async def rollback(self) -> None: + self.rolled_back = True + + +async def test_rate_control_maps_only_database_failures_and_rolls_back( + monkeypatch: pytest.MonkeyPatch, +) -> None: + now = datetime.now(UTC) + with pytest.raises(RateControlUnavailableError, match="^rate control unavailable$"): + await RateControlService(lambda: _FakeSession(enter_error=True)).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + + session = _FakeSession() + + async def fail_execute(*_args, **_kwargs): + raise SQLAlchemyError("private execute detail") + + monkeypatch.setattr(ApiRateControlRepository, "consume", fail_execute) + with pytest.raises(RateControlUnavailableError, match="^rate control unavailable$"): + await RateControlService(lambda: session).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + assert session.rolled_back is True + + async def successful_consume(*_args, **_kwargs): + return ConsumedCounter(now, now, now + timedelta(seconds=60), 1) + + async def successful_prune(*_args, **_kwargs): + return None + + async def fail_prune(*_args, **_kwargs): + raise SQLAlchemyError("private prune detail") + + prune_session = _FakeSession() + monkeypatch.setattr(ApiRateControlRepository, "consume", successful_consume) + monkeypatch.setattr(ApiRateControlRepository, "prune_expired", fail_prune) + with pytest.raises(RateControlUnavailableError, match="^rate control unavailable$"): + await RateControlService(lambda: prune_session).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + assert prune_session.rolled_back is True + + commit_session = _FakeSession(commit_error=True) + monkeypatch.setattr(ApiRateControlRepository, "consume", successful_consume) + monkeypatch.setattr(ApiRateControlRepository, "prune_expired", successful_prune) + with pytest.raises(RateControlUnavailableError, match="^rate control unavailable$"): + await RateControlService(lambda: commit_session).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + assert commit_session.rolled_back is True + + +async def test_retry_after_uses_only_returned_database_clock( + monkeypatch: pytest.MonkeyPatch, +) -> None: + database_now = datetime(2000, 1, 1, tzinfo=UTC) + + async def subsecond(*_args, **_kwargs): + return ConsumedCounter( + database_now, + database_now, + database_now + timedelta(milliseconds=200), + 2, + ) + + async def no_prune(*_args, **_kwargs): + return None + + monkeypatch.setattr(ApiRateControlRepository, "consume", subsecond) + monkeypatch.setattr(ApiRateControlRepository, "prune_expired", no_prune) + decision = await RateControlService(lambda: _FakeSession()).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + assert decision.retry_after == 1 + + async def beyond_window(*_args, **_kwargs): + return ConsumedCounter( + database_now, + database_now, + database_now + timedelta(seconds=120), + 2, + ) + + monkeypatch.setattr(ApiRateControlRepository, "consume", beyond_window) + decision = await RateControlService(lambda: _FakeSession()).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + assert decision.retry_after == 60 + + +async def test_rate_control_narrowly_maps_missing_database_and_propagates_cancel( + monkeypatch: pytest.MonkeyPatch, +) -> None: + def missing_database(): + raise RuntimeError("WORKSTREAM_DATABASE_URL must be set before database access") + + monkeypatch.setattr(rate_service_module, "get_session_factory", missing_database) + with pytest.raises(RateControlUnavailableError, match="^rate control unavailable$"): + await RateControlService().consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + + def unrelated_failure(): + raise RuntimeError("unrelated runtime failure") + + monkeypatch.setattr(rate_service_module, "get_session_factory", unrelated_failure) + with pytest.raises(RuntimeError, match="unrelated runtime failure"): + await RateControlService().consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + + async def cancel(*_args, **_kwargs): + raise asyncio.CancelledError + + monkeypatch.setattr(ApiRateControlRepository, "consume", cancel) + with pytest.raises(asyncio.CancelledError): + await RateControlService(lambda: _FakeSession()).consume( + control_scope=FIRST_ACCESS_SCOPE, + issuer=RATE_ISSUER, + subject=RATE_SUBJECT, + limit=1, + window_seconds=60, + secret=RATE_SECRET, + ) + + +def _verified_rate_identity(subject: str = RATE_SUBJECT) -> AuthVerificationResult: + return AuthVerificationResult( + token=VerifiedIssuerToken( + issuer=RATE_ISSUER, + subject=subject, + audience=("workstream",), + expires_at=2_000_000_000, + issued_at=1_999_999_000, + token_id="rate-token", + subject_kind="human", + scopes=frozenset({"workstream:access"}), + ) + ) + + +class _DecisionService: + def __init__(self, decisions: list[RateControlDecision] | None = None) -> None: + self.decisions = decisions or [] + self.calls: list[dict] = [] + + async def consume(self, **kwargs) -> RateControlDecision: + self.calls.append(kwargs) + if not self.decisions: + raise RateControlUnavailableError("private database detail") + return self.decisions.pop(0) + + +async def test_unattached_dependencies_emit_canonical_429_and_use_token_identity() -> None: + service = _DecisionService( + [ + RateControlDecision(True, 1, 60), + RateControlDecision(False, 31, 17), + ] + ) + app = create_app( + Settings(environment="test", api_rate_limit_key_secret=RATE_SECRET_TEXT) + ) + app.dependency_overrides[get_auth_verification_result] = _verified_rate_identity + app.dependency_overrides[get_rate_control_service] = lambda: service + + @app.post( + "/_test/first-access-rate", + dependencies=[Depends(enforce_first_access_rate_limit)], + ) + async def first_access() -> dict[str, bool]: + return {"allowed": True} + + @app.post( + "/_test/admin-mutation-rate", + dependencies=[Depends(enforce_admin_mutation_rate_limit)], + ) + async def admin_mutation() -> dict[str, bool]: + return {"allowed": True} + + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://testserver" + ) as client: + allowed = await client.post("/_test/first-access-rate") + denied = await client.post("/_test/admin-mutation-rate") + + assert allowed.status_code == 200 + assert denied.status_code == 429 + assert denied.headers["retry-after"] == "17" + assert denied.json() == { + "detail": "Rate limit exceeded", + "error": { + "code": "rate_limit_exceeded", + "message": "Rate limit exceeded", + "details": {}, + "correlation_id": denied.headers["x-correlation-id"], + "retryable": True, + }, + } + assert [call["control_scope"] for call in service.calls] == [ + FIRST_ACCESS_SCOPE, + ADMIN_MUTATION_SCOPE, + ] + assert all(call["issuer"] == RATE_ISSUER for call in service.calls) + assert all(call["subject"] == RATE_SUBJECT for call in service.calls) + + +@pytest.mark.parametrize( + ("settings", "subject", "service"), + [ + (Settings(environment="test"), RATE_SUBJECT, RateControlService()), + ( + Settings(environment="test", api_rate_limit_key_secret=RATE_SECRET_TEXT), + "x" * 4_097, + RateControlService(), + ), + ( + Settings(environment="test", api_rate_limit_key_secret=RATE_SECRET_TEXT), + "\ud800", + RateControlService(), + ), + ( + Settings(environment="test", api_rate_limit_key_secret=RATE_SECRET_TEXT), + RATE_SUBJECT, + _DecisionService(), + ), + ], +) +async def test_rate_dependency_unavailability_is_private_503( + settings: Settings, + subject: str, + service, +) -> None: + app = create_app(settings) + app.dependency_overrides[get_auth_verification_result] = lambda: _verified_rate_identity( + subject + ) + app.dependency_overrides[get_rate_control_service] = lambda: service + + @app.post( + "/_test/rate-unavailable", + dependencies=[Depends(enforce_first_access_rate_limit)], + ) + async def unavailable() -> None: + return None + + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://testserver" + ) as client: + response = await client.post("/_test/rate-unavailable") + + assert response.status_code == 503 + assert response.json() == { + "detail": "Service unavailable", + "error": { + "code": "service_unavailable", + "message": "Service unavailable", + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": True, + }, + } + assert subject not in response.text + + +def test_rate_dependencies_are_not_attached_to_production_routes() -> None: + forbidden = { + enforce_first_access_rate_limit, + enforce_admin_mutation_rate_limit, + } + + def dependency_calls(dependant) -> set: + calls = {dependant.call} + for dependency in dependant.dependencies: + calls.update(dependency_calls(dependency)) + return calls + + app = create_app() + assert all( + forbidden.isdisjoint(dependency_calls(route.dependant)) + for route in app.routes + if hasattr(route, "dependant") + ) diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index fbd28867..b5a38e3e 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -1203,10 +1203,11 @@ def test_legacy_compatibility_dependency_has_fixed_consumer_allowlist() -> None: } assert { path for path, source in sources.items() if "get_auth_verification_result" in source - } == {"api/deps/auth.py"} + } == {"api/deps/api_controls.py", "api/deps/auth.py"} assert {path for path, source in sources.items() if "AuthVerificationResult" in source} == { "adapters/auth/dev.py", "adapters/auth/flow.py", + "api/deps/api_controls.py", "api/deps/auth.py", "core/auth.py", "interfaces/auth.py", diff --git a/backend/tests/test_config.py b/backend/tests/test_config.py index b2677986..7c96972e 100644 --- a/backend/tests/test_config.py +++ b/backend/tests/test_config.py @@ -1,7 +1,13 @@ from __future__ import annotations +import base64 +import json +from collections.abc import Mapping +from pathlib import Path + import pytest -from pydantic import ValidationError +from pydantic import SecretStr, ValidationError +from pydantic_settings import BaseSettings from app.adapters.artifacts import resolve_artifact_store from app.adapters.auth.flow import FlowAuthVerifier @@ -12,9 +18,37 @@ from app.main import create_app +def _assert_secret_not_retained(value: object, secret: str, seen: set[int] | None = None) -> None: + """Assert a secret is unreachable through an error's public object graph.""" + if seen is None: + seen = set() + if id(value) in seen: + return + seen.add(id(value)) + if isinstance(value, str): + assert secret not in value + elif isinstance(value, SecretStr): + assert value.get_secret_value() != secret + elif isinstance(value, BaseException): + if isinstance(value, ValidationError): + _assert_secret_not_retained(value.errors(), secret, seen) + _assert_secret_not_retained(value.args, secret, seen) + _assert_secret_not_retained(vars(value), secret, seen) + _assert_secret_not_retained(value.__cause__, secret, seen) + _assert_secret_not_retained(value.__context__, secret, seen) + elif isinstance(value, Mapping): + for key, item in value.items(): + _assert_secret_not_retained(key, secret, seen) + _assert_secret_not_retained(item, secret, seen) + elif isinstance(value, (list, tuple, set)): + for item in value: + _assert_secret_not_retained(item, secret, seen) + + def test_default_settings_are_fail_closed(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("WORKSTREAM_DATABASE_URL", raising=False) monkeypatch.delenv("WORKSTREAM_DEV_AUTH_TOKEN", raising=False) + monkeypatch.delenv("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", raising=False) settings = Settings() @@ -23,6 +57,279 @@ def test_default_settings_are_fail_closed(monkeypatch: pytest.MonkeyPatch) -> No assert settings.auth_provider == "flow" assert settings.database_url is None assert settings.dev_auth_token is None + assert settings.api_rate_limit_key_secret is None + assert settings.api_first_access_rate_limit == 10 + assert settings.api_first_access_rate_window_seconds == 60 + assert settings.api_admin_mutation_rate_limit == 30 + assert settings.api_admin_mutation_rate_window_seconds == 60 + + +def test_rate_limit_secret_is_canonical_and_redacted() -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + settings = Settings(api_rate_limit_key_secret=encoded) + + assert settings.api_rate_limit_key_secret is not None + assert settings.api_rate_limit_key_secret.get_secret_value() == encoded + assert encoded not in repr(settings) + assert "api_rate_limit_key_secret" not in settings.model_dump() + + +def test_rate_limit_secret_loads_from_environment_and_dotenv( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + env_encoded = base64.b64encode(bytes(range(32))).decode("ascii") + dotenv_encoded = base64.b64encode(bytes(range(1, 33))).decode("ascii") + (tmp_path / ".env").write_text( + f"WORKSTREAM_API_RATE_LIMIT_KEY_SECRET={dotenv_encoded}\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", env_encoded) + + env_settings = Settings() + monkeypatch.delenv("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET") + dotenv_settings = Settings() + + assert env_settings.api_rate_limit_key_secret is not None + assert env_settings.api_rate_limit_key_secret.get_secret_value() == env_encoded + assert dotenv_settings.api_rate_limit_key_secret is not None + assert dotenv_settings.api_rate_limit_key_secret.get_secret_value() == dotenv_encoded + + +def test_rate_limit_secret_loads_from_layered_dotenv_files(tmp_path: Path) -> None: + first_encoded = base64.b64encode(bytes(range(32))).decode("ascii") + second_encoded = base64.b64encode(bytes(range(1, 33))).decode("ascii") + first = tmp_path / "first.env" + second = tmp_path / "second.env" + first.write_text( + f"WORKSTREAM_API_RATE_LIMIT_KEY_SECRET={first_encoded}\n", + encoding="utf-8", + ) + second.write_text( + f"WORKSTREAM_API_RATE_LIMIT_KEY_SECRET={second_encoded}\n", + encoding="utf-8", + ) + + settings = Settings(_env_file=(first, second)) + + assert settings.api_rate_limit_key_secret is not None + assert settings.api_rate_limit_key_secret.get_secret_value() == second_encoded + + +def test_rate_limit_secret_is_absent_from_unrelated_structured_errors() -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + + with pytest.raises(ValidationError) as caught: + Settings( + api_rate_limit_key_secret=encoded, + artifact_store_backend="flow_node", + ) + + assert encoded not in repr(caught.value.errors()) + assert encoded not in caught.value.json() + _assert_secret_not_retained(caught.value, encoded) + + +def test_environment_rate_limit_secret_is_absent_from_unrelated_structured_errors( + monkeypatch: pytest.MonkeyPatch, +) -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + monkeypatch.setenv("WORKSTREAM_API_RATE_LIMIT_KEY_SECRET", encoded) + + with pytest.raises(ValidationError) as caught: + Settings(artifact_store_backend="flow_node") + + assert encoded not in repr(caught.value.errors()) + assert encoded not in caught.value.json() + _assert_secret_not_retained(caught.value, encoded) + + +def test_dotenv_rate_limit_secret_is_absent_from_unrelated_structured_errors( + tmp_path: Path, +) -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + env_file = tmp_path / ".env" + env_file.write_text( + f"WORKSTREAM_API_RATE_LIMIT_KEY_SECRET={encoded}\n", + encoding="utf-8", + ) + + with pytest.raises(ValidationError) as caught: + Settings(_env_file=env_file, artifact_store_backend="flow_node") + + assert encoded not in repr(caught.value.errors()) + assert encoded not in caught.value.json() + _assert_secret_not_retained(caught.value, encoded) + + +def test_model_validate_rejects_rate_limit_secret_without_structured_echo() -> None: + invalid = "not-a-canonical-secret" + + with pytest.raises(ValueError, match="^invalid API rate limit key secret$") as caught: + Settings.model_validate({"api_rate_limit_key_secret": invalid}) + + assert not isinstance(caught.value, ValidationError) + assert invalid not in f"{caught.value!s} {caught.value!r}" + _assert_secret_not_retained(caught.value, invalid) + + +def test_model_validate_rate_limit_secret_is_absent_from_unrelated_errors() -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + + with pytest.raises(ValidationError) as caught: + Settings.model_validate( + { + "api_rate_limit_key_secret": encoded, + "artifact_store_backend": "flow_node", + } + ) + + assert encoded not in repr(caught.value.errors()) + assert encoded not in caught.value.json() + _assert_secret_not_retained(caught.value, encoded) + + +@pytest.mark.parametrize( + ("method_name", "base_method_name"), + [ + ("model_validate", "model_validate"), + ("model_validate_json", "model_validate"), + ("model_validate_strings", "model_validate_strings"), + ], +) +def test_alternate_validation_never_passes_secret_into_pydantic( + monkeypatch: pytest.MonkeyPatch, + method_name: str, + base_method_name: str, +) -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + observed: dict[str, object] = {} + + class PydanticBoundaryReached(Exception): + pass + + def capture_input( + cls: type[BaseSettings], + obj: object, + **kwargs: object, + ) -> BaseSettings: + observed["input"] = obj + raise PydanticBoundaryReached + + monkeypatch.setattr(BaseSettings, base_method_name, classmethod(capture_input)) + payload = {"api_rate_limit_key_secret": encoded} + method = getattr(Settings, method_name) + + with pytest.raises(PydanticBoundaryReached): + method(json.dumps(payload) if method_name.endswith("json") else payload) + + assert isinstance(observed["input"], Mapping) + assert observed["input"]["api_rate_limit_key_secret"] is None + _assert_secret_not_retained(observed, encoded) + + +@pytest.mark.parametrize("method_name", ["model_validate_json", "model_validate_strings"]) +def test_alternate_validation_loads_rate_limit_secret(method_name: str) -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + payload = {"api_rate_limit_key_secret": encoded} + method = getattr(Settings, method_name) + + settings = method(json.dumps(payload) if method_name.endswith("json") else payload) + + assert settings.api_rate_limit_key_secret is not None + assert settings.api_rate_limit_key_secret.get_secret_value() == encoded + + +@pytest.mark.parametrize("method_name", ["model_validate_json", "model_validate_strings"]) +def test_alternate_validation_rejects_rate_limit_secret_without_echo( + method_name: str, +) -> None: + invalid = "not-a-canonical-secret" + payload = {"api_rate_limit_key_secret": invalid} + method = getattr(Settings, method_name) + + with pytest.raises(ValueError, match="^invalid API rate limit key secret$") as caught: + method(json.dumps(payload) if method_name.endswith("json") else payload) + + assert not isinstance(caught.value, ValidationError) + assert invalid not in f"{caught.value!s} {caught.value!r}" + _assert_secret_not_retained(caught.value, invalid) + + +@pytest.mark.parametrize("method_name", ["model_validate_json", "model_validate_strings"]) +def test_alternate_validation_secret_is_absent_from_unrelated_errors( + method_name: str, +) -> None: + encoded = base64.b64encode(bytes(range(32))).decode("ascii") + payload = { + "api_rate_limit_key_secret": encoded, + "artifact_store_backend": "flow_node", + } + method = getattr(Settings, method_name) + + with pytest.raises(ValidationError) as caught: + method(json.dumps(payload) if method_name.endswith("json") else payload) + + assert encoded not in repr(caught.value.errors()) + assert encoded not in caught.value.json() + _assert_secret_not_retained(caught.value, encoded) + + +def test_model_validate_json_rejects_malformed_document_without_echo() -> None: + invalid = "not-a-canonical-secret" + payload = f'{{"api_rate_limit_key_secret":"{invalid}"' + + with pytest.raises(ValueError, match="^invalid settings JSON$") as caught: + Settings.model_validate_json(payload) + + assert not isinstance(caught.value, ValidationError) + assert invalid not in f"{caught.value!s} {caught.value!r}" + assert caught.value.__cause__ is None + assert caught.value.__context__ is None + _assert_secret_not_retained(caught.value, invalid) + + +@pytest.mark.parametrize( + "value", + [ + "", + " ", + "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=\n", + "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8", + base64.b64encode(bytes(31)).decode("ascii"), + base64.b64encode(bytes(65)).decode("ascii"), + "\u00e9" * 44, + ], +) +def test_rate_limit_secret_rejects_invalid_values_without_echo(value: str) -> None: + with pytest.raises(ValueError, match="^invalid API rate limit key secret$") as caught: + Settings(api_rate_limit_key_secret=value) + assert not isinstance(caught.value, ValidationError) + rendered = f"{caught.value!s} {caught.value!r}" + assert not hasattr(caught.value, "errors") + assert not hasattr(caught.value, "json") + if value.strip(): + assert value not in rendered + _assert_secret_not_retained(caught.value, value) + + +@pytest.mark.parametrize( + ("field_name", "value"), + [ + ("api_first_access_rate_limit", 0), + ("api_first_access_rate_limit", 10_001), + ("api_admin_mutation_rate_limit", 0), + ("api_admin_mutation_rate_limit", 10_001), + ("api_first_access_rate_window_seconds", 0), + ("api_first_access_rate_window_seconds", 3_601), + ("api_admin_mutation_rate_window_seconds", 0), + ("api_admin_mutation_rate_window_seconds", 3_601), + ], +) +def test_rate_limit_numeric_bounds_are_enforced(field_name: str, value: int) -> None: + with pytest.raises(ValidationError): + Settings(**{field_name: value}) def test_settings_reject_unknown_environment() -> None: diff --git a/docs/architecture_data_model.md b/docs/architecture_data_model.md index 6996e0e9..07c11827 100644 --- a/docs/architecture_data_model.md +++ b/docs/architecture_data_model.md @@ -111,6 +111,22 @@ Canonical idempotency records bind operation, actor, request hash, committed result, and replay status. Authority invalidation events are written atomically with grant/profile/link changes and drive bounded reconciliation. +### API Rate Control Counter + +`api_rate_control_counters` is a cross-replica fixed-window control for future +first-access and authority-management mutations. Its composite key is the +server-owned `control_scope` plus a 32-byte HMAC-SHA256 `key_digest` derived +from the exact verified issuer and opaque subject. It stores database-time +window start/expiry, a saturating `BIGINT` request count, and database-time +update evidence. It has no actor/profile foreign key or surrogate identifier +and stores no raw issuer, subject, email, role, token, claim, or network data. + +One PostgreSQL upsert atomically inserts, increments, saturates, or resets an +expired counter using a single statement timestamp. Consumption and bounded +expired-row pruning commit in a short independent session before a route may +continue. These counters are abuse controls, not identity, grants, product +authority, audit events, or a replacement for exact authorization checks. + ### Legacy Migration Existing external `ActorIdentity.actor_id` UUIDs may become canonical profile diff --git a/docs/operations_authorization_service.md b/docs/operations_authorization_service.md index fb0636ec..3b9915c4 100644 --- a/docs/operations_authorization_service.md +++ b/docs/operations_authorization_service.md @@ -60,6 +60,11 @@ preview fail closed when a required value is absent or outside its bound. | `WORKSTREAM_TOKEN_INTROSPECTION_WRITE_TIMEOUT_SECONDS` | Float `0.1..10` | `3` | | `WORKSTREAM_TOKEN_INTROSPECTION_POOL_TIMEOUT_SECONDS` | Float `0.1..10` | `1` | | `WORKSTREAM_TOKEN_INTROSPECTION_TOTAL_TIMEOUT_SECONDS` | Float `0.5..15` | `5` | +| `WORKSTREAM_API_RATE_LIMIT_KEY_SECRET` | Canonical padded RFC 4648 Base64 decoding to `32..64` bytes | Optional until a protected route is attached; then required | +| `WORKSTREAM_API_FIRST_ACCESS_RATE_LIMIT` | Integer `1..10000` | `10` | +| `WORKSTREAM_API_FIRST_ACCESS_RATE_WINDOW_SECONDS` | Integer `1..3600` | `60` | +| `WORKSTREAM_API_ADMIN_MUTATION_RATE_LIMIT` | Integer `1..10000` | `30` | +| `WORKSTREAM_API_ADMIN_MUTATION_RATE_WINDOW_SECONDS` | Integer `1..3600` | `60` | Secrets, private keys, bearer tokens, full claims, and raw JWKS documents must not appear in committed configuration or evidence. @@ -144,6 +149,50 @@ claims, subjects, emails, SQL, provider bodies, exception text, or secrets to responses or logs. AUTH-04A does not activate rate controls, grant APIs, or new product authority; those remain owned by later separately reviewed chunks. +## PostgreSQL Rate Controls + +AUTH-04B provides unattached dependencies for future first-access and +authority-management mutations. No production route is rate limited by this +chunk. When an owning route attaches one, every replica must use the same +secret and settings. Missing secret or database access fails the dependency +closed with retryable HTTP 503; an exhausted window returns retryable HTTP 429 +with an integer `Retry-After`. + +Generate the secret outside the repository and store it in the deployment +secret manager: + +```bash +python3 -c 'import base64,secrets; print(base64.b64encode(secrets.token_bytes(32)).decode())' +``` + +Counters store only the server-owned scope and an HMAC-SHA256 digest of the +verified issuer/subject. They do not store raw identity, actor, token, claim, +role, email, or network values. Consumption uses database time and commits in +an independent session before the protected mutation may continue or return +429, so downstream rollback does not restore allowance. + +Secret rotation intentionally creates a new digest space. Before rotating: + +1. Quiesce every protected write for at least the largest effective + pre-rotation window configured on any replica. +2. Rotate every replica while writes remain quiesced; mixed-secret replicas + must not serve protected writes. +3. Delete only rows expired by PostgreSQL time: + +```sql +DELETE FROM api_rate_control_counters +WHERE window_expires_at <= statement_timestamp(); +``` + +4. Confirm all replicas use the new secret, then resume protected writes. + +Runtime consumption opportunistically deletes at most 100 expired other rows. +On idle systems, operators may run the same expired-only SQL cleanup. Never +delete active rows to recover capacity, and never downgrade migration `0017` +while the table is nonempty; the guarded downgrade takes an exclusive table +lock, refuses, and rolls back. Quiesce every protected write before attempting +the downgrade so waiting writers cannot resume against a removed table. + ## JWKS Rotation And Outage Rotation procedure: