From 3f30c1ea76697982f7b41b6b2caf4f889c726091 Mon Sep 17 00:00:00 2001 From: Aleksandr Lesnenko Date: Mon, 8 Jun 2026 20:17:28 -0400 Subject: [PATCH 1/3] cli oauth --- .claude/skills/add-e2e-test/SKILL.md | 2 +- CLAUDE.md | 8 +- README.md | 37 ++-- src/commands/auth/list.test.ts | 11 +- src/commands/auth/list.ts | 52 +++-- src/commands/auth/login.ts | 274 ++++++++++++++++++----- src/commands/auth/logout.test.ts | 109 +++++++-- src/commands/auth/logout.ts | 34 ++- src/commands/auth/render.ts | 10 + src/commands/auth/status.test.ts | 18 +- src/commands/auth/status.ts | 23 +- src/commands/runtime.ts | 24 +- src/core/auth/callback-server.test.ts | 118 ++++++++++ src/core/auth/callback-server.ts | 176 +++++++++++++++ src/core/auth/credential.test.ts | 76 +++++++ src/core/auth/credential.ts | 80 +++++++ src/core/auth/oauth-login.test.ts | 231 ++++++++++++++++++++ src/core/auth/oauth-login.ts | 104 +++++++++ src/core/auth/oauth-session.test.ts | 68 ++++++ src/core/auth/oauth-session.ts | 51 +++++ src/core/auth/pkce.test.ts | 29 +++ src/core/auth/pkce.ts | 24 ++ src/core/auth/profile-record.ts | 49 ++++- src/core/auth/storage.test.ts | 190 +++++++++++++++- src/core/auth/storage.ts | 228 ++++++++++++++++--- src/core/auth/verify.ts | 12 +- src/core/config.test.ts | 208 +++++++++++++++++- src/core/config.ts | 170 +++++++++++++-- src/core/http/client.test.ts | 303 ++++++++++++++++++-------- src/core/http/client.ts | 149 +++++++------ src/core/http/fetch-capture.ts | 88 ++++++++ src/core/http/network-error.ts | 69 ++++++ src/core/http/oauth.test.ts | 269 +++++++++++++++++++++++ src/core/http/oauth.ts | 273 +++++++++++++++++++++++ src/core/url.test.ts | 169 +++++++++++--- src/core/url.ts | 38 +++- src/runtime/paginate.test.ts | 2 +- src/runtime/process.test.ts | 81 ++++++- src/runtime/process.ts | 51 +++++ tests/e2e/bootstrap-data.ts | 4 + tests/e2e/field.e2e.test.ts | 5 +- tests/e2e/oauth.e2e.test.ts | 104 +++++++++ tests/e2e/server-gate.ts | 10 + tests/e2e/setting.e2e.test.ts | 5 +- tests/e2e/setup/bootstrap.ts | 23 +- tests/e2e/setup/oauth-harness.ts | 138 ++++++++++++ tests/e2e/setup/reset.ts | 5 +- tests/e2e/transform.e2e.test.ts | 5 +- 48 files changed, 3785 insertions(+), 422 deletions(-) create mode 100644 src/core/auth/callback-server.test.ts create mode 100644 src/core/auth/callback-server.ts create mode 100644 src/core/auth/credential.test.ts create mode 100644 src/core/auth/credential.ts create mode 100644 src/core/auth/oauth-login.test.ts create mode 100644 src/core/auth/oauth-login.ts create mode 100644 src/core/auth/oauth-session.test.ts create mode 100644 src/core/auth/oauth-session.ts create mode 100644 src/core/auth/pkce.test.ts create mode 100644 src/core/auth/pkce.ts create mode 100644 src/core/http/fetch-capture.ts create mode 100644 src/core/http/network-error.ts create mode 100644 src/core/http/oauth.test.ts create mode 100644 src/core/http/oauth.ts create mode 100644 tests/e2e/oauth.e2e.test.ts create mode 100644 tests/e2e/setup/oauth-harness.ts diff --git a/.claude/skills/add-e2e-test/SKILL.md b/.claude/skills/add-e2e-test/SKILL.md index a4733f4..97abc54 100644 --- a/.claude/skills/add-e2e-test/SKILL.md +++ b/.claude/skills/add-e2e-test/SKILL.md @@ -67,7 +67,7 @@ beforeAll(async () => { ``` - The `Bootstrap` schema lives in `tests/e2e/bootstrap-data.ts`. **Never redeclare it.** If you need a new field, edit the schema there — the writer (`tests/e2e/setup/bootstrap.ts`) consumes the same type, so drift is mechanically prevented. -- The seeded `bootstrap.adminApiKey` authenticates as a synthetic api-key user (email `api-key-user-…@api-key.invalid`). For tests that need a real human admin, use `auth login --email --password` with `bootstrap.admin.email` / `bootstrap.admin.password` and **explain in the test name why** — don't paper over it. +- The seeded `bootstrap.adminApiKey` authenticates as a synthetic api-key user (email `api-key-user-…@api-key.invalid`). For tests that need a real human admin, run the in-process OAuth login harness (`tests/e2e/setup/oauth-harness.ts` — `consentingBrowser` with `bootstrap.admin`; OAuth-capable servers only, gate with `requireOAuthServer()`) and **explain in the test name why** — don't paper over it. - **Never invoke the setup wizard from a test.** That mutates global state. Bootstrap runs once per `bun run test:e2e` via `tests/e2e/setup/global-setup.ts`. - **Never hard-code an API key.** Always read from `bootstrap`. diff --git a/CLAUDE.md b/CLAUDE.md index 3ea0160..f4ffc34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,11 +30,11 @@ Metabase CLI. TypeScript ESM. citty + native `fetch` + Zod + @clack/prompts. oxl - `src/main.ts` — root citty command, lazy `subCommands`. - `src/commands/` — CLI shell only. No HTTP, no parsing, no formatting. - `src/core/` — pure logic, no CLI deps. - - `auth/` — storage + verify. + - `auth/` — credential storage + verify, plus the OAuth login flow: `credential.ts` (discriminated `Credential` union), `pkce.ts`, `callback-server.ts` (loopback redirect), `oauth-login.ts` (orchestration), `oauth-session.ts` (refresh/revoke). - `config.ts` — flag → env → stored resolver. Profile-aware (`resolveProfileName`, `resolveConfig`). All `METABASE_*` env-var reads live here. - `errors.ts` — `isNotFoundError`, `errorMessage` (Node error type guards used outside the HTTP boundary). - - `http/` — the HTTP boundary. `client.ts` wraps native `fetch` with `requestParsed(schema, path, opts)` (the ONLY typed-JSON path), `requestRaw`, `requestStream`. Retries are idempotency-aware: GET/HEAD/OPTIONS retry on retryable status codes by default; POST/PUT/PATCH/DELETE never retry on status (only on network/timeout). Callers may override via `RequestOptions.idempotent`. `errors.ts` owns the discriminated `MetabaseError` taxonomy and `toMetabaseError(unknown)`. `sanitize.ts` runs at `HttpError` construction — secret redaction is not optional. `retry.ts` is the backoff math; it is also the only `core/http/` site allowed to drive a `setTimeout`-based wait loop (via `node:timers/promises`) outside `src/runtime/poll.ts`. Nothing outside this directory may import a third-party HTTP library or call `fetch` directly; this is enforced by `tests/structure.test.ts`. - - `url.ts` — `normalizeUrl` and `originOnly`. The single permitted home for `new URL(...)` outside `src/core/http/**`; the URL helpers belong here, not at call sites. + - `http/` — the HTTP boundary. `client.ts` wraps native `fetch` with `requestParsed(schema, path, opts)` (the ONLY typed-JSON path), `requestRaw`, `requestStream`. Retries are idempotency-aware: GET/HEAD/OPTIONS retry on retryable status codes by default; POST/PUT/PATCH/DELETE never retry on status (only on network/timeout). Callers may override via `RequestOptions.idempotent`. `errors.ts` owns the discriminated `MetabaseError` taxonomy and `toMetabaseError(unknown)`. `sanitize.ts` runs at `HttpError` construction — secret redaction is not optional. `retry.ts` is the backoff math; it is also the only `core/http/` site allowed to drive a `setTimeout`-based wait loop (via `node:timers/promises`) outside `src/runtime/poll.ts`. `oauth.ts` is the OAuth protocol boundary (RFC 8414 discovery with same-origin endpoint pinning, dynamic client registration, token exchange/refresh/revocation); its schemas are protocol envelopes, not `src/domain/` resources. Nothing outside this directory may import a third-party HTTP library or call `fetch` directly; this is enforced by `tests/structure.test.ts`. + - `url.ts` — `normalizeUrl`, `displayUrl`, `assertEndpointOrigin`. The single permitted home for `new URL(...)` outside `src/core/http/**`; the URL helpers belong here, not at call sites. Base URLs may carry a subpath (`https://my.org.com/metabase`) — never reduce a stored instance URL to its origin, and always join request paths by concatenation, not `new URL(path, base)`. - `src/domain/` — one file per Metabase resource; Zod schema + inferred type co-located. See **Domain pattern**. - `src/output/` — presentation; takes typed values. - `src/runtime/` — platform glue (stdin, poll). @@ -122,7 +122,7 @@ Lives under `tests/e2e/`. The whole point is to run the **built `dist/cli.mjs`** Adding a new e2e test for command `mb `: 1. `tests/e2e/.e2e.test.ts` — drive the command via `runCli`, assert exit code, assert `--json` output through the schema imported from `src/commands//.ts` or `src/domain/.ts`. Each test gets its own config home via `mkTempConfigHome()` (or a small `makeIsolatedConfigHome` closure inside the file that pushes onto a `tempDirs` array drained in `afterEach`). -2. The seeded admin API key (`bootstrap.adminApiKey`) authenticates as a synthetic api-key user (`api-key-user-…@api-key.invalid`). For tests that need a real admin user, call `auth login` with admin email/password — but expose that need explicitly; don't paper over it. +2. The seeded admin API key (`bootstrap.adminApiKey`) authenticates as a synthetic api-key user (`api-key-user-…@api-key.invalid`). For tests that need a real admin user, run the in-process OAuth login harness (`tests/e2e/setup/oauth-harness.ts` `consentingBrowser` with `bootstrap.admin`, OAuth-capable servers only — see `oauth.e2e.test.ts`) — but expose that need explicitly; don't paper over it. The harness is also the only sanctioned home for raw `fetch` against Metabase from the oauth suite. 3. Never mutate snapshot state in tests. Snapshot/restore (`/api/testing/*`) is reserved for the bootstrap script. Adding a new field to `.bootstrap.json`: diff --git a/README.md b/README.md index 4154a3d..b3ef28c 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # metabase-cli -Command-line client for Metabase. Authenticates against an instance with an API key and stores it securely on your machine. +Command-line client for Metabase. Logs in to an instance in your browser (OAuth, Metabase v62+) or with an API key, and stores credentials securely on your machine. ## Supported Metabase versions @@ -43,25 +43,34 @@ Credentials are stored per-profile. The default profile is named `default`. Use ### `mb auth login` -Save credentials for a profile. On success the server is probed once — the rendered output shows the API-key user, role (`Admin`/`User`), and Metabase version, and the same values are cached in `/profiles.json` so later commands skip re-probing. Failure of either the auth probe (`/api/user/current`) or the server probe (`/api/session/properties`) rejects the login; an existing profile keeps its last-known-good `apiKey`/`url`/`lastProbe` and gains a `lastFailure` entry. +Log in to a Metabase instance and save the credential to a profile. Interactive login offers two methods: -| Flag | Description | -| ------------------------ | ---------------------------------------------------------- | -| `--url ` | Metabase URL. Falls back to `METABASE_URL`, then prompts. | -| `--api-key ` | API key. Visible in shell history — pipe on stdin instead. | -| `--profile `, `-p` | Profile to write to (default: `default`). | -| `--skip-verify` | Save without contacting the server (no probe, no cache). | +- **In your browser** (recommended; requires Metabase v62 or newer) — the CLI opens Metabase, you sign in with your password or SSO and approve the CLI, and a short-lived access token plus a rotating refresh token are stored. Tokens refresh automatically; you never paste a secret. +- **With an API key** — paste a key from Admin settings → Authentication → API keys. -Resolution order for the API key: `--api-key` → piped stdin → `METABASE_API_KEY` → interactive prompt. Stdin is auto-detected when not a TTY. +Against a server older than v62 the CLI detects the missing OAuth support and falls back to the API key prompt automatically. Supplying an API key (flag, env, or stdin) always skips the browser flow, so CI and scripts behave exactly as before. + +On success the server is probed once — the rendered output shows the user, role (`Admin`/`User`), and Metabase version, and the same values are cached in `/profiles.json` so later commands skip re-probing. Failure of either the auth probe (`/api/user/current`) or the server probe (`/api/session/properties`) rejects the login; an existing profile keeps its last-known-good credential and gains a `lastFailure` entry. + +| Flag | Description | +| ------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--url ` | Metabase URL, including any subpath if the instance is hosted under one (`https://my.org.com/metabase`). Falls back to `METABASE_URL`, then prompts. | +| `--api-key ` | API key. Skips the browser flow. Visible in shell history — pipe on stdin instead. | +| `--client-id ` | Pre-registered OAuth client id (only needed when dynamic client registration is disabled on the server). | +| `--profile `, `-p` | Profile to write to (default: `default`). | +| `--skip-verify` | Save without contacting the server (no probe, no cache). | + +Non-interactive (non-TTY) login requires an API key; resolution order: `--api-key` → piped stdin → `METABASE_API_KEY` (first non-empty wins). Without one, non-interactive login fails rather than prompting. ```sh +mb auth login # interactive: browser or API key echo "$MB_KEY" | mb auth login --url https://m.example.com mb auth login --url https://m.example.com < key.txt ``` ### `mb auth status` -Show whether a profile is authenticated. +Show whether a profile is authenticated. The output includes the auth method (`OAuth` or `API key`) alongside the cached user, role, and server version. ```sh mb auth status @@ -76,9 +85,9 @@ mb auth status --profile staging ### `mb auth list` -List configured authentication profiles. All profile metadata (URL, last successful probe, last failure) lives in `/profiles.json` at mode `0600`; the API key sits in the OS keychain when available, or inline in the same file when the keychain is unavailable. +List configured authentication profiles. All profile metadata (URL, auth method, last successful probe, last failure) lives in `/profiles.json` at mode `0600`; the secrets (API key, or OAuth access/refresh tokens) sit in the OS keychain when available, or inline in the same file when the keychain is unavailable. -`auth list` re-probes every profile in parallel — on success it refreshes `lastProbe` (Metabase version, token features, API-key user identity) and clears `lastFailure`; on failure it updates `lastFailure` and leaves the prior `lastProbe`/`url`/`apiKey` untouched. Rendered columns: `Profile | URL | Status | Role | Version | Last probed`. Failed rows append a one-line footer pointing at `mb auth login --profile `. +`auth list` re-probes every profile, one at a time — a probe can refresh and rewrite an expired OAuth token, so probes are serialized to avoid racing on the shared `profiles.json`. On success it refreshes `lastProbe` (Metabase version, token features, user identity) and clears `lastFailure`; on failure it updates `lastFailure` and leaves the prior `lastProbe`/`url`/credential untouched. Rendered columns: `Profile | URL | Auth | Status | Role | Version | Last probed`. Failed rows append a one-line footer pointing at `mb auth login --profile `. ```sh mb auth list @@ -91,7 +100,7 @@ mb auth list --json ### `mb auth logout` -Clear stored credentials for a profile. +Clear stored credentials for a profile. For an OAuth profile the refresh token is also revoked server-side, best-effort: local credentials are cleared first and a revocation failure only warns, so a slow or offline server never blocks the logout. ```sh mb auth logout --yes @@ -1360,7 +1369,7 @@ Exit codes: `0` success, `2` `ConfigError` (missing name, unknown name, `MB_SKIL | Variable | Effect | | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `METABASE_URL` | Default URL for `auth login` and config resolution. | -| `METABASE_API_KEY` | Default API key (overrides interactive prompt; not stored). | +| `METABASE_API_KEY` | Default API key (makes `auth login` non-interactive, skipping the browser flow; not stored). | | `METABASE_PROFILE` | Default profile when `--profile` is omitted. Falls back to `default`. | | `METABASE_VERBOSE` | When set to `1`, prints structured developer-detail JSON to stderr on failure. | | `METABASE_CLI_SKIP_PREFLIGHT` | When set to `1`, bypasses the per-command server version / token-feature preflight check. Escape hatch for patched Metabase builds; can mask real compatibility problems. | diff --git a/src/commands/auth/list.test.ts b/src/commands/auth/list.test.ts index 0c044d5..2ba8931 100644 --- a/src/commands/auth/list.test.ts +++ b/src/commands/auth/list.test.ts @@ -2,6 +2,7 @@ import { runCommand } from "citty"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { ZodType } from "zod"; +import type { Credential } from "../../core/auth/credential"; import { parseJson } from "../../runtime/json"; import type { Verification } from "../../core/auth/verify"; @@ -17,10 +18,11 @@ vi.mock("@napi-rs/keyring", async () => { }); vi.mock("../../core/auth/verify", () => ({ - verifyAndProbe: async (url: string, apiKey: string): Promise => { - const result = hoisted.verify.results.get(apiKey); + verifyAndProbe: async (url: string, credential: Credential): Promise => { + const key = credential.kind === "apiKey" ? credential.apiKey : credential.accessToken; + const result = hoisted.verify.results.get(key); if (result === undefined) { - throw new Error(`no verifyAndProbe result configured for apiKey "${apiKey}"`); + throw new Error(`no verifyAndProbe result configured for credential "${key}"`); } return result; }, @@ -108,7 +110,8 @@ describe("auth list command", () => { expect(envelope.returned).toBe(2); expect(envelope.data.map((entry) => entry.profile)).toEqual(["staging", "prod"]); expect(envelope.data.every((entry) => entry.status === "ok")).toBe(true); - expect(envelope.data[0]?.url).toBe("https://staging.example.com"); + // The subpath survives (instances hosted under a path stay distinguishable); query is dropped. + expect(envelope.data[0]?.url).toBe("https://staging.example.com/path"); expect(envelope.data[0]?.version).toEqual({ tag: "v0.58.7", major: 58, diff --git a/src/commands/auth/list.ts b/src/commands/auth/list.ts index 881437c..ff73c5e 100644 --- a/src/commands/auth/list.ts +++ b/src/commands/auth/list.ts @@ -2,15 +2,18 @@ import { z } from "zod"; import { listProfileRecords, - readProfile, + resolveRecordCredential, writeProbeFailure, writeProbeResult, } from "../../core/auth/storage"; import { verifyAndProbe, type VerifyFailure } from "../../core/auth/verify"; -import { originOnly } from "../../core/url"; +import { createCredentialRefresher } from "../../core/config"; +import { displayUrl } from "../../core/url"; import type { ServerInfo } from "../../core/version/probe"; import { ProbedUser, + profileAuthMethod, + ProfileAuthMethod, ProfileLastFailure, ProfileLastProbe, type ProfileRecord, @@ -22,7 +25,7 @@ import { renderList } from "../../output/render"; import { listEnvelopeSchema, wrapList } from "../../output/types"; import { ParsedVersionSchema } from "../../core/version/tag"; import { outputFlags } from "../flags"; -import { renderTimestamp, renderUserRole, renderVersionTag } from "./render"; +import { renderAuthMethod, renderTimestamp, renderUserRole, renderVersionTag } from "./render"; import { defineMetabaseCommand } from "../runtime"; const AuthProfileStatus = z.enum([ @@ -37,6 +40,7 @@ export type AuthProfileStatusValue = z.infer; export const AuthProfile = z.object({ profile: z.string(), url: z.string(), + method: ProfileAuthMethod, authenticated: z.boolean(), status: AuthProfileStatus, user: ProbedUser.nullable(), @@ -62,6 +66,7 @@ const authProfileView: ResourceView = { tableColumns: [ { key: "profile", label: "Profile" }, { key: "url", label: "URL" }, + { key: "method", label: "Auth", format: (value) => renderAuthMethod(value) }, { key: "status", label: "Status", format: (value) => renderStatus(value) }, { key: "user", label: "Role", format: (value) => renderUserRole(value) }, { key: "version", label: "Version", format: (value) => renderVersionTag(value) }, @@ -82,10 +87,11 @@ export default defineMetabaseCommand({ examples: ["mb auth list", "mb auth list --json"], async run({ ctx }) { const records = await listProfileRecords(); - const verifications = await Promise.all(records.map(verifyOne)); + // Verify and persist one profile at a time: a verify can refresh+rewrite an expired OAuth + // token, so serializing avoids two profiles racing on the shared profiles.json. const items: AuthProfileJson[] = []; - for (const entry of verifications) { - items.push(await persistAndProject(entry)); + for (const record of records) { + items.push(await persistAndProject(await verifyOne(record))); } renderList(wrapList(items), authProfileView, ctx); @@ -123,19 +129,27 @@ interface FailureOutcome { type VerificationOutcome = NoCredsOutcome | SuccessOutcome | FailureOutcome; async function verifyOne(record: ProfileRecord): Promise { - const profile = await readProfile(record.name); - if (profile === null) { + // Resolve from the record already in hand (listProfileRecords loaded it) rather than re-reading + // and re-parsing the whole profiles.json once per profile. + const resolved = resolveRecordCredential(record); + if (resolved === null) { return { record, url: record.url, verification: { kind: "no-creds" } }; } - const result = await verifyAndProbe(profile.url, profile.apiKey); + // Pass a refresher so an expired-but-refreshable OAuth profile self-heals on the 401 probe + // instead of being reported as auth-failed. + const result = await verifyAndProbe( + resolved.url, + resolved.credential, + createCredentialRefresher(record.name), + ); if (result.ok) { return { record, - url: profile.url, + url: resolved.url, verification: { kind: "success", user: result.user, server: result.server }, }; } - return { record, url: profile.url, verification: { kind: "failure", failure: result } }; + return { record, url: resolved.url, verification: { kind: "failure", failure: result } }; } async function persistAndProject(entry: VerificationEntry): Promise { @@ -151,7 +165,7 @@ async function persistAndProject(entry: VerificationEntry): Promise = { }; export default defineMetabaseCommand({ - meta: { name: "login", description: "Set Metabase credentials for a profile" }, + meta: { name: "login", description: "Log in to a Metabase instance for a profile" }, details: - "API-key auth only. The key is read from --api-key, $METABASE_API_KEY, or piped stdin (first non-empty wins); the URL from --url or $METABASE_URL. Both are prompted when stdin is a TTY.", + "Interactive login offers browser OAuth (recommended; Metabase v62+) or an API key — older servers fall back to the API key prompt automatically. Browser login opens Metabase, you sign in (password or SSO) and approve, and the CLI stores a refreshing access token. For CI/non-interactive use, supply an API key via --api-key, piped stdin, or $METABASE_API_KEY (first non-empty wins); any of these skips the browser flow, even on a TTY. The URL comes from --url or $METABASE_URL, prompted when stdin is a TTY.", capabilities: { minVersion: 58 }, args: { ...outputFlags, ...profileFlag, ...connectionFlags, + clientId: { + type: "string", + description: "Pre-registered OAuth client id (when dynamic registration is disabled)", + alias: "client-id", + }, "skip-verify": { type: "boolean", default: false, @@ -64,7 +79,7 @@ export default defineMetabaseCommand({ }, outputSchema: LoginResult, examples: [ - "mb auth login --url https://metabase.example.com < key.txt", + "mb auth login --url https://metabase.example.com", "echo $METABASE_API_KEY | mb auth login --url https://metabase.example.com", "mb auth login --profile staging --url https://staging.example.com", ], @@ -79,59 +94,205 @@ export default defineMetabaseCommand({ } const url = await resolveUrl(args.url, env.url); - const apiKey = await resolveApiKey(args.apiKey, env.apiKey); + const apiKey = await nonInteractiveApiKey(args.apiKey, env.apiKey); - if (args["skip-verify"]) { - const location = await writeProfile({ url, apiKey }, profileName); - if (location.backend === "file") { - warn(keyringFallbackWarning(location)); - } - renderSummary( - { - profile: profileName, - url, - authenticated: false, - user: null, - version: null, - }, - loginView, - `Saved credentials for profile "${profileName}" (${url}) without verifying.`, + if (apiKey !== null) { + await completeLogin( + profileName, + url, + { kind: "apiKey", apiKey }, + args["skip-verify"], ctx, + () => writeProfile({ url, apiKey }, profileName), ); return; } - const result = await verifyAndProbe(url, apiKey); - if (!result.ok) { - await writeProbeFailure(profileName, { kind: result.kind, reason: result.message }); - throw new ConfigError(formatVerifyFailureMessage(profileName, result)); + if (!process.stdin.isTTY) { + throw new ConfigError( + "interactive login requires a TTY; pass --api-key or set METABASE_API_KEY for non-interactive login", + ); } - const location = await writeProfile({ url, apiKey }, profileName); - if (location.backend === "file") { - warn(keyringFallbackWarning(location)); + const metadata = await probeOAuthSupport(url); + const method = await chooseLoginMethod(metadata, args.clientId); + + if (method === "apiKey") { + const promptedKey = await promptForApiKey(); + await completeLogin( + profileName, + url, + { kind: "apiKey", apiKey: promptedKey }, + args["skip-verify"], + ctx, + () => writeProfile({ url, apiKey: promptedKey }, profileName), + ); + return; } - await writeProbeResult(profileName, { user: result.user, server: result.server }); - const who = renderUserName(result.user); - const role = renderUserRole(result.user); - const versionTag = renderVersionTag(result.server.version); - const serverClause = versionTag === EMPTY_CELL ? "" : ` Server ${versionTag}.`; - renderSummary( + const credential = await oauthLogin( { - profile: profileName, - url, - authenticated: true, - user: result.user, - version: result.server.version, + baseUrl: url, + ...(metadata !== null && { metadata }), + ...(args.clientId !== undefined && { clientId: args.clientId }), }, - loginView, - `Logged in to ${url} as ${who} (${role}). Saved to profile "${profileName}".${serverClause}`, - ctx, + { openBrowser, onAuthorizeUrl: announceAuthorizeUrl, now: () => Date.now() }, + ); + await completeLogin(profileName, url, credential, args["skip-verify"], ctx, () => + writeOAuthProfile(url, credential, profileName), ); }, }); +type LoginMethod = "oauth" | "apiKey"; + +// Reaching the server but finding no OAuth authorization server (pre-v62) degrades to the API key +// prompt; not reaching it at all is an error worth stopping on before any credential is collected. +async function probeOAuthSupport(url: string): Promise { + try { + return await tryDiscoverMetadata(url); + } catch (error) { + if (error instanceof ConfigError) { + throw error; + } + throw new ConfigError(`could not reach ${url}: ${errorMessage(error)}`); + } +} + +async function chooseLoginMethod( + metadata: OAuthServerMetadata | null, + clientId: string | undefined, +): Promise { + if (metadata === null) { + if (clientId !== undefined) { + throw new ConfigError( + "--client-id was given but this Metabase does not support OAuth login (requires Metabase v62 or newer)", + ); + } + warn( + "This Metabase does not support browser login (requires Metabase v62 or newer); using an API key instead.", + ); + return "apiKey"; + } + if (clientId !== undefined) { + return "oauth"; + } + return promptSelect({ + message: "How do you want to log in?", + choices: [ + { + value: "oauth", + label: "In your browser (recommended)", + hint: "sign in to Metabase with password or SSO and approve the CLI", + }, + { + value: "apiKey", + label: "With an API key", + hint: "paste a key from Admin settings → Authentication → API keys", + }, + ], + initialValue: "oauth", + }); +} + +async function promptForApiKey(): Promise { + return promptPassword({ + message: "API key", + mask: "•", + validate: (input) => (input ? undefined : "API key is required"), + }); +} + +type PersistCredential = () => Promise; + +async function completeLogin( + profileName: string, + url: string, + credential: Credential, + skipVerify: boolean, + ctx: CommonContext, + persist: PersistCredential, +): Promise { + if (skipVerify) { + await persistWithWarning(persist); + renderSummary( + { profile: profileName, url, authenticated: false, user: null, version: null }, + loginView, + `Saved credentials for profile "${profileName}" (${url}) without verifying.`, + ctx, + ); + return; + } + + const result = await verifyAndProbe(url, credential); + if (!result.ok) { + await writeProbeFailure(profileName, { kind: result.kind, reason: result.message }); + await revokeUnsavedOAuthCredential(url, credential); + throw new ConfigError(formatVerifyFailureMessage(profileName, result)); + } + + await persistWithWarning(persist); + await writeProbeResult(profileName, { user: result.user, server: result.server }); + + const who = renderUserName(result.user); + const role = renderUserRole(result.user); + const versionTag = renderVersionTag(result.server.version); + const serverClause = versionTag === EMPTY_CELL ? "" : ` Server ${versionTag}.`; + renderSummary( + { + profile: profileName, + url, + authenticated: true, + user: result.user, + version: result.server.version, + }, + loginView, + `Logged in to ${url} as ${who} (${role}). Saved to profile "${profileName}".${serverClause}`, + ctx, + ); +} + +// The browser consent already minted live tokens; failing verify means they will never be saved, +// so release the grant server-side rather than leaving it registered with no holder. Best-effort, +// mirroring logout — the verify failure is what the user needs to see. +async function revokeUnsavedOAuthCredential(url: string, credential: Credential): Promise { + if (credential.kind !== "oauth") { + return; + } + try { + const revoked = await revokeOAuthCredential(url, credential); + if (!revoked) { + warn( + "server does not advertise a revocation endpoint; the unsaved tokens remain valid until they expire", + ); + } + } catch (error) { + warn(`could not revoke the unsaved tokens server-side: ${errorMessage(error)}`); + } +} + +async function persistWithWarning(persist: PersistCredential): Promise { + const location = await persist(); + if (location.backend === "file") { + warn(keyringFallbackWarning(location)); + } + const residual = consumeKeychainResidualWarning(); + if (residual !== null) { + warn(residual); + } + // The location warning above already covers an at-login plaintext fallback; discard the pending + // downgrade notice so the command shell doesn't print the same thing a second time. + consumeKeyringDowngradeWarning(); +} + +function announceAuthorizeUrl(url: string, opened: boolean): void { + if (opened) { + warn(`Opening your browser to finish login. If it didn't open, visit:\n ${url}`); + return; + } + warn(`Open this URL in your browser to finish login:\n ${url}`); +} + function formatVerifyFailureMessage(profileName: string, failure: VerifyFailure): string { const which = failure.which === "user" ? "/api/user/current" : "/api/session/properties"; return `verification failed (${which}): ${failure.message} — credentials were not saved for profile "${profileName}"`; @@ -165,18 +326,26 @@ async function resolveUrl(flagUrl: string | undefined, envUrl: string | null): P return promptForUrl(); } -async function resolveApiKey(flagKey: string | undefined, envKey: string | null): Promise { +async function nonInteractiveApiKey( + flagKey: string | undefined, + envKey: string | null, +): Promise { if (flagKey) { return flagKey; } - const piped = (await readInput({ required: false })).trim(); - if (piped) { - return piped; + if (!process.stdin.isTTY) { + const piped = (await readInput({ required: false })).trim(); + if (piped) { + return piped; + } } if (envKey) { + if (process.stdin.isTTY) { + warn("using the API key from $METABASE_API_KEY; unset it to choose browser login"); + } return envKey; } - return promptForApiKey(); + return null; } async function promptForUrl(): Promise { @@ -202,16 +371,3 @@ async function promptForUrl(): Promise { }); return normalizeUrl(value); } - -async function promptForApiKey(): Promise { - if (!process.stdin.isTTY) { - throw new ConfigError( - "--api-key, piped stdin, or METABASE_API_KEY required when stdin is not a TTY", - ); - } - return promptPassword({ - message: "API key", - mask: "•", - validate: (input) => (input ? undefined : "API key is required"), - }); -} diff --git a/src/commands/auth/logout.test.ts b/src/commands/auth/logout.test.ts index 4495277..6c7a0bf 100644 --- a/src/commands/auth/logout.test.ts +++ b/src/commands/auth/logout.test.ts @@ -12,14 +12,42 @@ vi.mock("@napi-rs/keyring", async () => { }); import logoutCommand from "./logout"; +import type { OAuthCredential } from "../../core/auth/credential"; import { - readProfile, - readProfileRecord, - writeProbeFailure, - writeProfile, + captureFetch, + jsonResponse, + type CapturedFetchCall, + type FetchScript, +} from "../../core/http/fetch-capture"; +import type { OAuthServerMetadata } from "../../core/http/oauth"; +import { + KEYCHAIN_RESIDUAL_NOTICE, + readProfileCredential, + writeOAuthProfile, } from "../../core/auth/storage"; import { setupTempConfigHome, type TempConfigHome } from "../../core/auth/temp-config-home"; +const OAUTH_CRED: OAuthCredential = { + kind: "oauth", + accessToken: "acc-1", + refreshToken: "ref-1", + expiresAt: "2099-01-01T00:00:00.000Z", + clientId: "client-1", +}; + +const METADATA: OAuthServerMetadata = { + issuer: "https://m.example.com", + authorization_endpoint: "https://m.example.com/oauth/authorize", + token_endpoint: "https://m.example.com/oauth/token", + revocation_endpoint: "https://m.example.com/oauth/revoke", +}; + +function installFetch(script: FetchScript): CapturedFetchCall[] { + const capture = captureFetch(script); + vi.stubGlobal("fetch", capture.fetch); + return capture.calls; +} + describe("auth logout command", () => { let home: TempConfigHome; @@ -30,28 +58,75 @@ describe("auth logout command", () => { afterEach(() => { vi.restoreAllMocks(); + vi.unstubAllGlobals(); + hoisted.controls.broken = false; home.cleanup(); }); - it("--yes clears the profile", async () => { - await writeProfile({ url: "https://m.example.com", apiKey: "secret" }); - expect(await readProfile()).not.toBeNull(); + // Plain api-key logout (clear + cleared:false + non-interactive auto-confirm) is covered against + // a real Metabase by the auth e2e suite. These unit tests cover only the OAuth-specific + // revoke/keychain behavior, which the e2e tier doesn't drive through the CLI (an OAuth login + // subprocess would need a real browser). + + it("revokes both OAuth tokens server-side (refresh first), then clears the profile", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH_CRED); + const calls = installFetch([ + jsonResponse(METADATA), + new Response("", { status: 200 }), + new Response("", { status: 200 }), + ]); + + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + await runCommand(logoutCommand, { rawArgs: ["--profile", "default", "--yes"] }); + + const revokes = calls.filter((call) => call.url === "https://m.example.com/oauth/revoke"); + expect(revokes.map((call) => call.body)).toEqual([ + new URLSearchParams({ token: "ref-1", client_id: "client-1" }).toString(), + new URLSearchParams({ token: "acc-1", client_id: "client-1" }).toString(), + ]); + expect(await readProfileCredential()).toBeNull(); + }); + it("still clears locally when server-side revocation fails", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH_CRED); + installFetch([jsonResponse(METADATA), jsonResponse({ error: "server_error" }, 500)]); + const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); vi.spyOn(process.stdout, "write").mockImplementation(() => true); + await runCommand(logoutCommand, { rawArgs: ["--profile", "default", "--yes"] }); - expect(await readProfile()).toBeNull(); + + expect(await readProfileCredential()).toBeNull(); // logout is not blocked by a revoke failure + const warned = stderr.mock.calls.map((call) => String(call[0])).join(""); + expect(warned).toContain("could not revoke tokens server-side"); }); - it("--yes removes the entry entirely, including any prior lastFailure", async () => { - await writeProfile({ url: "https://m.example.com", apiKey: "secret" }, "staging"); - await writeProbeFailure("staging", { - kind: "auth", - reason: "Invalid or unauthorized API key", - }); - expect(await readProfileRecord("staging")).not.toBeNull(); + it("warns and skips revocation when the server advertises no revocation endpoint", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH_CRED); + const { revocation_endpoint: _omit, ...withoutRevoke } = METADATA; + const calls = installFetch([jsonResponse(withoutRevoke)]); + const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + await runCommand(logoutCommand, { rawArgs: ["--profile", "default", "--yes"] }); + + expect(calls.some((call) => call.url.includes("/oauth/revoke"))).toBe(false); + expect(await readProfileCredential()).toBeNull(); + const warned = stderr.mock.calls.map((call) => String(call[0])).join(""); + expect(warned).toContain( + "server does not advertise a revocation endpoint; tokens remain valid until they expire", + ); + }); + it("warns when a keyring-backed token cannot be removed on logout", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH_CRED); // stored in the working keyring + hoisted.controls.broken = true; // vault refuses both reads and deletes now + const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); vi.spyOn(process.stdout, "write").mockImplementation(() => true); - await runCommand(logoutCommand, { rawArgs: ["--profile", "staging", "--yes"] }); - expect(await readProfileRecord("staging")).toBeNull(); + + // Keyring is unreadable, so the token can't be revoked server-side; logout still proceeds. + await runCommand(logoutCommand, { rawArgs: ["--profile", "default", "--yes"] }); + + const warned = stderr.mock.calls.map((call) => String(call[0])).join(""); + expect(warned).toContain(KEYCHAIN_RESIDUAL_NOTICE); }); }); diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 5c50814..ba440bf 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,8 +1,15 @@ import { z } from "zod"; -import { clearProfile } from "../../core/auth/storage"; +import { revokeOAuthCredential } from "../../core/auth/oauth-session"; +import { + clearProfile, + consumeKeychainResidualWarning, + readProfileCredential, +} from "../../core/auth/storage"; import { resolveProfileName } from "../../core/config"; +import { errorMessage } from "../../core/errors"; import type { ResourceView } from "../../domain/view"; +import { warn } from "../../output/notice"; import { promptConfirm } from "../../output/prompt"; import { renderSummary } from "../../output/render"; import { outputFlags, profileFlag } from "../flags"; @@ -26,7 +33,7 @@ const logoutView: ResourceView = { export default defineMetabaseCommand({ meta: { name: "logout", description: "Clear stored credentials for a profile" }, - capabilities: null, + capabilities: { minVersion: 58 }, args: { ...outputFlags, ...profileFlag, @@ -53,7 +60,30 @@ export default defineMetabaseCommand({ } } + // Read the credential before clearing so we can still revoke it afterward. + const resolved = await readProfileCredential(profileName); + const cleared = await clearProfile(profileName); + const residual = consumeKeychainResidualWarning(); + if (residual !== null) { + warn(residual); + } + + // Best-effort server-side revocation AFTER the durable local clear, so a slow/offline server + // never blocks (or hangs) the logout. A revocation failure only warns. + if (resolved !== null && resolved.credential.kind === "oauth") { + try { + const revoked = await revokeOAuthCredential(resolved.url, resolved.credential); + if (!revoked) { + warn( + "server does not advertise a revocation endpoint; tokens remain valid until they expire", + ); + } + } catch (error) { + warn(`could not revoke tokens server-side: ${errorMessage(error)}`); + } + } + const message = cleared ? `Cleared stored credentials for profile "${profileName}".` : `No stored credentials for profile "${profileName}".`; diff --git a/src/commands/auth/render.ts b/src/commands/auth/render.ts index d9ae753..21a495d 100644 --- a/src/commands/auth/render.ts +++ b/src/commands/auth/render.ts @@ -30,6 +30,16 @@ export function renderUserRole(value: unknown): string { return isAdmin ? "Admin" : "User"; } +export function renderAuthMethod(value: unknown): string { + if (value === "oauth") { + return "OAuth"; + } + if (value === "apiKey") { + return "API key"; + } + return EMPTY_CELL; +} + export function renderVersionTag(value: unknown): string { return pickString(value, "tag") ?? EMPTY_CELL; } diff --git a/src/commands/auth/status.test.ts b/src/commands/auth/status.test.ts index fcf3b31..a73a570 100644 --- a/src/commands/auth/status.test.ts +++ b/src/commands/auth/status.test.ts @@ -15,7 +15,7 @@ vi.mock("@napi-rs/keyring", async () => { }); import authStatusCommand, { AuthStatus } from "./status"; -import { writeProbeResult, writeProfile } from "../../core/auth/storage"; +import { writeOAuthProfile, writeProbeResult, writeProfile } from "../../core/auth/storage"; import { setupTempConfigHome, type TempConfigHome } from "../../core/auth/temp-config-home"; interface CapturedStdout { @@ -59,6 +59,7 @@ describe("auth status command", () => { profile: "default", present: false, url: null, + method: null, user: null, version: null, tokenFeatures: null, @@ -75,6 +76,7 @@ describe("auth status command", () => { profile: "default", present: true, url: "https://m.example.com", + method: "apiKey", user: null, version: null, tokenFeatures: null, @@ -83,6 +85,19 @@ describe("auth status command", () => { }); }); + it("reports method=oauth for a profile holding an OAuth credential", async () => { + await writeOAuthProfile("https://m.example.com", { + kind: "oauth", + accessToken: "acc", + refreshToken: "ref", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "c1", + }); + const capture = captureStdout(); + await runCommand(authStatusCommand, { rawArgs: ["--profile", "default", "--json"] }); + expect(capture.parse(AuthStatus).method).toBe("oauth"); + }); + it("surfaces the cached probe (user, version, lastProbedAt) when one exists", async () => { await writeProfile({ url: "https://m.example.com", apiKey: "secret" }); const probe = await writeProbeResult("default", { @@ -99,6 +114,7 @@ describe("auth status command", () => { profile: "default", present: true, url: "https://m.example.com", + method: "apiKey", user: { id: 42, name: "Alice", isAdmin: true }, version: { tag: "v0.58.7", major: 58, patch: 7 }, tokenFeatures: null, diff --git a/src/commands/auth/status.ts b/src/commands/auth/status.ts index 7897ebd..ce395d7 100644 --- a/src/commands/auth/status.ts +++ b/src/commands/auth/status.ts @@ -2,20 +2,32 @@ import { z } from "zod"; import { readProfileRecord } from "../../core/auth/storage"; import { resolveProfileName } from "../../core/config"; -import { originOnly } from "../../core/url"; +import { displayUrl } from "../../core/url"; import { ParsedVersionSchema } from "../../core/version/tag"; -import { ProbedUser, ProfileLastFailure } from "../../core/auth/profile-record"; +import { + ProbedUser, + profileAuthMethod, + ProfileAuthMethod, + ProfileLastFailure, +} from "../../core/auth/profile-record"; import { TokenFeatures } from "../../domain/session-properties"; import type { ResourceView } from "../../domain/view"; import { renderItem } from "../../output/render"; import { outputFlags, profileFlag } from "../flags"; import { defineMetabaseCommand } from "../runtime"; -import { renderTimestamp, renderUserName, renderUserRole, renderVersionTag } from "./render"; +import { + renderAuthMethod, + renderTimestamp, + renderUserName, + renderUserRole, + renderVersionTag, +} from "./render"; export const AuthStatus = z.object({ profile: z.string(), present: z.boolean(), url: z.string().nullable(), + method: ProfileAuthMethod.nullable(), user: ProbedUser.nullable(), version: ParsedVersionSchema.nullable(), tokenFeatures: TokenFeatures.nullable(), @@ -30,6 +42,7 @@ const authStatusView: ResourceView = { { key: "profile", label: "Profile" }, { key: "present", label: "Authenticated" }, { key: "url", label: "URL" }, + { key: "method", label: "Auth", format: (value) => renderAuthMethod(value) }, { key: "user", label: "Logged in as", format: (value) => renderUserName(value) }, { key: "user", label: "Role", format: (value) => renderUserRole(value) }, { key: "version", label: "Version", format: (value) => renderVersionTag(value) }, @@ -53,6 +66,7 @@ export default defineMetabaseCommand({ profile: profileName, present: false, url: null, + method: null, user: null, version: null, tokenFeatures: null, @@ -70,7 +84,8 @@ export default defineMetabaseCommand({ { profile: profileName, present: true, - url: originOnly(record.url), + url: displayUrl(record.url), + method: profileAuthMethod(record), user: probe?.user ?? null, version: probe?.version ?? null, tokenFeatures: probe?.tokenFeatures ?? null, diff --git a/src/commands/runtime.ts b/src/commands/runtime.ts index 30d48ce..2019e80 100644 --- a/src/commands/runtime.ts +++ b/src/commands/runtime.ts @@ -2,8 +2,13 @@ import { defineCommand } from "citty"; import type { ArgsDef, CommandDef, CommandMeta, ParsedArgs } from "citty"; import type { ZodType } from "zod"; -import { consumeLegacyStorageWarning, readProfileRecord } from "../core/auth/storage"; import { + consumeKeyringDowngradeWarning, + consumeLegacyStorageWarning, + readProfileRecord, +} from "../core/auth/storage"; +import { + createCredentialRefresher, isPreflightSkipped, resolveConfig, SKIP_PREFLIGHT_ENV, @@ -79,9 +84,10 @@ export function defineMetabaseCommand( if (cachedClient === null) { const resolved = await getResolvedConfig(); cachedClient = createClient( - { url: resolved.url, apiKey: resolved.apiKey }, + { url: resolved.url, credential: resolved.credential }, { getServerTag: async () => (await getServerInfo())?.version?.tag ?? null, + refreshCredential: createCredentialRefresher(resolved.profile), }, ); } @@ -106,7 +112,7 @@ export function defineMetabaseCommand( getServerInfo, }); } finally { - emitLegacyStorageWarningIfPending(); + emitPendingStorageWarnings(); } } catch (error) { reportError(error, reportFormat); @@ -122,10 +128,14 @@ export function defineMetabaseCommand( return cmd; } -function emitLegacyStorageWarningIfPending(): void { - const message = consumeLegacyStorageWarning(); - if (message !== null) { - warn(message); +function emitPendingStorageWarnings(): void { + const legacy = consumeLegacyStorageWarning(); + if (legacy !== null) { + warn(legacy); + } + const downgrade = consumeKeyringDowngradeWarning(); + if (downgrade !== null) { + warn(downgrade); } } diff --git a/src/core/auth/callback-server.test.ts b/src/core/auth/callback-server.test.ts new file mode 100644 index 0000000..7f4fa8a --- /dev/null +++ b/src/core/auth/callback-server.test.ts @@ -0,0 +1,118 @@ +import { assert, describe, expect, it } from "vitest"; + +import { ConfigError } from "../errors"; + +import { startCallbackServer, type CallbackServer } from "./callback-server"; + +const STATE = "the-state"; + +async function hit(redirectUri: string, query: string): Promise { + return fetch(`${redirectUri}${query}`); +} + +async function withServer(fn: (server: CallbackServer) => Promise): Promise { + const server = await startCallbackServer(STATE); + try { + await fn(server); + } finally { + server.close(); + } +} + +describe("startCallbackServer", () => { + it("exposes a 127.0.0.1 loopback redirect URI on an ephemeral port", async () => { + await withServer(async (server) => { + expect(server.redirectUri).toMatch(/^http:\/\/127\.0\.0\.1:\d+\/callback$/); + }); + }); + + it("resolves with the code and state from a successful callback", async () => { + await withServer(async (server) => { + const pending = server.waitForCallback(); + const response = await hit(server.redirectUri, `?code=the-code&state=${STATE}`); + expect(response.status).toBe(200); + expect(await pending).toEqual({ code: "the-code", state: STATE }); + }); + }); + + it("rejects when the provider redirects with an error", async () => { + await withServer(async (server) => { + const settled = server.waitForCallback().then( + (): unknown => new Error("expected rejection"), + (caught: unknown) => caught, + ); + const response = await hit( + server.redirectUri, + `?error=access_denied&error_description=User%20said%20no&state=${STATE}`, + ); + expect(response.status).toBe(400); + const error = await settled; + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe("authorization denied: User said no"); + }); + }); + + it("rejects a callback missing the code", async () => { + await withServer(async (server) => { + const settled = server.waitForCallback().then( + (): unknown => new Error("expected rejection"), + (caught: unknown) => caught, + ); + const response = await hit(server.redirectUri, `?state=${STATE}`); + expect(response.status).toBe(400); + const error = await settled; + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe("authorization callback missing code"); + }); + }); + + it("ignores a forged callback with the wrong state and resolves only on the genuine one", async () => { + await withServer(async (server) => { + const pending = server.waitForCallback(); + const forged = await hit(server.redirectUri, "?code=attacker&state=forged"); + expect(forged.status).toBe(400); + const genuine = await hit(server.redirectUri, `?code=real-code&state=${STATE}`); + expect(genuine.status).toBe(200); + expect(await pending).toEqual({ code: "real-code", state: STATE }); + }); + }); + + it("HTML-escapes the attacker-controlled error_description in the response page", async () => { + await withServer(async (server) => { + const settled = server.waitForCallback().catch((caught: unknown) => caught); + const payload = ""; + const response = await hit( + server.redirectUri, + `?error=access_denied&error_description=${encodeURIComponent(payload)}&state=${STATE}`, + ); + const body = await response.text(); + expect(body).not.toContain(payload); + expect(body).toContain("<script>alert(1)</script>"); + await settled; + }); + }); + + it("rejects with a timeout when no callback arrives within the window", async () => { + const timeoutMs = 25; + const server = await startCallbackServer(STATE, timeoutMs); + try { + const error = await server.waitForCallback().catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe(`timed out waiting for browser login after ${timeoutMs}ms`); + } finally { + server.close(); + } + }); + + it("404s requests to paths other than /callback", async () => { + await withServer(async (server) => { + const base = server.redirectUri.replace("/callback", ""); + const response = await fetch(`${base}/favicon.ico`); + expect(response.status).toBe(404); + await response.text(); + }); + }); +}); diff --git a/src/core/auth/callback-server.ts b/src/core/auth/callback-server.ts new file mode 100644 index 0000000..3cd1907 --- /dev/null +++ b/src/core/auth/callback-server.ts @@ -0,0 +1,176 @@ +import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; + +import { ConfigError } from "../errors"; + +const LOOPBACK_HOST = "127.0.0.1"; +const CALLBACK_PATH = "/callback"; +const DEFAULT_TIMEOUT_MS = 300_000; + +export interface CallbackParams { + code: string; + state: string; +} + +export interface CallbackServer { + redirectUri: string; + waitForCallback(): Promise; + close(): void; +} + +interface QueryResult { + code: string | null; + state: string | null; + error: string | null; + errorDescription: string | null; +} + +function parseCallbackQuery(rawUrl: string): QueryResult { + const queryStart = rawUrl.indexOf("?"); + const params = new URLSearchParams(queryStart === -1 ? "" : rawUrl.slice(queryStart + 1)); + return { + code: params.get("code"), + state: params.get("state"), + error: params.get("error"), + errorDescription: params.get("error_description"), + }; +} + +function isCallbackPath(rawUrl: string): boolean { + const queryStart = rawUrl.indexOf("?"); + const path = queryStart === -1 ? rawUrl : rawUrl.slice(0, queryStart); + return path === CALLBACK_PATH; +} + +// The message can carry attacker-controlled query content (error_description from the redirect), +// so everything interpolated into the page is escaped. +function escapeHtml(value: string): string { + return value + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +} + +function htmlPage(title: string, message: string): string { + return ( + '' + + escapeHtml(title) + + '' + + "

" + + escapeHtml(title) + + "

" + + escapeHtml(message) + + "

You can close this tab and return to your terminal.

" + ); +} + +function respond(res: ServerResponse, status: number, title: string, message: string): void { + // Close the socket after responding so a browser's keep-alive connection can't keep the loopback + // server (and the CLI process) alive after server.close(). + res.writeHead(status, { "content-type": "text/html; charset=utf-8", connection: "close" }); + res.end(htmlPage(title, message)); +} + +function serverPort(server: Server): number { + const address = server.address(); + if (address === null || typeof address === "string") { + throw new ConfigError("could not determine loopback callback port"); + } + const info: AddressInfo = address; + return info.port; +} + +// Bind an ephemeral loopback port and resolve once the browser redirect hits /callback with the +// expected state. RFC 8252 native-app flow: the redirect URI is http://127.0.0.1:/callback. +export function startCallbackServer( + expectedState: string, + timeoutMs = DEFAULT_TIMEOUT_MS, +): Promise { + return new Promise((resolveServer, rejectServer) => { + let settle: ((params: CallbackParams) => void) | null = null; + let fail: ((error: Error) => void) | null = null; + let outcome: CallbackParams | Error | null = null; + + const server = createServer((req: IncomingMessage, res: ServerResponse) => { + const rawUrl = req.url ?? ""; + if (!isCallbackPath(rawUrl)) { + respond(res, 404, "Not found", "Unexpected request."); + return; + } + const query = parseCallbackQuery(rawUrl); + // Validate state in the handler, before consuming the one-shot callback slot: a forged or + // stray loopback request (any local process can spray ports) must not abort the pending + // login or render a success page. Reject it and keep waiting for the genuine redirect. + if (query.state !== expectedState) { + respond(res, 400, "Login failed", "Invalid or missing state."); + return; + } + if (query.error !== null) { + const detail = query.errorDescription ?? query.error; + respond(res, 400, "Login failed", detail); + deliver(new ConfigError(`authorization denied: ${detail}`)); + return; + } + if (query.code === null) { + respond(res, 400, "Login failed", "Missing authorization code."); + deliver(new ConfigError("authorization callback missing code")); + return; + } + respond(res, 200, "Login complete", "Metabase CLI is now authorized."); + deliver({ code: query.code, state: query.state }); + }); + + function deliver(result: CallbackParams | Error): void { + if (outcome !== null) { + return; + } + outcome = result; + if (result instanceof Error) { + fail?.(result); + } else { + settle?.(result); + } + } + + const timer = setTimeout(() => { + deliver(new ConfigError(`timed out waiting for browser login after ${timeoutMs}ms`)); + }, timeoutMs); + timer.unref(); + + server.on("error", (error) => { + // Before listen resolves this fails startup; after, it aborts the pending login instead of + // stalling silently until the timeout fires. + rejectServer(error); + deliver(error); + }); + server.listen(0, LOOPBACK_HOST, () => { + const port = serverPort(server); + resolveServer({ + redirectUri: `http://${LOOPBACK_HOST}:${port}${CALLBACK_PATH}`, + waitForCallback() { + return new Promise((resolve, reject) => { + if (outcome !== null) { + if (outcome instanceof Error) { + reject(outcome); + } else { + resolve(outcome); + } + return; + } + settle = resolve; + fail = reject; + }); + }, + close() { + clearTimeout(timer); + server.close(); + // A browser's lingering keep-alive socket would otherwise hold the event loop (and the + // CLI process) open after close(). + server.closeAllConnections(); + }, + }); + }); + }); +} diff --git a/src/core/auth/credential.test.ts b/src/core/auth/credential.test.ts new file mode 100644 index 0000000..cccd4d9 --- /dev/null +++ b/src/core/auth/credential.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from "vitest"; + +import { + type ApiKeyCredential, + type OAuthCredential, + credentialAuthHeader, + credentialSecrets, + expiresAtFromNow, + isOAuthExpired, +} from "./credential"; + +const API_KEY: ApiKeyCredential = { kind: "apiKey", apiKey: "mb_secret" }; + +function oauth(overrides: Partial = {}): OAuthCredential { + return { + kind: "oauth", + accessToken: "access-tok", + refreshToken: "refresh-tok", + expiresAt: "2026-01-01T00:00:00.000Z", + clientId: "client-123", + ...overrides, + }; +} + +describe("credentialAuthHeader", () => { + it("uses the x-api-key header for an API key credential", () => { + expect(credentialAuthHeader(API_KEY)).toEqual({ name: "x-api-key", value: "mb_secret" }); + }); + + it("uses an Authorization Bearer header for an OAuth credential", () => { + expect(credentialAuthHeader(oauth())).toEqual({ + name: "authorization", + value: "Bearer access-tok", + }); + }); +}); + +describe("credentialSecrets", () => { + it("returns the API key as the only secret", () => { + expect(credentialSecrets(API_KEY)).toEqual(["mb_secret"]); + }); + + it("returns both OAuth tokens as secrets to redact", () => { + expect(credentialSecrets(oauth())).toEqual(["access-tok", "refresh-tok"]); + }); +}); + +describe("isOAuthExpired", () => { + const expiresAt = "2026-06-08T12:00:00.000Z"; + const expiryMs = Date.parse(expiresAt); + + it("is not expired well before the expiry", () => { + expect(isOAuthExpired(oauth({ expiresAt }), expiryMs - 5 * 60_000)).toBe(false); + }); + + it("treats the token as expired within the 60s skew window", () => { + expect(isOAuthExpired(oauth({ expiresAt }), expiryMs - 30_000)).toBe(true); + }); + + it("is expired at and after the expiry", () => { + expect(isOAuthExpired(oauth({ expiresAt }), expiryMs)).toBe(true); + expect(isOAuthExpired(oauth({ expiresAt }), expiryMs + 60_000)).toBe(true); + }); + + it("treats an unparseable expiry as expired (fail safe, never fail open)", () => { + expect(isOAuthExpired(oauth({ expiresAt: "not-a-date" }), expiryMs)).toBe(true); + expect(isOAuthExpired(oauth({ expiresAt: "" }), expiryMs)).toBe(true); + }); +}); + +describe("expiresAtFromNow", () => { + it("adds the lifetime in seconds to now and renders an ISO timestamp", () => { + const now = Date.parse("2026-06-08T12:00:00.000Z"); + expect(expiresAtFromNow(3600, now)).toBe("2026-06-08T13:00:00.000Z"); + }); +}); diff --git a/src/core/auth/credential.ts b/src/core/auth/credential.ts new file mode 100644 index 0000000..f30a7da --- /dev/null +++ b/src/core/auth/credential.ts @@ -0,0 +1,80 @@ +import type { OAuthTokens } from "../http/oauth"; + +export interface ApiKeyCredential { + kind: "apiKey"; + apiKey: string; +} + +export interface OAuthCredential { + kind: "oauth"; + accessToken: string; + refreshToken: string; + expiresAt: string; + clientId: string; +} + +export type Credential = ApiKeyCredential | OAuthCredential; + +// Returns a refreshed credential to retry with, or null when refresh is impossible/declined. +export type CredentialRefresher = () => Promise; + +export const API_KEY_HEADER = "x-api-key"; +export const AUTHORIZATION_HEADER = "authorization"; +const BEARER_PREFIX = "Bearer "; + +// Refresh slightly before the server-side expiry so a request never races the boundary. +const EXPIRY_SKEW_MS = 60_000; + +// Fallback access-token lifetime when the token endpoint omits `expires_in`. +export const DEFAULT_EXPIRES_IN_SECONDS = 3600; + +export interface AuthHeader { + name: string; + value: string; +} + +export function credentialAuthHeader(credential: Credential): AuthHeader { + if (credential.kind === "apiKey") { + return { name: API_KEY_HEADER, value: credential.apiKey }; + } + return { name: AUTHORIZATION_HEADER, value: BEARER_PREFIX + credential.accessToken }; +} + +export function credentialSecrets(credential: Credential): string[] { + if (credential.kind === "apiKey") { + return [credential.apiKey]; + } + return [credential.accessToken, credential.refreshToken]; +} + +export function isOAuthExpired(credential: OAuthCredential, nowMs: number): boolean { + const expiryMs = Date.parse(credential.expiresAt); + // An unparseable expiry can't be trusted — fail safe by treating it as expired so the + // credential is refreshed rather than used past an unknown lifetime. + if (Number.isNaN(expiryMs)) { + return true; + } + return expiryMs - EXPIRY_SKEW_MS <= nowMs; +} + +export function expiresAtFromNow(expiresInSeconds: number, nowMs: number): string { + return new Date(nowMs + expiresInSeconds * 1000).toISOString(); +} + +// Project a token-endpoint response onto an OAuthCredential. Shared by initial login and refresh so +// the access-token lifetime fallback lives in one place; the caller resolves the refresh token +// (required on login, retained-on-omission for refresh) and the client id. +export function oauthCredentialFromTokens( + tokens: Pick, + refreshToken: string, + clientId: string, + nowMs: number, +): OAuthCredential { + return { + kind: "oauth", + accessToken: tokens.access_token, + refreshToken, + expiresAt: expiresAtFromNow(tokens.expires_in ?? DEFAULT_EXPIRES_IN_SECONDS, nowMs), + clientId, + }; +} diff --git a/src/core/auth/oauth-login.test.ts b/src/core/auth/oauth-login.test.ts new file mode 100644 index 0000000..b3a9fe3 --- /dev/null +++ b/src/core/auth/oauth-login.test.ts @@ -0,0 +1,231 @@ +import { createHash } from "node:crypto"; + +import { afterEach, assert, describe, expect, it, vi } from "vitest"; + +import { ConfigError } from "../errors"; +import type { CodeExchange, OAuthTokens } from "../http/oauth"; + +const hoisted = vi.hoisted<{ + tokens: OAuthTokens; + metadata: OAuthServerMetadata; + registerCalls: number; + discoverCalls: number; + exchange: CodeExchange | null; +}>(() => ({ + tokens: { + access_token: "acc", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "ref", + }, + metadata: { + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: "https://mb.example.com/oauth/token", + registration_endpoint: "https://mb.example.com/oauth/register", + }, + registerCalls: 0, + discoverCalls: 0, + exchange: null, +})); + +vi.mock("../http/oauth", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + discoverMetadata: async () => { + hoisted.discoverCalls += 1; + return hoisted.metadata; + }, + registerClient: async () => { + hoisted.registerCalls += 1; + return { client_id: "client-xyz" }; + }, + exchangeCode: async (input: CodeExchange) => { + hoisted.exchange = input; + return hoisted.tokens; + }, + }; +}); + +import { OAuthServerMetadata } from "../http/oauth"; +import { oauthLogin } from "./oauth-login"; + +const DEFAULT_TOKENS: OAuthTokens = { + access_token: "acc", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "ref", +}; + +const DEFAULT_METADATA: OAuthServerMetadata = { + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: "https://mb.example.com/oauth/token", + registration_endpoint: "https://mb.example.com/oauth/register", +}; + +const NOW = Date.parse("2026-06-08T12:00:00.000Z"); + +// Simulates the browser: parses the authorize URL the CLI would open and hits the loopback redirect. +function browserDriver(): (url: string) => Promise { + return async (url: string): Promise => { + const parsed = new URL(url); + const redirectUri = parsed.searchParams.get("redirect_uri") ?? ""; + const state = parsed.searchParams.get("state") ?? ""; + await fetch(`${redirectUri}?code=test-code&state=${encodeURIComponent(state)}`); + return true; + }; +} + +// A hostile local request forges a wrong-state callback first; the genuine redirect follows. +function forgingThenGenuineBrowser(): (url: string) => Promise { + return async (url: string): Promise => { + const parsed = new URL(url); + const redirectUri = parsed.searchParams.get("redirect_uri") ?? ""; + const realState = parsed.searchParams.get("state") ?? ""; + await fetch(`${redirectUri}?code=attacker-code&state=forged`); + await fetch(`${redirectUri}?code=test-code&state=${encodeURIComponent(realState)}`); + return true; + }; +} + +describe("oauthLogin", () => { + afterEach(() => { + hoisted.tokens = { ...DEFAULT_TOKENS }; + hoisted.metadata = { ...DEFAULT_METADATA }; + hoisted.registerCalls = 0; + hoisted.discoverCalls = 0; + hoisted.exchange = null; + }); + + it("completes the PKCE loopback flow and assembles an OAuth credential", async () => { + const announced: string[] = []; + const credential = await oauthLogin( + { baseUrl: "https://mb.example.com" }, + { + openBrowser: browserDriver(), + onAuthorizeUrl: (url) => announced.push(url), + now: () => NOW, + }, + ); + expect(credential).toEqual({ + kind: "oauth", + accessToken: "acc", + refreshToken: "ref", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "client-xyz", + }); + expect(announced).toHaveLength(1); + expect(announced[0]).toContain("https://mb.example.com/oauth/authorize?"); + expect(announced[0]).toContain("code_challenge_method=S256"); + expect(announced[0]).toContain("client_id=client-xyz"); + + // The token exchange must carry the callback's code, the exact redirect_uri that was + // authorized, and the verifier whose S256 hash was sent as the code_challenge. + const authorizeParams = new URL(announced[0] ?? "").searchParams; + const redirectUri = authorizeParams.get("redirect_uri"); + assert(redirectUri !== null, "expected redirect_uri in the authorize URL"); + assert(hoisted.exchange !== null, "expected exchangeCode to be called"); + const { codeVerifier, ...exchangeRest } = hoisted.exchange; + expect(exchangeRest).toEqual({ + tokenEndpoint: "https://mb.example.com/oauth/token", + code: "test-code", + redirectUri, + clientId: "client-xyz", + }); + expect(createHash("sha256").update(codeVerifier).digest("base64url")).toBe( + authorizeParams.get("code_challenge"), + ); + }); + + it("joins authorize params with & when the endpoint already carries a query", async () => { + hoisted.metadata = { + ...DEFAULT_METADATA, + authorization_endpoint: "https://mb.example.com/oauth/authorize?tenant=t1", + }; + const announced: string[] = []; + await oauthLogin( + { baseUrl: "https://mb.example.com" }, + { + openBrowser: browserDriver(), + onAuthorizeUrl: (url) => announced.push(url), + now: () => NOW, + }, + ); + expect(announced[0]).toContain("/oauth/authorize?tenant=t1&response_type=code"); + }); + + it("uses caller-provided metadata without re-running discovery", async () => { + const credential = await oauthLogin( + { + baseUrl: "https://mb.example.com", + metadata: OAuthServerMetadata.parse(DEFAULT_METADATA), + }, + { openBrowser: browserDriver(), onAuthorizeUrl: () => undefined, now: () => NOW }, + ); + expect(credential.clientId).toBe("client-xyz"); + expect(hoisted.discoverCalls).toBe(0); + }); + + it("ignores a forged wrong-state callback and completes on the genuine redirect", async () => { + const credential = await oauthLogin( + { baseUrl: "https://mb.example.com" }, + { + openBrowser: forgingThenGenuineBrowser(), + onAuthorizeUrl: () => undefined, + now: () => NOW, + }, + ); + expect(credential).toEqual({ + kind: "oauth", + accessToken: "acc", + refreshToken: "ref", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "client-xyz", + }); + }); + + it("rejects when the token endpoint returns no refresh token", async () => { + hoisted.tokens = { + access_token: "acc", + token_type: "Bearer", + expires_in: 3600, + }; + const error = await oauthLogin( + { baseUrl: "https://mb.example.com" }, + { openBrowser: browserDriver(), onAuthorizeUrl: () => undefined, now: () => NOW }, + ).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toContain("did not return a refresh token"); + }); + + it("uses a provided client id and skips dynamic registration", async () => { + const announced: string[] = []; + const credential = await oauthLogin( + { baseUrl: "https://mb.example.com", clientId: "preset-client" }, + { + openBrowser: browserDriver(), + onAuthorizeUrl: (url) => announced.push(url), + now: () => NOW, + }, + ); + expect(credential.clientId).toBe("preset-client"); + expect(hoisted.registerCalls).toBe(0); + expect(announced[0]).toContain("client_id=preset-client"); + }); + + it("errors when dynamic registration is disabled and no client id is given", async () => { + const { registration_endpoint: _omit, ...withoutRegistration } = DEFAULT_METADATA; + hoisted.metadata = withoutRegistration; + const error = await oauthLogin( + { baseUrl: "https://mb.example.com" }, + { openBrowser: browserDriver(), onAuthorizeUrl: () => undefined, now: () => NOW }, + ).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toContain("dynamic client registration disabled"); + expect(hoisted.registerCalls).toBe(0); + }); +}); diff --git a/src/core/auth/oauth-login.ts b/src/core/auth/oauth-login.ts new file mode 100644 index 0000000..1295270 --- /dev/null +++ b/src/core/auth/oauth-login.ts @@ -0,0 +1,104 @@ +import { ConfigError } from "../errors"; +import { + discoverMetadata, + exchangeCode, + OAUTH_SCOPE, + registerClient, + type OAuthServerMetadata, +} from "../http/oauth"; + +import { startCallbackServer } from "./callback-server"; +import { oauthCredentialFromTokens, type OAuthCredential } from "./credential"; +import { generatePkce, randomState } from "./pkce"; + +const CLIENT_NAME = "Metabase CLI"; + +export interface OAuthLoginInput { + baseUrl: string; + // Discovery document already fetched by the caller (login probes it to pick the auth method); + // when omitted it is discovered here. + metadata?: OAuthServerMetadata; + clientId?: string; + timeoutMs?: number; +} + +export interface OAuthLoginDeps { + openBrowser: (url: string) => Promise; + onAuthorizeUrl: (url: string, opened: boolean) => void; + now: () => number; +} + +function buildAuthorizeUrl(authorizationEndpoint: string, params: Record): string { + // RFC 6749 §3.1 allows the authorization endpoint to carry its own query component. + const separator = authorizationEndpoint.includes("?") ? "&" : "?"; + return `${authorizationEndpoint}${separator}${new URLSearchParams(params).toString()}`; +} + +async function resolveClientId( + registrationEndpoint: string | undefined, + redirectUri: string, + provided: string | undefined, +): Promise { + if (provided !== undefined) { + return provided; + } + if (registrationEndpoint === undefined) { + throw new ConfigError( + "this Metabase has dynamic client registration disabled; pass --client-id with a pre-registered native client", + ); + } + const registered = await registerClient({ + registrationEndpoint, + redirectUri, + clientName: CLIENT_NAME, + }); + return registered.client_id; +} + +export async function oauthLogin( + input: OAuthLoginInput, + deps: OAuthLoginDeps, +): Promise { + const metadata = input.metadata ?? (await discoverMetadata(input.baseUrl)); + const pkce = generatePkce(); + const state = randomState(); + // The server validates state in-handler, so a forged callback can't consume the slot. + const server = await startCallbackServer(state, input.timeoutMs); + try { + const clientId = await resolveClientId( + metadata.registration_endpoint, + server.redirectUri, + input.clientId, + ); + const authorizeUrl = buildAuthorizeUrl(metadata.authorization_endpoint, { + response_type: "code", + client_id: clientId, + redirect_uri: server.redirectUri, + code_challenge: pkce.challenge, + code_challenge_method: "S256", + state, + scope: OAUTH_SCOPE, + }); + + const opened = await deps.openBrowser(authorizeUrl); + deps.onAuthorizeUrl(authorizeUrl, opened); + + const callback = await server.waitForCallback(); + + const tokens = await exchangeCode({ + tokenEndpoint: metadata.token_endpoint, + code: callback.code, + redirectUri: server.redirectUri, + clientId, + codeVerifier: pkce.verifier, + }); + + if (tokens.refresh_token === undefined) { + throw new ConfigError("token endpoint did not return a refresh token"); + } + + return oauthCredentialFromTokens(tokens, tokens.refresh_token, clientId, deps.now()); + } finally { + server.close(); + } +} diff --git a/src/core/auth/oauth-session.test.ts b/src/core/auth/oauth-session.test.ts new file mode 100644 index 0000000..c094369 --- /dev/null +++ b/src/core/auth/oauth-session.test.ts @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { captureFetch, jsonResponse, type FetchScript } from "../http/fetch-capture"; +import type { OAuthServerMetadata } from "../http/oauth"; + +import type { OAuthCredential } from "./credential"; +import { refreshOAuthCredential } from "./oauth-session"; + +const CREDENTIAL: OAuthCredential = { + kind: "oauth", + accessToken: "acc-1", + refreshToken: "ref-1", + expiresAt: "2026-06-08T12:00:00.000Z", + clientId: "client-1", +}; + +const METADATA: OAuthServerMetadata = { + issuer: "https://m.example.com", + authorization_endpoint: "https://m.example.com/oauth/authorize", + token_endpoint: "https://m.example.com/oauth/token", +}; + +const NOW = Date.parse("2026-06-08T12:00:00.000Z"); + +function installFetch(script: FetchScript): void { + vi.stubGlobal("fetch", captureFetch(script).fetch); +} + +describe("refreshOAuthCredential", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("adopts the rotated refresh token returned by the token endpoint", async () => { + installFetch([ + jsonResponse(METADATA), + jsonResponse({ + access_token: "acc-2", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "ref-2", + }), + ]); + const refreshed = await refreshOAuthCredential("https://m.example.com", CREDENTIAL, NOW); + expect(refreshed).toEqual({ + kind: "oauth", + accessToken: "acc-2", + refreshToken: "ref-2", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "client-1", + }); + }); + + it("retains the current refresh token when the endpoint omits one", async () => { + installFetch([ + jsonResponse(METADATA), + jsonResponse({ access_token: "acc-2", token_type: "Bearer", expires_in: 3600 }), + ]); + const refreshed = await refreshOAuthCredential("https://m.example.com", CREDENTIAL, NOW); + expect(refreshed).toEqual({ + kind: "oauth", + accessToken: "acc-2", + refreshToken: "ref-1", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "client-1", + }); + }); +}); diff --git a/src/core/auth/oauth-session.ts b/src/core/auth/oauth-session.ts new file mode 100644 index 0000000..4d73840 --- /dev/null +++ b/src/core/auth/oauth-session.ts @@ -0,0 +1,51 @@ +import { discoverMetadata, refreshTokens, revokeToken } from "../http/oauth"; + +import { oauthCredentialFromTokens, type OAuthCredential } from "./credential"; + +// Exchange the rotating refresh token for a fresh access token. The server rotates refresh tokens, +// so a new one is expected back; if the endpoint omits it we keep the current refresh token. +export async function refreshOAuthCredential( + baseUrl: string, + credential: OAuthCredential, + nowMs: number, +): Promise { + const metadata = await discoverMetadata(baseUrl); + const tokens = await refreshTokens({ + tokenEndpoint: metadata.token_endpoint, + refreshToken: credential.refreshToken, + clientId: credential.clientId, + }); + return oauthCredentialFromTokens( + tokens, + tokens.refresh_token ?? credential.refreshToken, + credential.clientId, + nowMs, + ); +} + +// Revoke both tokens server-side on logout. Metabase revokes exactly the token it is handed (no +// refresh-to-access cascade), so each must be revoked explicitly or the access token stays live +// until expiry. Refresh token goes first: if the second call fails, the long-lived grant is +// already dead. Returns false when the server advertises no revocation endpoint (nothing was +// revoked); network/protocol failures propagate to the caller. +export async function revokeOAuthCredential( + baseUrl: string, + credential: OAuthCredential, +): Promise { + const metadata = await discoverMetadata(baseUrl); + const revocationEndpoint = metadata.revocation_endpoint; + if (revocationEndpoint === undefined) { + return false; + } + await revokeToken({ + revocationEndpoint, + token: credential.refreshToken, + clientId: credential.clientId, + }); + await revokeToken({ + revocationEndpoint, + token: credential.accessToken, + clientId: credential.clientId, + }); + return true; +} diff --git a/src/core/auth/pkce.test.ts b/src/core/auth/pkce.test.ts new file mode 100644 index 0000000..742d4f0 --- /dev/null +++ b/src/core/auth/pkce.test.ts @@ -0,0 +1,29 @@ +import { createHash } from "node:crypto"; + +import { describe, expect, it } from "vitest"; + +import { generatePkce, randomState } from "./pkce"; + +function expectedChallenge(verifier: string): string { + return createHash("sha256").update(verifier).digest("base64url"); +} + +describe("generatePkce", () => { + it("produces a 43-char base64url verifier and its S256 challenge", () => { + const pkce = generatePkce(); + expect(pkce.verifier).toMatch(/^[A-Za-z0-9_-]{43}$/); + expect(pkce.challenge).toBe(expectedChallenge(pkce.verifier)); + }); + + it("produces a distinct verifier each call", () => { + expect(generatePkce().verifier).not.toBe(generatePkce().verifier); + }); +}); + +describe("randomState", () => { + it("produces a non-empty base64url string that varies per call", () => { + const state = randomState(); + expect(state).toMatch(/^[A-Za-z0-9_-]+$/); + expect(state).not.toBe(randomState()); + }); +}); diff --git a/src/core/auth/pkce.ts b/src/core/auth/pkce.ts new file mode 100644 index 0000000..8293993 --- /dev/null +++ b/src/core/auth/pkce.ts @@ -0,0 +1,24 @@ +import { createHash, randomBytes } from "node:crypto"; + +export interface Pkce { + verifier: string; + challenge: string; +} + +// RFC 7636: a 32-byte random value base64url-encodes to 43 chars, within the 43–128 range. +const VERIFIER_BYTES = 32; +const STATE_BYTES = 16; + +function base64Url(buffer: Buffer): string { + return buffer.toString("base64url"); +} + +export function generatePkce(): Pkce { + const verifier = base64Url(randomBytes(VERIFIER_BYTES)); + const challenge = base64Url(createHash("sha256").update(verifier).digest()); + return { verifier, challenge }; +} + +export function randomState(): string { + return base64Url(randomBytes(STATE_BYTES)); +} diff --git a/src/core/auth/profile-record.ts b/src/core/auth/profile-record.ts index acebf20..b4be959 100644 --- a/src/core/auth/profile-record.ts +++ b/src/core/auth/profile-record.ts @@ -28,16 +28,45 @@ export const ProfileLastFailure = z.object({ }); export type ProfileLastFailure = z.infer; -export const ProfileRecord = z.object({ - name: z.string(), - url: z.string(), - apiKey: z.string().nullable(), - lastProbe: ProfileLastProbe.nullable(), - lastFailure: ProfileLastFailure.nullable(), -}); +// Non-secret OAuth credential metadata persisted in the profile record. The access/refresh tokens +// themselves live in the OS keychain when available; `accessToken`/`refreshToken` are inlined here +// only as the plaintext-file fallback, mirroring how `apiKey` is handled. +export const ProfileOAuth = z + .object({ + accessToken: z.string().nullable(), + refreshToken: z.string().nullable(), + expiresAt: z.iso.datetime(), + clientId: z.string(), + }) + .loose(); +export type ProfileOAuth = z.infer; + +export const ProfileAuthMethod = z.enum(["oauth", "apiKey"]); +export type ProfileAuthMethod = z.infer; + +// `.loose()` so fields written by a newer CLI survive an older CLI's read-modify-write of the +// shared profiles.json. Every write path round-trips the whole file; a strict-strip schema would +// silently drop unknown keys (e.g. a future `oauth`-style block) and force a re-login. +export const ProfileRecord = z + .object({ + name: z.string(), + url: z.string(), + apiKey: z.string().nullable(), + oauth: ProfileOAuth.nullable().default(null), + lastProbe: ProfileLastProbe.nullable(), + lastFailure: ProfileLastFailure.nullable(), + }) + .loose(); export type ProfileRecord = z.infer; -export const ProfilesFile = z.object({ - profiles: z.array(ProfileRecord), -}); +export const ProfilesFile = z + .object({ + profiles: z.array(ProfileRecord), + }) + .loose(); export type ProfilesFile = z.infer; + +// Which mechanism the profile is configured with — not whether its secret currently resolves. +export function profileAuthMethod(record: ProfileRecord): ProfileAuthMethod { + return record.oauth !== null ? "oauth" : "apiKey"; +} diff --git a/src/core/auth/storage.test.ts b/src/core/auth/storage.test.ts index 083f711..864fde8 100644 --- a/src/core/auth/storage.test.ts +++ b/src/core/auth/storage.test.ts @@ -22,21 +22,34 @@ import * as storage from "./storage"; const { clearProfile, + consumeKeychainResidualWarning, + consumeKeyringDowngradeWarning, consumeLegacyStorageWarning, + KEYCHAIN_RESIDUAL_NOTICE, keyringFallbackWarning, LEGACY_STORAGE_NOTICE, listProfileNames, listProfileRecords, profilesFilePath, - readProfile, + readProfileCredential, readProfileRecord, + writeOAuthProfile, writeProbeFailure, writeProbeResult, writeProfile, } = storage; +import type { OAuthCredential } from "./credential"; import type { FileLocation } from "./storage"; +const OAUTH: OAuthCredential = { + kind: "oauth", + accessToken: "access-1", + refreshToken: "refresh-1", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "client-1", +}; + import { join } from "node:path"; function legacyCredentialsPath(): string { @@ -76,6 +89,7 @@ describe("profiles (keyring backend)", () => { name: "default", url: "https://m.example.com", apiKey: null, + oauth: null, lastProbe: null, lastFailure: null, }, @@ -85,20 +99,23 @@ describe("profiles (keyring backend)", () => { it("readProfile returns the URL with the API key from the keyring", async () => { await writeProfile({ url: "https://m.example.com", apiKey: "secret" }); - expect(await readProfile()).toEqual({ url: "https://m.example.com", apiKey: "secret" }); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: { kind: "apiKey", apiKey: "secret" }, + }); }); it("isolates named profiles", async () => { await writeProfile({ url: "https://default.example.com", apiKey: "default-key" }); await writeProfile({ url: "https://prod.example.com", apiKey: "prod-key" }, "prod"); - expect(await readProfile()).toEqual({ + expect(await readProfileCredential()).toEqual({ url: "https://default.example.com", - apiKey: "default-key", + credential: { kind: "apiKey", apiKey: "default-key" }, }); - expect(await readProfile("prod")).toEqual({ + expect(await readProfileCredential("prod")).toEqual({ url: "https://prod.example.com", - apiKey: "prod-key", + credential: { kind: "apiKey", apiKey: "prod-key" }, }); }); @@ -107,8 +124,11 @@ describe("profiles (keyring backend)", () => { await writeProfile({ url: "https://2.example.com", apiKey: "k2" }, "alpha"); await writeProfile({ url: "https://2b.example.com", apiKey: "k2b" }, "alpha"); expect(await listProfileNames()).toEqual(["zeta", "alpha"]); - const alpha = await readProfile("alpha"); - expect(alpha).toEqual({ url: "https://2b.example.com", apiKey: "k2b" }); + const alpha = await readProfileCredential("alpha"); + expect(alpha).toEqual({ + url: "https://2b.example.com", + credential: { kind: "apiKey", apiKey: "k2b" }, + }); }); it("clearProfile removes the entry from JSON and the keyring", async () => { @@ -116,9 +136,12 @@ describe("profiles (keyring backend)", () => { await writeProfile({ url: "https://b.example.com", apiKey: "b" }, "b"); expect(await clearProfile("a")).toBe(true); - expect(await readProfile("a")).toBeNull(); + expect(await readProfileCredential("a")).toBeNull(); expect(hoisted.store.get("metabase-cli:profile:a:apiKey")).toBeUndefined(); - expect(await readProfile("b")).toEqual({ url: "https://b.example.com", apiKey: "b" }); + expect(await readProfileCredential("b")).toEqual({ + url: "https://b.example.com", + credential: { kind: "apiKey", apiKey: "b" }, + }); }); it("clearProfile returns false when no entry matches the name", async () => { @@ -177,7 +200,10 @@ describe("profiles (file fallback when keyring is broken)", () => { it("readProfile returns the inline API key when the keyring is broken", async () => { await writeProfile({ url: "https://m.example.com", apiKey: "secret" }); - expect(await readProfile()).toEqual({ url: "https://m.example.com", apiKey: "secret" }); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: { kind: "apiKey", apiKey: "secret" }, + }); }); }); @@ -204,6 +230,7 @@ describe("readProfileRecord and listProfileRecords", () => { name: "staging", url: "https://m.example.com", apiKey: null, + oauth: null, lastProbe: null, lastFailure: null, }); @@ -391,3 +418,144 @@ describe("legacy storage detection", () => { expect(() => statSync(legacyRejectionsPath())).toThrow(/ENOENT/); }); }); + +describe("OAuth profiles (keyring backend)", () => { + let home: TempConfigHome; + + beforeEach(() => { + hoisted.store.clear(); + hoisted.controls.broken = false; + home = setupTempConfigHome(); + }); + + afterEach(() => { + home.cleanup(); + }); + + it("round-trips an OAuth credential, keeping both tokens in the keyring", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: OAUTH, + }); + expect(await readProfileRecord()).toEqual({ + name: "default", + url: "https://m.example.com", + apiKey: null, + oauth: { + accessToken: null, + refreshToken: null, + expiresAt: OAUTH.expiresAt, + clientId: "client-1", + }, + lastProbe: null, + lastFailure: null, + }); + expect(hoisted.store.get("metabase-cli:profile:default:oauthAccess")).toBe("access-1"); + expect(hoisted.store.get("metabase-cli:profile:default:oauthRefresh")).toBe("refresh-1"); + }); + + it("switching an OAuth profile to an API key clears the OAuth tokens", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); + await writeProfile({ url: "https://m.example.com", apiKey: "k" }); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: { kind: "apiKey", apiKey: "k" }, + }); + expect((await readProfileRecord())?.oauth).toBeNull(); + expect(hoisted.store.get("metabase-cli:profile:default:oauthAccess")).toBeUndefined(); + expect(hoisted.store.get("metabase-cli:profile:default:oauthRefresh")).toBeUndefined(); + }); + + it("switching an API key profile to OAuth clears the API key", async () => { + await writeProfile({ url: "https://m.example.com", apiKey: "k" }); + await writeOAuthProfile("https://m.example.com", OAUTH); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: OAUTH, + }); + expect((await readProfileRecord())?.apiKey).toBeNull(); + expect(hoisted.store.get("metabase-cli:profile:default:apiKey")).toBeUndefined(); + }); + + it("clearProfile removes the OAuth tokens from the keyring", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); + expect(await clearProfile()).toBe(true); + expect(hoisted.store.get("metabase-cli:profile:default:oauthAccess")).toBeUndefined(); + expect(hoisted.store.get("metabase-cli:profile:default:oauthRefresh")).toBeUndefined(); + expect(await readProfileCredential()).toBeNull(); + }); + + it("file-fallback rotated tokens win over a stale keyring entry after the vault recovers", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); // stored in the working keyring + const rotated: OAuthCredential = { + ...OAUTH, + accessToken: "access-2", + refreshToken: "refresh-2", + }; + hoisted.controls.broken = true; // the vault is unavailable during the refresh + try { + expect((await writeOAuthProfile("https://m.example.com", rotated)).backend).toBe("file"); + } finally { + hoisted.controls.broken = false; + } + // The recovered keyring still holds the pre-rotation tokens; the inline file copy is + // authoritative, so the stale keyring entry must not shadow it. + expect(hoisted.store.get("metabase-cli:profile:default:oauthRefresh")).toBe("refresh-1"); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: rotated, + }); + consumeKeyringDowngradeWarning(); // drain the downgrade notice this path raised + }); + + it("flags a residual-secret warning when a keyring-backed token cannot be removed", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); // stored in the working keyring + expect(consumeKeychainResidualWarning()).toBeNull(); // nothing pending yet + hoisted.controls.broken = true; // the vault now refuses deletes + try { + expect(await clearProfile()).toBe(true); // local record is still cleared + } finally { + hoisted.controls.broken = false; + } + expect(consumeKeychainResidualWarning()).toBe(KEYCHAIN_RESIDUAL_NOTICE); + expect(consumeKeychainResidualWarning()).toBeNull(); // consumed exactly once + }); +}); + +describe("OAuth profiles (file fallback)", () => { + let home: TempConfigHome; + + beforeEach(() => { + hoisted.store.clear(); + hoisted.controls.broken = true; + home = setupTempConfigHome(); + }); + + afterEach(() => { + hoisted.controls.broken = false; + home.cleanup(); + }); + + it("inlines the OAuth tokens in profiles.json when the keyring is broken", async () => { + const location = await writeOAuthProfile("https://m.example.com", OAUTH); + expect(location.backend).toBe("file"); + expect((await readProfileRecord())?.oauth).toEqual({ + accessToken: "access-1", + refreshToken: "refresh-1", + expiresAt: OAUTH.expiresAt, + clientId: "client-1", + }); + expect(await readProfileCredential()).toEqual({ + url: "https://m.example.com", + credential: OAUTH, + }); + }); + + it("does not flag a residual secret for an inline (file-fallback) profile", async () => { + await writeOAuthProfile("https://m.example.com", OAUTH); // inlined, never in the keyring + expect(await clearProfile()).toBe(true); + // a failed keyring delete is harmless here — the secret lived in the file we just removed + expect(consumeKeychainResidualWarning()).toBeNull(); + }); +}); diff --git a/src/core/auth/storage.ts b/src/core/auth/storage.ts index d637ae5..75628f8 100644 --- a/src/core/auth/storage.ts +++ b/src/core/auth/storage.ts @@ -8,12 +8,14 @@ import { isNotFoundError, ValidationError } from "../errors"; import { configDir } from "../paths"; import type { ServerInfo } from "../version/probe"; +import type { Credential, OAuthCredential } from "./credential"; import { ProfileLastFailure, ProfileLastProbe, ProfilesFile, type ProbedUser, type ProfileFailureKind, + type ProfileOAuth, type ProfileRecord, } from "./profile-record"; @@ -29,11 +31,23 @@ export const DEFAULT_PROFILE = "default"; export const LEGACY_STORAGE_NOTICE = "Old profile storage detected and ignored; re-run `mb auth login` for each profile."; +export const KEYCHAIN_RESIDUAL_NOTICE = + "warning: could not remove one or more secrets from the OS keychain; a stored token may remain — remove it manually or retry."; + export type ProfileApiKeyAccount = `profile:${string}:apiKey`; -export type CredentialAccount = ProfileApiKeyAccount; +export type ProfileOAuthAccessAccount = `profile:${string}:oauthAccess`; +export type ProfileOAuthRefreshAccount = `profile:${string}:oauthRefresh`; +export type CredentialAccount = + | ProfileApiKeyAccount + | ProfileOAuthAccessAccount + | ProfileOAuthRefreshAccount; export const account = { profileApiKey: (profile: string): ProfileApiKeyAccount => `profile:${profile}:apiKey`, + profileOAuthAccess: (profile: string): ProfileOAuthAccessAccount => + `profile:${profile}:oauthAccess`, + profileOAuthRefresh: (profile: string): ProfileOAuthRefreshAccount => + `profile:${profile}:oauthRefresh`, } as const; export interface KeyringLocation { @@ -58,12 +72,19 @@ export interface Profile { apiKey: string; } +export interface ResolvedCredential { + url: string; + credential: Credential; +} + export interface ProbeWriteInput { user: ProbedUser; server: ServerInfo; } let legacyWarningPending = false; +let keychainResidualPending = false; +let keyringDowngradeNotice: string | null = null; export function profilesFilePath(): string { return join(configDir(), PROFILES_FILE); @@ -77,6 +98,26 @@ export function consumeLegacyStorageWarning(): string | null { return LEGACY_STORAGE_NOTICE; } +// Surfaced after a logout/credential switch when a keyring-backed secret could not be confirmed +// removed (e.g. the OS vault was locked), so the user knows a token may still be on disk in the +// keychain. Cleared on read, like the legacy-storage notice. +export function consumeKeychainResidualWarning(): string | null { + if (!keychainResidualPending) { + return null; + } + keychainResidualPending = false; + return KEYCHAIN_RESIDUAL_NOTICE; +} + +// Surfaced after an automatic token refresh had to fall back to the plaintext file because the OS +// keychain was unavailable, silently moving a previously keyring-backed credential onto disk. The +// command shell consumes it so the downgrade can't go unnoticed. Cleared on read. +export function consumeKeyringDowngradeWarning(): string | null { + const message = keyringDowngradeNotice; + keyringDowngradeNotice = null; + return message; +} + function keyringEnabled(): boolean { return process.env["METABASE_CLI_DISABLE_KEYRING"] !== "1"; } @@ -110,14 +151,18 @@ function tryReadKeyring(key: CredentialAccount): string | null | undefined { } } -function tryRemoveKeyring(key: CredentialAccount): boolean | undefined { +// "skipped" — keyring disabled, nothing to do; "removed"/"absent" — delete succeeded (entry +// existed or not); "failed" — the backend threw, so we cannot confirm the secret is gone. +type KeyringRemoval = "skipped" | "removed" | "absent" | "failed"; + +function removeKeyringEntry(key: CredentialAccount): KeyringRemoval { if (!keyringEnabled()) { - return undefined; + return "skipped"; } try { - return new Entry(KEYRING_SERVICE, key).deletePassword(); + return new Entry(KEYRING_SERVICE, key).deletePassword() ? "removed" : "absent"; } catch { - return undefined; + return "failed"; } } @@ -173,9 +218,21 @@ async function writeProfilesFile(file: ProfilesFile): Promise { return; } await fs.mkdir(dirname(path), { recursive: true, mode: PROFILES_DIR_MODE }); - await fs.writeFile(path, JSON.stringify(file, null, 2) + "\n", { mode: PROFILES_FILE_MODE }); + // Write to a per-process temp file and atomically rename it into place. A crash or a concurrent + // writer can then never leave a half-written profiles.json — which readProfilesFile would treat + // as corrupt and discard, silently wiping every stored profile. (A concurrent writer may still + // overwrite a just-rotated token, but the client's reactive 401-refresh recovers that.) + const tmpPath = `${path}.${process.pid}.tmp`; + await fs.writeFile(tmpPath, JSON.stringify(file, null, 2) + "\n", { mode: PROFILES_FILE_MODE }); if (process.platform !== "win32") { - await fs.chmod(path, PROFILES_FILE_MODE); + await fs.chmod(tmpPath, PROFILES_FILE_MODE); + } + try { + await fs.rename(tmpPath, path); + } catch (error) { + // Don't leave a plaintext-token temp file behind if the rename fails. + await fs.unlink(tmpPath).catch(() => undefined); + throw error; } await cleanupLegacyFiles(); } @@ -191,6 +248,30 @@ function findRecord(file: ProfilesFile, name: string): ProfileRecord | null { return file.profiles.find((entry) => entry.name === name) ?? null; } +// Whether the given side's secret lives in the OS keyring (its inline copy is null) rather than in +// the file — the only case where a failed keyring delete can leave a residual secret behind. +function sideIsKeyringBacked(record: ProfileRecord, side: "apiKey" | "oauth"): boolean { + return side === "oauth" + ? record.oauth !== null && record.oauth.accessToken === null + : record.oauth === null && record.apiKey === null; +} + +// Flag a residual-secret warning only when a keyring delete failed for a side that was actually +// keyring-backed. Inline secrets are dropped with the record, so a failed delete there is harmless — +// warning on it would be a false positive on every login on a keyring-less host. +function flagResidualIfUnconfirmed( + existing: ProfileRecord | null, + cleared: "apiKey" | "oauth", + removals: KeyringRemoval[], +): void { + if (existing === null || !removals.includes("failed")) { + return; + } + if (sideIsKeyringBacked(existing, cleared)) { + keychainResidualPending = true; + } +} + function fileLocation(key: CredentialAccount): FileLocation { return { backend: "file", @@ -208,33 +289,65 @@ export function keyringFallbackWarning(location: FileLocation): string { return `warning: ${cause}; credentials stored as plaintext at ${location.path}`; } -async function persistApiKey(name: string, apiKey: string): Promise { - const key = account.profileApiKey(name); - if (trySetKeyring(key, apiKey)) { +function persistSecret(key: CredentialAccount, value: string): CredentialLocation { + if (trySetKeyring(key, value)) { return { backend: "keyring", service: KEYRING_SERVICE, account: key }; } + // Falling back to the plaintext file (keyring unavailable): drop any stale keyring entry so a + // recovered vault can't later shadow the file copy with an out-of-date secret. + removeKeyringEntry(key); return fileLocation(key); } -export async function readProfile(name: string = DEFAULT_PROFILE): Promise { +function resolveSecret(key: CredentialAccount, inline: string | null): string | null { + // The inline (file) copy is written only when the keyring write failed, so when it is present it + // is authoritative — a stale keyring entry from a since-recovered vault must not shadow it. + if (inline !== null) { + return inline; + } + return tryReadKeyring(key) ?? null; +} + +export async function readProfileCredential( + name: string = DEFAULT_PROFILE, +): Promise { const file = await readProfilesFile(); const record = findRecord(file, name); if (record === null) { return null; } - const apiKey = await resolveApiKey(record); - if (apiKey === null) { - return null; - } - return { url: record.url, apiKey }; + return resolveRecordCredential(record); } -async function resolveApiKey(record: ProfileRecord): Promise { - const fromKeyring = tryReadKeyring(account.profileApiKey(record.name)); - if (typeof fromKeyring === "string") { - return fromKeyring; +export function resolveRecordCredential(record: ProfileRecord): ResolvedCredential | null { + if (record.oauth !== null) { + const accessToken = resolveSecret( + account.profileOAuthAccess(record.name), + record.oauth.accessToken, + ); + const refreshToken = resolveSecret( + account.profileOAuthRefresh(record.name), + record.oauth.refreshToken, + ); + if (accessToken === null || refreshToken === null) { + return null; + } + return { + url: record.url, + credential: { + kind: "oauth", + accessToken, + refreshToken, + expiresAt: record.oauth.expiresAt, + clientId: record.oauth.clientId, + }, + }; } - return record.apiKey; + const apiKey = resolveSecret(account.profileApiKey(record.name), record.apiKey); + if (apiKey === null) { + return null; + } + return { url: record.url, credential: { kind: "apiKey", apiKey } }; } export async function readProfileRecord( @@ -254,33 +367,83 @@ export async function listProfileNames(): Promise { return file.profiles.map((entry) => entry.name); } +async function upsertRecord( + file: ProfilesFile, + name: string, + updated: ProfileRecord, +): Promise { + const exists = findRecord(file, name) !== null; + const profiles = exists + ? file.profiles.map((entry) => (entry.name === name ? updated : entry)) + : [...file.profiles, updated]; + await writeProfilesFile({ ...file, profiles }); +} + export async function writeProfile( profile: Profile, name: string = DEFAULT_PROFILE, ): Promise { - const location = await persistApiKey(name, profile.apiKey); + const location = persistSecret(account.profileApiKey(name), profile.apiKey); const inlineApiKey = location.backend === "file" ? profile.apiKey : null; const file = await readProfilesFile(); const existing = findRecord(file, name); + // Switching a profile to an API key clears any prior OAuth credential it held. + flagResidualIfUnconfirmed(existing, "oauth", [ + removeKeyringEntry(account.profileOAuthAccess(name)), + removeKeyringEntry(account.profileOAuthRefresh(name)), + ]); const updated: ProfileRecord = existing === null ? { name, url: profile.url, apiKey: inlineApiKey, + oauth: null, lastProbe: null, lastFailure: null, } - : { ...existing, url: profile.url, apiKey: inlineApiKey }; - const profiles = - existing === null - ? [...file.profiles, updated] - : file.profiles.map((entry) => (entry.name === name ? updated : entry)); - await writeProfilesFile({ ...file, profiles }); + : { ...existing, url: profile.url, apiKey: inlineApiKey, oauth: null }; + await upsertRecord(file, name, updated); return location; } +export async function writeOAuthProfile( + url: string, + credential: OAuthCredential, + name: string = DEFAULT_PROFILE, +): Promise { + const accessKey = account.profileOAuthAccess(name); + const refreshKey = account.profileOAuthRefresh(name); + const accessLocation = persistSecret(accessKey, credential.accessToken); + const refreshLocation = persistSecret(refreshKey, credential.refreshToken); + const onFile = accessLocation.backend === "file" || refreshLocation.backend === "file"; + + const oauth: ProfileOAuth = { + accessToken: onFile ? credential.accessToken : null, + refreshToken: onFile ? credential.refreshToken : null, + expiresAt: credential.expiresAt, + clientId: credential.clientId, + }; + const file = await readProfilesFile(); + const existing = findRecord(file, name); + // A credential that was keyring-backed but now lands on file (keychain hiccup during refresh) is + // a silent downgrade to plaintext — flag it so the command shell can warn. + if (onFile && existing !== null && sideIsKeyringBacked(existing, "oauth")) { + keyringDowngradeNotice = keyringFallbackWarning(fileLocation(accessKey)); + } + // Switching a profile to OAuth clears any prior API key it held. + flagResidualIfUnconfirmed(existing, "apiKey", [removeKeyringEntry(account.profileApiKey(name))]); + const updated: ProfileRecord = + existing === null + ? { name, url, apiKey: null, oauth, lastProbe: null, lastFailure: null } + : { ...existing, url, apiKey: null, oauth }; + await upsertRecord(file, name, updated); + return onFile + ? fileLocation(accessKey) + : { backend: "keyring", service: KEYRING_SERVICE, account: accessKey }; +} + export async function writeProbeResult( name: string, input: ProbeWriteInput, @@ -330,9 +493,16 @@ export async function writeProbeFailure( } export async function clearProfile(name: string = DEFAULT_PROFILE): Promise { - tryRemoveKeyring(account.profileApiKey(name)); const file = await readProfilesFile(); const existing = findRecord(file, name); + const removals = [ + removeKeyringEntry(account.profileApiKey(name)), + removeKeyringEntry(account.profileOAuthAccess(name)), + removeKeyringEntry(account.profileOAuthRefresh(name)), + ]; + if (existing !== null) { + flagResidualIfUnconfirmed(existing, existing.oauth !== null ? "oauth" : "apiKey", removals); + } if (existing === null) { return false; } diff --git a/src/core/auth/verify.ts b/src/core/auth/verify.ts index f02f2c5..ed593c5 100644 --- a/src/core/auth/verify.ts +++ b/src/core/auth/verify.ts @@ -4,6 +4,7 @@ import { createClient } from "../http/client"; import { HttpError } from "../http/errors"; import { probeServer, PROBE_TIMEOUT_MS, type ServerInfo } from "../version/probe"; +import type { Credential, CredentialRefresher } from "./credential"; import { type ProbedUser, type ProfileFailureKind } from "./profile-record"; const VERIFY_TIMEOUT_MS = 15_000; @@ -27,8 +28,15 @@ export interface VerifyFailure { export type Verification = VerifySuccess | VerifyFailure; -export async function verifyAndProbe(url: string, apiKey: string): Promise { - const client = createClient({ url, apiKey }); +export async function verifyAndProbe( + url: string, + credential: Credential, + refresh?: CredentialRefresher, +): Promise { + const client = createClient( + { url, credential }, + refresh === undefined ? {} : { refreshCredential: refresh }, + ); const userPromise = client.requestParsed(CurrentUser, USER_PATH, { timeoutMs: VERIFY_TIMEOUT_MS, retries: 0, diff --git a/src/core/config.test.ts b/src/core/config.test.ts index 07b9486..af8e326 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -1,8 +1,15 @@ import { afterEach, assert, beforeEach, describe, expect, it, vi } from "vitest"; -const hoisted = vi.hoisted(() => ({ +const hoisted = vi.hoisted<{ + store: Map; + controls: { broken: boolean }; + refreshError: Error | null; + refreshUrls: string[]; +}>(() => ({ store: new Map(), controls: { broken: false }, + refreshError: null, + refreshUrls: [], })); vi.mock("@napi-rs/keyring", async () => { @@ -10,11 +17,58 @@ vi.mock("@napi-rs/keyring", async () => { return createKeyringMockModule(hoisted); }); -import { writeProbeFailure, writeProbeResult, writeProfile } from "./auth/storage"; +vi.mock("./auth/oauth-session", () => ({ + refreshOAuthCredential: async ( + url: string, + credential: OAuthCredential, + ): Promise => { + hoisted.refreshUrls.push(url); + if (hoisted.refreshError !== null) { + throw hoisted.refreshError; + } + return { + ...credential, + accessToken: "refreshed-access", + refreshToken: "refreshed-refresh", + expiresAt: "2099-01-01T00:00:00.000Z", + }; + }, + revokeOAuthCredential: async (): Promise => true, +})); + +import type { OAuthCredential } from "./auth/credential"; +import { + readProfileCredential, + writeOAuthProfile, + writeProbeFailure, + writeProbeResult, + writeProfile, +} from "./auth/storage"; import { setupTempConfigHome, type TempConfigHome } from "./auth/temp-config-home"; -import { explicitProfileName, resolveConfig, resolveProfileName } from "./config"; +import { + createCredentialRefresher, + explicitProfileName, + resolveConfig, + resolveProfileName, +} from "./config"; import { ConfigError } from "./errors"; +const STORED_OAUTH: OAuthCredential = { + kind: "oauth", + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: "2000-01-01T00:00:00.000Z", + clientId: "c1", +}; + +const REFRESHED_OAUTH: OAuthCredential = { + kind: "oauth", + accessToken: "refreshed-access", + refreshToken: "refreshed-refresh", + expiresAt: "2099-01-01T00:00:00.000Z", + clientId: "c1", +}; + describe("resolveConfig", () => { let home: TempConfigHome; @@ -39,7 +93,7 @@ describe("resolveConfig", () => { }); expect(config).toEqual({ url: "https://flag.example.com", - apiKey: "flag-key", + credential: { kind: "apiKey", apiKey: "flag-key" }, profile: "default", source: "flag", }); @@ -51,7 +105,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({}); expect(config).toEqual({ url: "https://env.example.com", - apiKey: "env-key", + credential: { kind: "apiKey", apiKey: "env-key" }, profile: "default", source: "env", }); @@ -65,7 +119,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({}); expect(config).toEqual({ url: "https://saved.example.com", - apiKey: "saved-key", + credential: { kind: "apiKey", apiKey: "saved-key" }, profile: "default", source: "stored", }); @@ -77,7 +131,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({ profile: "staging" }); expect(config).toEqual({ url: "https://staging.example.com", - apiKey: "staging-key", + credential: { kind: "apiKey", apiKey: "staging-key" }, profile: "staging", source: "stored", }); @@ -90,7 +144,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({}); expect(config).toEqual({ url: "https://prod.example.com", - apiKey: "prod-key", + credential: { kind: "apiKey", apiKey: "prod-key" }, profile: "prod", source: "stored", }); @@ -103,7 +157,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({ profile: "staging" }); expect(config).toEqual({ url: "https://staging.example.com", - apiKey: "staging-key", + credential: { kind: "apiKey", apiKey: "staging-key" }, profile: "staging", source: "stored", }); @@ -114,7 +168,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({ url: "https://override.example.com" }); expect(config).toEqual({ url: "https://override.example.com", - apiKey: "saved-key", + credential: { kind: "apiKey", apiKey: "saved-key" }, profile: "default", source: "mixed", }); @@ -126,7 +180,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({}); expect(config).toEqual({ url: "https://saved.example.com", - apiKey: "env-key", + credential: { kind: "apiKey", apiKey: "env-key" }, profile: "default", source: "mixed", }); @@ -156,7 +210,7 @@ describe("resolveConfig", () => { const config = await resolveConfig({ profile: "still_works" }); expect(config).toEqual({ url: "https://saved.example.com", - apiKey: "saved-key", + credential: { kind: "apiKey", apiKey: "saved-key" }, profile: "still_works", source: "stored", }); @@ -252,3 +306,133 @@ describe("explicitProfileName", () => { expect(explicitProfileName(undefined)).toBeNull(); }); }); + +describe("resolveConfig OAuth credentials", () => { + let home: TempConfigHome; + + beforeEach(() => { + hoisted.store.clear(); + hoisted.refreshUrls = []; + home = setupTempConfigHome(); + delete process.env["METABASE_URL"]; + delete process.env["METABASE_API_KEY"]; + delete process.env["METABASE_PROFILE"]; + }); + + afterEach(() => { + home.cleanup(); + }); + + it("returns a stored OAuth credential that is still valid without refreshing", async () => { + await writeOAuthProfile( + "https://oauth.example.com", + { ...STORED_OAUTH, expiresAt: "2099-01-01T00:00:00.000Z" }, + "oauthp", + ); + const config = await resolveConfig({ profile: "oauthp" }); + expect(config).toEqual({ + url: "https://oauth.example.com", + credential: { ...STORED_OAUTH, expiresAt: "2099-01-01T00:00:00.000Z" }, + profile: "oauthp", + source: "stored", + }); + }); + + it("refuses to send a stored OAuth credential to a --url-overridden host", async () => { + await writeOAuthProfile("https://oauth.example.com", STORED_OAUTH, "oauthp"); + const error = await resolveConfig({ + profile: "oauthp", + url: "https://evil.example.com", + }).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe( + 'profile "oauthp" is a browser-login (OAuth) profile bound to https://oauth.example.com, ' + + "but the request URL is https://evil.example.com. " + + "Drop --url/METABASE_URL to use the profile's own URL, or run " + + "`mb auth login --url https://evil.example.com` to authenticate there.", + ); + }); + + it("allows a --url that matches the OAuth issuer after normalization", async () => { + const valid = { ...STORED_OAUTH, expiresAt: "2099-01-01T00:00:00.000Z" }; + await writeOAuthProfile("https://oauth.example.com", valid, "oauthp"); + const config = await resolveConfig({ profile: "oauthp", url: "https://oauth.example.com/" }); + expect(config).toEqual({ + url: "https://oauth.example.com", + credential: valid, + profile: "oauthp", + source: "mixed", + }); + }); + + it("proactively refreshes and persists an expired stored OAuth credential", async () => { + await writeOAuthProfile("https://oauth.example.com", STORED_OAUTH, "oauthp"); + const config = await resolveConfig({ profile: "oauthp" }); + expect(config).toEqual({ + url: "https://oauth.example.com", + credential: REFRESHED_OAUTH, + profile: "oauthp", + source: "stored", + }); + expect(await readProfileCredential("oauthp")).toEqual({ + url: "https://oauth.example.com", + credential: REFRESHED_OAUTH, + }); + // The refresh leg is pinned to the stored issuer the refresh token is bound to. + expect(hoisted.refreshUrls).toEqual(["https://oauth.example.com"]); + }); + + it("falls back to the existing credential when a proactive refresh fails", async () => { + await writeOAuthProfile("https://oauth.example.com", STORED_OAUTH, "oauthp"); + hoisted.refreshError = new Error("token endpoint unreachable"); + try { + const config = await resolveConfig({ profile: "oauthp" }); + // Best-effort: the command still resolves with the (expired) stored credential rather than + // throwing — the reactive 401-refresh will recover on the first request. + expect(config).toEqual({ + url: "https://oauth.example.com", + credential: STORED_OAUTH, + profile: "oauthp", + source: "stored", + }); + } finally { + hoisted.refreshError = null; + } + }); + + it("createCredentialRefresher refreshes and persists the stored OAuth credential", async () => { + await writeOAuthProfile("https://oauth.example.com", STORED_OAUTH, "oauthp"); + const refresh = createCredentialRefresher("oauthp"); + expect(await refresh()).toEqual(REFRESHED_OAUTH); + expect(await readProfileCredential("oauthp")).toEqual({ + url: "https://oauth.example.com", + credential: REFRESHED_OAUTH, + }); + // The reactive refresher reads the stored profile, so the refresh leg targets its issuer — + // never whatever URL the failing request was sent to. + expect(hoisted.refreshUrls).toEqual(["https://oauth.example.com"]); + }); + + it("createCredentialRefresher adds a re-login hint when the server rejects the refresh", async () => { + await writeOAuthProfile("https://oauth.example.com", STORED_OAUTH, "oauthp"); + hoisted.refreshError = new ConfigError("OAuth token refresh failed (400): invalid_grant"); + try { + const refresh = createCredentialRefresher("oauthp"); + const error = await refresh().catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe( + "OAuth token refresh failed (400): invalid_grant — run `mb auth login --profile oauthp` to log in again", + ); + } finally { + hoisted.refreshError = null; + } + }); + + it("createCredentialRefresher returns null for an API key profile", async () => { + await writeProfile({ url: "https://m.example.com", apiKey: "k" }, "keyp"); + const refresh = createCredentialRefresher("keyp"); + expect(await refresh()).toBeNull(); + }); +}); diff --git a/src/core/config.ts b/src/core/config.ts index d8a12d4..27b668e 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -1,4 +1,16 @@ -import { DEFAULT_PROFILE, readProfile, readProfileRecord } from "./auth/storage"; +import { + type Credential, + type CredentialRefresher, + isOAuthExpired, + type OAuthCredential, +} from "./auth/credential"; +import { refreshOAuthCredential } from "./auth/oauth-session"; +import { + DEFAULT_PROFILE, + readProfileCredential, + readProfileRecord, + writeOAuthProfile, +} from "./auth/storage"; import { ConfigError } from "./errors"; import { normalizeUrl } from "./url"; @@ -23,7 +35,7 @@ export interface ConfigFlags { export interface ResolvedConfig { url: string; - apiKey: string; + credential: Credential; profile: string; source: ConfigSource; } @@ -33,11 +45,16 @@ export interface EnvCredentials { apiKey: string | null; } -interface FieldResolution { +interface UrlResolution { value: string; source: "flag" | "env" | "stored"; } +interface CredentialResolution { + credential: Credential; + source: "flag" | "env" | "stored"; +} + export function resolveProfileName(profileFlag: string | undefined): string { return explicitProfileName(profileFlag) ?? DEFAULT_PROFILE; } @@ -56,27 +73,127 @@ export function readEnvCredentials(): EnvCredentials { export async function resolveConfig(flags: ConfigFlags): Promise { const profile = resolveProfileName(flags.profile); const env = readEnvCredentials(); - const flagUrl = flags.url; - const flagKey = flags.apiKey; + const hasUrl = Boolean(flags.url ?? env.url); + const hasKey = Boolean(flags.apiKey ?? env.apiKey); - const needsStored = (!flagUrl && !env.url) || (!flagKey && !env.apiKey); - const stored = needsStored ? await readProfile(profile) : null; + const stored = hasUrl && hasKey ? null : await readProfileCredential(profile); - const urlField = pickField(flagUrl, env.url, stored?.url); - const keyField = pickField(flagKey, env.apiKey, stored?.apiKey); + const url = resolveUrl(flags.url, env.url, stored?.url); + const credential = resolveCredential(flags.apiKey, env.apiKey, stored?.credential); - if (urlField === null || keyField === null) { + if (url === null || credential === null) { const hint = await failureHintForProfile(profile); throw new ConfigError( `Not authenticated for profile "${profile}". Run \`mb auth login\`, set ${ENV_URL}/${ENV_API_KEY}, or pass --url/--api-key.${hint}`, ); } + const normalizedUrl = normalizeUrl(url.value); + // OAuth credentials are always loaded from a stored profile, so stored?.url is the issuer the + // refresh token is bound to — refresh against that, never a (possibly --url-overridden) request URL. + assertOAuthUrlMatchesIssuer(credential.credential, stored?.url, normalizedUrl, profile); + const fresh = await ensureFreshCredential(profile, credential.credential, stored?.url); + return { - url: normalizeUrl(urlField.value), - apiKey: keyField.value, + url: normalizedUrl, + credential: fresh, profile, - source: urlField.source === keyField.source ? urlField.source : "mixed", + source: url.source === credential.source ? url.source : "mixed", + }; +} + +// A stored OAuth credential's bearer/refresh tokens are bound to the issuer that minted them. +// Refuse to send them to a different host named by --url/METABASE_URL: that would leak the bearer +// token to the foreign host, and the 401-refresh loop would keep minting fresh tokens for it too. +// API-key credentials are unaffected (and only OAuth credentials ever come from a stored profile). +function assertOAuthUrlMatchesIssuer( + credential: Credential, + issuerUrl: string | undefined, + requestUrl: string, + profile: string, +): void { + if (credential.kind !== "oauth" || issuerUrl === undefined) { + return; + } + const issuer = normalizeUrl(issuerUrl); + if (issuer === requestUrl) { + return; + } + throw new ConfigError( + `profile "${profile}" is a browser-login (OAuth) profile bound to ${issuer}, but the request URL is ${requestUrl}. ` + + `Drop --url/${ENV_URL} to use the profile's own URL, or run \`mb auth login --url ${requestUrl}\` to authenticate there.`, + ); +} + +// Proactively refresh a stored OAuth credential that has reached its expiry, persisting the rotated +// tokens so the next invocation starts fresh. API-key credentials pass through untouched. +async function ensureFreshCredential( + profile: string, + credential: Credential, + issuerUrl: string | undefined, +): Promise { + if (credential.kind !== "oauth" || issuerUrl === undefined) { + return credential; + } + if (!isOAuthExpired(credential, Date.now())) { + return credential; + } + let refreshed: OAuthCredential; + try { + refreshed = await refreshOAuthCredential(issuerUrl, credential, Date.now()); + } catch { + // Best-effort: a failed network refresh (server blip, offline) falls back to the existing + // credential and lets the reactive 401-refresh retry, rather than failing every command. + return credential; + } + // The rotated refresh token now in hand is the only valid copy — the server has consumed the old + // one. A persist failure must surface, not be swallowed: silently returning the old credential + // would leave the consumed token stored and brick the grant on the next refresh. + await persistRefreshed(profile, issuerUrl, refreshed); + return refreshed; +} + +async function refreshAndPersist( + profile: string, + issuerUrl: string, + credential: OAuthCredential, +): Promise { + const refreshed = await refreshOAuthCredential(issuerUrl, credential, Date.now()); + await persistRefreshed(profile, issuerUrl, refreshed); + return refreshed; +} + +// Persist rotated tokens. writeOAuthProfile flags a pending warning when the keychain was +// unavailable and the tokens had to land in the plaintext file — the command shell surfaces it so +// a background refresh can't silently downgrade keyring-backed credentials to disk. +async function persistRefreshed( + profile: string, + issuerUrl: string, + refreshed: OAuthCredential, +): Promise { + await writeOAuthProfile(issuerUrl, refreshed, profile); +} + +// Reactive refresher handed to the HTTP client: on a 401 it reloads the stored OAuth credential +// (picking up any rotated refresh token), refreshes against the credential's own issuer URL, +// persists, and returns the new credential. A refresh the server rejects (revoked/expired grant) +// is terminal for this credential, so the error tells the user how to recover. +export function createCredentialRefresher(profile: string): CredentialRefresher { + return async () => { + const stored = await readProfileCredential(profile); + if (stored === null || stored.credential.kind !== "oauth") { + return null; + } + try { + return await refreshAndPersist(profile, stored.url, stored.credential); + } catch (error) { + if (error instanceof ConfigError) { + throw new ConfigError( + `${error.message} — run \`mb auth login --profile ${profile}\` to log in again`, + ); + } + throw error; + } }; } @@ -91,11 +208,11 @@ async function failureHintForProfile(profile: string): Promise { return ` profile "${profile}" last verify failed: ${record.lastFailure.reason}. Run \`mb auth login --profile ${profile}\` to update the token.`; } -function pickField( - flag: string | null | undefined, - env: string | null | undefined, - stored: string | null | undefined, -): FieldResolution | null { +function resolveUrl( + flag: string | undefined, + env: string | null, + stored: string | undefined, +): UrlResolution | null { if (flag) { return { value: flag, source: "flag" }; } @@ -107,3 +224,20 @@ function pickField( } return null; } + +function resolveCredential( + flagKey: string | undefined, + envKey: string | null, + stored: Credential | undefined, +): CredentialResolution | null { + if (flagKey) { + return { credential: { kind: "apiKey", apiKey: flagKey }, source: "flag" }; + } + if (envKey) { + return { credential: { kind: "apiKey", apiKey: envKey }, source: "env" }; + } + if (stored) { + return { credential: stored, source: "stored" }; + } + return null; +} diff --git a/src/core/http/client.test.ts b/src/core/http/client.test.ts index a8137ee..57c647a 100644 --- a/src/core/http/client.test.ts +++ b/src/core/http/client.test.ts @@ -1,86 +1,22 @@ +import { setTimeout as delay } from "node:timers/promises"; + import { assert, describe, expect, it } from "vitest"; import { z } from "zod"; import packageJson from "../../../package.json" with { type: "json" }; +import type { OAuthCredential } from "../auth/credential"; import { NetworkError, ResponseShapeError, TimeoutError } from "../errors"; import { type ClientCredentials, createClient } from "./client"; import { HttpError } from "./errors"; +import { captureFetch, jsonResponse } from "./fetch-capture"; const CONFIG: ClientCredentials = { url: "https://m.example.com", - apiKey: "mb_test_key_abcdef0123", + credential: { kind: "apiKey", apiKey: "mb_test_key_abcdef0123" }, }; const PingResponse = z.object({ id: z.number().int(), email: z.string() }); -interface FakeFetchHandle { - fetch: typeof fetch; - calls: FetchCallRecord[]; -} - -interface FetchCallRecord { - url: string; - method: string; - headers: Record; -} - -type ResponseFactory = () => Response | Promise; -type FetchScript = ReadonlyArray; - -function makeFakeFetch(script: FetchScript): FakeFetchHandle { - const queue = [...script]; - const calls: FetchCallRecord[] = []; - const fetchImpl: typeof fetch = async (input, init) => { - calls.push({ - url: typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url, - method: init?.method ?? "GET", - headers: headersToRecord(init?.headers), - }); - const next = queue.shift(); - assert(next !== undefined, "fakeFetch: no more responses queued"); - if (next instanceof Error) { - throw next; - } - return typeof next === "function" ? await next() : next; - }; - return { fetch: fetchImpl, calls }; -} - -function headersToRecord(init: RequestInit["headers"]): Record { - const result: Record = {}; - if (!init) { - return result; - } - if (init instanceof Headers) { - for (const [key, value] of init.entries()) { - result[key] = value; - } - return result; - } - if (Array.isArray(init)) { - for (const entry of init) { - const key = entry[0]; - const value = entry[1]; - if (typeof key === "string" && typeof value === "string") { - result[key] = value; - } - } - return result; - } - for (const [key, value] of Object.entries(init)) { - if (typeof value === "string") { - result[key] = value; - } - } - return result; -} - -function jsonResponse(body: unknown): Response { - return new Response(JSON.stringify(body), { - headers: { "content-type": "application/json" }, - }); -} - const HANGING_FETCH: typeof fetch = (_input, init) => new Promise((_resolve, reject) => { init?.signal?.addEventListener("abort", () => { @@ -90,7 +26,7 @@ const HANGING_FETCH: typeof fetch = (_input, init) => describe("createClient.requestParsed", () => { it("returns parsed JSON on a 200 response", async () => { - const fakeFetch = makeFakeFetch([jsonResponse({ id: 1, email: "a@b.com" })]); + const fakeFetch = captureFetch([jsonResponse({ id: 1, email: "a@b.com" })]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); const result = await client.requestParsed(PingResponse, "/api/user/current"); @@ -105,12 +41,13 @@ describe("createClient.requestParsed", () => { accept: "application/json", "user-agent": `metabase-cli/${packageJson.version}`, }, + body: null, }, ]); }); it("retries 5xx responses and returns the eventual success body", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response("oops", { status: 500 }), new Response("oops again", { status: 502 }), jsonResponse({ id: 7, email: "u@m.com" }), @@ -124,7 +61,7 @@ describe("createClient.requestParsed", () => { }); it("throws HttpError when retries are exhausted", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response('{"message":"server boom"}', { status: 500, headers: { "content-type": "application/json" }, @@ -152,7 +89,7 @@ describe("createClient.requestParsed", () => { }); it("retries on 429 with Retry-After header", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response("rate", { status: 429, headers: { "retry-after": "0" } }), jsonResponse({ id: 1, email: "a@b.com" }), ]); @@ -165,7 +102,7 @@ describe("createClient.requestParsed", () => { }); it("retries network failures", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new TypeError("fetch failed"), jsonResponse({ id: 2, email: "x@y.com" }), ]); @@ -182,7 +119,7 @@ describe("createClient.requestParsed", () => { failure.cause = Object.assign(new Error("connect ECONNREFUSED 127.0.0.1:1"), { code: "ECONNREFUSED", }); - const fakeFetch = makeFakeFetch([failure]); + const fakeFetch = captureFetch([failure]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); const error = await client @@ -206,7 +143,7 @@ describe("createClient.requestParsed", () => { failure.cause = Object.assign(new Error("getaddrinfo ENOTFOUND m.example.com"), { code: "ENOTFOUND", }); - const fakeFetch = makeFakeFetch([failure]); + const fakeFetch = captureFetch([failure]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); const error = await client @@ -226,7 +163,7 @@ describe("createClient.requestParsed", () => { }); it("falls back to the raw fetch message when the cause carries no error code", async () => { - const fakeFetch = makeFakeFetch([new TypeError("fetch failed")]); + const fakeFetch = captureFetch([new TypeError("fetch failed")]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); const error = await client @@ -247,7 +184,7 @@ describe("createClient.requestParsed", () => { const body = { id: "not-a-number", email: "a@b.com" }; const expectedIssues = PingResponse.safeParse(body).error?.issues; assert(expectedIssues !== undefined, "expected zod failure for fixture body"); - const fakeFetch = makeFakeFetch([jsonResponse(body)]); + const fakeFetch = captureFetch([jsonResponse(body)]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); const error = await client @@ -273,7 +210,7 @@ describe("createClient.requestParsed", () => { const body = { id: "not-a-number", email: "a@b.com" }; const expectedIssues = PingResponse.safeParse(body).error?.issues; assert(expectedIssues !== undefined, "expected zod failure for fixture body"); - const fakeFetch = makeFakeFetch([jsonResponse(body)]); + const fakeFetch = captureFetch([jsonResponse(body)]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch, getServerTag: async () => "v0.58.7", @@ -299,7 +236,7 @@ describe("createClient.requestParsed", () => { }); it("threads getServerTag into HttpError so route-missing names the version", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response("API endpoint does not exist.", { status: 404, headers: { "content-type": "text/plain" }, @@ -325,7 +262,7 @@ describe("createClient.requestParsed", () => { }); it("throws HttpError on content-type mismatch with no silent downgrade", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response("oops", { status: 200, headers: { "content-type": "text/html" } }), ]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); @@ -341,7 +278,7 @@ describe("createClient.requestParsed", () => { }); it("throws HttpError when the response has no content-type header", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ () => { const response = new Response('{"id":1,"email":"a@b"}', { status: 200 }); response.headers.delete("content-type"); @@ -374,7 +311,7 @@ describe("createClient.requestParsed", () => { describe("createClient idempotency-aware retry", () => { it("does not retry POST on 5xx and surfaces the first HttpError", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response('{"message":"server boom"}', { status: 500, headers: { "content-type": "application/json" }, @@ -394,7 +331,7 @@ describe("createClient idempotency-aware retry", () => { }); it("does not retry POST on 429 either — non-idempotent never retries on status", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response('{"message":"slow down"}', { status: 429, headers: { "content-type": "application/json", "retry-after": "0" }, @@ -414,7 +351,7 @@ describe("createClient idempotency-aware retry", () => { }); it("retries POST on network failure", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new TypeError("fetch failed"), jsonResponse({ id: 9, email: "ok@m.com" }), ]); @@ -430,7 +367,7 @@ describe("createClient idempotency-aware retry", () => { }); it("retries non-idempotent calls when the caller explicitly opts in via idempotent: true", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response("oops", { status: 503 }), jsonResponse({ id: 4, email: "p@u.com" }), ]); @@ -447,7 +384,7 @@ describe("createClient idempotency-aware retry", () => { }); it("does not retry GET on 5xx when the caller forces idempotent: false", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response('{"message":"down"}', { status: 503, headers: { "content-type": "application/json" }, @@ -470,14 +407,14 @@ describe("createClient idempotency-aware retry", () => { describe("createClient sanitization", () => { it("strips the configured apiKey from error bodies", async () => { const apiKey = "configured_api_key_value"; - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response(`{"message":"forbidden","echo":"${apiKey}"}`, { status: 403, headers: { "content-type": "application/json" }, }), ]); const client = createClient( - { url: "https://m.example.com", apiKey }, + { url: "https://m.example.com", credential: { kind: "apiKey", apiKey } }, { fetchImpl: fakeFetch.fetch }, ); @@ -491,7 +428,7 @@ describe("createClient sanitization", () => { }); it("redacts secret response headers in HttpError detail", async () => { - const fakeFetch = makeFakeFetch([ + const fakeFetch = captureFetch([ new Response('{"message":"forbidden"}', { status: 403, headers: { @@ -519,7 +456,7 @@ describe("createClient sanitization", () => { describe("createClient query encoding", () => { it("encodes array query values as repeated keys and skips undefined entries", async () => { - const fakeFetch = makeFakeFetch([jsonResponse({ id: 1, email: "a@b.com" })]); + const fakeFetch = captureFetch([jsonResponse({ id: 1, email: "a@b.com" })]); const client = createClient(CONFIG, { fetchImpl: fakeFetch.fetch }); await client.requestParsed(PingResponse, "/api/search", { @@ -535,3 +472,187 @@ describe("createClient query encoding", () => { ); }); }); + +const OAUTH: OAuthCredential = { + kind: "oauth", + accessToken: "acc-1", + refreshToken: "ref-1", + expiresAt: "2026-06-08T13:00:00.000Z", + clientId: "c1", +}; + +function unauthorizedResponse(): Response { + return new Response('{"error":"unauthorized"}', { + status: 401, + headers: { "content-type": "application/json" }, + }); +} + +describe("createClient OAuth bearer auth", () => { + it("sends an Authorization Bearer header and no x-api-key", async () => { + const fakeFetch = captureFetch([jsonResponse({ id: 1, email: "a@b.c" })]); + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { fetchImpl: fakeFetch.fetch }, + ); + await client.requestParsed(PingResponse, "/api/user/current"); + expect(fakeFetch.calls[0]?.headers["authorization"]).toBe("Bearer acc-1"); + expect(fakeFetch.calls[0]?.headers["x-api-key"]).toBeUndefined(); + }); + + it("refreshes on 401 and replays the request with the new access token", async () => { + const fakeFetch = captureFetch([ + unauthorizedResponse(), + jsonResponse({ id: 1, email: "a@b.c" }), + ]); + const refreshed: OAuthCredential = { ...OAUTH, accessToken: "acc-2", refreshToken: "ref-2" }; + let refreshCalls = 0; + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { + fetchImpl: fakeFetch.fetch, + refreshCredential: async () => { + refreshCalls += 1; + return refreshed; + }, + }, + ); + const result = await client.requestParsed(PingResponse, "/api/user/current", { retries: 0 }); + expect(result).toEqual({ id: 1, email: "a@b.c" }); + expect(refreshCalls).toBe(1); + expect(fakeFetch.calls[0]?.headers["authorization"]).toBe("Bearer acc-1"); + expect(fakeFetch.calls[1]?.headers["authorization"]).toBe("Bearer acc-2"); + }); + + it("gives up after a single refresh when the replay still 401s", async () => { + const fakeFetch = captureFetch([unauthorizedResponse(), unauthorizedResponse()]); + let refreshCalls = 0; + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { + fetchImpl: fakeFetch.fetch, + refreshCredential: async () => { + refreshCalls += 1; + return { ...OAUTH, accessToken: "acc-2" }; + }, + }, + ); + const error = await client + .requestParsed(PingResponse, "/api/user/current", { retries: 0 }) + .catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(HttpError); + assert(error instanceof HttpError, "expected HttpError"); + expect(error.status).toBe(401); + expect(refreshCalls).toBe(1); + expect(fakeFetch.calls).toHaveLength(2); + }); + + it("shares a single refresh across concurrent 401s", async () => { + const fakeFetch = captureFetch([ + unauthorizedResponse(), + unauthorizedResponse(), + jsonResponse({ id: 1, email: "a@b.c" }), + jsonResponse({ id: 1, email: "a@b.c" }), + ]); + let refreshCalls = 0; + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { + fetchImpl: fakeFetch.fetch, + refreshCredential: async () => { + refreshCalls += 1; + // Yield a macrotask so both 401 handlers (pure microtask chains) join this refresh + // before it settles — the deterministic worst case for a duplicate-refresh bug. + await delay(0); + return { ...OAUTH, accessToken: "acc-2" }; + }, + }, + ); + const results = await Promise.all([ + client.requestParsed(PingResponse, "/api/user/current", { retries: 0 }), + client.requestParsed(PingResponse, "/api/user/current", { retries: 0 }), + ]); + expect(results).toEqual([ + { id: 1, email: "a@b.c" }, + { id: 1, email: "a@b.c" }, + ]); + expect(refreshCalls).toBe(1); + expect(fakeFetch.calls[2]?.headers["authorization"]).toBe("Bearer acc-2"); + expect(fakeFetch.calls[3]?.headers["authorization"]).toBe("Bearer acc-2"); + }); + + it("refreshes again on a later request when the token expires a second time", async () => { + const fakeFetch = captureFetch([ + unauthorizedResponse(), + jsonResponse({ id: 1, email: "a@b.c" }), + unauthorizedResponse(), + jsonResponse({ id: 1, email: "a@b.c" }), + ]); + let refreshCalls = 0; + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { + fetchImpl: fakeFetch.fetch, + refreshCredential: async () => { + refreshCalls += 1; + return { ...OAUTH, accessToken: `acc-${refreshCalls + 1}` }; + }, + }, + ); + await client.requestParsed(PingResponse, "/api/user/current", { retries: 0 }); + await client.requestParsed(PingResponse, "/api/user/current", { retries: 0 }); + // Each expiry event gets its own refresh — no per-client "refresh only once" latch. + expect(refreshCalls).toBe(2); + }); + + it("does not refresh an API key credential on 401", async () => { + const fakeFetch = captureFetch([unauthorizedResponse()]); + let refreshCalls = 0; + const client = createClient( + { url: "https://m.example.com", credential: { kind: "apiKey", apiKey: "k" } }, + { + fetchImpl: fakeFetch.fetch, + refreshCredential: async () => { + refreshCalls += 1; + return null; + }, + }, + ); + await client + .requestParsed(PingResponse, "/api/user/current", { retries: 0 }) + .catch(() => undefined); + expect(refreshCalls).toBe(0); + expect(fakeFetch.calls).toHaveLength(1); + }); + + it("redacts both OAuth tokens from error bodies", async () => { + const fakeFetch = captureFetch([ + new Response('{"echo":"acc-1 and ref-1"}', { + status: 403, + headers: { "content-type": "application/json" }, + }), + ]); + const client = createClient( + { url: "https://m.example.com", credential: OAUTH }, + { fetchImpl: fakeFetch.fetch }, + ); + const error = await client + .requestParsed(PingResponse, "/api/user/current", { retries: 0 }) + .catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(HttpError); + assert(error instanceof HttpError, "expected HttpError"); + expect(error.developerDetail.body).toBe('{"echo":"[REDACTED] and [REDACTED]"}'); + }); +}); + +describe("createClient subpath base URLs", () => { + it("joins request paths under a base URL that carries a subpath", async () => { + const fakeFetch = captureFetch([jsonResponse({ id: 1, email: "a@b.c" })]); + const client = createClient( + { url: "https://my.org.com/metabase", credential: { kind: "apiKey", apiKey: "k" } }, + { fetchImpl: fakeFetch.fetch }, + ); + await client.requestParsed(PingResponse, "/api/user/current"); + expect(fakeFetch.calls[0]?.url).toBe("https://my.org.com/metabase/api/user/current"); + }); +}); diff --git a/src/core/http/client.ts b/src/core/http/client.ts index 6cef4af..2a3ecac 100644 --- a/src/core/http/client.ts +++ b/src/core/http/client.ts @@ -11,10 +11,20 @@ import { import { JSON_CONTENT_TYPE, parseJson } from "../../runtime/json"; import { combineAborts, throwIfAborted } from "../../runtime/signal"; +import { + type Credential, + credentialAuthHeader, + credentialSecrets, + type CredentialRefresher, +} from "../auth/credential"; + import { HttpError, isRetryableStatus } from "./errors"; +import { buildNetworkError } from "./network-error"; import { backoffDelay, DEFAULT_MAX_RETRIES, runWithRetries, type RetryOutcome } from "./retry"; import type { RedactionContext } from "./sanitize"; +const UNAUTHORIZED_STATUS = 401; + export type HttpMethod = "GET" | "HEAD" | "OPTIONS" | "POST" | "PUT" | "PATCH" | "DELETE"; export type ExpectedContentType = "json" | "text" | "binary"; @@ -48,7 +58,7 @@ export interface Client { export interface ClientCredentials { url: string; - apiKey: string; + credential: Credential; } type FetchBody = NonNullable; @@ -65,11 +75,17 @@ interface PreparedRequest { callerSignal: AbortSignal | undefined; } +interface ExecOutcome { + response: Response; + prepared: PreparedRequest; +} + export type ServerTagResolver = () => Promise; export interface ClientOverrides { fetchImpl?: typeof fetch; getServerTag?: ServerTagResolver; + refreshCredential?: CredentialRefresher; } const NO_SERVER_TAG: ServerTagResolver = async () => null; @@ -77,9 +93,38 @@ const NO_SERVER_TAG: ServerTagResolver = async () => null; export function createClient(config: ClientCredentials, overrides: ClientOverrides = {}): Client { const fetchImpl = overrides.fetchImpl ?? globalThis.fetch.bind(globalThis); const getServerTag = overrides.getServerTag ?? NO_SERVER_TAG; - const redactionContext: RedactionContext = { - knownSecrets: new Set([config.apiKey]), - }; + const refreshCredential = overrides.refreshCredential; + let credential = config.credential; + const knownSecrets = new Set(credentialSecrets(credential)); + const redactionContext: RedactionContext = { knownSecrets }; + + // Single-flight: concurrent 401s (e.g. verify's parallel user+probe requests) must share one + // refresh. The server rotates refresh tokens, so a second concurrent refresh would replay the + // already-consumed token — which rotation reuse detection may answer by revoking the whole grant. + let refreshInFlight: Promise | null = null; + + function tryRefreshCredential(): Promise { + const refresh = refreshCredential; + if (credential.kind !== "oauth" || refresh === undefined) { + return Promise.resolve(false); + } + refreshInFlight ??= refreshOnce(refresh).finally(() => { + refreshInFlight = null; + }); + return refreshInFlight; + } + + async function refreshOnce(refresh: CredentialRefresher): Promise { + const next = await refresh(); + if (next === null) { + return false; + } + credential = next; + for (const secret of credentialSecrets(next)) { + knownSecrets.add(secret); + } + return true; + } async function attemptOnce(prepared: PreparedRequest, attempt: number): Promise { const hasRetriesLeft = attempt < prepared.retries; @@ -143,12 +188,30 @@ export function createClient(config: ClientCredentials, overrides: ClientOverrid return runWithRetries((attempt) => attemptOnce(prepared, attempt), prepared.callerSignal); } + // On a 401 with an OAuth credential, attempt a single token refresh and replay the request with + // the new access token. API-key credentials and non-401 errors propagate unchanged. + async function executeWithAuthRefresh(path: string, opts: RequestOptions): Promise { + const prepared = prepare(path, opts); + try { + return { response: await executeRaw(prepared), prepared }; + } catch (error) { + if (error instanceof HttpError && error.status === UNAUTHORIZED_STATUS) { + if (await tryRefreshCredential()) { + const retried = prepare(path, opts); + return { response: await executeRaw(retried), prepared: retried }; + } + } + throw error; + } + } + function prepare(path: string, opts: RequestOptions = {}): PreparedRequest { const method = opts.method ?? "GET"; const expectContentType = opts.expectContentType ?? "json"; const url = buildUrl(config.url, path, opts.query); const headers = new Headers(); - headers.set("x-api-key", config.apiKey); + const auth = credentialAuthHeader(credential); + headers.set(auth.name, auth.value); headers.set("accept", acceptHeader(expectContentType)); headers.set("user-agent", USER_AGENT); let body: FetchBody | null = null; @@ -180,11 +243,13 @@ export function createClient(config: ClientCredentials, overrides: ClientOverrid return { async requestRaw(path, opts) { - return executeRaw(prepare(path, opts)); + return (await executeWithAuthRefresh(path, opts ?? {})).response; }, async requestParsed(schema, path, opts) { - const prepared = prepare(path, { ...opts, expectContentType: "json" }); - const response = await executeRaw(prepared); + const { response, prepared } = await executeWithAuthRefresh(path, { + ...opts, + expectContentType: "json", + }); const text = await response.text(); try { return parseJson(text, schema, { source: prepared.url }); @@ -202,11 +267,10 @@ export function createClient(config: ClientCredentials, overrides: ClientOverrid } }, async requestStream(path, opts) { - const prepared = prepare(path, { + const { response, prepared } = await executeWithAuthRefresh(path, { ...opts, expectContentType: opts?.expectContentType ?? "binary", }); - const response = await executeRaw(prepared); if (!response.body) { throw new NetworkError("Response had no body to stream", { method: prepared.method, @@ -286,71 +350,6 @@ function throwContentTypeMismatch( }); } -interface NetworkTarget { - host: string; - hostname: string; -} - -const NETWORK_HINTS: Record string> = { - ECONNREFUSED: (t) => - `Connection refused by ${t.host} — is Metabase running and is the port correct?`, - ENOTFOUND: (t) => `Host not found: ${t.hostname} — check the URL.`, - EAI_AGAIN: (t) => `Could not resolve ${t.hostname} — check your network connection and the URL.`, - ECONNRESET: (t) => - `Connection to ${t.host} was reset — the server may have closed it, or http/https may be mismatched.`, - ETIMEDOUT: (t) => `Connection to ${t.host} timed out — check the host, port, and your network.`, -}; - -const TLS_ERROR_CODES: ReadonlySet = new Set([ - "EPROTO", - "DEPTH_ZERO_SELF_SIGNED_CERT", - "SELF_SIGNED_CERT_IN_CHAIN", - "UNABLE_TO_VERIFY_LEAF_SIGNATURE", - "CERT_HAS_EXPIRED", - "ERR_TLS_CERT_ALTNAME_INVALID", -]); - -function buildNetworkError(error: unknown, method: HttpMethod, url: string): NetworkError { - const fallback = errorMessage(error); - const code = causeCode(error); - const target = networkTarget(url); - return new NetworkError(networkMessage(code, target, fallback), { - method, - url, - cause: code ?? fallback, - }); -} - -function networkMessage(code: string | null, target: NetworkTarget, fallback: string): string { - if (code === null) { - return `Could not reach Metabase: ${fallback}`; - } - if (TLS_ERROR_CODES.has(code)) { - return ( - `Could not reach Metabase: TLS error contacting ${target.host} (${code}) — ` + - `the certificate could not be verified, or https:// was used against a plain-HTTP server.` - ); - } - const hint = NETWORK_HINTS[code]; - if (hint === undefined) { - return `Could not reach Metabase: ${fallback} (${code})`; - } - return `Could not reach Metabase: ${hint(target)}`; -} - -function causeCode(error: unknown): string | null { - const cause = error instanceof Error ? error.cause : undefined; - if (cause instanceof Error && "code" in cause && typeof cause.code === "string") { - return cause.code; - } - return null; -} - -function networkTarget(url: string): NetworkTarget { - const parsed = new URL(url); - return { host: parsed.host, hostname: parsed.hostname }; -} - async function readBodyForError(response: Response): Promise { try { const buffer = Buffer.from(await response.arrayBuffer()); diff --git a/src/core/http/fetch-capture.ts b/src/core/http/fetch-capture.ts new file mode 100644 index 0000000..39aa1cf --- /dev/null +++ b/src/core/http/fetch-capture.ts @@ -0,0 +1,88 @@ +// Test-only fetch double (the keyring-mock pattern): scripted responses plus a capture of every +// call, shared by the client/oauth/logout suites so each doesn't grow its own drifting stub. + +export interface CapturedFetchCall { + url: string; + method: string; + headers: Record; + body: string | null; +} + +type ResponseFactory = () => Response | Promise; +export type FetchScript = ReadonlyArray; + +export interface FetchCapture { + fetch: typeof fetch; + calls: CapturedFetchCall[]; +} + +export function captureFetch(script: FetchScript): FetchCapture { + const queue = [...script]; + const calls: CapturedFetchCall[] = []; + const fetchImpl: typeof fetch = async (input, init) => { + calls.push({ + url: typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url, + method: init?.method ?? "GET", + headers: headersToRecord(init?.headers), + body: bodyText(init?.body), + }); + const next = queue.shift(); + if (next === undefined) { + throw new Error("captureFetch: no more responses queued"); + } + if (next instanceof Error) { + throw next; + } + return typeof next === "function" ? await next() : next; + }; + return { fetch: fetchImpl, calls }; +} + +export function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +function bodyText(body: RequestInit["body"]): string | null { + if (body === undefined || body === null) { + return null; + } + if (typeof body === "string") { + return body; + } + if (body instanceof URLSearchParams) { + return body.toString(); + } + throw new Error("captureFetch: unsupported request body type"); +} + +function headersToRecord(init: RequestInit["headers"]): Record { + const result: Record = {}; + if (!init) { + return result; + } + if (init instanceof Headers) { + for (const [key, value] of init.entries()) { + result[key] = value; + } + return result; + } + if (Array.isArray(init)) { + for (const entry of init) { + const key = entry[0]; + const value = entry[1]; + if (typeof key === "string" && typeof value === "string") { + result[key] = value; + } + } + return result; + } + for (const [key, value] of Object.entries(init)) { + if (typeof value === "string") { + result[key] = value; + } + } + return result; +} diff --git a/src/core/http/network-error.ts b/src/core/http/network-error.ts new file mode 100644 index 0000000..1441e88 --- /dev/null +++ b/src/core/http/network-error.ts @@ -0,0 +1,69 @@ +import { errorMessage, NetworkError } from "../errors"; + +interface NetworkTarget { + host: string; + hostname: string; +} + +const NETWORK_HINTS: Record string> = { + ECONNREFUSED: (t) => + `Connection refused by ${t.host} — is Metabase running and is the port correct?`, + ENOTFOUND: (t) => `Host not found: ${t.hostname} — check the URL.`, + EAI_AGAIN: (t) => `Could not resolve ${t.hostname} — check your network connection and the URL.`, + ECONNRESET: (t) => + `Connection to ${t.host} was reset — the server may have closed it, or http/https may be mismatched.`, + ETIMEDOUT: (t) => `Connection to ${t.host} timed out — check the host, port, and your network.`, +}; + +const TLS_ERROR_CODES: ReadonlySet = new Set([ + "EPROTO", + "DEPTH_ZERO_SELF_SIGNED_CERT", + "SELF_SIGNED_CERT_IN_CHAIN", + "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + "CERT_HAS_EXPIRED", + "ERR_TLS_CERT_ALTNAME_INVALID", +]); + +// Map a fetch transport failure (DNS, refused, reset, TLS) to a NetworkError with a host-aware +// hint. Shared by the typed client and the OAuth protocol boundary so both surface the same +// diagnostics instead of a bare `TypeError: fetch failed`. +export function buildNetworkError(error: unknown, method: string, url: string): NetworkError { + const fallback = errorMessage(error); + const code = causeCode(error); + const target = networkTarget(url); + return new NetworkError(networkMessage(code, target, fallback), { + method, + url, + cause: code ?? fallback, + }); +} + +function networkMessage(code: string | null, target: NetworkTarget, fallback: string): string { + if (code === null) { + return `Could not reach Metabase: ${fallback}`; + } + if (TLS_ERROR_CODES.has(code)) { + return ( + `Could not reach Metabase: TLS error contacting ${target.host} (${code}) — ` + + `the certificate could not be verified, or https:// was used against a plain-HTTP server.` + ); + } + const hint = NETWORK_HINTS[code]; + if (hint === undefined) { + return `Could not reach Metabase: ${fallback} (${code})`; + } + return `Could not reach Metabase: ${hint(target)}`; +} + +function causeCode(error: unknown): string | null { + const cause = error instanceof Error ? error.cause : undefined; + if (cause instanceof Error && "code" in cause && typeof cause.code === "string") { + return cause.code; + } + return null; +} + +function networkTarget(url: string): NetworkTarget { + const parsed = new URL(url); + return { host: parsed.host, hostname: parsed.hostname }; +} diff --git a/src/core/http/oauth.test.ts b/src/core/http/oauth.test.ts new file mode 100644 index 0000000..450e7c2 --- /dev/null +++ b/src/core/http/oauth.test.ts @@ -0,0 +1,269 @@ +import { afterEach, assert, describe, expect, it, vi } from "vitest"; + +import { ConfigError, NetworkError } from "../errors"; + +import { USER_AGENT } from "./client"; +import { HttpError } from "./errors"; +import { captureFetch, jsonResponse, type FetchCapture, type FetchScript } from "./fetch-capture"; +import { + discoverMetadata, + exchangeCode, + OAUTH_UNSUPPORTED_MESSAGE, + refreshTokens, + revokeToken, + tryDiscoverMetadata, +} from "./oauth"; + +function installFetch(script: FetchScript): FetchCapture { + const capture = captureFetch(script); + vi.stubGlobal("fetch", capture.fetch); + return capture; +} + +const TOKEN_ENDPOINT = "https://mb.example.com/oauth/token"; + +describe("oauth HTTP boundary", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("discovers authorization server metadata", async () => { + const stub = installFetch([ + jsonResponse({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: TOKEN_ENDPOINT, + registration_endpoint: "https://mb.example.com/oauth/register", + }), + ]); + const metadata = await discoverMetadata("https://mb.example.com"); + expect(metadata).toEqual({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: TOKEN_ENDPOINT, + registration_endpoint: "https://mb.example.com/oauth/register", + }); + expect(stub.calls[0]?.url).toBe( + "https://mb.example.com/.well-known/oauth-authorization-server", + ); + }); + + it("discovers metadata for an instance hosted under a subpath", async () => { + const stub = installFetch([ + jsonResponse({ + issuer: "https://my.org.com/metabase", + authorization_endpoint: "https://my.org.com/metabase/oauth/authorize", + token_endpoint: "https://my.org.com/metabase/oauth/token", + }), + ]); + const metadata = await discoverMetadata("https://my.org.com/metabase"); + expect(metadata).toEqual({ + issuer: "https://my.org.com/metabase", + authorization_endpoint: "https://my.org.com/metabase/oauth/authorize", + token_endpoint: "https://my.org.com/metabase/oauth/token", + }); + expect(stub.calls[0]?.url).toBe( + "https://my.org.com/metabase/.well-known/oauth-authorization-server", + ); + }); + + it("treats the SPA shell served at the discovery path (pre-v62) as no OAuth support", async () => { + installFetch([ + new Response("Metabase", { + status: 200, + headers: { "content-type": "text/html;charset=utf-8" }, + }), + ]); + expect(await tryDiscoverMetadata("https://mb.example.com")).toBeNull(); + }); + + it("treats a 404 at the discovery path as no OAuth support", async () => { + installFetch([new Response("Not found.", { status: 404 })]); + expect(await tryDiscoverMetadata("https://mb.example.com")).toBeNull(); + }); + + it("discoverMetadata names the required Metabase version when OAuth is unsupported", async () => { + installFetch([new Response("Not found.", { status: 404 })]); + const error = await discoverMetadata("https://mb.example.com").catch( + (caught: unknown) => caught, + ); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe(OAUTH_UNSUPPORTED_MESSAGE); + }); + + it("rejects a discovery document whose endpoints point at another origin", async () => { + installFetch([ + jsonResponse({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + // A tampered document trying to redirect the token exchange (code + PKCE verifier). + token_endpoint: "https://attacker.example.com/oauth/token", + }), + ]); + const error = await discoverMetadata("https://mb.example.com").catch( + (caught: unknown) => caught, + ); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toContain("token endpoint"); + expect(error.message).toContain("does not match the Metabase URL"); + }); + + it("rejects a non-loopback http endpoint (no cleartext token transport)", async () => { + installFetch([ + jsonResponse({ + issuer: "http://mb.example.com", + authorization_endpoint: "http://mb.example.com/oauth/authorize", + token_endpoint: "http://mb.example.com/oauth/token", + }), + ]); + const error = await discoverMetadata("http://mb.example.com").catch( + (caught: unknown) => caught, + ); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toContain("must use https"); + }); + + it("allows a loopback http instance for local development", async () => { + installFetch([ + jsonResponse({ + issuer: "http://localhost:3000", + authorization_endpoint: "http://localhost:3000/oauth/authorize", + token_endpoint: "http://localhost:3000/oauth/token", + }), + ]); + const metadata = await discoverMetadata("http://localhost:3000"); + expect(metadata).toEqual({ + issuer: "http://localhost:3000", + authorization_endpoint: "http://localhost:3000/oauth/authorize", + token_endpoint: "http://localhost:3000/oauth/token", + }); + }); + + it("exchanges an authorization code for tokens with a form-encoded body", async () => { + const stub = installFetch([ + jsonResponse({ + access_token: "acc", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "ref", + scope: "mb:full", + }), + ]); + const tokens = await exchangeCode({ + tokenEndpoint: TOKEN_ENDPOINT, + code: "the-code", + redirectUri: "http://127.0.0.1:5000/callback", + clientId: "client-1", + codeVerifier: "verifier-1", + }); + expect(tokens).toEqual({ + access_token: "acc", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "ref", + scope: "mb:full", + }); + const call = stub.calls[0]; + expect(call?.method).toBe("POST"); + expect(call?.body).toBe( + new URLSearchParams({ + grant_type: "authorization_code", + code: "the-code", + redirect_uri: "http://127.0.0.1:5000/callback", + client_id: "client-1", + code_verifier: "verifier-1", + }).toString(), + ); + }); + + it("refreshes tokens with grant_type=refresh_token", async () => { + const stub = installFetch([ + jsonResponse({ access_token: "acc2", token_type: "Bearer", refresh_token: "ref2" }), + ]); + const tokens = await refreshTokens({ + tokenEndpoint: TOKEN_ENDPOINT, + refreshToken: "ref1", + clientId: "client-1", + }); + expect(tokens).toEqual({ access_token: "acc2", token_type: "Bearer", refresh_token: "ref2" }); + expect(stub.calls[0]?.body).toBe( + new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: "ref1", + client_id: "client-1", + }).toString(), + ); + }); + + it("surfaces the OAuth error_description on a non-2xx token response", async () => { + installFetch([ + jsonResponse({ error: "invalid_grant", error_description: "code is expired" }, 400), + ]); + const error = await refreshTokens({ + tokenEndpoint: TOKEN_ENDPOINT, + refreshToken: "ref1", + clientId: "client-1", + }).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toBe("OAuth token refresh failed (400): code is expired"); + }); + + it("classifies a 5xx token response as a retryable HttpError, not a terminal ConfigError", async () => { + installFetch([jsonResponse({ error: "temporarily_unavailable" }, 503)]); + const error = await refreshTokens({ + tokenEndpoint: TOKEN_ENDPOINT, + refreshToken: "ref1", + clientId: "client-1", + }).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(HttpError); + assert(error instanceof HttpError, "expected HttpError"); + expect(error.status).toBe(503); + expect(error.isRetryable).toBe(true); + }); + + it("maps a transport failure to a host-aware NetworkError, not a bare fetch error", async () => { + const cause = Object.assign(new Error("connect ECONNREFUSED 127.0.0.1:443"), { + code: "ECONNREFUSED", + }); + const transportFailure = Object.assign(new TypeError("fetch failed"), { cause }); + installFetch([transportFailure]); + const error = await refreshTokens({ + tokenEndpoint: TOKEN_ENDPOINT, + refreshToken: "ref1", + clientId: "client-1", + }).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(NetworkError); + assert(error instanceof NetworkError, "expected NetworkError"); + expect(error.message).toBe( + "Could not reach Metabase: Connection refused by mb.example.com — is Metabase running and is the port correct?", + ); + }); + + it("stamps the shared user-agent on OAuth requests", async () => { + const stub = installFetch([ + jsonResponse({ access_token: "acc", token_type: "Bearer", refresh_token: "ref" }), + ]); + await refreshTokens({ + tokenEndpoint: TOKEN_ENDPOINT, + refreshToken: "ref1", + clientId: "client-1", + }); + expect(stub.calls[0]?.headers["user-agent"]).toBe(USER_AGENT); + }); + + it("revokes a token with the token and client_id in the form body", async () => { + const stub = installFetch([new Response("", { status: 200 })]); + await revokeToken({ + revocationEndpoint: "https://mb.example.com/oauth/revoke", + token: "ref1", + clientId: "client-1", + }); + expect(stub.calls[0]?.body).toBe( + new URLSearchParams({ token: "ref1", client_id: "client-1" }).toString(), + ); + }); +}); diff --git a/src/core/http/oauth.ts b/src/core/http/oauth.ts new file mode 100644 index 0000000..ee07b5e --- /dev/null +++ b/src/core/http/oauth.ts @@ -0,0 +1,273 @@ +import { z } from "zod"; + +import { JSON_CONTENT_TYPE, parseJsonResult } from "../../runtime/json"; +import { ConfigError, errorMessage, TimeoutError } from "../errors"; +import { assertEndpointOrigin } from "../url"; + +import { USER_AGENT } from "./client"; +import { HttpError, isRetryableStatus } from "./errors"; +import { buildNetworkError } from "./network-error"; + +const DISCOVERY_PATH = "/.well-known/oauth-authorization-server"; +const FORM_CONTENT_TYPE = "application/x-www-form-urlencoded"; +const OAUTH_TIMEOUT_MS = 30_000; + +type OAuthMethod = "GET" | "POST"; + +interface OAuthRequest { + url: string; + method: OAuthMethod; + headers: Record; + body?: string | URLSearchParams; +} + +// Single fetch path for every OAuth protocol call: stamps the shared user-agent, enforces the +// timeout, and maps transport failures to the same NetworkError/TimeoutError taxonomy as the typed +// client — so a refresh that can't reach the server surfaces a diagnostic instead of a bare +// `TypeError: fetch failed` that masks the original error. +async function oauthFetch(request: OAuthRequest): Promise { + const timeoutSignal = AbortSignal.timeout(OAUTH_TIMEOUT_MS); + try { + return await fetch(request.url, { + method: request.method, + headers: { ...request.headers, "user-agent": USER_AGENT }, + ...(request.body !== undefined ? { body: request.body } : {}), + signal: timeoutSignal, + }); + } catch (error) { + if (timeoutSignal.aborted) { + throw new TimeoutError(`OAuth request timed out after ${OAUTH_TIMEOUT_MS}ms`, { + kind: "http", + method: request.method, + url: request.url, + timeoutMs: OAUTH_TIMEOUT_MS, + }); + } + throw buildNetworkError(error, request.method, request.url); + } +} + +// The CLI always authorizes with the single full-access scope; it never requests anything narrower, +// so scope is a fixed constant rather than a per-call parameter. +export const OAUTH_SCOPE = "mb:full"; + +export const OAuthServerMetadata = z + .object({ + issuer: z.string(), + authorization_endpoint: z.string(), + token_endpoint: z.string(), + registration_endpoint: z.string().optional(), + revocation_endpoint: z.string().optional(), + }) + .loose(); +export type OAuthServerMetadata = z.infer; + +export const RegisteredClient = z.object({ client_id: z.string() }).loose(); +export type RegisteredClient = z.infer; + +export const OAuthTokens = z + .object({ + access_token: z.string(), + token_type: z.string(), + expires_in: z.number().int().optional(), + refresh_token: z.string().optional(), + }) + .loose(); +export type OAuthTokens = z.infer; + +const OAuthErrorBody = z + .object({ error: z.string(), error_description: z.string().optional() }) + .loose(); + +export interface ClientRegistration { + registrationEndpoint: string; + redirectUri: string; + clientName: string; +} + +export interface CodeExchange { + tokenEndpoint: string; + code: string; + redirectUri: string; + clientId: string; + codeVerifier: string; +} + +export interface TokenRefresh { + tokenEndpoint: string; + refreshToken: string; + clientId: string; +} + +export interface TokenRevocation { + revocationEndpoint: string; + token: string; + clientId: string; +} + +async function readJson(response: Response, schema: z.ZodType, source: string): Promise { + let raw: unknown; + try { + raw = await response.json(); + } catch (error) { + throw new ConfigError(`${source}: invalid JSON response (${errorMessage(error)})`); + } + const parsed = schema.safeParse(raw); + if (!parsed.success) { + throw new ConfigError(`${source}: unexpected response shape`); + } + return parsed.data; +} + +async function failure( + response: Response, + source: string, + method: OAuthMethod, + url: string, +): Promise { + const rawBody = await response.text().catch(() => ""); + // Transient/server statuses (5xx, 429, 408) go through the HTTP taxonomy — retryable, exit 1 — + // not ConfigError. A ConfigError from a refresh is read as a dead grant ("log in again"); a + // token-endpoint blip must not be mistaken for one. + if (isRetryableStatus(response.status)) { + return new HttpError({ + status: response.status, + statusText: response.statusText, + method, + url, + responseHeaders: response.headers, + rawBody, + }); + } + const parsed = parseJsonResult(rawBody, OAuthErrorBody); + if (parsed.ok) { + const detail = parsed.value.error_description ?? parsed.value.error; + return new ConfigError(`${source} failed (${response.status}): ${detail}`); + } + return new ConfigError(`${source} failed (${response.status})`); +} + +async function postForm( + url: string, + params: Record, + schema: z.ZodType, + source: string, +): Promise { + const response = await oauthFetch({ + url, + method: "POST", + headers: { "content-type": FORM_CONTENT_TYPE, accept: JSON_CONTENT_TYPE }, + body: new URLSearchParams(params), + }); + if (!response.ok) { + throw await failure(response, source, "POST", url); + } + return readJson(response, schema, source); +} + +export const OAUTH_UNSUPPORTED_MESSAGE = + "this Metabase does not support OAuth login (requires Metabase v62 or newer)"; + +// Probe whether the server exposes an OAuth authorization server. Returns null when it does not: +// pre-v62 Metabase answers the discovery path with the SPA shell (200 text/html) or a 404, so +// "unsupported" is any non-2xx or non-JSON response. Network-level failures still throw. +// The well-known path is appended after any subpath (base + /.well-known/...), not inserted +// between host and path as RFC 8414 prescribes — Metabase routes it like /api/*, so a +// subpath-hosted instance serves discovery under its prefix. +export async function tryDiscoverMetadata(baseUrl: string): Promise { + const url = `${baseUrl}${DISCOVERY_PATH}`; + const response = await oauthFetch({ + url, + method: "GET", + headers: { accept: JSON_CONTENT_TYPE }, + }); + const contentType = response.headers.get("content-type") ?? ""; + if (!response.ok || !contentType.includes("json")) { + await response.body?.cancel().catch(() => undefined); + return null; + } + const metadata = await readJson(response, OAuthServerMetadata, "OAuth discovery"); + // Pin every endpoint we will send secrets to back to the configured base URL's origin before + // any caller uses them, so a tampered discovery document can't redirect tokens to another host. + assertEndpointOrigin(metadata.issuer, baseUrl, "issuer"); + assertEndpointOrigin(metadata.authorization_endpoint, baseUrl, "authorization endpoint"); + assertEndpointOrigin(metadata.token_endpoint, baseUrl, "token endpoint"); + if (metadata.registration_endpoint !== undefined) { + assertEndpointOrigin(metadata.registration_endpoint, baseUrl, "registration endpoint"); + } + if (metadata.revocation_endpoint !== undefined) { + assertEndpointOrigin(metadata.revocation_endpoint, baseUrl, "revocation endpoint"); + } + return metadata; +} + +export async function discoverMetadata(baseUrl: string): Promise { + const metadata = await tryDiscoverMetadata(baseUrl); + if (metadata === null) { + throw new ConfigError(OAUTH_UNSUPPORTED_MESSAGE); + } + return metadata; +} + +export async function registerClient(input: ClientRegistration): Promise { + const response = await oauthFetch({ + url: input.registrationEndpoint, + method: "POST", + headers: { "content-type": JSON_CONTENT_TYPE, accept: JSON_CONTENT_TYPE }, + body: JSON.stringify({ + application_type: "native", + client_name: input.clientName, + redirect_uris: [input.redirectUri], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "none", + scope: OAUTH_SCOPE, + }), + }); + if (!response.ok) { + throw await failure(response, "OAuth client registration", "POST", input.registrationEndpoint); + } + return readJson(response, RegisteredClient, "OAuth client registration"); +} + +export function exchangeCode(input: CodeExchange): Promise { + return postForm( + input.tokenEndpoint, + { + grant_type: "authorization_code", + code: input.code, + redirect_uri: input.redirectUri, + client_id: input.clientId, + code_verifier: input.codeVerifier, + }, + OAuthTokens, + "OAuth token exchange", + ); +} + +export function refreshTokens(input: TokenRefresh): Promise { + return postForm( + input.tokenEndpoint, + { + grant_type: "refresh_token", + refresh_token: input.refreshToken, + client_id: input.clientId, + }, + OAuthTokens, + "OAuth token refresh", + ); +} + +export async function revokeToken(input: TokenRevocation): Promise { + const response = await oauthFetch({ + url: input.revocationEndpoint, + method: "POST", + headers: { "content-type": FORM_CONTENT_TYPE }, + body: new URLSearchParams({ token: input.token, client_id: input.clientId }), + }); + if (!response.ok) { + throw await failure(response, "OAuth token revocation", "POST", input.revocationEndpoint); + } + // Drain the (ignored) success body so the socket is released and can't delay process exit. + await response.body?.cancel().catch(() => undefined); +} diff --git a/src/core/url.test.ts b/src/core/url.test.ts index 31761cd..e378d51 100644 --- a/src/core/url.test.ts +++ b/src/core/url.test.ts @@ -2,7 +2,7 @@ import * as fc from "fast-check"; import { assert, describe, expect, it } from "vitest"; import { ConfigError } from "./errors"; -import { normalizeUrl, originOnly } from "./url"; +import { assertEndpointOrigin, displayUrl, isLoopbackHost, normalizeUrl } from "./url"; describe("normalizeUrl", () => { it("strips a single trailing slash", () => { @@ -33,25 +33,29 @@ describe("normalizeUrl", () => { }); }); -describe("originOnly", () => { - it("returns the origin for a plain URL", () => { - expect(originOnly("https://m.example.com")).toBe("https://m.example.com"); +describe("displayUrl", () => { + it("returns a plain URL unchanged", () => { + expect(displayUrl("https://m.example.com")).toBe("https://m.example.com"); }); - it("strips path, query, and fragment", () => { - expect(originOnly("https://m.example.com/path?q=1#frag")).toBe("https://m.example.com"); + it("keeps a subpath (instances hosted under a path stay distinguishable)", () => { + expect(displayUrl("https://my.org.com/metabase")).toBe("https://my.org.com/metabase"); + }); + + it("strips query and fragment but keeps the path", () => { + expect(displayUrl("https://m.example.com/path?q=1#frag")).toBe("https://m.example.com/path"); }); it("strips embedded credentials", () => { - expect(originOnly("https://user:pass@m.example.com")).toBe("https://m.example.com"); + expect(displayUrl("https://user:pass@m.example.com")).toBe("https://m.example.com"); }); it("preserves non-default port", () => { - expect(originOnly("https://m.example.com:8443/x")).toBe("https://m.example.com:8443"); + expect(displayUrl("https://m.example.com:8443/x")).toBe("https://m.example.com:8443/x"); }); it("throws on a value that cannot be parsed as a URL", () => { - expect(() => originOnly("not a url")).toThrow(); + expect(() => displayUrl("not a url")).toThrow(); }); }); @@ -99,24 +103,99 @@ describe("normalizeUrl edge cases", () => { }); }); -describe("originOnly edge cases", () => { +describe("displayUrl edge cases", () => { it.each([ ["https://m.example.com", "https://m.example.com"], ["https://m.example.com/", "https://m.example.com"], - ["https://m.example.com/path", "https://m.example.com"], - ["https://m.example.com/a/b/c", "https://m.example.com"], + ["https://m.example.com/path", "https://m.example.com/path"], + ["https://m.example.com/a/b/c", "https://m.example.com/a/b/c"], + ["https://my.org.com/metabase/", "https://my.org.com/metabase"], ["https://m.example.com/?q=1", "https://m.example.com"], ["https://m.example.com/#frag", "https://m.example.com"], - ["https://m.example.com/path?q=1#frag", "https://m.example.com"], + ["https://m.example.com/path?q=1#frag", "https://m.example.com/path"], ["https://user:pass@m.example.com", "https://m.example.com"], - ["https://user:pass@m.example.com:8443/x?q=1", "https://m.example.com:8443"], - ["https://m.example.com:8443/x", "https://m.example.com:8443"], - ["http://m.example.com:80/x", "http://m.example.com"], - ["https://m.example.com:443/x", "https://m.example.com"], - ["http://[::1]:8080/path", "http://[::1]:8080"], - ["http://192.168.1.1/x?y=1", "http://192.168.1.1"], - ])("originOnly(%j) === %j", (input, expected) => { - expect(originOnly(input)).toBe(expected); + ["https://user:pass@m.example.com:8443/x?q=1", "https://m.example.com:8443/x"], + ["https://m.example.com:8443/x", "https://m.example.com:8443/x"], + ["http://m.example.com:80/x", "http://m.example.com/x"], + ["https://m.example.com:443/x", "https://m.example.com/x"], + ["http://[::1]:8080/path", "http://[::1]:8080/path"], + ["http://192.168.1.1/x?y=1", "http://192.168.1.1/x"], + ])("displayUrl(%j) === %j", (input, expected) => { + expect(displayUrl(input)).toBe(expected); + }); +}); + +describe("isLoopbackHost", () => { + it.each(["localhost", "127.0.0.1", "::1", "[::1]", "LOCALHOST"])("treats %j as loopback", (h) => { + expect(isLoopbackHost(h)).toBe(true); + }); + + it.each(["mb.example.com", "127.0.0.2", "169.254.0.1", "example.localhost.evil.com"])( + "treats %j as non-loopback", + (h) => { + expect(isLoopbackHost(h)).toBe(false); + }, + ); +}); + +describe("assertEndpointOrigin", () => { + const base = "https://mb.example.com"; + + it("accepts a same-origin https endpoint", () => { + expect(() => + assertEndpointOrigin("https://mb.example.com/oauth/token", base, "token endpoint"), + ).not.toThrow(); + }); + + it("ignores path differences (compares origin, not the full URL)", () => { + expect(() => + assertEndpointOrigin("https://mb.example.com/oauth/token", `${base}/subpath`, "endpoint"), + ).not.toThrow(); + }); + + it("rejects a cross-origin endpoint", () => { + const error = (() => { + try { + assertEndpointOrigin("https://attacker.example.com/oauth/token", base, "token endpoint"); + } catch (caught: unknown) { + return caught; + } + throw new Error("expected assertEndpointOrigin to throw"); + })(); + expect(error).toBeInstanceOf(ConfigError); + assert(error instanceof ConfigError, "expected ConfigError"); + expect(error.message).toContain("token endpoint"); + expect(error.message).toContain("does not match the Metabase URL"); + }); + + it("rejects a cross-port endpoint (port is part of the origin)", () => { + expect(() => + assertEndpointOrigin("https://mb.example.com:8443/oauth/token", base, "endpoint"), + ).toThrow(ConfigError); + }); + + it("rejects a non-loopback http endpoint", () => { + expect(() => + assertEndpointOrigin( + "http://mb.example.com/oauth/token", + "http://mb.example.com", + "endpoint", + ), + ).toThrow("must use https"); + }); + + it("allows an http endpoint on a loopback host", () => { + expect(() => + assertEndpointOrigin( + "http://localhost:3000/oauth/token", + "http://localhost:3000", + "endpoint", + ), + ).not.toThrow(); + }); + + it("rejects an unparseable endpoint", () => { + expect(() => assertEndpointOrigin("not a url", base, "endpoint")).toThrow(ConfigError); }); }); @@ -198,7 +277,42 @@ describe("normalizeUrl property tests", () => { }); }); -describe("originOnly property tests", () => { +describe("assertEndpointOrigin property tests", () => { + const hostname = fc + .stringMatching(/^[a-z][a-z0-9-]{0,30}\.[a-z]{2,6}$/) + .filter((value) => value.length > 0); + const optionalPort = fc.option( + fc.integer({ min: 1, max: 65535 }).map((port) => `:${port}`), + { nil: "" }, + ); + const origin = fc.tuple(hostname, optionalPort).map(([host, port]) => `https://${host}${port}`); + const path = fc + .array(fc.stringMatching(/^[a-z0-9]{1,8}$/), { minLength: 0, maxLength: 4 }) + .map((parts) => (parts.length === 0 ? "" : `/${parts.join("/")}`)); + + it("property: any path on the base origin is accepted", () => { + fc.assert( + fc.property(origin, path, (base, endpointPath) => { + expect(() => + assertEndpointOrigin(`${base}${endpointPath}`, base, "endpoint"), + ).not.toThrow(); + }), + ); + }); + + it("property: any endpoint on a different origin is rejected with ConfigError", () => { + fc.assert( + fc.property(origin, origin, path, (endpointOrigin, base, endpointPath) => { + fc.pre(new URL(endpointOrigin).origin !== new URL(base).origin); + expect(() => + assertEndpointOrigin(`${endpointOrigin}${endpointPath}`, base, "endpoint"), + ).toThrow(ConfigError); + }), + ); + }); +}); + +describe("displayUrl property tests", () => { const safeHost = fc .stringMatching(/^[a-z][a-z0-9-]{0,30}\.[a-z]{2,6}$/) .filter((host) => !host.startsWith("xn--")); @@ -209,13 +323,13 @@ describe("originOnly property tests", () => { fc.assert( fc.property(safeHost, optionalUser, (host, user) => { const input = `https://${user}${host}/some/path?q=1#frag`; - const result = originOnly(input); + const result = displayUrl(input); expect(result.includes("@")).toBe(false); }), ); }); - it("property: result has no path, query, or fragment", () => { + it("property: result keeps the path but drops query and fragment", () => { fc.assert( fc.property( safeHost, @@ -223,11 +337,8 @@ describe("originOnly property tests", () => { (host, parts) => { const path = parts.length > 0 ? `/${parts.join("/")}` : ""; const input = `http://${host}${path}?key=value#section`; - const result = originOnly(input); - expect(result.includes("?")).toBe(false); - expect(result.includes("#")).toBe(false); - const afterScheme = result.replace(/^https?:\/\//, ""); - expect(afterScheme.includes("/")).toBe(false); + const result = displayUrl(input); + expect(result).toBe(`http://${host}${path}`); }, ), ); diff --git a/src/core/url.ts b/src/core/url.ts index 6976478..6f47ee9 100644 --- a/src/core/url.ts +++ b/src/core/url.ts @@ -8,13 +8,39 @@ export function normalizeUrl(input: string): string { return trimmed; } -export function originOnly(input: string): string { +// Drops credentials, query, and fragment for display, but keeps the path — a Metabase instance may +// be hosted under a subpath (https://my.org.com/metabase), and two instances on one host must stay +// distinguishable in `auth status` / `auth list` output. +export function displayUrl(input: string): string { const parsed = new URL(input); - parsed.username = ""; - parsed.password = ""; - return parsed.origin; + const path = parsed.pathname.replace(/\/+$/, ""); + return `${parsed.origin}${path}`; } -export function localUrl(port: number): string { - return `http://localhost:${port}`; +const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "::1", "[::1]"]); + +export function isLoopbackHost(hostname: string): boolean { + return LOOPBACK_HOSTS.has(hostname.toLowerCase()); +} + +// Guard against a tampered or hostile authorization-server metadata document redirecting the +// CLI's OAuth secrets (authorization code, PKCE verifier, refresh token) to another host. Every +// endpoint the CLI sends secrets to must share the configured Metabase URL's origin, and must use +// https unless it is a loopback dev instance. +export function assertEndpointOrigin(endpoint: string, baseUrl: string, label: string): void { + let parsed: URL; + try { + parsed = new URL(endpoint); + } catch { + throw new ConfigError(`OAuth ${label} is not a valid URL: ${endpoint}`); + } + const base = new URL(baseUrl); + if (parsed.protocol !== "https:" && !isLoopbackHost(parsed.hostname)) { + throw new ConfigError(`OAuth ${label} must use https: ${endpoint}`); + } + if (parsed.origin !== base.origin) { + throw new ConfigError( + `OAuth ${label} origin (${parsed.origin}) does not match the Metabase URL (${base.origin})`, + ); + } } diff --git a/src/runtime/paginate.test.ts b/src/runtime/paginate.test.ts index a4df6de..8b0737a 100644 --- a/src/runtime/paginate.test.ts +++ b/src/runtime/paginate.test.ts @@ -6,7 +6,7 @@ import { collectPaginated, paginate } from "./paginate"; const CONFIG: ClientCredentials = { url: "https://m.example.com", - apiKey: "mb_test_key", + credential: { kind: "apiKey", apiKey: "mb_test_key" }, }; const Card = z.object({ id: z.number().int(), name: z.string() }); diff --git a/src/runtime/process.test.ts b/src/runtime/process.test.ts index 7d0c50a..f51364b 100644 --- a/src/runtime/process.test.ts +++ b/src/runtime/process.test.ts @@ -1,6 +1,85 @@ +import * as fc from "fast-check"; import { describe, expect, it } from "vitest"; -import { ProcessNotFoundError, runProcess, runProcessBinary, streamProcess } from "./process"; +import { + browserOpener, + ProcessNotFoundError, + runProcess, + runProcessBinary, + streamProcess, +} from "./process"; + +const AUTHORIZE_URL = + "https://mb.example.com/oauth/authorize?response_type=code&client_id=abc&state=xyz&scope=mb:full"; + +describe("browserOpener", () => { + it("passes the URL as a single argv element on darwin and linux", () => { + expect(browserOpener("darwin", AUTHORIZE_URL)).toEqual({ + command: "open", + args: [AUTHORIZE_URL], + windowsVerbatim: false, + }); + expect(browserOpener("linux", AUTHORIZE_URL)).toEqual({ + command: "xdg-open", + args: [AUTHORIZE_URL], + windowsVerbatim: false, + }); + }); + + it("escapes cmd metacharacters so a `&`-laden URL survives `cmd /c start` intact", () => { + expect(browserOpener("win32", AUTHORIZE_URL)).toEqual({ + command: "cmd", + args: [ + "/c", + "start", + '""', + "https://mb.example.com/oauth/authorize?response_type=code^&client_id=abc^&state=xyz^&scope=mb:full", + ], + windowsVerbatim: true, + }); + }); + + it("escapes injected command separators in a hostile authorization endpoint", () => { + const hostile = "https://mb.example.com/authorize?x=1&calc.exe|whoami>out"; + expect(browserOpener("win32", hostile).args[3]).toBe( + "https://mb.example.com/authorize?x=1^&calc.exe^|whoami^>out", + ); + }); + + it("property: every cmd metacharacter is ^-escaped and the URL round-trips", () => { + const urlChar = fc.constantFrom(...'&|<>^()"%!', ..."abz09:/?=._-~"); + const url = fc.array(urlChar, { minLength: 1, maxLength: 64 }).map((chars) => chars.join("")); + fc.assert( + fc.property(url, (input) => { + const escaped = browserOpener("win32", input).args[3] ?? ""; + expect(decodeCmdEscapes(escaped)).toBe(input); + }), + ); + }); +}); + +const CMD_METACHARACTERS = new Set('&|<>^()"%!'); + +// Walks the escaped string the way cmd.exe would: a ^ must be followed by the metacharacter it +// escapes, and no metacharacter may appear outside such a pair. Returns the decoded original. +function decodeCmdEscapes(escaped: string): string { + let decoded = ""; + let i = 0; + while (i < escaped.length) { + const char = escaped[i] ?? ""; + if (char === "^") { + const next = escaped[i + 1] ?? ""; + expect(CMD_METACHARACTERS.has(next)).toBe(true); + decoded += next; + i += 2; + continue; + } + expect(CMD_METACHARACTERS.has(char)).toBe(false); + decoded += char; + i += 1; + } + return decoded; +} describe("runProcess", () => { it("captures stdout and exit code 0", async () => { diff --git a/src/runtime/process.ts b/src/runtime/process.ts index e2ddb50..3f8f952 100644 --- a/src/runtime/process.ts +++ b/src/runtime/process.ts @@ -149,3 +149,54 @@ export function streamProcess( child.on("close", (code) => resolve(code)); }); } + +interface BrowserOpener { + command: string; + args: string[]; + windowsVerbatim: boolean; +} + +// cmd.exe treats these as control operators. An authorize URL is always multi-param, so it carries +// `&`; handed to `cmd /c start` unescaped, cmd splits on the first `&`, the browser opens a +// truncated URL (login can't complete), and the trailing fragments execute as commands — a +// server-influenced injection since the authorization endpoint comes from discovery metadata. +const CMD_METACHARACTERS = /[&|<>^()"%!]/g; + +function escapeForCmd(value: string): string { + return value.replace(CMD_METACHARACTERS, (char) => `^${char}`); +} + +export function browserOpener(platform: NodeJS.Platform, url: string): BrowserOpener { + if (platform === "darwin") { + return { command: "open", args: [url], windowsVerbatim: false }; + } + if (platform === "win32") { + // `start` reads a leading quoted token as the window title, so pass an empty title before the + // URL. windowsVerbatimArguments stops libuv from re-quoting our hand-escaped command line. + return { + command: "cmd", + args: ["/c", "start", '""', escapeForCmd(url)], + windowsVerbatim: true, + }; + } + return { command: "xdg-open", args: [url], windowsVerbatim: false }; +} + +// Best-effort launch of the platform browser, detached so the CLI keeps running while the user +// completes login. Resolves false (rather than throwing) when the opener binary is missing, so +// callers can fall back to printing the URL for manual paste. +export function openBrowser(url: string): Promise { + const opener = browserOpener(process.platform, url); + return new Promise((resolve) => { + const child = spawn(opener.command, opener.args, { + stdio: "ignore", + detached: true, + ...(opener.windowsVerbatim ? { windowsVerbatimArguments: true } : {}), + }); + child.on("error", () => resolve(false)); + child.on("spawn", () => { + child.unref(); + resolve(true); + }); + }); +} diff --git a/tests/e2e/bootstrap-data.ts b/tests/e2e/bootstrap-data.ts index 08b4ae4..0fceb46 100644 --- a/tests/e2e/bootstrap-data.ts +++ b/tests/e2e/bootstrap-data.ts @@ -39,6 +39,10 @@ export type SeededIds = z.infer; export const ServerIdentity = z.object({ version: ParsedVersionSchema.nullable(), tokenFeatures: TokenFeatures.nullable(), + // Whether the server exposes an OAuth authorization server (Metabase v62+); probed live during + // bootstrap. Defaults false for bootstrap files written before this field existed — re-run + // `bun run e2e:bootstrap` after deleting the stale file to refresh it. + oauthSupported: z.boolean().default(false), }); export type ServerIdentity = z.infer; diff --git a/tests/e2e/field.e2e.test.ts b/tests/e2e/field.e2e.test.ts index 8ebc271..bf1925e 100644 --- a/tests/e2e/field.e2e.test.ts +++ b/tests/e2e/field.e2e.test.ts @@ -20,7 +20,10 @@ describe("field e2e", () => { }); async function resolveFieldId(tableId: number, fieldName: string): Promise { - const client = createClient({ url: bootstrap.baseUrl, apiKey: bootstrap.adminApiKey }); + const client = createClient({ + url: bootstrap.baseUrl, + credential: { kind: "apiKey", apiKey: bootstrap.adminApiKey }, + }); const metadata = await client.requestParsed( TableQueryMetadata, `/api/table/${tableId}/query_metadata`, diff --git a/tests/e2e/oauth.e2e.test.ts b/tests/e2e/oauth.e2e.test.ts new file mode 100644 index 0000000..a544c51 --- /dev/null +++ b/tests/e2e/oauth.e2e.test.ts @@ -0,0 +1,104 @@ +import { afterEach, beforeAll, describe, expect, it } from "vitest"; + +import type { OAuthCredential } from "../../src/core/auth/credential"; +import { oauthLogin } from "../../src/core/auth/oauth-login"; +import { refreshOAuthCredential, revokeOAuthCredential } from "../../src/core/auth/oauth-session"; +import { AuthProfileListEnvelope } from "../../src/commands/auth/list"; +import { parseJson } from "../../src/runtime/json"; + +import { readBootstrap, type E2EBootstrap } from "./bootstrap-data"; +import { cleanupConfigHome, mkTempConfigHome, runCli } from "./run-cli"; +import { requireOAuthServer } from "./server-gate"; +import { + consentingBrowser, + fetchCurrentUserWithBearer, + refreshGrantStatus, + writeOAuthProfileIntoConfigHome, +} from "./setup/oauth-harness"; + +// The OAuth backend (bearer bridge + /oauth/* endpoints) ships in Metabase v62, so head images +// have it and the 58–61 matrix stacks don't. The suite self-skips when bootstrap's live discovery +// probe found no OAuth authorization server. +// +// `mb auth login` opens a real browser, so we can't drive it as a subprocess. Instead we run the +// library `oauthLogin()` in-process with an injected "browser" that authenticates as admin and +// completes the consent decision against the live server, then assert the real bearer/refresh/revoke +// lifecycle — the parts unit tests can only mock. + +describe.skipIf(requireOAuthServer() !== null)("oauth e2e", () => { + let bootstrap: E2EBootstrap; + const tempDirs: string[] = []; + + beforeAll(async () => { + bootstrap = await readBootstrap(); + }); + + afterEach(async () => { + await Promise.all(tempDirs.splice(0).map(cleanupConfigHome)); + }); + + async function login(): Promise { + return oauthLogin( + { baseUrl: bootstrap.baseUrl }, + { + openBrowser: consentingBrowser(bootstrap.baseUrl, bootstrap.admin), + onAuthorizeUrl: () => undefined, + now: () => Date.now(), + }, + ); + } + + it("registers a client, completes consent, and the bearer token authenticates the REST API", async () => { + const credential = await login(); + expect(credential.kind).toBe("oauth"); + expect(credential.accessToken).not.toBe(""); + expect(credential.refreshToken).not.toBe(""); + expect(credential.clientId).not.toBe(""); + + const user = await fetchCurrentUserWithBearer(bootstrap.baseUrl, credential.accessToken); + expect(user.email).toBe(bootstrap.admin.email); + expect(user.is_superuser).toBe(true); + }); + + it("refreshes the access token against the real token endpoint and the new token works", async () => { + const credential = await login(); + const refreshed = await refreshOAuthCredential(bootstrap.baseUrl, credential, Date.now()); + + expect(refreshed.accessToken).not.toBe(credential.accessToken); + expect(refreshed.refreshToken).not.toBe(""); // rotation-aware (new or retained) + + const user = await fetchCurrentUserWithBearer(bootstrap.baseUrl, refreshed.accessToken); + expect(user.email).toBe(bootstrap.admin.email); + }); + + it("revokes both tokens server-side so neither survives logout", async () => { + const credential = await login(); + + const revoked = await revokeOAuthCredential(bootstrap.baseUrl, credential); + expect(revoked).toBe(true); + + // The revoked grant is rejected outright: 400 invalid_grant. + expect(await refreshGrantStatus(bootstrap.baseUrl, credential)).toBe(400); + // The server does not cascade refresh-to-access, so the access token must be dead too. + await expect( + fetchCurrentUserWithBearer(bootstrap.baseUrl, credential.accessToken), + ).rejects.toThrow("failed (401)"); + }); + + it("persists the OAuth profile and `auth list` verifies it through the CLI", async () => { + const credential = await login(); + const configHome = await mkTempConfigHome(); + tempDirs.push(configHome); + await writeOAuthProfileIntoConfigHome(configHome, bootstrap.baseUrl, credential); + + const list = await runCli({ args: ["auth", "list", "--json"], configHome }); + expect(list.exitCode, list.stderr).toBe(0); + expect(list.stderr).not.toContain(credential.accessToken); + + const envelope = parseJson(list.stdout, AuthProfileListEnvelope); + expect(envelope.data).toHaveLength(1); + expect(envelope.data[0]?.method).toBe("oauth"); + expect(envelope.data[0]?.status).toBe("ok"); + expect(envelope.data[0]?.user?.isAdmin).toBe(true); + }); +}); diff --git a/tests/e2e/server-gate.ts b/tests/e2e/server-gate.ts index 2d97a2e..6660985 100644 --- a/tests/e2e/server-gate.ts +++ b/tests/e2e/server-gate.ts @@ -30,6 +30,16 @@ export function requireServer(required: Partial): string | null { return failure === null ? null : failure.detail; } +// Gate for the OAuth login suite: a version check would be wrong here (head images without the +// OAuth backend would run and fail), so bootstrap probes the discovery endpoint live and the +// suite keys off that. Re-run `bun run e2e:bootstrap` after switching images. +export function requireOAuthServer(): string | null { + if (readBootstrapSync().server.oauthSupported) { + return null; + } + return "server does not expose an OAuth authorization server (Metabase v62+) — re-run e2e:bootstrap if the image changed"; +} + // True only when the server version is known AND below `minVersion` — the exact condition under // which a non-baseline command's preflight raises a CapabilityError (exit 2) rather than warning // and proceeding on an unknown version. Lets a suite assert the gate fires on the sub-version diff --git a/tests/e2e/setting.e2e.test.ts b/tests/e2e/setting.e2e.test.ts index 3f79ff2..d75bcac 100644 --- a/tests/e2e/setting.e2e.test.ts +++ b/tests/e2e/setting.e2e.test.ts @@ -27,7 +27,10 @@ describe("setting e2e", () => { beforeAll(async () => { bootstrap = await readBootstrap(); - adminClient = createClient({ url: bootstrap.baseUrl, apiKey: bootstrap.adminApiKey }); + adminClient = createClient({ + url: bootstrap.baseUrl, + credential: { kind: "apiKey", apiKey: bootstrap.adminApiKey }, + }); }); afterEach(async () => { diff --git a/tests/e2e/setup/bootstrap.ts b/tests/e2e/setup/bootstrap.ts index 7614aa6..0498b99 100644 --- a/tests/e2e/setup/bootstrap.ts +++ b/tests/e2e/setup/bootstrap.ts @@ -8,6 +8,7 @@ import { errorMessage, isNotFoundError } from "../../../src/core/errors"; import { createClient, type Client } from "../../../src/core/http/client"; import { HttpError } from "../../../src/core/http/errors"; import { backoffDelay, DEFAULT_MAX_RETRIES, runWithRetries } from "../../../src/core/http/retry"; +import { tryDiscoverMetadata } from "../../../src/core/http/oauth"; import { probeServer } from "../../../src/core/version/probe"; import { CardQueryResult } from "../../../src/domain/card"; import { CurrentUser } from "../../../src/domain/user"; @@ -84,22 +85,36 @@ async function main(): Promise { const existing = await readStoredBootstrap(); if (existing && (await canReuseExisting(existing.adminApiKey))) { + // OAuth support depends on the booted image, not on the reused credentials — re-probe it so a + // stale bootstrap file (or an image swap on the same stack) can't pin the wrong answer. + const oauthSupported = (await tryDiscoverMetadata(BASE_URL)) !== null; + if (existing.server.oauthSupported !== oauthSupported) { + await writeStoredBootstrap({ ...existing, server: { ...existing.server, oauthSupported } }); + } process.stdout.write(`bootstrap: reusing ${BOOTSTRAP_FILE_PATH}\n`); return; } const sessionId = await ensureAdminSessionId(); const adminApiKey = await mintApiKey(sessionId, "e2e-admin-key", E2E_GROUPS.ADMIN); - const client = createClient({ url: BASE_URL, apiKey: adminApiKey }); + const client = createClient({ + url: BASE_URL, + credential: { kind: "apiKey", apiKey: adminApiKey }, + }); const apiKeyUser = await client.requestParsed(CurrentUser, "/api/user/current"); const seeded = await seedContent(client); - const server = await probeServer(client, { retries: DEFAULT_MAX_RETRIES }); + const probed = await probeServer(client, { retries: DEFAULT_MAX_RETRIES }); + const oauthSupported = (await tryDiscoverMetadata(BASE_URL)) !== null; + const server = { ...probed, oauthSupported }; const limitedGroupId = await createLimitedGroup(client); await revokeDefaultCollectionAccess(client, limitedGroupId, seeded.defaultCollectionId); const limitedApiKey = await mintApiKey(sessionId, "e2e-limited-key", limitedGroupId); - const limitedClient = createClient({ url: BASE_URL, apiKey: limitedApiKey }); + const limitedClient = createClient({ + url: BASE_URL, + credential: { kind: "apiKey", apiKey: limitedApiKey }, + }); const limitedKeyUser = await limitedClient.requestParsed(CurrentUser, "/api/user/current"); await assertLimitedKeyCannotQueryOrdersCard(limitedClient, seeded.ordersCardId); @@ -464,7 +479,7 @@ async function tryLogin(): Promise | null> { } async function keyStillWorks(apiKey: string): Promise { - const client = createClient({ url: BASE_URL, apiKey }); + const client = createClient({ url: BASE_URL, credential: { kind: "apiKey", apiKey } }); try { await client.requestParsed(CurrentUser, "/api/user/current"); return true; diff --git a/tests/e2e/setup/oauth-harness.ts b/tests/e2e/setup/oauth-harness.ts new file mode 100644 index 0000000..1212741 --- /dev/null +++ b/tests/e2e/setup/oauth-harness.ts @@ -0,0 +1,138 @@ +import { z } from "zod"; + +import type { OAuthCredential } from "../../../src/core/auth/credential"; +import { writeOAuthProfile } from "../../../src/core/auth/storage"; +import { CurrentUser } from "../../../src/domain/user"; +import { parseJson } from "../../../src/runtime/json"; +import type { E2EBootstrap } from "../bootstrap-data"; + +// Browser-simulation and raw-protocol helpers for the OAuth e2e suite. They live under setup/ +// because they speak HTTP to Metabase directly — the sanctioned home for fetch in the e2e tier: +// they stand in for the browser the CLI would open, and probe protocol-level outcomes (bearer +// acceptance, revoked-grant rejection) that no CLI command exposes. + +const SessionResponse = z.object({ id: z.string() }); + +type AdminCredentials = E2EBootstrap["admin"]; + +function decodeHtmlAttr(value: string): string { + return value + .replace(/&/g, "&") + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/</g, "<") + .replace(/>/g, ">"); +} + +// Simulates the browser the CLI would open: log in, render the consent page, approve it, and let +// the server redirect the authorization code to the CLI's loopback callback. +export function consentingBrowser( + baseUrl: string, + admin: AdminCredentials, +): (authorizeUrl: string) => Promise { + return async (authorizeUrl: string): Promise => { + const sessionRes = await fetch(`${baseUrl}/api/session`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ username: admin.email, password: admin.password }), + }); + const sessionId = parseJson(await sessionRes.text(), SessionResponse, { + source: "/api/session", + }).id; + + const authRes = await fetch(authorizeUrl, { + headers: { "x-metabase-session": sessionId }, + redirect: "manual", + }); + const cookie = (authRes.headers.get("set-cookie") ?? "").split(";")[0] ?? ""; + const html = await authRes.text(); + const fields: Record = {}; + for (const tag of html.match(/]*>/gi) ?? []) { + const name = tag.match(/name="([^"]*)"/i)?.[1]; + const value = tag.match(/value="([^"]*)"/i)?.[1]; + if (name !== undefined) { + fields[name] = decodeHtmlAttr(value ?? ""); + } + } + + const decisionRes = await fetch(`${baseUrl}/oauth/authorize/decision`, { + method: "POST", + headers: { + "content-type": "application/x-www-form-urlencoded", + "x-metabase-session": sessionId, + cookie, + }, + body: new URLSearchParams({ ...fields, approved: "true" }), + redirect: "manual", + }); + const location = decisionRes.headers.get("location"); + if (location === null) { + throw new Error(`consent decision did not redirect (status ${decisionRes.status})`); + } + // Hitting the loopback callback delivers the code+state to the CLI's callback server. + await fetch(location); + return true; + }; +} + +// GET /api/user/current with a bearer token; throws with status and body context when rejected. +export async function fetchCurrentUserWithBearer( + baseUrl: string, + accessToken: string, +): Promise { + const res = await fetch(`${baseUrl}/api/user/current`, { + headers: { authorization: `Bearer ${accessToken}` }, + }); + if (!res.ok) { + throw new Error( + `GET /api/user/current with bearer token failed (${res.status}): ${await res.text()}`, + ); + } + return parseJson(await res.text(), CurrentUser, { source: "/api/user/current" }); +} + +// Replay a refresh grant directly against /oauth/token, returning the HTTP status — lets a test +// assert a revoked refresh token is rejected (400 invalid_grant) without a CLI command for it. +export async function refreshGrantStatus( + baseUrl: string, + credential: OAuthCredential, +): Promise { + const res = await fetch(`${baseUrl}/oauth/token`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: credential.refreshToken, + client_id: credential.clientId, + }), + }); + await res.text(); + return res.status; +} + +// Persist an OAuth credential into an isolated config home (plaintext file, never the host +// keychain) so a CLI subprocess pointed at the same home reads the same credential. +export async function writeOAuthProfileIntoConfigHome( + configHome: string, + baseUrl: string, + credential: OAuthCredential, +): Promise { + const prevXdg = process.env["XDG_CONFIG_HOME"]; + const prevKeyring = process.env["METABASE_CLI_DISABLE_KEYRING"]; + process.env["XDG_CONFIG_HOME"] = configHome; + process.env["METABASE_CLI_DISABLE_KEYRING"] = "1"; + try { + await writeOAuthProfile(baseUrl, credential); + } finally { + restoreEnv("XDG_CONFIG_HOME", prevXdg); + restoreEnv("METABASE_CLI_DISABLE_KEYRING", prevKeyring); + } +} + +function restoreEnv(key: string, previous: string | undefined): void { + if (previous === undefined) { + delete process.env[key]; + } else { + process.env[key] = previous; + } +} diff --git a/tests/e2e/setup/reset.ts b/tests/e2e/setup/reset.ts index f633edf..d8e72fa 100644 --- a/tests/e2e/setup/reset.ts +++ b/tests/e2e/setup/reset.ts @@ -9,7 +9,10 @@ async function adminClient(): Promise { return cachedClient; } const bootstrap = await readBootstrap(); - cachedClient = createClient({ url: bootstrap.baseUrl, apiKey: bootstrap.adminApiKey }); + cachedClient = createClient({ + url: bootstrap.baseUrl, + credential: { kind: "apiKey", apiKey: bootstrap.adminApiKey }, + }); return cachedClient; } diff --git a/tests/e2e/transform.e2e.test.ts b/tests/e2e/transform.e2e.test.ts index 2dfebdc..f2a8156 100644 --- a/tests/e2e/transform.e2e.test.ts +++ b/tests/e2e/transform.e2e.test.ts @@ -94,7 +94,10 @@ describe.skipIf(skipReason !== null)("transform e2e", () => { beforeAll(async () => { bootstrap = await readBootstrap(); - adminClient = createClient({ url: bootstrap.baseUrl, apiKey: bootstrap.adminApiKey }); + adminClient = createClient({ + url: bootstrap.baseUrl, + credential: { kind: "apiKey", apiKey: bootstrap.adminApiKey }, + }); }); afterEach(async () => { From 72ce230da6fc3dd54de0b34bb71df1558b91f84e Mon Sep 17 00:00:00 2001 From: Aleksandr Lesnenko Date: Wed, 10 Jun 2026 17:11:53 -0400 Subject: [PATCH 2/3] support only full scope --- src/commands/auth/login.ts | 8 ++++---- src/core/http/oauth.test.ts | 32 +++++++++++++++++++++++++++++++- src/core/http/oauth.ts | 17 +++++++++++++---- tests/e2e/bootstrap-data.ts | 6 +++--- tests/e2e/oauth.e2e.test.ts | 8 +++++--- tests/e2e/server-gate.ts | 5 +++-- 6 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/commands/auth/login.ts b/src/commands/auth/login.ts index 48e18e0..c351bc1 100644 --- a/src/commands/auth/login.ts +++ b/src/commands/auth/login.ts @@ -60,7 +60,7 @@ const loginView: ResourceView = { export default defineMetabaseCommand({ meta: { name: "login", description: "Log in to a Metabase instance for a profile" }, details: - "Interactive login offers browser OAuth (recommended; Metabase v62+) or an API key — older servers fall back to the API key prompt automatically. Browser login opens Metabase, you sign in (password or SSO) and approve, and the CLI stores a refreshing access token. For CI/non-interactive use, supply an API key via --api-key, piped stdin, or $METABASE_API_KEY (first non-empty wins); any of these skips the browser flow, even on a TTY. The URL comes from --url or $METABASE_URL, prompted when stdin is a TTY.", + "Interactive login offers browser OAuth (recommended; Metabase v63+) or an API key — older servers fall back to the API key prompt automatically. Browser login opens Metabase, you sign in (password or SSO) and approve, and the CLI stores a refreshing access token. For CI/non-interactive use, supply an API key via --api-key, piped stdin, or $METABASE_API_KEY (first non-empty wins); any of these skips the browser flow, even on a TTY. The URL comes from --url or $METABASE_URL, prompted when stdin is a TTY.", capabilities: { minVersion: 58 }, args: { ...outputFlags, @@ -146,7 +146,7 @@ export default defineMetabaseCommand({ type LoginMethod = "oauth" | "apiKey"; -// Reaching the server but finding no OAuth authorization server (pre-v62) degrades to the API key +// Reaching the server but finding no CLI-capable OAuth server (pre-v63) degrades to the API key // prompt; not reaching it at all is an error worth stopping on before any credential is collected. async function probeOAuthSupport(url: string): Promise { try { @@ -166,11 +166,11 @@ async function chooseLoginMethod( if (metadata === null) { if (clientId !== undefined) { throw new ConfigError( - "--client-id was given but this Metabase does not support OAuth login (requires Metabase v62 or newer)", + "--client-id was given but this Metabase does not support OAuth login (requires Metabase v63 or newer)", ); } warn( - "This Metabase does not support browser login (requires Metabase v62 or newer); using an API key instead.", + "This Metabase does not support browser login (requires Metabase v63 or newer); using an API key instead.", ); return "apiKey"; } diff --git a/src/core/http/oauth.test.ts b/src/core/http/oauth.test.ts index 450e7c2..8c6c5e9 100644 --- a/src/core/http/oauth.test.ts +++ b/src/core/http/oauth.test.ts @@ -8,6 +8,7 @@ import { captureFetch, jsonResponse, type FetchCapture, type FetchScript } from import { discoverMetadata, exchangeCode, + OAUTH_SCOPE, OAUTH_UNSUPPORTED_MESSAGE, refreshTokens, revokeToken, @@ -34,6 +35,7 @@ describe("oauth HTTP boundary", () => { authorization_endpoint: "https://mb.example.com/oauth/authorize", token_endpoint: TOKEN_ENDPOINT, registration_endpoint: "https://mb.example.com/oauth/register", + scopes_supported: ["agent:sql:read", OAUTH_SCOPE], }), ]); const metadata = await discoverMetadata("https://mb.example.com"); @@ -42,6 +44,7 @@ describe("oauth HTTP boundary", () => { authorization_endpoint: "https://mb.example.com/oauth/authorize", token_endpoint: TOKEN_ENDPOINT, registration_endpoint: "https://mb.example.com/oauth/register", + scopes_supported: ["agent:sql:read", OAUTH_SCOPE], }); expect(stub.calls[0]?.url).toBe( "https://mb.example.com/.well-known/oauth-authorization-server", @@ -67,7 +70,34 @@ describe("oauth HTTP boundary", () => { ); }); - it("treats the SPA shell served at the discovery path (pre-v62) as no OAuth support", async () => { + it("treats an agent-API-only OAuth server (no full-access scope advertised) as no OAuth support", async () => { + installFetch([ + jsonResponse({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: TOKEN_ENDPOINT, + scopes_supported: ["agent:sql:read", "agent:query"], + }), + ]); + expect(await tryDiscoverMetadata("https://mb.example.com")).toBeNull(); + }); + + it("accepts a discovery document that omits scopes_supported", async () => { + installFetch([ + jsonResponse({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: TOKEN_ENDPOINT, + }), + ]); + expect(await tryDiscoverMetadata("https://mb.example.com")).toEqual({ + issuer: "https://mb.example.com", + authorization_endpoint: "https://mb.example.com/oauth/authorize", + token_endpoint: TOKEN_ENDPOINT, + }); + }); + + it("treats the SPA shell served at the discovery path (pre-v60) as no OAuth support", async () => { installFetch([ new Response("Metabase", { status: 200, diff --git a/src/core/http/oauth.ts b/src/core/http/oauth.ts index ee07b5e..20f1c75 100644 --- a/src/core/http/oauth.ts +++ b/src/core/http/oauth.ts @@ -58,6 +58,7 @@ export const OAuthServerMetadata = z token_endpoint: z.string(), registration_endpoint: z.string().optional(), revocation_endpoint: z.string().optional(), + scopes_supported: z.array(z.string()).optional(), }) .loose(); export type OAuthServerMetadata = z.infer; @@ -166,11 +167,13 @@ async function postForm( } export const OAUTH_UNSUPPORTED_MESSAGE = - "this Metabase does not support OAuth login (requires Metabase v62 or newer)"; + "this Metabase does not support OAuth login (requires Metabase v63 or newer)"; -// Probe whether the server exposes an OAuth authorization server. Returns null when it does not: -// pre-v62 Metabase answers the discovery path with the SPA shell (200 text/html) or a 404, so -// "unsupported" is any non-2xx or non-JSON response. Network-level failures still throw. +// Probe whether the server supports OAuth login for the CLI. Returns null when it does not: +// pre-v60 Metabase answers the discovery path with the SPA shell (200 text/html) or a 404, so +// "unsupported" is any non-2xx or non-JSON response; v60–62 expose an OAuth authorization +// server scoped to the agent API/MCP only — see the scopes_supported check below. Network-level +// failures still throw. // The well-known path is appended after any subpath (base + /.well-known/...), not inserted // between host and path as RFC 8414 prescribes — Metabase routes it like /api/*, so a // subpath-hosted instance serves discovery under its prefix. @@ -187,6 +190,12 @@ export async function tryDiscoverMetadata(baseUrl: string): Promise; export const ServerIdentity = z.object({ version: ParsedVersionSchema.nullable(), tokenFeatures: TokenFeatures.nullable(), - // Whether the server exposes an OAuth authorization server (Metabase v62+); probed live during - // bootstrap. Defaults false for bootstrap files written before this field existed — re-run - // `bun run e2e:bootstrap` after deleting the stale file to refresh it. + // Whether the server supports full-API OAuth login (Metabase v63+, full-access scope advertised + // in discovery); probed live during bootstrap. Defaults false for bootstrap files written before + // this field existed — re-run `bun run e2e:bootstrap` after deleting the stale file to refresh it. oauthSupported: z.boolean().default(false), }); export type ServerIdentity = z.infer; diff --git a/tests/e2e/oauth.e2e.test.ts b/tests/e2e/oauth.e2e.test.ts index a544c51..29d2ba7 100644 --- a/tests/e2e/oauth.e2e.test.ts +++ b/tests/e2e/oauth.e2e.test.ts @@ -16,9 +16,11 @@ import { writeOAuthProfileIntoConfigHome, } from "./setup/oauth-harness"; -// The OAuth backend (bearer bridge + /oauth/* endpoints) ships in Metabase v62, so head images -// have it and the 58–61 matrix stacks don't. The suite self-skips when bootstrap's live discovery -// probe found no OAuth authorization server. +// The full-API OAuth backend (the session-middleware bearer bridge plus the mb:full scope) ships +// in Metabase v63, so head images have it and the 58–61 matrix stacks don't: ≤59 has no OAuth +// server at all, and 60–62 expose one scoped to the agent API/MCP only — its bearer tokens 401 on +// the REST API. The suite self-skips when bootstrap's live discovery probe found no authorization +// server advertising the full-access scope. // // `mb auth login` opens a real browser, so we can't drive it as a subprocess. Instead we run the // library `oauthLogin()` in-process with an injected "browser" that authenticates as admin and diff --git a/tests/e2e/server-gate.ts b/tests/e2e/server-gate.ts index 6660985..d807f52 100644 --- a/tests/e2e/server-gate.ts +++ b/tests/e2e/server-gate.ts @@ -32,12 +32,13 @@ export function requireServer(required: Partial): string | null { // Gate for the OAuth login suite: a version check would be wrong here (head images without the // OAuth backend would run and fail), so bootstrap probes the discovery endpoint live and the -// suite keys off that. Re-run `bun run e2e:bootstrap` after switching images. +// suite keys off that. The probe also rejects the agent-API-only OAuth server v60–62 ship +// (no full-access scope advertised). Re-run `bun run e2e:bootstrap` after switching images. export function requireOAuthServer(): string | null { if (readBootstrapSync().server.oauthSupported) { return null; } - return "server does not expose an OAuth authorization server (Metabase v62+) — re-run e2e:bootstrap if the image changed"; + return "server does not support full-API OAuth login (Metabase v63+) — re-run e2e:bootstrap if the image changed"; } // True only when the server version is known AND below `minVersion` — the exact condition under From 9979befc4504631d3830c313bbb4e4b0d475ae03 Mon Sep 17 00:00:00 2001 From: Aleksandr Lesnenko Date: Wed, 10 Jun 2026 19:01:58 -0400 Subject: [PATCH 3/3] 0.1.12 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 41f93bb..b430192 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metabase/cli", - "version": "0.1.11", + "version": "0.1.12", "description": "Metabase CLI", "license": "AGPL-3.0", "repository": {