Conversation
The Facebook branch of getProvider aliased config.External.Facebook — a field of the long-lived, process-wide *conf.GlobalConfiguration shared across all requests — and wrote SkipNonceCheck on it on every Facebook id_token login. Concurrent Facebook logins (and any concurrent reader of that config) race on this write. Copy the provider config into a local value and override SkipNonceCheck on the copy, matching what the custom/default branches already do. Adds a test asserting the shared config's SkipNonceCheck is not mutated.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
In
getProvider, the Facebook branch aliases the shared configuration and mutates it:configis the long-lived*conf.GlobalConfigurationshared across all requests, socfgaliasesconfig.External.Facebookand this writes shared state on every Facebookid_tokenlogin. Concurrent Facebook logins — and any concurrent reader ofconfig.External.Facebook— race on this write (go test -raceterritory). The written value is alwaystrue, so the practical harm is limited, but it is a real data race and an unexpected persistent mutation of shared config.The
custom:and default branches already avoid this by building a localconf.OAuthProviderConfigurationvalue.Fix
Copy the provider config into a local value and set
SkipNonceCheckon the copy, leaving the shared config untouched.Testing
Added
TestGetProviderFacebookDoesNotMutateSharedConfig: with Facebook disabled,getProviderreturns before any OIDC discovery (no network), and the test assertsconfig.External.Facebook.SkipNonceCheckremainsfalseafter the call — it istrueon the old aliasing code.go vet,gofmt, and the OIDC test suite pass.