From 80908a10c839d20cfb8356deb7b597b15d93cd63 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 22:42:14 -0400 Subject: [PATCH 1/2] security(actions): harden apt package and resolve-version input handling (F-003, F-004) - Disallow carriage returns and newlines upfront in resolve-version to prevent GitHub output-file protocol forgery - Strictly validate generated build date against YYYY-MM-DD format - Sanitize APP_TITLE against carriage returns and newlines in step summary - Validate package tokens in install-apt-packages, rejecting options, path traversals, and injection payloads - Terminate options with -- when invoking apt-get install - Add unit tests for install-apt-packages validation and new test suite for resolve-version --- actions/install-apt-packages/action.yml | 2 +- actions/resolve-version/action.yml | 24 ++++- tests/test_install_apt_packages.py | 60 ++++++++++++ tests/test_resolve_version.py | 119 ++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 5 deletions(-) create mode 100644 tests/test_resolve_version.py diff --git a/actions/install-apt-packages/action.yml b/actions/install-apt-packages/action.yml index 6233abd..78fefae 100644 --- a/actions/install-apt-packages/action.yml +++ b/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 diff --git a/actions/resolve-version/action.yml b/actions/resolve-version/action.yml index 209328c..785d5cc 100644 --- a/actions/resolve-version/action.yml +++ b/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" diff --git a/tests/test_install_apt_packages.py b/tests/test_install_apt_packages.py index f800ca8..4c6178f 100644 --- a/tests/test_install_apt_packages.py +++ b/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() diff --git a/tests/test_resolve_version.py b/tests/test_resolve_version.py new file mode 100644 index 0000000..1322779 --- /dev/null +++ b/tests/test_resolve_version.py @@ -0,0 +1,119 @@ +"""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_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() From 7b38cb1f3195b984b01617cb282f92735ed5ca3e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 22:46:59 -0400 Subject: [PATCH 2/2] test(resolve-version): add test for CRLF sanitization in app-title --- tests/test_resolve_version.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_resolve_version.py b/tests/test_resolve_version.py index 1322779..c05d73e 100644 --- a/tests/test_resolve_version.py +++ b/tests/test_resolve_version.py @@ -69,6 +69,16 @@ class ResolveVersionTests(unittest.TestCase): 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)