Skip to content

Commit b237aa4

Browse files
committed
fix(auth): clamp sessions created before their member row exists
Two defects found by an independent audit of the session lifecycle. 1. First SSO sign-in into a policy-bearing org produced an UNCLAMPED 30-day session. `@better-auth/sso` creates the session before it writes `member` (`handleOAuthUserInfo` then `assignOrganizationFromProvider`), and it writes through the adapter directly, so no member hook fires. The create hook saw no membership and applied Better Auth's default expiry — and because a 30-day expiry reads as "not due for refresh", the sliding refresh would not re-clamp it for another day. Against an 8-hour policy that is a full day of bypass, on every first SSO login, deterministically. Closed with an `after` hook that re-applies the policy once membership is visible. It runs only for sessions created without an org — the only ones that can be affected — and is best-effort so it can never break sign-in. 2. The create hook's catch branched on a local that was stale by the time it was read. When the direct membership read failed, `clampExpiryForSession` could still resolve the org from cache and then throw on the policy read; the catch saw the stale `undefined` and took the permissive branch — failing OPEN on a path whose comment promised fail-closed. Membership is now resolved once, up front, through the shared cached helper, and both the clamp and the catch use that single value. Also: normalize the update hook's fallback expiry like its happy path, and correct the `getSessionPolicy` cost note — an org with bounds stored pays the entitlement check too, so roughly four reads, not one.
1 parent 30d5496 commit b237aa4

3 files changed

Lines changed: 88 additions & 38 deletions

File tree

apps/sim/lib/auth/auth.ts

Lines changed: 76 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,12 @@ import {
3434
import { getAccessControlConfig, isEmailBlockedByAccessControl } from '@/lib/auth/access-control'
3535
import { createAnonymousSession, ensureAnonymousUserExists } from '@/lib/auth/anonymous'
3636
import { getRequestedSignInProviderId, isSignInProviderAllowed } from '@/lib/auth/constants'
37-
import { getSessionCookieCacheVersion } from '@/lib/auth/security-policy'
38-
import { clampExpiryForSession } from '@/lib/auth/session-policy'
37+
import {
38+
getMemberOrganizationId,
39+
getSessionCookieCacheVersion,
40+
invalidateMembershipCache,
41+
} from '@/lib/auth/security-policy'
42+
import { applySessionPolicyToNewMember, clampExpiryForSession } from '@/lib/auth/session-policy'
3943
import { guardSubscriptionPlanWrites } from '@/lib/auth/stripe-adapter-guard'
4044
import { sendPlanWelcomeEmail } from '@/lib/billing'
4145
import {
@@ -661,28 +665,29 @@ export const auth = betterAuth({
661665
}
662666
}
663667

664-
// Resolved separately from the clamp below so the two failures stay
665-
// distinguishable: `null` means "confirmed not in an org", while
666-
// `undefined` means "could not tell". The clamp retries through the
667-
// membership cache, and the catch below branches on which one it got.
668-
let membershipOrgId: string | null | undefined
668+
// Membership is resolved ONCE, here, and the clamp is handed the
669+
// result. Resolving it in two places let the catch below branch on a
670+
// stale local while the clamp had actually resolved the org from
671+
// cache — failing open on a path documented as failing closed.
672+
let membershipOrgId: string | null
669673
try {
670-
const members = await db
671-
.select({ organizationId: schema.member.organizationId })
672-
.from(schema.member)
673-
.where(eq(schema.member.userId, session.userId))
674-
.limit(1)
675-
membershipOrgId = members[0]?.organizationId ?? null
674+
membershipOrgId = await getMemberOrganizationId(session.userId)
676675
logger.info(
677676
membershipOrgId ? 'Found organization for user' : 'No organizations found for user',
678677
{ userId: session.userId, organizationId: membershipOrgId ?? undefined }
679678
)
680679
} catch (error) {
681-
logger.error('Could not resolve organization for new session', {
680+
// The governing org is genuinely unknown, so a member is
681+
// indistinguishable from the majority of users who belong to no org.
682+
// Failing closed would take authentication down for everyone to
683+
// protect a policy most of them do not have. Allow, unclamped: the
684+
// after-hook above re-applies the policy once membership resolves,
685+
// and the update hook clamps retroactively from `createdAt`.
686+
logger.error('Could not resolve organization for new session; session not clamped', {
682687
error,
683688
userId: session.userId,
684689
})
685-
membershipOrgId = undefined
690+
return { data: session }
686691
}
687692

688693
try {
@@ -698,15 +703,15 @@ export const auth = betterAuth({
698703
},
699704
}
700705
} catch (error) {
701-
// Two different failures land here, and they get opposite answers.
702-
//
703-
// Org KNOWN: membership resolved, so this user is definitely
704-
// governed by an org — only the policy read failed. Fail closed.
705-
// The blast radius is org members during a window where the
706-
// `organization` read fails but the `member` read just succeeded,
707-
// which is narrow, loud, and self-clearing. Minting a full-length
708-
// session for a member whose policy we could not read is the one
709-
// outcome this feature exists to prevent.
706+
// Membership already resolved above, so reaching here means only
707+
// the POLICY read failed. For a member that is fail-closed: this
708+
// user is definitely governed by an org, so the "would break
709+
// sign-in for non-members too" argument does not apply. The blast
710+
// radius is org members during a window where the `organization`
711+
// read fails but the `member` read just succeeded — narrow, loud,
712+
// and self-clearing. Minting a full-length session for a member
713+
// whose policy we could not read is the one outcome this feature
714+
// exists to prevent.
710715
if (membershipOrgId) {
711716
logger.error('Refusing session: org session policy could not be resolved', {
712717
error,
@@ -718,17 +723,9 @@ export const auth = betterAuth({
718723
})
719724
}
720725

721-
// Org UNKNOWN: both the direct membership read and the cached
722-
// fallback failed, so we cannot tell a governed member from the
723-
// overwhelming majority of users who belong to no org at all.
724-
// Failing closed here would turn a `member` read failure into a
725-
// total authentication outage for everyone, to protect a policy
726-
// that most of them do not have. Allow, and let it self-correct:
727-
// the session takes the DB path at its next cookie-cache expiry
728-
// and the update hook clamps it retroactively (the max-lifetime
729-
// bound is measured from `createdAt`, so an over-long session
730-
// expires immediately on that refresh). Any admin action bumps the
731-
// security-policy version and closes the window at once.
726+
// Confirmed non-member: the clamp short-circuits before any policy
727+
// read for a null org, so this is defence in depth rather than a
728+
// live path. A user with no org has no policy to violate, so allow.
732729
logger.error('Error clamping new session to org policy; session not clamped', {
733730
error,
734731
userId: session.userId,
@@ -764,7 +761,9 @@ export const auth = betterAuth({
764761
error,
765762
userId: current.userId,
766763
})
767-
return { data: { ...data, expiresAt: current.expiresAt } }
764+
// Normalized like the happy path above: Better Auth context values
765+
// can cross a serialization boundary, and the column is a timestamp.
766+
return { data: { ...data, expiresAt: new Date(current.expiresAt) } }
768767
}
769768
},
770769
},
@@ -1066,6 +1065,47 @@ export const auth = betterAuth({
10661065

10671066
return
10681067
}),
1068+
/**
1069+
* Re-applies the org session policy to a session that was created before
1070+
* its `member` row existed.
1071+
*
1072+
* SSO JIT provisioning does exactly that: the callback creates the session
1073+
* first and only then writes `member` straight through the adapter
1074+
* (`@better-auth/sso` calls `handleOAuthUserInfo` before
1075+
* `assignOrganizationFromProvider`, and bypasses the organization plugin's
1076+
* `addMember`, so no member hook fires). The session-create hook therefore
1077+
* sees no membership and mints Better Auth's full 30-day expiry — and
1078+
* because a 30-day expiry reads as "not due for refresh", the sliding
1079+
* refresh would not re-clamp it for another day. On a first SSO sign-in
1080+
* into an org with session bounds, that is a real policy bypass.
1081+
*
1082+
* Runs only for sessions created WITHOUT an org (the sole case that can be
1083+
* affected) and is best-effort — a failure here must never break sign-in.
1084+
*/
1085+
after: createAuthMiddleware(async (ctx) => {
1086+
const created = ctx.context.newSession
1087+
if (!created?.session || created.session.activeOrganizationId) return
1088+
if ((created.session as { impersonatedBy?: string | null }).impersonatedBy) return
1089+
1090+
try {
1091+
// Drop the negative membership entry the create hook just cached, so
1092+
// the row written moments ago by the provisioning path is visible.
1093+
invalidateMembershipCache(created.user.id)
1094+
const organizationId = await getMemberOrganizationId(created.user.id)
1095+
if (!organizationId) return
1096+
1097+
logger.info('Applying org session policy to a session created pre-membership', {
1098+
userId: created.user.id,
1099+
organizationId,
1100+
})
1101+
await applySessionPolicyToNewMember(created.user.id, organizationId)
1102+
} catch (error) {
1103+
logger.error('Failed to apply org session policy after session creation', {
1104+
error,
1105+
userId: created.user.id,
1106+
})
1107+
}
1108+
}),
10691109
},
10701110
plugins: [
10711111
...(env.TURNSTILE_SECRET_KEY

apps/sim/lib/auth/security-policy.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,10 @@ export async function getSecurityPolicyVersion(
105105
}
106106

107107
/**
108-
* Publishes the authoritative version a write just committed.
108+
* Seeds THIS process with the version a write just committed. Other instances
109+
* still pick it up on their own next read, so org-wide propagation remains
110+
* bounded by {@link SECURITY_POLICY_VERSION_CACHE_TTL_MS} — this only removes
111+
* the calling instance's own re-read.
109112
*
110113
* Deliberately a SET rather than a delete. Deleting would leave no floor for the
111114
* monotonic guard above, so a read that started before the bump could land

apps/sim/lib/auth/session-policy.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,14 @@ async function isEntitledToEnforce(organizationId: string): Promise<boolean> {
5353
* its stored rules at enforcement time (`lib/logs/execution/logger.ts`,
5454
* `lib/workflows/executor/execution-core.ts`). This is not a hot path: it runs
5555
* on sign-in and on a sliding session refresh, which the 24h cookie cache
56-
* limits to roughly once per user per day — one indexed point lookup each.
56+
* limits to roughly once per user per day.
57+
*
58+
* Cost is one indexed lookup for an org with no bounds stored — the common
59+
* case. An org that DOES store bounds additionally pays the entitlement check
60+
* (owner lookup + block status + subscription), so about four reads. That is
61+
* affordable per sign-in, but it lands all at once for every member when a
62+
* policy save or org-wide revocation invalidates the whole org's cookie caches
63+
* together; watch it if enterprise orgs grow much larger.
5764
*
5865
* A cache here is what made an earlier version of this code incorrect. Better
5966
* Auth rewrites `expiresAt` to `now + 30d` on every refresh, so a refresh that

0 commit comments

Comments
 (0)