Fix cold-cache 401 failures - #60
Merged
Merged
Conversation
Authenticated V3 feeds (e.g. JFrog Artifactory) always failed on a cold on-disk cache because EnsureCachedAsync never registered the NuGet SDK's default credential service, and because 401/403 responses were misclassified as transient network failures and silently swallowed with no diagnostics. - Register the NuGet SDK's default credential service once per process (guarded by a static flag + lock) via DefaultCredentialServiceUtility.SetupDefaultCredentialService, mirroring the setup performed internally by the dotnet CLI and MSBuild restore. This is required for credential-provider-plugin/interactive scenarios (e.g. Azure Artifacts, JFrog credential-provider plugins); static packageSourceCredentials already work via HttpClientHandler regardless. - Detect HTTP 401/403 responses (via HttpRequestException.StatusCode on net5.0+ TFMs, with a regex-based fallback for netstandard2.0 and for wrapped NuGetProtocolException/FatalProtocolException) in GetFindPackageByIdResourceAsync and TryDownloadFromResourceAsync, and surface an actionable diagnostic (source name + status code) instead of silently treating the failure as transient. Genuine transient failures are unaffected. - Add NuGet.Credentials package reference (7.3.1, aligned with existing NuGet.Protocol/NuGet.Configuration references). - Add WireMock-based test infrastructure (Basic Auth gating on the service index, registration, and download endpoints, plus a packageSourceCredentials nuget.config overload) and three new tests covering: successful cold-cache download with static credentials, actionable diagnostic when credentials are missing, and direct verification that credential-service registration occurs. - Update requirements (Caching-NuGetCache-AuthenticatedSource), design, and verification documentation accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WireMock.Net transitively depends on NSwag.Core, which pulls in Scriban.Signed 7.2.0 - a version flagged with two known moderate severity vulnerabilities (GHSA-6q7j-xr26-3h2c, GHSA-q6rr-fm2g-g5x8). Scriban is only used internally by NSwag's Swagger/OpenAPI template rendering, which this project never exercises, but the vulnerable version still fails NuGetAudit (NU1902) as an error during restore. Temporarily pin Scriban.Signed directly to the patched 7.2.5 release in the test project. This pin should be removed once WireMock.Net / NSwag.Core update their own dependency past the vulnerable version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Enhances NuGetCache.EnsureCachedAsync to better support authenticated NuGet sources by registering NuGet’s default credential service and surfacing actionable diagnostics when a feed rejects requests with HTTP 401/403 (instead of silently treating them like transient network failures). Adds supporting LocalIntegration tests and updates design/verification/requirements documentation to describe and trace the new behavior.
Changes:
- Register NuGet’s default credential service once per process before querying sources.
- Detect 401/403 across exception chains and include source name + status code in final error diagnostics.
- Add authenticated-feed WireMock helpers, new integration tests, and update design + reqstream + verification docs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/DemaConsulting.NuGet.Caching/NuGetCache.cs |
Registers credential service and adds 401/403 detection + actionable diagnostics. |
src/DemaConsulting.NuGet.Caching/DemaConsulting.NuGet.Caching.csproj |
Adds NuGet.Credentials dependency needed for credential-service setup. |
test/DemaConsulting.NuGet.Caching.Tests/NuGetTestServer.cs |
Adds Basic Auth-protected v3 feed endpoints and settings helper with credentials. |
test/DemaConsulting.NuGet.Caching.Tests/NuGetCacheServerTests.cs |
Adds LocalIntegration tests for authenticated source behavior and credential service registration. |
test/DemaConsulting.NuGet.Caching.Tests/DemaConsulting.NuGet.Caching.Tests.csproj |
Pins Scriban.Signed to avoid known vulnerabilities from transitive dependencies. |
docs/design/nuget-caching/nuget-cache.md |
Documents credential-service registration and actionable 401/403 diagnostic flow. |
docs/verification/nuget-caching/nuget-cache.md |
Adds verification scenarios + requirement coverage mapping for authenticated sources. |
docs/reqstream/nuget-caching/nuget-cache.yaml |
Adds new requirement and maps it to the new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Tighten HttpStatusCodePattern to require the standard HTTP reason phrase (e.g. '401 (Unauthorized)') instead of a bare word-boundary number match, avoiding false positives on unrelated numbers (e.g. port numbers) elsewhere in an exception message. - Change the authenticated-catch-all WireMock mapping in NuGetTestServer to return 404 (not a fabricated 200) for unmapped paths once credentials are valid, so an unexpected endpoint probe surfaces realistically instead of masking a protocol bug as success. - Escape username/password values when generating packageSourceCredentials XML in CreateSettingsWithCredentials, so credentials containing '&', '<', '>', or '"' still produce a well-formed nuget.config. - Strengthen the credential-service registration regression test by adding an internal, test-only reset/observability hook (ResetCredentialServiceRegistrationForTests / IsCredentialServiceRegisteredForTests) so the test asserts a real false->true transition of the registration guard directly, instead of relying on the shared, process-wide HttpHandlerResourceV3.CredentialService property which other concurrently-running tests could also set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace hand-rolled volatile bool + lock double-checked-locking for credential-service registration with a dependency-injection design: new internal ICredentialServiceRegistrar interface + CredentialServiceRegistrar implementation (instance-scoped Lazy<bool>), with a single shared DefaultCredentialRegistrar instance for production callers. Tests now inject a SpyCredentialServiceRegistrar test double via a new internal EnsureCachedAsync overload instead of relying on shared static reset/observability hooks, eliminating the parallel-test flakiness risk. - Fix docs/design wording mismatches for the regex description and the NuGetProtocolException control-flow description. - Preserve an already-captured 401/403 (or generic protocol-error) diagnostic in GetFindPackageByIdResourceAsync when a later candidate fails with a non-actionable HttpRequestException, instead of discarding it unconditionally. - Apply XML-attribute escaping to globalPackagesFolder and sourceUrl (not just username/password) in CreateSettingsWithCredentials. - Update design/verification/reqstream docs to reflect the new DI design and corrected test names. Reviewed via the built-in change-review agent and a formal-review agent (gpt-5.4-mini) against the NuGetCaching-NuGetCache review-set; all issues resolved and re-verified clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NuGetCache.cs previously mixed five responsibilities: the public API surface, source-candidate enumeration with v2 OData fallback, HTTP-level authentication-failure classification, package download/install orchestration, and NuGet credential-service bootstrapping. Split the latter four into independently-testable internal sibling classes, leaving NuGetCache.cs as a thin orchestrator: - AuthFailureClassifier: HTTP status-code/auth-failure diagnosis from exception chains (pure logic, no I/O). - PackageSourceResolver: builds the ordered candidate repository list (v3 + v2-fallback-when-index.json) and resolves FindPackageByIdResource. - PackageDownloader: downloads a resolved FindPackageByIdResource into the global packages folder and owns the package-path convention, now exposed so NuGetCache can reuse it for its cache-hit fast path instead of duplicating it. - CredentialServiceRegistrar: the ICredentialServiceRegistrar interface, implementation, and default instance, moved out of NuGetCache verbatim. This is a pure structural refactor with no behavior change. Each new unit gets full formal software-item treatment matching NuGetCache/PathHelpers: its own reqstream requirements file, design doc, verification doc, and .reviewmark.yaml review-set entry, with traceability wired into NuGetCache's existing requirements via `children:` lists. Added dedicated unit tests (AuthFailureClassifierTests, PackageSourceResolverTests, PackageDownloaderTests) alongside the existing WireMock-based integration tests, which continue to pass unchanged. Formal review of the new/changed review sets found two compound requirements (Caching-NuGetCache-AuthenticatedSource and Caching-CredentialServiceRegistrar-EnsureRegistered, plus Caching-NuGetCache-NullValidation, Caching-PackageDownloader-InstallPackage, and Caching-PackageSourceResolver-V2Fallback) that were split into single-outcome requirements, and a test-isolation gap in the default-credential-registrar test: resetting/asserting the NuGet SDK's shared, process-wide HttpHandlerResourceV3.CredentialService static was not safe under xUnit's default cross-class parallelism, so test-collection parallelization is now disabled for the assembly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The SRP-extraction refactor added an internal ResetForTesting() method to the shipping CredentialServiceRegistrar class solely to let a test reset the shared, process-wide DefaultCredentialRegistrar's memoized Lazy<bool>. This reintroduced the exact test-hook-in-production-code anti-pattern the earlier DI seam (ICredentialServiceRegistrar) was designed to avoid. Fixed by having the sanity test construct its own fresh CredentialServiceRegistrar instance and pass it explicitly via the existing internal registrar-accepting EnsureCachedAsync overload, instead of resetting the shared singleton. A fresh instance naturally starts with unregistered memoization, so no reset hook is needed at all. Removed ResetForTesting() and reverted the backing field to readonly. Updated design/verification docs to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PackageSourceResolver: use the actually-failing candidate repository's URL (not the originally configured source URL) when building 401/403 auth-failure diagnostics and the generic protocol-error message, so the reported endpoint is accurate when the v2 OData fallback candidate is the one that failed. - Test project: mark the temporary Scriban.Signed NU1902 vulnerability pin as PrivateAssets=all so it does not flow transitively. - NuGetCacheServerTests: capture and restore the original HttpHandlerResourceV3.CredentialService value around the default-registrar sanity test, so a mid-test failure cannot leave shared static SDK state mutated for other tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
This pull request significantly enhances the NuGet caching implementation and its documentation to robustly support authenticated package sources. The main improvements are the detection and actionable reporting of authentication failures (HTTP 401/403) when accessing private feeds, and the explicit registration of the NuGet SDK's default credential service to enable credential-provider plugins. These changes ensure that users receive clear diagnostics when credentials are missing or incorrect, rather than ambiguous "not found" errors, and that all credential mechanisms supported by NuGet are available.
Key changes:
Authenticated Source Support
Caching-NuGetCache-AuthenticatedSource) specifying thatEnsureCachedAsyncmust honor configured credentials and report actionable diagnostics with the source name and HTTP status code when a source rejects a request with 401 or 403, instead of treating it as a transient network error. [1] [2]DefaultCredentialServiceUtility.SetupDefaultCredentialService) once per process before resolving anySourceRepository, ensuring credential-provider plugins and advanced authentication scenarios are supported. [1] [2] [3] [4] [5] [6] [7]Documentation and Test Coverage
Overall, these changes make the NuGet caching library more robust and user-friendly when dealing with private or enterprise package feeds requiring authentication.