From 51c0987b002a785caf6f56bc5d3c0bc43877ca3f Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 26 Jun 2026 19:13:36 +0200 Subject: [PATCH] fix(delegated): enforce allow-list on the single-tenant drill (home-tenant escalation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding (High). GetAllSessionsPageAsync / GetAllSessionsAsync route any non-empty tenantIdFilter straight to the per-tenant scan, ignoring allowedTenantIds. The middleware only resolves the delegated allow-list for a cross-tenant target (namedTarget != JWT tenant); when ?tenantId= equals the caller's own JWT/home tenant, crossTenant is false so the scoped-route check is skipped — but /api/global/sessions is GlobalReadOrDelegatedSubset, so a delegated-only caller is still admitted (with AllowedTenantIds for their managed set). The repo then drilled the home tenant anyway → a delegated MSP user could read their home tenant's sessions/stats without managing it or being a member. Fix: new DrillOutsideBound guard — when allowedTenantIds != null and tenantIdFilter is not in that set, return empty before the per-tenant scan. Applied to both the page (sessions list) and the drain (stats funnel through GetAllSessionsAsync). The middleware contract is unchanged by design (admit + publish AllowedTenantIds, handler/repo bounds); the bound now holds for the home-tenant case too. Tests: DelegatedBoundedAggregateTests.BoundedDrill/Stats_UnmanagedTenant_* (drill an unmanaged tenant → empty, the per-tenant SessionsIndex/Sessions scan never runs); PolicyEnforcementMiddlewareTests.Delegated_GlobalSessions_HomeTenantDrill_AdmittedButNotInAllowedSet pins that the middleware admits the home-tenant drill but never puts it in AllowedTenantIds. Backend 2489 green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../DelegatedBoundedAggregateTests.cs | 41 +++++++++++++++++++ .../PolicyEnforcementMiddlewareTests.cs | 23 +++++++++++ .../Services/TableStorageService.Sessions.cs | 20 +++++++++ 3 files changed, 84 insertions(+) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs index d411afdf..eda24528 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs @@ -24,6 +24,9 @@ public class DelegatedBoundedAggregateTests { private const string ConfigTenant = "11111111-1111-1111-1111-111111111111"; private const string ManagedTenant = "22222222-2222-2222-2222-222222222222"; + // A tenant the delegated caller is NOT managing — e.g. their own JWT/home tenant, the one route the + // middleware's crossTenant short-circuit does not allow-list-check. + private const string UnmanagedTenant = "33333333-3333-3333-3333-333333333333"; [Fact] public async Task BoundedAggregate_EmptyManagedSet_ReturnsEmpty_NeverUnboundedScan() @@ -64,6 +67,44 @@ public class DelegatedBoundedAggregateTests Times.AtLeastOnce()); } + [Fact] + public async Task BoundedDrill_UnmanagedTenant_ReturnsEmpty_NeverScansThatTenant() + { + // Delegated caller drills ?tenantId= — e.g. their own JWT/home + // tenant, where the middleware crossTenant short-circuit skips the allow-list check. Must read nothing. + var leak = new[] { new TableEntity(UnmanagedTenant, "row1"), new TableEntity(UnmanagedTenant, "row2") }; + var indexClient = MockTableClientReturning(leak); + var sessionsClient = MockTableClientReturning(leak); + + var storage = BuildStorage(new Mock(), sessionsClient, indexClient); + + var page = await storage.GetAllSessionsPageAsync( + tenantIdFilter: UnmanagedTenant, days: null, pageSize: 10, continuation: null, + allowedTenantIds: new[] { ManagedTenant }); + + Assert.Empty(page.Items); + AssertNeverQueried(indexClient); + AssertNeverQueried(sessionsClient); + } + + [Fact] + public async Task BoundedStats_UnmanagedTenant_ReturnsZero_NeverScansThatTenant() + { + // Same escalation via the stats path (global/stats/sessions?tenantId=). + var leak = new[] { new TableEntity(UnmanagedTenant, "row1") }; + var indexClient = MockTableClientReturning(leak); + var sessionsClient = MockTableClientReturning(leak); + + var storage = BuildStorage(new Mock(), sessionsClient, indexClient); + + var stats = await storage.GetAllSessionStatsAsync( + UnmanagedTenant, days: 7, allowedTenantIds: new[] { ManagedTenant }); + + Assert.Equal(0, stats.TotalLastNDays); + AssertNeverQueried(indexClient); + AssertNeverQueried(sessionsClient); + } + // ── helpers ────────────────────────────────────────────────────────────── private static TableStorageService BuildStorage( diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index f98b8f4a..83272093 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -544,6 +544,29 @@ public class PolicyEnforcementMiddlewareTests Assert.Equal(403, result.StatusCode); } + [Fact] + public async Task Delegated_GlobalSessions_HomeTenantDrill_AdmittedButNotInAllowedSet() + { + // global/sessions?tenantId= — crossTenant is false, so the delegated + // scoped-route check is skipped and the subset tier admits the caller. This is BY DESIGN (admit + + // publish AllowedTenantIds, handler bounds): the home tenant is NOT in AllowedTenantIds, so the repo + // (TableStorageService.DrillOutsideBound) returns empty. This test pins that contract so the bound is + // never silently widened to include the unmanaged home tenant. Covered end-to-end by + // DelegatedBoundedAggregateTests.BoundedDrill/Stats_UnmanagedTenant_*. + const string upn = "msp@partner.example"; + var h = BuildHarness(); + h.AsDelegated(TenantB); + + var result = await h.Middleware.DecideAsync("GET", "/api/global/sessions", TenantA, AuthedPrincipal(TenantA, upn)); + + Assert.True(result.Allowed); + var rc = result.Context!; + Assert.True(rc.IsDelegatedReader); + Assert.NotNull(rc.AllowedTenantIds); + Assert.DoesNotContain(TenantA.ToLowerInvariant(), rc.AllowedTenantIds!); + Assert.Contains(TenantB.ToLowerInvariant(), rc.AllowedTenantIds!); + } + [Fact] public async Task Delegated_AggregateOnlyRoute_WithTenantId_StaysBlocked() { diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs index 9950c069..95bfbd71 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs @@ -896,12 +896,27 @@ namespace AutopilotMonitor.Functions.Services /// Cross-tenant paged variant of . /// optionally restricts to one tenant. /// + /// + /// True when a BOUNDED (delegated/MSP) caller asks for a single-tenant drill that is OUTSIDE its + /// managed set. The middleware enforces the allow-list for a cross-tenant target, but NOT when + /// ?tenantId= equals the caller's own JWT tenant (crossTenant=false short-circuits the delegated + /// scoped-route check) — so the single-tenant drill in the repo must re-check the bound itself. + /// + private static bool DrillOutsideBound(IReadOnlyCollection? allowedTenantIds, string? tenantIdFilter) + => allowedTenantIds != null + && !string.IsNullOrEmpty(tenantIdFilter) + && !new HashSet(allowedTenantIds, StringComparer.OrdinalIgnoreCase).Contains(tenantIdFilter!); + public async Task> GetAllSessionsPageAsync( string? tenantIdFilter, int? days, int pageSize, string? continuation, IReadOnlyCollection? allowedTenantIds = null) { if (pageSize < 1) throw new ArgumentOutOfRangeException(nameof(pageSize)); + // Bounded (delegated) single-tenant drill outside the managed set → empty, never the per-tenant scan. + if (DrillOutsideBound(allowedTenantIds, tenantIdFilter)) + return new RawPage(new List(), null); + // When tenantIdFilter is set and is a valid tenantId, route through // the per-tenant SessionsIndex scan — natively-ordered, cheaper. if (!string.IsNullOrEmpty(tenantIdFilter)) @@ -920,6 +935,11 @@ namespace AutopilotMonitor.Functions.Services /// public async Task> GetAllSessionsAsync(string? tenantIdFilter = null, int? days = null, IReadOnlyCollection? allowedTenantIds = null) { + // Bounded (delegated) single-tenant drill outside the managed set → empty (covers stats too, + // which funnel through here). See DrillOutsideBound. + if (DrillOutsideBound(allowedTenantIds, tenantIdFilter)) + return new List(); + if (!string.IsNullOrEmpty(tenantIdFilter)) { return await GetSessionsAsync(tenantIdFilter!, days);