Hide secrets in source tooltip, correct ExpressionResolver logic for non-containers #7662

adamint · 2025-02-18T20:35:32Z

Description

Took the opportunity to clean up my code in GetSourceViewModel as well to add a type CommandLineInfo instead of relying on a tuple. Hides secrets in tooltips using the same masking characters as appear in the GridValue.

Also removes logic in ExpressionResolver that resolves some types of expressions only for containers. All expressions except EndpointReferenceExpression and EndpointReference should be resolved for all resource types. The only other difference between containers and other types of resources is how HostUrl is resolved (special-cased for containers).

Fixes #7634, fixes #7632

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Is this introducing a breaking change?
    - Yes
      - Link to aspire-docs issue (please use this breaking-change template):
    - No
      - Link to aspire-docs issue (please use this doc-idea template):
- No

…tooltip

Copilot

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

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

eerhardt · 2025-02-18T21:15:40Z

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

+            ConnectionStringReference cs => await ResolveInternalAsync(cs.Resource.ConnectionStringExpression).ConfigureAwait(false),
+            IResourceWithConnectionString cs => await ResolveInternalAsync(cs.ConnectionStringExpression).ConfigureAwait(false),


@davidfowl @mitchdenny @davidebbo - do we even need these 2 cases? Since ConnectionStringReference and IResourceWithConnectionString implement IValueProvider, shouldn't this just fall to that case?

(Note we don't necessarily need to change this in this PR, but I'm just trying to understand if I have the logic correct. If we are going to backport this to 9.1, we may not want to risk a change here.)

Nevermind, I think I understand why this is necessary - because we want to intercept any "host" references inside the ConnectionString and redirect them to the containerHostName.

Would it ever work if someone implemented an IValueProvider that contained a "host" reference, but wasn't one of these?

eerhardt

LGTM. I just had some questions for my learning.

adamint · 2025-02-18T22:06:30Z

Example with a connection string, like in @DamianEdwards's example in #7632 -

… ExpressionResolver.EvalExpressionAsync

…, to prevent non-optional but missing value from resolving

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

adamint · 2025-02-18T23:33:07Z

@eerhardt I added some additional special cases for certain resource kinds in ExpressionEvaluator, enough changes that need a re-review

danmoseley · 2025-02-19T15:57:04Z

@adamint please get this in 9.1 today if possible.

adamint · 2025-02-19T17:59:37Z

@adamint please get this in 9.1 today if possible.

Waiting on re-review. @eerhardt @davidfowl

eerhardt

The changes look good to me. Just a couple minor comments.

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

eerhardt · 2025-02-19T21:53:17Z

tests/Aspire.Dashboard.Tests/Model/ResourceSourceViewModelTests.cs

@@ -94,7 +96,7 @@ void AddStringProperty(string propertyName, string? propertyValue)
                value: "project",
                contentAfterValue: [new LaunchArgument("arg2", true), new LaunchArgument("--key", true), new LaunchArgument("secret", false)],
                valueToVisualize: "path/to/project arg2 --key secret",
-                tooltip: "path/to/project arg2 --key secret"));
+                tooltip: $"path/to/project arg2 --key {WebUtility.HtmlDecode(DashboardUIHelpers.GetMaskingText(6).Value)}"));


Are there other tests that should be added for this change? Specifically for the case(s) where we are changing ExpressionResolver - to catch secrets inside connection strings.

Added those tests in ExpressionResolverTests

Co-authored-by: James Newton-King <james@newtonking.com>

JamesNK

Dashboard changes look good. Need some tests and a little clean up.

Is there a sample where this can be manually tested?

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs

JamesNK · 2025-02-19T23:58:52Z

Is there a sample where this can be manually tested?

adamint · 2025-02-20T04:36:02Z

Is there a sample where this can be manually tested?

There is not, as there is no sample using ef migrate or this feature. We could add one? but I'm unsure exactly how to replicate the use case in #7632 - @DamianEdwards maybe for suggestion, or could you share the app you were referencing there?

I've been manually testing using the following, inspired by #7632 -

Update any service in TestShop.AppHost/Program.cs to include .WithArgs(catalogDb.Resource)

adamint · 2025-02-20T05:39:22Z

/backport to release/9.1

github-actions · 2025-02-20T05:39:33Z

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13428696603

davidfowl · 2025-02-20T07:35:20Z

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

+        // We are substituting our own logic for ConnectionStringReference's GetValueAsync.
+        // However, ConnectionStringReference#GetValueAsync will throw if the connection string is not optional but is not present.
+        // To avoid duplicating that logic, we can defer to ConnectionStringReference#GetValueAsync, which will throw if needed.
+        await ((IValueProvider)cs).GetValueAsync(cancellationToken).ConfigureAwait(false);


I would be worried about side effects. We should only do this in the null case to avoid double evaluation in the common case.

mitchdenny · 2025-02-20T09:52:46Z

~~@adamint I was taking a closer look at this PR and I noticed a bug. Lets say I do this:~~

builder.Configuration["ConnectionStrings:connstring"] = "dummyvalue";
var connstring = builder.AddConnectionString("connstring");
builder.AddExecutable("pinger", "path/to/ping", "localhost", connstring);

When this renders in the console it'll look like this. The connectionstring type name itself is being rendered into the source column. I don't think that is the intention? It should just show the secret placeholder value.

Scratch that could be an underlying issue with the args handling with executable resources. It happens in 9.0 as well so I'm looking into it. Maybe there is an inconsistency here between containers and executables.

mitchdenny · 2025-02-20T09:53:58Z

Also issue #7634 suggests that we should also have a show secrets button in the copy dialog. I'm not 100% sure I agree that we need to do that - but was that a deliberate choice. If it was just skipped for expediency for 9.1 and we still want to do it then we should re-open that issue or open a new one to make sure we don't loose track.

adamint · 2025-02-20T17:17:13Z

Also issue #7634 suggests that we should also have a show secrets button in the copy dialog. I'm not 100% sure I agree that we need to do that - but was that a deliberate choice. If it was just skipped for expediency for 9.1 and we still want to do it then we should re-open that issue or open a new one to make sure we don't loose track.

It's a deliberate choice, I'll make a comment in #7634 as well - opening the text visualizer is an explicit action. With secret properties in the resource details view, users wouldn't otherwise be able to hide secrets without them being initially masked, of course. Here, secrets are already hidden in the source column; if users want to see all args (masked), they can expand the column or hover to show the (masked) tooltip.

Adam Ratzman added 2 commits February 18, 2025 14:29

Replace tuple with record type in source view model, hide secrets in …

751ed4a

…tooltip

Remove sourceIsContainer checks in ResolveInternalAsync

c31bd4e

adamint requested review from davidfowl, JamesNK and Copilot February 18, 2025 20:35

Copilot AI reviewed Feb 18, 2025

View reviewed changes

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

Fix test

1fcc27a

eerhardt reviewed Feb 18, 2025

View reviewed changes

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

Adam Ratzman added 2 commits February 18, 2025 16:05

EndpointReference/Expression need to only be container

bc3fe8f

Apply suggestion

247cea7

eerhardt reviewed Feb 18, 2025

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs Show resolved Hide resolved

eerhardt reviewed Feb 18, 2025

View reviewed changes

eerhardt approved these changes Feb 18, 2025

View reviewed changes

Adam Ratzman added 2 commits February 18, 2025 16:55

special case ResourceWithConnectionStringSurrogate

5f1db82

Update comment

4357580

Adam Ratzman added 2 commits February 18, 2025 17:26

Fix discrepant behavior between ReferenceExpression.GetValueAsync and…

844a056

… ExpressionResolver.EvalExpressionAsync

Invoke GetValueAsync on ConnectionStringReference before resolving it…

e102dfe

…, to prevent non-optional but missing value from resolving

JamesNK reviewed Feb 18, 2025

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs Outdated Show resolved Hide resolved

adamint requested a review from eerhardt February 18, 2025 23:32

danmoseley added security 🔐 area-dashboard labels Feb 19, 2025

danmoseley added this to the 9.1 milestone Feb 19, 2025

eerhardt approved these changes Feb 19, 2025

View reviewed changes

Update src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

d5505ef

Co-authored-by: James Newton-King <james@newtonking.com>

JamesNK reviewed Feb 19, 2025

View reviewed changes

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

JamesNK reviewed Feb 19, 2025

View reviewed changes

src/Aspire.Dashboard/Model/ResourceSourceViewModel.cs Outdated Show resolved Hide resolved

danmoseley assigned adamint Feb 20, 2025

Adam Ratzman added 4 commits February 19, 2025 23:01

Improve comment in ResolveConnectionStringReferenceAsync

859060d

Change DashboardUIHelpers.GetMaskingText to return a record

72ceae0

clean up ResourceSourceViewModel

07a4b13

add additional secrets in test case

4c27dcc

JamesNK approved these changes Feb 20, 2025

View reviewed changes

Add additional ExpressionResolver tests

f36a730

adamint requested a review from mitchdenny as a code owner February 20, 2025 05:31

github-actions bot mentioned this pull request Feb 20, 2025

[release/9.1] Hide secrets in source tooltip, correct ExpressionResolver logic for non-containers #7708

Merged

adamint enabled auto-merge (squash) February 20, 2025 05:40

adamint merged commit a1136f8 into dotnet:main Feb 20, 2025
70 checks passed

davidfowl reviewed Feb 20, 2025

View reviewed changes

adamint mentioned this pull request Feb 20, 2025

Resolve DistributedApplicationResourceBuilder<ResourceWithConnectionStringSurrogate> correctly in args evaluation #7716

Merged

18 tasks

github-actions bot locked and limited conversation to collaborators Mar 23, 2025

		ConnectionStringReference cs => await ResolveInternalAsync(cs.Resource.ConnectionStringExpression).ConfigureAwait(false),
		IResourceWithConnectionString cs => await ResolveInternalAsync(cs.ConnectionStringExpression).ConfigureAwait(false),

Hide secrets in source tooltip, correct ExpressionResolver logic for non-containers #7662

Hide secrets in source tooltip, correct ExpressionResolver logic for non-containers #7662

Uh oh!

Conversation

adamint commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

adamint commented Feb 18, 2025

Uh oh!

Uh oh!

adamint commented Feb 18, 2025

Uh oh!

danmoseley commented Feb 19, 2025

Uh oh!

adamint commented Feb 19, 2025

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamint Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

JamesNK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Feb 19, 2025

Uh oh!

adamint commented Feb 20, 2025

Uh oh!

adamint commented Feb 20, 2025

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

Uh oh!

davidfowl Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

mitchdenny commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchdenny commented Feb 20, 2025

Uh oh!

adamint commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adamint commented Feb 18, 2025 •

edited

Loading

eerhardt Feb 19, 2025 •

edited

Loading

mitchdenny commented Feb 20, 2025 •

edited

Loading

adamint commented Feb 20, 2025 •

edited

Loading