From da3fb5aca3557a812629582d5e616347412a4dde Mon Sep 17 00:00:00 2001 From: Lewis Treacy Date: Sun, 26 Jul 2026 19:36:52 +0100 Subject: [PATCH] fix(op): isolate provider endpoints and CORS options from package globals Provider.endpoints was a *Endpoints pointing at the package-global DefaultEndpoints, so every WithCustom*Endpoint option wrote through into the global. Custom endpoints leaked into other providers built in the same process, and those writes raced with reads on a provider already serving requests. Store endpoints by value so each provider copies the defaults and customises its own copy. Copy defaultCORSOptions per provider for the same reason, since CORSOptions() hands its pointer to callers. Return copies from Scopes() and SupportedClaims() so callers cannot edit the exported default slices in place. Add regression tests for the cross-provider leak and the data race. --- pkg/op/discovery.go | 5 ++-- pkg/op/op.go | 8 +++-- pkg/op/op_test.go | 70 +++++++++++++++++++++++++++++++++++++++++++ pkg/op/server_http.go | 3 +- 4 files changed, 80 insertions(+), 6 deletions(-) diff --git a/pkg/op/discovery.go b/pkg/op/discovery.go index e3ca6035..0bce1cf0 100644 --- a/pkg/op/discovery.go +++ b/pkg/op/discovery.go @@ -3,6 +3,7 @@ package op import ( "context" "net/http" + "slices" jose "github.com/go-jose/go-jose/v4" @@ -105,7 +106,7 @@ func Scopes(c Configuration) []string { if ok && provider.config.SupportedScopes != nil { return provider.config.SupportedScopes } - return DefaultSupportedScopes + return slices.Clone(DefaultSupportedScopes) } func ResponseTypes(c Configuration) []string { @@ -230,7 +231,7 @@ func SupportedClaims(c Configuration) []string { return provider.config.SupportedClaims } - return DefaultSupportedClaims + return slices.Clone(DefaultSupportedClaims) } func CodeChallengeMethods(c Configuration) []oidc.CodeChallengeMethod { diff --git a/pkg/op/op.go b/pkg/op/op.go index bb789a39..cbd500bf 100644 --- a/pkg/op/op.go +++ b/pkg/op/op.go @@ -269,15 +269,17 @@ func NewProvider( NewAESCrypto(config.CryptoKey), }, ) + // Copied per provider: the options below assign into these and CORSOptions() hands its pointer to callers, so sharing the globals would let one provider reconfigure every other. + corsOpts := defaultCORSOptions o := &Provider{ config: config, storage: storage, accessTokenKeySet: keySet, idTokenHinKeySet: keySet, crypto: crypto, - endpoints: DefaultEndpoints, + endpoints: *DefaultEndpoints, timer: make(<-chan time.Time), - corsOpts: &defaultCORSOptions, + corsOpts: &corsOpts, } for _, optFunc := range opOpts { @@ -302,7 +304,7 @@ type Provider struct { config *Config issuer IssuerFromRequest insecure bool - endpoints *Endpoints + endpoints Endpoints storage Storage accessTokenKeySet oidc.KeySet idTokenHinKeySet oidc.KeySet diff --git a/pkg/op/op_test.go b/pkg/op/op_test.go index ca68ca07..08cf5bb7 100644 --- a/pkg/op/op_test.go +++ b/pkg/op/op_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync" "testing" "time" @@ -469,3 +470,72 @@ func TestWithCustomEndpoints(t *testing.T) { }) } } + +// A customisation must not reach the global or a sibling provider that never asked for it. +func TestNewProviderDoesNotMutateDefaultEndpoints(t *testing.T) { + defaultsBefore := *op.DefaultEndpoints + + customAuth := op.NewEndpoint("/custom/authorize") + _, err := op.NewOpenIDProvider(testIssuer, testConfig, + storage.NewStorage(storage.NewUserStore(testIssuer)), + op.WithCustomAuthEndpoint(customAuth), + ) + require.NoError(t, err) + + assert.Equal(t, defaultsBefore, *op.DefaultEndpoints, + "DefaultEndpoints was mutated by WithCustomAuthEndpoint") + + sibling, err := op.NewOpenIDProvider(testIssuer, testConfig, + storage.NewStorage(storage.NewUserStore(testIssuer)), + ) + require.NoError(t, err) + assert.NotEqual(t, customAuth, sibling.AuthorizationEndpoint(), + "sibling provider inherited a customisation it did not request") +} + +// WithCustom*Endpoint writes inside NewProvider raced with reads on an already-serving provider. +func TestNewProviderEndpointsConcurrentRace(t *testing.T) { + reader, err := op.NewOpenIDProvider(testIssuer, testConfig, + storage.NewStorage(storage.NewUserStore(testIssuer)), + ) + require.NoError(t, err) + + stop := make(chan struct{}) + readerDone := make(chan struct{}) + go func() { + defer close(readerDone) + for { + select { + case <-stop: + return + default: + _ = reader.AuthorizationEndpoint().Relative() + } + } + }() + + const builders = 8 + var builderWg sync.WaitGroup + builderWg.Add(builders) + for range builders { + go func() { + defer builderWg.Done() + _, err := op.NewOpenIDProvider(testIssuer, testConfig, + storage.NewStorage(storage.NewUserStore(testIssuer)), + op.WithCustomEndpoints( + op.NewEndpoint("/authorize"), + op.NewEndpoint("/oauth/token"), + op.NewEndpoint("/userinfo"), + op.NewEndpoint("/revoke"), + op.NewEndpoint("/end_session"), + op.NewEndpoint("/keys"), + ), + ) + assert.NoError(t, err) + }() + } + + builderWg.Wait() + close(stop) + <-readerDone +} diff --git a/pkg/op/server_http.go b/pkg/op/server_http.go index 274cabd3..c12ced8d 100644 --- a/pkg/op/server_http.go +++ b/pkg/op/server_http.go @@ -23,12 +23,13 @@ func RegisterServer(server Server, endpoints Endpoints, options ...ServerOption) decoder := schema.NewDecoder() decoder.IgnoreUnknownKeys(true) + corsOpts := defaultCORSOptions ws := &webServer{ router: chi.NewRouter(), server: server, endpoints: endpoints, decoder: decoder, - corsOpts: &defaultCORSOptions, + corsOpts: &corsOpts, } for _, option := range options {