From 657b888d133895c1a30b0aadb3b425be6474375f Mon Sep 17 00:00:00 2001 From: "Brett V. Forsgren" Date: Fri, 11 Sep 2026 10:51:59 -0600 Subject: [PATCH] Address NuGet discovery review feedback Normalize scheme-less service index URLs to HTTPS, reject non-HTTPS OIDC discovery redirects, and handle Azure DevOps hostnames case-insensitively. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/handlers/nuget_feed.go | 29 +++++- internal/handlers/nuget_feed_test.go | 142 ++++++++++++++++++++++++--- 2 files changed, 154 insertions(+), 17 deletions(-) diff --git a/internal/handlers/nuget_feed.go b/internal/handlers/nuget_feed.go index 29d8f61..e4b929b 100644 --- a/internal/handlers/nuget_feed.go +++ b/internal/handlers/nuget_feed.go @@ -171,6 +171,13 @@ func addNugetDiscoveryJob( sourceURLs map[string]struct{}, job nugetDiscoveryJob, ) { + serviceIndexURL, err := normalizeNugetServiceIndexURL(job.serviceIndexURL) + if err != nil { + logging.RequestLogf(nil, "skipping invalid NuGet service index URL %s: %v", job.serviceIndexURL, err) + return + } + job.serviceIndexURL = serviceIndexURL + key := nugetDiscoverySourceKey(job.serviceIndexURL) if _, ok := sourceURLs[key]; ok { logging.RequestLogf(nil, "skipping duplicate NuGet service index because it is already registered: %s", job.serviceIndexURL) @@ -180,6 +187,20 @@ func addNugetDiscoveryJob( *jobs = append(*jobs, job) } +func normalizeNugetServiceIndexURL(rawURL string) (string, error) { + parsedURL, err := helpers.ParseURLLax(rawURL) + if err != nil { + return "", err + } + if parsedURL.Hostname() == "" { + return "", fmt.Errorf("missing host") + } + if parsedURL.Scheme == "" { + parsedURL.Scheme = "https" + } + return parsedURL.String(), nil +} + func discoverNugetFeedURLs( jobs []nugetDiscoveryJob, client *http.Client, @@ -240,6 +261,9 @@ func discoverNugetFeedURLsForJob( discoveryClient := *client originalCheckRedirect := client.CheckRedirect discoveryClient.CheckRedirect = func(redirectReq *http.Request, via []*http.Request) error { + if job.oidcCredential != nil && !strings.EqualFold(redirectReq.URL.Scheme, "https") { + return fmt.Errorf("refusing to redirect OIDC-authenticated NuGet discovery to non-HTTPS URL %s", redirectReq.URL) + } if originalCheckRedirect != nil { if err := originalCheckRedirect(redirectReq, via); err != nil { return err @@ -489,8 +513,9 @@ func authenticateNugetRequest(req *http.Request, cred nugetFeedCredentials, prox } func shouldTreatTokenAsPassword(url *url.URL) bool { - if url.Hostname() == "pkgs.dev.azure.com" { + hostname := strings.ToLower(url.Hostname()) + if hostname == "pkgs.dev.azure.com" { return true } - return strings.HasSuffix(url.Hostname(), ".pkgs.visualstudio.com") && strings.Contains(url.Path, "/_packaging/") + return strings.HasSuffix(hostname, ".pkgs.visualstudio.com") && strings.Contains(url.Path, "/_packaging/") } diff --git a/internal/handlers/nuget_feed_test.go b/internal/handlers/nuget_feed_test.go index 64b12b4..662eaf3 100644 --- a/internal/handlers/nuget_feed_test.go +++ b/internal/handlers/nuget_feed_test.go @@ -240,21 +240,24 @@ func TestNugetFeedHandler(t *testing.T) { } func TestShouldTreatTokenAsPassword(t *testing.T) { - // Test case 1: URL with hostname "pkgs.dev.azure.com" - url1, _ := url.Parse("https://pkgs.dev.azure.com/example") - assert.True(t, shouldTreatTokenAsPassword(url1)) - - // Test case 2: URL with visualsutudio hostname suffix - url2, _ := url.Parse("https://example.pkgs.visualstudio.com/_packaging/") - assert.True(t, shouldTreatTokenAsPassword(url2)) - - // Test case 3: Similar but not exactly the same as test case 2; should fail - url3, _ := url.Parse("sneaky.example.com/nuget.visualstudio.com/_packaging") - assert.False(t, shouldTreatTokenAsPassword(url3)) - - // Test case 3: URL with hostname not equal to "pkgs.dev.azure.com" and not matching the pattern - url4, _ := url.Parse("https://example.com") - assert.False(t, shouldTreatTokenAsPassword(url4)) + for _, testCase := range []struct { + name string + rawURL string + expected bool + }{ + {name: "Azure DevOps", rawURL: "https://pkgs.dev.azure.com/example", expected: true}, + {name: "uppercase Azure DevOps", rawURL: "https://PKGS.DEV.AZURE.COM/example", expected: true}, + {name: "Visual Studio", rawURL: "https://example.pkgs.visualstudio.com/_packaging/", expected: true}, + {name: "uppercase Visual Studio", rawURL: "https://EXAMPLE.PKGS.VISUALSTUDIO.COM/_packaging/", expected: true}, + {name: "similar Visual Studio path", rawURL: "sneaky.example.com/nuget.visualstudio.com/_packaging"}, + {name: "unrelated host", rawURL: "https://example.com"}, + } { + t.Run(testCase.name, func(t *testing.T) { + parsedURL, err := url.Parse(testCase.rawURL) + require.NoError(t, err) + assert.Equal(t, testCase.expected, shouldTreatTokenAsPassword(parsedURL)) + }) + } } func TestUrlsCanBeDeterminedFromNuGetFeeds(t *testing.T) { @@ -394,6 +397,26 @@ func TestNewNugetFeedHandlerDiscoversResources(t *testing.T) { assertHasTokenAuth(t, req, "Bearer", "some-token", "resource discovered during construction") } +func TestNewNugetFeedHandlerNormalizesSchemeLessDiscoveryURL(t *testing.T) { + const resourceURL = "https://cdn.example.com/packages" + var discoveryURL string + client := &http.Client{ + Timeout: 5 * time.Second, + Transport: nugetRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + discoveryURL = req.URL.String() + return nugetDiscoveryResponse(resourceURL), nil + }), + } + handler := NewNugetFeedHandler(config.Credentials{ + {"type": "nuget_feed", "url": "nuget.example.com/index.json", "token": "some-token"}, + }, client) + + assert.Equal(t, "https://nuget.example.com/index.json", discoveryURL) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, resourceURL+"/example/index.json", nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertHasTokenAuth(t, req, "Bearer", "some-token", "resource discovered from scheme-less URL") +} + func TestNugetFeedHandlerProxyOnlyCredentials(t *testing.T) { handler := NewNugetFeedHandler(config.Credentials{ { @@ -779,6 +802,95 @@ func TestNugetFeedHandlerDiscoversThroughCrossOriginRedirectWithoutLeakingCreden assertHasTokenAuth(t, req, "Bearer", "some-token", "resource discovered after cross-origin redirect") } +func TestNugetFeedHandlerStripsAuthenticationAfterHTTPSDowngrade(t *testing.T) { + const resourceURL = "https://cdn.example.com/packages" + var firstRedirectAuth string + var finalRedirectAuth string + client := &http.Client{ + Timeout: 5 * time.Second, + Transport: nugetRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + switch { + case req.URL.Scheme == "https": + return nugetRedirectResponse("http://nuget.example.com/index.json"), nil + case req.URL.Path == "/index.json": + firstRedirectAuth = req.Header.Get("Authorization") + return nugetRedirectResponse("/v3/index.json"), nil + case req.URL.Path == "/v3/index.json": + finalRedirectAuth = req.Header.Get("Authorization") + return nugetDiscoveryResponse(resourceURL), nil + default: + return nil, fmt.Errorf("unexpected discovery URL %s", req.URL) + } + }), + } + handler := NewNugetFeedHandler(config.Credentials{ + {"type": "nuget_feed", "url": "https://nuget.example.com/source/index.json", "token": "some-token"}, + }, client) + + assert.Empty(t, firstRedirectAuth) + assert.Empty(t, finalRedirectAuth) + for _, redirectURL := range []string{ + "http://nuget.example.com/index.json", + "http://nuget.example.com/v3/index.json", + } { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, redirectURL, nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertUnauthenticated(t, req, "HTTP service-index redirect") + } + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, resourceURL+"/example/index.json", nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertHasTokenAuth(t, req, "Bearer", "some-token", "resource discovered after HTTPS downgrade") +} + +func TestNugetFeedHandlerRejectsNonHTTPSOIDCRedirect(t *testing.T) { + const tokenURL = "https://actions.example.test/token" //nolint:gosec // test URL + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", tokenURL) + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "request-token") + + var insecureDiscoveryRequested bool + client := &http.Client{ + Timeout: 5 * time.Second, + Transport: nugetRoundTripperFunc(func(req *http.Request) (*http.Response, error) { + var body string + switch req.URL.Hostname() { + case "actions.example.test": + body = `{"count":1,"value":"github-token"}` + case "login.microsoftonline.com": + body = `{"access_token":"oidc-token","expires_in":3600,"token_type":"Bearer"}` + case "nuget.example.com": + if req.URL.Scheme == "http" { + insecureDiscoveryRequested = true + return nugetDiscoveryResponse("https://cdn.example.com/packages"), nil + } + return nugetRedirectResponse("http://nuget.example.com/insecure/index.json"), nil + default: + return nil, fmt.Errorf("unexpected request URL %s", req.URL) + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + Request: req, + }, nil + }), + } + + handler := NewNugetFeedHandler(config.Credentials{ + { + "type": "nuget_feed", + "url": "https://nuget.example.com/index.json", + "tenant-id": "tenant", + "client-id": "client", + }, + }, client) + + assert.False(t, insecureDiscoveryRequested) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "http://nuget.example.com/insecure/index.json", nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertUnauthenticated(t, req, "rejected non-HTTPS OIDC redirect") +} + func TestNugetFeedHandlerAuthenticatesSameOriginServiceIndexRedirect(t *testing.T) { testCases := []struct { name string