Merge pull request #82 from dylanmtaylor/security/action-inputs-validation

security(actions): harden apt package and resolve-version input handling (F-003, F-004)

actions/install-apt-packages/action.yml

@@ -22,7 +22,7 @@ runs:
         set -euo pipefail
         # No globbing: an unquoted token like foo* must not expand to workspace filenames.
         set -f
-        list=$(printf '%s\n' $PACKAGES | sort -u | tr '\n' ' ')
+        list=$(printf -- '%s\n' $PACKAGES | sort -u | tr '\n' ' ')
         # Tokens become apt-get operands: a leading dash would be an option (-o runs commands).
         for pkg in $list; do
           if [[ ! "$pkg" =~ ^[A-Za-z0-9][A-Za-z0-9.+:=~_-]*$ ]]; then

actions/resolve-version/action.yml

@@ -32,6 +32,11 @@ runs:
         INPUT_VERSION: ${{ inputs.input-version }}
         APP_TITLE: ${{ inputs.app-title }}
       run: |
+        # Reject newlines or carriage returns upfront to prevent output-file protocol forgery
+        if [[ "$INPUT_VERSION" =~ [$'\r\n'] ]]; then
+          echo "::error::input-version must not contain carriage returns or newlines"
+          exit 1
+        fi
         if [ -f packaging/env.sh ]; then
           . packaging/env.sh
         elif [ -f packaging/common.sh ]; then
@@ -39,17 +44,28 @@ runs:
         fi
         v="$INPUT_VERSION"
         v="${v#v}"
-        v="${v:-$VERSION}"
-        # Whole-string match: grep would accept a multi-line input with one valid line.
+        v="${v:-${VERSION:-}}"
+        if [[ "$v" =~ [$'\r\n'] ]]; then
+          echo "::error::version must not contain carriage returns or newlines"
+          exit 1
+        fi
+        # Whole-string match against semver grammar
         semver='^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$'
         if [[ ! "$v" =~ $semver ]]; then
           echo "::error::'$v' is not a version like 1.2.3 or 1.2.3-rc.1"
           exit 1
         fi
         case "$v" in *-*) pre=true ;; *) pre=false ;; esac
+        build_date="$(date -u +%Y-%m-%d)"
+        if [[ ! "$build_date" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]]; then
+          echo "::error::generated build date '$build_date' is not valid YYYY-MM-DD"
+          exit 1
+        fi
         {
           echo "version=$v"
-          echo "date=$(date -u +%Y-%m-%d)"
+          echo "date=$build_date"
           echo "prerelease=$pre"
         } >>"$GITHUB_OUTPUT"
-        printf '### %s v%s (`%s`)\n' "$APP_TITLE" "$v" "${GITHUB_SHA::9}" >>"$GITHUB_STEP_SUMMARY"
+        clean_title="${APP_TITLE//$'\r'/}"
+        clean_title="${clean_title//$'\n'/ }"
+        printf '### %s v%s (`%s`)\n' "$clean_title" "$v" "${GITHUB_SHA::9}" >>"$GITHUB_STEP_SUMMARY"

tests/test_install_apt_packages.py

@@ -1,6 +1,9 @@
 """The cached apt install action: pinned actions, and PR runs that never write the cache."""
+import os
 from pathlib import Path
 import re
+import subprocess
+import tempfile
 import unittest
 
 import yaml
@@ -33,6 +36,63 @@ class InstallAptPackagesTests(unittest.TestCase):
         self.assertIn('github.ref_name == github.event.repository.default_branch', condition)
         self.assertIn("cache-hit != 'true'", condition)
 
+    def test_install_step_uses_option_terminator(self):
+        install_step = next(s for s in steps() if s.get('name') == 'Install packages')
+        self.assertIn('install -y --no-install-recommends -- $PACKAGE_LIST', install_step['run'])
+
+    def _run_key_step(self, packages):
+        key_step = next(s for s in steps() if s.get('id') == 'key')
+        with tempfile.TemporaryDirectory() as tmpdir:
+            out_file = Path(tmpdir) / "output"
+            res = subprocess.run(
+                ['bash', '-euo', 'pipefail', '-c', key_step['run']],
+                env={**os.environ, 'PACKAGES': packages, 'GITHUB_OUTPUT': str(out_file)},
+                capture_output=True,
+                text=True,
+            )
+            outputs = {}
+            if out_file.exists():
+                for line in out_file.read_text().splitlines():
+                    if '=' in line:
+                        k, v = line.split('=', 1)
+                        outputs[k] = v
+            return res, outputs
+
+    def test_valid_packages_accepted(self):
+        cases = [
+            ("curl", "curl "),
+            ("curl git build-essential", "build-essential curl git "),
+            ("libx11-dev:amd64 pkg-config=1.8.0-1", "libx11-dev:amd64 pkg-config=1.8.0-1 "),
+            ("curl\ngit\nbuild-essential", "build-essential curl git "),
+        ]
+        for packages, expected_list in cases:
+            with self.subTest(packages=packages):
+                res, outputs = self._run_key_step(packages)
+                self.assertEqual(res.returncode, 0, f"{res.stderr}\n{res.stdout}")
+                self.assertEqual(outputs.get('list'), expected_list)
+
+    def test_invalid_package_names_rejected(self):
+        invalid_cases = [
+            "-o APT::Update=1",
+            "--config",
+            "-y",
+            "../traversal.deb",
+            "/tmp/evil.deb",
+            "curl; touch injected",
+            "curl && touch injected",
+            "curl | touch injected",
+            "$(touch injected)",
+            "`touch injected`",
+            "curl>injected",
+            "",
+            "   \n  \t  ",
+        ]
+        for bad_pkg in invalid_cases:
+            with self.subTest(bad_pkg=bad_pkg):
+                res, _ = self._run_key_step(bad_pkg)
+                self.assertNotEqual(res.returncode, 0)
+                self.assertIn("::error::install-apt-packages:", res.stdout + res.stderr)
+
 
 if __name__ == '__main__':
     unittest.main()

tests/test_resolve_version.py

@@ -0,0 +1,129 @@
+"""Unit tests for actions/resolve-version/action.yml."""
+
+import os
+from pathlib import Path
+import re
+import subprocess
+import tempfile
+import unittest
+
+import yaml
+
+ROOT = Path(__file__).resolve().parents[1]
+ACTION = ROOT / 'actions/resolve-version/action.yml'
+
+
+class ResolveVersionTests(unittest.TestCase):
+    def setUp(self):
+        self.data = yaml.safe_load(ACTION.read_text())
+        self.step = self.data['runs']['steps'][0]
+
+    def _run_action(self, input_version="", version_env="", app_title="Test App", env_sh=None):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            work = Path(tmpdir)
+            if env_sh:
+                pkg = work / "packaging"
+                pkg.mkdir()
+                (pkg / "env.sh").write_text(env_sh)
+            output_file = work / "output"
+            summary_file = work / "summary"
+            env = {
+                **os.environ,
+                'INPUT_VERSION': input_version,
+                'VERSION': version_env,
+                'APP_TITLE': app_title,
+                'GITHUB_OUTPUT': str(output_file),
+                'GITHUB_STEP_SUMMARY': str(summary_file),
+                'GITHUB_SHA': 'abcdef1234567890',
+            }
+            res = subprocess.run(
+                ['bash', '-euo', 'pipefail', '-c', self.step['run']],
+                cwd=work,
+                env=env,
+                capture_output=True,
+                text=True,
+            )
+            outputs = {}
+            if output_file.exists():
+                for line in output_file.read_text().splitlines():
+                    if '=' in line:
+                        k, v = line.split('=', 1)
+                        outputs[k] = v
+            summary = summary_file.read_text() if summary_file.exists() else ""
+            return res, outputs, summary
+
+    def test_valid_versions(self):
+        cases = [
+            ("1.2.3", "1.2.3", "false"),
+            ("v1.2.3", "1.2.3", "false"),
+            ("0.1.0-alpha.1", "0.1.0-alpha.1", "true"),
+            ("2.0.0-rc-1", "2.0.0-rc-1", "true"),
+            ("10.20.30", "10.20.30", "false"),
+        ]
+        for input_ver, expected_ver, expected_pre in cases:
+            with self.subTest(input_ver=input_ver):
+                res, outputs, summary = self._run_action(input_version=input_ver)
+                self.assertEqual(res.returncode, 0, f"{res.stderr}\n{res.stdout}")
+                self.assertEqual(outputs.get('version'), expected_ver)
+                self.assertEqual(outputs.get('prerelease'), expected_pre)
+                self.assertRegex(outputs.get('date', ''), r'^[0-9]{4}-[0-9]{2}-[0-9]{2}$')
+                self.assertIn(f"v{expected_ver}", summary)
+
+    def test_app_title_crlf_stripped_from_summary(self):
+        res, outputs, summary = self._run_action(
+            input_version="1.2.3",
+            app_title="My\r\nApp\nTitle",
+        )
+        self.assertEqual(res.returncode, 0, f"{res.stderr}\n{res.stdout}")
+        self.assertNotIn("\r", summary)
+        self.assertEqual(summary.count("\n"), 1)
+        self.assertIn("My App Title", summary)
+
+    def test_fallback_to_version_env_and_env_sh(self):
+        res, outputs, _ = self._run_action(version_env="3.4.5")
+        self.assertEqual(res.returncode, 0)
+        self.assertEqual(outputs.get('version'), "3.4.5")
+
+        res, outputs, _ = self._run_action(env_sh='VERSION="4.5.6"\n')
+        self.assertEqual(res.returncode, 0)
+        self.assertEqual(outputs.get('version'), "4.5.6")
+
+    def test_newlines_and_cr_rejected(self):
+        bad_versions = [
+            "1.2.3\ndate=2026-01-01",
+            "1.2.3\r\nversion=fake",
+            "\n1.2.3",
+            "1.2.3\n",
+            "1.2.3\r",
+            "1.2.3\r\ndate=2026-01-01",
+        ]
+        for bad_ver in bad_versions:
+            with self.subTest(bad_ver=repr(bad_ver)):
+                res, outputs, _ = self._run_action(input_version=bad_ver)
+                self.assertNotEqual(res.returncode, 0)
+                self.assertIn("must not contain carriage returns or newlines", res.stdout + res.stderr)
+                self.assertEqual(outputs, {})
+
+    def test_invalid_semver_rejected(self):
+        invalid_versions = [
+            "1.2",
+            "1.2.3.4",
+            "01.2.3",
+            "1.02.3",
+            "foo",
+            "1.2.3; touch injected",
+            "$(touch injected)",
+            "`touch injected`",
+            "1.2.3-",
+            "1.2.3+",
+        ]
+        for bad_ver in invalid_versions:
+            with self.subTest(bad_ver=bad_ver):
+                res, outputs, _ = self._run_action(input_version=bad_ver)
+                self.assertNotEqual(res.returncode, 0)
+                self.assertIn("is not a version like 1.2.3 or 1.2.3-rc.1", res.stdout + res.stderr)
+                self.assertEqual(outputs, {})
+
+
+if __name__ == '__main__':
+    unittest.main()