refactor(auth): universal resolveUserOrgMembership helper for org auth-check pattern#3448
Conversation
…h-check pattern Replaces 14 copy-pasted call sites across organizations.ts and member-profiles.ts that did the WorkOS-membership-then-resolveUserRole auth check inline. Two problems with the old pattern: - Every admin-only org endpoint 403'd in dev mode because WorkOS doesn't know about DEV_USERS. The dev-mode bypass that solved this lived in exactly one place (GET /api/me/member-profile), and most other routes didn't have it. - 14 subtly different versions of the same auth check, each with its own error message and role logic. The helper does it once. In dev mode it reads from local organization_memberships (seeded by dev-setup at boot). In prod it defers to WorkOS as source of truth. Verified end-to-end: every admin endpoint the team page hits now responds 200 to a dev-mode admin user where most previously returned 403. The single remaining 404 in dev (api/billing) is unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Security review found a HIGH ship-blocker on PR #3448: - Add a hard prod-boot guard that crashes startup if DEV_USER_EMAIL + DEV_USER_ID are set in a production-shaped environment (NODE_ENV=production OR FLY_APP_NAME present). Override via ALLOW_DEV_MODE_IN_PROD=true for the rare one-off. Pre-PR the dev-mode bypass affected one read endpoint; post-PR it widens to 23 mutating routes (delete org, ban member, transfer ownership, etc), so a stray secret-flip would now grant owner-level takeover instead of read access. Code review found 4 more sites that match the auth-check pattern but weren't refactored in the original commit: - POST /api/organizations/:orgId/members (line 2177) - POST /api/organizations/:orgId/invitations (line 2360) - DELETE /api/organizations/:orgId/invitations/:invitationId (line 2469) - POST /api/organizations/:orgId/invitations/:invitationId/resend (line 2536) - POST /api/organizations/:orgId/members/by-email (line 3092 — the hybrid AAO-admin / static-API-key / org-role path; refactored carefully to preserve the conditional-skip for static admin keys) - PATCH/DELETE /api/organizations/:orgId/members/:membershipId (line 3291) All 6 now route through resolveUserOrgMembership. billing-public.ts and certification.ts have hand-rolled dev-mode bypasses (isDevUserInvoice, isOrgMember). Filed as separate followup — different patterns that need their own analysis. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed round-2 fixups addressing the security + code reviews: Security HIGH (resolved): Prod-boot guard added in Code FBM (resolved): Refactored 6 more auth-check sites I missed in the original commit — the invitations crud, the resend endpoint, the members PATCH/DELETE, and the member-add path. The last one (members/by-email) needed a careful pattern-preserve because it conditionally skips the WorkOS lookup for the static admin API key; new code maintains that skip while routing through the helper for non-static-key callers. Acknowledged but deferred (filing follow-up issues):
All other items ( Tests: 123 integration tests pass on the impacted suite. |
|
Round-2 fixups noted — prod-boot guard, 6 additional auth-check refactors, and the deferred items with follow-up issue plan all look well-reasoned. The (Note for infra: this PR comment reached the triage routine rather than the auto-fix feature — filter gap to investigate.) Generated by Claude Code |
…ng link (#3450) * refactor(auth): universal resolveUserOrgMembership helper for org auth-check pattern Replaces 14 copy-pasted call sites across organizations.ts and member-profiles.ts that did the WorkOS-membership-then-resolveUserRole auth check inline. Two problems with the old pattern: - Every admin-only org endpoint 403'd in dev mode because WorkOS doesn't know about DEV_USERS. The dev-mode bypass that solved this lived in exactly one place (GET /api/me/member-profile), and most other routes didn't have it. - 14 subtly different versions of the same auth check, each with its own error message and role logic. The helper does it once. In dev mode it reads from local organization_memberships (seeded by dev-setup at boot). In prod it defers to WorkOS as source of truth. Verified end-to-end: every admin endpoint the team page hits now responds 200 to a dev-mode admin user where most previously returned 403. The single remaining 404 in dev (api/billing) is unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(team): show brand-registry hierarchy classification + report-wrong link PR #3430's subsidiary toggle only showed children of the org. Two things owners actually need: - Parent (if classified as a subsidiary of another brand). Without this, an analyticsiq.com-style org can't tell they're classified under Alliant. - Classification confidence + last-validated date — context owners need to judge whether the data is trustworthy enough to flip the auto-provision toggle. GET /api/organizations/:orgId/domains now returns hierarchy_classification with self + parent. team.html renders a "Brand registry classification" section above the toggles showing parent → you → children, each with a "Report wrong" mailto link that routes corrections into the existing support flow without new infra. Verified end-to-end via Playwright in dev mode (depends on the universal dev-mode bypass from #3448): seeded a brand row + subsidiary, the hierarchy diagram renders correctly, "Report wrong" links work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(auth): prod-boot guard against dev mode + 4 missed auth-check sites Security review found a HIGH ship-blocker on PR #3448: - Add a hard prod-boot guard that crashes startup if DEV_USER_EMAIL + DEV_USER_ID are set in a production-shaped environment (NODE_ENV=production OR FLY_APP_NAME present). Override via ALLOW_DEV_MODE_IN_PROD=true for the rare one-off. Pre-PR the dev-mode bypass affected one read endpoint; post-PR it widens to 23 mutating routes (delete org, ban member, transfer ownership, etc), so a stray secret-flip would now grant owner-level takeover instead of read access. Code review found 4 more sites that match the auth-check pattern but weren't refactored in the original commit: - POST /api/organizations/:orgId/members (line 2177) - POST /api/organizations/:orgId/invitations (line 2360) - DELETE /api/organizations/:orgId/invitations/:invitationId (line 2469) - POST /api/organizations/:orgId/invitations/:invitationId/resend (line 2536) - POST /api/organizations/:orgId/members/by-email (line 3092 — the hybrid AAO-admin / static-API-key / org-role path; refactored carefully to preserve the conditional-skip for static admin keys) - PATCH/DELETE /api/organizations/:orgId/members/:membershipId (line 3291) All 6 now route through resolveUserOrgMembership. billing-public.ts and certification.ts have hand-rolled dev-mode bypasses (isDevUserInvoice, isOrgMember). Filed as separate followup — different patterns that need their own analysis. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(team): tenant-scoped ORDER BY, self-loop guard, source field, brand_json visual distinction Code-review FBM and product feedback on PR #3450: - SQL bug (code review): the ORDER BY scalar subquery for is_primary did not filter by workos_organization_id. If the same domain is verified across multiple tenants, the wrong org's primary flag could win the LIMIT 1. Added the tenant filter. - Self-loop guard (code review): the LEFT JOIN brands parent ON parent.domain = db.house_domain didn't guard against db.house_domain == db.domain. A malformed registry row pointing at itself would render "you are a child of yourself". Added parent.domain != db.domain to the join. - Dropped data.org_name from the mailto body — the /domains endpoint doesn't return it, so the line always rendered as "Org: " blank. - New `source` field on hierarchy_classification.self / .parent / inferred_subsidiaries (product). brand_json self-asserted gets a green "Asserted" badge; LLM-inferred gets a neutral "Inferred" badge. - For brand_json self-asserted parent relationships, render a small guidance hint pointing the admin at their own brand.json instead of the mailto support flow — fix is one PR away from where they look, not three weeks away after a triage review. - Confidence badge now suppressed for brand_json (the "confidence" frame is wrong for a self-assertion). Tests: 3 new cases (parent-row-doesn't-exist via LEFT JOIN miss, self-loop guard, source field surfacing for brand_json). 12/12 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… + isolation doc (#3471) * fix(auth): sign dev-session cookie + audit-log dev-bypass attribution + isolation doc Three security-medium items deferred from PR #3448 review: - Sign the dev-session cookie. Was the literal user key string ("admin" / "member"); anyone who could write a cookie on the domain (XSS, sibling subdomain) could pick a privileged dev user. Now HMAC-signed with a per-process secret generated at boot — invalid signatures fail verification on read. Format: ${userKey}.${base64url-hmac}. Timing- safe comparison. Cookies don't survive a server restart, which is the desired property: a cookie minted on someone else's box won't work on yours. - Audit-log dev-bypass attribution. resolveUserOrgMembership now returns via_dev_bypass: boolean. Routes that write registry_audit_log tag dev-bypass writes with details.auth_method = 'dev-bypass' so post- incident triage can distinguish them from real-user writes (synthetic user_dev_* IDs don't resolve in WorkOS). Applied to org rename + org settings PATCH; pattern documented for new audit-log writers. - New ops/dev-mode-isolation.md doc — operational invariants the bypass assumes (no shared dev/prod DB, no shared .env.local, prod-boot guard verification on every deploy). Incident response steps if dev mode ever activates in prod. Tests: 6 unit tests for cookie signing (round-trip, legacy format rejected, tampered sig rejected, forged user-key with valid sig from other key rejected, unknown user rejected, empty/missing rejected). 86/86 pass on the impacted integration suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): stub @workos-inc/node so dev-session-signing test doesn't need WorkOS env CI runs from repo root and picks up the root vitest.config.ts (no setupFiles), not server/vitest.config.ts. So the WORKOS_API_KEY mock that makes auth.ts loadable in local tests isn't set in CI. Stub the WorkOS module directly so the import doesn't depend on env-var ordering. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes the dev-mode-403 gap that bit me when verifying #3430 in dev mode: every admin-only org endpoint was 403'ing because WorkOS doesn't know about
DEV_USERS, and the dev-mode bypass that solved this lived in exactly one place (GET /api/me/member-profile). Each route would need its own copy or stay broken in dev.This consolidates the pattern.
What
New helper
resolveUserOrgMembership(workos, userId, orgId)inserver/src/utils/. Returns{ role, status } | null. In dev mode it reads from localorganization_memberships(seeded bydev-setup.tsat boot). In prod it defers to WorkOS as source of truth.Refactors 14 call sites across
organizations.tsandmember-profiles.tsthat did the inline WorkOS-membership-then-resolveUserRolepattern. Each becomes a one-liner:Verification
/api/organizations/:orgId/{domains, roles, seat-requests, ...}all returned 403. After, every endpoint responds 200. The single remaining 404 (/api/billing) is unrelated.Out of scope
listOrganizationMembershipsfor non-auth-check purposes (listing all members of an org, listing all of a user's orgs) are unchanged. The helper only replaces theuserId+orgIdfilter pattern that's the canonical "is this user a member of this org?" auth check.resolvePrimaryOrganizationfrom PR fix(membership): centralize primary_organization_id reads, add invariant #3375.🤖 Generated with Claude Code