security: enterprise review fixes — HTTPS enforcement, input limits, path redaction, CI alignment#49
security: enterprise review fixes — HTTPS enforcement, input limits, path redaction, CI alignment#49ashish993 wants to merge 4 commits into
Conversation
- Pin all GitHub Actions to immutable commit SHAs to prevent supply chain attacks via mutable tag references (actions/checkout, pnpm/action-setup, actions/setup-node, changesets/action) - Validate login_url is HTTPS before passing to system browser opener to prevent file:// or other protocol exploitation from a compromised broker response - Sanitize login_url in preAuthHelpMessage to HTTPS-only before embedding in LLM agent skill prompts to prevent prompt injection from a compromised broker
- Replace MD5 with SHA-256 in serverHash() cache key (packages/core/lib/cache.js) - Validate CALLE_TELEMETRY_URL is HTTPS before use (packages/cli/lib/config.js) - Add max polling attempt cap (600) to loginWithBroker alongside time deadline (packages/core/lib/broker-client.js) - Add retry with exponential backoff for 429/502/503/504 in requestJsonRpc (packages/core/lib/mcp-client.js) - Add MCP protocol version mismatch warning to stderr on initialize (packages/core/lib/mcp-client.js + cli.js)
- C-2: remove cache_path/pending_cache_path (home dir paths) from all public JSON payloads in cli.js; statusPayload now validates pending_login_url HTTPS before including it - H-1: add 64 KB max-size guard on --args-json in parseJsonObject() - H-2: add max 10 --to-phone numbers and max 2000 char --goal limit in buildPlanArguments() - L-1: truncate server-controlled error messages to 200 chars and strip newlines in McpHttpError to prevent injection into LLM context - L-4: omit --cache-root from loginCommand() output when it equals the default (~/.calle-mcp/cli) to avoid leaking home paths in hints - M-5: align CI workflow to Node 24 (matches release workflow) - Also: allow http:// on loopback (localhost/127.0.0.1) for local dev/test; HTTPS enforcement still applies for all external URLs
There was a problem hiding this comment.
Pull request overview
Security-hardening changes across the CLI and core libraries to address enterprise review findings, primarily focusing on preventing sensitive path leakage, enforcing HTTPS for externally supplied URLs, and bounding user/server-controlled inputs to reduce injection and resource-exhaustion risk.
Changes:
- Enforce HTTPS (with loopback-only HTTP exception) for CLI-supplied base/server/broker/auth URLs; validate broker-provided login URLs before printing/opening.
- Add input/output hardening: caps for
--args-json,--to-phone,--goal; truncate/strip newlines from server-controlled MCP error messages; redact cache paths from public JSON payloads. - Align CI to Node 24 and pin GitHub Actions to specific SHAs.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/lib/mcp-client.js | Adds retry logic, protocol mismatch warning hook, and sanitizes remote error messages. |
| packages/core/lib/cache.js | Changes cache key hashing from MD5 to SHA-256. |
| packages/core/lib/broker-client.js | Enforces HTTPS for broker login URL before printing/opening; adds polling attempt cap. |
| packages/cli/lib/config.js | Adds HTTPS enforcement for user-supplied endpoint URLs; makes telemetry URL misconfig non-fatal. |
| packages/cli/lib/cli.js | Adds input size/length limits, redacts cache paths from public outputs, and tightens login URL handling. |
| .gitignore | Ignores local run.md notes file. |
| .github/workflows/release.yml | Pins actions to SHAs; uses Node 24. |
| .github/workflows/ci.yml | Pins actions to SHAs; updates CI Node version to 24. |
Comments suppressed due to low confidence (1)
packages/cli/lib/cli.js:406
publicPendingLoginPayloadstill returns the broker-providedlogin_urlverbatim in JSON output. Sincelogin_urlis server-controlled, this can re-introduce unsafe/non-HTTPS URLs into public outputs even though other paths now validate/redact. Consider only includinglogin_urlwhen it parses ashttps:(or omitting/setting null otherwise), similar tostatusPayloadand the stderr/openBrowser safeguards.
pending_status: pending.status,
pending_created: created,
login_url: pending.login_url,
...(assistantHint ? { assistant_hint: assistantHint } : {}),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function requestJsonRpc(fetchImpl, url, { headers, payload, timeoutMs, sleepImpl = (ms) => new Promise((r) => setTimeout(r, ms)) }) { | ||
| let lastError; | ||
| for (let attempt = 1; attempt <= MAX_RETRY_ATTEMPTS; attempt++) { | ||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), timeoutMs); |
| headers: responseHeaders, | ||
| }); | ||
| if (RETRYABLE_STATUS_CODES.has(response.status) && attempt < MAX_RETRY_ATTEMPTS) { | ||
| lastError = err; |
| const safeMessage = rawMessage | ||
| ? rawMessage.slice(0, 200).replace(/[\r\n]+/g, " ").trim() |
| const retryAfter = Number(retryAfterHeader); | ||
| if (retryAfterHeader && !Number.isNaN(retryAfter) && retryAfter > 0) { | ||
| return Math.min(retryAfter * 1000, 30000); | ||
| } |
| export function resolveRuntimeConfig(options = {}, env = process.env) { | ||
| if (options.baseUrl) { | ||
| requireHttpsUrl(options.baseUrl, "--base-url"); | ||
| } | ||
| if (options.serverUrl) { | ||
| requireHttpsUrl(options.serverUrl, "--server-url"); | ||
| } | ||
| if (options.brokerBaseUrl) { | ||
| requireHttpsUrl(options.brokerBaseUrl, "--broker-base-url"); | ||
| } | ||
| if (options.authBaseUrl) { | ||
| requireHttpsUrl(options.authBaseUrl, "--auth-base-url"); | ||
| } | ||
| const baseUrl = normalizeBaseUrl(options.baseUrl || DEFAULT_BASE_URL); | ||
| const channel = options.channel || DEFAULT_CHANNEL; | ||
| const serverUrl = resolveServerUrl({ serverUrl: options.serverUrl, baseUrl, channel }); |
| const rawStr = String(raw); | ||
| if (Buffer.byteLength(rawStr, "utf8") > ARGS_JSON_MAX_BYTES) { | ||
| throw new InvalidArgumentsError(`${optionName} exceeds maximum size of 64 KB`); | ||
| } |
| if (toPhones.length > 10) { | ||
| throw new InvalidArgumentsError("--to-phone: maximum 10 numbers per request"); | ||
| } |
| const goal = requireStringOption(options, "goal", "--goal"); | ||
| if (goal.length > 2000) { | ||
| throw new InvalidArgumentsError("--goal: maximum 2000 characters"); | ||
| } |
| export function serverHash(serverUrl) { | ||
| return crypto.createHash("md5").update(serverUrl, "utf8").digest("hex"); | ||
| return crypto.createHash("sha256").update(serverUrl, "utf8").digest("hex"); | ||
| } |
Ray-56
left a comment
Ray-56
left a comment
There was a problem hiding this comment.
Reviewed locally. I recommend holding this PR until the following issues are addressed.
requestJsonRpcretries non-idempotenttools/callrequests.
packages/core/lib/mcp-client.js now retries every JSON-RPC request on 429/502/503/504 and some thrown fetch errors. That includes tools/call, which can execute non-idempotent operations such as run_call. If the server processes the first request but returns a transient 5xx, or if the response is lost, the retry can trigger duplicate side effects such as duplicate phone calls.
I reproduced this locally with a fake MCP server: the first tools/call returned 503, the client retried, and the server saw toolCalls: 2. Please restrict retries to safe/idempotent MCP methods such as initialize / tools/list, or add an explicit idempotency mechanism before retrying tool calls.
- Broker-controlled
login_urlis still returned verbatim in public JSON outputs.
The PR sanitizes the URL used in assistant_hint, stderr, and browser opening, but publicPendingLoginPayload() and authRequiredPayload() can still include the raw pending.login_url / cached pending login_url as top-level JSON fields. I reproduced auth login --start-only returning:
"login_url": "file:///tmp/injected"That reintroduces the unsafe server-controlled URL into the public surface this PR is trying to harden. Please reuse the same HTTPS-only sanitizer for any public login_url field, or omit the field when the URL is not safe.
Validation run on the PR head in a detached worktree with Node v22.21.1:
pnpm testpassedpnpm checkpassedpnpm pack:dry-runpassed
Release note: because this changes published behavior in @call-e/cli and @call-e/core, a patch changeset for both packages is recommended before merge.
Summary
Enterprise security review fixes across
packages/cli,packages/core, and.github/workflows. All changes passpnpm checkandpnpm test(27 unit + 13 E2E tests).Changes
Critical
cache_path/pending_cache_path(full home-directory paths) from all public JSON outputs (publicPendingLoginPayload,publicLoginPayload,statusPayload).statusPayloadnow also validatespending_login_urlis HTTPS before including it.login_urlfrom broker is HTTPS before writing to stderr or passing toopenBrowser.High
--args-jsoninparseJsonObject()to prevent oversized payloads.--to-phoneat 10 numbers and--goalat 2000 characters inbuildPlanArguments().--base-url,--server-url,--broker-base-url,--auth-base-url). Loopback addresses (localhost,127.0.0.1) are allowed with HTTP for local development/testing.Medium
Low
McpHttpErrorto prevent server-injected content from reaching LLM context.CALLE_TELEMETRY_URLmisconfiguration no longer throws and blocks all CLI commands — warns to stderr and disables telemetry gracefully.loginCommand()omits--cache-rootfrom the hint string when it equals the default (~/.calle-mcp/cli), avoiding home-directory path leakage in assistant hints.Testing