Archived
fix(auth): preserve OAuth interaction semantics
This commit is contained in:
@@ -29,7 +29,7 @@ describe("OAuthLoginFlowService", () => {
|
||||
const prompt = state.prompt;
|
||||
if (prompt === undefined) throw new Error("Expected prompt");
|
||||
expect(state).toMatchObject({ auth: { url: "https://example.test/auth", instructions: "Open it" }, progress: ["Waiting for code"] });
|
||||
expect(prompt).toMatchObject({ message: "Paste code", placeholder: "code", kind: "prompt" });
|
||||
expect(prompt).toMatchObject({ message: "Paste code", placeholder: "code", kind: "prompt", promptType: "text", allowEmpty: true });
|
||||
|
||||
const afterRespond = service.respond(state.flowId, prompt.requestId, "abc123");
|
||||
expect(afterRespond.prompt).toBeUndefined();
|
||||
@@ -41,18 +41,103 @@ describe("OAuthLoginFlowService", () => {
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("allows blank text responses for providers that use blank as a default", async () => {
|
||||
let domain: string | undefined;
|
||||
const service = new OAuthLoginFlowService();
|
||||
const state = service.start({
|
||||
providerId: "github-copilot",
|
||||
providerName: "GitHub Copilot",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
domain = await interaction.prompt({
|
||||
type: "text",
|
||||
message: "GitHub Enterprise URL/domain (blank for github.com)",
|
||||
});
|
||||
}),
|
||||
});
|
||||
|
||||
const prompt = state.prompt;
|
||||
if (prompt === undefined) throw new Error("Expected text prompt");
|
||||
expect(prompt).toMatchObject({ kind: "prompt", promptType: "text", allowEmpty: true });
|
||||
|
||||
service.respond(state.flowId, prompt.requestId, "");
|
||||
await flushAsyncLogin();
|
||||
|
||||
expect(domain).toBe("");
|
||||
expect(service.get(state.flowId).status).toBe("complete");
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("preserves secret prompt semantics behind the legacy prompt kind", () => {
|
||||
const service = new OAuthLoginFlowService();
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
await interaction.prompt({ type: "secret", message: "Enter secret", placeholder: "token" });
|
||||
}),
|
||||
});
|
||||
|
||||
const prompt = state.prompt;
|
||||
if (prompt === undefined) throw new Error("Expected secret prompt");
|
||||
expect(prompt).toMatchObject({
|
||||
kind: "prompt",
|
||||
promptType: "secret",
|
||||
message: "Enter secret",
|
||||
placeholder: "token",
|
||||
});
|
||||
expect(prompt).not.toHaveProperty("allowEmpty");
|
||||
expect(() => { service.respond(state.flowId, prompt.requestId, ""); }).toThrow("A value is required");
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("preserves info-event links without replacing the authorization URL", () => {
|
||||
const service = new OAuthLoginFlowService();
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
interaction.notify({ type: "auth_url", url: "https://example.test/login" });
|
||||
interaction.notify({
|
||||
type: "info",
|
||||
message: "Review the provider setup guide",
|
||||
links: [{ url: "https://example.test/docs", label: "Setup guide" }],
|
||||
});
|
||||
await interaction.prompt({ type: "text", message: "Continue" });
|
||||
}),
|
||||
});
|
||||
|
||||
expect(state).toMatchObject({
|
||||
auth: { url: "https://example.test/login" },
|
||||
progress: ["Review the provider setup guide"],
|
||||
info: [{ message: "Review the provider setup guide", links: [{ url: "https://example.test/docs", label: "Setup guide" }] }],
|
||||
});
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("surfaces device-code events through the auth field", () => {
|
||||
const service = new OAuthLoginFlowService();
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
interaction.notify({ type: "device_code", userCode: "WXYZ-1234", verificationUri: "https://example.test/device" });
|
||||
interaction.notify({
|
||||
type: "device_code",
|
||||
userCode: "WXYZ-1234",
|
||||
verificationUri: "https://example.test/device",
|
||||
intervalSeconds: 5,
|
||||
expiresInSeconds: 900,
|
||||
});
|
||||
await interaction.prompt({ type: "text", message: "Waiting" });
|
||||
}),
|
||||
});
|
||||
|
||||
expect(service.get(state.flowId)).toMatchObject({ auth: { url: "https://example.test/device", instructions: "Enter code: WXYZ-1234" } });
|
||||
expect(service.get(state.flowId)).toMatchObject({
|
||||
auth: {
|
||||
url: "https://example.test/device",
|
||||
instructions: "Enter code: WXYZ-1234",
|
||||
deviceCode: { userCode: "WXYZ-1234", intervalSeconds: 5, expiresInSeconds: 900 },
|
||||
},
|
||||
});
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
@@ -66,14 +151,14 @@ describe("OAuthLoginFlowService", () => {
|
||||
selectedValue = await interaction.prompt({
|
||||
type: "select",
|
||||
message: "Choose account",
|
||||
options: [{ id: "work", label: "Work" }, { id: "personal", label: "Personal" }],
|
||||
options: [{ id: "work", label: "Work", description: "Company account" }, { id: "personal", label: "Personal" }],
|
||||
});
|
||||
}),
|
||||
});
|
||||
|
||||
const select = state.select;
|
||||
if (select === undefined) throw new Error("Expected select prompt");
|
||||
expect(select).toMatchObject({ message: "Choose account", options: [{ value: "work", label: "Work" }, { value: "personal", label: "Personal" }] });
|
||||
expect(select).toMatchObject({ message: "Choose account", options: [{ value: "work", label: "Work", description: "Company account" }, { value: "personal", label: "Personal" }] });
|
||||
|
||||
service.respond(state.flowId, select.requestId, "personal");
|
||||
await flushAsyncLogin();
|
||||
@@ -83,25 +168,53 @@ describe("OAuthLoginFlowService", () => {
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("uses a manual-code prompt for callback-server flows", async () => {
|
||||
let manualValue: string | undefined;
|
||||
it("rejects responses outside the pending select options", () => {
|
||||
const service = new OAuthLoginFlowService();
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
manualValue = await interaction.prompt({ type: "manual_code", message: "Paste the callback URL or authorization code" });
|
||||
await interaction.prompt({
|
||||
type: "select",
|
||||
message: "Choose account",
|
||||
options: [{ id: "work", label: "Work" }],
|
||||
});
|
||||
}),
|
||||
});
|
||||
|
||||
const select = state.select;
|
||||
if (select === undefined) throw new Error("Expected select prompt");
|
||||
expect(() => { service.respond(state.flowId, select.requestId, "personal"); }).toThrow("Invalid OAuth selection");
|
||||
expect(service.get(state.flowId).select).toEqual(select);
|
||||
service.dispose();
|
||||
});
|
||||
|
||||
it("uses a manual-code prompt for callback-server flows and cleans up its abort listener", async () => {
|
||||
let manualValue: string | undefined;
|
||||
const service = new OAuthLoginFlowService();
|
||||
const controller = new AbortController();
|
||||
const removeAbortListener = vi.spyOn(controller.signal, "removeEventListener");
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
runtime: fakeRuntime(async (_providerId, interaction) => {
|
||||
manualValue = await interaction.prompt({
|
||||
type: "manual_code",
|
||||
message: "Paste the callback URL or authorization code",
|
||||
signal: controller.signal,
|
||||
});
|
||||
}),
|
||||
});
|
||||
|
||||
const prompt = state.prompt;
|
||||
if (prompt === undefined) throw new Error("Expected manual prompt");
|
||||
expect(prompt).toMatchObject({ kind: "manual", message: "Paste the callback URL or authorization code" });
|
||||
expect(prompt).toMatchObject({ kind: "manual", promptType: "manual_code", message: "Paste the callback URL or authorization code" });
|
||||
|
||||
service.respond(state.flowId, prompt.requestId, "https://localhost/callback?code=abc");
|
||||
await flushAsyncLogin();
|
||||
|
||||
expect(manualValue).toBe("https://localhost/callback?code=abc");
|
||||
expect(removeAbortListener).toHaveBeenCalledWith("abort", expect.any(Function));
|
||||
expect(service.get(state.flowId).status).toBe("complete");
|
||||
service.dispose();
|
||||
});
|
||||
@@ -110,6 +223,7 @@ describe("OAuthLoginFlowService", () => {
|
||||
const promptRejected = deferred<Error>();
|
||||
const service = new OAuthLoginFlowService();
|
||||
const controller = new AbortController();
|
||||
const removeAbortListener = vi.spyOn(controller.signal, "removeEventListener");
|
||||
const state = service.start({
|
||||
providerId: "test-provider",
|
||||
providerName: "Test Provider",
|
||||
@@ -128,6 +242,7 @@ describe("OAuthLoginFlowService", () => {
|
||||
expect(state.prompt).toMatchObject({ kind: "manual" });
|
||||
controller.abort();
|
||||
await expect(promptRejected.promise).resolves.toMatchObject({ message: "Prompt cancelled" });
|
||||
expect(removeAbortListener).toHaveBeenCalledWith("abort", expect.any(Function));
|
||||
|
||||
const afterAbort = service.get(state.flowId);
|
||||
expect(afterAbort.status).toBe("running");
|
||||
|
||||
@@ -6,12 +6,16 @@ import type { CommandOption, OAuthFlowState } from "../../shared/apiTypes.js";
|
||||
/** The single runtime capability this service drives — narrowed for testable DI. */
|
||||
type OAuthLoginRuntime = Pick<ModelRuntime, "login">;
|
||||
type TimerHandle = ReturnType<typeof setTimeout>;
|
||||
type SelectPrompt = Extract<AuthPrompt, { type: "select" }>;
|
||||
type ValuePrompt = Exclude<AuthPrompt, { type: "select" }>;
|
||||
|
||||
interface PendingOAuthRequest {
|
||||
requestId: string;
|
||||
allowEmpty: boolean;
|
||||
resolve: (value: string) => void;
|
||||
reject: (error: Error) => void;
|
||||
allowedValues?: ReadonlySet<string>;
|
||||
cleanup?: () => void;
|
||||
}
|
||||
|
||||
interface OAuthFlowRecord {
|
||||
@@ -79,13 +83,13 @@ export class OAuthLoginFlowService {
|
||||
void options.runtime.login(options.providerId, "oauth", interaction)
|
||||
.then(() => {
|
||||
if (!this.isCurrentRunning(record)) return;
|
||||
record.pending = undefined;
|
||||
this.clearPending(record);
|
||||
this.markTerminal(record, { ...withoutInteraction(record.state), status: "complete", progress: [...record.state.progress, "Login complete"] });
|
||||
options.onComplete?.();
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (this.flows.get(record.flowId) !== record) return;
|
||||
record.pending = undefined;
|
||||
this.clearPending(record);
|
||||
if (record.state.status !== "running") return;
|
||||
this.markTerminal(record, { ...withoutInteraction(record.state), status: "error", error: error instanceof Error ? error.message : String(error) });
|
||||
});
|
||||
@@ -106,7 +110,8 @@ export class OAuthLoginFlowService {
|
||||
const pending = record.pending;
|
||||
if (pending?.requestId !== requestId) throw new Error("OAuth login request expired");
|
||||
if (!pending.allowEmpty && value.trim() === "") throw new Error("A value is required");
|
||||
record.pending = undefined;
|
||||
if (pending.allowedValues !== undefined && !pending.allowedValues.has(value)) throw new Error("Invalid OAuth selection");
|
||||
this.clearPending(record);
|
||||
this.updateState(record, withoutInteraction(record.state));
|
||||
pending.resolve(value);
|
||||
return cloneState(record.state);
|
||||
@@ -117,8 +122,7 @@ export class OAuthLoginFlowService {
|
||||
if (record === undefined) throw new Error("OAuth login flow not found");
|
||||
if (record.state.status === "running") {
|
||||
record.abort.abort();
|
||||
const pending = record.pending;
|
||||
record.pending = undefined;
|
||||
const pending = this.clearPending(record);
|
||||
this.markTerminal(record, { ...withoutInteraction(record.state), status: "cancelled", error: "Login cancelled" });
|
||||
pending?.reject(new Error("Login cancelled"));
|
||||
}
|
||||
@@ -129,25 +133,15 @@ export class OAuthLoginFlowService {
|
||||
for (const record of this.flows.values()) {
|
||||
this.clearTimer(record);
|
||||
record.abort.abort();
|
||||
const pending = record.pending;
|
||||
record.pending = undefined;
|
||||
const pending = this.clearPending(record);
|
||||
pending?.reject(new Error("Login cancelled"));
|
||||
}
|
||||
this.flows.clear();
|
||||
}
|
||||
|
||||
private handlePrompt(record: OAuthFlowRecord, prompt: AuthPrompt): Promise<string> {
|
||||
if (prompt.type === "select") {
|
||||
return this.waitForSelect(record, prompt.message, prompt.options, prompt.signal);
|
||||
}
|
||||
// `manual_code` is the paste-back path for callback-server flows; text/secret
|
||||
// are ordinary interactive entry. Both map to the single web-UI prompt shape.
|
||||
const kind = prompt.type === "manual_code" ? "manual" : "prompt";
|
||||
return this.waitForPrompt(record, {
|
||||
message: prompt.message,
|
||||
...(prompt.placeholder === undefined ? {} : { placeholder: prompt.placeholder }),
|
||||
...(prompt.signal === undefined ? {} : { signal: prompt.signal }),
|
||||
}, kind);
|
||||
if (prompt.type === "select") return this.waitForSelect(record, prompt);
|
||||
return this.waitForPrompt(record, prompt);
|
||||
}
|
||||
|
||||
private handleEvent(record: OAuthFlowRecord, event: AuthEvent): void {
|
||||
@@ -156,75 +150,127 @@ export class OAuthLoginFlowService {
|
||||
case "auth_url":
|
||||
this.updateState(record, { ...record.state, auth: { url: event.url, ...(event.instructions === undefined ? {} : { instructions: event.instructions }) } });
|
||||
return;
|
||||
// Device-code flows have no redirect URL; reuse the auth field so the web UI
|
||||
// shows the verification link and user code without a dedicated API shape.
|
||||
// Keep the legacy auth URL/instructions while adding structured metadata
|
||||
// that newer browsers can use during rolling sessiond upgrades.
|
||||
case "device_code":
|
||||
this.updateState(record, { ...record.state, auth: { url: event.verificationUri, instructions: `Enter code: ${event.userCode}` } });
|
||||
this.updateState(record, {
|
||||
...record.state,
|
||||
auth: {
|
||||
url: event.verificationUri,
|
||||
instructions: `Enter code: ${event.userCode}`,
|
||||
deviceCode: {
|
||||
userCode: event.userCode,
|
||||
...(event.intervalSeconds === undefined ? {} : { intervalSeconds: event.intervalSeconds }),
|
||||
...(event.expiresInSeconds === undefined ? {} : { expiresInSeconds: event.expiresInSeconds }),
|
||||
},
|
||||
},
|
||||
});
|
||||
return;
|
||||
case "info":
|
||||
this.updateState(record, {
|
||||
...record.state,
|
||||
progress: [...record.state.progress, event.message],
|
||||
info: [
|
||||
...(record.state.info ?? []),
|
||||
{
|
||||
message: event.message,
|
||||
...(event.links === undefined ? {} : {
|
||||
links: event.links.map((link) => ({
|
||||
url: link.url,
|
||||
...(link.label === undefined ? {} : { label: link.label }),
|
||||
})),
|
||||
}),
|
||||
},
|
||||
],
|
||||
});
|
||||
return;
|
||||
case "progress":
|
||||
this.updateState(record, { ...record.state, progress: [...record.state.progress, event.message] });
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
private waitForPrompt(record: OAuthFlowRecord, prompt: { message: string; placeholder?: string; signal?: AbortSignal }, kind: "prompt" | "manual"): Promise<string> {
|
||||
private waitForPrompt(record: OAuthFlowRecord, prompt: ValuePrompt): Promise<string> {
|
||||
return new Promise((resolve, reject) => {
|
||||
if (!this.isCurrentRunning(record)) {
|
||||
reject(new Error("Login cancelled"));
|
||||
return;
|
||||
}
|
||||
if (prompt.signal?.aborted === true) {
|
||||
reject(new Error("Prompt cancelled"));
|
||||
return;
|
||||
}
|
||||
const requestId = crypto.randomUUID();
|
||||
record.pending = { requestId, allowEmpty: false, resolve, reject };
|
||||
this.bindPromptSignal(record, requestId, prompt.signal);
|
||||
const pending: PendingOAuthRequest = {
|
||||
requestId,
|
||||
allowEmpty: prompt.type === "text",
|
||||
resolve,
|
||||
reject,
|
||||
};
|
||||
record.pending = pending;
|
||||
if (!this.bindPromptSignal(record, pending, prompt.signal)) return;
|
||||
const base = withoutInteraction(record.state);
|
||||
this.updateState(record, {
|
||||
...base,
|
||||
prompt: {
|
||||
requestId,
|
||||
message: prompt.message,
|
||||
kind,
|
||||
kind: prompt.type === "manual_code" ? "manual" : "prompt",
|
||||
promptType: prompt.type,
|
||||
...(prompt.type === "text" ? { allowEmpty: true } : {}),
|
||||
...(prompt.placeholder === undefined ? {} : { placeholder: prompt.placeholder }),
|
||||
},
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
private waitForSelect(record: OAuthFlowRecord, message: string, promptOptions: readonly { id: string; label: string; description?: string }[], signal?: AbortSignal): Promise<string> {
|
||||
private waitForSelect(record: OAuthFlowRecord, prompt: SelectPrompt): Promise<string> {
|
||||
return new Promise((resolve, reject) => {
|
||||
if (!this.isCurrentRunning(record)) {
|
||||
reject(new Error("Login cancelled"));
|
||||
return;
|
||||
}
|
||||
if (signal?.aborted === true) {
|
||||
reject(new Error("Prompt cancelled"));
|
||||
return;
|
||||
}
|
||||
const requestId = crypto.randomUUID();
|
||||
const options: CommandOption[] = promptOptions.map((option) => ({ value: option.id, label: option.label }));
|
||||
record.pending = { requestId, allowEmpty: true, resolve, reject };
|
||||
this.bindPromptSignal(record, requestId, signal);
|
||||
const options: CommandOption[] = prompt.options.map((option) => ({
|
||||
value: option.id,
|
||||
label: option.label,
|
||||
...(option.description === undefined ? {} : { description: option.description }),
|
||||
}));
|
||||
const pending: PendingOAuthRequest = {
|
||||
requestId,
|
||||
allowEmpty: false,
|
||||
resolve,
|
||||
reject,
|
||||
allowedValues: new Set(options.map((option) => option.value)),
|
||||
};
|
||||
record.pending = pending;
|
||||
if (!this.bindPromptSignal(record, pending, prompt.signal)) return;
|
||||
const base = withoutInteraction(record.state);
|
||||
this.updateState(record, { ...base, select: { requestId, message, options } });
|
||||
this.updateState(record, { ...base, select: { requestId, message: prompt.message, options } });
|
||||
});
|
||||
}
|
||||
|
||||
// A prompt may carry its own AbortSignal (e.g. a manual_code prompt raced
|
||||
// against a callback server). When it fires, drop just that pending request
|
||||
// and clear the interaction from state — the overall login keeps running.
|
||||
private bindPromptSignal(record: OAuthFlowRecord, requestId: string, signal?: AbortSignal): void {
|
||||
if (signal === undefined) return;
|
||||
signal.addEventListener("abort", () => {
|
||||
const pending = record.pending;
|
||||
if (pending?.requestId !== requestId) return;
|
||||
record.pending = undefined;
|
||||
private bindPromptSignal(record: OAuthFlowRecord, pending: PendingOAuthRequest, signal?: AbortSignal): boolean {
|
||||
if (signal === undefined) return true;
|
||||
const onAbort = () => {
|
||||
if (record.pending !== pending) return;
|
||||
this.clearPending(record);
|
||||
if (this.isCurrentRunning(record)) this.updateState(record, withoutInteraction(record.state));
|
||||
pending.reject(new Error("Prompt cancelled"));
|
||||
}, { once: true });
|
||||
};
|
||||
pending.cleanup = () => { signal.removeEventListener("abort", onAbort); };
|
||||
signal.addEventListener("abort", onAbort, { once: true });
|
||||
if (signal.aborted) {
|
||||
onAbort();
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
private clearPending(record: OAuthFlowRecord): PendingOAuthRequest | undefined {
|
||||
const pending = record.pending;
|
||||
record.pending = undefined;
|
||||
pending?.cleanup?.();
|
||||
return pending;
|
||||
}
|
||||
|
||||
private isCurrentRunning(record: OAuthFlowRecord): boolean {
|
||||
@@ -270,8 +316,7 @@ export class OAuthLoginFlowService {
|
||||
private expireRunningFlow(record: OAuthFlowRecord): void {
|
||||
if (!this.isCurrentRunning(record)) return;
|
||||
record.abort.abort();
|
||||
const pending = record.pending;
|
||||
record.pending = undefined;
|
||||
const pending = this.clearPending(record);
|
||||
this.markTerminal(record, { ...withoutInteraction(record.state), status: "error", error: "OAuth login flow expired" });
|
||||
pending?.reject(new Error("OAuth login flow expired"));
|
||||
}
|
||||
@@ -300,9 +345,20 @@ function cloneState(state: OAuthFlowState): OAuthFlowState {
|
||||
return {
|
||||
...state,
|
||||
progress: [...state.progress],
|
||||
...(state.auth === undefined ? {} : { auth: { ...state.auth } }),
|
||||
...(state.auth === undefined ? {} : {
|
||||
auth: {
|
||||
...state.auth,
|
||||
...(state.auth.deviceCode === undefined ? {} : { deviceCode: { ...state.auth.deviceCode } }),
|
||||
},
|
||||
}),
|
||||
...(state.prompt === undefined ? {} : { prompt: { ...state.prompt } }),
|
||||
...(state.select === undefined ? {} : { select: { ...state.select, options: state.select.options.map((option) => ({ ...option })) } }),
|
||||
...(state.info === undefined ? {} : {
|
||||
info: state.info.map((item) => ({
|
||||
...item,
|
||||
...(item.links === undefined ? {} : { links: item.links.map((link) => ({ ...link })) }),
|
||||
})),
|
||||
}),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user