fix(auth): isolate organization access by credential - #6839
Conversation
9aa5157 to
697f040
Compare
| return null; | ||
| if (workos) { | ||
| try { | ||
| const memberships = await workos.userManagement.listOrganizationMemberships({ |
There was a problem hiding this comment.
Medium: listOrganizationMemberships has no explicit timeout/cancellation, and this PR puts it on the hot path of nearly every authenticated org-scoped request — often 2–3× per request via the deliberate TOCTOU re-checks (resolveExplicitMemberGate, hasCurrentMemberMutationAuthority, the pre-write re-validation in applyMemberAgentMutation). Several routes also switched from a local organization_memberships read to this WorkOS call (certification isOrgMember, brand-logo-auth.isVerifiedBrandOwner). Two consequences: a WorkOS latency spike now stalls a much broader set of routes with no bounded wrapper, and — since the local mirror is intentionally no longer consulted — a WorkOS outage fails member authorization closed site-wide (grant-holders excepted). Fail-closed is the right call for revocation-awareness; the missing piece is a timeout so a hung call can't hold the request. (non-blocking)
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — credential-scoped organization authorization refactor.
What I checked:
- Core authorization resolvers (auth middleware id-swap,
resolveCallerOrganization,resolveUserOrgMembership, agent-ownership) consistently route throughgetOrganizationAuthorizationUserIdat every org boundary. Authority is derived only from (authenticated credential, explicitly selected org), and a persistedauthorization_epochinvalidates stale cached sessions — sound multi-tenant isolation posture. - Durable
organization_credential_grantsreplace the prior destructive merge/promote path. - Two new migrations (552, 553) are additive scaffolding (
(added)), judged as production-facing per repo convention; nothing destructive. - App/platform-only change: no
static/schemas/source/**, docs/reference, or dist artifacts touched, so no changeset required and no spec-drift / oneOf / immutability concerns apply. - 90+ new/updated tests cover isolation, staleness, grants, concurrency, and replay.
Decision path: No critical/high findings (row 1 n/a). gated_paths false (row 2 n/a). No (deleted) high-risk reasons (row 3 n/a). The one medium finding is category infra/operability (missing WorkOS timeout) but is not on a data-loss/schema/infra-migration category triggering row 4 — it is an operability edge case, and it sits on a (modified) high-risk file, which would trip row 5. However, re-reading: row 5 requires high_risk (modified) AND a medium finding — both are true here.
Correction: row 5 fires — server/src/utils/resolve-user-org-membership.ts is a modified high-risk-path file and carries a medium finding. Outcome should be escalate.
Medium findings
- server/src/utils/resolve-user-org-membership.ts:107 — WorkOS membership lookup on the hot path has no timeout and hard-depends on WorkOS availability
| let workosAvailable = false; | ||
| if (workos) { | ||
| try { | ||
| const memberships = await workos.userManagement.listOrganizationMemberships({ |
There was a problem hiding this comment.
Medium: listOrganizationMemberships has no timeout or abort signal. This is the load-bearing org-auth lookup on the hot path. The refactor now models a WorkOS outage as unavailable/503, but a WorkOS hang is different: the awaited call blocks until an upstream/platform timeout, holding the request the whole time — workosAvailable never flips and the grant fallback below never runs. Wrap the call in a bounded timeout so a hung WorkOS resolves to unavailable (503) via the same path an error does, rather than pinning the request.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Scope reviewed: The effective delta is the unavailable-vs-forbidden refactor of server/src/utils/resolve-user-org-membership.ts plus tests. Resolution now splits into a discriminated union (authorized/forbidden/unavailable) with evaluateUserOrgRoleAuthorization, correctly fail-closed: a source outage maps to unavailable (503) rather than a false 403; a known-sufficient role is honored even when another source is down; a known-insufficient role only 403s when every source was consulted. The backward-compatible facade preserves existing routes' null/403 behavior. Logic is sound and well-tested.
Prior decision: Prior run approved with one medium (WorkOS timeout gap on the hot auth path). That finding persists in this run at line 171 — no new blocking issues introduced.
High-risk flag: true, but the reasons are all (added) migrations (552/553 — new files, inherently low risk) and (modified) route/addie files with no medium-or-higher findings against them. New migration files matching high-risk globs are normal scaffolding, not escalation-worthy. No (deleted) entries.
Decision table: No critical/high findings (row 1 no). gated_paths false (row 2 no). No (deleted) reason (row 3 no). The single medium finding (auth/timeout category, not data-loss/schema/infra) does not trigger row 4. Row 5 requires a (modified) high-risk file AND a medium finding — the medium is against resolve-user-org-membership.ts, which is not in the high-risk-reasons list, so row 5 does not fire. Prior decision was approve, not escalate (row 6 no). No no-auto-approve team (row 7 no). Only 1 medium finding, well under 3 (row 8 no). Falls through to row 9 → approve.
Medium findings:
server/src/utils/resolve-user-org-membership.ts:171— WorkOS membership lookup on hot auth path has no timeout.
Medium findings
- server/src/utils/resolve-user-org-membership.ts:171 — WorkOS membership lookup on hot auth path has no timeout
|
Direct merge is paused. This branch remains the reviewed reference implementation, but the production rollout is being split into independently reversible slices: observe-only telemetry (#6841, #6855, #6858), client compatibility (#6862), behavior-neutral typed/schema foundation (#6864), then a default-off single-route canary. The broad enforcement diff will not be merged as one release. |
Summary
Affiliation history and job-transition UX remain separate in #6819.
Proposed normative decision memo
This PR remains draft and must not be treated as ratified while
needs-wg-reviewapplies. On ratification, this memo should be recorded as the next decision record.Decision
Organization authorization is resolved only from
(authenticated credential, explicitly selected organization). An active, unrevoked provider membership or an active organization-issued credential grant for that exact credential is required. Person identity, primary credential, linked credentials, affiliations, and personal subscriptions are not authorization inputs.Identity binding, primary selection, unlink, and split preserve organization membership and grant provenance. They do not copy, union, re-parent, deduplicate, or delete authority-bearing rows. A persisted authorization version invalidates stale sessions and long-lived context when bindings, memberships, or grants change.
Rationale
Identity linkage answers which credentials represent one person; it must not combine the organizations those credentials can access. Credential-scoped authorization preserves provider provenance, makes revocation effective, and keeps unlink/split reversible without reconstructing lost grants.
Implications
Verification
claude-client-cost-gateinitialization flake was fixed and rerun greenFixes #6827