mirror of
https://github.com/azaion/annotations.git
synced 2026-06-21 19:21:06 +00:00
[AZ-PENDING-1] [AZ-PENDING-2] Testability fixes: JWKS gate + RMQ DNS
C01 (JWKS HTTPS env gate, src/Auth/JwtExtensions.cs) Gate HttpDocumentRetriever.RequireHttps on ASPNETCORE_ENVIRONMENT != "E2ETest" (case-insensitive). HTTPS is still enforced for Development, Staging, Production, and any unset value. Test harness can now serve JWKS over plain HTTP via the mock issuer documented in _docs/02_document/tests/environment.md. C02 (RabbitMQ host DNS resolution, src/Services/FailsafeProducer.cs) Resolve RABBITMQ_HOST via DNS when the value is not a literal IP. Adds ResolveHostAddress(host, ct) helper that uses IPAddress.TryParse first, then Dns.GetHostAddressesAsync. Fixes a latent production bug (operators using a DNS hostname like "rabbitmq" or "broker.internal" got a FormatException at startup) and unblocks the e2e Docker test harness where the broker is reachable only via service-name DNS. Review report: _docs/03_implementation/reviews/batch_01_review.md Verdict PASS_WITH_WARNINGS (1 Low/Maintainability finding, documented as deferred to Step 8 hardening). Tracker IDs are placeholders — Jira MCP unavailable. Real IDs to be assigned per _docs/_process_leftovers/2026-05-14_testability-tracker.md. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,120 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 01
|
||||
**Tasks**: `PENDING_refactor_jwks_https_env_gate` (C01), `PENDING_refactor_rabbitmq_host_dns_resolution` (C02)
|
||||
**Date**: 2026-05-14
|
||||
**Mode**: full (7 phases) on the batch's changed files
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Changed files
|
||||
|
||||
| File | Diff size | Owner component |
|
||||
|------|-----------|----------------|
|
||||
| `src/Auth/JwtExtensions.cs` | +11 / -1 | `06_platform` → Auth |
|
||||
| `src/Services/FailsafeProducer.cs` | +21 / -1 | `02_annotations-realtime-sync` |
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| F1 | Low | Maintainability | `src/Auth/JwtExtensions.cs:30-37` | Env-name reading scattered across Auth and Program |
|
||||
|
||||
## Finding Details
|
||||
|
||||
**F1: Env-name reading scattered across Auth and Program** (Low / Maintainability)
|
||||
- Location: `src/Auth/JwtExtensions.cs:30-37`
|
||||
- Description: `JwtExtensions.AddJwtAuth` now reads `Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")` inline. `Program.cs:53` already reads the same value via `builder.Environment.EnvironmentName` and passes it to `CorsConfigurationValidator.EnsureSafeForEnvironment`. Two slightly different access patterns for the same input value.
|
||||
- Suggestion: a future Step 8 hardening item can centralise this — e.g., add an `IHostEnvironment environment` parameter to `AddJwtAuth` and have `Program.cs` pass `builder.Environment` so both call sites use the same idiom. Deferred deliberately: the inline read is the smallest possible diff for the testability scope (no caller change at `Program.cs:49`), which is the explicit Step 4 envelope ("smallest beats elegant here").
|
||||
- Task: `PENDING_refactor_jwks_https_env_gate`
|
||||
- Blocks verdict? No (Low severity; documented under "Deferred to Step 8 Refactor" in `_docs/04_refactoring/01-testability-refactoring/list-of-changes.md`).
|
||||
|
||||
## Phase-by-Phase Notes
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
Read task specs `_docs/02_tasks/todo/01_refactor_jwks_https_env_gate.md` and `_docs/02_tasks/todo/02_refactor_rabbitmq_host_dns_resolution.md`. Read `_docs/00_problem/restrictions.md` (SW-03 RabbitMQ streams plugin, SW-05 ES256 verifier-only, ENV-01 env vars required, ENV-02 HTTP no in-image TLS) and `_docs/01_solution/solution.md`. Both task specs map their ACs to existing documented blackbox scenarios in `_docs/02_document/tests/`.
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
|
||||
**Task 01 — `jwks_https_env_gate`**
|
||||
|
||||
| AC | Behavior | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (HTTPS enforced outside test env) | `requireHttpsForJwks` is `true` when env is anything other than `E2ETest` (including unset, Development, Staging, Production) | ✓ Satisfied |
|
||||
| AC-2 (HTTPS relaxed under E2ETest only) | Case-insensitive equality with `"E2ETest"` → `false` | ✓ Satisfied |
|
||||
| AC-3 (Validation semantics unchanged) | `TokenValidationParameters` block, `IssuerSigningKeyResolver`, `JwksRetriever`, and policy registration are byte-for-byte identical | ✓ Satisfied |
|
||||
| AC-4 (Forgery / tamper / cross-policy rejected) | Algorithm pinning (`EcdsaSha256`), signature, lifetime, audience, issuer checks all preserved | ✓ Satisfied |
|
||||
|
||||
**Task 02 — `rabbitmq_host_dns_resolution`**
|
||||
|
||||
| AC | Behavior | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (Literal IP still works) | `IPAddress.TryParse(host, out var literal)` returns the literal, no DNS lookup | ✓ Satisfied |
|
||||
| AC-2 (DNS hostname now works) | Falls through to `Dns.GetHostAddressesAsync(host, ct)` and uses `addresses[0]` | ✓ Satisfied |
|
||||
| AC-3 (Unresolvable hostname surfaces through retry envelope) | `Dns.GetHostAddressesAsync` throws `SocketException` (or `InvalidOperationException` on zero-result) for unresolvable hosts; `ExecuteAsync`'s `catch (Exception ex)` at line 44 catches both, logs `ex.Message`, backs off 10 s — same surface behavior as a `FormatException` today | ✓ Satisfied |
|
||||
| AC-4 (Cancellation honored during resolution) | `ct` is forwarded to `Dns.GetHostAddressesAsync` | ✓ Satisfied |
|
||||
| AC-5 (Wire format / consumers unaffected) | `DrainQueue` (MessagePack serialize, gzip, queue-table delete) is untouched | ✓ Satisfied |
|
||||
|
||||
No scope creep detected — diffs are bounded to the documented OWNED files.
|
||||
|
||||
No contract sections in either task spec; consumer-side contract verification N/A.
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
- **SOLID**: each change has a single responsibility. `ResolveHostAddress` is a private static helper with one clear job.
|
||||
- **Error handling**: `ResolveHostAddress` throws `InvalidOperationException` if DNS returns zero addresses — explicit and meaningful. C01 reads an env var (cannot throw).
|
||||
- **Naming**: `requireHttpsForJwks`, `ResolveHostAddress`, `brokerAddress` — clear intent.
|
||||
- **Complexity**: `ResolveHostAddress` is 6 lines; cyclomatic 2. C01 is two lines of straight-line code. Both well under the 50-line / cyclo-10 thresholds.
|
||||
- **DRY**: no duplication introduced. Note F1 records a *cross-file* duplication pattern (env-name access) that pre-existed in spirit.
|
||||
- **Test quality**: N/A — no executable tests in this run (testability refactor produces the *prerequisites* for tests, which are implemented in Step 6).
|
||||
- **Dead code**: none.
|
||||
|
||||
### Phase 4 — Security Quick-Scan
|
||||
|
||||
- No SQL interpolation introduced.
|
||||
- No command-injection vector (no shell, no subprocess).
|
||||
- No hardcoded secrets / keys / passwords.
|
||||
- Input validation: `Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")` is operator-controlled; the only branch on it is "is it `E2ETest`?" — no value-dependent decoding/parsing.
|
||||
- `Dns.GetHostAddressesAsync(host, ct)` accepts operator-controlled input; no exfiltration / SSRF surface beyond what `RABBITMQ_HOST` already had (the producer already connects to whatever broker the operator configured).
|
||||
- Sensitive data in logs: `ResolveHostAddress`'s zero-address throw includes the host string in the exception message — this is the same host the operator already sees in their config; not a leak.
|
||||
- No insecure deserialization.
|
||||
- **AC-4 cross-check**: NFT-SEC-01 through NFT-SEC-10 scenarios (forgery, expired, wrong-iss, wrong-aud, alg-confusion, tamper) all remain blocked by the unchanged `TokenValidationParameters` and algorithm pinning. Verified by inspection of `JwtExtensions.cs` lines 38-65 (unchanged in the diff).
|
||||
|
||||
### Phase 5 — Performance Scan
|
||||
|
||||
- C01: one-time env-var read at startup. Trivial.
|
||||
- C02: one DNS lookup per drain cycle (~10 s cadence). One round-trip; cached by the OS resolver in practice. Negligible compared to the broker connect that follows.
|
||||
- No N+1 patterns. No unbounded fetches. No blocking I/O in async (we use the async DNS API throughout).
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
- Two tasks, disjoint files (`JwtExtensions.cs` vs `FailsafeProducer.cs`). No shared interface.
|
||||
- No conflicting patterns introduced.
|
||||
- No shared code duplicated across the two task implementations.
|
||||
- Both changes follow the project's existing patterns (private static helpers; case-insensitive env-name compare aligned with `CorsConfigurationValidator`).
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
- **Layer direction**: both files stay within their owned components. `src/Auth/` belongs to `06_platform`; `src/Services/FailsafeProducer.cs` to `02_annotations-realtime-sync` (per `_docs/02_document/module-layout.md`). No new imports across component boundaries.
|
||||
- **Public API respect**: no new cross-component imports. C01 adds `Environment.GetEnvironmentVariable` (BCL). C02 adds `Dns.GetHostAddressesAsync` (BCL, already in `System.Net` which was already imported at file top).
|
||||
- **No new cyclic dependencies**: import graph unchanged.
|
||||
- **Duplicate symbols across components**: `ResolveHostAddress` is a private static — not visible outside its containing class. No collision.
|
||||
- **Cross-cutting concerns not locally re-implemented**: F1 (env-name reading scattered across Auth and Program) is the only finding in this category — Low severity, deferred to Step 8.
|
||||
|
||||
## Baseline Delta
|
||||
|
||||
`_docs/02_document/architecture_compliance_baseline.md` baseline = `0 Critical, 0 High, 1 Medium (F1 DatasetService writes annotation table — RB-08), 2 Low (F2 ClassesController bypasses service — RB-06; F3 FailsafeProducer.EnqueueAsync static — accepted)`.
|
||||
|
||||
- **Carried over**: F1 (RB-08) — unchanged by this batch (DatasetService not touched). F2 (RB-06) — unchanged (ClassesController not touched). F3 (accepted) — unchanged (`EnqueueAsync` not touched).
|
||||
- **Resolved**: none. (This batch addresses *testability* fixes, not the structural items the baseline flagged.)
|
||||
- **Newly introduced**: F1 of this report (Low / Maintainability — env-name reading scattered). One finding. Already documented as deferred to Step 8.
|
||||
|
||||
No new Critical / High Architecture findings.
|
||||
|
||||
## Auto-Fix Decision
|
||||
|
||||
Per implement skill Step 10 auto-fix matrix: this batch has only one Low/Maintainability finding, which is **eligible for auto-fix** but already enumerated as a deferred Step 8 item by the user-approved scope. No auto-fix attempted; the finding is explicitly out of the testability envelope per `list-of-changes.md`.
|
||||
|
||||
## Verdict
|
||||
|
||||
**PASS_WITH_WARNINGS** — one Low/Maintainability finding (documented deferral). No Critical, no High. Implement skill proceeds to commit.
|
||||
Reference in New Issue
Block a user