fix(framework): correctness & perf findings from framework review#278
Open
antosubash wants to merge 2 commits into
Open
fix(framework): correctness & perf findings from framework review#278antosubash wants to merge 2 commits into
antosubash wants to merge 2 commits into
Conversation
…view Fixes from a five-area review of the framework layer (Core, Generator, Hosting, Database/Storage, frontend build). Scoped to the framework; no module behavior changed except where a module consumed a broken framework primitive. High - FeatureFlags gate + audit interceptor read ClaimTypes.NameIdentifier only, so under Keycloak/JWT (MapInboundClaims=false, id in "sub") per-user overrides were ignored and CreatedBy/UpdatedBy went null. Use sub-aware GetUserId()/GetRoles(). - Static-file cache middleware applied a 1yr public,immutable header to ANY request with a "?v=" query key, incl. authenticated HTML (e.g. /dashboard?v=1). Gate the immutable header on static-asset path prefixes. - LocalStorageProvider: ListAsync bypassed the traversal guard entirely; GetFullPath's boundary check lacked a separator (so /app/storage-backup passed an /app/storage check) and used OrdinalIgnoreCase on case-sensitive filesystems. Route ListAsync through the guard; require a separator boundary; OS-appropriate case sensitivity. - Multi-tenant query filter froze the first request's scoped ITenantContext into the cached EF model (cross-tenant leak). Reference the executing DbContext instance via new ITenantScopedDbContext.CurrentTenantId so EF re-evaluates per query. - Source generator re-scanned every referenced assembly (BCL, ASP.NET) on each compile. Filter out System./Microsoft./framework assemblies from the discovery walks. Medium - InertiaResult merged shared data by round-tripping props through a JsonElement DOM then re-serializing the whole dict — props encoded twice per render. Collapse to one Utf8JsonWriter pass (props win on collision, no duplicate keys). - InertiaLayoutDataMiddleware ran antiforgery crypto + menu filtering for every request incl. static assets and health probes. Gate to Inertia (X-Inertia) or HTML (Accept: text/html) requests. - EntityInterceptor/EntityChangeInterceptor only overrode the async SaveChanges path, so a sync SaveChanges() would hard-delete soft-delete entities and skip audit/events. Override the sync path on both. - Generated JSON resolver was uncompilable for init-only props / positional records (CS8852/CS7036). Treat init-only setters as non-settable; guard CreateObject on an accessible parameterless ctor. - Vite vendor plugin skipped rebuild whenever outputs existed, shipping stale React after a version bump. Key a manifest on resolved package versions. Low - PermissionMatcher wildcard match: span-based, no per-check substring allocation. - GlobalExceptionHandler: ArgumentNullException -> 500 (internal bug) not client 400. - InertiaMiddleware.Version: guard empty Assembly.Location (single-file publish crash). - SymbolHelpers: module-namespace prefix match now requires a dot boundary. - ViewPagesEmitter: hint name from module FQN (no collision for same-named classes). - SoftDeleteService.CoerceId: handle Guid / value-object keys instead of throwing. - Maintenance gate: correct the static-asset comment and exempt static-asset paths. - PermissionRegistryBuilder: O(1) HashSet dedup instead of O(n^2) list scan. - app.tsx: in-page hash anchors scroll natively instead of re-requesting via Inertia. - add-component.mjs: resolve packages/ or src/ UI dir; typecheck.mjs: cap tsc concurrency. Regression tests: cross-tenant isolation across a shared cached model; sub-aware GetUserId/GetRoles; wildcard module-boundary; ViewPages same-name collision. Deferred (documented in tasks/todo.md): generator pipeline decomposition onto MetadataReferencesProvider; @simplemodule/ui shared-chunk vendoring. Claude-Session: https://claude.ai/code/session_01GACvG1C7EZGSDDGTaBUFjg
antosubash
marked this pull request as ready for review
July 16, 2026 18:33
…`docker run` works
The runtime image runs as a non-root appuser, but the app's built-in default
connection string ("Data Source=app.db") resolves to the root-owned /app
workdir, so a standalone `docker run` of the image crashed at startup with
"SQLite Error 14: unable to open database file". The Dockerfile already creates
a writable /app/data directory for the database but never pointed the app at it.
Set Database__DefaultConnection to Data Source=/app/data/app.db in the runtime
stage. docker-compose still overrides this with its PostgreSQL configuration, so
the Postgres deployment path is unchanged; only the standalone-SQLite quickstart
is fixed.
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
Addresses the findings from a five-area review of the framework layer (Core, Generator, Hosting, Database/Storage, frontend build). Scoped to the framework; no module behavior changes except where a module consumed a broken framework primitive.
Two strategic rewrites are intentionally deferred (noted in
tasks/todo.md): splitting the generator pipeline ontoMetadataReferencesProvider, and de-duplicating@simplemodule/uiacross module bundles.High severity
ClaimTypes.NameIdentifieronly. Under Keycloak/JWT (MapInboundClaims=false, id insub), per-user feature overrides were silently ignored andCreatedBy/UpdatedBywent null. Now use the sub-awareGetUserId()/ newGetRoles().?v=cache poisoning. The static-file cache middleware applied a 1-yearpublic, immutableheader to any request with avquery key — including authenticated HTML (/dashboard?v=1), publicly cacheable across users behind a shared cache/CDN. Now gated on static-asset path prefixes.LocalStorageProviderpath traversal.ListAsyncbypassed the traversal guard entirely;GetFullPath'sStartsWith(_basePath)lacked a separator boundary (/app/storage-backuppassed an/app/storagecheck) and usedOrdinalIgnoreCaseon case-sensitive filesystems. RoutedListAsyncthrough the guard; require a separator boundary; OS-appropriate case sensitivity.DbContextvia newITenantScopedDbContext.CurrentTenantId, which EF re-evaluates per query. Zero production callers today, so the API change is safe.System./Microsoft./framework assemblies out of the discovery walks.Medium
Utf8JsonWriterpass; props still win on collision, no duplicate keys.X-Inertia) or HTML (Accept: text/html) requests.SaveChanges()would hard-delete soft-delete entities and skip audit/events. Sync overrides added to both.CreateObjectguarded on an accessible parameterless ctor.Low
PermissionMatcher span-based wildcard;
ArgumentNullException→ 500 not 400; single-file-publish version-init guard; namespace-boundary match; ViewPages FQN hint name;CoerceIdhandles Guid/value-object keys; maintenance-gate comment + static-asset exemption; O(1) permission dedup; in-page hash anchors scroll natively;add-component.mjspath resolution; cappedtscconcurrency.Tests & verification
New regression tests: cross-tenant isolation across a shared cached model (fails under the old code), sub-aware
GetUserId/GetRoles, wildcard module-boundary, ViewPages same-name collision.dotnet build— 0 errors (warnings-as-errors)dotnet test— Core 280, Database 94, Generator 220, DevTools 35 all pass; CLI exit 0Notes
LocalStoragetraversal (fix: optimize symbol resolution and enhance type mapping across emitters #3) andCoerceIdGuid (Add SessionStart hook for cloud agent setup #18) are verified by inspection + build — no dedicated storage test project exists (adding one needs slnx + Dockerfile wiring).[FormRequest]scan was intentionally kept: the two passes feed different models (TS interfaces vs diagnostics); only the redundant BCL walk was made cheap via the assembly filter.https://claude.ai/code/session_01GACvG1C7EZGSDDGTaBUFjg