# 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.