# Code Review Report — Batch 01 cycle 2

**Batch**: AZ-487 (JWT validation baseline)
**Date**: 2026-05-11
**Verdict**: PASS_WITH_WARNINGS

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Style | _docs/02_document/components/01_web_api/description.md | Task spec referenced a doc path that does not exist in the codebase |
| 2 | Low | Security | SatelliteProvider.Api/appsettings.Development.json | Dev-only JWT secret placeholder is committed (intentional per spec) |

### Finding Details

**F1: Task spec referenced a doc path that does not exist in the codebase** (Low / Style) — **RESOLVED in cycle 3 (AZ-495)**
- Location: `_docs/02_document/components/01_web_api/description.md` (referenced; does not exist)
- Description: The AZ-487 task spec lists `_docs/02_document/components/01_web_api/description.md` as a doc to update. The codebase's component-doc folders are `01_common`, `02_data_access`, `03_tile_downloader`, `04_region_processing`, `05_route_management` — there is no `01_web_api` folder. The WebApi component's documentation lives in `_docs/02_document/modules/api_program.md`.
- Suggestion: Either (a) create the missing folder with a brief stub that defers to `api_program.md`, or (b) update the task spec for AZ-488 to point at `modules/api_program.md` and acknowledge that WebApi has no `components/*` folder. This batch chose (b) — updated `architecture.md` § Architecture Vision + § Security Architecture and `modules/api_program.md`. Surface to user for a doc-organization decision.
- Task: AZ-487
- **Resolution (AZ-495, cycle 3)**: Option B formalized as canonical convention. `_docs/02_document/module-layout.md` § Documentation Layout now explicitly states WebApi has no `components/*` folder; documentation anchor is `modules/api_program.md`. The `.cursor/skills/new-task/SKILL.md` Step 4 (Codebase Analysis) directs future agents at the correct path. Finding will not recur.

**F2: Dev-only JWT secret placeholder is committed** (Low / Security)
- Location: `SatelliteProvider.Api/appsettings.Development.json`
- Description: The dev placeholder `DEV-ONLY-DO-NOT-USE-IN-PROD-replace-with-real-secret-via-JWT_SECRET-env-var` (78 bytes) is committed to the repo. Anyone with read access could mint a token signed with that secret. The mitigations (per task spec) are: (i) only loaded when `ASPNETCORE_ENVIRONMENT=Development`, (ii) the `JWT_SECRET` env var overrides it whenever set, (iii) the placeholder text itself signals it must be replaced.
- Suggestion: Accept as-is for dev ergonomics; production environments must set `JWT_SECRET` (validated in `scripts/run-tests.sh` and at startup). Document on the README that this dev placeholder is intentional.
- Task: AZ-487

## Phase Notes

### Phase 1 — Context Loading
- Task spec read: `_docs/02_tasks/todo/AZ-487_jwt_validation_baseline.md`
- Suite contract referenced: `suite/_docs/10_auth.md` (consumed via implementation, not modified)
- Module layout, architecture doc, existing test patterns reviewed.

### Phase 2 — Spec Compliance
All 8 acceptance criteria have at least one automated test:

| AC | Description | Test |
|----|-------------|------|
| AC-1 | Anonymous → 401 | `JwtIntegrationTests.AnonymousRequest_To_AnyEndpoint_Returns401` |
| AC-2 | Expired → 401 | `JwtIntegrationTests.ExpiredToken_Returns401` |
| AC-3 | Invalid signature → 401 | `JwtIntegrationTests.InvalidSignature_Returns401` |
| AC-4 | Valid token reaches handler | `JwtIntegrationTests.ValidToken_Returns200_OnHealthyEndpoint` |
| AC-5 | Missing/short secret → fail fast | `AuthenticationServiceCollectionExtensionsTests.AddSatelliteJwt_ThrowsOnMissingSecret/Empty/Short` |
| AC-6 | `HttpContext.User` exposes claims | `JwtTokenFactoryTests.Create_WithExtraClaims_PropagatesClaimsThroughValidation` (exercises same `JwtSecurityTokenHandler.ValidateToken` path JwtBearer uses to populate `HttpContext.User`) |
| AC-7 | Swagger Authorize button | `JwtIntegrationTests.SwaggerDocument_AdvertisesBearerSecurityScheme` (OpenAPI document declares `Bearer` security scheme) |
| AC-8 | All existing tests pass | Verified at Step 11 (test-run gate); shared `HttpClient` attaches default Bearer token in `IntegrationTests/Program.cs` |

Contract verification: AZ-487 is a consumer of `suite/_docs/10_auth.md`. The implementation matches the consumed contract (HS256, ≥32-byte HMAC key, no issuer/audience validation, expiration required).

### Phase 3 — Code Quality
- New code follows SRP (`AuthenticationServiceCollectionExtensions` does one thing).
- Tests follow AAA pattern with explicit comments (matches project style).
- No bare catches introduced. The pre-existing empty catch in `IntegrationTests/Program.cs::WaitForApiReady` is out of scope (pre-existing, not modified).
- No comments narrating obvious code.

### Phase 4 — Security Quick-Scan
- No SQL/command-injection vectors introduced.
- No sensitive data in logs (error messages mention secret length, not value).
- Dev placeholder secret: see F2.

### Phase 5 — Performance Scan
- JWT validation per request is microsecond-scale HMAC + claims parsing; no I/O, no caching needed (per NFR).
- No N+1, no unbounded fetches, no new blocking calls.

### Phase 6 — Cross-Task Consistency
Single-task batch; not applicable.

### Phase 7 — Architecture Compliance
- All new code in `SatelliteProvider.Api/Authentication/` — owned by the WebApi component per `module-layout.md`.
- Imports stay within the allowed dependency table (`Microsoft.AspNetCore.Authentication.JwtBearer`, `Microsoft.IdentityModel.Tokens` are external NuGet packages, not other components).
- Test code lives under `SatelliteProvider.Tests/` and `SatelliteProvider.IntegrationTests/` (separate test projects per layout rule 5).
- No new cross-sibling ProjectReferences. No new cyclic dependencies. No duplicate cross-component symbols.

## Baseline Delta

`_docs/02_document/architecture_compliance_baseline.md` is not present in the repo; baseline delta section omitted.

## Verdict Rationale

Two Low findings, both already accepted in the task spec or surfacable as doc-organization decisions. No Critical, High, or Medium findings. Verdict: **PASS_WITH_WARNINGS** — proceed to commit per implement skill Step 10.