fix(coderd)!: restrict OIDC email fallback to first-time account linking#25712
Conversation
|
/coder-agents-review |
|
Review posted | Chat Review history
deep-review v0.5.0 | Round 1 | Last posted: Round 1, 14 findings (3 P2, 6 P3, 4 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 3 P2, 5 P3, 3 Nit, 1 Note new. 0 dropped. Reviewed against 20b50dd..1d1e257. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Solid security fix. The core defense in findLinkedUser is correct, the SQL-level WHERE linked_id = '' guard is proper defense-in-depth, and the loginType parameter change fixes a real correctness bug beyond the security goal. The test suite covers the three critical paths (attacker blocked, first-time link, legacy backfill).
3 P2, 5 P3, 3 Nit, 1 Note.
The two P2s that need attention before merge: (1) a TOCTOU race between findLinkedUser (outside the transaction) and the backfill (inside), allowing a concurrent attacker to get a session on a legacy account, fixable with a 4-line re-check after UpdateUserLink; and (2) when signups are enabled and the email fallback blocks an attacker, the silent nil-error return causes oauthLogin to attempt CreateUser, hitting a unique constraint and returning 500 with raw DB error details instead of a clean 403. The third P2 is a test weakness: the backfill test asserts NotEmpty but never verifies the value is correct.
Legacy accounts with empty linked_id remain exploitable via the same attack vector until each user individually logs in. This is a documented design constraint (the alternative breaks all legacy users), but the residual risk is not tracked in a separate issue. This needs a human decision: file a tracking ticket or explicitly accept the gap.
"The backfill turns a repeatable-access vulnerability into a one-shot permanent-lockout-plus-takeover for legacy accounts. The PR is a net improvement, but the first login wins and the loser is locked out." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
Documentation CheckNo documentation changes are needed for this PR. This is a security fix that:
Automated review via Coder Agents |
| // Defense-in-depth: if a concurrent transaction backfilled | ||
| // linked_id between findLinkedUser and this point, reject. | ||
| if link.LinkedID != "" && link.LinkedID != params.LinkedID { | ||
| return xerrors.Errorf("identity mismatch: account bound to different subject") |
There was a problem hiding this comment.
Will this error result in a 5xx status code? I think we want this to result in a 4xx. Seems like idpsync.HTTPError can be used for this.
There was a problem hiding this comment.
Good catch, that bare error would have surfaced as a 500. Fixed in aea4e59: it now returns an idpsync.HTTPError with http.StatusForbidden, which both callers already render via idpsync.IsHTTPError(err) + hErr.Write(...). It uses the same message and static error page as the primary errLinkedIDAlreadyBound guard, so the rare race renders an identical clean 403.
if link.LinkedID != "" && link.LinkedID != params.LinkedID {
return &idpsync.HTTPError{
Code: http.StatusForbidden,
Msg: "Account already linked",
Detail: "This account is already linked to a different identity provider subject. Contact your administrator.",
RenderStaticPage: true,
}
}Note on testing: this branch only fires on a concurrent backfill that lands between findLinkedUser and UpdateUserLink. In the non-concurrent case findLinkedUser already returns errLinkedIDAlreadyBound for a populated, mismatched linked_id, so it never reaches here. Triggering it deterministically needs a store shim that returns an empty linked_id on the findLinkedUser read but a different one from UpdateUserLink. I left it untested since it's defense-in-depth, but happy to add the shim-based test if you'd like it covered.
Generated by Coder Agents on behalf of @f0ssel.
|
We should mark this a breaking change. I was looking for an issue I could not find. But I know we had an issue in the past with more than 1 account being bound to the same Coder account or something strange like this (if no linked id exists). That behavior is obviously not good, but is often not malicious. Just the same person twice or w/e. (I can't recall the specifics). Regardless, mark this behavior a breaking change so if something does happen, we can point to the release notes. |
| func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) { | ||
| fetch := func(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) { | ||
| return q.db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ | ||
| UserID: arg.UserID, | ||
| LoginType: arg.LoginType, | ||
| }) | ||
| } | ||
| return fetchAndQuery(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.UpdateUserLinkedID)(ctx, arg) |
There was a problem hiding this comment.
Should this be ActionUpdate instead?
Read is ResouceSystem which is not good. But write is ActionUpdate
coder/coderd/database/dbauthz/dbauthz.go
Lines 5901 to 5908 in 024f536
There was a problem hiding this comment.
Good call, switched to ActionUpdate in 61fa6e0. UpdateUserLinkedID now authorizes policy.ActionUpdate on rbac.ResourceUserObject(arg.UserID), matching the sibling InsertUserLink, instead of the fetchAndQuery(..., ActionUpdatePersonal, ...) form. That drops the redundant GetUserLinkByUserIDLoginType read and checks the write directly against the user object. Updated the dbauthz accounting test to assert ActionUpdate.
Generated by Coder Agents on behalf of @f0ssel.
|
Take this with a grain of salt Test reviewReviewed the three new tests against the intended behavior. What's covered correctly
What's missing1. No test for the issuer URL change scenario — this is the most impactful breaking behavior this PR introduces. A user with 2. No GitHub OAuth equivalent for 3.
|
|
Thanks @Emyrk. Addressed in 61fa6e0. Breaking change — Marked the PR Test review —
Also applied your dbauthz suggestion:
|
The findLinkedUser function falls back to email-based user lookup when no linked_id match is found. This fallback was used for all logins, not just first-time linking, allowing an attacker with the victim's email at the IdP (but a different subject) to hijack an existing account. Restrict the email fallback so that when a user found by email already has a user_link with a non-empty linked_id that differs from the current login's linked_id, the function does not return that user. This blocks account takeover while preserving first-time linking (no existing link) and legacy links (empty linked_id). Also backfill linked_id on legacy user_links via a new UpdateUserLinkedID query that only sets the value when currently empty. Fixes: https://linear.app/codercom/issue/PLAT-229
The TestMethodTestSuite/Accounting test requires every database method to have an authorization test. Add the missing test for the new UpdateUserLinkedID query.
CRF-3 (P2): Add defense-in-depth TOCTOU check inside oauthLogin transaction. After UpdateUserLink, verify link.LinkedID matches params.LinkedID to catch concurrent backfill races on legacy links. CRF-4 (P2): Return sentinel errLinkedIDAlreadyBound from findLinkedUser instead of nil. Both OIDC and GitHub callers now render a clean 403 with a descriptive message instead of leaking DB constraint errors. CRF-5 (P2): Save the sub, compute expected linked_id from fake.IssuerURL(), and assert exact equality in OIDCLegacyLinkBackfill. CRF-1 (P3): Callers now log a warn-level message when the sentinel error is returned, providing operator visibility. CRF-2 (Nit): Fix double-negative typo in findLinkedUser doc comment. CRF-6 (P3): Update doc comment to accurately describe email fallback scope (first-time linking and legacy links with empty linked_id). CRF-8 (P3): Add AllowSignups=true subtest to OIDCEmailFallbackBlocked to cover both signup paths. CRF-9 (P3): Add linked_id verification to OIDCFirstTimeLinkByEmailAllowed. CRF-10 (P3): Use fake.IssuerURL() for victim's linked_id in tests so the issuer format is realistic (same issuer, different subject). CRF-11, CRF-12 (Nit): Tighten comments to avoid restating guards.
- Authorize UpdateUserLinkedID with ActionUpdate on the user object, matching the sibling InsertUserLink, instead of ActionUpdatePersonal. - Add a GitHub OAuth test for the errLinkedIDAlreadyBound email-fallback block (previously only the OIDC path was covered). - Add an OIDC issuer-URL-change test documenting the deliberate breaking behavior for existing links recorded under a previous issuer. Co-authored-by: Coder Agents <agents@coder.com>
61fa6e0 to
fae5721
Compare
The defense-in-depth re-check inside the oauthLogin transaction returned a bare error, which surfaced as a 500. Return an idpsync.HTTPError with StatusForbidden so the concurrent-backfill race renders the same clean 403 as the primary findLinkedUser guard. Co-authored-by: Coder Agents <agents@coder.com>
Problem
findLinkedUserincoderd/userauth.gofalls back to email-based user lookup when nolinked_idmatch is found. This fallback was used for all logins, not just first-time linking. An attacker who registers the victim's email at the IdP (with a different OIDC subject) bypasses thelinked_idcheck and gets matched to the victim's Coder account.Combined with the
email_verifiedtype assertion bypass (PLAT-228), this creates a chained account-takeover vector.Fix
Restrict the email fallback in
findLinkedUserso that when a user found by email already has auser_linkwith a non-emptylinked_idthat differs from the current login'slinked_id, the function returns no user. This blocks account takeover while preserving:user_linkexists, email fallback works as before.linked_id(pre-migration), email fallback still works.linked_idresolves via the primary path, no fallback needed.Also adds a
UpdateUserLinkedIDquery to backfilllinked_idon legacy links (only when currently empty) during login, gradually migrating them to the secure path.The
findLinkedUsersignature now acceptsloginTypeexplicitly instead of relying onuser.LoginType, ensuring the correct link is checked in the legacy lookup.Breaking change
Marked
release/breaking. An account whoseuser_linkalready has a populatedlinked_idthat does not match the subject the IdP presents will now be denied login (403) instead of silently resolving via the email fallback. The most likely trigger is changingCODER_OIDC_ISSUER_URL(thelinked_idisissuer||subject), or two identities sharing one email. Accounts with an empty (legacy)linked_idare unaffected and are backfilled on their next login.Fixes: https://linear.app/codercom/issue/PLAT-229
Implementation details
Files changed
coderd/userauth.go: Core fix infindLinkedUser+ backfill logic inoauthLogincoderd/database/queries/user_links.sql: NewUpdateUserLinkedIDquerycoderd/database/dbauthz/dbauthz.go: Authorization for new query (ActionUpdateon the user object, matchingInsertUserLink)coderd/userauth_test.go: New OIDC and GitHub testsqueries.sql.go,querier.go,dbmock.go,querymetrics.goNew tests
TestUserOIDC/OIDCEmailFallbackBlockedByExistingLink: Attacker with a differentsubbut the same email is rejected (403) when the victim has an existing link (covers signups enabled and disabled).TestUserOIDC/OIDCFirstTimeLinkByEmailAllowed: User created via SCIM/API (nouser_link) can still link via email on first OIDC login, and thelinked_idis populated.TestUserOIDC/OIDCLegacyLinkBackfill: User with emptylinked_idcan login and theirlinked_idis backfilled with the correct value.TestUserOIDC/OIDCEmailFallbackBlockedByIssuerChange: Existing link recorded under a previous issuer is rejected (403) after the issuer changes (documents the breaking behavior).TestUserOAuth2Github/EmailFallbackBlockedByExistingLink: GitHub attacker with a different user ID but the victim's email is rejected (403).Note
This PR was authored by Coder Agents on behalf of @f0ssel.