feat: opt-in github numeric user id, reject non-2xx user info - #301
Merged
Conversation
GitHub logins are released on rename or account removal and can be claimed by someone else, so a local id derived from the login may be inherited by the next holder of the name. Every other oauth2 provider here keys on an immutable subject, github was the only one on a mutable field. Add Params.GithubNumericID and Service.AddGithubProviderWithNumericID to derive the id from the numeric account id instead. Off by default because it changes the id of every existing github user, and records stored under the old id cannot be remapped offline once a login is hashed. The numeric id is read from the raw response body with a typed unmarshal. data.Value is not usable for it, json numbers decode to float64 and format as "1.345027e+06". A missing or zero id keeps the login-based value rather than hashing an empty string. Applied to both the v1 (provider/) and v2 (v2/provider/) module paths.
- [major] [provider/providers.go:70] numeric ids shared a hash namespace with logins, which may be all-digit; domain-separate the hashed input with a "gid:" prefix - [major] [auth_test.go] AddGithubProviderWithNumericID had no test; add Service-level coverage plus the default-unchanged negative case - [minor] [provider/providers.go:69] warn when the numeric id is unusable instead of falling back silently - [minor] [provider/providers.go:52] default the logger in NewGithub, the mapUser closure captures p before initOauth2Handler defaults its own copy - [minor] [provider/providers.go:65] comment claimed the login fallback was safe; state the recycling caveat instead - [minor] [provider/oauth2.go:62] document the best-effort fallback on Params.GithubNumericID and the Service method - [minor] [auth.go:389] add the advanced-configuration note the AddMicrosoftProvider godoc carries - [minor] [README.md:749] use placeholders instead of the _example env vars, and name the direct-Params registration route Applied to both the v1 and v2 module paths.
- [major] [README.md:752] the direct-Params recipe omitted AllowedRedirectHosts and L; AddCustomHandler registers the handler as-is, so a service that hardened AllowedRedirectHosts in Opts would silently lose redirect validation for the github provider
The oauth1 and oauth2 callback handlers checked only the transport error from
the user info request, never the HTTP status. An error body from the provider
carries no identity fields, so mapUser hashed empty values and every failed
callback produced the same local id, e.g. github_ + sha1("") for github. A
revoked token, a rate limit or a provider outage was enough to sign the caller
in under that shared identity.
Reject any non-2xx user info response before reading and mapping it. Affects
all oauth2 providers and the oauth1 (twitter) path. Apple and telegram already
checked their status codes.
Applied to both the v1 and v2 module paths.
The "with extra scopes" subtest redeclared r inside the closure, shadowing the handler from the enclosing test. govet's shadow check is enabled and CI runs a pinned golangci-lint, so this failed the lint job. Rename to withAttrs. Applied to both the v1 and v2 module paths.
Coverage Report for CI Build 30137641650Coverage increased (+0.05%) to 85.492%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
two changes, both applied to v1 and v2.
opt-in github user id from the numeric account id
the github provider derives the local user id from the login,
github_<sha1(login)>. GitHub releases a username on rename or account deletion and anyone can claim it afterwards, so an id derived from login can be inherited by the next holder of the name. Every other oauth2 provider here already keys on an immutable subject: google onsub, the rest onidorid_str.new
provider.Params.GithubNumericIDandService.AddGithubProviderWithNumericIDderive the id from the immutable numeric account id instead. Off by default, because switching the derivation changes the id of every existing github user, and apps that key records, roles or blocks on that id cannot remap them offline. The README documents the caveat and the migration cost.the hashed input is domain separated with a
gid:prefix. Without it the two id spaces overlap: github allows all-digit logins, so login27385and account id 27385 produce the same local id, and those are two different real accounts.reject non-2xx user info responses
the oauth1 and oauth2 callback handlers checked only the transport error from the user info request, never the HTTP status. An error body carries no identity fields, so
mapUserhashed empty values and every failed callback produced the same local id,github_+ sha1("") for github. A revoked token, a rate limit or a provider outage was enough to sign the caller in under that shared identity.this affects all oauth2 providers and the oauth1 (twitter) path. Apple and telegram already checked their status codes.