Skip to content

Commit 9376fbc

Browse files
mrdailey99claude
andcommitted
fix(auth): address PR review comments on openBrowser
- Add explicit ChildProcess type to suppress implicit-any lint warning - Add no-op error handler to prevent unhandled EventEmitter crash on ENOENT - Extract getBrowserCommand() pure function for testability - Add unit tests covering all platforms and URL passthrough Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 1f975fb commit 9376fbc

2 files changed

Lines changed: 61 additions & 13 deletions

File tree

src/services/auth/loginFlow.ts

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import crypto from 'node:crypto';
1010
import http from 'node:http';
1111
import https from 'node:https';
12-
import { spawn } from 'node:child_process';
12+
import { spawn, type ChildProcess } from 'node:child_process';
1313
import { URL } from 'node:url';
1414

1515
// All three ports must be pre-registered in the Cognito App Client.
@@ -76,24 +76,32 @@ function isPortFree(port: number): Promise<boolean> {
7676
* Open a URL in the system browser. The URL is passed as an argument — not
7777
* interpolated into a shell string — to avoid command injection.
7878
*/
79-
export function openBrowser(url: string): void {
80-
// detached:true + stdio:'ignore' + unref() is the standard Node.js pattern for
81-
// fire-and-forget child processes — the event loop will not wait for them to exit.
82-
const spawnOpts = { detached: true, stdio: 'ignore' as const };
83-
let child;
84-
switch (process.platform) {
79+
/**
80+
* Return the platform-specific command and argument list for opening a URL
81+
* in the system browser. Exported so tests can assert the correct command is
82+
* chosen for each platform without actually spawning a process.
83+
*/
84+
export function getBrowserCommand(url: string, platform: NodeJS.Platform = process.platform): { cmd: string; args: string[] } {
85+
switch (platform) {
8586
case 'darwin':
86-
child = spawn('open', [url], spawnOpts);
87-
break;
87+
return { cmd: 'open', args: [url] };
8888
case 'win32':
8989
// Pass the URL via $args[0] so it is never interpolated into the -Command
9090
// string — avoids quote-breaking and injection risk from special characters.
91-
child = spawn('powershell.exe', ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url], spawnOpts);
92-
break;
91+
return { cmd: 'powershell.exe', args: ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url] };
9392
default:
94-
child = spawn('xdg-open', [url], spawnOpts);
95-
break;
93+
return { cmd: 'xdg-open', args: [url] };
9694
}
95+
}
96+
97+
export function openBrowser(url: string): void {
98+
// detached:true + stdio:'ignore' + unref() is the standard Node.js pattern for
99+
// fire-and-forget child processes — the event loop will not wait for them to exit.
100+
const { cmd, args } = getBrowserCommand(url);
101+
const child: ChildProcess = spawn(cmd, args, { detached: true, stdio: 'ignore' });
102+
// Suppress unhandled-error crashes if the browser executable is not found.
103+
// The login URL is already printed to the terminal so the user can open it manually.
104+
child.on('error', () => { /* intentional no-op */ });
97105
child.unref();
98106
}
99107

test/unit/commands/provar/auth/login.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { describe, it, beforeEach, afterEach } from 'mocha';
1515
import sinon from 'sinon';
1616
import {
1717
generatePkce,
18+
getBrowserCommand,
1819
loginFlowClient,
1920
type CognitoTokens,
2021
CALLBACK_PORTS,
@@ -59,6 +60,45 @@ function restoreHome(): void {
5960
fs.rmSync(tempDir, { recursive: true, force: true });
6061
}
6162

63+
// ── getBrowserCommand ─────────────────────────────────────────────────────────
64+
65+
describe('getBrowserCommand', () => {
66+
const url = 'https://example.com/login?code=abc&state=xyz';
67+
68+
it('uses "open" on macOS', () => {
69+
const { cmd, args } = getBrowserCommand(url, 'darwin');
70+
assert.equal(cmd, 'open');
71+
assert.deepEqual(args, [url]);
72+
});
73+
74+
it('uses powershell.exe with Start-Process on Windows', () => {
75+
const { cmd, args } = getBrowserCommand(url, 'win32');
76+
assert.equal(cmd, 'powershell.exe');
77+
assert.ok(args.includes('-NoProfile'), 'should pass -NoProfile');
78+
assert.ok(args.includes('Start-Process $args[0]') || args.join(' ').includes('Start-Process'), 'should use Start-Process');
79+
// URL is passed as a separate arg — never interpolated into the command string
80+
assert.equal(args[args.length - 1], url, 'URL must be the last argument');
81+
});
82+
83+
it('uses "xdg-open" on Linux', () => {
84+
const { cmd, args } = getBrowserCommand(url, 'linux');
85+
assert.equal(cmd, 'xdg-open');
86+
assert.deepEqual(args, [url]);
87+
});
88+
89+
it('uses "xdg-open" for unknown platforms', () => {
90+
const { cmd } = getBrowserCommand(url, 'freebsd');
91+
assert.equal(cmd, 'xdg-open');
92+
});
93+
94+
it('includes the full URL in args for all platforms', () => {
95+
for (const platform of ['darwin', 'win32', 'linux'] as NodeJS.Platform[]) {
96+
const { args } = getBrowserCommand(url, platform);
97+
assert.ok(args.includes(url), `URL must appear in args for platform ${platform}`);
98+
}
99+
});
100+
});
101+
62102
// ── generatePkce ──────────────────────────────────────────────────────────────
63103

64104
describe('generatePkce', () => {

0 commit comments

Comments
 (0)