Skip to content

Commit 63bd41b

Browse files
Brooooooklynclaude
andcommitted
fix(cli): finalize browser callback in request path; rethrow real install retry errors
F3: awaitBrowserCallback now runs a generic finalize() hook inside the request handler before the success page. On finalize success it shows the success page and resolves with the result; on finalize throw it shows the failure page and rejects. This restores base runLogin behavior (exchange in the request path, failure page on exchange failure) and makes `void github link` show success only after completeGithubLink succeeds. runLogin and runGithubLink refactored onto the hook. F4: runInstall's post-link retry no longer swallows all errors — it only keeps the actionable link/install guidance when the retry is still a 403 github_identity_required, and rethrows every other failure (401/426/500/other). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 025cc4b commit 63bd41b

5 files changed

Lines changed: 217 additions & 47 deletions

File tree

‎packages/void/src/cli/auth.ts‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -267,9 +267,12 @@ export function openBrowser(
267267
* authorize URL, and wait for the redirect back to `http://localhost:PORT/callback`.
268268
*
269269
* The server validates the `state` it appended to the callback URL against the
270-
* incoming request, renders a success/error HTML page, and resolves with the
271-
* callback request's search params (the caller reads `code` / `link_code` etc.).
272-
* The server is always closed before settling.
270+
* incoming request, then runs the caller's `finalize` hook INSIDE the request
271+
* handler (so the browser only ever sees the success page after the underlying
272+
* exchange/link actually succeeded). On finalize success it renders the success
273+
* HTML and resolves with finalize's return value; on finalize failure it
274+
* renders the same "could not complete" failure page the login flow uses and
275+
* rejects with the thrown error. The server is always closed before settling.
273276
*
274277
* `onUrl` is invoked with the authorize URL once the local server is listening,
275278
* so callers can show the URL to the user as a manual fallback when the browser
@@ -278,11 +281,18 @@ export function openBrowser(
278281
*
279282
* `execFile` is injectable so tests can avoid launching a real browser.
280283
*/
281-
export async function awaitBrowserCallback(options: {
284+
export async function awaitBrowserCallback<T>(options: {
282285
/** Caller-generated; appended to the callback URL and validated on the incoming request. */
283286
state: string;
284287
/** Given `http://localhost:PORT/callback?state=…`, return the URL to open. */
285288
buildAuthUrl: (callback: URL) => Promise<string> | string;
289+
/**
290+
* Runs INSIDE the request handler, after state/error/validate checks pass and
291+
* BEFORE the success page is rendered. Resolve to settle the outer promise
292+
* with this value (success page shown); throw to render the failure page and
293+
* reject the outer promise with the thrown error.
294+
*/
295+
finalize: (params: URLSearchParams) => Promise<T>;
286296
/** Map an `?error=…` query to a user message (return null to ignore). */
287297
describeError?: (params: URLSearchParams) => string | null;
288298
/**
@@ -298,13 +308,13 @@ export async function awaitBrowserCallback(options: {
298308
timeoutMs?: number;
299309
/** Injectable for tests; defaults to the real `node:child_process` `execFile`. */
300310
execFile?: typeof import('node:child_process').execFile;
301-
}): Promise<URLSearchParams> {
311+
}): Promise<T> {
302312
const http = await import('node:http');
303-
const { state, buildAuthUrl, describeError, validate, onUrl } = options;
313+
const { state, buildAuthUrl, finalize, describeError, validate, onUrl } = options;
304314
const timeoutMs = options.timeoutMs ?? 5 * 60 * 1000;
305315
const execFile = options.execFile ?? (await import('node:child_process')).execFile;
306316

307-
return new Promise<URLSearchParams>((resolve, reject) => {
317+
return new Promise<T>((resolve, reject) => {
308318
let settled = false;
309319

310320
const fail = (error: Error): void => {
@@ -316,16 +326,16 @@ export async function awaitBrowserCallback(options: {
316326
reject(error);
317327
};
318328

319-
const succeed = (params: URLSearchParams): void => {
329+
const succeed = (result: T): void => {
320330
if (settled) {
321331
return;
322332
}
323333
settled = true;
324334
server.close();
325-
resolve(params);
335+
resolve(result);
326336
};
327337

328-
const server = http.createServer((req, res) => {
338+
const server = http.createServer(async (req, res) => {
329339
const url = new URL(req.url!, `http://localhost`);
330340
if (url.pathname !== '/callback') {
331341
res.writeHead(404);
@@ -357,9 +367,30 @@ export async function awaitBrowserCallback(options: {
357367
return;
358368
}
359369

370+
// Finalize INSIDE the request path so the browser only sees the success
371+
// page once the underlying exchange/link succeeds. A finalize failure
372+
// (expired code, API error, 409 conflict, etc.) renders the failure page
373+
// and rejects — matching the base login flow's exchange-in-request-path
374+
// behavior. Guard against the timeout racing the in-flight finalize.
375+
let result: T;
376+
try {
377+
result = await finalize(url.searchParams);
378+
} catch (err) {
379+
if (settled) {
380+
return;
381+
}
382+
res.writeHead(400, { 'Content-Type': 'text/html' });
383+
res.end(page('Login failed', 'Could not complete login. Please try again.'));
384+
fail(err instanceof Error ? err : new Error('auth: Failed to complete login.'));
385+
return;
386+
}
387+
388+
if (settled) {
389+
return;
390+
}
360391
res.writeHead(200, { 'Content-Type': 'text/html' });
361392
res.end(page('Login successful!', 'You can close this tab and return to the terminal.'));
362-
succeed(url.searchParams);
393+
succeed(result);
363394
});
364395

365396
// Close immediately after callback — don't wait for keep-alive timeout
@@ -416,7 +447,9 @@ export async function runLogin(
416447
const codeVerifier = createCodeVerifier();
417448
const codeChallenge = createCodeChallenge(codeVerifier);
418449

419-
const params = await awaitBrowserCallback({
450+
// The exchange runs INSIDE the request handler (via finalize), so an exchange
451+
// failure renders the failure page — exactly as the base login flow did.
452+
return awaitBrowserCallback<string>({
420453
state,
421454
execFile,
422455
onUrl,
@@ -444,8 +477,6 @@ export async function runLogin(
444477
}
445478
return null;
446479
},
480+
finalize: (params) => exchangeLoginCode(apiUrl, params.get('code')!, state, codeVerifier),
447481
});
448-
449-
const code = params.get('code')!;
450-
return exchangeLoginCode(apiUrl, code, state, codeVerifier);
451482
}

‎packages/void/src/cli/github-cmd.ts‎

Lines changed: 35 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,21 @@ export async function runInstall(
152152
await linkIdentity(root, client);
153153
try {
154154
({ url } = await client.getGithubInstallUrl());
155-
} catch {
156-
log.error(
157-
'Could not start the GitHub App install after linking. Try `void github link`, then `void github install` again.',
158-
);
159-
process.exit(1);
155+
} catch (retryErr) {
156+
// Only the SAME identity-required 403 means linking didn't take — show
157+
// the actionable link/install guidance. Any other failure (401, 426,
158+
// 500, a different 403) is the real error and must surface normally.
159+
if (
160+
retryErr instanceof PlatformApiError &&
161+
retryErr.status === 403 &&
162+
retryErr.code === 'github_identity_required'
163+
) {
164+
log.error(
165+
'Could not start the GitHub App install after linking. Try `void github link`, then `void github install` again.',
166+
);
167+
process.exit(1);
168+
}
169+
throw retryErr;
160170
}
161171
} else {
162172
throw err;
@@ -189,25 +199,28 @@ export async function runGithubLink(root: string, client: PlatformClient): Promi
189199
const challenge = createCodeChallenge(verifier);
190200
const state = randomBytes(32).toString('hex');
191201

192-
const params = await awaitBrowserCallback({
193-
state,
194-
buildAuthUrl: async (callback) => {
195-
callback.searchParams.set('state', state);
196-
const { url } = await client.getGithubLinkUrl(callback.href, challenge);
197-
return url;
198-
},
199-
describeError: (p) => (p.get('error') ? 'GitHub authorization failed.' : null),
200-
onUrl: (url) => log.message(`Open this URL to link your GitHub account:\n${pc.cyan(url)}`),
201-
});
202-
203-
const linkCode = params.get('link_code');
204-
if (!linkCode) {
205-
throw new Error('github: link callback did not return a link code.');
206-
}
207-
202+
// completeGithubLink runs INSIDE the request handler (via finalize), so the
203+
// browser only shows the success page after the link actually succeeds; a 409
204+
// conflict rejects out of awaitBrowserCallback and is mapped below.
208205
let status: 'linked' | 'already_linked';
209206
try {
210-
({ status } = await client.completeGithubLink(linkCode, verifier));
207+
({ status } = await awaitBrowserCallback<{ status: 'linked' | 'already_linked' }>({
208+
state,
209+
buildAuthUrl: async (callback) => {
210+
callback.searchParams.set('state', state);
211+
const { url } = await client.getGithubLinkUrl(callback.href, challenge);
212+
return url;
213+
},
214+
describeError: (p) => (p.get('error') ? 'GitHub authorization failed.' : null),
215+
onUrl: (url) => log.message(`Open this URL to link your GitHub account:\n${pc.cyan(url)}`),
216+
finalize: async (params) => {
217+
const linkCode = params.get('link_code');
218+
if (!linkCode) {
219+
throw new Error('github: link callback did not return a link code.');
220+
}
221+
return client.completeGithubLink(linkCode, verifier);
222+
},
223+
}));
211224
} catch (err) {
212225
if (err instanceof PlatformApiError && err.code === 'github_already_linked') {
213226
log.error('Your Void account is already linked to a different GitHub account.');

‎packages/void/test/unit/auth-callback.test.ts‎

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,12 @@ const noopExecFile = vi.fn() as unknown as typeof import('node:child_process').e
1010
* (built from the localhost callback). Returns the callback URL so the test can
1111
* drive the localhost server directly.
1212
*/
13-
function startCallback(options?: {
13+
function startCallback<T = URLSearchParams>(options?: {
1414
describeError?: (params: URLSearchParams) => string | null;
15+
finalize?: (params: URLSearchParams) => Promise<T>;
1516
state?: string;
1617
}): {
17-
resultPromise: Promise<URLSearchParams>;
18+
resultPromise: Promise<T>;
1819
callbackPromise: Promise<URL>;
1920
state: string;
2021
} {
@@ -24,10 +25,14 @@ function startCallback(options?: {
2425
resolveCallback = resolve;
2526
});
2627

27-
const resultPromise = awaitBrowserCallback({
28+
const finalize =
29+
options?.finalize ?? ((params: URLSearchParams) => Promise.resolve(params as unknown as T));
30+
31+
const resultPromise = awaitBrowserCallback<T>({
2832
state,
2933
execFile: noopExecFile,
3034
describeError: options?.describeError,
35+
finalize,
3136
buildAuthUrl: (callback) => {
3237
resolveCallback(callback);
3338
return 'https://example.test/authorize';
@@ -38,8 +43,11 @@ function startCallback(options?: {
3843
}
3944

4045
describe('awaitBrowserCallback', () => {
41-
it('resolves with the callback params on a valid /callback request', async () => {
42-
const { resultPromise, callbackPromise, state } = startCallback();
46+
it('runs finalize in the request path and resolves with its return value', async () => {
47+
const finalize = vi.fn(async (params: URLSearchParams) => ({
48+
linked: params.get('link_code'),
49+
}));
50+
const { resultPromise, callbackPromise, state } = startCallback({ finalize });
4351
const callback = await callbackPromise;
4452

4553
// The helper appends the caller-supplied state to the callback URL.
@@ -54,13 +62,45 @@ describe('awaitBrowserCallback', () => {
5462
const res = await fetch(hit);
5563
expect(res.status).toBe(200);
5664

57-
const params = await resultPromise;
58-
expect(params.get('link_code')).toBe('abc');
59-
expect(params.get('state')).toBe(state);
65+
// finalize ran inside the request handler with the callback params, and the
66+
// outer promise resolves with finalize's return value.
67+
expect(finalize).toHaveBeenCalledTimes(1);
68+
expect(finalize.mock.calls[0]![0].get('link_code')).toBe('abc');
69+
expect(finalize.mock.calls[0]![0].get('state')).toBe(state);
70+
71+
await expect(resultPromise).resolves.toEqual({ linked: 'abc' });
6072
});
6173

62-
it('returns 400 and keeps listening on a state mismatch (does not resolve)', async () => {
63-
const { resultPromise, callbackPromise, state } = startCallback();
74+
it('renders the failure page and rejects when finalize throws (F3 regression pin)', async () => {
75+
const finalizeError = new Error('exchange blew up');
76+
const finalize = vi.fn(async () => {
77+
throw finalizeError;
78+
});
79+
const { resultPromise, callbackPromise, state } = startCallback({ finalize });
80+
const callback = await callbackPromise;
81+
82+
// Attach the rejection assertion before triggering the request so the
83+
// handler is in place when the promise settles (no unhandled rejection).
84+
const rejection = expect(resultPromise).rejects.toBe(finalizeError);
85+
86+
const hit = new URL(callback);
87+
hit.searchParams.set('state', state);
88+
hit.searchParams.set('link_code', 'abc');
89+
90+
const res = await fetch(hit);
91+
// The browser must NOT see the success page when finalize fails.
92+
expect(res.status).toBe(400);
93+
const body = await res.text();
94+
expect(body).not.toContain('successful');
95+
expect(body).toContain('Could not complete');
96+
97+
expect(finalize).toHaveBeenCalledTimes(1);
98+
await rejection;
99+
});
100+
101+
it('returns 400 and keeps listening on a state mismatch (does not call finalize)', async () => {
102+
const finalize = vi.fn(async (params: URLSearchParams) => params);
103+
const { resultPromise, callbackPromise, state } = startCallback({ finalize });
64104
const callback = await callbackPromise;
65105

66106
const bad = new URL(callback);
@@ -70,6 +110,9 @@ describe('awaitBrowserCallback', () => {
70110
const badRes = await fetch(bad);
71111
expect(badRes.status).toBe(400);
72112

113+
// A state mismatch is rejected before finalize runs.
114+
expect(finalize).not.toHaveBeenCalled();
115+
73116
// The genuine callback still completes — the server stayed open.
74117
const good = new URL(callback);
75118
good.searchParams.set('state', state);
@@ -79,6 +122,7 @@ describe('awaitBrowserCallback', () => {
79122
expect(goodRes.status).toBe(200);
80123

81124
const params = await resultPromise;
125+
expect(finalize).toHaveBeenCalledTimes(1);
82126
expect(params.get('link_code')).toBe('real');
83127
});
84128

@@ -107,6 +151,7 @@ describe('awaitBrowserCallback', () => {
107151
state: 'timeout-state',
108152
execFile: noopExecFile,
109153
timeoutMs: 10,
154+
finalize: async (params) => params,
110155
buildAuthUrl: (callback) =>
111156
`https://example.test/authorize?cb=${encodeURIComponent(callback.href)}`,
112157
});
@@ -124,6 +169,7 @@ describe('awaitBrowserCallback', () => {
124169
const resultPromise = awaitBrowserCallback({
125170
state: 's',
126171
execFile,
172+
finalize: async (params) => params,
127173
buildAuthUrl: (callback) => {
128174
resolveCallback(callback);
129175
return 'https://example.test/authorize';

‎packages/void/test/unit/auth-login.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,46 @@ describe('runLogin', () => {
138138
await expect(loginPromise).resolves.toBe('real-token');
139139
});
140140

141+
it('shows the failure page and throws when the code exchange fails (F3 regression)', async () => {
142+
// Override fetch so the localhost callback still hits the real server, but
143+
// the API token exchange returns a non-ok response (expired code, etc.).
144+
vi.stubGlobal('fetch', async (input: RequestInfo | URL, init?: RequestInit) => {
145+
const url =
146+
input instanceof Request ? input.url : input instanceof URL ? input.href : input.toString();
147+
if (url.startsWith('http://localhost:')) {
148+
return originalFetch(input, init);
149+
}
150+
return new Response('expired', { status: 400 });
151+
});
152+
153+
let resolveLoginUrl!: (url: string) => void;
154+
const loginUrlPromise = new Promise<string>((resolve) => {
155+
resolveLoginUrl = resolve;
156+
});
157+
158+
const loginPromise = runLogin('/project', 'github', resolveLoginUrl);
159+
// Attach the rejection assertion up front so the exchange failure doesn't
160+
// surface as an unhandled rejection.
161+
const rejection = expect(loginPromise).rejects.toThrow(/Failed to exchange login code/);
162+
163+
const loginUrl = new URL(await loginUrlPromise);
164+
const redirect = loginUrl.searchParams.get('redirect');
165+
expect(redirect).toBeTruthy();
166+
167+
const validUrl = new URL(redirect!);
168+
validUrl.searchParams.set('code', 'real-code');
169+
170+
const validResponse = await fetch(validUrl);
171+
// The exchange ran INSIDE the request handler and failed → failure page,
172+
// never the success page.
173+
expect(validResponse.status).toBe(400);
174+
const body = await validResponse.text();
175+
expect(body).not.toContain('successful');
176+
expect(body).toContain('Could not complete');
177+
178+
await rejection;
179+
});
180+
141181
it('sends Cloudflare Access headers when exchanging the login code', async () => {
142182
process.env.CF_ACCESS_TOKEN = 'cf-access-token';
143183

0 commit comments

Comments
 (0)