From b215602737d7f2bc9a4339cf43ac902040112d1a Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Wed, 27 May 2026 19:39:02 +0200 Subject: [PATCH] feat(backend,mcp): server-side fields= projection for paginated config/all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to opt-in pagination: fields= was a client-side trim in the MCP tool, so it did not round-trip across pages — following nextLink returned the full safe-field set again, and the shape differed page 1 vs 2+. Move the projection server-side (matching query_raw_sessions): - TenantConfigProjection: keep-list intersection — only the 9 non-sensitive fields can ever be emitted; requested keys are intersected (fields=teamsWebhookUrl yields nothing), tenantId always included. ParseFields is case-insensitive. - TenantConfigPagination: parse fields=; BuildNextLink echoes it so the projection carries across every page. fields is NOT in the continuation fingerprint (it is a projection, not a filter — changing it mid-stream keeps the cursor valid). - GetAllTenantConfigurationsFunction projects via TenantConfigProjection.ProjectAll. - MCP list_tenants passes fields to the backend (followNextLink params) and drops the client-side projectTenantFields; extractTenantList stays as defense-in-depth. Now every page returns exactly the requested columns and the backend sends less over the wire. Tests: TenantConfigProjection (incl. secret-cannot-be-requested) + pagination fields-echo; backend 24/24, MCP vitest 52/52. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../TenantConfigPaginationTests.cs | 22 ++++- .../TenantConfigProjectionTests.cs | 97 +++++++++++++++++++ .../GetAllTenantConfigurationsFunction.cs | 19 +--- .../Helpers/TenantConfigProjection.cs | 74 ++++++++++++++ .../Pagination/TenantConfigPagination.cs | 20 +++- .../src/__tests__/security-guards.test.ts | 28 +----- .../autopilot-monitor-mcp/src/tools/admin.ts | 37 ++----- 7 files changed, 224 insertions(+), 73 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigProjectionTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigProjection.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigPaginationTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigPaginationTests.cs index d082605e..a556c201 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigPaginationTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigPaginationTests.cs @@ -36,11 +36,20 @@ public class TenantConfigPaginationTests { { "pageSize", "100" }, { "continuation", "ENCODED" }, + { "fields", "tenantId,domainName" }, }); Assert.Null(parsed.Error); Assert.Equal(100, parsed.PageSize); Assert.Equal("ENCODED", parsed.Continuation); + Assert.Equal("tenantId,domainName", parsed.Fields); + } + + [Fact] + public void ParseQuery_leaves_fields_null_when_absent() + { + var parsed = TenantConfigPagination.ParseQuery(new NameValueCollection { { "pageSize", "100" } }); + Assert.Null(parsed.Fields); } [Fact] @@ -121,10 +130,21 @@ public class TenantConfigPaginationTests [Fact] public void BuildNextLink_targets_config_all_with_pageSize_and_escaped_continuation() { - var link = TenantConfigPagination.BuildNextLink(100, "TOKEN+/="); + var link = TenantConfigPagination.BuildNextLink(100, "TOKEN+/=", fields: null); Assert.StartsWith("/api/config/all?", link); Assert.Contains("pageSize=100", link); Assert.Contains("continuation=TOKEN%2B%2F%3D", link); + Assert.DoesNotContain("fields=", link); + } + + [Fact] + public void BuildNextLink_echoes_fields_so_the_projection_round_trips() + { + var link = TenantConfigPagination.BuildNextLink(100, "abc", fields: "tenantId,domainName"); + + Assert.Contains("pageSize=100", link); + Assert.Contains("continuation=abc", link); + Assert.Contains("fields=tenantId%2CdomainName", link); } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigProjectionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigProjectionTests.cs new file mode 100644 index 00000000..bbe675eb --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigProjectionTests.cs @@ -0,0 +1,97 @@ +using AutopilotMonitor.Functions.Helpers; +using AutopilotMonitor.Shared.Models; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Server-side keep-list projection for the paginated config/all surface. This is +/// the backend security boundary that keeps tenant secrets off the wire even when a +/// caller supplies fields= — requested keys are intersected with the keep-list, never +/// widened. tenantId is always present. +/// +public class TenantConfigProjectionTests +{ + private static TenantConfiguration FullConfig() => new() + { + TenantId = "contoso-tenant-id", + DomainName = "contoso.example.com", + PlanTier = "pro", + Disabled = true, + DisabledReason = "suspended", + OnboardedBy = "alice@contoso.example.com", + DataRetentionDays = 180, + // secret-bearing fields that must never be projected: + TeamsWebhookUrl = "https://contoso.webhook.office.com/secret", + DiagnosticsBlobSasUrl = "https://blob.core.windows.net/c?sig=SECRETSAS", + }; + + private static readonly string[] AllSafeKeys = + { + "tenantId", "domainName", "planTier", "disabled", "disabledReason", + "onboardedAt", "onboardedBy", "lastUpdated", "dataRetentionDays", + }; + + [Fact] + public void Project_with_null_fields_emits_every_safe_field_and_no_secret() + { + var p = TenantConfigProjection.Project(FullConfig(), requested: null); + + foreach (var key in AllSafeKeys) Assert.True(p.ContainsKey(key), $"missing {key}"); + Assert.Equal(AllSafeKeys.Length, p.Count); + + // Belt-and-suspenders: no projected value carries a secret marker. + var serialized = System.Text.Json.JsonSerializer.Serialize(p); + Assert.DoesNotContain("sig=", serialized); + Assert.DoesNotContain("webhook", serialized); + Assert.DoesNotContain("SECRET", serialized); + } + + [Fact] + public void Project_honours_a_requested_subset() + { + var requested = TenantConfigProjection.ParseFields("domainName,planTier"); + var p = TenantConfigProjection.Project(FullConfig(), requested); + + Assert.Equal(new[] { "domainName", "planTier", "tenantId" }, p.Keys.OrderBy(k => k)); + } + + [Fact] + public void Project_always_includes_tenantId_even_when_not_requested() + { + var requested = TenantConfigProjection.ParseFields("domainName"); + var p = TenantConfigProjection.Project(FullConfig(), requested); + + Assert.True(p.ContainsKey("tenantId")); + Assert.Equal("contoso-tenant-id", p["tenantId"]); + } + + [Fact] + public void Project_silently_drops_unknown_or_secret_field_requests() + { + // A caller cannot widen the projection to a secret (or any non-safe key). + var requested = TenantConfigProjection.ParseFields("tenantId,teamsWebhookUrl,bogus"); + var p = TenantConfigProjection.Project(FullConfig(), requested); + + Assert.Equal(new[] { "tenantId" }, p.Keys); + Assert.DoesNotContain("teamsWebhookUrl", p.Keys); + } + + [Fact] + public void ParseFields_is_case_insensitive_and_tolerates_whitespace_and_blanks() + { + var requested = TenantConfigProjection.ParseFields(" TENANTID , , DomainName "); + var p = TenantConfigProjection.Project(FullConfig(), requested); + + Assert.Equal(new[] { "domainName", "tenantId" }, p.Keys.OrderBy(k => k)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData(",, ,")] + public void ParseFields_returns_null_for_empty_input(string? fields) + { + Assert.Null(TenantConfigProjection.ParseFields(fields)); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs index 6ba80f62..4e9eb04f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Net; using System.Threading.Tasks; using System.Web; @@ -81,26 +80,16 @@ namespace AutopilotMonitor.Functions.Functions.Config var page = await _configService.GetConfigurationsPageAsync(parsed.PageSize.Value, azureToken); - // Keep-list projection — only non-sensitive identity / lifecycle / plan fields. - var tenants = page.Items.Select(c => new - { - tenantId = c.TenantId, - domainName = c.DomainName, - planTier = c.PlanTier, - disabled = c.Disabled, - disabledReason = c.DisabledReason, - onboardedAt = c.OnboardedAt, - onboardedBy = c.OnboardedBy, - lastUpdated = c.LastUpdated, - dataRetentionDays = c.DataRetentionDays, - }).ToList(); + // Keep-list projection — only non-sensitive fields, optionally narrowed to the + // caller's fields= subset. Secrets can never be selected (intersection only). + var tenants = TenantConfigProjection.ProjectAll(page.Items, parsed.Fields); string? nextLink = null; if (!string.IsNullOrEmpty(page.NextRawToken)) { var fp = TenantConfigPagination.Fingerprint(callerTenantId); var wireToken = ContinuationToken.Encode(page.NextRawToken!, callerTenantId, fp); - nextLink = TenantConfigPagination.BuildNextLink(parsed.PageSize.Value, wireToken); + nextLink = TenantConfigPagination.BuildNextLink(parsed.PageSize.Value, wireToken, parsed.Fields); } return await req.OkAsync(new diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigProjection.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigProjection.cs new file mode 100644 index 00000000..419a9a20 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/TenantConfigProjection.cs @@ -0,0 +1,74 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using AutopilotMonitor.Shared.Models; + +namespace AutopilotMonitor.Functions.Helpers +{ + /// + /// Server-side keep-list projection for the paginated GET /api/config/all surface. + /// Only non-sensitive identity / lifecycle / plan fields are ever emitted — secrets + /// (webhook / SAS / branding URLs, allow-lists) can never leave the backend even if a + /// caller requests them via fields=, because the requested set is intersected with + /// this keep-list (unknown keys are silently dropped, never widened). tenantId is + /// always included: the endpoint's whole purpose is tenant-ID discovery. + /// + public static class TenantConfigProjection + { + // Ordered keep-list: JSON key (camelCase — the wire contract the web + MCP already + // consume) -> value selector. This is the ONLY set of fields that can reach the wire. + private static readonly (string Key, Func Value)[] SafeFields = + { + ("tenantId", c => c.TenantId), + ("domainName", c => c.DomainName), + ("planTier", c => c.PlanTier), + ("disabled", c => c.Disabled), + ("disabledReason", c => c.DisabledReason), + ("onboardedAt", c => c.OnboardedAt), + ("onboardedBy", c => c.OnboardedBy), + ("lastUpdated", c => c.LastUpdated), + ("dataRetentionDays", c => c.DataRetentionDays), + }; + + /// + /// Parse a comma-separated field list into a case-insensitive set, or null when no + /// usable subset was requested (→ emit all safe fields). Keys outside the keep-list + /// are kept in the set but simply never match a real field in . + /// + public static HashSet? ParseFields(string? fields) + { + if (string.IsNullOrWhiteSpace(fields)) return null; + var requested = fields + .Split(',') + .Select(f => f.Trim()) + .Where(f => f.Length > 0) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + return requested.Count > 0 ? requested : null; + } + + /// + /// Project one tenant to a { jsonKey -> value } map containing only the requested + /// safe fields (or all safe fields when is null). + /// tenantId is always present regardless of the request. + /// + public static Dictionary Project(TenantConfiguration config, HashSet? requested) + { + if (config == null) throw new ArgumentNullException(nameof(config)); + var result = new Dictionary(SafeFields.Length); + foreach (var (key, value) in SafeFields) + { + if (requested == null || key == "tenantId" || requested.Contains(key)) + result[key] = value(config); + } + return result; + } + + /// Convenience: parse + project a sequence in one call. + public static List> ProjectAll( + IEnumerable configs, string? fields) + { + var requested = ParseFields(fields); + return configs.Select(c => Project(c, requested)).ToList(); + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Pagination/TenantConfigPagination.cs b/src/Backend/AutopilotMonitor.Functions/Pagination/TenantConfigPagination.cs index a87317f0..3c3951f1 100644 --- a/src/Backend/AutopilotMonitor.Functions/Pagination/TenantConfigPagination.cs +++ b/src/Backend/AutopilotMonitor.Functions/Pagination/TenantConfigPagination.cs @@ -31,6 +31,12 @@ namespace AutopilotMonitor.Functions.Pagination /// Null when the caller did not opt into pagination (legacy bare-array mode). public int? PageSize { get; init; } public string? Continuation { get; init; } + /// + /// Raw comma-separated field subset (e.g. "tenantId,domainName"), or null for all + /// safe fields. A projection, not a filter — it does NOT enter the continuation + /// fingerprint (changing it mid-stream keeps the cursor valid). + /// + public string? Fields { get; init; } public string? Error { get; init; } } @@ -38,6 +44,7 @@ namespace AutopilotMonitor.Functions.Pagination { var pageSizeRaw = query?["pageSize"]; var continuationRaw = query?["continuation"]; + var fieldsRaw = query?["fields"]; int? pageSize = null; if (!string.IsNullOrEmpty(pageSizeRaw)) @@ -54,7 +61,12 @@ namespace AutopilotMonitor.Functions.Pagination ? continuationRaw : null; - return new Parsed { PageSize = pageSize, Continuation = continuation }; + return new Parsed + { + PageSize = pageSize, + Continuation = continuation, + Fields = string.IsNullOrEmpty(fieldsRaw) ? null : fieldsRaw, + }; } public static bool TryAcceptContinuation( @@ -67,12 +79,16 @@ namespace AutopilotMonitor.Functions.Pagination return ContinuationToken.TryDecode(raw, callerTenantId, fp, out azureToken, out rejectReason); } - public static string BuildNextLink(int pageSize, string wireContinuation) + public static string BuildNextLink(int pageSize, string wireContinuation, string? fields) { var sb = new StringBuilder("/api/config/all"); sb.Append('?'); sb.Append("pageSize=").Append(pageSize.ToString(CultureInfo.InvariantCulture)); sb.Append("&continuation=").Append(System.Uri.EscapeDataString(wireContinuation)); + // Echo the projection so the caller can follow nextLink verbatim and keep the + // same lean column set across every page (fields is not in the token fingerprint). + if (!string.IsNullOrEmpty(fields)) + sb.Append("&fields=").Append(System.Uri.EscapeDataString(fields!)); return sb.ToString(); } } diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts index 162dadbd..6cb730a9 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -18,7 +18,7 @@ import { followNextLink } from '../client.js'; import { SessionIdSchema } from '../tools/shared.js'; import { ApiError } from '../client.js'; import { toolError } from '../tools/error-handler.js'; -import { extractTenantList, TENANT_SAFE_FIELDS, projectTenantFields } from '../tools/admin.js'; +import { extractTenantList, TENANT_SAFE_FIELDS } from '../tools/admin.js'; // Importing the OAuth helper requires the env var that gates module load. // Set a dummy value before the import resolves so the throw doesn't fire. @@ -481,29 +481,3 @@ describe('list_tenants — extractTenantList keep-list projection', () => { }); }); -describe('list_tenants — projectTenantFields client-side field trim', () => { - const tenants: Record[] = [ - { tenantId: 'aaa', domainName: 'alpha.example.com', planTier: 'pro', disabled: true }, - { tenantId: 'bbb', domainName: 'beta.onmicrosoft.com', planTier: 'free', disabled: false }, - ]; - - it('returns rows unchanged when no fields are given', () => { - expect(projectTenantFields(tenants, undefined)).toEqual(tenants); - expect(projectTenantFields(tenants, '')).toEqual(tenants); - }); - - it('keeps only the requested fields', () => { - const out = projectTenantFields(tenants, 'domainName'); - for (const t of out) expect(Object.keys(t).sort()).toEqual(['domainName', 'tenantId']); - }); - - it('always retains tenantId even when not requested', () => { - const out = projectTenantFields(tenants, 'planTier'); - expect(Object.keys(out[0]).sort()).toEqual(['planTier', 'tenantId']); - }); - - it('tolerates whitespace and empty entries in the fields list', () => { - const out = projectTenantFields(tenants, ' domainName , , planTier '); - expect(Object.keys(out[0]).sort()).toEqual(['domainName', 'planTier', 'tenantId']); - }); -}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts index 7e8706c8..629d13d4 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts @@ -40,27 +40,6 @@ export function extractTenantList(data: unknown): Record[] { }); } -/** - * Keep only a comma-separated subset of fields from already-safe tenant rows. - * `tenantId` is always retained — the tool's whole purpose is discovering tenant - * IDs for other tools. Pagination-independent (operates on whatever page came - * back), so it never interacts with the backend cursor. - */ -export function projectTenantFields( - rows: Record[], - fields: string | undefined, -): Record[] { - if (!fields) return rows; - const keep = new Set( - fields.split(',').map((f) => f.trim()).filter(Boolean).concat(['tenantId']), - ); - return rows.map((row) => { - const out: Record = {}; - for (const key of Object.keys(row)) if (keep.has(key)) out[key] = row[key]; - return out; - }); -} - export function registerAdminTools(server: McpServer): void { // Tool 11: get_api_usage server.tool( @@ -274,17 +253,18 @@ export function registerAdminTools(server: McpServer): void { 'Global Admin only. Use this to discover tenant IDs for the tenantId parameter of other tools when running ' + 'cross-tenant investigations. Returns only non-sensitive fields — secrets (webhook URLs, SAS URLs) are stripped ' + 'server-side. Tenants are sorted by tenantId and returned in pages (default 100). For lean ID discovery pass ' + - '`fields=tenantId,domainName`. ' + + '`fields=tenantId,domainName` — the projection is applied server-side and is echoed in nextLink, so it carries ' + + 'across every page automatically. ' + 'Pagination: when "nextLink" is present, more tenants are available — call again and pass that whole string ' + 'back as "continuation". Stop when nextLink is absent.', { fields: z.string().optional() .describe('Comma-separated subset of safe fields to return (e.g. "tenantId,domainName" for lean ID discovery). ' + - 'tenantId is always included. Default: all safe fields. Trims the returned page client-side.'), + 'tenantId is always included. Default: all safe fields. Applied server-side; unknown/secret keys are ignored.'), pageSize: z.coerce.number().int().min(1).max(1000).optional().default(100) .describe('Page size (1-1000, default 100). Tenants are sorted by tenantId; follow nextLink to fetch more.'), continuation: z.string().optional() - .describe('Either the opaque "continuation" value from a prior response or the full nextLink path — both are accepted; the latter is preferred so backend-echoed query params round-trip correctly.'), + .describe('Either the opaque "continuation" value from a prior response or the full nextLink path — both are accepted; the latter is preferred so backend-echoed query params (pageSize, fields) round-trip correctly.'), }, READ_ONLY, async (args) => withToolTelemetry('list_tenants', async () => { @@ -292,11 +272,12 @@ export function registerAdminTools(server: McpServer): void { const { fields, pageSize, continuation } = args; // /api/config/all supports opt-in pagination: passing pageSize switches the // backend from its legacy unpaginated bare array to a secret-stripped - // { count, tenants, nextLink } envelope. extractTenantList re-applies the - // keep-list as defense-in-depth; projectTenantFields honours an optional fields= trim. - const path = followNextLink('/api/config/all', { pageSize }, continuation); + // { count, tenants, nextLink } envelope, with fields= projected server-side + // and echoed in nextLink. extractTenantList re-applies the keep-list as + // defense-in-depth (harmless pass-through once the backend has projected). + const path = followNextLink('/api/config/all', { pageSize, fields }, continuation); const data = await apiFetch(path) as { count?: number; tenants?: unknown[]; nextLink?: string | null }; - const tenants = projectTenantFields(extractTenantList({ tenants: data?.tenants ?? [] }), fields); + const tenants = extractTenantList({ tenants: data?.tenants ?? [] }); return toolResultText( { count: tenants.length, tenants, nextLink: data?.nextLink ?? null }, MAX_RESULT_SIZE_CHARS.adminStream);