fix(cli): address code-review findings on OAuth PR

Author: jrusso1020

packages/cli/src/auth/_test-utils.ts

@@ -0,0 +1,54 @@
+/**
+ * Shared fixtures for auth-module tests. Centralises the env-snapshot
+ * + tmp-config-dir pattern so resolver.test.ts and oauth.test.ts don't
+ * each maintain a copy of the same beforeEach/afterEach plumbing.
+ *
+ * Only loaded by `*.test.ts` — runtime code doesn't depend on it.
+ */
+
+import { promises as fs } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+const ENV_KEYS = [
+  "HEYGEN_API_KEY",
+  "HYPERFRAMES_API_KEY",
+  "HEYGEN_CONFIG_DIR",
+  "HEYGEN_API_URL",
+  "HYPERFRAMES_OAUTH_CLIENT_ID",
+] as const;
+
+type EnvKey = (typeof ENV_KEYS)[number];
+
+export interface EnvFixture {
+  /** Tmp config dir; deleted on `restore()`. */
+  dir: string;
+  /** Restore env + delete tmp dir. Idempotent. */
+  restore: () => Promise<void>;
+}
+
+/**
+ * Take a snapshot of the auth-related env, clear them, make a tmp
+ * `HEYGEN_CONFIG_DIR`, and return a `restore()` that undoes all of
+ * the above.
+ */
+export async function setupTempAuthEnv(prefix = "hf-auth-test-"): Promise<EnvFixture> {
+  const dir = await fs.mkdtemp(join(tmpdir(), prefix));
+  const saved: Partial<Record<EnvKey, string | undefined>> = {};
+  for (const k of ENV_KEYS) {
+    saved[k] = process.env[k];
+    delete process.env[k];
+  }
+  process.env["HEYGEN_CONFIG_DIR"] = dir;
+
+  const restore = async (): Promise<void> => {
+    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 });
+  };
+
+  return { dir, restore };
+}

packages/cli/src/auth/browser.ts

@@ -0,0 +1,36 @@
+/**
+ * Open a URL in the user's default browser. Falls back to printing the
+ * URL when no browser is openable (SSH session, CI, `BROWSER=none`,
+ * or `open` rejects).
+ */
+
+import { c } from "../ui/colors.js";
+
+export interface OpenBrowserResult {
+  /** True when we successfully invoked the platform "open" command. */
+  opened: boolean;
+}
+
+export async function openBrowser(url: string): Promise<OpenBrowserResult> {
+  if (process.env["BROWSER"] === "none" || process.env["HF_NO_BROWSER"] === "1") {
+    printManualInstructions(url);
+    return { opened: false };
+  }
+  try {
+    const open = (await import("open")).default;
+    await open(url);
+    return { opened: true };
+  } catch (err) {
+    printManualInstructions(url, err instanceof Error ? err.message : String(err));
+    return { opened: false };
+  }
+}
+
+function printManualInstructions(url: string, detail?: string): void {
+  if (detail) {
+    console.error(c.warn(`Could not open browser automatically (${detail}).`));
+  } else {
+    console.error(c.warn("Browser auto-open is disabled."));
+  }
+  console.error(`Open this URL manually to continue:\n  ${c.accent(url)}`);
+}

packages/cli/src/auth/client.test.ts

@@ -133,6 +133,79 @@ describe("auth/client", () => {
     throw new Error("expected rejection");
   });
 
+  it("getCurrentUser retries once on 401 when refresh hook is configured for OAuth", async () => {
+    let callCount = 0;
+    const observed: string[] = [];
+    const fetchImpl = (async (_url: string, init?: RequestInit) => {
+      callCount++;
+      const headers = (init?.headers as Record<string, string>) ?? {};
+      observed.push(headers["authorization"] ?? "");
+      if (callCount === 1) return new Response("expired", { status: 401 });
+      return new Response(JSON.stringify({ email: "a@b" }), {
+        status: 200,
+        headers: { "content-type": "application/json" },
+      });
+    }) as unknown as typeof fetch;
+
+    const client = new AuthClient({
+      baseUrl: "https://api.test.example",
+      fetchImpl,
+      onUnauthenticatedRefresh: async () => "fresh_at",
+    });
+    const user = await client.getCurrentUser({
+      type: "oauth",
+      access_token: "stale_at",
+      refresh_token: "rt",
+      source: "file_json",
+      refreshable: true,
+    });
+    expect(user.email).toBe("a@b");
+    expect(observed[0]).toBe("Bearer stale_at");
+    expect(observed[1]).toBe("Bearer fresh_at");
+    expect(callCount).toBe(2);
+  });
+
+  it("getCurrentUser does NOT retry on 401 for api_key credentials", async () => {
+    let callCount = 0;
+    const fetchImpl = (async () => {
+      callCount++;
+      return new Response("invalid", { status: 401 });
+    }) as unknown as typeof fetch;
+    const client = new AuthClient({
+      baseUrl: "https://api.test.example",
+      fetchImpl,
+      onUnauthenticatedRefresh: async () => "fresh",
+    });
+    await expect(client.getCurrentUser(apiKeyCred())).rejects.toSatisfy((err) => {
+      return isAuthError(err) && (err as { code: string }).code === "UNAUTHENTICATED";
+    });
+    expect(callCount).toBe(1);
+  });
+
+  it("getCurrentUser surfaces 401 when refresh hook returns null (refresh failed)", async () => {
+    const fetchImpl = (async () =>
+      new Response("nope", { status: 401 })) as unknown as typeof fetch;
+    const { ErrRefreshFailed } = await import("./errors.js");
+    const client = new AuthClient({
+      baseUrl: "https://api.test.example",
+      fetchImpl,
+      onUnauthenticatedRefresh: async () => {
+        throw ErrRefreshFailed("invalid_grant");
+      },
+    });
+    await expect(
+      client.getCurrentUser({
+        type: "oauth",
+        access_token: "stale",
+        refresh_token: "rt",
+        source: "file_json",
+        refreshable: true,
+      }),
+    ).rejects.toSatisfy((err) => {
+      return isAuthError(err) && (err as { code: string }).code === "UNAUTHENTICATED";
+    });
+  });
+
   it("getCurrentUser sends the right header for oauth credentials", async () => {
     let captured: Record<string, string> = {};
     const fetchImpl = (async (_url: string, init?: RequestInit) => {

packages/cli/src/auth/client.ts

@@ -15,7 +15,7 @@
  * `movio/api_service/app/controller/user_v3.py`.
  */
 
-import { ErrApi, ErrUnauthenticated } from "./errors.js";
+import { ErrApi, ErrUnauthenticated, isAuthError } from "./errors.js";
 import type { ResolvedCredential } from "./resolver.js";
 
 const DEFAULT_BASE_URL = "https://api.heygen.com";
@@ -63,27 +63,63 @@ export interface AuthClientOptions {
   baseUrl?: string;
   /** Inject a custom fetch (used by tests). */
   fetchImpl?: typeof fetch;
+  /**
+   * Hook for refreshing an OAuth credential on 401. The hook should
+   * exchange the supplied refresh_token for new tokens, persist them,
+   * and return the new bearer to retry with. Wired in by the auth
+   * commands; injectable for tests.
+   */
+  onUnauthenticatedRefresh?: (refresh_token: string) => Promise<string>;
 }
 
 export class AuthClient {
   private readonly base: string;
   private readonly fetchImpl: typeof fetch;
+  private readonly onRefresh?: (refresh_token: string) => Promise<string>;
 
   constructor(opts: AuthClientOptions = {}) {
     this.base = (opts.baseUrl ?? apiBaseUrl()).replace(/\/+$/, "");
     this.fetchImpl = opts.fetchImpl ?? fetch;
+    this.onRefresh = opts.onUnauthenticatedRefresh;
   }
 
   /**
    * `GET /v3/users/me`. Throws `ErrUnauthenticated` on 401, `ErrApi`
    * on any other non-2xx or non-JSON body.
+   *
+   * On OAuth 401 with a refresh hook configured, the request is
+   * retried once after refreshing the access token. The retry's
+   * outcome is what the caller sees — if the refresh itself fails
+   * (REFRESH_FAILED) or the retry still 401s, the user lands on a
+   * "please log in again" path upstream.
    */
   async getCurrentUser(credential: ResolvedCredential): Promise<UserInfo> {
     const url = `${this.base}/v3/users/me`;
+    return await this.fetchUser(url, credential, true);
+  }
+
+  // fallow-ignore-next-line complexity
+  private async fetchUser(
+    url: string,
+    credential: ResolvedCredential,
+    allowRefresh: boolean,
+  ): Promise<UserInfo> {
     const headers = buildAuthHeaders(credential);
     const res = await this.fetchImpl(url, { method: "GET", headers });
 
     if (res.status === 401) {
+      if (
+        allowRefresh &&
+        credential.type === "oauth" &&
+        credential.refresh_token &&
+        this.onRefresh
+      ) {
+        const refreshed = await this.tryRefresh(credential.refresh_token);
+        if (refreshed) {
+          const next: ResolvedCredential = { ...credential, access_token: refreshed };
+          return await this.fetchUser(url, next, false);
+        }
+      }
       const detail = await safeText(res);
       throw ErrUnauthenticated(detail || `${res.status} ${res.statusText}`);
     }
@@ -99,6 +135,19 @@ export class AuthClient {
     }
     return extractUserInfo(payload);
   }
+
+  private async tryRefresh(refresh_token: string): Promise<string | null> {
+    if (!this.onRefresh) return null;
+    try {
+      return await this.onRefresh(refresh_token);
+    } catch (err) {
+      // Refresh failure should be surfaced upstream by the caller via
+      // the retry's 401, not by throwing here — so callers consistently
+      // see "please log in again" rather than mixed error types.
+      if (isAuthError(err) && err.code === "REFRESH_FAILED") return null;
+      throw err;
+    }
+  }
 }
 
 export function buildAuthHeaders(credential: ResolvedCredential): Record<string, string> {

packages/cli/src/auth/errors.ts

@@ -3,7 +3,13 @@
  * can map specific failures to friendly UX without parsing messages.
  */
 
-export type AuthErrorCode = "NOT_CONFIGURED" | "INVALID_STORE" | "API_ERROR" | "UNAUTHENTICATED";
+export type AuthErrorCode =
+  | "NOT_CONFIGURED"
+  | "INVALID_STORE"
+  | "API_ERROR"
+  | "UNAUTHENTICATED"
+  | "OAUTH_NOT_CONFIGURED"
+  | "REFRESH_FAILED";
 
 export class AuthError extends Error {
   readonly code: AuthErrorCode;
@@ -41,6 +47,20 @@ export const ErrUnauthenticated = (detail?: string) =>
 export const ErrApi = (status: number, detail: string) =>
   new AuthError("API_ERROR", `HeyGen API error (${status}): ${detail}`);
 
+export const ErrOAuthNotConfigured = () =>
+  new AuthError(
+    "OAUTH_NOT_CONFIGURED",
+    "OAuth client is not configured",
+    "Set HYPERFRAMES_OAUTH_CLIENT_ID, or run `hyperframes auth login --api-key`.",
+  );
+
+export const ErrRefreshFailed = (detail?: string) =>
+  new AuthError(
+    "REFRESH_FAILED",
+    detail ? `Failed to refresh OAuth tokens: ${detail}` : "Failed to refresh OAuth tokens",
+    "Run `hyperframes auth login` to re-authenticate.",
+  );
+
 export function isAuthError(err: unknown): err is AuthError {
   return err instanceof AuthError;
 }

packages/cli/src/auth/index.ts

@@ -15,3 +15,10 @@ export type { ResolvedCredential } from "./resolver.js";
 
 export { AuthClient } from "./client.js";
 export type { UserInfo } from "./client.js";
+
+export {
+  assertOAuthConfiguredOrExit,
+  refreshTokens,
+  revokeTokens,
+  startAuthorizationCodeFlow,
+} from "./oauth.js";

packages/cli/src/auth/loopback.test.ts

@@ -0,0 +1,76 @@
+import { afterEach, describe, expect, it } from "vitest";
+import { startLoopback, type LoopbackHandle } from "./loopback.js";
+
+describe("auth/loopback", () => {
+  let active: LoopbackHandle | null = null;
+
+  afterEach(async () => {
+    if (active) {
+      await active.close().catch(() => {});
+      active = null;
+    }
+  });
+
+  it("captures `code` when state matches", async () => {
+    const handle = await startLoopback({ state: "expected_state", timeoutMs: 5_000 });
+    active = handle;
+    const redirect = new URL(handle.redirectUri);
+    const callback = new URL(`${handle.redirectUri}?code=abc123&state=expected_state`);
+
+    const fetchPromise = fetch(callback.toString());
+    const result = await handle.result;
+    const res = await fetchPromise;
+
+    expect(result.code).toBe("abc123");
+    expect(result.redirectUri).toContain(redirect.host);
+    expect(res.status).toBe(200);
+    const body = await res.text();
+    expect(body).toContain("Signed in");
+  });
+
+  async function expectRejection(args: {
+    expectedState: string;
+    query: string;
+    pattern: RegExp;
+  }): Promise<void> {
+    const handle = await startLoopback({ state: args.expectedState, timeoutMs: 5_000 });
+    active = handle;
+    await fetch(`${handle.redirectUri}?${args.query}`).catch(() => {});
+    await expect(handle.result).rejects.toThrow(args.pattern);
+  }
+
+  it("rejects when state does not match", async () => {
+    await expectRejection({
+      expectedState: "expected",
+      query: "code=abc&state=wrong",
+      pattern: /state mismatch/i,
+    });
+  });
+
+  it("rejects when the IdP returns an error", async () => {
+    await expectRejection({
+      expectedState: "s",
+      query: "error=access_denied&error_description=user+denied&state=s",
+      pattern: /access_denied/,
+    });
+  });
+
+  it("rejects when code is missing from the callback", async () => {
+    await expectRejection({ expectedState: "s", query: "state=s", pattern: /code/ });
+  });
+
+  it("times out when no callback arrives", async () => {
+    const handle = await startLoopback({ state: "s", timeoutMs: 200 });
+    active = handle;
+    await expect(handle.result).rejects.toThrow(/timed out/i);
+  });
+
+  it("404s non-callback paths and does not resolve the flow", async () => {
+    const handle = await startLoopback({ state: "s", timeoutMs: 1_000 });
+    active = handle;
+    const res = await fetch(`${handle.redirectUri.replace("/oauth/callback", "/other")}`);
+    expect(res.status).toBe(404);
+    // Flow is still waiting — kill it via timeout.
+    await expect(handle.result).rejects.toThrow(/timed out/i);
+  });
+});