Skip to content

fix: MSRC Incident-31000000666371 - add authorization filtering to de… - #3737

Open
anushakolan wants to merge 5 commits into
mainfrom
fix/msrc-mcp-describe-entities-authz
Open

fix: MSRC Incident-31000000666371 - add authorization filtering to de…#3737
anushakolan wants to merge 5 commits into
mainfrom
fix/msrc-mcp-describe-entities-authz

Conversation

@anushakolan

@anushakolan anushakolan commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Why make this change?

Resolves MSRC Incident-31000000666371: Information Disclosure vulnerability in the MCP describe_entities tool.

The describe_entities MCP tool was returning schema metadata (entity names, field names, parameters, descriptions) for all configured entities without per-entity authorization checks. This allowed unauthenticated or low-privilege users to enumerate the complete database surface, including entities they should not have access to.

What is this change?

Added per-entity authorization filtering to describe_entities to align with REST, GraphQL, and OpenAPI interfaces.

  • Added HasAnyPermissionForEntity() — checks if any of the caller's roles has permissions on an entity
  • Inserted authorization check in the entity loop; entities without matching role permissions are skipped
  • Updated BuildPermissionsInfo() to return the union of permissions across all caller roles
  • Removed duplicate role-extraction block (cleanup)

Multi-role behavior: The caller's X-MS-API-ROLE header may contain comma-separated roles (e.g. reader,admin). An entity is included if any role grants access, and the permissions shown are the union across all roles — consistent with McpAuthorizationHelper.TryResolveAuthorizedRole used by other MCP tools.

How was this tested?

Unit Tests — 15/15 passing ✅

Test What it validates
DescribeEntities_RoleWithNoPermissions_ReturnsNoEntitiesError Role with no entity permissions sees nothing
DescribeEntities_LowPrivRole_SeesOnlyAuthorizedEntities reader sees Book, not GetBook (admin-only)
DescribeEntities_NoRole_ReturnsNoEntitiesError No role header → empty result
DescribeEntities_MultiRole_ReturnsUnionOfAuthorizedEntities reader,admin sees entities from both roles
11 existing dml-tools filtering tests No regression

Manual End-to-End Testing ✅

Setup: Two entities with role-based permissions, DAB on localhost:5000, SimulatorAuthentication.

  • Book: reader=READ, admin=ALL
  • Secret: admin=ALL only

Authorization scenarios:

Role header Entities returned Result
(none) (empty)
reader Book
admin Book, Secret
reader,admin Book, Secret (union)

Permissions union (reader,admin on Book): CREATE, DELETE, READ, UPDATE — union of reader's READ and admin's ALL. ✅

Existing features unaffected:

  • nameOnly=true — lightweight listing still works ✅
  • Entity filter (entities: ["Book"]) — still works ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses an information disclosure vulnerability in the MCP describe_entities tool by introducing per-entity authorization filtering, so schema metadata is only returned for entities the caller is permitted to access (bringing MCP discovery behavior closer to other DAB surfaces).

Changes:

  • Added authorization-based entity filtering to describe_entities via a new HasAnyPermissionForEntity helper.
  • Added unit tests covering “no permissions”, “low privilege”, and “no role” scenarios for describe_entities filtering.
  • Updated MCP test harness to allow specifying (or omitting) the request role header in the mocked HttpContext.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs Adds per-entity authorization filtering to prevent disclosure of metadata for unauthorized entities.
src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs Adds new tests for role-based filtering and updates helpers to simulate different role contexts.
Comments suppressed due to low confidence (1)

src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs:305

  • This test name says "ReturnsEmptyList", but the assertion expects an error result (NoEntitiesConfigured). Renaming to match the actual expectation will make the test intent clearer and avoid confusion for future readers.
        /// <summary>
        /// Verifies that a null/empty role (unauthenticated caller)
        /// receives no entities, even if some entities have "anonymous" permissions.
        /// describe_entities requires a valid role to be included in the response.
        /// </summary>
        [TestMethod]
        public async Task DescribeEntities_NoRole_ReturnsEmptyList()
        {
            // Arrange - Config with entities
            RuntimeConfig config = CreateConfigWithMixedEntityTypes();
            IServiceProvider serviceProvider = CreateServiceProvider(config, role: null);
            DescribeEntitiesTool tool = new();

            // Act
            CallToolResult result = await tool.ExecuteAsync(null, serviceProvider, CancellationToken.None);

            // Assert - No role should result in empty entity list
            AssertErrorResult(result, "NoEntitiesConfigured");
        }

