fix(op): isolate provider endpoints and CORS options from package globals - #927
Conversation
…bals 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.
| issuer IssuerFromRequest | ||
| insecure bool | ||
| endpoints *Endpoints | ||
| endpoints Endpoints |
There was a problem hiding this comment.
This changes the public contract and thus is a breaking change. We need to either keep it a pointer but copy the values, or merge this into next instead of main.
There was a problem hiding this comment.
Pull request overview
This PR fixes unintended cross-provider sharing of mutable defaults in the OP package by copying default endpoints/CORS options into each newly constructed provider and by preventing callers from mutating package-default slices via returned values.
Changes:
- Make
Provider.endpointsa per-providerEndpointsvalue copied fromDefaultEndpoints, soWithCustom*Endpointno longer mutates global defaults or other providers. - Copy CORS options per provider/server to avoid global CORS reconfiguration via returned pointers.
- Return cloned scope/claim slices from discovery helpers and add regression tests for endpoint isolation and a former race scenario.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/op/op.go | Copies default endpoints/CORS options into each provider; updates Provider.endpoints to a value. |
| pkg/op/server_http.go | Copies default server CORS options per registered server instance. |
| pkg/op/discovery.go | Uses slices.Clone to prevent callers from mutating default supported scopes/claims. |
| pkg/op/op_test.go | Adds regression tests for default endpoint mutation and a former race scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decoder := schema.NewDecoder() | ||
| decoder.IgnoreUnknownKeys(true) | ||
|
|
||
| corsOpts := defaultCORSOptions |
There was a problem hiding this comment.
I would like to see a copy/clone function for that so that this behavior is isolated.
| // 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 |
There was a problem hiding this comment.
I would like to see a copy/clone function for that so that this behavior is isolated.
Which Problems Are Solved
Provider.endpointswas a*Endpointspointing at the package-globalDefaultEndpoints, soWithCustomAuthEndpointand the otherWithCustom*Endpointoptions assigned through the pointer into the global rather than into the provider. A custom endpoint set on one provider appeared on every other provider constructed in the same process, including providers that never asked for it.Endpoints.go test -racereports it on(*Provider).TokenEndpoint().Scopes()andSupportedClaims()returned the exportedDefaultSupportedScopesandDefaultSupportedClaimsslices directly, so a caller that mutates the returned slice edits the package defaults for the whole process.How the Problems Are Solved
Provider.endpointsis now anEndpointsvalue instead of a*Endpoints.NewProvidercopies the defaults and eachWithCustom*Endpointassigns into the provider's own copy. The exportedDefaultEndpointskeeps its*Endpointstype, so there is no API change.NewProvidercopiesdefaultCORSOptionsper provider.CORSOptions()returns that pointer to callers, so while the global was shared,p.CORSOptions().AllowedOrigins = ...reconfigured CORS for every provider in the process.Scopes()andSupportedClaims()returnslices.Clone(...).Additional Changes
RegisterServeralso copiesdefaultCORSOptionsinstead of taking the address of the global. There is no reachable mutation path today, becauseWithServerCORSOptionsreplaces the pointer andwebServerexposes no accessor. It removes the last alias to the global and keeps both constructors consistent.main:TestNewProviderDoesNotMutateDefaultEndpointsfails withsibling provider inherited a customisation it did not request.TestNewProviderEndpointsConcurrentRacetrips the race detector.Additional Context
op.DefaultEndpointsbefore constructing a provider still works as a process-wide default. It no longer retroactively mutates providers that already exist, which is the racy behaviour being removed.