Get this on every PR automatically. Webhook fires, AI reviews, you approve.
Install on stripe/link-cli →
// reviewed by pulllight
PullLight found 5 issues
2 high
3 medium
// convert to your repo
PullLight caught 5 2 high, 3 medium on this PR.
Install on stripe/link-cli to get this on every PR automatically — 60 seconds.
Findings
5 issues
high
injection
The `message` string is interpolated directly into an HTML response without escaping: `res.end(`<html><body><p>${message}</p></body></html>`)`. Although `message` comes from the hardcoded `STATUS_MESSAGES` map (not user input), the `raw` value from `url.searchParams.get('status')` is used as the map key. If `STATUS_MESSAGES` ever gains a key whose value contains HTML, or if this pattern is copied elsewhere, it becomes an XSS vector. More critically, the redirect URI itself is attacker-controlled (the browser navigates to this server), so a crafted `?status=` value that falls through to the default message path is safe today but the pattern is fragile. The immediate concern is that `raw` is reflected indirectly — if `STATUS_MESSAGES[raw]` is undefined, the fallback is used, which is fine, but if a future entry contains `<script>` it would be injected. Harden by HTML-escaping `message` before writing it into the response.
// packages/cli/src/utils/local-callback-server.ts:62
high
resource-leak
When `server.listen` succeeds and `resolve()` is called, the server stays open indefinitely until the caller invokes `close()`. However, if `waitForCallback` is never called (e.g. the component unmounts before reaching that line), or if the caller's cleanup path fails to call `close()`, the HTTP server is never closed and the port is held open for the lifetime of the process. The `close` handle is stored in a local variable and passed back, but in `create.tsx` the `finally` block calls `close?.()` — if an exception is thrown *before* `close` is assigned (between `tryStartCallbackServer()` returning and `close = server.close` executing), the server leaks. Specifically in `request-approval.tsx` line ~35: `close = server?.close ?? null` is assigned after `await tryStartCallbackServer()` returns, so there is no window there, but in `create.tsx` the assignment `if (server) close = server.close` is also safe. The real leak is: if the component unmounts after the server starts but before `waitForCallback` resolves, the cleanup `return () => { cancelled = true; close?.(); }` does call `close()`, which is correct. However `server.close()` only stops accepting new connections — it does not forcibly close the already-open keep-alive connection from the browser redirect, so the server may not actually close. Consider calling `server.closeAllConnections()` (Node 18.2+) or tracking and destroying open sockets.
// packages/cli/src/utils/local-callback-server.ts:80
medium
missing-await
`onTerminal` is called synchronously but `onComplete` is called via `setTimeout` immediately after, without waiting for any state updates from `onTerminal` to flush. In React, `onTerminal` calls `setStatus(...)` and `setResult(...)`, but since these are batched, the `onComplete` callback fires after a fixed delay regardless of whether the terminal UI has rendered. This is the existing pattern so it's not a regression, but note that `onTerminal` in `create.tsx` does NOT call `setTimeout(() => onComplete(final), DISPLAY_DELAY_MS)` — that is done by the hook itself at line 76. This means `onComplete` is always called from the polling path, even for `denied`/`expired` states, which may be intentional but differs from the callback-server path in `create.tsx` where `onComplete` is only called for non-error terminal states (lines 113-125 of the diff). Verify this asymmetry is intentional.
// packages/cli/src/commands/spend-request/use-approval-polling.ts:76
medium
logic
After a successful callback-server flow with `callbackStatus === 'denied'` or `callbackStatus === 'expired'`, `onComplete(final)` is called via `setTimeout`. But for the same terminal states reached via the polling fallback path (`useApprovalPolling` → `onTerminal`), `onComplete` is also called (from the hook). However, in the `denied`/`expired` UI branches (lines ~193-232 of the diff), there is no `setTimeout(() => onComplete(...), DISPLAY_DELAY_MS)` call — the component renders the denied/expired view and stays there. The user is stuck unless `onComplete` is eventually called. Confirm that `onComplete` is always invoked for every terminal state to avoid the CLI hanging.
// packages/cli/src/commands/spend-request/create.tsx:113
medium
race-condition
`resolveCallback` is assigned inside the `new Promise` executor (synchronously), but the server request handler calls `resolveCallback?.({ status })`. If multiple browser requests arrive (e.g. user refreshes the redirect page), `resolveCallback` is called multiple times. The first call resolves the promise, subsequent calls are no-ops on the already-resolved promise, which is safe for the promise itself. However, the server continues to accept and respond to requests after the first callback, and each response writes to `res` — this is fine. The real issue is that after `resolveCallback` fires, the server is still listening and any subsequent request will call `resolveCallback?.()` again (it's not nulled out after first use). While Promise resolution is idempotent, consider setting `resolveCallback = undefined` after first invocation to make the intent explicit and avoid confusion.
// packages/cli/src/utils/local-callback-server.ts:57