feat(infra): Docker-compose configuration and environment-specific security guards for Beta deployment to Test environment #56
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mjasin/Nexus.Reader#56
Reference in New Issue
Block a user
Delete Branch "infra/beta-deploy-test"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request introduces the dedicated containerized infrastructure and configuration for deploying NexusReader's beta version in the Test environment.
Summary of Changes
Docker Infrastructure & Secrets:
docker-compose.test.yml: Configured dedicated database and auxiliary services (PostgreSQL 17, Qdrant, Neo4j) on isolated, non-standard ports to ensure zero conflict with the existing server configurations..env.test.template: Provided an environment variable template showing required setups, including mandatory database passwords, API keys, and admin custom passwords..gitignore: Excluded local.envfiles to prevent accidental commits of production or staging secrets.Database Hardening:
IDriverinstantiation uses basic auth when credentials are provided in configuration).DbInitializer.cs) to dynamically useNEXUS_ADMIN_PASSWORDfrom environment variables, falling back to a default password in local Development only.Feature-Flagged Restrictions:
appsettings.Test.json: ImplementedFeatures:AllowRegistrationandFeatures:AllowPasswordResetflags set tofalse.Program.cs): Intercepts requests to/identity/registerand/identity/forgotPassword(and their MVC/form variations) and rejects them with a403 Forbiddenresponse in restricted environments.Program.cs): Blocks new account provisioning via Google OAuth callback by checking theFeatures:AllowRegistrationconfiguration, redirecting users to the login page with a descriptive error.Login.razor,Register.razor): Conditionally hides registration/password reset links and intercepts manual navigation attempts to/account/registerby redirecting to login with a warning.Code Review — PR #56: Beta Docker Infrastructure & Feature-Gated Security Guards
This PR is well-structured and addresses a real deployment need. The Docker configuration, secret management, and UI feature-flag guards are solid. Below are findings, ordered by severity.
Summary
@@ -0,0 +25,4 @@dockerfile: Dockerfilecontainer_name: nexus-web-testports:- "${WEB_PORT:-5050}:5000"🟡 Design — Hardcoded internal port for Qdrant gRPC in
ConnectionStrings__QdrantConnectionThe web container connects to Qdrant via
http://qdrant:6334(internal container port), which is correct for inter-service Docker networking. However, the gRPC port6334is hardcoded in the environment variable string rather than being derived from theQDRANT_GRPC_PORTvariable. This is fine for the internal network but is inconsistent with how other connection strings are parameterized.Consider making this explicit with a comment noting this is the internal container-to-container port, not the host-mapped port, so future maintainers don't confuse it with
${QDRANT_GRPC_PORT}:@@ -0,0 +55,4 @@- QDRANT__SERVICE__API_KEY=${QDRANT_API_KEY:-}ports:- "${QDRANT_HTTP_PORT:-6343}:6333"- "${QDRANT_GRPC_PORT:-6344}:6334"🟡 Design — Qdrant healthcheck uses raw TCP socket; brittle and platform-dependent
The current healthcheck
bash -c 'exec 3<>/dev/tcp/127.0.0.1/6333'depends on bash's/dev/tcppseudo-device, which is not available in all minimal Docker images (e.g., Alpine-based). If the Qdrant image is ever updated to an Alpine base, this will silently fail, causing thewebservice to hang at startup.The Qdrant HTTP API exposes a
/healthzendpoint. Use that instead:@@ -69,3 +71,3 @@};adminUser.PasswordHash = passwordHasher.HashPassword(adminUser, "Admin123!");var adminPassword = configuration?["Nexus:AdminPassword"]🔴 Blocking — Triple-layer fallback exposes hardcoded default credential in production
The current fallback chain
Nexus:AdminPassword→NEXUS_ADMIN_PASSWORD(fromIConfiguration) →Environment.GetEnvironmentVariable("NEXUS_ADMIN_PASSWORD")→"Admin123!"is dangerous. IfNEXUS_ADMIN_PASSWORDis not injected at container startup (e.g. operator error, mis-spelled var), the process will silently seed the admin account with"Admin123!"without any warning — a critical security regression in Test/Prod.The
Environment.GetEnvironmentVariablecall is also redundant becauseIConfigurationin ASP.NET Core already reads environment variables. You only need two keys, and the final fallback must only be allowed inDevelopment.Suggested fix:
@@ -0,0 +230,4 @@_chatMessages.Add(new ChatMessage{Sender = "User",🟡 Design/Architecture —
AuthStateProviderinjected directly into UI Organism; bypasses established service patternThis component injects
AuthenticationStateProviderand callsGetAuthenticationStateAsync()directly to extracttenantId. The established pattern in this codebase is to read claims via a server-side service (e.g.,IIdentityService) or a pre-populatedUserInfomodel viaPersistentComponentState. CallingAuthStateProviderdirectly in a leaf Organism tightly couples it to the auth subsystem and makes it harder to test.Refactor to accept
tenantIdas a[Parameter]from the parentReaderLayout.razor, which already has access to the auth state. This keeps the Organism a pure presentation component.@@ -0,0 +261,4 @@});}else{🔴 Blocking — Regex compiled on every
ParseSegmentscall; violates static compiled-regex ruleA
new Regex(...)is created insideParseSegmentson every invocation. This creates a new regex engine instance and JIT-compiles it on each call — a known performance regression and a violation of the project's static compiled-regex standard (seeEpubReaderService.csfor the correct pattern).Suggested fix: Move the regex to a
private static readonlyfield at the top of the component's@codeblock:Then use
CitationRegex.Matches(text)inParseSegments.@@ -0,0 +81,4 @@{var isCurrent = cp == StateService.CurrentBlockId;<div class="checkpoint-item @(isCurrent ? "active" : "")" @onclick="() => SelectCheckpoint(cp)"><div class="checkpoint-indicator">🟢 Minor —
_scrollListenerReferenceinReaderCanvasis disposed withouttry/catch, swallowing potential JS interop errors silentlyIn
ReaderCanvas.DisposeAsync,_scrollListenerReference?.DisposeAsync()is wrapped in an emptycatch {}block. While teardown errors are generally non-fatal, a barecatch {}silently suppresses all exceptions includingJSDisconnectedException. Prefer at minimumcatch (Exception ex) { Logger.LogDebug(ex, "..."); }for consistency with the_viewportModuleteardown pattern directly above it.@@ -128,12 +207,15 @@}🟡 Design —
DotNetObjectReferencecreated insideInitializeObserverAsyncwithout being stored or disposedIn
InitializeObserverAsync, a newDotNetObjectReference.Create(this)is passed toinitObserver. This reference is not assigned to a field, so it cannot be disposed inDisposeAsync. The unmanaged JS side will hold a reference to the GC'd .NET object indefinitely, which is a memory leak.The same pattern was correctly fixed for
_selfReference(for the viewport observer). Apply the same treatment here — create a second_observerSelfReferencefield, assign it, and dispose it inDisposeAsync:@@ -467,4 +467,184 @@ main {.back-to-graph-btn:hover {🟢 Minor — Missing newline at end of file
Both
ReaderLayout.razor.cssandReaderCanvas.razor.cssend with\ No newline at end of file. This will generate noise in future diffs. Please add a trailing newline to both files.@@ -1,1 +1,1 @@@page "/serilog-demo"🟢 Minor/Suggestion —
#if DEBUGreplaced with@if (_isDebug)but_isDebuginitialization not visible in diffThe change from a compile-time
#if DEBUGto a runtime@if (_isDebug)is architecturally sound for AOT/Blazor Hybrid compatibility. However, please ensure_isDebugis properly initialized (e.g., viaSystem.Diagnostics.Debugger.IsAttachedor a[Parameter]/ service injection) and that the component doesn't render its diagnostic UI in Test/Production due to incorrect initialization logic.Follow-up Review — Commit
5340be30(chore(beta-deploy): refactor GlobalIntelligence for AOT/Regex compliance and apply CSS polish)Good progress. The commit addressed 2 of 9 issues. Here's the updated status for all open threads.
✅ Resolved
GlobalIntelligence—AuthenticationStateProviderinjected directlyTenantIdis now a[Parameter]GlobalIntelligence:ParseSegments—new Regex()on every callprivate static readonlycompiled fields (CitationRegex,BoldRegex,ItalicRegex,CodeBlockRegex,InlineCodeRegex)SerilogDemo.razor—_isDebuginitialization unclear_isDebugis backed by#if DEBUG/#elsecompile-time constants, producing dead code elimination in Release/Test builds🔴 Still Blocking (1)
DbInitializer.cs— Silent credential fallback in non-Development environments. The code is unchanged from the original. The triple-layer fallbackNexus:AdminPassword→NEXUS_ADMIN_PASSWORD(viaIConfiguration) →Environment.GetEnvironmentVariable("NEXUS_ADMIN_PASSWORD")→"Admin123!"remains. An operator misconfiguration will silently seed a weak admin password in Test/Production without any log warning or startup failure. This is the last blocking issue preventing merge.@@ -0,0 +55,4 @@- QDRANT__SERVICE__API_KEY=${QDRANT_API_KEY:-}ports:- "${QDRANT_HTTP_PORT:-6343}:6333"- "${QDRANT_GRPC_PORT:-6344}:6334"🟡 Design — Still unresolved. Qdrant healthcheck uses bash
/dev/tcp.The healthcheck remains
bash -c 'exec 3<>/dev/tcp/127.0.0.1/6333'. This is platform-dependent and not supported on Alpine-based images. Prefer the/healthzHTTP endpoint:@@ -69,3 +71,3 @@};adminUser.PasswordHash = passwordHasher.HashPassword(adminUser, "Admin123!");var adminPassword = configuration?["Nexus:AdminPassword"]🔴 Blocking — Still unresolved. Silent credential fallback in non-Development environments.
This code is identical to the original. The
"Admin123!"default will still be reached silently in Test/Production ifNEXUS_ADMIN_PASSWORDis absent (e.g. from a typo in the.envfile or a missing Docker secret). Thedocker-compose.test.ymldoes enforce${NEXUS_ADMIN_PASSWORD:?...}at the compose level, but this C# fallback provides a false safety net that can be triggered by non-compose deployments (e.g., directkubectl apply).Please add the environment check before applying the fallback:
This also eliminates the redundant
Environment.GetEnvironmentVariablecall, sinceIConfigurationalready reads env vars.@@ -137,3 +218,4 @@_scrollListenerReference = await module.InvokeAsync<IJSObjectReference>("initScrollListener", DotNetObjectReference.Create(this), ".reader-flow-container");}catch (Exception ex){🟡 Design — Still unresolved. Untracked
DotNetObjectReferenceinInitializeObserverAsync.DotNetObjectReference.Create(this)is called twice insideInitializeObserverAsync(once forinitObserverand once already implicitly forinitScrollListenervia the_scrollListenerReferencereturned value). The reference passed toinitObserveris not stored in a field and therefore cannot be disposed, creating a GC-root memory leak from the JS side.Create a dedicated
_observerSelfReferencefield:@@ -298,0 +436,4 @@if (_viewportModule != null){if (_selfReference != null){🟢 Minor — Still unresolved. Bare
catch {}inDisposeAsyncfor_scrollListenerReference.The empty catch block on
_scrollListenerReference.DisposeAsync()silently swallows all exceptions. For consistency with the_viewportModuleteardown immediately above it, use at minimum: