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 {