fix(cli): switch CLI auth to OAuth code + PKCE + loopback#3318
Merged
Conversation
The CLI's device flow always landed multi-org users on the wrong org.
Better Auth's `deviceAuthorization` mints a fresh session with
`activeOrganizationId = null`, and `customSession` then falls back to
`allMemberships[0]` — the org picked on the `/device` consent page
only ever wrote to the *web* session, never the new CLI session.
Replaces the device flow with RFC 8252 authorization_code + PKCE +
loopback redirect, matching what gcloud/stripe/vercel do. Consent-
time `customAccessTokenClaims` bakes `organizationId`, `email`, and
`plan` into the JWT, so the CLI's bearer carries the picked org (and
plan, for billing gates) with zero session lookups.
Server:
- Narrow `protectedProcedure`'s session to `AuthSession` =
`{user:{id,email}, session:{activeOrganizationId, plan}}` — the
complete set of fields any tRPC route actually reads today, plus
`plan` for imminent billing gates. `customSession` still returns
the full enriched shape for cookie callers.
- Rewrite `apps/api/src/trpc/context.ts` to resolve the session from
one of three sources, bearer-authoritative: (1) OAuth JWT via
`verifyAccessToken`+JWKS, (2) `sk_live_*` API key via
`verifyApiKey` with org from metadata, (3) cookie session via
`getSession`. A malformed or invalid bearer returns null (→ 401);
we never silently fall through to cookie auth on failure.
- Hydrate `user.me` from the DB so callers see consistent fields
regardless of auth method.
- Seed the `superset-cli` OAuth client at API startup via a new
`seedSupersetCliOAuthClient` helper called from Next.js's
`instrumentation.ts:register()` hook. Uses a hardcoded public
`client_id` instead of per-install dynamic registration, matching
every major CLI's pattern (gh/gcloud/stripe). Loopback ports
51789-51793 are seeded as `redirect_uris`.
- Add the `authTime` column `@better-auth/oauth-provider@1.5.6`
expects on `oauthRefreshTokens` (migration 0033).
- Extend `customAccessTokenClaims` to also return `email` and `plan`
so the JWT carries everything the narrowed session needs.
- Extract `resolvePlanForOrganization` shared between `customSession`
and `customAccessTokenClaims`.
- Extract `looksLikeJwt`/`parseApiKeyMetadata`/etc. into a shared
`apps/api/src/lib/auth-utils.ts` used by both the MCP auth-flow
and the new tRPC context builder.
- Delete `deviceAuthorization` plugin + `apps/web/src/app/device/`.
CLI:
- `packages/cli/src/lib/auth.ts` — `authorizationCodeAuth` does:
generate PKCE S256 + random state, bind a one-shot loopback HTTP
server on the first free candidate port, open the browser to
`/oauth2/authorize` with `prompt=consent` (always show the picker
so re-running login can switch orgs), verify the state on the
callback (CSRF), exchange the code at `/oauth2/token` with
`resource` in the POST body (required for JWT mint).
`refreshAccessToken` uses the same hardcoded `client_id`.
- `packages/cli/src/lib/config.ts` — new auth shape
`{accessToken, refreshToken, expiresAt}`, dropped `activeOrg` and
`clientIds`. Fix 0600 file mode bug (both on write and repair on
read for pre-0.1.x installs).
- `packages/cli/src/lib/api-client.ts` — explicit `bearer` option,
no more `config.auth` peek.
- `packages/cli/src/lib/resolve-auth.ts` — shared bearer resolution
used by the middleware, `auth check`, and host commands. Order:
`--api-key` flag → `SUPERSET_API_KEY` env → stored OAuth token
(pre-emptively refreshed if within 5 min of expiry).
- `packages/cli/src/lib/active-org.ts` — derive the active org from
the bearer (JWT claim for OAuth, API call for API keys).
- `packages/cli/src/commands/middleware.ts` — use `resolveAuth`,
pass bearer + authSource on ctx.
- `packages/cli/src/bin.ts` + `run-static.ts` — new `--api-key`
global flag (`.env("SUPERSET_API_KEY")`).
- `auth whoami` → `auth check`; new output reports auth source and
OAuth token expiry.
- Delete `org switch` — the JWT is pinned to the org at mint time,
so switching means re-running `auth login` and picking a different
org on the consent screen. `prompt=consent` on the authorize URL
ensures the picker always shows.
- `host start/stop/status` derive the active org via `getActiveOrgId`
instead of the dropped `config.activeOrg`.
Verified locally against a multi-org user: logout + login + pick
a different org swaps the JWT organizationId, user.me returns the
right account, token refresh advances `iat`/`exp` in the decoded
payload, and sending `Authorization: Bearer <invalid>` with a
browser cookie attached returns 401 instead of silently falling
through to the cookie user.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces device-code OAuth with a browser-based loopback authorization for CLI, introduces new CLI framework plugin/build tooling, adds web authorize UI and API endpoints for CLI code exchange, updates CLI auth/config handling and command context, and adds a DB column for oauth refresh token auth_time. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Terminal)
participant CLI as CLI App
participant Server as Local HTTP Server
participant Browser as Browser
participant WebApp as Web App ( /cli/authorize )
participant API as API Server
User->>CLI: superset auth login
CLI->>Server: start ephemeral loopback server
CLI->>Browser: open /cli/authorize?redirect_uri=...&state=...
Browser->>WebApp: GET /cli/authorize
WebApp->>API: validate session & fetch orgs
API-->>WebApp: user + organizations
WebApp-->>Browser: render CliAuthorizeForm
User->>Browser: select org & authorize
Browser->>API: POST /api/cli/create-code {organizationId}
API-->>Browser: { code }
Browser->>Server: redirect to callback/?code=...&state=...
Server->>CLI: deliver code to awaiting process
CLI->>API: POST /api/cli/exchange { code }
API->>API: create internal session (userId, org)
API-->>CLI: { token, expiresAt }
CLI->>CLI: store { accessToken, expiresAt }
CLI-->>User: login successful
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR replaces the CLI's Better Auth device flow with RFC 8252-compliant authorization_code + PKCE + loopback redirect, fixes the multi-org bug where the device flow silently ignored the org picked on the consent page, and makes the tRPC context builder bearer-authoritative (JWT → API key → cookie, with invalid bearers returning 401 rather than falling through to cookie auth). Key changes:
The overall design is sound and addresses the root-cause bug. Two UX/robustness issues are worth addressing before merge. Confidence Score: 4/5Safe to merge after addressing the missing URL feedback for headless environments and the Windows The core fix (baking organizationId into the JWT via customAccessTokenClaims) is correct and well-tested end-to-end. The bearer-authoritative tRPC context is architecturally sound with proper mutual exclusion between JWT and API-key paths. The PKCE flow is implemented correctly with state verification, timeout, and graceful cleanup. Two concrete issues prevent a 5: the authorization URL is never printed to the terminal (headless users are stuck), and the Windows
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as superset CLI
participant Browser
participant Loopback as 127.0.0.1:5178x/callback
participant API as api.superset.sh
participant JWKS as /api/auth/jwks
participant tRPC as /api/trpc
CLI->>CLI: generatePkce() + generateState()
CLI->>Loopback: bindLoopbackServer() on first free port
CLI->>Browser: openBrowser(authorize URL + code_challenge)
Browser->>API: GET /api/auth/oauth2/authorize?client_id=superset-cli
API->>Browser: redirect to /sign-in then /oauth/consent (org picker)
Browser->>API: POST consent (org selected, referenceId = orgId)
API->>Loopback: redirect to /callback?code=...&state=...
Loopback->>CLI: waitForCallback resolves with code
CLI->>API: POST /api/auth/oauth2/token (code + code_verifier + resource)
API->>CLI: access_token JWT with organizationId+plan, refresh_token
CLI->>CLI: writeConfig mode 0600
Note over CLI,tRPC: Subsequent CLI commands
CLI->>tRPC: Bearer JWT
tRPC->>JWKS: fetch JWKS to verify JWT
JWKS->>tRPC: public key
tRPC->>tRPC: check sub + email + organizationId claims
tRPC->>CLI: protectedProcedure response
Reviews (1): Last reviewed commit: "fix(cli): switch CLI auth to OAuth code ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // auto-approve from cached consent. | ||
| authorizeUrl.searchParams.set("prompt", "consent"); | ||
|
|
||
| await openBrowser(authorizeUrl.toString()); |
There was a problem hiding this comment.
openBrowser is called but its result is not awaited and errors are silently swallowed. In headless environments (SSH tunnels, CI, remote dev machines, WSL without a browser configured) the browser will never open and the user is left staring at "Waiting for browser authorization…" for the full 5-minute timeout with no URL to paste manually.
Consider printing the URL to the terminal in addition to (or as a fallback from) opening the browser, which is the pattern used by gh auth login, gcloud auth login, and stripe login:
Suggested change
| await openBrowser(authorizeUrl.toString()); | |
| console.error(`\nOpen the following URL in your browser:\n ${authorizeUrl.toString()}\n`); | |
| await openBrowser(authorizeUrl.toString()); |
Or alternatively, capture the exec error and only print the URL when the browser launch fails.
Comment on lines
+69
to
+78
| async function openBrowser(url: string): Promise<void> { | ||
| const cmd = | ||
| process.platform === "darwin" | ||
| ? "open" | ||
| : process.platform === "win32" | ||
| ? "start" | ||
| : "xdg-open"; | ||
|
|
||
| const { exec } = await import("node:child_process"); | ||
| exec(`${openCmd} "${verificationUrl}"`); | ||
|
|
||
| // Step 3: Poll for token | ||
| const interval = (codeData.interval || 5) * 1000; | ||
| const deadline = Date.now() + codeData.expires_in * 1000; | ||
|
|
||
| while (Date.now() < deadline) { | ||
| if (signal.aborted) { | ||
| throw new CLIError("Login cancelled"); | ||
| } | ||
|
|
||
| await sleep(interval); | ||
|
|
||
| const tokenRes = await fetch(`${apiUrl}/api/auth/device/token`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| device_code: codeData.device_code, | ||
| grant_type: "urn:ietf:params:oauth:grant-type:device_code", | ||
| client_id: clientId, | ||
| }), | ||
| }); | ||
| exec(`${cmd} "${url}"`); | ||
| } |
There was a problem hiding this comment.
start command needs an empty title argument
start on Windows is a cmd.exe built-in whose first quoted argument is interpreted as the window title, not as the program to launch. Running start "http://..." opens a new terminal window titled with the URL rather than a browser.
The fix is to pass an empty title as the first argument:
Suggested change
| async function openBrowser(url: string): Promise<void> { | |
| const cmd = | |
| process.platform === "darwin" | |
| ? "open" | |
| : process.platform === "win32" | |
| ? "start" | |
| : "xdg-open"; | |
| const { exec } = await import("node:child_process"); | |
| exec(`${openCmd} "${verificationUrl}"`); | |
| // Step 3: Poll for token | |
| const interval = (codeData.interval || 5) * 1000; | |
| const deadline = Date.now() + codeData.expires_in * 1000; | |
| while (Date.now() < deadline) { | |
| if (signal.aborted) { | |
| throw new CLIError("Login cancelled"); | |
| } | |
| await sleep(interval); | |
| const tokenRes = await fetch(`${apiUrl}/api/auth/device/token`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| device_code: codeData.device_code, | |
| grant_type: "urn:ietf:params:oauth:grant-type:device_code", | |
| client_id: clientId, | |
| }), | |
| }); | |
| exec(`${cmd} "${url}"`); | |
| } | |
| const cmd = | |
| process.platform === "darwin" | |
| ? "open" | |
| : process.platform === "win32" | |
| ? 'cmd /c start ""' | |
| : "xdg-open"; | |
| const { exec } = await import("node:child_process"); | |
| exec(`${cmd} "${url}"`); |
This would produce cmd /c start "" "http://..." which correctly opens the URL in the default browser on Windows.
Comment on lines
+50
to
+77
| if (looksLikeJwt(bearer)) { | ||
| try { | ||
| const payload = (await verifyAccessToken(bearer, { | ||
| jwksUrl: `${apiUrl}/api/auth/jwks`, | ||
| verifyOptions: { | ||
| issuer: apiUrl, | ||
| audience: [apiUrl, `${apiUrl}/`], | ||
| }, | ||
| })) as Record<string, unknown>; | ||
|
|
||
| if ( | ||
| typeof payload.sub === "string" && | ||
| typeof payload.email === "string" && | ||
| typeof payload.organizationId === "string" | ||
| ) { | ||
| const plan = typeof payload.plan === "string" ? payload.plan : null; | ||
| return sessionFromClaims( | ||
| payload.sub, | ||
| payload.email, | ||
| payload.organizationId, | ||
| plan, | ||
| ); | ||
| } | ||
| return null; // verified but missing required claims — reject | ||
| } catch { | ||
| return null; // verification failed — reject | ||
| } | ||
| } |
There was a problem hiding this comment.
verifyAccessToken is called with a raw jwksUrl string on every tRPC request that carries a Bearer JWT. If better-auth/oauth2's implementation doesn't cache the JWKS document in-process, this adds an outbound HTTP round-trip (to the same API server!) on every CLI call.
It's worth verifying that the library caches the JWKS, or explicitly constructing a jose createRemoteJWKSet instance at module level and passing it in, which guarantees caching:
// module level (evaluated once per worker):
import { createRemoteJWKSet, jwtVerify } from "jose";
const JWKS = createRemoteJWKSet(new URL(`${apiUrl}/api/auth/jwks`));If verifyAccessToken already handles caching internally this is a no-op, but it's worth a quick check to avoid a hidden latency cliff under load.
Comment on lines
+119
to
+121
| // customSession already populated `plan` on the cookie session. | ||
| (cookieSession.session as { plan?: string | null }).plan ?? null, | ||
| ); |
There was a problem hiding this comment.
plan on the cookie session
cookieSession.session is typed by Better Auth's generated types, which don't include plan because it's added dynamically by customSession. The as { plan?: string | null } cast bypasses TypeScript's type system and will silently return undefined (treated as null here) if customSession is ever disabled or renamed.
Consider expressing the intent more explicitly:
Suggested change
| // customSession already populated `plan` on the cookie session. | |
| (cookieSession.session as { plan?: string | null }).plan ?? null, | |
| ); | |
| // `customSession` augments the session object with `plan`; the | |
| // Better Auth type doesn't include it, so we read it safely. | |
| const sessionWithPlan = cookieSession.session as typeof cookieSession.session & { | |
| plan?: string | null; | |
| }; | |
| return sessionFromClaims( | |
| cookieSession.user.id, | |
| cookieSession.user.email, | |
| cookieSession.session.activeOrganizationId ?? null, | |
| sessionWithPlan.plan ?? null, | |
| ); |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/api/agent/[transport]/auth-flow.ts (1)
165-212:⚠️ Potential issue | 🟠 MajorReject invalid
Authorizationheaders instead of falling back to the cookie session.If a bearer is present but is neither a valid API key nor a verifiable JWT, this still drops into
getSession(). That breaks the new bearer-authoritative contract and can authenticate the request under the cookie org after a stale/wrong bearer is supplied.Suggested fix
export async function verifyToken( req: Request, deps: McpRequestDeps, ): Promise<AuthInfo | undefined> { const bearerToken = getBearerToken(req); const apiUrl = normalizeApiUrl(deps.apiUrl); let oauthVerificationError: unknown; if (bearerToken) { if (isApiKeyBearerToken(bearerToken)) { try { const result = await deps.authApi.verifyApiKey({ body: { key: bearerToken }, }); if (result.valid && result.key) { return buildApiKeyAuthInfo(result.key); } } catch (error) { console.error("[mcp/auth] API key verification failed", { errorName: getSafeAuthErrorName(error), }); } return undefined; } if (looksLikeJwt(bearerToken)) { try { const payload = (await deps.verifyAccessToken(bearerToken, { jwksUrl: `${apiUrl}/api/auth/jwks`, verifyOptions: { issuer: apiUrl, audience: [apiUrl, `${apiUrl}/`], }, })) as Record<string, unknown>; return buildOAuthAuthInfo(bearerToken, payload); } catch (error) { oauthVerificationError = error; } } + + if (oauthVerificationError) { + console.error("[mcp/auth] Access token verification failed", { + errorName: getSafeAuthErrorName(oauthVerificationError), + }); + } + + return undefined; } const session = await deps.authApi.getSession({ headers: req.headers }); if (session?.session) { return buildSessionAuthInfo(session); } - - if (oauthVerificationError) { - console.error("[mcp/auth] Access token verification failed", { - errorName: getSafeAuthErrorName(oauthVerificationError), - }); - } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/agent/`[transport]/auth-flow.ts around lines 165 - 212, If an Authorization bearer is present but fails both API-key and JWT verification, stop falling back to cookie sessions: after the API key branch (isApiKeyBearerToken) and the JWT branch (looksLikeJwt / deps.verifyAccessToken) ensure that if bearerToken exists and neither buildApiKeyAuthInfo nor buildOAuthAuthInfo was returned (i.e., oauthVerificationError set or API key invalid), you immediately log the failure and return undefined instead of proceeding to deps.authApi.getSession; update the control flow around bearerToken, oauthVerificationError, and the session retrieval (deps.authApi.getSession / buildSessionAuthInfo) so cookie sessions are only checked when no Authorization header was supplied.
🧹 Nitpick comments (4)
packages/cli/src/run-static.ts (1)
25-32: Consolidate duplicated global option definitions.
globalsare now defined in both this file andpackages/cli/src/bin.ts(Line 23-25). Extracting a sharedcreateGlobalOptions()(or constant) would prevent static/dev mode drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/run-static.ts` around lines 25 - 32, Duplicate global option definitions (the globals object in run-static.ts and the one in bin.ts) should be consolidated into a single exported creator so both modules share the same options; create and export a createGlobalOptions() function or a GLOBAL_OPTIONS constant that returns the object currently in globals, replace the local `globals` in run-static.ts and the one in bin.ts with an import of that shared creator/constant, and ensure names (json, quiet, device, apiKey) and their yargs/schema calls are identical so static/dev modes stay in sync.packages/db/src/seed-oauth-clients.ts (1)
43-60: Make the seededsuperset-clirow convergent, not just idempotent.
onConflictDoNothing()leaves any existingsuperset-cliclient untouched. Since redirect URI matching is exact here, a stale row will keep breaking CLI login after future port/scope changes until someone fixes the DB manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/seed-oauth-clients.ts` around lines 43 - 60, The seedSupersetCliOAuthClient function currently uses onConflictDoNothing which leaves stale rows unchanged; change the upsert to be convergent by replacing onConflictDoNothing with an onConflictDoUpdate/upsert that targets oauthClients.clientId (for SUPERSET_CLI_CLIENT_ID) and sets the writable columns you want to reconcile (e.g., redirectUris -> LOOPBACK_REDIRECT_URIS, grantTypes, responseTypes, scopes, metadata, tokenEndpointAuthMethod, public, name, and updatedAt) so existing superset-cli rows are updated to the desired values instead of left untouched.packages/cli/src/lib/auth.ts (1)
69-78: Minor: Consider usingspawnwith argument array to avoid shell interpretation.Using
execwith string interpolation could be vulnerable to command injection ifapiUrl(from config) contains shell metacharacters. While the URL is typically trusted, usingspawn/execFilewith an args array is safer.♻️ Safer browser opening
async function openBrowser(url: string): Promise<void> { - const cmd = + const { spawn } = await import("node:child_process"); + const command = process.platform === "darwin" ? "open" : process.platform === "win32" - ? "start" + ? "cmd" : "xdg-open"; - const { exec } = await import("node:child_process"); - exec(`${cmd} "${url}"`); + const args = + process.platform === "win32" + ? ["/c", "start", "", url] + : [url]; + spawn(command, args, { detached: true, stdio: "ignore" }).unref(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/auth.ts` around lines 69 - 78, The openBrowser function currently uses exec with a concatenated command string which risks shell interpretation; change it to use child_process.spawn (or execFile) with an args array so the URL is passed as an argument instead of interpolated into a shell command. Locate openBrowser and replace the exec(`${cmd} "${url}"`) call with a spawn/execFile invocation that passes cmd and [url] (or the appropriate platform-safe variant) as separate arguments and handle the returned ChildProcess errors/exit; ensure imports reference node:child_process.spawn or execFile and that you preserve the platform branching around the cmd variable.apps/api/src/trpc/context.ts (1)
74-76: Consider logging errors before returning null for debugging production issues.The empty catch blocks intentionally reject on any failure (per the bearer-authoritative design), but silently swallowing errors makes debugging production auth failures difficult. A debug-level log would preserve the security posture while aiding observability.
♻️ Optional: Add debug logging
} catch { - return null; // verification failed — reject + // verification failed — reject (log at debug level for production debugging) + console.debug("[trpc/context] JWT verification failed"); + return null; }} catch { + console.debug("[trpc/context] API key verification failed"); return null; }Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trpc/context.ts` around lines 74 - 76, In the catch blocks that currently do "return null; // verification failed — reject" (the token verification try/catch where verification fails), add a debug-level log of the caught error (e.g., logger.debug or console.debug) before returning null, including error.message and a short contextual tag (but do NOT log the raw token or any sensitive auth data); apply the same change to the other identical catch block mentioned (lines 103–105) so both token-verification failures produce non-sensitive debug logs for troubleshooting.
🤖 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/auth/src/lib/resolve-plan-for-organization.ts`:
- Around line 17-22: The findFirst call to db.query.subscriptions.findFirst in
resolve-plan-for-organization.ts is non-deterministic when multiple active
subscriptions exist; modify the call to include an explicit orderBy (for example
order by createdAt desc or id asc) so selection of the "active" subscription is
deterministic, and optionally add a database/ORM unique constraint or check in
the same module to enforce a single active subscription per organization; update
the code paths that reference the subscription variable so they continue to work
with the chosen ordering.
In `@packages/auth/src/server.ts`:
- Around line 227-235: customAccessTokenClaims is currently returning claims
with organizationId or email possibly undefined; update it so the function first
extracts organizationId (from referenceId) and email (from user), validate both
are non-empty strings, and if either is missing throw an error to abort the
token exchange; only after validation call
resolvePlanForOrganization(organizationId) and return the claims {
organizationId, plan, email } so tokens are only minted when both bearer claims
are present.
In `@packages/cli/src/lib/config.ts`:
- Around line 54-63: The writeConfig function currently writes directly to
CONFIG_PATH using writeFileSync which can expose a newly written refreshToken
via the old file mode; change the logic in writeConfig to write the JSON to a
new temporary file (e.g., CONFIG_PATH + ".tmp" or using a unique suffix) with
mode 0o600, flush/close it, then atomically replace the original by calling
renameSync(tempPath, CONFIG_PATH); import renameSync from node:fs and keep the
chmodSync fallback if desired, and continue to call ensureDir() before creating
the temp file to ensure the target directory exists.
---
Outside diff comments:
In `@apps/api/src/app/api/agent/`[transport]/auth-flow.ts:
- Around line 165-212: If an Authorization bearer is present but fails both
API-key and JWT verification, stop falling back to cookie sessions: after the
API key branch (isApiKeyBearerToken) and the JWT branch (looksLikeJwt /
deps.verifyAccessToken) ensure that if bearerToken exists and neither
buildApiKeyAuthInfo nor buildOAuthAuthInfo was returned (i.e.,
oauthVerificationError set or API key invalid), you immediately log the failure
and return undefined instead of proceeding to deps.authApi.getSession; update
the control flow around bearerToken, oauthVerificationError, and the session
retrieval (deps.authApi.getSession / buildSessionAuthInfo) so cookie sessions
are only checked when no Authorization header was supplied.
---
Nitpick comments:
In `@apps/api/src/trpc/context.ts`:
- Around line 74-76: In the catch blocks that currently do "return null; //
verification failed — reject" (the token verification try/catch where
verification fails), add a debug-level log of the caught error (e.g.,
logger.debug or console.debug) before returning null, including error.message
and a short contextual tag (but do NOT log the raw token or any sensitive auth
data); apply the same change to the other identical catch block mentioned (lines
103–105) so both token-verification failures produce non-sensitive debug logs
for troubleshooting.
In `@packages/cli/src/lib/auth.ts`:
- Around line 69-78: The openBrowser function currently uses exec with a
concatenated command string which risks shell interpretation; change it to use
child_process.spawn (or execFile) with an args array so the URL is passed as an
argument instead of interpolated into a shell command. Locate openBrowser and
replace the exec(`${cmd} "${url}"`) call with a spawn/execFile invocation that
passes cmd and [url] (or the appropriate platform-safe variant) as separate
arguments and handle the returned ChildProcess errors/exit; ensure imports
reference node:child_process.spawn or execFile and that you preserve the
platform branching around the cmd variable.
In `@packages/cli/src/run-static.ts`:
- Around line 25-32: Duplicate global option definitions (the globals object in
run-static.ts and the one in bin.ts) should be consolidated into a single
exported creator so both modules share the same options; create and export a
createGlobalOptions() function or a GLOBAL_OPTIONS constant that returns the
object currently in globals, replace the local `globals` in run-static.ts and
the one in bin.ts with an import of that shared creator/constant, and ensure
names (json, quiet, device, apiKey) and their yargs/schema calls are identical
so static/dev modes stay in sync.
In `@packages/db/src/seed-oauth-clients.ts`:
- Around line 43-60: The seedSupersetCliOAuthClient function currently uses
onConflictDoNothing which leaves stale rows unchanged; change the upsert to be
convergent by replacing onConflictDoNothing with an onConflictDoUpdate/upsert
that targets oauthClients.clientId (for SUPERSET_CLI_CLIENT_ID) and sets the
writable columns you want to reconcile (e.g., redirectUris ->
LOOPBACK_REDIRECT_URIS, grantTypes, responseTypes, scopes, metadata,
tokenEndpointAuthMethod, public, name, and updatedAt) so existing superset-cli
rows are updated to the desired values instead of left untouched.
🪄 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: 28de0c8b-3679-40e1-b8d7-0b678e9f020e
📒 Files selected for processing (32)
apps/api/src/app/api/agent/[transport]/auth-flow.tsapps/api/src/instrumentation.tsapps/api/src/lib/auth-utils.tsapps/api/src/trpc/context.tsapps/web/src/app/device/page.tsxpackages/auth/src/lib/resolve-plan-for-organization.tspackages/auth/src/server.tspackages/cli/src/bin.tspackages/cli/src/commands/auth/check/command.tspackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/auth/whoami/command.tspackages/cli/src/commands/host/start/command.tspackages/cli/src/commands/host/status/command.tspackages/cli/src/commands/host/stop/command.tspackages/cli/src/commands/index.tspackages/cli/src/commands/middleware.tspackages/cli/src/commands/org/switch/command.tspackages/cli/src/lib/active-org.tspackages/cli/src/lib/api-client.tspackages/cli/src/lib/auth.tspackages/cli/src/lib/config.tspackages/cli/src/lib/resolve-auth.tspackages/cli/src/run-static.tspackages/db/drizzle/0033_add_oauth_refresh_token_auth_time.sqlpackages/db/drizzle/meta/0033_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/package.jsonpackages/db/src/schema/auth.tspackages/db/src/seed-oauth-clients.tspackages/trpc/src/index.tspackages/trpc/src/router/user/user.tspackages/trpc/src/trpc.ts
💤 Files with no reviewable changes (3)
- packages/cli/src/commands/auth/whoami/command.ts
- apps/web/src/app/device/page.tsx
- packages/cli/src/commands/org/switch/command.ts
Comment on lines
+227
to
+235
| customAccessTokenClaims: async ({ user, referenceId }) => { | ||
| const organizationId = referenceId ?? undefined; | ||
| const plan = await resolvePlanForOrganization(organizationId); | ||
| return { | ||
| organizationId, | ||
| plan, | ||
| email: (user as { email?: string } | undefined)?.email, | ||
| }; | ||
| }, |
Contributor
There was a problem hiding this comment.
Fail the token exchange if required bearer claims are missing.
This can currently mint an access token with organizationId or email as undefined, but apps/api/src/trpc/context.ts only accepts bearer sessions when both claims are strings. That turns a consent/token exchange into an immediately unusable token instead of a deterministic auth failure.
Suggested fix
customAccessTokenClaims: async ({ user, referenceId }) => {
- const organizationId = referenceId ?? undefined;
+ if (!referenceId) {
+ throw new Error("OAuth consent is missing an organization");
+ }
+ if (!user?.email) {
+ throw new Error("OAuth access token requires a user email");
+ }
+ const organizationId = referenceId;
const plan = await resolvePlanForOrganization(organizationId);
return {
organizationId,
plan,
- email: (user as { email?: string } | undefined)?.email,
+ email: user.email,
};
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth/src/server.ts` around lines 227 - 235, customAccessTokenClaims
is currently returning claims with organizationId or email possibly undefined;
update it so the function first extracts organizationId (from referenceId) and
email (from user), validate both are non-empty strings, and if either is missing
throw an error to abort the token exchange; only after validation call
resolvePlanForOrganization(organizationId) and return the claims {
organizationId, plan, email } so tokens are only minted when both bearer claims
are present.
Comment on lines
+54
to
+63
| export function writeConfig(config: SupersetConfig): void { | ||
| ensureDir(); | ||
| writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2)); | ||
| // Pass mode on create. writeFileSync ignores mode for existing files, so | ||
| // we follow up with chmodSync to repair any pre-existing world-readable file. | ||
| writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2), { mode: 0o600 }); | ||
| try { | ||
| chmodSync(CONFIG_PATH, 0o600); | ||
| } catch { | ||
| // chmod failure is non-fatal. | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t rewrite an existing permissive config before its mode is fixed.
For pre-existing config.json, writeFileSync(..., { mode: 0o600 }) still writes through the old mode first. Since this file now holds a long-lived refreshToken, there’s a window where newly written secrets remain readable until chmodSync runs.
Safer pattern
export function writeConfig(config: SupersetConfig): void {
ensureDir();
- // Pass mode on create. writeFileSync ignores mode for existing files, so
- // we follow up with chmodSync to repair any pre-existing world-readable file.
- writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2), { mode: 0o600 });
- try {
- chmodSync(CONFIG_PATH, 0o600);
- } catch {
- // chmod failure is non-fatal.
- }
+ const tempPath = `${CONFIG_PATH}.tmp`;
+ writeFileSync(tempPath, JSON.stringify(config, null, 2), { mode: 0o600 });
+ writeFileSync(tempPath, JSON.stringify(config, null, 2), { mode: 0o600 });
+ renameSync(tempPath, CONFIG_PATH);
}You’d also need to import renameSync from node:fs. Writing to a new 0600 temp file and renaming avoids exposing freshly rotated tokens via an older 0644 file mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/config.ts` around lines 54 - 63, The writeConfig
function currently writes directly to CONFIG_PATH using writeFileSync which can
expose a newly written refreshToken via the old file mode; change the logic in
writeConfig to write the JSON to a new temporary file (e.g., CONFIG_PATH +
".tmp" or using a unique suffix) with mode 0o600, flush/close it, then
atomically replace the original by calling renameSync(tempPath, CONFIG_PATH);
import renameSync from node:fs and keep the chmodSync fallback if desired, and
continue to call ensureDir() before creating the temp file to ensure the target
directory exists.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 32 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/host/stop/command.ts">
<violation number="1" location="packages/cli/src/commands/host/stop/command.ts:17">
P2: Avoid mandatory `myOrganization` lookup in `host stop`; it introduces an unnecessary network dependency and can prevent stopping a local daemon when API lookup fails.</violation>
</file>
<file name="packages/cli/src/lib/config.ts">
<violation number="1" location="packages/cli/src/lib/config.ts:48">
P2: Do not silently swallow config-permission repair failures; emit a warning so insecure token-file permissions are detectable.
(Based on your team's feedback about handling errors explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/api/src/trpc/context.ts">
<violation number="1" location="apps/api/src/trpc/context.ts:50">
P2: The `looksLikeJwt` check runs before `isApiKeyBearerToken`, but it's the broader matcher (any `x.y.z` string). If an API key ever contains two dots it will be misrouted to JWT verification and silently rejected. Check `isApiKeyBearerToken` first since its `sk_live_` prefix is strictly more specific.</violation>
<violation number="2" location="apps/api/src/trpc/context.ts:74">
P2: This `catch` silently swallows errors from JWT verification (including JWKS fetch failures, network errors, misconfigured URLs). If the JWKS endpoint becomes unreachable, all CLI/MCP users get 401s with no server-side log to diagnose it. Log a warning with context before returning null.
(Based on your team's feedback about not silently swallowing errors in catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/commands/host/start/command.ts">
<violation number="1" location="packages/cli/src/commands/host/start/command.ts:19">
P2: Avoid unconditional `myOrganization` lookup before manifest check; it can block the existing-process early return and duplicates API-key org lookup work.</violation>
</file>
<file name="packages/cli/src/lib/auth.ts">
<violation number="1" location="packages/cli/src/lib/auth.ts:77">
P1: On Windows, `start "<url>"` treats the quoted argument as a **window title**, not a URL to open — this opens a blank CMD prompt instead of the browser. The `start` built-in requires an explicit (possibly empty) title as the first quoted argument: `start "" "<url>"`.
Secondary: `exec` spawns a shell, so a `apiUrl` containing shell metacharacters (e.g. backticks) could be interpreted. Consider `execFile` for the non-Windows platforms to avoid the shell entirely.</violation>
<violation number="2" location="packages/cli/src/lib/auth.ts:241">
P1: Print the authorization URL to stderr before attempting to open the browser. In headless environments (SSH, WSL, remote dev boxes, CI) `openBrowser` will silently fail and the user is stuck at "Waiting for browser authorization…" for the full 5-minute timeout with no URL to copy-paste. Every major CLI (`gh`, `gcloud`, `stripe`) prints the URL alongside launching the browser.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // auto-approve from cached consent. | ||
| authorizeUrl.searchParams.set("prompt", "consent"); | ||
|
|
||
| await openBrowser(authorizeUrl.toString()); |
Contributor
There was a problem hiding this comment.
P1: Print the authorization URL to stderr before attempting to open the browser. In headless environments (SSH, WSL, remote dev boxes, CI) openBrowser will silently fail and the user is stuck at "Waiting for browser authorization…" for the full 5-minute timeout with no URL to copy-paste. Every major CLI (gh, gcloud, stripe) prints the URL alongside launching the browser.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/auth.ts, line 241:
<comment>Print the authorization URL to stderr before attempting to open the browser. In headless environments (SSH, WSL, remote dev boxes, CI) `openBrowser` will silently fail and the user is stuck at "Waiting for browser authorization…" for the full 5-minute timeout with no URL to copy-paste. Every major CLI (`gh`, `gcloud`, `stripe`) prints the URL alongside launching the browser.</comment>
<file context>
@@ -1,163 +1,350 @@
+ // auto-approve from cached consent.
+ authorizeUrl.searchParams.set("prompt", "consent");
+
+ await openBrowser(authorizeUrl.toString());
+
+ const code = await waitForCallback({
</file context>
Suggested change
| await openBrowser(authorizeUrl.toString()); | |
| console.error(`\nOpen the following URL in your browser:\n ${authorizeUrl.toString()}\n`); | |
| await openBrowser(authorizeUrl.toString()); |
Comment on lines
+17
to
+18
| const orgRecord = await api.user.myOrganization.query(); | ||
| const orgName = orgRecord?.name ?? organizationId; |
Contributor
There was a problem hiding this comment.
P2: Avoid mandatory myOrganization lookup in host stop; it introduces an unnecessary network dependency and can prevent stopping a local daemon when API lookup fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/host/stop/command.ts, line 17:
<comment>Avoid mandatory `myOrganization` lookup in `host stop`; it introduces an unnecessary network dependency and can prevent stopping a local daemon when API lookup fails.</comment>
<file context>
@@ -1,23 +1,23 @@
+ (opts.options as { apiKey?: string }).apiKey,
+ );
+ const organizationId = await getActiveOrgId(api, bearer, authSource);
+ const orgRecord = await api.user.myOrganization.query();
+ const orgName = orgRecord?.name ?? organizationId;
</file context>
Suggested change
| const orgRecord = await api.user.myOrganization.query(); | |
| const orgName = orgRecord?.name ?? organizationId; | |
| const orgName = organizationId; |
| @@ -1,20 +1,24 @@ | |||
| import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; | |||
| import { | |||
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow config-permission repair failures; emit a warning so insecure token-file permissions are detectable.
(Based on your team's feedback about handling errors explicitly and avoiding silent 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/lib/config.ts, line 48:
<comment>Do not silently swallow config-permission repair failures; emit a warning so insecure token-file permissions are detectable.
(Based on your team's feedback about handling errors explicitly and avoiding silent catch blocks.) </comment>
<file context>
@@ -34,12 +38,29 @@ function ensureDir() {
+ if ((stat.mode & 0o077) !== 0) {
+ chmodSync(CONFIG_PATH, 0o600);
+ }
+ } catch {
+ // stat/chmod failure is non-fatal — proceed with the read.
+ }
</file context>
Reverts the oauthProvider JWT infrastructure (context.ts rewrite, AuthSession narrowing, auth-utils, plan helper, user.me hydration) and replaces it with normal Better Auth sessions — the same token format the desktop and web app use. New flow: CLI opens browser to /cli/authorize (org picker page), user picks an org, page calls /api/cli/create-code (stores a one-time code in Redis keyed to userId:orgId, 5min TTL), redirects to the CLI's loopback callback with the code. CLI exchanges the code at /api/cli/exchange, which creates a session via internalAdapter.createSession with activeOrganizationId set correctly. CLI stores the session token and uses it as Authorization: Bearer — getSession + bearer() + customSession handles the rest. Context.ts untouched. Net -548 lines server-side. CLI auth.ts 160 lines (was 435).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/api/src/app/api/cli/exchange/route.ts (2)
10-15: Add request body validation using Zod.The request body is cast without validation. Using Zod would provide runtime type safety and clearer error messages.
🛠️ Proposed fix using Zod validation
import { auth } from "@superset/auth/server"; import { Redis } from "@upstash/redis"; +import { z } from "zod"; import { env } from "@/env"; +const exchangeSchema = z.object({ + code: z.string().min(1), +}); + export async function POST(req: Request) { - const body = (await req.json()) as { code?: string }; - const code = body.code; - if (!code) { - return Response.json({ error: "code required" }, { status: 400 }); - } + const parsed = exchangeSchema.safeParse(await req.json()); + if (!parsed.success) { + return Response.json({ error: "code required" }, { status: 400 }); + } + const { code } = parsed.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/cli/exchange/route.ts` around lines 10 - 15, Replace the unchecked cast of body in the POST handler with Zod validation: import { z } from "zod", define a schema (e.g., const ExchangeSchema = z.object({ code: z.string().min(1) })), parse the incoming JSON with ExchangeSchema.safeParse or parseAsync inside the POST function instead of using (await req.json()) as { code?: string }, and return a Response.json with a 400 status and the validation error messages when validation fails; then use the validated value (e.g., validated.code) for the rest of the logic.
25-28: Stored value format is fragile if IDs contain colons.Splitting on
:assumes neitheruserIdnororganizationIdcontains that character. While UUIDs don't contain colons, using a more robust delimiter or JSON serialization would be safer.🔧 Alternative: Use JSON for stored value
In
create-code/route.ts:- await redis.set(`cli:code:${code}`, `${session.user.id}:${organizationId}`, { + await redis.set(`cli:code:${code}`, JSON.stringify({ userId: session.user.id, organizationId }), { ex: 300, });In
exchange/route.ts:- const [userId, organizationId] = value.split(":"); - if (!userId || !organizationId) { + let userId: string, organizationId: string; + try { + const parsed = JSON.parse(value) as { userId: string; organizationId: string }; + userId = parsed.userId; + organizationId = parsed.organizationId; + } catch { return Response.json({ error: "Malformed code data" }, { status: 500 }); } + if (!userId || !organizationId) { + return Response.json({ error: "Malformed code data" }, { status: 500 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/cli/exchange/route.ts` around lines 25 - 28, The current split-based unpacking (value.split(":") -> userId, organizationId) is fragile if IDs contain colons; change the flow to serialize the stored code payload as JSON and parse it here instead: replace the split logic with a safe JSON.parse of value, validate that the resulting object has userId and organizationId strings, and handle JSON.parse errors and missing fields by returning a proper error Response (keep the existing Response.json usage). Update the counterpart that writes the code (the create-code route) to store JSON.stringify({ userId, organizationId }) so the exchange route can reliably parse it.apps/api/src/app/api/cli/create-code/route.ts (2)
28-32: Add request body validation using Zod.Similar to the exchange endpoint, using Zod would provide runtime validation and clearer error messages.
🛠️ Proposed fix using Zod validation
import { auth } from "@superset/auth/server"; import { db } from "@superset/db/client"; import { members } from "@superset/db/schema"; import { Redis } from "@upstash/redis"; import { and, eq } from "drizzle-orm"; +import { z } from "zod"; import { env } from "@/env"; +const createCodeSchema = z.object({ + organizationId: z.string().uuid(), +}); + // ... generateCode function ... export async function POST(req: Request) { // ... session check ... - const body = (await req.json()) as { organizationId?: string }; - const organizationId = body.organizationId; - if (!organizationId) { - return Response.json({ error: "organizationId required" }, { status: 400 }); - } + const parsed = createCodeSchema.safeParse(await req.json()); + if (!parsed.success) { + return Response.json({ error: "organizationId required" }, { status: 400 }); + } + const { organizationId } = parsed.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/cli/create-code/route.ts` around lines 28 - 32, The route currently reads and manually checks the request body for organizationId (using req.json() and the organizationId variable) but lacks runtime schema validation; replace this with a Zod schema (e.g., const CreateCodeBody = z.object({ organizationId: z.string().nonempty() })) and use CreateCodeBody.parse or safeParse on the parsed body to validate and extract organizationId, returning a Response.json with status 400 and the Zod error details when validation fails; update any code paths that use organizationId to rely on the parsed/validated value.
22-26: Checksession.userexistence for consistency.The codebase pattern (see
apps/api/src/app/api/electric/[...path]/route.ts:26-32) checks both session andsession.user. IfgetSessioncan return a session without a user in edge cases, this could cause issues at line 36.🛡️ Proposed fix
export async function POST(req: Request) { const session = await auth.api.getSession({ headers: req.headers }); - if (!session) { + if (!session?.user) { return Response.json({ error: "Not authenticated" }, { status: 401 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/cli/create-code/route.ts` around lines 22 - 26, The POST handler currently only checks for a falsy session after calling auth.api.getSession; update it to validate that session.user also exists (i.e., guard against a session object with no user) before proceeding. In the POST function, after auth.api.getSession({ headers: req.headers }) ensure you return a 401 Response.json({ error: "Not authenticated" }) when either session is falsy or session.user is falsy so downstream code relying on session.user does not error.packages/auth/src/server.ts (1)
223-225: Consider validatingreferenceIdbefore minting access tokens.The
customAccessTokenClaimscallback allowsorganizationIdto beundefinedwhenreferenceIdis null. Per the PR objectives, the tRPC context requires bearer sessions to have validorganizationId. Tokens minted without an organization may be immediately unusable.🛡️ Proposed fix to validate referenceId
- customAccessTokenClaims: ({ referenceId }) => ({ - organizationId: referenceId ?? undefined, - }), + customAccessTokenClaims: ({ referenceId }) => { + if (!referenceId) { + throw new Error("OAuth consent is missing an organization"); + } + return { organizationId: referenceId }; + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth/src/server.ts` around lines 223 - 225, customAccessTokenClaims is currently setting organizationId to undefined when referenceId is null which allows minting tokens without a valid organization and breaks tRPC bearer session expectations; modify the customAccessTokenClaims callback to validate referenceId (non-null, non-empty, correct format) before returning claims and if validation fails either throw an error or halt token minting so no access token is issued without a valid organizationId; update logic around customAccessTokenClaims and any calling code that mints tokens to handle the validation error and ensure bearer sessions always receive a populated organizationId.apps/web/src/app/cli/authorize/page.tsx (1)
53-78: Handle the edge case where user has no organizations.If
organizationsis empty (e.g., user's personal org creation failed during signup),CliAuthorizeFormwill have no organizations to display. Consider showing an error state.🛡️ Proposed fix
const trpc = await api(); const organizations = await trpc.user.myOrganizations.query(); + if (organizations.length === 0) { + return ( + <div className="flex min-h-screen items-center justify-center"> + <p className="text-destructive"> + No organizations found. Please contact support. + </p> + </div> + ); + } + return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/cli/authorize/page.tsx` around lines 53 - 78, The page assumes organizations returned by trpc.user.myOrganizations.query are non-empty before rendering CliAuthorizeForm; add an explicit empty-state check after fetching organizations and before rendering CliAuthorizeForm (use the local variable organizations) and render an error/fallback UI when organizations.length === 0 (e.g., a descriptive message and a link/contact action or a retry button) or disable the form inputs; ensure the fallback is shown instead of calling CliAuthorizeForm with an empty organizations prop so the UI doesn't break.
🤖 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/host/stop/command.ts`:
- Around line 17-18: The org name lookup via api.user.myOrganization.query()
(used to set orgName from orgRecord?.name) must be made best-effort so failures
don't abort host stop; wrap the call in a try/catch (or otherwise handle
rejection) and on error or falsy orgRecord fall back to using organizationId for
orgName, ensuring any thrown exception from api.user.myOrganization.query() is
caught and does not prevent the stop flow from continuing.
In `@packages/cli/src/lib/auth.ts`:
- Around line 29-38: openBrowser currently interpolates the URL into a shell
command (risking command injection); change it to avoid using exec with a shell
and instead call child_process.spawn (or execFile) with the URL as a separate
argument so it is not interpreted by a shell. Specifically, update the
openBrowser function to import spawn (or execFile), and: for darwin use
spawn("open", [url]), for linux use spawn("xdg-open", [url]), and for windows
call spawn("cmd", ["/c", "start", "", url]) so the URL is passed as an argument
(do not build a single string or enable shell execution). Ensure you keep the
existing async/await import pattern and handle/ignore the child process stdio
and errors appropriately in openBrowser.
In `@packages/cli/src/lib/resolve-auth.ts`:
- Around line 19-27: If the caller passed an explicit apiKeyOption but it trims
to an empty string we should fail fast instead of falling back to OAuth; update
the logic around apiKeyOption/bearer and authSource so that you detect presence
of the flag (apiKeyOption !== undefined or check process.argv for "--api-key")
and if present but apiKeyOption.trim() === "" throw a clear error (e.g. "empty
--api-key provided") rather than setting bearer to falsy and proceeding; keep
the existing process.argv check that sets authSource to "env" when a non-empty
key came from env, but ensure empty explicit inputs are rejected before
determining authSource.
---
Nitpick comments:
In `@apps/api/src/app/api/cli/create-code/route.ts`:
- Around line 28-32: The route currently reads and manually checks the request
body for organizationId (using req.json() and the organizationId variable) but
lacks runtime schema validation; replace this with a Zod schema (e.g., const
CreateCodeBody = z.object({ organizationId: z.string().nonempty() })) and use
CreateCodeBody.parse or safeParse on the parsed body to validate and extract
organizationId, returning a Response.json with status 400 and the Zod error
details when validation fails; update any code paths that use organizationId to
rely on the parsed/validated value.
- Around line 22-26: The POST handler currently only checks for a falsy session
after calling auth.api.getSession; update it to validate that session.user also
exists (i.e., guard against a session object with no user) before proceeding. In
the POST function, after auth.api.getSession({ headers: req.headers }) ensure
you return a 401 Response.json({ error: "Not authenticated" }) when either
session is falsy or session.user is falsy so downstream code relying on
session.user does not error.
In `@apps/api/src/app/api/cli/exchange/route.ts`:
- Around line 10-15: Replace the unchecked cast of body in the POST handler with
Zod validation: import { z } from "zod", define a schema (e.g., const
ExchangeSchema = z.object({ code: z.string().min(1) })), parse the incoming JSON
with ExchangeSchema.safeParse or parseAsync inside the POST function instead of
using (await req.json()) as { code?: string }, and return a Response.json with a
400 status and the validation error messages when validation fails; then use the
validated value (e.g., validated.code) for the rest of the logic.
- Around line 25-28: The current split-based unpacking (value.split(":") ->
userId, organizationId) is fragile if IDs contain colons; change the flow to
serialize the stored code payload as JSON and parse it here instead: replace the
split logic with a safe JSON.parse of value, validate that the resulting object
has userId and organizationId strings, and handle JSON.parse errors and missing
fields by returning a proper error Response (keep the existing Response.json
usage). Update the counterpart that writes the code (the create-code route) to
store JSON.stringify({ userId, organizationId }) so the exchange route can
reliably parse it.
In `@apps/web/src/app/cli/authorize/page.tsx`:
- Around line 53-78: The page assumes organizations returned by
trpc.user.myOrganizations.query are non-empty before rendering CliAuthorizeForm;
add an explicit empty-state check after fetching organizations and before
rendering CliAuthorizeForm (use the local variable organizations) and render an
error/fallback UI when organizations.length === 0 (e.g., a descriptive message
and a link/contact action or a retry button) or disable the form inputs; ensure
the fallback is shown instead of calling CliAuthorizeForm with an empty
organizations prop so the UI doesn't break.
In `@packages/auth/src/server.ts`:
- Around line 223-225: customAccessTokenClaims is currently setting
organizationId to undefined when referenceId is null which allows minting tokens
without a valid organization and breaks tRPC bearer session expectations; modify
the customAccessTokenClaims callback to validate referenceId (non-null,
non-empty, correct format) before returning claims and if validation fails
either throw an error or halt token minting so no access token is issued without
a valid organizationId; update logic around customAccessTokenClaims and any
calling code that mints tokens to handle the validation error and ensure bearer
sessions always receive a populated organizationId.
🪄 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: e231024d-8e73-4f00-93d6-b3f104c61058
📒 Files selected for processing (14)
apps/api/src/app/api/cli/create-code/route.tsapps/api/src/app/api/cli/exchange/route.tsapps/web/src/app/cli/authorize/components/CliAuthorizeForm/CliAuthorizeForm.tsxapps/web/src/app/cli/authorize/components/CliAuthorizeForm/index.tsapps/web/src/app/cli/authorize/page.tsxpackages/auth/src/server.tspackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/host/start/command.tspackages/cli/src/commands/host/status/command.tspackages/cli/src/commands/host/stop/command.tspackages/cli/src/lib/active-org.tspackages/cli/src/lib/auth.tspackages/cli/src/lib/config.tspackages/cli/src/lib/resolve-auth.ts
✅ Files skipped from review due to trivial changes (2)
- apps/web/src/app/cli/authorize/components/CliAuthorizeForm/index.ts
- packages/cli/src/commands/auth/login/command.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cli/src/commands/host/start/command.ts
- packages/cli/src/commands/host/status/command.ts
- packages/cli/src/lib/config.ts
- packages/cli/src/lib/active-org.ts
Comment on lines
+17
to
+18
| const orgRecord = await api.user.myOrganization.query(); | ||
| const orgName = orgRecord?.name ?? organizationId; |
Contributor
There was a problem hiding this comment.
Do not let org-name lookup failures block host stop.
Line 17 is only used for display text, but an exception there aborts the stop flow before process termination. Make this lookup best-effort and fall back to organizationId.
Suggested patch
- const orgRecord = await api.user.myOrganization.query();
- const orgName = orgRecord?.name ?? organizationId;
+ let orgName = organizationId;
+ try {
+ const orgRecord = await api.user.myOrganization.query();
+ orgName = orgRecord?.name ?? organizationId;
+ } catch {
+ // best-effort display lookup; stopping the daemon should still proceed
+ }📝 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 orgRecord = await api.user.myOrganization.query(); | |
| const orgName = orgRecord?.name ?? organizationId; | |
| let orgName = organizationId; | |
| try { | |
| const orgRecord = await api.user.myOrganization.query(); | |
| orgName = orgRecord?.name ?? organizationId; | |
| } catch { | |
| // best-effort display lookup; stopping the daemon should still proceed | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/host/stop/command.ts` around lines 17 - 18, The org
name lookup via api.user.myOrganization.query() (used to set orgName from
orgRecord?.name) must be made best-effort so failures don't abort host stop;
wrap the call in a try/catch (or otherwise handle rejection) and on error or
falsy orgRecord fall back to using organizationId for orgName, ensuring any
thrown exception from api.user.myOrganization.query() is caught and does not
prevent the stop flow from continuing.
Comment on lines
+29
to
+38
| async function openBrowser(url: string): Promise<void> { | ||
| const cmd = | ||
| process.platform === "darwin" | ||
| ? "open" | ||
| : process.platform === "win32" | ||
| ? "start" | ||
| : "xdg-open"; | ||
|
|
||
| const { exec } = await import("node:child_process"); | ||
| exec(`${openCmd} "${verificationUrl}"`); | ||
|
|
||
| // Step 3: Poll for token | ||
| const interval = (codeData.interval || 5) * 1000; | ||
| const deadline = Date.now() + codeData.expires_in * 1000; | ||
|
|
||
| while (Date.now() < deadline) { | ||
| if (signal.aborted) { | ||
| throw new CLIError("Login cancelled"); | ||
| } | ||
|
|
||
| await sleep(interval); | ||
|
|
||
| const tokenRes = await fetch(`${apiUrl}/api/auth/device/token`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| device_code: codeData.device_code, | ||
| grant_type: "urn:ietf:params:oauth:grant-type:device_code", | ||
| client_id: clientId, | ||
| }), | ||
| }); | ||
| exec(`${cmd} "${url}"`); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential command injection in openBrowser.
The URL is interpolated directly into a shell command. While the URL is constructed from known sources, a malicious SUPERSET_WEB_URL env var or crafted config could inject shell commands via characters like ", $, or backticks.
🛡️ Proposed fix using spawn instead of exec
async function openBrowser(url: string): Promise<void> {
- const cmd =
+ const { spawn } = await import("node:child_process");
+ const cmd =
process.platform === "darwin"
? "open"
: process.platform === "win32"
- ? "start"
+ ? "cmd"
: "xdg-open";
- const { exec } = await import("node:child_process");
- exec(`${cmd} "${url}"`);
+ const args =
+ process.platform === "win32" ? ["/c", "start", "", url] : [url];
+ spawn(cmd, args, { detached: true, stdio: "ignore" }).unref();
}📝 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
| async function openBrowser(url: string): Promise<void> { | |
| const cmd = | |
| process.platform === "darwin" | |
| ? "open" | |
| : process.platform === "win32" | |
| ? "start" | |
| : "xdg-open"; | |
| const { exec } = await import("node:child_process"); | |
| exec(`${openCmd} "${verificationUrl}"`); | |
| // Step 3: Poll for token | |
| const interval = (codeData.interval || 5) * 1000; | |
| const deadline = Date.now() + codeData.expires_in * 1000; | |
| while (Date.now() < deadline) { | |
| if (signal.aborted) { | |
| throw new CLIError("Login cancelled"); | |
| } | |
| await sleep(interval); | |
| const tokenRes = await fetch(`${apiUrl}/api/auth/device/token`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| device_code: codeData.device_code, | |
| grant_type: "urn:ietf:params:oauth:grant-type:device_code", | |
| client_id: clientId, | |
| }), | |
| }); | |
| exec(`${cmd} "${url}"`); | |
| } | |
| async function openBrowser(url: string): Promise<void> { | |
| const { spawn } = await import("node:child_process"); | |
| const cmd = | |
| process.platform === "darwin" | |
| ? "open" | |
| : process.platform === "win32" | |
| ? "cmd" | |
| : "xdg-open"; | |
| const args = | |
| process.platform === "win32" ? ["/c", "start", "", url] : [url]; | |
| spawn(cmd, args, { detached: true, stdio: "ignore" }).unref(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/auth.ts` around lines 29 - 38, openBrowser currently
interpolates the URL into a shell command (risking command injection); change it
to avoid using exec with a shell and instead call child_process.spawn (or
execFile) with the URL as a separate argument so it is not interpreted by a
shell. Specifically, update the openBrowser function to import spawn (or
execFile), and: for darwin use spawn("open", [url]), for linux use
spawn("xdg-open", [url]), and for windows call spawn("cmd", ["/c", "start", "",
url]) so the URL is passed as an argument (do not build a single string or
enable shell execution). Ensure you keep the existing async/await import pattern
and handle/ignore the child process stdio and errors appropriately in
openBrowser.
Comment on lines
+19
to
+27
| let bearer = apiKeyOption?.trim(); | ||
| let authSource: AuthSource = bearer ? "flag" : "oauth"; | ||
|
|
||
| if (bearer && !process.argv.some((arg) => arg.startsWith("--api-key"))) { | ||
| authSource = "env"; | ||
| } | ||
|
|
||
| if (!bearer) { | ||
| if (!config.auth) { |
Contributor
There was a problem hiding this comment.
Reject empty explicit API-key inputs instead of silently falling back to OAuth.
On Line 19 and Line 26, a whitespace-only apiKeyOption is trimmed to an empty string and then treated as “no bearer”, which falls back to config.auth. That can run commands under a different identity than the caller intended. Fail fast when an explicit key value is provided but empty.
Suggested patch
- let bearer = apiKeyOption?.trim();
+ const providedBearer = apiKeyOption;
+ let bearer = providedBearer?.trim();
+ if (providedBearer !== undefined && !bearer) {
+ throw new CLIError(
+ "Invalid API key",
+ "Provide a non-empty --api-key (or SUPERSET_API_KEY)",
+ );
+ }📝 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
| let bearer = apiKeyOption?.trim(); | |
| let authSource: AuthSource = bearer ? "flag" : "oauth"; | |
| if (bearer && !process.argv.some((arg) => arg.startsWith("--api-key"))) { | |
| authSource = "env"; | |
| } | |
| if (!bearer) { | |
| if (!config.auth) { | |
| const providedBearer = apiKeyOption; | |
| let bearer = providedBearer?.trim(); | |
| if (providedBearer !== undefined && !bearer) { | |
| throw new CLIError( | |
| "Invalid API key", | |
| "Provide a non-empty --api-key (or SUPERSET_API_KEY)", | |
| ); | |
| } | |
| let authSource: AuthSource = bearer ? "flag" : "oauth"; | |
| if (bearer && !process.argv.some((arg) => arg.startsWith("--api-key"))) { | |
| authSource = "env"; | |
| } | |
| if (!bearer) { | |
| if (!config.auth) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/resolve-auth.ts` around lines 19 - 27, If the caller
passed an explicit apiKeyOption but it trims to an empty string we should fail
fast instead of falling back to OAuth; update the logic around
apiKeyOption/bearer and authSource so that you detect presence of the flag
(apiKeyOption !== undefined or check process.argv for "--api-key") and if
present but apiKeyOption.trim() === "" throw a clear error (e.g. "empty
--api-key provided") rather than setting bearer to falsy and proceeding; keep
the existing process.argv check that sets authSource to "env" when a non-empty
key came from env, but ensure empty explicit inputs are rejected before
determining authSource.
Contributor
There was a problem hiding this comment.
4 issues found across 21 files (changes from recent commits).
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/trpc/src/router/user/user.ts">
<violation number="1">
P1: `user.me` now returns only the narrowed session user (`id`, `email`) instead of the full user row, which is a breaking behavior change for callers expecting profile fields like `name`/`image`.</violation>
</file>
<file name="packages/cli/src/lib/resolve-auth.ts">
<violation number="1" location="packages/cli/src/lib/resolve-auth.ts:33">
P2: Expired OAuth tokens now hard-fail instead of refreshing, which regresses non-interactive auth continuity for normal CLI use.</violation>
</file>
<file name="apps/api/src/app/api/cli/exchange/route.ts">
<violation number="1" location="apps/api/src/app/api/cli/exchange/route.ts:18">
P1: TOCTOU race: the `get` then `del` is not atomic, so the same authorization code can be redeemed for multiple sessions concurrently. Use a Redis transaction (`multi`/`exec`) to atomically read and delete the code in a single round-trip.</violation>
</file>
<file name="packages/cli/src/commands/auth/login/command.ts">
<violation number="1" location="packages/cli/src/commands/auth/login/command.ts:49">
P2: `auth login` JSON output dropped organization identity fields, which can break script consumers relying on org-scoped output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| const key = `cli:code:${code}`; | ||
| const value = await redis.get<string>(key); |
Contributor
There was a problem hiding this comment.
P1: TOCTOU race: the get then del is not atomic, so the same authorization code can be redeemed for multiple sessions concurrently. Use a Redis transaction (multi/exec) to atomically read and delete the code in a single round-trip.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/app/api/cli/exchange/route.ts, line 18:
<comment>TOCTOU race: the `get` then `del` is not atomic, so the same authorization code can be redeemed for multiple sessions concurrently. Use a Redis transaction (`multi`/`exec`) to atomically read and delete the code in a single round-trip.</comment>
<file context>
@@ -0,0 +1,45 @@
+ }
+
+ const key = `cli:code:${code}`;
+ const value = await redis.get<string>(key);
+ if (!value) {
+ return Response.json({ error: "Invalid or expired code" }, { status: 400 });
</file context>
| "Run: superset auth login (or set SUPERSET_API_KEY)", | ||
| ); | ||
| } | ||
| if (config.auth.expiresAt < Date.now()) { |
Contributor
There was a problem hiding this comment.
P2: Expired OAuth tokens now hard-fail instead of refreshing, which regresses non-interactive auth continuity for normal CLI use.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/resolve-auth.ts, line 33:
<comment>Expired OAuth tokens now hard-fail instead of refreshing, which regresses non-interactive auth continuity for normal CLI use.</comment>
<file context>
@@ -47,8 +30,8 @@ export async function resolveAuth(
}
- if (config.auth.expiresAt - Date.now() < REFRESH_LEEWAY_MS) {
- await refreshAccessToken(config);
+ if (config.auth.expiresAt < Date.now()) {
+ throw new CLIError("Session expired", "Run: superset auth login");
}
</file context>
cli-framework now exposes a bin with `dev` and `build` subcommands. Consumer writes cli.config.ts (defineConfig) and commands/; zero glue files. No bin.ts, no commands/index.ts stub, no runtime plugin() registration. `cli-framework dev` scans the commands directory at runtime; `cli-framework build` generates a temp entry in .cache/cli-framework and calls Bun.build() with createCommandsPlugin, which now has an onResolve hook so no filesystem stub is needed. createCommand<CliContext>() gives every command typed ctx. Group-level middleware skip markers are gone — commands that don't need middleware declare `skipMiddleware: true`. active-org.ts and run-static.ts are deleted; the CLI is now one code path. Also fixes three pre-existing packaging gaps surfaced while testing the staged tarball end-to-end on darwin-arm64: 1. libsql native binding never shipped. It's transitively pulled by mastra; now listed in NATIVE_PACKAGES with platform subpackages (@libsql/darwin-arm64 etc.) in TARGET_NATIVE_PACKAGES. 2. findPackagePath couldn't locate transitive packages in Bun's isolated store. Added a `.bun/<encoded>@*/node_modules/<name>` fallback so transitives (not just direct host-service deps) are walkable. 3. Native ABI mismatch. Desktop's root install:deps runs electron-rebuild on every `bun install`, clobbering the hoisted build/Release/*.node binaries with Electron-ABI (143) builds that can't load under Node 22 (ABI 127). fixNativeBinariesForNode now runs after the copy: downloads the Node-ABI better-sqlite3 prebuild straight from GitHub releases, deletes node-pty/build/ so the `bindings` loader falls through to N-API prebuilds/, and @parcel/watcher-<target> is listed so the platform subpackage (untouched by electron-rebuild) is copied and preferred at runtime over the clobbered main package build. Verified end-to-end on darwin-arm64: bun run build:dist produces a tarball whose bin/superset-host starts, runs migrations via better-sqlite3, and listens on Hono.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/tasks/list/command.ts (1)
6-18:⚠️ Potential issue | 🟠 MajorFilter and pagination flags are currently no-ops.
Line 6–18 advertises filters/limits, but Line 26 calls
task.all.query()without passingoptions, so user inputs are ignored.Suggested fix
-run: async ({ ctx }) => { - const result = await ctx.api.task.all.query(); +run: async ({ ctx, options }) => { + const result = await ctx.api.task.all.query({ + status: options.status ?? undefined, + priority: options.priority ?? undefined, + assigneeMe: options.assigneeMe ?? undefined, + creatorMe: options.creatorMe ?? undefined, + search: options.search ?? undefined, + limit: options.limit ?? undefined, + offset: options.offset ?? undefined, + }); return result.map((r) => ({ ...r.task, assignee: r.assignee?.name ?? "—", })); },Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/tasks/list/command.ts` around lines 6 - 18, The CLI defines filter/pagination flags (status, priority, assigneeMe, creatorMe, search, limit, offset) but never uses them when calling task.all.query(); fix by building a query/filter object from the parsed options and pass it into task.all.query(). Map status, priority, search, limit, and offset directly, and translate assigneeMe/creatorMe into the appropriate assignee/creator filter using the current user id (via your existing auth/currentUser helper), then call task.all.query(query) instead of task.all.query() so user flags are applied.
♻️ Duplicate comments (3)
packages/cli/src/lib/auth.ts (2)
21-33:⚠️ Potential issue | 🟠 MajorAvoid shell-interpolating the browser URL.
This still passes
urlthroughexec(...), so a craftedSUPERSET_WEB_URLor config value can inject shell metacharacters on the user's machine. Usespawn/execFileand pass the URL as a separate argument instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/auth.ts` around lines 21 - 33, openBrowser currently interpolates the URL into shell commands using exec, allowing shell injection; update openBrowser to import and use child_process.spawn or execFile instead of exec and pass the URL as a separate argument (e.g., call "open" with [url], "xdg-open" with [url], and on Windows invoke "cmd" with args ["/c","start","", url] or use spawn/execFile in a way that does not run a shell) so the URL is never shell-interpolated; adjust the import and each process.platform case in the openBrowser function accordingly and ensure shell is disabled.
160-164:⚠️ Potential issue | 🟠 MajorPropagate cancellation to the token exchange.
waitForCallback()respectssignal, but thisfetch()does not. If the API hangs after the browser step,superset auth logincan block indefinitely even after cancellation. Pass the samesignalintofetch()or wrap it with a timeout.Suggested fix
const response = await fetch(`${apiUrl}/api/cli/exchange`, { method: "POST", headers: { "Content-Type": "application/json" }, + signal, body: JSON.stringify({ code }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/auth.ts` around lines 160 - 164, The fetch call that posts the exchanged code (inside the token exchange in auth.ts) doesn't honor the AbortSignal used by waitForCallback(), so cancellation can leave superset auth login hanging; update the fetch invocation in the function handling the exchange (the block that calls fetch(`${apiUrl}/api/cli/exchange`, { ... })) to include the same signal from waitForCallback() (or a derived timeout abort controller) in its options, ensuring the token exchange request is aborted when the caller cancels or when waitForCallback() times out.packages/cli/src/lib/config.ts (1)
46-51:⚠️ Potential issue | 🟠 MajorUse temp-file + atomic rename for secret writes.
Writing directly to
CONFIG_PATHcan still expose newly written token data through old permissive mode untilchmodSyncrepairs it.🔒 Safer write pattern
import { chmodSync, existsSync, mkdirSync, readFileSync, + renameSync, statSync, writeFileSync, } from "node:fs"; @@ export function writeConfig(config: SupersetConfig): void { ensureDir(); - writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2), { + const tempPath = `${CONFIG_PATH}.tmp`; + writeFileSync(tempPath, JSON.stringify(config, null, 2), { mode: 0o600, }); + renameSync(tempPath, CONFIG_PATH); try { chmodSync(CONFIG_PATH, 0o600); } catch {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/config.ts` around lines 46 - 51, Replace the direct write-then-chmod pattern by writing the JSON to a temporary file and atomically renaming it into CONFIG_PATH to avoid a window where the file has permissive mode; specifically, create a temp path (e.g., `${CONFIG_PATH}.tmp`), call writeFileSync(tempPath, JSON.stringify(config, null, 2), { mode: 0o600 }), fsync the file if available, then renameSync(tempPath, CONFIG_PATH) and remove any leftover temp on error; remove the separate chmodSync(CONFIG_PATH) fallback and keep try/catch around the rename to handle failures. Ensure you update references to writeFileSync and chmodSync in this module so the code uses the temp-write + rename pattern atomically for CONFIG_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/cli/authorize/page.tsx`:
- Around line 40-43: The current redirect_uri validation uses startsWith and is
bypassable; replace it by parsing redirect_uri with the URL parser (new
URL(redirect_uri)) and enforce: url.protocol === "http:", url.hostname is
exactly "127.0.0.1" or "localhost", and url.username and url.password are empty;
update the conditional around redirect_uri in page.tsx (the block that currently
checks redirect_uri.startsWith(...)) to perform these checks and reject any
redirect_uri that fails them.
In `@packages/cli-framework/src/plugin.ts`:
- Around line 54-76: The generated module always imports rootMiddleware even
when none exists; change the generator in plugin.ts to conditionally include the
import and export based on middlewareFile: only push `import rootMiddleware from
"${commandsDir}/${middlewareFile}";` when middlewareFile is truthy, and when
absent emit `export const middleware = undefined;` (or a noop) instead of
`export const middleware = rootMiddleware;` so builds don't fail when no root
middleware is present.
- Around line 54-61: The generated import statements in plugin.ts build block
use raw string interpolation for module specifiers (variables: commandsDir,
middlewareFile, commandFiles, metaFiles) which can break on Windows or with
special characters; update the lines array construction to wrap each module
specifier with JSON.stringify(...) so the import specifiers are safely quoted
(e.g., the middleware import and the cmd{i}/meta{i} imports should use
JSON.stringify(`${commandsDir}/${...}`)) mirroring the safer pattern used in
build.ts.
In `@packages/cli-framework/src/runner.ts`:
- Around line 169-173: routeCommand is treating the first "-" segment as the end
of routing, so commands like "superset --json auth check" are seen as
no-command; fix by stripping/consuming declared global flags from args before
calling routeCommand (or update routeCommand to ignore leading global-options),
e.g. detect known global option names (from globalConfigs) and their values in
the args array and remove them so that routeCommand(root, argsWithoutGlobals)
receives only the command path and its args; keep generateRootHelp(name,
version, root, globalConfigs) behavior unchanged and ensure remainingArgs still
contains only command-specific args after this preprocessing.
- Around line 234-244: The middleware block allows middleware to short-circuit
and never call next(), leaving ctx as an empty object and causing runtime errors
in cmd.run; modify the middleware invocation (the block that calls middleware
with options: parsed.options and next: async ...) to track whether next was
invoked (e.g., a nextCalled flag set to true inside the provided next), and
after awaiting middleware (when !cmd.skipMiddleware) throw a clear error or
abort if nextWasNotCalled so commands (cmd.run) are never executed with an
uninitialized ctx; reference the middleware parameter, the next callback, the
ctx variable and cmd.skipMiddleware/cmd.run when making this change.
- Around line 60-63: The UNAUTHORIZED branch inside run() currently writes a
hardcoded "superset auth login" remediation via process.stderr.write when code
=== "UNAUTHORIZED"; change this to emit a generic re-authentication hint or use
a configurable option (e.g., accept an authCommand / remediationMessage in
run()'s options or a provided logger) instead of the literal "superset" string
so framework consumers can supply their own CLI command; update the UNAUTHORIZED
branch to use the provided option (falling back to a neutral message like "Hint:
please re-authenticate") and ensure process.stderr.write references that
variable.
- Around line 206-231: The code maps declared positional args (positionalConfigs
→ argsResult) but doesn't reject surplus entries in parsed.positionals, so add a
check after the for-loop that if no variadic argument was encountered (i.e.,
none of the posConfig.isVariadic branches executed) and posIdx <
parsed.positionals.length then throw a CLIError indicating unexpected extra
positional arguments (include the extra values or count in the message);
reference parsed.positionals, posIdx, posConfig.isVariadic, argsResult and
CLIError to locate where to add this validation.
In `@packages/cli/scripts/build-dist.ts`:
- Around line 161-169: The current loop over readdirSync(bunStore) can pick an
arbitrary .bun entry when multiple versions exist; update the logic that
inspects bunStore to collect all entries that startWith(prefix), extract their
version suffixes (the part after `${encoded}@`), use a deterministic selection
strategy (e.g., parse versions with semver and pick the highest/most recent with
semver.rcompare), then build the candidate path for that chosen entry and return
realpathSync(candidate) only for that selected entry; reference symbols to
change: packageName, bunStore, encoded, prefix, readdirSync, existsSync, and
realpathSync.
In `@packages/cli/src/commands/tasks/create/command.ts`:
- Around line 16-21: The CLI fails to pass the --branch option into the task
creation API call: update the call to ctx.api.task.createFromUi.mutate to
include the branch field (e.g. add branch: options.branch ?? undefined or
branchId: options.branch ?? undefined matching the API shape) so the supplied
options.branch is forwarded when creating a task; modify the object passed to
createFromUi.mutate (in command.ts) to include that branch mapping.
In `@packages/cli/src/commands/tasks/delete/command.ts`:
- Around line 6-13: The delete command incorrectly always calls
ctx.api.task.bySlug.query for each ids entry; change the run handler to detect
whether idOrSlug is a UUID (same check used in
apps/desktop/tasks/$taskId/page.tsx — e.g. a UUID regex or isUUID util) and call
ctx.api.task.byId.query(idOrSlug) for UUIDs and
ctx.api.task.bySlug.query(idOrSlug) for others before calling
ctx.api.task.delete.mutate(task.id); apply the same pattern to the corresponding
get and update command handlers so UUIDs route to byId and non-UUIDs to bySlug.
In `@packages/cli/src/lib/auth.ts`:
- Around line 146-164: The OAuth PKCE flow is missing: create a high-entropy
code_verifier (e.g., codeVerifier) and derive a code_challenge (SHA-256 then
base64url) before building authorizeUrl; add
authorizeUrl.searchParams.set("code_challenge", codeChallenge) and
.set("code_challenge_method","S256") so the browser request includes the
challenge, and then include the original code_verifier in the exchange request
body (replace body: JSON.stringify({ code }) with JSON.stringify({ code,
code_verifier: codeVerifier })) when calling fetch to
`${apiUrl}/api/cli/exchange`; update any related variables (authorizeUrl,
openBrowser, waitForCallback, fetch) accordingly.
In `@packages/cli/src/lib/config.ts`:
- Around line 35-42: The readConfig function currently returns the raw result of
JSON.parse which can produce legacy auth objects missing a numeric expiresAt;
update readConfig to parse safely and then validate the shape (e.g., ensure the
top-level object is an object and, if an auth property exists, that
auth.expiresAt is a number) and if validation fails normalize to a safe empty
config (or drop the invalid auth field) before returning; perform this
validation after JSON.parse and wrap parsing in try/catch so malformed JSON also
yields the safe default. Ensure you update the logic in readConfig and reference
the SupersetConfig shape and the auth.expiresAt property when implementing the
checks.
---
Outside diff comments:
In `@packages/cli/src/commands/tasks/list/command.ts`:
- Around line 6-18: The CLI defines filter/pagination flags (status, priority,
assigneeMe, creatorMe, search, limit, offset) but never uses them when calling
task.all.query(); fix by building a query/filter object from the parsed options
and pass it into task.all.query(). Map status, priority, search, limit, and
offset directly, and translate assigneeMe/creatorMe into the appropriate
assignee/creator filter using the current user id (via your existing
auth/currentUser helper), then call task.all.query(query) instead of
task.all.query() so user flags are applied.
---
Duplicate comments:
In `@packages/cli/src/lib/auth.ts`:
- Around line 21-33: openBrowser currently interpolates the URL into shell
commands using exec, allowing shell injection; update openBrowser to import and
use child_process.spawn or execFile instead of exec and pass the URL as a
separate argument (e.g., call "open" with [url], "xdg-open" with [url], and on
Windows invoke "cmd" with args ["/c","start","", url] or use spawn/execFile in a
way that does not run a shell) so the URL is never shell-interpolated; adjust
the import and each process.platform case in the openBrowser function
accordingly and ensure shell is disabled.
- Around line 160-164: The fetch call that posts the exchanged code (inside the
token exchange in auth.ts) doesn't honor the AbortSignal used by
waitForCallback(), so cancellation can leave superset auth login hanging; update
the fetch invocation in the function handling the exchange (the block that calls
fetch(`${apiUrl}/api/cli/exchange`, { ... })) to include the same signal from
waitForCallback() (or a derived timeout abort controller) in its options,
ensuring the token exchange request is aborted when the caller cancels or when
waitForCallback() times out.
In `@packages/cli/src/lib/config.ts`:
- Around line 46-51: Replace the direct write-then-chmod pattern by writing the
JSON to a temporary file and atomically renaming it into CONFIG_PATH to avoid a
window where the file has permissive mode; specifically, create a temp path
(e.g., `${CONFIG_PATH}.tmp`), call writeFileSync(tempPath,
JSON.stringify(config, null, 2), { mode: 0o600 }), fsync the file if available,
then renameSync(tempPath, CONFIG_PATH) and remove any leftover temp on error;
remove the separate chmodSync(CONFIG_PATH) fallback and keep try/catch around
the rename to handle failures. Ensure you update references to writeFileSync and
chmodSync in this module so the code uses the temp-write + rename pattern
atomically for CONFIG_PATH.
🪄 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: 8202d8ae-7b93-491c-9c1f-0ab3410558de
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
apps/api/src/app/api/cli/create-code/route.tsapps/api/src/app/api/cli/exchange/route.tsapps/web/src/app/cli/authorize/components/CliAuthorizeForm/CliAuthorizeForm.tsxapps/web/src/app/cli/authorize/page.tsxpackages/cli-framework/package.jsonpackages/cli-framework/src/bin.tspackages/cli-framework/src/build.tspackages/cli-framework/src/cli.tspackages/cli-framework/src/command.tspackages/cli-framework/src/config.tspackages/cli-framework/src/dev.tspackages/cli-framework/src/index.tspackages/cli-framework/src/middleware.tspackages/cli-framework/src/plugin.tspackages/cli-framework/src/router.tspackages/cli-framework/src/runner.tspackages/cli/DISTRIBUTION.mdpackages/cli/cli.config.tspackages/cli/package.jsonpackages/cli/scripts/build-dist.tspackages/cli/src/bin.tspackages/cli/src/commands/auth/check/command.tspackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/auth/logout/command.tspackages/cli/src/commands/auth/middleware.tspackages/cli/src/commands/devices/list/command.tspackages/cli/src/commands/host/install/command.tspackages/cli/src/commands/host/middleware.tspackages/cli/src/commands/host/start/command.tspackages/cli/src/commands/host/status/command.tspackages/cli/src/commands/host/stop/command.tspackages/cli/src/commands/index.tspackages/cli/src/commands/org/list/command.tspackages/cli/src/commands/tasks/create/command.tspackages/cli/src/commands/tasks/delete/command.tspackages/cli/src/commands/tasks/get/command.tspackages/cli/src/commands/tasks/list/command.tspackages/cli/src/commands/tasks/update/command.tspackages/cli/src/commands/workspaces/create/command.tspackages/cli/src/commands/workspaces/delete/command.tspackages/cli/src/commands/workspaces/list/command.tspackages/cli/src/lib/api-client.tspackages/cli/src/lib/auth.tspackages/cli/src/lib/command.tspackages/cli/src/lib/config.tspackages/cli/src/lib/env.tspackages/cli/src/lib/host/spawn.tspackages/cli/src/run-static.tspackages/cli/tsconfig.json
💤 Files with no reviewable changes (7)
- packages/cli/src/commands/host/middleware.ts
- packages/cli/src/commands/auth/middleware.ts
- packages/cli-framework/src/middleware.ts
- packages/cli-framework/src/cli.ts
- packages/cli/src/bin.ts
- packages/cli/src/commands/index.ts
- packages/cli/src/run-static.ts
✅ Files skipped from review due to trivial changes (5)
- packages/cli/DISTRIBUTION.md
- packages/cli-framework/package.json
- packages/cli/src/lib/host/spawn.ts
- packages/cli/tsconfig.json
- packages/cli/src/lib/env.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/cli/src/lib/api-client.ts
- packages/cli/src/commands/auth/login/command.ts
- packages/cli/src/commands/host/status/command.ts
- packages/cli/src/commands/host/stop/command.ts
- apps/api/src/app/api/cli/exchange/route.ts
- apps/api/src/app/api/cli/create-code/route.ts
- apps/web/src/app/cli/authorize/components/CliAuthorizeForm/CliAuthorizeForm.tsx
- packages/cli/src/commands/host/start/command.ts
Comment on lines
+40
to
+43
| if ( | ||
| !redirect_uri.startsWith("http://127.0.0.1:") && | ||
| !redirect_uri.startsWith("http://localhost:") | ||
| ) { |
Contributor
There was a problem hiding this comment.
Parse redirect_uri instead of prefix-matching it.
This check is bypassable with userinfo, e.g. http://127.0.0.1:51789@attacker.example/callback. That still passes startsWith(...) but sends the auth code to a non-loopback host. Parse the URL and require protocol === "http:", hostname to be exactly 127.0.0.1 or localhost, and empty username/password.
Suggested fix
- if (
- !redirect_uri.startsWith("http://127.0.0.1:") &&
- !redirect_uri.startsWith("http://localhost:")
- ) {
+ let validatedRedirectUri: string;
+ try {
+ const parsed = new URL(redirect_uri);
+ const isLoopbackHost =
+ parsed.hostname === "127.0.0.1" || parsed.hostname === "localhost";
+ if (
+ parsed.protocol !== "http:" ||
+ !isLoopbackHost ||
+ parsed.username ||
+ parsed.password
+ ) {
+ throw new Error("invalid redirect");
+ }
+ validatedRedirectUri = parsed.toString();
+ } catch {
return (
<div className="flex min-h-screen items-center justify-center">
<p className="text-destructive">
Invalid redirect_uri — only loopback addresses are allowed.
</p>
</div>
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/cli/authorize/page.tsx` around lines 40 - 43, The current
redirect_uri validation uses startsWith and is bypassable; replace it by parsing
redirect_uri with the URL parser (new URL(redirect_uri)) and enforce:
url.protocol === "http:", url.hostname is exactly "127.0.0.1" or "localhost",
and url.username and url.password are empty; update the conditional around
redirect_uri in page.tsx (the block that currently checks
redirect_uri.startsWith(...)) to perform these checks and reject any
redirect_uri that fails them.
Comment on lines
+16
to
21
| const result = await ctx.api.task.createFromUi.mutate({ | ||
| title: options.title, | ||
| description: options.description ?? undefined, | ||
| priority: options.priority, | ||
| assigneeId: options.assignee ?? undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
--branch is defined but never applied in task creation.
Line 13 defines the option, but Line 16–21 omits it from createFromUi.mutate(...), so users cannot set branch on create.
Suggested fix
run: async ({ ctx, options }) => {
const result = await ctx.api.task.createFromUi.mutate({
title: options.title,
description: options.description ?? undefined,
priority: options.priority,
assigneeId: options.assignee ?? undefined,
+ branch: options.branch ?? undefined,
});📝 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 result = await ctx.api.task.createFromUi.mutate({ | |
| title: options.title, | |
| description: options.description ?? undefined, | |
| priority: options.priority, | |
| assigneeId: options.assignee ?? undefined, | |
| }); | |
| const result = await ctx.api.task.createFromUi.mutate({ | |
| title: options.title, | |
| description: options.description ?? undefined, | |
| priority: options.priority, | |
| assigneeId: options.assignee ?? undefined, | |
| branch: options.branch ?? undefined, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/tasks/create/command.ts` around lines 16 - 21, The
CLI fails to pass the --branch option into the task creation API call: update
the call to ctx.api.task.createFromUi.mutate to include the branch field (e.g.
add branch: options.branch ?? undefined or branchId: options.branch ?? undefined
matching the API shape) so the supplied options.branch is forwarded when
creating a task; modify the object passed to createFromUi.mutate (in command.ts)
to include that branch mapping.
Comment on lines
6
to
+13
| args: [positional("ids").required().variadic().desc("Task IDs or slugs")], | ||
| run: async (opts) => { | ||
| const api = opts.ctx.api as ApiClient; | ||
| const ids = opts.args.ids as string[]; | ||
|
|
||
| run: async ({ ctx, args }) => { | ||
| // Required variadic positional — framework guarantees non-empty at runtime | ||
| const ids = args.ids as string[]; | ||
| for (const idOrSlug of ids) { | ||
| // Try as slug first, then as UUID | ||
| const task = await api.task.bySlug.query(idOrSlug); | ||
| if (!task) { | ||
| throw new CLIError(`Task not found: ${idOrSlug}`); | ||
| } | ||
| await api.task.delete.mutate(task.id); | ||
| const task = await ctx.api.task.bySlug.query(idOrSlug); | ||
| if (!task) throw new CLIError(`Task not found: ${idOrSlug}`); | ||
| await ctx.api.task.delete.mutate(task.id); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect task router/procedure definitions around bySlug:"
rg -n --type=ts -C4 '\bbySlug\b'
echo
echo "2) Inspect CLI call sites and user-facing contract:"
rg -n --type=ts -C3 'Task IDs or slugs|task\.bySlug\.query\(|task\.byId\.'Repository: superset-sh/superset
Length of output: 8980
Fix CLI task commands to handle IDs and slugs with conditional routing.
The delete command advertises "Task IDs or slugs" but uses only ctx.api.task.bySlug.query(idOrSlug). This mismatch exists in get and update commands too. The router has separate byId (UUID-validated) and bySlug (any string) procedures; the desktop app (apps/desktop/.../tasks/$taskId/page.tsx) correctly checks the input format and routes to the appropriate procedure. Update CLI commands to follow the same pattern: route UUIDs to byId and non-UUIDs to bySlug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/tasks/delete/command.ts` around lines 6 - 13, The
delete command incorrectly always calls ctx.api.task.bySlug.query for each ids
entry; change the run handler to detect whether idOrSlug is a UUID (same check
used in apps/desktop/tasks/$taskId/page.tsx — e.g. a UUID regex or isUUID util)
and call ctx.api.task.byId.query(idOrSlug) for UUIDs and
ctx.api.task.bySlug.query(idOrSlug) for others before calling
ctx.api.task.delete.mutate(task.id); apply the same pattern to the corresponding
get and update command handlers so UUIDs route to byId and non-UUIDs to bySlug.
Comment on lines
+146
to
164
| const authorizeUrl = new URL(`${webUrl}/cli/authorize`); | ||
| authorizeUrl.searchParams.set("redirect_uri", redirectUri); | ||
| authorizeUrl.searchParams.set("state", state); | ||
|
|
||
| await openBrowser(authorizeUrl.toString()); | ||
|
|
||
| const code = await waitForCallback({ | ||
| server, | ||
| port, | ||
| expectedState: state, | ||
| signal, | ||
| timeoutMs: 5 * 60 * 1000, | ||
| }); | ||
|
|
||
| const response = await fetch(`${apiUrl}/api/cli/exchange`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| client_name: CLI_CLIENT_NAME, | ||
| redirect_uris: ["http://localhost/callback"], | ||
| grant_types: ["authorization_code"], | ||
| response_types: ["code"], | ||
| token_endpoint_auth_method: "none", | ||
| }), | ||
| body: JSON.stringify({ code }), | ||
| }); |
Contributor
There was a problem hiding this comment.
PKCE is missing from the code flow.
The browser step only sends state, and the exchange step only sends { code }. Since apps/api/src/app/api/cli/exchange/route.ts:15-51 also redeems a code without a verifier, any process that captures the loopback code can exchange it. Generate a code_verifier, send its code_challenge with the authorize request, and require the matching code_verifier during exchange.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/auth.ts` around lines 146 - 164, The OAuth PKCE flow is
missing: create a high-entropy code_verifier (e.g., codeVerifier) and derive a
code_challenge (SHA-256 then base64url) before building authorizeUrl; add
authorizeUrl.searchParams.set("code_challenge", codeChallenge) and
.set("code_challenge_method","S256") so the browser request includes the
challenge, and then include the original code_verifier in the exchange request
body (replace body: JSON.stringify({ code }) with JSON.stringify({ code,
code_verifier: codeVerifier })) when calling fetch to
`${apiUrl}/api/cli/exchange`; update any related variables (authorizeUrl,
openBrowser, waitForCallback, fetch) accordingly.
Comment on lines
35
to
42
| export function readConfig(): SupersetConfig { | ||
| if (!existsSync(CONFIG_PATH)) return {}; | ||
| try { | ||
| const stat = statSync(CONFIG_PATH); | ||
| if ((stat.mode & 0o077) !== 0) chmodSync(CONFIG_PATH, 0o600); | ||
| } catch {} | ||
| return JSON.parse(readFileSync(CONFIG_PATH, "utf-8")); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate parsed config shape before returning it.
SupersetConfig is compile-time only; JSON.parse can return legacy auth objects missing a numeric expiresAt. That can bypass expiry checks downstream and produce stale-token behavior instead of forcing re-login.
💡 Suggested hardening
export function readConfig(): SupersetConfig {
if (!existsSync(CONFIG_PATH)) return {};
try {
const stat = statSync(CONFIG_PATH);
if ((stat.mode & 0o077) !== 0) chmodSync(CONFIG_PATH, 0o600);
} catch {}
- return JSON.parse(readFileSync(CONFIG_PATH, "utf-8"));
+ const parsed = JSON.parse(readFileSync(CONFIG_PATH, "utf-8")) as SupersetConfig;
+ if (
+ parsed.auth &&
+ (typeof parsed.auth.accessToken !== "string" ||
+ typeof parsed.auth.expiresAt !== "number" ||
+ !Number.isFinite(parsed.auth.expiresAt))
+ ) {
+ delete parsed.auth;
+ }
+ return parsed;
}📝 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
| export function readConfig(): SupersetConfig { | |
| if (!existsSync(CONFIG_PATH)) return {}; | |
| try { | |
| const stat = statSync(CONFIG_PATH); | |
| if ((stat.mode & 0o077) !== 0) chmodSync(CONFIG_PATH, 0o600); | |
| } catch {} | |
| return JSON.parse(readFileSync(CONFIG_PATH, "utf-8")); | |
| } | |
| export function readConfig(): SupersetConfig { | |
| if (!existsSync(CONFIG_PATH)) return {}; | |
| try { | |
| const stat = statSync(CONFIG_PATH); | |
| if ((stat.mode & 0o077) !== 0) chmodSync(CONFIG_PATH, 0o600); | |
| } catch {} | |
| const parsed = JSON.parse(readFileSync(CONFIG_PATH, "utf-8")) as SupersetConfig; | |
| if ( | |
| parsed.auth && | |
| (typeof parsed.auth.accessToken !== "string" || | |
| typeof parsed.auth.expiresAt !== "number" || | |
| !Number.isFinite(parsed.auth.expiresAt)) | |
| ) { | |
| delete parsed.auth; | |
| } | |
| return parsed; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/lib/config.ts` around lines 35 - 42, The readConfig function
currently returns the raw result of JSON.parse which can produce legacy auth
objects missing a numeric expiresAt; update readConfig to parse safely and then
validate the shape (e.g., ensure the top-level object is an object and, if an
auth property exists, that auth.expiresAt is a number) and if validation fails
normalize to a safe empty config (or drop the invalid auth field) before
returning; perform this validation after JSON.parse and wrap parsing in
try/catch so malformed JSON also yields the safe default. Ensure you update the
logic in readConfig and reference the SupersetConfig shape and the
auth.expiresAt property when implementing the checks.
Contributor
There was a problem hiding this comment.
1 issue found across 51 files (changes from recent commits).
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/lib/auth.ts">
<violation number="1" location="packages/cli/src/lib/auth.ts:132">
P2: getWebUrl no longer maps non-production API hosts (e.g., localhost:3101 or api.*) to the web app host. That makes CLI login open `/cli/authorize` on the API server for local/self-hosted setups. Restore the prior host/port mapping when SUPERSET_WEB_URL isn’t set.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export function getWebUrl(config: SupersetConfig): string { | ||
| if (process.env.SUPERSET_WEB_URL) return process.env.SUPERSET_WEB_URL; | ||
| const apiUrl = config.apiUrl ?? "https://api.superset.sh"; | ||
| return apiUrl.replace("api.superset.sh", "app.superset.sh"); |
Contributor
There was a problem hiding this comment.
P2: getWebUrl no longer maps non-production API hosts (e.g., localhost:3101 or api.*) to the web app host. That makes CLI login open /cli/authorize on the API server for local/self-hosted setups. Restore the prior host/port mapping when SUPERSET_WEB_URL isn’t set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/auth.ts, line 132:
<comment>getWebUrl no longer maps non-production API hosts (e.g., localhost:3101 or api.*) to the web app host. That makes CLI login open `/cli/authorize` on the API server for local/self-hosted setups. Restore the prior host/port mapping when SUPERSET_WEB_URL isn’t set.</comment>
<file context>
@@ -102,43 +87,49 @@ function waitForCallback({
- );
+ if (process.env.SUPERSET_WEB_URL) return process.env.SUPERSET_WEB_URL;
+ const apiUrl = config.apiUrl ?? "https://api.superset.sh";
+ return apiUrl.replace("api.superset.sh", "app.superset.sh");
}
</file context>
Seven independent fixes against comments on 311067d: - plugin.ts: conditionally import root middleware. dev.ts treats commandsDir/middleware.ts as optional, but the build plugin used to emit an unconditional import — a consumer without a root middleware would work in dev and crash at build. - plugin.ts: JSON.stringify import specifiers in the generated module. Matches the pattern already used in build.ts. Raw interpolation was fragile on Windows paths or repo paths with special characters. - runner.ts: thread opts.name through handleError instead of hardcoding "superset auth login" in the UNAUTHORIZED branch. run() is a framework-level API. - runner.ts: leading global flags no longer break routing. `superset --json auth check` used to print root help because routeCommand stops at the first `-` segment. Added a splitArgsForRouting helper that peels known globals from argv before routing and concatenates them back into remainingArgs for the command parser. Applied to both the help path and main routing. - runner.ts: reject surplus positionals. Non-variadic commands previously silently dropped trailing positional arguments, so typos like `tasks get ABC extra` ran successfully. - runner.ts: require middleware to invoke next() before running typed commands. A short-circuiting middleware used to leave ctx as {} while cmd.run received it typed as the consumer's CliContext, turning a middleware bug into a runtime undefined access. Now throws CLIError with a clear message. - build-dist.ts: error on ambiguous .bun store matches. If the isolated store contains multiple versions of the same package, readdirSync previously returned whichever one the filesystem yielded first. Now collects all matches and throws unless exactly one is found.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/scripts/build-dist.ts (1)
17-19:⚠️ Potential issue | 🟡 MinorDocumentation mentions unsupported target.
The usage example references
--target=darwin-x64, butdarwin-x64is not inVALID_TARGETS(onlydarwin-arm64andlinux-x64are valid). Either add darwin-x64 support or update the documentation.🔧 Suggested fix
- * bun run scripts/build-dist.ts --target=darwin-x64 + * bun run scripts/build-dist.ts --target=linux-x64🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 17 - 19, The usage examples in build-dist.ts reference an unsupported target ("darwin-x64"); update either the VALID_TARGETS constant to include "darwin-x64" (if you intend to support that target) or remove/replace "darwin-x64" from the documented examples so examples match VALID_TARGETS. Locate the VALID_TARGETS array and the usage comment lines in build-dist.ts (symbols: VALID_TARGETS and the example lines containing --target=darwin-x64) and either add "darwin-x64" to the array and ensure any build logic supports it, or delete/replace that example entry so the docs only show valid targets ("darwin-arm64" and "linux-x64").
🧹 Nitpick comments (2)
packages/cli-framework/src/plugin.ts (1)
41-53: Consider adding error handling for Glob operations.The
Glob.scanSynccalls (lines 42-53) currently allow errors to bubble up to the Bun build system. While this is acceptable for build tooling, wrapping the scanning logic in a try-catch and providing a more descriptive error message (e.g., mentioning thecommandsDirpath) could improve the developer experience when builds fail.♻️ Optional: Add descriptive error handling
build.onLoad({ filter: loadFilter, namespace: NAMESPACE }, () => { + try { const commandFiles = Array.from( new Glob("**/command.ts").scanSync({ cwd: commandsDir, onlyFiles: true, }), ).sort(); const metaFiles = Array.from( new Glob("**/meta.ts").scanSync({ cwd: commandsDir, onlyFiles: true, }), ).sort(); + } catch (error) { + throw new Error( + `Failed to scan commands directory at ${commandsDir}: ${error}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-framework/src/plugin.ts` around lines 41 - 53, Wrap the Glob.scanSync calls inside the build.onLoad handler with try-catch to catch any errors from new Glob(...).scanSync and rethrow or log a clearer error that includes the commandsDir path and context (e.g., when populating commandFiles and metaFiles in build.onLoad using loadFilter and NAMESPACE). Specifically, surround the two Array.from(new Glob("**/command.ts").scanSync(...)) and Array.from(new Glob("**/meta.ts").scanSync(...)) calls with a single try block and in the catch include a descriptive message referencing commandsDir and the failing Glob pattern before throwing or returning an appropriate build error.packages/cli/scripts/build-dist.ts (1)
63-68: Consider adding a compile-time or runtime check for NODE_ABI/NODE_VERSION consistency.
NODE_ABImust stay in sync withNODE_VERSION. IfNODE_VERSIONis bumped without updatingNODE_ABI, the build will silently fetch incompatiblebetter-sqlite3prebuilds. A static assertion or runtime validation would prevent this drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/build-dist.ts` around lines 63 - 68, Add a runtime validation in build-dist.ts to ensure the hardcoded NODE_ABI constant stays consistent with the current Node version: compare the NODE_ABI constant to process.versions.modules (or derive ABI from the NODE_VERSION constant if present) and throw or exit with a clear error if they differ, so build steps that reference NODE_ABI (the NODE_ABI symbol) fail fast; implement this check early in the script (before any better-sqlite3 prebuild fetch) and include a concise error message instructing to update NODE_ABI when NODE_VERSION is bumped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/scripts/build-dist.ts`:
- Around line 17-19: The usage examples in build-dist.ts reference an
unsupported target ("darwin-x64"); update either the VALID_TARGETS constant to
include "darwin-x64" (if you intend to support that target) or remove/replace
"darwin-x64" from the documented examples so examples match VALID_TARGETS.
Locate the VALID_TARGETS array and the usage comment lines in build-dist.ts
(symbols: VALID_TARGETS and the example lines containing --target=darwin-x64)
and either add "darwin-x64" to the array and ensure any build logic supports it,
or delete/replace that example entry so the docs only show valid targets
("darwin-arm64" and "linux-x64").
---
Nitpick comments:
In `@packages/cli-framework/src/plugin.ts`:
- Around line 41-53: Wrap the Glob.scanSync calls inside the build.onLoad
handler with try-catch to catch any errors from new Glob(...).scanSync and
rethrow or log a clearer error that includes the commandsDir path and context
(e.g., when populating commandFiles and metaFiles in build.onLoad using
loadFilter and NAMESPACE). Specifically, surround the two Array.from(new
Glob("**/command.ts").scanSync(...)) and Array.from(new
Glob("**/meta.ts").scanSync(...)) calls with a single try block and in the catch
include a descriptive message referencing commandsDir and the failing Glob
pattern before throwing or returning an appropriate build error.
In `@packages/cli/scripts/build-dist.ts`:
- Around line 63-68: Add a runtime validation in build-dist.ts to ensure the
hardcoded NODE_ABI constant stays consistent with the current Node version:
compare the NODE_ABI constant to process.versions.modules (or derive ABI from
the NODE_VERSION constant if present) and throw or exit with a clear error if
they differ, so build steps that reference NODE_ABI (the NODE_ABI symbol) fail
fast; implement this check early in the script (before any better-sqlite3
prebuild fetch) and include a concise error message instructing to update
NODE_ABI when NODE_VERSION is bumped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60569c0b-1a96-40e2-9108-992ed794aa80
📒 Files selected for processing (3)
packages/cli-framework/src/plugin.tspackages/cli-framework/src/runner.tspackages/cli/scripts/build-dist.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-framework/src/runner.ts
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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-framework/src/runner.ts">
<violation number="1" location="packages/cli-framework/src/runner.ts:108">
P2: Normalize global flag names/aliases before storing them in `splitArgsForRouting`; otherwise dashed aliases (e.g. `-j`) won’t be recognized during routing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const globalsByName = new Map<string, ProcessedBuilderConfig>(); | ||
| for (const cfg of Object.values(globalConfigs)) { | ||
| globalsByName.set(cfg.name, cfg); | ||
| for (const alias of cfg.aliases) globalsByName.set(alias, cfg); |
Contributor
There was a problem hiding this comment.
P2: Normalize global flag names/aliases before storing them in splitArgsForRouting; otherwise dashed aliases (e.g. -j) won’t be recognized during routing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli-framework/src/runner.ts, line 108:
<comment>Normalize global flag names/aliases before storing them in `splitArgsForRouting`; otherwise dashed aliases (e.g. `-j`) won’t be recognized during routing.</comment>
<file context>
@@ -91,6 +91,53 @@ function processGlobals(
+ const globalsByName = new Map<string, ProcessedBuilderConfig>();
+ for (const cfg of Object.values(globalConfigs)) {
+ globalsByName.set(cfg.name, cfg);
+ for (const alias of cfg.aliases) globalsByName.set(alias, cfg);
+ }
+
</file context>
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 11, 2026
…h#3318) * fix(cli): switch CLI auth to OAuth code + PKCE + loopback The CLI's device flow always landed multi-org users on the wrong org. Better Auth's `deviceAuthorization` mints a fresh session with `activeOrganizationId = null`, and `customSession` then falls back to `allMemberships[0]` — the org picked on the `/device` consent page only ever wrote to the *web* session, never the new CLI session. Replaces the device flow with RFC 8252 authorization_code + PKCE + loopback redirect, matching what gcloud/stripe/vercel do. Consent- time `customAccessTokenClaims` bakes `organizationId`, `email`, and `plan` into the JWT, so the CLI's bearer carries the picked org (and plan, for billing gates) with zero session lookups. Server: - Narrow `protectedProcedure`'s session to `AuthSession` = `{user:{id,email}, session:{activeOrganizationId, plan}}` — the complete set of fields any tRPC route actually reads today, plus `plan` for imminent billing gates. `customSession` still returns the full enriched shape for cookie callers. - Rewrite `apps/api/src/trpc/context.ts` to resolve the session from one of three sources, bearer-authoritative: (1) OAuth JWT via `verifyAccessToken`+JWKS, (2) `sk_live_*` API key via `verifyApiKey` with org from metadata, (3) cookie session via `getSession`. A malformed or invalid bearer returns null (→ 401); we never silently fall through to cookie auth on failure. - Hydrate `user.me` from the DB so callers see consistent fields regardless of auth method. - Seed the `superset-cli` OAuth client at API startup via a new `seedSupersetCliOAuthClient` helper called from Next.js's `instrumentation.ts:register()` hook. Uses a hardcoded public `client_id` instead of per-install dynamic registration, matching every major CLI's pattern (gh/gcloud/stripe). Loopback ports 51789-51793 are seeded as `redirect_uris`. - Add the `authTime` column `@better-auth/oauth-provider@1.5.6` expects on `oauthRefreshTokens` (migration 0033). - Extend `customAccessTokenClaims` to also return `email` and `plan` so the JWT carries everything the narrowed session needs. - Extract `resolvePlanForOrganization` shared between `customSession` and `customAccessTokenClaims`. - Extract `looksLikeJwt`/`parseApiKeyMetadata`/etc. into a shared `apps/api/src/lib/auth-utils.ts` used by both the MCP auth-flow and the new tRPC context builder. - Delete `deviceAuthorization` plugin + `apps/web/src/app/device/`. CLI: - `packages/cli/src/lib/auth.ts` — `authorizationCodeAuth` does: generate PKCE S256 + random state, bind a one-shot loopback HTTP server on the first free candidate port, open the browser to `/oauth2/authorize` with `prompt=consent` (always show the picker so re-running login can switch orgs), verify the state on the callback (CSRF), exchange the code at `/oauth2/token` with `resource` in the POST body (required for JWT mint). `refreshAccessToken` uses the same hardcoded `client_id`. - `packages/cli/src/lib/config.ts` — new auth shape `{accessToken, refreshToken, expiresAt}`, dropped `activeOrg` and `clientIds`. Fix 0600 file mode bug (both on write and repair on read for pre-0.1.x installs). - `packages/cli/src/lib/api-client.ts` — explicit `bearer` option, no more `config.auth` peek. - `packages/cli/src/lib/resolve-auth.ts` — shared bearer resolution used by the middleware, `auth check`, and host commands. Order: `--api-key` flag → `SUPERSET_API_KEY` env → stored OAuth token (pre-emptively refreshed if within 5 min of expiry). - `packages/cli/src/lib/active-org.ts` — derive the active org from the bearer (JWT claim for OAuth, API call for API keys). - `packages/cli/src/commands/middleware.ts` — use `resolveAuth`, pass bearer + authSource on ctx. - `packages/cli/src/bin.ts` + `run-static.ts` — new `--api-key` global flag (`.env("SUPERSET_API_KEY")`). - `auth whoami` → `auth check`; new output reports auth source and OAuth token expiry. - Delete `org switch` — the JWT is pinned to the org at mint time, so switching means re-running `auth login` and picking a different org on the consent screen. `prompt=consent` on the authorize URL ensures the picker always shows. - `host start/stop/status` derive the active org via `getActiveOrgId` instead of the dropped `config.activeOrg`. Verified locally against a multi-org user: logout + login + pick a different org swaps the JWT organizationId, user.me returns the right account, token refresh advances `iat`/`exp` in the decoded payload, and sending `Authorization: Bearer <invalid>` with a browser cookie attached returns 401 instead of silently falling through to the cookie user. * fix: remove oauth client seed, insert row directly into prod/dev * fix(cli): replace JWT auth with session tokens Reverts the oauthProvider JWT infrastructure (context.ts rewrite, AuthSession narrowing, auth-utils, plan helper, user.me hydration) and replaces it with normal Better Auth sessions — the same token format the desktop and web app use. New flow: CLI opens browser to /cli/authorize (org picker page), user picks an org, page calls /api/cli/create-code (stores a one-time code in Redis keyed to userId:orgId, 5min TTL), redirects to the CLI's loopback callback with the code. CLI exchanges the code at /api/cli/exchange, which creates a session via internalAdapter.createSession with activeOrganizationId set correctly. CLI stores the session token and uses it as Authorization: Bearer — getSession + bearer() + customSession handles the rest. Context.ts untouched. Net -548 lines server-side. CLI auth.ts 160 lines (was 435). * refactor(cli): framework-owned dev/build, fix native packaging cli-framework now exposes a bin with `dev` and `build` subcommands. Consumer writes cli.config.ts (defineConfig) and commands/; zero glue files. No bin.ts, no commands/index.ts stub, no runtime plugin() registration. `cli-framework dev` scans the commands directory at runtime; `cli-framework build` generates a temp entry in .cache/cli-framework and calls Bun.build() with createCommandsPlugin, which now has an onResolve hook so no filesystem stub is needed. createCommand<CliContext>() gives every command typed ctx. Group-level middleware skip markers are gone — commands that don't need middleware declare `skipMiddleware: true`. active-org.ts and run-static.ts are deleted; the CLI is now one code path. Also fixes three pre-existing packaging gaps surfaced while testing the staged tarball end-to-end on darwin-arm64: 1. libsql native binding never shipped. It's transitively pulled by mastra; now listed in NATIVE_PACKAGES with platform subpackages (@libsql/darwin-arm64 etc.) in TARGET_NATIVE_PACKAGES. 2. findPackagePath couldn't locate transitive packages in Bun's isolated store. Added a `.bun/<encoded>@*/node_modules/<name>` fallback so transitives (not just direct host-service deps) are walkable. 3. Native ABI mismatch. Desktop's root install:deps runs electron-rebuild on every `bun install`, clobbering the hoisted build/Release/*.node binaries with Electron-ABI (143) builds that can't load under Node 22 (ABI 127). fixNativeBinariesForNode now runs after the copy: downloads the Node-ABI better-sqlite3 prebuild straight from GitHub releases, deletes node-pty/build/ so the `bindings` loader falls through to N-API prebuilds/, and @parcel/watcher-<target> is listed so the platform subpackage (untouched by electron-rebuild) is copied and preferred at runtime over the clobbered main package build. Verified end-to-end on darwin-arm64: bun run build:dist produces a tarball whose bin/superset-host starts, runs migrations via better-sqlite3, and listens on Hono. * fix(cli): PR review followups on framework refactor Seven independent fixes against comments on 311067d: - plugin.ts: conditionally import root middleware. dev.ts treats commandsDir/middleware.ts as optional, but the build plugin used to emit an unconditional import — a consumer without a root middleware would work in dev and crash at build. - plugin.ts: JSON.stringify import specifiers in the generated module. Matches the pattern already used in build.ts. Raw interpolation was fragile on Windows paths or repo paths with special characters. - runner.ts: thread opts.name through handleError instead of hardcoding "superset auth login" in the UNAUTHORIZED branch. run() is a framework-level API. - runner.ts: leading global flags no longer break routing. `superset --json auth check` used to print root help because routeCommand stops at the first `-` segment. Added a splitArgsForRouting helper that peels known globals from argv before routing and concatenates them back into remainingArgs for the command parser. Applied to both the help path and main routing. - runner.ts: reject surplus positionals. Non-variadic commands previously silently dropped trailing positional arguments, so typos like `tasks get ABC extra` ran successfully. - runner.ts: require middleware to invoke next() before running typed commands. A short-circuiting middleware used to leave ctx as {} while cmd.run received it typed as the consumer's CliContext, turning a middleware bug into a runtime undefined access. Now throws CLIError with a clear message. - build-dist.ts: error on ambiguous .bun store matches. If the isolated store contains multiple versions of the same package, readdirSync previously returned whichever one the filesystem yielded first. Now collects all matches and throws unless exactly one is found.
6 tasks
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
activeOrganizationId = null;customSessionfell back toallMemberships[0], which meant the org picked on the/deviceconsent page was silently ignored. New flow bakesorganizationIdinto the JWT claim at mint time so every tRPC call reads the correct org with no session lookup.apps/web/src/app/device/are deleted.superset-clipublic client_id seeded at API startup viainstrumentation.ts:register()instead of per-install dynamic registration. Matches gh/gcloud/stripe industry pattern; dynamic registration is still allowed for third-party MCP clients but a follow-up PR should marktrusted: trueclients with a verified badge on the consent page.sk_live_*API key (viaverifyApiKey+ metadata.organizationId) → cookie session. Invalid bearer returns 401 — never silently falls through to cookie auth. Headless / CI path isSUPERSET_API_KEY=sk_live_....protectedProcedure's session type to exactly what routes read (user.{id,email}+session.{activeOrganizationId, plan}).planis baked into the JWT for imminent billing gates. Full enriched shape still returned to cookie callers viacustomSession.org switch— the JWT is pinned at login time, so switching means re-runningauth loginwithprompt=consentalways showing the org picker.auth whoami→auth checkwith auth-source reporting.authTimecolumn tooauthRefreshTokens(migration 0033) that@better-auth/oauth-provider@1.5.6expects.Verified end-to-end (local dev)
organizationId,plan: pro(for paying orgs),email,azp: superset-cli, correctaud, 1hexpauth checkshows user + org + "OAuth session (expires in 60 min)"expiresAtto the past triggers pre-emptive refresh on next command, newiat/expin the JWTAuthorization: Bearer <invalid-jwt>alongside a valid browser cookie returns 401, doesn't fall through to cookie auth-rw-------after any CLI commandsuperset-clirow auto-seeded on fresh API boot; idempotent across restartsFollow-up (not in this PR)
oauthClients.metadata.trustedand render first-party clients with a verified badge, self-registered DCR clients with "⚠ Unverified application" — closes the phishing vector that unauthenticated DCR currently leaves open.redirect_uristo loopback only (same file that already normalizes localhost↔127.0.0.1 inapps/api/src/app/api/auth/[...all]/route.ts).jwtProcedureintoprotectedProcedure: migratedevice.ensureV2Host/checkHostAccess/setHostOnline+organization.getActiveFromJwtoff thejwt()plugin (pluralorganizationIdsclaim) and ontooauthProvider(singularorganizationId). Requires coordinated rollout across desktop + relay + host-service.Test plan
superset auth loginon a multi-org user lands on the picked org (the exact bug)superset auth checkreports the right org + auth sourceSUPERSET_API_KEY=sk_live_... superset auth checkreturns the API key's scoped orgSummary by cubic
Switches the CLI from device flow to a browser-based loopback login with a one‑time code that creates an org‑scoped session, fixing the multi‑org bug. Also moves the CLI to
@superset/cli-frameworkdev/build and tightens native packaging; includes follow‑up framework fixes for routing and build stability.New Features
/cli/authorizewith org picker →/api/cli/create-code→ CLI exchanges at/api/cli/exchangeto mint a session with the correctactiveOrganizationId; server removes device flow and/device.auth check; removesauth whoamiandorg switch. Host commands derive the active org via API.--api-keyorSUPERSET_API_KEY(precedence: flag → env → stored session). Config perms fixed to 0600; stored shape is{ accessToken, expiresAt }.@superset/cli-frameworknow providesdev/buildwithcli.config.ts; commands can setskipMiddleware: true. Packaging fixes include shipping libsql bindings, preferring platform@parcel/watcher, matching Node ABI forbetter-sqlite3, and resolving transitives in Bun’s store.next()before typed commands run, clearer unauthorized errors, andbuild-disterrors on ambiguous.bunstore matches.Migration
superset auth loginto create a new session and pick an org; re-run to switch orgs.0033addingauth_timetooauth_refresh_tokens.Written for commit f7b0bbf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores