Fix dashboard URL in aspire describe to use returnUrl instead of path concat#15337
Fix dashboard URL in aspire describe to use returnUrl instead of path concat#15337adamint wants to merge 2 commits intomicrosoft:release/13.2from
Conversation
The dashboardBaseUrl passed to ResourceSnapshotMapper is a login URL with a token (e.g., https://host/login?t=TOKEN). CombineUrl naively appended the resource path with '/', producing malformed URLs like: https://host/login\?t\=TOKEN/\?resource\=name Use AddReturnUrl instead, which properly appends a returnUrl query parameter so the dashboard redirects after login: https://host/login\?t\=TOKEN\&returnUrl\=%2F%3Fresource%3Dname Fixes microsoft#15171
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15337Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15337" |
There was a problem hiding this comment.
Pull request overview
Fixes generation of per-resource dashboard links in the Aspire CLI when the provided dashboard URL is actually a login URL that already contains query parameters (e.g., .../login?t=TOKEN), by appending the resource destination via a returnUrl query parameter instead of path concatenation.
Changes:
- Update CLI resource mapping to build dashboard links using
returnUrlon the login URL (avoids malformed...login?t=TOKEN/?resource=...links). - Add
DashboardUrls.AddReturnUrl()helper to appendreturnUrlto an existing URL with query parameters. - Expand/adjust unit tests to validate correct dashboard URL construction and null handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Backchannel/ResourceSnapshotMapperTests.cs | Updates/extends tests to assert dashboard URLs use returnUrl and avoid malformed concatenation. |
| src/Shared/DashboardUrls.cs | Adds AddReturnUrl() helper that appends a returnUrl query parameter to an existing URL. |
| src/Aspire.Cli/Backchannel/ResourceSnapshotMapper.cs | Switches dashboard URL construction from CombineUrl() to AddReturnUrl() for login URLs with tokens. |
You can also share your feedback on Copilot code review. Take the survey.
| // Must NOT produce the old malformed URL: .../login?t=TOKEN/?resource=name | ||
| Assert.DoesNotContain("?t=e3dad6d7140e583d/", result.DashboardUrl); | ||
|
|
||
| // Must use &returnUrl= with the resource path URL-encoded | ||
| Assert.StartsWith("https://host:16323/login?t=e3dad6d7140e583d&returnUrl=", result.DashboardUrl); | ||
| } |
|
aspire describe shouldn't have the token in the URL. I think the bug here is the path/querystring isn't being stripped from the base URL. |
The base URL isn't included anywhere else in the resource JSON. Should we then add a separate field |
|
The dashboard URL is provided when the app starts. I don't want the dashboard token in URLs for each resource or piece of telemetry that dashboard links to. Maybe that will change in the future, but not for now. |
| var resourcePath = DashboardUrls.ResourcesUrl(snapshot.Name); | ||
| dashboardUrl = DashboardUrls.CombineUrl(dashboardBaseUrl, resourcePath); | ||
| dashboardUrl = DashboardUrls.AddReturnUrl(dashboardBaseUrl, resourcePath); |
There was a problem hiding this comment.
This should take the scheme, host, port from dashboardBaseUrl, and combine it with resourcePath.
dotnet-policy-service
bot
added
the
needs-author-action
|
Replaced with #15495 |
Description
Fix malformed dashboard URLs in
aspire describeoutput.The
dashboardBaseUrlpassed toResourceSnapshotMapperis a login URL with a token (e.g.,https://host/login?t=TOKEN).CombineUrlnaively appended the resource path with/, producing malformed URLs:This PR uses
AddReturnUrlinstead, which properly appends areturnUrlquery parameter so the dashboard redirects after login:Re @JamesNK's comment: agreed that not every URL needs to go through
/login. However,dashboardBaseUrlin this code path is always theBaseUrlWithLoginTokenfromDashboardUrlsHelper— the base URL without the token isn't surfaced separately to the resource mapper. Since thedashboardUrlper resource is the only URL consumers get (e.g., fromaspire describe --format json), we keep it as a full login URL withreturnUrlso it works as a single click-through link. Consumers that want the plain dashboard base URL can strip the/login?...portion themselves.Fixes #15171
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: