fix: make headless api-key auth work end-to-end (CLI + server org resolution)#3493
Open
saddlepaddle wants to merge 4 commits intomainfrom
Open
fix: make headless api-key auth work end-to-end (CLI + server org resolution)#3493saddlepaddle wants to merge 4 commits intomainfrom
saddlepaddle wants to merge 4 commits intomainfrom
Conversation
Three bugs were blocking a fresh Linux box from running `superset host start` with a `sk_live_` key end-to-end: 1. The tRPC client in the CLI sent the key as `Authorization: Bearer <key>`, which better-auth's api-key plugin does not accept — the plugin requires `x-api-key`. Every CLI call returned 401 under api-key auth. Fix: switch to `x-api-key` for `sk_live_`-prefixed bearers, keep `Authorization: Bearer` for OAuth tokens. 2. `JwtApiAuthProvider` in the host service made the same mistake on its `/api/auth/token` JWT exchange. So even if the CLI bootstrapped correctly, the host service failed to mint a JWT, the relay tunnel never connected, and `host start` timed out waiting on health. 3. `build-dist.ts` on `linux-x64` deleted node-pty's `build/Release/` expecting an in-package N-API prebuild as a fallback, but node-pty only ships prebuilds for darwin/win32. The shipped host service bundle had no `pty.node`, so every invocation crashed at `Failed to load native module: pty.node`. Fix: on linux-x64, run `node-gyp rebuild` in the staged copy; node-pty uses node-addon-api so the resulting binding is ABI-stable. Dropped the over-strict `spawn-helper` existence check because that target is only built on macOS. With all three landed: `superset auth check` and `superset host start --daemon` work through a linux-x64 bundle in a fresh sandbox, signed in via `SUPERSET_API_KEY=sk_live_...`.
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:
📝 WalkthroughWalkthroughSpecial-case handling for production API keys ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiClient
participant HostService
participant AuthServer
participant DB
Client->>ApiClient: Send request with token
alt token starts with "sk_live_"
ApiClient->>HostService: HTTP request with x-api-key
HostService->>AuthServer: Validate token (x-api-key)
AuthServer->>DB: Load API key row (metadata.organizationId)
AuthServer-->>HostService: Session payload limited to that org
HostService-->>ApiClient: Response (org-bound)
else non "sk_live_"
ApiClient->>HostService: HTTP request with Authorization: Bearer
HostService->>AuthServer: Validate token (Bearer)
AuthServer->>DB: Load user and org memberships
AuthServer-->>HostService: Session payload with resolved orgs
HostService-->>ApiClient: Response (resolved orgs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
api keys created via `apiKeyRouter.create` already store their intended org in `metadata.organizationId`, but the trpc context creator never read it. The api-key-synthesized session had no `activeOrganizationId`, so requests fell through to the default newest-membership resolver and silently routed to the wrong org whenever the user belonged to more than one. This was most visible via `superset auth check` reporting a different org than the one the key was minted for. Fix: new `resolveApiKey` helper (`apps/api/src/lib/api-key-org.ts`) that pulls `x-api-key` / `Bearer sk_live_...` from the request, calls `auth.api.verifyApiKey`, reads `metadata.organizationId`, and verifies the user is still a member of that org. Four outcomes: - not-api-key → use session as-is (OAuth / no auth) - no-organization-metadata → use session as-is (legacy key, pre-router) - ok → override session.activeOrganizationId - invalid → null the session, request gets 401 The membership check is the security boundary for api-key deprovisioning when a user is removed from an org: we do not delete/disable keys on removal (reversible, non-destructive) but we also do not let them authenticate as the org they no longer belong to. If the user is re-added, their key starts working again. Blast radius: 390 api keys in prod today, 367 with org metadata, 0 currently orphaned — this does not break any existing consumer. The 23 legacy keys without `metadata.organizationId` fall through to the existing resolver unchanged (no behavior change for them).
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Greptile SummaryThis PR fixes three independent bugs that were preventing
Confidence Score: 4/5Safe to merge — all three bugs are correctly fixed; only non-blocking P2 cleanups remain The two auth header fixes are clean, minimal, and consistent with the same pattern already used in the server-side auth-flow. The linux build fix correctly handles the missing N-API prebuild case and the ABI-stability reasoning is sound. The only remaining items are P2 style concerns in build-dist.ts (tarball bloat from intermediate build artifacts, undocumented Python/gcc dependency) that don't affect runtime correctness. packages/cli/scripts/build-dist.ts — tarball artifact cleanup after node-gyp rebuild
|
| Filename | Overview |
|---|---|
| packages/cli/scripts/build-dist.ts | Adds linux-x64 node-pty build-from-source path; logic is correct but leaves intermediate build artifacts (obj files, Makefiles) in the tarball — only pty.node is needed at runtime |
| packages/cli/src/lib/api-client.ts | Conditionally uses x-api-key header for sk_live_* tokens; fix is correct and consistent with the isApiKeyBearerToken pattern used in auth-flow.ts |
| packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.ts | Mirrors the api-client.ts header fix for the JWT exchange flow; correctly applies x-api-key for sk_live_* session tokens |
Sequence Diagram
sequenceDiagram
participant CLI as superset CLI
participant API as Superset API
participant HS as host-service
Note over CLI,API: Before fix — API key path returns 401
CLI->>API: POST /api/trpc (Authorization: Bearer token)
API-->>CLI: 401 Unauthorized
Note over CLI,API: After fix — correct header routing
CLI->>API: POST /api/trpc (x-api-key: token)
API-->>CLI: 200 OK
Note over HS,API: JWT mint exchange (JwtAuthProvider)
HS->>API: GET /api/auth/token (x-api-key: token)
API-->>HS: short-lived JWT
Note over HS,API: Subsequent relay requests use JWT
HS->>API: POST /api/... (Authorization: Bearer JWT)
API-->>HS: 200 OK
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 276-281
Comment:
**Build artifacts bloat the linux tarball**
After `node-gyp rebuild` succeeds, only `build/Release/pty.node` is needed at runtime. The entire `build/` directory — which includes `obj.target/` (compiled `.o` files), `Makefile`, `config.gypi`, and other intermediate artifacts — stays in the staging tree and is packed into the tarball. Depending on the machine, this can easily add 5–20 MB.
The darwin path avoids this cleanly by removing `build/` entirely (darwin falls back to the in-package prebuilds). The linux path should do the same with a targeted cleanup after the verification:
```ts
const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 274-275
Comment:
**`node-gyp rebuild` requires Python 3 and a C++ toolchain**
`node-gyp rebuild` will fail with a cryptic error if `python3` or `gcc`/`g++` are not available on the build machine. This path is new to the script and could surprise first-time linux contributors. Consider adding a preflight check or a clearer failure message:
```ts
console.log("[build-dist] compiling node-pty from source for linux-x64 (requires python3 + gcc)");
```
Or check explicitly before calling `exec`:
```ts
await exec("python3", ["--version"]).catch(() => {
throw new Error("node-gyp requires python3 — install it and re-run");
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(api): resolve api-key active org fro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+276
to
+281
| const builtBinding = join(nodePtyBuild, "Release", "pty.node"); | ||
| if (!existsSync(builtBinding)) { | ||
| throw new Error(`node-pty build did not produce ${builtBinding}`); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
After node-gyp rebuild succeeds, only build/Release/pty.node is needed at runtime. The entire build/ directory — which includes obj.target/ (compiled .o files), Makefile, config.gypi, and other intermediate artifacts — stays in the staging tree and is packed into the tarball. Depending on the machine, this can easily add 5–20 MB.
The darwin path avoids this cleanly by removing build/ entirely (darwin falls back to the in-package prebuilds). The linux path should do the same with a targeted cleanup after the verification:
const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 276-281
Comment:
**Build artifacts bloat the linux tarball**
After `node-gyp rebuild` succeeds, only `build/Release/pty.node` is needed at runtime. The entire `build/` directory — which includes `obj.target/` (compiled `.o` files), `Makefile`, `config.gypi`, and other intermediate artifacts — stays in the staging tree and is packed into the tarball. Depending on the machine, this can easily add 5–20 MB.
The darwin path avoids this cleanly by removing `build/` entirely (darwin falls back to the in-package prebuilds). The linux path should do the same with a targeted cleanup after the verification:
```ts
const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+274
to
+275
| console.log("[build-dist] compiling node-pty from source for linux-x64"); | ||
| await exec("npx", ["--yes", "node-gyp", "rebuild"], nodePtyDir); |
There was a problem hiding this comment.
node-gyp rebuild requires Python 3 and a C++ toolchain
node-gyp rebuild will fail with a cryptic error if python3 or gcc/g++ are not available on the build machine. This path is new to the script and could surprise first-time linux contributors. Consider adding a preflight check or a clearer failure message:
console.log("[build-dist] compiling node-pty from source for linux-x64 (requires python3 + gcc)");Or check explicitly before calling exec:
await exec("python3", ["--version"]).catch(() => {
throw new Error("node-gyp requires python3 — install it and re-run");
});Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 274-275
Comment:
**`node-gyp rebuild` requires Python 3 and a C++ toolchain**
`node-gyp rebuild` will fail with a cryptic error if `python3` or `gcc`/`g++` are not available on the build machine. This path is new to the script and could surprise first-time linux contributors. Consider adding a preflight check or a clearer failure message:
```ts
console.log("[build-dist] compiling node-pty from source for linux-x64 (requires python3 + gcc)");
```
Or check explicitly before calling `exec`:
```ts
await exec("python3", ["--version"]).catch(() => {
throw new Error("node-gyp requires python3 — install it and re-run");
});
```
How can I resolve this? If you propose a fix, please make it concise.
saddlepaddle
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/lib/api-key-org.ts (1)
63-93: LGTM with optional observability suggestion.The implementation is solid:
- Proper discriminated union return types for clear caller handling
- Membership verification prevents access for deprovisioned users
- Try-catch ensures verification failures don't crash the request
Consider adding structured logging for
"invalid"outcomes (lines 73, 87, 91) to support security auditing and debugging. This would help trace why API key authentications fail in production.,
📝 Optional: Add audit logging for failed verifications
+import { logger } from "@/lib/logger"; // or your logging solution + export async function resolveApiKey( req: Request, auth: typeof betterAuth, userId: string, ): Promise<ApiKeyResolution> { const apiKey = extractApiKey(req); if (!apiKey) return { kind: "not-api-key" }; try { const result = await auth.api.verifyApiKey({ body: { key: apiKey } }); - if (!result.valid || !result.key) return { kind: "invalid" }; + if (!result.valid || !result.key) { + logger.warn("API key verification failed", { userId }); + return { kind: "invalid" }; + } const metadata = parseApiKeyMetadata(result.key.metadata); const organizationId = metadata?.organizationId; if (typeof organizationId !== "string") { return { kind: "no-organization-metadata" }; } const membership = await db.query.members.findFirst({ where: and( eq(members.userId, userId), eq(members.organizationId, organizationId), ), }); - if (!membership) return { kind: "invalid" }; + if (!membership) { + logger.warn("API key org membership check failed", { userId, organizationId }); + return { kind: "invalid" }; + } return { kind: "ok", organizationId }; } catch { + logger.error("API key resolution error", { userId }); return { kind: "invalid" }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/api-key-org.ts` around lines 63 - 93, Add structured audit logging inside resolveApiKey around the early returns so failed API-key resolutions are recorded: when extractApiKey() yields no key, when auth.api.verifyApiKey(...) returns invalid or missing key, when parseApiKeyMetadata(...) yields a non-string organizationId, and when db.query.members.findFirst(...) finds no membership. Use the project's central logger (e.g., processLogger or auditLogger) to log a concise event with masked apiKey, userId, reason ("no-api-key"|"verify-failed"|"no-organization-metadata"|"no-membership") and any available metadata (organizationId if present) before returning the corresponding { kind: ... } result; keep logs structured and avoid emitting raw secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/lib/api-key-org.ts`:
- Around line 63-93: Add structured audit logging inside resolveApiKey around
the early returns so failed API-key resolutions are recorded: when
extractApiKey() yields no key, when auth.api.verifyApiKey(...) returns invalid
or missing key, when parseApiKeyMetadata(...) yields a non-string
organizationId, and when db.query.members.findFirst(...) finds no membership.
Use the project's central logger (e.g., processLogger or auditLogger) to log a
concise event with masked apiKey, userId, reason
("no-api-key"|"verify-failed"|"no-organization-metadata"|"no-membership") and
any available metadata (organizationId if present) before returning the
corresponding { kind: ... } result; keep logs structured and avoid emitting raw
secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24715049-e1cd-443e-a98c-a4828a1ac54e
📒 Files selected for processing (2)
apps/api/src/lib/api-key-org.tsapps/api/src/trpc/context.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 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="apps/api/src/lib/api-key-org.ts">
<violation number="1" location="apps/api/src/lib/api-key-org.ts:32">
P2: Silent `catch {}` on an auth-critical path swallows verification and DB errors without any logging. Add at least a warning log with context so failures are observable in production.
(Based on your team's feedback about handling async errors explicitly and not silently swallowing failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth/src/server.ts (1)
42-59: Consider excluding arrays from valid metadata.
typeof parsed === "object"returnstruefor arrays. If metadata like"[]"is passed, it would be returned asRecord<string, unknown>which may not be the intended behavior.🔧 Proposed fix
function parseApiKeyMetadata( metadata: unknown, ): Record<string, unknown> | null { if (!metadata) return null; if (typeof metadata === "string") { try { const parsed = JSON.parse(metadata); - return parsed && typeof parsed === "object" + return parsed && typeof parsed === "object" && !Array.isArray(parsed) ? (parsed as Record<string, unknown>) : null; } catch { return null; } } - return typeof metadata === "object" + return typeof metadata === "object" && !Array.isArray(metadata) ? (metadata as Record<string, unknown>) : null; }🤖 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 42 - 59, The parseApiKeyMetadata function treats arrays as valid objects because it only checks typeof === "object"; update it to exclude arrays by ensuring parsed is non-null, not an array (use Array.isArray(parsed) check) before casting to Record<string, unknown>, and do the same for the branch that returns metadata when metadata is already an object (ensure metadata is not null and not an array before casting).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/auth/src/server.ts`:
- Around line 42-59: The parseApiKeyMetadata function treats arrays as valid
objects because it only checks typeof === "object"; update it to exclude arrays
by ensuring parsed is non-null, not an array (use Array.isArray(parsed) check)
before casting to Record<string, unknown>, and do the same for the branch that
returns metadata when metadata is already an object (ensure metadata is not null
and not an array before casting).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0cf4fd5-0a36-4108-b5bc-6083c7f405f0
📒 Files selected for processing (1)
packages/auth/src/server.ts
Contributor
There was a problem hiding this comment.
2 issues 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="apps/api/src/trpc/context.ts">
<violation number="1">
P0: This change bypasses api-key org resolution/membership enforcement by passing the raw session directly. Invalid api-key contexts will no longer be nulled here, and org scoping from api-key metadata is not applied.</violation>
</file>
<file name="packages/auth/src/server.ts">
<violation number="1" location="packages/auth/src/server.ts:609">
P2: These two independent DB queries (`apikeys.findFirst` and `members.findMany`) run sequentially but have no data dependency. Parallelize them with `Promise.all` to cut per-request latency on every API-key-authenticated call.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/auth/src/server.ts">
<violation number="1" location="packages/auth/src/server.ts:230">
P2: The API-key org resolution logic (metadata lookup → membership check → org narrowing) is duplicated nearly verbatim between `definePayload` and `customSession`. Extract it into a shared helper like `resolveApiKeyOrgBinding(db, sessionId, userId)` returning `{ boundOrganizationId, boundMembership }` — the membership query can select both `userId` and `role` to satisfy both callers. This avoids drift if the metadata format or membership check changes.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // `ctx.organizationIds.includes(input.organizationId)` check. | ||
| // Matches the narrowing `customSession` does for the | ||
| // protectedProcedure path. | ||
| if (session.token?.startsWith("sk_live_") && session.id) { |
Contributor
There was a problem hiding this comment.
P2: The API-key org resolution logic (metadata lookup → membership check → org narrowing) is duplicated nearly verbatim between definePayload and customSession. Extract it into a shared helper like resolveApiKeyOrgBinding(db, sessionId, userId) returning { boundOrganizationId, boundMembership } — the membership query can select both userId and role to satisfy both callers. This avoids drift if the metadata format or membership check changes.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/auth/src/server.ts, line 230:
<comment>The API-key org resolution logic (metadata lookup → membership check → org narrowing) is duplicated nearly verbatim between `definePayload` and `customSession`. Extract it into a shared helper like `resolveApiKeyOrgBinding(db, sessionId, userId)` returning `{ boundOrganizationId, boundMembership }` — the membership query can select both `userId` and `role` to satisfy both callers. This avoids drift if the metadata format or membership check changes.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.) </comment>
<file context>
@@ -215,10 +215,46 @@ export const auth = betterAuth({
+ // `ctx.organizationIds.includes(input.organizationId)` check.
+ // Matches the narrowing `customSession` does for the
+ // protectedProcedure path.
+ if (session.token?.startsWith("sk_live_") && session.id) {
+ const keyRow = await db.query.apikeys.findFirst({
+ where: eq(authSchema.apikeys.id, session.id),
</file context>
…d org
Two additions on top of the helper-in-context `resolveApiKey` change:
1. Top-level `hooks.before` matcher that rejects any
`POST /api-key/update` request whose body contains a `metadata`
field. Closes the pivot where an attacker holding a stolen
`sk_live_*` key could call `/api-key/update` on that same key and
rewrite `metadata.organizationId` — which is the authority for the
org binding `resolveApiKey` enforces — to any value, bypassing the
strict check. Other update fields (name, enabled, expiresIn) remain
mutable. No codebase callers of `auth.api.updateApiKey` and no UI
surface for metadata updates.
2. `jwt.definePayload` detects api-key-synthesized sessions via
`session.token?.startsWith("sk_live_") && session.id` and, for
those, emits `organizationIds: [boundOrg]` in the JWT claim instead
of the full user membership list. This closes the `jwtProcedure`
Pattern B pivot: routes like `v2-workspace.create`,
`v2-project.create`, and `device.create` read `ctx.organizationIds`
from the JWT payload and gate input via
`ctx.organizationIds.includes(input.organizationId)`. Without the
narrowing, a JWT minted from an api key by the host service would
carry every org the owner is a member of.
Verified end-to-end against localhost:4521 with a Superset-bound
`sk_live_*` key whose owner is a member of three orgs:
- `GET /api/auth/token` → JWT payload contains
`organizationIds: [superset-uuid]` only, not all three.
- `v2Workspace.create` with `input.organizationId = kiets-org-uuid`
→ 403 "Not a member of this organization".
- `v2Workspace.create` with `input.organizationId = superset-uuid`
→ gets past the membership check (fails later at project lookup
with a fake projectId, the expected "authorized but no data" mode).
- `POST /api-key/update` with `{ metadata: {...} }` → 400 with the
lockdown message; with `{ name }` or `{ enabled }` → 200.
Not closed by this change (deferred follow-up):
`protectedProcedure` Pattern B routes that call
`verifyOrgMembership(ctx.session.user.id, input.organizationId)`
directly against the db (integration/linear, integration/github,
organization.{removeMember,leave,updateMemberRole,...}). Those read
memberships from the db, not from the session, so narrowing the
session doesn't constrain them. An api-key-authed request can still
reach them for any org the owner belongs to. Closing this requires
either a session marker ("this came from an api key") that
`verifyOrgMembership` can check, or a procedure-base refactor.
saddlepaddle
force-pushed
the
satya-patel/api-key-headless-cli
branch
from
32ab788 to
a27eaf9
Compare
After `node-gyp rebuild` produces `build/Release/pty.node`, the staging tree still contains `obj.target/` (compiled .o files), `Makefile`, `config.gypi`, `.deps/`, and `node_addon_api_except.stamp` alongside the binding. Only `pty.node` is needed at runtime; the rest ships ~a few MB of dead weight in the tarball. Fix: after verifying the binding exists, read its bytes, wipe `build/` entirely, then recreate `build/Release/` with just `pty.node`. Matches how the darwin path leaves `build/` in a minimal state (there, by deleting it entirely and falling back to the in-package prebuilds). Verified against a linux-x64 build in a sandbox: - before: `build/` contains Release/pty.node + obj.target/ + Makefile + config.gypi + .deps/ + node_addon_api_except.stamp - after: `build/` contains only `Release/pty.node` (68K total) - tar listing shows exactly 3 entries under node-pty/build/ Also tighten the log line to mention the toolchain dependency (python3 + gcc) so first-time linux contributors get a clearer hint if node-gyp rebuild fails for environmental reasons. Addresses Greptile P2 review comments on PR #3493.
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
End-to-end fix for headless api-key auth on Linux. Four bugs total, three client-side, one server-side. The server-side fix has been refactored from the original "helper called from tRPC context" shape into a single branch inside
customSession, which is the right architectural location and saves DB round-trips.Scale (for context on blast radius)
metadata.organizationIdafter the one-time backfill that ships with this PR (367 already had it, 23 legacy keys backfilled to the owner's oldest membership)auth.api.updateApiKey— the desktop UI only does create + delete, so the metadata lockdown introduced here breaks no existing pathThe bugs
1. CLI sent the wrong header shape for api keys (client)
packages/cli/src/lib/api-client.tssent every bearer asAuthorization: Bearer <token>. Forsk_live_*keys this returns 401 from better-auth — itsapiKey()plugin requiresx-api-key: <token>. So even though--api-key/SUPERSET_API_KEYflows existed on the CLI surface, they never actually worked end-to-end.Fix: conditional header based on token prefix.
2. Host service's JWT exchange had the same bug (client)
packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.tscalls/api/auth/tokento mint a short-lived JWT from the session token it receives from the CLI. Same header-shape bug → even if the CLI bootstrapped correctly, the host service would fail the JWT exchange, the relay tunnel would never connect, andsuperset host startwould time out at the 10s health check with no visible error (daemon-mode stdio is discarded).Same fix.
3.
build-dist.tsproduced a broken linux-x64 bundle (client)node-ptyships in-package N-API prebuilds fordarwin-arm64,darwin-x64,win32-arm64, andwin32-x64— but not linux-x64. The existingfixNativeBinariesForNodelogic deleted the staged copy'sbuild/Release/on the assumption thatbindingswould fall back toprebuilds/<target>/pty.node. On linux that target doesn't exist, so every invocation of the shipped bundle crashed at module load:Fix: on
linux-x64, runnode-gyp rebuildin the stagednode-ptydirectory.node-ptyusesnode-addon-apiso the resulting binding is ABI-stable across Node versions. Also dropped an over-strict existence check forspawn-helper(that target is only built on macOS perbinding.gyp).4. Server didn't read
metadata.organizationIdfrom api keys (server)apiKeyRouter.createstores the intended org inmetadata.organizationId, butcustomSessioninpackages/auth/src/server.tswas runningresolveSessionOrganizationStatefor every session (api-key or OAuth) and ignoring the metadata entirely. For api-key sessions the resolver also generated wasted DB I/O — it tried toUPDATE auth.sessionsbysession.id, which for api-key-synthesized sessions equals anapikeys.idwith no row in the sessions table, so the UPDATE matched 0 rows and fell through to a no-result SELECT. The end state wasactiveOrganizationId: null, then the original PR's tRPC-context helper would do a secondverifyApiKeycall to override it.Fix: branch inside
customSessionfor api-key-synthesized sessions, detected viasession.token?.startsWith("sk_live_") && session.id. The branch:apikeysby primary key (session.id === apikey.id) — single indexed read, noverifyApiKeyround-trip.organizationIdsarray for the enriched session.activeOrganizationIdtometadata.organizationIdif the user is still a member, elsenull.session.plankeeps working.The OAuth path is untouched — the branch is
if (isApiKeySession) { … return …; }and the existingresolveSessionOrganizationStateflow runs for everything else.Cost accounting per api-key tRPC request:
The remaining write is better-auth's own
lastRequestupdate duringvalidateApiKey, which the plugin does unconditionally (withrateLimit.enabled: falseit still returns{ update: { lastRequest: now } }per@better-auth/api-key/dist/index.mjs:1595). Eliminating it would require monkey-patching the plugin and is out of scope.Side effect:
apps/api/src/app/api/proxy/linear-image/route.tscallsauth.api.getSessiondirectly without the tRPC context wrapper, so under the original PR shape it was still silently resolvingactiveOrganizationIdtonullfor any api-key request. With the fix incustomSession, that route gets the correct org for free.4b. API key
metadata.organizationIdwas freely mutable (server)While auditing the strictness of the new binding, verified the actual exposure against the plugin source (
@better-auth/api-key/dist/index.mjs:1487+): thePOST /api-key/updateroute accepts ametadatafield on the request body with no validation that the neworganizationIdis a real org or one the caller is a member of.getSessionFromCtxhonors api-key sessions, so an attacker holding a stolensk_live_*key could call/api-key/updateon that same key and rewritemetadata.organizationIdto any value, bypassing the strict binding entirely.Fix: top-level
hooks.beforematcher onbetterAuth({...})that throwsBAD_REQUESTwheneverPOST /api-key/updatecarries ametadatafield. Net effect: api key metadata is mint-immutable. To rebind a key to a different org, delete it and create a new one.Why this is safe to ship without breaking anything:
grep -RIn "updateApiKey\|api-key/update" packages appsreturns zero hits. We never callauth.api.updateApiKeyserver-side.delete(ApiKeysSettings.tsx:100) andcreate(ApiKeysSettings.tsx:74).name,enabled,expiresIn) remain mutable. Onlymetadatais locked.hooksconfig to merge with — greenfield addition.Membership enforcement on a key whose org you've left
When a user is removed from an org their api key was minted for, the runtime check sets
activeOrganizationIdtonulland any org-scoped procedure throwsFORBIDDENviarequireActiveOrgId(packages/trpc/src/router/utils/active-org.ts:11–13). No api key rows are deleted or disabled. If the user is re-added to the org, the key starts working again with no admin action. Same security outcome as deletion at the auth layer, fully reversible, no destructive cleanup.Backfill
Before this PR, 23 of 390 keys had no
metadata.organizationId(legacy keys minted beforeapiKeyRouter.createstarted populating metadata). Under the new strict semantics those would returnFORBIDDENon every request. To avoid breakage, this PR includes a one-time SQL backfill executed against prod that setsmetadata.organizationIdon each legacy key to the owner's oldest membership (ORDER BY created_at ASC LIMIT 1):22 of the 23 affected users are in exactly one org, so the guess is unambiguous. The 23rd (one user with two orgs) gets a best-effort guess; if it's wrong, the runtime check in
customSessioncleanly returnsFORBIDDENand the user re-mints via the existing UI — strictly better than today's silent newest-membership misrouting. Backfill executed against prod as part of this PR; verified 390/390 keys now have validmetadata.organizationId.Verification
Manual end-to-end against the existing sandbox + prod API:
git clone github.com/superset-sh/superset && bun installbun run packages/cli/scripts/build-dist.ts --target=linux-x64→ produces a working tarball~/.superset/bin, add to PATHexport SUPERSET_API_KEY=sk_live_...superset auth check→ signs in, reports the correct org once this branch is deployed (the org whose UUID is inmetadata.organizationId)superset host start --daemon --port 4879→Host service running on port 4879 (pid N) · Connected to relaysuperset host status→<org>: running (pid N){"status":"ok"}Before this PR each of the four steps above failed at a different layer: CLI 401 (bug 1), host-service JWT 401 (bug 2),
pty.nodemissing on linux (bug 3), wrong org reported onauth check(bug 4). After, the full chain works.Test plan
bun run typecheckclean inapps/api,packages/cli,packages/host-service,packages/authbun run lint:fixcleanbun run packages/cli/scripts/build-dist.ts --target=linux-x64succeeds end-to-end including the node-pty rebuildsuperset host starton linux-x64 via api key against prod API (bugs 1–3, verified in a fresh sandbox)metadata.organizationIdafter this PRsuperset auth checkagainst prod withsk_live_*key whosemetadata.organizationIdis a non-default org — should report the key's org, not the defaultPOST /api/auth/api-key/updatewithbody: { keyId, metadata: { organizationId: <other> } }must returnBAD_REQUEST.body: { keyId, name: "renamed" }must succeed.darwin-arm64build still produces a loadablepty.node(only linux-x64 path verified here)FORBIDDENimmediately, with no automated cleanup; reversible if they're re-added)Commits
fix(cli): make headless api-key auth work on linux— bugs 1, 2, 3 (CLI x-api-key header, host-service JWT exchange header, linux node-pty build)fix(api): resolve api-key active org from metadata, enforce membership— original helper-in-context approach for bug 4 (superseded by commit 3)refactor(auth): hoist api-key org binding into customSession + lock metadata— moves the bug-4 fix intocustomSession, eliminates redundant DB work, adds the metadata-immutability lockdown for the security pivotThe intermediate commit 2 is left in history because it's a working-state checkpoint — the refactor in commit 3 is easier to review as a delta against it. Squash before merge if preferred.
Not in this PR (deferred)
apiKeyRouter.createto accept an explicitorganizationIdinstead of pulling from the user's current active session (the hidden-state bug that makes it easy to mint a key for the wrong org without realizing it). Backward-compatible addition; can ship separately alongside a dashboard UI change (org picker at mint time).Summary by CodeRabbit
New Features
Bug Fixes
Chores