fix: command injection in browser open during login

[thebenignhacker]

Summary

• Replace child_process.exec() with child_process.spawn() in client.mjs to prevent shell metacharacter injection via crafted server URLs during the CLI login flow

Context

Flagged by automated PR review on #22. A crafted server URL like https://localhost$(whoami) could execute arbitrary commands when passed through exec() with string interpolation.

Test plan

• JS SDK: 244/244 tests pass
• spawn() with array args prevents shell interpretation of URL contents

[github-actions]

Claude finished @abdelsfane's task —— View job

---

Security Review ✅

• Read repository setup instructions (CLAUDE.md)
• Read changed file and understand the fix
• Verify command injection fix is correct
• Check for other security vulnerabilities
• Trace through logic with concrete inputs
• Post comprehensive review findings

---

Findings

✅ Command Injection Fix (Item 9) - CORRECT

File: sdk/javascript/lib/client.mjs:128

The PR correctly fixes the command injection vulnerability:

• Before: m.exec(\${cmd} "${authUrl}"`)` - vulnerable to shell metacharacter injection
• After: m.spawn(cmd, [authUrl], { stdio: 'ignore', detached: true }).unref() - safe array-based args

Attack vector mitigated: A malicious URL like https://localhost$(whoami) would execute arbitrary commands via exec() with string interpolation. The fix uses spawn() with array arguments, which passes the URL as a single argument without shell interpretation.

Trace verification:

1. Input: https://localhost$(whoami) → validateServerUrl() passes (valid URL format)
2. Old code: exec("open \"https://localhost$(whoami)\"") → shell executes $(whoami) ❌
3. New code: spawn("open", ["https://localhost$(whoami)"]) → treats entire string as literal URL argument ✅

🟡 SSRF Protection Gap (Item 6)

File: sdk/javascript/lib/client.mjs:18-23

The SSRF blocklist has coverage gaps:

const BLOCKED_HOSTS = [
  '169.254.169.254',            // AWS metadata
  'metadata.google.internal',   // GCP metadata
  '100.100.100.200',            // Alibaba metadata
  'fd00::',                     // IPv6 link-local
];

Missing:

• 127.0.0.1 / localhost (local services)
• 0.0.0.0 (any address binding)
• 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 (private networks)
• fe80::/10 (IPv6 link-local - only fd00:: prefix is blocked)
• DNS rebinding attacks (URL validation happens once, DNS can change before fetch)

Recommendation: Consider using a more comprehensive SSRF protection library or expanding the blocklist to cover RFC1918 private ranges and localhost variants.

⚠️ Open Redirect Risk (Item 6)

File: sdk/javascript/lib/client.mjs:120

const authUrl = `${server}/auth/cli?redirect=http://localhost:${CALLBACK_PORT}/callback`;

The redirect parameter is user-controllable via the server variable. While validateServerUrl() ensures server uses http/https, there's no validation that the OAuth endpoint respects the redirect parameter or that the parameter isn't tampered with during the OAuth flow.

Trace:

1. Input: server = "https://evil.com" → validateServerUrl() passes
2. authUrl = "https://evil.com/auth/cli?redirect=http://localhost:9876/callback"
3. If evil.com's OAuth endpoint doesn't validate the redirect, it could redirect elsewhere after auth

Recommendation: Document that the server endpoint must validate redirect URLs, or implement client-side validation of the callback URL in the HTTP server handler (line 104).

✅ Session Token Handling (Item 4) - SECURE

File: sdk/javascript/lib/client.mjs:54 and credentials.mjs:27

• Tokens stored with 0o600 permissions (user-only read/write)
• Tokens sent via Cookie header (not logged in URL)
• No hardcoded credentials found

✅ URL Parsing Logic (Item 10-12) - CORRECT

File: sdk/javascript/lib/client.mjs:25-39

Traced validateServerUrl() with test inputs:

1. https://localhost:8003 → Returns https://localhost:8003 (trailing slash removed) ✅
2. ftp://evil.com → Throws "must use http or https" ✅
3. https://169.254.169.254 → Throws "blocked address" ✅
4. not-a-url → Throws "Invalid server URL" ✅

Logic is correct - no tautologies or contradictions.

✅ JWT Parsing (Item 3) - SAFE

File: credentials.mjs:50-67

Uses JSON.parse(Buffer.from(parts[1], 'base64url')) - safe deserialization. No eval or unsafe deserialization patterns.

✅ No Other High-Risk Patterns

Searched for:

• ❌ SQL injection (no SQL queries in changed files)
• ❌ eval/Function/vm usage (only safe regex.exec in scanner files)
• ❌ Prototype pollution (no __proto__ access)
• ❌ Weak crypto via Math.random (no crypto usage in client.mjs)

---

Summary

Total Issues Found: 2 security concerns (1 gap in SSRF protection, 1 potential open redirect)

The command injection fix is correct and effective. The use of spawn() with array arguments properly prevents shell interpretation of the URL.

Two additional security concerns identified:

1. SSRF blocklist is incomplete (missing localhost, private ranges)
2. Potential open redirect if server OAuth endpoint doesn't validate redirect parameter

Both are outside the scope of this PR but worth addressing in follow-up work.

FINDINGS_COUNT=2

--- | Branch: fix/command-injection-client

[github-actions]

Manual review required. This PR did not meet auto-merge criteria:

• Security review found issues (findings: unknown)
• PR author 'abdelsfane' is not in the trusted auto-merge list
• PR is too new (2 min < 5 min minimum)