From 8e4d5620efb798d738081f8cdb20af03a48a3446 Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 17:57:04 -0700 Subject: [PATCH 1/8] fix(memfs): reset inherited credential helpers for the Letta host Carved out of #3653 so the auth approach can be reviewed and reverted independently of the shared-memory subcommand. Git accumulates credential helpers across config scopes and asks each in order; on macOS, Xcode's system gitconfig registers osxkeychain, which can hold a stale Letta token and answer before the repo-local helper (observed as HTTP 500s on agent-run `git pull` in shared memory mounts, LET-10545). Write an empty helper entry before ours to reset the inherited list, scoped to the Letta remote URL only. This matches what the harness's own git invocations have always done via `-c credential.helper=` in buildGitAuthArgs. Also makes the stale mount-path collision error actionable and exports cloneRepositoryMount for tests. Known issue (do not merge yet): the two new credential-reset tests assume the unix inline helper and fail on Windows CI; they need to be skipped or ported to the .cmd helper path. Open design question from review: whether to keep the persisted-token approach at all, vs injecting auth into agent shells (getShellEnv) like the harness does per-invocation, which would eliminate the on-disk token. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/source-file-size-baseline.json | 2 +- src/agent/memory-git.auth.test.ts | 144 +++++++++++++++++++++++++ src/agent/memory-git.ts | 34 ++++-- 3 files changed, 168 insertions(+), 12 deletions(-) diff --git a/scripts/source-file-size-baseline.json b/scripts/source-file-size-baseline.json index 42863974c8..d7aa9a8837 100644 --- a/scripts/source-file-size-baseline.json +++ b/scripts/source-file-size-baseline.json @@ -1,6 +1,6 @@ { "src/agent/client-skills.test.ts": 1196, - "src/agent/memory-git.ts": 2128, + "src/agent/memory-git.ts": 2140, "src/backend/local-backend.test.ts": 2532, "src/backend/local/local-backend.ts": 1014, "src/backend/local/local-store.ts": 3594, diff --git a/src/agent/memory-git.auth.test.ts b/src/agent/memory-git.auth.test.ts index 7e5d12aff9..4ce14dfe6b 100644 --- a/src/agent/memory-git.auth.test.ts +++ b/src/agent/memory-git.auth.test.ts @@ -14,6 +14,7 @@ import { buildGitAuthArgs, buildMemfsGitProxyArgs, buildNonInteractiveGitEnv, + cloneRepositoryMount, formatGitCredentialHelperPath, getAgentRootDir, getGitRemoteUrl, @@ -707,3 +708,146 @@ describe("syncPendingMemoryCommitsAfterTurn", () => { expect(git(repo, "rev-list --count @{u}..HEAD").trim()).toBe("1"); }); }); + +describe("cloneRepositoryMount", () => { + function makeRemoteWithContent(): string { + const remote = makeBareGitRepo(); + const source = cloneRepo(remote); + commitFile(source, "shared.md", "shared content"); + git(source, "push -u origin main"); + return remote; + } + + test("clones a fresh mount and configures credential reset + helper", async () => { + process.env.LETTA_BASE_URL = "https://api.letta.com"; + // The persistent credential helper is deliberately skipped in Desktop + // proxy transport sessions; this test covers the normal CLI path. + delete process.env.LETTA_MEMFS_GIT_PROXY_BASE_URL; + const remote = makeRemoteWithContent(); + const mountParent = mkdtempSync(join(tmpdir(), "repo-mount-")); + tempDirs.push(mountParent); + const directory = join(mountParent, "shared-notes"); + + await cloneRepositoryMount({ + agentId: "agent-123", + repositoryName: "shared-notes", + directory, + remoteUrl: remote, + token: "test-token", + }); + + expect(existsSync(join(directory, ".git"))).toBe(true); + expect(existsSync(join(directory, "shared.md"))).toBe(true); + // Credential helper list is reset (empty entry) before our inline helper, + // so inherited system/global helpers (e.g. osxkeychain) cannot answer + // first with a stale credential for the Letta host. + const helpers = git( + directory, + "config --local --get-all credential.https://api.letta.com.helper", + ).split("\n"); + expect(helpers[0]).toBe(""); + expect(helpers[1]).toContain("password="); + }); + + test("fails with an actionable error when a non-git directory occupies the mount path", async () => { + process.env.LETTA_BASE_URL = "https://api.letta.com"; + const remote = makeRemoteWithContent(); + const mountParent = mkdtempSync(join(tmpdir(), "repo-mount-")); + tempDirs.push(mountParent); + const directory = join(mountParent, "shared-notes"); + + // Simulate the stale plain-directory state agents created by hand. + mkdirSync(directory, { recursive: true }); + writeFileSync(join(directory, "stranded.md"), "un-synced work", "utf-8"); + + await expect( + cloneRepositoryMount({ + agentId: "agent-123", + repositoryName: "shared-notes", + directory, + remoteUrl: remote, + token: "test-token", + }), + ).rejects.toThrow(/not a git repository.*Move or delete/s); + + // The directory is left untouched for the agent to inspect and resolve. + expect(existsSync(join(directory, "stranded.md"))).toBe(true); + expect(existsSync(join(directory, ".git"))).toBe(false); + }); + + test("pulls an existing git mount", async () => { + process.env.LETTA_BASE_URL = "https://api.letta.com"; + const remote = makeRemoteWithContent(); + const mountParent = mkdtempSync(join(tmpdir(), "repo-mount-")); + tempDirs.push(mountParent); + const directory = join(mountParent, "shared-notes"); + + await cloneRepositoryMount({ + agentId: "agent-123", + repositoryName: "shared-notes", + directory, + remoteUrl: remote, + token: "test-token", + }); + + // Push a new commit from another clone, then re-sync. + const other = cloneRepo(remote); + commitFile(other, "update.md", "second agent write"); + git(other, "push origin main"); + + await cloneRepositoryMount({ + agentId: "agent-123", + repositoryName: "shared-notes", + directory, + remoteUrl: remote, + token: "test-token", + }); + + expect(existsSync(join(directory, "update.md"))).toBe(true); + }); +}); + +describe("credential helper reset behavior", () => { + test("repo-local reset entry prevents inherited helpers from answering for the Letta host", () => { + const repo = makeGitRepo(); + const helperCalledMarker = join(repo, "global-helper-called"); + const fakeGlobalHelper = join(repo, "fake-global-helper.sh"); + writeFileSync( + fakeGlobalHelper, + `#!/bin/sh\ntouch ${helperCalledMarker}\necho "username=stale"\necho "password=stale-keychain-token"\n`, + { mode: 0o755 }, + ); + const globalConfig = join(repo, "globalconfig"); + writeFileSync( + globalConfig, + `[credential]\n\thelper = ${fakeGlobalHelper}\n`, + "utf-8", + ); + + // Mirror configureLocalCredentialHelper's write pattern. + git( + repo, + 'config --replace-all credential.https://api.letta.com.helper ""', + ); + execSync( + `git config --add credential.https://api.letta.com.helper '!f() { echo "username=letta"; echo "password=fresh-token"; }; f'`, + { cwd: repo, stdio: "ignore" }, + ); + + const filled = execSync("git credential fill", { + cwd: repo, + encoding: "utf-8", + input: "protocol=https\nhost=api.letta.com\n\n", + env: { + ...process.env, + GIT_CONFIG_GLOBAL: globalConfig, + GIT_CONFIG_SYSTEM: "/dev/null", + GIT_TERMINAL_PROMPT: "0", + }, + }); + + expect(filled).toContain("username=letta"); + expect(filled).toContain("password=fresh-token"); + expect(existsSync(helperCalledMarker)).toBe(false); + }); +}); diff --git a/src/agent/memory-git.ts b/src/agent/memory-git.ts index 8be00a3edc..c46f5cfc30 100644 --- a/src/agent/memory-git.ts +++ b/src/agent/memory-git.ts @@ -324,13 +324,20 @@ async function prepareAttachedRepositoryForGitOps(args: { await ensureLocalMemfsGitConfig(args.directory, args.agentId); } -async function cloneRepositoryMount(args: { +export async function cloneRepositoryMount(args: { agentId: string; repositoryName: string; directory: string; remoteUrl: string; token: string; }): Promise { + if (existsSync(args.directory) && !existsSync(join(args.directory, ".git"))) { + throw new Error( + `repository mount path already exists and is not a git repository: ${args.directory}. ` + + `Move or delete that directory, then re-run the sync to clone the mount.`, + ); + } + if (!existsSync(args.directory)) { mkdirSync(args.directory, { recursive: true }); try { @@ -347,10 +354,6 @@ async function cloneRepositoryMount(args: { rmSync(args.directory, { recursive: true, force: true }); throw err; } - } else if (!existsSync(join(args.directory, ".git"))) { - throw new Error( - `repository mount path already exists and is not a git repository: ${args.directory}`, - ); } else { await prepareAttachedRepositoryForGitOps(args); await runGitWithRetry(args.directory, ["pull", "--ff-only"], args.token, { @@ -750,16 +753,25 @@ echo password=${token} helper = `!f() { echo "username=letta"; echo "password=${token}"; }; f`; } + // Git accumulates credential helpers across config scopes (system → global + // → local) and asks each in order; a system/global helper like macOS + // osxkeychain can hold a stale credential for the Letta host and answer + // before our repo-local helper, sending the wrong identity (observed as + // HTTP 500s on plain `git pull` in shared memory mounts, LET-10545). An + // empty helper entry resets the accumulated list, so write "" then our + // helper — scoped to the Letta remote URL only, leaving the user's helpers + // intact for every other host (e.g. /memory-repository GitHub mirrors). + const writeHelperWithReset = async (key: string) => { + await gitConfig(dir, ["config", "--replace-all", key, ""]); + await gitConfig(dir, ["config", "--add", key, helper]); + }; + // Primary config: normalized origin key (most robust for git's credential lookup) - await gitConfig(dir, [ - "config", - `credential.${normalizedBaseUrl}.helper`, - helper, - ]); + await writeHelperWithReset(`credential.${normalizedBaseUrl}.helper`); // Backcompat: also set raw configured URL key if it differs (older repos/configs) if (rawBaseUrl !== normalizedBaseUrl) { - await gitConfig(dir, ["config", `credential.${rawBaseUrl}.helper`, helper]); + await writeHelperWithReset(`credential.${rawBaseUrl}.helper`); } debugLog( From 1d9a26406ef7274b39ed74afbeee836358153435 Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 18:21:22 -0700 Subject: [PATCH 2/8] feat(cli): resolve memory-repo git auth via a dynamic credential helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the static token persisted in memory repos' .git/config (and the Windows .cmd token file) with `letta git-credential` — a dynamic helper following the `gh auth git-credential` pattern. The repo config now holds only a command reference: [credential "https://api.letta.com"] helper = ; resets inherited helpers (osxkeychain) helper = !letta git-credential ; resolves harness auth per operation Fixes both defects from LET-10545's plain-git failure mode: the keychain can no longer answer first with a stale token (reset entry, kept from the previous commit), and rotation can no longer strand a stale token on disk (token resolved fresh via getClient — env key, keychain OAuth, single- flight refresh — on every git network operation). store/erase are no-ops, so nothing writes our token back into the keychain either. Latency: the helper runs on every push/pull, so standalone-entry.ts dispatches `git-credential` before importing the main CLI graph and the subcommand module has no static imports. Measured ~50-70ms end to end vs ~1.1s through the full graph. Also drops the platform split that broke Windows CI: `!` helpers run under git's bundled sh everywhere, and the rewritten tests use argv-array git invocations with no chmod'd scripts. Legacy .cmd helper files are removed on the next configure pass; existing repos self-heal on pull/sync. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/source-file-size-baseline.json | 2 +- src/agent/memory-git.auth.test.ts | 110 +++++++++----- src/agent/memory-git.ts | 81 +++++------ src/cli/subcommands/git-credential.test.ts | 157 ++++++++++++++++++++ src/cli/subcommands/git-credential.ts | 158 +++++++++++++++++++++ src/cli/subcommands/router.ts | 7 + src/standalone-entry.ts | 15 +- 7 files changed, 442 insertions(+), 88 deletions(-) create mode 100644 src/cli/subcommands/git-credential.test.ts create mode 100644 src/cli/subcommands/git-credential.ts diff --git a/scripts/source-file-size-baseline.json b/scripts/source-file-size-baseline.json index d7aa9a8837..776e5cedb5 100644 --- a/scripts/source-file-size-baseline.json +++ b/scripts/source-file-size-baseline.json @@ -1,6 +1,6 @@ { "src/agent/client-skills.test.ts": 1196, - "src/agent/memory-git.ts": 2140, + "src/agent/memory-git.ts": 2123, "src/backend/local-backend.test.ts": 2532, "src/backend/local/local-backend.ts": 1014, "src/backend/local/local-store.ts": 3594, diff --git a/src/agent/memory-git.auth.test.ts b/src/agent/memory-git.auth.test.ts index 4ce14dfe6b..dd4fc9888b 100644 --- a/src/agent/memory-git.auth.test.ts +++ b/src/agent/memory-git.auth.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { execSync } from "node:child_process"; +import { execFileSync, execSync } from "node:child_process"; import { existsSync, mkdirSync, @@ -7,7 +7,7 @@ import { rmSync, writeFileSync, } from "node:fs"; -import { tmpdir } from "node:os"; +import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import { assertMemoryRepoCleanForWrite, @@ -15,7 +15,6 @@ import { buildMemfsGitProxyArgs, buildNonInteractiveGitEnv, cloneRepositoryMount, - formatGitCredentialHelperPath, getAgentRootDir, getGitRemoteUrl, getMemoryRepoDir, @@ -311,18 +310,6 @@ describe("normalizeCredentialBaseUrl", () => { }); }); -describe("formatGitCredentialHelperPath", () => { - test("normalizes slashes and escapes whitespace for helper command parsing", () => { - expect( - formatGitCredentialHelperPath( - String.raw`C:\Users\Jane Doe\.letta\agents\agent-1\memory\.git\letta-credential-helper.cmd`, - ), - ).toBe( - "C:/Users/Jane\\ Doe/.letta/agents/agent-1/memory/.git/letta-credential-helper.cmd", - ); - }); -}); - describe("git auth hardening", () => { test("auth args pass Basic auth and suppress inherited credential helpers", () => { const args = buildGitAuthArgs("token-123"); @@ -738,15 +725,17 @@ describe("cloneRepositoryMount", () => { expect(existsSync(join(directory, ".git"))).toBe(true); expect(existsSync(join(directory, "shared.md"))).toBe(true); - // Credential helper list is reset (empty entry) before our inline helper, - // so inherited system/global helpers (e.g. osxkeychain) cannot answer - // first with a stale credential for the Letta host. + // Credential helper list is reset (empty entry) before the dynamic + // helper, so inherited system/global helpers (e.g. osxkeychain) cannot + // answer first with a stale credential for the Letta host. The helper + // itself is dynamic — no token is persisted in the config. const helpers = git( directory, "config --local --get-all credential.https://api.letta.com.helper", ).split("\n"); expect(helpers[0]).toBe(""); - expect(helpers[1]).toContain("password="); + expect(helpers[1]).toBe("!letta git-credential"); + expect(helpers.join("\n")).not.toContain("password="); }); test("fails with an actionable error when a non-git directory occupies the mount path", async () => { @@ -790,10 +779,13 @@ describe("cloneRepositoryMount", () => { token: "test-token", }); - // Push a new commit from another clone, then re-sync. + // Push a new commit from another clone, then re-sync. Also plant a + // legacy static-token helper script: re-configuring must remove it. const other = cloneRepo(remote); commitFile(other, "update.md", "second agent write"); git(other, "push origin main"); + const legacyHelper = join(directory, ".git", "letta-credential-helper.cmd"); + writeFileSync(legacyHelper, "@echo off\necho password=stale\n", "utf-8"); await cloneRepositoryMount({ agentId: "agent-123", @@ -804,44 +796,50 @@ describe("cloneRepositoryMount", () => { }); expect(existsSync(join(directory, "update.md"))).toBe(true); + expect(existsSync(legacyHelper)).toBe(false); }); }); describe("credential helper reset behavior", () => { test("repo-local reset entry prevents inherited helpers from answering for the Letta host", () => { const repo = makeGitRepo(); - const helperCalledMarker = join(repo, "global-helper-called"); - const fakeGlobalHelper = join(repo, "fake-global-helper.sh"); - writeFileSync( - fakeGlobalHelper, - `#!/bin/sh\ntouch ${helperCalledMarker}\necho "username=stale"\necho "password=stale-keychain-token"\n`, - { mode: 0o755 }, + // Everything below is platform-agnostic: `!` helpers always run under + // git's bundled sh (also on Windows), config writes use argv arrays (no + // outer shell quoting), and no chmod'd script files are involved. + const helperCalledMarker = join(repo, "global-helper-called").replace( + /\\/g, + "/", ); const globalConfig = join(repo, "globalconfig"); writeFileSync( globalConfig, - `[credential]\n\thelper = ${fakeGlobalHelper}\n`, + `[credential]\n\thelper = "!f() { touch ${helperCalledMarker}; echo username=stale; echo password=stale-keychain-token; }; f"\n`, "utf-8", ); - // Mirror configureLocalCredentialHelper's write pattern. - git( - repo, - 'config --replace-all credential.https://api.letta.com.helper ""', - ); - execSync( - `git config --add credential.https://api.letta.com.helper '!f() { echo "username=letta"; echo "password=fresh-token"; }; f'`, - { cwd: repo, stdio: "ignore" }, + // Mirror configureLocalCredentialHelper's write pattern: reset entry, + // then the dynamic helper (faked here so `fill` needs no letta on PATH). + const key = "credential.https://api.letta.com.helper"; + execFileSync("git", ["config", "--replace-all", key, ""], { cwd: repo }); + execFileSync( + "git", + [ + "config", + "--add", + key, + "!f() { echo username=letta; echo password=fresh-token; }; f", + ], + { cwd: repo }, ); - const filled = execSync("git credential fill", { + const filled = execFileSync("git", ["credential", "fill"], { cwd: repo, encoding: "utf-8", input: "protocol=https\nhost=api.letta.com\n\n", env: { ...process.env, GIT_CONFIG_GLOBAL: globalConfig, - GIT_CONFIG_SYSTEM: "/dev/null", + GIT_CONFIG_SYSTEM: platform() === "win32" ? "NUL" : "/dev/null", GIT_TERMINAL_PROMPT: "0", }, }); @@ -850,4 +848,42 @@ describe("credential helper reset behavior", () => { expect(filled).toContain("password=fresh-token"); expect(existsSync(helperCalledMarker)).toBe(false); }); + + test("without the reset entry the inherited helper answers first (regression control)", () => { + const repo = makeGitRepo(); + const globalConfig = join(repo, "globalconfig"); + writeFileSync( + globalConfig, + `[credential]\n\thelper = "!f() { echo username=stale; echo password=stale-keychain-token; }; f"\n`, + "utf-8", + ); + + const key = "credential.https://api.letta.com.helper"; + execFileSync( + "git", + [ + "config", + "--add", + key, + "!f() { echo username=letta; echo password=fresh-token; }; f", + ], + { cwd: repo }, + ); + + const filled = execFileSync("git", ["credential", "fill"], { + cwd: repo, + encoding: "utf-8", + input: "protocol=https\nhost=api.letta.com\n\n", + env: { + ...process.env, + GIT_CONFIG_GLOBAL: globalConfig, + GIT_CONFIG_SYSTEM: platform() === "win32" ? "NUL" : "/dev/null", + GIT_TERMINAL_PROMPT: "0", + }, + }); + + // Git returns the FIRST helper's answer — this documents the poisoning + // mechanism the reset entry exists to prevent. + expect(filled).toContain("password=stale-keychain-token"); + }); }); diff --git a/src/agent/memory-git.ts b/src/agent/memory-git.ts index c46f5cfc30..b31834a6fe 100644 --- a/src/agent/memory-git.ts +++ b/src/agent/memory-git.ts @@ -20,7 +20,7 @@ import { rmSync, writeFileSync, } from "node:fs"; -import { homedir, platform } from "node:os"; +import { homedir } from "node:os"; import { dirname, isAbsolute, join } from "node:path"; import { promisify } from "node:util"; import { getClient } from "@/backend/api/client"; @@ -115,16 +115,6 @@ export function normalizeCredentialBaseUrl(serverUrl: string): string { } } -/** - * Format an executable helper path for git config values. - * - * Git splits helper commands on whitespace, so we must escape any - * spaces/tabs in absolute paths (common on Windows profile paths). - */ -export function formatGitCredentialHelperPath(path: string): string { - return path.replace(/\\/g, "/").replace(/\s/g, "\\$&"); -} - function normalizeRemoteUrl(url: string): string { return url.trim().replace(/\/+$/, ""); } @@ -320,7 +310,7 @@ async function prepareAttachedRepositoryForGitOps(args: { token: string; }): Promise { await maybeUpdateRepositoryRemoteOrigin(args); - await configureLocalCredentialHelper(args.directory, args.token); + await configureLocalCredentialHelper(args.directory); await ensureLocalMemfsGitConfig(args.directory, args.agentId); } @@ -709,20 +699,25 @@ async function runGitWithRetry( throw new Error(`Unexpected retry loop exit for ${operation}`); } +/** + * The credential helper written into memory repos' .git/config. Dynamic: + * `letta git-credential` (src/cli/subcommands/git-credential.ts) resolves the + * current harness token on every git network operation, so no secret is + * persisted and OAuth rotation cannot strand a stale credential. Relies on + * `letta` being resolvable from PATH when git runs — true in agent shells + * (shell shim) and normal installs. Git executes `!` helpers through its + * bundled sh on every platform, including Windows. + */ +const DYNAMIC_CREDENTIAL_HELPER = "!letta git-credential"; + /** * Configure a local credential helper in the repo's .git/config * so plain `git push` / `git pull` work without auth prefixes. - * Skipped in Desktop proxy transport mode because the listener only has a - * local session token; persisting that token under api.letta.com would break - * normal CLI/TUI sessions that share the same memory repo. - * - * On Windows, we write a batch script because the bash-style inline - * helper (`!f() { ... }; f`) doesn't work in PowerShell/cmd. + * Skipped in Desktop proxy transport mode because the proxy owns auth there; + * agent-run git is URL-rewritten to the localhost proxy and never needs a + * credential for the canonical host. */ -async function configureLocalCredentialHelper( - dir: string, - token: string, -): Promise { +async function configureLocalCredentialHelper(dir: string): Promise { const rawBaseUrl = getMemfsServerUrl(); const normalizedBaseUrl = normalizeCredentialBaseUrl(rawBaseUrl); @@ -735,22 +730,16 @@ async function configureLocalCredentialHelper( return; } - let helper: string; + const helper = DYNAMIC_CREDENTIAL_HELPER; - if (platform() === "win32") { - // Windows: write a batch script to .git/ and reference it - const helperScriptPath = join(dir, ".git", "letta-credential-helper.cmd"); - const batchScript = `@echo off -echo username=letta -echo password=${token} -`; - writeFileSync(helperScriptPath, batchScript, "utf-8"); - // Use a normalized path and escape whitespace for profiles like "Jane Doe". - helper = formatGitCredentialHelperPath(helperScriptPath); - debugLog("memfs-git", `Wrote Windows credential helper script`); - } else { - // Unix/macOS: use inline bash helper - helper = `!f() { echo "username=letta"; echo "password=${token}"; }; f`; + // Remove the legacy Windows batch helper, which embedded a static token. + const legacyHelperScriptPath = join( + dir, + ".git", + "letta-credential-helper.cmd", + ); + if (existsSync(legacyHelperScriptPath)) { + rmSync(legacyHelperScriptPath, { force: true }); } // Git accumulates credential helpers across config scopes (system → global @@ -1106,10 +1095,9 @@ function isRecoverableMemoryPullHistoryError(error: unknown): boolean { async function prepareMemoryRepoForGitOps( memoryDir: string, agentId: string, - token: string, ): Promise { await maybeUpdateMemoryRemoteOrigin(memoryDir, agentId); - await configureLocalCredentialHelper(memoryDir, token); + await configureLocalCredentialHelper(memoryDir); installPreCommitHook(memoryDir); installPostCommitHook(memoryDir); await ensureLocalMemfsGitConfig(memoryDir, agentId); @@ -1375,12 +1363,7 @@ export async function commitMemoryWrite( ); } - const token = await getAuthToken(); - await prepareMemoryRepoForGitOps( - params.memoryDir, - params.author.agentId, - token, - ); + await prepareMemoryRepoForGitOps(params.memoryDir, params.author.agentId); const commitResult = await commitMemoryPaths( params.memoryDir, @@ -1689,7 +1672,7 @@ export async function cloneMemoryRepo(agentId: string): Promise { // Configure local credential helper so the agent can do plain // `git push` / `git pull` without auth prefixes. - await configureLocalCredentialHelper(dir, token); + await configureLocalCredentialHelper(dir); // Install commit hooks (pre-commit validates frontmatter; post-commit mirrors) installPreCommitHook(dir); @@ -1719,7 +1702,7 @@ export async function pullMemory( await maybeUpdateMemoryRemoteOrigin(dir, agentId); // Self-healing: ensure credential helper, hooks, and identity config are current - await configureLocalCredentialHelper(dir, token); + await configureLocalCredentialHelper(dir); installPreCommitHook(dir); installPostCommitHook(dir); await ensureLocalMemfsGitConfig(dir, agentId); @@ -1815,7 +1798,7 @@ export async function pushMemory(agentId: string): Promise { const token = await getAuthToken(); const dir = getMemoryRepoDir(agentId); - await prepareMemoryRepoForGitOps(dir, agentId, token); + await prepareMemoryRepoForGitOps(dir, agentId); await runGit(dir, ["push", "-u", "origin", "main"], token); } @@ -2023,7 +2006,7 @@ export async function syncPendingMemoryCommitsAfterTurn( } const token = await getAuthToken(); - await prepareMemoryRepoForGitOps(memoryDir, agentId, token); + await prepareMemoryRepoForGitOps(memoryDir, agentId); const divergence = await getMemoryAheadBehind(memoryDir); if (!divergence || divergence.ahead <= 0) { return { diff --git a/src/cli/subcommands/git-credential.test.ts b/src/cli/subcommands/git-credential.test.ts new file mode 100644 index 0000000000..3a46b2d4b0 --- /dev/null +++ b/src/cli/subcommands/git-credential.test.ts @@ -0,0 +1,157 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { + parseGitCredentialInput, + requestMatchesLettaHost, + runGitCredentialSubcommand, +} from "@/cli/subcommands/git-credential"; + +describe("parseGitCredentialInput", () => { + test("parses key=value lines and stops at the blank line", () => { + expect( + parseGitCredentialInput( + "protocol=https\nhost=api.letta.com\npath=v1/git/agent-1/state.git\n\nusername=ignored\n", + ), + ).toEqual({ + protocol: "https", + host: "api.letta.com", + path: "v1/git/agent-1/state.git", + }); + }); + + test("keeps '=' inside values and skips malformed lines", () => { + expect( + parseGitCredentialInput("host=localhost:8283\nnoequals\n=empty\na=b=c\n"), + ).toEqual({ host: "localhost:8283", a: "b=c" }); + }); + + test("handles empty input", () => { + expect(parseGitCredentialInput("")).toEqual({}); + }); +}); + +describe("requestMatchesLettaHost", () => { + test("matches the configured host, with and without port", () => { + expect( + requestMatchesLettaHost( + { host: "api.letta.com" }, + "https://api.letta.com", + ), + ).toBe(true); + expect( + requestMatchesLettaHost( + { host: "localhost:8283" }, + "http://localhost:8283", + ), + ).toBe(true); + }); + + test("rejects other hosts, missing hosts, and bad base URLs", () => { + expect( + requestMatchesLettaHost({ host: "github.com" }, "https://api.letta.com"), + ).toBe(false); + expect(requestMatchesLettaHost({}, "https://api.letta.com")).toBe(false); + expect(requestMatchesLettaHost({ host: "api.letta.com" }, "")).toBe(false); + }); +}); + +describe("runGitCredentialSubcommand", () => { + let stdout: string[] = []; + let stderr: string[] = []; + const originalWrite = process.stdout.write; + const originalError = console.error; + + beforeEach(() => { + stdout = []; + stderr = []; + process.stdout.write = ((chunk: string) => { + stdout.push(String(chunk)); + return true; + }) as typeof process.stdout.write; + console.error = (...args: unknown[]) => { + stderr.push(args.join(" ")); + }; + }); + + afterEach(() => { + process.stdout.write = originalWrite; + console.error = originalError; + }); + + const deps = (overrides: Record = {}) => ({ + resolveBaseUrl: async () => "https://api.letta.com", + resolveToken: async () => "fresh-token", + input: "protocol=https\nhost=api.letta.com\n\n", + ...overrides, + }); + + test("get prints the credential for the Letta host", async () => { + const code = await runGitCredentialSubcommand(["get"], deps()); + expect(code).toBe(0); + expect(stdout.join("")).toBe("username=letta\npassword=fresh-token\n"); + }); + + test("get stays silent for foreign hosts", async () => { + const code = await runGitCredentialSubcommand( + ["get"], + deps({ input: "protocol=https\nhost=github.com\n\n" }), + ); + expect(code).toBe(0); + expect(stdout.join("")).toBe(""); + }); + + test("get stays silent when unauthenticated", async () => { + const code = await runGitCredentialSubcommand( + ["get"], + deps({ resolveToken: async () => "" }), + ); + expect(code).toBe(0); + expect(stdout.join("")).toBe(""); + }); + + test("store and erase are silent no-ops", async () => { + for (const action of ["store", "erase"]) { + const code = await runGitCredentialSubcommand( + [action], + deps({ input: "host=api.letta.com\npassword=whatever\n\n" }), + ); + expect(code).toBe(0); + } + expect(stdout.join("")).toBe(""); + }); + + test("unknown action fails with usage", async () => { + const code = await runGitCredentialSubcommand(["frobnicate"], deps()); + expect(code).toBe(1); + expect(stderr.join("\n")).toContain("Usage"); + }); + + test("fails fast when token resolution exceeds the deadline", async () => { + const code = await runGitCredentialSubcommand( + ["get"], + deps({ + deadlineMs: 20, + resolveToken: () => + new Promise((resolve) => + setTimeout(() => resolve("late"), 5_000), + ), + }), + ); + expect(code).toBe(1); + expect(stdout.join("")).toBe(""); + expect(stderr.join("\n")).toContain("timed out"); + }); + + test("fails without echoing secrets when resolution throws", async () => { + const code = await runGitCredentialSubcommand( + ["get"], + deps({ + resolveToken: async () => { + throw new Error("keychain unavailable"); + }, + }), + ); + expect(code).toBe(1); + expect(stdout.join("")).toBe(""); + expect(stderr.join("\n")).toContain("keychain unavailable"); + }); +}); diff --git a/src/cli/subcommands/git-credential.ts b/src/cli/subcommands/git-credential.ts new file mode 100644 index 0000000000..397f07a9ad --- /dev/null +++ b/src/cli/subcommands/git-credential.ts @@ -0,0 +1,158 @@ +/** + * `letta git-credential` — dynamic git credential helper for Letta-hosted + * repos (agent MemFS and shared memory mounts). + * + * Repo-local git config points at this subcommand (see + * configureLocalCredentialHelper in @/agent/memory-git), following the + * `gh auth git-credential` pattern: the token is resolved fresh from harness + * auth (env key, keychain OAuth, refresh) on every git network operation, so + * nothing secret is persisted on disk and rotation cannot strand a stale + * credential. + * + * Protocol: git invokes ` get|store|erase` with `key=value` lines on + * stdin. Only `get` answers, and only for the configured Letta host; `store` + * and `erase` are deliberate no-ops (the repo-local reset entry ensures no + * other helper stores our token either, which is what used to poison the + * macOS keychain). + * + * LATENCY: this runs on every `git push`/`pull`/`fetch` in a memory repo. + * src/standalone-entry.ts dispatches here before importing the main CLI + * graph, and this module has NO static imports — everything heavy (settings, + * API client) is imported lazily inside the token resolver. Keep it that way. + */ + +const RESOLVE_DEADLINE_MS = 10_000; + +type GitCredentialDeps = { + /** Resolve the current harness API token ("" when unauthenticated). */ + resolveToken?: () => Promise; + /** Resolve the canonical Letta MemFS base URL. */ + resolveBaseUrl?: () => Promise; + /** Raw stdin content (tests inject; defaults to reading process.stdin). */ + input?: string; + deadlineMs?: number; +}; + +/** Parse git's `key=value` credential-protocol lines (stops at blank line). */ +export function parseGitCredentialInput(input: string): Record { + const attributes: Record = {}; + for (const line of input.split("\n")) { + if (line.trim() === "") break; + const separator = line.indexOf("="); + if (separator <= 0) continue; + attributes[line.slice(0, separator)] = line.slice(separator + 1); + } + return attributes; +} + +/** + * Match git's requested host (`host[:port]`) against the configured Letta + * base URL. Anything else gets no answer — this helper only ever speaks for + * the Letta remote it was configured for. + */ +export function requestMatchesLettaHost( + request: Record, + baseUrl: string, +): boolean { + const requestHost = request.host?.trim(); + if (!requestHost) return false; + try { + const parsed = new URL(baseUrl.trim()); + return requestHost === parsed.host || requestHost === parsed.hostname; + } catch { + return false; + } +} + +async function readStdinToEnd(): Promise { + const chunks: Buffer[] = []; + for await (const chunk of process.stdin) { + chunks.push(Buffer.from(chunk)); + } + return Buffer.concat(chunks).toString("utf-8"); +} + +async function defaultResolveBaseUrl(): Promise { + const { settingsManager } = await import("@/settings-manager"); + await settingsManager.initialize(); + const { getMemfsServerUrl } = await import("@/backend/api/memfs-git-proxy"); + return getMemfsServerUrl(); +} + +/** + * Token resolution delegates to getClient(), which owns the full story: + * env LETTA_API_KEY → keychain secure tokens → single-flight OAuth refresh + * (persisted back to settings). Duplicating any of that here would fork + * auth behavior; the import cost is paid only after the host check passes. + */ +async function defaultResolveToken(): Promise { + const { getClient } = await import("@/backend/api/client"); + const client = await getClient(); + // biome-ignore lint/suspicious/noExplicitAny: accessing internal client options, same as memory-git's getAuthToken + return (client as any)._options?.apiKey ?? ""; +} + +function withDeadline(promise: Promise, ms: number): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new Error(`timed out after ${ms}ms`)), + ms, + ); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error) => { + clearTimeout(timer); + reject(error); + }, + ); + }); +} + +export async function runGitCredentialSubcommand( + argv: string[], + deps: GitCredentialDeps = {}, +): Promise { + const action = argv[0]; + if (action !== "get" && action !== "store" && action !== "erase") { + console.error("Usage: letta git-credential "); + return 1; + } + + // Always drain stdin so git never blocks on a closed-pipe write. + const input = deps.input ?? (await readStdinToEnd()); + + // store/erase: nothing is persisted, nothing to erase. + if (action !== "get") return 0; + + const request = parseGitCredentialInput(input); + const deadlineMs = deps.deadlineMs ?? RESOLVE_DEADLINE_MS; + + try { + const baseUrl = await withDeadline( + (deps.resolveBaseUrl ?? defaultResolveBaseUrl)(), + deadlineMs, + ); + // Not our host: stay silent and let git move on. Exit 0 mirrors how + // gh/gcloud helpers decline requests outside their domain. + if (!requestMatchesLettaHost(request, baseUrl)) return 0; + + const token = await withDeadline( + (deps.resolveToken ?? defaultResolveToken)(), + deadlineMs, + ); + if (!token) return 0; + + process.stdout.write(`username=letta\npassword=${token}\n`); + return 0; + } catch (error) { + // Fail fast (git surfaces "credential helper exited") rather than hang a + // push on a wedged keychain read or refresh call. Never echo the request. + console.error( + `letta git-credential: ${error instanceof Error ? error.message : String(error)}`, + ); + return 1; + } +} diff --git a/src/cli/subcommands/router.ts b/src/cli/subcommands/router.ts index bda23a0d87..ce7a1efd28 100644 --- a/src/cli/subcommands/router.ts +++ b/src/cli/subcommands/router.ts @@ -111,6 +111,13 @@ export async function runSubcommand(argv: string[]): Promise { const { runChannelGatewaySubcommand } = await import("./channel-gateway"); return runChannelGatewaySubcommand(rest); } + // The built binary dispatches git-credential in standalone-entry.ts + // before this router loads; this case covers the dev path + // (`bun src/index.ts git-credential`). + case "git-credential": { + const { runGitCredentialSubcommand } = await import("./git-credential"); + return runGitCredentialSubcommand(rest); + } case "local-backend": return runLocalBackendSubcommand(rest); case "trajectories": diff --git a/src/standalone-entry.ts b/src/standalone-entry.ts index 8085bd6fc4..3b972ea0e2 100644 --- a/src/standalone-entry.ts +++ b/src/standalone-entry.ts @@ -1,10 +1,23 @@ -import { registerBunOAuthFlows } from "@earendil-works/pi-ai/bun-oauth"; +// Fast path: `letta git-credential` runs on every git network operation in +// memory repos, so it must not pay for the full CLI import graph (Ink, +// telemetry, providers). Dispatch it before anything else loads. Everything +// in this file must stay dynamically imported — a static import would be +// hoisted ahead of this check and defeat the bypass. +if (process.argv[2] === "git-credential") { + const { runGitCredentialSubcommand } = await import( + "./cli/subcommands/git-credential" + ); + process.exit(await runGitCredentialSubcommand(process.argv.slice(3))); +} // pi-ai keeps Node-only OAuth implementations behind bundler-opaque imports. // Register the statically bundled loaders before the application imports any // provider runtime so the standalone letta.js never looks for sibling files. // This mirrors pi's standalone CLI bootstrap for the pinned pi-ai release: // https://github.com/earendil-works/pi/blob/20be4b18d4c57487f8993d2762bace129f0cf7c6/packages/coding-agent/src/bun/cli.ts#L2-L12 +const { registerBunOAuthFlows } = await import( + "@earendil-works/pi-ai/bun-oauth" +); registerBunOAuthFlows(); await import("./index"); From f6cf7428872f2459fc899b812e6f895406794311 Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 18:34:29 -0700 Subject: [PATCH 3/8] fix(auth): serialize OAuth refresh across processes and harden the helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two blockers from external design review of the dynamic credential helper, plus a host-matching hardening: 1. Cross-process refresh race: the server rotates the refresh token on every refresh (refresh_token_mode: "new"), and the existing single-flight guard is per-process — concurrent letta processes (CLI sessions, listeners, git-spawned helper invocations) could both burn the same refresh token and race their keychain writes, with the loser durably persisting an invalidated token (= logout). getClient()'s refresh block now runs under a file lock (~/.letta/oauth-refresh.lock, via the existing withFileLock util); waiters re-read the stored tokens after acquiring the lock and reuse the winner's result instead of refreshing again. Since both git delivery paths (harness extraHeader and the credential helper) funnel through getClient(), the lock sits below both — one resolution authority, two thin delivery adapters. 2. Durable persistence before exit: the rotated tokens are flushed before the lock releases, and the stored key is read back to surface silent persistence failures (best-effort when no keychain is available). The helper also awaits its stdout write, so the process.exit() in standalone-entry cannot truncate the credential handed to git. 3. Host matching now requires exact protocol + host[:port]: a plaintext-http remote pointed at the Letta hostname no longer receives the token. Fast path unaffected: still ~50ms end to end via standalone-entry. Co-Authored-By: Claude Opus 5 (1M context) --- src/backend/api/client.ts | 96 +++++++++++++++++----- src/cli/subcommands/git-credential.test.ts | 58 +++++++++++-- src/cli/subcommands/git-credential.ts | 26 ++++-- 3 files changed, 145 insertions(+), 35 deletions(-) diff --git a/src/backend/api/client.ts b/src/backend/api/client.ts index 9de28cf05f..2953243590 100644 --- a/src/backend/api/client.ts +++ b/src/backend/api/client.ts @@ -1,10 +1,12 @@ -import { hostname } from "node:os"; +import { homedir, hostname } from "node:os"; +import { join } from "node:path"; import Letta from "@letta-ai/letta-client"; import { LETTA_CLOUD_API_URL } from "@/auth/oauth"; import { refreshAccessTokenSingleFlight } from "@/auth/oauth-refresh"; import { type Settings, settingsManager } from "@/settings-manager"; import { trackBoundaryError } from "@/telemetry/error-reporting"; import { isDebugEnabled } from "@/utils/debug"; +import { withFileLock } from "@/utils/file-lock"; import { createTimingFetch, isTimingsEnabled } from "@/utils/timing"; import packageJson from "../../../package.json"; @@ -171,6 +173,71 @@ export function getClientDefaultHeaders(): Record { }; } +const TOKEN_REFRESH_WINDOW_MS = 5 * 60 * 1000; +const OAUTH_REFRESH_LOCK_TIMEOUT_MS = 30_000; + +/** + * Refresh OAuth tokens under a cross-process file lock. + * + * Every letta process refreshes through this path — CLI sessions, listeners, + * and each `letta git-credential` helper invocation spawned by git — and the + * server rotates the refresh token on every refresh (refresh_token_mode: + * "new"). Two concurrent refreshes therefore both burn the same refresh + * token and race their keychain writes; the loser can durably persist an + * already-invalidated token and log the user out. The in-process + * single-flight cannot see other processes, so a file lock serializes them: + * after acquiring it, re-read the stored tokens and reuse the winner's + * result instead of refreshing again. + */ +async function refreshTokensUnderCrossProcessLock( + fallbackRefreshToken: string, +): Promise { + const lockPath = join(homedir(), ".letta", "oauth-refresh.lock"); + return await withFileLock( + lockPath, + async () => { + // Another process may have completed the refresh while we waited. + const latest = await settingsManager.getSettingsWithSecureTokens(); + const latestKey = latest.env?.LETTA_API_KEY; + if ( + latestKey && + latest.tokenExpiresAt && + latest.tokenExpiresAt - Date.now() >= TOKEN_REFRESH_WINDOW_MS + ) { + return latestKey; + } + + const now = Date.now(); + const tokens = await refreshAccessTokenSingleFlight( + latest.refreshToken ?? fallbackRefreshToken, + settingsManager.getOrCreateDeviceId(), + hostname(), + ); + settingsManager.updateSettings({ + env: { LETTA_API_KEY: tokens.access_token }, + refreshToken: + tokens.refresh_token || latest.refreshToken || fallbackRefreshToken, + tokenExpiresAt: now + tokens.expires_in * 1000, + }); + // The rotated refresh token must be durably persisted before the lock + // releases — the pre-rotation token is already dead server-side. + // flush() awaits the write but swallows its errors, so read the stored + // key back where possible; an empty read-back means no keychain is + // available (file-fallback storage) and cannot be verified here. + await settingsManager.flush(); + const persisted = await settingsManager.getSettingsWithSecureTokens(); + const persistedKey = persisted.env?.LETTA_API_KEY; + if (persistedKey && persistedKey !== tokens.access_token) { + throw new Error( + "OAuth refresh succeeded but the rotated token failed to persist; if this recurs, re-run `letta` to re-authenticate", + ); + } + return tokens.access_token; + }, + { timeoutMs: OAUTH_REFRESH_LOCK_TIMEOUT_MS }, + ); +} + export async function getClient() { if (_testClientOverride) { return (await _testClientOverride()) as Letta; @@ -216,30 +283,15 @@ export async function getClient() { const now = Date.now(); const expiresAt = settings.tokenExpiresAt; - // Refresh if token expires within 5 minutes, or if the access token is - // missing entirely (e.g. transient keychain read failure during the - // delete-then-set window of a concurrent refresh). - if (!apiKey || expiresAt - now < 5 * 60 * 1000) { + // Refresh if token expires within the refresh window, or if the access + // token is missing entirely (e.g. transient keychain read failure during + // the delete-then-set window of a concurrent refresh). + if (!apiKey || expiresAt - now < TOKEN_REFRESH_WINDOW_MS) { try { - // Get or generate device ID (should always exist, but fallback just in case) - const deviceId = settingsManager.getOrCreateDeviceId(); - const deviceName = hostname(); - - const tokens = await refreshAccessTokenSingleFlight( + apiKey = await refreshTokensUnderCrossProcessLock( settings.refreshToken, - deviceId, - deviceName, ); - - // Update settings with new token (secrets handles secure storage automatically) - settingsManager.updateSettings({ - env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: tokens.refresh_token || settings.refreshToken, - tokenExpiresAt: now + tokens.expires_in * 1000, - }); - - apiKey = tokens.access_token; - _cachedApiKey = tokens.access_token; + _cachedApiKey = apiKey; } catch (error) { trackBoundaryError({ errorType: "auth_token_refresh_failed", diff --git a/src/cli/subcommands/git-credential.test.ts b/src/cli/subcommands/git-credential.test.ts index 3a46b2d4b0..21ec95c032 100644 --- a/src/cli/subcommands/git-credential.test.ts +++ b/src/cli/subcommands/git-credential.test.ts @@ -30,27 +30,60 @@ describe("parseGitCredentialInput", () => { }); describe("requestMatchesLettaHost", () => { - test("matches the configured host, with and without port", () => { + test("matches exact protocol + host, with and without port", () => { expect( requestMatchesLettaHost( - { host: "api.letta.com" }, + { protocol: "https", host: "api.letta.com" }, "https://api.letta.com", ), ).toBe(true); expect( requestMatchesLettaHost( - { host: "localhost:8283" }, + { protocol: "http", host: "localhost:8283" }, "http://localhost:8283", ), ).toBe(true); }); - test("rejects other hosts, missing hosts, and bad base URLs", () => { + test("rejects protocol downgrades and port mismatches", () => { + // A plaintext-http remote at the Letta hostname must not get the token. + expect( + requestMatchesLettaHost( + { protocol: "http", host: "api.letta.com" }, + "https://api.letta.com", + ), + ).toBe(false); + expect( + requestMatchesLettaHost( + { protocol: "https", host: "api.letta.com:8443" }, + "https://api.letta.com", + ), + ).toBe(false); + expect( + requestMatchesLettaHost( + { protocol: "https", host: "api.letta.com" }, + "https://api.letta.com:8443", + ), + ).toBe(false); + }); + + test("rejects other hosts, missing fields, and bad base URLs", () => { expect( - requestMatchesLettaHost({ host: "github.com" }, "https://api.letta.com"), + requestMatchesLettaHost( + { protocol: "https", host: "github.com" }, + "https://api.letta.com", + ), ).toBe(false); expect(requestMatchesLettaHost({}, "https://api.letta.com")).toBe(false); - expect(requestMatchesLettaHost({ host: "api.letta.com" }, "")).toBe(false); + expect( + requestMatchesLettaHost( + { host: "api.letta.com" }, + "https://api.letta.com", + ), + ).toBe(false); + expect( + requestMatchesLettaHost({ protocol: "https", host: "api.letta.com" }, ""), + ).toBe(false); }); }); @@ -63,8 +96,19 @@ describe("runGitCredentialSubcommand", () => { beforeEach(() => { stdout = []; stderr = []; - process.stdout.write = ((chunk: string) => { + process.stdout.write = (( + chunk: string, + encodingOrCallback?: unknown, + maybeCallback?: unknown, + ) => { stdout.push(String(chunk)); + const callback = + typeof encodingOrCallback === "function" + ? encodingOrCallback + : maybeCallback; + if (typeof callback === "function") { + (callback as () => void)(); + } return true; }) as typeof process.stdout.write; console.error = (...args: unknown[]) => { diff --git a/src/cli/subcommands/git-credential.ts b/src/cli/subcommands/git-credential.ts index 397f07a9ad..9e12c2493f 100644 --- a/src/cli/subcommands/git-credential.ts +++ b/src/cli/subcommands/git-credential.ts @@ -46,19 +46,26 @@ export function parseGitCredentialInput(input: string): Record { } /** - * Match git's requested host (`host[:port]`) against the configured Letta - * base URL. Anything else gets no answer — this helper only ever speaks for - * the Letta remote it was configured for. + * Match git's requested protocol + host (`host[:port]`) exactly against the + * configured Letta base URL. Anything else gets no answer — this helper only + * ever speaks for the Letta remote it was configured for. Protocol is + * checked so a plaintext-http remote pointed at the Letta hostname cannot + * coax the token out over an unencrypted transport. */ export function requestMatchesLettaHost( request: Record, baseUrl: string, ): boolean { const requestHost = request.host?.trim(); - if (!requestHost) return false; + const requestProtocol = request.protocol?.trim(); + if (!requestHost || !requestProtocol) return false; try { const parsed = new URL(baseUrl.trim()); - return requestHost === parsed.host || requestHost === parsed.hostname; + // URL#host omits the protocol's default port, matching how git reports + // the host for default-port remotes. + return ( + `${requestProtocol}:` === parsed.protocol && requestHost === parsed.host + ); } catch { return false; } @@ -145,7 +152,14 @@ export async function runGitCredentialSubcommand( ); if (!token) return 0; - process.stdout.write(`username=letta\npassword=${token}\n`); + // Await the write: the standalone entry calls process.exit() right after + // this returns, and an unflushed pipe write would truncate the credential + // git receives. + await new Promise((resolve, reject) => { + process.stdout.write(`username=letta\npassword=${token}\n`, (error) => + error ? reject(error) : resolve(), + ); + }); return 0; } catch (error) { // Fail fast (git surfaces "credential helper exited") rather than hang a From 807f98acf2e1f2a9b46463070d106279fda35ab3 Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 18:55:04 -0700 Subject: [PATCH 4/8] fix(auth): make the refresh lock's snapshot durable and its holds bounded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the three blockers from re-review of 1527e4e8: 1. Waiter-reuses-winner now actually observes the winner. The under-lock re-read previously used getSettingsWithSecureTokens(), whose expiry is this process's in-memory copy and whose keychain read is skipped inside runtime scopes — so a fresh helper re-rotated unnecessarily and a long-running harness could burn an already-invalidated refresh token. New readPersistedAuthTokens() (src/auth/persisted-tokens.ts) reads the settings file from disk and the keychain directly, bypassing every in-process cache, and reports whether the read was strict. The refresh also now prefers the persisted refresh token over the caller's copy. 2. Persistence verification is no longer cache-backed. The post-flush read-back uses the same strict snapshot and verifies BOTH tokens — a partial keychain write (new access token, old refresh token) previously passed the access-only check and stranded auth on the next refresh. Non-strict read-backs (no keychain / runtime scope) are accepted and documented as unverifiable. 3. Lock holds are bounded so a deadline can no longer orphan the lock for 90s: the refresh fetch aborts at 15s (AbortSignal.timeout), keychain snapshot reads soft-cap at 5s, lock acquisition times out at 20s, and the lock's stale reap is 30s. The helper deadline rises to 60s — above the sum of all internal bounds — making it a true backstop instead of something that fires mid-lock. refreshTokensUnderCrossProcessLock is now exported with injectable deps; tests cover waiter-reuse (zero refreshes), persisted-refresh-token preference, partial-persist detection, non-strict acceptance, lock release on failure, and two contenders yielding exactly one refresh. Co-Authored-By: Claude Opus 5 (1M context) --- src/auth/oauth.ts | 8 + src/auth/persisted-tokens.test.ts | 76 +++++++++ src/auth/persisted-tokens.ts | 129 +++++++++++++++ src/backend/api/client-refresh-lock.test.ts | 172 ++++++++++++++++++++ src/backend/api/client.ts | 110 +++++++++---- src/cli/subcommands/git-credential.ts | 10 +- 6 files changed, 472 insertions(+), 33 deletions(-) create mode 100644 src/auth/persisted-tokens.test.ts create mode 100644 src/auth/persisted-tokens.ts create mode 100644 src/backend/api/client-refresh-lock.test.ts diff --git a/src/auth/oauth.ts b/src/auth/oauth.ts index 8a462a5f4a..51c4e09f65 100644 --- a/src/auth/oauth.ts +++ b/src/auth/oauth.ts @@ -480,6 +480,13 @@ export async function pollForToken( /** * Refresh an access token using a refresh token */ + +// Refresh runs while the cross-process OAuth refresh lock is held (see +// refreshTokensUnderCrossProcessLock); an unbounded fetch would hold that +// lock past its stale-reap window and stall every other letta process's +// auth, so abort rather than hang. +const REFRESH_FETCH_TIMEOUT_MS = 15_000; + export async function refreshAccessToken( refreshToken: string, deviceId: string, @@ -492,6 +499,7 @@ export async function refreshAccessToken( { method: "POST", headers: { "Content-Type": "application/json" }, + signal: AbortSignal.timeout(REFRESH_FETCH_TIMEOUT_MS), body: JSON.stringify({ grant_type: "refresh_token", client_id: OAUTH_CONFIG.clientId, diff --git a/src/auth/persisted-tokens.test.ts b/src/auth/persisted-tokens.test.ts new file mode 100644 index 0000000000..11fea75da2 --- /dev/null +++ b/src/auth/persisted-tokens.test.ts @@ -0,0 +1,76 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { readPersistedAuthTokens } from "@/auth/persisted-tokens"; + +// Force the non-strict (file-only) path: keychain availability differs per +// platform/CI, and these tests must not read the developer's real keychain. +const ORIGINAL_SKIP = process.env.LETTA_SKIP_KEYCHAIN_CHECK; +process.env.LETTA_SKIP_KEYCHAIN_CHECK = "1"; + +afterEach(() => { + if (ORIGINAL_SKIP === undefined) delete process.env.LETTA_SKIP_KEYCHAIN_CHECK; + else process.env.LETTA_SKIP_KEYCHAIN_CHECK = ORIGINAL_SKIP; + process.env.LETTA_SKIP_KEYCHAIN_CHECK = "1"; +}); + +function writeSettingsFile(content: unknown): string { + const dir = mkdtempSync(join(tmpdir(), "persisted-tokens-")); + const path = join(dir, "settings.json"); + writeFileSync(path, JSON.stringify(content), "utf-8"); + return path; +} + +describe("readPersistedAuthTokens (file-backed path)", () => { + test("reads expiry and tokens from the settings file on disk", async () => { + const path = writeSettingsFile({ + tokenExpiresAt: 1_800_000_000_000, + refreshToken: "file-refresh", + env: { LETTA_API_KEY: "file-key" }, + }); + const snapshot = await readPersistedAuthTokens(path); + expect(snapshot).toEqual({ + apiKey: "file-key", + refreshToken: "file-refresh", + tokenExpiresAt: 1_800_000_000_000, + strict: false, + }); + }); + + test("returns nulls for a missing or malformed file", async () => { + const missing = await readPersistedAuthTokens( + join(mkdtempSync(join(tmpdir(), "persisted-tokens-")), "nope.json"), + ); + expect(missing).toEqual({ + apiKey: null, + refreshToken: null, + tokenExpiresAt: null, + strict: false, + }); + + const dir = mkdtempSync(join(tmpdir(), "persisted-tokens-")); + const corrupt = join(dir, "settings.json"); + writeFileSync(corrupt, "{not json", "utf-8"); + expect(await readPersistedAuthTokens(corrupt)).toEqual({ + apiKey: null, + refreshToken: null, + tokenExpiresAt: null, + strict: false, + }); + }); + + test("ignores wrong-typed and empty fields", async () => { + const path = writeSettingsFile({ + tokenExpiresAt: "soon", + refreshToken: "", + env: { LETTA_API_KEY: 42 }, + }); + expect(await readPersistedAuthTokens(path)).toEqual({ + apiKey: null, + refreshToken: null, + tokenExpiresAt: null, + strict: false, + }); + }); +}); diff --git a/src/auth/persisted-tokens.ts b/src/auth/persisted-tokens.ts new file mode 100644 index 0000000000..5d597533c7 --- /dev/null +++ b/src/auth/persisted-tokens.ts @@ -0,0 +1,129 @@ +/** + * Durable auth snapshot for the cross-process OAuth refresh lock. + * + * The refresh lock's waiter-reuses-winner logic must observe what another + * PROCESS persisted, so this reader deliberately bypasses every in-process + * cache: `tokenExpiresAt` comes from the settings file on disk (not + * settingsManager's in-memory copy, which is stale the moment another + * process refreshes), and the tokens come from a direct keychain read (not + * the secure-token cache, which setSecureTokens updates even when the + * underlying write fails). + */ + +import { readFile } from "node:fs/promises"; +import { homedir } from "node:os"; +import { join } from "node:path"; +import { getRuntimeContext } from "@/runtime-context"; +import { + getApiKey, + getRefreshToken, + isKeychainAvailable, +} from "@/utils/secrets"; + +export interface PersistedAuthTokens { + apiKey: string | null; + refreshToken: string | null; + tokenExpiresAt: number | null; + /** + * True when the keychain was actually consulted. False inside runtime + * scopes (Bun 1.3.0 can crash on keychain reads there — same guard as + * settingsManager.getSettingsWithSecureTokens), when the keychain is + * unavailable (file-fallback installs), or when the read timed out. + * Non-strict values come from the settings file and may be incomplete; + * callers must not use them to verify persistence. + */ + strict: boolean; +} + +/** + * Keychain reads run while the refresh lock is held; a wedged keychain must + * not hold the lock past its stale-reap window, so cap the read and degrade + * to the file-backed (non-strict) snapshot. + */ +const KEYCHAIN_READ_TIMEOUT_MS = 5_000; + +function defaultSettingsFilePath(): string { + // Mirrors settingsManager.getSettingsPath(). + const home = process.env.HOME || homedir(); + return join(home, ".letta", "settings.json"); +} + +function withSoftTimeout( + promise: Promise, + ms: number, +): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(null), ms); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + () => { + clearTimeout(timer); + resolve(null); + }, + ); + }); +} + +export async function readPersistedAuthTokens( + settingsFilePath: string = defaultSettingsFilePath(), +): Promise { + let fileApiKey: string | null = null; + let fileRefreshToken: string | null = null; + let fileTokenExpiresAt: number | null = null; + try { + const raw = JSON.parse(await readFile(settingsFilePath, "utf-8")) as { + tokenExpiresAt?: unknown; + refreshToken?: unknown; + env?: { LETTA_API_KEY?: unknown }; + }; + if (typeof raw.tokenExpiresAt === "number") { + fileTokenExpiresAt = raw.tokenExpiresAt; + } + if (typeof raw.refreshToken === "string" && raw.refreshToken) { + fileRefreshToken = raw.refreshToken; + } + if (typeof raw.env?.LETTA_API_KEY === "string" && raw.env.LETTA_API_KEY) { + fileApiKey = raw.env.LETTA_API_KEY; + } + } catch { + // Missing or unreadable settings file — every field stays null. + } + + const fileOnly: PersistedAuthTokens = { + apiKey: fileApiKey, + refreshToken: fileRefreshToken, + tokenExpiresAt: fileTokenExpiresAt, + strict: false, + }; + + if (getRuntimeContext()) { + return fileOnly; + } + + const keychain = await withSoftTimeout( + (async () => { + if (!(await isKeychainAvailable())) return null; + const [apiKey, refreshToken] = await Promise.all([ + getApiKey(), + getRefreshToken(), + ]); + return { apiKey, refreshToken }; + })(), + KEYCHAIN_READ_TIMEOUT_MS, + ); + if (!keychain) { + return fileOnly; + } + + return { + // Keychain owns the tokens when populated; the file values only matter + // for installs that never migrated into the keychain. + apiKey: keychain.apiKey ?? fileApiKey, + refreshToken: keychain.refreshToken ?? fileRefreshToken, + tokenExpiresAt: fileTokenExpiresAt, + strict: true, + }; +} diff --git a/src/backend/api/client-refresh-lock.test.ts b/src/backend/api/client-refresh-lock.test.ts new file mode 100644 index 0000000000..aea72096dc --- /dev/null +++ b/src/backend/api/client-refresh-lock.test.ts @@ -0,0 +1,172 @@ +import { describe, expect, mock, test } from "bun:test"; +import { existsSync, mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { TokenResponse } from "@/auth/oauth"; +import type { PersistedAuthTokens } from "@/auth/persisted-tokens"; +import { refreshTokensUnderCrossProcessLock } from "@/backend/api/client"; + +const FRESH_EXPIRY = Date.now() + 60 * 60 * 1000; +const STALE_EXPIRY = Date.now() - 1_000; + +function snapshot( + overrides: Partial, +): PersistedAuthTokens { + return { + apiKey: null, + refreshToken: null, + tokenExpiresAt: null, + strict: true, + ...overrides, + }; +} + +function makeLockPath(): string { + return join(mkdtempSync(join(tmpdir(), "oauth-lock-")), "refresh.lock"); +} + +const ROTATED: TokenResponse = { + access_token: "new-access", + refresh_token: "new-refresh", + expires_in: 3600, +} as TokenResponse; + +describe("refreshTokensUnderCrossProcessLock", () => { + test("waiter reuses the winner's persisted token without refreshing", async () => { + const lockPath = makeLockPath(); + const refresh = mock(async () => ROTATED); + const persist = mock(async () => {}); + const result = await refreshTokensUnderCrossProcessLock("fallback", { + readTokens: async () => + snapshot({ + apiKey: "winner-access", + refreshToken: "winner-refresh", + tokenExpiresAt: FRESH_EXPIRY, + }), + refresh, + persist, + lockPath, + }); + expect(result).toBe("winner-access"); + expect(refresh).not.toHaveBeenCalled(); + expect(persist).not.toHaveBeenCalled(); + expect(existsSync(lockPath)).toBe(false); + }); + + test("refreshes with the persisted refresh token, not the caller's stale copy", async () => { + const lockPath = makeLockPath(); + const reads = [ + snapshot({ + apiKey: "old-access", + refreshToken: "disk-refresh", + tokenExpiresAt: STALE_EXPIRY, + }), + snapshot({ apiKey: "new-access", refreshToken: "new-refresh" }), + ]; + const refresh = mock(async (refreshToken: string) => { + expect(refreshToken).toBe("disk-refresh"); + return ROTATED; + }); + const persisted: unknown[] = []; + const result = await refreshTokensUnderCrossProcessLock( + "stale-in-memory-refresh", + { + readTokens: async () => reads.shift() ?? snapshot({}), + refresh, + persist: async (updates) => { + persisted.push(updates); + }, + lockPath, + }, + ); + expect(result).toBe("new-access"); + expect(refresh).toHaveBeenCalledTimes(1); + expect(persisted).toEqual([ + { + env: { LETTA_API_KEY: "new-access" }, + refreshToken: "new-refresh", + tokenExpiresAt: expect.any(Number), + }, + ]); + expect(existsSync(lockPath)).toBe(false); + }); + + test("throws when the strict read-back is missing the rotated refresh token", async () => { + const lockPath = makeLockPath(); + const reads = [ + snapshot({ refreshToken: "disk-refresh", tokenExpiresAt: STALE_EXPIRY }), + // Partial persistence: new access token stored, refresh token still old. + snapshot({ apiKey: "new-access", refreshToken: "disk-refresh" }), + ]; + await expect( + refreshTokensUnderCrossProcessLock("fallback", { + readTokens: async () => reads.shift() ?? snapshot({}), + refresh: async () => ROTATED, + persist: async () => {}, + lockPath, + }), + ).rejects.toThrow(/failed to persist/); + expect(existsSync(lockPath)).toBe(false); + }); + + test("accepts a non-strict read-back (no keychain to verify against)", async () => { + const lockPath = makeLockPath(); + const reads = [ + snapshot({ refreshToken: "disk-refresh", tokenExpiresAt: STALE_EXPIRY }), + snapshot({ apiKey: null, refreshToken: null, strict: false }), + ]; + const result = await refreshTokensUnderCrossProcessLock("fallback", { + readTokens: async () => reads.shift() ?? snapshot({}), + refresh: async () => ROTATED, + persist: async () => {}, + lockPath, + }); + expect(result).toBe("new-access"); + }); + + test("releases the lock when the refresh itself fails", async () => { + const lockPath = makeLockPath(); + await expect( + refreshTokensUnderCrossProcessLock("fallback", { + readTokens: async () => snapshot({ tokenExpiresAt: STALE_EXPIRY }), + refresh: async () => { + throw new Error("network down"); + }, + persist: async () => {}, + lockPath, + }), + ).rejects.toThrow("network down"); + expect(existsSync(lockPath)).toBe(false); + }); + + test("serializes two same-process contenders: loser reuses the winner's result", async () => { + const lockPath = makeLockPath(); + // Shared "persisted store": the winner's persist() updates it, so the + // loser's under-lock read sees fresh tokens and skips its own refresh. + let store = snapshot({ + refreshToken: "disk-refresh", + tokenExpiresAt: STALE_EXPIRY, + }); + const refresh = mock(async () => ROTATED); + const deps = { + readTokens: async () => store, + refresh, + persist: async () => { + store = snapshot({ + apiKey: "new-access", + refreshToken: "new-refresh", + tokenExpiresAt: FRESH_EXPIRY, + }); + }, + lockPath, + }; + const [first, second] = await Promise.all([ + refreshTokensUnderCrossProcessLock("fallback", deps), + refreshTokensUnderCrossProcessLock("fallback", deps), + ]); + expect(first).toBe("new-access"); + expect(second).toBe("new-access"); + expect(refresh).toHaveBeenCalledTimes(1); + expect(existsSync(lockPath)).toBe(false); + }); +}); diff --git a/src/backend/api/client.ts b/src/backend/api/client.ts index 2953243590..59604dc311 100644 --- a/src/backend/api/client.ts +++ b/src/backend/api/client.ts @@ -1,8 +1,9 @@ import { homedir, hostname } from "node:os"; import { join } from "node:path"; import Letta from "@letta-ai/letta-client"; -import { LETTA_CLOUD_API_URL } from "@/auth/oauth"; +import { LETTA_CLOUD_API_URL, type TokenResponse } from "@/auth/oauth"; import { refreshAccessTokenSingleFlight } from "@/auth/oauth-refresh"; +import { readPersistedAuthTokens } from "@/auth/persisted-tokens"; import { type Settings, settingsManager } from "@/settings-manager"; import { trackBoundaryError } from "@/telemetry/error-reporting"; import { isDebugEnabled } from "@/utils/debug"; @@ -174,7 +175,36 @@ export function getClientDefaultHeaders(): Record { } const TOKEN_REFRESH_WINDOW_MS = 5 * 60 * 1000; -const OAUTH_REFRESH_LOCK_TIMEOUT_MS = 30_000; +// Every operation under the lock is individually bounded (keychain reads +// soft-cap at 5s in readPersistedAuthTokens, the refresh fetch aborts at +// 15s in refreshAccessToken), so a legitimate holder finishes well inside +// the stale window and an orphaned lock (crashed/killed holder) is reaped +// quickly instead of blocking every git operation for 90s. +const OAUTH_REFRESH_LOCK_TIMEOUT_MS = 20_000; +const OAUTH_REFRESH_LOCK_STALE_MS = 30_000; + +type RefreshLockDeps = { + readTokens?: typeof readPersistedAuthTokens; + refresh?: (refreshToken: string) => Promise; + /** Persist rotated tokens; defaults to settingsManager update + flush. */ + persist?: (updates: Partial) => Promise; + lockPath?: string; +}; + +function defaultRefresh(refreshToken: string): Promise { + return refreshAccessTokenSingleFlight( + refreshToken, + settingsManager.getOrCreateDeviceId(), + hostname(), + ); +} + +async function defaultPersistRefreshedTokens( + updates: Partial, +): Promise { + settingsManager.updateSettings(updates); + await settingsManager.flush(); +} /** * Refresh OAuth tokens under a cross-process file lock. @@ -185,56 +215,72 @@ const OAUTH_REFRESH_LOCK_TIMEOUT_MS = 30_000; * "new"). Two concurrent refreshes therefore both burn the same refresh * token and race their keychain writes; the loser can durably persist an * already-invalidated token and log the user out. The in-process - * single-flight cannot see other processes, so a file lock serializes them: - * after acquiring it, re-read the stored tokens and reuse the winner's - * result instead of refreshing again. + * single-flight cannot see other processes, so a file lock serializes them. + * + * Waiter-reuses-winner: after acquiring the lock, the PERSISTED snapshot is + * read (settings file + direct keychain, bypassing this process's caches — + * see readPersistedAuthTokens). A fresh persisted expiry means another + * process already refreshed: reuse its token instead of burning the rotated + * refresh token again. Exported for tests. */ -async function refreshTokensUnderCrossProcessLock( +export async function refreshTokensUnderCrossProcessLock( fallbackRefreshToken: string, + deps: RefreshLockDeps = {}, ): Promise { - const lockPath = join(homedir(), ".letta", "oauth-refresh.lock"); + const readTokens = deps.readTokens ?? readPersistedAuthTokens; + const refresh = deps.refresh ?? defaultRefresh; + const persist = deps.persist ?? defaultPersistRefreshedTokens; + const lockPath = + deps.lockPath ?? join(homedir(), ".letta", "oauth-refresh.lock"); return await withFileLock( lockPath, async () => { - // Another process may have completed the refresh while we waited. - const latest = await settingsManager.getSettingsWithSecureTokens(); - const latestKey = latest.env?.LETTA_API_KEY; + const before = await readTokens(); if ( - latestKey && - latest.tokenExpiresAt && - latest.tokenExpiresAt - Date.now() >= TOKEN_REFRESH_WINDOW_MS + before.apiKey && + before.tokenExpiresAt && + before.tokenExpiresAt - Date.now() >= TOKEN_REFRESH_WINDOW_MS ) { - return latestKey; + // Another process refreshed while we waited on the lock. + return before.apiKey; } const now = Date.now(); - const tokens = await refreshAccessTokenSingleFlight( - latest.refreshToken ?? fallbackRefreshToken, - settingsManager.getOrCreateDeviceId(), - hostname(), - ); - settingsManager.updateSettings({ + // Prefer the persisted refresh token: with rotation, a long-running + // process's in-memory copy may already be invalidated by a refresh + // another process performed. + const refreshTokenToUse = before.refreshToken ?? fallbackRefreshToken; + const tokens = await refresh(refreshTokenToUse); + const rotatedRefreshToken = tokens.refresh_token || refreshTokenToUse; + await persist({ env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: - tokens.refresh_token || latest.refreshToken || fallbackRefreshToken, + refreshToken: rotatedRefreshToken, tokenExpiresAt: now + tokens.expires_in * 1000, }); // The rotated refresh token must be durably persisted before the lock - // releases — the pre-rotation token is already dead server-side. - // flush() awaits the write but swallows its errors, so read the stored - // key back where possible; an empty read-back means no keychain is - // available (file-fallback storage) and cannot be verified here. - await settingsManager.flush(); - const persisted = await settingsManager.getSettingsWithSecureTokens(); - const persistedKey = persisted.env?.LETTA_API_KEY; - if (persistedKey && persistedKey !== tokens.access_token) { + // releases — the pre-rotation token is already dead server-side, and + // the persistence path swallows write errors (flush() awaits but never + // rejects). Verify with a strict, cache-bypassing read of BOTH tokens: + // a partial keychain write (new access token, old refresh token) would + // pass an access-only check and strand auth on the next refresh. A + // non-strict read-back (no keychain / runtime scope) cannot verify and + // is accepted as-is. + const after = await readTokens(); + if ( + after.strict && + (after.apiKey !== tokens.access_token || + after.refreshToken !== rotatedRefreshToken) + ) { throw new Error( - "OAuth refresh succeeded but the rotated token failed to persist; if this recurs, re-run `letta` to re-authenticate", + "OAuth refresh succeeded but the rotated tokens failed to persist; if this recurs, re-run `letta` to re-authenticate", ); } return tokens.access_token; }, - { timeoutMs: OAUTH_REFRESH_LOCK_TIMEOUT_MS }, + { + timeoutMs: OAUTH_REFRESH_LOCK_TIMEOUT_MS, + staleMs: OAUTH_REFRESH_LOCK_STALE_MS, + }, ); } diff --git a/src/cli/subcommands/git-credential.ts b/src/cli/subcommands/git-credential.ts index 9e12c2493f..345257af60 100644 --- a/src/cli/subcommands/git-credential.ts +++ b/src/cli/subcommands/git-credential.ts @@ -21,7 +21,15 @@ * API client) is imported lazily inside the token resolver. Keep it that way. */ -const RESOLVE_DEADLINE_MS = 10_000; +// Last-resort backstop, deliberately larger than the sum of every internal +// bound (lock acquisition 20s + keychain soft-timeouts 5s + refresh fetch +// abort 15s): in normal operation those bounds guarantee completion long +// before this fires, so expiry means a truly wedged primitive. It matters +// that this stays a backstop — a deadline that fires while getClient() +// holds the refresh lock abandons the operation without cancelling it, and +// the standalone entry's process.exit() would orphan the lock until the +// 30s stale reap. +const RESOLVE_DEADLINE_MS = 60_000; type GitCredentialDeps = { /** Resolve the current harness API token ("" when unauthenticated). */ From 6f3c2c9ab11092098398da90b25e93ffe80fd81a Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 20:21:04 -0700 Subject: [PATCH 5/8] feat(auth): coordinate every rotating-token path through one refresh lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final scoping pass on the credential-helper PR: - refreshTokensCoordinated moves to @/auth/oauth-refresh (the listener layer cannot import backend/api/client) and now backs every path that spends a refresh token: getClient(), the WebSocket listener, the startup refresh in index.ts, and the ChatGPT usage service — the review asked for three; the fourth (usage service) was doing the same uncoordinated rotation and is routed too. One resolution authority, thin delivery adapters above it. - The lock is proper-lockfile instead of the hand-rolled stale-lock algorithm: a live holder keeps the lock fresh via mtime touch, so staleness only ever reaps dead holders. Custom file-lock.ts remains for its other consumer (reflection transcripts). - Durable reads fail closed. readPersistedAuthTokens now distinguishes its sources: "keychain" (authoritative), "file" (keychain genuinely unavailable — the settings file IS durable storage, used and verified), and "runtime-scope" (reads skipped; unverifiable). A keychain that is available but errors or times out mid-read throws KeychainReadError instead of degrading — no rotation on possibly-stale data. - Waiter-reuses-winner gains the rotation signal: a persisted refresh token that differs from the caller's means a peer rotated, so the winner's access token is adopted even when expiry alone is inconclusive. - Multi-process proof (ported from the parked coordination branch): four real bun subprocesses against one shared store — exactly one refresh with the shared lock, >1 in the barrier-controlled unlocked control, and an abandoned lock from a dead holder is reaped. - Real-git integration test: `git credential fill` in a mount configured by cloneRepositoryMount, with a hostile inherited helper and a fake `letta` on PATH — proves the reset entry silences the inherited helper and the dynamic helper's answer wins, on every platform (extensionless sh script; git runs helpers under its bundled sh). Built-artifact latency (bun letta.js, macOS arm64): cold 0.9s, warm ~310ms per credential fill — bundle parse dominates; dev-source fast path is ~50ms. A separately-compiled tiny helper entrypoint can recover the difference if 310ms is deemed too slow. Co-Authored-By: Claude Opus 5 (1M context) --- bun.lock | 12 +- package.json | 4 +- src/agent/memory-git.auth.test.ts | 56 +++- .../oauth-refresh-coordinated.test.ts} | 126 ++++++-- src/auth/oauth-refresh-multi-process.test.ts | 282 ++++++++++++++++++ src/auth/oauth-refresh.ts | 152 ++++++++++ src/auth/persisted-tokens.test.ts | 8 +- src/auth/persisted-tokens.ts | 86 +++--- src/backend/api/client.ts | 125 +------- src/cli/subcommands/listen-auth.test.ts | 32 +- src/index.ts | 25 +- src/providers/chatgpt-usage-service.ts | 32 +- src/websocket/listener/auth.ts | 37 ++- 13 files changed, 735 insertions(+), 242 deletions(-) rename src/{backend/api/client-refresh-lock.test.ts => auth/oauth-refresh-coordinated.test.ts} (54%) create mode 100644 src/auth/oauth-refresh-multi-process.test.ts diff --git a/bun.lock b/bun.lock index 8e55e862e4..0cd67be36a 100644 --- a/bun.lock +++ b/bun.lock @@ -10,12 +10,14 @@ "@modelcontextprotocol/sdk": "1.30.0", "@pierre/diffs": "1.2.2", "@scarf/scarf": "^1.4.0", + "@types/proper-lockfile": "^4.1.4", "cron-parser": "^5.6.1", "cross-spawn": "^7.0.6", "glob": "^13.0.0", "ink-link": "^5.0.0", "node-pty": "^1.1.0", "open": "^10.2.0", + "proper-lockfile": "^4.1.2", "react": "18.2.0", "sharp": "^0.34.5", "shiki": "^4.0.2", @@ -310,6 +312,8 @@ "@types/picomatch": ["@types/picomatch@4.0.2", "", {}, "sha512-qHHxQ+P9PysNEGbALT8f8YOSHW0KJu6l2xU8DYY0fu/EmGxXdVnuTLvFUvBgPJMSqXq29SYHveejeAha+4AYgA=="], + "@types/proper-lockfile": ["@types/proper-lockfile@4.1.4", "", { "dependencies": { "@types/retry": "*" } }, "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ=="], + "@types/qs": ["@types/qs@6.15.0", "", {}, "sha512-JawvT8iBVWpzTrz3EGw9BTQFg3BQNmwERdKE22vlTxawwtbyUSlMppvZYKLZzB5zgACXdXxbD3m1bXaMqP/9ow=="], "@types/range-parser": ["@types/range-parser@1.2.7", "", {}, "sha512-hKormJbkJqzQGhziax5PItDUTMAM9uE2XXQmM37dyd4hVM+5aVl7oVxMVUiVQn2oCQFN/LKCZdvSM0pFRqbSmQ=="], @@ -896,6 +900,8 @@ "process-nextick-args": ["process-nextick-args@2.0.1", "", {}, "sha512-3ouUOpQhtgrbOa17J7+uxOTpITYWaGP7/AhoR3+A+/1e9skrzelGi/dXzEYyvbxubEF6Wn2ypscTKiKJFFn1ag=="], + "proper-lockfile": ["proper-lockfile@4.1.2", "", { "dependencies": { "graceful-fs": "^4.2.4", "retry": "^0.12.0", "signal-exit": "^3.0.2" } }, "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA=="], + "property-information": ["property-information@7.1.0", "", {}, "sha512-TwEZ+X+yCJmYfL7TPUOcvBZ4QfoT5YenQiJuX//0th53DE6w0xxLEtfK3iyryQFddXuvkIk51EEgrJQ0WJkOmQ=="], "protobufjs": ["protobufjs@7.5.8", "", { "dependencies": { "@protobufjs/aspromise": "^1.1.2", "@protobufjs/base64": "^1.1.2", "@protobufjs/codegen": "^2.0.5", "@protobufjs/eventemitter": "^1.1.0", "@protobufjs/fetch": "^1.1.0", "@protobufjs/float": "^1.0.2", "@protobufjs/inquire": "^1.1.1", "@protobufjs/path": "^1.1.2", "@protobufjs/pool": "^1.1.0", "@protobufjs/utf8": "^1.1.1", "@types/node": ">=13.7.0", "long": "^5.0.0" } }, "sha512-dvpCIeLPbXZS/Ete7yLaO7RenOdken2NHKykBXbsaGxZT0UTltcarBciw+A78SRQs9iMAAVpsYA+l8b1hTePIA=="], @@ -940,7 +946,7 @@ "restore-cursor": ["restore-cursor@4.0.0", "", { "dependencies": { "onetime": "^5.1.0", "signal-exit": "^3.0.2" } }, "sha512-I9fPXU9geO9bHOt9pHHOhOkYerIMsmVaWB0rA2AI9ERh/+x/i7MV5HKBNrg+ljO5eoPVgCcnFuRjJ9uH6I/3eg=="], - "retry": ["retry@0.13.1", "", {}, "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg=="], + "retry": ["retry@0.12.0", "", {}, "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow=="], "rfdc": ["rfdc@1.4.1", "", {}, "sha512-q1b3N5QkRUWUl7iyylaaj3kOpIT0N2i9MqIEQXP73GVsN9cw3fdx8X63cEmWhJGi2PPCF23Ijp7ktmd39rawIA=="], @@ -1144,6 +1150,8 @@ "@shikijs/transformers/@shikijs/types": ["@shikijs/types@3.23.0", "", { "dependencies": { "@shikijs/vscode-textmate": "^10.0.2", "@types/hast": "^3.0.4" } }, "sha512-3JZ5HXOZfYjsYSk0yPwBrkupyYSLpAE26Qc0HLghhZNGTZg/SKxXIIgoxOpmmeQP0RRSDJTk1/vPfw9tbw+jSQ=="], + "@slack/web-api/retry": ["retry@0.13.1", "", {}, "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg=="], + "@smithy/credential-provider-imds/@smithy/core": ["@smithy/core@3.24.2", "", { "dependencies": { "@aws-crypto/crc32": "5.2.0", "@smithy/types": "^4.14.1", "tslib": "^2.6.2" } }, "sha512-IKS7qX59fAGCYBmt5JChcDswQDupZqT2Yn2ZBA3UgTlsjRNNkQzZobbn95xoAAdtTyJmBiJB3Y02qR3rgy3Zog=="], "@smithy/credential-provider-imds/@smithy/types": ["@smithy/types@4.14.1", "", { "dependencies": { "tslib": "^2.6.2" } }, "sha512-59b5HtSVrVR/eYNei3BUj3DCPKD/G7EtDDe7OEJE7i7FtQFugYo6MxbotS8mVJkLNVf8gYaAlEBwwtJ9HzhWSg=="], @@ -1208,6 +1216,8 @@ "p-queue/eventemitter3": ["eventemitter3@4.0.7", "", {}, "sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw=="], + "p-retry/retry": ["retry@0.13.1", "", {}, "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg=="], + "precinct/commander": ["commander@12.1.0", "", {}, "sha512-Vw8qHK3bZM9y/P10u3Vib8o/DdkvA2OtPtZvD871QKjy74Wj1WSKFILMPRPSdUSx5RFK1arlJzEtA4PkFgnbuA=="], "sass-lookup/commander": ["commander@12.1.0", "", {}, "sha512-Vw8qHK3bZM9y/P10u3Vib8o/DdkvA2OtPtZvD871QKjy74Wj1WSKFILMPRPSdUSx5RFK1arlJzEtA4PkFgnbuA=="], diff --git a/package.json b/package.json index 0940609a60..eb38fa84cb 100644 --- a/package.json +++ b/package.json @@ -117,12 +117,14 @@ "@modelcontextprotocol/sdk": "1.30.0", "@pierre/diffs": "1.2.2", "@scarf/scarf": "^1.4.0", - "cross-spawn": "^7.0.6", + "@types/proper-lockfile": "^4.1.4", "cron-parser": "^5.6.1", + "cross-spawn": "^7.0.6", "glob": "^13.0.0", "ink-link": "^5.0.0", "node-pty": "^1.1.0", "open": "^10.2.0", + "proper-lockfile": "^4.1.2", "react": "18.2.0", "sharp": "^0.34.5", "shiki": "^4.0.2", diff --git a/src/agent/memory-git.auth.test.ts b/src/agent/memory-git.auth.test.ts index dd4fc9888b..8ac5c528a7 100644 --- a/src/agent/memory-git.auth.test.ts +++ b/src/agent/memory-git.auth.test.ts @@ -8,7 +8,7 @@ import { writeFileSync, } from "node:fs"; import { platform, tmpdir } from "node:os"; -import { join } from "node:path"; +import { delimiter, join } from "node:path"; import { assertMemoryRepoCleanForWrite, buildGitAuthArgs, @@ -798,6 +798,60 @@ describe("cloneRepositoryMount", () => { expect(existsSync(join(directory, "update.md"))).toBe(true); expect(existsSync(legacyHelper)).toBe(false); }); + + test("real git executes the dynamic helper from PATH and ignores inherited helpers", async () => { + // End-to-end through actual `git credential fill` in a mount configured + // by cloneRepositoryMount: proves the two-line config (reset + dynamic + // helper) makes git (a) skip a hostile inherited helper and (b) resolve + // `letta` from PATH and use its answer. The fake `letta` is an + // extensionless sh script — git runs `!` helpers under its bundled sh on + // every platform, including Git-for-Windows. + process.env.LETTA_BASE_URL = "https://api.letta.com"; + delete process.env.LETTA_MEMFS_GIT_PROXY_BASE_URL; + const remote = makeRemoteWithContent(); + const mountParent = mkdtempSync(join(tmpdir(), "repo-mount-")); + tempDirs.push(mountParent); + const directory = join(mountParent, "shared-notes"); + await cloneRepositoryMount({ + agentId: "agent-123", + repositoryName: "shared-notes", + directory, + remoteUrl: remote, + token: "test-token", + }); + + const binDir = join(mountParent, "bin"); + mkdirSync(binDir, { recursive: true }); + writeFileSync( + join(binDir, "letta"), + '#!/bin/sh\n[ "$1" = "git-credential" ] || exit 1\n[ "$2" = "get" ] || exit 0\ncat >/dev/null\necho username=letta\necho password=dynamic-token\n', + { mode: 0o755 }, + ); + + const globalConfig = join(mountParent, "globalconfig"); + writeFileSync( + globalConfig, + `[credential]\n\thelper = "!f() { echo username=stale; echo password=stale-keychain-token; }; f"\n`, + "utf-8", + ); + + const filled = execFileSync("git", ["credential", "fill"], { + cwd: directory, + encoding: "utf-8", + input: "protocol=https\nhost=api.letta.com\n\n", + env: { + ...process.env, + PATH: `${binDir}${delimiter}${process.env.PATH ?? ""}`, + GIT_CONFIG_GLOBAL: globalConfig, + GIT_CONFIG_SYSTEM: platform() === "win32" ? "NUL" : "/dev/null", + GIT_TERMINAL_PROMPT: "0", + }, + }); + + expect(filled).toContain("username=letta"); + expect(filled).toContain("password=dynamic-token"); + expect(filled).not.toContain("stale-keychain-token"); + }); }); describe("credential helper reset behavior", () => { diff --git a/src/backend/api/client-refresh-lock.test.ts b/src/auth/oauth-refresh-coordinated.test.ts similarity index 54% rename from src/backend/api/client-refresh-lock.test.ts rename to src/auth/oauth-refresh-coordinated.test.ts index aea72096dc..1bf2f1ea69 100644 --- a/src/backend/api/client-refresh-lock.test.ts +++ b/src/auth/oauth-refresh-coordinated.test.ts @@ -3,8 +3,11 @@ import { existsSync, mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { TokenResponse } from "@/auth/oauth"; -import type { PersistedAuthTokens } from "@/auth/persisted-tokens"; -import { refreshTokensUnderCrossProcessLock } from "@/backend/api/client"; +import { refreshTokensCoordinated } from "@/auth/oauth-refresh"; +import { + KeychainReadError, + type PersistedAuthTokens, +} from "@/auth/persisted-tokens"; const FRESH_EXPIRY = Date.now() + 60 * 60 * 1000; const STALE_EXPIRY = Date.now() - 1_000; @@ -16,13 +19,18 @@ function snapshot( apiKey: null, refreshToken: null, tokenExpiresAt: null, - strict: true, + source: "keychain", ...overrides, }; } function makeLockPath(): string { - return join(mkdtempSync(join(tmpdir(), "oauth-lock-")), "refresh.lock"); + return join(mkdtempSync(join(tmpdir(), "oauth-lock-")), "oauth-refresh"); +} + +// proper-lockfile locks `.lock`; assert on that. +function lockHeld(lockPath: string): boolean { + return existsSync(`${lockPath}.lock`); } const ROTATED: TokenResponse = { @@ -31,12 +39,12 @@ const ROTATED: TokenResponse = { expires_in: 3600, } as TokenResponse; -describe("refreshTokensUnderCrossProcessLock", () => { - test("waiter reuses the winner's persisted token without refreshing", async () => { +describe("refreshTokensCoordinated", () => { + test("waiter reuses the winner's persisted token via fresh expiry", async () => { const lockPath = makeLockPath(); const refresh = mock(async () => ROTATED); const persist = mock(async () => {}); - const result = await refreshTokensUnderCrossProcessLock("fallback", { + const result = await refreshTokensCoordinated("fallback", { readTokens: async () => snapshot({ apiKey: "winner-access", @@ -50,7 +58,27 @@ describe("refreshTokensUnderCrossProcessLock", () => { expect(result).toBe("winner-access"); expect(refresh).not.toHaveBeenCalled(); expect(persist).not.toHaveBeenCalled(); - expect(existsSync(lockPath)).toBe(false); + expect(lockHeld(lockPath)).toBe(false); + }); + + test("waiter reuses the winner via refresh-token rotation even with stale expiry", async () => { + // The persisted refresh token differs from ours: a peer rotated, ours is + // spent, and using it would revoke theirs — adopt the peer's access token. + const lockPath = makeLockPath(); + const refresh = mock(async () => ROTATED); + const result = await refreshTokensCoordinated("our-spent-refresh", { + readTokens: async () => + snapshot({ + apiKey: "winner-access", + refreshToken: "winner-refresh", + tokenExpiresAt: null, + }), + refresh, + persist: async () => {}, + lockPath, + }); + expect(result).toBe("winner-access"); + expect(refresh).not.toHaveBeenCalled(); }); test("refreshes with the persisted refresh token, not the caller's stale copy", async () => { @@ -68,17 +96,14 @@ describe("refreshTokensUnderCrossProcessLock", () => { return ROTATED; }); const persisted: unknown[] = []; - const result = await refreshTokensUnderCrossProcessLock( - "stale-in-memory-refresh", - { - readTokens: async () => reads.shift() ?? snapshot({}), - refresh, - persist: async (updates) => { - persisted.push(updates); - }, - lockPath, + const result = await refreshTokensCoordinated("disk-refresh", { + readTokens: async () => reads.shift() ?? snapshot({}), + refresh, + persist: async (updates) => { + persisted.push(updates); }, - ); + lockPath, + }); expect(result).toBe("new-access"); expect(refresh).toHaveBeenCalledTimes(1); expect(persisted).toEqual([ @@ -88,10 +113,10 @@ describe("refreshTokensUnderCrossProcessLock", () => { tokenExpiresAt: expect.any(Number), }, ]); - expect(existsSync(lockPath)).toBe(false); + expect(lockHeld(lockPath)).toBe(false); }); - test("throws when the strict read-back is missing the rotated refresh token", async () => { + test("throws when the read-back is missing the rotated refresh token", async () => { const lockPath = makeLockPath(); const reads = [ snapshot({ refreshToken: "disk-refresh", tokenExpiresAt: STALE_EXPIRY }), @@ -99,23 +124,45 @@ describe("refreshTokensUnderCrossProcessLock", () => { snapshot({ apiKey: "new-access", refreshToken: "disk-refresh" }), ]; await expect( - refreshTokensUnderCrossProcessLock("fallback", { + refreshTokensCoordinated("disk-refresh", { readTokens: async () => reads.shift() ?? snapshot({}), refresh: async () => ROTATED, persist: async () => {}, lockPath, }), ).rejects.toThrow(/failed to persist/); - expect(existsSync(lockPath)).toBe(false); + expect(lockHeld(lockPath)).toBe(false); }); - test("accepts a non-strict read-back (no keychain to verify against)", async () => { + test("verifies the file fallback when the keychain is genuinely unavailable", async () => { + // source: "file" is authoritative durable storage and MUST be verified — + // only runtime-scope read-backs are unverifiable. + const lockPath = makeLockPath(); + const reads = [ + snapshot({ + refreshToken: "disk-refresh", + tokenExpiresAt: STALE_EXPIRY, + source: "file", + }), + snapshot({ apiKey: "new-access", refreshToken: "stale", source: "file" }), + ]; + await expect( + refreshTokensCoordinated("disk-refresh", { + readTokens: async () => reads.shift() ?? snapshot({}), + refresh: async () => ROTATED, + persist: async () => {}, + lockPath, + }), + ).rejects.toThrow(/failed to persist/); + }); + + test("accepts a runtime-scope read-back (keychain reads skipped)", async () => { const lockPath = makeLockPath(); const reads = [ snapshot({ refreshToken: "disk-refresh", tokenExpiresAt: STALE_EXPIRY }), - snapshot({ apiKey: null, refreshToken: null, strict: false }), + snapshot({ apiKey: null, refreshToken: null, source: "runtime-scope" }), ]; - const result = await refreshTokensUnderCrossProcessLock("fallback", { + const result = await refreshTokensCoordinated("disk-refresh", { readTokens: async () => reads.shift() ?? snapshot({}), refresh: async () => ROTATED, persist: async () => {}, @@ -124,10 +171,27 @@ describe("refreshTokensUnderCrossProcessLock", () => { expect(result).toBe("new-access"); }); + test("fails closed on a keychain read error instead of rotating blind", async () => { + const lockPath = makeLockPath(); + const refresh = mock(async () => ROTATED); + await expect( + refreshTokensCoordinated("fallback", { + readTokens: async () => { + throw new KeychainReadError("keychain read timed out (5000ms)"); + }, + refresh, + persist: async () => {}, + lockPath, + }), + ).rejects.toThrow(KeychainReadError); + expect(refresh).not.toHaveBeenCalled(); + expect(lockHeld(lockPath)).toBe(false); + }); + test("releases the lock when the refresh itself fails", async () => { const lockPath = makeLockPath(); await expect( - refreshTokensUnderCrossProcessLock("fallback", { + refreshTokensCoordinated("fallback", { readTokens: async () => snapshot({ tokenExpiresAt: STALE_EXPIRY }), refresh: async () => { throw new Error("network down"); @@ -136,7 +200,7 @@ describe("refreshTokensUnderCrossProcessLock", () => { lockPath, }), ).rejects.toThrow("network down"); - expect(existsSync(lockPath)).toBe(false); + expect(lockHeld(lockPath)).toBe(false); }); test("serializes two same-process contenders: loser reuses the winner's result", async () => { @@ -144,7 +208,7 @@ describe("refreshTokensUnderCrossProcessLock", () => { // Shared "persisted store": the winner's persist() updates it, so the // loser's under-lock read sees fresh tokens and skips its own refresh. let store = snapshot({ - refreshToken: "disk-refresh", + refreshToken: "shared-refresh", tokenExpiresAt: STALE_EXPIRY, }); const refresh = mock(async () => ROTATED); @@ -161,12 +225,12 @@ describe("refreshTokensUnderCrossProcessLock", () => { lockPath, }; const [first, second] = await Promise.all([ - refreshTokensUnderCrossProcessLock("fallback", deps), - refreshTokensUnderCrossProcessLock("fallback", deps), + refreshTokensCoordinated("shared-refresh", deps), + refreshTokensCoordinated("shared-refresh", deps), ]); expect(first).toBe("new-access"); expect(second).toBe("new-access"); expect(refresh).toHaveBeenCalledTimes(1); - expect(existsSync(lockPath)).toBe(false); + expect(lockHeld(lockPath)).toBe(false); }); }); diff --git a/src/auth/oauth-refresh-multi-process.test.ts b/src/auth/oauth-refresh-multi-process.test.ts new file mode 100644 index 0000000000..6945f9f845 --- /dev/null +++ b/src/auth/oauth-refresh-multi-process.test.ts @@ -0,0 +1,282 @@ +/** + * Cross-process refresh coordination, exercised with real subprocesses. + * + * The other refresh tests all run in one process, so they only ever prove the + * in-process layers work. This spawns N `bun` workers that each call + * `refreshTokensCoordinated` against one shared credential file and one shared + * lockfile, and counts how many actually reached the refresh call. + * + * Oracle: refresh-count.txt holds one byte per refresh. With the shared lock + * it must contain exactly 1. + * + * A barrier is what keeps this honest. Without one, worker A can finish the + * whole refresh before worker B starts and the test would pass with no lock at + * all. Two barrier placements are needed because reads happen inside the + * critical section: + * + * • start barrier — releases all workers together just before they contend, + * used for the locked case. A barrier inside the read would deadlock there, + * with the lock holder waiting on workers blocked on the lock. + * • load barrier — holds every worker until all have read, used only by the + * unlocked control to stop the losers from adopting the winner's rotation. + */ + +import { afterEach, describe, expect, test } from "bun:test"; +import { + mkdirSync, + mkdtempSync, + rmSync, + statSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const REFRESH_MODULE = join(import.meta.dir, "oauth-refresh.ts"); +const REPO_ROOT = join(import.meta.dir, "..", ".."); + +const tempDirs: string[] = []; + +function makeShareDir(): string { + const dir = mkdtempSync(join(tmpdir(), "letta-refresh-multiproc-")); + tempDirs.push(dir); + return dir; +} + +/** + * Worker body. Stores the credential as one JSON file so the whole record + * rotates atomically. readTokens maps it onto the durable-snapshot shape the + * coordinator consumes (source "file" = authoritative and verifiable). + */ +const WORKER_SCRIPT = ` +const { refreshTokensCoordinated } = await import(process.env.REFRESH_MODULE); +const { appendFileSync, readFileSync, writeFileSync, statSync } = await import("node:fs"); +const { join } = await import("node:path"); + +const shareDir = process.env.SHARE_DIR; +const credentialPath = join(shareDir, "credential.json"); +const counterPath = join(shareDir, "refresh-count.txt"); +const readyPath = join(shareDir, "first-load-ready.txt"); +const workerCount = Number(process.env.WORKER_COUNT); + +/** + * Block until every worker has reached this point. + * + * This must sit before the refresh call, not inside readTokens: the + * coordinated lifecycle reads storage only while holding the lock, so a + * barrier in the read would have the lock holder waiting on workers that are + * themselves blocked on the lock. + */ +function waitForBarrier(label) { + appendFileSync(readyPath, "."); + const deadline = Date.now() + 10000; + for (;;) { + let ready = 0; + try { ready = statSync(readyPath).size; } catch {} + if (ready >= workerCount) return; + if (Date.now() >= deadline) throw new Error(label + " barrier timed out"); + Bun.sleepSync(5); + } +} + +let firstLoad = true; +async function readTokens() { + let record = null; + try { + record = JSON.parse(readFileSync(credentialPath, "utf8")); + } catch { + record = null; + } + // Control mode only: hold every worker until all have read, so none can see + // a peer's rotation. Deadlocks under a shared lock, where reads happen + // inside the critical section — hence the separate start barrier there. + if (process.env.LOAD_BARRIER === "1" && firstLoad) { + firstLoad = false; + waitForBarrier("load"); + } + return { + apiKey: record?.accessToken ?? null, + refreshToken: record?.refreshToken ?? null, + tokenExpiresAt: record?.expiresAt ?? null, + source: "file", + }; +} + +async function persist(updates) { + writeFileSync(credentialPath, JSON.stringify({ + accessToken: updates.env.LETTA_API_KEY, + refreshToken: updates.refreshToken, + expiresAt: updates.tokenExpiresAt, + }), "utf8"); +} + +// One byte per refresh; O_APPEND is atomic on POSIX. +async function refresh() { + appendFileSync(counterPath, "."); + const stamp = String(Date.now()) + "-" + String(process.pid); + return { + access_token: "at-" + stamp, + refresh_token: "rt-rotated-" + stamp, + token_type: "Bearer", + expires_in: 3600, + }; +} + +try { + if (process.env.BARRIER === "1") waitForBarrier("start"); + const accessToken = await refreshTokensCoordinated("rt-initial", { + lockPath: process.env.LOCK_PATH, + readTokens, + persist, + refresh, + }); + process.stdout.write("ok:" + accessToken + "\\n"); +} catch (error) { + process.stdout.write("err:" + (error?.message ?? String(error)) + "\\n"); +} +`; + +/** Seed a credential that is inside the refresh window, so a refresh is due. */ +function seedStaleCredential(shareDir: string): void { + writeFileSync( + join(shareDir, "credential.json"), + JSON.stringify({ + accessToken: "at-initial", + refreshToken: "rt-initial", + expiresAt: Date.now() + 60_000, + }), + "utf8", + ); +} + +function refreshCount(shareDir: string): number { + try { + return statSync(join(shareDir, "refresh-count.txt")).size; + } catch { + return 0; + } +} + +interface WorkerOutcome { + exitCode: number; + stdout: string; + stderr: string; +} + +async function spawnWorkers(options: { + shareDir: string; + count: number; + /** Per-worker lock path. A shared path serializes; distinct paths do not. */ + lockPathFor: (id: number) => string; + /** Release all workers together just before they contend for the lock. */ + barrier: boolean; + /** Control only: hold all workers until each has read the credential. */ + loadBarrier?: boolean; +}): Promise { + const scriptPath = join(options.shareDir, "worker.mjs"); + writeFileSync(scriptPath, WORKER_SCRIPT, "utf8"); + + const running = Array.from({ length: options.count }, (_unused, id) => + Bun.spawn(["bun", scriptPath], { + cwd: REPO_ROOT, + env: { + ...process.env, + REFRESH_MODULE: REFRESH_MODULE, + SHARE_DIR: options.shareDir, + LOCK_PATH: options.lockPathFor(id), + WORKER_COUNT: String(options.count), + BARRIER: options.barrier ? "1" : "0", + LOAD_BARRIER: options.loadBarrier ? "1" : "0", + }, + stdout: "pipe", + stderr: "pipe", + }), + ); + + return await Promise.all( + running.map(async (child) => { + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { exitCode, stdout, stderr }; + }), + ); +} + +afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop() as string, { recursive: true, force: true }); + } +}); + +describe("refreshTokensCoordinated across processes", () => { + test("four concurrent processes produce exactly one refresh", async () => { + const shareDir = makeShareDir(); + seedStaleCredential(shareDir); + const lockPath = join(shareDir, "oauth-refresh"); + + const workers = await spawnWorkers({ + shareDir, + count: 4, + lockPathFor: () => lockPath, + barrier: true, + }); + + for (const worker of workers) { + expect(worker.stdout.startsWith("ok:"), worker.stderr).toBe(true); + expect(worker.exitCode).toBe(0); + } + expect(refreshCount(shareDir)).toBe(1); + }, 60_000); + + test("interleaved reads without a shared lock refresh once per process", async () => { + // Control, so the assertion above cannot pass vacuously. + // + // Unshared locks alone are not enough to show the difference: the winner + // persists its rotation fast enough that the losers read it and adopt it, + // so storage coalesces them even with no lock. Holding every worker until + // all four have read is what actually reproduces the unsynchronized case, + // and then each one spends the same refresh_token. + const shareDir = makeShareDir(); + seedStaleCredential(shareDir); + + const workers = await spawnWorkers({ + shareDir, + count: 4, + lockPathFor: (id) => join(shareDir, `unshared-${String(id)}`), + barrier: false, + loadBarrier: true, + }); + + for (const worker of workers) { + expect(worker.stdout.startsWith("ok:"), worker.stderr).toBe(true); + } + expect(refreshCount(shareDir)).toBeGreaterThan(1); + }, 60_000); + + test("a lock abandoned by a dead process does not wedge the next one", async () => { + const shareDir = makeShareDir(); + seedStaleCredential(shareDir); + const lockPath = join(shareDir, "oauth-refresh"); + // proper-lockfile locks are directories reaped by mtime staleness. A live + // holder touches the mtime continuously; backdating it simulates a holder + // that died without releasing. + const abandonedLockDir = `${lockPath}.lock`; + mkdirSync(abandonedLockDir); + const past = new Date(Date.now() - 60_000); + utimesSync(abandonedLockDir, past, past); + + const [worker] = await spawnWorkers({ + shareDir, + count: 1, + lockPathFor: () => lockPath, + barrier: false, + }); + + expect(worker?.stdout.startsWith("ok:"), worker?.stderr).toBe(true); + expect(refreshCount(shareDir)).toBe(1); + }, 60_000); +}); diff --git a/src/auth/oauth-refresh.ts b/src/auth/oauth-refresh.ts index b0cdc1d3a4..17660d7e57 100644 --- a/src/auth/oauth-refresh.ts +++ b/src/auth/oauth-refresh.ts @@ -1,9 +1,21 @@ +import { homedir } from "node:os"; +import { join } from "node:path"; +import lockfile from "proper-lockfile"; import { refreshAccessToken, type TokenResponse } from "@/auth/oauth"; +import { readPersistedAuthTokens } from "@/auth/persisted-tokens"; type RefreshAccessToken = typeof refreshAccessToken; const inFlightRefreshes = new Map>(); +/** + * Refresh a credential no more than once at a time within this process. + * + * This is only the first of two layers: it cannot see peer letta processes, + * so on its own it still lets two invocations rotate the same refresh_token + * concurrently. Any path that PERSISTS the result must go through + * {@link refreshTokensCoordinated} instead of calling this directly. + */ export async function refreshAccessTokenSingleFlight( refreshToken: string, deviceId: string, @@ -26,3 +38,143 @@ export async function refreshAccessTokenSingleFlight( } } } + +/** Treat a token with less remaining life than this as needing rotation. */ +export const TOKEN_REFRESH_WINDOW_MS = 5 * 60 * 1000; + +/** + * Lock acquisition gives up after ~20s of retrying. proper-lockfile keeps a + * live holder's lock fresh by touching its mtime, so `stale` only reaps + * locks whose holder actually died (crash, SIGKILL) — no hand-rolled + * staleness math. + */ +const LOCK_RETRIES = { + retries: 40, + factor: 1, + minTimeout: 250, + maxTimeout: 500, +} as const; +const LOCK_STALE_MS = 15_000; + +type SettingsUpdates = { + env: { LETTA_API_KEY: string }; + refreshToken: string; + tokenExpiresAt: number; +}; + +export type CoordinatedRefreshDeps = { + readTokens?: typeof readPersistedAuthTokens; + refresh?: (refreshToken: string) => Promise; + /** Persist rotated tokens; defaults to settingsManager update + flush. */ + persist?: (updates: SettingsUpdates) => Promise; + lockPath?: string; +}; + +async function defaultRefresh(refreshToken: string): Promise { + const { hostname } = await import("node:os"); + const { settingsManager } = await import("@/settings-manager"); + return refreshAccessTokenSingleFlight( + refreshToken, + settingsManager.getOrCreateDeviceId(), + hostname(), + ); +} + +async function defaultPersist(updates: SettingsUpdates): Promise { + const { settingsManager } = await import("@/settings-manager"); + settingsManager.updateSettings(updates); + await settingsManager.flush(); +} + +function defaultLockPath(): string { + return join(homedir(), ".letta", "oauth-refresh"); +} + +/** + * Refresh OAuth tokens under a cross-process file lock. Every rotating-token + * call site funnels through here — getClient(), the WebSocket listener, the + * startup refresh in index.ts, the ChatGPT usage service, and each + * `letta git-credential` helper invocation spawned by git. + * + * The server rotates the refresh token on every refresh + * (refresh_token_mode: "new"), so two uncoordinated refreshes both spend the + * same refresh token and race their keychain writes; the loser can durably + * persist an already-invalidated token and log the user out. The in-process + * single-flight cannot see other processes; proper-lockfile serializes them. + * + * Waiter-reuses-winner: after acquiring the lock, the PERSISTED snapshot is + * read (settings file + direct keychain, bypassing this process's caches — + * see readPersistedAuthTokens). Two independent signals mean another process + * already rotated: a fresh persisted expiry, or a persisted refresh token + * that differs from the caller's (ours is spent — using it would revoke + * theirs). Either way the winner's access token is adopted instead of + * refreshing again. + * + * Fails closed: a keychain that errors or times out mid-read throws + * (KeychainReadError) rather than rotating based on possibly-stale data. + */ +export async function refreshTokensCoordinated( + fallbackRefreshToken: string, + deps: CoordinatedRefreshDeps = {}, +): Promise { + const readTokens = deps.readTokens ?? readPersistedAuthTokens; + const refresh = deps.refresh ?? defaultRefresh; + const persist = deps.persist ?? defaultPersist; + const lockPath = deps.lockPath ?? defaultLockPath(); + + const release = await lockfile.lock(lockPath, { + realpath: false, + stale: LOCK_STALE_MS, + retries: LOCK_RETRIES, + }); + try { + const before = await readTokens(); + const peerRotated = + before.apiKey && + ((before.tokenExpiresAt !== null && + before.tokenExpiresAt - Date.now() >= TOKEN_REFRESH_WINDOW_MS) || + (before.refreshToken !== null && + fallbackRefreshToken !== "" && + before.refreshToken !== fallbackRefreshToken)); + if (peerRotated && before.apiKey) { + return before.apiKey; + } + + const now = Date.now(); + // Prefer the persisted refresh token: with rotation, a long-running + // process's in-memory copy may already be invalidated by a refresh + // another process performed. + const refreshTokenToUse = before.refreshToken ?? fallbackRefreshToken; + if (!refreshTokenToUse) { + throw new Error("no refresh token available"); + } + const tokens = await refresh(refreshTokenToUse); + const rotatedRefreshToken = tokens.refresh_token || refreshTokenToUse; + await persist({ + env: { LETTA_API_KEY: tokens.access_token }, + refreshToken: rotatedRefreshToken, + tokenExpiresAt: now + tokens.expires_in * 1000, + }); + // The rotated refresh token must be durably persisted before the lock + // releases — the pre-rotation token is already dead server-side, and + // the persistence path swallows write errors (flush() awaits but never + // rejects). Verify with the same cache-bypassing snapshot, checking + // BOTH tokens: a partial keychain write (new access token, old refresh + // token) would pass an access-only check and strand auth on the next + // refresh. Only a runtime-scope read-back (keychain reads skipped) + // cannot verify and is accepted as-is. + const after = await readTokens(); + if ( + after.source !== "runtime-scope" && + (after.apiKey !== tokens.access_token || + after.refreshToken !== rotatedRefreshToken) + ) { + throw new Error( + "OAuth refresh succeeded but the rotated tokens failed to persist; if this recurs, re-run `letta` to re-authenticate", + ); + } + return tokens.access_token; + } finally { + await release(); + } +} diff --git a/src/auth/persisted-tokens.test.ts b/src/auth/persisted-tokens.test.ts index 11fea75da2..508960ab2f 100644 --- a/src/auth/persisted-tokens.test.ts +++ b/src/auth/persisted-tokens.test.ts @@ -34,7 +34,7 @@ describe("readPersistedAuthTokens (file-backed path)", () => { apiKey: "file-key", refreshToken: "file-refresh", tokenExpiresAt: 1_800_000_000_000, - strict: false, + source: "file", }); }); @@ -46,7 +46,7 @@ describe("readPersistedAuthTokens (file-backed path)", () => { apiKey: null, refreshToken: null, tokenExpiresAt: null, - strict: false, + source: "file", }); const dir = mkdtempSync(join(tmpdir(), "persisted-tokens-")); @@ -56,7 +56,7 @@ describe("readPersistedAuthTokens (file-backed path)", () => { apiKey: null, refreshToken: null, tokenExpiresAt: null, - strict: false, + source: "file", }); }); @@ -70,7 +70,7 @@ describe("readPersistedAuthTokens (file-backed path)", () => { apiKey: null, refreshToken: null, tokenExpiresAt: null, - strict: false, + source: "file", }); }); }); diff --git a/src/auth/persisted-tokens.ts b/src/auth/persisted-tokens.ts index 5d597533c7..6489ef0f4a 100644 --- a/src/auth/persisted-tokens.ts +++ b/src/auth/persisted-tokens.ts @@ -25,20 +25,33 @@ export interface PersistedAuthTokens { refreshToken: string | null; tokenExpiresAt: number | null; /** - * True when the keychain was actually consulted. False inside runtime - * scopes (Bun 1.3.0 can crash on keychain reads there — same guard as - * settingsManager.getSettingsWithSecureTokens), when the keychain is - * unavailable (file-fallback installs), or when the read timed out. - * Non-strict values come from the settings file and may be incomplete; - * callers must not use them to verify persistence. + * Where the token values came from: + * - "keychain" — direct keychain read succeeded; values are authoritative + * and a post-persist read-back can verify against them. + * - "file" — the keychain is genuinely unavailable on this install, so the + * settings file IS durable token storage (persistSettingsAndTokens falls + * back to it); values are authoritative and verifiable. + * - "runtime-scope" — keychain reads are skipped inside runtime scopes + * (Bun 1.3.0 can crash there — same guard as + * settingsManager.getSettingsWithSecureTokens), so on keychain installs + * the file carries only the expiry. Values may be incomplete; callers + * must not use them to verify persistence. + * + * A keychain that is available but errors or times out mid-read does NOT + * degrade to "file" — readPersistedAuthTokens throws KeychainReadError + * instead, so callers fail closed rather than rotate a refresh token based + * on possibly-stale data. */ - strict: boolean; + source: "keychain" | "file" | "runtime-scope"; } +/** A confirmed-available keychain failed or timed out mid-read. */ +export class KeychainReadError extends Error {} + /** * Keychain reads run while the refresh lock is held; a wedged keychain must - * not hold the lock past its stale-reap window, so cap the read and degrade - * to the file-backed (non-strict) snapshot. + * not hold the lock indefinitely, so cap the read. Expiry fails closed (see + * KeychainReadError) — it never silently degrades to the file snapshot. */ const KEYCHAIN_READ_TIMEOUT_MS = 5_000; @@ -48,20 +61,24 @@ function defaultSettingsFilePath(): string { return join(home, ".letta", "settings.json"); } -function withSoftTimeout( - promise: Promise, - ms: number, -): Promise { - return new Promise((resolve) => { - const timer = setTimeout(() => resolve(null), ms); +function withReadDeadline(promise: Promise, ms: number): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new KeychainReadError(`keychain read timed out (${ms}ms)`)), + ms, + ); promise.then( (value) => { clearTimeout(timer); resolve(value); }, - () => { + (error) => { clearTimeout(timer); - resolve(null); + reject( + new KeychainReadError( + `keychain read failed: ${error instanceof Error ? error.message : String(error)}`, + ), + ); }, ); }); @@ -92,38 +109,39 @@ export async function readPersistedAuthTokens( // Missing or unreadable settings file — every field stays null. } - const fileOnly: PersistedAuthTokens = { + const fileValues = { apiKey: fileApiKey, refreshToken: fileRefreshToken, tokenExpiresAt: fileTokenExpiresAt, - strict: false, }; if (getRuntimeContext()) { - return fileOnly; + return { ...fileValues, source: "runtime-scope" }; } - const keychain = await withSoftTimeout( - (async () => { - if (!(await isKeychainAvailable())) return null; - const [apiKey, refreshToken] = await Promise.all([ - getApiKey(), - getRefreshToken(), - ]); - return { apiKey, refreshToken }; - })(), + // isKeychainAvailable() reports genuine unavailability (no Bun secrets, + // headless Linux without a session bus, LETTA_SKIP_KEYCHAIN_CHECK) as + // false — that is the file-fallback install, where the settings file is + // the durable store. Errors past that point are a different animal. + const available = await withReadDeadline( + isKeychainAvailable(), KEYCHAIN_READ_TIMEOUT_MS, ); - if (!keychain) { - return fileOnly; + if (!available) { + return { ...fileValues, source: "file" }; } + const [apiKey, refreshToken] = await withReadDeadline( + Promise.all([getApiKey(), getRefreshToken()]), + KEYCHAIN_READ_TIMEOUT_MS, + ); + return { // Keychain owns the tokens when populated; the file values only matter // for installs that never migrated into the keychain. - apiKey: keychain.apiKey ?? fileApiKey, - refreshToken: keychain.refreshToken ?? fileRefreshToken, + apiKey: apiKey ?? fileApiKey, + refreshToken: refreshToken ?? fileRefreshToken, tokenExpiresAt: fileTokenExpiresAt, - strict: true, + source: "keychain", }; } diff --git a/src/backend/api/client.ts b/src/backend/api/client.ts index 59604dc311..2c98b8d951 100644 --- a/src/backend/api/client.ts +++ b/src/backend/api/client.ts @@ -1,13 +1,12 @@ -import { homedir, hostname } from "node:os"; -import { join } from "node:path"; import Letta from "@letta-ai/letta-client"; -import { LETTA_CLOUD_API_URL, type TokenResponse } from "@/auth/oauth"; -import { refreshAccessTokenSingleFlight } from "@/auth/oauth-refresh"; -import { readPersistedAuthTokens } from "@/auth/persisted-tokens"; +import { LETTA_CLOUD_API_URL } from "@/auth/oauth"; +import { + refreshTokensCoordinated, + TOKEN_REFRESH_WINDOW_MS, +} from "@/auth/oauth-refresh"; import { type Settings, settingsManager } from "@/settings-manager"; import { trackBoundaryError } from "@/telemetry/error-reporting"; import { isDebugEnabled } from "@/utils/debug"; -import { withFileLock } from "@/utils/file-lock"; import { createTimingFetch, isTimingsEnabled } from "@/utils/timing"; import packageJson from "../../../package.json"; @@ -174,116 +173,6 @@ export function getClientDefaultHeaders(): Record { }; } -const TOKEN_REFRESH_WINDOW_MS = 5 * 60 * 1000; -// Every operation under the lock is individually bounded (keychain reads -// soft-cap at 5s in readPersistedAuthTokens, the refresh fetch aborts at -// 15s in refreshAccessToken), so a legitimate holder finishes well inside -// the stale window and an orphaned lock (crashed/killed holder) is reaped -// quickly instead of blocking every git operation for 90s. -const OAUTH_REFRESH_LOCK_TIMEOUT_MS = 20_000; -const OAUTH_REFRESH_LOCK_STALE_MS = 30_000; - -type RefreshLockDeps = { - readTokens?: typeof readPersistedAuthTokens; - refresh?: (refreshToken: string) => Promise; - /** Persist rotated tokens; defaults to settingsManager update + flush. */ - persist?: (updates: Partial) => Promise; - lockPath?: string; -}; - -function defaultRefresh(refreshToken: string): Promise { - return refreshAccessTokenSingleFlight( - refreshToken, - settingsManager.getOrCreateDeviceId(), - hostname(), - ); -} - -async function defaultPersistRefreshedTokens( - updates: Partial, -): Promise { - settingsManager.updateSettings(updates); - await settingsManager.flush(); -} - -/** - * Refresh OAuth tokens under a cross-process file lock. - * - * Every letta process refreshes through this path — CLI sessions, listeners, - * and each `letta git-credential` helper invocation spawned by git — and the - * server rotates the refresh token on every refresh (refresh_token_mode: - * "new"). Two concurrent refreshes therefore both burn the same refresh - * token and race their keychain writes; the loser can durably persist an - * already-invalidated token and log the user out. The in-process - * single-flight cannot see other processes, so a file lock serializes them. - * - * Waiter-reuses-winner: after acquiring the lock, the PERSISTED snapshot is - * read (settings file + direct keychain, bypassing this process's caches — - * see readPersistedAuthTokens). A fresh persisted expiry means another - * process already refreshed: reuse its token instead of burning the rotated - * refresh token again. Exported for tests. - */ -export async function refreshTokensUnderCrossProcessLock( - fallbackRefreshToken: string, - deps: RefreshLockDeps = {}, -): Promise { - const readTokens = deps.readTokens ?? readPersistedAuthTokens; - const refresh = deps.refresh ?? defaultRefresh; - const persist = deps.persist ?? defaultPersistRefreshedTokens; - const lockPath = - deps.lockPath ?? join(homedir(), ".letta", "oauth-refresh.lock"); - return await withFileLock( - lockPath, - async () => { - const before = await readTokens(); - if ( - before.apiKey && - before.tokenExpiresAt && - before.tokenExpiresAt - Date.now() >= TOKEN_REFRESH_WINDOW_MS - ) { - // Another process refreshed while we waited on the lock. - return before.apiKey; - } - - const now = Date.now(); - // Prefer the persisted refresh token: with rotation, a long-running - // process's in-memory copy may already be invalidated by a refresh - // another process performed. - const refreshTokenToUse = before.refreshToken ?? fallbackRefreshToken; - const tokens = await refresh(refreshTokenToUse); - const rotatedRefreshToken = tokens.refresh_token || refreshTokenToUse; - await persist({ - env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: rotatedRefreshToken, - tokenExpiresAt: now + tokens.expires_in * 1000, - }); - // The rotated refresh token must be durably persisted before the lock - // releases — the pre-rotation token is already dead server-side, and - // the persistence path swallows write errors (flush() awaits but never - // rejects). Verify with a strict, cache-bypassing read of BOTH tokens: - // a partial keychain write (new access token, old refresh token) would - // pass an access-only check and strand auth on the next refresh. A - // non-strict read-back (no keychain / runtime scope) cannot verify and - // is accepted as-is. - const after = await readTokens(); - if ( - after.strict && - (after.apiKey !== tokens.access_token || - after.refreshToken !== rotatedRefreshToken) - ) { - throw new Error( - "OAuth refresh succeeded but the rotated tokens failed to persist; if this recurs, re-run `letta` to re-authenticate", - ); - } - return tokens.access_token; - }, - { - timeoutMs: OAUTH_REFRESH_LOCK_TIMEOUT_MS, - staleMs: OAUTH_REFRESH_LOCK_STALE_MS, - }, - ); -} - export async function getClient() { if (_testClientOverride) { return (await _testClientOverride()) as Letta; @@ -334,9 +223,7 @@ export async function getClient() { // the delete-then-set window of a concurrent refresh). if (!apiKey || expiresAt - now < TOKEN_REFRESH_WINDOW_MS) { try { - apiKey = await refreshTokensUnderCrossProcessLock( - settings.refreshToken, - ); + apiKey = await refreshTokensCoordinated(settings.refreshToken); _cachedApiKey = apiKey; } catch (error) { trackBoundaryError({ diff --git a/src/cli/subcommands/listen-auth.test.ts b/src/cli/subcommands/listen-auth.test.ts index a0bcc3fafc..20a8e7620a 100644 --- a/src/cli/subcommands/listen-auth.test.ts +++ b/src/cli/subcommands/listen-auth.test.ts @@ -16,6 +16,30 @@ const requestDeviceCodeMock = mock(async (): Promise => { const pollForTokenMock = mock(async (): Promise => { throw new Error("pollForToken not mocked"); }); +// Durable-snapshot fake for the coordinated refresh. Before a refresh it +// reports a stale stored refresh token; once the (stubbed) +// settingsManager.updateSettings has run it reflects that write, so the +// coordinator's persistence read-back verifies against this fake store +// instead of the developer's real settings file and keychain. +let persistedUpdates: { + env: { LETTA_API_KEY: string }; + refreshToken: string; + tokenExpiresAt: number; +} | null = null; +const readPersistedTokensFake = async () => + persistedUpdates + ? { + apiKey: persistedUpdates.env.LETTA_API_KEY, + refreshToken: persistedUpdates.refreshToken, + tokenExpiresAt: persistedUpdates.tokenExpiresAt, + source: "file" as const, + } + : { + apiKey: null, + refreshToken: "refresh-token", + tokenExpiresAt: Date.now() - 1000, + source: "file" as const, + }; const { __listenSubcommandTestUtils } = await import( "@/cli/subcommands/listen" @@ -37,11 +61,13 @@ describe("listen subcommand auth resolution", () => { refreshAccessTokenMock.mockReset(); requestDeviceCodeMock.mockReset(); pollForTokenMock.mockReset(); + persistedUpdates = null; __listenerAuthTestUtils.setOAuthDepsForTests({ LETTA_CLOUD_API_URL: "https://api.letta.com", refreshAccessToken: refreshAccessTokenMock, requestDeviceCode: requestDeviceCodeMock, pollForToken: pollForTokenMock, + readPersistedTokens: readPersistedTokensFake, }); delete process.env.LETTA_API_KEY; @@ -125,7 +151,11 @@ describe("listen subcommand auth resolution", () => { }); test("refreshes saved Letta Cloud tokens when they are expired", async () => { - const updateSettingsMock = mock(() => {}); + const updateSettingsMock = mock( + (updates: NonNullable) => { + persistedUpdates = updates; + }, + ); const flushMock = mock(async () => {}); settingsManager.getSettingsWithSecureTokens = mock(async () => ({ diff --git a/src/index.ts b/src/index.ts index 1a55de7a5f..45b4707872 100755 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,4 @@ #!/usr/bin/env bun -import { hostname } from "node:os"; import { APIError } from "@letta-ai/letta-client/core/error"; import type { AgentState } from "@letta-ai/letta-client/resources/agents/agents"; import type { Message } from "@letta-ai/letta-client/resources/agents/messages"; @@ -29,7 +28,8 @@ import { buildCreateAgentOptionsForPersonality } from "./agent/personality"; import { resolvePersonalityId } from "./agent/personality-presets"; import type { MemoryPromptMode } from "./agent/prompt-assets"; import { resolveSkillSourcesSelection } from "./agent/skill-sources"; -import { LETTA_CLOUD_API_URL, refreshAccessToken } from "./auth/oauth"; +import { LETTA_CLOUD_API_URL } from "./auth/oauth"; +import { refreshTokensCoordinated } from "./auth/oauth-refresh"; import { type Backend, type BackendMode, @@ -130,23 +130,10 @@ async function refreshStartupOAuthToken( } try { - const now = Date.now(); - const deviceId = settingsManager.getOrCreateDeviceId(); - const deviceName = hostname(); - const tokens = await refreshAccessToken( - settings.refreshToken, - deviceId, - deviceName, - ); - - settingsManager.updateSettings({ - env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: tokens.refresh_token || settings.refreshToken, - tokenExpiresAt: now + tokens.expires_in * 1000, - }); - await settingsManager.flush(); - - return tokens.access_token; + // Coordinated: serializes with every other rotating-token path (other + // sessions, listeners, git-credential helpers) so the startup refresh + // cannot spend a refresh token a peer just rotated. + return await refreshTokensCoordinated(settings.refreshToken); } catch (error) { trackCliBoundaryError( "startup_auth_token_refresh_failed", diff --git a/src/providers/chatgpt-usage-service.ts b/src/providers/chatgpt-usage-service.ts index 7316974eb9..13fd70b2e1 100644 --- a/src/providers/chatgpt-usage-service.ts +++ b/src/providers/chatgpt-usage-service.ts @@ -1,9 +1,6 @@ import { hostname } from "node:os"; -import { - LETTA_CLOUD_API_URL, - refreshAccessToken as refreshLettaAccessToken, - type TokenResponse, -} from "@/auth/oauth"; +import { LETTA_CLOUD_API_URL, type TokenResponse } from "@/auth/oauth"; +import { refreshTokensCoordinated } from "@/auth/oauth-refresh"; import { getLettaCodeHeaders } from "@/backend/api/http-headers"; import { getLocalOAuthApiKey, @@ -708,17 +705,20 @@ async function cloudApiKey(input: { settings.tokenExpiresAt - input.now < TOKEN_REFRESH_BUFFER_MS)) ) { try { - const refresh = input.refreshAccessToken ?? refreshLettaAccessToken; - const tokens = await refresh( - settings.refreshToken, - settingsManager.getOrCreateDeviceId(), - hostname(), - ); - apiKey = tokens.access_token; - settingsManager.updateSettings({ - env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: tokens.refresh_token || settings.refreshToken, - tokenExpiresAt: input.now + tokens.expires_in * 1000, + // Coordinated: serializes with every other rotating-token path and + // persists under the same lock; injected refresh replaces the network call. + const injectedRefresh = input.refreshAccessToken; + apiKey = await refreshTokensCoordinated(settings.refreshToken, { + ...(injectedRefresh + ? { + refresh: (refreshToken: string) => + injectedRefresh( + refreshToken, + settingsManager.getOrCreateDeviceId(), + hostname(), + ), + } + : {}), }); } catch (error) { return { diff --git a/src/websocket/listener/auth.ts b/src/websocket/listener/auth.ts index cedf4ada14..59366030bf 100644 --- a/src/websocket/listener/auth.ts +++ b/src/websocket/listener/auth.ts @@ -5,7 +5,11 @@ import { refreshAccessToken, requestDeviceCode, } from "@/auth/oauth"; -import { refreshAccessTokenSingleFlight } from "@/auth/oauth-refresh"; +import { + refreshAccessTokenSingleFlight, + refreshTokensCoordinated, +} from "@/auth/oauth-refresh"; +import { readPersistedAuthTokens } from "@/auth/persisted-tokens"; import { settingsManager } from "@/settings-manager"; import { deriveListenerInstanceId, @@ -25,6 +29,8 @@ type ListenerOAuthDeps = { pollForToken: typeof pollForToken; refreshAccessToken: typeof refreshAccessToken; requestDeviceCode: typeof requestDeviceCode; + /** Durable-snapshot reader for the coordinated refresh (tests inject). */ + readPersistedTokens: typeof readPersistedAuthTokens; }; type ListenerAuthOptions = { @@ -40,6 +46,7 @@ const defaultListenerOAuthDeps: ListenerOAuthDeps = { pollForToken, refreshAccessToken, requestDeviceCode, + readPersistedTokens: readPersistedAuthTokens, }; let listenerOAuthDepsOverride: ListenerOAuthDeps | null = null; @@ -135,25 +142,25 @@ async function refreshListenerAccessToken( throw new MissingListenerApiKeyError(); } - const now = Date.now(); console.log("Access token expired, refreshing..."); - const tokens = await refreshAccessTokenSingleFlight( - settings.refreshToken, - deviceId, - connectionName, - getListenerOAuthDeps().refreshAccessToken, - ); - - settingsManager.updateSettings({ - env: { LETTA_API_KEY: tokens.access_token }, - refreshToken: tokens.refresh_token ?? settings.refreshToken, - tokenExpiresAt: now + tokens.expires_in * 1000, + // Coordinated: serializes with every other rotating-token path (CLI + // sessions, git-credential helpers) and persists under the same lock. + // The injected refresh fn (test override) is threaded through the + // in-process single-flight, matching the previous behavior. + const accessToken = await refreshTokensCoordinated(settings.refreshToken, { + readTokens: getListenerOAuthDeps().readPersistedTokens, + refresh: (refreshToken) => + refreshAccessTokenSingleFlight( + refreshToken, + deviceId, + connectionName, + getListenerOAuthDeps().refreshAccessToken, + ), }); - await settingsManager.flush(); console.log("Token refreshed successfully."); - return tokens.access_token; + return accessToken; } async function runListenerOAuthLogin( From 2e3c238d889d835b0e901e578c996a25e0815d5a Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 20:46:38 -0700 Subject: [PATCH 6/8] chore: ratchet index.ts size baseline after rebase Co-Authored-By: Claude Opus 5 (1M context) --- scripts/source-file-size-baseline.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/source-file-size-baseline.json b/scripts/source-file-size-baseline.json index 776e5cedb5..68b8320d66 100644 --- a/scripts/source-file-size-baseline.json +++ b/scripts/source-file-size-baseline.json @@ -24,7 +24,7 @@ "src/cli/subcommands/skills.ts": 1264, "src/headless.ts": 5242, "src/hooks/integration.test.ts": 1147, - "src/index.ts": 2776, + "src/index.ts": 2763, "src/mods/learning-harness.ts": 2434, "src/mods/mod-engine.test.ts": 2153, "src/mods/mod-engine.ts": 1847, From cd00c8f6d791df2d395a6ec476c670dc357e40f7 Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 20:56:23 -0700 Subject: [PATCH 7/8] test(auth): align refresh-path tests with fail-closed persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit client-soft-fail and the two listener auth suites neutered persistence (no-op updateSettings) or omitted a snapshot source, so the coordinated refresh's durable read-back correctly rejected them as failed persists — which is the exact behavior the read-back exists to catch, but here it was the test harness, not the product, failing to persist. - client-soft-fail scenarios run under LETTA_SKIP_KEYCHAIN_CHECK so the durable snapshot is file-backed and deterministic on every platform (Linux CI has no keychain; macOS runners do), and the keychain-recovery scenario persists refreshed tokens into the temp-HOME settings file so the read-back can verify them. - Both listener suites inject a readPersistedTokens fake that reflects what their stubbed updateSettings captured. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent/client-soft-fail.test.ts | 22 ++++++++++++++++++- src/websocket/listener/auth.test.ts | 34 +++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/agent/client-soft-fail.test.ts b/src/agent/client-soft-fail.test.ts index 942798c755..2e3f0d4a10 100644 --- a/src/agent/client-soft-fail.test.ts +++ b/src/agent/client-soft-fail.test.ts @@ -43,6 +43,12 @@ async function runIsolatedClientScript( HOME: homeDir, USERPROFILE: homeDir, LETTA_CODE_AGENT_ROLE: "subagent", + // Force readPersistedAuthTokens (the coordinated refresh's durable + // snapshot) onto its file-backed path: these scenarios stub keychain + // behavior at the settingsManager level, and the machine keychain must + // never be consulted or written. Also makes the scenarios deterministic + // across platforms (Linux CI has no keychain; macOS runners do). + LETTA_SKIP_KEYCHAIN_CHECK: "1", }; delete env.LETTA_API_KEY; delete env.LETTA_BASE_URL; @@ -238,7 +244,21 @@ describe("getClient soft failures", () => { }); await settingsManager.flush(); - settingsManager.updateSettings = () => {}; + // Persist refreshed tokens into the temp-HOME settings file instead + // of the real store: the coordinated refresh verifies persistence + // with a durable read-back (file-backed here via + // LETTA_SKIP_KEYCHAIN_CHECK), so a swallowing no-op would correctly + // be rejected as a failed persist. + const { writeFileSync } = await import("node:fs"); + const { join } = await import("node:path"); + const settingsPath = join(process.env.HOME, ".letta", "settings.json"); + settingsManager.updateSettings = (updates) => { + writeFileSync(settingsPath, JSON.stringify({ + env: { LETTA_API_KEY: updates.env?.LETTA_API_KEY }, + refreshToken: updates.refreshToken, + tokenExpiresAt: updates.tokenExpiresAt, + }), "utf-8"); + }; let refreshReadCount = 0; let fetchCalls = 0; diff --git a/src/websocket/listener/auth.test.ts b/src/websocket/listener/auth.test.ts index a116c67a4f..885af13939 100644 --- a/src/websocket/listener/auth.test.ts +++ b/src/websocket/listener/auth.test.ts @@ -42,21 +42,51 @@ describe("listener auth", () => { const originalListenerInstanceId = process.env[LISTENER_INSTANCE_ID_ENV]; let settings: ListenerSettings; - const updateSettingsMock = mock(() => {}); + // Durable-snapshot fake for the coordinated refresh: reflects what + // updateSettingsMock captured so the coordinator's persistence read-back + // verifies against the fake store instead of the developer's real + // settings file and keychain. Before any persist, it reports the + // scenario's stored refresh token as stale. + let persistedUpdates: { + env: { LETTA_API_KEY: string }; + refreshToken: string; + tokenExpiresAt: number; + } | null = null; + const readPersistedTokensFake = async () => + persistedUpdates + ? { + apiKey: persistedUpdates.env.LETTA_API_KEY, + refreshToken: persistedUpdates.refreshToken, + tokenExpiresAt: persistedUpdates.tokenExpiresAt, + source: "file" as const, + } + : { + apiKey: null, + refreshToken: settings.refreshToken ?? null, + tokenExpiresAt: Date.now() - 1000, + source: "file" as const, + }; + const updateSettingsMock = mock( + (updates: NonNullable) => { + persistedUpdates = updates; + }, + ); const flushMock = mock(async () => {}); beforeEach(() => { settings = { env: {} } as ListenerSettings; + persistedUpdates = null; refreshAccessTokenMock.mockReset(); requestDeviceCodeMock.mockReset(); pollForTokenMock.mockReset(); - updateSettingsMock.mockReset(); + updateSettingsMock.mockClear(); flushMock.mockReset(); __listenerAuthTestUtils.setOAuthDepsForTests({ LETTA_CLOUD_API_URL: "https://api.letta.com", refreshAccessToken: refreshAccessTokenMock, requestDeviceCode: requestDeviceCodeMock, pollForToken: pollForTokenMock, + readPersistedTokens: readPersistedTokensFake, }); settingsManager.getSettingsWithSecureTokens = mock( async () => settings, From c33fa1e58d7b147f75c41c4f88d4893d2bb4ab4c Mon Sep 17 00:00:00 2001 From: cpacker Date: Mon, 3 Aug 2026 21:32:08 -0700 Subject: [PATCH 8/8] fix(test): stop the standalone-entry probe build exhausting CI descriptors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the Linux CI mass failure (~190 tests, present on every run of this branch): standalone-entry.test.ts bundles standalone-entry with Bun.build IN-PROCESS, and the git-credential fast path made that traversal pull the subcommand's entire lazy graph (settings, auth, telemetry — probe bundle 94KB → 349KB). On low-ulimit runners the file descriptor pressure broke bun's module resolution for every test file loaded afterwards ("Cannot find module '@/backend'"), failing ~190 unrelated tests. Reproduced locally with ulimit -n 512 and bisected by A/B-swapping standalone-entry between 7d20005f and HEAD. The probe now stubs ./cli/subcommands/git-credential exactly like it already stubs ./index — the test verifies pi-ai OAuth flows are statically embedded, nothing else. The real build is unaffected and still bundles the subcommand. Also hardens the multi-process suite for full-suite runs: hermetic worker env (no inherited suite env mutations), 30s barrier deadline for loaded runners, and the unsynchronized control now accepts the two timing-dependent worker outcomes it legitimately produces — every worker persists to one store, so read-back verification correctly rejects whoever was overwritten ("failed to persist"); the control's oracle is the refresh count, which must exceed one. Co-Authored-By: Claude Opus 5 (1M context) --- src/auth/oauth-refresh-multi-process.test.ts | 31 +++++++++++++++++--- src/standalone-entry.test.ts | 21 +++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/auth/oauth-refresh-multi-process.test.ts b/src/auth/oauth-refresh-multi-process.test.ts index 6945f9f845..967c5244ba 100644 --- a/src/auth/oauth-refresh-multi-process.test.ts +++ b/src/auth/oauth-refresh-multi-process.test.ts @@ -70,7 +70,9 @@ const workerCount = Number(process.env.WORKER_COUNT); */ function waitForBarrier(label) { appendFileSync(readyPath, "."); - const deadline = Date.now() + 10000; + // Generous: under a loaded full-suite run, sibling workers' bun cold + // starts can take seconds each before they reach the barrier. + const deadline = Date.now() + 30000; for (;;) { let ready = 0; try { ready = statSync(readyPath).size; } catch {} @@ -177,11 +179,20 @@ async function spawnWorkers(options: { const scriptPath = join(options.shareDir, "worker.mjs"); writeFileSync(scriptPath, WORKER_SCRIPT, "utf8"); + // Hermetic env: inheriting the suite's process.env lets earlier test + // files' env mutations leak into the workers (this whole suite runs in one + // process). Workers only need to resolve bun/the module and write to the + // share dir. + const workerEnv = { + PATH: process.env.PATH ?? "", + HOME: process.env.HOME ?? "", + USERPROFILE: process.env.USERPROFILE ?? "", + }; const running = Array.from({ length: options.count }, (_unused, id) => Bun.spawn(["bun", scriptPath], { cwd: REPO_ROOT, env: { - ...process.env, + ...workerEnv, REFRESH_MODULE: REFRESH_MODULE, SHARE_DIR: options.shareDir, LOCK_PATH: options.lockPathFor(id), @@ -226,7 +237,10 @@ describe("refreshTokensCoordinated across processes", () => { }); for (const worker of workers) { - expect(worker.stdout.startsWith("ok:"), worker.stderr).toBe(true); + expect( + worker.stdout.startsWith("ok:"), + `stdout=${worker.stdout} stderr=${worker.stderr}`, + ).toBe(true); expect(worker.exitCode).toBe(0); } expect(refreshCount(shareDir)).toBe(1); @@ -251,8 +265,17 @@ describe("refreshTokensCoordinated across processes", () => { loadBarrier: true, }); + // Individual worker outcomes are timing-dependent here BY DESIGN: every + // worker persists its own rotation to the same store, so the read-back + // verification correctly rejects whoever gets overwritten before + // verifying ("failed to persist"). Both terminal states are evidence of + // the unsynchronized world; the oracle is the refresh count. for (const worker of workers) { - expect(worker.stdout.startsWith("ok:"), worker.stderr).toBe(true); + const outcome = worker.stdout; + expect( + outcome.startsWith("ok:") || outcome.includes("failed to persist"), + `stdout=${worker.stdout} stderr=${worker.stderr}`, + ).toBe(true); } expect(refreshCount(shareDir)).toBeGreaterThan(1); }, 60_000); diff --git a/src/standalone-entry.test.ts b/src/standalone-entry.test.ts index df3b646a60..81ff554fc3 100644 --- a/src/standalone-entry.test.ts +++ b/src/standalone-entry.test.ts @@ -16,8 +16,20 @@ test("standalone bundle resolves statically embedded OAuth flows", async () => { const tempDir = await mkdtemp(join(projectRoot, ".standalone-oauth-test-")); tempDirs.push(tempDir); const probePath = join(tempDir, "oauth-probe.ts"); + const gitCredentialStubPath = join(tempDir, "git-credential-stub.ts"); const outputPath = join(tempDir, "oauth-probe.js"); + // Stub the git-credential fast path the same way ./index is stubbed: this + // test only verifies that pi-ai's OAuth flows are statically embedded. + // Without the stub, Bun.build traverses the subcommand's entire lazy graph + // (settings, auth, telemetry) IN-PROCESS, and on low-ulimit runners (Linux + // CI) the file-descriptor pressure breaks module resolution for every test + // file loaded after this one. + await writeFile( + gitCredentialStubPath, + "export async function runGitCredentialSubcommand() {\n return 1;\n}\n", + ); + await writeFile( probePath, `import { openaiCodexProvider } from "@earendil-works/pi-ai/providers/openai-codex"; @@ -51,6 +63,15 @@ console.log(auth.apiKey); } return undefined; }); + build.onResolve( + { filter: /^\.\/cli\/subcommands\/git-credential$/ }, + ({ importer }) => { + if (importer.endsWith("standalone-entry.ts")) { + return { path: gitCredentialStubPath }; + } + return undefined; + }, + ); }, }, ],