fix(coderd)!: restrict OIDC email fallback to first-time account linking by f0ssel · Pull Request #25712 · coder/coder
f0ssel
marked this pull request as ready for review
f0ssel
changed the title
fix(coderd): restrict OIDC email fallback to first-time account linking
fix(coderd)!: restrict OIDC email fallback to first-time account linking
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>
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>
f0ssel
deleted the
fix/plat-229-oidc-linked-id-email-fallback
branch
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