diff --git a/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md b/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md index 68aff9f..843d899 100644 --- a/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md +++ b/aidlc-docs/features/gitea-deployment-workflow/aidlc-state.md @@ -5,7 +5,7 @@ - **Feature Slug**: gitea-deployment-workflow - **Project Type**: Brownfield - **Start Date**: 2026-07-27T00:00:00Z -- **Current Stage**: CONSTRUCTION - Code Generation, Round 1 (U1 + U2 complete, awaiting approval) +- **Current Stage**: CONSTRUCTION - Round 2, NFR Design complete for U3 + U4 (awaiting approval) - **Branch**: feature/gitea-deployment-workflow ## Workspace State @@ -65,9 +65,9 @@ U1 Hosting & Serving · U2 Data Durability · U3 Security Headers & CSP · U4 Ob Execution rounds (Q4 = B): **R1** = U1 + U2 · **R2** = U3 + U4 · **R3** = U5 + U6 · **R4** = U7. One commit per unit; single PR at the end (Q6 = A). -- [~] Functional Design — **EXECUTE for U1, U2, U3, U4**; SKIP for U5, U6, U7. **U1 ✅ U2 ✅** approved 2026-07-27 +- [x] Functional Design — **EXECUTE for U1, U2, U3, U4**; SKIP for U5, U6, U7. **U1 ✅ U2 ✅** approved 2026-07-27 · **U3 ✅ U4 ✅** 2026-07-28 - [ ] NFR Requirements — **SKIP (all units)** — already comprehensively captured in `requirements.md` § 5 and § 6 -- [ ] NFR Design — **EXECUTE for U3, U4**; SKIP for the rest. *Deliberate deviation from the default NFR-Requirements/NFR-Design coupling — rationale in the execution plan.* +- [x] NFR Design — **EXECUTE for U3, U4**; SKIP for the rest. *Deliberate deviation from the default NFR-Requirements/NFR-Design coupling — rationale in the execution plan.* **U3 ✅ U4 ✅** 2026-07-28 — 11 patterns for U3, 10 for U4. Closed OPEN-01; raised REF-U3-01 - [ ] Infrastructure Design — **EXECUTE for U6, U7**; SKIP for the rest - [~] Code Generation — **EXECUTE** (all 7 units, each built and tested before its completion message). **U1 ✅ U2 ✅** generated and verified 2026-07-27 — build 0 errors, 253 backend tests pass (was 219) - [ ] Build and Test — **EXECUTE** @@ -84,9 +84,19 @@ Execution rounds (Q4 = B): **R1** = U1 + U2 · **R2** = U3 + U4 · **R3** = U5 + ## Current Status - **Lifecycle Phase**: CONSTRUCTION -- **Current Stage**: Code Generation complete for Round 1 (U1 Hosting & Serving, U2 Data Durability) -- **Next Stage**: Round 2 — U3 Security Headers & CSP + U4 Observability, starting with Functional Design -- **Status**: Awaiting Round 1 code approval +- **Current Stage**: Round 2 — NFR Design complete for U3 Security Headers & CSP and U4 Observability +- **Next Stage**: Code Generation for U3 + U4 +- **Status**: Awaiting NFR Design approval. Round 1 (U1 + U2) code approved and committed 2026-07-28 + +## Round 2 Design Record (2026-07-28) +- Functional Design U3 + U4 complete and committed (`357d395`) +- NFR Design U3 — `construction/u3-security-headers/nfr-design/` — 11 patterns. All new types in `Core/Hosting/Security/`, so `Core.Tests` can reach them (avoids repeating U1's Step 11 deviation) +- NFR Design U4 — `construction/u4-observability/nfr-design/` — 10 patterns. `Sentry.AspNetCore` 6.8.0 into `Core`; `@sentry/react` ^10.68.0 into `frontend` +- **OPEN-01 CLOSED**: correlation ID = W3C trace ID from the ambient `Activity`, `TraceIdentifier` as fallback. Rationale: propagates master→slave via `traceparent`, and equals the `traceId` ASP.NET Core's `ProblemDetails` already returns +- **REF-U3-01 raised**: BR-U3-22's Umami-origin startup warning is **not implementable** — the backend cannot read `VITE_UMAMI_WEBSITE_ID`. Withdrawn from U3 and replaced by a **blocking U5 CI gate** comparing the frontend build variable against that environment's `SecurityHeaders:AllowedScriptOrigins` +- **Additions beyond the functional design**, each with rationale in the pattern docs: `Set-Cookie` added to the scrub list; a `sentry-tunnel` rate limiter; `OnRejected` on the existing rate limiter (today a `429` leaves no trace anywhere); `SentrySdk.FlushAsync` before the migration-failure rethrow (otherwise the one `Critical` event dies with the process) +- **Three defaults chosen rather than escalated** (each one line to change, listed at the end of U4's pattern doc): JSON console outside Development, `TracesSampleRate` 0.1, tunnel cap 200 KB +- U4 modifies two files from already-committed units — `DatabaseMigrationExtensions` (U2) and `AdminTokenValidator` (U1). Both additive; to be named in the U4 commit message ## Round 1 Verification Record (2026-07-27) - `dotnet build SlpModularCms.sln -c Release` — 0 errors diff --git a/aidlc-docs/features/gitea-deployment-workflow/audit.md b/aidlc-docs/features/gitea-deployment-workflow/audit.md index 6693801..e41773d 100644 --- a/aidlc-docs/features/gitea-deployment-workflow/audit.md +++ b/aidlc-docs/features/gitea-deployment-workflow/audit.md @@ -800,3 +800,80 @@ Note this is a **blocking** consideration under the enabled Security Baseline ex **No blocking security findings. No new deviation** — FU2 = C removed the need for the DEV-06 that Q2 = B would have required. --- + +## 2026-07-28 — CONSTRUCTION: NFR Design (Round 2, U3 + U4) + +**Stage**: NFR Design — executed for U3 and U4 only, per the execution plan. NFR Requirements skipped for all units (already captured in `requirements.md` § 5 and § 6). + +No question round. The two decisions the earlier stages had left open were technical rather than preferential, and three remaining values had defensible defaults; all five are recorded below with the reasoning, and each is one line to change. + +### OPEN-01 CLOSED — the correlation ID is the W3C trace ID + +Decided: the trace ID from the ambient `Activity`, with `HttpContext.TraceIdentifier` as the fallback when no `Activity` exists. Enabled by `ActivityTrackingOptions` plus `IncludeScopes`, so every entry from every category — framework included — carries it with **no change to any call site**. + +Two reasons, the first decisive: + +1. **It crosses the master/slave HTTP boundary.** `HttpClient` injects `traceparent`; the slave's hosting layer adopts it. The hardest diagnostic question in this codebase — "the master says the slave rejected its API key; what did the slave see?" — is answerable with one identifier. `TraceIdentifier` is host-local and cannot answer it at all. +2. **It is already the value the client is shown.** ASP.NET Core's `ProblemDetails` writes `traceId` as `Activity.Current?.Id ?? HttpContext.TraceIdentifier`. Choosing the W3C trace ID makes the log entry, the Sentry event, the slave's log entry and the browser's error response carry one value. Choosing `TraceIdentifier` would create two competing identifiers for the same request. + +Trap recorded: `ActivityTrackingOptions` populates the scope, `IncludeScopes` renders it. Set the first and forget the second and every log line looks entirely normal with no correlation ID and no error anywhere. A test asserts the field is present in rendered output. + +### REF-U3-01 — BR-U3-22 is not implementable and has been replaced + +BR-U3-22 asked the backend to warn at startup when a Umami website ID is configured but its script origin is missing from the CSP. **The backend cannot see that ID**: it is a `VITE_` variable baked into the frontend bundle at build time. The check would either always warn or never warn. + +Withdrawn from U3 and moved to the **U5 CI workflow**, where both values are visible: if `VITE_UMAMI_SCRIPT_URL` is set for an environment's frontend build, its origin must appear in that environment's `SecurityHeaders:AllowedScriptOrigins`, or the **pipeline fails**. Stronger than the rule it replaces — drift now blocks the deployment instead of writing a warning into a log on a Raspberry Pi. + +Carried to U5 as a required gate. + +### Four risks closed that the functional design did not name + +1. **`Set-Cookie` added to the scrub list.** The functional design named the request `Cookie` header. Response headers can be attached to an event, and `Set-Cookie` on the login and refresh responses carries the `refreshToken` being issued. Scrubbing the request cookie while sending the response cookie would protect nothing. +2. **`SetBeforeSendTransaction` as well as `SetBeforeSend`.** Performance transactions carry request data too. Scrubbing only events leaves a second channel open — less obvious precisely because nobody thinks of a transaction as containing headers. Enabling tracing later without touching that file would start leaking. +3. **`OnRejected` on the rate limiter.** `AddCmsRateLimiting` sets only `RejectionStatusCode`. Today a brute-force attempt against `/api/v1/Auth/login` returns `429` and leaves **no trace anywhere** — the one rate limiter this application has is unobservable. The `RateLimitTriggered` security event needs this callback to exist. +4. **`SentrySdk.FlushAsync` before the migration-failure rethrow.** The SDK sends in the background; a process that throws during startup and exits kills the sender first. The migration failure is the one `Critical` event in the system, and it was the event most likely never to arrive. Bounded at five seconds: a host that cannot reach its database is already down. + +### Two "looks correct, does nothing" traps recorded with tests attached + +- **Sentry groups log-derived events by message template.** Emitted with interpolation, every distinct email produces a separate Sentry issue, and an alert rule of the form "more than 20 failed logins in 5 minutes" can never fire — no single issue ever reaches 20. FR-19 would be unimplementable while appearing to work. Resolved with source-generated `LoggerMessage` (fixed templates, `EventId` 5001–5006) plus an `ISentryEventProcessor` that maps the ID to a `security_event` tag, so alert rules filter on a tag rather than message text. +- **`DefaultHttpContext.Response.OnStarting` is a no-op.** A unit test that drives the security-headers middleware through a `DefaultHttpContext` finds no headers — and a test asserting *nothing was set* passes for the wrong reason and keeps passing after the middleware is deleted. Resolved by putting all decision logic in a static, dependency-free `SecurityHeaderWriter` tested against a bare `HeaderDictionary`, leaving the middleware as glue whose only risk is ordering, which is verified at Build and Test. + +### Three defaults chosen rather than escalated + +| Decision | Chosen | Reasoning | +|---|---|---| +| Console format | JSON outside Development, human-readable locally | Supervised process on the Pi; `TraceId` becomes a queryable field. Both set `IncludeScopes`, so the information is identical either way | +| `TracesSampleRate` | `0.1` on both sides | Sentry's free plan counts transactions against the same quota as errors, and this design's value is in errors. The reference project's `1.0` is for a low-traffic marketing site with no API | +| Tunnel max payload | 200 KB | Envelopes with a stack trace and breadcrumbs run tens of KB | + +Each is one line; all three are listed at the end of U4's pattern document for review. + +### Packages added + +| Project | Package | Version | +|---|---|---| +| `SlpModularCms.Core` | `Sentry.AspNetCore` | `6.8.0` — latest on nuget.org; `net10.0` asset to be confirmed at Code Generation | +| `frontend` | `@sentry/react` | `^10.68.0` — same major as the reference project | + +`Sentry.AspNetCore` goes into `Core` rather than the hosts, because the scrubber, the processor, `UseCmsSentry` and the tunnel all live there and both hosts consume them. **FR-22 interaction**: this adds a new dependency subtree while U5 is due to pin two packages against advisories — the new tree must be checked in the same pass, not assumed clean because it is new. + +### Three divergences from the reference project, all deliberate + +- **The dev tunnel proxy targets the local API, not Sentry's ingest host.** The reference proxies straight to Sentry, which hard-codes the project ID in `vite.config.ts` and carries a `LET OP` comment about keeping it in sync by hand. Targeting the local API removes both the synchronisation and the committed project ID. +- **`UmamiAnalytics` drops the script-removing cleanup.** Under React 18 `StrictMode` the double-invocation becomes inject → remove → inject, and removing the element does not unregister Umami's listeners, so the first page view can be counted twice. The component lives for the application's lifetime and has nothing to clean up; the existing duplicate guard handles re-invocation. +- **`tanstackRouterBrowserTracingIntegration` not adopted.** It needs the router instance, and importing `@/router` from `sentry.ts` — called before anything else in `main.tsx` — inverts the initialisation order and pulls the whole route tree into startup. Can be added later inside `main.tsx`, where the router is already imported. + +### Artifacts generated +- `construction/u3-security-headers/nfr-design/`: `nfr-design-patterns.md` (11 patterns), `logical-components.md` +- `construction/u4-observability/nfr-design/`: `nfr-design-patterns.md` (10 patterns), `logical-components.md` +- `inception/requirements/requirements.md`: OPEN-01 struck through and closed; open-item count 3 → 2 + +### Security Compliance (Security Baseline extension — enabled, blocking) +- **SECURITY-03 — compliant, and now mechanised.** The correlation ID arrives by configuration rather than by discipline. EF's `Database.Command` category pinned at `Warning`, because at `Information` it prints parameter values and the login path passes a normalised email through it. +- **SECURITY-11 — compliant.** The tunnel destination is parsed once at startup from the DSN and no part of it can come from the request, which is the single rule separating a tunnel from a server-side request forgery primitive. A rate limiter was added because the other controls bound what each call can do but not how many calls there can be. +- **SECURITY-14 — addressed; DEV-01 unchanged.** Six tagged event types; alert rules configured in Operations. +- **SECURITY-15 — compliant.** Both units fail closed at startup (`ValidateOnStart`, unparseable DSN) and neither throws into the response path. + +**No blocking security findings. No new deviation.** + +--- diff --git a/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/logical-components.md b/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/logical-components.md new file mode 100644 index 0000000..91d29c5 --- /dev/null +++ b/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/logical-components.md @@ -0,0 +1,146 @@ +# Logical Components — U3 HTTP Security Headers & CSP + +All new types live in `SlpModularCms.Core/Hosting/Security/`, alongside the existing `AdminTokenValidator` from U1. Nothing is added to `SlpModularCms.Api`, so everything here is reachable by `SlpModularCms.Core.Tests` — deliberately, after U1's Step 11 deviation showed what happens when host-only code needs testing. + +--- + +## Component Wiring + +```mermaid +graph TD + subgraph Configuration["Configuration"] + Section["SecurityHeaders section
appsettings per environment"] + Options["SecurityHeadersOptions"] + Validator["IValidateOptions
ValidateOnStart"] + end + + subgraph Policy["Policy Composition — startup only"] + Catalog["CspPolicyCatalog
static, in code"] + Provider["ICspPolicyProvider
CspPolicyProvider"] + Frozen["FrozenDictionary<string, SecurityHeaderSet>"] + end + + subgraph Request["Request Path — per response"] + Middleware["SecurityHeadersMiddleware"] + Resolver["PathPolicyResolver"] + State["HeaderWriteState
struct-like state object"] + Writer["SecurityHeaderWriter
static, pure"] + end + + subgraph Host["Host Wiring"] + AddExt["AddCmsSecurityHeaders"] + UseExt["UseCmsSecurityHeaders"] + Logger["ILogger — startup origins,
disabled warning"] + end + + Response["HTTP response headers"] + + Section --> Options + Options --> Validator + Options --> Provider + Options --> Resolver + Catalog --> Provider + Provider --> Frozen + AddExt --> Options + AddExt --> Provider + AddExt --> Resolver + UseExt --> Middleware + UseExt --> Logger + Middleware --> Resolver + Middleware --> Provider + Middleware --> State + State --> Writer + Writer --> Response + + classDef cfg fill:#fbd38d,stroke:#c05621,stroke-width:1px,color:#000; + classDef code fill:#9ae6b4,stroke:#2f855a,stroke-width:1px,color:#000; + classDef runtime fill:#90cdf4,stroke:#2b6cb0,stroke-width:1px,color:#000; + classDef host fill:#d6bcfa,stroke:#6b46c1,stroke-width:1px,color:#000; + classDef out fill:#e2e8f0,stroke:#4a5568,stroke-width:1px,color:#000; + class Section,Options,Validator cfg; + class Catalog,Provider,Frozen,Writer code; + class Middleware,Resolver,State runtime; + class AddExt,UseExt,Logger host; + class Response out; +``` + +Text alternative: configuration binds to validated options that feed both the policy provider and the path resolver at startup; the provider composes the in-code catalog into a frozen dictionary of header sets; per response the middleware resolves a policy name, registers a response-start callback carrying a state object, and a static writer applies the headers. + +--- + +## Component Responsibilities + +| Component | Lifetime | Responsibility | Pattern | +|---|---|---|---| +| `SecurityHeadersOptions` | Options singleton | Binds `Enabled`, `DefaultPolicy`, `PathPolicies`, `AllowedScriptOrigins`, `AllowedConnectOrigins` | 1, 11 | +| `PathPolicyRule` | Record | One `PathPrefix` → `Policy` pair | 3 | +| `CspPolicyCatalog` | Static | The two policy definitions. Pure function of origin lists → directive list | 2 | +| `SecurityHeaderSet` | Record | The three HTML-only header values for one policy | 2 | +| `ICspPolicyProvider` / `CspPolicyProvider` | Singleton | Composes and caches header sets once; one lookup per response | 2 | +| `PathPolicyResolver` | Singleton | Ordered, segment-aware prefix match → policy name | 3 | +| `SecurityHeadersMiddleware` | Per-request instance, singleton delegate | Gate on `Enabled`, resolve the policy, register the response-start callback | 4 | +| `SecurityHeaderWriter` | Static | Per-header scoping, HSTS gating, never-overwrite. **All decision logic** | 5, 6 | +| `SecurityHeadersExtensions` | Static | `AddCmsSecurityHeaders` / `UseCmsSecurityHeaders`, startup logging | 9, 10 | + +**Why `SecurityHeaderWriter` is static and separate from the middleware**: it is the only component whose behaviour is worth exhaustive testing, and `DefaultHttpContext` cannot drive it through the middleware (Pattern 5). Splitting it makes the interesting part testable with `new HeaderDictionary()` and leaves the middleware as glue with nothing to get wrong except ordering — which is verified at Build and Test. + +**Why `PathPolicyResolver` is a class rather than a method on the provider**: it answers a different question (which policy) from the provider (what the policy contains), and the `/administrator` versus `/admin` trap deserves its own test class rather than being buried in provider tests. + +--- + +## DI Registration Order + +Inside `AddCmsSecurityHeaders(configuration)`: + +``` +1. services.AddOptions() + .Bind(configuration.GetSection("SecurityHeaders")) + .Validate(...) // unknown policy name, origin format + .ValidateOnStart() +2. services.AddSingleton() +3. services.AddSingleton() +``` + +Called from `Program.cs` next to the other `AddCms*` calls, before module registration. Order within the file does not matter — nothing here is overridable by a module, unlike Data Protection in U2. + +`UseCmsSecurityHeaders()` is called **first inside `UseExceptionHandler()`**; see Pattern 10. That position *does* matter, in both directions. + +--- + +## Both Hosts + +`SlpModularCms.Api` and `SlpModularCms.Api.Slave` both call `AddCmsSecurityHeaders` and `UseCmsSecurityHeaders`. The slave host serves no admin SPA and no public website today, but it does serve `/api/v1` and `/health`, and it will be reached directly during diagnosis. There is no reason for it to be the one host without `nosniff` and HSTS. + +The slave's `PathPolicies` defaults are identical; the paths that do not exist there simply never match. + +--- + +## NFR Coverage Traceability + +| NFR / Rule | Pattern | Component | +|---|---|---| +| NFR-01 — no server configuration | Headers emitted in-process | `SecurityHeadersMiddleware`, `SecurityHeaderWriter` | +| NFR-03 — startup cost | Compose twice at startup, never per request | `CspPolicyProvider` + `FrozenDictionary` | +| NFR-06 — testability | Decision logic in pure static functions | `SecurityHeaderWriter`, `CspPolicyCatalog`, `PathPolicyResolver` | +| SECURITY-04 | All five headers; CSP on every HTML path | `SecurityHeaderWriter`, `CspPolicyCatalog` | +| SECURITY-11 | CSP as a second layer behind output escaping | `CspPolicyCatalog` | +| SECURITY-15 — fail closed | `ValidateOnStart`; never throws per request | Pattern 1, Pattern 7 | +| BR-U3-01…03 — per-header scope | Explicit `sendHsts` + content-type parse | `SecurityHeaderWriter` | +| BR-U3-04 — never overwrite | `TryAdd` rather than the indexer | `SecurityHeaderWriter` | +| BR-U3-05 — write at response start | `OnStarting(callback, state)` with a static delegate | `SecurityHeadersMiddleware` | +| BR-U3-06, BR-U3-09 — position | Inside the exception handler, before static files | `UseCmsSecurityHeaders` call site | +| BR-U3-07 — never throw | Catch-and-log inside the callback | `SecurityHeadersMiddleware` | +| BR-U3-10…19 — policy content | Two policies in code, origins from configuration | `CspPolicyCatalog` | +| BR-U3-20 — unknown name fatal | `ValidateOnStart` | Pattern 1 | +| BR-U3-22 | **Refined — REF-U3-01**, moved to the U5 CI gate | Carried to U5 | +| BR-U3-23, BR-U3-24 — startup logging | Logged from the `Use` extension | `SecurityHeadersExtensions` | + +--- + +## Carried Forward + +| Item | To | Why | +|---|---|---| +| **REF-U3-01** — Umami origin drift gate | **U5 CI Workflow** | The backend cannot see `VITE_UMAMI_WEBSITE_ID`; the CI job can see both sides and must fail on drift | +| Pipeline-order verification | **Build and Test** | A misordered registration passes every unit test and serves the whole website without a CSP | +| Header presence on a real `503` and a real static asset | **Build and Test** | Needs a running host with a real response feature | diff --git a/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/nfr-design-patterns.md b/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/nfr-design-patterns.md new file mode 100644 index 0000000..baff847 --- /dev/null +++ b/aidlc-docs/features/gitea-deployment-workflow/construction/u3-security-headers/nfr-design/nfr-design-patterns.md @@ -0,0 +1,273 @@ +# NFR Design Patterns — U3 HTTP Security Headers & CSP + +**Unit**: U3 HTTP Security Headers & CSP +**NFRs addressed**: NFR-01 (no server configuration), NFR-03 (startup cost), NFR-06 (testability), SECURITY-04, SECURITY-11, SECURITY-15 + +These patterns exist because U3 moves work that normally lives in nginx into the request pipeline. That changes the failure modes: a bad nginx config fails loudly at reload, whereas a bad middleware silently emits nothing. Every pattern below is chosen so that "wrong" is visible rather than quiet. + +--- + +## Pattern 1 — Fail Closed at Startup: `ValidateOnStart` + +**Rule**: BR-U3-20 — an unknown policy name must stop the process. + +**Pattern**: Bind `SecurityHeadersOptions` through the options builder with a validator and `ValidateOnStart()`, so the failure happens during host start rather than on the first request. + +```csharp +services.AddOptions() + .Bind(configuration.GetSection(SecurityHeadersOptions.SectionName)) + .Validate(o => o.PathPolicies.All(r => CspPolicyCatalog.IsKnownPolicy(r.Policy)), + "SecurityHeaders:PathPolicies contains an unknown policy name.") + .Validate(o => CspPolicyCatalog.IsKnownPolicy(o.DefaultPolicy), + "SecurityHeaders:DefaultPolicy is not a known policy.") + .Validate(o => o.AllowedScriptOrigins.All(IsOrigin) && + o.AllowedConnectOrigins.All(IsOrigin), + "SecurityHeaders origins must be scheme-and-host only, without a path.") + .ValidateOnStart(); +``` + +**Why `ValidateOnStart` and not a constructor check**: options are resolved lazily. Without `ValidateOnStart`, a typo in `PathPolicies` is discovered when the first request arrives — by which time the deployment has already been reported as successful and the health check is green. `ValidateOnStart` registers an `IStartupValidator` that runs inside `IHost.StartAsync`, so `dotnet SlpModularCms.Api.dll` exits non-zero and the release switch is visibly broken. + +**Consequence for the deploy workflow (U6)**: this makes a smoke request unnecessary to catch configuration typos — the process simply does not come up. Worth stating, because it is the difference between "deployment failed" and "deployment succeeded and the site is broken". + +--- + +## Pattern 2 — Precomposed Immutable Policies: `ICspPolicyProvider` + +**Rule**: BR-U3-18 — policy strings are composed once and reused. + +```csharp +public interface ICspPolicyProvider +{ + SecurityHeaderSet Get(string policyName); + IReadOnlyCollection PolicyNames { get; } +} + +public sealed record SecurityHeaderSet( + string ContentSecurityPolicy, + string FrameOptions, + string ReferrerPolicy); +``` + +**Implementation shape**: a singleton whose constructor builds a `FrozenDictionary` from `CspPolicyCatalog` (the in-code policy definitions) plus the configured origin lists. Nothing is composed per request; the middleware performs one dictionary lookup and three header writes. + +**Why `FrozenDictionary`**: read-only after startup and read on every response. This is the exact workload it exists for, and the build cost is paid once against a two-entry dictionary. + +**Why the split into catalog and provider**: `CspPolicyCatalog` is a pure static function of `(policy name, origin lists) → directive list` with no dependencies, so the directive content is unit-testable without a service provider or configuration. The provider only caches it. + +--- + +## Pattern 3 — Path Matching: `StartsWithSegments`, First Match Wins + +**Rule**: BR-U3-12. + +```csharp +foreach (var rule in _rules) // ordered, from configuration +{ + if (path.StartsWithSegments(rule.Prefix, StringComparison.OrdinalIgnoreCase)) + { + return rule.Policy; + } +} +return _defaultPolicy; +``` + +**`StartsWithSegments`, never `string.StartsWith`.** `"/administrator".StartsWith("/admin")` is `true`. A future route named `/admin-tools` or a public page at `/administration` would silently inherit the strict policy and lose its inline scripts — a failure that looks like a broken page with a console error and no server-side trace at all. `StartsWithSegments` compares whole path segments and does not match. + +**First match wins, not longest prefix.** The rule list is ordered, and order is meaningful. Longest-prefix matching would be more forgiving but would make the effective policy non-obvious from reading the configuration. Order is documented in the configuration comment and asserted in a test that puts a broader prefix before a narrower one and expects the broader one to win. + +**Prefixes are normalised at startup** — a configured `admin` or `/admin/` both become `/admin` — because `StartsWithSegments` requires a leading slash and treats a trailing slash inconsistently across overloads. + +--- + +## Pattern 4 — Writing at Response Start Without a Per-Request Closure + +**Rule**: BR-U3-05 — the content type is unknown before the pipeline continues. + +```csharp +public async Task InvokeAsync(HttpContext context) +{ + if (!_options.Enabled) + { + await _next(context); + return; + } + + var headerSet = _policyProvider.Get(_resolver.Resolve(context.Request.Path)); + context.Response.OnStarting(WriteHeadersCallback, + new HeaderWriteState(context.Response, headerSet)); + + await _next(context); +} + +private static readonly Func WriteHeadersCallback = static state => +{ + var s = (HeaderWriteState)state; + SecurityHeaderWriter.Apply(s.Response.Headers, s.Response.ContentType, s.HeaderSet, s.SendHsts); + return Task.CompletedTask; +}; +``` + +**Why the `(callback, state)` overload with a `static` delegate**: the lambda overload allocates a closure and a delegate on **every response**, including every static asset of the public website. Static assets are the bulk of the traffic in this hosting model. A cached static delegate plus one small state object is the difference between two allocations per response and none beyond the state. + +**Why not `IHttpResponseFeature` interception or a `Stream` wrapper**: both are heavier, both risk interfering with `SendFileAsync` fast paths used by static-file middleware, and neither buys anything — headers are still mutable at response start. + +--- + +## Pattern 5 — The Header Decision as a Pure Function (the testability pattern) + +**NFR-06**, and a trap worth naming. + +`DefaultHttpContext.Response.OnStarting(...)` **does nothing** — there is no response feature to trigger it. A unit test that builds a `DefaultHttpContext`, invokes the middleware and asserts on `context.Response.Headers` will find them empty, and the natural reaction is to assume the middleware is broken. The opposite mistake is worse: a test that asserts *nothing was set* passes for the wrong reason and keeps passing after the middleware is deleted. + +**Pattern**: all decision logic lives in a static, dependency-free writer. The middleware is glue. + +```csharp +internal static class SecurityHeaderWriter +{ + public static void Apply( + IHeaderDictionary headers, + string? contentType, + SecurityHeaderSet set, + bool sendHsts) + { + headers.TryAdd("X-Content-Type-Options", "nosniff"); + if (sendHsts) + { + headers.TryAdd("Strict-Transport-Security", HstsValue); + } + + if (!IsHtml(contentType)) + { + return; + } + + headers.TryAdd("Content-Security-Policy", set.ContentSecurityPolicy); + headers.TryAdd("X-Frame-Options", set.FrameOptions); + headers.TryAdd("Referrer-Policy", set.ReferrerPolicy); + } +} +``` + +| Component | Test style | Why | +|---|---|---| +| `CspPolicyCatalog` | Pure function over origin lists | Directive content, `'unsafe-inline'` placement, BR-U3-13 | +| `SecurityHeaderWriter` | `new HeaderDictionary()` directly | Per-header scoping, HSTS gating, never-overwrite | +| `PathPolicyResolver` | Pure function over `PathString` | `/administrator` vs `/admin`, ordering | +| Options validation | `ValidateOnStart` via a real `ServiceProvider` | Startup failure on an unknown name | +| Composed middleware | Carried to phase-level Build and Test | Needs a real response feature; `DefaultHttpContext` cannot express it | + +**`TryAdd`, not the indexer** (BR-U3-04): `headers["X-Frame-Options"] = value` overwrites. `TryAdd` is a no-op when the key exists, which is exactly the rule, and it states the intent in the call rather than in a surrounding `if`. + +--- + +## Pattern 6 — Content-Type Detection + +```csharp +private static bool IsHtml(string? contentType) => + contentType is not null && + MediaTypeHeaderValue.TryParse(contentType, out var parsed) && + parsed.MediaType.Equals("text/html", StringComparison.OrdinalIgnoreCase); +``` + +**Why parse rather than `Contains("text/html")`**: the real header is `text/html; charset=utf-8`, so a substring check happens to work — until something serves `application/xhtml+xml` or a type whose *parameter* contains the string. Parsing states the intent and costs nothing at this volume. + +**`null` is not HTML**, which is what makes the `304`, redirect and no-body cases in BR-U3's edge-case table fall out automatically rather than needing their own branches. + +--- + +## Pattern 7 — Never Throw Into the Response Path + +**Rule**: BR-U3-07. + +An exception thrown from an `OnStarting` callback surfaces after the response has begun — too late for the exception handler, and it produces a truncated or malformed response instead of a diagnosable error. The callback therefore wraps the writer: + +```csharp +try +{ + SecurityHeaderWriter.Apply(...); +} +catch (Exception ex) +{ + s.Logger.LogError(ex, "Failed to apply security headers to the response."); +} +``` + +**This is deliberately the one place that swallows.** The alternative — letting it propagate — converts a header-writing bug into a corrupted response for the user. The startup validation in Pattern 1 is what keeps this catch from hiding configuration errors: by the time a request arrives, the configuration is already known good, so anything reaching this catch is a code defect and belongs in the log (and, via U4, in Sentry at `Error`). + +--- + +## Pattern 8 — HSTS Gating Resolved Once + +`sendHsts` is `!IsDevelopment()`, evaluated **once at startup** and stored on the middleware, not per request. `IWebHostEnvironment.IsDevelopment()` is a string comparison against `EnvironmentName` that cannot change while the process runs; evaluating it per response is pure waste on a static-file workload. + +The value is passed explicitly into the writer rather than read from ambient state, so the Development and non-Development cases are both directly testable (BR-U3-02, BR-U3-08). + +--- + +## Pattern 9 — Startup Logging and the Umami Drift Check + +**Rules**: BR-U3-22, BR-U3-23, BR-U3-24. + +BR-U3-23 (log permitted origins) and BR-U3-24 (warn when disabled) are logged from `UseCmsSecurityHeaders()`, which runs once during pipeline construction and has `app.Services` available. No hosted service is needed. + +### Refinement REF-U3-01 — BR-U3-22 cannot work as specified + +BR-U3-22 asks for a startup warning when "a Umami website ID is configured but its script origin is absent from the allowed origins". **The backend cannot perform that check.** The Umami website ID is a `VITE_` variable baked into the frontend bundle at build time; the running host never sees it. Comparing `AllowedScriptOrigins` against a value the process cannot read would either always warn or never warn. + +The check is therefore **moved to where both values are visible — the CI workflow (U5)**: + +| Where | What is checked | On failure | +|---|---|---| +| ~~Backend startup~~ | ~~Umami ID configured, origin missing from CSP~~ | Withdrawn — not implementable | +| **U5 CI workflow** | If `VITE_UMAMI_SCRIPT_URL` is set for an environment's frontend build, its origin must appear in that environment's `SecurityHeaders:AllowedScriptOrigins` | **Pipeline fails** | +| Backend startup (retained) | Permitted script and connect origins | Logged at `Information` (BR-U3-23) | + +This is stronger than the rule it replaces: a drift between the frontend build and the backend CSP now blocks the deployment instead of writing a warning into a log file on a Raspberry Pi that nobody reads until something is already broken. + +**Carried to U5** as a required gate. Recorded here rather than silently dropped, because "we warn about this" appearing in a design and not in the code is exactly the kind of gap that survives to production. + +--- + +## Pattern 10 — Pipeline Position + +``` +UseExceptionHandler() ← already present + UseCmsSecurityHeaders() ← NEW: first thing inside the exception handler + UseRateLimiter() + UseHttpsRedirection() + UseCmsStaticContent() ← short-circuits; must be after us (BR-U3-06) + UseCors() + UseModules() ← availability gate; its 503 still gets headers + UseAuthentication() / UseAuthorization() + MapControllers() / MapCmsHealthChecks() / MapCmsSpaFallbacks() +``` + +Two ordering constraints, both load-bearing: + +- **Inside `UseExceptionHandler`** (BR-U3-09): the exception handler re-executes the pipeline from inside itself, so middleware registered outside it never observes the `ProblemDetails` response. Registering there means error responses carry the headers too. +- **Before `UseCmsStaticContent`** (BR-U3-06): static-file middleware terminates the request. Everything registered after it is invisible to the public website — which is almost all of the HTML this host serves. + +**Ordering is verified by test, not by comment.** A phase-level Build and Test assertion requests a static website asset and a `503` from the availability gate and asserts on the headers of both. A misordered registration compiles, starts, passes every unit test, and serves the entire website without a CSP. + +--- + +## Pattern 11 — Configuration Shape + +```jsonc +"SecurityHeaders": { + "Enabled": true, + "DefaultPolicy": "Relaxed", + "PathPolicies": [ + { "PathPrefix": "/admin", "Policy": "Strict" }, + { "PathPrefix": "/api/v1", "Policy": "Strict" }, + { "PathPrefix": "/health", "Policy": "Strict" } + ], + "AllowedScriptOrigins": [], + "AllowedConnectOrigins": [] +} +``` + +Follows the existing `IOptions` convention used by `JwtSettings`, `Availability`, `MasterModule`, `MasterPolling` and `RateLimiting`. No secret appears in this section — origins are public hostnames — so it is committed with real values per environment rather than supplied through environment variables (contrast D-16, which governs credentials). + +**The defaults are the production values.** An environment that supplies no `SecurityHeaders` section at all gets the strict policy on `/admin`, `/api/v1` and `/health` and the relaxed policy elsewhere. Omission cannot produce an unprotected host. diff --git a/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/logical-components.md b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/logical-components.md new file mode 100644 index 0000000..ffb8450 --- /dev/null +++ b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/logical-components.md @@ -0,0 +1,233 @@ +# Logical Components — U4 Observability Integration + +Backend types live in `SlpModularCms.Core/Hosting/Observability/`, with the exception of `SecurityEvents`, which sits at `SlpModularCms.Core/Observability/` because it is referenced from four assemblies and is not host wiring. Everything is in `Core`, therefore everything is reachable from `SlpModularCms.Core.Tests`. + +Frontend additions are two components, one library module and three modified files under `frontend/src/`. + +--- + +## Backend Component Wiring + +```mermaid +graph TD + subgraph Config["Configuration"] + Section["Observability section"] + Options["ObservabilityOptions"] + LogCfg["Logging:LogLevel
raised to Information"] + end + + subgraph Logging["Logging — always active"] + Activity["ActivityTrackingOptions
TraceId, SpanId, ParentId"] + Console["JsonConsole or SimpleConsole
IncludeScopes = true"] + end + + subgraph SentryWiring["Sentry — only when a DSN is present"] + UseExt["UseCmsSentry"] + Scrubber["ISentryEventScrubber"] + Processor["SecurityEventProcessor
ISentryEventProcessor"] + Sdk["Sentry SDK
MinimumEventLevel = Warning
MinimumBreadcrumbLevel = Information"] + end + + subgraph Events["Alertable Events"] + SecEvents["SecurityEvents
LoggerMessage, EventId 5001-5006"] + Reason["BypassRejectionReason"] + Sites["Emission sites:
Identity, Master, Availability, Core"] + end + + subgraph Tunnel["Sentry Tunnel"] + Target["SentryTunnelTarget
singleton, parsed at startup"] + Endpoint["MapSentryTunnel
/sentry-tunnel"] + Limiter["sentry-tunnel rate limiter"] + Client["Named HttpClient
5s timeout, no retry"] + end + + Ingest["Sentry ingest"] + + Section --> Options + Options --> UseExt + Options --> Target + LogCfg --> Console + Activity --> Console + Activity --> Sdk + UseExt --> Sdk + Scrubber --> Sdk + Processor --> Sdk + Sites --> SecEvents + SecEvents --> Reason + SecEvents --> Console + SecEvents --> Sdk + Endpoint --> Limiter + Endpoint --> Target + Endpoint --> Client + Client --> Ingest + Sdk --> Ingest + + classDef cfg fill:#fbd38d,stroke:#c05621,stroke-width:1px,color:#000; + classDef always fill:#9ae6b4,stroke:#2f855a,stroke-width:1px,color:#000; + classDef optional fill:#90cdf4,stroke:#2b6cb0,stroke-width:1px,color:#000; + classDef guard fill:#fbb6ce,stroke:#b83280,stroke-width:1px,color:#000; + classDef ext fill:#e2e8f0,stroke:#4a5568,stroke-width:1px,color:#000; + class Section,Options,LogCfg cfg; + class Activity,Console,SecEvents,Reason,Sites always; + class UseExt,Sdk,Target,Endpoint,Client,Processor optional; + class Scrubber,Limiter guard; + class Ingest ext; +``` + +Text alternative: activity tracking and console logging are always active and independent of Sentry; the Sentry SDK is registered only when a DSN is present and always behind the scrubber and the security-event tag processor; the six security events are ordinary log entries that reach both destinations; the tunnel endpoint forwards browser envelopes to a destination parsed once at startup. + +--- + +## Frontend Component Wiring + +```mermaid +graph TD + main["main.tsx"] + sentryts["lib/sentry.ts
initSentry — NEW"] + config["lib/config.ts
MODIFIED"] + apiclient["lib/api-client.ts
MODIFIED — empty base"] + vite["vite.config.ts
MODIFIED — define + dev proxy"] + boundary["SentryErrorBoundary
NEW"] + umami["UmamiAnalytics
NEW"] + inner["InnerApp"] + tunnelpath["POST /sentry-tunnel"] + + main --> sentryts + main --> boundary + main --> umami + boundary --> inner + config --> sentryts + config --> apiclient + config --> umami + vite --> sentryts + vite --> tunnelpath + sentryts --> tunnelpath + + classDef newc fill:#9ae6b4,stroke:#2f855a,stroke-width:1px,color:#000; + classDef modc fill:#fbd38d,stroke:#c05621,stroke-width:1px,color:#000; + classDef exist fill:#90cdf4,stroke:#2b6cb0,stroke-width:1px,color:#000; + classDef ext fill:#e2e8f0,stroke:#4a5568,stroke-width:1px,color:#000; + class sentryts,boundary,umami newc; + class config,apiclient,vite modc; + class main,inner exist; + class tunnelpath ext; +``` + +Text alternative: a new sentry library module is initialised first from main.tsx and posts to the same-origin tunnel; the modified config module feeds Sentry, the API client and the Umami component; vite supplies the release version and, in development, proxies the tunnel path to the local API. + +--- + +## Backend Component Responsibilities + +| Component | Lifetime | Responsibility | Pattern | +|---|---|---|---| +| `ObservabilityOptions` | Options singleton | `SentryDsn`, `Environment`, `TracesSampleRate`, `TunnelMaxPayloadBytes` | 10 | +| `LoggingExtensions.AddCmsLogging` | Static | Activity tracking, `IncludeScopes`, console provider per environment | 1, 2 | +| `SentryExtensions.UseCmsSentry` | Static | No-op without a DSN; otherwise thresholds, scrubber, processor | 3, 8 | +| `ISentryEventScrubber` / `SentryEventScrubber` | Singleton | Removes four credential headers and the body from events **and** transactions | 4 | +| `SecurityEventProcessor` | Singleton | Maps `EventId` → `security_event` tag so alert rules filter on a tag | 6 | +| `SecurityEvents` | Static partial | Six source-generated `LoggerMessage` methods, `EventId` 5001–5006 | 6 | +| `BypassRejectionReason` | Enum | Why an admin bypass was refused — never the token | 6 | +| `SentryTunnelTarget` | Singleton | Envelope endpoint parsed from the DSN once, at startup | 7 | +| `SentryTunnelExtensions.MapSentryTunnel` | Static | The `/sentry-tunnel` endpoint: size cap, forward, `202` on failure | 7 | + +### Modified existing components + +| File | Change | Why | +|---|---|---| +| `Program.cs` (both hosts) | `AddCmsLogging`, `UseCmsSentry`, `MapSentryTunnel` | Host wiring | +| `appsettings.json` (both hosts) | `Observability` section; `Logging` raised to `Information`; EF command logging pinned at `Warning` | Pattern 3 — otherwise breadcrumbs arrive empty and EF logs parameter values | +| `ServiceCollectionExtensions.AddCmsRateLimiting` | `OnRejected` callback; new `sentry-tunnel` limiter | Pattern 6, Pattern 7 — `429`s currently leave no trace at all | +| `DatabaseMigrationExtensions` (U2) | `FlushAsync` before rethrow | Pattern 5 — otherwise the `Critical` event dies with the process | +| `AdminTokenValidator` (U1) | Overload returning `BypassRejectionReason` | Pattern 6 | +| `AvailabilityMiddleware` | Emit `AdminBypassRejected` | Pattern 6 | + +Two of these touch files U1 and U2 already committed. Both are additive and both are named in the U4 commit message. + +--- + +## Frontend Component Responsibilities + +| Component | Kind | Responsibility | +|---|---|---| +| `lib/sentry.ts` | Module | `initSentry()`: skip without a DSN, tunnel path, no router integration | +| `components/SentryErrorBoundary.tsx` | Class component | Catch render errors, report, show a recoverable fallback with no exception text | +| `components/UmamiAnalytics.tsx` | Component | Inject the script once, never in development, no cleanup | +| `lib/config.ts` | Modified | Empty-or-URL schema, four new fields | +| `lib/api-client.ts` | Modified | Correct URL construction with an empty base | +| `main.tsx` | Modified | `initSentry()` first; boundary inside `AuthProvider`; `UmamiAnalytics` alongside `InnerApp` | +| `vite.config.ts` | Modified | `__APP_VERSION__` define; dev proxy for `/sentry-tunnel` → local API | +| `vite-env.d.ts` | Modified | Declare the five new `VITE_` variables and `__APP_VERSION__` | + +--- + +## Packages Added + +| Project | Package | Version | Note | +|---|---|---|---| +| `SlpModularCms.Core` | `Sentry.AspNetCore` | `6.8.0` | Latest on nuget.org as of 2026-07-28. **`net10.0` compatibility to be confirmed at code generation** — if the package resolves only to a `net9.0` asset the build still succeeds, but a warning must not be ignored silently | +| `frontend` | `@sentry/react` | `^10.68.0` | Same major as the reference project | + +`Sentry.AspNetCore` in `Core` rather than in the two host projects, because `UseCmsSentry`, the scrubber, the processor and the tunnel all live in `Core` and both hosts consume them. It brings `Sentry` and `Sentry.Extensions.Logging` transitively. + +**FR-22 interaction**: this adds a dependency subtree to `Core` while U5 is due to pin two packages against known advisories. The new tree must be checked in the same pass, not assumed clean because it is new. + +--- + +## DI Registration Order + +``` +builder.Logging: + 1. AddCmsLogging(builder.Environment) ← ClearProviders, ActivityTracking, IncludeScopes +builder.WebHost: + 2. UseCmsSentry(builder.Configuration) ← no-op without a DSN +builder.Services: + 3. AddOptions().Bind(...).ValidateOnStart() + 4. AddSingleton() + 5. AddSingleton() + 6. AddSingleton() ← throws at startup on an unparseable DSN + 7. AddHttpClient("sentry-tunnel") + 8. AddCmsRateLimiting(...) ← existing call, now also registers "sentry-tunnel" +app: + 9. MapSentryTunnel() ← after MapControllers, before MapCmsSpaFallbacks +``` + +**Steps 1 and 2 are order-critical** (BR-U4-04): a Sentry initialisation problem must be logged by an already-registered provider. + +**Step 9's position matters**: `MapCmsSpaFallbacks` maps `{*path:nonfile}`. A catch-all registered before the tunnel would not actually shadow it — route precedence favours the literal segment — but relying on precedence for a security-relevant endpoint is a poor trade against writing the two lines in the obvious order. + +--- + +## NFR Coverage Traceability + +| NFR / Rule | Pattern | Component | +|---|---|---| +| SECURITY-03 — correlation ID on every entry | 1 | `ActivityTrackingOptions` + `IncludeScopes` | +| SECURITY-03 — no secrets or PII in logs | 3, 4 | EF command logging pinned; `SecurityEvents` templates carry no credentials | +| SECURITY-11 — no request forgery | 7 | `SentryTunnelTarget` — destination never from the caller | +| SECURITY-14 — alertable security events | 6 | `SecurityEvents` + `SecurityEventProcessor` tag | +| SECURITY-15 — failures never propagate | 8 | `UseCmsSentry` no-op path; tunnel returns `202` on forwarding failure | +| NFR-01 — no server configuration | 7 | The tunnel is in-process, not in nginx | +| NFR-07 — no secrets committed | 10 | DSN empty in `appsettings`, supplied as `Observability__SentryDsn` | +| NFR-08 — graceful degradation | 8 | Three fully functional configurations | +| BR-U4-01…05 | 1, 2, 3 | Logging always active, before Sentry, `Information` console | +| BR-U4-06, BR-U4-07 | 3 | `MinimumEventLevel` / `MinimumBreadcrumbLevel` | +| BR-U4-08…12 | 8 | DSN gate, environment and release tags | +| BR-U4-13, BR-U4-14 | 4 | `SentryEventScrubber`, in-process, two layers | +| BR-U4-15…21 | 7 | Tunnel endpoint and target | +| BR-U4-22…25 | 9 | `config.ts` union schema, `api-client.ts` URL construction | +| BR-U4-26…29 | 9 | `initSentry` DSN gate, `UmamiAnalytics` guards | + +--- + +## Carried Forward + +| Item | To | Why | +|---|---|---| +| Sentry alert rules for `security_event:*` | **Operations — Monitoring Setup** (FR-19) | The tag exists in code; the rules are configured in Sentry | +| `Observability__SentryDsn` as an environment variable per environment | **U6 Deploy Workflow** | Must not be committed | +| `VITE_SENTRY_DSN`, `VITE_APP_ENV`, `VITE_UMAMI_*` per environment build | **U5 CI Workflow** | Build-time values; two artifacts (D-15) | +| New dependency subtree checked against advisories | **U5** (FR-22) | `Sentry.AspNetCore` is new and must not be assumed clean | +| `Sentry.AspNetCore` `net10.0` asset confirmation | **Code Generation** | A `net9.0` fallback builds but should be a conscious acceptance | +| Trace ID propagation master → slave verified end to end | **Build and Test** | Needs both hosts running and a real master/slave call — the whole justification for Pattern 1 | +| `apiBaseUrl === ''` request construction verified | **Build and Test** + unit test | Only fails in the production configuration, which nobody runs locally | diff --git a/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/nfr-design-patterns.md b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/nfr-design-patterns.md new file mode 100644 index 0000000..5ed3fea --- /dev/null +++ b/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/nfr-design-patterns.md @@ -0,0 +1,462 @@ +# NFR Design Patterns — U4 Observability Integration + +**Unit**: U4 Observability Integration +**NFRs addressed**: NFR-01 (no server configuration), NFR-07 (no secrets in the repository), NFR-08 (graceful degradation), SECURITY-03, SECURITY-11, SECURITY-14, SECURITY-15 + +Two things dominate this design. First, **every observability service must be optional** — the same binary has to run with no DSN, no Umami and no network egress at all. Second, **the mechanisms that protect credentials and enable alerting are the ones most likely to look correct while doing nothing**, so each is designed to be independently testable rather than asserted by a comment. + +--- + +## Pattern 1 — OPEN-01 Resolved: the Correlation ID Is the W3C Trace ID + +**OPEN-01** asked whether the correlation identifier required by SECURITY-03 should be `HttpContext.TraceIdentifier` or the W3C `traceparent` trace ID. **Decision: the W3C trace ID**, taken from the ambient `Activity`, with `TraceIdentifier` as the fallback when no `Activity` exists. + +```csharp +builder.Logging.Configure(options => + options.ActivityTrackingOptions = + ActivityTrackingOptions.TraceId | + ActivityTrackingOptions.SpanId | + ActivityTrackingOptions.ParentId); +``` + +That single call puts `TraceId` into the logging scope of **every** entry from every category, including framework categories, without touching a single call site. BR-U4-03 ("every log entry carries a correlation identifier") is satisfied by configuration rather than by discipline — which matters, because the alternative is remembering to pass an ID into thousands of existing log calls. + +### Why the W3C trace ID rather than `TraceIdentifier` + +| Property | `HttpContext.TraceIdentifier` | W3C trace ID | +|---|---|---| +| Crosses the master/slave HTTP boundary | **No** — host-local, meaningless elsewhere | **Yes** — `HttpClient` injects `traceparent` automatically | +| Matches what the client already sees | No | **Yes** — see below | +| Understood by Sentry natively | No | **Yes** — becomes the event's trace | +| Available outside a request (startup, background service) | No — there is no `HttpContext` | Yes, when an `Activity` is started | +| Cost | None | One line of configuration | + +**The decisive argument is the master/slave boundary.** This codebase's hardest diagnostic question is "the master says the slave rejected its API key — what did the slave actually see?". `TraceIdentifier` cannot answer it: the master's ID and the slave's ID are unrelated strings. The W3C trace ID is propagated by `HttpClient` on the outbound call and adopted by the slave's hosting layer, so both sides' log entries and both sides' Sentry events carry the **same** value. That is the entire reason a correlation ID exists here. + +**The second argument is alignment with what already exists.** ASP.NET Core's built-in `ProblemDetails` writes `traceId` as `Activity.Current?.Id ?? HttpContext.TraceIdentifier`. Choosing the W3C trace ID means the value in the log, the value in the Sentry event, the value propagated to the slave and the value the browser was shown in the error response are **one value** — so an operator can be handed a `traceId` from a screenshot and find the log entry. Choosing `TraceIdentifier` would produce two competing identifiers for the same request, which is worse than having one imperfect one. + +The `?? TraceIdentifier` fallback is kept for exactly the framework's reason: no `Activity` means no trace ID, and a null correlation ID is worse than a host-local one. + +### The trap that makes this silently not work + +Scopes are only rendered if the console provider is told to render them: + +```csharp +options.IncludeScopes = true; // without this, TraceId is collected and then discarded +``` + +`ActivityTrackingOptions` populates the scope; `IncludeScopes` is what writes it out. Configure the first and forget the second and every log entry looks completely normal, with no correlation ID and no error anywhere. **A test asserts a `TraceId` field is present in rendered console output**, not merely that the option was set. + +--- + +## Pattern 2 — Registration Order: Logging Before Sentry + +**Rule**: BR-U4-04. + +```csharp +// 1. Logging first — so a Sentry problem is itself logged. +builder.Logging.ClearProviders(); +builder.Logging.Configure(o => o.ActivityTrackingOptions = ...); +if (builder.Environment.IsDevelopment()) +{ + builder.Logging.AddSimpleConsole(o => { o.IncludeScopes = true; o.SingleLine = true; }); +} +else +{ + builder.Logging.AddJsonConsole(o => { o.IncludeScopes = true; }); +} + +// 2. Sentry second, and only when a DSN is present. +builder.WebHost.UseCmsSentry(builder.Configuration); +``` + +**Why the two console formats differ by environment**: in test and production the process is supervised (systemd on the Pi) and its stdout lands in the journal, where JSON is greppable and `TraceId` is a field rather than a substring. Locally a developer reads it with their eyes, and JSON is hostile to that. Both set `IncludeScopes`, so the correlation ID is present either way — the formatting differs, the information does not. + +**Why `ClearProviders()`**: the default host already added a console provider without `IncludeScopes`. Adding a second one produces every line twice, once with the correlation ID and once without — which reads as a logging bug and wastes real time. + +--- + +## Pattern 3 — Threshold Split Maps Onto Two Existing Options + +**Rules**: BR-U4-02, BR-U4-06, BR-U4-07. This is the reconciliation of Q3 = C with Q16 = C, and it needs no custom code: + +```csharp +options.MinimumBreadcrumbLevel = LogLevel.Information; // BR-U4-07 +options.MinimumEventLevel = LogLevel.Warning; // BR-U4-06 +``` + +| Destination | Threshold | Mechanism | +|---|---|---| +| Console | `Information`, framework included | `Logging:LogLevel:Default` raised from `Warning` to `Information` | +| Sentry breadcrumbs | `Information` | `MinimumBreadcrumbLevel` | +| Sentry events | `Warning` | `MinimumEventLevel` | + +**`appsettings.json` must change too.** Today it says `"Default": "Warning"` and `"Microsoft.AspNetCore": "Warning"`. Leaving those in place would cap the console *and* the breadcrumbs at `Warning`, and every Sentry event would arrive with an empty breadcrumb trail — the feature present, configured, and useless. Both move to `Information`; `Microsoft.EntityFrameworkCore.Database.Command` stays at `Warning`, because `Information` there logs every SQL statement including parameter values. + +That last exclusion is not a volume concern, it is BR-U4-05: EF's command logging at `Information` prints parameter values, and the login path passes a normalised email through it. + +--- + +## Pattern 4 — Scrubbing as an Injectable, Testable Filter + +**Rules**: BR-U4-13, BR-U4-14. Defence in depth, in two layers. + +### Layer 1 — never capture the body at all + +```csharp +options.MaxRequestBodySize = RequestSize.None; // the SDK default; set explicitly +``` + +Set explicitly with a comment, because the safest version of "the request body is never sent to Sentry" is that it is never read into an event in the first place. Relying on the default means a future `RequestSize.Small` added by someone chasing a bug quietly starts shipping login payloads. + +### Layer 2 — remove credential headers before send + +```csharp +public interface ISentryEventScrubber +{ + SentryEvent Scrub(SentryEvent @event); +} + +internal sealed class SentryEventScrubber : ISentryEventScrubber +{ + internal static readonly string[] RemovedHeaders = + [ + "Cookie", // carries refreshToken + "Set-Cookie", + "Authorization", // bearer token + "X-Master-Api-Key" // master/slave shared secret + ]; + + public SentryEvent Scrub(SentryEvent @event) + { + foreach (var header in RemovedHeaders) + { + @event.Request.Headers.Remove(header); + } + + @event.Request.Data = null; + return @event; + } +} +``` + +Wired in through both send hooks: + +```csharp +options.SetBeforeSend((e, _) => scrubber.Scrub(e)); +options.SetBeforeSendTransaction((t, _) => scrubber.ScrubTransaction(t)); +``` + +**Why `SetBeforeSendTransaction` as well as `SetBeforeSend`**: performance transactions carry request data too. Scrubbing only events leaves a second, less obvious channel open — and it is less obvious precisely because nobody thinks of a transaction as containing headers. Enabling tracing later without touching this file would otherwise start leaking. + +**Why an interface rather than an inline lambda**: the scrub list is the single most security-critical piece of code in this unit, and a lambda inside a `UseSentry` callback cannot be unit-tested without initialising the SDK. As an injected class it is tested directly: construct a `SentryEvent` with all four headers plus a body, scrub, assert each is gone and that the retained fields (method, path, query, user agent, IP, username) survive. + +**`Set-Cookie` is in the list although the functional design did not name it.** Response headers can be attached to an event, and `Set-Cookie` on the login and refresh responses contains the `refreshToken` being issued. Removing the request cookie while sending the response cookie would protect nothing. + +**Not configurable, by construction** (functional design, `domain-entities.md`): `RemovedHeaders` is a `static readonly` array in code, not an options property. A configurable scrub list is a supported way to switch the protection off by omission. + +--- + +## Pattern 5 — Flush Before Exit, or the Event That Matters Most Is the One You Lose + +The Sentry SDK batches and sends in the background. A process that throws during startup and exits kills that background sender before it transmits — so the **migration failure**, the one event in the whole system that requires immediate human attention (BR-U4 security event table, level `Critical`), is exactly the event most likely never to arrive. + +`MigrateCoreDatabase` (U2) therefore gains a flush on its failure path: + +```csharp +catch (Exception ex) +{ + logger.LogCritical(ex, "..."); // existing behaviour + await SentrySdk.FlushAsync(TimeSpan.FromSeconds(5)); + throw; // still fails fast — unchanged +} +``` + +Five seconds, bounded: a host that cannot reach its database is already down, and five seconds of additional downtime buys the alert that tells someone why. `FlushAsync` is a no-op when the SDK was never initialised, so the no-DSN path is unaffected. + +This is a **change to a U2 file made by U4** and is called out in the commit message, since U2 is already committed. + +--- + +## Pattern 6 — Alertable Security Events: Source-Generated Templates + a Sentry Tag + +**Rules**: the six events in `domain-entities.md`; FR-19 alert rules; SECURITY-14. This is the pattern that decides whether FR-19 is implementable at all. + +### The problem + +Sentry groups log-derived events by their **message**. Emitted with interpolation — + +```csharp +_logger.LogWarning($"Login failed for {email} on {endpoint}"); // WRONG +``` + +— every distinct email produces a **separate Sentry issue**. An alert rule of the form "more than 20 failed logins in 5 minutes" can then never fire, because no single issue ever reaches 20. The feature appears to work: events arrive, they are visible, they are tagged. Only the alerting silently cannot exist. + +### The pattern + +One source-generated `LoggerMessage` per event type, with a fixed template and a stable `EventId`: + +```csharp +internal static partial class SecurityEvents +{ + public const int FailedLoginEventId = 5001; + public const int AuthorizationDeniedEventId = 5002; + public const int MasterApiKeyRejectedEventId = 5003; + public const int AdminBypassRejectedEventId = 5004; + public const int RateLimitTriggeredEventId = 5005; + public const int MigrationFailureEventId = 5006; + + [LoggerMessage( + EventId = FailedLoginEventId, + Level = LogLevel.Warning, + Message = "Security event: login failed on {Endpoint} (account exists: {AccountExists})")] + public static partial void FailedLogin(ILogger logger, string endpoint, bool accountExists); + + [LoggerMessage( + EventId = AdminBypassRejectedEventId, + Level = LogLevel.Warning, + Message = "Security event: admin bypass rejected on {Path} (reason: {Reason})")] + public static partial void AdminBypassRejected(ILogger logger, string path, BypassRejectionReason reason); + + // …4 more +} +``` + +The template is a compile-time constant, so all failed logins group into one Sentry issue with the variable parts as structured fields. The rate-based alert rule becomes possible. + +### Tagging, so alert rules filter on a tag rather than a substring + +```csharp +internal sealed class SecurityEventProcessor : ISentryEventProcessor +{ + private static readonly FrozenDictionary Tags = /* 5001 → "failed_login", … */; + + public SentryEvent Process(SentryEvent @event) + { + if (@event.Extra.TryGetValue("EventId", out var id) && + id is int eventId && + Tags.TryGetValue(eventId, out var tag)) + { + @event.SetTag("security_event", tag); + } + return @event; + } +} +``` + +Alert rules then read `security_event:failed_login`, which is stable across message-wording changes. Matching on message text would break the day someone improves the wording — silently, because a rule that matches nothing looks identical to a rule with nothing to match. + +### `BypassRejectionReason` + +An enum — `InvalidSignature`, `Expired`, `WrongIssuer`, `WrongAudience`, `NotAdmin`, `Malformed` — not a string, and never the token. `AdminTokenValidator` (U1) currently returns `bool`; it gains an overload returning the reason so this event can carry it. The distinction is operationally real: `InvalidSignature` means someone is forging tokens, `Expired` means an administrator left a tab open. + +### Where each event is emitted + +| Event | Assembly | Emission site | +|---|---|---| +| Failed login | `Modules.Identity` | Auth service login path | +| Authorization denied | `Core` | Authorization failure handler | +| Master API key rejected | `Modules.Master` | API key authentication handler | +| Admin bypass rejected | `Modules.Availability` | `AvailabilityMiddleware.IsAdminBypass` | +| Rate limit triggered | `Core` | `AddCmsRateLimiting` → `OnRejected` | +| Migration failure | `Core` | `MigrateCoreDatabase` catch (Pattern 5) | + +`SecurityEvents` therefore lives in **`SlpModularCms.Core`**, which all four assemblies already reference. + +**`OnRejected` does not exist yet** on the rate limiter — `AddCmsRateLimiting` sets only `RejectionStatusCode`. It is added here. Without it, a brute-force attempt against `/api/v1/Auth/login` returns `429` and leaves no trace anywhere, which makes the one rate limiter this application has unobservable. + +--- + +## Pattern 7 — The Sentry Tunnel Endpoint + +**Rules**: BR-U4-15…21. An anonymous endpoint that performs an outbound request on demand needs every one of these constraints. + +### Destination computed once, at startup + +```csharp +public sealed class SentryTunnelTarget +{ + public Uri EnvelopeEndpoint { get; } // https://{host}/api/{projectId}/envelope/ + public bool IsConfigured { get; } + + // Parsed from the DSN: https://{publicKey}@{host}/{projectId} +} +``` + +Registered as a singleton and parsed at startup. **No part of the destination ever comes from the request** (BR-U4-16). This is the single rule that separates a tunnel from a server-side request forgery primitive, and computing it once makes it structurally impossible to accidentally read from the caller. + +An unparseable DSN fails at startup rather than per request — consistent with U3 Pattern 1 and with SECURITY-15. + +### Size limit enforced twice + +```csharp +if (request.ContentLength > maxBytes) +{ + return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); +} + +var buffer = await request.Body.ReadAtMostAsync(maxBytes + 1, cancellationToken); +if (buffer.Length > maxBytes) +{ + return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); +} +``` + +**Both checks are needed.** `Content-Length` is absent under chunked transfer encoding and is attacker-controlled in any case, so the pre-check is an optimisation, not the control. The bounded read is the control. Trusting `ContentLength` alone gives an anonymous caller an unbounded memory allocation on a Raspberry Pi. + +### Path, and why it is `/sentry-tunnel` + +Same path as the reference project, where nginx serves it. Here the application does — NFR-01 forbids depending on the reverse proxy — but keeping the path identical means the frontend `tunnel` option, the vite dev proxy and any operator's muscle memory carry over unchanged between the two workspaces. + +Deliberately **not** under `/api/v1`: it is not a versioned CMS API and must not appear in the OpenAPI document, and `/api/v1` maps to the strict CSP path list in U3 for reasons that have nothing to do with this endpoint. + +### Rate limiting — added beyond what the functional design asked for + +The tunnel gets its own fixed-window limiter (`"sentry-tunnel"`, in the existing `RateLimiting` configuration section). An anonymous endpoint that triggers an outbound HTTPS request per call is a free amplifier and a way to burn the Sentry plan's quota from outside. The three existing controls — fixed destination, size cap, no DSN means no forwarding — bound *what* each call can do but not *how many* calls there can be. This closes that. + +### Behaviour on failure + +| Condition | Response | Rationale | +|---|---|---| +| No DSN configured | `404` | BR-U4-18. `404` rather than `503`, because with no DSN the endpoint genuinely does not exist | +| Payload too large | `413` | BR-U4-17 | +| Rate limit exceeded | `429` | Existing limiter behaviour | +| Forwarding to Sentry fails | `202`, logged server-side at `Warning` | The browser must not retry or console-error over a failed error report. Failing to report an error must not itself become an error | +| Instance availability-disabled | `503` from the gate | BR-U4-20, accepted loss | + +**BR-U4-20 needs no code.** The tunnel is a mapped endpoint, and `AvailabilityMiddleware` runs before endpoint execution, so it is gated by default. Not being on `_bypassPrefixes` is the whole implementation — worth stating so nobody "fixes" it later. + +### `HttpClient` + +A named client via `IHttpClientFactory` with a short timeout (5s) and no retry. A dropped error report is acceptable; a request thread held open by an anonymous caller is not. + +--- + +## Pattern 8 — Graceful Absence Everywhere + +**Rules**: BR-U4-08, BR-U4-09, NFR-08. + +```csharp +public static IWebHostBuilder UseCmsSentry(this IWebHostBuilder b, IConfiguration config) +{ + var dsn = config["Observability:SentryDsn"]; + if (string.IsNullOrWhiteSpace(dsn)) + { + return b; // BR-U4-08 — a supported state, not an error, and not a warning + } + return b.UseSentry(o => { /* … */ }); +} +``` + +**No warning is logged when the DSN is absent.** Local development is the common case, and a startup warning that always appears is noise that trains people to ignore startup warnings — including the ones from U3 Pattern 9 that matter. + +Beyond that, the SDK is already fire-and-forget: an unreachable Sentry queues, then drops. Nothing wraps a request in a try/catch for Sentry's benefit, because there is nothing to catch. The only place the design adds a deliberate barrier is the tunnel's `202`-on-failure above. + +--- + +## Pattern 9 — Frontend: Sentry, Umami, and Same-Origin Configuration + +### Packages + +| Package | Version | Note | +|---|---|---| +| `@sentry/react` | `^10.68.0` | Same major as the reference project, so patterns transfer | +| — | | No Umami package; a script tag is the whole integration | + +### `lib/sentry.ts` + +```ts +export function initSentry(): void { + const { sentryDsn, appEnv } = getAppConfig(); + if (!sentryDsn) return; // BR-U4-26 + + Sentry.init({ + dsn: sentryDsn, + environment: appEnv, + release: __APP_VERSION__, + tracesSampleRate: 0.1, + sendDefaultPii: false, + tunnel: '/sentry-tunnel', // BR-U4-15 — never the ingest URL + }); +} +``` + +`__APP_VERSION__` requires `define` in `vite.config.ts` and a declaration in `vite-env.d.ts`, both following the reference project exactly. + +**`tanstackRouterBrowserTracingIntegration` is not adopted.** The reference project uses it, but it needs the router instance, and importing `@/router` from `sentry.ts` — which `main.tsx` calls before anything else — inverts the initialisation order and drags the whole route tree into the startup path. Route-level tracing is not worth that; it can be added later inside `main.tsx` where the router is already imported. + +### The dev tunnel proxy points at our own API + +```ts +server: { + proxy: { + '/sentry-tunnel': { target: 'https://localhost:7221', changeOrigin: true, secure: false }, + }, +}, +``` + +Different from the reference, and the difference is the point: the reference proxies straight to Sentry's ingest host, which means the DSN's project ID is hard-coded in `vite.config.ts` and has to stay in sync by hand (the reference file carries a `LET OP` comment saying exactly that). Here the proxy targets the local API, which derives the destination from its own DSN — so there is nothing to keep in sync and no project ID in a committed file. + +### `config.ts` — relaxed by exactly one case + +```ts +const configSchema = z.object({ + apiBaseUrl: z.union([z.literal(''), z.string().url()]), // BR-U4-24 + appTitle: z.string(), + sentryDsn: z.string().optional(), + appEnv: z.string().optional(), + umamiScriptUrl: z.string().optional(), + umamiWebsiteId: z.string().optional(), +}); +``` + +`apiBaseUrl` resolves via `import.meta.env.VITE_API_BASE_URL ?? ''`. `z.union` with `z.literal('')` permits empty and still rejects `htp://localhost:7221`; `z.string().url().optional()` would not, and `z.string()` alone would accept anything. + +**`api-client.ts` needs one check.** With `apiBaseUrl === ''`, request URLs must be built as `` `${base}${path}` `` where `path` already starts with `/`. Any code doing `` `${base}/${path}` `` or `new URL(path, base)` breaks on an empty base — `new URL` throws on an empty base. This is verified by test rather than by reading, because the failure only appears in the production configuration, which is the one nobody runs locally. + +### `UmamiAnalytics` + +Adopted essentially verbatim from the reference project: `useEffect`, guard on `import.meta.env.DEV || !scriptUrl || !websiteId`, guard on an existing element by id, `defer`, `data-website-id`. + +One deliberate change: **no cleanup that removes the script.** The reference returns a cleanup removing the element, which under React 18 `StrictMode` double-invocation means inject → remove → inject. Umami's script registers listeners and sends the initial page view on load; removing the element does not unregister them, so the double-invocation can produce a duplicated first page view. The component is mounted for the application's lifetime and has nothing to clean up. The duplicate guard (`getElementById`) handles re-invocation on its own. + +This is why the functional design's `UmamiAnalytics` test list includes "not injected twice on re-render" — that assertion is what catches the regression if the cleanup is ever reinstated. + +--- + +## Pattern 10 — Configuration Shape + +```jsonc +"Observability": { + "SentryDsn": "", // from an environment variable in test/production + "Environment": "", // falls back to ASPNETCORE_ENVIRONMENT + "TracesSampleRate": 0.1, + "TunnelMaxPayloadBytes": 204800 // 200 KB +} +``` + +`Logging` changes alongside it: + +```jsonc +"Logging": { + "LogLevel": { + "Default": "Information", + "Microsoft.AspNetCore": "Information", + "Microsoft.EntityFrameworkCore.Database.Command": "Warning" + } +} +``` + +**The DSN is committed empty and supplied per environment as `Observability__SentryDsn`.** Consistent with D-16 and NFR-07. A DSN is not a standing credential — the frontend's copy is in the page source — but it identifies a project, it belongs to an account, and there is no reason for the repository to be where it is written down. + +--- + +## Decisions Taken Here Without Asking + +Three values were chosen rather than escalated, because a sensible default exists and the work would otherwise have blocked. Each is one line to change: + +| Decision | Chosen | Reasoning | Change it if | +|---|---|---|---| +| Console format | JSON outside Development, human-readable locally | Supervised process in test/production; journald and `jq` both handle JSON, and `TraceId` becomes a field | You read production logs by eye more often than you query them | +| `TracesSampleRate` | `0.1`, backend and frontend | Sentry's free plan counts transactions against the same quota as errors, and this design's value is in errors, not performance traces. The reference project uses `1.0` — for a low-traffic marketing site with no API | Traffic stays low and you want full traces | +| Tunnel max payload | 200 KB | Sentry envelopes with a stack trace and breadcrumbs run tens of KB; 200 KB is generous without being an allocation risk | Envelopes are rejected in practice | diff --git a/aidlc-docs/features/gitea-deployment-workflow/inception/requirements/requirements.md b/aidlc-docs/features/gitea-deployment-workflow/inception/requirements/requirements.md index d93d644..68f99a2 100644 --- a/aidlc-docs/features/gitea-deployment-workflow/inception/requirements/requirements.md +++ b/aidlc-docs/features/gitea-deployment-workflow/inception/requirements/requirements.md @@ -342,7 +342,7 @@ DEV-01…04 are **pre-existing or cost-driven** and none is introduced by this f | ID | Item | To be resolved | |---|---|---| -| OPEN-01 | **Correlation/request ID in logs** is required by SECURITY-03 but does not exist today. Needs a decision on mechanism (ASP.NET Core `TraceIdentifier` versus `W3C traceparent`). | NFR Design / Construction | +| ~~OPEN-01~~ | ~~**Correlation/request ID in logs** is required by SECURITY-03 but does not exist today. Needs a decision on mechanism (ASP.NET Core `TraceIdentifier` versus `W3C traceparent`).~~ **RESOLVED 2026-07-28** at NFR Design for U4: the **W3C trace ID** from the ambient `Activity`, with `TraceIdentifier` as the fallback when no `Activity` exists. Enabled through `ActivityTrackingOptions` plus `IncludeScopes`, so every entry from every category carries it without changing any call site. Chosen because it propagates across the master/slave HTTP boundary via `traceparent` — the one diagnostic question `TraceIdentifier` cannot answer — and because it is the same value ASP.NET Core's `ProblemDetails` already returns to the client. See `construction/u4-observability/nfr-design/nfr-design-patterns.md` Pattern 1. | Closed | | ~~OPEN-02~~ | ~~`AvailabilityMiddleware.IsAdminBypass` reads the JWT without validating its signature.~~ **RESOLVED 2026-07-27** at Application Design (Q12 = A): folded into this feature as **FR-24**, landing in the same unit as the `/health` bypass since both touch the same middleware. | Closed | | OPEN-03 | **Exact patched versions** for `Microsoft.OpenApi` and `System.Security.Cryptography.Xml` (FR-22) must be resolved and verified against the advisories. | Construction | | OPEN-04 | **Whether production stays on the Pi long enough** that FTPS is never built. D-02 requires only that the design allows it; the trigger for actually building it is a business decision. | Deferred by design | @@ -353,9 +353,9 @@ DEV-01…04 are **pre-existing or cost-driven** and none is introduced by this f This feature turns a manually deployed modular-monolith CMS into one with an automated, auditable pipeline, on hosting where nothing can be configured server-side. -**24 functional requirements, 10 non-functional requirements, 32 traced decisions, 7 assumptions, 3 remaining open items, 4 documented security deviations.** +**24 functional requirements, 10 non-functional requirements, 32 traced decisions, 7 assumptions, 2 remaining open items, 4 documented security deviations.** -*(FR-24 added and OPEN-02 closed at Application Design on 2026-07-27.)* +*(FR-24 added and OPEN-02 closed at Application Design on 2026-07-27. OPEN-01 closed at NFR Design on 2026-07-28.)* The three requirements that carry the most risk if implemented carelessly: