C#: Re-factor FeedManager to allow better unit testing. - #8
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22468 Source head: b76f793
✅ Shipwright · ApproveRecommendation: approve PR #8 · Tier
Findings (9)
Fireworks usage: 35,565 input · 1,512 output · 37,077 total tokens · $0.0088 · 22s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| /// <summary> | ||
| /// The full address of the Dependabot proxy, if available. | ||
| /// </summary> | ||
| string Address { get; } |
There was a problem hiding this comment.
Shipwright · HIGH
The new public interfaces expose mutable concrete collection types: IDependabotProxy.RegistryURLs is HashSet<string> and IFileProvider exposes ICollection<string> for Projects, Sol
Impact: The new public interfaces expose mutable concrete collection types: IDependabotProxy.RegistryURLs is HashSet<string> and IFileProvider exposes ICollection<string> for Projects, Solutions, NugetConfigs, etc. This leaks implementation details and allows callers to mutate state shared with the implementation. For example, FeedManager copies RegistryURLs into an ImmutableHashSet in the constructor (FeedManag…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| this.sdks = sdks; | ||
| this.nugetFeedsFromConfig = nugetFeedsFromConfig; | ||
| this.nugetFeedsFromFolder = nugetFeedsFromFolder; | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
The test stub DotNetStub in Semmle.Extraction.Tests/DotNetStub.cs has a Restore method that always returns new RestoreResult(true, Array.Empty<string>()) and ignores the RestoreSet
Impact: The test stub DotNetStub in Semmle.Extraction.Tests/DotNetStub.cs has a Restore method that always returns new RestoreResult(true, Array.Empty<string>()) and ignores the RestoreSettings, including NugetSources. This means the new source-argument behavior in DotNet.GetRestoreArgs is not actually verified by the FeedManager tests; the tests only verify FeedManager's feed selection, not that the final restore com…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| { | ||
| httpClientHandler.Proxy = new WebProxy(dependabotProxy.Address); | ||
|
|
||
| if (dependabotProxy.Certificate != null) |
There was a problem hiding this comment.
Shipwright · HIGH
The certificate validation callback moved to FeedManagerIO.IsFeedReachable (FeedManagerIO.cs:52-70) still uses a custom root trust callback that adds dependabotProxy.Certificate to
Impact: The certificate validation callback moved to FeedManagerIO.IsFeedReachable (FeedManagerIO.cs:52-70) still uses a custom root trust callback that adds dependabotProxy.Certificate to the chain's CustomTrustStore and calls chain.Build(cert). This callback is invoked for every HTTPS request made through the proxy, including arbitrary NuGet feed URLs. If the proxy address or certificate is attacker-influenced (e.g., via…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| namespace Semmle.Extraction.CSharp.DependencyFetching | ||
| { | ||
| public interface IDependabotProxy : IDisposable | ||
| { |
There was a problem hiding this comment.
Shipwright · HIGH
The new public interfaces IDependabotProxy, IFeedManagerIO, and IFileProvider are declared public in a namespace that appears to be internal implementation (Semmle.Extraction.CShar
Impact: The new public interfaces IDependabotProxy, IFeedManagerIO, and IFileProvider are declared public in a namespace that appears to be internal implementation (Semmle.Extraction.CSharp.DependencyFetching). Making these public expands the API surface and allows external code to implement or mock these abstractions, including the certificate validation and proxy configuration. This increases the attack surface and makes…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,109 @@ | |||
|
|
|||
There was a problem hiding this comment.
Shipwright · LOW
The new file FeedManagerIO.cs has a leading blank line and an extra blank line before the closing brace (FeedManagerIO.cs:1, 107-108).
Impact: The new file FeedManagerIO.cs has a leading blank line and an extra blank line before the closing brace (FeedManagerIO.cs:1, 107-108). This is a minor style inconsistency that suggests the file was not formatted with the project's standard tooling.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,16 @@ | |||
|
|
|||
There was a problem hiding this comment.
Shipwright · LOW
The interface IFeedManagerIO has a leading blank line before the namespace declaration (IFeedManagerIO.cs:1).
Impact: The interface IFeedManagerIO has a leading blank line before the namespace declaration (IFeedManagerIO.cs:1). This is a minor style issue and inconsistent with the other new interface files.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
In this PR we
This is in preparation for using feeds configured via the dependabot proxy as "base" feed(s) (instead of the current hardcoded public
nuget.orgfeed).DCA looks good.
Source merge-base:
973d5b503fa8fd5f7576253375d81b9b5629aad8Source head:
b76f7939d7fb7d5a10a1de2b271c11516d0a73e3