fix: critical issues — page-registry guard, deployment-security hardening, DbContext races/outbox, docs/CI reconciliation - #246
Merged
Conversation
validate-pages.mjs scanned modules/<Name>/src/<Name>, but the real layout is
src/SimpleModule.<Name> — it validated zero C# files and printed success in CI
and 'npm run check' since the layout change. Fix the path by scanning every
project dir under src/ (skipping obj/bin/wwwroot, whose stale artifacts contain
Render calls for deleted pages), resolve Inertia.Render arguments that are
same-file string consts (UnlockAccountEndpoint pattern), and add a self-check
that fails the run when zero C# files or zero endpoints are found repo-wide so
path drift can never silently disable the guard again.
The runtime fallback was equally dead: app.tsx called showErrorToast(), which
does not exist (ReferenceError instead of an error toast on failed page loads).
Root cause it survived: ClientApp had no tsconfig.json and was excluded from
scripts/typecheck.mjs. Add the tsconfig, wire ClientApp into typecheck (with a
hard failure if its tsconfig ever disappears), and fix the two latent bugs the
new checking exposed: router.on('exception') is not an Inertia v3 event (the
network-error toast never fired — the event is 'networkError'), and the page
resolver must return the component, not the module wrapper.
Sequentialize the three surviving Task.WhenAll-over-scoped-DbContext sites (AdminService.GetAdminOverviewAsync, Admin UsersEditEndpoint, TenantFeatureHelper.GetOverridesForTenantAsync — the last threw whenever 2+ active feature flags existed). EF Core forbids concurrent operations on one context instance, so these surfaced as intermittent HTTP 500s under load, exactly like the UsersEndpoint instance fixed in #244. Route TenantService.CreateTenantAsync and FileStorageService UploadFileAsync / DeleteFileAsync through IDbContextOutbox instead of SaveChangesAsync followed by bus.PublishAsync — the old pattern lost the event when the process died between the two calls, and DeleteFileAsync additionally skipped the event whenever blob deletion failed after the DB commit. Create flows need the database-generated id in the event payload, so they save inside an explicit transaction and let SaveChangesAndFlushMessagesAsync persist the envelope, commit, and only then flush — FakeDbContextOutbox now mirrors that commit-current-transaction behavior.
A default deployment was one HTTP request away from an admin token: the compiled-in admin@simplemodule.dev / Admin123! fallback was seeded in every environment (with only a log warning), the shipped docker-compose ran in Development which turns on the ROPC password grant, and the host cleared KnownProxies/KnownIPNetworks so X-Forwarded-For spoofing bypassed the per-IP rate limiter that would slow brute force. Closing all links in the chain: - UserSeedService now fails host startup outside Development when Seed:AdminPassword is unset (SeedConfigurationException escapes the tolerate-DB-errors catch) instead of seeding the published default; the demo user is skipped entirely unless Seed:UserPassword is provided. - New OpenIddictProductionGuard fails startup in Production when OpenIddict:AllowPasswordGrant is enabled or when signing/encryption certificates are missing (ephemeral keys regenerate per restart and invalidate every issued token). - Forwarded-header trust is now explicit: loopback by default, with ForwardedHeaders:KnownProxies / KnownNetworks / TrustAllProxies config instead of unconditionally trusting any client-supplied header. - docker-compose.yml pins OpenIddict__AllowPasswordGrant=false, requires SEED_ADMIN_PASSWORD via .env, and documents the Production switches. - UploadEndpoint actually enforces FileStorageModuleOptions: 413 above MaxFileSizeMb, 400 for extensions outside AllowedExtensions (both were defined but never consulted).
CLAUDE.md and the Constitution described a system that is not on main: 11 load-test scenarios (mostly against Products/Orders/Marketplace/PageBuilder modules that exist only as untracked artifacts), benchmarks 'for every module', and a dual-provider CI that never existed. Rewrite the testing sections around what is actually tracked (6 NBomber scenarios, 5 benchmark classes), switch the page-registry examples from the phantom Products module to the real Tenants module — the old example also showed the wrong src/<Name> layout, which is exactly the path bug that silently disabled validate-pages — and state plainly that the PostgreSQL CI leg is a known gap rather than claiming it runs. CI gains the security scanning an auth framework published to NuGet should have had: a CodeQL workflow (C# + JS/TS), Dependabot for nuget/npm/actions, and a vulnerable-package audit job that fails on known-vulnerable NuGet dependencies (including transitive).
…corded) Correctness: - ForwardedHeaders:KnownProxies/KnownNetworks now accept the comma-separated scalar form the docker-compose comment documents (Get<string[]>() returned null for a scalar env var, silently leaving the proxy untrusted), and parse via TryParse with a named error instead of an opaque FormatException. - docker-compose worker gets the same Seed__AdminPassword/Seed__UserPassword as the api: the worker runs UserSeedService too, so without them it would win the startup race and seed the admin with the compiled-in default — defeating the api's required SEED_ADMIN_PASSWORD. - FileStorageService.UploadFileAsync scopes blob rollback to the pre-commit window: a post-commit outbox-flush failure no longer deletes a blob whose StoredFile row is already durably committed. - UserSeedService and OpenIddictProductionGuard now share HostEnvironmentExtensions.IsLocalOrTest (Development+Testing). This closes the inconsistency where the guard fired only in Production (Staging bypassed it) while the seeder hard-failed in every non-Development env (crashing Testing). - UsersEditEndpoint reverted to Task.WhenAll: its three contracts resolve distinct DbContexts (UsersDbContext / PermissionsDbContext / OpenIddict token store), so there was never a shared-context race — only AdminService genuinely shares UsersDbContext. Corrected the misleading comment. Hardening of the guards themselves: - ConfigKeys.OpenIddictAllowPasswordGrant constant replaces three hardcoded key strings (a rename would otherwise silently disable the production guard). - validate-pages reports interpolated Inertia.Render($"…") as unresolved instead of skipping it; validate-i18n gains the same fail-on-zero self-check. Cleanup: - UploadEndpoint resolves IOptions and parses the extension allowlist once at Map() time instead of allocating a HashSet per request. Larger architectural findings (shared outbox helper, production-config validation abstraction, module-options↔settings binding, the SM0060 analyzer, TenantFeatureHelper N+1) are recorded in tasks/todo.md as deliberate follow-ups.
Round-2 review found no new bugs but flagged the env-branching seed/guard logic as untested — and those paths (refuse default admin password in a real deployment; refuse the password grant / ephemeral keys outside local) can't be exercised by the browser QA run, only by unit tests. - Extract UserSeedService.ResolveSeedPassword as a pure, internal decision function so the 'never seed a real deployment with the compiled-in default' branching is testable without an Identity stack; 9 cases cover configured / local-default / required-fail / optional-skip / empty-string. - HostEnvironmentExtensions.IsLocalOrTest: 7 cases pin the Development/Testing → true, Production/Staging/QA/custom → false classification both guards depend on. - OpenIddictProductionGuard: 8 cases assert it tolerates unsafe config locally, throws on password-grant or missing certs in Production/Staging/QA, and passes when fully configured. Full suite: 19/19 assemblies, 0 failures; npm run check green.
…d validation e2e Enforcing FileStorageModuleOptions.AllowedExtensions (added this branch) exposed a contradiction in the framework: the default allowlist (.jpg…/.doc/.docx/.pdf/ .zip) omits plain text, yet filestorage-crud.spec.ts uploads a .txt file and asserts 201. A file-storage module that rejects plain .txt and .csv by default is an oversight, so add both common safe text formats to the default rather than weaken the test. Also fixes a latent bug in the FileStorage.AllowedExtensions setting default, which carried embedded literal quotes. Adds a durable regression test (FileStorage upload validation) asserting a disallowed extension (.exe) is rejected with 400 and an allowed one (.txt) is accepted — locking in the enforcement so it can't silently regress.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the four critical issues from the framework review, the recurring bug classes behind them, and the doc/CI gaps — verified by automated CI, two adversarial code-review rounds, a browser QA pass, and the e2e suite.
1. Page-registry footgun guard — resurrected on both ends.
validate-pages.mjsscannedmodules/<Name>/src/<Name>but the real layout issrc/SimpleModule.<Name>, so it validated zero C# files and printed success in CI andnpm run check. Fixed the path (scan every project dir undersrc/, skipobj/bin/wwwroot), resolveInertia.Rendercalls that use same-file string consts, flag interpolatedRender($"…"), and add a fail-on-zero self-check so path drift can never silently disable it again. Now validates 73 endpoints across 553 files.app.tsxcalledshowErrorToast(...)(doesn't exist →ReferenceError). Root cause: ClientApp had notsconfig.jsonand was excluded fromtypecheck.mjs. Added the tsconfig + wired ClientApp into typecheck, which then exposed two more latent bugs —router.on('exception')isn't an Inertia v3 event (nownetworkError), and the resolver returned a module wrapper where Inertia needs the component.2. Default deployment hardened (was one request from an admin token).
UserSeedServicefails host startup outside Development/Testing whenSeed:AdminPasswordis unset instead of seeding the publishedAdmin123!default.OpenIddictProductionGuardrefuses the ROPC password grant and ephemeral signing keys in real deployments. Both guards shareHostEnvironmentExtensions.IsLocalOrTestso they never disagree.KnownProxies/KnownNetworks/TrustAllProxies) instead of trusting any spoofedX-Forwarded-For.docker-compose.ymlpinsOpenIddict__AllowPasswordGrant=false, requiresSEED_ADMIN_PASSWORD(api and worker), and documents proxy config.UploadEndpointnow enforcesFileStorageModuleOptions(413 overMaxFileSizeMb, 400 outsideAllowedExtensions).3. Remaining #242-class DbContext races + lost-event outbox gaps.
AdminService); revertedUsersEditEndpointto parallel after verifying its 3 contracts use distinct DbContexts; fixedTenantFeatureHelper(threw with 2+ active flags).TenantService.CreateTenantAsyncandFileStorageServiceupload/delete throughIDbContextOutboxso events commit atomically with the DB write (the old SaveChanges-then-Publish lost events on crash; delete also dropped the event on a failed blob delete).4. Docs reconciled with the tracked codebase.
src/<Name>layout was the origin of the validate-pages bug).dotnet list package --vulnerableCI gate.Verification
dotnet build -warnaserror: 0 warnings / 0 errorsdotnet test: 19/19 assemblies, 0 failures (incl. 24 new tests covering the security guards:HostEnvironmentExtensions,OpenIddictProductionGuard,UserSeedService.ResolveSeedPassword)npm run check: biome + validate-pages + i18n + framework-scope + typecheck 14/14npm run build: all workspaces.txt/.csv/.png→201,.exe→400), upload/delete outbox flows, admin user-edit parallel revert (all tabs render), 404 error page, zero JS console errorsDeferred follow-ups (recorded in
tasks/todo.md, not silently dropped): the SM0060 analyzer for the race/outbox bug classes, a shared outbox helper, a framework production-config validation abstraction, module-options↔Settings binding, theTenantFeatureHelperN+1, and a PostgreSQL CI leg.Test plan
npm run validate-pagesnow reports a non-zero endpoint countSeed:AdminPasswordfails fast