From 76076cbd905a26ef05ebc20a6465f03d6bcc2de9 Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Tue, 12 May 2026 01:15:21 +0300 Subject: [PATCH] [AZ-491] [AZ-492] [AZ-493] [AZ-494] [AZ-495] [AZ-496] Cycle 3 Step 9: 6 task specs Drains the cycle-2 retrospective top-3 improvement actions plus three carried-forward security and process items, into 6 individually- trackable PBIs: - AZ-491 (3 SP) consolidate JWT test-mint helpers (Retro Action 1) - AZ-492 (3 SP) perf harness PT-07 + PT-08 + JWT-attach (Retro Act. 2) - AZ-493 (2 SP) integration test DB-reset hook (Retro Action 3) - AZ-494 (2 SP) JWT iss/aud validation (cycle-2 F-AUTH-2 Medium) - AZ-495 (1 SP) doc-folder convention for WebApi (cycle 1+2 F1 carry) - AZ-496 (2 SP) bump AspNetCore 8.0.21 -> 8.0.25 (cycle 1+2 D1+D3) All 6 at-or-below the user-rule 5 SP cap. AZ-494 gated on admin-team confirming iss/aud values. Cycle 3 total: 13 SP. Autodev pointer advances to Step 10 Implement. Co-authored-by: Cursor --- _docs/02_tasks/_dependencies_table.md | 25 ++++ .../AZ-491_consolidate_jwt_test_helpers.md | 119 +++++++++++++++ ...Z-492_perf_harness_pt07_pt08_jwt_attach.md | 131 ++++++++++++++++ .../AZ-493_integration_test_db_reset_hook.md | 133 +++++++++++++++++ .../todo/AZ-494_jwt_iss_aud_validation.md | 140 ++++++++++++++++++ .../todo/AZ-495_doc_folder_convention.md | 119 +++++++++++++++ .../todo/AZ-496_bump_aspnetcore_8025.md | 119 +++++++++++++++ _docs/_autodev_state.md | 4 +- 8 files changed, 788 insertions(+), 2 deletions(-) create mode 100644 _docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md create mode 100644 _docs/02_tasks/todo/AZ-492_perf_harness_pt07_pt08_jwt_attach.md create mode 100644 _docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md create mode 100644 _docs/02_tasks/todo/AZ-494_jwt_iss_aud_validation.md create mode 100644 _docs/02_tasks/todo/AZ-495_doc_folder_convention.md create mode 100644 _docs/02_tasks/todo/AZ-496_bump_aspnetcore_8025.md diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index 2bb55f1..2995e8d 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -71,6 +71,19 @@ Roadmap: `_docs/04_refactoring/03-code-quality-refactoring/analysis/refactoring_ | AZ-487 | JWT validation baseline (HS256, JWT_SECRET, all endpoints) | — (consumes suite-level contract `suite/_docs/10_auth.md`) | 2 | Done (In Testing) | | AZ-488 | UAV tile upload endpoint with batch + 5-rule quality gate | AZ-487 (hard prereq), AZ-484 contract `tile-storage.md` v1.0.0 | 8 (over-cap, user-accepted) | Done (In Testing) | +### Step 9 cycle 3 — New Task: Cycle-2 follow-ups (test infra + security hardening + process) + +Source: cycle-2 retrospective top-3 improvement actions + carried-forward security and process items (`_docs/06_metrics/retro_2026-05-11_cycle2.md`). + +| Task | Title | Depends On | Points | Status | +|------|-------|-----------|--------|--------| +| AZ-491 | Consolidate JWT test-mint helpers | — (logically follows AZ-487 which introduced both copies) | 3 | To Do | +| AZ-492 | Perf harness: PT-07 + PT-08 + JWT-attach in run-performance-tests.sh | AZ-487 (hard — Bearer token); AZ-491 (soft — token-mint reuse) | 3 | To Do | +| AZ-493 | Integration test DB-reset hook | — | 2 | To Do | +| AZ-494 | JWT iss/aud validation (enable + configure) | AZ-487 (extends `AddSatelliteJwt`); external: admin team confirms iss/aud values | 2 | To Do (blocked on cross-team input) | +| AZ-495 | Resolve doc-folder convention for WebApi component | — | 1 | To Do | +| AZ-496 | Bump Microsoft.AspNetCore.OpenApi + JwtBearer to 8.0.25 | — | 2 | To Do | + ## Execution Order ### Step 6 @@ -97,6 +110,17 @@ Phase 4 (Typing/config/tooling/polish): AZ-371 → AZ-370 → AZ-373 → AZ-374 1. AZ-487 — JWT validation baseline (must merge first; AZ-488 hard-depends on it) 2. AZ-488 — UAV tile upload endpoint + 5-rule quality gate (consumer of both AZ-484 contract and AZ-487 auth) +### Step 9 cycle 3 + +Independent tracks — most tasks can run in parallel; the only ordering constraint is the AZ-491 → AZ-492 soft dependency for token-mint reuse. + +1. AZ-495 (1 SP) — doc-folder convention. Cheapest unblocker; lands first to stop the F1 recurrence. +2. AZ-491 (3 SP) — consolidate JWT test-mint helpers. Pre-stages AZ-492 if implementer picks Option B. +3. AZ-493 (2 SP) — integration test DB-reset hook. Independent. +4. AZ-496 (2 SP) — bump AspNetCore 8.0.25. Independent. +5. AZ-492 (3 SP) — perf harness. After AZ-491 if Option B; else any time. +6. AZ-494 (2 SP) — JWT iss/aud validation. Gated on cross-team input; not blocked by other cycle-3 tasks. + ## Total Effort Step 6: 6 tasks, 17 story points @@ -104,6 +128,7 @@ Step 8 (02-coupling-refactoring): 6 tasks, 17 story points Step 8 (03-code-quality-refactoring): 27 tasks, ~66 story points Step 9 cycle 1: 1 task created (AZ-484, 5 pts) Step 9 cycle 2: 2 tasks created (AZ-487 = 2 pts, AZ-488 = 8 pts over-cap user-accepted) — total 10 pts +Step 9 cycle 3: 6 tasks created (AZ-491 = 3 pts, AZ-492 = 3 pts, AZ-493 = 2 pts, AZ-494 = 2 pts, AZ-495 = 1 pt, AZ-496 = 2 pts) — total 13 pts ## Coverage Verification diff --git a/_docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md b/_docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md new file mode 100644 index 0000000..53838d6 --- /dev/null +++ b/_docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md @@ -0,0 +1,119 @@ +# Consolidate JWT test-mint helpers + +**Task**: AZ-491_consolidate_jwt_test_helpers +**Name**: Consolidate JWT test-mint helpers +**Description**: Eliminate the duplicate JWT-minting logic that exists in both `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` by moving the canonical implementation to a single shared location consumed by both test projects. +**Complexity**: 3 points +**Dependencies**: None (no in-repo task dependency; logically follows AZ-487 which introduced both copies) +**Component**: Test infrastructure (`SatelliteProvider.Tests` + `SatelliteProvider.IntegrationTests`) +**Tracker**: AZ-491 +**Epic**: none (cycle-3 test-infrastructure hardening) + +## Problem + +During cycle 2 (AZ-487), JWT token-minting logic was written twice — once in `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` (unit-side) and once in `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken` (integration-side). The two implementations are near-identical: same signing algorithm, same claim shape, same negative-lifetime workaround. Both contained the same `Expires < NotBefore` bug and both required independent fixes in separate commits (`f64d0d7` for unit-side, `11b7074` for integration-side). + +The cycle-2 retrospective identified this as Improvement Action 1: "Consolidate JWT token-mint helpers into one shared utility" (`_docs/06_metrics/retro_2026-05-11_cycle2.md` § 7). LESSONS.md ring buffer carries the lesson under `[testing]`. Until this is resolved, every future JWT-side change pays a dual-fix tax, and the two implementations will continue to drift on shape, claim handling, or signing parameters — a real security-relevance risk for the surface that mints test credentials. + +## Outcome + +- A single canonical JWT-minting implementation exists, consumable by both `SatelliteProvider.Tests` and `SatelliteProvider.IntegrationTests`. +- `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken` and `MintExpiredToken` and `TamperSignature` are deleted; existing call sites (`Program.cs`, `JwtIntegrationTests`, `UavUploadTests`) consume the consolidated factory. +- Runner-side concerns (`JwtTestHelpers.cs::ResolveSecretOrThrow`, `AttachDefaultAuthorization`) remain in `SatelliteProvider.IntegrationTests` — only the mint logic moves. +- All 253 unit tests + the full integration test suite pass end-to-end with the consolidated factory. +- A code-review checklist item is added to `.cursor/skills/code-review/SKILL.md` Phase 6 so that future duplicate-helper situations are flagged at review time, not at retrospective time. + +## Scope + +### Included + +- Move (or re-locate) the canonical `JwtTokenFactory` class so both test projects can reference it. Implementer chooses between two viable shapes (see § Risks & Mitigation Risk 1): + - **Option A**: new test-support class library `SatelliteProvider.TestSupport` (no test framework), referenced by both `SatelliteProvider.Tests` and `SatelliteProvider.IntegrationTests`. Larger upfront cost; cleaner long-term layering. + - **Option B**: `SatelliteProvider.IntegrationTests` gains a `ProjectReference` to `SatelliteProvider.Tests`, consuming the existing `TestUtilities/JwtTokenFactory.cs` directly. Smaller cost; integration tests acquire a dependency on unit tests (slightly unusual but precedented in some .NET test layouts). +- Delete `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken`, `MintExpiredToken`, and `TamperSignature`. Their integration-test call sites switch to the canonical factory. +- Update all call sites: `SatelliteProvider.IntegrationTests/Program.cs`, `JwtIntegrationTests.cs`, `UavUploadTests.cs`. +- Add a code-review checklist row in `.cursor/skills/code-review/SKILL.md` Phase 6 (Cross-Task Consistency): "If two test projects contain near-identical helper logic with no shared source-of-truth, flag as a Medium finding for consolidation." +- Update `_docs/02_document/modules/tests_unit.md` and `tests_integration.md` to point at the new consolidated location (one-line each). + +### Excluded + +- Any change to production code in `SatelliteProvider.Api`, `SatelliteProvider.Common`, or service projects. +- Any change to `UavTileImageFactory` (also a candidate for consolidation but out of scope here — keep this PBI surgical). +- Any change to `ResolveSecretOrThrow` / `AttachDefaultAuthorization` — these are runner-side concerns and stay in `SatelliteProvider.IntegrationTests`. +- Changes to the JWT algorithm, claim shape, or `TokenValidationParameters` — this PBI is a pure refactor, not a behavior change. + +## Acceptance Criteria + +**AC-1: Single source of truth** +Given the repository after this PBI lands +When a developer searches for `JwtSecurityToken(` constructor calls in test code +Then exactly one location implements JWT minting; no duplicate implementation exists in another test project. + +**AC-2: Existing integration tests pass unchanged in behavior** +Given the full integration test suite (`scripts/run-tests.sh --full`) +When the suite executes against the consolidated factory +Then all integration test scenarios produce the same pass/fail outcomes as they did prior to this PBI. + +**AC-3: Existing unit tests pass unchanged in behavior** +Given the full unit test suite (`dotnet test SatelliteProvider.Tests`) +When the suite executes against the consolidated factory +Then all 253 unit test scenarios produce the same pass/fail outcomes as they did prior to this PBI. + +**AC-4: Runner-side concerns preserved** +Given the integration test runner +When the runner starts up +Then `ResolveSecretOrThrow` still reads `JWT_SECRET` from the environment with the same minimum-length validation, AND `AttachDefaultAuthorization` still sets the `Authorization: Bearer …` header on the shared `HttpClient`. + +**AC-5: Cycle-2 fixes remain effective** +Given a fixture that requests an expired-lifetime token (negative `TimeSpan`) +When the consolidated factory mints the token +Then `Expires` is greater than `NotBefore` (the cycle-2 `IDX12401` fix is preserved), AND the token fails `ValidateLifetime` downstream (the lifetime check still fires). + +**AC-6: Code-review rule lands** +Given a future PR that adds a near-identical helper to two test projects +When the code-review skill runs Phase 6 (Cross-Task Consistency) +Then the checklist item from this PBI fires and the finding is recorded as a Medium-severity duplicate-helper item. + +## Non-Functional Requirements + +**Compatibility** +- Must compile against `net8.0` target framework (same as all existing projects). +- Must not introduce new external NuGet dependencies — the consolidated factory uses only `System.IdentityModel.Tokens.Jwt` and `Microsoft.IdentityModel.Tokens`, already referenced by both test projects. + +**Reliability** +- The negative-lifetime workaround documented in the existing factory (cycle-2 `f64d0d7` / `11b7074` commits) must be preserved exactly — `Expires > NotBefore` is invariant. + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| AC-1 | The consolidated factory is the sole `JwtSecurityToken` constructor call site in test code | A grep of `new JwtSecurityToken(` across all `**/*.cs` under both test projects returns exactly one match (in the consolidated factory) | +| AC-3 | All existing `JwtTokenFactoryTests` cases | Pass against the consolidated factory at its new location | +| AC-5 | Expired-token fixture path | The factory's `Create` (or new equivalent) with `lifetime: TimeSpan.FromMinutes(-5)` produces `Expires > NotBefore` AND the resulting token fails `ValidateLifetime` | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-2 | Integration test runner started with a valid `JWT_SECRET` | `JwtIntegrationTests` + `UavUploadTests` run end-to-end through the consolidated factory | Same pass/fail outcomes as the pre-PBI baseline | Compatibility | +| AC-4 | Integration test runner started with `JWT_SECRET` set to a 32-byte value | `ResolveSecretOrThrow` returns the secret; `AttachDefaultAuthorization` sets the `Authorization` header on the shared `HttpClient` | Runner-side behavior unchanged | — | + +## Constraints + +- Stay within the existing project layout — the chosen Option A or B must not require renaming or restructuring `SatelliteProvider.Tests` / `SatelliteProvider.IntegrationTests`. +- The consolidated factory must remain a `public static class` (current shape) — do not introduce DI, abstractions, or factories around it. This is test infrastructure, not production code. +- Comment in the consolidated factory must retain the explanation of the `Expires <= NotBefore` workaround so future readers don't "fix" the apparent oddity. + +## Risks & Mitigation + +**Risk 1: Option A vs Option B trade-off** +- *Risk*: Option A (new `SatelliteProvider.TestSupport` library) is the cleaner long-term shape but costs ~1 extra csproj, a new ProjectReference graph entry, and a new module in `module-layout.md`. Option B (integration tests reference unit tests) is cheaper but creates a slightly unusual dependency direction. +- *Mitigation*: The implementer chooses based on whether other shared test utilities (e.g., `UavTileImageFactory`) are likely to be consolidated in the next 1–2 cycles. If yes, Option A pays for itself quickly. If no, Option B is fine. Document the choice in the batch report so future agents understand the precedent. + +**Risk 2: Drift in the comment block that documents the `Expires <= NotBefore` workaround** +- *Risk*: Moving the file may invite a "cleanup" of the comment, removing the rationale and re-introducing the `IDX12401` bug on a future edit. +- *Mitigation*: AC-5 + the unit test that exercises the expired-token path catch any regression. The comment block survives the move verbatim. + +**Risk 3: Removal of duplicate file exposes hidden coupling** +- *Risk*: Some other test file may currently `using SatelliteProvider.IntegrationTests` with intent to consume `JwtTestHelpers` symbols beyond just `MintValidToken` (e.g., `DefaultSubject` constant). +- *Mitigation*: Before deletion, grep for all references to `JwtTestHelpers.` symbols in the codebase and replace each one with the consolidated equivalent. The `DefaultSubject` constant moves with the mint logic. diff --git a/_docs/02_tasks/todo/AZ-492_perf_harness_pt07_pt08_jwt_attach.md b/_docs/02_tasks/todo/AZ-492_perf_harness_pt07_pt08_jwt_attach.md new file mode 100644 index 0000000..1ebea0e --- /dev/null +++ b/_docs/02_tasks/todo/AZ-492_perf_harness_pt07_pt08_jwt_attach.md @@ -0,0 +1,131 @@ +# Performance harness: PT-07 + PT-08 + JWT-attach in run-performance-tests.sh + +**Task**: AZ-492_perf_harness_pt07_pt08_jwt_attach +**Name**: Perf harness drains all 3 deferred items +**Description**: Promote the deferred PT-07 (route-tile fetch warm cache) and PT-08 (UAV tile batch upload latency) NFRs into actual runnable scenarios in `scripts/run-performance-tests.sh`, AND fix the script so PT-01..PT-06 stop returning 401 against the post-AZ-487 build by attaching a Bearer token to every request. +**Complexity**: 3 points +**Dependencies**: AZ-487 (Bearer-token attach depends on the JWT_SECRET / token-mint surface introduced in AZ-487); soft dependency on AZ-491 (Option B token-mint reuse) +**Component**: Test infrastructure (`scripts/run-performance-tests.sh`) + perf NFR coverage (`_docs/02_document/tests/performance-tests.md`) +**Tracker**: AZ-492 +**Epic**: none (cycle-3 test-infrastructure hardening) + +## Problem + +The performance test gate has now been 0-of-N for two cycles running, and the perf-side rot is actively masking real regressions: + +1. **Cycle 1 (AZ-484)**: PT-07 (route-tile fetch warm cache) was added to `performance-tests.md` and the traceability matrix, but the runner-script implementation was not. Recorded as `Deferred — harness work tracked in _docs/_process_leftovers/2026-05-11_perf-pt07-harness.md`. Step 15 Performance Test gate marked all scenarios `Unverified`. Cycle 1 retrospective Action 2 introduced the "Deferred-status NFRs are allowed at most once" rule (LESSONS.md `[process]`). +2. **Cycle 2 (AZ-488)**: PT-08 (UAV tile batch upload latency) was added to `performance-tests.md`, again as Deferred under the cycle-1-sanctioned escape hatch. The leftover file was updated with a PT-08 follow-on instruction. +3. **Cycle 2 (AZ-487 side effect)**: `scripts/run-performance-tests.sh` does not attach an `Authorization: Bearer …` header to its outbound requests. After AZ-487 made every endpoint `RequireAuthorization()`, PT-01..PT-06 now return 401 for every call. Step 15 Performance Test gate at cycle 2 had to be skipped because of this script rot. Recorded as a third item in `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md`. + +Cycle 2 retrospective Improvement Action 2 (`_docs/06_metrics/retro_2026-05-11_cycle2.md` § 7) promoted "schedule PT-07 + PT-08 + JWT-attach as actual feature work" to a top-3 action. Per the cycle-2 LESSONS.md `[process]` rule, any new Deferred-status NFR after this point requires this PBI to land first. + +## Outcome + +- `scripts/run-performance-tests.sh` mints (or reads from `.env`) a valid HS256 JWT signed with `JWT_SECRET` and attaches `Authorization: Bearer ` to every probe request. +- PT-01..PT-06 return real HTTP 200 responses with measurements written to `_docs/06_metrics/perf_.md` — not 401, not `Unverified`. +- PT-07 (route-tile fetch warm cache) is implemented as a runnable scenario in the script. Its row in `performance-tests.md` moves from `Status: Deferred` to `Status: Implemented` with the measurement target documented. +- PT-08 (UAV tile batch upload latency) is implemented as a runnable scenario in the script. Same status transition. +- The leftover file `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` is deleted (or empty enough to delete) after this PBI lands — all three items resolved. +- A regression test in CI runs the perf script in smoke mode (single iteration per scenario) to keep the script honest going forward. + +## Scope + +### Included + +- Add Bearer-token attach logic to `scripts/run-performance-tests.sh`. Two viable shapes (implementer's choice): + - **Option A**: script accepts `PERF_JWT_TOKEN` env var (operator pre-mints) and attaches via `curl -H "Authorization: Bearer $PERF_JWT_TOKEN"`. + - **Option B**: script invokes a small `dotnet run --project SatelliteProvider.TestSupport` (or equivalent) to mint the token from `JWT_SECRET` on the fly, then attaches it. Reuses the consolidated factory from `01_consolidate_jwt_test_helpers` if that PBI ships first. +- Implement PT-07 scenario per its existing spec in `_docs/02_document/tests/performance-tests.md` (cold + warm region request, measure `tile_lookup_ms` vs `total_ms`). +- Implement PT-08 scenario per its existing spec (UAV batch upload of N tiles, measure end-to-end latency + per-item gate cost). +- Update `performance-tests.md`: move PT-07 and PT-08 from `Status: Deferred` to `Status: Implemented`; document measurement targets and acceptable variance. +- Update `_docs/02_document/tests/traceability-matrix.md` — refresh PT-07 + PT-08 coverage notes from `Unverified` to actual scenario IDs. +- Add a CI smoke run of the perf script (or document why none — e.g., needs full DB seed; in that case, gate at Step 15 stays manual but the script is verified per cycle by autodev). +- Delete `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` (or trim to only any genuinely-future items not addressed here). + +### Excluded + +- Any new perf scenarios beyond PT-07 + PT-08 + the 401 fix. +- Any change to production code; this is harness-only work. +- Any threshold-based PASS/FAIL gating in autodev Step 15 — the existing skill handles threshold comparison; this PBI only makes the scenarios runnable. +- Replacing `curl`-based probes with a structured load tool (k6, JMeter, etc.) — that is a separate decision. + +## Acceptance Criteria + +**AC-1: PT-01..PT-06 no longer 401** +Given the post-AZ-487 build is running with a valid `JWT_SECRET` +When `scripts/run-performance-tests.sh` runs the existing PT-01..PT-06 scenarios +Then every probe request receives an HTTP 200/2xx response (or the scenario's documented expected status), AND zero 401 responses appear in the perf log output. + +**AC-2: PT-07 runs to completion** +Given a clean tile cache state +When the script runs PT-07 (cold then warm region request) +Then `tile_lookup_ms` and `total_ms` are measured for both cold and warm requests, AND the warm `tile_lookup_ms` is documented as less than the cold value (no specific threshold required — just measurable). + +**AC-3: PT-08 runs to completion** +Given a valid JWT token with the `GPS` permission +When the script runs PT-08 (UAV batch upload of N tiles, N = parameterized, default 10) +Then the script reports per-item gate cost and end-to-end batch latency, AND all N items return either `accepted` or a documented reject reason (no `STORAGE_FAILURE` from harness misconfiguration). + +**AC-4: Spec status reflects implementation** +Given the post-PBI repository state +When `_docs/02_document/tests/performance-tests.md` is read +Then PT-07 and PT-08 are marked `Status: Implemented` (or equivalent active state), AND the section is reformatted to no longer claim "harness work tracked in ". + +**AC-5: Leftover drained** +Given `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` +When this PBI lands +Then the file is either deleted or trimmed to genuinely-future items only (and the cycle-1 + cycle-2 follow-on instructions removed because they are now resolved). + +**AC-6: Token-mint surface reused, not duplicated** +Given the consolidated JWT factory exists (after `01_consolidate_jwt_test_helpers`) OR the existing per-project mint helpers +When the perf script mints its token +Then it reuses the canonical surface from `01_consolidate_jwt_test_helpers` (if that PBI has landed) rather than inlining a third mint implementation in the shell script. + +## Non-Functional Requirements + +**Reliability** +- Script must fail loudly (non-zero exit code, clear message) if `JWT_SECRET` is unset or shorter than 32 bytes — same contract as `scripts/run-tests.sh`. +- Script must not silently skip a scenario; if a scenario cannot run, exit non-zero with an explicit reason. + +**Compatibility** +- Bash 4+ compatible (the script already uses bash; do not introduce Python or other runtime dependencies for token minting unless they leverage an existing project — Option B above). + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| AC-1 | The Bearer-attach logic itself (if a helper function is introduced) | Returns a valid `Authorization: Bearer ` header value when invoked with a known secret | +| AC-6 | Token mint reuses canonical surface (after `01_consolidate_jwt_test_helpers` lands) | A grep across `scripts/` and test projects shows no new `new JwtSecurityToken(` constructor calls | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-1 | API container running with auth; valid `JWT_SECRET` exported | Run `scripts/run-performance-tests.sh` PT-01..PT-06 | All requests return non-401 status codes | Reliability | +| AC-2 | Clean tile cache; valid Bearer token attached | Run PT-07 cold + warm region request | Cold and warm `tile_lookup_ms` reported separately; warm < cold (no threshold required) | — | +| AC-3 | Valid Bearer token with `GPS` permission attached | Run PT-08 UAV batch upload of 10 tiles | All 10 items return `accepted` or documented reject; per-item gate cost reported | — | +| AC-4 | Post-PBI repo state | Read `performance-tests.md` | PT-07 + PT-08 status `Implemented`; no "Deferred — harness work" language | — | + +## Constraints + +- Must not regress the existing `scripts/run-tests.sh` flow. +- Must not bake a Bearer token into git-tracked files. The token is minted at runtime or read from a git-ignored env var. +- If the consolidated test-support library from `01_consolidate_jwt_test_helpers` exists, this PBI MUST reuse it for token minting — no third copy of the mint logic. + +## Risks & Mitigation + +**Risk 1: Dependency on `01_consolidate_jwt_test_helpers`** +- *Risk*: This PBI is cleaner if Option B (mint via `SatelliteProvider.TestSupport`) is taken AND that project exists. If `01_consolidate_jwt_test_helpers` chose Option B for itself (no new library), Option B here is unavailable. +- *Mitigation*: Implementer picks between Option A (pre-minted token via env var) and Option B based on the state of `01_consolidate_jwt_test_helpers` at start of work. Option A always works regardless. + +**Risk 2: Perf measurements are unstable on dev hardware** +- *Risk*: PT-07 + PT-08 are timing-sensitive. Running on a developer laptop will produce noisy results that look like regressions. +- *Mitigation*: The script reports measurements but does NOT gate on them. Autodev Step 15 already has an A/B/C gate on threshold failures handled by the test-run skill in perf mode. This PBI's scope is "make scenarios runnable", not "set thresholds". + +**Risk 3: Token expiry mid-test** +- *Risk*: A 1-hour-lifetime token may expire during a long perf run. +- *Mitigation*: Mint with a generous lifetime (e.g., 4h) when starting the script. Document the lifetime choice in the script header. + +**Risk 4: CI smoke run becomes a maintenance burden** +- *Risk*: A CI-side perf smoke run that runs every commit may be flaky and become a source of noise. +- *Mitigation*: Document explicitly whether a CI smoke run is added. If added, run only on `dev` push (not every PR commit) and only with a tight per-scenario timeout to catch script-rot, not perf regressions. diff --git a/_docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md b/_docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md new file mode 100644 index 0000000..2cb11be --- /dev/null +++ b/_docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md @@ -0,0 +1,133 @@ +# Integration test DB-reset hook + +**Task**: AZ-493_integration_test_db_reset_hook +**Name**: Integration test DB-reset hook +**Description**: Replace the cycle-2 wallclock-seeded `_coordinateCounter` workaround in `UavUploadTests` with a real Postgres state-reset hook executed at integration test runner startup, so future integration tests don't have to invent their own collision-avoidance scheme against the persistent Docker volume. +**Complexity**: 2 points +**Dependencies**: None +**Component**: Test infrastructure (`SatelliteProvider.IntegrationTests`) + optionally `scripts/run-tests.sh` +**Tracker**: AZ-493 +**Epic**: none (cycle-3 test-infrastructure hardening) + +## Problem + +During cycle 2 Step 11 (Run Tests), the new `UavUploadTests.MultiSourceCoexistence_AZ484_Cycle2` failed with `duplicate key value violates unique constraint "idx_tiles_unique_location_source"` while seeding a `google_maps` row. Root cause: the `_coordinateCounter` field in `UavUploadTests` reset to 0 on every test-runner process start, while the Postgres named volume in `docker-compose.yml` persists `tiles` rows across `docker-compose down` cycles. + +The cycle-2 fix (`dc3dabe [AZ-488] fix: seed UavUploadTests coordinate counter from wall-clock`) seeded `_coordinateCounter` from `(int)((DateTime.UtcNow.Ticks / TimeSpan.TicksPerSecond) % 1_000_000)`. This makes per-run coordinates *probabilistically* unique enough to avoid collision in normal usage — but it's a workaround, not isolation: + +- Two fast back-to-back runs within the same second produce the same seed. +- Parallel test execution against the same DB will collide. +- Every new integration test that inserts into `tiles` will have to invent its own collision-avoidance scheme. +- The DB grows monotonically across cycles, making `docker-compose down` (without `-v`) progressively slower. + +Cycle-2 retrospective Improvement Action 3 (`_docs/06_metrics/retro_2026-05-11_cycle2.md` § 7) promoted this to a top-3 action. LESSONS.md `[testing]` carries the lesson "Integration tests must explicitly reset DB state at startup — relying on wallclock seeds is a workaround, not isolation". + +## Outcome + +- Integration test runner explicitly resets relevant DB tables (`tiles`, `regions`, `routes`, `route_points`, `route_regions`) to a known empty state at startup before any test class runs. +- The wallclock-seeded `_coordinateCounter` in `UavUploadTests` is no longer required — `_coordinateCounter` can return to 0-initialized behavior. (Implementer's call whether to actually revert the seed; the safety net works either way.) +- The DB-reset behavior is opt-out — running with an explicit flag (e.g., `--keep-state`) skips the reset so a developer can inspect leftover state. +- `scripts/run-tests.sh` invokes the reset path by default; only `--keep-state` skips it. +- `_docs/02_document/modules/tests_integration.md` documents the new convention so future test authors know to rely on it rather than invent their own scheme. +- Existing integration tests pass end-to-end against the reset hook. + +## Scope + +### Included + +- Add a DB-reset routine to `SatelliteProvider.IntegrationTests/Program.cs` (or a new helper class) that: + - Connects to Postgres via the same connection string used by other tests. + - Truncates (in FK-safe order: `route_regions` → `route_points` → `routes` → `regions` → `tiles`) all tables that integration tests insert into. + - Runs ONLY when `ASPNETCORE_ENVIRONMENT=Testing` OR an explicit env var (e.g. `INTEGRATION_RESET_STATE=1`) is set. Never runs against a non-test environment. +- Wire the reset routine to execute exactly once at startup, before any test class instantiation, with structured logging of what was truncated. +- Add `--keep-state` flag handling so a developer can `./scripts/run-tests.sh --full --keep-state` for debugging. +- Optionally: extend `scripts/run-tests.sh` with `--reset-volume` flag that does `docker-compose down -v` between runs — but the in-runner truncate is the primary path. +- Optionally revert the `_coordinateCounter` wallclock-seed workaround in `UavUploadTests`, OR keep it as defense-in-depth. Implementer's choice; document the decision in the batch report. +- Update `_docs/02_document/modules/tests_integration.md` § Reliability with a short paragraph: "Integration tests rely on a startup DB-reset; new tests can use deterministic seed values." + +### Excluded + +- Any change to production code or the API container. +- Migration to a per-test-class transaction-rollback pattern (e.g., test fixtures wrapping each test in a transaction) — that's a larger refactor, separate PBI if ever needed. +- Resetting the file system (`./tiles/`, `./ready/`) — UAV uploads do write to `./tiles/uav/`, but the file-side collision risk is lower; address only if a test demonstrably hits it. +- Renaming or restructuring `UavUploadTests` itself. + +## Acceptance Criteria + +**AC-1: Empty-state on startup** +Given the integration test runner starts up with `ASPNETCORE_ENVIRONMENT=Testing` +When the runner connects to Postgres +Then `SELECT count(*) FROM tiles`, `regions`, `routes`, `route_points`, `route_regions` each return 0 before the first test class begins. + +**AC-2: Wallclock workaround no longer needed** +Given `UavUploadTests._coordinateCounter` is reverted to a 0-initialized default (implementer's choice) +When the full integration suite runs back-to-back twice within the same calendar second +Then both runs pass without any `duplicate key value violates unique constraint "idx_tiles_unique_location_source"` errors. + +**AC-3: Opt-out preserves state** +Given `scripts/run-tests.sh --full --keep-state` is invoked +When the runner starts +Then the DB-reset routine is skipped, AND rows from a previous run are still present at runner startup. + +**AC-4: Reset only fires in test environment** +Given the runner is somehow misconfigured to point at a non-test connection string (e.g., `ASPNETCORE_ENVIRONMENT=Production`) +When the runner starts +Then the DB-reset routine refuses to execute and exits with a clear error message. + +**AC-5: Documentation reflects new convention** +Given the post-PBI repo +When `_docs/02_document/modules/tests_integration.md` is read +Then a Reliability paragraph documents the startup-reset convention and the `--keep-state` opt-out. + +**AC-6: Existing tests pass unchanged** +Given the full integration test suite +When it runs against the reset hook +Then all existing test scenarios produce the same pass/fail outcomes as before this PBI. + +## Non-Functional Requirements + +**Reliability** +- Reset MUST be idempotent — multiple invocations produce the same final state. +- Reset MUST run inside a transaction (or be otherwise atomic) so a mid-truncate failure doesn't leave the DB in a half-empty state. + +**Performance** +- Reset on an empty DB completes in < 100 ms. +- Reset on a DB with O(10K) rows completes in < 1 s (TRUNCATE is O(1) for table-size in PG, but FK-cascade order matters). + +**Safety** +- The reset code path MUST refuse to run if connected to a database whose name does not contain `_test` or whose environment is not `Testing`. Two independent guards (env var + connection-string check). + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| AC-4 | Reset routine called with `ASPNETCORE_ENVIRONMENT=Production` | Throws (or no-ops with structured error log) — does NOT execute truncates | +| AC-1 | Truncate-order helper (if extracted as a method) | Returns the table names in FK-safe order | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-1 | DB seeded with leftover rows from a previous run | Run `./scripts/run-tests.sh --full` | All target tables empty at start of test execution; full suite passes | Reliability | +| AC-2 | Two back-to-back full runs within 1s | Run `./scripts/run-tests.sh --full` twice without `--keep-state` | Both runs pass with zero unique-constraint violations | Reliability | +| AC-3 | DB seeded with previous-run rows | Run `./scripts/run-tests.sh --full --keep-state` | Rows present at startup; full suite still passes (or fails on AC-2 collision — that's expected) | — | + +## Constraints + +- Reset must use parameterless TRUNCATE (no per-test data injection from the helper) — keep the helper minimal. +- Must not require new NuGet dependencies — Npgsql + Dapper are already present. +- Connection-string detection MUST use the same `ConnectionStrings:Postgres` key as the production code; do not introduce a second connection-string concept for tests. + +## Risks & Mitigation + +**Risk 1: Accidental truncate against production DB** +- *Risk*: A misconfigured CI runner or developer with `JWT_SECRET` overridden but `ConnectionStrings:Postgres` pointing at a real DB would have their data wiped. +- *Mitigation*: Two-guard model — refuse to truncate unless BOTH (a) `ASPNETCORE_ENVIRONMENT=Testing` AND (b) the DB name contains `_test` (or equivalent). The integration test compose file already uses a separate DB; verify and document the name pattern. + +**Risk 2: Truncate ordering wrong for future schema additions** +- *Risk*: A future PBI adds a new table with a foreign key to `tiles` but doesn't update the reset order; reset fails on FK constraint. +- *Mitigation*: Document the order in `_docs/02_document/modules/tests_integration.md`. Add a checklist row to the new-task / decompose templates: "If your task adds a table referenced by integration tests, update the DB-reset helper's truncate order." + +**Risk 3: Tests that expect existing data from a previous run** +- *Risk*: If a developer pattern emerged (post-cycle-2) where one test class inserts and a later test class reads, those will break. +- *Mitigation*: Search for cross-test-class data dependencies before this PBI lands. Cycle-1 / cycle-2 integration tests do not exhibit this pattern — each test class seeds its own data. AC-6 catches any unknown coupling. diff --git a/_docs/02_tasks/todo/AZ-494_jwt_iss_aud_validation.md b/_docs/02_tasks/todo/AZ-494_jwt_iss_aud_validation.md new file mode 100644 index 0000000..a0d3c6e --- /dev/null +++ b/_docs/02_tasks/todo/AZ-494_jwt_iss_aud_validation.md @@ -0,0 +1,140 @@ +# JWT iss/aud validation + +**Task**: AZ-494_jwt_iss_aud_validation +**Name**: JWT iss/aud validation +**Description**: Flip `ValidateIssuer` and `ValidateAudience` from `false` to `true` in `AddSatelliteJwt`, configure the expected `iss` / `aud` values via configuration, and update Bearer-token consumers and tests to mint tokens with matching claims. Closes Medium-severity finding F-AUTH-2 from the cycle-2 security audit. +**Complexity**: 2 points +**Dependencies**: AZ-487 (extends `AddSatelliteJwt` configuration); external dependency: admin team must confirm the expected `iss` / `aud` values before implementation can begin. +**Component**: WebApi (`SatelliteProvider.Api/Authentication`) + Test infrastructure +**Tracker**: AZ-494 +**Epic**: none (cycle-3 security hardening) + +## Problem + +The cycle-2 security audit (`_docs/05_security/security_report.md`, finding F-AUTH-2) noted that `AddSatelliteJwt` currently sets `ValidateIssuer = false` and `ValidateAudience = false` (`SatelliteProvider.Api/Authentication/AuthenticationServiceCollectionExtensions.cs`). The cycle-2 deploy report R3 carries the same item as an open follow-up. + +This was intentional during AZ-487 — the suite-level auth contract (`suite/_docs/10_auth.md`) does not currently mandate specific `iss`/`aud` values, and the admin team had not yet decided what those values should be. The mitigation in cycle 2 was: signature + lifetime validation alone are sufficient to keep this finding at Medium, not High. But the door is left open for token reuse across other satellite-suite services that share the same `JWT_SECRET` — a service-specific `aud` claim is the proper remediation. + +OWASP A07 (`_docs/05_security/owasp_review.md`) lists iss/aud validation as part of the JWT hardening backlog; this PBI closes the remaining gap. + +**Prerequisite: cross-team input required.** Before implementation can begin, the admin team must confirm: +1. The exact `iss` value the admin API stamps into tokens (e.g., `https://admin.azaion.example/`). +2. The `aud` value satellite-provider should require (e.g., `satellite-provider` or a URI). + +If the admin team has not provided these values when the PBI is picked up, the implementer STOPS and surfaces the blocker to the user. No fallback / placeholder value should be hardcoded. + +## Outcome + +- `AddSatelliteJwt` accepts `ValidIssuer` and `ValidAudience` from configuration (env var `JWT_ISSUER` / `JWT_AUDIENCE` preferred; `Jwt:Issuer` / `Jwt:Audience` config keys as fallback). +- `TokenValidationParameters` sets `ValidateIssuer = true`, `ValidateAudience = true`, `ValidIssuer = `, `ValidAudience = `. +- Startup fails fast if `JWT_ISSUER` or `JWT_AUDIENCE` is unset — same contract as `JWT_SECRET` (cycle-2 AC-5 of AZ-487). +- Test fixtures (consolidated JWT factory, or per-project helpers if `01_consolidate_jwt_test_helpers` hasn't shipped yet) mint tokens with matching `iss` / `aud` claims so existing tests continue to pass. +- `appsettings.Development.json` includes clearly-tagged dev values for `Jwt:Issuer` and `Jwt:Audience`. +- `.env.example` documents the two new env vars. +- `docker-compose.yml` + `docker-compose.tests.yml` forward `JWT_ISSUER` / `JWT_AUDIENCE` to both the API and the test runner. +- `_docs/05_security/security_report.md` and `owasp_review.md` are updated: F-AUTH-2 status moves from `Open` to `Resolved` with a reference to this PBI. + +## Scope + +### Included + +- Extend `AuthenticationServiceCollectionExtensions.AddSatelliteJwt` to read `JWT_ISSUER` / `JWT_AUDIENCE` from env / config and assign them to `TokenValidationParameters`. +- Add fail-fast validation: throw `InvalidOperationException` at startup if either value is missing or whitespace-only. +- Update test fixtures to mint tokens with the configured `iss` / `aud`: + - `JwtTokenFactory.Create` (or whatever shape exists post-`01_consolidate_jwt_test_helpers`) gains optional `issuer` / `audience` parameters with sensible test defaults. + - `SatelliteProvider.IntegrationTests/Program.cs` reads `JWT_ISSUER` / `JWT_AUDIENCE` from env and passes them to the default token. +- Update `appsettings.json` (empty placeholders), `appsettings.Development.json` (DEV-ONLY values). +- Update `.env.example` and `scripts/run-tests.sh` to load the two new env vars from `.env`. +- Update `_docs/02_document/modules/api_program.md` § Security and `architecture.md` § Security Architecture to document the iss/aud validation. +- Update `_docs/05_security/security_report.md` (mark F-AUTH-2 Resolved) and `owasp_review.md` § A07 (update the iss/aud bullet). +- Coordinate write-back: append a one-line "iss/aud validation enabled" entry to `suite/_docs/10_auth.md` if (and only if) the suite-level contract needs an update — open question for the admin team. + +### Excluded + +- Any change to the signing algorithm (still HS256). +- Any change to the `JWT_SECRET` validation, clock skew, or `RequireSignedTokens` semantics. +- Adding `nbf` (not-before) validation — already implicitly handled by `JwtSecurityToken`. +- Token expiry duration changes. +- Multi-tenant token validation (multiple valid issuers / audiences) — out of scope unless the admin team's confirmed values require it. + +## Acceptance Criteria + +**AC-1: Issuer validation enforced** +Given the API is running with `JWT_ISSUER=https://admin.example/` +When a request arrives with a Bearer token whose `iss` claim is `https://other.example/` +Then the request returns `401 Unauthorized` with the `WWW-Authenticate` header indicating issuer mismatch. + +**AC-2: Audience validation enforced** +Given the API is running with `JWT_AUDIENCE=satellite-provider` +When a request arrives with a Bearer token whose `aud` claim is `mission-planner` +Then the request returns `401 Unauthorized` with the `WWW-Authenticate` header indicating audience mismatch. + +**AC-3: Matching iss + aud accepted** +Given the API is running with configured `JWT_ISSUER` and `JWT_AUDIENCE` +When a request arrives with a Bearer token whose `iss` and `aud` claims match exactly +Then the request is authenticated and the existing endpoint handler is reached. + +**AC-4: Missing config fails fast** +Given the API starts without `JWT_ISSUER` (or with empty value) +When the application boots +Then it throws `InvalidOperationException` with a clear message naming the missing env var. + +**AC-5: Existing tests pass with matched fixtures** +Given the full integration suite is updated to mint tokens with the configured `iss`/`aud` +When the suite runs +Then all scenarios pass. + +**AC-6: Security artifacts updated** +Given the post-PBI repo +When `_docs/05_security/security_report.md` is read +Then F-AUTH-2 is marked Resolved with a reference to this PBI's tracker ID. + +**AC-7: Suite contract reflects reality** +Given the admin team's confirmed values +When implementation lands +Then `suite/_docs/10_auth.md` either documents the values or explicitly notes that satellite-provider validates them locally without dictating suite-wide values. + +## Non-Functional Requirements + +**Security** +- The configured `JWT_ISSUER` / `JWT_AUDIENCE` are NOT secrets — they can be public. But they MUST be operator-configurable so a production rotation (e.g., admin API URL change) does not require a code change. + +**Compatibility** +- Backwards-compatible with existing token consumers ONLY IF those consumers update their token-issuance to include the new claims. This is a coordinated rollout — see Risk 1. + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| AC-1 | `TokenValidationParameters.ValidIssuer` matches configured value | Returns the configured value | +| AC-2 | `TokenValidationParameters.ValidAudience` matches configured value | Returns the configured value | +| AC-4 | `AddSatelliteJwt` with `JWT_ISSUER` unset | Throws `InvalidOperationException` with `JWT_ISSUER` in message | +| AC-4 | `AddSatelliteJwt` with `JWT_AUDIENCE` unset | Throws `InvalidOperationException` with `JWT_AUDIENCE` in message | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-1 | API running with `JWT_ISSUER` set; token minted with wrong `iss` | `GET /api/satellite/tiles/latlon` with the wrong-iss token | 401 Unauthorized | Security | +| AC-2 | API running with `JWT_AUDIENCE` set; token minted with wrong `aud` | Same as AC-1 with wrong `aud` | 401 Unauthorized | Security | +| AC-3 | API running with matching values; token minted with matching `iss` + `aud` | Same probe | 200 OK | — | + +## Constraints + +- The configured `JWT_ISSUER` and `JWT_AUDIENCE` must come from environment / configuration — never hardcoded in source. +- Test environment uses dev-only values clearly tagged (e.g., `DEV-ONLY-iss-…`). +- Coordinated rollout: admin team confirms iss / aud values BEFORE implementation begins. + +## Risks & Mitigation + +**Risk 1: Cross-team coordination delay** +- *Risk*: Admin team has not yet decided on `iss` / `aud` values. Implementer is blocked. +- *Mitigation*: This PBI is explicitly gated on cross-team input. The blocker is recorded as a hard prerequisite in the task header. If the admin team is unresponsive after a reasonable window, this PBI is parked, not force-completed with placeholder values. + +**Risk 2: Coordinated breakage of existing token consumers** +- *Risk*: `gps-denied-onboard` and the mission-planner UI currently mint tokens whose `iss` / `aud` may not match what we configure. Flipping the validation flag breaks them. +- *Mitigation*: Coordinate the rollout with consumer teams BEFORE this PBI ships to production. Stage in `dev` first; verify consumer-side tokens still authenticate. The deploy report (analogous to cycle-2 R1) will flag this risk. + +**Risk 3: Future multi-tenant support** +- *Risk*: At some point we may need to accept tokens from multiple issuers (e.g., legacy + new admin API). Hardcoding a single `ValidIssuer` complicates that. +- *Mitigation*: The configuration shape allows array values (`TokenValidationParameters.ValidIssuers`) if/when needed. This PBI implements the single-value case; the upgrade path is straightforward. diff --git a/_docs/02_tasks/todo/AZ-495_doc_folder_convention.md b/_docs/02_tasks/todo/AZ-495_doc_folder_convention.md new file mode 100644 index 0000000..c423f76 --- /dev/null +++ b/_docs/02_tasks/todo/AZ-495_doc_folder_convention.md @@ -0,0 +1,119 @@ +# Doc-folder convention for WebApi component + +**Task**: AZ-495_doc_folder_convention +**Name**: Resolve doc-folder convention for WebApi +**Description**: Settle the recurring F1 code-review finding (cycle 1 AZ-487 + cycle 2 AZ-488) where task specs reference `_docs/02_document/components/01_web_api/description.md` which does not exist. Pick one: (a) create the stub folder, or (b) formalize the "WebApi lives in `modules/api_program.md`" convention. Update task-spec templates accordingly so the finding stops recurring. +**Complexity**: 1 point +**Dependencies**: None +**Component**: Documentation conventions (`_docs/02_document/`) + task-spec templates (`.cursor/skills/decompose/templates/task.md`) +**Tracker**: AZ-495 +**Epic**: none (process / quality) + +## Problem + +The cycle-1 (AZ-487) and cycle-2 (AZ-488) code reviews each surfaced an identical F1 finding (Low / Style): the task spec referenced `_docs/02_document/components/01_web_api/description.md`, a folder that has never existed in the codebase. The existing component-doc folders are: + +``` +_docs/02_document/components/ + 01_common/ + 02_data_access/ + 03_tile_downloader/ + 04_region_processing/ + 05_route_management/ +``` + +The WebApi component's documentation lives in `_docs/02_document/modules/api_program.md`. Each cycle's batch worked around the missing folder by updating `modules/api_program.md` instead. The decision item — create the stub folder OR formalize the modules-only convention — was carried as an open item but never settled. + +The cycle-2 retrospective (`_docs/06_metrics/retro_2026-05-11_cycle2.md` § 9) lists this as a decision item carried over. Until it's settled, every future cycle that touches WebApi will repeat the same finding and the same workaround. + +## Outcome + +- The decision is made and reflected in `_docs/02_document/module-layout.md` (canonical convention). +- Task-spec templates and any decompose-skill prompts that auto-generate task docs no longer reference a nonexistent path. +- The cycle 1 + cycle 2 code-review findings (F1) are explicitly resolved with a back-reference to this PBI's tracker ID. + +## Scope + +### Included + +Option **A — Create stub folder** (matches the layout convention of other components): +- Create `_docs/02_document/components/01_web_api/description.md` as a brief stub that: + - Documents the WebApi component at a level consistent with the other 5 component descriptions (purpose, architectural pattern, upstream/downstream). + - Either inlines the full content (preferred for consistency) or includes a single "for module-level detail, see `_docs/02_document/modules/api_program.md`" pointer. +- Renumber existing components if needed (e.g., 01_common stays, WebApi becomes 06_web_api at the layer-4 position — implementer's call based on architecture layering). +- Update task-spec templates to point at the new path. + +Option **B — Formalize modules-only convention for WebApi**: +- Update `_docs/02_document/module-layout.md` § "Documentation layout" with an explicit note: "The WebApi component (`SatelliteProvider.Api`) has no `components/*` folder — its documentation lives in `_docs/02_document/modules/api_program.md` because the API layer is documented at module-level rather than component-level." +- Update task-spec templates and any decompose-skill scaffolds to reference `modules/api_program.md` instead of a nonexistent `components/01_web_api/description.md` path. +- Add a checklist row to `.cursor/skills/new-task/SKILL.md` Step 4 (Codebase Analysis): "WebApi documentation is in `modules/api_program.md`, not `components/`." + +Either option: +- Update `_docs/03_implementation/reviews/batch_01_cycle2_review.md` and `batch_02_cycle2_review.md` with a one-line resolution note pointing at this PBI. +- Update the cycle-2 retrospective decision-item list (§ 9) to mark F1 resolved. + +### Excluded + +- Rewriting any existing module documentation. The module-level `api_program.md` stays where it is; this PBI is about the convention, not the content. +- Restructuring the other 5 component folders. +- Changing the layout of `modules/` vs `components/` — that's a much larger decision. + +## Acceptance Criteria + +**AC-1: One option chosen and documented** +Given the post-PBI repository +When `_docs/02_document/module-layout.md` is read +Then either (a) a `components/01_web_api/description.md` (or its renumbered equivalent) exists with non-trivial content, OR (b) an explicit "WebApi has no components/* folder" convention is documented with rationale. + +**AC-2: Templates updated** +Given a fresh `/new-task` invocation that touches WebApi +When the skill generates a task spec +Then the spec references the correct documentation location (Option A: the new component folder; Option B: `modules/api_program.md`) — never the nonexistent path. + +**AC-3: Code-review findings explicitly resolved** +Given the post-PBI repository +When `_docs/03_implementation/reviews/batch_01_cycle2_review.md` and `batch_02_cycle2_review.md` are read +Then their F1 finding sections include a resolution note pointing at this PBI. + +**AC-4: Regression check** +Given a hypothetical next cycle's code review +When the review runs Phase 6 (Cross-Task Consistency) +Then no F1-shaped finding fires because either the path now exists OR the convention is documented and templates were updated. + +## Non-Functional Requirements + +**Compatibility** +- Must not break links from any existing documentation file. If Option A introduces a new path, run a grep for `01_web_api` across all `_docs/` files and confirm none reference a path that hasn't been created. +- Must not break the existing `monorepo-document` skill workflow (which reads component descriptions). + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| (none) | This is a documentation / convention PBI; no code paths to unit-test | — | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-2 | Fresh `/new-task` invocation touching a WebApi-side change | The generated task spec's Documentation section | References the correct path; no reference to nonexistent path | — | + +## Constraints + +- The decision is binary — pick A or B, not "both" or "neither". +- If Option A is chosen, the new file's content must be substantive enough to justify its existence (≥ 30 lines), not a one-line redirect placeholder. +- If Option B is chosen, the convention note in `module-layout.md` must include the rationale, not just the rule. + +## Risks & Mitigation + +**Risk 1: Decision deferred again** +- *Risk*: This PBI ends up "let's revisit later" without picking A or B, and the F1 finding recurs in cycle 3. +- *Mitigation*: The PBI is explicitly a forced decision. Implementer or operator picks A or B during the kickoff conversation; no third "deferred" option is acceptable. + +**Risk 2: Renumbering ripples (Option A)** +- *Risk*: If Option A inserts `01_web_api` and renumbers existing folders, all existing component-doc links across `_docs/` break. +- *Mitigation*: Prefer appending (e.g., `06_web_api`) rather than renumbering. Run a grep for `components/0` references before/after to catch any broken link. + +**Risk 3: Convention drift (Option B)** +- *Risk*: Future developers see 5 `components/` folders and 1 module file and instinctively create a 6th `components/` folder for WebApi. +- *Mitigation*: The note in `module-layout.md` is canonical. Add a checklist line to `new-task` skill so the agent surfaces the convention at task-creation time. diff --git a/_docs/02_tasks/todo/AZ-496_bump_aspnetcore_8025.md b/_docs/02_tasks/todo/AZ-496_bump_aspnetcore_8025.md new file mode 100644 index 0000000..0c0b4bc --- /dev/null +++ b/_docs/02_tasks/todo/AZ-496_bump_aspnetcore_8025.md @@ -0,0 +1,119 @@ +# Bump AspNetCore.OpenApi + JwtBearer to 8.0.25 + +**Task**: AZ-496_bump_aspnetcore_8025 +**Name**: Bump AspNetCore 8.0.21 → 8.0.25 +**Description**: Bump both `Microsoft.AspNetCore.OpenApi` (cycle-1 finding D1) and `Microsoft.AspNetCore.Authentication.JwtBearer` (cycle-2 finding D3) from `8.0.21` to the latest `8.0.x` patch (`8.0.25`) in a single coordinated commit. Closes CVE-2026-26130 (SignalR DoS — not reachable in this app, but the runtime patch is the recommended hardening). +**Complexity**: 2 points +**Dependencies**: None +**Component**: WebApi (`SatelliteProvider.Api`) — dependency upgrade only +**Tracker**: AZ-496 +**Epic**: none (cycle-3 security hardening) + +## Problem + +The cycle-1 security audit (`_docs/05_security/dependency_scan.md` finding D1) flagged `Microsoft.AspNetCore.OpenApi 8.0.21` as one patch behind 8.0.25 due to CVE-2026-26130 (SignalR DoS via unbounded buffer). Disposition was "not exploitable in this app — no SignalR use", but the recommended hardening is still to bump the package. + +The cycle-2 security audit added finding D3: `Microsoft.AspNetCore.Authentication.JwtBearer 8.0.21` (added by AZ-487) shares the same 8.0.21 patch line. Pinning a second 8.0.21 package raises the cost of not doing the bump — every additional package implicitly hardcodes the runtime expectation. + +Cycle-2 security report explicitly recommends bumping **both** packages in a single coordinated commit so the project's ASP.NET Core 8 LTS footprint stays uniform. + +Pre-public-network exposure is the trigger for actually executing this hardening (per `_docs/05_security/security_report.md`). Cycle 3 is a good time because no high-risk new code is shipping concurrently, so the bump can be exercised against the full test suite as a standalone change. + +## Outcome + +- Both `Microsoft.AspNetCore.OpenApi` and `Microsoft.AspNetCore.Authentication.JwtBearer` are pinned to `8.0.25` (or the latest available 8.0.x patch at implementation time, whichever is higher). +- All unit + integration tests pass against the bumped packages. +- Container image build succeeds (Dockerfile is still pinned to a runtime image ≥ 8.0.25 so the deployed binary actually picks up the patched ASP.NET Core runtime). +- `_docs/05_security/dependency_scan.md` D1 + D3 entries are marked Resolved with a reference to this PBI's tracker ID. +- `_docs/05_security/security_report.md` D1/D3 row moves from Open to Resolved. + +## Scope + +### Included + +- Edit `SatelliteProvider.Api/SatelliteProvider.Api.csproj`: + - `` → `Version="8.0.25"` + - `` → `Version="8.0.25"` +- Edit `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj`: + - `` → `Version="8.0.25"` +- Verify the runtime image in `SatelliteProvider.Api/Dockerfile` (or wherever the runtime base image is pinned) is at ≥ 8.0.25. Bump if not. +- Run the full test suite (`./scripts/run-tests.sh --full`) — all green required for this PBI to land. +- Update `_docs/05_security/dependency_scan.md`: mark D1 and D3 Resolved with a one-line note pointing at this PBI's tracker ID. +- Update `_docs/05_security/security_report.md` summary table: D1/D3 status Open → Resolved. +- Verify version consistency: any other 8.0.x ASP.NET Core packages (if any) are bumped to the same patch line to keep `coderule.mdc`'s "consistent package versions" rule satisfied. + +### Excluded + +- Bumping to ASP.NET Core 9.x or 10.x — this is a patch-level bump only. +- Bumping unrelated packages (Dapper, Npgsql, ImageSharp) — separate decisions. +- Updating `Microsoft.NET.Test.Sdk` (finding D2) — that's a test-only Low and a separate PBI if/when it lands. Mention it in the batch report as the next dep-bump candidate. +- Any production code change. + +## Acceptance Criteria + +**AC-1: Both packages pinned to 8.0.25** +Given the post-PBI `*.csproj` files +When `grep "Microsoft.AspNetCore" SatelliteProvider.Api/SatelliteProvider.Api.csproj SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` is run +Then both `Microsoft.AspNetCore.OpenApi` AND `Microsoft.AspNetCore.Authentication.JwtBearer` resolve to `Version="8.0.25"`. + +**AC-2: Runtime image at or above 8.0.25** +Given the post-PBI `SatelliteProvider.Api/Dockerfile` +When the runtime base image is inspected +Then it pins (or transitively resolves to) ≥ 8.0.25. + +**AC-3: All tests pass against the bumped packages** +Given the bumped repository +When `./scripts/run-tests.sh --full` is run +Then all unit + integration tests pass with no new failures vs. the pre-bump baseline. + +**AC-4: Security findings updated** +Given the post-PBI repository +When `_docs/05_security/dependency_scan.md` is read +Then D1 and D3 status is Resolved with a reference to this PBI's tracker ID, AND the summary table in `_docs/05_security/security_report.md` reflects the same. + +**AC-5: Image build succeeds** +Given the bumped repository +When `docker-compose build` is run +Then the API image builds successfully without warnings about package downgrade or framework conflict. + +## Non-Functional Requirements + +**Compatibility** +- 8.0.21 → 8.0.25 is a patch-level bump within the 8 LTS line; no public API change expected. If any API drift is observed, STOP and document — this would be unusual and worth investigating. +- The bump must not change `TokenValidationParameters` semantics (cycle-2 JWT validation behavior preserved). + +**Security** +- The bump's primary security benefit (CVE-2026-26130) is in code paths NOT reachable from this app. The bump is "hygiene" rather than "active CVE closure", but D1's disposition already explained this and the cycle-2 audit reiterated. + +## Unit Tests + +| AC Ref | What to Test | Required Outcome | +|--------|-------------|-----------------| +| AC-3 | All existing unit tests | Pass unchanged | + +## Blackbox Tests + +| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References | +|--------|------------------------|-------------|-------------------|----------------| +| AC-3 | Bumped repo | `./scripts/run-tests.sh --full` | All green | Compatibility | +| AC-5 | Bumped repo | `docker-compose build` | Successful image build | — | + +## Constraints + +- This is a coordinated bump: BOTH packages move together. Do not bump only one. +- The chosen target version MUST be the same across `Api.csproj` and `Tests.csproj` for `JwtBearer` (no version-drift between projects). +- If a higher 8.0.x patch is available at implementation time (e.g., 8.0.27), use that and document the choice in the batch report. + +## Risks & Mitigation + +**Risk 1: Latent API drift between 8.0.21 and 8.0.25** +- *Risk*: A behavioral change in the JWT bearer middleware between patch versions affects existing test fixtures or production behavior. +- *Mitigation*: AC-3 (full test suite green) is the gate. The Microsoft.AspNetCore patch notes between 8.0.21 and 8.0.25 are short and well-scoped (mostly hotfixes); a behavior drift is unlikely but the test suite would catch it. + +**Risk 2: Runtime image upgrade required** +- *Risk*: The Dockerfile base image may still be at 8.0.21; bumping only NuGet packages doesn't help if the runtime is stale. +- *Mitigation*: AC-2 covers this explicitly. Bump the Dockerfile base image in the same commit if needed. + +**Risk 3: Cascade bumps** +- *Risk*: Bumping these two packages may pull in transitive dependency updates that conflict with `Microsoft.Extensions.*` (currently at 9.0.10). +- *Mitigation*: A `dotnet restore` (run by the user, not the agent — per AGENTS.md) will surface any conflict immediately. If it occurs, document the cascade and decide whether to fold the cascade in or scope-cut. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 7a0cfe7..7c6d7e4 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -2,8 +2,8 @@ ## Current Step flow: existing-code -step: 9 -name: New Task +step: 10 +name: Implement status: not_started sub_step: phase: 0