Default telemetry API to disabled when frontend has auth#14990
Default telemetry API to disabled when frontend has auth#14990mitchdenny wants to merge 1 commit intorelease/13.2from
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14990Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14990" |
There was a problem hiding this comment.
Pull request overview
Adjusts Aspire Dashboard configuration so the Telemetry HTTP API defaults to disabled when the frontend is authenticated but the API would otherwise run unsecured (no API key), reducing the risk of exposing telemetry on the dashboard port.
Changes:
- Added post-configure logic to default
Dashboard:Api:Enabledtofalsewhen frontend auth is enabled and API auth would beUnsecured. - Expanded integration test coverage for the new default/override behaviors.
- Updated API option XML docs and the dashboard HTTP API spec to document the new default behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/Aspire.Dashboard.Tests/Integration/TelemetryApiTests.cs | Adds configuration-focused integration tests for API enabled/disabled defaults and overrides. |
| src/Aspire.Dashboard/Configuration/PostConfigureDashboardOptions.cs | Implements the new default-disable behavior for the telemetry API in authenticated frontend scenarios. |
| src/Aspire.Dashboard/Configuration/DashboardOptions.cs | Updates XML docs for ApiOptions.Enabled to describe the new defaults. |
| docs/specs/dashboard-http-api.md | Documents the updated default behavior for Dashboard:Api:Enabled and adds guidance on overriding it. |
| // to prevent unauthenticated access to telemetry data on the same port. | ||
| if (options.Api.Enabled is null && | ||
| options.Frontend.AuthMode is not FrontendAuthMode.Unsecured && | ||
| options.Api.AuthMode is ApiAuthMode.Unsecured) |
There was a problem hiding this comment.
The new default-disable condition is based on options.Api.AuthMode is ApiAuthMode.Unsecured, but the comment/doc say this should happen specifically when no API key is configured. As written, this can also disable the API when a key is configured but Api:AuthMode is explicitly set to Unsecured, which contradicts the intended rule and the updated docs. Consider checking key presence directly (e.g., string.IsNullOrEmpty(options.Api.PrimaryApiKey) (and/or secondary) in addition to Enabled is null and frontend auth mode) rather than relying on AuthMode alone.
| options.Api.AuthMode is ApiAuthMode.Unsecured) | |
| string.IsNullOrEmpty(options.Api.PrimaryApiKey) && | |
| string.IsNullOrEmpty(options.Api.SecondaryApiKey)) |
| // Assert - API should default to disabled when frontend has auth but no API key | ||
| var options = app.Services.GetRequiredService<IOptionsMonitor<DashboardOptions>>().CurrentValue; | ||
| Assert.False(options.Api.Enabled); | ||
| } |
There was a problem hiding this comment.
DashboardOptions.Api.Enabled is a nullable bool?, so using Assert.False(options.Api.Enabled) is ambiguous (and may not compile depending on Assert overloads) and can unintentionally treat null the same as false. Since this test is validating the new behavior of explicitly defaulting the API to disabled, assert the exact nullable value (e.g., that it is false).
|
|
||
| // Assert - API should be enabled because explicitly configured | ||
| var options = app.Services.GetRequiredService<IOptionsMonitor<DashboardOptions>>().CurrentValue; | ||
| Assert.True(options.Api.Enabled); |
There was a problem hiding this comment.
DashboardOptions.Api.Enabled is bool?. Assert.True(options.Api.Enabled) can be brittle/unclear for nullable values and may not be asserting what you intend if Enabled is ever null. Since this case sets Dashboard:Api:Enabled explicitly, assert that the value is exactly true (nullable comparison) to ensure the test is validating the explicit override behavior.
| Assert.True(options.Api.Enabled); | |
| Assert.Equal(true, options.Api.Enabled); |
| - The API shares the same port as the Dashboard frontend (default: 18888). | ||
| - Hosters may set `Enabled: false` to disable the API for security. | ||
| - When the frontend requires authentication (e.g., OpenID Connect) and no API key is configured, the API is disabled by default. Set `Enabled: true` explicitly to override. | ||
| - API keys are shared with MCP configuration—setting either configures both. |
There was a problem hiding this comment.
The note "API keys are shared with MCP configuration—setting either configures both" doesn’t match the current implementation in PostConfigureDashboardOptions (it only copies Api -> Mcp, not the other direction). With the updated defaulting rules for Api:Enabled, this doc can further confuse hosters who set only MCP keys and expect the API to be treated as keyed. Update the note to reflect the one-way fallback (or implement bidirectional behavior).
| - API keys are shared with MCP configuration—setting either configures both. | |
| - MCP configuration falls back to the Dashboard API keys when MCP keys are not set; configuring MCP keys does not configure Dashboard API keys or affect `Api:Enabled`. |
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22746216176 |
|
Replaced by #15154 |
Description
Default
Dashboard:Api:Enabledtofalsewhen frontend requires authentication and no API key is configured.Checklist