fix(platform): SSO team-sync provenance, 2FA grace anchor, SCIM guards - #3173
Draft
larryro wants to merge 4 commits into
Draft
fix(platform): SSO team-sync provenance, 2FA grace anchor, SCIM guards#3173larryro wants to merge 4 commits into
larryro wants to merge 4 commits into
Conversation
On every SSO sign-in the group→team sync pruned the user from EVERY org team whose name was missing from the IdP claim and deleted the team when it emptied — including teams an admin built under Settings > Teams and teams SCIM provisions, whose sso_provisioning_links Group row was left orphaned so the IdP's next group sync 404ed. A routine login dismantled structures the org manages elsewhere. Migration 0070 adds app.sso_synced_teams / app.sso_synced_team_members, recording what the sync itself created; the reconcile is scoped to that: only memberships the sync granted are revoked, only teams the sync created are reaped (and never while a SCIM Group link claims them), excluded groups are unmanaged, stale provenance is swept lazily, and org deletion drops the rows with the teams. No backfill — unknown provenance reads as "not mine", so nothing pre-existing is ever destroyed.
The grace anchor was set only in the Better Auth password after-hook. handleSsoLogin and the trusted-headers door mint their session rows directly, so for users under an enforced policy (exemptSsoUsers=false) no anchor was ever persisted: evaluateTwoFactorEnforcement recomputed the deadline as now + grace on every read and 'blocked' was never reached — a perpetual grace banner and no enrolment deadline. anchorTwoFactorGraceOnSignIn is the one sign-in anchor all three doors run; the SSO door anchors before minting (no orphan session on failure), the proxy door inside its transaction.
The cookie carries signCookieValue's `${token}.${signature}`, but the
reuse lookup matched it against the bare token column — never a hit, so
the same-session refresh and the account-switch branches were dead and
every request fell through to adopting an arbitrary session row of the
user (unordered LIMIT 1): two devices behind the proxy shared one
session, signing out on one killed both, account switches left orphans.
The route now verifies the cookie (verifySignedValue) and looks up the
bare token; a cookie that fails verification counts as absent; the
cross-device fallback is removed — no cookie means a fresh session.
Three SCIM holes, each refused inside the transaction before any write: - PATCH/PUT active:false on the org owner: DELETE protected the owner, PATCH wrote role=disabled and locked the whole org out of administration. Same classifyDeprovision verdict → 403 mutability. - Group writes accepted any member id: a foreign tenant's user became a teamMember row in this org's team (or a garbage id an opaque 500). Every desired/added id must be a member of the token's org → 400 invalidValue. - userName rewrites updated the global "user".email unconditionally: a collision was a unique-index 500 the IdP retries forever, and one org's IdP could redirect a cross-org user's sign-in identity. Now: unchanged → no-op; taken → 409 uniqueness (also on a lost race); account with memberships in any other org → 403 mutability, identity untouched (the #3159 cross-org authority rule applied to SCIM). scimResponseForAppError maps the coded refusals to their RFC 7644 answers; the docs state the contracts in en/de/fr.
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.
Closes the still-live SSO/SCIM identity-sync findings from the backend deep review (mediums): a data-loss on every SSO login, three SCIM authorization holes, an SSO 2FA-grace gap, and the dead trusted-headers cookie branch. Base is
fedc8cc15(includes #3131 and #3159); each finding was verified against it first.Per-finding outcome
sso/service.tsprune)sso_provisioning_linksGroup row)handleSsoLoginminted the session with noevaluateTwoFactorEnforcement/setGraceUntilIfAbsent. New sharedanchorTwoFactorGraceOnSignInruns on the password hook, the SSO door and the trusted-headers door.active:falsecan deactivate the org owner (DELETE protects them)patchUserranplanActivationunconditionally. It now refuses the owner via the sameclassifyDeprovisionverdict DELETE uses → 403mutability, before any write.setTeamMembersand thepatchGroupadd-branch inserted whatever ids arrived. Every desired/added id is now checked against the token org'smemberrows → 400invalidValue, nothing written.UPDATE "user" SET email. Contract below.${token}.${signature}against the token column; every request fell through to an unordered "any session of this user" adoption. The route now verifies the cookie (verifySignedValue) and looks up the bare token; the cross-device fallback is removed.TRUSTED_HEADERS_INTERNAL_SECRETgate can never fire — route passes the env value to itself92f19584b)Remote-Internal-Secret(orTRUSTED_SECRET_HEADER) and the service compares withtimingSafeEqual; covered bytrusted-headers.test.ts(missing/wrong/unset secret refused) and the integration lane'ssecretGateassertion.The team-sync provenance rule
syncTeamsFromGroupNamesmay prune only what it created. Two new tables,app.sso_synced_teamsandapp.sso_synced_team_members(migration0070_sso_team_sync_provenance.sql), record the teams and memberships the sync itself inserted:No backfill: pre-existing teams and memberships have unknown provenance, and unknown reads as "not mine" — the sync never destroys what it cannot prove it created. Consequence for existing deployments: memberships the sync granted before 0070 are no longer revoked automatically (an admin removes them once); everything the sync grants from now on reconciles as before. The base integration lane's own expectation — the sync-created
Opsteam IS reaped when the group disappears — still holds.The SCIM
userNamecontract (patchUser/ PUT)"user".emailis the account's global sign-in identity, shared by every org the user belongs to, so an org's SCIM may rewrite it only when the account is that org's alone:uniqueness(scim_user_conflict), also on a lost race (SQLSTATE 23505 mapped) — never a unique-index 500 the IdP retries as a server fault;mutability(scim_identity_shared), identity untouched — the cross-org authority rule of fix(platform): close account and credential authorization holes #3159 applied to SCIM: one org's IdP cannot redirect a cross-org user's login identity (and, with a password reset, take the account over).active,externalIdand the display name stay org-scoped and apply as before (the create path already renames only a member of this org; the display name is not an authentication factor, so it deliberately stays under the existing rule).Trusted-headers session contract
Reuse is bound to the browser's own verified cookie: same user + live → refreshed and handed back; another user's session → deleted (account switch) and a fresh session minted; no cookie or a cookie that fails verification → a fresh session. The "any session row of this user" fallback is gone: it silently shared one session across devices (sign out on one killed both).
Tests
Colocated vitest (
--project server): 57 tests acrossdomains/sso/service.test.ts,domains/sso/trusted-headers.test.ts,domains/scim/service.test.ts(new),domains/scim/routes.test.ts(new). Run against base source, 26 of the 57 fail — the regression tests for every finding above (admin membership preserved / sync-created team reaped, SSO + proxy grace anchor, owner PATCH refused, 409 collision, cross-org identity refused, foreign group member refused ×3, bare-token cookie lookup, unverified cookie ignored, no cross-device adoption, error mapping); on the branch 57/57 pass.Integration probes (
backend/integration-check.ts), onerecordeach: SSO team sync leaves admin/excluded/SCIM teams intact while reapingOps(provenance rows asserted); SSO sign-in anchors the grace clock once and a backdated anchor blocks the SSO session on org routes (403two_factor_enrollment_required) with enrolment reachable; SCIM guards (owneractive:false→ 403mutability, role staysowner; foreign group member → 400invalidValue, no team/teamMember rows; userName collision → 409; shared account → 403 with email unchanged; sole-org rename → 200, normalized); trusted-headers (second device gets its own session, account switch kills the other user's session).Verification
bunx tsc --noEmit(platform): clean.bunx oxlint --type-aware(platform): clean.bunx vitest --run --project server backend/domains/sso backend/domains/scim: 57/57 (base: 31/57 — 26 red).bun run --filter @tale/docs test: green (docs pages edited in en/de/fr).bun run --filter @tale/ui test(i18n suite): 1168/1168 tests pass; the single failing filesrc/app-shell.test.tsxis environmental to the worktree's symlinkednode_modules(viteDenied ID …@fontsource/inter…woff2outside the worktree root), unrelated.backend:integrationon a throwaway tale-db + MinIO, fresh per run,SANDBOX_LLM_GATEWAY_ADMIN_PASSWORDset: branch 379/379, basefedc8cc15375/375. (One earlier branch attempt died incheckDocumentWriteGuardson a Postgres40001serialization failure in the audit chain head during concurrent uploads — a documents lane far ahead of these probes, the known flake; the rerun on a fresh stack was clean.)Cross-class discoveries (not fixed here)
organizations/service.ts) does not cleanapp.sso_connectionsorapp.sso_provisioning_linksfor the deleted org — a dead org's SCIM token hash stays resolvable. Belongs to the org-lifecycle lane; this PR only adds its own two provenance tables to that cascade.handleSsoLoginskips the team sync entirely when the IdP returns no groups (args.groups?.length), so a user removed from ALL groups keeps every synced membership until they hold at least one group again. Kept as is on purpose — the guard protects against a failed group fetch — but with provenance scoping an explicit empty claim could now safely reconcile; worth a deliberate decision.