C#: Improve BMN feed checking & handling.#21684
Open
michaelnebel wants to merge 23 commits intogithub:mainfrom
Open
C#: Improve BMN feed checking & handling.#21684michaelnebel wants to merge 23 commits intogithub:mainfrom
michaelnebel wants to merge 23 commits intogithub:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs
Fixed
Show fixed
Hide fixed
michaelnebel
force-pushed
the
csharp/improve-reachability-checks
branch
from
958f01a to
0451894
Compare
…plicit this qualifiers.
…pecific sources for restoring if private registries are used of if nuget feed reachability check is performed.
…on an environment variable setting) are still used as sources.
…se unless there is a timeout when trying to reach them.
michaelnebel
force-pushed
the
csharp/improve-reachability-checks
branch
from
b47a7c6 to
5e5a428
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the C# build-mode:none NuGet dependency restore flow to explicitly control which NuGet sources are passed to dotnet restore, based on discovered nuget.config feeds, private registries, and reachability checks, and aligns telemetry + integration test expectations with the new behavior.
Changes:
- Compute explicit vs inherited NuGet feeds, probe reachability (including 401/403), and pass an explicit
-s/--sourcelist todotnet restore(including private registries when configured). - Refactor feed probing logic and adjust
nuget.exerestore behavior to avoid adding the default feed when it is unreachable. - Update change notes and integration test expected telemetry values.
Show a summary per file
| File | Description |
|---|---|
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs | Core logic changes: feed discovery, reachability probing, explicit dotnet restore source argument construction, telemetry updates. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetExeWrapper.cs | Gate adding default nuget.org source on whether the default feed is reachable. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/IDotNet.cs | Rename restore argument field from generic ExtraArgs to NugetSources. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs | Wire NugetSources into the constructed dotnet restore command line. |
| csharp/ql/integration-tests/posix/standalone_dependencies_nuget_config_error/CompilationInfo.expected | Update expected telemetry values to match new reachability semantics. |
| csharp/ql/lib/change-notes/2026-04-10-nuget-feed-usage-in-bmn.md | Document the behavioral change in build-mode:none restore source handling. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 3
Comment on lines
+812
to
+830
| // Exclude any feeds from the feed check that are configured by the corresponding environment variable. | ||
| // These feeds are always assumed to be reachable. | ||
| var excludedFeeds = GetExcludedFeeds(); | ||
|
|
||
| var excludedFeeds = EnvironmentVariables.GetURLs(EnvironmentVariableNames.ExcludedNugetFeedsFromResponsivenessCheck) | ||
| .ToHashSet(); | ||
|
|
||
| if (excludedFeeds.Count > 0) | ||
| HashSet<string> feedsToCheck = feeds.Where(feed => | ||
| { | ||
| logger.LogInfo($"Excluded NuGet feeds from responsiveness check: {string.Join(", ", excludedFeeds.OrderBy(f => f))}"); | ||
| } | ||
| if (excludedFeeds.Contains(feed)) | ||
| { | ||
| logger.LogInfo($"Not checking reachability of NuGet feed '{feed}' as it is in the list of excluded feeds."); | ||
| return false; | ||
| } | ||
| return true; | ||
| }).ToHashSet(); | ||
|
|
||
| reachableFeeds = GetReachableNuGetFeeds(feedsToCheck, isFallback: false, out var isTimeout).ToHashSet(); | ||
|
|
||
| var (initialTimeout, tryCount) = GetFeedRequestSettings(isFallback: false); | ||
| var allReachable = reachableFeeds.Count == feedsToCheck.Count; | ||
| EmitUnreachableFeedsDiagnostics(allReachable); | ||
| return !isTimeout; |
Contributor
Author
There was a problem hiding this comment.
Oh yes, that is definitely a mistake.
| { | ||
| ExecuteGetRequest(feed, client, cts.Token).GetAwaiter().GetResult(); | ||
| logger.LogInfo($"Attempt {i + 1}/{tryCount} to reach NuGet feed '{feed}'."); | ||
| var response = ExecuteGetRequest(feed, client, cts.Token).GetAwaiter().GetResult(); |
Comment on lines
+342
to
+357
| private string? MakeRestoreSourcesArgument(IEnumerable<string> feeds) | ||
| { | ||
| // If there are no feeds, we want to override any default feeds that `dotnet restore` would use by passing a dummy source argument. | ||
| if (!feeds.Any()) | ||
| { | ||
| return $" -s \"{emptyPackageDirectory.DirInfo.FullName}\""; | ||
| } | ||
|
|
||
| // Add package sources. If any are present, they override all sources specified in | ||
| // the configuration file(s). | ||
| var feedArgs = new StringBuilder(); | ||
| foreach (var feed in feeds) | ||
| { | ||
| feedArgs.Append($" -s {feed}"); | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is a good point as well. It appears that the dependency manager generally uses poor mans quoting (prefixing and postfixing with hardcoded "). I will add " for this case as well, but we should probably introduce proper escaping everywhere (follow up PR).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the C# build-mode: none dependency restoration flow to proactively discover NuGet feeds, probe their reachability, and pass an explicit set of sources to dotnet restore (plus adjusted telemetry and test expectations) to avoid slow restores caused by problematic feeds (e.g., 401/403/timeouts).
Changes:
- Discover explicit vs inherited NuGet feeds (plus private registries) and, when enabled, only pass reachable feeds to
dotnet restorevia explicit-sarguments. - Refactor feed reachability probing (including richer telemetry/diagnostics) and gate default-feed usage for
nuget.exerestores. - Update integration test expectations and add a changenote for the behavior change.
Show a summary per file
| File | Description |
|---|---|
| csharp/ql/lib/change-notes/2026-04-10-nuget-feed-usage-in-bmn.md | Documents the new buildless NuGet source handling behavior. |
| csharp/ql/integration-tests/posix/standalone_dependencies_nuget_config_error/CompilationInfo.expected | Updates expected telemetry outputs to match new reachability semantics. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs | Implements feed discovery, reachability checks, explicit dotnet restore source handling, and related telemetry/diagnostics. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetExeWrapper.cs | Adds conditional default-source injection based on default feed reachability. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/IDotNet.cs | Renames restore argument field from ExtraArgs to NugetSources. |
| csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs | Threads NugetSources into constructed dotnet restore arguments. |
Copilot's findings
Comments suppressed due to low confidence (1)
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs:156
- When
CheckNugetFeedResponsivenessis enabled, the code buildsNugetSourcesand passes-sarguments todotnet restore, which overrides all sources fromnuget.config. However, feed discovery currently only keepshttp(s)URLs (seeGetFeedsskipping non-URLs), so local folder/file-based package sources fromnuget.configwill be dropped from the restore inputs, potentially breaking restores that rely on local sources. Consider preserving non-URL sources (e.g. local paths /file://sources) and either treating them as always reachable or checking them via filesystem existence rather than HTTP.
// If feed responsiveness is being checked, we only want to use the feeds that are reachable (note this set includes private
// registry feeds if they are reachable).
explicitNugetSources = MakeRestoreSourcesArgument(reachableFeeds);
- Files reviewed: 6/6 changed files
- Comments generated: 2
Comment on lines
+151
to
+153
| // Inherited feeds should only be used, if they are indeed reachable (as they may be environment specific). | ||
| reachableFeeds.UnionWith(GetReachableNuGetFeeds(inheritedFeeds, isFallback: false, out var _)); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is intentional - to not change the semantics of the environment variable (and to not make any further changes in this PR). However, I agree that we should consider to use it for inherited feeds as well.
csharp/ql/lib/change-notes/2026-04-10-nuget-feed-usage-in-bmn.md
Outdated
Show resolved
Hide resolved
michaelnebel
force-pushed
the
csharp/improve-reachability-checks
branch
from
75dc980 to
c623e23
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR we make some improvements to the C# NuGet feed check and use of private registries.
The primary change(s) in this PR
dotnet restore(both for projects and solutions) which overrides the use ofnuget.configfiles (unless NuGet feed reachability check is disabled and no private registries are configured). For both explicit and implicit feeds, we only use the feeds that we can successfully connect to (there is still an envionment variable that makes it possible to override the feed check for a list of feeds - and then they will be included no matter what). However, this is still a significant change in the semantics of the dependency resolution. Previously, we used all feeds and letdotnet restoresort out issues with the feeds. However, it turns out that it might cause a significant slowdown to the restore process, if we use feeds that eg. issues 401/403 responses. Furthermore, we are now explicitly providing a list of NuGet sources based on all thenuget.configin the repository - which is an over-approximation (asnuget.configfiles can be project/solution specific - which was previously respected as we letdotnet restorehandle it). If this turns out to be an issue, we might need to further improve the solution. In any case, this is still an improvement over the old solution as the use of private registries is now aligned with how thedotnet restoreprocess is performed.dotnet restoreto make sure that we include the private registries.All NuGet feeds reachable. Previously, we only considered a feed to be unreachable in case there was a timeout. Now other errors like 401/403 are included.standalone_dependencies_nuget_config_errorare expected. As the feed in thenuget.configis it not reachable it means thatAll NuGet feeds reachableis false (and thus0is reported).Failed project restore with package source erroris now0as the restore process now excludes the unreachable feed from thenuget.configfile (and instead we get another NuGet error as the restore process attempts to find theNewtonSoft.Jsonpackage in theemptyfolder).The fallback in case there is a timeout on an explicit feed has been kept in the implementation. I think we should consider removing this fallback (it was made prior to any of the logic, which overrides the
nuget.configfiles) and just rely on supplying the reachable feeds to thedotnet restoreprocess.The documentation for the
-s(NuGet source) can be seen here.I encourage to review all the changes in this PR in one go (review by commit is NOT encouraged as there is some going back and forth due to experimentation).
DCA looks good; For our existing suite there doesn't appear to be any regressions. However, I expect improved performance in cases where there are issues with connecting to the feeds.