Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anushakolan wants to merge 5 commits into
base: main
Choose a base branch
from
+252 −58
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f6e097
fix: MSRC Incident-31000000666371 - add authorization filtering to de…
anushakolan Jul 27, 2026
6ff4677
Fix unit test failures
anushakolan Jul 27, 2026
07383db
Fix failing MSSQL Tests.
anushakolan Jul 27, 2026
218e06d
Merge branch 'main' into fix/msrc-mcp-describe-entities-authz
anushakolan Jul 28, 2026
4408964
fix(mcp): support multi-role authorization in describe_entities with …
anushakolan Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 69 additions & 53 deletions src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,58 +89,26 @@ public Task<CallToolResult> ExecuteAsync(
IHttpContextAccessor httpContextAccessor = serviceProvider.GetRequiredService<IHttpContextAccessor>();
HttpContext? httpContext = httpContextAccessor.HttpContext;

// Get current user's role for permission filtering
// For discovery tools like describe_entities, we use the first valid role from the header
// This differs from operation-specific tools that check permissions per entity per operation
string? currentUserRole = null;
// Get the caller's roles for authorization filtering.
// All roles from the header are collected and an entity is visible if ANY role grants access,
// matching the behavior of other MCP tools (McpAuthorizationHelper.TryResolveAuthorizedRole)
// and REST/GraphQL endpoints.
string[]? currentUserRoles = null;
if (httpContext != null && authResolver.IsValidRoleContext(httpContext))

@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.

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.

{
string roleHeader = httpContext.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER].ToString();
if (!string.IsNullOrWhiteSpace(roleHeader))
{
string[] roles = roleHeader
.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);

if (roles.Length > 1)
{
logger?.LogWarning("Multiple roles detected in request header: [{Roles}]. Using first role '{FirstRole}' for entity discovery. " +
"Consider using a single role for consistent permission reporting.",
string.Join(", ", roles), roles[0]);
}

// For discovery operations, take the first role from comma-separated list
// This provides a consistent view of available entities for the primary role
currentUserRole = roles.FirstOrDefault();
}
}

// Get current user's role for permission filtering
// For discovery tools like describe_entities, we use the first valid role from the header
// This differs from operation-specific tools that check permissions per entity per operation
if (httpContext != null && authResolver.IsValidRoleContext(httpContext))
{
string roleHeader = httpContext.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER].ToString();
if (!string.IsNullOrWhiteSpace(roleHeader))
{
string[] roles = roleHeader
.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);

if (roles.Length > 1)
{
logger?.LogWarning("Multiple roles detected in request header: [{Roles}]. Using first role '{FirstRole}' for entity discovery. " +
"Consider using a single role for consistent permission reporting.",
string.Join(", ", roles), roles[0]);
}

// For discovery operations, take the first role from comma-separated list
// This provides a consistent view of available entities for the primary role
currentUserRole = roles.FirstOrDefault();
currentUserRoles = roleHeader
.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)
.Distinct(StringComparer.OrdinalIgnoreCase)
.ToArray();
}
}

(bool nameOnly, HashSet<string>? entityFilter) = ParseArguments(arguments, logger);

