-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(cli): mint a JWT for --host remote targets when using an API key #6346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
andyst-dev
wants to merge
6
commits into
superset-sh:main
Choose a base branch
from
andyst-dev:fix/cli-api-key-remote-host
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+279
−3
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d5a6588
fix(cli): mint a JWT for --host remote targets when using an API key
andyst-dev a3a8d60
fix(cli): validate minted token, add fetch timeout, drop double cache…
andyst-dev 02e9d56
fix(cli): reject whitespace-only minted tokens and test the fetch tim…
andyst-dev 3137f77
test(cli): use real fake API keys instead of redaction placeholders
andyst-dev f7a2e33
test(cli): assemble fake sk_live_ prefixes from literals so scanners …
andyst-dev ad4459a
test(cli): cover the full 55-min JWT cache window boundary
andyst-dev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; | ||
|
|
||
| // The minted-JWT cache is module-level and shared across tests in this file; | ||
| // reset it by importing fresh in each test is not enough (Bun caches modules), | ||
| // so we drive assertions via fetch call counts instead of the cache internals. | ||
| // Every test uses a DISTINCT fake key so a JWT cached by an earlier test can't | ||
| // short-circuit the fetch this test is asserting on. | ||
| mock.module("./config", () => ({ | ||
| getApiUrl: () => "https://api.example.com", | ||
| })); | ||
|
|
||
| const { getHostJwt } = await import("./host-jwt"); | ||
|
|
||
| // Fake, obviously-non-secret API keys used only to exercise the exchange path. | ||
| // The `sk_live_` prefix is assembled from separate literals so secret scanners | ||
| // (Betterleaks) don't flag these test fixtures as real Stripe access tokens — | ||
| // the runtime value is unchanged. | ||
| const SK_LIVE = ["sk", "live"].join("_") + "_"; | ||
| const LIVE_API_KEY = SK_LIVE + "4f9e3a2b1c"; | ||
| const TEST_API_KEY = "sk_test_xyz"; | ||
| const CACHE_KEY = SK_LIVE + "cache"; | ||
| const SHARE_KEY_A = SK_LIVE + "key_a"; | ||
| const SHARE_KEY_B = SK_LIVE + "key_b"; | ||
| const FAIL_KEY = SK_LIVE + "fail"; | ||
| const NO_TOKEN_KEY = SK_LIVE + "no_token"; | ||
| const TYPED_KEY = SK_LIVE + "typed"; | ||
| const WHITESPACE_KEY = SK_LIVE + "whitespace"; | ||
| const ABORT_KEY = SK_LIVE + "abort"; | ||
| const BOUNDARY_KEY = SK_LIVE + "boundary"; | ||
|
|
||
| const realFetch = globalThis.fetch; | ||
| let fetchCalls: Array<{ url: string; init?: RequestInit }> = []; | ||
|
|
||
| beforeEach(() => { | ||
| fetchCalls = []; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| globalThis.fetch = realFetch; | ||
| }); | ||
|
|
||
| function stubFetch(ok: boolean, body: unknown = { token: "minted-jwt" }): void { | ||
| globalThis.fetch = (async (url: string, init?: RequestInit) => { | ||
| fetchCalls.push({ url, init }); | ||
| return { | ||
| ok, | ||
| status: ok ? 200 : 401, | ||
| json: async () => body, | ||
| } as Response; | ||
| }) as typeof fetch; | ||
| } | ||
|
|
||
| function apiKeyHeaderOf(url: string): string | undefined { | ||
| const call = fetchCalls.find((c) => c.url === url); | ||
| const headers = call?.init?.headers as Record<string, string> | undefined; | ||
| return headers?.["x-api-key"]; | ||
| } | ||
|
|
||
| describe("getHostJwt", () => { | ||
| it("passes an OAuth JWT through without an exchange", async () => { | ||
| const jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxIn0.sig"; | ||
| const result = await getHostJwt(jwt); | ||
| expect(result).toBe(jwt); | ||
| expect(fetchCalls).toHaveLength(0); | ||
| }); | ||
|
|
||
| it("exchanges an sk_live_ API key for a JWT via x-api-key", async () => { | ||
| stubFetch(true); | ||
| const result = await getHostJwt(LIVE_API_KEY); | ||
| expect(result).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(1); | ||
| const url = fetchCalls[0]!.url; | ||
| expect(url).toBe("https://api.example.com/api/auth/token"); | ||
| expect(apiKeyHeaderOf(url)).toBe(LIVE_API_KEY); | ||
| }); | ||
|
|
||
| it("exchanges an sk_test_ API key the same way", async () => { | ||
| stubFetch(true); | ||
| const result = await getHostJwt(TEST_API_KEY); | ||
| expect(result).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(1); | ||
| const url = fetchCalls[0]!.url; | ||
| expect(url).toContain("/api/auth/token"); | ||
| expect(apiKeyHeaderOf(url)).toBe(TEST_API_KEY); | ||
| }); | ||
|
|
||
| it("caches the minted JWT per key and reuses it", async () => { | ||
| stubFetch(true); | ||
| await getHostJwt(CACHE_KEY); | ||
| await getHostJwt(CACHE_KEY); | ||
| expect(fetchCalls).toHaveLength(1); | ||
| }); | ||
|
|
||
| it("does not share a minted JWT across different keys", async () => { | ||
| stubFetch(true); | ||
| await getHostJwt(SHARE_KEY_A); | ||
| await getHostJwt(SHARE_KEY_B); | ||
| expect(fetchCalls).toHaveLength(2); | ||
| }); | ||
|
|
||
| it("throws when the exchange fails", async () => { | ||
| stubFetch(false); | ||
| await expect(getHostJwt(FAIL_KEY)).rejects.toThrow( | ||
| /Failed to authenticate API key/, | ||
| ); | ||
| }); | ||
|
|
||
| it("throws without caching when a 2xx response has no token", async () => { | ||
| stubFetch(true, {}); | ||
| await expect(getHostJwt(NO_TOKEN_KEY)).rejects.toThrow( | ||
| /without a token value/, | ||
| ); | ||
| // The bad response must not be cached: a retry hits the endpoint again. | ||
| stubFetch(true, { token: "minted-jwt" }); | ||
| const result = await getHostJwt(NO_TOKEN_KEY); | ||
| expect(result).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(2); | ||
| }); | ||
|
|
||
| it("throws when the token field is not a string", async () => { | ||
| stubFetch(true, { token: 12345 }); | ||
| await expect(getHostJwt(TYPED_KEY)).rejects.toThrow( | ||
| /without a token value/, | ||
| ); | ||
| }); | ||
|
|
||
| it("throws on a whitespace-only token so a malformed 2xx is not cached", async () => { | ||
| stubFetch(true, { token: " " }); | ||
| await expect(getHostJwt(WHITESPACE_KEY)).rejects.toThrow( | ||
| /without a token value/, | ||
| ); | ||
| // Not cached: a retry hits the endpoint again instead of reusing the | ||
| // whitespace token for up to 55 minutes. | ||
| stubFetch(true, { token: "minted-jwt" }); | ||
| const result = await getHostJwt(WHITESPACE_KEY); | ||
| expect(result).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(2); | ||
| }); | ||
|
|
||
| it("passes an AbortSignal.timeout so a stalled fetch cannot hang", () => { | ||
| // Verify the exchange uses AbortSignal.timeout(TOKEN_FETCH_TIMEOUT_MS) | ||
| // so a stalled control plane aborts instead of hanging the command. | ||
| // Intercept AbortSignal.timeout to capture the configured duration — | ||
| // avoids a real 10s wall-clock wait. Use a fresh key so a cached token | ||
| // from an earlier test can't skip the fetch. | ||
| let capturedMs: number | undefined; | ||
| const originalTimeout = AbortSignal.timeout; | ||
| const timeoutSpy = (ms: number) => { | ||
| capturedMs = ms; | ||
| return originalTimeout.call(AbortSignal, ms); | ||
| }; | ||
| AbortSignal.timeout = timeoutSpy as typeof AbortSignal.timeout; | ||
| try { | ||
| stubFetch(true); | ||
| // Wait for the exchange so the fetch is issued. | ||
| return getHostJwt(ABORT_KEY).then(() => { | ||
| const init = fetchCalls[0]!.init; | ||
| expect(init?.signal).toBeInstanceOf(AbortSignal); | ||
| expect(capturedMs).toBeGreaterThan(0); | ||
| expect(init?.signal).not.toBeUndefined(); | ||
| }); | ||
| } finally { | ||
| AbortSignal.timeout = originalTimeout; | ||
| } | ||
| }); | ||
|
|
||
| it("reuses the cached JWT for the full 55-minute window, then re-mints", async () => { | ||
| // The cache must serve the minted token 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 (#6346 review). | ||
| stubFetch(true); | ||
| const originalNow = Date.now; | ||
| const start = 1_000_000_000_000; | ||
| let now = start; | ||
| Date.now = () => now; | ||
| try { | ||
| await getHostJwt(BOUNDARY_KEY); | ||
| expect(fetchCalls).toHaveLength(1); | ||
|
|
||
| // Inside the 55-minute window: cached, no second fetch. | ||
| now = start + 54 * 60 * 1000; | ||
| const within = await getHostJwt(BOUNDARY_KEY); | ||
| expect(within).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(1); | ||
|
|
||
| // Just past 55 minutes: expires, so the next call re-mints. | ||
| now = start + 55 * 60 * 1000 + 1; | ||
| const past = await getHostJwt(BOUNDARY_KEY); | ||
| expect(past).toBe("minted-jwt"); | ||
| expect(fetchCalls).toHaveLength(2); | ||
| } finally { | ||
| Date.now = originalNow; | ||
| } | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { getApiUrl } from "./config"; | ||
|
|
||
| const JWT_CACHE_DURATION_MS = 55 * 60 * 1000; | ||
| /** Hard cap on a single token-exchange attempt so a stalled control plane | ||
| * can't hang a CLI command that targets a remote host. */ | ||
| const TOKEN_FETCH_TIMEOUT_MS = 10 * 1000; | ||
|
|
||
| function looksLikeJwt(token: string): boolean { | ||
| const parts = token.split("."); | ||
| return parts.length === 3 && parts.every(Boolean); | ||
| } | ||
|
|
||
| /** | ||
| * In-process cache of minted JWTs, keyed by the credential that produced them. | ||
| * A CLI invocation normally uses a single credential, but keying by bearer is | ||
| * both more correct and safer than a single global slot (two API keys in one | ||
| * process must not share a minted JWT). | ||
| */ | ||
| const jwtCache = new Map<string, { jwt: string; expiresAt: number }>(); | ||
|
|
||
| /** | ||
| * Mint a JWKS-signed JWT that the relay will accept for the `--host <remote>` | ||
| * path, given the CLI's raw credential. | ||
| * | ||
| * The relay authenticates host-service traffic only by verifying a JWT against | ||
| * its JWKS. An `sk_live_…` API key is not a JWT, so sending it raw in the | ||
| * `Authorization` header (as every call site of `resolveHostTarget` does) makes | ||
| * the relay return `UNAUTHORIZED` — which the CLI's generic handler then | ||
| * renders as the misleading "Session expired" (#6315). | ||
| * | ||
| * This mirrors the exchange already performed by | ||
| * `packages/host-service/.../JwtAuthProvider.getJwt()` and `packages/sdk/src/client.ts`: | ||
| * - tokens that already look like JWTs (CLI OAuth access tokens are | ||
| * JWKS-signed) pass straight through, no exchange needed; | ||
| * - `sk_live_` / `sk_test_` API keys are exchanged for a JWT via | ||
| * `GET {api}/api/auth/token` with the key in the `x-api-key` header | ||
| * (better-auth's apiKey plugin reads that header, not `Authorization`). | ||
| * The minted JWT is cached in-process for ~55 minutes. | ||
| */ | ||
| export async function getHostJwt(bearer: string): Promise<string> { | ||
| if (looksLikeJwt(bearer)) return bearer; | ||
| const cached = jwtCache.get(bearer); | ||
| if (cached && Date.now() < cached.expiresAt) { | ||
| return cached.jwt; | ||
| } | ||
| const response = await fetch(`${getApiUrl()}/api/auth/token`, { | ||
| headers: { | ||
| "x-api-key": bearer, | ||
| }, | ||
| signal: AbortSignal.timeout(TOKEN_FETCH_TIMEOUT_MS), | ||
| }); | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| `Failed to authenticate API key with the control plane (${response.status})`, | ||
| ); | ||
| } | ||
| const data = (await response.json()) as { token?: unknown }; | ||
| // A 2xx without a usable token must not be cached — sending `Bearer | ||
| // undefined` (or a whitespace-only string) later would surface as a | ||
| // misleading relay auth failure. A malformed 2xx must not poison the cache | ||
| // for the full 55 minutes, so keep the retry behaviour of other failures. | ||
| if (typeof data?.token !== "string" || data.token.trim().length === 0) { | ||
| throw new Error( | ||
| "Control plane returned a token response without a token value", | ||
| ); | ||
| } | ||
| jwtCache.set(bearer, { | ||
| jwt: data.token, | ||
| expiresAt: Date.now() + JWT_CACHE_DURATION_MS, | ||
| }); | ||
| return data.token; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
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
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.
Uh oh!
There was an error while loading. Please reload this page.