Skip to content

Conversation

@anuchandy
Copy link
Member

What does this PR do?

External process commands (like azqr) use IExternalProcessService to spawn local processes. In HTTP + OBO mode, this is a security risk: processes run under the server's host identity (not the OBO user's context), and malicious requests could exhaust server resources.

  • Add IsHttpOnBehalfOfMode() method to ServiceStartOptions
  • Conditionally exclude AzqrCommand registration in ExtensionSetup

Fixes #1521

[Any additional context, screenshots, or information that helps reviewers]

GitHub issue number?

[Link to the GitHub issue this PR addresses]

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Updated servers/Azure.Mcp.Server/CHANGELOG.md and/or servers/Fabric.Mcp.Server/CHANGELOG.md for product changes (features, bug fixes, UI/UX, updated dependencies)
  • For MCP tool changes:
    • One tool per PR: This PR adds or modifies only one MCP tool for faster review cycles
    • Updated servers/Azure.Mcp.Server/README.md and/or servers/Fabric.Mcp.Server/README.md documentation
    • Validate README.md changes using script at eng/scripts/Process-PackageReadMe.ps1. See Package README
    • Updated command list in /servers/Azure.Mcp.Server/docs/azmcp-commands.md and/or /docs/fabric-commands.md
    • Run .\eng\scripts\Update-AzCommandsMetadata.ps1 to update tool metadata in azmcp-commands.md (required for CI)
    • For new or modified tool descriptions, ran ToolDescriptionEvaluator and obtained a score of 0.4 or more and a top 3 ranking for all related test prompts
    • For tools with new names, including new tools or renamed tools, update consolidated-tools.json
    • For new tools associated with Azure services or publicly available tools/APIs/products, add URL to documentation in the PR description
  • Extra steps for Azure MCP Server tool changes:
    • Updated test prompts in /servers/Azure.Mcp.Server/docs/e2eTestPrompts.md
    • 👉 For Community (non-Microsoft team member) PRs:
      • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
      • Manual tests run: added comment /azp run mcp - pullrequest - live to run Live Test Pipeline

@anuchandy anuchandy self-assigned this Jan 20, 2026
@anuchandy anuchandy requested a review from wbreza as a code owner January 20, 2026 02:45
Copilot AI review requested due to automatic review settings January 20, 2026 02:45
@anuchandy anuchandy requested review from a team and jongio as code owners January 20, 2026 02:45
@github-actions github-actions bot added the tools-Azd Azure Developer CLI related label Jan 20, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a security enhancement to disable external process commands (specifically azqr) when the MCP server is running in HTTP mode with On-Behalf-Of authentication. This prevents security risks where spawned processes would execute under the server's host identity rather than the user's context.

Changes:

  • Added IsHttpOnBehalfOfMode() helper method to ServiceStartOptions to detect HTTP + OBO mode
  • Implemented conditional registration logic in ExtensionSetup to exclude AzqrCommand in HTTP + OBO scenarios
  • Updated changelog to document the security enhancement

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
core/Azure.Mcp.Core/src/Areas/Server/Options/ServiceStartOptions.cs Adds helper method to detect HTTP + On-Behalf-Of mode
tools/Azure.Mcp.Tools.Extension/src/ExtensionSetup.cs Implements conditional registration to exclude azqr command in HTTP + OBO mode
servers/Azure.Mcp.Server/changelog-entries/1768876346795.yaml Documents the security feature addition

Comment on lines +89 to +93
public bool IsHttpOnBehalfOfMode()
{
return Transport == TransportTypes.Http
&& OutgoingAuthStrategy == OutgoingAuthStrategy.UseOnBehalfOf;
}
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The new method IsHttpOnBehalfOfMode() and the conditional registration logic in ExtensionSetup lack test coverage. Consider adding unit tests to verify:

  1. IsHttpOnBehalfOfMode() returns true when Transport is Http and OutgoingAuthStrategy is UseOnBehalfOf
  2. IsHttpOnBehalfOfMode() returns false for other combinations (stdio transport, different auth strategies, etc.)
  3. ShouldExposeExternalProcessCommands() correctly excludes azqr command registration when in HTTP + OBO mode
  4. ShouldExposeExternalProcessCommands() allows azqr command registration when ServiceStartOptions is null or not in HTTP + OBO mode

These tests would help ensure the security fix works as intended and prevent regressions.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like some good things to practice with having GitHub Copilot CLI or MCP server tools within VS Code follow.

@anuchandy anuchandy added remote-mcp and removed tools-Azd Azure Developer CLI related labels Jan 20, 2026
@joshfree joshfree moved this from Untriaged to In Progress in Azure MCP Server Jan 20, 2026
@joshfree joshfree added this to the 2026-01 milestone Jan 20, 2026
/// Determines whether the server is running in HTTP mode with On-Behalf-Of authentication.
/// </summary>
/// <returns>True if running in HTTP transport with On-Behalf-Of authentication; false otherwise.</returns>
public bool IsHttpOnBehalfOfMode()
Copy link
Member

@vukelich vukelich Jan 22, 2026

Choose a reason for hiding this comment

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

Subjective style: this can be a property which looks a subjectively cleaner with c# syntax.

[JsonIgnore]
public bool IsHttpOnBehalfOfMode => Transport == TransportTypes.Http && OutgoingAuthStrategy == OutgoingAuthStrategy.UseOnBehalfOf;

At the end of the day, both are creating parameterless methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - yes, that’s actually what Copilot suggested. I was thinking it could be a method since we’re "computing" a boolean (coming from a Java mindset), but let’s go with a property given that’s more idiomatic.

Copy link
Member

Choose a reason for hiding this comment

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

C# properties are a hybrid between fields and methods. While writing code, they can be referenced using field-like syntax (i.e., usage infers reading vs writing), but during compilation, they are translated into methods.

Image

Comment on lines +89 to +93
public bool IsHttpOnBehalfOfMode()
{
return Transport == TransportTypes.Http
&& OutgoingAuthStrategy == OutgoingAuthStrategy.UseOnBehalfOf;
}
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like some good things to practice with having GitHub Copilot CLI or MCP server tools within VS Code follow.

var extension = new CommandGroup(Name, "Extension commands for additional Azure tooling functionality. Includes running Azure Quick Review (azqr) commands directly from the MCP server to get service recommendations, generating Azure CLI commands from user intent, and getting installation instructions for Azure CLI, Azure Developer CLI and Azure Core Function Tools CLI.", Title);

// Azure CLI and Azure Developer CLI tools are hidden
// extension.AddCommand("az", new AzCommand(loggerFactory.CreateLogger<AzCommand>()));
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend removing this comment OR putting it into the new if block body related to external process commands. If it returns, I suspect we'd want it guarded by the same logic.

{
ServiceStartOptions? startOptions = serviceProvider.GetService<ServiceStartOptions>();

if (startOptions is null)
Copy link
Member

Choose a reason for hiding this comment

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

If you want some modern C# null variable checking syntactic sugar:

if (serviceProvider.GetService<ServiceStartOptions>() is ServiceStartOptions startOptions)
{
	return !startOptions.IsHttpOnBehalfOfMode();
}

return true/whateverDefault;

Totally optional. Current code is fine. Just teaching.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, will use type pattern matching here


public CommandGroup RegisterCommands(IServiceProvider serviceProvider)
{
var extension = new CommandGroup(Name, "Extension commands for additional Azure tooling functionality. Includes running Azure Quick Review (azqr) commands directly from the MCP server to get service recommendations, generating Azure CLI commands from user intent, and getting installation instructions for Azure CLI, Azure Developer CLI and Azure Core Function Tools CLI.", Title);
Copy link
Member

Choose a reason for hiding this comment

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

Will this message need to be updated to exclude azqr if we don't add it below?


if (startOptions is null)
{
// First container (CLI routing) - startOptions not available, allow all commands
Copy link
Member

Choose a reason for hiding this comment

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

First container

Now, I know what this means, but I doubt anyone else would. Please reference the Program.cs files in the servers folder where the multiple containers are detailed. Ugly for now, but referencing that design complexity in a cleaner way is out of scope for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - yes, the multi-container setup is pretty confusing :). Let me link to the entry point / program.cs where that is happening.

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

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

[BUG] Block external process commands (that spawn az, azd, etc.) from being invoked in remote mode

3 participants