if (currentUserRole == null)
if (currentUserRoles == null)
{
logger?.LogWarning("Current user role could not be determined from HTTP context or role header. " +
"Entity permissions will be empty (no permissions shown) rather than using anonymous permissions. " +
Expand Down Expand Up @@ -178,6 +146,15 @@ public Task<CallToolResult> ExecuteAsync(
continue;
}

// Authorization filtering: skip entities none of the caller's roles have permission on.
// This prevents information disclosure of schema metadata (entity/field/parameter names and descriptions)
// for entities the caller is not authorized to access, matching REST/GraphQL/OpenAPI behavior.
// If currentUserRoles is null or empty, no entities are visible (empty result).
if (!HasAnyPermissionForEntity(entity, currentUserRoles))
{
continue;
}

try
{
DatabaseObject? databaseObject = null;
Expand All @@ -204,7 +181,7 @@ public Task<CallToolResult> ExecuteAsync(

Dictionary<string, object?> entityInfo = nameOnly
? BuildBasicEntityInfo(entityName, entity)
: BuildFullEntityInfo(entityName, entity, currentUserRole, databaseObject);
: BuildFullEntityInfo(entityName, entity, currentUserRoles, databaseObject);

entityList.Add(entityInfo);
}
Expand Down Expand Up @@ -401,6 +378,45 @@ private static bool ShouldIncludeEntity(string entityName, HashSet<string>? enti
return entityFilter == null || entityFilter.Count == 0 || entityFilter.Contains(entityName);
}

/// <summary>
/// Determines whether the specified entity is accessible to any of the given roles.
/// An entity is accessible if at least one role has at least one permission defined for it.
/// This prevents information disclosure of schema metadata (entity names, fields, parameters, descriptions)
/// for unauthorized entities, matching REST/GraphQL/OpenAPI authorization behavior.
/// </summary>
/// <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)

@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.

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.

{
// No roles = no access to any entity (matches DML tool authorization model)
if (roles == null || roles.Length == 0)
{
return false;
}

// No permissions defined = not accessible
if (entity.Permissions == null || !entity.Permissions.Any())
{
return false;
}

// Entity is accessible if ANY of the caller's roles has permissions (actions) defined for it
foreach (EntityPermission permission in entity.Permissions)
{
if (roles.Any(role => string.Equals(permission.Role, role, StringComparison.OrdinalIgnoreCase)))
{
// Role found - check if it has any actions
if (permission.Actions != null && permission.Actions.Any())
{
return true;
}
}
}

return false;
}

/// <summary>
/// Creates a dictionary containing basic information about an entity.
/// </summary>
Expand Down Expand Up @@ -432,7 +448,7 @@ private static bool ShouldIncludeEntity(string entityName, HashSet<string>? enti
/// <returns>
/// 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 •
edited
Loading

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?

{
// Use GraphQL singular name as alias if available, otherwise use entity name
string displayName = !string.IsNullOrWhiteSpace(entity.GraphQL?.Singular)
Expand All @@ -451,7 +467,7 @@ private static bool ShouldIncludeEntity(string entityName, HashSet<string>? enti
info["parameters"] = BuildParameterMetadataInfo(databaseObject);
}

info["permissions"] = BuildPermissionsInfo(entity, currentUserRole);
info["permissions"] = BuildPermissionsInfo(entity, currentUserRoles);

return info;
}
Expand Down Expand Up @@ -539,14 +555,14 @@ private static List<object> BuildParameterMetadataInfo(DatabaseObject? databaseO
};

/// <summary>
/// Build a list of permission metadata info for the current user's role
/// Build a union of permission metadata for all of the current user's roles.
/// </summary>
/// <param name="entity">The entity object</param>
/// <param name="currentUserRole">The current user's role - if null, returns empty permissions</param>
/// <returns>A list of permissions available to the current user's role for this entity</returns>
private static string[] BuildPermissionsInfo(Entity entity, string? currentUserRole)
/// <param name="currentUserRoles">The current user's roles - if null or empty, returns empty permissions</param>
/// <returns>A sorted list of permissions available to any of the current user's roles for this entity</returns>
private static string[] BuildPermissionsInfo(Entity entity, string[]? currentUserRoles)
{
if (entity.Permissions == null || string.IsNullOrWhiteSpace(currentUserRole))
if (entity.Permissions == null || currentUserRoles == null || currentUserRoles.Length == 0)
{
return Array.Empty<string>();
}
Expand All @@ -558,11 +574,11 @@ private static string[] BuildPermissionsInfo(Entity entity, string? currentUserR

HashSet<string> permissions = new(StringComparer.OrdinalIgnoreCase);

// Only include permissions for the current user's role
// Include permissions for any of the current user's roles (union)
foreach (EntityPermission permission in entity.Permissions)
{
// Check if this permission applies to the current user's role
if (!string.Equals(permission.Role, currentUserRole, StringComparison.OrdinalIgnoreCase))
// Check if this permission applies to any of the current user's roles
if (!currentUserRoles.Any(role => string.Equals(permission.Role, role, StringComparison.OrdinalIgnoreCase)))
{
continue;
}
Expand Down
Loading