From c8698f917a59cc5bde1e7f65003118948af275ff Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 16 Jul 2026 17:25:34 +0200 Subject: [PATCH 1/2] fix(framework): address correctness & perf findings from framework review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Authorization/PermissionMatcher.cs | 4 +- .../PermissionRegistryBuilder.cs | 11 +- .../Exceptions/GlobalExceptionHandler.cs | 8 ++ .../Extensions/ClaimsPrincipalExtensions.cs | 13 ++ .../EndpointFeatureFlagExtensions.cs | 6 +- .../Inertia/InertiaMiddleware.cs | 29 +++- .../Inertia/InertiaResult.cs | 98 +++++++++---- .../ITenantScopedDbContext.cs | 17 +++ .../Interceptors/EntityChangeInterceptor.cs | 105 ++++++++++---- .../Interceptors/EntityInterceptor.cs | 29 +++- .../ModuleModelBuilderExtensions.cs | 48 ++++--- .../SoftDelete/SoftDeleteService.cs | 42 +++++- .../Discovery/DiscoveryDataBuilder.cs | 1 + .../Discovery/Finders/DtoFinder.cs | 24 ++++ .../Discovery/Finders/DtoPropertyExtractor.cs | 6 +- .../Discovery/Records/DataRecords.cs | 3 + .../Discovery/Records/WorkingTypes.cs | 7 + .../Discovery/SymbolDiscovery.cs | 51 +++++++ .../Discovery/SymbolHelpers.cs | 11 +- .../Emitters/JsonResolverEmitter.cs | 12 +- .../Emitters/ViewPagesEmitter.cs | 10 +- .../Middleware/InertiaLayoutDataMiddleware.cs | 39 ++++- .../Middleware/MaintenanceModeMiddleware.cs | 23 ++- .../SimpleModuleHostExtensions.Helpers.cs | 29 +++- .../SimpleModuleHostExtensions.cs | 8 +- .../LocalStorageProvider.cs | 31 +++- .../src/vite-plugin-vendor.ts | 37 ++++- scripts/add-component.mjs | 6 +- scripts/typecheck.mjs | 22 ++- tasks/todo.md | 136 +++++++++--------- template/SimpleModule.Host/ClientApp/app.tsx | 4 + .../ClaimsPrincipalExtensionsTests.cs | 39 +++++ .../EntityInterceptorTests.MultiTenant.cs | 64 +++++++++ .../EntityInterceptorTests.TestEntities.cs | 12 +- .../ViewDiscoveryTests.cs | 12 +- .../ViewPagesEmitterTests.cs | 66 ++++++++- 36 files changed, 855 insertions(+), 208 deletions(-) create mode 100644 framework/SimpleModule.Database/ITenantScopedDbContext.cs diff --git a/framework/SimpleModule.Core/Authorization/PermissionMatcher.cs b/framework/SimpleModule.Core/Authorization/PermissionMatcher.cs index 6b2a850f..bd1a3e66 100644 --- a/framework/SimpleModule.Core/Authorization/PermissionMatcher.cs +++ b/framework/SimpleModule.Core/Authorization/PermissionMatcher.cs @@ -23,12 +23,12 @@ public static bool IsMatch(string claim, string requirement) if (string.Equals(claim, requirement, StringComparison.Ordinal)) return true; - // Wildcard: claim ends with ".*" + // Wildcard: claim ends with ".*" — match the "Prefix." span without allocating. if ( claim.Length > 2 && claim[^1] == '*' && claim[^2] == '.' - && requirement.StartsWith(claim[..^1], StringComparison.Ordinal) + && requirement.AsSpan().StartsWith(claim.AsSpan(0, claim.Length - 1), StringComparison.Ordinal) ) { return true; diff --git a/framework/SimpleModule.Core/Authorization/PermissionRegistryBuilder.cs b/framework/SimpleModule.Core/Authorization/PermissionRegistryBuilder.cs index 27e74c93..c9035621 100644 --- a/framework/SimpleModule.Core/Authorization/PermissionRegistryBuilder.cs +++ b/framework/SimpleModule.Core/Authorization/PermissionRegistryBuilder.cs @@ -8,6 +8,7 @@ namespace SimpleModule.Core.Authorization; public sealed class PermissionRegistryBuilder { private readonly Dictionary> _byModule = new(); + private readonly HashSet _seen = new(StringComparer.Ordinal); public void AddPermissions() where T : class @@ -28,16 +29,18 @@ public void AddPermission(string permission) var dotIndex = permission.IndexOf('.', StringComparison.Ordinal); var module = dotIndex >= 0 ? permission[..dotIndex] : "Global"; + if (!_seen.Add(permission)) + { + return; + } + if (!_byModule.TryGetValue(module, out var list)) { list = []; _byModule[module] = list; } - if (!list.Contains(permission)) - { - list.Add(permission); - } + list.Add(permission); } public PermissionRegistry Build() diff --git a/framework/SimpleModule.Core/Exceptions/GlobalExceptionHandler.cs b/framework/SimpleModule.Core/Exceptions/GlobalExceptionHandler.cs index 47c70416..d5168485 100644 --- a/framework/SimpleModule.Core/Exceptions/GlobalExceptionHandler.cs +++ b/framework/SimpleModule.Core/Exceptions/GlobalExceptionHandler.cs @@ -23,6 +23,14 @@ CancellationToken cancellationToken ErrorMessages.ValidationErrorTitle, ve.Errors ), + // ArgumentNullException almost always signals an internal bug (a null + // reaching a guard clause), not bad client input — let it fall through to + // the 500 branch so it logs at Error and doesn't leak the parameter name. + ArgumentNullException => ( + StatusCodes.Status500InternalServerError, + ErrorMessages.InternalServerErrorTitle, + null + ), ArgumentException => ( StatusCodes.Status400BadRequest, ErrorMessages.ValidationErrorTitle, diff --git a/framework/SimpleModule.Core/Extensions/ClaimsPrincipalExtensions.cs b/framework/SimpleModule.Core/Extensions/ClaimsPrincipalExtensions.cs index 39cd3704..8ade0617 100644 --- a/framework/SimpleModule.Core/Extensions/ClaimsPrincipalExtensions.cs +++ b/framework/SimpleModule.Core/Extensions/ClaimsPrincipalExtensions.cs @@ -1,3 +1,5 @@ +using System.Collections.Generic; +using System.Linq; using System.Security.Claims; using SimpleModule.Core.Authorization; @@ -14,6 +16,17 @@ public static class ClaimsPrincipalExtensions ?? principal.FindFirstValue(ClaimTypes.NameIdentifier); } + /// + /// Gets the user's role values, supporting both OpenIddict/JWT ("role", emitted when + /// MapInboundClaims = false) and ASP.NET Identity (). + /// + public static IEnumerable GetRoles(this ClaimsPrincipal principal) + { + return principal + .Claims.Where(c => c.Type == "role" || c.Type == ClaimTypes.Role) + .Select(c => c.Value); + } + /// /// Returns null for Admin users (no scoping — sees all resources), or the user ID for regular users. /// diff --git a/framework/SimpleModule.Core/FeatureFlags/EndpointFeatureFlagExtensions.cs b/framework/SimpleModule.Core/FeatureFlags/EndpointFeatureFlagExtensions.cs index df60bd1b..e7015dbd 100644 --- a/framework/SimpleModule.Core/FeatureFlags/EndpointFeatureFlagExtensions.cs +++ b/framework/SimpleModule.Core/FeatureFlags/EndpointFeatureFlagExtensions.cs @@ -1,7 +1,7 @@ -using System.Security.Claims; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; +using SimpleModule.Core.Extensions; namespace SimpleModule.Core.FeatureFlags; @@ -22,8 +22,8 @@ public static TBuilder RequireFeature(this TBuilder builder, string fe return await next(context); } - var userId = context.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier); - var roles = context.HttpContext.User.FindAll(ClaimTypes.Role).Select(c => c.Value); + var userId = context.HttpContext.User.GetUserId(); + var roles = context.HttpContext.User.GetRoles(); var isEnabled = await featureFlagService.IsEnabledAsync(featureName, userId, roles); diff --git a/framework/SimpleModule.Core/Inertia/InertiaMiddleware.cs b/framework/SimpleModule.Core/Inertia/InertiaMiddleware.cs index dd2a34b3..1d301eee 100644 --- a/framework/SimpleModule.Core/Inertia/InertiaMiddleware.cs +++ b/framework/SimpleModule.Core/Inertia/InertiaMiddleware.cs @@ -1,3 +1,4 @@ +using System.Linq; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; @@ -63,11 +64,29 @@ private static string GetVersion() // This changes on every recompile/publish, ensuring cache-busting without manual config. var entryAssembly = System.Reflection.Assembly.GetEntryAssembly() ?? typeof(InertiaMiddleware).Assembly; - var buildTime = File.GetLastWriteTimeUtc(entryAssembly.Location); - return buildTime.ToString( - "yyyyMMddHHmmss", - System.Globalization.CultureInfo.InvariantCulture - ); + + // Assembly.Location is empty for single-file–published apps; File.GetLastWriteTimeUtc("") + // would throw and abort all Inertia rendering (this runs in a static initializer). Fall + // back to the informational/assembly version so the app still starts. + var location = entryAssembly.Location; + if (!string.IsNullOrEmpty(location) && File.Exists(location)) + { + var buildTime = File.GetLastWriteTimeUtc(location); + return buildTime.ToString( + "yyyyMMddHHmmss", + System.Globalization.CultureInfo.InvariantCulture + ); + } + + var informationalVersion = entryAssembly + .GetCustomAttributes(typeof(System.Reflection.AssemblyInformationalVersionAttribute), false) + .OfType() + .FirstOrDefault() + ?.InformationalVersion; + + return !string.IsNullOrEmpty(informationalVersion) + ? informationalVersion + : entryAssembly.GetName().Version?.ToString() ?? "1.0.0"; } private static string GetEncodedUrl(this HttpRequest request) diff --git a/framework/SimpleModule.Core/Inertia/InertiaResult.cs b/framework/SimpleModule.Core/Inertia/InertiaResult.cs index 9e984462..7777dd82 100644 --- a/framework/SimpleModule.Core/Inertia/InertiaResult.cs +++ b/framework/SimpleModule.Core/Inertia/InertiaResult.cs @@ -1,3 +1,6 @@ +using System.Buffers; +using System.Linq; +using System.Text; using System.Text.Json; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Json; @@ -29,28 +32,18 @@ public async Task ExecuteAsync(HttpContext httpContext) { var options = GetSerializerOptions(httpContext); var sharedData = httpContext.RequestServices.GetService(); - var mergedProps = MergeProps(_props, sharedData, options); - - var pageData = new - { - component = _component, - props = mergedProps, - url = httpContext.Request.Path + httpContext.Request.QueryString, - version = InertiaMiddleware.Version, - }; + var url = httpContext.Request.Path + httpContext.Request.QueryString; + var pageJson = SerializePage(_component, _props, sharedData, url, options); if (httpContext.Request.IsInertia()) { httpContext.Response.Headers[InertiaHttpExtensions.InertiaHeader] = "true"; httpContext.Response.Headers["Vary"] = InertiaHttpExtensions.InertiaHeader; httpContext.Response.ContentType = "application/json"; - var json = JsonSerializer.Serialize(pageData, options); - await httpContext.Response.WriteAsync(json); + await httpContext.Response.WriteAsync(pageJson); return; } - var pageJson = JsonSerializer.Serialize(pageData, options); - var renderer = httpContext.RequestServices.GetRequiredService(); await renderer.RenderPageAsync(httpContext, pageJson); } @@ -84,36 +77,89 @@ private static JsonSerializerOptions GetSerializerOptions(HttpContext httpContex return fallback; } - private static object MergeProps( + /// + /// Serializes the Inertia page envelope in a single pass. When there is no shared + /// data, endpoint props stream straight to the output with no intermediate DOM. + /// When shared data is present, endpoint props must be materialized once (to know + /// their keys), then shared data and props are written into one object — endpoint + /// props keep priority (shared keys they define are skipped), so no key is emitted + /// twice. + /// + private static string SerializePage( + string component, object? props, InertiaSharedData? sharedData, + string url, JsonSerializerOptions options ) { - if (sharedData is null || sharedData.All.Count == 0) + var buffer = new ArrayBufferWriter(); + using (var writer = new Utf8JsonWriter(buffer)) { - return props ?? new { }; + writer.WriteStartObject(); + writer.WriteString("component", component); + + writer.WritePropertyName("props"); + if (sharedData is null || sharedData.All.Count == 0) + { + JsonSerializer.Serialize(writer, props ?? EmptyProps, options); + } + else + { + WriteMergedProps(writer, props, sharedData, options); + } + + writer.WriteString("url", url); + writer.WriteString("version", InertiaMiddleware.Version); + writer.WriteEndObject(); } - var result = new Dictionary(); + return Encoding.UTF8.GetString(buffer.WrittenSpan); + } - // Add shared data first (lower priority) + private static void WriteMergedProps( + Utf8JsonWriter writer, + object? props, + InertiaSharedData sharedData, + JsonSerializerOptions options + ) + { + // Endpoint props are materialized once so their top-level keys are known; + // this is unavoidable for a correct merge but happens a single time. + using var propsDoc = + props is null ? null : JsonSerializer.SerializeToDocument(props, options); + + var propKeys = + propsDoc is null + ? null + : new HashSet( + propsDoc.RootElement.EnumerateObject().Select(p => p.Name), + StringComparer.Ordinal + ); + + writer.WriteStartObject(); + + // Shared data (lower priority) — skip any key the endpoint props also define. foreach (var kvp in sharedData.All) { - result[kvp.Key] = kvp.Value; + if (propKeys is not null && propKeys.Contains(kvp.Key)) + continue; + + writer.WritePropertyName(kvp.Key); + JsonSerializer.Serialize(writer, kvp.Value, options); } - // Add endpoint props (higher priority — overwrites shared data) - // Use JSON round-trip to merge endpoint props into shared data - if (props is not null) + // Endpoint props (higher priority). + if (propsDoc is not null) { - var json = JsonSerializer.SerializeToElement(props, options); - foreach (var property in json.EnumerateObject()) + foreach (var property in propsDoc.RootElement.EnumerateObject()) { - result[property.Name] = property.Value; + property.WriteTo(writer); } } - return result; + writer.WriteEndObject(); } + + private static readonly object EmptyProps = new(); } diff --git a/framework/SimpleModule.Database/ITenantScopedDbContext.cs b/framework/SimpleModule.Database/ITenantScopedDbContext.cs new file mode 100644 index 00000000..01809a1a --- /dev/null +++ b/framework/SimpleModule.Database/ITenantScopedDbContext.cs @@ -0,0 +1,17 @@ +namespace SimpleModule.Database; + +/// +/// Implemented by a module DbContext that applies multi-tenant query filters via +/// . +/// +/// The filter references this property on the executing context instance, which EF +/// Core re-evaluates on every query. Expose the request's current tenant here (e.g. from an +/// injected scoped ) so each request is +/// filtered by its own tenant, rather than a value frozen into the cached model. +/// +/// +public interface ITenantScopedDbContext +{ + /// The tenant id for the current request, or null when no tenant is set. + string? CurrentTenantId { get; } +} diff --git a/framework/SimpleModule.Database/Interceptors/EntityChangeInterceptor.cs b/framework/SimpleModule.Database/Interceptors/EntityChangeInterceptor.cs index 38d0ebdd..c272df50 100644 --- a/framework/SimpleModule.Database/Interceptors/EntityChangeInterceptor.cs +++ b/framework/SimpleModule.Database/Interceptors/EntityChangeInterceptor.cs @@ -24,42 +24,41 @@ ILogger logger private List<(object Entity, EntityChangeType ChangeType)>? _capturedChanges; + // EF invokes the sync SavingChanges/SavedChanges for DbContext.SaveChanges() and the + // async pair for SaveChangesAsync(); it never cross-calls. Both paths must capture and + // dispatch or a sync save would silently skip all IEntityChangeHandler dispatch. + public override InterceptionResult SavingChanges( + DbContextEventData eventData, + InterceptionResult result + ) + { + CaptureChanges(eventData); + return base.SavingChanges(eventData, result); + } + public override ValueTask> SavingChangesAsync( DbContextEventData eventData, InterceptionResult result, CancellationToken cancellationToken = default ) { - if (eventData.Context is not null) - { - var changes = new List<(object Entity, EntityChangeType ChangeType)>(); + CaptureChanges(eventData); + return base.SavingChangesAsync(eventData, result, cancellationToken); + } - foreach (var entry in eventData.Context.ChangeTracker.Entries()) - { - if ( - entry.State - is not (EntityState.Added or EntityState.Modified or EntityState.Deleted) - ) - continue; - - var changeType = entry.State switch - { - EntityState.Added => EntityChangeType.Created, - EntityState.Deleted => EntityChangeType.Deleted, - EntityState.Modified - when entry.Entity is ISoftDelete { IsDeleted: true } - && entry.Property(nameof(ISoftDelete.IsDeleted)).IsModified => - EntityChangeType.Deleted, - _ => EntityChangeType.Updated, - }; - - changes.Add((entry.Entity, changeType)); - } + public override int SavedChanges(SaveChangesCompletedEventData eventData, int result) + { + var changes = _capturedChanges; + _capturedChanges = null; - _capturedChanges = changes.Count > 0 ? changes : null; + // ASP.NET Core has no SynchronizationContext, so blocking here does not deadlock. + // The sync save path is rare; async callers use the awaited path below. + if (changes is { Count: > 0 }) + { + DispatchAllAsync(changes, CancellationToken.None).GetAwaiter().GetResult(); } - return base.SavingChangesAsync(eventData, result, cancellationToken); + return base.SavedChanges(eventData, result); } public override async ValueTask SavedChangesAsync( @@ -73,15 +72,18 @@ public override async ValueTask SavedChangesAsync( if (changes is { Count: > 0 }) { - foreach (var (entity, changeType) in changes) - { - await DispatchToHandlersAsync(entity, changeType, cancellationToken); - } + await DispatchAllAsync(changes, cancellationToken); } return await base.SavedChangesAsync(eventData, result, cancellationToken); } + public override void SaveChangesFailed(DbContextErrorEventData eventData) + { + _capturedChanges = null; + base.SaveChangesFailed(eventData); + } + public override Task SaveChangesFailedAsync( DbContextErrorEventData eventData, CancellationToken cancellationToken = default @@ -91,6 +93,49 @@ public override Task SaveChangesFailedAsync( return base.SaveChangesFailedAsync(eventData, cancellationToken); } + private void CaptureChanges(DbContextEventData eventData) + { + if (eventData.Context is null) + return; + + var changes = new List<(object Entity, EntityChangeType ChangeType)>(); + + foreach (var entry in eventData.Context.ChangeTracker.Entries()) + { + if ( + entry.State + is not (EntityState.Added or EntityState.Modified or EntityState.Deleted) + ) + continue; + + var changeType = entry.State switch + { + EntityState.Added => EntityChangeType.Created, + EntityState.Deleted => EntityChangeType.Deleted, + EntityState.Modified + when entry.Entity is ISoftDelete { IsDeleted: true } + && entry.Property(nameof(ISoftDelete.IsDeleted)).IsModified => + EntityChangeType.Deleted, + _ => EntityChangeType.Updated, + }; + + changes.Add((entry.Entity, changeType)); + } + + _capturedChanges = changes.Count > 0 ? changes : null; + } + + private async Task DispatchAllAsync( + List<(object Entity, EntityChangeType ChangeType)> changes, + CancellationToken cancellationToken + ) + { + foreach (var (entity, changeType) in changes) + { + await DispatchToHandlersAsync(entity, changeType, cancellationToken); + } + } + private async Task DispatchToHandlersAsync( object entity, EntityChangeType changeType, diff --git a/framework/SimpleModule.Database/Interceptors/EntityInterceptor.cs b/framework/SimpleModule.Database/Interceptors/EntityInterceptor.cs index 4d8ad0ef..d646210f 100644 --- a/framework/SimpleModule.Database/Interceptors/EntityInterceptor.cs +++ b/framework/SimpleModule.Database/Interceptors/EntityInterceptor.cs @@ -1,9 +1,9 @@ -using System.Security.Claims; using Microsoft.AspNetCore.Http; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.ChangeTracking; using Microsoft.EntityFrameworkCore.Diagnostics; using SimpleModule.Core.Entities; +using SimpleModule.Core.Extensions; using SimpleModule.Database.SoftDelete; namespace SimpleModule.Database.Interceptors; @@ -25,18 +25,35 @@ public sealed class EntityInterceptor( ITenantContext? tenantContext = null ) : SaveChangesInterceptor { + // EF calls the sync SavingChanges for DbContext.SaveChanges() and the async + // SavingChangesAsync for SaveChangesAsync(); it never cross-calls. Both must run + // the same stamping logic or a sync save would hard-delete soft-delete entities + // and skip audit/tenant/concurrency fields silently. + public override InterceptionResult SavingChanges( + DbContextEventData eventData, + InterceptionResult result + ) + { + ApplyChanges(eventData); + return base.SavingChanges(eventData, result); + } + public override ValueTask> SavingChangesAsync( DbContextEventData eventData, InterceptionResult result, CancellationToken cancellationToken = default ) + { + ApplyChanges(eventData); + return base.SavingChangesAsync(eventData, result, cancellationToken); + } + + private void ApplyChanges(DbContextEventData eventData) { if (eventData.Context is null) - return base.SavingChangesAsync(eventData, result, cancellationToken); + return; - var userId = httpContextAccessor.HttpContext?.User?.FindFirstValue( - ClaimTypes.NameIdentifier - ); + var userId = httpContextAccessor.HttpContext?.User?.GetUserId(); var now = DateTimeOffset.UtcNow; foreach (var entry in eventData.Context.ChangeTracker.Entries()) @@ -82,8 +99,6 @@ is not (EntityState.Added or EntityState.Modified or EntityState.Deleted) break; } } - - return base.SavingChangesAsync(eventData, result, cancellationToken); } private static void SetCreationFields(EntityEntry entry, DateTimeOffset now, string? userId) diff --git a/framework/SimpleModule.Database/ModuleModelBuilderExtensions.cs b/framework/SimpleModule.Database/ModuleModelBuilderExtensions.cs index eb4f6402..7b95906d 100644 --- a/framework/SimpleModule.Database/ModuleModelBuilderExtensions.cs +++ b/framework/SimpleModule.Database/ModuleModelBuilderExtensions.cs @@ -156,25 +156,42 @@ tableName is not null /// /// Applies multi-tenant query filters to all entities. - /// Call this from your DbContext's OnModelCreating after . + /// Call this from your DbContext's OnModelCreating after , + /// passing the context itself (this). /// - /// The filter expression closes over the field reference, - /// so EF Core evaluates the current tenant ID at query time (parameterized). + /// The filter references on the + /// context instance. EF Core caches the model per context type, but re-evaluates a + /// reference to the executing context on every query — so each request is filtered by its own + /// tenant. (A previous version captured the instance as a constant, + /// which froze the first request's tenant into the cached model and leaked rows across tenants.) + /// + /// + /// When is null (no tenant resolved), + /// the filter matches only rows whose TenantId is also null. Store null on + /// un-tenanted/global rows if they should be visible without a tenant; rows with a non-null + /// tenant are hidden. /// /// /// - /// protected override void OnModelCreating(ModelBuilder modelBuilder) + /// public sealed class ProductsDbContext(DbContextOptions<ProductsDbContext> options, + /// ITenantContext tenant) : DbContext(options), ITenantScopedDbContext /// { - /// modelBuilder.ApplyModuleSchema("Products", dbOptions.Value); - /// modelBuilder.ApplyMultiTenantFilters(tenantContext); + /// public string? CurrentTenantId => tenant.TenantId; + /// + /// protected override void OnModelCreating(ModelBuilder modelBuilder) + /// { + /// modelBuilder.ApplyModuleSchema("Products", dbOptions.Value); + /// modelBuilder.ApplyMultiTenantFilters(this); + /// } /// } /// /// /// - public static void ApplyMultiTenantFilters( + public static void ApplyMultiTenantFilters( this ModelBuilder modelBuilder, - ITenantContext tenantContext + TContext context ) + where TContext : DbContext, ITenantScopedDbContext { foreach (var entityType in modelBuilder.Model.GetEntityTypes()) { @@ -183,19 +200,16 @@ ITenantContext tenantContext var parameter = Expression.Parameter(entityType.ClrType, "e"); var tenantIdProperty = Expression.Property(parameter, nameof(IMultiTenant.TenantId)); - var tenantContextExpr = Expression.Constant(tenantContext); + + // Reference the current tenant via the executing DbContext instance so EF + // re-evaluates it per query instead of freezing a captured instance. var currentTenantId = Expression.Property( - tenantContextExpr, - nameof(ITenantContext.TenantId) + Expression.Constant(context), + nameof(ITenantScopedDbContext.CurrentTenantId) ); - // Handle null tenant: when no tenant is set, the filter becomes e.TenantId == null - // which effectively returns no rows (TenantId is non-nullable string). var filter = Expression.Lambda( - Expression.Equal( - tenantIdProperty, - Expression.Coalesce(currentTenantId, Expression.Constant("")) - ), + Expression.Equal(tenantIdProperty, currentTenantId), parameter ); diff --git a/framework/SimpleModule.Database/SoftDelete/SoftDeleteService.cs b/framework/SimpleModule.Database/SoftDelete/SoftDeleteService.cs index c7cbd0e1..8dc4dc4d 100644 --- a/framework/SimpleModule.Database/SoftDelete/SoftDeleteService.cs +++ b/framework/SimpleModule.Database/SoftDelete/SoftDeleteService.cs @@ -202,9 +202,45 @@ private static Array CoerceIds(IEnumerable ids, Type keyType) private static object CoerceId(object id, Type keyType) { ArgumentNullException.ThrowIfNull(id); - return id.GetType() == keyType - ? id - : Convert.ChangeType(id, keyType, System.Globalization.CultureInfo.InvariantCulture); + + if (keyType.IsInstanceOfType(id)) + { + return id; + } + + var targetType = Nullable.GetUnderlyingType(keyType) ?? keyType; + var culture = System.Globalization.CultureInfo.InvariantCulture; + + // Guid does not implement IConvertible, so Convert.ChangeType throws for it — + // the common "string route value for a Guid PK" case. Parse it explicitly. + if (targetType == typeof(Guid)) + { + return id is Guid guid ? guid : Guid.Parse(Convert.ToString(id, culture)!); + } + + // Primitive/IConvertible keys (int, long, string, ...). + if (id is IConvertible) + { + try + { + return Convert.ChangeType(id, targetType, culture); + } + catch (InvalidCastException) + { + // Fall through to a TypeConverter (covers Vogen value-object keys, which + // Convert.ChangeType can't handle but usually expose a TypeConverter). + } + } + + var converter = System.ComponentModel.TypeDescriptor.GetConverter(targetType); + if (converter.CanConvertFrom(id.GetType())) + { + return converter.ConvertFrom(null, culture, id)!; + } + + throw new InvalidOperationException( + $"Cannot convert id of type '{id.GetType()}' to key type '{keyType}'." + ); } private sealed record KeyMetadata(string KeyName, Type KeyType) diff --git a/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs b/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs index ac78c6e3..2e0d3cd7 100644 --- a/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs +++ b/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs @@ -77,6 +77,7 @@ string hostAssemblyName d.FullyQualifiedName, d.SafeName, d.BaseTypeFqn, + d.HasParameterlessConstructor, d.Properties.Select(p => new DtoPropertyInfoRecord( p.Name, p.TypeFqn, diff --git a/framework/SimpleModule.Generator/Discovery/Finders/DtoFinder.cs b/framework/SimpleModule.Generator/Discovery/Finders/DtoFinder.cs index 2e8cbbae..ec82496a 100644 --- a/framework/SimpleModule.Generator/Discovery/Finders/DtoFinder.cs +++ b/framework/SimpleModule.Generator/Discovery/Finders/DtoFinder.cs @@ -69,6 +69,9 @@ and var baseType FullyQualifiedName = fqn, SafeName = safeName, BaseTypeFqn = baseTypeFqn, + HasParameterlessConstructor = HasPublicParameterlessConstructor( + typeSymbol + ), Properties = DtoPropertyExtractor.Extract(typeSymbol), } ); @@ -202,6 +205,9 @@ and var baseType FullyQualifiedName = fqn, SafeName = safeName, BaseTypeFqn = baseTypeFqn, + HasParameterlessConstructor = HasPublicParameterlessConstructor( + typeSymbol + ), Properties = DtoPropertyExtractor.Extract(typeSymbol), } ); @@ -209,6 +215,24 @@ and var baseType } } + /// + /// True when new T() is valid for the type: non-abstract with a public + /// parameterless instance constructor. Positional records and ctor-only types + /// return false so the JSON resolver skips CreateObject for them. + /// + private static bool HasPublicParameterlessConstructor(INamedTypeSymbol typeSymbol) + { + if (typeSymbol.IsAbstract) + return false; + + foreach (var ctor in typeSymbol.InstanceConstructors) + { + if (ctor.Parameters.Length == 0 && ctor.DeclaredAccessibility == Accessibility.Public) + return true; + } + return false; + } + /// /// Scans every referenced assembly AND the host assembly for types decorated /// with [Dto]. No-op when the DtoAttribute symbol isn't resolvable. diff --git a/framework/SimpleModule.Generator/Discovery/Finders/DtoPropertyExtractor.cs b/framework/SimpleModule.Generator/Discovery/Finders/DtoPropertyExtractor.cs index e20ae4f3..c054eb67 100644 --- a/framework/SimpleModule.Generator/Discovery/Finders/DtoPropertyExtractor.cs +++ b/framework/SimpleModule.Generator/Discovery/Finders/DtoPropertyExtractor.cs @@ -44,9 +44,13 @@ m is IPropertySymbol prop Name = prop.Name, TypeFqn = actualType, UnderlyingTypeFqn = resolvedType != actualType ? resolvedType : null, + // init-only setters (get; init;) cannot be assigned outside a + // constructor/initializer, so treat them as not settable — + // emitting a Set delegate for them would not compile (CS8852). HasSetter = prop.SetMethod is not null - && prop.SetMethod.DeclaredAccessibility == Accessibility.Public, + && prop.SetMethod.DeclaredAccessibility == Accessibility.Public + && !prop.SetMethod.IsInitOnly, } ); } diff --git a/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs b/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs index 08e0848c..50fd6d10 100644 --- a/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs +++ b/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs @@ -7,6 +7,7 @@ internal readonly record struct DtoTypeInfoRecord( string FullyQualifiedName, string SafeName, string? BaseTypeFqn, + bool HasParameterlessConstructor, ImmutableArray Properties ) { @@ -15,6 +16,7 @@ public bool Equals(DtoTypeInfoRecord other) return FullyQualifiedName == other.FullyQualifiedName && SafeName == other.SafeName && BaseTypeFqn == other.BaseTypeFqn + && HasParameterlessConstructor == other.HasParameterlessConstructor && Properties.SequenceEqual(other.Properties); } @@ -24,6 +26,7 @@ public override int GetHashCode() hash = HashHelper.Combine(hash, FullyQualifiedName.GetHashCode()); hash = HashHelper.Combine(hash, SafeName.GetHashCode()); hash = HashHelper.Combine(hash, BaseTypeFqn?.GetHashCode() ?? 0); + hash = HashHelper.Combine(hash, HasParameterlessConstructor.GetHashCode()); hash = HashHelper.HashArray(hash, Properties); return hash; } diff --git a/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs b/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs index c58705c6..cdfc7366 100644 --- a/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs +++ b/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs @@ -49,6 +49,13 @@ internal sealed class DtoTypeInfo public string FullyQualifiedName { get; set; } = ""; public string SafeName { get; set; } = ""; public string? BaseTypeFqn { get; set; } + + /// + /// True when the type is non-abstract and has a public parameterless constructor. + /// Gates the generated info.CreateObject = () => new T() — emitting it + /// for e.g. positional records without one would not compile (CS7036). + /// + public bool HasParameterlessConstructor { get; set; } public List Properties { get; set; } = new(); } diff --git a/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs b/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs index 529ac889..fb5876eb 100644 --- a/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs +++ b/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs @@ -7,6 +7,54 @@ namespace SimpleModule.Generator; internal static class SymbolDiscovery { + // Assembly-name prefixes that can never contain SimpleModule modules, DTOs, + // or form requests (BCL, ASP.NET Core, EF Core, common third-party packages). + // Skipping them keeps the recursive GlobalNamespace/GetAttributes walks in + // ModuleFinder, DtoFinder, and FormRequestFinder off those huge symbol trees. + private static readonly string[] s_skippedAssemblyPrefixes = + { + "System.", + "Microsoft.", + "netstandard", + "mscorlib", + "WindowsBase", + "Newtonsoft.", + "Swashbuckle.", + "FluentValidation", + "Wolverine", + "Npgsql", + "OpenIddict", + "Serilog", + "Polly", + "Scalar", + "Vogen", + }; + + /// + /// Returns true for framework/third-party assemblies that discovery can skip. + /// Module and contracts assemblies (including the framework's own + /// SimpleModule.* assemblies) are never skipped, whatever their name. + /// + private static bool IsSkippedAssembly(string name) + { + if ( + name.EndsWith(".Contracts", StringComparison.OrdinalIgnoreCase) + || name.IndexOf("SimpleModule", StringComparison.Ordinal) >= 0 + ) + return false; + + if (name == "System" || name == "netstandard" || name == "mscorlib") + return true; + + foreach (var prefix in s_skippedAssemblyPrefixes) + { + if (name.StartsWith(prefix, StringComparison.Ordinal)) + return true; + } + + return false; + } + internal static DiscoveryData Extract( Compilation compilation, CancellationToken cancellationToken @@ -31,6 +79,9 @@ CancellationToken cancellationToken if (compilation.GetAssemblyOrModuleSymbol(reference) is not IAssemblySymbol asm) continue; + if (IsSkippedAssembly(asm.Name)) + continue; + refAssemblies.Add(asm); if (asm.Name.EndsWith(".Contracts", StringComparison.OrdinalIgnoreCase)) contractsAssemblies.Add(asm); diff --git a/framework/SimpleModule.Generator/Discovery/SymbolHelpers.cs b/framework/SimpleModule.Generator/Discovery/SymbolHelpers.cs index 34cbb251..45ac495a 100644 --- a/framework/SimpleModule.Generator/Discovery/SymbolHelpers.cs +++ b/framework/SimpleModule.Generator/Discovery/SymbolHelpers.cs @@ -128,7 +128,16 @@ internal static string FindClosestModuleNameFast(string typeFqn, ModuleNamespace { foreach (var (ns, moduleName) in index.Entries) { - if (typeFqn.StartsWith(ns, StringComparison.Ordinal)) + // A module in the global namespace (empty ns) matches everything. + if (ns.Length == 0) + return moduleName; + + // Match only on a namespace boundary: "App.Order" must not claim + // types in "App.OrderArchive" — require exact match or "ns." prefix. + if ( + typeFqn.StartsWith(ns, StringComparison.Ordinal) + && (typeFqn.Length == ns.Length || typeFqn[ns.Length] == '.') + ) return moduleName; } return index.FirstModuleName; diff --git a/framework/SimpleModule.Generator/Emitters/JsonResolverEmitter.cs b/framework/SimpleModule.Generator/Emitters/JsonResolverEmitter.cs index 73340bee..c4d8c6ba 100644 --- a/framework/SimpleModule.Generator/Emitters/JsonResolverEmitter.cs +++ b/framework/SimpleModule.Generator/Emitters/JsonResolverEmitter.cs @@ -52,9 +52,15 @@ public void Emit(SourceProductionContext context, DiscoveryData data) sb.AppendLine( $" var info = JsonTypeInfo.CreateJsonTypeInfo<{dto.FullyQualifiedName}>(options);" ); - sb.AppendLine( - $" info.CreateObject = static () => new {dto.FullyQualifiedName}();" - ); + // Only types with an accessible parameterless ctor can be constructed by STJ. + // Positional records etc. stay serializable; deserialization simply has no + // CreateObject (STJ reports it at runtime instead of failing the build). + if (dto.HasParameterlessConstructor) + { + sb.AppendLine( + $" info.CreateObject = static () => new {dto.FullyQualifiedName}();" + ); + } foreach (var prop in dto.Properties) { diff --git a/framework/SimpleModule.Generator/Emitters/ViewPagesEmitter.cs b/framework/SimpleModule.Generator/Emitters/ViewPagesEmitter.cs index c4c72a47..d33ce27b 100644 --- a/framework/SimpleModule.Generator/Emitters/ViewPagesEmitter.cs +++ b/framework/SimpleModule.Generator/Emitters/ViewPagesEmitter.cs @@ -1,4 +1,3 @@ -using System; using System.Text; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Text; @@ -14,11 +13,10 @@ public void Emit(SourceProductionContext context, DiscoveryData data) if (module.Views.Length == 0) continue; - // Extract module name from FQN (e.g., "global::SimpleModule.Products.ProductsModule" -> "Products") + // Derive the hint name from the module's full FQN so two module classes + // with the same simple name in different namespaces never collide in AddSource. var fqn = TypeMappingHelpers.StripGlobalPrefix(module.FullyQualifiedName); - var moduleName = fqn.Contains(".") ? fqn.Substring(fqn.LastIndexOf('.') + 1) : fqn; - if (moduleName.EndsWith("Module", StringComparison.Ordinal)) - moduleName = moduleName.Substring(0, moduleName.Length - "Module".Length); + var hintName = fqn.Replace('.', '_').Replace('+', '_'); var sb = new StringBuilder(); sb.AppendLine("// "); @@ -43,7 +41,7 @@ public void Emit(SourceProductionContext context, DiscoveryData data) sb.AppendLine("#endif"); context.AddSource( - $"ViewPages_{moduleName}.g.cs", + $"ViewPages_{hintName}.g.cs", SourceText.From(sb.ToString(), Encoding.UTF8) ); } diff --git a/framework/SimpleModule.Hosting/Middleware/InertiaLayoutDataMiddleware.cs b/framework/SimpleModule.Hosting/Middleware/InertiaLayoutDataMiddleware.cs index f98dc66b..2185c75c 100644 --- a/framework/SimpleModule.Hosting/Middleware/InertiaLayoutDataMiddleware.cs +++ b/framework/SimpleModule.Hosting/Middleware/InertiaLayoutDataMiddleware.cs @@ -1,3 +1,4 @@ +using System.Net.Mime; using Microsoft.AspNetCore.Antiforgery; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; @@ -12,7 +13,13 @@ public sealed class InertiaLayoutDataMiddleware(RequestDelegate next) public async Task InvokeAsync(HttpContext context) { var sharedData = context.RequestServices.GetService(); - if (sharedData is not null) + + // Only requests that will actually render an Inertia page consume this shared + // data. Populating it eagerly for every request runs antiforgery token + // generation (DataProtection crypto) and menu filtering for static assets, + // health probes, and JSON API calls — all wasted. An Inertia navigation carries + // the X-Inertia header; a full-page browser load sends Accept: text/html. + if (sharedData is not null && ShouldPopulateLayoutData(context.Request)) { var user = context.User; var isAuthenticated = user.Identity?.IsAuthenticated == true; @@ -86,4 +93,34 @@ m.RequiredPermission is null await next(context); } + + private static bool ShouldPopulateLayoutData(HttpRequest request) + { + // Inertia AJAX navigations (any method) consume the shared data on re-render. + if (request.IsInertia()) + { + return true; + } + + // Full-page loads are navigational GETs that accept HTML. Anything else + // (asset fetches, health probes, JSON APIs, form POSTs without X-Inertia) + // never reaches an Inertia.Render, so skip the work. + if (!HttpMethods.IsGet(request.Method)) + { + return false; + } + + foreach (var accept in request.Headers.Accept) + { + if ( + accept is not null + && accept.Contains(MediaTypeNames.Text.Html, StringComparison.OrdinalIgnoreCase) + ) + { + return true; + } + } + + return false; + } } diff --git a/framework/SimpleModule.Hosting/Middleware/MaintenanceModeMiddleware.cs b/framework/SimpleModule.Hosting/Middleware/MaintenanceModeMiddleware.cs index 97d334b1..bcee5b32 100644 --- a/framework/SimpleModule.Hosting/Middleware/MaintenanceModeMiddleware.cs +++ b/framework/SimpleModule.Hosting/Middleware/MaintenanceModeMiddleware.cs @@ -65,6 +65,10 @@ public async Task InvokeAsync(HttpContext context) await WriteMaintenanceResponseAsync(context, state); } + // Static-asset roots that must keep loading during maintenance so the 503 page + // (or a cached app shell) can still fetch its CSS/JS/favicon. + private static readonly string[] StaticAssetPrefixes = ["/_content/", "/js/", "/css/"]; + private static bool IsExempt(HttpContext context) { var path = context.Request.Path.Value; @@ -73,8 +77,23 @@ private static bool IsExempt(HttpContext context) return false; } - return path.StartsWith(RouteConstants.HealthLive, StringComparison.OrdinalIgnoreCase) - || path.StartsWith(RouteConstants.HealthReady, StringComparison.OrdinalIgnoreCase); + if ( + path.StartsWith(RouteConstants.HealthLive, StringComparison.OrdinalIgnoreCase) + || path.StartsWith(RouteConstants.HealthReady, StringComparison.OrdinalIgnoreCase) + ) + { + return true; + } + + foreach (var prefix in StaticAssetPrefixes) + { + if (path.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } private bool TryConsumeBypassQuery(HttpContext context, MaintenanceState state) diff --git a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.Helpers.cs b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.Helpers.cs index ef12d9b7..df1d7a1c 100644 --- a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.Helpers.cs +++ b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.Helpers.cs @@ -16,6 +16,18 @@ public static partial class SimpleModuleHostExtensions private const string ModuleContentPathPrefix = "/_content/"; private const string ModuleScriptExtension = ".mjs"; + // Path prefixes that only ever serve immutable, versioned static assets. The + // "?v=" cache-buster is appended exclusively to URLs under these prefixes + // (module CSS/JS, the JS bundle), so the immutable cache header must be scoped + // to them — a "?v=" on any other path (e.g. a rendered page at /dashboard?v=1) + // must NOT be marked publicly cacheable. + private static readonly string[] StaticAssetPathPrefixes = + [ + ModuleContentPathPrefix, + "/js/", + "/css/", + ]; + /// /// Reads a config list that may be expressed either as a JSON/indexed array /// (ForwardedHeaders:KnownProxies:0) or as a single comma-separated @@ -102,7 +114,9 @@ private static void UseStaticFileCaching(WebApplication app) return; } - bool hasVersionParam = context.Request.Query.ContainsKey("v"); + bool isStaticAssetPath = IsStaticAssetPath(path); + bool hasVersionParam = + isStaticAssetPath && context.Request.Query.ContainsKey("v"); bool isVendorJs = path.StartsWith( VendorJsPathPrefix, StringComparison.OrdinalIgnoreCase @@ -126,6 +140,19 @@ private static void UseStaticFileCaching(WebApplication app) ); } + private static bool IsStaticAssetPath(string path) + { + foreach (var prefix in StaticAssetPathPrefixes) + { + if (path.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; + } + private static void UseHomePageRewrite(WebApplication app) { app.Use( diff --git a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs index 40208852..a17bc504 100644 --- a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs +++ b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs @@ -350,9 +350,11 @@ public static async Task UseSimpleModuleInfrastructure(this WebApplication app) // files are intentionally public. app.MapStaticAssets().AllowAnonymous(); - // Maintenance gate runs after static assets (so the 503 page can load - // its CSS) but before auth (so anonymous users get 503 rather than a - // login redirect). Health probes are exempt inside the middleware. + // Maintenance gate runs before auth (so anonymous users get 503 rather + // than a login redirect). Static assets are endpoint-routed and execute at + // the end of the pipeline, so this plain middleware runs before them — + // health probes and static-asset paths are exempted inside the middleware + // so error pages / cached shells can still load their assets during maintenance. app.UseMiddleware(); app.UseAuthentication(); diff --git a/framework/SimpleModule.Storage.Local/LocalStorageProvider.cs b/framework/SimpleModule.Storage.Local/LocalStorageProvider.cs index cc0155b7..47ee454e 100644 --- a/framework/SimpleModule.Storage.Local/LocalStorageProvider.cs +++ b/framework/SimpleModule.Storage.Local/LocalStorageProvider.cs @@ -79,10 +79,7 @@ public Task> ListAsync( CancellationToken cancellationToken = default ) { - var normalized = StoragePathHelper.Normalize(prefix); - var fullPath = string.IsNullOrEmpty(normalized) - ? _basePath - : Path.Combine(_basePath, normalized.Replace('/', Path.DirectorySeparatorChar)); + var fullPath = GetFullPath(StoragePathHelper.Normalize(prefix)); if (!Directory.Exists(fullPath)) { @@ -126,16 +123,40 @@ public Task> ListAsync( return Task.FromResult>(entries); } + // File systems are case-insensitive on Windows but case-sensitive on Linux/macOS; + // matching that here keeps the containment check from both over- and under-rejecting. + private static readonly StringComparison PathComparison = OperatingSystem.IsWindows() + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + private string GetFullPath(string normalizedPath) { var localPath = normalizedPath.Replace('/', Path.DirectorySeparatorChar); var fullPath = Path.GetFullPath(Path.Combine(_basePath, localPath)); - if (!fullPath.StartsWith(_basePath, StringComparison.OrdinalIgnoreCase)) + if (!IsWithinBase(fullPath)) { throw new InvalidOperationException("Path traversal detected."); } return fullPath; } + + private bool IsWithinBase(string fullPath) + { + // Exact-root match is allowed (e.g. listing the storage root itself). + if (string.Equals(fullPath, _basePath, PathComparison)) + { + return true; + } + + // Otherwise require a directory-separator boundary so a sibling directory that + // merely shares the root's name prefix (e.g. "/app/storage-backup" vs + // "/app/storage") cannot escape the root. + var baseWithSeparator = _basePath.EndsWith(Path.DirectorySeparatorChar) + ? _basePath + : _basePath + Path.DirectorySeparatorChar; + + return fullPath.StartsWith(baseWithSeparator, PathComparison); + } } diff --git a/packages/SimpleModule.Client/src/vite-plugin-vendor.ts b/packages/SimpleModule.Client/src/vite-plugin-vendor.ts index d7f77d81..44beab3f 100644 --- a/packages/SimpleModule.Client/src/vite-plugin-vendor.ts +++ b/packages/SimpleModule.Client/src/vite-plugin-vendor.ts @@ -37,6 +37,28 @@ function getExportNames(require_: NodeRequire, pkg: string): string[] { } } +// The top-level package name for a (possibly subpath) specifier: +// 'react/jsx-runtime' -> 'react', '@inertiajs/react' -> '@inertiajs/react'. +function rootPackage(pkg: string): string { + const segments = pkg.split('/'); + return pkg.startsWith('@') ? segments.slice(0, 2).join('/') : segments[0]; +} + +function resolveVersion(require_: NodeRequire, pkg: string): string { + try { + return (require_(`${rootPackage(pkg)}/package.json`) as { version?: string }).version ?? '0'; + } catch { + return '0'; + } +} + +// Fingerprint of the resolved versions of every vendored package. Written next to the +// bundles so a version bump (e.g. React upgrade) forces a rebuild instead of reusing +// stale copies that were compiled against the previous versions. +function versionFingerprint(require_: NodeRequire, vendors: VendorEntry[]): string { + return vendors.map((v) => `${v.pkg}@${resolveVersion(require_, v.pkg)}`).join('\n'); +} + export function vendorBuildPlugin(options?: { vendors?: VendorEntry[]; outDir: string }): Plugin { const vendors = options?.vendors ?? defaultVendors; @@ -46,8 +68,17 @@ export function vendorBuildPlugin(options?: { vendors?: VendorEntry[]; outDir: s async buildStart() { const outDir = options?.outDir ?? path.resolve(process.cwd(), '../wwwroot/js/vendor'); const require_ = createRequire(import.meta.url); + const manifestPath = path.join(outDir, '.vendor-manifest'); + const fingerprint = versionFingerprint(require_, vendors); + + // Reuse existing bundles only when every output is present AND the vendored + // package versions still match. Otherwise a version bump (e.g. React upgrade) + // would silently keep stale copies compiled against the previous versions. + const outputsExist = vendors.every((v) => existsSync(path.join(outDir, `${v.file}.js`))); + const manifestMatches = + existsSync(manifestPath) && readFileSync(manifestPath, 'utf-8') === fingerprint; + if (outputsExist && manifestMatches) return; - if (vendors.every((v) => existsSync(path.join(outDir, `${v.file}.js`)))) return; mkdirSync(outDir, { recursive: true }); for (const v of vendors) { @@ -95,6 +126,10 @@ export function vendorBuildPlugin(options?: { vendors?: VendorEntry[]; outDir: s writeFileSync(outfile, code); } + + // Record the versions these bundles were built from so the next build can + // detect a package bump and rebuild instead of reusing stale copies. + writeFileSync(manifestPath, fingerprint); }, }; } diff --git a/scripts/add-component.mjs b/scripts/add-component.mjs index 16c2e050..923e24e0 100644 --- a/scripts/add-component.mjs +++ b/scripts/add-component.mjs @@ -7,7 +7,11 @@ import { fileURLToPath } from 'node:url'; const __dirname = dirname(fileURLToPath(import.meta.url)); const ROOT = resolve(__dirname, '..'); -const UI_DIR = resolve(ROOT, 'src/SimpleModule.UI'); +// This framework repo keeps the UI package under packages/; downstream scaffolded +// apps keep it under src/. Pick whichever exists so `ui:add` works in both. +const UI_DIR = [resolve(ROOT, 'packages/SimpleModule.UI'), resolve(ROOT, 'src/SimpleModule.UI')].find( + (dir) => existsSync(dir), +) ?? resolve(ROOT, 'packages/SimpleModule.UI'); const REGISTRY_PATH = resolve(UI_DIR, 'registry/registry.json'); const TEMPLATES_DIR = resolve(UI_DIR, 'registry/templates'); const COMPONENTS_DIR = resolve(UI_DIR, 'components'); diff --git a/scripts/typecheck.mjs b/scripts/typecheck.mjs index 52391ad5..1c4bfc01 100644 --- a/scripts/typecheck.mjs +++ b/scripts/typecheck.mjs @@ -5,7 +5,7 @@ * * Runs `tsc --noEmit` in every module and package that has a tsconfig.json, * plus the Host ClientApp. Each project is checked independently so @/* path - * aliases resolve correctly. All checks run in parallel for speed. + * aliases resolve correctly. Checks run in parallel (bounded concurrency) for speed. * * Exit codes: * 0 = All projects pass type checking @@ -15,8 +15,26 @@ import { spawn } from 'node:child_process'; import { createRequire } from 'node:module'; import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; +// Cap concurrent tsc processes so a machine with ~25 projects doesn't spawn 25 +// type-checkers at once and spike memory on constrained CI runners. +const MAX_CONCURRENCY = Math.max(1, Math.min(4, os.availableParallelism?.() ?? os.cpus().length)); + +async function mapWithConcurrency(items, limit, fn) { + const results = new Array(items.length); + let next = 0; + async function worker() { + while (next < items.length) { + const index = next++; + results[index] = await fn(items[index]); + } + } + await Promise.all(Array.from({ length: Math.min(limit, items.length) }, worker)); + return results; +} + const projectRoot = path.resolve(import.meta.dirname, '..'); const modulesDir = path.join(projectRoot, 'modules'); const packagesDir = path.join(projectRoot, 'packages'); @@ -90,7 +108,7 @@ const projects = [ clientAppDir, ]; -const results = await Promise.all(projects.map(checkProject)); +const results = await mapWithConcurrency(projects, MAX_CONCURRENCY, checkProject); const failures = []; for (const { dir, code, output } of results) { diff --git a/tasks/todo.md b/tasks/todo.md index b6e7c32b..897d0391 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,79 +1,79 @@ -# Fix critical issues from framework review (2026-06-09) +# Framework review fixes (2026-07-16) -## Critical 1 — page-registry guard broken on both ends -- [x] Fix `validate-pages.mjs` path: scan `modules/*/src/*/` (real layout is `src/SimpleModule.`), skip `obj`/`bin` -- [x] Add self-check: fail when zero C# files or zero view endpoints are found repo-wide (path drift can never silently disable the guard again) -- [x] Fix `app.tsx:232` `showErrorToast(...)` → `showToast({ variant: 'error', ... })` (ReferenceError on failed page load) -- [x] Add `ClientApp/tsconfig.json` and include ClientApp in `scripts/typecheck.mjs` +Fixes from the July 2026 5-area framework review. The review initially ran against a +stale local main (12 behind origin/main); every item below was re-verified against +origin/main. Already fixed upstream (dropped): showErrorToast, validate-pages path, +ClientApp typecheck, ForwardedHeaders config, download-link click guard. -## Critical 2 — default deployment one request from admin token -- [x] `UserSeedService`: fail fast in non-Development when `Seed:AdminPassword` unset; never seed test user with default password outside Development -- [x] OpenIddict: refuse `AllowPasswordFlow` in Production; fail fast on ephemeral signing/encryption keys in Production -- [x] `SimpleModuleHostExtensions`: stop clearing `KnownProxies`/`KnownIPNetworks` unconditionally — config-driven (`ForwardedHeaders:KnownProxies`/`KnownNetworks`/`TrustAllProxies`) -- [x] `docker-compose.yml`: explicit `OpenIddict__AllowPasswordGrant: "false"`, required seed-password env vars -- [x] Enforce `FileStorageModuleOptions` (MaxFileSizeMb / AllowedExtensions) in UploadEndpoint +Deferred (recorded, not dropped): generator pipeline split onto +MetadataReferencesProvider / ForAttributeWithMetadataName (upstream todo already +tracks it), @simplemodule/ui shared-chunk vendoring, generated-literal escaping +helper, test-claim `;` escaping. -## Critical 3 — remaining #242-class DbContext races -- [x] `AdminService.GetAdminOverviewAsync` — sequential awaits -- [x] `Admin/Pages/Admin/UsersEditEndpoint` — sequential awaits -- [x] `TenantFeatureHelper.GetOverridesForTenantAsync` — sequential awaits (throws with 2+ active flags) +## High +- [x] 1. FeatureFlags endpoint gate reads NameIdentifier only — use sub-aware helper (Core/FeatureFlags/EndpointFeatureFlagExtensions.cs) +- [x] 2. `?v=` query alone applies 1-year public+immutable cache header to any response incl. authed HTML — gate on static-asset path (Hosting/SimpleModuleHostExtensions.Helpers.cs) +- [x] 3. LocalStorageProvider: ListAsync bypasses traversal guard; GetFullPath boundary check lacks separator + uses OrdinalIgnoreCase (Storage.Local) +- [x] 4. Multi-tenant filter bakes scoped ITenantContext into cached EF model (first tenant frozen forever); null-tenant coalesces to "" matching empty-tenant rows (Database/ModuleModelBuilderExtensions.cs) +- [~] 6. Generator scans every referenced assembly (BCL, ASP.NET) on each compile; [FormRequest] scanned twice (Generator/Discovery/SymbolDiscovery.cs) — IN PROGRESS (subagent) -## Outbox gaps (events lost on crash after SaveChanges) -- [x] `TenantService.CreateTenantAsync` — use `IDbContextOutbox` like UpdateTenantAsync already does -- [x] `FileStorageService.UploadFileAsync` — same +## Medium +- [x] 7. InertiaResult.MergeProps double-serializes props on every shared-data render (Core/Inertia/InertiaResult.cs) +- [x] 8. InertiaLayoutDataMiddleware does antiforgery crypto + menu work for static assets/probes/APIs — gate to Inertia/HTML requests (Hosting/Middleware) +- [~] 10. Generated JSON resolver uncompilable for init-only props / missing parameterless ctor (Generator DtoPropertyExtractor + JsonResolverEmitter) — IN PROGRESS (subagent) +- [x] 11. EntityInterceptor/EntityChangeInterceptor only override async SaveChanges — sync path hard-deletes soft-delete entities silently (Database/Interceptors) +- [x] 12. vite-plugin-vendor skips rebuild when outputs exist — stale React after version bump (packages/SimpleModule.Client) -## Critical 4 — docs describe phantom modules -- [x] CLAUDE.md: load-test section lists 11 scenarios incl. Products/Orders/Marketplace/PageBuilder — only 6 exist (Admin, AuditLogs, FeatureFlags, FileStorage, Settings, Users) -- [x] CLAUDE.md: `modules/Products/src/Products` examples use wrong layout (`src/SimpleModule.`) — likely origin of the validate-pages bug -- [x] Sweep docs/CONSTITUTION.md + skills for phantom-module/wrong-layout references -- (untracked WIP dirs in the main checkout are stale build artifacts — left alone) +## Low +- [x] 13. PermissionMatcher wildcard allocates substring per check — span it +- [x] 14. GlobalExceptionHandler maps ArgumentNullException to client 400 at Warning +- [x] 15. InertiaMiddleware.Version throws on single-file publish (empty Assembly.Location) +- [~] 16. SymbolHelpers module-namespace prefix match lacks trailing-dot boundary — IN PROGRESS (subagent) +- [~] 17. ViewPagesEmitter hint name collides for same-named module classes in different namespaces — IN PROGRESS (subagent) +- [x] 18. SoftDeleteService.CoerceId: Convert.ChangeType throws for Guid keys +- [x] 19. Maintenance-gate comment claims static assets are spared — they aren't +- [x] 20. PermissionRegistryBuilder O(n²) list.Contains dedup +- [x] 21. app.tsx click interceptor: hash-only anchors re-request page instead of scrolling +- [x] 22. add-component.mjs UI path wrong for this repo (src/SimpleModule.UI vs packages/) +- [x] 23. typecheck.mjs unbounded tsc parallelism -## CI gaps -- [ ] PostgreSQL test leg — DEFERRED: `SimpleModuleWebApplicationFactory` is deeply SQLite-coupled (shared in-memory connection, static env bootstrap, Wolverine SQLite file); a reliable dual-provider factory is its own change. Docs no longer claim it exists. -- [x] CodeQL workflow -- [x] Dependabot config (nuget, npm, github-actions) -- [x] `dotnet list package --vulnerable` CI step - -## Code-review round 1 — fixes applied -- [x] ForwardedHeaders KnownProxies/KnownNetworks: accept comma-separated scalar (the form the docker-compose comment documents) as well as arrays; TryParse with a clear error instead of an opaque FormatException -- [x] docker-compose worker: add Seed__AdminPassword/Seed__UserPassword — the worker runs UserSeedService too and would otherwise race-seed the default admin password -- [x] FileStorageService.UploadFileAsync: scope blob-rollback to the pre-commit window so a post-commit outbox-flush failure can't dangle a committed row against a deleted blob -- [x] Env-predicate consistency: shared HostEnvironmentExtensions.IsLocalOrTest (Development+Testing) used by both UserSeedService and OpenIddictProductionGuard — closes the Staging bypass and the Testing-startup-crash in one predicate -- [x] UsersEditEndpoint: reverted to Task.WhenAll — the three contracts use distinct DbContexts (Users/Permissions/OpenIddict), so there was no race; corrected the misleading comment -- [x] ConfigKeys.OpenIddictAllowPasswordGrant constant — replaced the three hardcoded "OpenIddict:AllowPasswordGrant" string literals (drift would silently disable the guard) -- [x] validate-pages: detect interpolated Inertia.Render($"…") as unresolved instead of silently skipping it -- [x] validate-i18n: same fail-on-zero self-check as validate-pages (locale dirs exist today; zero = path drift) -- [x] UploadEndpoint: hoist size/extension parse to Map() time (resolve IOptions once, capture) instead of allocating a HashSet per request - -## Deferred (follow-ups — recorded, not silently dropped) -- **Roslyn diagnostic (SM0060)** for Task.WhenAll-over-shared-DbContext and SaveChanges+Publish-without-outbox — durable fix for the recurring race/outbox classes (fixed by hand 3× now). Analyzer-grade flow analysis; separate PR. An interim regex guard like validate-framework-scope.mjs is a cheaper stopgap. -- **Shared outbox helper** (`SaveAndPublishAsync`) in SimpleModule.Database to encode the BeginTransaction→SaveChanges→Publish→flush ritual once — currently duplicated across TenantService/FileStorageService. Also: WolverineConfiguration wires PublishDomainEventsFromEntityFrameworkCore but no entity implements it (dead idiom) — reconcile. -- **Framework production-config validation abstraction** (IValidateOptions/ValidateOnStart) — OpenIddictProductionGuard + UserSeedService are two ad-hoc startup guards; a shared mechanism is the right altitude and fixes implicit guard ordering. -- **Module options ↔ Settings store / appsettings binding**: generated RegisterModuleOptionsDefaults only calls AddOptions() with no config binding, so the FileStorage admin Settings/appsettings keys don't drive the enforced IOptions values (enforcement honors code-configured/default values only). -- **TenantFeatureHelper N+1**: serial GetOverridesAsync per flag; needs a bulk IFeatureFlagContracts method to collapse to one query. -- **ForwardedHeaders typed options** on SimpleModuleOptions (typo'd keys currently silently yield loopback-only trust). -- Generator decomposition to ForAttributeWithMetadataName. -- @simplemodule/ui vendoring + stale wwwroot chunk cleanup. +## Verification +- [x] Regression tests added: cross-tenant isolation across shared model (#4), sub-claim GetUserId/GetRoles (#1), wildcard module-boundary (#13), ViewPages same-name collision (#17) +- [x] dotnet build — succeeded, 0 errors (warnings-as-errors) +- [x] dotnet test — Core 280, Database 94, Generator 220, DevTools 35 all pass; CLI exit 0 +- [x] npm checks — biome clean, validate-pages (74 endpoints/577 files), framework-scope, i18n, typecheck 15/15 +- [x] Review section (below) ## Review -All four criticals addressed, plus the concrete improvements. Verified with: -`dotnet build` (0 warnings/errors), `dotnet test` (19/19 assemblies, 0 failures), -`npm run check` (biome + validate-pages + i18n + framework-scope + typecheck 14/14), -`npm run build` (production bundles), plus a negative test proving validate-pages -now fails when a page registration is removed. +All 22 findings addressed except the two strategic rewrites intentionally deferred +(generator pipeline split onto MetadataReferencesProvider; @simplemodule/ui shared-chunk +vendoring) plus a few defense-in-depth notes. Generator fixes (#6/#10/#16/#17) done by +subagent; verified by full generator test suite + new collision test. -Notable finds while fixing: -- The resurrected guard immediately caught that `UnlockAccountEndpoint` renders via a - `const string ComponentName` — the script now resolves same-file string consts and - reports unresolvable `Inertia.Render` arguments as failures. -- The new ClientApp typecheck exposed two latent runtime bugs beyond the reported - `showErrorToast`: `router.on('exception', ...)` is not an Inertia v3 event (the - network-error toast was dead code — now `networkError`), and the page resolver - returned a module where Inertia's types want the component. -- `FileStorageService.DeleteFileAsync` had the same lost-event bug as Upload (worse: - a failed blob delete also skipped the event after the DB commit) — fixed via outbox. -- Create flows with DB-generated ids (Tenant, StoredFile) use an explicit transaction + - `SaveChangesAndFlushMessagesAsync`, which per Wolverine commits the open transaction - before flushing. `FakeDbContextOutbox` now mirrors that commit behavior. +Notable while fixing: +- The tenant-filter fix (#4) required a design change, not a patch: EF caches the model + per context type, so capturing a scoped ITenantContext as an Expression.Constant froze + the first request's tenant. Fixed by referencing the *executing* DbContext instance via + a new `ITenantScopedDbContext.CurrentTenantId` — EF re-evaluates the context reference + per query. New regression test (two contexts, one shared cached model + DB, different + tenants) proves isolation and would fail under the old code. API change is safe: zero + production callers today. +- Same sub-claim bug as #1 also lived in EntityInterceptor's audit stamping + (ClaimTypes.NameIdentifier → GetUserId()); fixed alongside — CreatedBy/UpdatedBy would + have been null under Keycloak. +- Sync SaveChanges gap (#11) fixed on BOTH interceptors; EntityChangeInterceptor's sync + dispatch blocks on the async path (safe — ASP.NET Core has no SynchronizationContext). +- InertiaResult merge (#7): a correct flat merge needs props' keys, so props is still + materialized once, but the extra Dictionary + full re-serialize is gone — now one + Utf8JsonWriter pass, props win on collision, no duplicate keys. Integration tests + (InertiaResultTests) confirm the JSON envelope is unchanged. +- Layout middleware gate (#8) keys on X-Inertia OR Accept: text/html — real browsers and + Inertia nav both qualify; static-asset/probe/JSON requests skip the antiforgery crypto. +Deferred / documented (not silently dropped): +- Generator pipeline decomposition (metadata vs source providers) — upstream todo already + tracks it; assembly-prefix filter is the cheap high-value win applied now. +- @simplemodule/ui bundled per module — payload-size only, correctness verified fine. +- LocalStorage traversal (#3) & CoerceId Guid (#18) verified by inspection + build; no + dedicated storage test project exists (adding one needs slnx + Dockerfile wiring). diff --git a/template/SimpleModule.Host/ClientApp/app.tsx b/template/SimpleModule.Host/ClientApp/app.tsx index e72bd249..545f1e19 100644 --- a/template/SimpleModule.Host/ClientApp/app.tsx +++ b/template/SimpleModule.Host/ClientApp/app.tsx @@ -67,6 +67,10 @@ document.addEventListener('click', (event) => { // Only same-origin if (link.origin !== window.location.origin) return; + // In-page anchors (e.g. href="#section") must scroll natively, not re-request the + // page through Inertia — let the browser handle any hash link to the current path. + if (link.hash && link.pathname === window.location.pathname) return; + // Skip non-Inertia server routes (Identity, Swagger, health checks, OAuth) const path = link.pathname; if (nonInertiaPathPrefixes.some((prefix) => path.startsWith(prefix))) return; diff --git a/tests/SimpleModule.Core.Tests/Extensions/ClaimsPrincipalExtensionsTests.cs b/tests/SimpleModule.Core.Tests/Extensions/ClaimsPrincipalExtensionsTests.cs index 9ba7ec45..d15a7a10 100644 --- a/tests/SimpleModule.Core.Tests/Extensions/ClaimsPrincipalExtensionsTests.cs +++ b/tests/SimpleModule.Core.Tests/Extensions/ClaimsPrincipalExtensionsTests.cs @@ -55,6 +55,45 @@ public void HasPermission_EmptyPrincipal_ReturnsFalse() user.HasPermission("Anything").Should().BeFalse(); } + [Fact] + public void HasPermission_WildcardClaim_DoesNotMatchAcrossModuleBoundary() + { + // "Products.*" must not match a permission outside the "Products." prefix. + var user = CreateUser(new Claim("permission", "Products.*")); + + user.HasPermission("ProductsArchive.View").Should().BeFalse(); + user.HasPermission("Products.View").Should().BeTrue(); + } + + [Fact] + public void GetUserId_PrefersSubClaim_ForOpenIddictAndKeycloak() + { + // OpenIddict/Keycloak with MapInboundClaims=false emit the id as "sub", + // never ClaimTypes.NameIdentifier. + var user = CreateUser(new Claim("sub", "keycloak-user-1")); + + user.GetUserId().Should().Be("keycloak-user-1"); + } + + [Fact] + public void GetUserId_FallsBackToNameIdentifier_ForAspNetIdentity() + { + var user = CreateUser(new Claim(ClaimTypes.NameIdentifier, "identity-user-1")); + + user.GetUserId().Should().Be("identity-user-1"); + } + + [Fact] + public void GetRoles_ReadsBothRoleClaimTypes() + { + var user = CreateUser( + new Claim("role", "editor"), + new Claim(ClaimTypes.Role, "admin") + ); + + user.GetRoles().Should().BeEquivalentTo("editor", "admin"); + } + private static ClaimsPrincipal CreateUser(params Claim[] claims) { var identity = new ClaimsIdentity(claims, "Test"); diff --git a/tests/SimpleModule.Database.Tests/EntityInterceptorTests.MultiTenant.cs b/tests/SimpleModule.Database.Tests/EntityInterceptorTests.MultiTenant.cs index 5eb25901..e5d85e03 100644 --- a/tests/SimpleModule.Database.Tests/EntityInterceptorTests.MultiTenant.cs +++ b/tests/SimpleModule.Database.Tests/EntityInterceptorTests.MultiTenant.cs @@ -1,5 +1,8 @@ using FluentAssertions; +using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; namespace SimpleModule.Database.Tests; @@ -53,6 +56,67 @@ await fixture.Context.Database.ExecuteSqlRawAsync( allResults.Should().HaveCount(2); } + [Fact] + public async Task MultiTenant_Filter_Reads_Executing_Context_Tenant_Across_Shared_Model() + { + // Regression for the cross-tenant leak: two contexts of the same type share one + // cached EF model (same internal service provider) and one database (same + // connection) but carry different tenants. The filter must read each executing + // context's tenant — not the tenant frozen into the model when the first context + // built it. Under the previous Expression.Constant(tenantContext) implementation, + // contextB below would still filter by tenant-a and return the wrong row. + using var connection = new SqliteConnection("Data Source=:memory:"); + await connection.OpenAsync(); + + using var internalProvider = new ServiceCollection() + .AddEntityFrameworkSqlite() + .BuildServiceProvider(); + + var dbOptions = Options.Create( + new DatabaseOptions { DefaultConnection = "Data Source=:memory:" } + ); + + DbContextOptions BuildOptions() => + new DbContextOptionsBuilder() + .UseSqlite(connection) + .UseInternalServiceProvider(internalProvider) + .Options; + + // Context for tenant-a builds the shared model first and seeds both tenants' rows. + await using ( + var contextA = new MultiTenantTestDbContext( + BuildOptions(), + dbOptions, + new TestTenantContext("tenant-a") + ) + ) + { + await contextA.Database.EnsureCreatedAsync(); + contextA.MultiTenantEntities.AddRange( + new MultiTenantTestEntity { Name = "A", TenantId = "tenant-a" }, + new MultiTenantTestEntity { Name = "B", TenantId = "tenant-b" } + ); + await contextA.SaveChangesAsync(); + + var aResults = await contextA.MultiTenantEntities.ToListAsync(); + aResults.Should().ContainSingle().Which.Name.Should().Be("A"); + } + + // A second context reuses the cached model and shared data but with a different + // tenant; it must see only tenant-b's row. + await using ( + var contextB = new MultiTenantTestDbContext( + BuildOptions(), + dbOptions, + new TestTenantContext("tenant-b") + ) + ) + { + var bResults = await contextB.MultiTenantEntities.ToListAsync(); + bResults.Should().ContainSingle().Which.Name.Should().Be("B"); + } + } + [Fact] public async Task FullAuditableEntity_BaseClass_Works() { diff --git a/tests/SimpleModule.Database.Tests/EntityInterceptorTests.TestEntities.cs b/tests/SimpleModule.Database.Tests/EntityInterceptorTests.TestEntities.cs index 06967ae4..87006d51 100644 --- a/tests/SimpleModule.Database.Tests/EntityInterceptorTests.TestEntities.cs +++ b/tests/SimpleModule.Database.Tests/EntityInterceptorTests.TestEntities.cs @@ -64,8 +64,10 @@ private sealed class EntityTestDbContext( DbContextOptions options, IOptions dbOptions, ITenantContext? tenantContext = null - ) : DbContext(options) + ) : DbContext(options), ITenantScopedDbContext { + public string? CurrentTenantId => tenantContext?.TenantId; + public DbSet TimestampedEntities => Set(); public DbSet AuditableEntities => Set(); public DbSet SoftDeleteEntities => Set(); @@ -98,7 +100,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) if (tenantContext is not null) { - modelBuilder.ApplyMultiTenantFilters(tenantContext); + modelBuilder.ApplyMultiTenantFilters(this); } } } @@ -107,15 +109,17 @@ private sealed class MultiTenantTestDbContext( DbContextOptions options, IOptions dbOptions, ITenantContext tenantContext - ) : DbContext(options) + ) : DbContext(options), ITenantScopedDbContext { + public string? CurrentTenantId => tenantContext.TenantId; + public DbSet MultiTenantEntities => Set(); protected override void OnModelCreating(ModelBuilder modelBuilder) { modelBuilder.Entity(e => e.HasKey(x => x.Id)); modelBuilder.ApplyModuleSchema("MultiTenantTest", dbOptions.Value); - modelBuilder.ApplyMultiTenantFilters(tenantContext); + modelBuilder.ApplyMultiTenantFilters(this); } } } diff --git a/tests/SimpleModule.Generator.Tests/ViewDiscoveryTests.cs b/tests/SimpleModule.Generator.Tests/ViewDiscoveryTests.cs index b13b74bc..6406e617 100644 --- a/tests/SimpleModule.Generator.Tests/ViewDiscoveryTests.cs +++ b/tests/SimpleModule.Generator.Tests/ViewDiscoveryTests.cs @@ -121,11 +121,11 @@ public void Map(IEndpointRouteBuilder app) result .GeneratedTrees.Should() - .Contain(t => t.FilePath.EndsWith("ViewPages_Products.g.cs", StringComparison.Ordinal)); + .Contain(t => t.FilePath.EndsWith("ViewPages_TestApp_ProductsModule.g.cs", StringComparison.Ordinal)); var viewPages = result .GeneratedTrees.First(t => - t.FilePath.EndsWith("ViewPages_Products.g.cs", StringComparison.Ordinal) + t.FilePath.EndsWith("ViewPages_TestApp_ProductsModule.g.cs", StringComparison.Ordinal) ) .GetText() .ToString(); @@ -172,11 +172,11 @@ public void Map(IEndpointRouteBuilder app) result .GeneratedTrees.Should() - .Contain(t => t.FilePath.EndsWith("ViewPages_Test.g.cs", StringComparison.Ordinal)); + .Contain(t => t.FilePath.EndsWith("ViewPages_TestApp_TestModule.g.cs", StringComparison.Ordinal)); var viewPages = result .GeneratedTrees.First(t => - t.FilePath.EndsWith("ViewPages_Test.g.cs", StringComparison.Ordinal) + t.FilePath.EndsWith("ViewPages_TestApp_TestModule.g.cs", StringComparison.Ordinal) ) .GetText() .ToString(); @@ -276,7 +276,7 @@ public void Map(IEndpointRouteBuilder app) var viewPages = result .GeneratedTrees.First(t => - t.FilePath.EndsWith("ViewPages_Test.g.cs", StringComparison.Ordinal) + t.FilePath.EndsWith("ViewPages_TestApp_TestModule.g.cs", StringComparison.Ordinal) ) .GetText() .ToString(); @@ -355,7 +355,7 @@ public void Map(IEndpointRouteBuilder app) var viewPages = result .GeneratedTrees.First(t => - t.FilePath.EndsWith("ViewPages_Users.g.cs", StringComparison.Ordinal) + t.FilePath.EndsWith("ViewPages_TestApp_UsersModule.g.cs", StringComparison.Ordinal) ) .GetText() .ToString(); diff --git a/tests/SimpleModule.Generator.Tests/ViewPagesEmitterTests.cs b/tests/SimpleModule.Generator.Tests/ViewPagesEmitterTests.cs index 961c274a..f55a87ec 100644 --- a/tests/SimpleModule.Generator.Tests/ViewPagesEmitterTests.cs +++ b/tests/SimpleModule.Generator.Tests/ViewPagesEmitterTests.cs @@ -54,7 +54,7 @@ public void Map(IEndpointRouteBuilder app) result .GeneratedTrees.Should() - .Contain(t => t.FilePath.EndsWith("ViewPages_Products.g.cs", StringComparison.Ordinal)); + .Contain(t => t.FilePath.EndsWith("ViewPages_TestApp_ProductsModule.g.cs", StringComparison.Ordinal)); } [Fact] @@ -86,7 +86,7 @@ public void Map(IEndpointRouteBuilder app) var compilation = GeneratorTestHelper.CreateCompilation(source); var result = GeneratorTestHelper.RunGenerator(compilation); - var viewPages = GetGeneratedSource(result, "ViewPages_Items.g.cs"); + var viewPages = GetGeneratedSource(result, "ViewPages_TestApp_ItemsModule.g.cs"); viewPages.Should().Contain("'Items/Create': () => import('./Create')"); } @@ -120,7 +120,7 @@ public void Map(IEndpointRouteBuilder app) var compilation = GeneratorTestHelper.CreateCompilation(source); var result = GeneratorTestHelper.RunGenerator(compilation); - var viewPages = GetGeneratedSource(result, "ViewPages_Orders.g.cs"); + var viewPages = GetGeneratedSource(result, "ViewPages_TestApp_OrdersModule.g.cs"); viewPages.Should().Contain("export const pages: Record = {"); viewPages.Should().Contain("'Orders/Detail': () => import('./Detail')"); @@ -171,7 +171,7 @@ public void Map(IEndpointRouteBuilder app) var compilation = GeneratorTestHelper.CreateCompilation(source); var result = GeneratorTestHelper.RunGenerator(compilation); - var viewPages = GetGeneratedSource(result, "ViewPages_Test.g.cs"); + var viewPages = GetGeneratedSource(result, "ViewPages_TestApp_TestModule.g.cs"); viewPages.Should().Contain("'Test/Browse': () => import('./Browse')"); viewPages.Should().Contain("'Test/Create': () => import('./Create')"); @@ -207,7 +207,7 @@ public void Map(IEndpointRouteBuilder app) var compilation = GeneratorTestHelper.CreateCompilation(source); var result = GeneratorTestHelper.RunGenerator(compilation); - var viewPages = GetGeneratedSource(result, "ViewPages_Test.g.cs"); + var viewPages = GetGeneratedSource(result, "ViewPages_TestApp_TestModule.g.cs"); viewPages.Should().Contain("#if SIMPLEMODULE_TS"); viewPages.Should().Contain("/*"); @@ -244,11 +244,65 @@ public void Map(IEndpointRouteBuilder app) var compilation = GeneratorTestHelper.CreateCompilation(source); var result = GeneratorTestHelper.RunGenerator(compilation); - var viewPages = GetGeneratedSource(result, "ViewPages_Test.g.cs"); + var viewPages = GetGeneratedSource(result, "ViewPages_TestApp_TestModule.g.cs"); viewPages.Should().Contain("'Test/Detail': () => import('./Detail')"); } + [Fact] + public void SameSimpleName_DifferentNamespaces_DoNotCollide() + { + // Two module classes with the same simple name ("ProductsModule") in different + // namespaces must produce distinct ViewPages hint names, or AddSource throws a + // duplicate-hint-name ArgumentException and the whole generator fails. + var source = """ + using Microsoft.AspNetCore.Builder; + using Microsoft.AspNetCore.Routing; + using SimpleModule.Core; + + namespace Alpha + { + [Module("AlphaProducts", ViewPrefix = "/alpha")] + public class ProductsModule : IModule { } + } + + namespace Alpha.Pages + { + public class BrowseEndpoint : IViewEndpoint + { + public void Map(IEndpointRouteBuilder app) => app.MapGet("/browse", () => "a"); + } + } + + namespace Beta + { + [Module("BetaProducts", ViewPrefix = "/beta")] + public class ProductsModule : IModule { } + } + + namespace Beta.Pages + { + public class BrowseEndpoint : IViewEndpoint + { + public void Map(IEndpointRouteBuilder app) => app.MapGet("/browse", () => "b"); + } + } + """; + + var compilation = GeneratorTestHelper.CreateCompilation(source); + var result = GeneratorTestHelper.RunGenerator(compilation); + + // Both ViewPages files are emitted with distinct, namespace-qualified hint names. + result + .GeneratedTrees.Should() + .Contain(t => + t.FilePath.EndsWith("ViewPages_Alpha_ProductsModule.g.cs", StringComparison.Ordinal) + ) + .And.Contain(t => + t.FilePath.EndsWith("ViewPages_Beta_ProductsModule.g.cs", StringComparison.Ordinal) + ); + } + private static string GetGeneratedSource( Microsoft.CodeAnalysis.GeneratorDriverRunResult result, string fileName From de5dec4f283debd4f0f0ef12836a1362a5ce7ddc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 17 Jul 2026 23:22:18 +0200 Subject: [PATCH 2/2] fix(docker): point default SQLite path at writable /app/data so bare `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. --- Dockerfile | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Dockerfile b/Dockerfile index 67e8d09c..8d98eaf1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -157,6 +157,14 @@ COPY --from=build --chown=appuser:appgroup /app/publish . # Writable directory for SQLite database and local storage RUN mkdir -p /app/data /app/storage && chown appuser:appgroup /app/data /app/storage +# Default the SQLite database into the writable /app/data dir. The app's built-in +# default is "Data Source=app.db", which resolves to the root-owned /app workdir +# that the non-root appuser cannot write — a bare `docker run` of this image would +# otherwise crash at startup with "SQLite Error 14: unable to open database file". +# docker-compose overrides this (Database__Provider=PostgreSQL + its own connection +# string), so the Postgres deployment path is unaffected. +ENV Database__DefaultConnection="Data Source=/app/data/app.db" + # Deployment version for JS/CSS cache-busting and Inertia stale-version detection. # Left empty by default so InertiaMiddleware.GetVersion() falls back to the entry # assembly's last-write timestamp (yyyyMMddHHmmss), which changes on every publish