Repository navigation
fix: /admin/users concurrency race (#242) and host module schema isolation (#229) - #244
Merged
Merged
Conversation
GetUsersPagedAsync and GetAllRolesAsync were run via Task.WhenAll, but both contracts (IUserAdminContracts, IRoleAdminContracts) resolve the same scoped UsersDbContext. EF Core forbids concurrent operations on one context instance, so the parallel await intermittently threw and surfaced as HTTP 500. Await the two queries sequentially. The roles list is tiny, so the extra round-trip is negligible, and the shared-context concurrency is removed structurally.
…ng module (#229) The generated HostDbContext (which physically creates the schema) prefixed only each module's explicit DbSet entities, then swept every remaining entity into the identity module's prefix. A child entity reachable only through a navigation — which EF's relationship-discovery convention adds to the model without a DbSet — fell into that catch-all (e.g. Users_RecurringLine), while the owning module's context queried Invoices_RecurringLine, producing a "no such table" error. Add ApplyHostModuleSchemas: seed module ownership from declared DbSets, propagate it across foreign keys to a fixed point so navigation-only children (and grandchildren) inherit their principal's module, then fall back to the identity module only for genuinely unowned tables (ASP.NET Identity). The host generator now emits an entity->module seed map and one call to this helper, replacing the brittle per-DbSet emission. All existing module entities have DbSets, so every table name is unchanged and no migration drift is introduced.
antosubash
enabled auto-merge (squash)
June 5, 2026 09:16
Deploying simplemodule-website with
|
| Latest commit: |
2d817e7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f015cad.simplemodule-website.pages.dev |
| Branch Preview URL: | https://fix-github-bugs-229-242.simplemodule-website.pages.dev |
antosubash
added a commit
that referenced
this pull request
Jun 10, 2026
…ning, DbContext races/outbox, docs/CI reconciliation (#246) * fix: resurrect the page-registry guard on both ends (CI and runtime) 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. * fix: remaining #242-class DbContext races and lost-event outbox gaps 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. * fix(security): harden the default deployment posture 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). * docs+ci: reconcile docs with the tracked codebase; add security scanning 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). * fix: address code-review round 1 (8 findings + deferred follow-ups recorded) 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. * test: cover the security-critical environment guards (review round 2) 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. * fix(filestorage): allow .txt/.csv in the default upload allowlist; add 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.
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
1 participant
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
Fixes two open
bug-labeled issues.#242 — intermittent HTTP 500 on
/admin/usersUsersEndpointranGetUsersPagedAsyncandGetAllRolesAsyncviaTask.WhenAll, butIUserAdminContractsandIRoleAdminContractsboth resolve the same scopedUsersDbContext. EF Core forbids concurrent operations on a single context instance, so the parallel await intermittently threw and surfaced as a 500. The two queries now await sequentially, removing the shared-context concurrency structurally (the roles list is tiny, so the extra round-trip is negligible).#229 — navigation-only child entity created under the wrong module's schema
The generated
HostDbContext(which physically creates the schema) prefixed only each module's explicitDbSetentities, then swept every remaining entity into the identity module's prefix. A child entity reachable only through a navigation — which EF's relationship-discovery convention adds to the model without aDbSet— fell into that catch-all (e.g.Users_RecurringLine), while the owning module's context queriedInvoices_RecurringLine, producing a "no such table" error.New
ApplyHostModuleSchemashelper: seeds module ownership from declaredDbSets, propagates it across foreign keys to a fixed point so navigation-only children and grandchildren inherit their principal's module, then falls back to the identity module only for genuinely unowned tables (ASP.NET Identity). The host generator now emits an entity→module seed map and a single call to this helper, replacing the brittle per-DbSetemission.Verification
DbSet, so all table names are unchanged; only genuine Identity tables use the fallback.dotnet build— 0 errors (warnings-as-errors enabled globally).dotnet test --no-build— 1208 passed, 0 failed, 0 skipped (incl. Database 93, Generator 206, Admin 29; integration suites boot the real generatedHostDbContext). NewHostModuleSchemaTestsreproduce Module child entity (a [Dto] with Id) can get its table created under the wrong module's schema prefix (no such table) #229: a navigation-only child, a 2-level grandchild, and a Postgres-schema variant all land in the owning module — notUsers_; an unrelated table still falls back to identity.Browser/E2E verification is N/A: these are framework/source-generator and backend-endpoint changes with no frontend surface (#229 is codegen; #242 is an intermittent race proven by the structural fix + green integration tests).
Test plan
dotnet test)[Dto]child entity creates its table under the owning module's schema/prefix