fix(terraform-hook): improve Makefile generation (#9271)

fix(terraform-hook): prevent shell command injection in Makefile generation

The Terraform hook’s Makefile generator interpolated an untrusted
Terraform resource address into a shell command string, escaping only
the double-quote character. Backticks and $(…) were not escaped,
allowing an attacker-controlled Terraform resource address (via a
crafted for_each key or tampered plan file) to execute arbitrary
commands on the build host during ‘sam build –hook-name terraform’.
Replace the double-quote-only escaping with shlex.quote(), after
first escaping make’s own $macroexpansion(macro\; expansion\; ($ -> 
$), since make
processes the recipe line before handing it to the shell.
Also reject resource addresses containing embedded newlines or
carriage returns: make parses Makefiles line-by-line before shelling
out, so shlex.quote() alone cannot neutralize an embedded newline,
which could otherwise split one recipe line into multiple physical
lines in the generated Makefile.

style: fix black formatting in terraform hook test file

fix: address PR review comments on injection fix



Escape all non-printable characters (not just \n/\r) in the error
message preview for rejected resource addresses, so an attacker who
triggers the newline-rejection path cannot also smuggle ANSI escape
sequences or other control bytes into unfiltered stderr output.

Fix the shell-injection regression test to work reliably on Windows
CI: skip it on Windows (it depends on /bin/sh and POSIX /tmp
semantics that don’t translate cleanly to MSYS/Git-Bash path
handling), and use a per-test unique marker path via
tempfile.TemporaryDirectory() instead of a hardcoded /tmp path.



fix: keep untrusted Terraform values off the Makefile shell command line

Address PR review feedback: shlex.quote() only produces POSIX-safe
quoting, but the Terraform hook is also supported on Windows, where
GNU make falls back to cmd.exe as SHELL when sh.exe is not found on
PATH. cmd.exe does not treat single quotes as quoting characters, so
the previous fix broke every recipe (–expression always contains
‘|’, which cmd.exe interprets as a pipe outside of quotes).
Rather than trying to find an escaping scheme that is simultaneously
safe for make’s macro expansion and portable across both shells,
write the untrusted expression/resource address to a small JSON
‘args file’ in the SAM-CLI-controlled output directory, and pass only
that file’s path (never user data) on the Makefile recipe’s command
line. copy_terraform_built_artifacts.py reads –target/–expression
from this file via a new –args-file option, falling back to the
existing –target/–expression flags for backward compatibility.
This removes the need for shell/make escaping entirely for these
values, since they never reach a shell or make’s macro expansion -
only the JSON parser in copy_terraform_built_artifacts.py.  It also
addresses two test-quality issues flagged in review: the injection
test using ‘&&’ after an always-failing preceding command (vacuous
pass), and no coverage distinguishing escaped from unescaped ‘${}^{\mathrm{\prime }}handling(mootnow,sinc{e}^{\mathrm{\prime }}\text{'}\; handling\; (moot\; now,\; since\; \text{'}$‘ is no longer special-cased).
Removed InvalidTerraformResourceAddressException and its newline
rejection logic, since embedded newlines are handled safely by JSON
encoding and no longer risk splitting a Makefile recipe line.
sim: https://t.corp.amazon.com/P475859948

fix: sort imports to satisfy ruff I001 (make pr lint failure)

sim: https://t.corp.amazon.com/P475859948

fix: address further PR review comments on args-file handling


Guard the –args-file read in copy_terraform_built_artifacts.py
against OSError/ValueError (missing/unreadable/corrupt file) and a
non-object JSON top level, converting them into this script’s
existing error-reporting convention (LOG.error + cli_exit()) rather
than letting them surface as an uncaught traceback wrapped in a
generic make failure

Use a deterministic args file name (logical_id.args.json) instead of
appending uuid4(), since logical_id is already unique per resource
and never attacker-influenced. sam build always re-runs prepare, so
the uuid-based name caused a new file to accumulate on every build
with nothing to clean them up; the deterministic name is overwritten
each run instead

Added tests for both: deterministic-name/overwrite behavior in
test_makefile_generator.py, and clean-error-on-bad-args-file in
test_copy_terraform_built_artifacts.py


sim: https://t.corp.amazon.com/P475859948

fix: create output_dir before writing args file, add contract tests


_write_makerule_args_file() could run before generate_makefile() has
a chance to create output_dir, since the prepare-hook contract does
not guarantee the directory pre-exists (hook.py’s prepare() creates
it itself rather than assuming a caller did). Call os.makedirs(…,
exist_ok=True) before opening the args file for writing.

Added test_write_makerule_args_file_creates_output_dir_if_missing to
cover this directly (previous tests masked it by always
pre-creating output_dir).

Added test_script_output_path_directory_args_file, a positive
integration test proving a valid –args-file actually supplies
expression/target to the script end-to-end, so a producer/consumer
key mismatch would be caught (previous new tests only covered
failure paths: missing/malformed args file).


sim: https://t.corp.amazon.com/P475859948

fix: bound args file name to avoid exceeding filename length limits

build_cfn_logical_id() can produce a logical_id up to 255 characters
(247 human-readable + 8 hash chars). Appending ‘.args.json’ (10 chars)
to that could push the args file name past the 255-byte per-component
limit enforced by most filesystems (ext4/xfs/btrfs/APFS) and Windows,
causing open() to raise OSError for deeply-nested Lambda resources
with long Terraform addresses.
Truncate logical_id to 236 characters and append an 8-char checksum
of the full logical_id before the suffix, keeping the name at 254
bytes worst case while still being deterministic and disambiguating
logical IDs that happen to share the same truncated prefix.
Also removed unused Path/skipIf imports left over from an earlier
revision of the injection regression tests.
sim: https://t.corp.amazon.com/P475859948

fix: truncate args file name on UTF-8 bytes, not characters

logical_id is Unicode-aware (build_cfn_logical_id() keeps any
Unicode alphanumeric character, not just ASCII), so a logical_id
built from a non-ASCII for_each key (e.g. CJK) can be up to 255
characters while each character is multiple bytes in UTF-8.
Truncating on character count let the encoded file name exceed
the 255-byte per-component filesystem/OS limit that the truncation
was meant to enforce.
Truncate on the UTF-8 encoded bytes instead (decoding back with
errors=’ignore’ to drop any partial trailing multi-byte character),
so the byte length bound holds regardless of script. The checksum
is still computed over the full, untruncated logical_id.
Also corrected the docstring’s claim that logical_id is never

fix: name args files by hash and defer all writes to generate_makefile

Two review findings, addressed together since the second builds on the
first:

The args file name ({truncated_logical_id}{checksum}.args.json)
guarded against the 255-byte per-component filesystem/OS filename
limit, but not Windows’ 260-character MAX_PATH limit on the total
path once combined with a real project directory path - which a
deeply-nested Terraform module address can hit well before the
per-component limit. Switched to a pure 16-char hash of logical_id
(str_checksum(logical_id)[:16]}.args.json, 26 characters total),
which sidesteps both limits regardless of how long or non-ASCII
logical_id is. This also let us delete the byte-vs-character
truncation logic and its three dedicated tests.

_build_makerule_python_command had become a function with a
filesystem side effect (creating the args file) called once per
resource inside enrich_resources_and_generate_makefile‘s loop. If
a later resource in that loop raised (e.g. an unrecognized sam
metadata resource type), earlier resources’ args files were already
on disk with no Makefile ever produced to go with them - orphaned
files with nothing to clean them up. Made
_build_makerule_python_command (and
generate_makefile_rule_for_lambda_resource above it) pure again:
they now return a PendingArgsFile (path, expression, target)
instead of writing it. All args files are written in a single place

generate_makefile() - only after every rule in the batch has
been generated successfully. generate_makefile() also now prunes
any stale *.args.json left behind by a previous run (e.g. for a
renamed or removed Lambda resource) before writing the current set,
so nothing accumulates indefinitely either.



Testing:

Updated unit tests in test_makefile_generator.py for the new pure
function signatures; added test_get_args_file_path_keeps_file_name_short_regardless_of_logical_id
and test_generate_makefile_prunes_stale_args_files_and_writes_new_ones
Added test_enrich_resources_and_generate_makefile_does_not_write_anything_when_a_later_resource_fails
to test_enrich.py, which does NOT mock generate_makefile_rule_for_lambda_resource
so it genuinely exercises the pure-function contract, and asserts
generate_makefile is never called when a later resource fails
406/406 tests passing across tests/unit/hook_packages/terraform/ and
tests/integration/scripts/test_copy_terraform_built_artifacts.py
ruff check samcli schema and black –check clean
Verified end-to-end with a real sam build --hook-name terraform run
seeded with a stale args file using the old naming scheme: confirmed
it is pruned and replaced with a new 16-char-hash-named file


fix: fix Windows CI failures from the previous commit

Two issues surfaced by make pr on windows-latest CI, both introduced by
the previous commit:

test_build_makerule_python_command compared PendingArgsFile.path (a
native-OS-style absolute path, backslash-separated on Windows) against
a value built with os.path.join(terraform_application_dir,
args_file_relative_path), where args_file_relative_path was parsed out
of the recipe text and is always unix-style (forward-slash), since the
recipe is handed to a shell that may be cmd.exe. On Windows,
os.path.join only inserts a backslash between the two arguments - it
doesn’t normalize slashes already present inside
args_file_relative_path - producing a mixed-separator path that never
equals PendingArgsFile.path. Fixed by converting PendingArgsFile.path
the same way _build_makerule_python_command itself does (relative_to +
convert_path_to_unix_path) and comparing that to the recipe’s value,
rather than trying to reconstruct the native path via string joining.

Pruning stale args files via
glob.glob(os.path.join(output_directory_path, “*.args.json”)) applies
fnmatch pattern semantics to every path component, not just the final
one. A project path containing a glob metacharacter ([, ], *,
?) makes the pattern silently match nothing, so pruning does not
fail loudly - it just never removes anything, which is the exact
accumulation problem pruning was added to prevent. Replaced glob with
os.listdir + suffix filtering, which has no pattern-expansion
semantics on the directory path at all. Added
test_generate_makefile_prunes_stale_args_files_in_a_directory_with_glob_metacharacters
to cover the case neither existing test could reach (mocked test
asserts on a literal string; the other uses a tempfile.TemporaryDirectory
path, which never contains metacharacters).


Testing:

407/407 tests passing across tests/unit/hook_packages/terraform/ and
tests/integration/scripts/test_copy_terraform_built_artifacts.py
ruff check samcli schema and black –check clean
Verified the glob bug empirically before fixing: glob.glob against a
path containing “proj[1]” returned [] despite the file existing (confirmed
via os.listdir), and returned the file once wrapped in glob.escape()


Co-authored-by: Chengjun Li licjun@amazon.com