Comment thread src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs Outdated
Comment thread src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs
// matching the behavior of other MCP tools (McpAuthorizationHelper.TryResolveAuthorizedRole)
// and REST/GraphQL endpoints.
string[]? currentUserRoles = null;
if (httpContext != null && authResolver.IsValidRoleContext(httpContext))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsValidRoleContext() validates the complete X-MS-API-ROLE value as one role by calling User.IsInRole(roleHeader), but this code then splits that validated value and authorizes each component independently.

For X-MS-API-ROLE: reader,admin, a principal with separate reader and admin claims will fail validation because IsInRole("reader,admin") is false, so the advertised multi-role behavior does not work through the real resolver. Conversely, a principal with one literal reader,admin role claim passes validation and is then reinterpreted as possessing the separate reader and admin roles. That can expand the validated authorization context, and custom role strings are not schema-restricted from containing commas.

I think we should follow DAB's existing single-role request model. If multi-role authorization is intended, each parsed role must be independently validated against HttpContext.User, and I think that behavior should probably be in the role-context middleware/helper rather than only in describe_entities.

/// <param name="entity">The entity to check.</param>
/// <param name="roles">The roles to check permissions for. If null or empty, the entity is not accessible.</param>
/// <returns><see langword="true"/> if any role has permission on the entity; otherwise, <see langword="false"/>.</returns>
private static bool HasAnyPermissionForEntity(Entity entity, string[]? roles)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this checks raw entity.Permissions for an exact role-name match, but I think we should use the IAuthorizationResolver for this.

Consider when an entity grants READ to anonymous, the authorization resolver copies that permission to authenticated doesn't it? And can't unconfigured named roles inherit from authenticated?

BuildPermissionsInfo() has the same issue.

Can we derive from IAuthorizationResolver.AreRoleAndOperationDefinedForEntity(entityName, role, operation) for each valid operation, and use that same set both to decide entity visibility and to populate the permissions response. This keeps the resolver as the single source of truth for inheritance and operation expansion.

/// A dictionary containing the entity's name, description, fields, parameters (if applicable), and permissions.
/// </returns>
private static Dictionary<string, object?> BuildFullEntityInfo(string entityName, Entity entity, string? currentUserRole, DatabaseObject? databaseObject)
private static Dictionary<string, object?> BuildFullEntityInfo(string entityName, Entity entity, string[]? currentUserRoles, DatabaseObject? databaseObject)

@aaronburtle aaronburtle Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the entity-level filtering does not fully address the reported metadata disclosure because BuildFullEntityInfo() still passes every configured field to BuildFieldMetadataInfo().

If a role can read an entity but its action limits fields with fields.include/fields.exclude, the response still exposes the names and descriptions of fields that role cannot access. The MSRC seems to not want those kinds of field disclosures.

If we don't want to expose that information, can we instead calculate the union of allowed exposed columns across the caller's authorized operations using IAuthorizationResolver.GetAllowedExposedColumns(), then filter entity.Fields by field.Alias ?? field.Name before projecting the metadata?

@@ -464,13 +621,23 @@ private static IServiceProvider CreateServiceProvider(RuntimeConfig config)

// Mock IAuthorizationResolver
Mock<IAuthorizationResolver> mockAuthResolver = new();

@aaronburtle aaronburtle Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test currently mocks IsValidRoleContext() as true for every non-null role string, so it bypasses the production authorization boundary.

Can we exercise this scenario with a real DefaultHttpContext, actual role claims, and the real authorization resolver (or otherwise mock the exact claim/header semantics). Could this test also cover authenticated inheriting anonymous and a named role inheriting authenticated, as I dont think we cover that scenario fully.

CallToolResult result = await tool.ExecuteAsync(null, serviceProvider, CancellationToken.None);

// Assert - Union of reader + admin → both Book and GetBook visible
AssertSuccessResultWithEntityNames(result, new[] { "Book", "GetBook" }, Array.Empty<string>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me that this test verifies only the union of visible entity names rather than the permission union claimed by the PR, because AssertSuccessResultWithEntityNames() never inspects the permissions property.

Can we assert that Book returns exactly CREATE, DELETE, READ, and UPDATE, and that GetBook returns exactly EXECUTE. Otherwise I think BuildPermissionsInfo() can regress or return incomplete inherited permissions while this test continues to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mcp-server security

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants