Skip to content

fix(security): 2 improvements across 2 files#354

Open
tomaioo wants to merge 2 commits into
openagents-org:developfrom
tomaioo:fix/security/command-injection-risk-in-signing-script
Open

fix(security): 2 improvements across 2 files#354
tomaioo wants to merge 2 commits into
openagents-org:developfrom
tomaioo:fix/security/command-injection-risk-in-signing-script

Conversation

@tomaioo

@tomaioo tomaioo commented Apr 21, 2026

Copy link
Copy Markdown

Summary

fix(security): 2 improvements across 2 files

Problem

Severity: High | File: packages/launcher/scripts/azure-sign.js:L57

The Windows signing helper builds a shell command string with unescaped values (endpoint, account, certProfile, and filePath) and executes it with execSync. If any of these values contain shell metacharacters (or if filePath is attacker-controlled in a compromised build environment), arbitrary command execution can occur during CI/build.

Solution

Avoid string-based shell execution. Use execFileSync/spawnSync with argument arrays, and validate/whitelist expected formats for environment variables and file paths before execution.

Changes

  • packages/launcher/scripts/azure-sign.js (modified)
  • packages/launcher/src/main/preload.js (modified)

tomaioo added 2 commits April 21, 2026 11:07
- Security: Command injection risk in signing script via shell command construction
- Security: Renderer-exposed IPC allows arbitrary shell command execution

Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Command injection risk in signing script via shell command construction
- Security: Renderer-exposed IPC allows arbitrary shell command execution

Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
@vercel

vercel Bot commented Apr 21, 2026

Copy link
Copy Markdown

@tomaioo is attempting to deploy a commit to the Raphael's projects Team on Vercel.

A member of the Team first needs to authorize it.

@zomux

zomux commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Security Audit Review

Change 1: azure-sign.jsexecSyncexecFileSync

This fix is legitimate and well-implemented:

  • The current code on develop passes args through a shell via execSync, which is a real command injection risk if filePath contains shell metacharacters.
  • Switching to execFileSync avoids the shell entirely — correct fix.
  • Input validation on endpoint, account, certProfile, and filePath is a nice addition.

Change 2: preload.js — removing shellExec / openTerminal bridges ❌

This change breaks existing functionality and is an incomplete fix:

  1. openTerminal is actively called by the renderer at renderer.js:374 and renderer.js:1412. Removing the preload bridge without updating the renderer will break the "Open Terminal" feature in the launcher UI.

  2. The IPC handlers in main.js (lines 579 and 625) are not removed. Deleting only the preload bridge is security theater — the handlers remain registered and could still be reached if context isolation were ever misconfigured. A complete fix would remove (or sanitize) the handlers themselves.

  3. No tests are included to verify the signing changes work, or that the removed APIs don't regress existing features.

Recommendation

  • The azure-sign.js change is good — could be merged independently.
  • The preload.js change should not be merged as-is. If the goal is to remove shell exec capability, the handlers in main.js and the call sites in renderer.js need to be addressed together.

Thanks for the contribution!

@zomux

zomux commented May 24, 2026

Copy link
Copy Markdown
Contributor

Hi @tomaioo — the security fixes here (execFileSync + input validation in azure-sign.js, removing shellExec/openTerminal from preload) are solid and we'd like to merge them.

However, the launcher has since been rewritten from JS to TypeScript. Both files this PR modifies no longer exist on develop:

  • packages/launcher/scripts/azure-sign.js → deleted
  • packages/launcher/src/main/preload.js → replaced by packages/launcher/src/preload/index.ts

The shellExec and openTerminal IPC bridges still exist in the new preload/index.ts and should still be removed. Could you rebase this PR against the current develop branch and port the fixes to the new file locations?

If you don't have time, let us know and we can port the changes ourselves.

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