security: avoid interpolating values into a bash -c string in downloadCli

CodeQL flagged the install.sh invocation (js/actions/uncontrolled- command-line) - correctly this time, unlike the pre-existing false positive on src/index.ts:45. resolvedVersion/destDir were passed safely as quoted positional params ($0/$1/$2) rather than concatenated into the command text, so it wasn't exploitable, but building a `bash -c '... "$0" ...'` string at all is exactly the shape that query looks for, and there was a strictly better option available: fetch install.sh's content directly, write it to a file, and run that file with a plain args array - the same shape this file's own callers already use for the CLI binary itself, with no shell-text construction step for the query to flag in the first place. Also fixes a real, environment-dependent test bug found while touching this: the "restores from cache" test's real fs.chmod call throws ENOENT on Linux for a path that doesn't exist on disk, gets swallowed by restoreFromCache's own try/catch, and silently falls through to the real install path - passing locally only because the chmod call is skipped entirely on `win32` (a Windows dev machine), never because the cache-restore logic under test actually worked. fs/promises is now mocked like @actions/cache and @actions/exec already were.
2026-09-29 12:07:05 -07:00 · 2026-08-26 23:28:32 +01:00
parent b695be34bb
commit 1ab7c56964
2 changed files with 83 additions and 45 deletions
@@ -1,10 +1,20 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import * as fs from 'node:fs/promises';
 import * as cache from '@actions/cache';
 import * as exec from '@actions/exec';
 import { binaryNameFor, downloadCli, resolveLatestTag } from './download-cli';

 vi.mock('@actions/exec');
 vi.mock('@actions/cache');
+vi.mock('node:fs/promises');
+
+function mockInstallScriptFetch(): void {
+  globalThis.fetch = vi.fn(async () => ({
+    ok: true,
+    status: 200,
+    text: async () => '#!/usr/bin/env bash\necho mock install.sh',
+  })) as unknown as typeof fetch;
+}

 describe('binaryNameFor', () => {
  it('is game-ci.exe on win32', () => {
@@ -112,20 +122,37 @@ describe('resolveLatestTag', () => {
 });

 describe('downloadCli', () => {
+  const originalFetch = globalThis.fetch;
+
  beforeEach(() => {
    vi.mocked(cache.isFeatureAvailable).mockReturnValue(false);
+    // Real chmod/mkdir/writeFile aren't under test here (that's install.sh's
+    // job, and it isn't actually run - see mockInstallScriptFetch) and the
+    // paths involved don't exist on disk in this test environment. Without
+    // these, restoreFromCache's real fs.chmod call throws ENOENT on any
+    // platform where that branch actually runs (linux/darwin - the
+    // `process.platform !== 'win32'` guard means it's silently skipped, and
+    // the bug masked, on a Windows dev machine) - restoreFromCache's own
+    // try/catch then swallows that and returns null, so a cache-hit test
+    // silently falls through to the real install path instead of catching
+    // what it's meant to catch.
+    vi.mocked(fs.chmod).mockResolvedValue(undefined);
+    vi.mocked(fs.mkdir).mockResolvedValue(undefined);
+    vi.mocked(fs.writeFile).mockResolvedValue(undefined);
  });

  afterEach(() => {
    vi.resetAllMocks();
+    globalThis.fetch = originalFetch;
  });

  // The actual install mechanics (platform detection, archive format,
  // extraction) live in game-ci/cli's own scripts/install.sh now, fetched
  // and run at the resolved version's tag - see game-ci/cli#187. This
-  // wrapper's own job is just: build that command correctly, and take
-  // install.sh's stdout as the binary path.
+  // wrapper's own job is just: fetch that script, run it correctly, and
+  // take its stdout as the binary path.
  it('fetches and runs install.sh for the given version, returning its stdout as the binary path', async () => {
+    mockInstallScriptFetch();
    vi.mocked(exec.exec).mockImplementation(async (_cmd, _args, options) => {
      options?.listeners?.stdout?.(Buffer.from('/tmp/game-ci-cli-cache/v0.1.32/game-ci\n'));
      return 0;
@@ -134,52 +161,59 @@ describe('downloadCli', () => {
    const binaryPath = await downloadCli('v0.1.32');

    expect(binaryPath).toBe('/tmp/game-ci-cli-cache/v0.1.32/game-ci');
+    expect(globalThis.fetch).toHaveBeenCalledWith(
+      'https://raw.githubusercontent.com/game-ci/cli/v0.1.32/scripts/install.sh',
+    );
    expect(exec.exec).toHaveBeenCalledWith(
      'bash',
-      expect.arrayContaining([
-        'https://raw.githubusercontent.com/game-ci/cli/v0.1.32/scripts/install.sh',
-        'v0.1.32',
-      ]),
+      expect.arrayContaining(['v0.1.32']),
      expect.anything(),
    );
  });

  it('resolves "latest" to a concrete tag before fetching install.sh', async () => {
-    const originalFetch = globalThis.fetch;
-    globalThis.fetch = vi.fn(async () => ({
-      ok: true,
-      status: 200,
-      json: async () => ({ tag_name: 'v0.1.33' }),
-    })) as unknown as typeof fetch;
+    globalThis.fetch = vi.fn(async (url: string) => {
+      if (url.includes('/releases/latest')) {
+        return { ok: true, status: 200, json: async () => ({ tag_name: 'v0.1.33' }) };
+      }
+      return { ok: true, status: 200, text: async () => 'echo mock install.sh' };
+    }) as unknown as typeof fetch;

    vi.mocked(exec.exec).mockImplementation(async (_cmd, _args, options) => {
      options?.listeners?.stdout?.(Buffer.from('/tmp/game-ci\n'));
      return 0;
    });

-    try {
-      await downloadCli('latest');
+    await downloadCli('latest');

-      expect(exec.exec).toHaveBeenCalledWith(
-        'bash',
-        expect.arrayContaining([
-          'https://raw.githubusercontent.com/game-ci/cli/v0.1.33/scripts/install.sh',
-          'v0.1.33',
-        ]),
-        expect.anything(),
-      );
-    } finally {
-      globalThis.fetch = originalFetch;
-    }
+    expect(globalThis.fetch).toHaveBeenCalledWith(
+      'https://raw.githubusercontent.com/game-ci/cli/v0.1.33/scripts/install.sh',
+    );
+    expect(exec.exec).toHaveBeenCalledWith(
+      'bash',
+      expect.arrayContaining(['v0.1.33']),
+      expect.anything(),
+    );
+  });
+
+  it('throws a clear error when the install.sh fetch fails', async () => {
+    globalThis.fetch = vi.fn(async () => ({
+      ok: false,
+      status: 404,
+    })) as unknown as typeof fetch;
+
+    await expect(downloadCli('v0.1.32')).rejects.toThrow(/404/);
+    expect(exec.exec).not.toHaveBeenCalled();
  });

  it('throws a clear error when install.sh produces no output', async () => {
+    mockInstallScriptFetch();
    vi.mocked(exec.exec).mockImplementation(async () => 0);

    await expect(downloadCli('v0.1.32')).rejects.toThrow(/produced no output/);
  });

-  it('restores from cache instead of running install.sh on a cache hit', async () => {
+  it('restores from cache instead of fetching install.sh on a cache hit', async () => {
    vi.mocked(cache.isFeatureAvailable).mockReturnValue(true);
    vi.mocked(cache.restoreCache).mockResolvedValue('game-ci-cli-v0.1.32-key');

@@ -137,28 +137,32 @@ export async function downloadCli(version: string): Promise<string> {
  const installScriptUrl = `https://raw.githubusercontent.com/${CLI_REPO}/${resolvedVersion}/scripts/install.sh`;
  core.info(`Installing game-ci CLI ${resolvedVersion} via ${installScriptUrl}`);

+  // Fetched and written to a file, then run as `bash <path> <args...>`,
+  // rather than piped straight into a `bash -c '... "$0" ...'` wrapper: a
+  // curl-into-bash one-liner needs resolvedVersion/destDir interpolated
+  // into the -c script text (even safely, as quoted positional params),
+  // which is exactly the shape CodeQL's uncontrolled-command-line query
+  // flags. Passing a real file path plus a plain string[] of args - the
+  // same pattern this file's own callers already use for the CLI binary
+  // itself - has no such shell-text construction step to flag at all.
+  const scriptResponse = await fetch(installScriptUrl);
+  if (!scriptResponse.ok) {
+    throw new Error(
+      `Failed to fetch install.sh for game-ci CLI ${resolvedVersion}: ` +
+        `GitHub returned ${scriptResponse.status} for ${installScriptUrl}.`,
+    );
+  }
+  const scriptPath = path.join(os.tmpdir(), `game-ci-install-${resolvedVersion.replace(/[^\w.-]/g, '_')}.sh`);
+  await fs.writeFile(scriptPath, await scriptResponse.text(), { mode: 0o755 });
+
  let stdout = '';
-  await exec.exec(
-    'bash',
-    [
-      '-c',
-      // `set -o pipefail` matters here: without it, a failed curl (e.g. a
-      // typo'd/deleted tag giving a 404) still exits 0 because it's not the
-      // pipeline's last command, and the inner `bash -s` would silently run
-      // on an empty script instead of failing loudly.
-      'set -o pipefail; curl -fsSL "$0" | bash -s -- "$1" "$2"',
-      installScriptUrl,
-      resolvedVersion,
-      destDir,
-    ],
-    {
-      listeners: {
-        stdout: (data: Buffer) => {
-          stdout += data.toString();
-        },
+  await exec.exec('bash', [scriptPath, resolvedVersion, destDir], {
+    listeners: {
+      stdout: (data: Buffer) => {
+        stdout += data.toString();
      },
    },
-  );
+  });

  // install.sh writes progress to stderr and only the final binary path to
  // stdout, but take the last non-empty line regardless - defensive against