From 90d48cf3c014236a71f4edc29649f013fad0275b Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Thu, 14 May 2026 20:24:30 +0300 Subject: [PATCH] [AZ-PENDING-1] [AZ-PENDING-2] Testability fixes: JWKS gate + RMQ DNS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../reviews/batch_01_review.md | 120 ++++++++++++++++++ _docs/_autodev_state.md | 6 +- src/Auth/JwtExtensions.cs | 12 +- src/Services/FailsafeProducer.cs | 22 +++- 4 files changed, 155 insertions(+), 5 deletions(-) create mode 100644 _docs/03_implementation/reviews/batch_01_review.md diff --git a/_docs/03_implementation/reviews/batch_01_review.md b/_docs/03_implementation/reviews/batch_01_review.md new file mode 100644 index 0000000..7a24258 --- /dev/null +++ b/_docs/03_implementation/reviews/batch_01_review.md @@ -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. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 78962cf..3ac1616 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -6,9 +6,9 @@ step: 4 name: Code Testability Revision status: in_progress sub_step: - phase: 3 - name: phase-2-task-decomposition - detail: "" + phase: 4 + name: phase-4-implementation-runner + detail: "review PASS_WITH_WARNINGS; commit pending" retry_count: 0 cycle: 1 tracker: local diff --git a/src/Auth/JwtExtensions.cs b/src/Auth/JwtExtensions.cs index cb50825..101145c 100644 --- a/src/Auth/JwtExtensions.cs +++ b/src/Auth/JwtExtensions.cs @@ -27,10 +27,20 @@ public static class JwtExtensions // document; admin only exposes JWKS, so we wire a JWKS-only retriever. // The manager caches the document and refreshes on the default schedule // (matches admin's Cache-Control: public, max-age=3600 on /.well-known/jwks.json). + // + // RequireHttps is relaxed only when ASPNETCORE_ENVIRONMENT=E2ETest so the + // blackbox harness can serve its mock JWKS over the test-net HTTP issuer + // (architecture.md Open Risks Section 6). Any other environment — including + // unset, Development, Staging, Production — keeps the HTTPS enforcement. + var requireHttpsForJwks = !string.Equals( + Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT"), + "E2ETest", + StringComparison.OrdinalIgnoreCase); + var jwksConfigManager = new ConfigurationManager( jwksUrl, new JwksRetriever(), - new HttpDocumentRetriever { RequireHttps = true }); + new HttpDocumentRetriever { RequireHttps = requireHttpsForJwks }); services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) .AddJwtBearer(options => diff --git a/src/Services/FailsafeProducer.cs b/src/Services/FailsafeProducer.cs index 646eab6..1a4745a 100644 --- a/src/Services/FailsafeProducer.cs +++ b/src/Services/FailsafeProducer.cs @@ -51,9 +51,16 @@ public class FailsafeProducer( private async Task ProcessQueue(CancellationToken ct) { + // RABBITMQ_HOST is documented as accepting either a literal IP or a DNS + // hostname (container service name, k8s service, etc.). IPEndPoint takes + // an IPAddress, so non-IP values must be resolved before construction — + // a bare IPAddress.Parse here throws FormatException on every drain cycle + // for any hostname value. + var brokerAddress = await ResolveHostAddress(config.Host, ct); + var streamSystem = await StreamSystem.Create(new StreamSystemConfig { - Endpoints = [new IPEndPoint(IPAddress.Parse(config.Host), config.Port)], + Endpoints = [new IPEndPoint(brokerAddress, config.Port)], UserName = config.Username, Password = config.Password }); @@ -192,6 +199,19 @@ public class FailsafeProducer( } } + private static async Task ResolveHostAddress(string host, CancellationToken ct) + { + if (IPAddress.TryParse(host, out var literal)) + return literal; + + var addresses = await Dns.GetHostAddressesAsync(host, ct); + if (addresses.Length == 0) + throw new InvalidOperationException( + $"DNS resolution for RABBITMQ_HOST '{host}' returned no addresses."); + + return addresses[0]; + } + public static async Task EnqueueAsync(AppDataConnection db, string annotationId, QueueOperation operation) { var ids = JsonSerializer.Serialize(new[] { annotationId });