fix(terraform-hook): improve Makefile generation - #9271
Merged
Conversation
…ration 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 $ 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.
- 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.
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 '$' handling (moot now, since '$' 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
- 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
- _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
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
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
Two review findings, addressed together since the second builds on the
first:
1. 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.
2. `_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
Two issues surfaced by `make pr` on windows-latest CI, both introduced by the previous commit: 1. 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. 2. 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()
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
reedham-aws
approved these changes
Sep 11, 2026
ckawl
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue(s) does this change fix?
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.