mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 13:26:04 -06:00
fix(ui): recover GitHub previews when GitHub auth is stale (#121454)
* fix(ui): recover GitHub previews from stale auth * fix(ui): keep GitHub preview helpers internal
This commit is contained in:
committed by
GitHub
parent
23d1eca455
commit
17bea1136f
@@ -5,7 +5,7 @@ export { isRecord } from "@openclaw/normalization-core/record-coerce";
|
||||
import { readResponseWithLimit } from "../infra/http-body.js";
|
||||
|
||||
export const GITHUB_API_ORIGIN = "https://api.github.com";
|
||||
export const GITHUB_JSON_MAX_BYTES = 256 * 1024;
|
||||
const GITHUB_JSON_MAX_BYTES = 256 * 1024;
|
||||
export const GITHUB_REQUEST_TIMEOUT_MS = 8_000;
|
||||
const GITHUB_API_VERSION = "2022-11-28";
|
||||
const GITHUB_API_MAX_REDIRECTS = 3;
|
||||
@@ -119,16 +119,6 @@ export async function readBoundedResponse(response: Response, maxBytes: number):
|
||||
}
|
||||
}
|
||||
|
||||
export function upstreamErrorStatus(status: number): number {
|
||||
if (status === 404) {
|
||||
return 404;
|
||||
}
|
||||
if (status === 403 || status === 429) {
|
||||
return 429;
|
||||
}
|
||||
return 502;
|
||||
}
|
||||
|
||||
// GitHub reports quota exhaustion as 429 or as 403 with exhausted-quota
|
||||
// headers; a bare 403 is a permission response and must stay distinguishable
|
||||
// so callers can degrade optional fetches instead of flagging rate limits.
|
||||
@@ -142,25 +132,36 @@ function isGitHubRateLimitResponse(response: Response): boolean {
|
||||
);
|
||||
}
|
||||
|
||||
function jsonErrorStatus(response: Response): number {
|
||||
function githubResponseErrorStatus(response: Response): number {
|
||||
if (isGitHubRateLimitResponse(response)) {
|
||||
return 429;
|
||||
}
|
||||
if (response.status === 404 || response.status === 403) {
|
||||
if (response.status === 401 || response.status === 403 || response.status === 404) {
|
||||
return response.status;
|
||||
}
|
||||
return 502;
|
||||
}
|
||||
|
||||
/** Fetch a GitHub API JSON document with bounded size and normalized errors. */
|
||||
export async function fetchGitHubJson(
|
||||
rawUrl: string,
|
||||
fetchImpl: typeof fetch,
|
||||
token?: string,
|
||||
): Promise<unknown> {
|
||||
const response = await fetchGitHubApi(rawUrl, fetchImpl, token);
|
||||
// Optional host auth raises quota and unlocks private-repo reads, but an
|
||||
// unusable credential must not disable public GitHub data that works anonymously.
|
||||
export async function withOptionalGitHubAuth<T>(
|
||||
token: string | undefined,
|
||||
request: (token: string | undefined) => Promise<T>,
|
||||
): Promise<T> {
|
||||
try {
|
||||
return await request(token);
|
||||
} catch (error) {
|
||||
const status = error instanceof ControlUiGitHubError ? error.statusCode : 0;
|
||||
if (token && [401, 403, 429].includes(status)) {
|
||||
return request(undefined);
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
export async function readGitHubJsonResponse(response: Response): Promise<unknown> {
|
||||
if (!response.ok) {
|
||||
const status = jsonErrorStatus(response);
|
||||
const status = githubResponseErrorStatus(response);
|
||||
await discardResponse(response);
|
||||
throw new ControlUiGitHubError(status, `GitHub request failed (${response.status})`);
|
||||
}
|
||||
@@ -171,3 +172,14 @@ export async function fetchGitHubJson(
|
||||
throw new ControlUiGitHubError(502, "GitHub response was not valid JSON");
|
||||
}
|
||||
}
|
||||
|
||||
/** Fetch a GitHub API JSON document with bounded size and normalized errors. */
|
||||
export function fetchGitHubJson(
|
||||
rawUrl: string,
|
||||
fetchImpl: typeof fetch,
|
||||
token?: string,
|
||||
): Promise<unknown> {
|
||||
return withOptionalGitHubAuth(token, async (requestToken) =>
|
||||
readGitHubJsonResponse(await fetchGitHubApi(rawUrl, fetchImpl, requestToken)),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -244,6 +244,33 @@ describe("loadControlUiGitHubPreview", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("retries stale optional authentication anonymously for public previews", async () => {
|
||||
vi.stubEnv("GH_TOKEN", "stale-github-token");
|
||||
const fetchMock = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValueOnce(githubJson({ message: "Bad credentials" }, 401))
|
||||
.mockResolvedValueOnce(
|
||||
githubJson(
|
||||
previewPayload({
|
||||
user: { login: "octocat" },
|
||||
}),
|
||||
),
|
||||
);
|
||||
|
||||
const preview = await loadControlUiGitHubPreview(
|
||||
{ kind: "pull", number: 70012, owner: "openclaw", repo: "openclaw" },
|
||||
fetchMock,
|
||||
);
|
||||
|
||||
expect(preview.login).toBe("octocat");
|
||||
expect(fetchMock).toHaveBeenCalledTimes(2);
|
||||
expect(fetchMock.mock.calls[0]?.[1]?.headers).toHaveProperty(
|
||||
"Authorization",
|
||||
"Bearer stale-github-token",
|
||||
);
|
||||
expect(fetchMock.mock.calls[1]?.[1]?.headers).not.toHaveProperty("Authorization");
|
||||
});
|
||||
|
||||
it("follows GitHub API redirects for renamed public repositories", async () => {
|
||||
vi.stubEnv("GH_TOKEN", "github-test-token");
|
||||
const fetchMock = vi
|
||||
|
||||
@@ -6,15 +6,15 @@ import {
|
||||
discardResponse,
|
||||
fetchGitHubApi,
|
||||
GITHUB_API_ORIGIN,
|
||||
GITHUB_JSON_MAX_BYTES,
|
||||
GITHUB_REQUEST_TIMEOUT_MS,
|
||||
githubApiToken,
|
||||
isRecord,
|
||||
optionalNumber,
|
||||
optionalString,
|
||||
readBoundedResponse,
|
||||
readGitHubJsonResponse,
|
||||
requiredString,
|
||||
upstreamErrorStatus,
|
||||
withOptionalGitHubAuth,
|
||||
} from "./control-ui-github-api.js";
|
||||
|
||||
const GITHUB_AVATAR_HOST = "avatars.githubusercontent.com";
|
||||
@@ -101,21 +101,9 @@ async function assertPublicRepositoryUrl(
|
||||
): Promise<void> {
|
||||
// Private and missing repositories stop at this same request boundary before
|
||||
// any item fetch, so operator.read callers cannot probe private item numbers.
|
||||
const response = await fetchGitHubApi(repositoryUrl, fetchImpl, token);
|
||||
if (!response.ok) {
|
||||
await discardResponse(response);
|
||||
throw new ControlUiGitHubError(
|
||||
upstreamErrorStatus(response.status),
|
||||
`GitHub repository request failed (${response.status})`,
|
||||
);
|
||||
}
|
||||
const body = await readBoundedResponse(response, GITHUB_JSON_MAX_BYTES);
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(body.toString("utf8"));
|
||||
} catch {
|
||||
throw new ControlUiGitHubError(502, "GitHub repository response was not valid JSON");
|
||||
}
|
||||
const parsed = await readGitHubJsonResponse(
|
||||
await fetchGitHubApi(repositoryUrl, fetchImpl, token),
|
||||
);
|
||||
if (!isRecord(parsed) || parsed.private !== false) {
|
||||
throw new ControlUiGitHubError(404, "GitHub repository is not public");
|
||||
}
|
||||
@@ -255,34 +243,22 @@ async function fetchPreview(
|
||||
if (token) {
|
||||
await assertPublicRepositoryUrl(repositoryApiUrl(target), fetchImpl, token);
|
||||
}
|
||||
const response = await fetchGitHubApi(
|
||||
previewApiUrl(target),
|
||||
fetchImpl,
|
||||
token,
|
||||
token
|
||||
? async (url) => {
|
||||
const repositoryUrl = redirectedRepositoryApiUrl(target, url);
|
||||
if (!repositoryUrl) {
|
||||
throw new ControlUiGitHubError(502, "GitHub item returned an unsafe redirect");
|
||||
const parsed = await readGitHubJsonResponse(
|
||||
await fetchGitHubApi(
|
||||
previewApiUrl(target),
|
||||
fetchImpl,
|
||||
token,
|
||||
token
|
||||
? async (url) => {
|
||||
const repositoryUrl = redirectedRepositoryApiUrl(target, url);
|
||||
if (!repositoryUrl) {
|
||||
throw new ControlUiGitHubError(502, "GitHub item returned an unsafe redirect");
|
||||
}
|
||||
await assertPublicRepositoryUrl(repositoryUrl, fetchImpl, token);
|
||||
}
|
||||
await assertPublicRepositoryUrl(repositoryUrl, fetchImpl, token);
|
||||
}
|
||||
: undefined,
|
||||
: undefined,
|
||||
),
|
||||
);
|
||||
if (!response.ok) {
|
||||
await discardResponse(response);
|
||||
throw new ControlUiGitHubError(
|
||||
upstreamErrorStatus(response.status),
|
||||
`GitHub request failed (${response.status})`,
|
||||
);
|
||||
}
|
||||
const body = await readBoundedResponse(response, GITHUB_JSON_MAX_BYTES);
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(body.toString("utf8"));
|
||||
} catch {
|
||||
throw new ControlUiGitHubError(502, "GitHub response was not valid JSON");
|
||||
}
|
||||
if (!isRecord(parsed)) {
|
||||
throw new ControlUiGitHubError(502, "GitHub response was not an object");
|
||||
}
|
||||
@@ -318,7 +294,9 @@ export function loadControlUiGitHubPreview(
|
||||
const successCacheMs = token ? AUTHENTICATED_SUCCESS_CACHE_MS : ANONYMOUS_SUCCESS_CACHE_MS;
|
||||
const entry: CacheEntry<ControlUiGitHubPreview> = {
|
||||
expiresAt: now + successCacheMs,
|
||||
promise: fetchPreview(target, fetchImpl, token).catch((error: unknown) => {
|
||||
promise: withOptionalGitHubAuth(token, (requestToken) =>
|
||||
fetchPreview(target, fetchImpl, requestToken),
|
||||
).catch((error: unknown) => {
|
||||
// Short failure caching protects the anonymous GitHub quota when a user
|
||||
// repeatedly crosses a private, missing, or rate-limited link.
|
||||
entry.expiresAt = Date.now() + FAILURE_CACHE_MS;
|
||||
|
||||
@@ -33,12 +33,15 @@ describe("parseGitHubRemoteUrl", () => {
|
||||
describe("loadControlUiSessionPullRequests", () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers();
|
||||
vi.stubEnv("GH_TOKEN", "");
|
||||
vi.stubEnv("GITHUB_TOKEN", "");
|
||||
cacheEpochMs += 10 * 60_000;
|
||||
vi.setSystemTime(cacheEpochMs);
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await evictPullRequestCache();
|
||||
vi.unstubAllEnvs();
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
@@ -93,6 +96,28 @@ describe("loadControlUiSessionPullRequests", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("retries stale optional authentication anonymously for session PRs", async () => {
|
||||
vi.stubEnv("GH_TOKEN", "stale-github-token");
|
||||
const fetchImpl = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValueOnce(githubJson({ message: "Bad credentials" }, 401))
|
||||
.mockResolvedValueOnce(githubJson([pullListItem({ merged_at: "2026-07-09T10:00:00Z" })]));
|
||||
|
||||
const result = await loadControlUiSessionPullRequests(
|
||||
{ sessionKey: "agent:main:main" },
|
||||
{ fetchImpl, resolveGitContext },
|
||||
);
|
||||
|
||||
expect(result.pullRequests).toHaveLength(1);
|
||||
expect(result.pullRequests[0]).toMatchObject({ number: 103469, state: "merged" });
|
||||
expect(fetchImpl).toHaveBeenCalledTimes(2);
|
||||
expect(fetchImpl.mock.calls[0]?.[1]?.headers).toHaveProperty(
|
||||
"Authorization",
|
||||
"Bearer stale-github-token",
|
||||
);
|
||||
expect(fetchImpl.mock.calls[1]?.[1]?.headers).not.toHaveProperty("Authorization");
|
||||
});
|
||||
|
||||
it("skips diff and check fetches for merged PRs", async () => {
|
||||
const fetchImpl = routedFetch([
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user