Skip to content

fix: allow node preflight to use explicit binary#5145

Merged
zarenner merged 1 commit into
mainfrom
zr/recover-tool-cache-and-node-bin
Jun 18, 2026
Merged

fix: allow node preflight to use explicit binary#5145
zarenner merged 1 commit into
mainfrom
zr/recover-tool-cache-and-node-bin

Conversation

@zarenner

@zarenner zarenner commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Update the Copilot Node.js preflight to accept GH_AW_NODE_BIN when it points to an executable.
  • Keep the existing command -v node fallback for normal PATH-based resolution.
  • Include GH_AW_NODE_BIN and PATH in the error output when Node cannot be found.

Problem

Copilot workflows can run through AWF with Node installed by actions/setup-node in the runner tool cache. The generated gh-aw workflow records the resolved Node executable before invoking AWF:

GH_AW_NODE_BIN=$(command -v node 2>/dev/null || true)
export GH_AW_NODE_BIN

The generated command later uses that value to run the Copilot harness inside the chroot.

However, AWF performs its own Node preflight before the generated command runs. That preflight only checked command -v node, so it could fail when sudo secure_path removed the setup-node tool-cache directory from PATH, even though GH_AW_NODE_BIN already pointed at the correct executable and the tool cache was mounted into the chroot.

Fix

The preflight now first checks whether GH_AW_NODE_BIN is set and executable. If so, it accepts that as proof that Node is available. If not, it falls back to the existing command -v node behavior.

This aligns AWF's early preflight with the Node executable the generated agent command would use.

Test

Not run; shell-only entrypoint change.

@zarenner zarenner force-pushed the zr/recover-tool-cache-and-node-bin branch from 4af57c0 to c9a2923 Compare June 17, 2026 03:02
@zarenner zarenner changed the title Recover tool cache paths and honor explicit Node binary Allow node preflight to use explicit binary Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 97.30% 97.34% 📈 +0.04%
Statements 97.16% 97.20% 📈 +0.04%
Functions 98.84% 98.84% ➡️ +0.00%
Branches 91.92% 91.96% 📈 +0.04%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/workdir-setup.ts 92.7% → 94.5% (+1.82%) 92.7% → 94.5% (+1.82%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@zarenner zarenner changed the title Allow node preflight to use explicit binary fix: Allow node preflight to use explicit binary Jun 17, 2026
@zarenner zarenner changed the title fix: Allow node preflight to use explicit binary Allow node preflight to use explicit binary Jun 17, 2026
@zarenner zarenner changed the title Allow node preflight to use explicit binary fix: allow node preflight to use explicit binary Jun 17, 2026
@zarenner zarenner marked this pull request as ready for review June 17, 2026 03:39
Copilot AI review requested due to automatic review settings June 17, 2026 03:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates AWF’s agent chroot startup preflight for Copilot runs to recognize an explicitly-provided Node.js executable path (GH_AW_NODE_BIN) before falling back to PATH-based discovery, improving compatibility with runner environments where sudo/secure_path trims toolcache PATH entries.

Changes:

  • Treat GH_AW_NODE_BIN as a valid Node presence signal when it is set and executable.
  • Preserve the existing command -v node fallback when GH_AW_NODE_BIN is unset or unusable.
  • Add GH_AW_NODE_BIN and PATH to the Node-missing error output for better diagnostics.
Show a summary per file
File Description
containers/agent/entrypoint.sh Updates the chroot startup script’s Node preflight to honor GH_AW_NODE_BIN and improve Node-missing diagnostics.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Security Guard completed successfully!

@github-actions

Copy link
Copy Markdown
Contributor

Security Review: Node Preflight Change

This PR modifies a security-critical file (containers/agent/entrypoint.sh) to trust the GH_AW_NODE_BIN environment variable without path validation.

⚠️ Potential Security Concern

File: containers/agent/entrypoint.sh (lines 955-957)

Code:

if [ -n "${GH_AW_NODE_BIN:-}" ] && [ -x "${GH_AW_NODE_BIN}" ]; then
  echo "[entrypoint] Node.js available via GH_AW_NODE_BIN: ${GH_AW_NODE_BIN}"

Issue: The code accepts any executable path from GH_AW_NODE_BIN without validating it points to a safe location. While the -x test confirms executability, there's no check that the path is within expected directories (e.g., /opt/hostedtoolcache, /usr/bin, whitelisted mounts).

Risk: If an attacker can control GH_AW_NODE_BIN (e.g., through workflow injection), they could potentially point it to a malicious binary, bypassing PATH-based security controls.

🛡️ Mitigating Factors

  • Runs within chroot environment (/host), limiting path resolution scope
  • Variable appears to be set by AWF-generated workflows, not directly user-controlled
  • Selective bind mounts restrict available filesystem paths

💡 Suggested Fix

Add path validation before trusting GH_AW_NODE_BIN:

if [ -n "${GH_AW_NODE_BIN:-}" ] && [ -x "${GH_AW_NODE_BIN}" ]; then
  case "${GH_AW_NODE_BIN}" in
    /opt/hostedtoolcache/*|/usr/bin/*|/usr/local/bin/*|/host/usr/*)
      echo "[entrypoint] Node.js available via GH_AW_NODE_BIN: ${GH_AW_NODE_BIN}"
      ;;
    *)
      echo "[entrypoint][WARN] GH_AW_NODE_BIN points to unexpected location: ${GH_AW_NODE_BIN}" >&2
      # Fall through to command -v check
      if ! command -v node >/dev/null 2>&1; then
        # error handling...
      fi
      ;;
  esac
elif command -v node >/dev/null 2>&1; then

This ensures the binary is in an expected location before accepting it.

Generated by Security Guard for issue #5145 · 43.2 AIC · ⊞ 25.8K ·

@zarenner zarenner merged commit 68b9d23 into main Jun 18, 2026
95 of 102 checks passed
@zarenner zarenner deleted the zr/recover-tool-cache-and-node-bin branch June 18, 2026 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants