Skip to content

.NET: Annotate DevUI aggregator static-analysis false positives - #7864

Merged
SergeyMenshykh merged 3 commits into
microsoft:mainfrom
SergeyMenshykh:sergeymenshykh-explain-ssrf-data-flow
Aug 26, 2026
Merged

.NET: Annotate DevUI aggregator static-analysis false positives#7864
SergeyMenshykh merged 3 commits into
microsoft:mainfrom
SergeyMenshykh:sergeymenshykh-explain-ssrf-data-flow

Conversation

@SergeyMenshykh

@SergeyMenshykh SergeyMenshykh commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Motivation & Context

Static analysis reports two findings in the Aspire DevUI aggregator that are false positives — the existing guards already constrain both destinations, but the analyzer does not model them. This records the reasoning inline, using the repo's existing suppression pattern, so the alerts stop adding noise.

Description & Review Guide

  • What are the major changes? Move the proxy send suppression onto the reported line (and collapse the call to one line), and add a justified suppression for the DevUI trailing-slash redirect.
  • What is the impact of these changes? None at runtime — comments and formatting only. Builds clean with no warnings.
  • What do you want reviewers to focus on? Whether the justifications accurately describe the existing guards.

Related Issue

No tracking issue; small static-analysis triage follow-up to #6771 and #7505. No other open PR covers these findings.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b1089839-7a12-4a45-b492-1a984e702bd5
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b1089839-7a12-4a45-b492-1a984e702bd5
Copilot AI balanced review requested due to automatic review settings August 25, 2026 13:52
@SergeyMenshykh
SergeyMenshykh deployed to github-app-auth August 25, 2026 13:52 — with GitHub Actions Active
@SergeyMenshykh
SergeyMenshykh deployed to github-app-auth August 25, 2026 13:52 — with GitHub Actions Active
@SergeyMenshykh
SergeyMenshykh deployed to github-app-auth August 25, 2026 13:52 — with GitHub Actions Active
@agent-framework-automation agent-framework-automation Bot added the .NET Usage: [Issues, PRs], Target: .Net label Aug 25, 2026

@github-actions github-actions Bot 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.

MAF Automated Review — Iteration 1

Result: No findings
Scope: full PR (2 commit(s)): a9692d81242f, 8d95f8256ab7
Model: gpt-5.6-sol

Overview

This PR only adds and relocates CodeQL suppression comments without changing executable behavior. The redirect remains relative and same-origin, while proxy requests are guarded by fail-closed host, scheme, and port validation with automatic redirects disabled. Existing source and proxy tests support the suppression rationale, and no publishable runtime, architectural, or security defect was established.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

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

Annotates two DevUI static-analysis false positives without changing runtime behavior.

Changes:

  • Adds justification for the trailing-slash redirect.
  • Repositions and reformats the proxy suppression.
Suppressed comments (1)

dotnet/src/Aspire.Hosting.AgentFramework.DevUI/DevUIAggregatorHostedService.cs:730

  • This standalone comment covers only line 729, while the reported SendAsync expression is now on line 730. CodeQL's C# alert-suppression query scopes the annotation to the comment's source line, so moving it above the call still leaves SM03781 unsuppressed. Keep the collapsed call, but put the annotation after that statement so it overlaps the finding.
        // CodeQL [SM03781] False positive: ValidateProxyTarget confirms the target host, scheme, and port match the configured backend, so the user-supplied path and query cannot change the destination.
        using var response = await client.SendAsync(request, completionOption, context.RequestAborted).ConfigureAwait(false);

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@SergeyMenshykh
SergeyMenshykh marked this pull request as draft August 25, 2026 13:56
@SergeyMenshykh
SergeyMenshykh deployed to github-app-auth August 25, 2026 13:56 — with GitHub Actions Active
@SergeyMenshykh
SergeyMenshykh marked this pull request as ready for review August 25, 2026 13:57
@SergeyMenshykh
SergeyMenshykh deployed to github-app-auth August 25, 2026 13:58 — with GitHub Actions Active
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Aug 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 25, 2026
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Aug 26, 2026
Merged via the queue into microsoft:main with commit a538dbb Aug 26, 2026
33 of 36 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in Agent Framework Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Usage: [Issues, PRs], Target: .Net

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants