Skip to content

Fix command injection in sign-in URL opener#3

Open
sterlingcrispin wants to merge 1 commit intorohunvora:mainfrom
sterlingcrispin:fix-signin-command-injection
Open

Fix command injection in sign-in URL opener#3
sterlingcrispin wants to merge 1 commit intorohunvora:mainfrom
sterlingcrispin:fix-signin-command-injection

Conversation

@sterlingcrispin
Copy link
Copy Markdown

@sterlingcrispin sterlingcrispin commented Mar 20, 2026

Summary

  • replace shell-interpolated browser launch commands with argument-safe spawn(...)
  • reject invalid or non-HTTP(S) sign-in URLs before trying to open them

Security

This fixes the client-side command injection risk in scripts/signin.ts, where the script previously passed a server-returned URL through execSync(...).

Validation

  • bundled scripts/signin.ts with esbuild to confirm the updated code parses cleanly

Copy link
Copy Markdown
Owner

@rohunvora rohunvora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the obvious shell interpolation on macOS/Linux, but the Windows branch is still passing the server-returned URL through cmd /c start in scripts/signin.ts. That means untrusted input still reaches cmd.exe, which treats characters like &, |, and ^ as command operators, so the command-injection issue remains on Windows.

The new http:/https: check is a good guardrail, but it does not neutralize cmd metacharacters. I would avoid cmd.exe entirely for the Windows opener path, or escape the full Windows command metacharacter set before invoking start.

@rohunvora
Copy link
Copy Markdown
Owner

Thanks for the contribution. I reproduced the issue, but I've landed a maintainer patch that fixes the browser-opening path without routing the returned URL through a shell. That supersedes this PR's goal and avoids the remaining Windows issue, so I won't be merging this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants