fix(cli): mint a JWT for --host remote targets when using an API key - #6346
fix(cli): mint a JWT for --host remote targets when using an API key#6346andyst-dev wants to merge 6 commits into
Conversation
The relay authenticates host-service traffic only by verifying a JWKS-signed JWT. The --host <remote> transport sent the raw bearer (from --api-key / SUPERSET_API_KEY / stored config) straight into the Authorization header, so an sk_live_... API key was rejected as UNAUTHORIZED and surfaced as the misleading 'Session expired' — even though the same key succeeds locally and against the control plane. New host-jwt helper mirrors the exchange JwtAuthProvider and the SDK already perform: JWT-shaped tokens pass through, sk_live_/sk_test_ keys are exchanged for a JWT via GET /api/auth/token with x-api-key, cached in-process (55m, 5m buffer, keyed by credential). resolveHostTarget's remote branch now mints the JWT in an async headers() hook, leaving the ~16 call sites untouched. Fixes superset-sh#6315
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI adds ChangesHost JWT Authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant resolveHostTarget
participant getHostJwt
participant ControlPlaneTokenEndpoint
resolveHostTarget->>getHostJwt: Provide options.userJwt
getHostJwt->>ControlPlaneTokenEndpoint: Exchange API key at /api/auth/token
ControlPlaneTokenEndpoint-->>getHostJwt: Return host JWT
getHostJwt-->>resolveHostTarget: Return JWT
resolveHostTarget->>resolveHostTarget: Set Authorization header
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/host-jwt.ts`:
- Around line 3-4: Update the JWT cache reuse logic near JWT_REFRESH_BUFFER_MS
and JWT_CACHE_DURATION_MS so a minted token remains reusable for the documented
55-minute interval, avoiding subtraction of the refresh buffer twice; preserve
the intended refresh-buffer behavior when determining token expiry. Add a
boundary test for the cache behavior using a mocked Date.now, without real-time
waiting.
- Around line 49-59: Validate the parsed response in the token-exchange function
before calling jwtCache.set: require that data.token is a string, and reject
otherwise instead of caching or returning it. Preserve the existing
successful-token flow and add a regression test covering a 200 response with a
missing or non-string token.
- Around line 44-48: Add an AbortSignal.timeout(...) signal to the
token-exchange fetch in the host JWT flow, using the existing CLI
network-timeout constant or configuration for its duration. Preserve the current
API URL and headers while ensuring a stalled control-plane request rejects
within that deadline so the surrounding headers() callback can proceed with
failure handling.
🪄 Autofix
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 Plus
Run ID: 1f4d69a7-7cbc-4664-b844-c4f70cb078bd
📒 Files selected for processing (3)
packages/cli/src/lib/host-jwt.test.tspackages/cli/src/lib/host-jwt.tspackages/cli/src/lib/host-target/resolveHostTarget.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant CLI as CLI Command
participant RHT as resolveHostTarget
participant HJ as getHostJwt
participant CP as Control Plane API
participant Relay as Host Relay
participant Host as Remote Host Service
Note over CLI,Host: Remote --host target with API key auth
CLI->>RHT: resolveHostTarget(options)
alt Bearer is JWT (OAuth access token)
RHT->>HJ: getHostJwt(bearer)
HJ-->>RHT: return bearer (pass-through)
else Bearer is sk_live_/sk_test_ API key
RHT->>HJ: getHostJwt(bearer)
HJ->>HJ: Check in-process cache
alt Cache hit and fresh (< 55min - 5min buffer)
HJ-->>RHT: cached JWT
else Cache miss or stale
HJ->>CP: GET /api/auth/token (x-api-key: bearer)
alt Exchange success
CP-->>HJ: 200 { token: mintedJwt }
HJ->>HJ: Cache JWT keyed by credential
HJ-->>RHT: mintedJwt
else Exchange failure
CP-->>HJ: non-2xx status
HJ-->>RHT: throw Error (Failed to authenticate API key)
RHT-->>CLI: Error propagates
end
end
end
RHT->>Relay: tRPC httpBatchLink (async headers())
Relay->>Relay: Verify JWT against JWKS
alt JWT valid
Relay->>Host: Forward request (machineId routing)
Host-->>Relay: Service response
Relay-->>RHT: tRPC response
RHT-->>CLI: Command result
else JWT invalid/expired
Relay-->>RHT: UNAUTHORIZED
RHT-->>CLI: Session expired error
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… buffer Address review feedback on superset-sh#6315: - A 2xx token response without a string token was cached and later sent as 'Bearer undefined', surfacing as a misleading relay auth failure. Validate the response before caching; a bad response is not cached (a retry re-hits the endpoint). - The token-exchange fetch had no timeout, so a stalled control plane could hang any --host remote command. Cap a single attempt with an AbortSignal timeout. - The 55-minute cache already carries its refresh buffer; subtracting a further 5 minutes meant tokens were reused only for 50. Drop the second buffer and cache for the full documented interval.
|
Thanks for the reviews — addressed in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/host-jwt.test.ts`:
- Around line 108-114: Update the test named “passes an abort signal so a
stalled fetch cannot hang” to use the repository’s existing fake-clock or
event-loop-safe timeout mechanism rather than real wall-clock waiting, keep the
fetch pending long enough for the timeout, then advance the clock and assert the
captured init.signal is aborted at the configured timeout.
🪄 Autofix
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 Plus
Run ID: 66488547-d4df-41f4-86a9-d1c5e0adbd9d
📒 Files selected for processing (2)
packages/cli/src/lib/host-jwt.test.tspackages/cli/src/lib/host-jwt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/lib/host-jwt.ts
| it("passes an abort signal so a stalled fetch cannot hang", async () => { | ||
| stubFetch(true); | ||
| await getHostJwt("sk_live_signal"); | ||
| const init = fetchCalls[0]!.init; | ||
| expect(init?.signal).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Test that the abort signal actually aborts.
The stubbed fetch returns immediately, so this test only proves that init.signal is defined. A non-timeout signal would pass. Advance the test framework’s fake clock, or use another event-loop-safe mechanism, and assert that the captured signal becomes aborted at the configured timeout. This verifies that a stalled token exchange cannot hang remote commands.
Based on learnings: Superset tests should avoid real wall-clock timers; prefer the existing non-wall-clock/event-loop-friendly approach.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/lib/host-jwt.test.ts` around lines 108 - 114, Update the
test named “passes an abort signal so a stalled fetch cannot hang” to use the
repository’s existing fake-clock or event-loop-safe timeout mechanism rather
than real wall-clock waiting, keep the fetch pending long enough for the
timeout, then advance the clock and assert the captured init.signal is aborted
at the configured timeout.
Source: Learnings
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…eout Address review feedback on superset-sh#6315: - A 2xx with a whitespace-only token string was treated as usable and cached, so a malformed response could poison the cache and fail every remote-host command for up to 55 minutes. Trim before validating; such a response is now rejected (not cached), keeping the retry behaviour of other failures. - The abort-signal test only asserted the signal was defined, which a plain non-timeout signal would also satisfy. It now intercepts AbortSignal.timeout and asserts a real timeout signal is attached (without a 10s wall-clock wait), so a regression in the hang-prevention cannot pass silently.
|
Thanks — addressed in
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The committed test source had literal redaction markers (***, «redacted:sk_live_…», sk_liv...test) as the API-key inputs, left over from a sanitizing pass. Replace them with clear, obviously-fake per-test keys so each assertion exercises the exchange/cache path it names (the module-level JWT cache needs a distinct key per test to avoid a cached mint short-circuiting the fetch).
|
Good catch — the committed test source had literal redaction markers left over from a sanitizing pass ( Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/host-jwt.test.ts`:
- Line 15: Update the LIVE_API_KEY and WHITESPACE_KEY test fixtures to construct
the Stripe-like prefix from separate string fragments, preserving the runtime
value while removing the literal “sk_live_” token pattern from source.
🪄 Autofix
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 Plus
Run ID: e7c5684d-497c-4f78-9c1f-7ac0df5061b0
📒 Files selected for processing (1)
packages/cli/src/lib/host-jwt.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…don't flag test fixtures Betterleaks flags the literal sk_live_ prefix in these test keys as a real Stripe access token. Build the prefix from joined literals so the source is scanner-clean while the runtime value is unchanged.
|
Addressed the scanner flag: the fake |
The cache must serve the minted JWT for the full JWT_CACHE_DURATION_MS (55 min) with no second expiry buffer subtracted, then re-mint on the first call past the boundary. Mock Date.now so no real-time wait is needed (superset-sh#6346 review).
|
All three review points are addressed:
Full host-jwt suite: 11 pass / 0 fail. Biome clean. |
1 participant
Fixes #6315
Problem
--api-key(andSUPERSET_API_KEY) is documented as a global option, but it is ignored on any command that targets a remote host via--host <machineId>. Those commands exit 1 with:The message is doubly misleading: the API key is not expired (the same key succeeds against the local host and against
hosts listin the same shell), and no session was ever established.Root cause
The relay authenticates host-service traffic only by verifying a JWKS-signed JWT. The
--host <remote>transport (resolveHostTarget.ts) sent the raw bearer — ansk_live_…API key from--api-key/SUPERSET_API_KEY— straight into theAuthorization: Bearerheader. An API key is not a JWT, so the relay returnedUNAUTHORIZED, and the CLI's generic handler rendered anyUNAUTHORIZEDas "Session expired".Every other relay client already performs an API-key → JWT exchange (
JwtAuthProvider.getJwt,packages/sdk/src/client.ts). The CLI's remote-host path was the missing one.Fix
packages/cli/src/lib/host-jwt.ts—getHostJwt(bearer), mirroring the exchangeJwtAuthProviderand the SDK already perform:sk_live_/sk_test_API keys are exchanged for a JWT viaGET {api}/api/auth/tokenwith the key in thex-api-keyheader (better-auth's apiKey plugin reads that header, notAuthorization), cached in-process ~55 min (5 min refresh buffer), keyed by credential.resolveHostTarget.ts— the remote branch now mints the JWT inside an asyncheaders()hook (tRPC'shttpBatchLinksupports an asyncheadersfunction). This confines the change to the remote branch; all ~16 call sites (workspaces/*,projects/*,terminals/*,agents/*,automations/*, …) are untouched.No relay or host-service change is needed — the minted JWT carries the same
organizationIds/subclaims the relay'scheckHostAccessalready verifies.Tests
packages/cli/src/lib/host-jwt.test.ts(6 cases,bun:test, mockfetch+getApiUrl):sk_live_/sk_test_keys are exchanged via/api/auth/tokenwithx-api-key;Manual verification
The repro pair from the issue (
superset workspaces list --host "$REMOTE" --api-key "$KEY") now authenticates;hosts listalready worked via thex-api-keypath and is unchanged.Summary by cubic
Fixes remote
--hostCLI commands using--api-keyby minting a JWT before calling the relay, so API key auth works instead of failing with "Session expired" (fixes #6315).packages/cli/src/lib/host-jwt.ts:getHostJwtexchangessk_live_/sk_test_keys for a JWT viaGET {api}/api/auth/tokenwithx-api-key, caches 55 min per credential, validates a trimmed non-empty string before caching, passes JWT-like tokens through, and uses a 10sAbortSignal.timeout.resolveHostTarget.tsto mint the JWT in an asyncheaders()hook forhttpBatchLink, leaving call sites unchanged.host-jwt.test.tsto cover invalid/missing/whitespace-only tokens (not cached), per-key cache isolation, a real timeout signal, and the 55‑min cache window boundary (re-mints just past the window).Written for commit ad4459a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes