satellite-provider/_docs/03_implementation/reviews/batch_01_cycle2_review.md

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)

• 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

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.