-
Notifications
You must be signed in to change notification settings - Fork 2
Develop #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Develop #119
Changes from 5 commits
c3e4245
17de177
1f975fb
9376fbc
fc43ff8
41dc2de
3a533e7
6a10625
57812b1
31e948e
b8f3c87
410dcc0
568fb7c
7dfee1a
9704976
b230cd6
e1c3905
e6c4b9b
09bb3ae
51f52be
632c561
72650c2
4fe68be
f8737fe
65d4867
cbccb64
58f1ca9
ff87107
ea7d071
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| import crypto from 'node:crypto'; | ||
| import http from 'node:http'; | ||
| import https from 'node:https'; | ||
| import { execFile } from 'node:child_process'; | ||
| import { spawn, type ChildProcess } from 'node:child_process'; | ||
| import { URL } from 'node:url'; | ||
|
|
||
| // All three ports must be pre-registered in the Cognito App Client. | ||
|
|
@@ -76,21 +76,35 @@ function isPortFree(port: number): Promise<boolean> { | |
| * Open a URL in the system browser. The URL is passed as an argument — not | ||
| * interpolated into a shell string — to avoid command injection. | ||
| */ | ||
| export function openBrowser(url: string): void { | ||
| switch (process.platform) { | ||
| /** | ||
| * Return the platform-specific command and argument list for opening a URL | ||
| * in the system browser. Exported so tests can assert the correct command is | ||
| * chosen for each platform without actually spawning a process. | ||
| */ | ||
| export function getBrowserCommand(url: string, platform: NodeJS.Platform = process.platform): { cmd: string; args: string[] } { | ||
| switch (platform) { | ||
| case 'darwin': | ||
| execFile('open', [url]); | ||
| break; | ||
| return { cmd: 'open', args: [url] }; | ||
| case 'win32': | ||
| // Pass the URL via $args[0] so it is never interpolated into the -Command | ||
| // string — avoids quote-breaking and injection risk from special characters. | ||
| execFile('powershell.exe', ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url]); | ||
| break; | ||
| return { cmd: 'powershell.exe', args: ['-NoProfile', '-Command', 'Start-Process $args[0]', '-args', url] }; | ||
| default: | ||
| execFile('xdg-open', [url]); | ||
| return { cmd: 'xdg-open', args: [url] }; | ||
| } | ||
| } | ||
|
|
||
| export function openBrowser(url: string): void { | ||
| // detached:true + stdio:'ignore' + unref() is the standard Node.js pattern for | ||
| // fire-and-forget child processes — the event loop will not wait for them to exit. | ||
| const { cmd, args } = getBrowserCommand(url); | ||
| const child: ChildProcess = spawn(cmd, args, { detached: true, stdio: 'ignore' }); | ||
| // Suppress unhandled-error crashes if the browser executable is not found. | ||
| // The login URL is already printed to the terminal so the user can open it manually. | ||
| child.on('error', () => { /* intentional no-op */ }); | ||
| child.unref(); | ||
| } | ||
|
|
||
| // ── Localhost callback server ───────────────────────────────────────────────── | ||
|
|
||
| /** | ||
|
|
@@ -107,26 +121,33 @@ export function listenForCallback(port: number, expectedState?: string): Promise | |
| const callbackState = parsed.searchParams.get('state'); | ||
|
|
||
| if (expectedState && callbackState !== expectedState) { | ||
| res.writeHead(400, { 'Content-Type': 'text/html; charset=utf-8' }); | ||
| res.writeHead(400, { 'Content-Type': 'text/html; charset=utf-8', Connection: 'close' }); | ||
| res.end( | ||
| '<html><body style="font-family:sans-serif;padding:2rem;max-width:480px">' + | ||
| '<h2 style="color:#c23934">Authentication failed</h2>' + | ||
| '<p>Invalid state parameter — possible CSRF attack. Please try again.</p>' + | ||
| '</body></html>' | ||
| ); | ||
| server.close(); | ||
| server.closeAllConnections?.(); | ||
| reject(new Error('OAuth callback state mismatch — possible CSRF. Try again.')); | ||
|
||
| return; | ||
| } | ||
|
|
||
| res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); | ||
| // 'Connection: close' tells the browser to close the TCP connection after | ||
| // this response so server.close() has no lingering keep-alive sockets to | ||
| // wait for, allowing the Node.js event loop to exit promptly. | ||
| res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8', Connection: 'close' }); | ||
| res.end( | ||
| '<html><body style="font-family:sans-serif;padding:2rem;max-width:480px">' + | ||
| '<h2 style="color:#0070d2">Authentication complete</h2>' + | ||
| '<p>You can close this tab and return to the terminal.</p>' + | ||
| '</body></html>' | ||
| ); | ||
| server.close(); | ||
| // Destroy any sockets that are still open (e.g. a browser that ignores | ||
| // the Connection:close header). Requires Node 18.2+. | ||
| server.closeAllConnections?.(); | ||
|
||
|
|
||
| if (code) { | ||
| resolve(code); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two adjacent JSDoc blocks here; the first one (“Open a URL in the system browser…”) is now orphaned and no longer documents any symbol. This can confuse generated docs and makes it unclear which function the injection-safety note applies to. Consider merging the two blocks or moving the injection-safety text onto the
openBrowserJSDoc and keeping only one JSDoc block per export.