Compose NuGet configuration for integration restores - #19763
Compose NuGet configuration for integration restores#19763David Negstad (danegsta) wants to merge 31 commits into
Conversation
David Negstad (danegsta)
changed the title
There was a problem hiding this comment.
Pull request overview
Unifies polyglot integration restore behavior across generated AppHost projects, including source resolution, closure generation, cache layout, and runtime configuration.
Changes:
- Adds shared C# project, restore-source, closure, and environment helpers.
- Updates prebuilt and repository-mode AppHost generation.
- Expands restore fingerprinting and project-reference tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Cli.Tests/Projects/PrebuiltAppHostServerTests.cs |
Tests restore configuration, closure outputs, and cache normalization. |
src/Aspire.Cli/Utils/CliPathHelper.cs |
Adds path and staging-cache helpers. |
src/Aspire.Cli/Projects/PrebuiltAppHostServer.cs |
Adopts shared restore and closure infrastructure. |
src/Aspire.Cli/Projects/IntegrationRestoreSourceResolver.cs |
Centralizes channel and source resolution. |
src/Aspire.Cli/Projects/IntegrationClosureEnvironment.cs |
Centralizes runtime integration environment variables. |
src/Aspire.Cli/Projects/IntegrationClosureBuilder.cs |
Centralizes closure project generation and manifest reading. |
src/Aspire.Cli/Projects/DotNetBasedAppHostServerProject.cs |
Uses the shared C# project model. |
src/Aspire.Cli/Projects/CSharpProjectFile.cs |
Introduces a generated C# project model. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19763Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19763" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/Aspire.Cli/Projects/PrebuiltAppHostServer.cs:750
- This only checks
PackageSourceMappings, but the no-channel path returns all explicit channel URLs inAdditionalSourceswhile leavingPackageSourceMappingsnull. If one of those channels contains a SAS/user-info URL, it is written into the long-livedIntegrationRestore.csprojasRestoreAdditionalProjectSources, and restore-cache skipping remains enabled. Include all effective sources in the credential check and keep the generated project/config ephemeral (or reject the ambiguous no-channel case) so credentials are never persisted.
var hasCredentialBearingRestoreSource = restoreSources.PackageSourceMappings?.Any(
static mapping => PackageSourceOverrideMappings.HasCredentialMaterial(mapping.Source)) == true;
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Cli/Projects/DotNetBasedAppHostServerProject.cs:164
- This set now stores project paths, but
OrdinalIgnoreCasecollapses distinct paths such as/src/Foo/Foo.csprojand/src/foo/foo.csprojon case-sensitive Linux/macOS filesystems, silently omitting one validProjectReference. Use ordinal path identity here; the restore graph uses the same rule specifically to avoid dropping distinct paths (PrebuiltAppHostServer.cs:378-380).
var addedProjects = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
c9c2ea5 to
7ea840a
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Case-sensitive identity comparisons can preserve overridden NuGet sources or credentials.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🔵 Needs a closer look
NuGet’s Unix $NAME variables are incorrectly converted into literal relative filesystem paths.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Cli/Packaging/NuGetConfigComposer.cs:251
- On macOS/Linux, NuGet supports
$NAMEenvironment variables in config values, butEnvironment.ExpandEnvironmentVariablesonly expands%NAME%. A valid source such as$PACKAGEHOME/packagestherefore remains relative here and is rewritten to<config-dir>/$PACKAGEHOME/packages, causing restore to use the wrong path. Expand NuGet's platform-specific syntax (or use NuGet.Configuration to obtain the effective value) before resolving relative paths.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Explain why effective configuration flattening remains in the Native AOT CLI instead of moving to aspire-managed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
Keep Aspire-owned diagnostics and temporary configuration credential-aware without isolating or purging NuGet's standard restore intermediates and package caches. Also remove the unrelated custom Aspire.Hosting replacement behavior from the composer layer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
47a0f04 to
543ea16
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Valid configurations can be miscomposed, and credential material can persist or influence durable cache identifiers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- src/Aspire.Cli/Resources/NewCommandStrings.Designer.cs: Generated file
- Files reviewed: 45/46 changed files
- Comments generated: 4
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
Tests selector4 / 99 PR test projects · 4 PR jobs · 1 advisory-only target, from 47 changed files. Selected PR test projects (4 / 99)
Selected PR jobs (4)
Advisory workflow impact (1)
How these were chosen — grouped by what changed📦 affected project 🔧 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
🟡 Changes recommended
Credential-bearing sources can be persisted, while section casing and global-package cache invalidation remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- src/Aspire.Cli/Resources/NewCommandStrings.Designer.cs: Generated file
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:236
- Removing
globalPackagesFolderfromCacheIdentityleaves project-reference restore skipping blind to an effective config change. For non-staging composed restores,PrebuiltAppHostServeruses this identity and only addsNUGET_PACKAGESor an explicit staging override separately; changing<config><add key="globalPackagesFolder" .../>can therefore keep the old restore stamp and reuse assets from the previous folder. Include the config-defined value in this identity (or pass its resolved effective value separately).
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:318 - NuGet section identity is case-sensitive, but this case-insensitive lookup can select an earlier noncanonical
<PackageSources>section instead of the active<packageSources>emitted by the merger. In that hierarchy,mappedSourceKeysis computed from the ignored section and an explicitly mapped source can remain disabled. Select only the canonical section.
This issue also appears on line 341 of the same file.
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:342
- This lookup can select a noncanonical
<DisabledPackageSources>section before the active lowercase section. The method then removes entries from an ignored section and leaves the real mapped source disabled. Match the canonical NuGet section name exactly.
var disabledSources = configuration?.Elements()
.FirstOrDefault(element => string.Equals(element.Name.LocalName, "disabledPackageSources", StringComparison.OrdinalIgnoreCase));
src/Aspire.Cli/Commands/AddCommand.cs:496
- This treats noncanonical
<PackageSourceMapping>/<PackageSource>elements as active even though NuGet section identity is case-sensitive. If an inherited config contains only those ignored elements,aspire addenables package source mapping in the new local config with mappings only for the hive, preventing other inherited feeds from serving packages. Check only canonical element names.
var effectiveConfig = await NuGetConfigComposer.ComposeAsync(configPaths, cancellationToken);
return effectiveConfig.Root?
.Elements()
.FirstOrDefault(element => string.Equals(
element.Name.LocalName,
"packageSourceMapping",
StringComparison.OrdinalIgnoreCase))?
.Elements()
.Any(element => string.Equals(
element.Name.LocalName,
"packageSource",
StringComparison.OrdinalIgnoreCase)) == true;
- Files reviewed: 45/46 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
There was a problem hiding this comment.
🔵 Needs a closer look
Credential-bearing sources can be persisted in generated projects and encoded into deterministic workspace cache identities.
Review details
Files not reviewed (1)
- src/Aspire.Cli/Resources/NewCommandStrings.Designer.cs: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Cli/NuGet/BundleNuGetService.cs:395
- These fields can contain clear-text passwords or SAS tokens, and they are fed into the deterministic 64-bit
XxHash3cache-directory name below. This leaves a reusable verifier for low-entropy credentials in the workspace cache, allowing offline guessing even after the temporary config is deleted. Credential-bearing restores should use a random non-reusable cache identity (or another design that does not derive persistent names from secret material).
src/Aspire.Cli/Commands/NewCommand.cs:500
- Removing this guard allows
--sourceURLs containing user-info, SAS/query tokens, or fragments to flow intoCreateOrUpdateNuGetConfigForSourceOverrideAsync, which writes them verbatim to the generated project's long-livednuget.config. That file is likely to be committed, and it is not the temporary configuration whose cleanup is described in the PR. Keep rejecting credential-bearing sources on project-scaffolding paths, or persist only a sanitized URL and require credentials from a provider/user-level config.
var source = parseResult.GetValue(s_sourceOption);
if (!string.IsNullOrWhiteSpace(source))
{
source = PackageSourceOverrideMappings.ResolveForWorkingDirectory(source, ExecutionContext.WorkingDirectory);
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:236
identityDocumentcan contain clear-text package credentials, proxy passwords, client-certificate passwords, or authenticated source URLs. Hashing it with deterministic 64-bitXxHash3and then usingCacheIdentityin persistent restore fingerprints/cache paths exposes a reusable verifier for low-entropy secrets after the temporary file is removed. Return a random/non-reusable identity for credential-bearing documents (or otherwise exclude secret values from persistent identities).
var bytes = Encoding.UTF8.GetBytes(identityDocument.ToString(SaveOptions.DisableFormatting));
return Convert.ToHexString(XxHash3.Hash(bytes));
- Files reviewed: 45/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
There was a problem hiding this comment.
🟡 Changes recommended
Configuration composition and persistence still contain security-sensitive validation gaps and cache-identity correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/Aspire.Cli/Packaging/NuGetConfigComposer.cs:225
- Unknown NuGet items are identified by element name with ordinal comparison, but this initial case-insensitive check merges custom items such as
<Foo>and<foo>. The flattened hierarchy can therefore differ from NuGet's effective settings. Keep case-insensitive matching for the known item types that require it, but compare unknown item names ordinally.
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:318 - This case-insensitive lookup can select an inactive
<PackageSources>section before the canonical<packageSources>section thatNuGetConfigMergergenerated. In that valid mixed-case composition, no mapped keys are found and explicitly mapped sources remain disabled. Use an ordinal lookup for the canonical NuGet section.
This issue also appears on line 341 of the same file.
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:342
- This has the same section-identity problem for disabled sources: an earlier inactive
<DisabledPackageSources>section can be selected instead of the canonical section, so the requested mapped source is never re-enabled. Look up the canonical section name ordinally.
var disabledSources = configuration?.Elements()
.FirstOrDefault(element => string.Equals(element.Name.LocalName, "disabledPackageSources", StringComparison.OrdinalIgnoreCase));
- Files reviewed: 31/32 changed files
- Comments generated: 3
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
There was a problem hiding this comment.
🔵 Needs a closer look
Relative colon-containing paths are not rebased, and case-insensitive mapping detection can activate inactive NuGet configuration.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Cli/Packaging/NuGetConfigComposer.cs:386
Uri.TryCreate(path, UriKind.Relative, ...)rejects valid Unix relative paths that contain a colon (for examplerelative:feedorC:/feed), so this returns them unchanged. After the config is copied to the temporary directory, NuGet resolves that source/fallback/certificate path relative to the temp directory instead of the originating config directory. The existingPackageSourceOverrideMappingsTestsexplicitly treats these shapes as relative Unix paths; use the same source/path classification here rather than URI-relative validity.
src/Aspire.Cli/Commands/AddCommand.cs:496
- This case-insensitive lookup treats an inactive
<PackageSourceMapping>section as effective, even though NuGet section identity is ordinal and the composer intentionally preserves case variants.CreateAdditiveLocalSourceNuGetConfigthen writes a canonical mapping containing only the local hive, unexpectedly enabling package-source mapping and making inherited feeds ineligible for transitive packages. Check only the canonicalpackageSourceMapping/packageSourceelements.
.FirstOrDefault(element => string.Equals(
element.Name.LocalName,
"packageSourceMapping",
StringComparison.OrdinalIgnoreCase))?
.Elements()
.Any(element => string.Equals(
element.Name.LocalName,
"packageSource",
StringComparison.OrdinalIgnoreCase)) == true;
- Files reviewed: 31/32 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
There was a problem hiding this comment.
🔵 Needs a closer look
URI normalization can bypass source redaction and create duplicate unauthenticated restore sources.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Aspire.Managed/NuGet/Commands/RestoreCommand.cs:244
- Ordinal equality preserves case-sensitive URL paths, but it also treats harmless scheme/host casing differences as distinct sources. If the config contains an authenticated
https://HOST/feedand--sourcesupplieshttps://host/feed, this appends a second unauthenticated source; NuGet can then query both and fail on the duplicate. Deduplicate with URI-aware identity (case-insensitive scheme/host, case-sensitive path/query, and platform-aware local paths) rather than whole-string ordinal comparison.
src/Aspire.Cli/Utils/PackageSourceRedactor.cs:81
- This exact ordinal replacement can miss the same credential-bearing URL after NuGet normalizes it. For example, a source with surrounding whitespace is classified as sensitive, but NuGet reports the trimmed URL, so the raw
sourceis not found and its user-info/query can reach logs and exceptions. Replace trimmed and canonical URI forms as well (while preserving case-sensitive path/query semantics), and cover normalized diagnostic output.
foreach (var source in sensitiveSources.OrderByDescending(static source => source.Length))
{
value = value.Replace(source, RedactForDisplay(source), StringComparison.Ordinal);
- Files reviewed: 31/32 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d38799a7-a99c-48c5-b45b-9d6594dd1830
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
There was a problem hiding this comment.
🔵 Needs a closer look
Configuration composition can select inactive case-variant sections and incorrectly collapse case-distinct fallback-folder entries.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/Aspire.Cli/Packaging/NuGetConfigComposer.cs:232
fallbackPackageFolderskeys are merged ordinally by NuGet (AddItem.EqualsusesStringComparison.Ordinal), but this generic case-insensitive key comparison collapses case-distinct entries across configuration layers. For example, a workspaceSHAREDfolder causes an inheritedsharedfolder to be removed, whereas NuGet keeps and searches both; packages available only in the inherited fallback can then stop resolving. Use ordinal key identity for this section (while retaining the source-specific behavior where required).
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:318- NuGet section identity is ordinal, but this case-insensitive lookup can select an earlier inactive
<PackageSources>section instead of the canonical<packageSources>section produced by the composer/merger. In that casemappedSourceKeysis computed from the wrong sources, so an explicitly mapped canonical source may remain disabled. Select the canonical section exactly.
This issue also appears on line 341 of the same file.
src/Aspire.Cli/Packaging/TemporaryNuGetConfig.cs:342
- This case-insensitive lookup has the same section-identity problem: an inactive
<DisabledPackageSources>appearing before the canonical section is selected, so entries in the actual<disabledPackageSources>section are never removed and the requested mapped source stays disabled. Select only the canonical section.
var disabledSources = configuration?.Elements()
.FirstOrDefault(element => string.Equals(element.Name.LocalName, "disabledPackageSources", StringComparison.OrdinalIgnoreCase));
- Files reviewed: 32/33 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
Depends on #19915.
Polyglot integration restores can receive NuGet settings from multiple configuration layers and package channels. Passing only package source URLs loses source mappings, disabled-source state, credentials, client certificates, fallback folders, and path origins; shared caches can then reuse packages or restore metadata produced under a different effective configuration.
Integration restores now compose the effective NuGet hierarchy into a temporary configuration, rebase origin-relative paths, preserve source-key identity and aliases, and re-enable explicitly mapped sources. Bundled and generated-project restores use the same effective inputs and include the relevant configuration, source, fallback-folder, and managed-component identities in cache invalidation.
Aspire redacts known credential-bearing source URLs from restore diagnostics and removes the temporary NuGet configuration and source-property files that it creates. Standard NuGet restore outputs under
objand the global packages folder retain NuGet's normal contents and lifetime; this change does not scrub or isolate those artifacts.Security considerations
The composed temporary configuration may copy credentials, client certificates, and authenticated source URLs from the user's effective NuGet configuration. Aspire deletes its temporary copy after the restore and redacts known credential-bearing source URLs from its own diagnostics, but it does not provide guarantees beyond standard NuGet behavior for restore intermediates or package metadata.
Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?