From 32d984e50735dca23ae229c15c07a74431a60330 Mon Sep 17 00:00:00 2001 From: roboclaw-bot <309084314+roboclaw-bot@users.noreply.github.com> Date: Thu, 27 Aug 2026 04:40:05 +0000 Subject: [PATCH] fix(gateway): isolate mutable GitHub identity checks Co-authored-by: Jesse Merhi <79823012+jesse-merhi@users.noreply.github.com> --- docs/concepts/user-model.md | 2 +- .../github-user-identity-coordinator.ts | 115 ++++-------------- src/gateway/github-user-identity.test.ts | 67 +++------- src/gateway/github-user-identity.ts | 35 ++++-- 4 files changed, 68 insertions(+), 151 deletions(-) diff --git a/docs/concepts/user-model.md b/docs/concepts/user-model.md index ff39870a81ec..aa874f4dc5fd 100644 --- a/docs/concepts/user-model.md +++ b/docs/concepts/user-model.md @@ -20,7 +20,7 @@ GitHub-backed sign-in is supported through Cloudflare Access and Tailscale Serve The **GitHub account** row is read-only. Generic trusted proxies, token, password, and unauthenticated connections cannot claim a GitHub account, and agent or tool GitHub credentials are never used for this identity. The forwarded Cloudflare Access assertion is connection-scoped: OpenClaw does not persist, export, log, or expose it to the UI or model. -Identity lookup runs after WebSocket sign-in, so connection status and other identity-independent reads remain available. Profile and session work waits for the lookup; a Cloudflare or GitHub rate limit or network failure returns retryable unavailability without exposing a mutable alias or erasing a previously verified account. Concurrent checks for the same GitHub login share only the active request; a later connection verifies the mutable login again before attaching a profile. Immutable numeric account-id results can be reused during a rate-limit backoff. A later request, connection, or Profile refresh retries the lookup. GitHub login renames are reconciled by numeric account id so profile history and preferences stay attached to one person. +Identity lookup runs after WebSocket sign-in, so connection status and other identity-independent reads remain available. Profile and session work waits for the lookup; a Cloudflare or GitHub rate limit or network failure returns retryable unavailability without exposing a mutable alias or erasing a previously verified account. GitHub lookups are serialized against the shared credential quota and independently verified for every connection before profile attachment. A later request, connection, or Profile refresh retries the lookup. GitHub login renames are reconciled by numeric account id so profile history and preferences stay attached to one person. Public commit metadata is a separate choice. **Git co-author credit** defaults off. Enabling it adds the verified account's public GitHub noreply address to commits created from shared sessions; OpenClaw never requests or stores a private GitHub email for this feature. Signing in as a different numeric GitHub account resets the choice, so one account cannot inherit another account's consent. diff --git a/src/gateway/github-user-identity-coordinator.ts b/src/gateway/github-user-identity-coordinator.ts index 3ed349b4f19f..6c3672c773d0 100644 --- a/src/gateway/github-user-identity-coordinator.ts +++ b/src/gateway/github-user-identity-coordinator.ts @@ -1,107 +1,59 @@ import { KeyedAsyncQueue } from "../plugin-sdk/keyed-async-queue.js"; import { ControlUiGitHubError } from "./control-ui-github-api.js"; -const SUCCESS_CACHE_MS = 5 * 60_000; const RATE_LIMIT_FALLBACK_MS = 60_000; const CAPACITY_RETRY_MS = 1_000; const CACHE_LIMIT = 200; export type ResolvedGitHubUserIdentity = { accountId: number; login: string }; -type LookupEntry = { - freshUntil: number; - inFlight?: Promise; - value?: ResolvedGitHubUserIdentity; -}; - class GitHubUserIdentityCoordinator { private readonly backoffs = new Map(); - private readonly lookups = new Map(); + private pendingLookups = 0; private queue = new KeyedAsyncQueue(); lookup(params: { credentialScope: string; - identityKind: "account-id" | "login"; - lookupKey: string; request: () => Promise; }): Promise { - const cacheKey = `${params.credentialScope}:${params.lookupKey}`; - const cacheCompleted = params.identityKind === "account-id"; - const cached = this.touchLookup(cacheKey); - if (cacheCompleted && cached?.value && cached.freshUntil > Date.now()) { - return Promise.resolve(cached.value); - } - if (cached?.inFlight) { - return cached.inFlight; - } - const backoffRemaining = this.backoffRemaining(params.credentialScope); if (backoffRemaining > 0) { - // Numeric account IDs are immutable. Login aliases are not, so they must - // never use stale verification when GitHub cannot confirm current ownership. - if (cacheCompleted && cached?.value) { - return Promise.resolve(cached.value); - } return Promise.reject(this.rateLimitError(backoffRemaining)); } - if (!cached) { - this.pruneLookups(CACHE_LIMIT - 1); - if (this.lookups.size >= CACHE_LIMIT) { - return Promise.reject(this.capacityError()); - } + if (this.pendingLookups >= CACHE_LIMIT) { + return Promise.reject(this.capacityError()); } - - const entry = cached ?? { freshUntil: 0 }; - const current = this.queue.enqueue(params.credentialScope, async () => { - const queuedBackoffRemaining = this.backoffRemaining(params.credentialScope); - if (queuedBackoffRemaining > 0) { - if (cacheCompleted && entry.value) { - return entry.value; + return this.queue.enqueue( + params.credentialScope, + async () => { + const queuedBackoffRemaining = this.backoffRemaining(params.credentialScope); + if (queuedBackoffRemaining > 0) { + throw this.rateLimitError(queuedBackoffRemaining); } - throw this.rateLimitError(queuedBackoffRemaining); - } - try { - const identity = await params.request(); - if (!cacheCompleted) { - return identity; - } - entry.value = identity; - entry.freshUntil = Date.now() + SUCCESS_CACHE_MS; - return identity; - } catch (error) { - if (error instanceof ControlUiGitHubError && error.statusCode === 429) { - const retryAfterMs = error.retryAfterMs ?? RATE_LIMIT_FALLBACK_MS; - this.setBackoff(params.credentialScope, retryAfterMs); - if (cacheCompleted && entry.value) { - return entry.value; + try { + return await params.request(); + } catch (error) { + if (error instanceof ControlUiGitHubError && error.statusCode === 429) { + const retryAfterMs = error.retryAfterMs ?? RATE_LIMIT_FALLBACK_MS; + this.setBackoff(params.credentialScope, retryAfterMs); } - } - throw error; - } - }); - entry.inFlight = current; - this.lookups.delete(cacheKey); - this.lookups.set(cacheKey, entry); - void current.then( - () => { - entry.inFlight = undefined; - if (!cacheCompleted) { - this.lookups.delete(cacheKey); + throw error; } }, - () => { - entry.inFlight = undefined; - if (!entry.value) { - this.lookups.delete(cacheKey); - } + { + onEnqueue: () => { + this.pendingLookups += 1; + }, + onSettle: () => { + this.pendingLookups -= 1; + }, }, ); - return current; } reset(): void { this.backoffs.clear(); - this.lookups.clear(); + this.pendingLookups = 0; this.queue = new KeyedAsyncQueue(); } @@ -114,16 +66,6 @@ class GitHubUserIdentityCoordinator { return remaining; } - private pruneLookups(maxSize: number): void { - while (this.lookups.size > maxSize) { - const oldestIdle = [...this.lookups].find(([, entry]) => !entry.inFlight); - if (!oldestIdle) { - return; - } - this.lookups.delete(oldestIdle[0]); - } - } - private capacityError(): ControlUiGitHubError { return new ControlUiGitHubError( 429, @@ -153,15 +95,6 @@ class GitHubUserIdentityCoordinator { this.backoffs.delete(oldestKey); } } - - private touchLookup(cacheKey: string): LookupEntry | undefined { - const cached = this.lookups.get(cacheKey); - if (cached) { - this.lookups.delete(cacheKey); - this.lookups.set(cacheKey, cached); - } - return cached; - } } export const githubUserIdentityCoordinator = new GitHubUserIdentityCoordinator(); diff --git a/src/gateway/github-user-identity.test.ts b/src/gateway/github-user-identity.test.ts index 463cfac851fe..1d67787aaf7e 100644 --- a/src/gateway/github-user-identity.test.ts +++ b/src/gateway/github-user-identity.test.ts @@ -304,49 +304,17 @@ describe("authenticated GitHub identity sync", () => { }); }); - it("deduplicates concurrent lookups across Gateway connections", async () => { - await withOpenClawTestState({ scenario: "minimal" }, async () => { - const profile = ensureProfileForTailscaleIdentity({ login: "ada@github" }); - let resolveLookup: ((response: Response) => void) | undefined; - const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementationOnce( - async () => - await new Promise((resolve) => { - resolveLookup = resolve; - }), - ); - - const connections = Array.from({ length: 10 }, () => - createAuthenticatedGitHubIdentitySync({ - authResult: { - ok: true, - method: "tailscale", - user: "ada@github", - tailscaleIdentity: { login: "ada@github", name: "Ada" }, - }, - }), - ); - const requests = connections.map((sync) => { - if (!sync) { - throw new Error("GitHub test identity did not produce a sync function"); - } - return sync(); - }); - await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledOnce()); - resolveLookup?.(githubResponse({ id: 583231, login: "Ada" })); - - await expect(Promise.all(requests)).resolves.toEqual( - Array.from({ length: 10 }, () => expect.objectContaining({ profileId: profile.id })), - ); - expect(getUserProfileListItem(profile.id).githubIdentity).toMatchObject({ login: "Ada" }); - expect(fetchMock).toHaveBeenCalledOnce(); - }); - }); - - it("freshly verifies a mutable login before attaching a later connection", async () => { + it("independently verifies a mutable login before attaching a concurrent connection", async () => { await withOpenClawTestState({ scenario: "minimal" }, async () => { + let resolveFirst: ((response: Response) => void) | undefined; const fetchMock = vi .spyOn(globalThis, "fetch") - .mockResolvedValueOnce(githubResponse({ id: 1, login: "ada" })) + .mockImplementationOnce( + async () => + await new Promise((resolve) => { + resolveFirst = resolve; + }), + ) .mockResolvedValueOnce(githubResponse({ id: 2, login: "ada" })); const createSync = () => createAuthenticatedGitHubIdentitySync({ @@ -358,8 +326,15 @@ describe("authenticated GitHub identity sync", () => { }, }); - const first = await createSync()?.(); - const reassigned = await createSync()?.(); + const firstRequest = createSync()?.(); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledOnce()); + const reassignedRequest = createSync()?.(); + expect(fetchMock).toHaveBeenCalledOnce(); + + resolveFirst?.(githubResponse({ id: 1, login: "ada" })); + const first = await firstRequest; + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + const reassigned = await reassignedRequest; expect(fetchMock).toHaveBeenCalledTimes(2); expect(reassigned?.profileId).not.toBe(first?.profileId); @@ -377,8 +352,6 @@ describe("authenticated GitHub identity sync", () => { const inFlight = Array.from({ length: 200 }, (_, index) => githubUserIdentityCoordinator.lookup({ credentialScope: "capacity-test", - identityKind: "login", - lookupKey: `login:user-${index}`, request: async () => { await requestGate; return { accountId: index + 1, login: `user-${index}` }; @@ -391,8 +364,6 @@ describe("authenticated GitHub identity sync", () => { await expect( githubUserIdentityCoordinator.lookup({ credentialScope: "capacity-test", - identityKind: "login", - lookupKey: "login:overflow", request: overflowRequest, }), ).rejects.toMatchObject({ statusCode: 429, retryAfterMs: 1_000 }); @@ -415,14 +386,10 @@ describe("authenticated GitHub identity sync", () => { const first = githubUserIdentityCoordinator.lookup({ credentialScope: "shared-quota", - identityKind: "login", - lookupKey: "login:ada", request: firstRequest, }); const second = githubUserIdentityCoordinator.lookup({ credentialScope: "shared-quota", - identityKind: "login", - lookupKey: "login:grace", request: secondRequest, }); await vi.waitFor(() => expect(firstRequest).toHaveBeenCalledOnce()); diff --git a/src/gateway/github-user-identity.ts b/src/gateway/github-user-identity.ts index 53d525620f18..5c0db63754ef 100644 --- a/src/gateway/github-user-identity.ts +++ b/src/gateway/github-user-identity.ts @@ -33,6 +33,19 @@ type AuthenticatedGitHubIdentitySyncResult = { profileId: string; updatedAt: num export type AuthenticatedGitHubIdentitySync = () => Promise; type GitHubApiCredentialScope = ReturnType; +class GitHubIdentityLookupError extends ControlUiGitHubError { + constructor( + error: ControlUiGitHubError, + readonly retryableForCachedIdentity: boolean, + ) { + super( + error.statusCode, + error.message, + error.retryAfterMs === undefined ? undefined : { retryAfterMs: error.retryAfterMs }, + ); + } +} + function headerValue(value: string | string[] | undefined): string | undefined { return Array.isArray(value) ? value[0] : value; } @@ -116,13 +129,21 @@ async function resolveCloudflareAccessIdentity( } async function fetchGitHubIdentityPayload(rawUrl: string, token: string | undefined) { + let response: Response | undefined; try { - return await readGitHubJsonResponse(await fetchGitHubApi(rawUrl, fetch, token)); + response = await fetchGitHubApi(rawUrl, fetch, token); + return await readGitHubJsonResponse(response); } catch (error) { if (error instanceof ControlUiGitHubError) { - throw error; + throw new GitHubIdentityLookupError( + error, + response === undefined || error.statusCode === 429 || response.status >= 500, + ); } - throw new ControlUiGitHubError(502, "GitHub request failed"); + throw new GitHubIdentityLookupError( + new ControlUiGitHubError(502, "GitHub request failed"), + response === undefined, + ); } } @@ -136,8 +157,6 @@ async function resolveGitHubUserIdentityByLogin( } return githubUserIdentityCoordinator.lookup({ credentialScope: credential.cacheScope, - identityKind: "login", - lookupKey: `login:${requestedLogin}`, request: async () => { const payload = await fetchGitHubIdentityPayload( `${GITHUB_API_ORIGIN}/users/${encodeURIComponent(requestedLogin)}`, @@ -166,8 +185,6 @@ function resolveGitHubUserIdentityById( ): Promise { return githubUserIdentityCoordinator.lookup({ credentialScope: credential.cacheScope, - identityKind: "account-id", - lookupKey: `id:${accountId}`, request: async () => { const payload = await fetchGitHubIdentityPayload( `${GITHUB_API_ORIGIN}/user/${accountId}`, @@ -275,8 +292,8 @@ export function createAuthenticatedGitHubIdentitySync(params: { identity = await resolveGitHubUserIdentityById(accessIdentity.accountId, credential); } catch (error) { const retryable = - error instanceof ControlUiGitHubError && - (error.statusCode === 429 || error.statusCode >= 500); + (error instanceof GitHubIdentityLookupError && error.retryableForCachedIdentity) || + (error instanceof ControlUiGitHubError && error.statusCode === 429); if (retryable) { // Retry failures may reuse only the exact verified email + immutable-account binding. const cached = resolveCachedGitHubIdentity({