Skip to content

fix: execFileSync shell command built from environment values - #806

Open
bjormpro wants to merge 2 commits into
coinbase:masterfrom
bjormpro:patch-2
Open

fix: execFileSync shell command built from environment values#806
bjormpro wants to merge 2 commits into
coinbase:masterfrom
bjormpro:patch-2

Conversation

@bjormpro

@bjormpro bjormpro commented Jul 24, 2026

Copy link
Copy Markdown

What changed? Why?

Dynamically constructing a shell command with values from the local environment, such as file paths, may inadvertently change the meaning of the shell command. Such changes can occur when an environment value contains characters that the shell interprets in a special way, for instance quotes and spaces. This can result in the shell command misbehaving, or even allowing a malicious user to execute arbitrary commands on the system.

Use execFileSync (or spawn with argument arrays) instead of execSync with a command string, so file paths are passed as literal arguments and never interpreted by a shell.

Best single fix in apps/expo-app/scripts/utils/AndroidBuilder.mjs:

  • Update the child_process import to include execFileSync and remove execSync usage.
  • Replace all three execSync(...) invocations in applyBundle with execFileSync(binary, args, options).
  • Keep existing behavior (cwd, stdio: 'inherit') unchanged.

This addresses both alert variants at once because both tainted paths are no longer concatenated into a shell command string.

Root cause (required for bugfixes)

UI changes

iOS Old iOS New
old screenshot new screenshot
Android Old Android New
old screenshot new screenshot
Web Old Web New
old screenshot new screenshot

Testing

How has it been tested?

  • Unit tests
  • Interaction tests
  • Pseudo State tests
  • Manual - Web
  • Manual - Android (Emulator / Device)
  • Manual - iOS (Emulator / Device)

Testing instructions

Illustrations/Icons Checklist

Required if this PR changes files under packages/illustrations/** or packages/icons/**

  • verified visreg changes with Terran (include link to visreg run/approval)
  • all illustration/icons names have been reviewed by Dom and/or Terran

Change management

type=emergency
risk=high
impact=sev1

automerge=false

Use `execFileSync` (or `spawn` with argument arrays) instead of `execSync` with a command string, so file paths are passed as literal arguments and never interpreted by a shell.

Best single fix in `apps/expo-app/scripts/utils/AndroidBuilder.mjs`:
- Update the child_process import to include `execFileSync` and remove `execSync` usage.
- Replace all three `execSync(...)` invocations in `applyBundle` with `execFileSync(binary, args, options)`.
- Keep existing behavior (`cwd`, `stdio: 'inherit'`) unchanged.

This addresses both alert variants at once because both tainted paths are no longer concatenated into a shell command string.
@cb-heimdall

cb-heimdall commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 1
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2
CODEOWNERS 🟡 See below

🟡 CODEOWNERS

Code Owner Status Calculation
ui-systems-eng-team 🟡 0/1
Denominator calculation
Additional CODEOWNERS Requirement
Show calculation
Sum 0
0
From CODEOWNERS 1
Sum 1

Comment thread tools/generateTarballs.mjs
Comment thread packages/mobile-visreg/src/upload.mjs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants