From d846392a093587c350c7ec271c213cdc71fa657e Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 20:23:56 -0500 Subject: [PATCH] Migrate npm handler to OIDCRegistry Replace manual OIDC credential map, mutex, and direct hostname lookup with the shared OIDCRegistry type. npm previously stored OIDC credentials keyed by hostname only, which caused collisions when multiple npm registries shared a host. OIDCRegistry preserves the full registry URL, fixing this. The Cloudsmith-specific X-Api-Key handling is now provided by OIDCRegistry.TryAuth() automatically. Co-authored-by: James Garratt <572389+microblag@users.noreply.github.com> --- internal/handlers/npm_registry.go | 47 ++++---------------- internal/handlers/oidc_handling_test.go | 59 +++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/internal/handlers/npm_registry.go b/internal/handlers/npm_registry.go index 37a9d49..04b04ff 100644 --- a/internal/handlers/npm_registry.go +++ b/internal/handlers/npm_registry.go @@ -1,10 +1,8 @@ package handlers import ( - "fmt" "net/http" "strings" - "sync" "github.com/elazarl/goproxy" @@ -17,9 +15,8 @@ import ( // NPMRegistryHandler handles requests to NPM registries, adding auth to // requests to registries for which we have credentials. type NPMRegistryHandler struct { - credentials []npmRegistryCredentials - oidcCredentials map[string]*oidc.OIDCCredential - mutex sync.RWMutex + credentials []npmRegistryCredentials + oidcRegistry *oidc.OIDCRegistry } type npmRegistryCredentials struct { @@ -33,8 +30,8 @@ type npmRegistryCredentials struct { // NewNPMRegistryHandler returns a new NPMRegistryHandler, func NewNPMRegistryHandler(creds config.Credentials) *NPMRegistryHandler { handler := NPMRegistryHandler{ - credentials: []npmRegistryCredentials{}, - oidcCredentials: make(map[string]*oidc.OIDCCredential), + credentials: []npmRegistryCredentials{}, + oidcRegistry: oidc.NewOIDCRegistry(), } for _, cred := range creds { @@ -44,19 +41,8 @@ func NewNPMRegistryHandler(creds config.Credentials) *NPMRegistryHandler { registry := cred.GetString("registry") - oidcCredential, _ := oidc.CreateOIDCCredential(cred) - if oidcCredential != nil { - host := cred.Host() - if host == "" && registry != "" { - regURL, err := helpers.ParseURLLax(registry) - if err == nil { - host = regURL.Hostname() - } - } - if host != "" { - handler.oidcCredentials[host] = oidcCredential - logging.RequestLogf(nil, "registered %s OIDC credentials for npm registry: %s", oidcCredential.Provider(), host) - } + // OIDC credentials are not used as static credentials. + if oidcCred, _, _ := handler.oidcRegistry.Register(cred, []string{"registry", "url"}, "npm registry"); oidcCred != nil { continue } @@ -86,25 +72,8 @@ func (h *NPMRegistryHandler) HandleRequest(req *http.Request, ctx *goproxy.Proxy } // Try OIDC credentials first - h.mutex.RLock() - oidcCred, hasOIDC := h.oidcCredentials[reqHost] - h.mutex.RUnlock() - - if hasOIDC { - token, err := oidc.GetOrRefreshOIDCToken(oidcCred, req.Context()) - if err != nil { - logging.RequestLogf(ctx, "* failed to get token via OIDC for %s: %v", reqHost, err) - // Fall through to try static credentials - } else { - if oidcCred.Provider() == "cloudsmith" { - logging.RequestLogf(ctx, "* authenticating npm registry request with OIDC API key (host: %s)", reqHost) - req.Header.Set("X-Api-Key", token) - } else { - logging.RequestLogf(ctx, "* authenticating npm registry request with OIDC token (host: %s)", reqHost) - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - } - return req, nil - } + if h.oidcRegistry.TryAuth(req, ctx) { + return req, nil } // Fall back to static credentials diff --git a/internal/handlers/oidc_handling_test.go b/internal/handlers/oidc_handling_test.go index dc742e6..bca579f 100644 --- a/internal/handlers/oidc_handling_test.go +++ b/internal/handlers/oidc_handling_test.go @@ -721,7 +721,7 @@ func TestOIDCURLsAreAuthenticated(t *testing.T) { }, urlMocks: []mockHttpRequest{}, expectedLogLines: []string{ - "registered aws OIDC credentials for npm registry: npm.example.com", + "registered aws OIDC credentials for npm registry: https://npm.example.com", }, urlsToAuthenticate: []string{ "https://npm.example.com/some-package", @@ -743,7 +743,7 @@ func TestOIDCURLsAreAuthenticated(t *testing.T) { }, urlMocks: []mockHttpRequest{}, expectedLogLines: []string{ - "registered azure OIDC credentials for npm registry: npm.example.com", + "registered azure OIDC credentials for npm registry: https://npm.example.com", }, urlsToAuthenticate: []string{ "https://npm.example.com/some-package", @@ -764,7 +764,7 @@ func TestOIDCURLsAreAuthenticated(t *testing.T) { }, urlMocks: []mockHttpRequest{}, expectedLogLines: []string{ - "registered jfrog OIDC credentials for npm registry: jfrog.example.com", + "registered jfrog OIDC credentials for npm registry: https://jfrog.example.com", }, urlsToAuthenticate: []string{ "https://jfrog.example.com/some-package", @@ -787,7 +787,7 @@ func TestOIDCURLsAreAuthenticated(t *testing.T) { }, urlMocks: []mockHttpRequest{}, expectedLogLines: []string{ - "registered cloudsmith OIDC credentials for npm registry: cloudsmith.example.com", + "registered cloudsmith OIDC credentials for npm registry: https://cloudsmith.example.com", }, urlsToAuthenticate: []string{ "https://cloudsmith.example.com/some-package", @@ -1390,3 +1390,54 @@ func TestOIDCURLsAreAuthenticated(t *testing.T) { }) } } + +// TestNPMOIDCSameHostDifferentPaths verifies that two npm OIDC credentials on +// the same host with different URL paths do not collide — each request is +// authenticated with the credential whose path is the longest prefix match. +func TestNPMOIDCSameHostDifferentPaths(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + tenantA := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + tenantB := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + clientId := "87654321-4321-4321-4321-210987654321" + + tokenUrl := "https://token.actions.example.com" //nolint:gosec // test URL + httpmock.RegisterResponder("GET", tokenUrl, + httpmock.NewStringResponder(200, `{"count":1,"value":"sometoken"}`)) + + httpmock.RegisterResponder("POST", fmt.Sprintf("https://login.microsoftonline.com/%s/oauth2/v2.0/token", tenantA), + httpmock.NewStringResponder(200, `{"access_token":"__token_A__","expires_in":3600,"token_type":"Bearer"}`)) + httpmock.RegisterResponder("POST", fmt.Sprintf("https://login.microsoftonline.com/%s/oauth2/v2.0/token", tenantB), + httpmock.NewStringResponder(200, `{"access_token":"__token_B__","expires_in":3600,"token_type":"Bearer"}`)) + + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", tokenUrl) + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "sometoken") + + creds := config.Credentials{ + config.Credential{ + "type": "npm_registry", + "url": "https://pkgs.example.com/org/feed-A", + "tenant-id": tenantA, + "client-id": clientId, + }, + config.Credential{ + "type": "npm_registry", + "url": "https://pkgs.example.com/org/feed-B", + "tenant-id": tenantB, + "client-id": clientId, + }, + } + + handler := NewNPMRegistryHandler(creds) + + // Request to feed-A path should get token A + reqA := httptest.NewRequest("GET", "https://pkgs.example.com/org/feed-A/some-package", nil) + reqA = handleRequestAndClose(handler, reqA, nil) + assertHasTokenAuth(t, reqA, "Bearer", "__token_A__", "feed-A should use token A") + + // Request to feed-B path should get token B + reqB := httptest.NewRequest("GET", "https://pkgs.example.com/org/feed-B/some-package", nil) + reqB = handleRequestAndClose(handler, reqB, nil) + assertHasTokenAuth(t, reqB, "Bearer", "__token_B__", "feed-B should use token B") +}