feat(cli,trpc): organization override via header, no session mutation#3638
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRenames CLI organization commands, adds organization selection/switch in CLI, threads an optional organizationId through CLI config and API client headers, and refactors TRPC auth to resolve/validate an organization header setting Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (Login)
participant Config as Config Store
participant API as API Client
participant Server as Server
User->>CLI: superset auth login
CLI->>API: GET /user/myOrganizations
API-->>CLI: organizations list
alt multiple orgs
CLI->>User: prompt to select org
User->>CLI: choose org
else single org
CLI-->>CLI: auto-select org
end
CLI->>Config: write organizationId
CLI->>API: init client with organizationId
API->>Server: request with x-superset-organization-id header
Server-->>API: authorized response
API-->>CLI: login success
sequenceDiagram
participant Client as Client (CLI/UI)
participant TRPC as TRPC Server
participant Auth as Auth Middleware
participant OrgCheck as Org Validation
participant Procedure as Router Procedure
participant DB as Database
Client->>TRPC: request (may include x-superset-organization-id)
TRPC->>Auth: verify session
Auth-->>TRPC: session valid
TRPC->>OrgCheck: read header & check membership
OrgCheck->>DB: query members for user & header org
DB-->>OrgCheck: membership result
alt membership exists
OrgCheck-->>TRPC: set ctx.activeOrganizationId
TRPC->>Procedure: execute with ctx.activeOrganizationId
Procedure->>DB: org-scoped query/mutation
DB-->>Procedure: result
Procedure-->>Client: response
else no membership
OrgCheck-->>TRPC: throw FORBIDDEN
TRPC-->>Client: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR decouples the CLI's active organization from the web app's Better Auth session by introducing an Key changes:
Minor observations (all non-blocking):
Confidence Score: 5/5Safe to merge — all findings are non-blocking style suggestions with no correctness or security regressions. The core mechanism is sound: membership is validated server-side before any header override takes effect, the session-org fallback preserves backward compatibility for the web app, and the migration of call sites is purely mechanical. All four inline comments are P2 style/clarity issues that don't affect runtime correctness or security. No files require special attention;
|
| Filename | Overview |
|---|---|
| packages/trpc/src/trpc.ts | Core change: protectedProcedure now runs a second middleware that resolves activeOrganizationId from an HTTP header with DB membership validation, falling back to the session value. Logic is correct; minor: skipped re-verification when header org equals session org (intentional, undocumented). |
| packages/trpc/src/router/utils/active-org.ts | Signature of requireActiveOrgId and requireActiveOrgMembership changed from session to ctx; now reads ctx.activeOrganizationId. Results in a redundant DB membership check in the header-override path (middleware already verified), but is correct and provides defense-in-depth. |
| packages/cli/src/commands/auth/login/command.ts | Login now fetches all user orgs and presents an interactive selector for multi-org users; org ID is persisted to config. Non-TTY multi-org path silently falls back to session org. Redundant myOrganization call for single-org users. Empty catch {} loses the explanatory comment. |
| packages/cli/src/commands/organization/switch/command.ts | New command that validates org membership client-side (by filtering myOrganizations) and writes the chosen org UUID to config. Supports both ID and slug lookup. Correct and clean. |
| packages/cli/src/lib/api-client.ts | Conditionally adds x-superset-organization-id header when organizationId is provided in opts. Clean, minimal change. |
| packages/shared/src/constants.ts | Adds ORGANIZATION_HEADER = "x-superset-organization-id" constant shared between CLI and tRPC server to avoid string duplication. |
| packages/trpc/src/router/automation/automation.ts | Mechanical migration: all requireActiveOrgMembership(ctx.session) calls updated to requireActiveOrgMembership(ctx). No logic changes. |
| packages/trpc/src/router/billing/billing.ts | Migrates ctx.session.session.activeOrganizationId to ctx.activeOrganizationId in requireOwnerWithCustomer and invoices. Type annotation on requireOwnerWithCustomer simplified accordingly. |
Sequence Diagram
sequenceDiagram
participant CLI
participant Server as tRPC Server
participant DB
Note over CLI: config.organizationId set by login/switch
CLI->>Server: Request + Authorization: Bearer token + x-superset-organization-id: orgId
Server->>Server: middleware 1 - validate session
Server->>Server: middleware 2 - read header org ID
alt headerOrgId absent or same as session org
Server->>Server: activeOrganizationId = sessionOrgId
else headerOrgId differs from session org
Server->>DB: SELECT from members WHERE userId AND organizationId
DB-->>Server: membership row or null
alt no membership
Server-->>CLI: FORBIDDEN
else membership found
Server->>Server: activeOrganizationId = headerOrgId
end
end
Server->>Server: Router handler uses ctx.activeOrganizationId
Server-->>CLI: Response
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/cli/src/commands/auth/login/command.ts
Line: 74
Comment:
**Silent `catch {}` swallows org-selection errors**
The previous code had an inline `// Non-fatal` comment to explain why errors were acceptable here. Removing it makes it harder to reason about this intentional swallowing at a glance. More importantly, the try block now covers significantly more logic than before (two extra API calls + a `writeConfig`) — if `api.user.myOrganizations.query()` or the final `writeConfig` fails, the user is silently left without an `organizationId` in their config with no indication of what went wrong.
Consider restoring a minimal comment or logging a debug/warning so failure is diagnosable:
```suggestion
} catch (err) {
// Non-fatal: org selection is best-effort; auth token was already saved above.
// In debug mode, surface the error so it's diagnosable.
if (process.env.DEBUG) p.log.warn(`Org selection failed: ${err}`);
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/trpc/src/trpc.ts
Line: 59-73
Comment:
**Membership check skipped when header org matches session org**
When `headerOrgId === sessionOrgId`, the block is skipped entirely and `activeOrganizationId` is set directly from the session value with no re-verification. This is intentional and not a regression (the session was already validated by Better Auth at login time), but it means a user who was removed from an organization after their session was issued will continue to have `activeOrganizationId` set to that org for as long as their session lives — both before and after this PR.
This is an inherent limitation of trusting `session.activeOrganizationId` as a source of truth. Worth documenting with an inline comment so future readers understand the deliberate trade-off rather than wondering whether a membership check was accidentally omitted.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/cli/src/commands/auth/login/command.ts
Line: 45-46
Comment:
**Redundant `myOrganization` call for single-org users**
`sessionActive` is fetched unconditionally so it can be used as `initialValue` in the interactive selector. However, when `organizations.length === 1`, the `else if` branch overwrites `chosenId` with `organizations[0]?.id` regardless, making the `myOrganization` query a wasted round-trip for the most common case (single-org users). Consider fetching `sessionActive` lazily — only when the multi-org TTY branch is reached.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/trpc/src/router/utils/active-org.ts
Line: 28-35
Comment:
**Double membership verification when header override is used**
`requireActiveOrgMembership` calls `verifyOrgMembership`, which issues a DB query against the `members` table. When the request came through the `headerOrgId !== sessionOrgId` path in `protectedProcedure`, that same check was already performed. Routers that call this helper (e.g. `automation`, `task`, `v2-project`) will now fire two membership queries per request in the header-override case. This is defense-in-depth and not a correctness issue, but may be worth a comment since it looks accidental.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(cli,trpc): organization override vi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const chosen = organizations.find((o) => o.id === chosenId); | ||
| if (chosen) p.log.info(`Organization: ${chosen.name}`); | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
catch {} swallows org-selection errors
The previous code had an inline // Non-fatal comment to explain why errors were acceptable here. Removing it makes it harder to reason about this intentional swallowing at a glance. More importantly, the try block now covers significantly more logic than before (two extra API calls + a writeConfig) — if api.user.myOrganizations.query() or the final writeConfig fails, the user is silently left without an organizationId in their config with no indication of what went wrong.
Consider restoring a minimal comment or logging a debug/warning so failure is diagnosable:
Suggested change
| } catch {} | |
| } catch (err) { | |
| // Non-fatal: org selection is best-effort; auth token was already saved above. | |
| // In debug mode, surface the error so it's diagnosable. | |
| if (process.env.DEBUG) p.log.warn(`Org selection failed: ${err}`); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/commands/auth/login/command.ts
Line: 74
Comment:
**Silent `catch {}` swallows org-selection errors**
The previous code had an inline `// Non-fatal` comment to explain why errors were acceptable here. Removing it makes it harder to reason about this intentional swallowing at a glance. More importantly, the try block now covers significantly more logic than before (two extra API calls + a `writeConfig`) — if `api.user.myOrganizations.query()` or the final `writeConfig` fails, the user is silently left without an `organizationId` in their config with no indication of what went wrong.
Consider restoring a minimal comment or logging a debug/warning so failure is diagnosable:
```suggestion
} catch (err) {
// Non-fatal: org selection is best-effort; auth token was already saved above.
// In debug mode, surface the error so it's diagnosable.
if (process.env.DEBUG) p.log.warn(`Org selection failed: ${err}`);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+59
to
+73
| if (headerOrgId && headerOrgId !== sessionOrgId) { | ||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(members.userId, ctx.session.user.id), | ||
| eq(members.organizationId, headerOrgId), | ||
| ), | ||
| }); | ||
| if (!membership) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: `Not a member of organization ${headerOrgId}`, | ||
| }); | ||
| } | ||
| activeOrganizationId = headerOrgId; | ||
| } |
There was a problem hiding this comment.
When headerOrgId === sessionOrgId, the block is skipped entirely and activeOrganizationId is set directly from the session value with no re-verification. This is intentional and not a regression (the session was already validated by Better Auth at login time), but it means a user who was removed from an organization after their session was issued will continue to have activeOrganizationId set to that org for as long as their session lives — both before and after this PR.
This is an inherent limitation of trusting session.activeOrganizationId as a source of truth. Worth documenting with an inline comment so future readers understand the deliberate trade-off rather than wondering whether a membership check was accidentally omitted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/trpc.ts
Line: 59-73
Comment:
**Membership check skipped when header org matches session org**
When `headerOrgId === sessionOrgId`, the block is skipped entirely and `activeOrganizationId` is set directly from the session value with no re-verification. This is intentional and not a regression (the session was already validated by Better Auth at login time), but it means a user who was removed from an organization after their session was issued will continue to have `activeOrganizationId` set to that org for as long as their session lives — both before and after this PR.
This is an inherent limitation of trusting `session.activeOrganizationId` as a source of truth. Worth documenting with an inline comment so future readers understand the deliberate trade-off rather than wondering whether a membership check was accidentally omitted.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+45
to
+46
| const organizations = await api.user.myOrganizations.query(); | ||
| const sessionActive = await api.user.myOrganization.query(); |
There was a problem hiding this comment.
myOrganization call for single-org users
sessionActive is fetched unconditionally so it can be used as initialValue in the interactive selector. However, when organizations.length === 1, the else if branch overwrites chosenId with organizations[0]?.id regardless, making the myOrganization query a wasted round-trip for the most common case (single-org users). Consider fetching sessionActive lazily — only when the multi-org TTY branch is reached.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/commands/auth/login/command.ts
Line: 45-46
Comment:
**Redundant `myOrganization` call for single-org users**
`sessionActive` is fetched unconditionally so it can be used as `initialValue` in the interactive selector. However, when `organizations.length === 1`, the `else if` branch overwrites `chosenId` with `organizations[0]?.id` regardless, making the `myOrganization` query a wasted round-trip for the most common case (single-org users). Consider fetching `sessionActive` lazily — only when the multi-org TTY branch is reached.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
28
to
35
| export async function requireActiveOrgMembership( | ||
| session: Session, | ||
| ctx: ProtectedContext, | ||
| message?: string, | ||
| ) { | ||
| const organizationId = requireActiveOrgId(session, message); | ||
| await verifyOrgMembership(session.user.id, organizationId); | ||
| const organizationId = requireActiveOrgId(ctx, message); | ||
| await verifyOrgMembership(ctx.session.user.id, organizationId); | ||
| return organizationId; | ||
| } |
There was a problem hiding this comment.
requireActiveOrgMembership calls verifyOrgMembership, which issues a DB query against the members table. When the request came through the headerOrgId !== sessionOrgId path in protectedProcedure, that same check was already performed. Routers that call this helper (e.g. automation, task, v2-project) will now fire two membership queries per request in the header-override case. This is defense-in-depth and not a correctness issue, but may be worth a comment since it looks accidental.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/router/utils/active-org.ts
Line: 28-35
Comment:
**Double membership verification when header override is used**
`requireActiveOrgMembership` calls `verifyOrgMembership`, which issues a DB query against the `members` table. When the request came through the `headerOrgId !== sessionOrgId` path in `protectedProcedure`, that same check was already performed. Routers that call this helper (e.g. `automation`, `task`, `v2-project`) will now fire two membership queries per request in the header-override case. This is defense-in-depth and not a correctness issue, but may be worth a comment since it looks accidental.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/trpc/src/router/billing/billing.ts (1)
68-92:⚠️ Potential issue | 🟠 MajorRequire owner access before returning invoice URLs.
invoicesnow usesctx.activeOrganizationIddirectly and returns StripehostedInvoiceUrlvalues without the owner check used bydetailsandportal. Any org member could retrieve billing invoice links for the active org.🛡️ Proposed fix
invoices: protectedProcedure.query(async ({ ctx }) => { - const activeOrgId = ctx.activeOrganizationId; - if (!activeOrgId) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "No active organization", - }); - } - - const subscription = await db.query.subscriptions.findFirst({ - where: eq(subscriptions.referenceId, activeOrgId), - }); - - if (!subscription?.stripeCustomerId) { + const stripeCustomerId = await requireOwnerWithCustomer(ctx); + if (!stripeCustomerId) { return []; } const twelveMonthsAgo = subtractMonthsClamped(new Date(), 12); const stripeInvoices = await stripeClient.invoices.list({ - customer: subscription.stripeCustomerId, + customer: stripeCustomerId, limit: 100, status: "paid", created: { gte: Math.floor(twelveMonthsAgo.getTime() / 1000) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/billing/billing.ts` around lines 68 - 92, The invoices handler currently uses ctx.activeOrganizationId and returns Stripe hostedInvoiceUrl values without verifying the caller is an organization owner; add the same owner-access guard used by the details and portal endpoints: in the invoices protectedProcedure (before fetching subscriptions or returning invoice URLs) verify the current user is an owner of ctx.activeOrganizationId and throw a TRPCError with code "FORBIDDEN" if not; only include/return hostedInvoiceUrl (or full invoice links) when that owner check passes, otherwise return no URLs or an empty result consistent with the other endpoints.packages/cli/CLI_SPEC.md (1)
1109-1120:⚠️ Potential issue | 🟡 MinorDoc/impl mismatch: spec says
<nameOrId>but command matchesidorslug.The spec advertises
superset organization switch <nameOrId>and the "Switched to Acme Corp" example implies name-based selection, butpackages/cli/src/commands/organization/switch/command.tsonly matches onorganization.id === idOrSlug || organization.slug === idOrSlug. Passing a display name like"Acme Corp"will returnOrganization not found. Either update the spec to<idOrSlug>or extend the matcher to also compare againstname.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/CLI_SPEC.md` around lines 1109 - 1120, The docs and implementation disagree: the CLI spec says `switch <nameOrId>` but the code in `packages/cli/src/commands/organization/switch/command.ts` only matches `organization.id === idOrSlug || organization.slug === idOrSlug`; update the matcher to also accept display names by adding a comparison against `organization.name` (e.g., `organization.name === idOrSlug`), and normalize inputs (trim and compare case-insensitively) so passing "Acme Corp" will succeed; keep the existing id/slug checks and update error text unchanged.
🧹 Nitpick comments (2)
packages/trpc/src/router/chat/chat.ts (1)
56-63: Consider using the shared active-org guard.These three blocks now duplicate the same
ctx.activeOrganizationIdcheck. SincerequireActiveOrgId(ctx)already centralizes this behavior, using it here would reduce drift.♻️ Proposed cleanup
import { protectedProcedure } from "../../trpc"; +import { requireActiveOrgId } from "../utils/active-org"; import { uploadChatAttachment } from "./utils/upload-chat-attachment"; @@ - const organizationId = ctx.activeOrganizationId; - - if (!organizationId) { - throw new TRPCError({ - code: "FORBIDDEN", - message: "No active organization selected", - }); - } + const organizationId = requireActiveOrgId(ctx); @@ - const organizationId = ctx.activeOrganizationId; - - if (!organizationId) { - throw new TRPCError({ - code: "FORBIDDEN", - message: "No active organization selected", - }); - } + const organizationId = requireActiveOrgId(ctx); @@ - const organizationId = ctx.activeOrganizationId; - - if (!organizationId) { - throw new TRPCError({ - code: "FORBIDDEN", - message: "No active organization selected", - }); - } + const organizationId = requireActiveOrgId(ctx);Also applies to: 88-95, 124-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/chat/chat.ts` around lines 56 - 63, Replace the repeated manual checks of ctx.activeOrganizationId in chat router handlers with the shared guard requireActiveOrgId(ctx): call const organizationId = requireActiveOrgId(ctx) instead of reading ctx.activeOrganizationId and throwing a TRPCError manually; update each duplicate block (the occurrences around the current organizationId checks in chat.ts) to use requireActiveOrgId so the centralized logic is reused and the local throw branches are removed.packages/cli/src/commands/organization/switch/command.ts (1)
12-21: Consider matchingorganization.nametoo (or clarify the arg).The CLI spec documents the argument as
<nameOrId>with a "Switched to Acme Corp" example, but this matcher only acceptsidorslug. Adding a case-insensitive name match (or renaming the spec argument to<idOrSlug>) would avoid confusingOrganization not founderrors for users following the documented UX.♻️ Example: include name match
const match = organizations.find( (organization) => - organization.id === idOrSlug || organization.slug === idOrSlug, + organization.id === idOrSlug || + organization.slug === idOrSlug || + organization.name.toLowerCase() === idOrSlug.toLowerCase(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/organization/switch/command.ts` around lines 12 - 21, The matcher only checks organization.id and organization.slug but the CLI docs/arg refer to nameOrId; update the lookup in the organizations.find call to also match organization.name case-insensitively against idOrSlug (e.g., compare lowercased values) so names like "Acme Corp" succeed, and retain the existing CLIError when no match; reference the organizations.find usage and the fields organization.id, organization.slug, organization.name and the idOrSlug variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/auth/login/command.ts`:
- Around line 40-74: The broad empty catch block around the org-selection flow
hides failures from api.user.me.query, api.user.myOrganizations.query,
api.user.myOrganization.query and writeConfig(config); narrow or remove it so
real errors surface: only swallow non-critical operations (e.g. the
p.log.info(`${user.name}...`) call) inside their own try/catch, but let failures
from writeConfig, organization selection (p.select/p.cancel) and the API queries
propagate (or at minimum log with p.log.warn/error before swallowing). Update
the try/catch to target the specific calls (or add per-call try/catch) and
ensure writeConfig(config) errors are not silently ignored.
---
Outside diff comments:
In `@packages/cli/CLI_SPEC.md`:
- Around line 1109-1120: The docs and implementation disagree: the CLI spec says
`switch <nameOrId>` but the code in
`packages/cli/src/commands/organization/switch/command.ts` only matches
`organization.id === idOrSlug || organization.slug === idOrSlug`; update the
matcher to also accept display names by adding a comparison against
`organization.name` (e.g., `organization.name === idOrSlug`), and normalize
inputs (trim and compare case-insensitively) so passing "Acme Corp" will
succeed; keep the existing id/slug checks and update error text unchanged.
In `@packages/trpc/src/router/billing/billing.ts`:
- Around line 68-92: The invoices handler currently uses
ctx.activeOrganizationId and returns Stripe hostedInvoiceUrl values without
verifying the caller is an organization owner; add the same owner-access guard
used by the details and portal endpoints: in the invoices protectedProcedure
(before fetching subscriptions or returning invoice URLs) verify the current
user is an owner of ctx.activeOrganizationId and throw a TRPCError with code
"FORBIDDEN" if not; only include/return hostedInvoiceUrl (or full invoice links)
when that owner check passes, otherwise return no URLs or an empty result
consistent with the other endpoints.
---
Nitpick comments:
In `@packages/cli/src/commands/organization/switch/command.ts`:
- Around line 12-21: The matcher only checks organization.id and
organization.slug but the CLI docs/arg refer to nameOrId; update the lookup in
the organizations.find call to also match organization.name case-insensitively
against idOrSlug (e.g., compare lowercased values) so names like "Acme Corp"
succeed, and retain the existing CLIError when no match; reference the
organizations.find usage and the fields organization.id, organization.slug,
organization.name and the idOrSlug variable when making the change.
In `@packages/trpc/src/router/chat/chat.ts`:
- Around line 56-63: Replace the repeated manual checks of
ctx.activeOrganizationId in chat router handlers with the shared guard
requireActiveOrgId(ctx): call const organizationId = requireActiveOrgId(ctx)
instead of reading ctx.activeOrganizationId and throwing a TRPCError manually;
update each duplicate block (the occurrences around the current organizationId
checks in chat.ts) to use requireActiveOrgId so the centralized logic is reused
and the local throw branches are removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc257ec6-8431-41eb-9a60-899182d6e86e
📒 Files selected for processing (22)
packages/cli/CLI_SPEC.mdpackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/organization/list/command.tspackages/cli/src/commands/organization/meta.tspackages/cli/src/commands/organization/switch/command.tspackages/cli/src/lib/api-client.tspackages/cli/src/lib/config.tspackages/cli/src/lib/resolve-auth.tspackages/shared/src/constants.tspackages/trpc/src/router/agent/agent.tspackages/trpc/src/router/api-key/api-key.tspackages/trpc/src/router/automation/automation.tspackages/trpc/src/router/billing/billing.tspackages/trpc/src/router/chat/chat.tspackages/trpc/src/router/device/device.tspackages/trpc/src/router/organization/organization.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/user/user.tspackages/trpc/src/router/utils/active-org.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.tspackages/trpc/src/trpc.ts
CLI sends `x-superset-organization-id` when `organizationId` is set in config. A new middleware on `protectedProcedure` validates membership and exposes `ctx.activeOrganizationId`, falling back to the session's active org when the header is absent. Router call sites migrate to the new ctx field; `requireActiveOrgId`/`requireActiveOrgMembership` take `ctx`. Adds `superset organization switch <idOrSlug>`; renames `commands/org/` to `commands/organization/`. `auth login` picks an org when the user has multiple memberships.
saddlepaddle
force-pushed
the
relay-sprites
branch
from
e3873cb to
cb0eb43
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/commands/auth/login/command.ts">
<violation number="1" location="packages/cli/src/commands/auth/login/command.ts:74">
P2: Avoid silently swallowing errors in the post-login org fetch path; log a warning so failures are observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/CLI_SPEC.md">
<violation number="1" location="packages/cli/CLI_SPEC.md:25">
P2: Documenting `--organization` here is misleading because the CLI doesn't define that global flag.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const chosen = organizations.find((o) => o.id === chosenId); | ||
| if (chosen) p.log.info(`Organization: ${chosen.name}`); | ||
| } | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P2: Avoid silently swallowing errors in the post-login org fetch path; log a warning so failures are observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/auth/login/command.ts, line 74:
<comment>Avoid silently swallowing errors in the post-login org fetch path; log a warning so failures are observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) </comment>
<file context>
@@ -40,12 +40,38 @@ export default command({
+ const chosen = organizations.find((o) => o.id === chosenId);
+ if (chosen) p.log.info(`Organization: ${chosen.name}`);
+ }
+ } catch {}
p.outro("Logged in successfully.");
</file context>
Suggested change
| } catch {} | |
| } catch { | |
| p.log.warn("Failed to load organization information after login"); | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/trpc/src/trpc.ts (1)
38-71: Optional: consider micro-caching or short-circuit to reduce per-request DB lookups.Every protected request where
headerOrgIddiffers fromsessionOrgIdnow issues an extramembersquery. For CLI users this is every request (since the CLI typically has no session-side active org set via web). Not a blocker given the query is indexed on(userId, organizationId), but worth keeping in mind if protected endpoints become hot. A per-request memo is already implicit (middleware runs once per call); a cross-request cache would need care around revocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/trpc.ts` around lines 38 - 71, Add a short-lived membership cache to avoid hitting db.query.members.findFirst on every request when headerOrgId !== sessionOrgId: implement a small in-memory cache (e.g., Map or LRU) keyed by `${userId}:${organizationId}` with a very short TTL (seconds) and use a helper like checkMembershipCached(userId, orgId) from the protectedProcedure org-check middleware before calling db.query.members.findFirst; keep the existing authorization logic and only fall back to db.query.members.findFirst when the cache miss occurs, and ensure cache entries store the boolean membership result and are invalidated/expire quickly to avoid staleness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/trpc/src/trpc.ts`:
- Around line 49-71: The headerOrgId read via
ctx.headers.get(ORGANIZATION_HEADER) must be validated as a UUID before being
used in the DB query; update the middleware that reads headerOrgId (the use
async ({ ctx, next }) => { ... } block) to run z.string().uuid().parse/header
validation on headerOrgId (or z.safeParse and throw a TRPCError({ code:
"BAD_REQUEST", message: "Invalid organization id" })) and only call
db.query.members.findFirst with the validated value; alternatively wrap
db.query.members.findFirst / eq(members.organizationId, headerOrgId) in a
try/catch and translate any Postgres uuid parse errors into a TRPCError({ code:
"FORBIDDEN", message: ... }) so malformed headers do not surface as 500s.
---
Nitpick comments:
In `@packages/trpc/src/trpc.ts`:
- Around line 38-71: Add a short-lived membership cache to avoid hitting
db.query.members.findFirst on every request when headerOrgId !== sessionOrgId:
implement a small in-memory cache (e.g., Map or LRU) keyed by
`${userId}:${organizationId}` with a very short TTL (seconds) and use a helper
like checkMembershipCached(userId, orgId) from the protectedProcedure org-check
middleware before calling db.query.members.findFirst; keep the existing
authorization logic and only fall back to db.query.members.findFirst when the
cache miss occurs, and ensure cache entries store the boolean membership result
and are invalidated/expire quickly to avoid staleness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ab68cb7-aac9-4d94-940c-156a64f1a520
📒 Files selected for processing (22)
packages/cli/CLI_SPEC.mdpackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/organization/list/command.tspackages/cli/src/commands/organization/meta.tspackages/cli/src/commands/organization/switch/command.tspackages/cli/src/lib/api-client.tspackages/cli/src/lib/config.tspackages/cli/src/lib/resolve-auth.tspackages/shared/src/constants.tspackages/trpc/src/router/agent/agent.tspackages/trpc/src/router/api-key/api-key.tspackages/trpc/src/router/automation/automation.tspackages/trpc/src/router/billing/billing.tspackages/trpc/src/router/chat/chat.tspackages/trpc/src/router/device/device.tspackages/trpc/src/router/organization/organization.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/user/user.tspackages/trpc/src/router/utils/active-org.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.tspackages/trpc/src/trpc.ts
✅ Files skipped from review due to trivial changes (4)
- packages/trpc/src/router/api-key/api-key.ts
- packages/trpc/src/router/agent/agent.ts
- packages/trpc/src/router/user/user.ts
- packages/cli/src/lib/config.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/cli/src/lib/resolve-auth.ts
- packages/trpc/src/router/device/device.ts
- packages/trpc/src/router/v2-workspace/v2-workspace.ts
- packages/shared/src/constants.ts
- packages/trpc/src/router/chat/chat.ts
- packages/trpc/src/router/v2-project/v2-project.ts
- packages/trpc/src/router/organization/organization.ts
- packages/trpc/src/router/automation/automation.ts
- packages/trpc/src/router/billing/billing.ts
- packages/cli/src/commands/auth/login/command.ts
- packages/cli/src/commands/organization/switch/command.ts
- packages/trpc/src/router/utils/active-org.ts
- packages/cli/CLI_SPEC.md
Comment on lines
+49
to
+71
| .use(async ({ ctx, next }) => { | ||
| const sessionOrgId = ctx.session.session.activeOrganizationId ?? null; | ||
| const headerOrgId = ctx.headers.get(ORGANIZATION_HEADER)?.trim() || null; | ||
|
|
||
| let activeOrganizationId = sessionOrgId; | ||
| if (headerOrgId && headerOrgId !== sessionOrgId) { | ||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(members.userId, ctx.session.user.id), | ||
| eq(members.organizationId, headerOrgId), | ||
| ), | ||
| }); | ||
| if (!membership) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: `Not a member of organization ${headerOrgId}`, | ||
| }); | ||
| } | ||
| activeOrganizationId = headerOrgId; | ||
| } | ||
|
|
||
| return next({ ctx: { ...ctx, activeOrganizationId } }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm members.organizationId column type
rg -nP -C2 'organizationId' packages/db/src/schema 2>/dev/null | head -80
fd -t f 'schema' packages/db | headRepository: superset-sh/superset
Length of output: 4917
🏁 Script executed:
#!/bin/bash
# Find and display the members table schema definition
find packages/db/src/schema -type f -name "*.ts" -exec grep -l "export const members" {} \;Repository: superset-sh/superset
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the auth.ts file to see members table definition
cat -n packages/db/src/schema/auth.ts | grep -A 50 "export const members"Repository: superset-sh/superset
Length of output: 2131
🏁 Script executed:
#!/bin/bash
# Read the trpc.ts file context around the middleware to check for existing validation/error handling
cat -n packages/trpc/src/trpc.ts | sed -n '40,80p'Repository: superset-sh/superset
Length of output: 1527
🏁 Script executed:
#!/bin/bash
# Check if ORGANIZATION_HEADER is defined and look for any existing header validation patterns
rg -n "ORGANIZATION_HEADER" packages/trpc/src/trpc.ts
rg -n "\.uuid\(\)" packages/trpc/src/ | head -20Repository: superset-sh/superset
Length of output: 2080
Validate the organization header as a UUID before using it in the DB query.
headerOrgId is untrusted client input passed directly into eq(members.organizationId, headerOrgId) without validation. Since members.organizationId is a uuid column, a malformed header (e.g. a slug, arbitrary string, or attacker-supplied value) will cause Postgres to throw invalid input syntax for type uuid, surfacing to the caller as a 500 INTERNAL_SERVER_ERROR rather than the intended 403 FORBIDDEN. The PR description notes the CLI supports switch <idOrSlug>, making it plausible for a non-UUID to leak into this header.
The codebase already uses z.string().uuid() extensively for UUID validation (e.g., in workspace and v2-project routers), so adding this validation is consistent with existing patterns.
Recommend validating the header as a UUID up-front (rejecting with BAD_REQUEST) or wrapping the lookup in a try/catch that maps DB validation errors to FORBIDDEN.
🛡️ Suggested validation
+import { z } from "zod";
@@
- const headerOrgId = ctx.headers.get(ORGANIZATION_HEADER)?.trim() || null;
+ const rawHeader = ctx.headers.get(ORGANIZATION_HEADER)?.trim() || null;
+ if (rawHeader && !z.string().uuid().safeParse(rawHeader).success) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: `Invalid ${ORGANIZATION_HEADER} header: expected a UUID`,
+ });
+ }
+ const headerOrgId = rawHeader;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .use(async ({ ctx, next }) => { | |
| const sessionOrgId = ctx.session.session.activeOrganizationId ?? null; | |
| const headerOrgId = ctx.headers.get(ORGANIZATION_HEADER)?.trim() || null; | |
| let activeOrganizationId = sessionOrgId; | |
| if (headerOrgId && headerOrgId !== sessionOrgId) { | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(members.userId, ctx.session.user.id), | |
| eq(members.organizationId, headerOrgId), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: `Not a member of organization ${headerOrgId}`, | |
| }); | |
| } | |
| activeOrganizationId = headerOrgId; | |
| } | |
| return next({ ctx: { ...ctx, activeOrganizationId } }); | |
| }); | |
| .use(async ({ ctx, next }) => { | |
| const sessionOrgId = ctx.session.session.activeOrganizationId ?? null; | |
| const rawHeader = ctx.headers.get(ORGANIZATION_HEADER)?.trim() || null; | |
| if (rawHeader && !z.string().uuid().safeParse(rawHeader).success) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: `Invalid ${ORGANIZATION_HEADER} header: expected a UUID`, | |
| }); | |
| } | |
| const headerOrgId = rawHeader; | |
| let activeOrganizationId = sessionOrgId; | |
| if (headerOrgId && headerOrgId !== sessionOrgId) { | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(members.userId, ctx.session.user.id), | |
| eq(members.organizationId, headerOrgId), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: `Not a member of organization ${headerOrgId}`, | |
| }); | |
| } | |
| activeOrganizationId = headerOrgId; | |
| } | |
| return next({ ctx: { ...ctx, activeOrganizationId } }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/src/trpc.ts` around lines 49 - 71, The headerOrgId read via
ctx.headers.get(ORGANIZATION_HEADER) must be validated as a UUID before being
used in the DB query; update the middleware that reads headerOrgId (the use
async ({ ctx, next }) => { ... } block) to run z.string().uuid().parse/header
validation on headerOrgId (or z.safeParse and throw a TRPCError({ code:
"BAD_REQUEST", message: "Invalid organization id" })) and only call
db.query.members.findFirst with the validated value; alternatively wrap
db.query.members.findFirst / eq(members.organizationId, headerOrgId) in a
try/catch and translate any Postgres uuid parse errors into a TRPCError({ code:
"FORBIDDEN", message: ... }) so malformed headers do not surface as 500s.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
- Scope the post-login try/catch to the non-essential user.me info line; let myOrganizations / writeConfig errors surface. - Skip myOrganization round-trip for single-org users; only fetch the session's active org when the multi-org selector needs it as a default. - Remove --organization from the global flags doc since it isn't wired as an actual global (use `organization switch` instead).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/auth/login/command.ts`:
- Around line 40-51: The code currently creates an API client that can inherit
config.organizationId and then calls api.user.myOrganizations.query(), which
risks being scoped to a stale org; instead create an unscoped discovery client
(use createApiClient for discovery without sending any organization header) to
call user.me.query() and user.myOrganizations.query(), then compare the returned
organizations to the saved config.organizationId (or previousOrganizationId
prompt default) and only persist config.organizationId via writeConfig(config)
if the selected org is present in that returned list; if not present, clear or
replace config.organizationId before calling writeConfig(config).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec507ce2-e0e7-4aee-95b9-b42538386539
📒 Files selected for processing (2)
packages/cli/CLI_SPEC.mdpackages/cli/src/commands/auth/login/command.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/CLI_SPEC.md
Comment on lines
+40
to
+51
| const api = createApiClient(config, { bearer: result.accessToken }); | ||
|
|
||
| try { | ||
| const api = createApiClient(config, { bearer: result.accessToken }); | ||
| const user = await api.user.me.query(); | ||
| const organization = await api.user.myOrganization.query(); | ||
| p.log.info(`${user.name} (${user.email})`); | ||
| if (organization) p.log.info(`Organization: ${organization.name}`); | ||
| } catch { | ||
| // Non-fatal | ||
| } catch (error) { | ||
| p.log.warn( | ||
| `Could not fetch user profile: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| } | ||
|
|
||
| const organizations = await api.user.myOrganizations.query(); |
Contributor
There was a problem hiding this comment.
Avoid scoping login discovery with a stale organization header.
createApiClient(config, ...) can include the previously saved config.organizationId, so myOrganizations may be rejected for users who are logging into a different account or lost access to that org. Use an unscoped discovery client for login/org selection, then only persist a selected org after validating it belongs to the returned list.
Suggested direction
- const api = createApiClient(config, { bearer: result.accessToken });
+ const previousOrganizationId = config.organizationId;
+ const api = createApiClient(
+ { ...config, organizationId: undefined },
+ { bearer: result.accessToken },
+ );Then use previousOrganizationId only as a prompt default if it exists in organizations; otherwise clear/replace config.organizationId before the final writeConfig(config).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const api = createApiClient(config, { bearer: result.accessToken }); | |
| try { | |
| const api = createApiClient(config, { bearer: result.accessToken }); | |
| const user = await api.user.me.query(); | |
| const organization = await api.user.myOrganization.query(); | |
| p.log.info(`${user.name} (${user.email})`); | |
| if (organization) p.log.info(`Organization: ${organization.name}`); | |
| } catch { | |
| // Non-fatal | |
| } catch (error) { | |
| p.log.warn( | |
| `Could not fetch user profile: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| } | |
| const organizations = await api.user.myOrganizations.query(); | |
| const previousOrganizationId = config.organizationId; | |
| const api = createApiClient( | |
| { ...config, organizationId: undefined }, | |
| { bearer: result.accessToken }, | |
| ); | |
| try { | |
| const user = await api.user.me.query(); | |
| p.log.info(`${user.name} (${user.email})`); | |
| } catch (error) { | |
| p.log.warn( | |
| `Could not fetch user profile: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| } | |
| const organizations = await api.user.myOrganizations.query(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/auth/login/command.ts` around lines 40 - 51, The
code currently creates an API client that can inherit config.organizationId and
then calls api.user.myOrganizations.query(), which risks being scoped to a stale
org; instead create an unscoped discovery client (use createApiClient for
discovery without sending any organization header) to call user.me.query() and
user.myOrganizations.query(), then compare the returned organizations to the
saved config.organizationId (or previousOrganizationId prompt default) and only
persist config.organizationId via writeConfig(config) if the selected org is
present in that returned list; if not present, clear or replace
config.organizationId before calling writeConfig(config).
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.
Summary
x-superset-organization-idwhenorganizationIdis set in~/superset/config.json; a new middleware onprotectedProcedurevalidates membership and exposesctx.activeOrganizationId. Falls back to the Better Auth session's active org when the header is absent, so the web app is unaffected.superset organization switch <idOrSlug>and an org picker atsuperset auth loginfor multi-org users. Renamescommands/org/→commands/organization/.user,organization,api-key,chat,agent,device,billing,paidPlanProcedure) migrate fromctx.session.session.activeOrganizationIdtoctx.activeOrganizationId.requireActiveOrgId/requireActiveOrgMembershiptakectxinstead ofsession.Motivation: the CLI used to inherit whichever org the user last clicked in the web app — you couldn't pin a sprite's host-service to a specific org, and two CLIs as the same user fought over the shared session row. Now the CLI declares its target org per-request without ever writing to the session.
jwtProcedureis untouched — the host-service's device JWT carries its ownorganizationIds.Test plan
Verified live against a dev Neon branch +
apps/api+apps/web:organization listwith noorganizationId→ active = session's org (backwards compat)organization switch <slug>writes to configorganization listafter switch → active reflects the header overridesessions.activeOrganizationIdunchanged after switch (web session untouched)organization listwith non-member UUID →FORBIDDEN: Not a member of organization …organizationId(verified via echo server)bun run typecheck+bun run lint:fixpassSummary by cubic
Adds per-request organization selection for the CLI via the
x-superset-organization-idheader without changing the web session. TRPC exposesctx.activeOrganizationId, validates membership, and falls back to the session when the header is missing.New Features
x-superset-organization-idwhenorganizationIdis set in~/superset/config.json(API client passes it automatically).superset organization switch <idOrSlug>; renamedsuperset orgtosuperset organization; removed the global--organizationflag (use the switch command).superset auth loginshows an org picker for multi-org users (auto-selects if only one), saves the choice, and handles cancel/errors cleanly.Refactors
protectedProceduremiddleware setsctx.activeOrganizationIdfrom header or session and verifies membership.ctx.activeOrganizationId;requireActiveOrgId/requireActiveOrgMembershipnow takectx. Migrated call sites inuser,organization,api-key,chat,agent,device,billing,automation,task,v2-project, andv2-workspace.Written for commit 972bd4a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores