fix(platform): harden SSO trust boundaries (binding, replay, origins) - #3131
Merged
Conversation
/me/memberOf and /me/appRoleAssignments page at 100 entries; reading only page 1 silently truncated the group list, and with autoProvisionTeam the team sync then PRUNED every membership beyond the first page on each login. Follow @odata.nextLink to exhaustion, bounded at 50 pages — past the cap (or on any non-OK page) the read throws, landing in the callers' existing failed-fetch path: team sync skipped, memberships preserved. A nextLink leaving the Graph origin is refused so the bearer token never follows a foreign link.
Behind the reverse proxy req.url carries the internal upstream origin (http even when the deployment terminates TLS). The SAML ACS handler, /api/sso/set-session and the trusted-headers door derived the login finish from it, so on standard https deployments they set the unprefixed better-auth.session_token cookie while Better Auth (useSecureCookies from SITE_URL) reads __Secure-better-auth.session_token — the user authenticated, then landed logged out; redirects pointed at the internal host. Prefer SITE_URL, falling back to the request origin only when it is unset — the idiom the OIDC authorize/callback handlers always used, now shared as publicOrigin().
The route passed process.env.TRUSTED_HEADERS_INTERNAL_SECRET as the caller-supplied secret, so trustedHeadersAuthenticate compared the env var against itself — the guard could never fail, and anyone reaching the endpoint minted a session as whoever Remote-Email named. The stock proxy chain forwards /api/trusted-headers/* verbatim, so topology was the only defense. Now the caller value comes from the request header the authenticating proxy injects (Remote-Internal-Secret, renamable via TRUSTED_SECRET_HEADER) and is compared constant-time against the env secret. Fail closed: TRUSTED_HEADERS_ENABLED without the secret refuses with a config error instead of running open. Breaking for deployments that enabled the mode without a secret — documented in the self-hosted authentication guide and environment reference (en/de/fr).
…ache buildSaml never set validateInResponseTo (node-saml defaults to never) and the AuthnRequest ID from getAuthorizeUrlAsync was discarded, so a captured SAMLResponse validated repeatedly until NotOnOrAfter closed — each replay minting a fresh session. Wire ValidateInResponseTo.ifPresent with a CacheProvider backed by the new app.saml_request_ids table (0059): issued request IDs are stored where every instance can see them, a response's InResponseTo must match one, and node-saml consumes the ID on success — one-time use, atomic DELETE..RETURNING. ifPresent (not always) keeps IdP-initiated posts working; their replay window stays bounded by the assertion's NotBefore/NotOnOrAfter, which node-saml already enforces alongside the audience restriction. Rows expire after node-saml's 8h request window, pruned lazily on the next save.
findOrCreateSsoUser matched the asserted email across ALL orgs, silently auto-joined the user into the connection's org, and minted a session as them. Org admins fully self-serve their IdP config, so any org could point its connection at an IdP it controls, assert a victim's email, and walk away with a session as that user — cross-org account takeover. The contract now: an org's IdP signs in users the org already has (a member row — invited and accepted, SCIM-provisioned, or admin-added) and JIT-creates users new to the deployment, org-bound. An existing user with no membership in the connection's org is refused inside the same transaction, before any write — no account link, no auto-join, no session — with the actionable sso.errors.notOrgMember error on the login page (en/de/fr) and an sso_login_failed audit row on both the OIDC callback (which never audited business refusals) and SAML ACS lanes. Session minting keeps binding activeOrganizationId to the connection's org.
…ct TS Explicit Response/unknown annotations break the TS7022 inference cycle in the pagination loop (nextLink feeds the next fetch), the adapter's optional getGroups/getAppRoles are exercised through throwing wrappers instead of unbound destructured references, and the Hono request helper is async so its Response | Promise<Response> return normalizes.
The parallel fix branch fix/tasks-org-scoping (PR #3130, opened first) already claims 0059 off the same main; filename order IS apply order and the number must never be reused, so this branch takes the next slot. Nothing has shipped under the old name.
larryro
marked this pull request as ready for review
September 2, 2026 11:19
This was referenced Sep 3, 2026
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
1 participant
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.
Fixes the SSO trust-boundary class from the deep review: org-scoped identity binding, the inert trusted-headers internal-secret guard, SAML assertion replay, proxy-origin cookie/redirect correctness, and Entra group-sync truncation.
The SSO org-binding contract (finding 1, critical)
findOrCreateSsoUsermatched the asserted email across all orgs, silently auto-joined the user into the connection's org, and minted a user-scoped session. Org admins fully self-serve their IdP config (PUT /api/app/sso/config/*, orgSettings gate), so any org could point its connection at an IdP it controls, assert a victim's email, and hold a session as that user — cross-org account takeover.The contract now enforced (inside the same transaction, before any write):
autoProvisionRole(unchanged).sso.errors.notOrgMembermessage (en/de/fr), and ansso_login_failedaudit row is written on both lanes (the OIDC callback previously audited only thrown errors, never business refusals — it now mirrors the ACS posture).Membership therefore comes only from flows the user or the org's own directory controls: invite acceptance (better-auth core requires the invitee's authenticated session + email match before the member row exists), SCIM provisioning (org-resolved from the per-org token hash; SCIM already refuses cross-tenant email collisions with 409), the org-admin add-member surface (audited), or SSO JIT creation of a genuinely new user. Users can still legitimately span orgs — each org that wants an existing user must bring them in through one of those flows first.
Session minting keeps binding
activeOrganizationIdto the connection's org (asserted by tests now). Better Auth'sset-activere-checks membership on every switch.Per-finding fixes
domains/sso/service.ts(above)domains/sso/service.test.ts(refusal writes nothing; member sign-in; JIT create; session org binding) + integration probe: existing outside-org user refused, no auto-join, auditedTRUSTED_HEADERS_INTERNAL_SECRETcompared against itself (high, ×3)Remote-Internal-Secret, renamable viaTRUSTED_SECRET_HEADER) against the env secret, constant-time (timingSafeEqual). Fail closed:TRUSTED_HEADERS_ENABLEDwithout the secret refuses with a config error. The stock Caddy chain forwards/api/trusted-headers/*verbatim and injects nothing — the legit path is the operator's authenticating proxy injecting the header, now documented (en/de/fr: self-hosted authentication guide + environment reference)domains/sso/trusted-headers.test.ts(9: service guard + route door, wrong/missing/unset refused, right secret passes, custom header name) + integration probesValidateInResponseTo.ifPresent+ a PG-backedCacheProviderover newapp.saml_request_ids(migration0060): issued AuthnRequest IDs are shared across instances, a response's InResponseTo must match one, node-saml consumes it on success (one-time use; atomicDELETE..RETURNING).ifPresentkeeps IdP-initiated posts supported — their replay window stays bounded byNotBefore/NotOnOrAfter+ audience, which node-saml enforces (documented inbuildSaml)saml/validate_assertion.test.ts(real node-saml + real signed assertions: replay refused, forged id refused, one-time consumption, IdP-initiated still accepted, redirect stores the id) +saml-request-cache.test.ts(provider contract) + integration probes (row lifecycle, replay + forged refused over real PG)publicOrigin()(SITE_URL, falling back to the request origin — the OIDC handlers' existing inline idiom, now shared) used by the SAML ACS,/api/sso/set-session, and the trusted-headers door, which had the same defect: behind TLS termination they set the unprefixed cookie while Better Auth (useSecureCookiesfrom SITE_URL) reads__Secure-…, and redirects targeted the internal upstreamlogin/public_origin.test.ts(4: prefers SITE_URL, trailing-slash normalization, http stays http, fallback)getGroups/getAppRolesfollow@odata.nextLinkto exhaustion, capped at 50 pages (~5000 entries); past the cap (or any non-OK page) they throw, which lands in the callers' existing failed-fetch path — team sync skipped, memberships preserved — instead of a silent partial list thatsyncTeamsFromGroupNameswould prune against. A nextLink leaving the Graph origin is refused so the bearer token never follows a foreign linkentra_id/adapter.test.ts(7 new: two-page union, single page, cap throws, off-origin refused, non-OK propagates, app-role union, failed fetch degrades to[])Refuted / narrowed
createSsoUserSessionsetsactiveOrganizationId); now asserted by tests and documented.getGroupsreads userinfo claims, no pagination lane exists there; finding 5 is Entra-only.Breaking / operator-visible changes
TRUSTED_HEADERS_ENABLED=truewithoutTRUSTED_HEADERS_INTERNAL_SECRETrefuses logins with a config error (and a precise operator log line) after upgrade — previously the mode ran with no request authentication at all, which is the vulnerability. Docs updated (en/de/fr).Cross-class discoveries (follow-up candidates)
set-activeinto any other org that user belongs to (Better Auth verifies membership, but the rogue-IdP scenario for shared members remains). Restricting SSO-minted sessions to their connection's org — or requiring fresh auth on switch — is an org-activation-model decision worth a dedicated issue.POST /api/app/members(admin add-existing-user) is consent-free — an org admin who learns a victim's internal userId can add them as a member without an invitation acceptance, which weakens the membership gate (it is audited and requires the non-guessable userId, so materially harder than the email-only vector fixed here).accountrow is keyed (userId, providerId), so a user signing into two orgs that both use e.g.entra-idhasaccountId/tokens overwritten by whichever org they logged into last. No session impact, but Graph-token consumers could read the other org's token. Pre-existing; out of this class.!result.successpath; worth remembering the pattern for other reused 0.4 handlers.migrations:check(named in the repo contract) does not exist as a script in this repo state — the migration is proven bybackend:integrationper the create-migration skill.Verification
bun run --filter @tale/platform typecheck— green.bun run --filter @tale/platform lint— green.bun run --filter @tale/docs test— 194/194 green (locale-tree/outline over the en/de/fr doc edits).bun run --filter @tale/ui test— all 1168 tests green incl. the de/fr voice/terminology checks; one suite (app-shell.test.tsx) errors loading a font through the worktree's node_modules symlink (Vite fs-allow, unrelated to this diff — no packages/ui file is touched).bun run --filter @tale/platform backend:integrationagainst a throwaway Postgres + MinIO — 281/281 checks passed (boot migrations incl.0060applied twice concurrently, plus the new probes:SAML: InResponseTo one-time use,SAML: an existing user outside the org is refused,trusted-headers authsecret gate).