From 48b4beb6c65e072cf44e579e7038f20e059ebdae Mon Sep 17 00:00:00 2001 From: James Date: Fri, 26 Jun 2026 16:09:57 +0000 Subject: [PATCH 1/2] feat(cli): persist + show friendly user identity; preserve unknown credential fields The `~/.heygen/credentials` file is SHARED with the Go `heygen` CLI. This is the hyperframes-side mirror of heygen-cli#197, which adds an optional `user` block to that file. Two CLIs writing one file must round-trip each other's data without loss. Load-bearing change: the credentials reader/writer now PRESERVES unknown fields on round-trip. Previously readStore/writeStore stripped any key this CLI didn't model, so writing the file back would silently drop the `user` block heygen-cli wrote (and any future key). Unrecognized top-level keys, and unknown keys inside `oauth` / `user`, are captured on a hidden symbol slot and re-emitted verbatim. Known fields stay strictly validated. Also mirrors heygen-cli#197's friendly-display feature: - New optional `user` block schema (email/first_name/last_name/username), all omitempty; legacy files without it parse fine. - After login (OAuth + api-key paths) probe /v3/users/me, persist the block, and show a friendly name (email > "first last" > username). Probe failure is non-fatal (login still succeeds); a stale block is cleared on probe failure so a wrong account can't surface. - `auth status` surfaces the persisted block (persisted_user in JSON, a cached Account row in human output) for file-sourced credentials; env-sourced credentials skip it (the on-disk block may belong to a different key). - Fixed the OAuth write path to carry the user block + unknown keys across a fresh login / refresh (it previously rebuilt a minimal record). Tests: preserve-unknown-fields round-trip (top-level, oauth, user), the exact cross-CLI `user`-block scenario, schema round-trip + omitempty, backwards-compat with legacy files, login persistence + graceful probe failure + stale-clear, and the `auth status` surface. Full CLI suite (1009 tests) green; oxlint + oxfmt + tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/auth/index.ts | 10 +- packages/cli/src/auth/oauth.test.ts | 27 +++ packages/cli/src/auth/oauth.ts | 7 +- packages/cli/src/auth/store.test.ts | 154 ++++++++++++- packages/cli/src/auth/store.ts | 215 ++++++++++++++++-- packages/cli/src/auth/user.test.ts | 148 ++++++++++++ packages/cli/src/auth/user.ts | 96 ++++++++ packages/cli/src/commands/auth/login.test.ts | 75 +++++- packages/cli/src/commands/auth/login.ts | 62 ++++- .../cli/src/commands/auth/status-user.test.ts | 142 ++++++++++++ packages/cli/src/commands/auth/status.ts | 61 ++++- 11 files changed, 960 insertions(+), 37 deletions(-) create mode 100644 packages/cli/src/auth/user.test.ts create mode 100644 packages/cli/src/auth/user.ts create mode 100644 packages/cli/src/commands/auth/status-user.test.ts diff --git a/packages/cli/src/auth/index.ts b/packages/cli/src/auth/index.ts index eb2c99fa4b..1786e991d9 100644 --- a/packages/cli/src/auth/index.ts +++ b/packages/cli/src/auth/index.ts @@ -6,7 +6,15 @@ export { isAuthError } from "./errors.js"; export { clearOAuth, deleteStore, isHeaderSafe, readStore, writeStore } from "./store.js"; -export type { Credentials } from "./store.js"; +export type { Credentials, StoredUserInfo } from "./store.js"; + +export { + clearUserInfo, + isUserInfoEmpty, + loadUserInfo, + saveUserInfo, + userDisplayName, +} from "./user.js"; export { configDir, credentialPath } from "./paths.js"; diff --git a/packages/cli/src/auth/oauth.test.ts b/packages/cli/src/auth/oauth.test.ts index 2b88441ff2..585f1d53b3 100644 --- a/packages/cli/src/auth/oauth.test.ts +++ b/packages/cli/src/auth/oauth.test.ts @@ -1,3 +1,4 @@ +import { promises as fs } from "node:fs"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { setupTempAuthEnv } from "./_test-utils.js"; import { isAuthError } from "./errors.js"; @@ -296,5 +297,31 @@ describe("auth/oauth", () => { expect(credentials.oauth?.access_token).toBe("new_at"); expect(credentials.oauth?.refresh_token).toBe("new_rt"); }); + + it("preserves the user block AND unknown/foreign keys across fresh login", async () => { + // The OAuth write path must not drop co-located data the other CLI + // (or a prior login) wrote. Seed a user block plus a future key, + // then log in fresh — both must survive the OAuth-block overwrite. + const path = (await import("./paths.js")).credentialPath(); + await fs.writeFile( + path, + JSON.stringify({ + oauth: { access_token: "old_at" }, + user: { email: "jane@example.com", username: "jdoe" }, + future_field: { keep: true }, + }), + { mode: 0o600 }, + ); + const fetchImpl = tokenFetch({ access_token: "new_at", expires_in: 3600 }); + await startAuthorizationCodeFlow({ fetchImpl }); + + const { credentials } = await readStore(); + expect(credentials.oauth?.access_token).toBe("new_at"); + expect(credentials.user).toEqual({ email: "jane@example.com", username: "jdoe" }); + + // The unknown key is on a hidden slot — assert via the raw file. + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.future_field).toEqual({ keep: true }); + }); }); }); diff --git a/packages/cli/src/auth/oauth.ts b/packages/cli/src/auth/oauth.ts index ddfc31ddaa..33e739915b 100644 --- a/packages/cli/src/auth/oauth.ts +++ b/packages/cli/src/auth/oauth.ts @@ -414,8 +414,11 @@ async function persistOAuth( const oauth: OAuthTokens = opts.preserveMissing ? { ...existing.oauth, ...tokens } : { ...tokens }; - // Keep co-located API key in both modes. - await writeStore({ ...(existing.api_key ? { api_key: existing.api_key } : {}), oauth }); + // Start from the existing record so co-located data survives: the + // api_key, the friendly-display `user` block, AND any unknown/foreign + // keys another CLI wrote (carried on a hidden symbol slot by spread). + // Only the `oauth` block is overwritten here. + await writeStore({ ...existing, oauth }); } async function readJsonOrThrow(res: Response): Promise { diff --git a/packages/cli/src/auth/store.test.ts b/packages/cli/src/auth/store.test.ts index 5e5567b719..9c83ee831e 100644 --- a/packages/cli/src/auth/store.test.ts +++ b/packages/cli/src/auth/store.test.ts @@ -173,12 +173,143 @@ describe("auth/store", () => { await expect(readStore(path)).rejects.toSatisfy((err) => isAuthError(err)); }); - it("drops unknown top-level keys", async () => { + it("exposes only the typed surface for known keys (unknown keys hidden from callers)", async () => { + // The typed `credentials` view shows only the modelled keys — + // unknown/foreign keys are captured in a hidden (symbol-keyed) + // passthrough slot, not the enumerable surface, so callers can't + // accidentally read them. Round-trip preservation is covered below. await fs.writeFile(path, JSON.stringify({ api_key: "hg_x", future_field: { stuff: 1 } }), { mode: 0o600, }); const result = await readStore(path); - expect(result.credentials).toEqual({ api_key: "hg_x" }); + expect(Object.keys(result.credentials)).toEqual(["api_key"]); + expect(result.credentials.api_key).toBe("hg_x"); + }); + + // --- Cross-CLI forward compatibility: unknown-field preservation. --- + // The credentials file is SHARED with the Go `heygen` CLI. If this CLI + // strips keys it doesn't model when it writes the file back, it + // silently destroys the other CLI's data (and vice versa). The writer + // MUST round-trip unknown fields untouched. + + it("preserves an unknown TOP-LEVEL key across a read → write round-trip", async () => { + // Simulate a file another CLI version wrote with a future key. + await fs.writeFile( + path, + JSON.stringify({ api_key: "hg_x", future_field: { nested: [1, 2], flag: true } }), + { mode: 0o600 }, + ); + // Read it, then write back a typed update (here: just the api_key it + // surfaced). The unknown key must survive. + const { credentials } = await readStore(path); + await writeStore(credentials, path); + + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.api_key).toBe("hg_x"); + expect(onDisk.future_field).toEqual({ nested: [1, 2], flag: true }); + }); + + it("preserves the heygen-cli `user` block when this CLI rewrites only the credential", async () => { + // The exact cross-CLI data-loss scenario: heygen-cli wrote a `user` + // block; hyperframes-cli updates the api_key and must not drop it. + await fs.writeFile( + path, + JSON.stringify({ + api_key: "hg_old", + user: { email: "jane@example.com", first_name: "Jane", last_name: "Doe", username: "jdoe" }, + }), + { mode: 0o600 }, + ); + const { credentials } = await readStore(path); + credentials.api_key = "hg_new"; + await writeStore(credentials, path); + + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.api_key).toBe("hg_new"); + expect(onDisk.user).toEqual({ + email: "jane@example.com", + first_name: "Jane", + last_name: "Doe", + username: "jdoe", + }); + }); + + it("preserves an unknown key INSIDE the oauth sub-object", async () => { + await fs.writeFile( + path, + JSON.stringify({ + oauth: { access_token: "at_1", id_token: "future_id_token_value" }, + }), + { mode: 0o600 }, + ); + const { credentials } = await readStore(path); + await writeStore(credentials, path); + + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.oauth.access_token).toBe("at_1"); + expect(onDisk.oauth.id_token).toBe("future_id_token_value"); + }); + + it("preserves an unknown key INSIDE the user sub-object", async () => { + await fs.writeFile( + path, + JSON.stringify({ + api_key: "hg_x", + user: { email: "u@example.com", avatar_url: "https://cdn/x.png" }, + }), + { mode: 0o600 }, + ); + const { credentials } = await readStore(path); + await writeStore(credentials, path); + + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.user.email).toBe("u@example.com"); + expect(onDisk.user.avatar_url).toBe("https://cdn/x.png"); + }); + + it("round-trips the user block (schema + omitempty: empty fields are not written)", async () => { + const creds: Credentials = { + api_key: "hg_x", + user: { email: "u@example.com", username: "u" }, + }; + await writeStore(creds, path); + const result = await readStore(path); + expect(result.credentials.user).toEqual({ email: "u@example.com", username: "u" }); + + // omitempty: only the populated fields appear on disk — no empty + // first_name / last_name strings littering the file. + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.user).toEqual({ email: "u@example.com", username: "u" }); + expect(Object.keys(onDisk.user)).toEqual(["email", "username"]); + }); + + it('omits an all-empty user block entirely (no `"user": {}` litter)', async () => { + await writeStore({ api_key: "hg_x", user: {} }, path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.user).toBeUndefined(); + expect(onDisk.api_key).toBe("hg_x"); + }); + + it("backwards-compat: a legacy file WITHOUT a user block parses with user undefined", async () => { + await fs.writeFile(path, JSON.stringify({ api_key: "hg_legacy" }), { mode: 0o600 }); + const result = await readStore(path); + expect(result.source).toBe("file_json"); + expect(result.credentials.api_key).toBe("hg_legacy"); + expect(result.credentials.user).toBeUndefined(); + }); + + it("ignores a malformed user sub-field rather than rejecting the whole file", async () => { + // The user block is additive metadata — a junk sub-field must never + // block resolving a perfectly good api_key. Non-string fields are + // dropped; the credential survives. + await fs.writeFile( + path, + JSON.stringify({ api_key: "hg_x", user: { email: "u@example.com", first_name: 12345 } }), + { mode: 0o600 }, + ); + const result = await readStore(path); + expect(result.credentials.api_key).toBe("hg_x"); + expect(result.credentials.user).toEqual({ email: "u@example.com" }); }); it("deleteStore is idempotent", async () => { @@ -202,6 +333,25 @@ describe("auth/store", () => { await expect(fs.access(path)).rejects.toThrow(); }); + it("clearOAuth keeps the user block (and unknown keys) when an api_key survives", async () => { + await fs.writeFile( + path, + JSON.stringify({ + api_key: "hg_keep", + oauth: { access_token: "drop_me" }, + user: { email: "u@example.com" }, + future_field: 1, + }), + { mode: 0o600 }, + ); + await clearOAuth(path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.oauth).toBeUndefined(); + expect(onDisk.api_key).toBe("hg_keep"); + expect(onDisk.user).toEqual({ email: "u@example.com" }); + expect(onDisk.future_field).toBe(1); + }); + it("clearOAuth is a no-op when file is absent", async () => { await clearOAuth(path); await expect(fs.access(path)).rejects.toThrow(); diff --git a/packages/cli/src/auth/store.ts b/packages/cli/src/auth/store.ts index 4c753a1227..ab8296259c 100644 --- a/packages/cli/src/auth/store.ts +++ b/packages/cli/src/auth/store.ts @@ -11,6 +11,12 @@ * "expires_at": "2026-06-25T12:00:00Z", * "scope": "openid profile", * "token_type": "Bearer" + * }, + * "user": { + * "email": "...", + * "first_name": "...", + * "last_name": "...", + * "username": "..." * } * } * @@ -19,6 +25,15 @@ * trimmed contents as an API key; the next write upgrades to JSON. * * Writes go to a temp file + rename, 0600 mode, parent dir 0700. + * + * Cross-CLI forward compatibility: this file is SHARED with the Go + * `heygen` CLI (and any future tool). Either CLI may write keys this + * version doesn't model yet. To avoid one CLI silently clobbering the + * other's data on round-trip, the reader stashes every unrecognized + * top-level key (and every unrecognized key inside the `oauth` / `user` + * sub-objects) into a hidden passthrough bag, and the writer re-emits + * them verbatim. Known fields are still strictly validated; the + * passthrough is purely additive and never feeds an HTTP header. */ import { promises as fs } from "node:fs"; @@ -29,6 +44,28 @@ import { ErrInvalidStore } from "./errors.js"; const FILE_MODE = 0o600; const DIR_MODE = 0o700; +/** + * Symbol-keyed slot holding the raw JSON of any keys this CLI version + * doesn't model, captured at read time and re-emitted verbatim at write + * time. A symbol (rather than a string key) keeps it off the typed + * surface so callers can't accidentally read/write it, and `Object.keys` + * / `JSON.stringify` skip it. See the module header for the rationale. + */ +const UNKNOWN = Symbol("hf.credentials.unknownFields"); + +/** Keys this CLI version models at the top level of the credentials file. */ +const KNOWN_ROOT_KEYS = new Set(["api_key", "oauth", "user"]); +/** Keys this CLI version models inside the `oauth` sub-object. */ +const KNOWN_OAUTH_KEYS = new Set([ + "access_token", + "refresh_token", + "expires_at", + "scope", + "token_type", +]); +/** Keys this CLI version models inside the `user` sub-object. */ +const KNOWN_USER_KEYS = new Set(["email", "first_name", "last_name", "username"]); + export interface OAuthTokens { access_token: string; refresh_token?: string; @@ -36,11 +73,33 @@ export interface OAuthTokens { expires_at?: string; scope?: string; token_type?: string; + /** Unknown/future keys captured for cross-CLI round-trip. */ + [UNKNOWN]?: Record; +} + +/** + * Friendly-display metadata captured at login time from `/v3/users/me`. + * NOT a credential — additive identity info persisted alongside the + * credential so `auth status` can show "Logged in as ..." without + * re-hitting the API. All fields optional; a file with no `user` block + * (a pre-this-change login) is fully backwards-compatible. Mirrors the + * `user` block heygen-cli writes — see `internal/auth/user_store.go`. + */ +export interface StoredUserInfo { + email?: string; + first_name?: string; + last_name?: string; + username?: string; + /** Unknown/future keys captured for cross-CLI round-trip. */ + [UNKNOWN]?: Record; } export interface Credentials { api_key?: string; oauth?: OAuthTokens; + user?: StoredUserInfo; + /** Unknown/future top-level keys captured for cross-CLI round-trip. */ + [UNKNOWN]?: Record; } export type StoreSource = "file_json" | "file_legacy" | "absent"; @@ -102,11 +161,18 @@ export async function deleteStore(path = credentialPath()): Promise { export async function clearOAuth(path = credentialPath()): Promise { const { credentials, source } = await readStore(path); if (source === "absent" || !credentials.oauth) return; - if (!credentials.api_key) { + // Drop oauth, keep everything else (api_key, the friendly-display + // user block, and any unknown/foreign keys) so a logout that only + // clears the OAuth session doesn't silently wipe co-located data. + const next: Credentials = { ...credentials }; + delete next.oauth; + if (!next.api_key) { + // No credential survives. The leftover user block / unknown keys are + // orphaned metadata with no credential to attach to — drop the file. await deleteStore(path); return; } - await writeStore({ api_key: credentials.api_key }, path); + await writeStore(next, path); } async function ensureDir(dir: string): Promise { @@ -139,30 +205,115 @@ function parseJsonStore(text: string): Credentials { if (obj["oauth"] !== undefined && obj["oauth"] !== null) { out.oauth = parseOAuth(obj["oauth"]); } + if (obj["user"] !== undefined && obj["user"] !== null) { + out.user = parseUser(obj["user"]); + } + // Capture any top-level keys this CLI version doesn't model so the + // next write round-trips them instead of dropping another CLI's data. + const unknownRoot = collectUnknown(obj, KNOWN_ROOT_KEYS); + if (unknownRoot) out[UNKNOWN] = unknownRoot; return out; } -function parseOAuth(raw: unknown): OAuthTokens { - if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { - throw ErrInvalidStore("oauth must be a JSON object"); +/** Optional-field picker variants used by the data-driven parsers. */ +type FieldPicker = "header_safe" | "non_empty"; + +const PICKERS: Record< + FieldPicker, + (obj: Record, key: string) => string | undefined +> = { + header_safe: pickHeaderSafeString, + non_empty: pickNonEmptyString, +}; + +/** + * Copy each `[field, picker]` from `obj` onto `out` when the picker + * yields a value. Data-driven so the optional-field handling stays a + * single loop instead of a long if-chain per parser. `out` is written + * through an index cast — the `spec` field names are the contract that + * keeps the assignments type-correct at the call site. + */ +function assignOptionalStrings( + out: object, + obj: Record, + spec: readonly [string, FieldPicker][], +): void { + const target = out as Record; + for (const [field, picker] of spec) { + const v = PICKERS[picker](obj, field); + if (v) target[field] = v; } - const obj = raw as Record; +} + +const OAUTH_OPTIONAL: readonly [string, FieldPicker][] = [ + ["refresh_token", "header_safe"], + ["expires_at", "non_empty"], + ["scope", "non_empty"], + ["token_type", "non_empty"], +]; + +function parseOAuth(raw: unknown): OAuthTokens { + const obj = asJsonObject(raw, "oauth"); const accessToken = pickHeaderSafeString(obj, "access_token"); if (!accessToken) { throw ErrInvalidStore("oauth.access_token must be a non-empty string with no control chars"); } const out: OAuthTokens = { access_token: accessToken }; - const refresh = pickHeaderSafeString(obj, "refresh_token"); - if (refresh) out.refresh_token = refresh; - const exp = pickNonEmptyString(obj, "expires_at"); - if (exp) out.expires_at = exp; - const scope = pickNonEmptyString(obj, "scope"); - if (scope) out.scope = scope; - const tokenType = pickNonEmptyString(obj, "token_type"); - if (tokenType) out.token_type = tokenType; + assignOptionalStrings(out, obj, OAUTH_OPTIONAL); + const unknownOAuth = collectUnknown(obj, KNOWN_OAUTH_KEYS); + if (unknownOAuth) out[UNKNOWN] = unknownOAuth; return out; } +const USER_OPTIONAL: readonly [string, FieldPicker][] = [ + ["email", "non_empty"], + ["first_name", "non_empty"], + ["last_name", "non_empty"], + ["username", "non_empty"], +]; + +/** + * Parse the friendly-display `user` block. Every field is optional and + * lenient — a wrong-typed or empty value is simply skipped rather than + * rejected, because this is additive METADATA, not a credential, and a + * malformed sub-field must never block resolving a perfectly good + * api_key / oauth token. (Contrast with `parseOAuth`, where a missing + * access_token is a hard error.) Unknown keys round-trip. + */ +function parseUser(raw: unknown): StoredUserInfo { + const obj = asJsonObject(raw, "user"); + const out: StoredUserInfo = {}; + assignOptionalStrings(out, obj, USER_OPTIONAL); + const unknownUser = collectUnknown(obj, KNOWN_USER_KEYS); + if (unknownUser) out[UNKNOWN] = unknownUser; + return out; +} + +/** Narrow `raw` to a plain JSON object or throw a labelled error. */ +function asJsonObject(raw: unknown, label: string): Record { + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) { + throw ErrInvalidStore(`${label} must be a JSON object`); + } + return raw as Record; +} + +/** + * Return a shallow copy of every entry in `obj` whose key is not in + * `known`, or `undefined` when there are none. Used to capture + * unrecognized JSON for verbatim re-emission on the next write. + */ +function collectUnknown( + obj: Record, + known: Set, +): Record | undefined { + let bag: Record | undefined; + for (const key of Object.keys(obj)) { + if (known.has(key)) continue; + (bag ??= {})[key] = obj[key]; + } + return bag; +} + function parseJsonObject(text: string, label: string): Record { let raw: unknown; try { @@ -223,19 +374,39 @@ function pickRequiredStringOrAbsent( } function serializeCredentials(c: Credentials): Record { - const out: Record = {}; + // Re-emit unrecognized top-level keys first so the known fields below + // are authoritative (collectUnknown already excludes known keys, so + // there's no real collision — this is belt-and-suspenders). + const out: Record = { ...(c[UNKNOWN] ?? {}) }; if (c.api_key) out["api_key"] = c.api_key; - if (c.oauth) { - const oauth: Record = { access_token: c.oauth.access_token }; - if (c.oauth.refresh_token) oauth["refresh_token"] = c.oauth.refresh_token; - if (c.oauth.expires_at) oauth["expires_at"] = c.oauth.expires_at; - if (c.oauth.scope) oauth["scope"] = c.oauth.scope; - if (c.oauth.token_type) oauth["token_type"] = c.oauth.token_type; - out["oauth"] = oauth; + if (c.oauth) out["oauth"] = serializeOAuth(c.oauth); + if (c.user) { + const user = serializeUser(c.user); + // Omit an all-empty user block entirely (no empty `"user": {}` litter). + if (Object.keys(user).length > 0) out["user"] = user; } return out; } +function serializeOAuth(o: OAuthTokens): Record { + const oauth: Record = { ...(o[UNKNOWN] ?? {}) }; + oauth["access_token"] = o.access_token; + if (o.refresh_token) oauth["refresh_token"] = o.refresh_token; + if (o.expires_at) oauth["expires_at"] = o.expires_at; + if (o.scope) oauth["scope"] = o.scope; + if (o.token_type) oauth["token_type"] = o.token_type; + return oauth; +} + +function serializeUser(u: StoredUserInfo): Record { + const user: Record = { ...(u[UNKNOWN] ?? {}) }; + if (u.email) user["email"] = u.email; + if (u.first_name) user["first_name"] = u.first_name; + if (u.last_name) user["last_name"] = u.last_name; + if (u.username) user["username"] = u.username; + return user; +} + /** * Legacy-plaintext heuristic. HeyGen API keys come in multiple formats * (`sk_V2_…`, historic `hg_…`, partner keys, etc.) and the CLI should diff --git a/packages/cli/src/auth/user.test.ts b/packages/cli/src/auth/user.test.ts new file mode 100644 index 0000000000..3333bcc00b --- /dev/null +++ b/packages/cli/src/auth/user.test.ts @@ -0,0 +1,148 @@ +import { promises as fs } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { readStore, writeStore } from "./store.js"; +import { + clearUserInfo, + isUserInfoEmpty, + loadUserInfo, + saveUserInfo, + userDisplayName, + type StoredUserInfo, +} from "./user.js"; + +async function makeTmpDir(): Promise { + return fs.mkdtemp(join(tmpdir(), "hf-auth-user-")); +} + +describe("auth/user — userDisplayName priority", () => { + const cases: { name: string; ui: StoredUserInfo; want: string | undefined }[] = [ + { + name: "email wins over everything", + ui: { email: "u@example.com", first_name: "Jane", last_name: "Doe", username: "jdoe" }, + want: "u@example.com", + }, + { + name: "no email → first last", + ui: { first_name: "Jane", last_name: "Doe", username: "jdoe" }, + want: "Jane Doe", + }, + { name: "only first name", ui: { first_name: "Jane", username: "jdoe" }, want: "Jane" }, + { name: "only last name", ui: { last_name: "Doe", username: "jdoe" }, want: "Doe" }, + { name: "only username", ui: { username: "jdoe" }, want: "jdoe" }, + { name: "all empty → undefined", ui: {}, want: undefined }, + ]; + for (const tc of cases) { + it(tc.name, () => { + expect(userDisplayName(tc.ui)).toBe(tc.want); + }); + } +}); + +describe("auth/user — isUserInfoEmpty", () => { + it("true for an all-empty block", () => { + expect(isUserInfoEmpty({})).toBe(true); + }); + it("false when any field is set", () => { + expect(isUserInfoEmpty({ email: "u@example.com" })).toBe(false); + expect(isUserInfoEmpty({ username: "u" })).toBe(false); + expect(isUserInfoEmpty({ first_name: "J" })).toBe(false); + expect(isUserInfoEmpty({ last_name: "D" })).toBe(false); + }); +}); + +describe("auth/user — save / load / clear", () => { + let dir: string; + let path: string; + + beforeEach(async () => { + dir = await makeTmpDir(); + path = join(dir, "credentials"); + }); + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + it("round-trips a user block through the credentials file", async () => { + await writeStore({ api_key: "hg_x" }, path); + const ui: StoredUserInfo = { + email: "u@example.com", + first_name: "Jane", + last_name: "Doe", + username: "jdoe", + }; + await saveUserInfo(ui, path); + expect(await loadUserInfo(path)).toEqual(ui); + }); + + it("saveUserInfo preserves a co-located api_key", async () => { + await writeStore({ api_key: "hg_keep" }, path); + await saveUserInfo({ email: "u@example.com" }, path); + const { credentials } = await readStore(path); + expect(credentials.api_key).toBe("hg_keep"); + expect(credentials.user).toEqual({ email: "u@example.com" }); + }); + + it("saveUserInfo upgrades a legacy plaintext file to JSON with the user block", async () => { + await fs.writeFile(path, "hg_legacy_key\n", { mode: 0o600 }); + await saveUserInfo({ email: "u@example.com" }, path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.api_key).toBe("hg_legacy_key"); + expect(onDisk.user).toEqual({ email: "u@example.com" }); + }); + + it("saveUserInfo with an empty block is a no-op (does not blank an existing block)", async () => { + await writeStore({ api_key: "hg_x", user: { email: "keep@example.com" } }, path); + const before = await fs.readFile(path, "utf8"); + await saveUserInfo({}, path); + const after = await fs.readFile(path, "utf8"); + expect(after).toBe(before); + expect(await loadUserInfo(path)).toEqual({ email: "keep@example.com" }); + }); + + it("loadUserInfo returns null for an absent file", async () => { + expect(await loadUserInfo(path)).toBeNull(); + }); + + it("loadUserInfo returns null for a legacy file without a user block (backwards-compat)", async () => { + await fs.writeFile(path, JSON.stringify({ api_key: "hg_legacy" }), { mode: 0o600 }); + expect(await loadUserInfo(path)).toBeNull(); + }); + + it("clearUserInfo removes only the user block, keeping the credential", async () => { + await writeStore({ api_key: "hg_keep", user: { email: "u@example.com" } }, path); + await clearUserInfo(path); + const { credentials } = await readStore(path); + expect(credentials.api_key).toBe("hg_keep"); + expect(credentials.user).toBeUndefined(); + }); + + it("clearUserInfo removes the whole file when no credential survives", async () => { + // A file holding ONLY a user block (no credential) — clearing leaves + // nothing, so the file should be removed entirely. + await fs.writeFile(path, JSON.stringify({ user: { email: "u@example.com" } }), { mode: 0o600 }); + await clearUserInfo(path); + await expect(fs.access(path)).rejects.toThrow(); + }); + + it("clearUserInfo is a no-op when there is no user block", async () => { + await writeStore({ api_key: "hg_only" }, path); + const before = await fs.readFile(path, "utf8"); + await clearUserInfo(path); + const after = await fs.readFile(path, "utf8"); + expect(after).toBe(before); + }); + + it("saveUserInfo does not clobber an unknown/foreign top-level key", async () => { + // The cross-CLI invariant exercised through the persistence helper. + await fs.writeFile(path, JSON.stringify({ api_key: "hg_x", future_field: 42 }), { + mode: 0o600, + }); + await saveUserInfo({ email: "u@example.com" }, path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.future_field).toBe(42); + expect(onDisk.user).toEqual({ email: "u@example.com" }); + }); +}); diff --git a/packages/cli/src/auth/user.ts b/packages/cli/src/auth/user.ts new file mode 100644 index 0000000000..ca8e315170 --- /dev/null +++ b/packages/cli/src/auth/user.ts @@ -0,0 +1,96 @@ +/** + * Persistence + display helpers for the friendly-display `user` block in + * the shared `~/.heygen/credentials` file. + * + * The `user` block is additive METADATA captured at login time from + * `GET /v3/users/me` — NOT a credential. It lets `auth status` (and the + * post-login "Logged in as ..." line) show a recognizable identity + * without re-hitting the API on every invocation, and keeps working when + * the API is unreachable. + * + * This file is SHARED with the Go `heygen` CLI, which writes the same + * `user` block (see `heygen-cli/internal/auth/user_store.go`). Persisting + * here goes through `readStore` / `writeStore`, which round-trip every + * unrecognized key — so saving our user block never clobbers a key the + * other CLI wrote. + */ + +import { credentialPath } from "./paths.js"; +import { readStore, writeStore, deleteStore, type StoredUserInfo } from "./store.js"; + +export type { StoredUserInfo } from "./store.js"; + +/** True when `u` carries no friendly fields worth surfacing. */ +export function isUserInfoEmpty(u: StoredUserInfo): boolean { + return !u.email && !u.first_name && !u.last_name && !u.username; +} + +/** + * Most friendly name available, in priority order: + * email > "first last" > first > last > username > undefined. + * The caller falls back to its own marker (e.g. "(unknown user)") on + * `undefined`. Mirrors `UserInfo.DisplayName()` in heygen-cli. + */ +export function userDisplayName(u: StoredUserInfo): string | undefined { + if (u.email) return u.email; + const name = combineName(u.first_name, u.last_name); + if (name) return name; + return u.username || undefined; +} + +function combineName(first?: string, last?: string): string { + if (first && last) return `${first} ${last}`; + return first || last || ""; +} + +/** + * Persist the friendly-display block, preserving any co-located api_key / + * oauth blocks and unknown/foreign keys. An all-empty `StoredUserInfo` + * is a no-op (the caller should typically gate on `isUserInfoEmpty` + * before calling, but we guard here too so a failed probe can't blank an + * existing block by accident). + * + * A broken pre-existing file surfaces as a thrown `ErrInvalidStore` from + * `readStore` — callers treat persistence as best-effort and warn rather + * than fail the login. + */ +export async function saveUserInfo(info: StoredUserInfo, path = credentialPath()): Promise { + if (isUserInfoEmpty(info)) return; + // readStore preserves co-located api_key / oauth blocks and any + // unknown/foreign keys (captured on a hidden slot), so writing back + // only attaches the user block. A legacy single-line plaintext file + // parses into { api_key }, so this upgrades it to JSON in passing. + const { credentials } = await readStore(path); + credentials.user = { ...info }; + await writeStore(credentials, path); +} + +/** + * Read the friendly-display block. Returns `null` when the file is absent + * or carries no `user` block (a pre-this-change login). Genuine parse + * errors propagate so the caller can warn rather than silently pretend + * the file is clean. + */ +export async function loadUserInfo(path = credentialPath()): Promise { + const { credentials, source } = await readStore(path); + if (source === "absent") return null; + if (!credentials.user || isUserInfoEmpty(credentials.user)) return null; + return credentials.user; +} + +/** + * Remove the friendly-display block, leaving any co-located credential + * (and unknown/foreign keys) intact. When no credential survives, the + * orphaned-metadata file is removed entirely. A no-op when there is + * nothing to clear. + */ +export async function clearUserInfo(path = credentialPath()): Promise { + const { credentials, source } = await readStore(path); + if (source === "absent" || !credentials.user) return; + delete credentials.user; + if (!credentials.api_key && !credentials.oauth) { + await deleteStore(path); + return; + } + await writeStore(credentials, path); +} diff --git a/packages/cli/src/commands/auth/login.test.ts b/packages/cli/src/commands/auth/login.test.ts index deaf2ac6c6..3f332b98b2 100644 --- a/packages/cli/src/commands/auth/login.test.ts +++ b/packages/cli/src/commands/auth/login.test.ts @@ -5,19 +5,27 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { readStore, writeStore } from "../../auth/store.js"; // Mock only AuthClient — keep the real store/resolver so the test -// exercises the actual on-disk rollback behavior. `verifyResult` -// controls what `getCurrentUser` does per test. -const verifyState = vi.hoisted(() => ({ reject: false })); +// exercises the actual on-disk rollback / persistence behavior. +// `verifyState` controls what `getCurrentUser` returns per test: +// - reject: throw ErrUnauthenticated (invalid key path) +// - user: the /v3/users/me identity returned on success +const verifyState = vi.hoisted( + () => + ({ reject: false, user: { email: "alice@example.com" } }) as { + reject: boolean; + user: Record; + }, +); vi.mock("../../auth/index.js", async (orig) => { const actual = await orig(); class MockAuthClient { - async getCurrentUser(): Promise<{ email: string }> { + async getCurrentUser(): Promise> { if (verifyState.reject) { const { ErrUnauthenticated: rej } = await import("../../auth/errors.js"); throw rej("invalid key"); } - return { email: "alice@example.com" }; + return verifyState.user; } } return { ...actual, AuthClient: MockAuthClient }; @@ -37,6 +45,7 @@ describe("auth login --api-key rollback", () => { } process.env["HEYGEN_CONFIG_DIR"] = dir; verifyState.reject = false; + verifyState.user = { email: "alice@example.com" }; // process.exit throws so we can assert the post-rollback state. vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { throw new Error(`process.exit:${code ?? 0}`); @@ -88,4 +97,60 @@ describe("auth login --api-key rollback", () => { const { credentials } = await readStore(); expect(credentials.api_key).toBe("hg_goodkey456"); }); + + it("persists the friendly user block from /v3/users/me on a successful login", async () => { + verifyState.user = { + email: "jane@example.com", + first_name: "Jane", + last_name: "Doe", + username: "jdoe", + }; + await runLogin("hg_goodkey456"); + const { credentials } = await readStore(); + expect(credentials.api_key).toBe("hg_goodkey456"); + expect(credentials.user).toEqual({ + email: "jane@example.com", + first_name: "Jane", + last_name: "Doe", + username: "jdoe", + }); + }); + + it("clears a stale user block when the new key's identity probe returns nothing", async () => { + // Prior login left a user block on disk. The new key is valid but + // /v3/users/me returns no identity fields — the stale block must be + // cleared so `auth status` can't surface the previous account. + await writeStore({ api_key: "hg_old", user: { email: "old@example.com" } }); + verifyState.user = {}; // verified, but no identity returned + await runLogin("hg_newgoodkey"); + + const { credentials } = await readStore(); + expect(credentials.api_key).toBe("hg_newgoodkey"); + expect(credentials.user).toBeUndefined(); + }); + + it("rollback on a rejected key restores the previous user block too", async () => { + await writeStore({ api_key: "hg_prev", user: { email: "prev@example.com" } }); + verifyState.reject = true; + await expect(runLogin("hg_badnewkey")).rejects.toThrow(/process\.exit:1/); + + const { credentials } = await readStore(); + expect(credentials.api_key).toBe("hg_prev"); + expect(credentials.user).toEqual({ email: "prev@example.com" }); + }); + + it("preserves an unknown/foreign top-level key across a successful re-login", async () => { + // Cross-CLI invariant end-to-end: a key heygen-cli (or a future + // version) wrote must survive a hyperframes-cli login round-trip. + await fs.writeFile(join(dir, "credentials"), JSON.stringify({ future_field: { x: 1 } }), { + mode: 0o600, + }); + verifyState.user = { email: "jane@example.com" }; + await runLogin("hg_goodkey456"); + + const onDisk = JSON.parse(await fs.readFile(join(dir, "credentials"), "utf8")); + expect(onDisk.api_key).toBe("hg_goodkey456"); + expect(onDisk.user).toEqual({ email: "jane@example.com" }); + expect(onDisk.future_field).toEqual({ x: 1 }); + }); }); diff --git a/packages/cli/src/commands/auth/login.ts b/packages/cli/src/commands/auth/login.ts index c9d95c63fd..fd88b6a0d5 100644 --- a/packages/cli/src/commands/auth/login.ts +++ b/packages/cli/src/commands/auth/login.ts @@ -26,15 +26,21 @@ import { stdin as input } from "node:process"; import { AuthClient, assertOAuthConfiguredOrExit, + clearUserInfo, deleteStore, isAuthError, isHeaderSafe, + isUserInfoEmpty, readStore, refreshTokens, + saveUserInfo, startAuthorizationCodeFlow, tryResolveCredential, + userDisplayName, writeStore, type Credentials, + type StoredUserInfo, + type UserInfo, } from "../../auth/index.js"; import { c } from "../../ui/colors.js"; @@ -97,18 +103,63 @@ async function reportIdentity(): Promise { }); try { const user = await client.getCurrentUser(credential); - const identity = user.email ?? user.username ?? "(unknown user)"; + // Persist the friendly-display block alongside the OAuth tokens so + // `auth status` can show "Logged in as ..." without re-hitting + // /v3/users/me. Best-effort — a persist failure never fails the login. + await persistUserInfo(user); + const identity = userDisplayName(toStoredUserInfo(user)) ?? "(unknown user)"; console.log(c.success(`✓ Signed in as ${identity}.`)); } catch (err) { // Don't roll back — the OAuth tokens are valid on disk; this is a - // transient verify-side issue. Surface as a warning so the user - // can re-check with `auth status` rather than re-running login. + // transient verify-side issue. The identity probe failed, so any + // stale user block from a prior login (possibly a DIFFERENT account) + // is cleared so `auth status` can't surface the wrong identity. + await clearUserInfoBestEffort(); console.error( c.warn(`Signed in. Identity check failed (transient): ${(err as Error).message}`), ); } } +/** Project the API `/v3/users/me` view onto the on-disk identity block. */ +function toStoredUserInfo(user: UserInfo): StoredUserInfo { + const out: StoredUserInfo = {}; + if (user.email) out.email = user.email; + if (user.first_name) out.first_name = user.first_name; + if (user.last_name) out.last_name = user.last_name; + if (user.username) out.username = user.username; + return out; +} + +/** + * Persist the friendly-display block (best-effort). A non-empty block is + * saved; an empty one (the API returned no identity fields) clears any + * stale block so a wrong account can't surface in `auth status`. A + * persist/clear failure is warned, never fatal — the credential is valid + * on disk and that's what matters. + */ +async function persistUserInfo(user: UserInfo): Promise { + const stored = toStoredUserInfo(user); + try { + if (isUserInfoEmpty(stored)) { + await clearUserInfo(); + } else { + await saveUserInfo(stored); + } + } catch (err) { + console.error(c.dim(`(warning: could not persist user info: ${(err as Error).message})`)); + } +} + +/** Drop any stale user block; best-effort, never fatal. */ +async function clearUserInfoBestEffort(): Promise { + try { + await clearUserInfo(); + } catch (err) { + console.error(c.dim(`(warning: could not clear stale user info: ${(err as Error).message})`)); + } +} + // fallow-ignore-next-line complexity async function runApiKeyLogin(inlineKey: string): Promise { const key = await collectApiKey(inlineKey); @@ -170,7 +221,10 @@ async function verifyAndReport(key: string): Promise { const client = new AuthClient(); try { const user = await client.getCurrentUser({ type: "api_key", key, source: "file_json" }); - const identity = user.email ?? user.username ?? "(unknown user)"; + // Persist the friendly-display block next to the now-verified api_key + // so `auth status` can show a recognizable identity. Best-effort. + await persistUserInfo(user); + const identity = userDisplayName(toStoredUserInfo(user)) ?? "(unknown user)"; console.log(c.success(`✓ API key saved. Authenticated as ${identity}.`)); return true; } catch (err) { diff --git a/packages/cli/src/commands/auth/status-user.test.ts b/packages/cli/src/commands/auth/status-user.test.ts new file mode 100644 index 0000000000..d7c7462f67 --- /dev/null +++ b/packages/cli/src/commands/auth/status-user.test.ts @@ -0,0 +1,142 @@ +import { promises as fs } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { writeStore } from "../../auth/store.js"; + +// Mock only AuthClient so the live /v3/users/me probe is controllable; +// keep the real store/resolver/user helpers so the test exercises the +// actual on-disk persisted-user-block surfacing. +// - apiReject: throw ErrApi on getCurrentUser (simulates an API blip) +// - user: the live identity returned on success +const probeState = vi.hoisted( + () => + ({ apiReject: false, user: { email: "live@example.com" } }) as { + apiReject: boolean; + user: Record; + }, +); + +vi.mock("../../auth/index.js", async (orig) => { + const actual = await orig(); + class MockAuthClient { + async getCurrentUser(): Promise> { + if (probeState.apiReject) { + const { ErrApi } = await import("../../auth/errors.js"); + throw ErrApi(503, "service unavailable"); + } + return probeState.user; + } + } + return { ...actual, AuthClient: MockAuthClient }; +}); + +const ENV_KEYS = ["HEYGEN_API_KEY", "HYPERFRAMES_API_KEY", "HEYGEN_CONFIG_DIR"] as const; + +describe("auth status — persisted user block surface", () => { + let dir: string; + const saved: Partial> = {}; + let stdout: string[]; + + beforeEach(async () => { + dir = await fs.mkdtemp(join(tmpdir(), "hf-status-")); + for (const k of ENV_KEYS) { + saved[k] = process.env[k]; + delete process.env[k]; + } + process.env["HEYGEN_CONFIG_DIR"] = dir; + probeState.apiReject = false; + probeState.user = { email: "live@example.com" }; + stdout = []; + vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { + throw new Error(`process.exit:${code ?? 0}`); + }) as never); + vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => { + stdout.push(args.join(" ")); + }); + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + for (const k of ENV_KEYS) { + const v = saved[k]; + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + await fs.rm(dir, { recursive: true, force: true }); + }); + + async function runStatus(asJson: boolean): Promise { + const cmd = (await import("./status.js")).default; + try { + await (cmd.run as (ctx: { args: Record }) => Promise)({ + args: { json: asJson }, + }); + return 0; + } catch (err) { + const m = /process\.exit:(\d+)/.exec((err as Error).message); + if (m) return Number(m[1]); + throw err; + } + } + + function lastJson(): Record { + return JSON.parse(stdout[stdout.length - 1] ?? "{}"); + } + + it("surfaces the persisted user block (with resolved display_name) for a file credential", async () => { + await writeStore({ + api_key: "hg_x", + user: { email: "jane@example.com", first_name: "Jane", last_name: "Doe", username: "jdoe" }, + }); + + const code = await runStatus(true); + expect(code).toBe(0); + const payload = lastJson(); + expect(payload["source"]).toBe("file_json"); + expect(payload["persisted_user"]).toEqual({ + email: "jane@example.com", + first_name: "Jane", + last_name: "Doe", + username: "jdoe", + display_name: "jane@example.com", + }); + }); + + it("backwards-compat: a file credential with no user block reports persisted_user: null", async () => { + await writeStore({ api_key: "hg_legacy" }); + + const code = await runStatus(true); + expect(code).toBe(0); + const payload = lastJson(); + expect(payload["persisted_user"]).toBeNull(); + // The live `user` field (from the API probe) is unchanged / additive. + expect(payload["user"]).toEqual({ email: "live@example.com" }); + }); + + it("skips the persisted block for an env-sourced credential (could be a different key)", async () => { + // Seed a file-side user block, then resolve via the env key — the + // active credential is env, so the file block must NOT be surfaced. + await writeStore({ api_key: "hg_file", user: { email: "file-user@example.com" } }); + process.env["HEYGEN_API_KEY"] = "hg_env_key"; + + const code = await runStatus(true); + expect(code).toBe(0); + const payload = lastJson(); + expect(payload["source"]).toBe("env"); + expect(payload["persisted_user"]).toBeNull(); + }); + + it("falls back to the cached identity in human output when the live probe fails", async () => { + await writeStore({ api_key: "hg_x", user: { email: "cached@example.com" } }); + probeState.apiReject = true; + + const code = await runStatus(false); + expect(code).toBe(1); // API failure → non-zero exit + const text = stdout.join("\n"); + expect(text).toContain("API check failed"); + expect(text).toContain("cached@example.com"); + expect(text).toContain("cached"); + }); +}); diff --git a/packages/cli/src/commands/auth/status.ts b/packages/cli/src/commands/auth/status.ts index d083b1079f..f801243f05 100644 --- a/packages/cli/src/commands/auth/status.ts +++ b/packages/cli/src/commands/auth/status.ts @@ -18,9 +18,12 @@ import { defineCommand } from "citty"; import { AuthClient, isAuthError, + loadUserInfo, refreshTokens, tryResolveCredential, + userDisplayName, type ResolvedCredential, + type StoredUserInfo, type UserInfo, } from "../../auth/index.js"; import { getSystemMeta } from "../../telemetry/system.js"; @@ -36,9 +39,21 @@ import { interface VerifiedStatus { credential: ResolvedCredential; user: UserInfo | null; + /** + * The friendly-display block persisted at login time, when the active + * credential is file-sourced and a block is on disk. `null` for + * env-sourced credentials (the on-disk block could belong to a + * different key) and for pre-this-change credentials files. + */ + persistedUser: StoredUserInfo | null; apiError: string | null; } +/** True for credentials resolved from the shared file (not env). */ +function isFileSource(source: ResolvedCredential["source"]): boolean { + return source === "file_json" || source === "file_legacy"; +} + export default defineCommand({ meta: { name: "status", description: "Show the active HeyGen credential" }, args: { @@ -130,25 +145,47 @@ async function verify(credential: ResolvedCredential): Promise { // invalidate the old RT on every refresh). onUnauthenticatedRefresh: async (rt) => await refreshTokens(rt), }); + const persistedUser = await loadPersistedUser(credential); try { const user = await client.getCurrentUser(credential); - return { credential, user, apiError: null }; + return { credential, user, persistedUser, apiError: null }; } catch (err) { if (!isAuthError(err)) throw err; return { credential, user: null, + persistedUser, apiError: err instanceof Error ? err.message : String(err), }; } } +/** + * Load the persisted friendly-display block, but only for file-sourced + * credentials. An env credential (`HEYGEN_API_KEY` / `HYPERFRAMES_API_KEY`) + * could belong to a different key than the on-disk block, so surfacing + * that block would mislabel the active account. A read error is swallowed + * — the block is purely cosmetic and must never break `auth status`. + */ +async function loadPersistedUser(credential: ResolvedCredential): Promise { + if (!isFileSource(credential.source)) return null; + try { + return await loadUserInfo(); + } catch { + return null; + } +} + function printJsonStatus(s: VerifiedStatus): void { const payload: Record = { configured: true, source: s.credential.source, type: s.credential.type, user: s.user, + // The friendly-display block persisted at login (file-sourced creds + // only). Strictly additive — the live `user` field above is + // unchanged. Lets callers read identity offline / on an API blip. + persisted_user: persistedUserJson(s.persistedUser), api_error: s.apiError, }; if (s.credential.type === "oauth") { @@ -159,6 +196,24 @@ function printJsonStatus(s: VerifiedStatus): void { console.log(JSON.stringify(payload, null, 2)); } +/** + * Shape the persisted block for JSON: the four optional fields plus the + * resolved `display_name` (email > "first last" > username). Returns + * `null` when nothing is persisted so the field is an explicit null + * rather than an empty object. + */ +function persistedUserJson(u: StoredUserInfo | null): Record | null { + if (!u) return null; + const display = userDisplayName(u); + return { + email: u.email ?? null, + first_name: u.first_name ?? null, + last_name: u.last_name ?? null, + username: u.username ?? null, + display_name: display ?? null, + }; +} + function printHumanStatus(s: VerifiedStatus): void { const rows = collectStatusRows(s); for (const [label, value] of rows) console.log(`${c.bold(label)} ${value}`); @@ -173,6 +228,10 @@ function collectStatusRows(s: VerifiedStatus): [string, string][] { if (s.credential.type === "oauth") rows.push(...oauthRows(s.credential)); if (s.apiError) { rows.push([c.error("API check failed:"), s.apiError]); + // Fall back to the persisted identity so the user still sees who + // they're logged in as when the live probe is unreachable. + const cached = s.persistedUser && userDisplayName(s.persistedUser); + if (cached) rows.push(["Account:", `${cached} ${c.dim("(cached)")}`]); return rows; } if (s.user) rows.push(...identityRows(s.user)); From e96aca260b57e75921b7b62508af406ab4bc5a9b Mon Sep 17 00:00:00 2001 From: James Date: Fri, 26 Jun 2026 16:39:00 +0000 Subject: [PATCH 2/2] fix(cli): preserve unknown credential data in cleanup/rollback paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Magi's REQUEST_CHANGES on #1741. The credentials reader/writer already round-trips unknown/foreign keys (the cross-CLI forward-compat contract), but three destructive paths still deleted the whole file when no known api_key/oauth survived — even when the hidden Symbol-keyed unknown-field bag held a future credential another CLI owns. That clobbers exactly the data this PR preserves. - Add `hasPreservedUnknownData(record)` to store.ts (checks the top-level unknown bag + the oauth/user sub-object bags) and export it via the barrel. - `clearOAuth`, `clearUserInfo`, and the failed `auth login --api-key` rollback now write the credential-less remnant (carrying the unknown bag) instead of deleting the file when unknown/foreign data survives. They still delete when nothing worth preserving remains. - Regression tests: rollback path + both cleanup paths (clearOAuth, clearUserInfo) preserve a foreign top-level key; `hasPreservedUnknownData` unit tests at all three levels. Also addresses the review's minor items: - Add a refresh-path round-trip test (`refreshTokens`) proving an unknown key inside the oauth sub-object survives a no-rotation refresh — the most-frequent write path, previously only implicitly covered. - Clarify the `userDisplayName` / `combineName` docstrings: precedence is `email > "first last" > first-only > last-only > username`. - Replace the stale `expires_at` example date in store.ts with ``. Full CLI suite green (1020 tests); tsc, oxlint, oxfmt --check all clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/auth/index.ts | 9 +- packages/cli/src/auth/oauth.test.ts | 32 ++++++ packages/cli/src/auth/store.test.ts | 101 ++++++++++++++++++- packages/cli/src/auth/store.ts | 46 ++++++++- packages/cli/src/auth/user.test.ts | 19 ++++ packages/cli/src/auth/user.ts | 35 +++++-- packages/cli/src/commands/auth/login.test.ts | 19 ++++ packages/cli/src/commands/auth/login.ts | 14 ++- 8 files changed, 257 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/auth/index.ts b/packages/cli/src/auth/index.ts index 1786e991d9..710297b9c0 100644 --- a/packages/cli/src/auth/index.ts +++ b/packages/cli/src/auth/index.ts @@ -5,7 +5,14 @@ export { isAuthError } from "./errors.js"; -export { clearOAuth, deleteStore, isHeaderSafe, readStore, writeStore } from "./store.js"; +export { + clearOAuth, + deleteStore, + hasPreservedUnknownData, + isHeaderSafe, + readStore, + writeStore, +} from "./store.js"; export type { Credentials, StoredUserInfo } from "./store.js"; export { diff --git a/packages/cli/src/auth/oauth.test.ts b/packages/cli/src/auth/oauth.test.ts index 585f1d53b3..c280daf451 100644 --- a/packages/cli/src/auth/oauth.test.ts +++ b/packages/cli/src/auth/oauth.test.ts @@ -183,6 +183,38 @@ describe("auth/oauth", () => { expect(credentials.oauth?.access_token).toBe("new_at"); }); + it("preserves an unknown key INSIDE the oauth sub-object across a refresh", async () => { + // The refresh path is `persistOAuth(preserveMissing: true)`, i.e. + // `{ ...existing.oauth, ...tokens }` — object spread carries the + // hidden Symbol-keyed unknown bag from `existing.oauth`, so a key + // another CLI wrote inside `oauth` (e.g. an `id_token`) must survive + // the no-rotation refresh. Refresh is the most-frequent write path, + // so a silent regression here would be the worst case. + const path = (await import("./paths.js")).credentialPath(); + await fs.writeFile( + path, + JSON.stringify({ + oauth: { + access_token: "old_at", + refresh_token: "keep_me_rt", + id_token: "future_id_token_value", + }, + }), + { mode: 0o600 }, + ); + const fetchImpl = (async () => + new Response(JSON.stringify({ access_token: "new_at", expires_in: 3600 }), { + status: 200, + headers: { "content-type": "application/json" }, + })) as unknown as typeof fetch; + await refreshTokens("keep_me_rt", { fetchImpl }); + + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.oauth.access_token).toBe("new_at"); + // The unknown oauth sub-key rode through on the hidden slot. + expect(onDisk.oauth.id_token).toBe("future_id_token_value"); + }); + it("throws REFRESH_FAILED on 400/401", async () => { const fetchImpl = (async () => new Response("invalid_grant", { status: 400 })) as unknown as typeof fetch; diff --git a/packages/cli/src/auth/store.test.ts b/packages/cli/src/auth/store.test.ts index 9c83ee831e..aa94a05b0c 100644 --- a/packages/cli/src/auth/store.test.ts +++ b/packages/cli/src/auth/store.test.ts @@ -3,7 +3,14 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { isAuthError } from "./errors.js"; -import { clearOAuth, deleteStore, readStore, writeStore, type Credentials } from "./store.js"; +import { + clearOAuth, + deleteStore, + hasPreservedUnknownData, + readStore, + writeStore, + type Credentials, +} from "./store.js"; async function makeTmpDir(): Promise { return fs.mkdtemp(join(tmpdir(), "hf-auth-store-")); @@ -356,4 +363,96 @@ describe("auth/store", () => { await clearOAuth(path); await expect(fs.access(path)).rejects.toThrow(); }); + + // --- Destructive paths must not clobber preserved unknown data. --- + // When clearing the only known credential would otherwise delete the + // file, a surviving unknown/foreign top-level key (a future credential + // another CLI owns) must keep the file alive — deleting would clobber + // exactly the cross-CLI data this machinery exists to preserve. + + it("clearOAuth keeps the file (writing the unknown bag) when no api_key but a foreign top-level key survives", async () => { + await fs.writeFile( + path, + JSON.stringify({ + oauth: { access_token: "drop_me" }, + future_credential: { token: "owned_by_other_cli" }, + }), + { mode: 0o600 }, + ); + await clearOAuth(path); + // File must still exist and carry the foreign key. + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.oauth).toBeUndefined(); + expect(onDisk.future_credential).toEqual({ token: "owned_by_other_cli" }); + }); + + it("clearOAuth keeps the file when no api_key but a foreign key survives inside the user block", async () => { + // The user block has no known friendly fields, only a foreign sub-key + // — the block itself survives the oauth clear, so its unknown data + // must too. + await fs.writeFile( + path, + JSON.stringify({ + oauth: { access_token: "drop_me" }, + user: { external_org_id: "org_123" }, + }), + { mode: 0o600 }, + ); + await clearOAuth(path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.oauth).toBeUndefined(); + expect(onDisk.user).toEqual({ external_org_id: "org_123" }); + }); + + it("clearOAuth still deletes the file when only a known (empty-after-clear) surface remains", async () => { + // No api_key, no foreign data — just the oauth block being cleared. + // Nothing worth preserving, so the file goes. + await writeStore({ oauth: { access_token: "only" } }, path); + await clearOAuth(path); + await expect(fs.access(path)).rejects.toThrow(); + }); + + describe("hasPreservedUnknownData", () => { + it("false for an empty record", () => { + expect(hasPreservedUnknownData({})).toBe(false); + }); + + it("false for a record with only known fields", () => { + expect( + hasPreservedUnknownData({ + api_key: "hg_x", + oauth: { access_token: "at" }, + user: { email: "u@example.com" }, + }), + ).toBe(false); + }); + + it("true when a top-level unknown key was captured at read time", async () => { + await fs.writeFile(path, JSON.stringify({ api_key: "hg_x", future_field: 1 }), { + mode: 0o600, + }); + const { credentials } = await readStore(path); + expect(hasPreservedUnknownData(credentials)).toBe(true); + }); + + it("true when an unknown key was captured inside the oauth sub-object", async () => { + await fs.writeFile( + path, + JSON.stringify({ oauth: { access_token: "at", id_token: "future" } }), + { mode: 0o600 }, + ); + const { credentials } = await readStore(path); + expect(hasPreservedUnknownData(credentials)).toBe(true); + }); + + it("true when an unknown key was captured inside the user sub-object", async () => { + await fs.writeFile( + path, + JSON.stringify({ api_key: "hg_x", user: { email: "u@example.com", avatar_url: "x" } }), + { mode: 0o600 }, + ); + const { credentials } = await readStore(path); + expect(hasPreservedUnknownData(credentials)).toBe(true); + }); + }); }); diff --git a/packages/cli/src/auth/store.ts b/packages/cli/src/auth/store.ts index ab8296259c..6315591437 100644 --- a/packages/cli/src/auth/store.ts +++ b/packages/cli/src/auth/store.ts @@ -8,7 +8,7 @@ * "oauth": { * "access_token": "...", * "refresh_token": "...", - * "expires_at": "2026-06-25T12:00:00Z", + * "expires_at": "", * "scope": "openid profile", * "token_type": "Bearer" * }, @@ -34,6 +34,14 @@ * sub-objects) into a hidden passthrough bag, and the writer re-emits * them verbatim. Known fields are still strictly validated; the * passthrough is purely additive and never feeds an HTTP header. + * + * The same contract binds the destructive paths (`clearOAuth`, + * `clearUserInfo`, and the failed `auth login --api-key` rollback): when + * removing a credential would leave the file with no known credential but + * a surviving unknown/foreign top-level (or user-block) key, they write + * the credential-less remnant rather than deleting the file — see + * `hasPreservedUnknownData`. Deleting there would clobber exactly the + * cross-CLI data this machinery exists to preserve. */ import { promises as fs } from "node:fs"; @@ -157,6 +165,32 @@ export async function deleteStore(path = credentialPath()): Promise { } } +/** + * True when `credentials` carries any unrecognized/foreign data captured + * on a hidden passthrough slot — either a top-level unknown key, or an + * unknown key inside the `oauth` / `user` sub-objects. + * + * The cleanup / rollback paths (`clearOAuth`, `clearUserInfo`, the failed + * `auth login --api-key` rollback) use this to decide between deleting the + * file and writing a credential-less remnant. When no known credential + * survives BUT foreign data does, that data may be a future credential or + * metadata key another CLI owns — deleting the file would clobber exactly + * what the cross-CLI forward-compatibility contract promises to preserve. + * So those paths write the remaining record (carrying the unknown bag) + * instead of deleting. Only when nothing worth preserving remains do they + * delete. + */ +export function hasPreservedUnknownData(credentials: Credentials): boolean { + if (hasUnknownBag(credentials[UNKNOWN])) return true; + if (hasUnknownBag(credentials.oauth?.[UNKNOWN])) return true; + if (hasUnknownBag(credentials.user?.[UNKNOWN])) return true; + return false; +} + +function hasUnknownBag(bag: Record | undefined): boolean { + return bag !== undefined && Object.keys(bag).length > 0; +} + /** Remove only the `oauth` block. Used by `auth logout --keep-api-key`. */ export async function clearOAuth(path = credentialPath()): Promise { const { credentials, source } = await readStore(path); @@ -166,9 +200,13 @@ export async function clearOAuth(path = credentialPath()): Promise { // clears the OAuth session doesn't silently wipe co-located data. const next: Credentials = { ...credentials }; delete next.oauth; - if (!next.api_key) { - // No credential survives. The leftover user block / unknown keys are - // orphaned metadata with no credential to attach to — drop the file. + if (!next.api_key && !hasPreservedUnknownData(next)) { + // Nothing worth preserving survives. The leftover user block had no + // friendly fields and there's no foreign/unknown data to round-trip, + // so this is orphaned metadata with no credential to attach to — drop + // the file. (A surviving top-level / user-block unknown key, by + // contrast, may be a future credential another CLI owns, so we keep + // the file in that case.) await deleteStore(path); return; } diff --git a/packages/cli/src/auth/user.test.ts b/packages/cli/src/auth/user.test.ts index 3333bcc00b..ce6a3fb4ec 100644 --- a/packages/cli/src/auth/user.test.ts +++ b/packages/cli/src/auth/user.test.ts @@ -135,6 +135,25 @@ describe("auth/user — save / load / clear", () => { expect(after).toBe(before); }); + it("clearUserInfo keeps the file (preserving a foreign top-level key) when no credential survives", async () => { + // The file holds a user block plus a future/foreign top-level key but + // NO known credential. Clearing the user block must NOT delete the + // file — the foreign key may be a credential another CLI owns, and + // dropping it would clobber the cross-CLI data the store preserves. + await fs.writeFile( + path, + JSON.stringify({ + user: { email: "u@example.com" }, + future_credential: { token: "owned_by_other_cli" }, + }), + { mode: 0o600 }, + ); + await clearUserInfo(path); + const onDisk = JSON.parse(await fs.readFile(path, "utf8")); + expect(onDisk.user).toBeUndefined(); + expect(onDisk.future_credential).toEqual({ token: "owned_by_other_cli" }); + }); + it("saveUserInfo does not clobber an unknown/foreign top-level key", async () => { // The cross-CLI invariant exercised through the persistence helper. await fs.writeFile(path, JSON.stringify({ api_key: "hg_x", future_field: 42 }), { diff --git a/packages/cli/src/auth/user.ts b/packages/cli/src/auth/user.ts index ca8e315170..bb4146747a 100644 --- a/packages/cli/src/auth/user.ts +++ b/packages/cli/src/auth/user.ts @@ -16,7 +16,13 @@ */ import { credentialPath } from "./paths.js"; -import { readStore, writeStore, deleteStore, type StoredUserInfo } from "./store.js"; +import { + readStore, + writeStore, + deleteStore, + hasPreservedUnknownData, + type StoredUserInfo, +} from "./store.js"; export type { StoredUserInfo } from "./store.js"; @@ -27,9 +33,13 @@ export function isUserInfoEmpty(u: StoredUserInfo): boolean { /** * Most friendly name available, in priority order: - * email > "first last" > first > last > username > undefined. - * The caller falls back to its own marker (e.g. "(unknown user)") on - * `undefined`. Mirrors `UserInfo.DisplayName()` in heygen-cli. + * email > "first last" > first-only > last-only > username > undefined. + * The "first-only" / "last-only" intermediates come from `combineName`, + * which returns `first || last || ""` when only one of the two is present + * (so a user with just a first name still resolves to that first name, + * not straight to username). The caller falls back to its own marker + * (e.g. "(unknown user)") on `undefined`. Mirrors `UserInfo.DisplayName()` + * in heygen-cli. */ export function userDisplayName(u: StoredUserInfo): string | undefined { if (u.email) return u.email; @@ -38,6 +48,12 @@ export function userDisplayName(u: StoredUserInfo): string | undefined { return u.username || undefined; } +/** + * `"first last"` when both are present; otherwise `first || last || ""` + * (one-sided fall-through), and `""` when neither is set. The empty-string + * return is treated as falsy by `userDisplayName`, so it falls through to + * `username`. + */ function combineName(first?: string, last?: string): string { if (first && last) return `${first} ${last}`; return first || last || ""; @@ -80,15 +96,18 @@ export async function loadUserInfo(path = credentialPath()): Promise { const { credentials, source } = await readStore(path); if (source === "absent" || !credentials.user) return; delete credentials.user; - if (!credentials.api_key && !credentials.oauth) { + if (!credentials.api_key && !credentials.oauth && !hasPreservedUnknownData(credentials)) { await deleteStore(path); return; } diff --git a/packages/cli/src/commands/auth/login.test.ts b/packages/cli/src/commands/auth/login.test.ts index 3f332b98b2..b1d103ee99 100644 --- a/packages/cli/src/commands/auth/login.test.ts +++ b/packages/cli/src/commands/auth/login.test.ts @@ -139,6 +139,25 @@ describe("auth login --api-key rollback", () => { expect(credentials.user).toEqual({ email: "prev@example.com" }); }); + it("rollback on a rejected key preserves a prior foreign top-level key (no known credential)", async () => { + // The prior file held ONLY a future/foreign top-level key — no + // api_key, no oauth. A rejected new key must roll back WITHOUT + // deleting the file, or the foreign credential another CLI owns is + // clobbered. (Before the fix, rollback deleted the file because + // neither api_key nor oauth was present.) + await fs.writeFile( + join(dir, "credentials"), + JSON.stringify({ future_credential: { token: "owned_by_other_cli" } }), + { mode: 0o600 }, + ); + verifyState.reject = true; + await expect(runLogin("hg_badnewkey")).rejects.toThrow(/process\.exit:1/); + + const onDisk = JSON.parse(await fs.readFile(join(dir, "credentials"), "utf8")); + expect(onDisk.api_key).toBeUndefined(); + expect(onDisk.future_credential).toEqual({ token: "owned_by_other_cli" }); + }); + it("preserves an unknown/foreign top-level key across a successful re-login", async () => { // Cross-CLI invariant end-to-end: a key heygen-cli (or a future // version) wrote must survive a hyperframes-cli login round-trip. diff --git a/packages/cli/src/commands/auth/login.ts b/packages/cli/src/commands/auth/login.ts index fd88b6a0d5..6058857a1e 100644 --- a/packages/cli/src/commands/auth/login.ts +++ b/packages/cli/src/commands/auth/login.ts @@ -28,6 +28,7 @@ import { assertOAuthConfiguredOrExit, clearUserInfo, deleteStore, + hasPreservedUnknownData, isAuthError, isHeaderSafe, isUserInfoEmpty, @@ -201,13 +202,18 @@ async function snapshotStore(): Promise { async function rollback(previous: Credentials): Promise { try { - if (previous.api_key || previous.oauth) { + if (previous.api_key || previous.oauth || hasPreservedUnknownData(previous)) { + // Restore the prior state. This branch also covers the case where + // the only prior content was an unknown/foreign top-level key (a + // future credential another CLI owns): writing `previous` back + // re-emits that key, so the rollback doesn't clobber cross-CLI data + // the file had before this login attempt. await writeStore(previous); console.error(c.dim("Rolled back to the previous credential.")); } else { - // No prior credential — restore true absence. Leaving the - // rejected key on disk would make the next `auth status` / - // command silently resolve a known-bad key. + // No prior credential and nothing worth preserving — restore true + // absence. Leaving the rejected key on disk would make the next + // `auth status` / command silently resolve a known-bad key. await deleteStore(); console.error(c.dim("Removed the rejected credential.")); }