fix(security): systemic IDOR safeguard via EF global query filters
Defense-in-depth tenant isolation: every user-owned entity (Email, Sender, MailThread, MailDomain, Attachment, Label, SyncState, AnalyticsAggregate, WidgetLayout, UnsubscribeItem) gets a global query filter restricting reads to the authenticated user. AppDbContext takes an optional ICurrentUser; CurrentUserId is Guid.Empty for background workers / design-time, which DISABLES the filter so sync and tooling (which already scope by an explicit userId) are unaffected. On the HTTP attack surface a forgotten manual `WHERE UserId ==` can no longer leak another tenant's rows. Phase 1 confirmed no active IDOR; this is preventive, and prioritised now because the upcoming automation engine will add many new queries. Also: moved the Npgsql-only tsvector FTS mapping out of EmailConfiguration into AppDbContext.OnModelCreating, guarded by Database.IsRelational() (Ignored otherwise), so non-relational test providers work — honouring the existing Email.SearchVector comment. Production (Npgsql) model is unchanged; no migration needed. Adds 3 cross-user tenant-isolation integration tests. All 38 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
cesnimda
@@ -8,7 +8,22 @@ namespace InboxIntel.Infrastructure.Persistence;
|
||||
|
||||
public class AppDbContext : DbContext, IAppDbContext
|
||||
{
|
||||
public AppDbContext(DbContextOptions<AppDbContext> options) : base(options) { }
|
||||
private readonly ICurrentUser? _currentUser;
|
||||
|
||||
public AppDbContext(DbContextOptions<AppDbContext> options, ICurrentUser? currentUser = null) : base(options)
|
||||
=> _currentUser = currentUser;
|
||||
|
||||
/// <summary>
|
||||
/// Tenant id used by the global query filters below. Resolves to the
|
||||
/// authenticated user during an HTTP request. It is <see cref="Guid.Empty"/>
|
||||
/// when there is no current user (background workers, design-time tooling,
|
||||
/// startup migration) — in which case filtering is DISABLED, because those
|
||||
/// paths are trusted server code that already scope their own queries by a
|
||||
/// userId passed in explicitly. The security value is on the HTTP attack
|
||||
/// surface: a forgotten manual <c>WHERE UserId ==</c> can no longer leak
|
||||
/// another tenant's rows, since the filter restricts to the caller.
|
||||
/// </summary>
|
||||
public Guid CurrentUserId => _currentUser?.UserId ?? Guid.Empty;
|
||||
|
||||
public DbSet<User> Users => Set<User>();
|
||||
public DbSet<Email> Emails => Set<Email>();
|
||||
@@ -26,6 +41,41 @@ public class AppDbContext : DbContext, IAppDbContext
|
||||
protected override void OnModelCreating(ModelBuilder modelBuilder)
|
||||
{
|
||||
modelBuilder.ApplyConfigurationsFromAssembly(Assembly.GetExecutingAssembly());
|
||||
|
||||
// Defense-in-depth tenant isolation (systemic IDOR safeguard). Every
|
||||
// user-owned entity is filtered to the current user so a query that forgets
|
||||
// its manual `WHERE UserId ==` clause cannot leak across tenants. Applied
|
||||
// uniformly to all user-scoped entities so EF sees no filtered/unfiltered
|
||||
// navigation mismatch. Bypassed when CurrentUserId is Guid.Empty (workers).
|
||||
modelBuilder.Entity<Email>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<Sender>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<MailThread>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<MailDomain>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<Attachment>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<Label>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<SyncState>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<AnalyticsAggregate>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<WidgetLayout>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
modelBuilder.Entity<UnsubscribeItem>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||
|
||||
// PostgreSQL full-text search: generated tsvector over subject + body with a
|
||||
// GIN index, maintained by the DB and read-only in code. The tsvector type is
|
||||
// Npgsql-only, so map it only for relational providers and ignore it otherwise
|
||||
// (e.g. the InMemory provider used by tests). Production behaviour is unchanged.
|
||||
if (Database.IsRelational())
|
||||
{
|
||||
modelBuilder.Entity<Email>().Property(e => e.SearchVector)
|
||||
.HasColumnType("tsvector")
|
||||
.HasComputedColumnSql(
|
||||
"to_tsvector('english', coalesce(\"Subject\",'') || ' ' || coalesce(\"BodyText\",''))",
|
||||
stored: true);
|
||||
modelBuilder.Entity<Email>().HasIndex(e => e.SearchVector).HasMethod("GIN");
|
||||
}
|
||||
else
|
||||
{
|
||||
modelBuilder.Entity<Email>().Ignore(e => e.SearchVector);
|
||||
}
|
||||
|
||||
base.OnModelCreating(modelBuilder);
|
||||
}
|
||||
|
||||
|
||||
@@ -36,13 +36,8 @@ public class EmailConfiguration : IEntityTypeConfiguration<Email>
|
||||
.HasForeignKey(e => e.SenderId)
|
||||
.OnDelete(DeleteBehavior.Restrict);
|
||||
|
||||
// PostgreSQL full-text search: generated tsvector over subject + body,
|
||||
// with a GIN index. Maintained by the database, read-only in code.
|
||||
b.Property(e => e.SearchVector)
|
||||
.HasColumnType("tsvector")
|
||||
.HasComputedColumnSql(
|
||||
"to_tsvector('english', coalesce(\"Subject\",'') || ' ' || coalesce(\"BodyText\",''))",
|
||||
stored: true);
|
||||
b.HasIndex(e => e.SearchVector).HasMethod("GIN");
|
||||
// NOTE: the PostgreSQL full-text `tsvector` mapping is applied in
|
||||
// AppDbContext.OnModelCreating, guarded by Database.IsRelational(), so that
|
||||
// non-relational test providers (InMemory) can ignore the Npgsql-only type.
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
using FluentAssertions;
|
||||
using InboxIntel.Application.Abstractions;
|
||||
using InboxIntel.Domain.Entities;
|
||||
using InboxIntel.Infrastructure.Persistence;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Xunit;
|
||||
|
||||
namespace InboxIntel.IntegrationTests;
|
||||
|
||||
/// <summary>
|
||||
/// Verifies the global query-filter IDOR safeguard on AppDbContext: an HTTP-scoped
|
||||
/// context (with a current user) sees only that user's rows even if a query forgets
|
||||
/// its manual UserId filter; a worker-scoped context (Guid.Empty) sees everything.
|
||||
/// </summary>
|
||||
public class TenantIsolationTests
|
||||
{
|
||||
private sealed class FakeCurrentUser : ICurrentUser
|
||||
{
|
||||
public Guid UserId { get; set; }
|
||||
public bool IsAuthenticated => UserId != Guid.Empty;
|
||||
}
|
||||
|
||||
private static DbContextOptions<AppDbContext> InMemory(string name)
|
||||
=> new DbContextOptionsBuilder<AppDbContext>().UseInMemoryDatabase(name).Options;
|
||||
|
||||
private static readonly Guid UserA = Guid.NewGuid();
|
||||
private static readonly Guid UserB = Guid.NewGuid();
|
||||
|
||||
private static async Task SeedAsync(DbContextOptions<AppDbContext> opts)
|
||||
{
|
||||
// Seed with no current user (Guid.Empty) so the filter is bypassed for writes/reads here.
|
||||
using var seed = new AppDbContext(opts, new FakeCurrentUser());
|
||||
seed.Emails.Add(new Email { UserId = UserA, GmailMessageId = "a1", Subject = "A-one" });
|
||||
seed.Emails.Add(new Email { UserId = UserA, GmailMessageId = "a2", Subject = "A-two" });
|
||||
seed.Emails.Add(new Email { UserId = UserB, GmailMessageId = "b1", Subject = "B-one" });
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Authenticated_context_sees_only_its_own_rows_even_without_manual_filter()
|
||||
{
|
||||
var opts = InMemory(nameof(Authenticated_context_sees_only_its_own_rows_even_without_manual_filter));
|
||||
await SeedAsync(opts);
|
||||
|
||||
// Note: NO manual .Where(e => e.UserId == ...) here — the global filter must enforce it.
|
||||
using var ctx = new AppDbContext(opts, new FakeCurrentUser { UserId = UserA });
|
||||
var emails = await ctx.Emails.ToListAsync();
|
||||
|
||||
emails.Should().HaveCount(2);
|
||||
emails.Should().OnlyContain(e => e.UserId == UserA);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Other_users_row_is_invisible_by_id()
|
||||
{
|
||||
var opts = InMemory(nameof(Other_users_row_is_invisible_by_id));
|
||||
await SeedAsync(opts);
|
||||
|
||||
using var ctx = new AppDbContext(opts, new FakeCurrentUser { UserId = UserA });
|
||||
// Fetch B's row by its GmailMessageId — classic IDOR attempt; must return null.
|
||||
var leaked = await ctx.Emails.FirstOrDefaultAsync(e => e.GmailMessageId == "b1");
|
||||
|
||||
leaked.Should().BeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Empty_current_user_bypasses_the_filter_for_background_workers()
|
||||
{
|
||||
var opts = InMemory(nameof(Empty_current_user_bypasses_the_filter_for_background_workers));
|
||||
await SeedAsync(opts);
|
||||
|
||||
// Guid.Empty == background/worker scope: must see all tenants' rows so sync/upsert works.
|
||||
using var ctx = new AppDbContext(opts, new FakeCurrentUser { UserId = Guid.Empty });
|
||||
var all = await ctx.Emails.ToListAsync();
|
||||
|
||||
all.Should().HaveCount(3);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user