Restore eager NuGet service index discovery

[brettfo]

Short description

Restore eager NuGet service-index discovery while retaining the useful credential matching and deduplication behavior introduced after the original implementation.

Paged v2 feeds and v3 service indexes can identify authenticated endpoints that differ from the configured repository URL. The proxy must discover those endpoints before the updater starts, otherwise requests can be made before the proxy knows which credentials to inject.

What are you trying to accomplish?

PR #203 changed NuGet discovery from eager initialization to deferred response interception. That caused update-job failures when authenticated resource URLs were needed before the proxy had observed the service-index response. This PR restores eager initialization and improves its performance, matching, determinism, and redirect handling.

The proxy still completes NuGet discovery before opening port 1080. Discovery requests now run through a bounded pool of eight workers, while results are merged in configured credential order.

Behavior before PR #203 compared with this PR

Area	Before PR #203	Current behavior
Initialization	Service indexes were fetched eagerly and serially.	Service indexes are fetched eagerly with up to eight concurrent requests. Port 1080 is opened only after all discovery finishes.
Discovery client	Used a private client with a 10-second timeout and the default transport.	Uses the proxy's shared 10-second client and restricted transport, including the safe dialer and TLS settings.
Unusable credentials	Credentials without a token or password were stored and queried without authentication.	Unusable static credentials are ignored and do not trigger discovery.
Duplicate sources	Every configured source was queried, including equivalent duplicates.	Canonically equivalent sources are queried once, with first-configured precedence. HTTP and HTTPS sources remain distinct.
Credential precedence	Discovery was interleaved with configuration processing, so an earlier discovered credential could shadow a later explicit credential.	All explicit credentials are registered before discovered credentials, so explicit configuration wins. Duplicate discovered resources use deterministic configured-source order.
Path matching	Used decoded string-prefix matching; /feed could match /feedback, and escaped separators could be conflated with literal separators. The first matching credential won.	Uses canonical path-segment matching, preserves escaped separators, rejects ambiguous dot segments, and selects the most-specific matching URL credential.
Host credentials	A host credential could shadow a URL credential based on configuration order.	URL-scoped credentials take precedence; host credentials are fallback matches.
Proxy-only credentials	Had no special NuGet behavior and acted like ordinary credentials.	Are host-only fallback credentials used only for HTTPS on the default port when no explicit/OIDC credential or usable request authorization applies.
Response handling	Parsed any response below HTTP 400 and could panic on short or empty unrecognized bodies.	Parses only HTTP 200 responses and safely handles empty, short, invalid, and non-success responses.
Service-index redirects	Relied on the standard client's redirect-header behavior and did not register redirected index URLs.	Explicitly retains Authorization or X-Api-Key across redirects, including redirects to another host, and registers every followed redirect URL for later authenticated requests.
Discovered resources	Registered service-index resource URLs with the source credential, including resources on other hosts.	Retains that behavior while canonicalizing and deduplicating registrations.
OIDC matching	Used decoded path-prefix matching.	Uses canonical, most-specific path matching and participates in concurrent discovery, deduplication, and redirect registration.

Authentication formatting is unchanged: username/password values use Basic authentication, ordinary tokens use Bearer authentication, and Azure DevOps tokens are treated as Basic-auth passwords. Discovery still supports v2 xml:base and non-template v3 resources.

Anything you want to highlight for special attention from reviewers?

• Authentication is intentionally retained when a service-index redirect changes hosts, matching the treatment of cross-host resource URLs returned by NuGet indexes.
• Discovery concurrency is capped at eight workers. Workers collect results independently; shared credential and OIDC state is updated afterward in deterministic configuration order.
• Canonical static URL deduplication is scheme-agnostic for compatibility, while source discovery keeps HTTP and HTTPS distinct.

How will you know you've accomplished your goal?

• All NuGet resource and redirected service-index URLs are registered before the proxy accepts connections.
• Slow feeds do not serialize discovery of every other configured feed.
• The most-specific applicable credential is selected safely and deterministically.
• Explicit credentials take precedence over discovered credentials.
• Cross-host redirects and cross-host resources retain authentication.
• Empty, malformed, and non-200 discovery responses do not add credentials or panic.
• go test ./... passes.

Checklist

• I have run the complete test suite to ensure all tests and linters pass.
• I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
• I have written clear and descriptive commit messages.
• I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
• I have ensured that the code is well-documented and easy to understand.

[Copilot]

Copilot review overview

🟡 Changes recommended

Eager discovery can currently panic on empty/short service-index bodies and also attempts to parse some non-2xx responses as service indexes, which risks startup failures and incorrect discovery.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 · 1 · 1

New issues introduced by this change (3)

Severity	Finding
	internal/​handlers/​nuget_feed.go — extraUrlsFromSourceResponse can panic on empty or short bodies: the empty-body guard was removed…
	internal/​handlers/​nuget_feed.go — discoverNugetFeedURLsForJob currently treats non-error non-2xx responses (e.g., 3xx without…
	internal/​handlers/​nuget_feed_test.go — The updated NuGet discovery parsing no longer has regression coverage for empty/short bodies;…

What changed in this PR

Restores eager NuGet service-index discovery during proxy startup (undoing the deferred-on-response approach from #203) so NuGet resource URLs are registered before the proxy begins serving traffic, while using bounded concurrency and preserving configured credential ordering.

Changes:

• Move NuGet service-index/resource discovery back into NewNugetFeedHandler, using a bounded worker pool and merging results deterministically.
• Remove the deferred discovery request/response hooks from the proxy pipeline and NuGet handler.
• Update NuGet and OIDC handler tests to mock constructor-time discovery and add a concurrency regression test.

File	Description
proxy.go	Removes deferred NuGet discovery hook wiring and registers NuGet handler in the normal request chain.
internal/​handlers/​oidc_handling_test.go	Adds URL-level HTTP mocks to support NuGet constructor-time discovery for OIDC cases.
internal/​handlers/​nuget_feed.go	Reworks NuGet handler to eagerly discover service index resources concurrently during construction.
internal/​handlers/​nuget_feed_test.go	Updates NuGet handler tests for eager discovery and adds a concurrency/ordering regression test.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Eager discovery currently fails for scheme-less configured NuGet service-index URLs (missing protocol scheme), which can prevent required authenticated resource URL discovery at startup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	internal/​handlers/​nuget_feed.go — Scheme-less NuGet service-index URLs (e.g. "nuget.example.com/v3/index.json") are accepted…

Issues resolved since last review (3)

Severity	Finding
	internal/​handlers/​nuget_feed_test.go — The updated NuGet discovery parsing no longer has regression coverage for empty/short bodies;… View resolved comment
	internal/​handlers/​nuget_feed.go — discoverNugetFeedURLsForJob currently treats non-error non-2xx responses (e.g., 3xx without… View resolved comment
	internal/​handlers/​nuget_feed.go — extraUrlsFromSourceResponse can panic on empty or short bodies: the empty-body guard was removed… View resolved comment

Suppressed comments (1)

internal/handlers/nuget_feed.go:130

• Static NuGet feed credentials also allow scheme-less URLs for request matching, but eager discovery currently enqueues the raw configured string as a discovery URL. That will fail request creation (missing protocol scheme) and skip discovery. For scheme-less sources, consider discovering both https:// and http:// variants (matching the earlier scheme-less behavior) while keeping explicit http/https sources distinct.

		if !proxyOnly && url != "" {
			logging.RequestLogf(nil, "fetching service index for nuget feed %s", url)
			addNugetDiscoveryJob(&discoveryJobs, discoverySourceURLs, nugetDiscoveryJob{
				serviceIndexURL:  url,
				staticCredential: feedCred,

[jeffwidman]

this is an agent review comment guided by jeff

1. Cross-origin redirect authentication is a new policy change, not part of restoring eager discovery. Current main authenticates service-index redirects only while they remain same-origin; its regression test requires cross-origin redirects and later hops on that foreign origin to remain unauthenticated, while still registering resources learned from the final index. The pre-Defer NuGet service index discovery until requested #203 eager implementation also used the standard Go client behavior, which strips Authorization on redirects to unrelated hosts. PR Restore eager NuGet service index discovery #240 preserved this behavior until 1410ce1, which now copies Authorization and X-Api-Key to every redirect and registers every redirect URL for future authentication. This broadens credential exposure beyond both baselines and also sends credentials across HTTPS-to-HTTP redirects, bypassing the normal HTTPS-only OIDC gate. PR Reject ambiguous NuGet credential routes #206 discussed duplicate resource-route claims and did not change the redirect policy on main. Please restore the pre-1410ce1 same-origin guard and test: continue following cross-origin redirects without credentials and parse their final index, but authenticate only same-origin redirect hops. The PR description should likewise stop claiming cross-host redirect authentication as part of this revert.

2. Scheme-less configured feed URLs no longer get eager discovery. NuGet matching accepts URLs such as nuget.example.com/v3/index.json, and current main can learn resources when that configured index is requested over HTTP or HTTPS. PR Restore eager NuGet service index discovery #240 queues the raw value and passes it to http.NewRequestWithContext, which fails with missing protocol scheme; the configured route can still authenticate, but delegated resource and redirect routes are never discovered. Please normalize scheme-less sources to an explicit discovery URL while retaining their request-matching behavior, or explicitly preserve the current HTTP/HTTPS discovery semantics another way, and add regression coverage.

[brettfo]

Noting for future reference.

1. Cross-origin redirect authentication. This is added by design. There's nothing in the NuGet spec that says the service index must return same-origin hosts for the other endpoints and in fact it's common to use a CDN or similar here, especially Azure DevOps Artifacts feeds. Currently the ADO feeds don't require auth on the 30x redirects but there's nothing to say that can't happen in the future.
2. Not adding a scheme (http or https) to registry values is by design. The only kinds of values allowed by NuGet are either http, https, or directory paths and since dependabot doesn't mount any user-defined directories, this means only http and https are allowed and everything else is a configuration error. This is to force users to explicitly set https when appropriate so we don't make an incorrect assumption.

[jeffwidman]

this is an agent review comment guided by jeff

Do not retain authentication across an HTTPS-to-HTTP redirect. The redirect callback currently copies Authorization and X-Api-Key to every destination. An HTTPS service index can therefore cause static credentials or an exchanged OIDC token to be sent over plaintext, bypassing the handler’s normal HTTPS-only OIDC protection. Static credentials are also registered for the HTTP redirect URL and may be sent there on later requests. Please strip authentication for HTTPS-to-HTTP redirects, keep it stripped for the remainder of that redirect chain, and add regression coverage for both static and OIDC credentials.

[JamieMagee]

Can we reject non-HTTPS redirects for OIDC discovery? IMO removing auth headers isn't enough here.

[JamieMagee]

Can you make sure to resolve schema-less URls e.g. feed.example.com/v3/index.json, to absolute URLs before http.NewRequestWithContext? Otherwise discovery fails and later successful index requests won't register credentials for the package endpoints.

[brettfo]

Are schema-less URIs common here? Technically they're not allowed in NuGet.Config so I'd prefer to reject them unless we have a reason to accept them.

[JamieMagee]

This bypasses normaliseHost, so the case-sensitive Azure check selects Bearer instead of Basic auth for a colonless PAT on PKGS.DEV.AZURE.COM.

[brettfo]

In an effort to unblock some scenarios, this will be merged as-is and I'll immediately start work on all remaining comments here.