mirror of
https://github.com/cline/cline.git
synced 2026-08-28 19:48:08 +08:00
fix(sdk): don't log out Codex/OCA users when token refresh fails transiently (#13565)
* fix(sdk): don't log out Codex/OCA users when token refresh fails transiently Port the cline-provider refresh semantics to openai-codex and oca: a transient refresh failure (network error, timeout, server 5xx) with an already-expired access token now rethrows instead of returning null. A null return means the refresh token was REJECTED and re-auth is required; treating an outage blip as a rejection is what turned it into a forced 'openai-codex requires re-authentication.' task stop while the settings UI still showed the user as signed in. Both providers also emit user.auth_refresh_soft_failure telemetry on transient failures (the 'prevented logout' counter the cline provider already has) and attach status/errorCode details to the genuine invalid_grant logout event. * refactor: collapse duplicate soft-failure telemetry branches and test Review feedback: compute tokenExpired once and emit the soft-failure event once in both providers, then return current credentials or rethrow. Fold the codex soft-failure telemetry assertions into the existing still-usable-token test instead of a near-duplicate case.
This commit is contained in:
committed by
GitHub
parent
62f471f233
commit
006de710d5
@@ -128,12 +128,27 @@ describe("auth/codex token lifecycle", () => {
|
||||
),
|
||||
);
|
||||
|
||||
const capture = vi.fn();
|
||||
const current = createCredentials({ expires: 150_000 });
|
||||
const result = await getValidOpenAICodexCredentials(current, {
|
||||
refreshBufferMs: 60_000,
|
||||
retryableTokenGraceMs: 30_000,
|
||||
telemetry: { capture } as never,
|
||||
});
|
||||
expect(result).toBe(current);
|
||||
expect(capture).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: "user.auth_refresh_soft_failure",
|
||||
properties: expect.objectContaining({
|
||||
provider: "openai-codex",
|
||||
status: 500,
|
||||
tokenExpired: false,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(capture).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ event: "user.auth_logged_out" }),
|
||||
);
|
||||
nowSpy.mockRestore();
|
||||
});
|
||||
|
||||
@@ -153,4 +168,75 @@ describe("auth/codex token lifecycle", () => {
|
||||
"Failed to refresh OpenAI Codex token",
|
||||
);
|
||||
});
|
||||
|
||||
it("throws on transient refresh error when the token is already expired", async () => {
|
||||
// A server error landing after expiry is NOT an invalid grant; returning
|
||||
// null here is what turned an outage blip into a forced
|
||||
// "requires re-authentication" stop.
|
||||
const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000);
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn(
|
||||
async () =>
|
||||
new Response(
|
||||
JSON.stringify({
|
||||
error: "server_error",
|
||||
error_description: "temporary issue",
|
||||
}),
|
||||
{ status: 500, headers: { "Content-Type": "application/json" } },
|
||||
),
|
||||
),
|
||||
);
|
||||
|
||||
const capture = vi.fn();
|
||||
await expect(
|
||||
getValidOpenAICodexCredentials(createCredentials({ expires: 90_000 }), {
|
||||
telemetry: { capture } as never,
|
||||
}),
|
||||
).rejects.toThrow("Token refresh failed: 500");
|
||||
expect(capture).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: "user.auth_refresh_soft_failure",
|
||||
properties: expect.objectContaining({
|
||||
provider: "openai-codex",
|
||||
status: 500,
|
||||
tokenExpired: true,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(capture).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ event: "user.auth_logged_out" }),
|
||||
);
|
||||
nowSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("throws on a network-level refresh failure when the token is already expired", async () => {
|
||||
const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000);
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn(async () => {
|
||||
throw new TypeError("fetch failed");
|
||||
}),
|
||||
);
|
||||
|
||||
const capture = vi.fn();
|
||||
await expect(
|
||||
getValidOpenAICodexCredentials(createCredentials({ expires: 90_000 }), {
|
||||
telemetry: { capture } as never,
|
||||
}),
|
||||
).rejects.toThrow("Failed to refresh OpenAI Codex token");
|
||||
expect(capture).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: "user.auth_refresh_soft_failure",
|
||||
properties: expect.objectContaining({
|
||||
provider: "openai-codex",
|
||||
tokenExpired: true,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(capture).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ event: "user.auth_logged_out" }),
|
||||
);
|
||||
nowSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -10,6 +10,7 @@ import { nanoid } from "nanoid";
|
||||
import {
|
||||
captureAuthFailed,
|
||||
captureAuthLoggedOut,
|
||||
captureAuthRefreshSoftFailure,
|
||||
captureAuthStarted,
|
||||
captureAuthSucceeded,
|
||||
identifyAccount,
|
||||
@@ -423,16 +424,44 @@ export async function getValidOpenAICodexCredentials(
|
||||
);
|
||||
return refreshed;
|
||||
} catch (error) {
|
||||
const failureDetails = {
|
||||
status:
|
||||
error instanceof OpenAICodexOAuthTokenError ? error.status : undefined,
|
||||
errorCode:
|
||||
error instanceof OpenAICodexOAuthTokenError
|
||||
? error.errorCode
|
||||
: undefined,
|
||||
errorName: error instanceof Error ? error.name : undefined,
|
||||
};
|
||||
if (
|
||||
error instanceof OpenAICodexOAuthTokenError &&
|
||||
error.isLikelyInvalidGrant()
|
||||
) {
|
||||
captureAuthLoggedOut(options?.telemetry, "openai-codex", "invalid_grant");
|
||||
captureAuthLoggedOut(
|
||||
options?.telemetry,
|
||||
"openai-codex",
|
||||
"invalid_grant",
|
||||
{
|
||||
status: error.status,
|
||||
errorCode: error.errorCode,
|
||||
},
|
||||
);
|
||||
return null;
|
||||
}
|
||||
if (currentCredentials.expires - Date.now() > retryableTokenGraceMs) {
|
||||
const tokenExpired =
|
||||
currentCredentials.expires - Date.now() <= retryableTokenGraceMs;
|
||||
captureAuthRefreshSoftFailure(options?.telemetry, "openai-codex", {
|
||||
...failureDetails,
|
||||
tokenExpired,
|
||||
});
|
||||
if (!tokenExpired) {
|
||||
return currentCredentials;
|
||||
}
|
||||
return null;
|
||||
// Rethrow instead of returning null. A null from this function means the
|
||||
// refresh token was REJECTED (re-auth required); a network blip or server
|
||||
// error that happens to land after expiry must not be mistaken for that —
|
||||
// callers turn null into a forced "requires re-authentication" stop even
|
||||
// though the very next refresh attempt would likely succeed.
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -213,6 +213,77 @@ describe("auth/oca getValidOcaCredentials", () => {
|
||||
nowSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("throws on transient refresh error when the token is already expired", async () => {
|
||||
// A server error landing after expiry is NOT an invalid grant; returning
|
||||
// null here is what turned an outage blip into a forced
|
||||
// "requires re-authentication" stop.
|
||||
const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000);
|
||||
const fetchMock = vi
|
||||
.fn()
|
||||
.mockImplementationOnce(
|
||||
async () =>
|
||||
new Response(
|
||||
JSON.stringify({
|
||||
token_endpoint: "https://idcs.expired/oauth2/v1/token",
|
||||
}),
|
||||
{
|
||||
status: 200,
|
||||
headers: { "Content-Type": "application/json" },
|
||||
},
|
||||
),
|
||||
)
|
||||
.mockImplementationOnce(
|
||||
async () =>
|
||||
new Response(
|
||||
JSON.stringify({
|
||||
error: "server_error",
|
||||
error_description: "temporary issue",
|
||||
}),
|
||||
{
|
||||
status: 500,
|
||||
headers: { "Content-Type": "application/json" },
|
||||
},
|
||||
),
|
||||
);
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
|
||||
const capture = vi.fn();
|
||||
await expect(
|
||||
getValidOcaCredentials(
|
||||
createCredentials({ expires: 90_000 }),
|
||||
{
|
||||
refreshBufferMs: 60_000,
|
||||
retryableTokenGraceMs: 30_000,
|
||||
telemetry: { capture } as never,
|
||||
},
|
||||
{
|
||||
config: {
|
||||
internal: {
|
||||
clientId: "client-3",
|
||||
idcsUrl: "https://idcs.expired",
|
||||
scopes: "openid offline_access",
|
||||
baseUrl: "https://oca.example.com",
|
||||
},
|
||||
},
|
||||
},
|
||||
),
|
||||
).rejects.toThrow("Token refresh failed: 500");
|
||||
expect(capture).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: "user.auth_refresh_soft_failure",
|
||||
properties: expect.objectContaining({
|
||||
provider: "oca",
|
||||
status: 500,
|
||||
tokenExpired: true,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(capture).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ event: "user.auth_logged_out" }),
|
||||
);
|
||||
nowSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("re-discovers token endpoint shortly after discovery fallback errors", async () => {
|
||||
const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000);
|
||||
const fetchMock = vi
|
||||
|
||||
@@ -3,6 +3,7 @@ import { nanoid } from "nanoid";
|
||||
import {
|
||||
captureAuthFailed,
|
||||
captureAuthLoggedOut,
|
||||
captureAuthRefreshSoftFailure,
|
||||
captureAuthStarted,
|
||||
captureAuthSucceeded,
|
||||
identifyAccount,
|
||||
@@ -512,13 +513,34 @@ export async function getValidOcaCredentials(
|
||||
try {
|
||||
return await refreshOcaToken(currentCredentials, providerOptions);
|
||||
} catch (error) {
|
||||
const telemetry = providerOptions?.telemetry ?? options?.telemetry;
|
||||
const failureDetails = {
|
||||
status: error instanceof OcaOAuthTokenError ? error.status : undefined,
|
||||
errorCode:
|
||||
error instanceof OcaOAuthTokenError ? error.errorCode : undefined,
|
||||
errorName: error instanceof Error ? error.name : undefined,
|
||||
};
|
||||
if (error instanceof OcaOAuthTokenError && error.isLikelyInvalidGrant()) {
|
||||
captureAuthLoggedOut(providerOptions?.telemetry, "oca", "invalid_grant");
|
||||
captureAuthLoggedOut(telemetry, "oca", "invalid_grant", {
|
||||
status: error.status,
|
||||
errorCode: error.errorCode,
|
||||
});
|
||||
return null;
|
||||
}
|
||||
if (currentCredentials.expires - Date.now() > retryableTokenGraceMs) {
|
||||
const tokenExpired =
|
||||
currentCredentials.expires - Date.now() <= retryableTokenGraceMs;
|
||||
captureAuthRefreshSoftFailure(telemetry, "oca", {
|
||||
...failureDetails,
|
||||
tokenExpired,
|
||||
});
|
||||
if (!tokenExpired) {
|
||||
return currentCredentials;
|
||||
}
|
||||
return null;
|
||||
// Rethrow instead of returning null. A null from this function means the
|
||||
// refresh token was REJECTED (re-auth required); a network blip or server
|
||||
// error that happens to land after expiry must not be mistaken for that —
|
||||
// callers turn null into a forced "requires re-authentication" stop even
|
||||
// though the very next refresh attempt would likely succeed.
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user