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

Code Review — Batch 05 cycle 3 (AZ-494)

Reviewer: autodev in-band Scope: AZ-494 (JWT iss/aud validation) — implementation, tests, config, docs Verdict: PASS_WITH_WARNINGS — 0 Critical, 0 High, 0 Medium, 1 Low (cross-repo doc), 1 acknowledged operational gate by user decision.

1. Spec compliance

All ACs except AC-7 are addressed. AC-7 (suite-level contract) requires writing to a file in the parent monorepo (suite/_docs/10_auth.md), which sits outside this workspace and outside autodev's blast radius. The cycle-2 deploy report has been updated to document the cross-repo obligation so it doesn't get lost.

The user's Option B selection is honoured exactly:

• appsettings.json ships with empty Jwt.Issuer / Jwt.Audience → production deploy without env vars fails at startup.
• appsettings.Development.json ships with DEV-ONLY--prefixed placeholders → local + Docker-compose flows work without touching .env.
• The env-var precedence + fail-fast pattern is identical to the existing JWT_SECRET flow from AZ-487, so there's no second mental model for operators to learn.

2. Code quality

Area	Observation	Verdict
Single source of truth	JwtTokenFactory.Create remains the only new JwtSecurityToken(...) call site in the source tree (verified by grep). The new iss/aud params slot in alongside the existing secret / subject / lifetime / extraClaims / algorithm parameters — no parallel code path was introduced.	✓
Helper layering	JwtTestHelpers.MintAuthenticated / MintExpired are thin wrappers (≤ 12 lines each) that resolve iss/aud from env and delegate to the factory. They live in SatelliteProvider.IntegrationTests because they read env vars — exactly the runner-side concern documented in module-layout.md.	✓
Fail-fast extraction	The third copy of the "resolve required value, throw with a humanised message" pattern was extracted into a single ResolveRequiredOrThrow helper. ResolveSecretOrThrow remains separate because it has the additional ≥ 32-byte size check from AZ-487. This is acceptable: two distinct invariants.	✓
Test mutual-isolation	Each new [Fact] saves and restores both JWT_ISSUER and JWT_AUDIENCE via try/finally, mirroring the existing pattern for JWT_SECRET. No parallel-test interference risk.	✓
Surface area minimisation	JwtTokenFactory.Create gained two optional, nullable parameters at the END of the signature with defaults of null. Old callers behave identically.	✓
Error message clarity	All three fail-fast messages name (a) the env var, (b) the config key, (c) the AZ-494 spec, and use the human label ("JWT issuer", "JWT audience").	✓
Logging	No new debug/trace logs added — only the existing startup-time InvalidOperationException path. This matches coderule.mdc.	✓
Naming	MintAuthenticated / MintExpired / ResolveIssuerOrThrow / ResolveAudienceOrThrow all describe caller intent precisely. No vague "data"/"item"/"candidate" names.	✓

3. Security

Aspect	Status
F-AUTH-2 (iss/aud not validated)	Resolved — ValidateIssuer = true + ValidateAudience = true, both wired to real values
Token revocation list	Still absent — listed as a new Low finding in owasp_review.md A07
Secret leakage	No iss/aud value is logged at WARN / ERROR / INFO. Only the startup banner (Console.Out) prints them — intentional, for diagnosis.
Production safety	appsettings.json empty → process won't start without explicit env vars. Cannot accidentally ship a hard-coded prod issuer.
Test/prod isolation	DEV-ONLY- prefix means a grep can surface placeholder values at any time.

4. Performance

No measurable impact. TokenValidationParameters now checks two additional string comparisons per request. Sub-microsecond per request — far below the 1 ms AZ-487 NFR budget. PT-07 / PT-08 from AZ-492 will pick up any aggregate regression at the next perf run.

5. Cross-task consistency

• AZ-487 (JWT base layer) — extended, not replaced. The RequireSignedTokens / RequireExpirationTime / ClockSkew = 30 s / secret ≥ 32 bytes invariants all remain.
• AZ-491 (consolidate JWT test helpers) — the new MintAuthenticated / MintExpired wrappers strengthen the AZ-491 contract (delegate to the canonical factory; no parallel mint logic).
• AZ-492 (perf harness) — PerfBootstrap.MintToken was updated in the same way as integration call sites. Without this update the perf harness would have started emitting 401s after AZ-494 landed.
• AZ-493 (DB reset hook) — unaffected; runs against the same env.
• AZ-495 (doc folder convention) — unaffected.
• AZ-496 (bump aspnetcore 8.0.25) — directly relevant: AZ-494's ValidateIssuer / ValidateAudience semantics are unchanged across Microsoft.AspNetCore.Authentication.JwtBearer 8.0.21 → 8.0.25.

6. Findings

Low

L1 — Cross-repo doc deferred (AC-7)

• Where: suite/_docs/10_auth.md (outside this workspace).
• Why noted: AC-7 explicitly calls for an update there; the autodev process treats this as out-of-scope per the workspace-boundary rule.
• Disposition: documented in deploy_cycle2.md's R3 follow-up section.

Acknowledged operational gate (NOT a finding)

The Option B contract is deliberately that production deploys without real iss/aud values fail at startup. That's the user-selected forcing function for admin-team confirmation. Not a code defect.

7. AC-by-AC re-check

AC	Verification	Verdict
AC-1	JwtIntegrationTests.WrongIssuer_Returns401 asserts 401 against https://wrong-issuer.invalid/	✓
AC-2	JwtIntegrationTests.WrongAudience_Returns401 asserts 401 against wrong-audience-not-satellite	✓
AC-3	JwtIntegrationTests.ValidToken_Returns200_OnHealthyEndpoint mints through MintAuthenticated (env-sourced iss/aud)	✓
AC-4	4 fail-fast unit tests + manual smoke ("docker compose up without env vars") path documented	✓
AC-5	All integration test call sites migrated to MintAuthenticated; verified via grep	✓
AC-6	security_report.md + owasp_review.md updated	✓
AC-7	Cross-repo write deferred; documented	◐ deferred

8. Verdict

PASS_WITH_WARNINGS — proceed to ship. The single Low finding is structurally outside this workspace.